execution, db: state-cache review follow-ups; wire frozen-block catchup into the apply stream - #23018
Closed
yperbasis wants to merge 94 commits into
Closed
execution, db: state-cache review follow-ups; wire frozen-block catchup into the apply stream#23018yperbasis wants to merge 94 commits into
yperbasis wants to merge 94 commits into
Conversation
Serialize snapshot-freshness admission with canonical cache apply so an older RPC or read-ahead snapshot cannot repopulate state after an authoritative update or physical delete. Route account, storage, code, and derived code-hash fills through the combined admission APIs. Add embedded-RPC integration coverage plus cache and read-ahead concurrency tests.
…e-rpc-repro # Conflicts: # db/state/execctx/domain_shared.go # execution/cache/cache_test.go # execution/exec/blocks_read_ahead.go # execution/exec/blocks_read_ahead_test.go
… the cache-aggregator guard The ForbidVisibilityLowering wiring was repeated at both cache wire-up sites. Centralize the cast, the fills-enabled condition and the rationale in execctx.GuardAggregatorForCache, duck-typed so the storage layer and the cache stay decoupled; future wire-ups have one named function to call instead of a pattern to copy.
With USE_STATE_CACHE=false SetStateCache is a no-op, so no SD ever wires the cache and no fill can happen — but the guard still keyed only on STATE_CACHE_FILLS and forbade visibility lowering on the aggregator for a cache that never gets wired. Gate on dbg.UseStateCache too.
…smatch; build no cache when disabled The centralized guard silently returned when the DB could not produce its aggregator — a TemporalRwDB wrapper hiding Agg (memory_mutation-style) would silently drop the load-bearing invariant the concrete casts used to enforce loudly. For a fill-enabled cache the shape mismatch now panics, naming the type; a nil or apply-only cache still needs no guard. USE_STATE_CACHE=false now constructs no cache at all instead of building one that SharedDomains never wires: read-ahead previously kept filling the unused cache, wasting allocation and falsifying the no-fill rationale. With construction gated, the guard no longer consults the global flag. Also reconcile the SetStateCache doc with the code (population is post-commit via Commit, the invariant is guard-enforced) and drop an incident anecdote per the comment policy.
… harden the visibility guard USE_STATE_CACHE=false was not allocation-free end to end: backend.go could build a budget-sized cache that NewExecModule then discarded, leaking its memory-envelope reservation, and ExecModuleTester built a default cache regardless of the flag. Callers now pass a byte budget instead of a constructed cache; newDomainStateCache in the module is the single construction site (none when disabled, pinned by test), and ExecModule.Close releases the reservation for per-fixture harnesses. Guard hardening per review: cover the second panic branch (aggregator without ForbidVisibilityLowering) in the guard test, evaluate Agg() once, and take dirtyFilesLock in ForbidVisibilityLowering so 'from then on' holds against a recalcVisibleFiles already in flight. Also trim the SetStateCache doc to what the method manages (it does not wire read-ahead).
Ethereum.Stop never released the domain state cache's memory-envelope reservation, so per-fixture backends (EngineApiTester) accumulated reservations across a test binary. Close the module after chainDB.Close, mirroring ExecModuleTester's teardown order. Also update the NewDefaultStateCache doc: harnesses now set a budget, they no longer pass a constructed cache.
… symmetry apply() checks the immutable caches array before taking the write lock, and the fill paths clone the value before taking the read lock — a rejected fill wastes one copy (rare), but Apply never waits on a fill's memcpy of up-to-24KB code. Aggregator.Close clears the visibility- lowering flag under dirtyFilesLock, matching the setter. The early SharedDomains.Close in the RPC resurrection tests now says it is deliberate (the view outlives the overlay teardown, as across a background commit), so it does not read as a use-after-close. Also fix import grouping in exec_module.go.
…ommit-apply wording TestEngineApiNodeCloseReleasesCacheBudget drives the real EngineApiTester → node.Close → Ethereum.Stop path and asserts cachebudget.Global returns to its pre-construction level (red with the Stop-time ExecModule.Close removed, green with it). Replace the stale flush-apply vocabulary in package docs, comments, the fills-disabled log line and test text with commit/unwind and post-commit apply — applies happen after tx.Commit succeeds, never at Flush. Trim the ExecModule.Close doc to the invariant.
…y-II end DomainVisibleEnd reported the history-II visible end, but a dependency checker can clamp the values view below it — reads in that gap fall back to older file values, so the frontier overstated what the view serves and a stale fill could pass admission. Clamp to the values end when the two diverge. The forbid-lowering assert watched only the domain-values ends, a different quantity than DomainVisibleEnd derives frontiers from; a history-II end could lower without tripping it. Add the dhii arm. Both pinned red first via a dependency-clamped visible bundle. Also narrow the pending-stash comment: the durable-MDBX guarantee covers applies, not reads that fill between flush and a failed (fatal) commit.
Batched applies: Commit routes its pending state updates through Applier.ApplyAll, which takes the admission write lock once per 4096- update chunk instead of once per key — main's pending walk had no global lock at all, so per-key locking was a regression, and chunking bounds how long concurrent RPC fills wait during a big batch apply. Code values are still cloned and hashed outside the lock. ~815 -> ~714 ns/update uncontended; the contention win is the point. Flush now returns an error on a cache-attached SD instead of a doc comment asking callers to route through Commit; the memo test moved to Commit's validate window, which pins the fresher-frontier behavior where it actually matters. kv.TemporalRwDB carries Agg() any, so GuardAggregatorForCache takes the typed DB and a shape that cannot produce its aggregator no longer compiles (membatchwithdb's temporaldb returns nil and fails the guard loudly). The aggregator half stays duck-typed: execctx cannot import db/state. Frontier lookups tolerate a tx whose Debug() is nil (MemoryMutation over a nil db): no exact frontier, no fill, reads unaffected. Fill admission outcomes are counted and reported by PrintStatsAndReset to measure how much reader warming survives a real sync's commit cadence (parallel-exec workers hold one RO tx per run, so their fills are expected to stop after the first mid-run commit). Also state the per-domain admission invariant at appliedEnd and trim view.go rationale that duplicates the PR description.
The test removed a still-mapped .ef file from disk, which Windows forbids — both windows CI shards failed on ReloadFiles' remove. CloseIf deletes the dirty item and closes its mmaps, exercising the same recalcVisibleFiles chokepoint on every platform. Red-on-revert of the history-II assert arm re-verified with the new trigger.
…ache-review-followups
Reporting the values end kept fills flowing from a view that is not consistent as of any txNum: DB-resident keys read fresh while gap keys read older file values, and raising the dependent file's visibility later reveals state without any cache apply — nothing would ever invalidate a fill (or a negative entry) made during the clamp, so a cold cache could serve stale data until the key's next write. DomainVisibleEnd now returns ok=false while clamped: reads work, fills are skipped. Red-first via the flipped test expectation.
Reporting the values end kept fills flowing from a view that is not consistent as of any txNum: DB-resident keys read fresh while gap keys read older file values, and raising the dependent file's visibility later reveals state without any cache apply — nothing would ever invalidate a fill (or a negative entry) made during the clamp, so a cold cache could serve stale data until the key's next write. DomainVisibleEnd now returns ok=false while clamped: reads work, fills are skipped. Red-first via the flipped test expectation.
…ache-review-followups
End-to-end shape of the review reproduction: commit v1 (the cache holds it), write v2 through another cache-attached SD — a plain Flush plus tx.Commit would leave the cache serving v1 while MDBX holds v2. The test pins the rejection at exactly that step and that routing through Commit keeps the cache coherent. Fails without the Flush guard (verified by reverting it).
…or at the cache, assert at wiring Combine the guard with the cache per review: GuardAggregatorForCache becomes StateCache.BindAggregator, and SetStateCache asserts the binding before wiring a fill-enabled cache — a future call site cannot forget the load-bearing guard. Integration wiring reordered to bind before wiring. Apply-only mode (STATE_CACHE_FILLS=false) no longer binds a frontier on the plain miss path just for Fill to no-op — one allocation per cold miss saved, pinned by an AllocsPerRun test. CanFill now means what it says: fills can go through this view (frontier present and fills enabled).
…te cache ProcessFrozenBlocks advanced durable state through SharedDomains with no attached cache — the one writer outside the apply stream. Engine endpoints are live before Start, so pre-catchup reads could populate the cache and nothing ever overwrote or fenced those entries; a read view opened before the boundary could refill stale state even across a clear. Both SD creation sites now go through newFrozenBlocksSD, which attaches the module's state cache and code store: catchup commits apply to the cache post-commit and advance the admission frontier, so pre-catchup fills are rejected by the ordinary gate — admission is the fence. The test pins both halves: applies overwrite a pre-seeded stale entry, and a pre-catchup view's refill is rejected. Closes #22925.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR follows up on the state-cache admission work by (a) ensuring frozen-block startup processing participates in the same cache apply/admission stream, (b) batching authoritative cache applies to reduce lock contention during large commits, and (c) strengthening the “visibility lowering” guard wiring via a typed DB hook plus runtime assertions and counters.
Changes:
- Wire
ProcessFrozenBlocks’SharedDomainscreation through a helper that attaches the module’sStateCacheandCodeStore, preventing pre-catchup stale refills. - Add
Applier.ApplyAll([]Update)with chunked locking, plus fill admission counters reported inPrintStatsAndReset. - Add
TemporalRwDB.Agg() anyand move the aggregator-guard binding toStateCache.BindAggregator, asserted atSharedDomains.SetStateCache; rejectFlushon cache-attachedSharedDomains.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| execution/execmodule/executor.go | Adds newFrozenBlocksSD and threads caches into frozen-block startup processing. |
| execution/execmodule/exec_module.go | Switches to StateCache.BindAggregator(db) and passes caches into ProcessFrozenBlocks. |
| execution/execmodule/exec_module_internal_test.go | Adds coverage for frozen-block startup wiring and cache admission behavior. |
| execution/cache/view.go | Refines CanFill semantics and introduces Update + Applier.ApplyAll. |
| execution/cache/state_cache.go | Implements chunked applyAll, fill admission counters, and BindAggregator + wiring assert support. |
| execution/cache/apply_all_test.go | New tests/benchmark for ApplyAll, chunk boundaries, and admission counters. |
| db/state/execctx/statecache_readfill_test.go | Moves/extends tests around flush-vs-commit, binding enforcement, nil-debug frontier behavior, and apply-only miss-path allocations. |
| db/state/execctx/domain_shared.go | Tolerates nil Debug() for frontier lookup, asserts bound aggregator in SetStateCache, rejects Flush with a state cache, and switches commit to ApplyAll. |
| db/kv/membatchwithdb/memory_mutation.go | Implements Agg() any for temporaldb (returns nil). |
| db/kv/kv_interface.go | Extends TemporalRwDB interface with Agg() any. |
| cmd/integration/commands/stages.go | Binds aggregator before wiring a fill-enabled state cache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+57
to
+60
| func TestFrozenBlocksSDWiredToStateCache(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ctx := t.Context() |
Comment on lines
+661
to
+663
| // Agg returns the DB's state-files aggregator as `any` (the concrete type | ||
| // lives above the kv layer); nil when the DB has none. | ||
| Agg() any |
…e branch stash cacheUpdate served both cache and branch tuples before ApplyAll split them; only the commitment branch remains, so the domain field was dead. Also note ApplyAll's slice ownership in its doc.
yperbasis
changed the title
# Conflicts: # db/state/execctx/domain_shared.go # db/state/execctx/statecache_readfill_test.go # execution/cache/state_cache.go # execution/cache/view.go # execution/execmodule/exec_module.go # execution/execmodule/exec_module_internal_test.go
Member
Author
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.
Closes #22925.
Follow-ups to #22444 (now merged; GitHub retargeted this PR to
mainautomatically). Together with two fixes folded into #22444 itself, this addresses Alex's second review point by point — and closes #22925, whose fix is a natural extension of the same machinery.Closes #22925 — frozen-block startup processing joins the apply stream
ProcessFrozenBlocksadvanced durable state through SharedDomains with no attached cache — the one writer outside the apply stream. Engine endpoints are live beforeStart, so pre-catchup reads could populate the cache, and nothing ever overwrote those entries; as the issue notes, a clear alone cannot fix it (a read view opened before the boundary refills stale state afterward). Both SD creation sites now go throughnewFrozenBlocksSD, which attaches the module's state cache and code store: catchup commits apply post-commit and advance the admission frontier, so pre-catchup fills are rejected by the ordinary gate — admission is the fence; no clear, no drain, no snapshot-generation guard needed. The test pins both halves (applies overwrite a pre-seeded stale entry; a pre-catchup view's refill is rejected). This wiring routes the system's biggest commit batches through the apply path, which is what the chunkedApplyAllbelow is sized for.In #22444 directly
DomainVisibleEndnow clamps to the domain-values end when a dependency checker leaves it below the history-II end (a frontier must never overstate what the values view serves — the overstatement could admit a stale fill), and the forbid-lowering assert gained a history-II arm so the quantity frontiers derive from is itself guarded. Both pinned red first via a dependency-clamped visible bundle.In this PR
Applier.ApplyAlltakes the admission write lock once per 4096-update chunk instead of once per key;Commit's pending walk uses it. Chunking bounds how long concurrent RPC fills wait during a big batch apply (one lock over a whole million-key batch would stall every in-flight fill for the full walk). Code values are still cloned and hashed outside the lock, and one-lock-per-chunk is strictly stronger than per-key, so the ordering argument is unchanged. Equivalence and chunk-boundary tests;BenchmarkApplierApply: ~815 → ~714 ns/update uncontended — the contention win against readerRLocks is the real target.Flushreturns an error on a cache-attached SD. The memo test moved intoCommit's validate window, which pins the fresher-frontier behavior exactly where it matters (reads between the internal flush and the commit). A second test pins the incoherence the rejection prevents end to end: commit v1, flush v2 through another cache-attached SD — the cache would serve v1 while MDBX holds v2; the rejection fires at exactly that step, and Commit keeps the two coherent.kv.TemporalRwDBcarriesAgg() any, and the guard becameStateCache.BindAggregator(db): a DB shape that cannot produce its aggregator no longer compiles (membatchwithdb'stemporaldbreturns nil and fails loudly at runtime). The aggregator half stays duck-typed — the concrete type lives indb/state, above the cache package. On "the real wiring point isSetStateCache": it now asserts the binding before wiring a fill-enabled cache, so no future call site can forget the guard — no DB handle needed at the wiring point, just a bound marker on the cache.Debug(). Frontier lookups tolerate a tx without a debug backend: no exact frontier, no fill, reads unaffected. (An SD cannot even be constructed over such a tx —NewSharedDomainsneedsDebug()— so the guarded surface is the per-read tx handle.)PrintStatsAndReset. Structural answer to the question: background workers hold one RO tx perRun, so their fills are expected to stop after the first mid-run commit — rejection is the safe direction, and the counters measure what that costs on a real sync. (The scarier variant is excluded: read-only txs use their own tx-local frontier memo, never the shared SD memo, so workers with different-aged txs cannot contaminate each other's frontiers.)STATE_CACHE_FILLS=falsethe plain miss path bound a frontier only forFillto no-op (~162 ns, one allocation vs the ~101 ns no-cache baseline). The fill block is now gated onFillsEnabled, pinned by anAllocsPerRuntest, andCanFillmeans what it says (frontier present and fills enabled).appliedEnd(a global frontier would starve every quiet domain's fills);view.gorationale that duplicated the execution, db: bind StateCache fills to transaction views and reject stale fills #22444 description is trimmed; theFlushdoc shrank to match its now-enforced contract.Verified with
go test(and-raceonexecution/cache,db/state/execctx) across cache, execctx, temporal, membatchwithdb, db/state, execmodule, exec, stagedsync;make lintclean.