execution/cache: review findings for #21386 (StateCache LRU + (txNum,epoch) lazy unwind)

Follow-up findings from a code review of #21386 (*execution/cache: StateCache LRU + Mode + `(txNum,epoch)` lazy unwind*), reviewed at head `73d0911`.

These are review findings, not a merge blocker per se — filing as a tracking issue so they aren't lost. Line numbers are post-image (PR branch) and may drift; the function/symbol anchors are stable. Severity is my own assessment.

> **Note:** The high-risk areas were investigated and found **sound** — see *Investigated & found sound* at the bottom so these aren't re-litigated. The metrics-collector OOM was already fixed in `73d0911`.

---

## Correctness / behavior

### 1. (med-high) CodeCache content layers freeze when full instead of evicting; orphans never reclaimed
`execution/cache/code_cache.go` — `hashToCode`, `codeHashToCode`, `codeSizeByCodeHash` are plain `maphash.Map`s with no LRU; `putAccounted` only *refuses* once `counter+cost > capacity` (no eviction). The addr LRUs are built with `lru.New(...)` and **no `OnEvict`**, so evicting a cold address orphans its content entry, which is then never re-`Get`-reached and never stale-dropped. There is also **no live `StateCache.Clear()` caller** in the PR (invalidation is purely epoch-based, which never drops committed entries `txNum < floor` or orphans).

*Effect:* over a sustained run / full sync, once unique bytecode exceeds the cap (512 MB bytes, or 1M size-entries) the code byte+size caches fill permanently and refuse all newly-seen contracts — so EXTCODE*/CALL on freshly-active code keeps paying the full file-accessor + decompression stack. This is the opposite of the PR's "the working set warms up instead of being periodically dropped" goal, and inconsistent with the account/storage `GenericCache`s, which *do* evict LRU. Consider applying the same LRU/eviction policy (or an OnEvict that reclaims content) to the code content layers.

### 2. (med) Production Account cache is ~384 MB, not the configured 1 GB
`execution/cache/state_cache.go` (`newDomainCacheBytes`) + `DefaultAccountCacheBytes = 1 * datasize.GB`. `capacityEntries = 1GB / 96 ≈ 11.2M` is clamped to `1<<22 = 4.19M` (to bound freelru's eager slot allocation). In `ModeEvictLRU` the **entry count is the binding limit**, so residency settles at ≈ 4.19M × 96 B ≈ 384 MB. The inline comment "the byte budget still bounds residency below this cap" is inverted — the entry cap binds first, so the 1 GB budget is unreachable for Account. Either raise the clamp for this case or document that Account is entry-capped at ~384 MB. (Storage, 150 MB → ~1.79M entries, is unaffected.)

### 3. (med) `putAccounted` over-cap back-out uses a non-accounted raw `m.Delete`, drifting the byte counter under concurrency
`execution/cache/code_cache.go` — the back-out `if counter.Add(cost) > capacity { counter.Add(-cost); m.Delete(key) }` uses a raw delete instead of the accounted `LoadAndDelete`, breaking the file's own "only the goroutine that removes adjusts the counter" rule.

*Effect (near capacity — which finding 1 makes reachable):* a concurrent stale-drop `LoadAndDelete` of the same key double-subtracts the cost (counter under-counts → admits past the byte budget), or a concurrent insert+account followed by W1's stale `Delete` removes an accounted entry with no compensation (counter wedges high → refuses inserts while the layer is near-empty). Drift is one entry per occurrence and does not self-heal. Fix: route the back-out through the accounted `LoadAndDelete` (subtract only on `removed`), or make insert+account a single critical section.

### 4. (low — assert-gated) False "stateCache divergence" panic during an in-flight unwind
`db/state/execctx/domain_shared.go` (`getLatestMetered`, `AssertStateCache` branch). A cache hit is served when `cStep <= maxStep`, but the authoritative `vDB` is read with the *same* `maxStep` bound; when an unwind has lowered `maxStep` and the cached entry's `cStep` is strictly below it, MDBX can hold a newer value in `(cStep, maxStep]`, so `bytes.Equal(v, vDB)` fails and it panics although the cache served a legitimately-bounded value. Only fires under `ASSERT_STATE_CACHE=true` (production-safe), but a spurious panic in assert/CI builds is disruptive. (The new `dbErr` guard added just above it is correct.)

---

## Cleanup / quality

### 5. Dead code that advertises a guarantee it doesn't provide
`db/state/execctx/domain_shared.go` + `execution/commitment/commitmentdb/commitment_context.go`. `DetachBranchCache()`, `ClearBranchCache()`, and `ProbeReadLayers()` have **zero call sites** in the PR; `ProbeReadLayers` and `Metrics()` were also added to the `sd` interface, forcing every implementer/stub to satisfy unused methods. Worse, `DetachBranchCache`'s docstring claims it prevents fork-validation from "read[ing] stale committed branches (wrong trie root → INVALID payload)" — a guard that is never wired, so a reader believes a safety mechanism exists when correctness actually rests on the epoch-bumping unwind. Wire them in as defense-in-depth or delete them.

### 6. Redundant `crypto.Keccak256` on every cold code read + warmup prefetch
`db/state/execctx/domain_shared.go` (cold `CodeDomain` populate + `Commit` callback) and `execution/exec/blocks_read_ahead.go` (`cachePopulatingGetter`). Each recomputes `keccak(code)` over the full bytecode to key the content cache, although the codeHash is already in the just-decoded account record (the old path keyed by the cheap `maphash`, no keccak). Net-new crypto over (up to ~24 KB) bytecode per cold read / warmup for a value the design already has.

### 7. Duplicated `maxStep` gate + full account deserialize on the fast path
`db/state/execctx/domain_shared.go`. The stateCache gate (`cStep := kv.Step(cTxNum/StepSize()); cStep > maxStep`) and branchCache gate (`cStep := kv.Step(cStepU64)`, deliberately *not* divided) are two copies of "respect maxStep" that differ in a subtle divide/don't-divide way the comments admit caused a real bug — fold into one helper on the cache `Get`. Separately, `decodeAccountCodeHash` runs a full `accounts.DeserialiseV3` on every `codeHashForAddr` mem-hit just to extract the 32-byte codeHash, on the fast path it's meant to accelerate.

---

## Conventions

### 8. CLAUDE.md comment-policy violations (concentrated in `domain_shared.go`)
CLAUDE.md mandates "*One sentence; rarely two; never a paragraph … Free of forensic detail … Strip … PR/issue references, … incident anecdotes*"; `.claude/rules/comments.md` bars scope/limitation narration ("forward-only", "band-aid", "Deprecated:"). The diff adds many 6–12-line multi-section docstrings (e.g. `SetStateCache`, `Commit`, `Flush`, `GetCode`), issue refs (`#21752`, `#22116`), incident/branch artifacts (`ca5daf64`, a Hoodi block, "mainnet block 25151825 tx 31"), client name-drops (reth/geth/Nethermind/revm), and a self-described "band-aid" comment. Best fixed in-PR: trim to one or two sentences each; move the forensic/PR-history detail to the PR description (where the `73d0911` commit message already does it correctly).

---

## Latent note (not a confirmed bug)
Negative cache entries for missing accounts/storage are stamped `txNum = stepSize-1`, so they survive any unwind (`txNum < floor`). Every path traced corrects them via the commit-callback overwrite or unwind-resurrection-into-mem, so it isn't provably observable — but a defensive empty-value guard (or stamping the absence with the observation txNum) would remove the load-bearing reliance on that overwrite firing.

---

## Investigated & found sound (so they aren't re-litigated)
- **Unwind floor / off-by-one:** both unwind paths pass `Min(unwindPoint+1)` (first rolled-back txNum), matching the `IsStale: txNum >= floor` test.
- **`UnwindTo(>)` → `Unwind(>=)` boundary shift:** a *fix*, not a regression (old code retained the entry exactly at `txN==floor`).
- **Warmup vs unwind contamination** (the hazard `drainReadAhead`'s own doc describes): closed by the module semaphore (warmup fires only inside `ValidateChain`; the cache-bumping unwinds hold the same lock) + a `warmWg.Done()→Wait()` happens-before edge ordering all warmup Puts before the epoch bump. The non-drained `UnwindExecutionStage` paths run no cache-populating warmup.
- **Removed unsound codeHash bypass:** does **not** return — setters resolve `prevVal` via the addr-keyed `GetLatest`; only getters use the codeHash shortcut.
- **Removed per-write cache updates / `ValidateAndPrepare` / `ClearWithHash` / `RevertWithDiffset`:** correctly subsumed by mem-first masking + commit-gated population + epoch-based invalidation. `eth_simulateV1` neither pollutes nor reads the shared caches (direct `GetMemBatch`+`GetAsOf` readers).
- **Metrics collector lifecycle:** `Send`/`TrySend`/`Snapshot`/`Stop` are nil-safe and non-blocking post-`Stop`; `grouped` is single-owner. The unconditional-creation OOM was fixed in `73d0911`.

---

## Recommendation: enforce the lazy-unwind invariants in code, not convention

The `(epoch, floor)` lazy-unwind model itself is sound, but two of the things that keep it correct are maintained by *routing discipline* rather than *enforced*, and both have a wrong-state-root failure mode if a future change violates them. This is exactly the case the repo's own comment policy calls out:

> If a constraint really needs to be enforced for the codebase's safety, prefer **code that enforces it** (a runtime assert, a type the caller can't misuse, a single private constructor) over a comment that describes it. A `panic` survives refactors; a long comment doesn't.

The two load-bearing assumptions are the ones listed under *Investigated & found sound* above:

**A. Every cache-epoch-bumping unwind is fenced against an in-flight cache-populating warmup.** Holds today only because (a) the three engine paths call `drainReadAhead()` before `UnwindTo`, and (b) the stage-loop `UnwindExecutionStage` paths bump the epoch but run no cache-populating warmup (`BlocksReadAhead` there uses a plain `NewReaderV3`). Both are conventions a future change can silently break — e.g. wiring `cachePopulatingGetter` into pipeline warmup, or adding a new unwind caller that forgets to drain. Failure mode: a fire-and-forget warmup `Put` stamped with the *post-unwind* epoch laundering a dead-fork value as live → served as canonical → wrong root. The single chokepoint where the epoch is actually bumped is `SharedDomains.Unwind` (all paths funnel through it), but the "is a warmup in flight" signal lives on `BlockReadAheader.warming`, which the SD doesn't own — so a literal assert there needs a little plumbing. Two clean options:
- **Adopt the drain-free getter already tracked in #22116**: have the warmup getter capture the epoch at read-start and skip/stamp-through a concurrent bump, so a late write *cannot* launder a dead-fork value by construction — this removes the assumption rather than asserting it (preferred).
- Or funnel every unwind through one method that drains unconditionally (instead of three callers each remembering), and `assert` no warmup is in flight at that point, so a new unwind caller can't bypass the fence.

**B. Every fork-validation that could read a stale shared branch performs an epoch-bumping `sd.Unwind` first.** This is what makes the dead `DetachBranchCache()` (finding 5) safe to leave uncalled — but the guard the method *advertises* isn't actually wired. Enforce by either wiring `DetachBranchCache()` into the fork-validation SD setup as defense-in-depth (cheap, and makes the isolation explicit instead of implicit), or deleting it and replacing the implicit guarantee with a typed fork-validation SD that structurally cannot see the shared cache.

**Durable fix for both** is the direction the PR description already names: make app-level domains first-class typed objects that *own* their caches (the `add_execution_context_with_caches` POC), so ownership + coherence are enforced by the type system rather than by remembering to drain/detach. Until then, a runtime assert at the single unwind entry point is the cheapest way to convert "correct by convention" into "fails loud if violated."

cc @mh0lt

---

## Addendum (2026-07-02) — from the #22146 review

Four findings folded in from a code review of #22146 (*read-ahead warmup must not clobber fresher StateCache entries*), reviewed at head `e7cbf189`. All four are **pre-existing relative to that PR** (none should gate it); items 9–10 are correctness, 11–12 cleanup. Line numbers are #22146-branch post-image.

### 9. (med) Embedded-rpcdaemon read-fill is a second unconditional-`Put` snapshot writer — same clobber shape #22146 fixes for the warmup
`db/state/execctx/domain_shared.go` (`getLatestMetered` read-fill, ~:1234) + `execution/execmodule/exec_module.go` (`CacheView.Get` → `AsGetter(c.tx)`, ~:152). An embedded eth_call at `latest` reads with the RPC caller's own roTx — outside the engine semaphore and outside `drainReadAhead` — and a cache miss lands in the unconditional `sd.stateCache.Put(domain, k, v, readTxNum)` with the step-upper-bound stamp. A read straddling an FCU commit can therefore overwrite the flush's fresh value with the pre-flush one (wrong root on the next block, same mechanism as the warmup bug). The verified window is much narrower than the warmup's: `TemporalMemBatch.Flush` doesn't drain `mem`, so a live pre-commit View sees the new value mem-first and never Puts; what remains is a µs-scale straddle (miss → DB-read old → flush-apply new → Put old) or a wider path via LRU eviction/`Delete` plus a post-`bgSD.Close()` View, which I did not line-verify. The #22146 generalization applies directly: read-fills never carry newer information than a flush-apply, so the read-fill `Put` can become `PutIfAbsent` — near-free, and closes the writer class rather than the one call site. (Also relates to the drain-free getter tracked in #22116.)

### 10. (med-low, narrow window) Startup gap: pre-`Start` warmup entries survive `ProcessFrozenBlocks` uninvalidated
`execution/execmodule/exec_module.go` (`Start` → `ProcessFrozenBlocks`, ~:675→:683). Engine servers are live before `go s.execModule.Start()` (node/eth/backend.go:1437; the Stop-side `WaitForWarmup` only gates `chainDB.Close`), and `ValidateChain` fires `AddHeaderAndBody` (:489/:502) *before* its too-far-away check (:516) — so a payload validated in the pre-Start window warms the cache with pre-catchup state, nil negatives included. PFB's SharedDomains never get `SetStateCache`, and PFB neither `Clear()`s nor epoch-bumps, so those entries stay live through the whole catch-up and are served cache-before-aggTx afterwards → stale value → wrong root until restart. Upstream gates (ACCEPTED at diff ≥ `maxReorgDepth`=96, downloader gap rejection, Busy once PFB holds the semaphore) bound this to a near-head payload in the ms-scale pre-Start window with frozen blocks pending — narrow, but nothing enforces it. This doesn't contradict the semaphore fence under *Investigated & found sound* (nothing races an epoch bump; the cache is simply never told about PFB's writes — a third fence variant beyond invariant A's two). Cheap close: `drainReadAhead()` + `stateCache.Clear()` (or an epoch bump) in `Start` after the semaphore acquire, before PFB.

### 11. (low) `currentSize` double-subtract: `Delete`/stale-drop/eviction bypass the #22146 put stripes
`execution/cache/generic_cache.go` — `put`'s update-in-place delta `currentSize.Add(newSize-existing.size)` (~:234) and collision subtract (~:264) run under a stripe, but `Delete` (~:273) and `GetWithTxNum`'s stale-drop `Remove` (~:189) don't, and freelru fires the size-subtracting `OnEvict` (~:125) on both `Remove` and capacity eviction — so a removal interleaved between `put`'s `data.Get` and `data.Add` double-subtracts `existing.size`. Pre-existing: the pre-stripe `Put` had the identical unlocked window since #21386, and #22146's stripes strictly narrow it. Default `ModeEvictLRU` never consults `currentSize` for admission (stats-only skew); `ModeNoOp` (diagnostic) loosens the byte check but the entry cap still bounds. Tidy-up: have `Delete` and the stale-drop take the same stripe.

### 12. (low — extends finding 6) Warm-path prefetch copy/keccak now discarded by the if-absent no-op
`execution/cache/state_cache.go` (`PutIfAbsent` ~:257, `PutCodeWithHashIfAbsent` ~:176) + `execution/exec/blocks_read_ahead.go` (~:102). With #22146, a live entry makes the conditional put a no-op — but by then the `common.Copy` (and for code the caller-side `Keccak256`, the same cost finding 6 flags) has already run, so the steady-state warm prefetch allocates-and-discards: thousands of ≤100 B copies plus ~1–3 MB/block of code hash+copy, low-single-digit ms across the background workers. A `GetWithTxNum` liveness pre-check before the copy/keccak is safe as a pure optimization (`PutIfAbsent` still decides under its lock) and reduces warm-case lock traffic.