address review comments

Author: chfast

lib/evmone_precompiles/kzg.cpp

@@ -1,5 +1,4 @@
 #include "kzg.hpp"
-#include "sha256.hpp"
 #include <blst.h>
 #include <algorithm>
 #include <optional>
@@ -60,11 +59,16 @@ std::optional<blst_p1_affine> validate_G1(std::span<const std::byte, 48> b) noex
     blst_p1_affine r;
     if (blst_p1_uncompress(&r, reinterpret_cast<const uint8_t*>(b.data())) != BLST_SUCCESS)
         return std::nullopt;
-    if (!blst_p1_affine_in_g1(&r))  // Subgroup check is required by the spec but not testable.
+
+    // Subgroup check is required by the spec but there are no test vectors
+    // with points outside G1 which would satisfy the final pairings check.
+    if (!blst_p1_affine_in_g1(&r))
         return std::nullopt;
     return r;
 }
 
+/// Add two points from E1 and convert the result to affine form.
+/// The conversion to affine is very costly so use only if the affine of the result is needed.
 blst_p1_affine add_or_double(const blst_p1_affine& p, const blst_p1& q) noexcept
 {
     blst_p1 r;
@@ -77,14 +81,16 @@ blst_p1_affine add_or_double(const blst_p1_affine& p, const blst_p1& q) noexcept
 blst_p1 mult(const blst_p1& p, const blst_scalar& v) noexcept
 {
     blst_p1 r;
-    blst_p1_mult(&r, &p, v.b, 255);
+    blst_p1_mult(&r, &p, v.b, BLS_MODULUS_BITS);
     return r;
 }
 
-blst_p2_affine add(const blst_p2_affine& p, const blst_p2& q) noexcept
+/// Add two points from E2 and convert the result to affine form.
+/// The conversion to affine is very costly so use only if the affine of the result is needed.
+blst_p2_affine add_or_double(const blst_p2_affine& p, const blst_p2& q) noexcept
 {
     blst_p2 r;
-    blst_p2_add_affine(&r, &q, &p);
+    blst_p2_add_or_double_affine(&r, &q, &p);
     blst_p2_affine ra;
     blst_p2_to_affine(&ra, &r);
     return ra;
@@ -93,7 +99,7 @@ blst_p2_affine add(const blst_p2_affine& p, const blst_p2& q) noexcept
 blst_p2 mult(const blst_p2& p, const blst_scalar& v) noexcept
 {
     blst_p2 r;
-    blst_p2_mult(&r, &p, v.b, 255);
+    blst_p2_mult(&r, &p, v.b, BLS_MODULUS_BITS);
     return r;
 }
 
@@ -108,12 +114,12 @@ bool pairings_verify(
 }
 }  // namespace
 
-bool kzg_verify_proof(const std::byte versioned_hash[32], const std::byte z[32],
+bool kzg_verify_proof(const std::byte versioned_hash[VERSIONED_HASH_SIZE], const std::byte z[32],
     const std::byte y[32], const std::byte commitment[48], const std::byte proof[48]) noexcept
 {
     std::byte computed_versioned_hash[32];
     sha256(computed_versioned_hash, commitment, 48);
-    computed_versioned_hash[0] = std::byte{0x01};
+    computed_versioned_hash[0] = VERSIONED_HASH_VERSION_KZG;
     if (!std::ranges::equal(std::span{versioned_hash, 32}, computed_versioned_hash))
         return false;
 
@@ -147,7 +153,7 @@ bool kzg_verify_proof(const std::byte versioned_hash[32], const std::byte z[32],
     const auto neg_Z = mult(G2_GENERATOR_NEGATIVE, *zz);
 
     // Compute X - Z which is [s - z]₂.
-    const auto X_sub_Z = add(KZG_SETUP_G2_1, neg_Z);
+    const auto X_sub_Z = add_or_double(KZG_SETUP_G2_1, neg_Z);
 
     // e(C - [y]₁, [1]₂) =? e(Pi, [s - z]₂)
     return pairings_verify(C_sub_Y, *Pi, X_sub_Z);

lib/evmone_precompiles/kzg.hpp

@@ -1,8 +1,28 @@
 #pragma once
-#include <cstddef>
+#include "sha256.hpp"
+#include <intx/intx.hpp>
 
 namespace evmone::crypto
 {
-bool kzg_verify_proof(const std::byte versioned_hash[32], const std::byte z[32],
+using intx::operator""_u256;
+
+/// Length (in bytes) of the versioned hash (based on SHA256).
+constexpr auto VERSIONED_HASH_SIZE = SHA256_HASH_SIZE;
+
+/// The KZG version number of the versioned hash.
+constexpr std::byte VERSIONED_HASH_VERSION_KZG{0x01};
+
+/// An EIP-4844 parameter.
+constexpr auto FIELD_ELEMENTS_PER_BLOB = 4096_u256;
+
+/// Scalar field modulus of BLS12-381.
+constexpr auto BLS_MODULUS =
+    52435875175126190479447740508185965837690552500527637822603658699938581184513_u256;
+
+/// Number of significant bits of the BLS_MODULUS.
+constexpr size_t BLS_MODULUS_BITS = 255;
+static_assert(BLS_MODULUS >> BLS_MODULUS_BITS == 0);
+
+bool kzg_verify_proof(const std::byte versioned_hash[VERSIONED_HASH_SIZE], const std::byte z[32],
     const std::byte y[32], const std::byte commitment[48], const std::byte proof[48]) noexcept;
-}
+}  // namespace evmone::crypto

test/precompiles_bench/precompiles_bench.cpp

@@ -124,7 +124,7 @@ const inline std::array inputs<PrecompileId::ecpairing>{
 
 template <>
 const inline std::array inputs<PrecompileId::point_evaluation>{
-    // Inputs taken randomely from Mainnet
+    // Inputs taken randomly from Mainnet
     // https://etherscan.io/address/0x000000000000000000000000000000000000000a/advanced#internaltx
     "012b08a0504a63aac18383db69fe6b52fc833e3d060b87c2726c4140c909d91807dddd3c80995c2bb3012943e2036e77490b1f6ddc58ca39a4fb4f3225ae56ab11dc2c4d89f777f0f5c2a51f45b73ff1538761f9cf23ed74c74472fea625ad8bace1db77e25ceb316d914182e05dd810f112352e1d6ed9e47af28e2f64e22b94c411794359c2273bc10bc0390963fb1a97bb642307bfa4424c66bd90ecc0ecffd5045e492b40304df20346693db7450457e2c72588a6a2b1a16909e2ab1e6284"_hex,
     "019cd755316533108b9eade41e35a16442ae76acd5b7d4e8903ecb9d9f48348a00000000000000000000000000000000dd372dcb4e5565861fc29cfb12f4373861e6e2dfca75084191a505f7988db8e82a4a4a09734b6fd7677d590a1cb512768c381fc4957f406ef89996d9dfa1d39b5c8d1368569e56fd61036c537400a3f4515eeb0c4d183142daa2c30423e0c3fa84667445c1669d3a3e3fce8a1144811e4452841399318c21cca9d20c91fb162929c4e96d391b70158bcd4c69b682b272"_hex,

test/state/precompiles.cpp

@@ -293,14 +293,6 @@ ExecutionResult blake2bf_execute(const uint8_t* input, [[maybe_unused]] size_t i
     return {EVMC_SUCCESS, sizeof(h)};
 }
 
-namespace
-{
-using intx::operator""_u256;
-constexpr auto FIELD_ELEMENTS_PER_BLOB = 4096_u256;
-constexpr auto BLS_MODULUS =
-    52435875175126190479447740508185965837690552500527637822603658699938581184513_u256;
-}  // namespace
-
 ExecutionResult point_evaluation_execute(const uint8_t* input, size_t input_size, uint8_t* output,
     [[maybe_unused]] size_t output_size) noexcept
 {
@@ -317,8 +309,10 @@ ExecutionResult point_evaluation_execute(const uint8_t* input, size_t input_size
     if (!r)
         return {EVMC_PRECOMPILE_FAILURE, 0};
 
-    intx::be::unsafe::store(output, FIELD_ELEMENTS_PER_BLOB);
-    intx::be::unsafe::store(output + 32, BLS_MODULUS);
+    // Return FIELD_ELEMENTS_PER_BLOB and BLS_MODULUS as padded 32 byte big endian values
+    // as required by the EIP-4844.
+    intx::be::unsafe::store(output, crypto::FIELD_ELEMENTS_PER_BLOB);
+    intx::be::unsafe::store(output + 32, crypto::BLS_MODULUS);
     return {EVMC_SUCCESS, 64};
 }
 

test/unittests/precompiles_kzg_test.cpp

@@ -3,7 +3,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #include <evmc/evmc.hpp>
-#include <evmc/hex.hpp>
 #include <evmone_precompiles/kzg.hpp>
 #include <evmone_precompiles/sha256.hpp>
 #include <gtest/gtest.h>
@@ -30,7 +29,7 @@ auto versioned_hash(std::span<const std::byte> input) noexcept
 {
     std::array<std::byte, 32> hash{};
     sha256(hash.data(), input.data(), input.size());
-    hash[0] = std::byte{1};
+    hash[0] = VERSIONED_HASH_VERSION_KZG;
     return hash;
 }
 }  // namespace