precompiles: Implement KZG proof verification (aka "point evaluation") #979
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #979 +/- ##
=======================================
Coverage 94.18% 94.18%
=======================================
Files 147 149 +2
Lines 15843 15884 +41
=======================================
+ Hits 14922 14961 +39
- Misses 921 923 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
chfast
force-pushed
the
precompiles/kzg_point_evaluation
branch
from
8fb5592 to
d8d3821
Compare
chfast
force-pushed
the
precompiles/kzg_point_evaluation
branch
from
d8d3821 to
6ae4882
Compare
|
Benchmarks of all precompiles using clang-18 compiler on Intel Ultra 7 155U 4.0 GHz: |
|
The benchmarks of Constantine for reference (same hardware and compiler): |
| constexpr blst_p2_affine KZG_SETUP_G2_1{ | ||
| {{{0x6120a2099b0379f9, 0xa2df815cb8210e4e, 0xcb57be5577bd3d4f, 0x62da0ea89a0c93f8, | ||
| 0x02e0ee16968e150d, 0x171f09aea833acd5}, | ||
| {0x11a3670749dfd455, 0x04991d7b3abffadc, 0x85446a8e14437f41, 0x27174e7b4e76e3f2, | ||
| 0x7bfa6dd397f60a20, 0x02fcc329ac07080f}}}, | ||
| {{{0xaa130838793b2317, 0xe236dd220f891637, 0x6502782925760980, 0xd05c25f60557ec89, | ||
| 0x6095767a44064474, 0x185693917080d405}, | ||
| {0x549f9e175b03dc0a, 0x32c0c95a77106cfe, 0x64a74eae5705d080, 0x53deeaf56659ed9e, | ||
| 0x09a1d368508afb93, 0x12cf3a4525b5e9bd}}}}; |
There was a problem hiding this comment.
Are these checked against the json somewhere?
There are actually multiple trusted setups, so if we change it in the future to a different one, it would be good to have some check that fails somewhere.
Not a blocker imo
There was a problem hiding this comment.
For reference, this is from the trusted setup of size 4096, where size denotes how many G1 elements there are. There are always 65 G2 elements IIRC in all of the trusted setups that we created
There was a problem hiding this comment.
The original code pointed to https://github.com/ethereum/consensus-specs/blob/dev/presets/mainnet/trusted_setups/trusted_setup_4096.json, but it is not very useful, because I don't know what this JSON has (e.g. it has 8192 entries for G1?). With some effort we could take the pointed value and pre-process it to the blst internal form at compile time. But we are not ready for it yet.
| std::optional<blst_scalar> validate_scalar(std::span<const std::byte, 32> b) noexcept | ||
| { | ||
| blst_scalar v; | ||
| blst_scalar_from_bendian(&v, reinterpret_cast<const uint8_t*>(b.data())); | ||
| return blst_scalar_fr_check(&v) ? std::optional{v} : std::nullopt; | ||
| } | ||
|
|
||
| /// Uncompress and validate a point from G1 subgroup. | ||
| std::optional<blst_p1_affine> validate_G1(std::span<const std::byte, 48> b) noexcept |
There was a problem hiding this comment.
I haven't looked at the rest of the code, so this may not be easy; the 32 and 48 are well defined constants in the spec -- is it not possible to use them instead of using magic numbers?
There was a problem hiding this comment.
Yes, make sense. I plan to work on this but I need a separate PR for this to explore some directions (e.g. use std::span).
lib/evmone_precompiles/kzg.cpp
Outdated
| 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. |
There was a problem hiding this comment.
Note to self: check if this is actually doing a subgroup check or just a group check
There was a problem hiding this comment.
There is blst_p1_affine_on_curve which would be a group check, so in g1 is the subgroup check
| blst_p1 r; | ||
| blst_p1_add_or_double_affine(&r, &q, &p); | ||
| blst_p1_affine ra; | ||
| blst_p1_to_affine(&ra, &r); |
There was a problem hiding this comment.
Note that this may hurt performance, if you plan to do multiple adds or doubles in a row
lib/evmone_precompiles/kzg.cpp
Outdated
| blst_p1 mult(const blst_p1& p, const blst_scalar& v) noexcept | ||
| { | ||
| blst_p1 r; | ||
| blst_p1_mult(&r, &p, v.b, 255); |
There was a problem hiding this comment.
255 is the safe choice here, though I think there might be a method in blst that tells you how many bits a scalar is and that could be used to optimize this a bit -- unclear to me what the diff would be though
There was a problem hiding this comment.
This is what the BLST C++ API is using. I used a named constant now.
test/state/precompiles.cpp
Outdated
|
|
||
| ExecutionResult point_evaluation_execute(const uint8_t* input, size_t input_size, uint8_t* output, | ||
| [[maybe_unused]] size_t output_size) noexcept | ||
| { | ||
| assert(output_size >= 64); | ||
| if (input_size != 192) | ||
| return {EVMC_PRECOMPILE_FAILURE, 0}; | ||
|
|
||
| const auto r = crypto::kzg_verify_proof(reinterpret_cast<const std::byte*>(&input[0]), | ||
| reinterpret_cast<const std::byte*>(&input[32]), | ||
| reinterpret_cast<const std::byte*>(&input[64]), | ||
| reinterpret_cast<const std::byte*>(&input[96]), | ||
| reinterpret_cast<const std::byte*>(&input[96 + 48])); | ||
|
|
||
| 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 {EVMC_SUCCESS, 64}; | ||
| } |
There was a problem hiding this comment.
For this, could we link to the eip method or explain what the offsets are being used for?
lib/evmone_precompiles/kzg.cpp
Outdated
| { | ||
| std::byte computed_versioned_hash[32]; | ||
| sha256(computed_versioned_hash, commitment, 48); | ||
| computed_versioned_hash[0] = std::byte{0x01}; |
There was a problem hiding this comment.
Note to self: 0x01 is VERSIONED_HASH_VERSION_KZG
There was a problem hiding this comment.
The constant name in use now.
| const auto neg_Y = mult(G1_GENERATOR_NEGATIVE, *yy); | ||
|
|
||
| // Compute C - Y. It can happen that C == -Y so doubling may be needed. | ||
| const auto C_sub_Y = add_or_double(*C, neg_Y); |
There was a problem hiding this comment.
I would probably always use add_or_double everywhere as I'm guessing the performance impact is negligible and you won't need to justify using one over the other
| ExecutionResult point_evaluation_execute(const uint8_t* input, size_t input_size, uint8_t* output, | ||
| [[maybe_unused]] size_t output_size) noexcept | ||
| { | ||
| assert(output_size >= 64); |
There was a problem hiding this comment.
Oh the output could have came from something that allocated more than 64 bytes, the caller can just take the first 64 bytes
There was a problem hiding this comment.
These are internals of the precompiles framework. It is expected the caller to allocate at least 64 for the output.
chfast
force-pushed
the
precompiles/kzg_point_evaluation
branch
3 times, most recently
from
c863dbc to
97dfa6f
Compare
|
Big thanks @kevaundray for the review. I believe I addressed most of the comments. |
Yep -- I have a note to look at In particular, it calls |
This is also my concern, although @mratsim said this is fine. I'll try to investigate too. |
chfast
force-pushed
the
precompiles/kzg_point_evaluation
branch
from
97dfa6f to
5699512
Compare
The void blst_aggregated_in_g1(vec384fp12 ret, const POINTonE1_affine *sig)
{ miller_loop_n(ret, (const POINTonE2_affine *)&BLS12_381_G2, sig, 1); } |
chfast
force-pushed
the
precompiles/kzg_point_evaluation
branch
from
5699512 to
4155aaa
Compare
| const blst_p1_affine& a1, const blst_p1_affine& b1, const blst_p2_affine& b2) noexcept | ||
| { | ||
| blst_fp12 left; | ||
| blst_aggregated_in_g1(&left, &a1); |
There was a problem hiding this comment.
This in practice just does miller_loop with G2 generator, do it may be good not to confuse readers with using this API.
There was a problem hiding this comment.
imo this would be better -- following the method and seeing signature was a bit confusing at first
| blst_aggregated_in_g1(&left, &a1); | ||
| blst_fp12 right; | ||
| blst_miller_loop(&right, &b2, &b1); | ||
| return blst_fp12_finalverify(&left, &right); |
There was a problem hiding this comment.
This seems to be doing the same as "classic" pairings_verify from c-kzg.
- https://github.com/ethereum/c-kzg-4844/blob/v2.0.1/src/common/utils.c#L164
- https://github.com/supranational/blst/blob/v0.3.13/src/aggregate.c#L506
How about instead of negating the a1 at runtime we pre-compute negative G2 generator?
Can we also use blst_miller_loop_n?
There was a problem hiding this comment.
Yep, can compute the negative G2 generator at compile time -- I think the negation would cost on the order of 10 nanoseconds which is likely why its done at runtime in other codebases (pairings is on the order of a millisecond)
chfast
force-pushed
the
precompiles/kzg_point_evaluation
branch
from
4155aaa to
0aa75e4
Compare
|
Can we add a source where |
Implement the KZG proof verification following the spec of `verify_kfz_proof` in https://eips.ethereum.org/EIPS/eip-4844#point-evaluation-precompile.
Use `crypto::kzg_verify_proof` to implement and enable the `point_evaluation` precompile.
chfast
force-pushed
the
precompiles/kzg_point_evaluation
branch
from
0aa75e4 to
85e7ff7
Compare
Implement the KZG proof verification using the blst library.
Add native
point_evaluationimplementation.Add benchmarks and report the comparison with Silkworm's implementation (see PR).