Add real EIP-4844 KZG support (vendored blst + c-kzg-4844)#99
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (91)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (62)
📝 WalkthroughWalkthroughAdds vendored BLST and c-kzg sources, wires them into the build, and exposes KZG/EIP-4844 and EIP-7594 APIs through C and Zig wrappers. It also adds docs, tests, vendor/license files, and a Blob sidecar builder. ChangesVendored KZG and BLST stack
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/kzg.zig (1)
342-348: 💤 Low valueComment mentions restoration but no code follows.
The comment at line 347-348 says "Restore for any subsequent ordering-independent tests" but there's no actual restoration code. While other tests call
init()at their start anyway, consider either removing the misleading comment or adding the restoration:💡 Option: Remove misleading comment
test "kzg verify rejects when not initialized" { // Ensure clean state for this test. deinit(); var blob: Blob = `@splat`(0); try testing.expectError(error.NotInitialized, blobToKzgCommitment(&blob)); - // Restore for any subsequent ordering-independent tests. }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/kzg.zig` around lines 342 - 348, The trailing comment "Restore for any subsequent ordering-independent tests" in the test "kzg verify rejects when not initialized" is misleading because no restoration occurs; fix by either removing that comment or actually restoring the global state by calling init() at the end of this test (after the testing.expectError call) so subsequent tests have the expected initialized state; locate the test block around the deinit() and blobToKzgCommitment(&blob) usage and apply one of those two changes.src/crypto/blst/src/e2.c (1)
260-260: src/crypto/blst/src/e2.c: Line 260BLS12_381_Rx.pis effectively equivalent toBLS12_381_Rx.p2forPOINTonE2::Z
POINTonE2::Zisvec384x(FP2:vec384x[2]), andvec_is_equalcompares bytes oversizeof(in->Z).BLS12_381_Rxis a union where.pand.p2share the same underlying storage, with the FP2 “one” laid out as(ONE_MONT_P, 0), so comparing againstBLS12_381_Rx.povervec384xbytes matches whatBLS12_381_Rx.p2would provide. Consider using.p2here purely for consistency/readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/crypto/blst/src/e2.c` at line 260, The comparison uses BLS12_381_Rx.p which is backend-equivalent to p2 but confusing for the FP2 field: update the vec_is_equal call that compares POINTonE2::Z (a vec384x[2] FP2) to use BLS12_381_Rx.p2 instead of BLS12_381_Rx.p for clarity/readability while keeping the same byte-length (sizeof(in->Z)) and the same vec_is_equal usage; locate the check around the vec_is_equal(in->Z, BLS12_381_Rx.p, sizeof(in->Z)) and replace the right-hand operand with BLS12_381_Rx.p2.src/crypto/blst/src/vect.h (1)
35-40: 💤 Low valueDead code: unreachable preprocessor branch.
This
#elifbranch is unreachable because lines 18-24 already handle the__BLST_NO_ASM__and__wasm64__cases. The condition at line 35 can never be true.This is harmless and likely an artifact of the patch described in lines 11-17. Could be removed for clarity, but not required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/crypto/blst/src/vect.h` around lines 35 - 40, The preprocessor branch that typedefs limb_t and defines LIMB_T_BITS and __BLST_NO_ASM__ is unreachable because __BLST_NO_ASM__ and __wasm64__ are already handled earlier; remove the redundant `#elif` block (the typedef unsigned int limb_t, # define LIMB_T_BITS 32 and the # define __BLST_NO_ASM__ inside it) to eliminate dead code and avoid confusing duplicated definitions, ensuring any remaining references to limb_t and LIMB_T_BITS come from the intended earlier branch.src/crypto/blst/src/pairing.c (1)
427-429: 💤 Low valuePotential undefined behavior with NULL pointer arithmetic (vendored code).
If the first element of
QsorPsarrays is NULL, this performsNULL+1sinceQptr/Pptrare initialized to NULL on lines 423-424. Per the C standard, pointer arithmetic on NULL is undefined behavior.This appears to be intentional API design where NULL means "contiguous from previous," implying the caller must ensure the first element is non-NULL. Given this is vendored blst code with established usage patterns, no change is recommended, but worth noting if issues arise during integration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/crypto/blst/src/pairing.c` around lines 427 - 429, The loop using Qptr and Pptr risks NULL pointer arithmetic when the first element of Qs or Ps is NULL; update the initialization and loop in pairing.c so Qptr and Pptr are initialized from the first array element (e.g., set Qptr = *Qs ? *Qs : NULL and similarly for Pptr) and then within the for-loop use conditional logic that advances via pointer arithmetic only when the previous pointer is non-NULL (i.e., if (*Qs) use *Qs++ else if (Qptr) Qptr = Qptr+1; same for Ps), or alternatively require and assert the first element is non-NULL before entering the loop to avoid NULL+1 undefined behavior; reference Qptr, Pptr, Qs, Ps and the for (i = 0, j = 0; j < n; j++) loop when making changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/crypto/c-kzg/src/eip7594/fft.c`:
- Around line 267-277: Both coset_fft and poly_lagrange_to_monomial allocate
buffers (e.g., call new_fr_array to create in_shifted) before validating the
input length; move the length validation to the top of each function so invalid
n (or related length parameters) are checked and return an error code before any
heap allocation. Specifically, in functions coset_fft and
poly_lagrange_to_monomial, validate the polynomial length arguments (n and any
dependent sizes) and return C_KZG_BAD_ARGS (or the function's error) immediately
if invalid, then proceed to call new_fr_array and other allocation/processing
(e.g., creating in_shifted, calling shift_poly and fr_fft) only after the checks
pass. Ensure all early-return paths consistently free nothing (since nothing
allocated) and maintain existing error codes.
In `@src/kzg.zig`:
- Around line 197-201: The deinit() function has a TOCTOU race: replace the
plain load-check with an atomic compare-and-exchange on init_state so only one
caller transitions the state (e.g., STATE_READY -> STATE_DEINITIALIZING or
STATE_UNINIT) and performs free_trusted_setup(&settings); use `@atomicCmpxchg`
(cmpxchgStrong semantics) against init_state and only call free_trusted_setup
when your cmpxchg succeeded; ensure you store the final STATE_UNINIT (with
.release) after the free, and leave other callers returning early when their
cmpxchg fails.
---
Nitpick comments:
In `@src/crypto/blst/src/e2.c`:
- Line 260: The comparison uses BLS12_381_Rx.p which is backend-equivalent to p2
but confusing for the FP2 field: update the vec_is_equal call that compares
POINTonE2::Z (a vec384x[2] FP2) to use BLS12_381_Rx.p2 instead of BLS12_381_Rx.p
for clarity/readability while keeping the same byte-length (sizeof(in->Z)) and
the same vec_is_equal usage; locate the check around the vec_is_equal(in->Z,
BLS12_381_Rx.p, sizeof(in->Z)) and replace the right-hand operand with
BLS12_381_Rx.p2.
In `@src/crypto/blst/src/pairing.c`:
- Around line 427-429: The loop using Qptr and Pptr risks NULL pointer
arithmetic when the first element of Qs or Ps is NULL; update the initialization
and loop in pairing.c so Qptr and Pptr are initialized from the first array
element (e.g., set Qptr = *Qs ? *Qs : NULL and similarly for Pptr) and then
within the for-loop use conditional logic that advances via pointer arithmetic
only when the previous pointer is non-NULL (i.e., if (*Qs) use *Qs++ else if
(Qptr) Qptr = Qptr+1; same for Ps), or alternatively require and assert the
first element is non-NULL before entering the loop to avoid NULL+1 undefined
behavior; reference Qptr, Pptr, Qs, Ps and the for (i = 0, j = 0; j < n; j++)
loop when making changes.
In `@src/crypto/blst/src/vect.h`:
- Around line 35-40: The preprocessor branch that typedefs limb_t and defines
LIMB_T_BITS and __BLST_NO_ASM__ is unreachable because __BLST_NO_ASM__ and
__wasm64__ are already handled earlier; remove the redundant `#elif` block (the
typedef unsigned int limb_t, # define LIMB_T_BITS 32 and the # define
__BLST_NO_ASM__ inside it) to eliminate dead code and avoid confusing duplicated
definitions, ensuring any remaining references to limb_t and LIMB_T_BITS come
from the intended earlier branch.
In `@src/kzg.zig`:
- Around line 342-348: The trailing comment "Restore for any subsequent
ordering-independent tests" in the test "kzg verify rejects when not
initialized" is misleading because no restoration occurs; fix by either removing
that comment or actually restoring the global state by calling init() at the end
of this test (after the testing.expectError call) so subsequent tests have the
expected initialized state; locate the test block around the deinit() and
blobToKzgCommitment(&blob) usage and apply one of those two changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f946411-13c3-468a-9915-2e196324c93d
📒 Files selected for processing (91)
CHANGELOG.mdREADME.mdbuild.zigbuild.zig.zondocs/content/docs/kzg.mdxdocs/content/docs/meta.jsonsrc/blob.zigsrc/crypto/blst/LICENSEsrc/crypto/blst/bindings/blst.hsrc/crypto/blst/bindings/blst_aux.hsrc/crypto/blst/src/aggregate.csrc/crypto/blst/src/bulk_addition.csrc/crypto/blst/src/bytes.hsrc/crypto/blst/src/client_min_pk.csrc/crypto/blst/src/client_min_sig.csrc/crypto/blst/src/consts.csrc/crypto/blst/src/consts.hsrc/crypto/blst/src/cpuid.csrc/crypto/blst/src/e1.csrc/crypto/blst/src/e2.csrc/crypto/blst/src/ec_mult.hsrc/crypto/blst/src/ec_ops.hsrc/crypto/blst/src/errors.hsrc/crypto/blst/src/exp.csrc/crypto/blst/src/exports.csrc/crypto/blst/src/fields.hsrc/crypto/blst/src/fp12_tower.csrc/crypto/blst/src/hash_to_field.csrc/crypto/blst/src/keygen.csrc/crypto/blst/src/map_to_g1.csrc/crypto/blst/src/map_to_g2.csrc/crypto/blst/src/multi_scalar.csrc/crypto/blst/src/no_asm.hsrc/crypto/blst/src/pairing.csrc/crypto/blst/src/pentaroot-addchain.hsrc/crypto/blst/src/pentaroot.csrc/crypto/blst/src/point.hsrc/crypto/blst/src/rb_tree.csrc/crypto/blst/src/recip-addchain.hsrc/crypto/blst/src/recip.csrc/crypto/blst/src/server.csrc/crypto/blst/src/sha256.hsrc/crypto/blst/src/sqrt-addchain.hsrc/crypto/blst/src/sqrt.csrc/crypto/blst/src/vect.csrc/crypto/blst/src/vect.hsrc/crypto/c-kzg/LICENSEsrc/crypto/c-kzg/VENDOR.mdsrc/crypto/c-kzg/src/ckzg.csrc/crypto/c-kzg/src/ckzg.hsrc/crypto/c-kzg/src/common/alloc.csrc/crypto/c-kzg/src/common/alloc.hsrc/crypto/c-kzg/src/common/bytes.csrc/crypto/c-kzg/src/common/bytes.hsrc/crypto/c-kzg/src/common/ec.csrc/crypto/c-kzg/src/common/ec.hsrc/crypto/c-kzg/src/common/fr.csrc/crypto/c-kzg/src/common/fr.hsrc/crypto/c-kzg/src/common/lincomb.csrc/crypto/c-kzg/src/common/lincomb.hsrc/crypto/c-kzg/src/common/ret.hsrc/crypto/c-kzg/src/common/utils.csrc/crypto/c-kzg/src/common/utils.hsrc/crypto/c-kzg/src/eip4844/blob.csrc/crypto/c-kzg/src/eip4844/blob.hsrc/crypto/c-kzg/src/eip4844/eip4844.csrc/crypto/c-kzg/src/eip4844/eip4844.hsrc/crypto/c-kzg/src/eip7594/cell.csrc/crypto/c-kzg/src/eip7594/cell.hsrc/crypto/c-kzg/src/eip7594/eip7594.csrc/crypto/c-kzg/src/eip7594/eip7594.hsrc/crypto/c-kzg/src/eip7594/fft.csrc/crypto/c-kzg/src/eip7594/fft.hsrc/crypto/c-kzg/src/eip7594/fk20.csrc/crypto/c-kzg/src/eip7594/fk20.hsrc/crypto/c-kzg/src/eip7594/poly.csrc/crypto/c-kzg/src/eip7594/poly.hsrc/crypto/c-kzg/src/eip7594/recovery.csrc/crypto/c-kzg/src/eip7594/recovery.hsrc/crypto/c-kzg/src/setup/settings.hsrc/crypto/c-kzg/src/setup/setup.csrc/crypto/c-kzg/src/setup/setup.hsrc/crypto/c-kzg/src/trusted_setup.txtsrc/crypto/c-kzg/test_vectors/ATTRIBUTION.mdsrc/crypto/c-kzg/test_vectors/blob_to_kzg_commitment_19b3f3f8c98ea31e.yamlsrc/crypto/c-kzg/test_vectors/compute_blob_kzg_proof_19b3f3f8c98ea31e.yamlsrc/crypto/c-kzg/test_vectors/verify_blob_kzg_proof_correct_19b3f3f8c98ea31e.yamlsrc/crypto/c-kzg/test_vectors/verify_blob_kzg_proof_incorrect_19b3f3f8c98ea31e.yamlsrc/kzg.zigsrc/kzg_vectors_test.zigsrc/root.zig
Closes the one client-library feature gap vs Voltaire: building a blob transaction sidecar from raw blob data. Real, vector-verified crypto -- no stubs. - Vendored blst v0.3.14 (portable no-asm C) and c-kzg-4844 v2.1.1 under src/crypto/blst and src/crypto/c-kzg, matching the existing secp256k1/XKCP vendoring pattern. blst built with -D__BLST_NO_ASM__ via a small documented vect.h patch that hoists the no-asm branch above the arch branch (the pure-C backend only defines llimb_t for 32-bit limbs). build.zig gains addKzg; build.zig.zon .paths updated. - src/kzg.zig (eth.kzg): init/deinit load the embedded mainnet trusted setup via fmemopen over @embedfile (no disk access, idempotent once- guard); blobToKzgCommitment, computeBlobKzgProof, verifyBlobKzgProof, verifyBlobKzgProofBatch over the c-kzg FFI. blob.buildSidecar wires raw blob -> commitment + proof + versioned hash. - Official c-kzg-4844 v2.1.1 test vectors asserted byte-for-byte (blob_to_kzg_commitment / compute_blob_kzg_proof / verify both correct and incorrect), plus round-trip, batch, versioned-hash, and lifecycle tests. 839/839 tests pass on 0.16.0 (clean build compiles the C from scratch); fmt clean; docs build (new kzg.mdx). Note: a clean build now compiles blst (~2 min) the first time, then caches.
Two concurrent deinit() calls could both observe STATE_READY and both call free_trusted_setup. Claim the teardown with a single cmpxchg(READY -> UNINIT) so only the winning thread frees.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Closes the one genuine client-library feature gap vs Voltaire: building a blob-transaction sidecar from raw blob data. We had blob types + versioned-hash computation but no KZG commitment/proof crypto. This is real, vector-verified KZG -- no stubs.
What
src/crypto/blst/src/crypto/c-kzg, matching the existing secp256k1/XKCP vendoring (zero external deps).build.ziggainsaddKzg;.pathsupdated.llimb_tfor 32-bit limbs, butvect.hselects 64-bit on aarch64 before the no-asm branch. A small, commentedvect.hpatch hoists the__BLST_NO_ASM__branch above the arch branch, so-D__BLST_NO_ASM__yields the pure-C backend on every target -- no per-arch assembly to vendor.src/kzg.zig(eth.kzg):init/deinitload the embedded mainnet trusted setup viafmemopenover@embedFile(no disk access, idempotent once-guard);blobToKzgCommitment,computeBlobKzgProof,verifyBlobKzgProof,verifyBlobKzgProofBatch.blob.buildSidecar(raw_blob)-> commitment + proof + versioned hash.Correctness (vector-verified)
The official c-kzg-4844 v2.1.1 test vectors are embedded and asserted byte-for-byte:
blob_to_kzg_commitmentandcompute_blob_kzg_proofreproduce the official output exactly, andverify_blob_kzg_proofreturns true/false for the correct/incorrect vectors. Plus round-trip, batch, versioned-hash, and lifecycle tests.Verification
839/839 tests pass on 0.16.0 with a clean build compiling blst + c-kzg from scratch;
fmtclean; docs build (newkzg.mdx). Note: the first clean build now compiles blst (~2 min), then caches.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests