[r3.5] execution/stagedsync: isolate commitment computes for blocks that own no changeset#22135
Merged
AskAlexSharov merged 9 commits intoJul 2, 2026
Merged
Conversation
…hat own no changeset Bring release/3.5's parallel step-boundary commitment up to #22092: the changeset-isolation fix plus the compute-path refactor it builds on. A step checkpoint — and, under forcePerBlockCompute, a pre-window per-block compute — on the lagging calculator could record its [state] marker + branches into a later in-window block's live changeset; unwind replays them and corrupts that block's post-unwind commitment state and the SeekCommitment resume marker. Isolation is decided by ownsChangeset (genesis excluded, window starts at perBlockFrom == changesetWindowStart): a block owning no changeset computes under a nil accumulator; an in-window block keeps the hash-aware wrap. r3.5 adaptation: newCommitmentCalculator sets ModeUpdate unconditionally — release/3.5 has no commitment.ModeParallel.
…omputeMode computeWithBlockAccumulator read the block's changeset via GetChangesetByHash before taking changesetMu, so the apply loop could SavePastChangesetAccumulator and rotate the live accumulator in the gap — routing this block's [state] write into the next block's changeset, which a tip reorg then mis-reverts. Move the lookup inside the lock so cs is consistent with the compute; no new lock order (GetChangesetByHash takes only pastChangesLock). Rework TestHandleMessage_StepBoundaryRecordsIntoOwnChangesetInWindow to save the block's changeset and install a different live accumulator, so it drives the hash-routing (cs!=nil) path and pins the checkpoint into the block's own changeset rather than riding the cs==nil fallthrough. Collapse computeMode's resetFlags+advance (always equal across all call sites) into a single midBlock bool so a mid-block-vs-block-end mismatch can't be built.
…ateWrites ApplyStateWrites computed step-boundary commitment inline, but serial exec already calls CommitStepBoundary right after it (double compute at each step edge), and parallel forced skipStepBoundaryCommitment=true so the inline block never ran there. Remove the inline block — serial keeps its checkpoint via the explicit CommitStepBoundary, parallel via the calculator — and drop the now-dead skipStepBoundaryCommitment field, setter, and parallel toggle.
… doc ApplyStateWrites no longer computes step-boundary commitment, so it no longer uses this predicate; drop the callsite list and keep the divergence rationale.
AskAlexSharov
approved these changes
Jul 2, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Backports to release/3.5 a commitment-calculator correctness fix that prevents commitment checkpoint/branch writes from being recorded into the wrong block’s changeset when the calculator lags the exec loop (especially for blocks that do not own a changeset). This protects unwind/resume (SeekCommitment) correctness and avoids post-unwind commitment corruption.
Changes:
- Refactors the parallel commitment calculator’s compute paths into a shared
compute()helper and introducesownsChangeset()-driven isolation for blocks outside the changeset window. - Removes step-boundary commitment computation from
StateV3.ApplyStateWrites, relying instead on the explicit serialCommitStepBoundarycall or the parallel calculator. - Adds targeted tests to pin the “no leak to live accumulator” behavior for pre-window step-edge and per-block compute cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| execution/state/rw_v3.go | Removes in-ApplyStateWrites step-edge commitment; simplifies StateV3 by deleting the skip flag and setter, keeping step-edge commitment in CommitStepBoundary only. |
| execution/stagedsync/exec3_parallel.go | Drops the parallel executor’s SetSkipStepBoundaryCommitment toggling (no longer needed after StateV3 change). |
| execution/stagedsync/committer.go | Implements ownsChangeset() isolation and consolidates compute logic to prevent changeset pollution from lagging computes; adds isolated compute path. |
| execution/stagedsync/committer_step_boundary_test.go | Adds regression tests ensuring mid-block step checkpoints and pre-window per-block computes don’t pollute a later block’s live changeset. |
| db/state/execctx/domain_shared.go | Comment-only clarification for step-edge predicate sharing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+622
to
+629
| // asOfStorageEnumerator lists the persisted storage slots under an address via | ||
| // the calculator's stable roTx snapshot (the pre-cycle baseline the trie was | ||
| // built from), so a self-destruct deletes the whole subtree. The exec loop's | ||
| // DomainDelPrefix runs with inline TouchKey disabled in parallel mode, so this | ||
| // is the parallel path's equivalent of serial's per-slot delete touches. | ||
| type asOfStorageEnumerator struct { | ||
| reader *asOfStateReader | ||
| } |
…l-step-boundary-commitment-35
…mmitment-35' into awskii/parallel-step-boundary-commitment-35
Comment on lines
362
to
366
| // ApplyStateWrites applies account/storage/code mutations. When blockCache is | ||
| // non-nil (parallel executor), writes go to the block-level cache and only | ||
| // TouchKey is called for per-TX commitment tracking. The cache is flushed to | ||
| // SharedDomains at block boundary. When blockCache is nil (serial executor), | ||
| // writes go directly to SharedDomains via DomainPut. |
Comment on lines
+618
to
+625
| // asOfStorageEnumerator lists the persisted storage slots under an address via | ||
| // the calculator's stable roTx snapshot (the pre-cycle baseline the trie was | ||
| // built from), so a self-destruct deletes the whole subtree. The exec loop's | ||
| // DomainDelPrefix runs with inline TouchKey disabled in parallel mode, so this | ||
| // is the parallel path's equivalent of serial's per-slot delete touches. | ||
| type asOfStorageEnumerator struct { | ||
| reader *asOfStateReader | ||
| } |
The pre-window transition block never owns a changeset, so !ownsChangeset already forces isolation; the flag and its disjunct were unreachable.
Comment on lines
+617
to
+622
| // asOfStorageEnumerator lists the persisted storage slots under an address via | ||
| // the calculator's stable roTx snapshot (the pre-cycle baseline the trie was | ||
| // built from), so a self-destruct deletes the whole subtree. The exec loop's | ||
| // DomainDelPrefix runs with inline TouchKey disabled in parallel mode, so this | ||
| // is the parallel path's equivalent of serial's per-slot delete touches. | ||
| type asOfStorageEnumerator struct { |
AskAlexSharov
pushed a commit
that referenced
this pull request
Jul 2, 2026
…culator self-destruct (#22147) Follow-up to #22135, which merged before this fix landed. On release/3.5 the parallel commitment calculator zeroed only EVM-touched storage slots on self-destruct. The exec loop's `DomainDelPrefix` wipes the whole subtree, but with inline TouchKey disabled the calculator never sees those deletions, so untouched on-disk slots survived in commitment — a wrong storage/state root when a pre-existing contract self-destructs under `--experimental.parallel-commitment`. `asOfStorageEnumerator` was already present on the branch but unwired. This connects it: - add `storageEnum` to `calcState` + `deleteStorageSubtree`, called from the `SelfDestructPath` handler - enumerate and delete the full persisted storage subtree, matching main and serial's `DomainDelPrefix` - port `TestSDOfPreExistingContract_DeletesUntouchedSlots` from main Only the experimental parallel-commitment path is affected; default single-trie/serial is unchanged.
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.
Brings the changeset-isolation fix from #22092 (still open on
main) to release/3.5, which has an earlier snapshot of this feature via #22111 (branchawskii/commitment-fail-fast-35). Pulls in the fix plus the compute-path refactor it builds on.The calculator goroutine lags the exec loop that owns the changeset accumulator. On the
cs == nilfall-through a step checkpoint's[state]marker + branches were recorded into whatever accumulator was live — for a pre-window block, that's a later in-window block's. Unwind replays those diffs and corrupts that block's post-unwind commitment state and theSeekCommitmentresume marker. Reachable viacomputeStepBoundary(mid-block step edge) and, underforcePerBlockCompute(--prune.include-commitment-history), pre-window per-block computes.ownsChangeset(n)(genesis excluded, window starts atperBlockFrom == changesetWindowStart) now decides isolation: a block owning no changeset computes under a nil accumulator; an in-window block keeps the hash-aware wrap so its checkpoint records into its own changeset with the correct pre-block prevData.r3.5-specific adaptations
newCommitmentCalculatorsetscommitment.ModeUpdateunconditionally — release/3.5 has nocommitment.ModeParallel.Fixes #21992