Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates the aggregator to the v2 wire format, replacing algorithm-prefixed imprints with raw 32-byte hashes. The core of the change involves a redesigned Sparse Merkle Tree (SMT) implementation that utilizes domain-separated hashing, unary passthrough nodes, and LSB-first bit addressing. Additionally, the PR introduces the InclusionCert wire format and removes legacy v1 RPC endpoints. Feedback identifies a critical gap in test coverage due to skipped sharding end-to-end tests and recommends implementing full cryptographic verification in performance tests to ensure proof correctness.
There was a problem hiding this comment.
Pull request overview
This PR upgrades the aggregator’s Sparse Merkle Tree and inclusion proof handling to the v2 “yellowpaper-aligned” semantics and wire format, shifting identifiers to raw 32-byte hashes and moving the public JSON-RPC surface away from v1 endpoints.
Changes:
- Implement v2 SMT hashing semantics (domain-separated leaf/node hashing, unary passthrough) and 256-bit raw-key addressing (LSB-first).
- Introduce the v2 inclusion proof payload built around
(CertificationData, CertificateBytes, UnicityCertificate)and verification viaInclusionCert+UC.IR.h. - Remove/disable v1-facing integration paths (methods/tests/docs) and update storage/root-hash handling to use raw 32-byte roots.
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/integration/sharding_e2e_test.go | Skips sharding E2E while child-mode v2 proofs are pending; updates SMT key length usage. |
| test/integration/commitment_versions_compatibility_test.go | Removes v1/v2 compatibility integration test (v1 surface removed). |
| pkg/api/types_test.go | Updates fixtures to use raw 32-byte v2 identifiers. |
| pkg/api/types.go | Redefines v2 inclusion proof payload and adds v2 proof verification bound to UC.IR.h + InclusionCert. |
| pkg/api/state_id_path_test.go | Adds test ensuring legacy-prefixed vs raw encodings map to same path. |
| pkg/api/state_id_bitorder_test.go | Adds tests pinning LSB-first key bit addressing and roundtrip conversions. |
| pkg/api/state_id.go | Introduces canonical 256-bit key constants and implements raw-key path encoding/decoding utilities. |
| pkg/api/smt.go | Updates MerkleTreePath verification to v2 hashing rules and key reconstruction. |
| pkg/api/merkle_tree_path_verify_test.go | Adapts SMT path verification tests to v2 key size and normalization. |
| pkg/api/inclusion_proof_v2_verify_test.go | Adds unit tests for InclusionProofV2.Verify success/failure cases. |
| pkg/api/inclusion_cert_test.go | Adds unit tests for InclusionCert/ExclusionCert codecs and inclusion verification behavior. |
| pkg/api/inclusion_cert.go | Adds InclusionCert + ExclusionCert wire codecs and v2 inclusion verification logic. |
| pkg/api/certification_request.go | Updates v2 docs/comments to describe raw 32-byte hashes and new hashing semantics. |
| internal/smt/yellowpaper_hash_semantics_test.go | Adds tests pinning domain separation and unary passthrough semantics. |
| internal/smt/thread_safe_smt_test.go | Adds test coverage for AddPreHashedLeaf behavior in parent SMT usage. |
| internal/smt/thread_safe_smt_snapshot.go | Exposes raw root + inclusion cert generation on snapshots. |
| internal/smt/thread_safe_smt.go | Adds raw-root and inclusion-cert getters; implements AddPreHashedLeaf via AddLeaf. |
| internal/smt/smt_test.go | Updates SMT root fixtures/vectors for v2 hashing semantics and 256-bit keys. |
| internal/smt/smt_memory_benchmark_test.go | Updates benchmark SMT key length to v2 256-bit. |
| internal/smt/smt_debug_test.go | Updates SMT initialization to v2 key length constant. |
| internal/smt/smt.go | Implements v2 SMT hashing, stores leaf key bytes, adds raw-root getter and inclusion cert generation. |
| internal/smt/inclusion_cert_test.go | Adds SMT-level inclusion cert generator tests (golden vectors, fuzz-ish checks). |
| internal/smt/golden_vectors_test.go | Adds/updates golden vectors for v2 root and bitmap/sibling extraction. |
| internal/signing/certification_request_validator.go | Updates transaction-hash validation error message for strict 32-byte v2 hash. |
| internal/service/v2_surface_test.go | Adds test asserting removed v1 JSON-RPC methods return MethodNotFound. |
| internal/service/service_test.go | Updates service tests to validate v2 proof structure and verify via InclusionProofV2.Verify. |
| internal/service/service.go | Reworks GetInclusionProofV2 to emit v2 certificates and bind roots via raw SMT root / UC.IR.h. |
| internal/service/parent_service.go | Updates parent shard proof block lookup to use raw root hash bytes. |
| internal/round/smt_persistence_integration_test.go | Updates persistence/restoration tests to use raw-root form and v2 key length. |
| internal/round/round_manager_test.go | Updates SMT instantiation to v2 key length constant. |
| internal/round/round_manager.go | Updates persistence restore to invert fixed-bytes keys back into sentinel paths; compares roots as raw bytes. |
| internal/round/recovery_test.go | Updates recovery logic/tests to store SMT keys as fixed bytes and leaf values via v2 LeafValue(). |
| internal/round/recovery.go | Updates recovery to use fixed-byte SMT keys and FixedBytesToPath when rebuilding leaves. |
| internal/round/precollection_test.go | Updates test SMT creation to v2 key length constant. |
| internal/round/parent_round_manager_test.go | Updates parent manager tests to use raw-root comparisons and bytes.Equal. |
| internal/round/parent_round_manager.go | Updates parent root handling to use raw 32-byte roots and byte comparisons. |
| internal/round/finalize_duplicate_test.go | Updates SMT creation to v2 key length constant. |
| internal/round/batch_processor.go | Updates block proposal/root handling to raw-root bytes; converts persisted SMT node keys via PathToFixedBytes. |
| internal/models/certification_request_leafvalue_test.go | Adds test pinning v2 leaf value semantics (tx hash bytes). |
| internal/models/certification_request.go | Changes v2 leaf value to use transaction hash bytes (not CertificationData hash). |
| internal/metrics/metrics.go | Removes v1 methods from knownMethods label allowlist. |
| internal/ha/block_syncer_test.go | Updates block sync tests to use v2 key length, v2 leaf values, and raw-root comparisons. |
| internal/ha/block_syncer.go | Updates SMT update/verification flow to use fixed-byte keys and raw-root comparisons. |
| internal/gateway/server.go | Removes v1 JSON-RPC handlers from standalone surface; registers only v2 methods. |
| internal/gateway/handlers_v1.go | Deletes v1 JSON-RPC handler implementations. |
| internal/gateway/handlers.go | Adds strict v2 stateId length validation and updated handler docs. |
| internal/gateway/docs_test.go | Updates docs test expectations to raw 32-byte hashes. |
| internal/gateway/docs.go | Updates interactive docs to reflect get_inclusion_proof.v2 and raw 32-byte identifiers. |
| internal/bft/client_stub_test.go | Adds test ensuring stub BFT client populates a UC with IR.Hash bound to block root. |
| internal/bft/client_stub.go | Sets UC.InputRecord.Hash from block root in the stubbed BFT client. |
| cmd/performance-test/main.go | Updates perf test to v2 semantics and reduces verification to non-empty cert presence. |
| cmd/commitment/main.go | Updates CLI to submit certification_request and verify v2 proofs via InclusionProofV2.Verify. |
| cmd/aggregator/main.go | Updates SMT key length to v2 constant for all modes. |
| README.md | Updates API docs for get_inclusion_proof.v2 wire format, hashing rules, and key encoding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tence error handling)
…-format-v2-sharding # Conflicts: # internal/smt/thread_safe_smt_test.go
…o ParentInclusionFragment.Verify Extract shared MatchesShardPrefix into pkg/api and replace three duplicate implementations. Make ParentInclusionFragment.Verify take an explicit keyLength parameter instead of deriving it from shardID.BitLen().
Implement sharded v2 inclusion proof composition
MastaP
left a comment
There was a problem hiding this comment.
Thanks for the v2 rework — existing reviewer threads (SMT hash-cache race, uint8 depth overflow, silent convertLeavesToNodes skip, child-mode get_inclusion_proof.v2, v1 Service-method cleanup, TestShardingE2E skip) already look addressed; skipping those to avoid noise. A few additional observations on the current tip:
Potential issues
-
Child-mode proof composition doesn't assert root coherence.
GetInclusionProofV2(internal/service/service.go) callsGetLatestByRootHash(rootHashRaw)and latersmtInstance.GetRootHashRaw()forComposeInclusionCertunderFinalizationReadLock. Today the lock makes those observations consistent, but there's no explicit assertion that the two raw roots match — if the locking invariant ever loosens, composition could silently mix a child cert from root A with a parent fragment bound to root B.ThreadSafeSMT.GetShardInclusionFragmentWithRootalready atomically snapshots fragment+root under one RLock; mirroring that with aGetInclusionCertWithRootwould make the invariant local to the API and survive future refactors. -
byte(d)cast inInclusionCert.Verify.pkg/api/inclusion_cert.go:~117hashes{0x01, byte(d)}wheredloops frommaxDepth - 1 = 255down. TodaymaxDepth == StateTreeKeyLengthBits == 256, so the cast is safe — but if the key length is ever widened,byte(d)silently wraps and two structurally different nodes collide. Either reject depths > 255 explicitly or document the wire cap onStateTreeKeyLengthBitsat the type level. -
ParentBlockNumber == 0sentinel is ambiguous.validateBlockForModeininternal/round/batch_processor.gotreats zero as "missing parent block number." Parent block indices start at 1 in practice, butuint64(0)is also the zero value after a decode bug or a test that forgot to set it. Consider*uint64or a pairedHasParentMetadataflag to distinguish "unset" from "legitimately zero." -
InclusionCert.MarshalBinarycan't fail. The(error)return is alwaysnil. Same forExclusionCert.MarshalBinary. Either keep forencoding.BinaryMarshalercompatibility (fine — just add a one-line comment) or drop the error. -
RootShardInclusionProof.IsValidhides UC decode errors. Inpkg/api/types.go,ucInputRecordHashRawfailures collapse intofalse, whichpollForParentProofthen logs as a plain retry. A malformed UC from a misbehaving parent will loop silently. Consider(bool, error)so the poll loop can log at Warn when decoding fails (distinct from "not ready yet").
Style / maintainability
-
Depth arithmetic duplicated.
depthOffset + (commonPath.BitLen() - 1)appears 4× ininternal/smt/smt.goAddLeaf/buildTree. A small helper (nodeDepthAt(offset, commonPath)) would make the depth math easier to audit now that it's load-bearing for the v2 hash. -
selectShardIndexfallback comment.cmd/performance-test/main.go:~196says "Fallback only: keep distribution aligned with LSB-first key layout" but only looks atkeyBytes[0]. Worth stating that this branch only fires when the request didn't already match any configured shard, so readers don't mistake it for routing logic. -
pathToBitmapAndSiblingstest helper is scaffolding.internal/smt/golden_vectors_test.gocontains a helper that reverses between leaf-to-root and root-to-leaf sibling order for cross-format parity, with a comment mentioning "Rugregator" (typo → "Aggregator"). Once the legacyMerkleTreePathtype is removed, this helper can go too. -
AddPreHashedLeafwas previously a silent no-op. Good catch that this now actually delegates toAddLeaf; the newTestThreadSafeSMT_AddPreHashedLeaf_StoresChildRootpins the behavior. Worth confirming nothing else in tree construction relied on the old broken semantics (a grep suggests no).
Other notes
- Test coverage is strong:
golden_vectors_test.go,inclusion_cert_test.go(523 lines of wire + verify edge cases),yellowpaper_hash_semantics_test.go,thread_safe_smt_test.go, the new child-mode compose tests inservice_test.go, andstate_id_bitorder_test.goall look like solid regression protection against bit-order / wire-shape drift. - Security-wise, removing
JoinPaths(string-compared "child root must match first parent step") in favor of typed binaryInclusionCert.Verifybound strictly toUC.IR.his a clear net improvement. ensureHashes()on every write is O(dirty path) not O(N) thanks to thehashSetshort-circuit, so the race fix shouldn't movemake performance-testnumbers meaningfully — worth a run on the sharded topology to confirm before merge.
No description provided.