execution, db: keep StateCache coherent through startup catchup; batch applies - #23033
execution, db: keep StateCache coherent through startup catchup; batch applies#23033yperbasis wants to merge 17 commits into
Conversation
…up into the apply stream Follow-ups to #22444, addressing the remaining post-approval review points, plus the fix for #22925 built on the same machinery. - Commit routes pending state updates through Applier.ApplyAll: the admission write lock is taken once per 4096-update chunk instead of once per key (main's walk had no global lock, so per-key locking was a regression; chunking bounds how long concurrent fills wait). - Flush returns an error on a cache-attached SD: a plain Flush would leave the cache serving pre-flush values forever. The memo test moved into Commit's validate window; an end-to-end test pins the incoherence the rejection prevents. - kv.TemporalRwDB carries Agg() any and the visibility guard became StateCache.BindAggregator; SetStateCache asserts the binding, so no wiring site can forget the load-bearing guard. - Frontier lookups tolerate a tx whose Debug() is nil: no exact frontier, no fill, reads unaffected. - Fill admission outcomes (admitted/rejected) are counted and reported by PrintStatsAndReset. - Apply-only mode (STATE_CACHE_FILLS=false) no longer binds a frontier on the miss path just for Fill to no-op; CanFill means what it says. - ProcessFrozenBlocks' SharedDomains are wired to the state cache: catchup commits apply post-commit and advance the admission frontier, so pre-catchup read views cannot refill stale state — admission is the fence. Closes #22925. - Per-domain admission invariant stated at appliedEnd; duplicated rationale trimmed from view.go; dead domain field dropped from the branch stash.
yperbasis
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request follows up on the state-cache admission work by ensuring frozen-block startup processing participates in the same authoritative apply stream as regular execution, and by reducing lock contention during large cache apply batches. It also tightens correctness constraints around cache wiring (aggregator binding) and flush/commit behavior, with additional tests to pin the intended invariants.
Changes:
- Wire
ProcessFrozenBlocksthrough aSharedDomainsconstructor that attaches the moduleStateCache/CodeStore, so startup catchup commits update cache state and advance the admission frontier. - Add
StateCache.Applier().ApplyAllwith chunked locking to bound fill starvation during large apply batches; updateSharedDomains.Committo use it. - Enforce
FlushvsCommitsemantics for cache-attachedSharedDomains, add aggregator binding plumbing (TemporalRwDB.Agg()+StateCache.BindAggregator), and expand tests/benchmarks.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
execution/execmodule/executor.go |
Adds newFrozenBlocksSD and routes frozen-block processing through cache-attached SharedDomains. |
execution/execmodule/exec_module.go |
Binds aggregator via StateCache.BindAggregator and passes cache/store into frozen-block processing. |
execution/execmodule/exec_module_internal_test.go |
Adds a regression test asserting frozen-block SDs apply into the cache and reject pre-catchup stale refills. |
execution/cache/view.go |
Introduces Applier.ApplyAll and refines ReadView.CanFill semantics. |
execution/cache/state_cache.go |
Implements chunked applyAll, adds fill admission counters, and adds BindAggregator / bound-marker state. |
execution/cache/apply_all_test.go |
New tests for ApplyAll equivalence, chunk-boundary behavior, admission counters, plus benchmark coverage. |
db/state/execctx/statecache_readfill_test.go |
Extends tests for flush rejection, binding enforcement, nil-debug behavior, and apply-only miss-path allocations. |
db/state/execctx/domain_shared.go |
Rejects Flush on cache-attached SDs, batches cache updates via ApplyAll, gates fill binding by FillsEnabled, and asserts aggregator binding at wiring. |
db/kv/membatchwithdb/memory_mutation.go |
Implements the new TemporalRwDB.Agg() method (returns nil). |
db/kv/kv_interface.go |
Extends TemporalRwDB with Agg() any. |
cmd/integration/commands/stages.go |
Ensures aggregator binding happens before wiring a fill-enabled cache into SharedDomains. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A DB whose Agg() returns nil (membatchwithdb's temporaldb) panicked with the type-mismatch message, reading 'aggregator <nil> lacks ForbidVisibilityLowering'. Same failure, clearer diagnosis.
AskAlexSharov
left a comment
AskAlexSharov
left a comment
There was a problem hiding this comment.
Reviewed the full diff plus the surrounding call graph: every SharedDomains.Flush caller, the TemporalRwDB implementers, all CanFill/FillsEnabled sites, and the DomainVisibleEnd backends.
The three load-bearing arguments hold up:
ApplyAllonly ever widens critical sections relative to per-keyApply, so the set of states a concurrent fill can observe is a subset ofmain's. Chunking is safe for the same reason.- No production path calls
Flushon a cache-attached SD. The only non-test callers are the integration unwind path,t8ntool, and the statetest harness, all cache-less. - Admission really does fence pre-catchup views:
DomainVisibleEndresolves throughiit.visibleEnd(tx), so it reflects the tx's committed II rows and not just the file frontier. A read tx opened after catchup sees the advance; one opened before does not, and its fills are rejected.
Four things worth a look.
1. The new counters sit on the same cache line as appliedEnd
StateCache lays out as caches [6]Cache (0..96) → admissionMu (96..120) → appliedEnd [6]uint64 (120..168) → fillsAdmitted (168) → fillsRejected (176). The 128..192 line therefore holds appliedEnd[StorageDomain], appliedEnd[CodeDomain] and both counters.
Before this PR a fill read that line and never wrote it. Now every fill — admitted or rejected — does an atomic RMW on it, invalidating it for every other worker about to read appliedEnd under RLock. That is a third contended atomic on the fill path (RLock/RUnlock already own the admissionMu line), and it lands on exactly the workload the PR is optimising for. Given the headline is "the contention win is the point," it seems a shame to hand part of it back to telemetry. Padding the counters onto their own line, or hanging them off a separate allocation, costs nothing.
2. ApplyAll's in-place-rewrite contract is avoidable
The "the updates slice is consumed and may be rewritten in place" clause exists solely because of u.Val = bytes.Clone(u.Val) for code. Carrying the clone in a codeVals[i] alongside codeHashes[i] removes the contract entirely, and an exported method that rewrites its caller's slice is worth removing on principle.
Also: on the only production caller the clone is redundant. Commit already deep-copies every callback value into pendingState, so each code value is copied twice per commit.
3. ProcessFrozenBlocks now buffers the batch's write set a second time
Question, not an objection. Attaching the caches also attaches the code store, so during initial sync Commit now accumulates
pendingState— a cloned key and value per flushed Accounts/Storage/Code tuple,codeStoreWrites— another cloned value plus a Keccak per code write,
and holds both across tx.Commit(). sd.mem is bounded by the exec batch size, so at the moment MDBX is at its own peak the process now carries roughly a second copy of the batch's state write set. On main, PFB stashed commitment branches only; FCU batches are one block, so this pressure is specific to the initial-sync path and new. Did initial sync get an RSS check with this on? If it turns out to matter, applying the stash in the same chunks it is collected in — rather than one slab — caps it.
Separately: wiring the code store into PFB goes beyond "join the apply stream." It adds an MDBX write per contract deployed over the whole chain. Probably what you want (a warm code store after initial sync), but it isn't in the description and it is the part of this change with the largest operational footprint.
4. require.Zero(t, allocs) is broader than the claim it pins
TestApplyOnlyMissPathBindsNoFrontier asserts zero allocations across the entire GetLatest miss path, not just the frontier boxing it is about. Any future allocation anywhere in the read path fails it with "an apply-only cache must not bind a frontier on the miss path", which sends the next person down the wrong trail. Measuring the same read with fills enabled and asserting the difference is one alloc pins exactly the stated behaviour and nothing else.
Minor
Flushrejects a wiring mistake with anerrorwhileSetStateCachepanics on the neighbouring wiring mistake. Both are programmer errors, but the error can be swallowed by adeferor a//nolint:errcheckand the panic cannot. Worth being consistent one way or the other.- The counters miss every fill that dies before the admission check.
ok == falsefrom a frontier — a dependency-clamped values view is exactly that case — is a silent drop, soadmitted + rejectedis less than fills attempted. If the number is meant to answer "how much reader warming survives," a third "no frontier" bucket makes it complete and much harder to misread. AggregatorBound()has no nil-receiver guard whileBindAggregatordoes. The single call site checks!= nilfirst, so it is only an asymmetry.
Everything else reads clean: promoting Agg() any onto TemporalRwDB buys a compile-time check in place of a duck-typed panic; the Debug() == nil tolerance matches MemoryMutation's untyped-nil return; and the Frontier doc trim loses nothing, since the dependency-clamp rationale still lives at its canonical place in AggregatorRoTx.DomainVisibleEnd.
yperbasis
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
execution/cache/state_cache.go:295
- In fillCodeIfFresh, codeHash is derived from the original
valueslice before cloning it. This differs from the Apply/ApplyAll paths (which clone before hashing) and can produce a codeHash that does not match the stored bytes ifvalueis mutated/reused while the call is in-flight (even briefly), breaking codeHash→code coherence in CodeCache.
codeHash := crypto.Keccak256(value)
cloned := bytes.Clone(value)
c.admissionMu.RLock()
…to the admission fence Downloaded state files publish through agg.OpenFolder without a single Apply: appliedEnd does not move, and even if it did, admission only rejects future fills — it cannot evict entries already inside. A cache entry filled before startup catchup and untouched by later execution served stale state indefinitely, and a plain clear is not enough because a read view opened before publication refills the cleared slot past a cold gate. Applier.AbsorbFilesExtension does both halves under one admission lock: advance appliedEnd to the new file visibility and drop every entry. ProcessFrozenBlocks calls it right after RunSnapshots, covering the execution loop, the onlySnapDownload return and the IsDomainAheadOfBlocks early return. File publication that stays within applied ranges (local segment building) is a strict no-op, so the every-merge path never churns the cache.
Attaching the CodeStore to frozen-block catchup commits writes every deployed contract's code into TblCodeCache, and Evict is the only cap enforcement — previously it ran only on the FCU prune path, which a node does not reach until catchup completes, so a full-chain catchup could grow the table far past its byte cap. Mirror the forkchoice prune callback's eviction in the catch-up PruneFn. No new test: the eviction mechanics, including the restart re-seeding of the byte counter that a long catchup exercises, are pinned by TestCodeStore_TwoTierAndEvict; the call site mirrors the proven forkchoice pattern, and pinning it directly would need an injectable table cap plus a full pipeline harness.
seedAddrCodeHash runs the same accounts-frontier admission check as the fill functions but reported nothing, so code-heavy workloads could reject seeds in volume while the stats showed zero rejections — the exact silent signal the counters exist to reveal. Count both outcomes where the decision is made; FillCodeSize stays uncounted since it is content-addressed and makes no admission decision.
Every fill RMWs a counter, and the counters landed on the cache line holding appliedEnd[Storage] and appliedEnd[Code] — turning a line that concurrent fills only read into one that ping-pongs between cores, handing part of the batched-apply contention win back to telemetry. Group the fields the fill path reads (appliedEnd, disableFills, aggBound) ahead of a 64-byte pad and put the write-hot counters behind it. A layout test pins the separation with unsafe.Offsetof so a field reorder cannot silently reintroduce the coupling.
require.Zero pinned the whole read path's allocation count to the frontier-boxing claim: any future allocation anywhere in the miss path would fail with a misleading message. Measure the same negative read with and without an apply-only cache and assert the difference is zero — only the cache attachment itself can fail it. Still red without the FillsEnabled gate (verified by reverting it).
…ary test The test assumed capacity for all 4099 entries, but the LRU grows into the process-global cachebudget envelope: under pressure (CI memory, race-detector inflation, parallel tests holding reservations) Reserve is denied, the cache stays near its start size and the oldest entries are legitimately evicted — index 0 failed across CI shards while the chunking was correct. Assert the seam-spanning tail indices, which are inserted last and survive any plausible capacity.
The code-value clone landed in the caller's Update via u.Val = bytes.Clone(u.Val), forcing a 'may be rewritten in place' contract on an exported method. Carry the clone in a codeVals slice parallel to codeHashes instead; the caller's slice is never written, and the contract clause is gone. The clone stays even though Commit already deep-copies (code values are copied twice on that path): dropping it would trade a copy of rare, small data for an aliasing obligation on every ApplyAll caller. Pinned by pointer identity — require.Same on unsafe.SliceData, since require.Equal dereferences and passes on equal pointees.
…ouring assert Flush answered one wiring bug with an error while SetStateCache answers the adjacent one with a panic. Both are programmer errors that silently corrupt cache coherence if allowed to proceed, and the error variant can be swallowed by errcheck suppression or a log-and-continue — converting a loud first-CI-run failure back into silent stale reads. Escalate the wiring branch to a panic; Flush keeps its error return for the real flushMem error paths.
admitted+rejected read as all fill attempts but undercounted: an attempt dies before the admission compare when the frontier answers ok=false (remote, history-disabled, dependency-clamped views), so the stats could show healthy admission while fills died wholesale one step earlier. A third bucket counts those at the three early returns; attempts that never happen (fills disabled, no frontier bound) stay uncounted by design. The counter sits behind the telemetry pad, so no new false sharing.
|
All taken, each as its own commit:
On the buffering question: no RSS check yet, honestly. The peak is inherent — the stash must outlive |
yperbasis
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
execution/cache/state_cache.go:536
- PrintStatsAndReset now logs fill-admission counters at Info level. PrintCacheStats is called from execution loops, so this can produce a high-volume Info log stream during sync; other cache stats in this package use Debug logging (e.g., GenericCache.PrintStatsAndReset, CodeCache.PrintStatsAndReset). Consider using Debug here as well to avoid noisy default logs.
admitted, rejected, noFrontier := c.fillsAdmitted.Swap(0), c.fillsRejected.Swap(0), c.fillsNoFrontier.Swap(0)
if admitted+rejected+noFrontier > 0 {
log.Info("[cache] fill admission", "admitted", admitted, "rejected", rejected, "noFrontier", noFrontier)
}
The watermark test was written with the boundary-hook commit but never staged (git add -u skips new files); it sat untracked, breaking compilation on sibling branches.
…test" This reverts commit 5991d05.
…iven absorb loop The cache-line test derived its counter span from fillsRejected, so fillsNoFrontier — added one commit later — sat outside the asserted range; dormant while the counters trail the struct, but a reorder could overlap it with the hot line unnoticed. absorbFilesExtension now iterates every cached domain instead of a hardcoded trio: a future fill-capable domain missing from the list would silently not be fenced at publication.
Clone code bytes before hashing in fillCodeIfFresh, matching the apply paths, so stored bytes and codeHash cannot diverge under a reused caller buffer. Tolerate a nil debug backend in the absorb frontier (unreachable from the module's own DB; consistency with the execctx frontier lookups). Log fill-admission stats at Debug like the sibling cache stats — PrintCacheStats runs per commit cycle.
|
A convergence note for rebasing this PR onto #23095: the assert-at-wiring shape here is stronger than the detached binding helper currently used by #23095, and the finished stack should keep only one mechanism. Suggested combined end state:
BranchCache needs separate activation because it can be enabled without StateCache. That should not require the That leaves one aggregator-level invariant, activated automatically by either cache, while preserving the cannot-forget property of this PR and the file-publication binding required by #23095. |
…followups # Conflicts: # db/state/execctx/domain_shared.go # execution/cache/state_cache.go # execution/cache/view.go # execution/execmodule/exec_module_internal_test.go
Closes #22925. Follow-ups to #22444.
#22444's fill admission is sound on two premises: every writer of durable state applies to the cache (so
appliedEndtracks reality), and every wiring step actually happens. This PR enforces both premises in code and makes the apply path cheap enough to put the biggest writer on it.Performance
Commitroutes its pending state updates throughApplier.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, so per-key locking was a regression — and every acquisition barriers concurrent RPC fills. Chunking bounds a fill's wait to one chunk, and one lock per chunk is strictly stronger than per-key, so the ordering argument is unchanged.BenchmarkApplierApply: ~815 → ~714 ns/update uncontended; the contention win is the point. Commit-time memory: the post-commit stash peaks at ~430 B per flushed tuple (~205 MB for a 500k-tuple commit, measured heap-probe, transient) — linear in the batch write set and bounded by the exec batch caps; an OS-level RSS check on a real initial sync is queued with the measurement run.STATE_CACHE_FILLS=falsethe miss path bound a frontier only forFillto no-op (~162 ns, 1 alloc vs the ~101 ns no-cache baseline). The fill block is now gated onFillsEnabled, pinned by anAllocsPerRuntest, andCanFillmeans what it says: fills can go through this view.Invariants enforced in code, not comments
StateCache.BindAggregator(db), andSetStateCacheasserts the binding — a wiring site that forgets it fails loudly at startup, not silently at a later frontier lowering.kv.TemporalRwDBnow carriesAgg() any, so a DB shape that cannot produce its aggregator no longer compiles (membatchwithdb'stemporaldbreturns nil and fails the assert).Flushpanics on a cache-attached SD. A plainFlushneither applies nor invalidates, so the cache would keep serving pre-flush values for the flushed keys; callers must route throughCommit. A panic rather than an error, matching theSetStateCacheassert: both are wiring bugs, and an error can be swallowed. Tested both as a contract and end-to-end (the cache serving v1 while MDBX holds v2 is exactly what the panic prevents).Debug() == nil(aMemoryMutationover a nil db) means "no exact frontier": reads work, fills are skipped.Coherence and observability
ProcessFrozenBlocksadvanced durable state throughSharedDomainswith no attached cache — the one writer outside the apply stream — while engine endpoints are already live. Its SD construction now attaches the module's caches, so catchup commits apply post-commit and advance the admission frontier: pre-catchup read views cannot refill stale state. Snapshot publication (RunSnapshots→agg.OpenFolder) is the one startup state-advance with no applies at all, so it gets its own operation:Applier.AbsorbFilesExtensionadvances the admission frontiers to the new file visibility and drops every entry, atomically — pre-publication views cannot refill what was dropped, and publication that stays within applied ranges (local segment building) is a strict no-op. Here it runs onProcessFrozenBlocks' normal exits; db/state, execution: reconcile StateCache and BranchCache at the file-publication boundary #23047 (stacked) moves it to the publication boundary itself, which also covers error paths, mid-run downloads, and the commitment BranchCache — closing StateCache: fills can go stale when a snapshot download extends file visibility with never-applied state #23028. Note the operational footprint of attaching the code store to catchup: it goes beyond cache coherence — every contract deployed over the whole chain gets an MDBX write intoTblCodeCacheduring initial sync, capped by the eviction now running on the catch-up prune path (the final batch's overshoot is transient — the first forkchoice prune evicts it; the eviction counter's rollback drift, pre-existing on the forkchoice path, is CodeStore: TblCodeCache size counter drifts when a transaction rolls back after Evict #23048). The reward is a warm code store at the catchup→tip handover.sequenceDiagram participant Reader as Pre-catchup reader (frontier N) participant Cache as shared StateCache participant PFB as Frozen-block catchup Reader->>Cache: GetLatest(K) misses → fill(v_old) PFB->>PFB: executes frozen blocks, commits v_new at txNum M >= N Note over Cache: main — commit bypasses the cache: v_old stays "latest" indefinitely PFB->>Cache: this PR — post-commit apply: K = v_new, appliedEnd = M+1 Reader->>Cache: later miss → fill(v_old) from frontier N Note over Cache: this PR — rejected, N < appliedEndPrintStatsAndResetreports fill attempts by outcome — admitted, rejected, and no-frontier (attempts dying on anok=falsefrontier before the admission compare, so the three sum to attempts — save one deliberate gap: empty-value code fills don't count, since code negatives are represented via the addr→codeHash sentinel instead): the lens for how much reader warming survives a given commit cadence (parallel-exec workers, for example, hold one read tx per run, so their fills stop after the first mid-run commit — the counters measure what that costs on a real sync).Also: the per-domain admission invariant is stated at
appliedEnd, rationale duplicated from the #22444 description is trimmed fromview.go, and the deaddomainfield is dropped from the branch stash.How to review
execution/cache/view.go+state_cache.go— the new API surface:ApplyAll(chunked locking),BindAggregator(the bound marker), counters.db/state/execctx/domain_shared.go—Commit's batch apply,Flush's rejection,SetStateCache's assert, theFillsEnabledgates.execution/execmodule/executor.go—newFrozenBlocksSD, the three-line fix for execution: fence StateCache across frozen-block startup processing #22925.Testing
Every behavior change is pinned red→green:
ApplyAllequivalence, chunk-boundary and benchmark coverage; theFlushrejection (contract + incoherence scenario); theSetStateCacheassert; nil-Debug()reads; the apply-only allocation gate; and the frozen-block wiring (a pre-seeded stale entry overwritten by catchup applies, a pre-catchup view's refill rejected).go testacross all touched packages with-raceonexecution/cache,db/state/execctx,execution/execmodule;make lintclean.