execution/execmodule: drain read-ahead only before an actual unwind - #23003
Closed
yperbasis wants to merge 1 commit into
Closed
execution/execmodule: drain read-ahead only before an actual unwind#23003yperbasis wants to merge 1 commit into
yperbasis wants to merge 1 commit into
Conversation
This was referenced Aug 7, 2026
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Aug 13, 2026
…igontech#23005) Fixes erigontech#22463. ## Summary `StateCache` stores latest committed state. Unwind already made resident dead-fork entries stale, but readers could add those values again from an old or transient view: a transaction could survive or first bind during unwind, staged unwind rows still existed in the backing database, and read-ahead could fill concurrently. This PR closes those windows by binding fill authority to both the durable `PlainStateVersion` and the lifetime of the original `ReadView`. Reads constrained by a staged unwind cannot fill, cache changes are published only after the database commit, and read-ahead cannot cross the unwind transition. Snapshot and immutable-file publication are a separate coherence boundary. erigontech#23028 still requires erigontech#23047 or an equivalent publication hook and is not addressed here. Bounded speculative-unwind fills in the separate commitment `BranchCache` pre-exist this PR and are tracked in erigontech#23253. ## Review guide Suggested order: 1. `execution/cache/view.go` and `state_cache.go`: fill admission and publication. 2. `db/state/execctx/domain_shared.go`: transaction identity, bounded reads, and commit/unwind integration. 3. `db/kv/membatchwithdb/memory_mutation.go` and `db/state/temporal_mem_batch.go`: `PlainStateVersion` ownership and monotonicity. 4. `execution/exec/blocks_read_ahead.go` and `execution/execmodule`: read-ahead exclusion and lifecycle. Focused regression tests sit beside each area. ## Correctness invariants | Marker | Protects | | --- | --- | | `PlainStateVersion` | The durable state visible to a transaction | | `readViewEpoch` | Whether a `ReadView` predates the latest unwind or state discontinuity | | Per-cache entry epoch and unwind floor | Whether a stored value belongs to the retained fork | Once the cache has a durable state version, an admission-gated state fill is accepted only if the view has the published state version and current epoch, publication is not in progress, its exact domain frontier is not behind the cache, and the read has no staged-unwind step bound. Content-addressed code-size fills do not need these state-view checks. An ineligible view may still read cache hits; only its fill authority is revoked. `WithFrontier` preserves the original epoch, so rebinding cannot renew an old view. Stored entries remain O(1) to invalidate and are discarded lazily. The three markers stay separate because durable state, reader, and stored-entry lifetimes change at different boundaries. ## Commit and unwind flow 1. Staging an unwind revokes existing views, invalidates stored entries, and records the lowest staged boundary. Bounded reads cannot fill. 2. Flush advances `PlainStateVersion` exactly once with the domain writes and collects cache updates without publishing them. 3. The database transaction commits. 4. Cache publication applies the complete batch. It repeats unwind invalidation at the durable boundary, rejects delayed or out-of-order versions, preserves entries after a continuous forward commit, and clears them when continuity is unknown. During publication, reads and view binding remain available, but fills are disabled. A view bound during publication remains fill-inert until explicitly rebound. `MemoryMutation` resolves untouched sequences from its backing transaction and flushes only changed sequence keys, so it cannot replay an older state version. Pre-commit notifications receive the projected version explicitly rather than deriving it from overlay sequence writes. Notification ordering itself is unchanged and remains tracked in erigontech#23240. One semaphore permit covers read-ahead warmup and unwind exclusion. A warmup acquires it without blocking, so work requested while another warmup or an unwind owns or waits for the permit is skipped rather than queued. Unwind callers acquire it with their context and abort before staging if cancellation wins. `updateForkChoice` and `SetHead` hold the permit through unwind and publication; `ValidateChain` acquires it only when it stages an unwind. Every FCU currently excludes warmup, including FCUs that do not unwind; narrowing that scope is tracked in erigontech#23003. ## Performance - The cache-hit path is unchanged. - `View(nil)` adds one atomic load. Binding a fill-enabled `ReadView` also takes `admissionMu.RLock` to check publication and state-version eligibility; getters retain that view instead of paying the binding cost per key. - Normal getters reuse the `SharedDomains` transaction's memoized state version. A different transaction resolves its version at initial binding. If that resolution temporarily fails, later cache misses retry it; each retry is local to that miss. - Unwind invalidation remains O(1), with no cache scan or diff replay. - Each accepted warmup performs one uncontended semaphore acquisition. Rejected warmups do not start a goroutine, and the gate is never touched per key. ## Validation Regression tests cover old and newly bound views across every unwind phase, bounded state and code-hash reads, delayed publications, memory-overlay state versions, read-ahead exclusion and cancellation, and valid forward fills. --------- Co-authored-by: Alexey Sharov <askalexsharov@gmail.com>
updateForkChoice drained in-flight warmup at the top of every FCU — duplicate and non-reorg FCUs included — waiting for the whole warmup on the tip hot path. Fill admission orders warmup fills against the flush cache-apply, so the forward direction needs no drain; only the unwind epoch bump does (a pre-unwind view passes admission once the applied frontier is lowered). Move the drain into unwindIfNeeded's unwind branch, matching unwindToCommonCanonical and SetHead, which already drain there. No new warmup can start while the FCU holds the semaphore, so the later drain covers the same window.
yperbasis
force-pushed
the
yperbasis/fcu-drain-unwind-only
branch
from
August 15, 2026 07:30
8c5dd9a to
b22d4d3
Compare
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
1 participant
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.
Stacked on #22444 (base branch:
test/statecache-delete-rpc-repro); draft until it merges. Performance change, not a fix — it depends on #22444's fill admission for its safety argument.What
updateForkChoicedrained any in-flight read-ahead warmup at the top of every FCU — duplicate and non-reorg FCUs included — blocking the tip hot path on the preceding newPayload's full warmup. This moves the drain intounwindIfNeeded's unwind branch, so only an actual reorg waits.Why it is safe
frontier >= appliedEndunder the same lock), so nothing needs to wait for it.UnwindTo/RunUnwind, matchingunwindToCommonCanonicalandSetHead, which already drain there.forkValidator.ClearWithUnwind(the deferred FCU cleanup) only clears fork-validator bookkeeping — no cache epoch bump — so no other epoch-bump site in the FCU flow needs the drain.Testing
go test ./execution/execmodule/ ./execution/exec/and-raceover the Unwind/ForkChoice/SetHead tests;make lintclean.