commitment: preserve account load flags across storage folds - #22604
Conversation
There was a problem hiding this comment.
Pull request overview
This PR cherry-picks a fix in the execution/commitment hashed Hex-Patricia trie folding logic to preserve account “loaded” flags when folding storage subtrees. This prevents ModeUpdate from re-loading (and potentially overwriting) BAL-provided account fields with stale state-reader values during storage collapses, and adds a regression test to lock in the behavior.
Changes:
- Preserve existing account-load flags when folding nodes in the storage portion of the hashed keyspace (
fillFromLowerCell). - Add a regression test that reproduces the “account updated + storage fold” pattern and asserts
ModeUpdatematchesModeDirect.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| execution/commitment/hex_patricia_hashed.go | Adjusts fold propagation to merge (rather than overwrite) load flags when folding storage-depth nodes. |
| execution/commitment/hex_patricia_hashed_test.go | Adds a regression test ensuring ModeUpdate preserves account data across a storage fold and matches ModeDirect root output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0b98403 to
8ae5ccd
Compare
mh0lt
left a comment
There was a problem hiding this comment.
Approve — and empirically confirmed against a real consensus failure.
This fixes a pre-existing wrong-trie-root bug on the --experimental.bal (BAL-driven parallel commitment) path that I had independently root-caused on glamsterdam-devnet-6 at block 70097 (BAL-driven root 248a318a… vs correct e4e83f65…; reproduced on both main and #22409). My instrumentation pinned the divergence to the ModeUpdate fold: fold(197 updates)→248a vs incremental(227)→e4e8 on the same computeIsolated path — and, tellingly, re-emitting the "missing" account leaves at the changeset level was a no-op, which pointed at exactly this: the corruption is the trie-compute loaded flag, not a missing update. When a storage row (lowDepth > 64) folds up into an account cell, the old cell.loaded = lowCell.loaded clobbered cellLoadAccount, so the account looked unloaded and its BAL-provided value got overwritten by a stale state-reader read → wrong root. Merging the flags (addFlag) is correct and minimal.
Validation: built main + #22495 + this PR, ran with --experimental.bal + parallel commitment against a datadir at block 70096 on glamsterdam-devnet-6. It executed cleanly through 70097 and on to head 145030 with zero wrong-trie-root / BAL-mismatch events (previously it halted at 70097). So this resolves the bug end-to-end.
Context for #22409: it inherits this on its next merge-from-main; no conflict (does not touch hex_patricia_hashed.go). LGTM.
Cherry-pick of #22603 onto `main`, without its prerequisite PR chain. Preserves account load flags when folding storage subtrees in the hashed Patricia trie, preventing BAL-provided account values from being overwritten by stale state-reader values during `ModeUpdate`. ## Validation - `go test ./execution/commitment -run '^TestModeUpdatePreservesAccountAcrossStorageFold$' -count=1` - `make lint` (two consecutive clean runs) - `make erigon integration`
Cherry-pick of #22603 onto
main, without its prerequisite PR chain.Preserves account load flags when folding storage subtrees in the hashed Patricia trie, preventing BAL-provided account values from being overwritten by stale state-reader values during
ModeUpdate.Validation
go test ./execution/commitment -run '^TestModeUpdatePreservesAccountAcrossStorageFold$' -count=1make lint(two consecutive clean runs)make erigon integration