[wip] execution/commitment: keep the account nav path across a storage propagate fold - #22496
[wip] execution/commitment: keep the account nav path across a storage propagate fold#22496AskAlexSharov wants to merge 6 commits into
Conversation
…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. This branch is based on #22141, whose base predates the #22362 revert and so already carries #22257 — only the fix itself is needed here. Not verified against mainnet block 5227811: reproducing that needs a from-genesis re-exec with --experimental.parallel-commitment on mainnet data.
The base branch regenerates the {0,1} step while src still holds those files
open. POSIX allows rename-over-open-file so Linux and macOS pass, but Windows
denies it and the test fails in both matrix legs:
renaming: rename v1.0-accounts.0-1.kv.<n>.tmp v1.0-accounts.0-1.kv:
Access is denied.
Only add the {1,2} step: publishing a newer generation is all the test needs,
and the pin assertions are unchanged.
This is the same fix main already carries via #22354 and belongs to the base
branch (#22141), not here; it is ported so this PR's own CI is reviewable.
|
CI was red on The base regenerates the Ported that one-line fix here so this PR's CI is reviewable on its own merits. It belongs to #22141, not to this PR — if #22141 rebases onto Worth noting the same staleness cuts the other way: rebasing #22141 onto |
There was a problem hiding this comment.
Pull request overview
Fixes a commitment-trie unfold navigation corruption that can occur when a storage-row propagate fold bubbles into an account leaf, causing the account cell’s keccak-derived nav path to be overwritten with storage nibbles (leading to “empty branch data read during unfold”). Also ports a Windows-specific test adjustment to avoid rename-over-open-file failures when publishing a new visible-files generation.
Changes:
- Update
cell.fillFromLowerCellto synchashedExtensiononly for cells without any plain key (i.e., cells that actually navigate via extension nibbles). - Add focused regression tests covering both the storage-fold-into-account case and the branch-cell sync case.
- Adjust
TestFilesPin_PinsSourceGenerationgeneration publishing to avoid rewriting an open file on Windows.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
execution/commitment/streaming_commitment.go |
Reorders root-related fields (no behavioral change) to match updated commentary/structure. |
execution/commitment/storage_fold_navpath_regression_test.go |
Adds regression tests ensuring storage propagate folds don’t clobber account navigation paths, while branch cells still sync nav paths. |
execution/commitment/hex_patricia_hashed.go |
Implements the core fix: conditional hashedExtension sync only when the cell has no plain key. |
db/state/aggregator_visible_from_test.go |
Modifies the test’s file-generation step to avoid Windows “Access is denied” from rename-over-open-file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…EventChannel
Cherry-pick of the fix opened as a standalone PR against main. This branch's
base predates it, and the flake is red on this PR's race leg.
ConsumeEvents pushed three events into a capacity-2 channel while Run was
already consuming, then asserted event1 was evicted. A consumer that drains
event1 before the third push leaves the queue below capacity, so event1 is
never evicted and arrives first:
--- FAIL: TestEventChannel/ConsumeEvents
expected: "event2"
actual : "event1"
Push all three before starting Run; PushEvent evicts synchronously under the
queue mutex, so the drop is deterministic. Assertions unchanged.
|
Pushed two fixes for this PR's CI, neither of which is this PR's own bug:
Opened standalone against
|
…th tests SeedRootFrom ranged sc.splits with no lock while TouchKey inserts into it under trieMu's write lock — 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. Tests: cover the guard's third arm (a keyless cell deep in storage still syncs, so the skip is keyed on the plain key rather than on depth), pin the expected nibbles instead of comparing extLen to hashedExtLen, and use length.Addr.
… churn The guard `accountAddrLen == 0 && storageAddrLen == 0` reads on its own; the why lives in the fix commit's message. Restore the original root-field order in StreamingCommitter so only the SeedRootFrom lock change remains.
Same fix as #22495, stacked on #22141 instead of
main.#22141's base predates the #22362 revert, so it already carries #22257 — only the fix itself is needed here (~87 lines vs #22495's ~600).
The bug
fillFromLowerCellsyncedhashedExtensionon every prepended extension. When a storage row (lowDepth > 64) propagates into an account leaf the cell is still the account cell, so 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 —mount[f] build: unfold: empty branch data read during unfoldat mainnet block 5227811, #22362's stated root cause.Fix: sync only when the cell has no plain key — only such a cell navigates by its extension. Red-first tests pin both halves.
Also here
A port of main's #22354 fix for
TestFilesPin_PinsSourceGeneration(WindowsAccess is deniedon rename-over-open-file). #22141's base predates it. That commit belongs to #22141 and should be dropped if it rebases onto main.Why #22141 needs one of these
#22141 turns on
ERIGON_COMMITMENT_PARALLELin CI. Its ubuntu leg is green only because its base predates the revert. Rebased onto main it inherits the revert and goes red with wrong trie roots across 11 tests.[wip]: not verified against mainnet block 5227811.
cc @awskii