execution/commitment: fix parallel commitment wrong trie root (re-land #22257 + storage-fold nav path fix) - #22495
Merged
Merged
Conversation
Restores the trie-state round-trip fixes reverted by #22362, which left parallel commitment unable to compute a correct root: with COMMITMENT_PARALLEL=true main fails 11 tests across execution/tests, execution/verify, execution/execmodule, execution/state/genesiswrite and rpc/jsonrpc with a wrong trie root at block 1-2. The corruption that motivated the revert is fixed in the follow-up commit; this one is a plain revert of #22362 so the two are reviewable apart. The #22361 short-mode guard in db/test/domain_shared_bench_test.go, which #22362 carried in alongside the revert, is deliberately left in place — it is unrelated to the trie work and still needed. Reverts #22362.
…agate fold fillFromLowerCell's only caller, foldPropagate, resets upCell.extLen but not upCell.hashedExtLen. When a storage row (lowDepth > 64) propagates into an account leaf, the opening guard `lowCell.accountAddrLen > 0 || lowDepth < 64` is skipped, so the cell stays the account cell; the prepend branch still fires via `lowCell.storageAddrLen == 0 && lowDepth > 64` and the sync then overwrote that cell's keccak-derived account navigation path with an extension in storage nibble space. The next unfold followed the bogus path to a prefix owning no branch record and failed with "empty branch data read during unfold" — the mount[f] failure at mainnet block 5227811 that #22362 reverted #22257 for. Sync only when the cell carries no plain key: such a cell navigates by its extension, so the nibbles are also its unfold path. A cell with a plain key navigates by the derived path and must keep it. The other suspect named by #22362, the SetState leaf-root derive, only fires when the root itself is a leaf, so it is a no-op on a trie with many accounts and is left as #22257 wrote it. Not verified against mainnet block 5227811: reproducing that needs a from-genesis re-exec with --experimental.parallel-commitment on mainnet data.
…trip tests The re-landed test file predates #22449, which enabled the rangeint analyzer, so its counted loops fail lint against main's config. Convert them to range over int.
Contributor
There was a problem hiding this comment.
Pull request overview
Re-lands prior trie state round-trip fixes needed for parallel commitment correctness, and patches a storage-fold navigation-path corruption that could lead to “empty branch data read during unfold” (notably affecting parallel commitment roots at very early blocks).
Changes:
- Add
SeedRootFrom+ root seeding inStreamingCommitterso restarted/restored templates correctly seed the next base when the on-disk root has no branch record. - Fix
fillFromLowerCellto only synchashedExtensionwhen the cell has no plain key, preventing storage-extension nibbles from overwriting account navigation paths. - Add regression tests for restart parity / propagate-folded roots and for the storage-fold nav-path bug; refactor testkit to carry state blobs between batches.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| execution/commitment/streaming_commitment.go | Adds root seeding (SeedRootFrom) and uses it when building fresh bases; refactors root unfolding via unfoldRootWall. |
| execution/commitment/storage_fold_navpath_regression_test.go | New regression tests ensuring storage-fold propagation doesn’t corrupt account navigation paths, while account-branch cells still sync nav paths. |
| execution/commitment/state_roundtrip_regression_test.go | New restart-lifecycle parity tests across sequential/parallel/streaming engines, including propagate-root and seed-change scenarios. |
| execution/commitment/parallel_testkit_test.go | Updates testkit to carry EncodeCurrentState/SetState blobs across batches and to seed streaming bases via SeedRootFrom. |
| execution/commitment/parallel_patricia_hashed.go | Seeds streaming committer base from the template root each Process to keep SetState restore target and committer in sync. |
| execution/commitment/parallel_mount.go | Introduces unfoldRootWall helper and uses it to avoid over-unfolding restored root extensions. |
| execution/commitment/hex_patricia_hashed.go | Fixes fillFromLowerCell nav-path sync condition; hardens needUnfolding, sets rootPresent on propagate folds, and re-derives leaf-root nav paths in SetState. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… splits TouchKey inserts into sc.splits under trieMu's write lock. SeedRootFrom ranged over the same map with no lock, so a touch concurrent with a reseed hits the runtime's "concurrent map iteration and map write" fatal error. Take trieMu.RLock for the iteration: it excludes TouchKey's write lock, and the trieMu -> s.mu order matches the existing scheduler path.
Cover the guard's third arm: a keyless cell deep in storage still navigates by its extension and must sync, so the skip is keyed on the plain key rather than on depth. Without it, tightening the guard to lowDepth < 64 passes. Also pin the expected nibbles instead of comparing extLen to hashedExtLen, and use length.Addr rather than a bare 20.
… churn The guard `accountAddrLen == 0 && storageAddrLen == 0` reads on its own; the why lives in the fix commit's message. Append rootSeeded to the existing root-field block instead of reordering it, so the struct diff is a pure add.
Collaborator
Author
CI:
|
mh0lt
approved these changes
Jul 19, 2026
AskAlexSharov
changed the title
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
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.
Parallel commitment computes a wrong trie root at block 1-2 on main. With
ERIGON_COMMITMENT_PARALLEL=true, 11 tests fail (execution/testsx7,execution/verify,execution/execmodule,execution/state/genesiswrite,rpc/jsonrpc).Bisected to #22362, which reverted #22257. Parallel commitment needs #22257; main lost it.
#22257 was reverted because it wedged mainnet block 5227811 (
mount[f] build: unfold: empty branch data read during unfold). That bug is fixed here.The bug
fillFromLowerCellsyncedhashedExtensionon every prepended extension. When a storage row (lowDepth > 64) propagates into an account leaf, this guard is skipped:so the cell is still the account cell, and the sync overwrote its keccak-derived account nav path with storage nibbles. The next unfold followed the bogus path to a prefix owning no branch record.
It fires on main today, in an existing test:
Fix: sync only when the cell has no plain key — only such a cell navigates by its extension.
Commits
rangeint).Status
Full suite green in both modes — only
cmd/utils/appx2, which fail identically on plain main.[wip]: not verified against mainnet block 5227811. That needs the from-genesis re-exec A/B that produced #22362. The mechanism #22362 named is shown real and fixed; the link to that block is inference.
cc @awskii