execution, db/state: StateCache follow-ups - #22159
Closed
yperbasis wants to merge 35 commits into
Closed
Conversation
The warmBody prefetch goroutine reads committed (parent-block) state and Put it into the StateCache unconditionally. A laggard Put landing after the FCU flush's cache-apply replaced the flushed value with the pre-flush snapshot, stamped with the current epoch — permanently valid to the lazy unwind staleness check. The next block then executed against the stale value: wrong trie root / INVALID payload (the eest-spec-enginextests merge-queue flake first seen after #21386 merged). Fix in two layers: prefetch writes go through the new PutIfAbsent / PutCodeWithHashIfAbsent (live entries kept, stale ones replaced), so either interleaving converges to the authoritative value; and updateForkChoice drains in-flight warmup at entry — before the unwind epoch-bump and the flush cache-apply — subsuming the unwind-path-only drain.
The if-absent check+insert was Get-then-Add on a per-call-locked LRU, so a conditional writer could check (absent), lose the CPU to an authoritative Put, then clobber it. Serialize same-key writers: striped mutexes in GenericCache, a bind mutex around the CodeCache addr-to-code binding. Red-tested by hammering a single key with a concurrent Put/PutIfAbsent pair — pre-fix the stale value won within a few thousand rounds.
…sent rationale Route Put/PutIfAbsent and PutCodeWithHash/PutCodeWithHashIfAbsent through shared private cores so the nil-cache and KeyCommitmentState guards live in one place, matching the GenericCache/CodeCache shape. State the if-absent rationale once on Cache.PutIfAbsent; the other sites keep terse pointers.
CodeCache.putCode drops zero-length puts, so the comment's 'no code' claim was wrong (flagged by Copilot review; the wording predates this PR).
Extracts just the codeHash field from a SerialiseV3-encoded account record, skipping the full decode (balance parse, codeHash interning) that DeserialiseV3 pays. Bounds-checked against truncated records and faithful to CodeHash.IsEmpty for non-canonical records that spell out the empty/zero sentinel. Groundwork for #22120 finding 7: SharedDomains.codeHashForAddr runs this per mem-hit on the codeHash fast path.
…p-in-flight assert Three of the #22120 findings against the cache package itself: Finding 11: GenericCache.Delete and the lazy stale-drop inside GetWithTxNum removed entries outside the put stripes, so a removal interleaved between put's data.Get and data.Add double-subtracted the displaced entry's size (once via freelru's OnEvict, once via put's update delta) — currentSize drifted one entry per occurrence and never healed. Both removal paths now take the key's stripe, with a re-check under lock. Hammer tests reproduced the drift (exactly one entry size) before the fix and run race-clean after. Finding 12: new ContainsLive probes (Peek-based — no hit/miss counters, no LRU recency) let conditional writers skip preparing a put that a live entry would no-op: StateCache.put skips the value copy, and StateCache.HasLiveCode lets the read-ahead prefetcher skip the keccak+copy for an already-bound address. Advisory only — PutIfAbsent still decides under the key's stripe. Enforcement (issue recommendation A, cheap variant): StateCache gains a warmup-in-flight gauge (WarmupStarted/WarmupDone); Unwind panics under ASSERT_STATE_CACHE when a cache-populating warmup is still in flight, converting the drain-before-epoch-bump convention into a loud failure. The drain-free getter remains tracked in #22116.
… domain progress Read-ahead warmup follow-ups from #22120: Finding 12: with if-absent semantics a live entry makes the conditional put a no-op, but the prefetcher had already paid the keccak over the full bytecode plus the value copy by then — the steady-state warm prefetch allocated and discarded ~1-3 MB of hash+copy work per block. The code branch now probes HasLiveCode before preparing the put; the accounts/storage copy elision lives in StateCache.put (previous commit). A live binding makes the prefetch a full no-op: it no longer populates content layers for its superseded snapshot code either. Latent-note fix: negative results (missing account, empty slot) carry no step to derive an unwind bound from, and the synthetic step-0 stamp ((0+1)*stepSize-1) sat below every realistic unwind floor, making cached negatives immortal. Correctness relied on the flush-callback overwrite always firing. Negatives are now stamped with the domain's progress at observation time (max committed txNum in the read snapshot), so they drop on any unwind that could matter instead of outliving the fact they cache. Also brackets the fire-and-forget warmBody goroutine with the new WarmupStarted/WarmupDone gauge; the release is ordered before warmWg.Done so a WaitForWarmup return implies the gauge is back to zero.
…old maxStep gates The SharedDomains half of the #22120 findings: Finding 9: the getLatestMetered read-fill was a second unconditional-Put snapshot writer — an embedded-RPC eth_call straddling an FCU commit (or a bounded read during an in-flight unwind) could overwrite the flush-applied value with the pre-flush one, the same clobber shape #22146 fixes for the warmup. Read-fills never carry newer information than a flush-apply, so both fills (domain values and code) now use the if-absent puts. Finding 4: the ASSERT_STATE_CACHE divergence check compared a cache hit against a DB read bounded by the same maxStep the mem overlay published for a key an in-flight unwind re-bound. In that window MDBX still holds the not-yet-deleted dying row inside the bound, so the authoritative read returns dead-fork bytes and the assert panicked on a legitimate below-floor hit. The assert now runs only when no per-key bound is active. (The Value==nil delete-only diff that produces this signal comes from legacy V0-format persisted changesets; current V1 diffs are served from mem directly — see DomainRoTx.unwind.) Finding 7: the stateCache and branchCache maxStep gates were two copies of the same rule differing only in a subtle divide/don't-divide unit conversion; both now call one servableUnderBound helper carrying the shared rationale, with the unit conversion explicit at each site. codeHashForAddr's per-mem-hit full account decode is replaced by the targeted DeserialiseV3CodeHash extractor. Finding 5: ClearBranchCache and DetachBranchCache had no callers, and DetachBranchCache's docstring advertised a fork-validation guard that was never wired — correctness rests on the epoch-bumping sd.Unwind, which every unwind path already funnels through. Deleted rather than wired: detaching would cost fork validation its warm branch cache. ProbeReadLayers is kept — it gains a caller in #22154. Latent-note fix (SD side): negative read-fills are stamped with the domain's progress instead of the synthetic step-0 bound, mirroring the warmup getter.
…rozenBlocks Finding 10 of #22120: engine servers are live before ExecModule.Start, and ValidateChain fires the read-ahead warmup before its too-far-away check, so a payload validated in the pre-Start window warms the cache with pre-catchup state — nil negatives included. Frozen-block processing advances state without touching the cache (its SDs are never wired to it) and neither clears nor epoch-bumps, so those entries stayed live through the whole catch-up and were served cache-before-aggTx afterwards: stale value, wrong root until restart. Start now drains any in-flight warmup and clears the cache under the semaphore before frozen-block processing; no new warmup can start until Start releases it. No isolated unit test: reproducing needs a live engine server racing PFB startup in the ms-scale pre-Start window. The change composes two already-tested primitives (drainReadAhead, StateCache.Clear) at a point where the semaphore excludes concurrent writers.
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up fixes for the StateCache/StateCache warmup and read-fill behavior, focused on correctness under unwind/flush races and reducing unnecessary work on hot paths.
Changes:
- Add side-effect-free “live entry” probes (
ContainsLive/HasLiveCode) to avoid wasteful copies/keccak work for conditional writers and prefetchers. - Fix cache removal/write interleavings by striping stale-drop and delete paths under the same per-key put stripe to prevent size-accounting drift.
- Improve correctness around warmup/unwind ordering (assert-gated warmup-in-flight), negative-entry stamping, and reduce account decode overhead via a targeted
DeserialiseV3CodeHash.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| execution/types/accounts/account.go | Adds DeserialiseV3CodeHash fast-path extractor for account codeHash. |
| execution/types/accounts/account_test.go | Adds tests for DeserialiseV3CodeHash correctness and malformed inputs. |
| execution/execmodule/exec_module.go | Drains read-ahead and clears state cache before ProcessFrozenBlocks to avoid stale pre-Start warmups. |
| execution/exec/blocks_read_ahead.go | Stamps negative warmup results with domain progress and avoids keccak/copy when code binding is already live; adds warmup in-flight bracketing. |
| execution/exec/blocks_read_ahead_test.go | Adds tests for negative-unwind drop and “live binding makes prefetch a no-op” behavior. |
| execution/cache/state_cache.go | Adds warmup-in-flight gauge + assert on unwind; uses ContainsLive to skip unnecessary copies for PutIfAbsent; adds HasLiveCode. |
| execution/cache/generic_cache.go | Stripes delete + stale-drop removals to avoid size drift; adds ContainsLive. |
| execution/cache/code_cache.go | Adds ContainsLive for addr→code binding liveness checks. |
| execution/cache/cache.go | Extends Cache interface with ContainsLive. |
| execution/cache/cache_test.go | Adds concurrency/atomicity and semantics tests for Delete/stale-drop striping and ContainsLive; tests unwind warmup assert. |
| db/state/execctx/statecache_readfill_test.go | Adds tests for assert behavior during in-flight unwind, read-fill not clobbering live entries, and negative stamping. |
| db/state/execctx/domain_shared.go | Refactors maxStep gating into helper; switches read-fill to if-absent; stamps negatives with domain progress; uses targeted codeHash extractor; removes dead branch-cache APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…review-fixes # Conflicts: # execution/cache/cache.go # execution/cache/cache_test.go # execution/cache/state_cache.go # execution/exec/blocks_read_ahead.go # execution/exec/blocks_read_ahead_test.go
A WaitForWarmup landing between WarmupStarted and warmWg.Add saw a zero wg counter and returned while the gauge was already raised, so a drain-then-bump caller could trip the ASSERT_STATE_CACHE panic in StateCache.Unwind on a correctly-drained warmup. Today's callers cannot interleave there (the module semaphore excludes AddHeaderAndBody from every drain-then-bump path), but the assert exists to be trustworthy when that convention is violated, so its wiring must not manufacture false positives on its own. Raising the gauge after Add restores the invariant from both sides: a Wait either returns before Add (gauge still zero) or blocks until Done (gauge dropped first, defers run LIFO). Flagged by Copilot on #22159.
yperbasis
changed the title
yperbasis
changed the title
…review-fixes # Conflicts: # db/state/execctx/domain_shared.go # execution/cache/generic_cache.go
…e a resize maybeGrow copied entries and then swapped generations, so a striped writer that loaded the old generation before the swap landed its write in the abandoned copy — the migrated (older) value resurfaced as live: a stale serve, not the documented benign miss. Growth now publishes the new generation first, fences the put stripes, and migrates each key under its stripe with if-absent semantics; the grow trigger moves out of put's striped section (the fence would self-deadlock). growLRU keeps the old order: its content-addressed values are immutable per key, so a lost copy there really is a benign miss.
Deleting the entry left the key absent, so a read-fill from a reader straddling the deletion (a pre-delete snapshot) re-inserted the old value as live — PutIfAbsent only defers to live entries — and the deleted account or slot was served as canonical. A nil tombstone stamped with the delete's txNum defends the key and drops on an unwind at or below the deletion. The code layers keep the delete: they cannot represent negatives.
…ine codeHash extraction, add negative-read benchmarks Code negatives in the warmup getter no longer pay a DomainProgress call and a liveness probe for a put CodeCache drops anyway. The decodeAccountCodeHash wrapper was a pure pass-through — call accounts.DeserialiseV3CodeHash directly. The new benchmarks bound the negative-stamp cost: the keys-table LastKey is ~290 ns and the whole cache-side fill ~0.5 us per cold negative (M2 Max), paid once per key between invalidations.
yperbasis
changed the title
The swap-then-migrate resize left each key absent from the primary generation until migrated, so a conditional put arriving mid-resize found a gap and filled it with a stale value — the writer class PutIfAbsent exists to close, reopened for the resize window. The pre-stripe grow trigger also paid a ShardedLRU.Len (a lock per shard) on every put, including warm updates. The copy now runs with every put stripe held and the swap publishes a fully-copied generation, so no operation ever sees a partial one; the fence sweep, per-key migration and superseded-size accounting go away with the window. The trigger returns to the insert path and the grow runs after the stripe is released (growth takes every stripe).
… simplify put's grow trigger The Cache.ContainsLive interface method had one caller — StateCache.put's pre-check — whose only real value (skipping the keccak for already-bound code) lives in HasLiveCode, which reaches CodeCache.ContainsLive concretely; for domain values the probe traded a tens-of-bytes copy against an extra Peek on the common miss path. putLocked now returns whether the caller should grow, replacing the defer-ordering trick, and warmBody builds its getter through one constructor. TestGenericCache_PutIfAbsentDefersAcrossGrow is rebuilt to be scheduling-proof: reaching Len >= startCap organically requires every freelru shard full, which makes the hot key evictable and a stale insert legitimate — a flake under CPU contention. The cache is now seeded below capacity pressure with the hot key inserted last (the LRU victim is always an older seed key) and the grow is forced by lowering curCap.
…deletion marker for the code cache A code deletion was applied to the cache as a raw Delete, so a read-fill from a reader straddling the deletion (a pre-delete snapshot) re-bound the deleted code as a live hit — the same resurrection the account and storage tombstones close, flagged by Copilot. An authoritative nil put now binds a live no-code marker (versionedAddressID.deleted, stamped with the delete's txNum): a valid negative on read, a binding conditional fills defer to, replaced by an authoritative rebind and dropped by an unwind at or below the deletion. The flush-apply writes Put(CodeDomain, key, nil, txN), symmetric with the other domains. Also tightens the DeserialiseV3CodeHash doc: it parses only up to and including the codeHash field; later fields are not validated.
With the flush-apply writing tombstones and no-code markers, Delete lost its last production caller — and any future call would reopen the resurrection window the markers close, since removing an entry leaves nothing for a straddling conditional fill to defer to. Drop it from the Cache interface and every implementation so the wrong operation is not expressible; the stale-drop keeps the stripe-serialized removal rationale. The hammer tests that used Delete as a reset now race on fresh keys per round, and the stale-drop drift check asserts the exact expected residency instead of zero-after-delete.
yperbasis
marked this pull request as draft
July 10, 2026 09:11
…nil puts" This reverts commit 9072533.
…no-code deletion marker for the code cache" This reverts commit 23bcf56.
…pply" This reverts commit 561b29d.
Keep fresh-key setup in the PutIfAbsent concurrency tests and exact residency accounting in the stale-drop test. Restore only the Delete-specific coverage needed after extracting tombstones from this PR.
A put racing Clear could load the retiring generation, land its entry where no reader sees it, and add the entry's size after Clear zeroed the counter — a phantom byte count that never drains. Clear now runs the counter reset, coherence re-init and generation swap with every put stripe held, mirroring maybeGrow's swap; lock order (resizeMu → stripes) is unchanged, so writers either complete before the swap or land in the fresh generation.
A capacity eviction is a size-subtracting writer the put stripes cannot serialize: freelru picks its victim per shard (hash bits 16+), so an insert on one stripe can evict a key whose own update — on another stripe — sits between its Get and Add, and the update's newSize-existing.size delta then double-subtracts the victim's size. Replace the delta accounting (update and collision branches) with remove-then-add, making the OnEvict callback the sole subtractor, as putContent already does. Intentional removals (update, Delete, dropStale) are compensated in the evictions metric, which also stops stale drops counting in both staleEvicted and evictions.
Generation swaps stay unfenced here on purpose: the layers it backs are content-addressed, so a lost write is a benign miss and a raced removal resurrects correct bytes that drop on the next stale read. State that, the counter approximation it implies, and that mutable-per-key values belong on GenericCache's fenced swap instead.
… drain as a failed precondition WaitForWarmup could return on context cancellation with the warmup still running, so "WaitForWarmup returned ⟹ warmup gauge is zero" held only for the completed-wait path; a payload validation racing module shutdown could then bump the cache epoch un-drained (an open-tx unwind never re-checks the context) and trip the ASSERT_STATE_CACHE gauge assert. WaitForWarmup and drainReadAhead now report whether the warmup fully drained — false only when the module context is cancelled — and the epoch-bump/Clear call sites return instead of proceeding. The DB-close caller keeps ignoring the result: it only needs a bounded wait.
…he snapshot AddHeaderAndBody read bra.stateCache separately for WarmupStarted, the deferred WarmupDone, and warmBody's getter wiring, so a SetStateCache racing the launch could split the pair (negative gauge) or tick one cache's gauge while the workers populate another — the one whose gauge Unwind asserts. Capture the pointer once and thread it through warmBody so the Started/Done bracket and the puts all bind to the same cache.
…fenced copy Grows are silent, so the once-per-lifetime writer stall of the fenced copy (~150ms for the final 1GB-accounts step) surfaces as an unexplained FCU/execution latency blip. One Debug line per grow with the caps, copied count and the alloc/fenced split makes it self-explaining.
This was referenced Jul 14, 2026
Merged
Member
Author
|
This PR grew too large; it is superseded by four independent (non-stacked) PRs that together reproduce its full diff — verified by merging all four onto main and diffing the union against this branch (byte-identical on every touched file):
Finding 6 remains a won't-fix as described here; the unwind-side read-fill residual is tracked in #22463. This branch is left intact for reference. |
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Jul 15, 2026
…a full account decode (erigontech#22468) Split from erigontech#22159 (the erigontech#22120 StateCache review findings — finding 7's decode-cost half). ## What changed `codeHashForAddr` fully decoded every account record it touched — balance parse plus codeHash interning per mem hit — just to read one field. `accounts.DeserialiseV3CodeHash` parses the SerialiseV3 layout only up to and including the codeHash field: - bounds-safe on every truncation point (returns nil on malformed input; the full decoder indexes without length checks), - returns nil for both no-code sentinel spellings (zero hash, empty-code keccak), matching `CodeHash.IsEmpty`, - returns a subslice of `enc`, valid only while `enc` is — all four call sites in `codeHashForAddr` consume it synchronously within the tx, and the one retained copy (`PutAddrCodeHash`) goes through a fixed `[32]byte`. `decodeAccountCodeHash` is deleted; its call sites switch to the extractor. ## Testing - `TestDeserialiseV3CodeHash` cross-validates the extractor against the full `DeserialiseV3` decode over a nonce × balance × codeHash × incarnation matrix. - `TestDeserialiseV3CodeHashMalformed` walks every truncation point of a record (nil at any cut into the codeHash, the hash beyond it), rejects non-32-byte codeHash fields, and pins the sentinel spellings to nil. Verification: `execution/types/accounts` + `db/state/execctx` suites, repeated clean `make lint`. Touches `domain_shared.go` in hunks disjoint from erigontech#22467; the two merge independently.
Sahil-4555
pushed a commit
to Sahil-4555/erigon
that referenced
this pull request
Jul 16, 2026
…counting exact (erigontech#22466) Split from erigontech#22159 (the erigontech#22120 StateCache review findings): the `GenericCache` concurrency and accounting fixes, self-contained to `execution/cache`. ## What changed **Jump-grow fence.** `maybeGrow` now copies and swaps the generation with every put stripe held (allocation stays outside the fence). Previously a striped put could land in the retiring generation — not a benign miss: the copy may already have migrated the key's *older* value, which then resurfaced as live; and a `PutIfAbsent` arriving in the mid-resize gap could install a stale snapshot value as live, defeating the if-absent semantics the read-fill paths rely on. Grow detection moves inside the stripe and the grow itself outside it (calling it stripe-held would self-deadlock against take-them-all); the triggering insert and racers until the swap evict at the pre-grow cap — a transient bounded by the grow window, noted in source. Generations carry an explicit freelru shard count that doubles across grows only while per-shard capacity does not shrink, so the migration copy can never overfill a shard and evict — left to freelru, a grown generation could pick more, smaller shards and silently drop clustered entries. **Clear fence.** `Clear` is the second wholesale generation replacement; it now runs its counter reset, coherence re-init and swap under the same fence, so a racing put can neither land in the retired generation nor add its size after the counter was zeroed. Readers stay lock-free and are ordered instead: `GetWithTxNum` snapshots coherence before loading the generation and `Clear` re-inits coherence only after its swap, so an entry captured from the retiring generation is always judged by pre-init coherence that still carries the unwind — judged against the live state, the re-init (fresh epoch, lifted floor) revalidated dead-fork entries for in-flight readers. A live entry judged by a pre-Clear snapshot degrades to a safe miss via the stale-drop's re-check. **Epoch stamp under the stripe.** `put` samples `coh.Epoch()` with the key's stripe held, next to the generation load the fence synchronizes. Read before the stripe, the stamp raced `Clear`'s coherence re-init: a put that lost the stripe to `Clear` landed a pre-Clear epoch on an entry in the post-Clear generation, and once a later unwind re-reached that epoch value the entry aliased the live epoch and served dead-fork state despite a txNum at or above the floor. **Exact size accounting.** `Delete` and the lazy stale-drop run under the key's put stripe, and `currentSize` is subtracted solely via the `OnEvict` callback (update/collision paths do remove-then-add instead of delta arithmetic). freelru picks eviction victims per shard — hash bits 16 and up, which the put stripes (bits 0–7) don't cover — so any subtraction computed outside the callback races a cross-stripe capacity eviction and double-subtracts. Capacity evictions are counted from `freelru.Add`'s evicted return at the call sites — OnEvict also fires for intentional Removes, and routing those through the metric races a concurrent stats reset — which also stops stale drops counting in both `staleEvicted` and `evictions`. The byte counter is reserved before a remove-then-add, so a ModeNoOp admission never observes a transient dip and over-admits past the budget. **Observability.** One Debug line per grow with the caps, shard count, copied and copy-evicted counts, and the alloc/fenced duration split. Measured on the production accounts geometry (1 GB / 96 B avg), the final 4.19M-entry step is ~470 ms total of which ~150 ms is the writer-visible fenced copy — once per process lifetime. **growLRU contract.** The CodeCache layers' `growLRU` keeps its unfenced swap deliberately; its doc comment now states why that is safe only for content-addressed layers (lost write = benign miss, raced removal = resurrect-once dropped on the next stale read, counters approximate) and points mutable-per-key values at the fenced `GenericCache`. ## Testing TDD: each behavioral fix has coverage that failed on the pre-fix code. - `TestGenericCache_PutNotLostAcrossGrow` and `TestGenericCache_PutIfAbsentDefersAcrossGrow` (fail in the first rounds when the fence is removed) - `TestGenericCache_GrowMigrationLossless` (clustered keys deterministically evicted by a resharding migration pre-fix) - `TestGenericCache_ClearRacingPut_EpochAlias` (deterministic stripe-parking choreography; fails every run pre-fix) - `TestGenericCache_ClearRacingGet_DeadEntryStaysDead` (reader gated on the fence reaching its stripe; served dead-fork state within tens of rounds pre-fix) - `TestDomainCache_ClearAtomicWithPut_NoSizeDrift` - `TestDomainCache_DeleteAtomicWithPut_NoSizeDrift` and `TestDomainCache_StaleDropAtomicWithPut_NoSizeDrift` - `TestGenericCache_CapacityEvictionAtomicWithPut_NoSizeDrift` (cap-1 cache forces same-shard cross-stripe eviction; reproduces the drift in ~0.2 s pre-fix) - `TestGenericCache_StatsResetAtomicWithDelete_NoPhantomEvictions` (intentional removals leak into the metric within milliseconds pre-fix) - `TestGenericCache_ModeNoOpAdmissionAtomicWithUpdate` (over-admission past a full budget pre-fix) Test caches now close on cleanup, returning their envelope reservations — the process-global `cachebudget.Global` otherwise accumulates leaked reservations across the package run, which would starve the grow tests' `Reserve` calls. Verification: `go test ./execution/cache/...`, `-race` on the whole concurrency family, repeated clean `make lint`. Note: erigontech#22159's warmup-lifecycle split also appends tests to `cache_test.go`; whichever lands second rebases trivially.
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Jul 16, 2026
…and unwinds (erigontech#22467) Split from erigontech#22159 (the erigontech#22120 StateCache review findings): the `SharedDomains` read-path changes — findings 4, 5, 7 (bound unification) and 9 — plus the immortal-negative fix applied at both fill sites (SD read-fill and warmBody read-ahead), and a mem-batch contract fix (`kv.NoStepBound`) so the per-key unwind bound survives step 0. ## What changed **Finding 9 — read-fills defer to authority.** `getLatestMetered` populates the cache with if-absent semantics (`PutIfAbsent`, `PutCodeWithHashIfAbsent`): a read-fill never carries newer information than a flush-apply, so a snapshot reader can no longer overwrite a live authoritative entry (e.g. an embedded-RPC read straddling an FCU commit). **Immortal negatives.** Missing accounts and empty slots are stamped with the domain's progress at observation time rather than a synthetic step-zero bound, so an unwind can invalidate them. The benchmarked keys-table `LastKey` cost is ~290 ns and the complete cold-negative fill ~0.5 µs (M2 Max); both are paid only on reads that already traverse the file-accessor stack. Applied at both fill sites into the process-global cache: the SD read-fill and warmBody's read-ahead prefetcher (`cachePopulatingGetter`, which wraps the raw temporal tx and therefore doesn't inherit the SD fix). Observable side effect: a cached empty-value hit returns a progress-derived step rather than a deletion step; no consumer reads the step of an empty value (the write path, commitment, and RPC readers all discard it), and the hit path documents this. **Finding 4 — no false `ASSERT_STATE_CACHE` panic during in-flight unwinds.** The divergence assert runs only when the mem overlay publishes no per-key `maxStep` bound: during an in-flight unwind MDBX still holds the dying row inside the bound, so the "authoritative" comparison read can return dead-fork bytes and blame the cache for a legitimate below-floor hit. The bound now survives step 0: a plain mem miss returns `kv.NoStepBound` instead of 0, so a delete-shape bound at step 0 is no longer conflated with "no bound" — a young chain's whole state lives in step 0. **Finding 7 (bounds) — one gate, explicit units.** The state-cache and branch-cache maxStep gates share `servableUnderBound`; the StateCache divides its txNum stamp by the step size, the BranchCache uses on-disk step indices directly — the unit mismatch that previously defeated the gate once. **Finding 5 — dead `DetachBranchCache` deleted.** It advertised a fork-validation guard that was never wired; the actual isolation mechanism is the epoch-bumping `sd.Unwind`, and detaching would discard a useful warm branch cache. ## Testing TDD: each behavioral fix has coverage that failed on the pre-fix code. - `TestAssertStateCache_NoFalsePanicDuringInFlightUnwind` (plus a step-0 variant pinning the `kv.NoStepBound` signal) - `TestReadFill_DoesNotClobberLiveEntry` (the fall-through read serves the maxStep-bounded row without replacing the live entry) - `TestReadFill_NegativeStampedWithProgress` - `TestCachePopulatingGetterNegativeDroppedByUnwind` (the warmBody fill site) - `BenchmarkDomainProgress` / `BenchmarkGetLatestColdNegative` quantify the negative-stamp cost The read-fill regressions run in the short suite (no `testing.Short` guards) — at ~20-30 ms each they are not long-running, and they carry the PR's core behavioral coverage. Verification: full `db/state`, `db/state/execctx`, and `execution/exec` suites, repeated clean `make lint`. ## Notes - The known residual — an in-flight-unwind fill of a dying row when no live entry blocks it — is tracked in erigontech#22463; the one-line `maxStep` fill-skip proposed there composes with this PR. - erigontech#22444's linearized fill admission rewrites the same fill call sites (`PutIfFresh`); merge order needs coordinating — whichever lands second carries a rebase that is mechanical except for preserving the negative progress-stamp through the `PutIfAbsent` → `PutIfFresh` switch. Details in erigontech#22444's "Interplay with erigontech#22467" section.
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.
Important
Closed as superseded — split into four independent PRs that together reproduce this branch's full diff (union verified byte-identical): #22466 (GenericCache fences & accounting), #22467 (SD read-fills), #22468 (codeHash extractor), #22469 (warmup lifecycle). Finding 6 stays a won't-fix as documented below; the unwind-side read-fill residual is tracked in #22463.
Addresses #22120 (the #21386 StateCache review findings): findings 1, 2, 3, and 8 landed with #22154; this PR addresses the rest, with finding 6 documented as a won't-fix and recommendation A implemented using the assert variant named in the issue. The preferred drain-free getter remains tracked in #22116.
The flush-apply deletion/tombstone work previously included here has moved to #22444. This PR retains the separate jump-grow correctness fix surfaced during review.
Coverage
ASSERT_STATE_CACHEpanic during in-flight unwindDetachBranchCachePutwriterStartwarmup gap aroundProcessFrozenBlockscurrentSizedouble-subtract via unstriped removalsWhat changed
Finding 4 — the divergence assert runs only when the mem overlay publishes no per-key
maxStepbound. During an in-flight unwind MDBX can still hold a dying row inside the bound, so comparing it against a legitimate below-floor cache hit produced a false panic.Finding 5 / recommendation B —
DetachBranchCacheis deleted (ClearBranchCachealready left with #22154). It advertised a fork-validation guard that was never wired; the actual mechanism is the epoch-bumpingsd.Unwind, and detaching would discard a useful warm branch cache.Finding 6 — won't fix. The cold-read
keccak(v)keeps content-addressed code entries self-consistent when parallel execution exposes skewed cross-account codeHash reads. Finding 12 removes the recurring warm-path hash/copy cost instead.Finding 7 — state-cache and branch-cache bounds now share
servableUnderBound, with their different units explicit at each call site.codeHashForAddruses the targetedaccounts.DeserialiseV3CodeHashextractor instead of fully decoding balances and interning hashes on every mem hit.Finding 9 —
getLatestMeteredread-fills usePutIfAbsentandPutCodeWithHashIfAbsent. A read-fill cannot carry newer information than flush-apply, so a snapshot reader can no longer overwrite a live authoritative cache entry.Finding 10 —
ExecModule.Startdrains warmup and clears the state cache under the module semaphore beforeProcessFrozenBlocks, preventing pre-start payload validation from leaving pre-catchup cache entries live.Finding 11 —
GenericCache.Deleteand the lazy stale-drop take the key's put stripe and re-check under lock, preventingOnEvictaccounting from racing the update delta and subtracting the displaced entry twice.Finding 12 — a side-effect-free
CodeCache.ContainsLiveprobe lets read-ahead skip the keccak and copy for an already-live address binding. Domain values deliberately keep the single conditional put because their copies are small and the common fill path is a miss.Latent note — missing accounts and empty slots are stamped with domain progress at observation time rather than a synthetic step-zero stamp, so an unwind can invalidate them. Code read misses remain uncached. The benchmarked keys-table
LastKeycost is approximately 290 ns and the complete cold-negative cache fill approximately 0.5 µs on an M2 Max.Recommendation A —
WarmupStartedandWarmupDonetrack cache-populating warmup, andUnwindpanics underASSERT_STATE_CACHEif one remains in flight. This converts the drain-before-epoch-bump convention into a loud invariant failure.Jump-grow resize — growth now fences put stripes while copying and publishes only a fully populated generation. Writers cannot land in an abandoned generation or expose a partial generation to conditional puts.
Testing
TDD: each behavioral fix has coverage that failed on the pre-fix code.
TestDomainCache_DeleteAtomicWithPut_NoSizeDriftandTestDomainCache_StaleDropAtomicWithPut_NoSizeDriftTestGenericCache_PutNotLostAcrossGrowandTestGenericCache_PutIfAbsentDefersAcrossGrowTestAssertStateCache_NoFalsePanicDuringInFlightUnwindTestReadFill_DoesNotClobberLiveEntryTestReadFill_NegativeStampedWithProgressandTestCachePopulatingGetterNegativeDropsOnUnwindTestCachePopulatingGetterSkipsContentForLiveBindingTestDeserialiseV3CodeHash*TestCodeCache_ContainsLiveandTestStateCache_UnwindAssertsWarmupInFlightFinding 10 has no isolated test because reproduction requires a live engine server racing frozen-block processing during startup; it composes two tested primitives under the semaphore.
Verification includes the affected package suites, race coverage for the cache packages, reorg/fork engine tests, repeated clean
make lintruns, andmake erigon integration.