execution/cache: prevent dead-fork StateCache fills across unwind - #23005
Conversation
d4844d2 to
f6c3652
Compare
Stamp cache read views with an unwind generation and reject fills from older generations. Skip direct and derived fills while the mem overlay supplies a per-key unwind bound.
Rename the unwind admission generation to readViewEpoch and document why it remains separate from per-cache entry epochs.
…unwind-readmission
There was a problem hiding this comment.
Pull request overview
This pull request hardens execution/cache against reorg/unwind edge-cases where reads from pre-unwind MVCC snapshots (or bounded “in-flight unwind” reads) could previously repopulate the shared StateCache with dead-fork values. It does so by adding a StateCache-wide read-view epoch that revokes fill authority (not read ability) from older ReadViews after an unwind, and by skipping cache fills when a read is step-bounded by the in-memory overlay.
Changes:
- Add
readViewEpochtoStateCacheand snapshot it into eachReadView; unwind advances the epoch, and admission-gated fills reject older epochs. - Preserve the original epoch when binding a frontier later (
ReadView.WithFrontier), preventing older views from becoming “current” by re-binding. - Skip read-fill (and derived addr→codeHash seeding) when the mem overlay indicates a bounded read (
maxStep != kv.NoStepBound), avoiding caching transient “dying row” results during staged unwinds.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
execution/cache/view.go |
Extends ReadView with readViewEpoch, adds WithFrontier, and threads the epoch through fill/seed paths. |
execution/cache/state_cache.go |
Introduces StateCache.readViewEpoch, gates fill admission on epoch equality, and advances the epoch on unwind. |
execution/cache/cache.go |
Updates package-level documentation to reflect read-view epoch semantics during unwinds. |
execution/cache/cache_test.go |
Adds/adjusts unit tests covering refill behavior across unwind and ensuring “ahead of apply” readers can still fill. |
db/state/execctx/domain_shared.go |
Skips fills on bounded reads; uses WithFrontier to bind a frontier without changing the original view epoch; skips derived code-hash seeding when bounded. |
db/state/execctx/statecache_readfill_test.go |
Adds tests ensuring bounded in-flight unwind reads do not populate StateCache or derived addr→codeHash mappings. |
db/state/execctx/statecache_rpc_integration_test.go |
Adds an integration test ensuring an embedded RPC view opened pre-unwind cannot refill an unwound account into StateCache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Background exec workers bind getters with a nil chainTx and open the real tx on their first task. The generation check dereferenced the placeholder eagerly (tx.ViewID()), panicking every parallel-exec worker pool reset — all EEST shards and benchmarks red. A nil tx gets the rejected frontier: the placeholder getter can never fill, and the worker replaces it before reading.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
db/state/execctx/statecache_readfill_test.go:329
- Close this cache at test cleanup.
StateCache.Closereleases reservations from the process-global cache budget; without it, this parallel test leaves those reservations active and can affect cache sizing in later tests.
sc := newSmallStateCache()
db/state/execctx/statecache_readfill_test.go:302
- Close this cache at test cleanup.
StateCache.Closereleases reservations from the process-global cache budget; without it, this parallel test leaves those reservations active and can affect cache sizing in later tests.
This issue also appears on line 329 of the same file.
sc := newSmallStateCache()
…unwind-readmission
|
Thanks for the detailed pass. Disposition on the current head (
For the smaller items:
The two earlier items you noted remain fixed: rejected Verification for the latest changes: full |
Part of erigontech#21860. Complements the lower-level cache tests in erigontech#23005 with running-node coverage for the RPC/unwind failure class in erigontech#22463. ## Why The missing safety net was the complete interaction between HTTP RPC requests and an Engine API reorg: - three tagged requests bind to old head **A**; - forkchoice unwinds to ancestor **B**; - newly bound `latest` requests observe **B** through overlay publication, durable commit, and database fallback; - the delayed **A** requests may finish from their valid MVCC views, but must not refill `StateCache` with dead-fork data. Timing sleeps cannot define these windows reliably, so the test pauses execution at synchronous lifecycle boundaries. ## Scenario ```mermaid sequenceDiagram participant CL as Mock CL participant FCU as ExecModule FCU participant OldRPC as RPC views bound to A participant Overlay as SharedDomains overlay participant DB as MDBX participant NewRPC as newly bound RPC CL->>FCU: forkchoiceUpdated(A) FCU->>Overlay: publish A OldRPC->>OldRPC: bind tagged views and pause before reading FCU->>DB: commit A FCU->>Overlay: clear A CL->>FCU: forkchoiceUpdated(B) FCU->>FCU: unwind to B Note over FCU,DB: committed A remains visible until B is published FCU->>Overlay: publish B NewRPC->>Overlay: assert B before commit FCU->>DB: commit B NewRPC->>Overlay: assert B after commit FCU->>Overlay: clear B NewRPC->>DB: assert durable B OldRPC-->>OldRPC: finish from the old A views CL->>FCU: forkchoiceUpdated(C) FCU->>Overlay: publish transaction-free C NewRPC->>Overlay: repeat canonical reads through C Note over NewRPC,Overlay: untouched keys probe StateCache for stale A refill FCU->>Overlay: clear C ``` The **B** overlay publication is the visibility switch. Before it, newly bound requests see committed **A**; from publication onward, they must consistently see **B**. The final **C** step is an explicitly transaction-free child of **B**, built through `testing_buildBlockV1` with a non-nil empty transaction list. This bypasses the txpool, so unwound transactions cannot enter **C** and its fresh overlay cannot answer the probed keys from its own writes. Repeated reads therefore exercise the shared `StateCache` and expose any stale **A** refill. ## Assertions | Boundary | What the test proves | | --- | --- | | Tagged A RPC views bound | Exactly the delayed storage, code, and nonce requests are pinned before their first read | | B unwind complete | Replacement state is still private; committed A remains visible | | B overlay published | Dead-fork storage is absent while retained account, code, and nonce remain | | B committed and overlay cleared | The same result survives commit and MDBX fallback | | Delayed A views released | Old values may return, but cannot repopulate canonical cache entries | | C transaction-free overlay cache probes | Repeated reads return B-equivalent state, proving the shared cache was not poisoned by A | Each lifecycle assertion uses a separate persistent `StateChurn` contract, so an earlier RPC cache fill cannot mask a later path. Their selected slot is absent at **B** and written only on **A**, exercising removal of dead-fork storage while preserving the account. A fifth contract exists only on **A**, exercising complete account and code removal. The generated **A** storage value is required to be non-zero, so the distinction cannot silently collapse. Non-zero value restoration remains covered by the broader StateChurn suite and erigontech#23005. ## Design and production impact - `StateTransitionObserver` exposes five inline boundaries: RPC view bound, unwind complete, overlay published, commit complete, and overlay cleared. - The delayed calls use a dedicated tagged RPC client. The RPC-view barrier accepts only matching server-side peer contexts, so unrelated cache views cannot consume its slots. - Publication, commit, and clear observations are paired with the FCU that actually published the overlay. An FCU response is not treated as proof that deferred teardown has finished. - The observer is optional and nil in production. RPC view observation is added by an `engineapitester` cache decorator, leaving the production `Cache.View` path unchanged. - The real overlay event bus remains drop-tolerant; it is not reused as a blocking test barrier. - Node shutdown drains detached forkchoice work before closing the database, preventing an in-flight transaction from blocking DB shutdown. - The test uses real HTTP `eth_getStorageAt`, `eth_getCode`, and `eth_getTransactionCount` calls and real Engine API forkchoice updates. Channels establish ordering; timeouts only bound failures. - The `testing` RPC namespace is enabled only for this `engineapitester` fixture so **C** can bypass the txpool; production RPC configuration is unchanged. - The obsolete erigontech#22299 `latest`-nonce workaround is removed; erigontech#22326 now supplies the normal pending-nonce behavior. ## Relationship and scope erigontech#23005 tests cache admission and commit/publication interleavings directly. This PR tests the surrounding HTTP RPC, Engine API, overlay, commit, and fallback wiring. It intentionally does not duplicate every internal midpoint. Coverage is a deterministic in-process full node. Crash windows, randomized reorg schedules, real-CL/Kurtosis runs, and the exact DB-commit/cache-publication midpoint remain separate erigontech#21860 layers. ## Review order 1. `execution/execmodule/state_transition.go` and `forkchoice.go`: boundary semantics and placement. 2. `node/eth/backend.go` and `execution/engineapi/engineapitester`: full-node wiring, test-only RPC/cache hooks, empty-payload construction, and shutdown draining. 3. `execution/engineapi/engine_api_rpc_unwind_test.go`: reorg scenario and cache-refill assertions. 4. `engine_api_state_churn_reorg_test.go`: shared churn helper and pending-nonce path.
Fixes #22463.
Summary
StateCachestores 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
PlainStateVersionand the lifetime of the originalReadView. 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. #23028 still requires #23047 or an equivalent publication hook and is not addressed here. Bounded speculative-unwind fills in the separate commitment
BranchCachepre-exist this PR and are tracked in #23253.Review guide
Suggested order:
execution/cache/view.goandstate_cache.go: fill admission and publication.db/state/execctx/domain_shared.go: transaction identity, bounded reads, and commit/unwind integration.db/kv/membatchwithdb/memory_mutation.goanddb/state/temporal_mem_batch.go:PlainStateVersionownership and monotonicity.execution/exec/blocks_read_ahead.goandexecution/execmodule: read-ahead exclusion and lifecycle.Focused regression tests sit beside each area.
Correctness invariants
PlainStateVersionreadViewEpochReadViewpredates the latest unwind or state discontinuityOnce 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.
WithFrontierpreserves 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
PlainStateVersionexactly once with the domain writes and collects cache updates without publishing them.During publication, reads and view binding remain available, but fills are disabled. A view bound during publication remains fill-inert until explicitly rebound.
MemoryMutationresolves 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 #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.
updateForkChoiceandSetHeadhold the permit through unwind and publication;ValidateChainacquires it only when it stages an unwind. Every FCU currently excludes warmup, including FCUs that do not unwind; narrowing that scope is tracked in #23003.Performance
View(nil)adds one atomic load. Binding a fill-enabledReadViewalso takesadmissionMu.RLockto check publication and state-version eligibility; getters retain that view instead of paying the binding cost per key.SharedDomainstransaction'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.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.