Conversation
…tment backlog findings
…cular acceptance oracle
… the v3 root oracle
…ead of re-encoding whole nodes
…to the worker, pool the per-node child key, drop two uncalled v3 helpers
…s, skip the legacy-keyed BAL warmup on a v3 datadir, report per-table chaindata in the bench
…, whose records are not at the legacy compact key
… is below the benchmark noise floor and the precondition has no guard
…laims but the domain does not hold
… domain returned before unfolding a row
…the helpers no production path calls
# Conflicts: # db/integrity/commitment_integrity.go # db/state/statecfg/state_schema.go
…ed, and drops two private paths
…e-shaped prefix still parses as legacy
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed compile-time issues in new/modified code and a legacy squeeze-path bug that can incorrectly transform the commitment state blob.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR evolves Erigon’s commitment storage to a v3 “edge-record” format (one KV record per touched trie edge) and wires that format through state/commitment read paths, merge/squeeze tooling, metrics, and tests, while also pulling in the stagedsync calcState per-block dirty list optimization.
Changes:
- Introduce v3 key/record encoding (per-edge records) and per-file version gating/format resolution so a datadir keeps its on-disk commitment format.
- Update commitment read/write paths (SharedDomains, warmup, simulation, deep fold) to efficiently walk sibling records and correctly meter reads.
- Expand tests/tooling (integration command, integrity/squeeze/merge/version tests) to cover both v2 bundled rows and v3 edge-record behavior.
File summaries
| File | Description |
|---|---|
| rpc/jsonrpc/witness_cache_builder_test.go | Switch commitment state reads to commitmentdb helper |
| rpc/jsonrpc/rpc_branch_cache_test.go | Use IsCommitmentStateKey filtering in tests |
| rpc/jsonrpc/eth_simulation.go | Add v3 record read path for simulation state reader |
| execution/stagedsync/stage_snapshots.go | Use commitmentdb latest-state accessor |
| execution/stagedsync/committer.go | Meter commitment record reads via worker metrics |
| execution/stagedsync/committer_step_boundary_test.go | Handle legacy vs v3 commitment state key/value in tests |
| execution/stagedsync/committer_reread_test.go | Add regression test for v3 reread across computes |
| execution/execmodule/exec_module_test.go | Handle legacy vs v3 commitment state value in execmodule tests |
| execution/exec/bal_commitment_warmup.go | Skip legacy compact-key lookups on v3 datadirs |
| execution/exec/bal_commitment_warmup_test.go | Update warmup tests for BranchCache edge-record mode |
| execution/commitment/warmuper.go | Reduce allocations and support v3 mask-driven warmup |
| execution/commitment/streaming_deep_fold.go | Support unfolding storage base via v3 records; mask propagation fixes |
| execution/commitment/state_blob_test.go | Add state-blob encoding/legacy decode tests and ordering checks |
| execution/commitment/recording_context.go | Add BranchWithMask passthrough + recording |
| execution/commitment/preload_test.go | Update BranchCache constructor usage |
| execution/commitment/preload_parallel_bench_test.go | Update BranchCache constructor usage in benchmark |
| execution/commitment/patricia_state_mock_test.go | Extend mock state to emulate v3 record reads and trie config |
| execution/commitment/parallel_testkit_test.go | Parameterize tests across v2/v3 formats |
| execution/commitment/parallel_patricia_hashed.go | Track update count once; plumb edge-record config into template |
| execution/commitment/parallel_patricia_hashed_test.go | Remove obsolete helper after testkit refactor |
| execution/commitment/parallel_mount.go | Ensure mounted workers inherit root masks/storage-account context |
| execution/commitment/parallel_metrics_test.go | Validate progress + branch-write publication for v2 and v3 |
| execution/commitment/nibbles/nibbles.go | Add allocation-reusing CompactToHexInto |
| execution/commitment/nibbles/nibbles_v3.go | Add v3 node-key and child-key encoding/decoding helpers |
| execution/commitment/hex_patricia_hashed_test.go | Update state encode/decode expectations to new state blob shape |
| execution/commitment/deepfold_test.go | Expand deepfold tests to run across both formats |
| execution/commitment/config.go | Add TrieConfig EdgeRecords toggle |
| execution/commitment/commitmentdb/reader_test.go | Add history/Latest tombstone preservation tests for v3 |
| execution/commitment/commitmentdb/deferred_v3_test.go | Add v3 deferred overlay + collector tombstone tests |
| execution/commitment/commitmentdb/commitment_context_test.go | Add edge-record synthesize and child-count routing tests |
| execution/commitment/commitment_test.go | Make BranchData helpers return errors for corrupt/truncated buffers |
| execution/commitment/branch_test.go | Clarify legacy contract-hash tests and add edge-shaped prefix case |
| execution/commitment/branch_cache_children_test.go | Add tests for caching/merging per-child v3 records |
| execution/commitment/adaptive_pin.go | Route contract hash resolution via cache helper |
| execution/commitment/adaptive_pin_test.go | Update BranchCache constructor usage |
| db/state/statecfg/versions.yaml | Bump commitment domain/hist/index versions for v3 |
| db/state/statecfg/version_schema_gen.go | Regenerate schema versions for commitment v3 and accessors |
| db/state/statecfg/statecfg.go | Add EdgeRecordsInCommitment domain config flag |
| db/state/statecfg/state_schema.go | Add v3 write-version gate; enable ordered accessors; env knob |
| db/state/statecfg/commitment_v3_version_test.go | Validate v3 version gates and schema wiring |
| db/state/statecfg/commitment_format_test.go | Assert schema follows edge-records knob |
| db/state/squeeze.go | Gate squeeze transforms by file format (legacy vs v3) |
| db/state/squeeze_test.go | Pin test aggregator to legacy format where needed |
| db/state/rebuild_shard_merge_test.go | Update debug reads to legacy state key |
| db/state/metrics.go | Add counters for record-walk outcomes/files consulted/scanned |
| db/state/merge.go | Derive merge output file version from inputs; per-file state key handling |
| db/state/history_test.go | Update fixtures to legacy state key |
| db/state/execctx/domain_shared.go | Add ReadCommitmentRecords path and commitment state fallback in GetAsOf |
| db/state/execctx/domain_shared_test.go | Read latest commitment state across both formats in tests |
| db/state/execctx/commitment_node_read_metrics_test.go | Validate known vs unknown mask node-read counters |
| db/state/domain.go | Name merged commitment .kv files based on input format; reject mixed format merges |
| db/state/domain_test.go | Update version acceptance tests and commitment-domain iterator expectations |
| db/state/domain_stream.go | Extend merge cursor items with commitment format metadata |
| db/state/domain_committed.go | Avoid transforming state key/state blob when expanding shortened keys |
| db/state/commitment_warmup_metrics_test.go | Ensure commitment reads are metered in both formats |
| db/state/commitment_version_testutil_test.go | Pin aggregator tests to legacy commitment format where required |
| db/state/commitment_tombstone_test.go | Validate tombstone semantics across scans and merges |
| db/state/commitment_records_test.go | Add sibling-run scan tests and nil-interface cursor regression |
| db/state/commitment_record_mask_metrics_test.go | Validate record-walk counters and exhaustion behavior |
| db/state/commitment_read_metrics_test.go | Validate v3 record reads meter IO and skip metering when disabled |
| db/state/commitment_merge_format_test.go | Validate merge version derives from inputs; reject mixed formats |
| db/state/commitment_format_resolve.go | Resolve commitment format from existing datadir files |
| db/state/commitment_format_resolve_test.go | Tests for datadir format resolution (v2/v3/mixed/fresh) |
| db/state/commitment_existence_prune_test.go | Tests for existence-filter pruning safety with v3 child keys |
| db/state/commitment_db_run_test.go | Validate db/file record-walk matches flat latest view across lifecycle |
| db/state/commitment_convert_test.go | Detect v1/v2/v3 encodings; refuse v3 conversions; add fixtures |
| db/state/aggregator2.go | Default test aggregator to legacy commitment edge-records off; resolve format at open |
| db/state/aggregator.go | Add edge-records override/apply; create BranchCache in correct mode; expose domain cfg |
| db/state/aggregator_test.go | Update commitment schema builder to ordered accessors |
| db/state/aggregator_refs_test.go | Force legacy format in reference-expansion tests |
| db/integrity/commitment_version_test.go | Update fixtures to legacy state key |
| db/integrity/commitment_version_integration_test.go | Force legacy format where required for version-regime checks |
| db/integrity/commitment_v3_history_test.go | Add v3 history integrity test exercising historical rebuild path |
| db/integrity/commitment_state_verify_test.go | Force legacy format for state-verify tests |
| db/integrity/commitment_plainvalues_firststart_integration_test.go | Force legacy format during first-start integration test |
| db/integrity/commitment_edge_records_test.go | Refuse v3 edge-record files in legacy integrity checks; key selection tests |
| db/integrity/commitment_deref_counts_test.go | Update fixtures to legacy state key |
| db/datastruct/btindex/btree_index.go | Minor comment cleanup |
| cmd/utils/app/export_preimages_cmd.go | Fallback to legacy state key when reading commitment state |
| cmd/integration/Readme.md | Document new “commitment branch” inspection command usage |
| cmd/integration/commands/commitment.go | Print v3 edge records when available; state-key handling improvements |
| cmd/integration/commands/commitment_edge_records_test.go | Add formatting tests for edge-record output |
Review details
Suppressed comments (2)
execution/commitment/warmuper.go:216
- This loop does not compile:
range nextNibbleattempts to range over an int. If the intent is to skip over all child cells beforenextNibble, use an index-based loop from 0 tonextNibble-1.
db/state/squeeze.go:268 - When squeezing legacy (non-edge-record) commitment files, the value transformer is currently applied to every key, including the commitment state blob. The state value must not be transformed (it is not a branch row), otherwise the squeezed file becomes unreadable/corrupt.
- Files reviewed: 112/112 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…-state fixture, point the KVI guard at rcache
|
Reviewed the core mechanism and everything this diff changes on the legacy path, rather than all 112 files. Build and vet are clean, and One confirmed regression, one wrong load-bearing comment, and the two blocking claims on this PR are false. 1. Squeeze no longer skips the commitment state blob on legacy files
- if !bytes.Equal(k, commitmentdb.KeyCommitmentState) {
+ if !edgeRecords {
v, err = vt(v, af.startTxNum, af.endTxNum)The two conditions are orthogonal. Edge-record files skip the transform because their records are not reference-encoded; the state blob is skipped because it is not a branch row at all. On a v2 file What So a 45-byte blob aborts the whole squeeze through Tests miss it because 2. The record-walk comment is wrong for every even-depth node
That holds for odd-depth parents only. For an even-depth parent the terminal byte is
The last two rows are the comment's own condition, confirmed. The first two are not, and they cover half of all nodes including the root. The code is fine: The comment is the problem, because it tells a future reader the slow path is a rare 1-in-16 shape when it is the common case, and both the length check and the re-seek look like dead weight under that reading. Removing either is a wrong-root bug. Worth restating as the actual condition, or dropping the condition and just saying descendants can sort between children so the run re-seeks. Also worth noting that 3. Both blocking claims are falseThe only "changes recommended" verdict here rests on two compile errors that do not exist:
Ranging over an integer has been legal since Go 1.22 and slice-to-array conversion since 1.20. This module is on Go 1.26. Nothing to chase. Draft gateThe body already says it, but it is the thing that decides when this can leave draft: |
…torRoTx, not a copy of the whole DomainCfg
|
Picking up the two suppressed comments, which have no thread to reply on.
One thing worth recording: the blob was passing through unchanged today. |
|
Not a code review — 10.7k lines across 113 files is past what a single pass can verify, and the branch is rpcdaemon does not start, on both chains. Scope. A commitment scheme change is exactly the case |
…e pass, not ten probes per child
# Conflicts: # execution/commitment/hex_patricia_hashed.go # execution/commitment/preload_test.go # execution/stagedsync/committer_step_boundary_test.go
…ers, publish them outside the pin controller
… records past the trunk depth
Commitment writes one
.kvrecord per touched trie edge instead of one bundled row per branch node carrying all 16 child cells; leaves are records too, not fields of the parent row. An edgeP->nis keyednodeKey(P) || (0x80|n), so a node's children sort contiguously and a read walks them with one cursor seek plusNext()rather than a descent per nibble. Format tables and open questions are in #23773.The write-size win is real and does not pay for the read:
Paired from-0 mainnet arms on the same main base, launched the same second on identical EPYC 4344P / 125 GB hosts: edev
v3-from0-20260916against snap-arb1cdwn-from0-20260916, 90 s CPU profile plus a 120 s counter window. Not rewriting an unchanged edge is exactly what scatters a node's ~10 edges across 4.29 files, so read depth grows with step count — v3 was 8.0% faster on hoodi to 1M and is 7.5% slower here at 4.5M. The comparison arm also carries the fork-walk, so only the per-read rows are format-only.Changes
.kvversion plumbing. A file is edge-record format iff its version is >= 3.0, so a datadir keeps what it was built with and a merge names its output from its inputs, never from the live flag.BenchmarkCommitmentFormatCost.AccessorHashMaptoAccessorBTree | AccessorExistence, because a v3 read walks siblings with an ordered cursor. An existing datadir therefore carries.kviwhere this branch wants.btand.kvei, and both RPC integration jobs fail with the daemon never opening 8545.AdaptivePinControllerwas their only publisher and it is optional, so with pinning off no tier counter reached Prometheus.Not included: history and accessor migration, so no published snapshot set can move yet; and
COMMITMENT_EDGE_RECORDShas no CLI orerigondb.tomlsetting, so every fresh datadir on this branch writes v3.