db, execution: harden StateCache read-fills against stale overwrites and unwinds - #22467
Conversation
…r per-key bounds, stamp negatives with progress Split from #22159 (StateCache review findings #22120). - Read-fills use if-absent semantics: a read-fill never carries newer information than a flush-apply, so it must not overwrite a live authoritative entry (e.g. an embedded-RPC read straddling an FCU commit). Code fills go through PutCodeWithHashIfAbsent, keyed by keccak(v). - Negative results (missing account, empty slot) are stamped with the domain's progress at observation time instead of a synthetic step-zero bound, so any unwind drops them instead of letting them outlive the fact. - The ASSERT_STATE_CACHE divergence assert is skipped while the mem overlay bounds the key (in-flight unwind): MDBX still holds the not-yet-deleted dying row inside the bound, so the authoritative read can return dead-fork bytes and blame the cache for a legitimate hit. - The state-cache and branch-cache maxStep gates share servableUnderBound, with their different units explicit per call site. - Dead DetachBranchCache is deleted: it advertised a fork-validation guard that was never wired; the actual mechanism is the epoch-bumping sd.Unwind.
There was a problem hiding this comment.
Pull request overview
Updates SharedDomains read-path behavior to make StateCache read-fills non-authoritative (no overwrites), unify per-key bound gating, and ensure negative cache entries can be invalidated by unwinds; adds targeted tests and microbenchmarks for these behaviors.
Changes:
- Switch SD read-fills to if-absent cache population and gate
ASSERT_STATE_CACHEdivergence checks under per-key bounds. - Unify maxStep bound gating via
servableUnderBound, with explicit unit conversion at call sites. - Stamp negative cache entries with domain progress and add tests/benchmarks covering the new semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| db/state/execctx/domain_shared.go | Read-path cache semantics: bound gate helper, assert gating, if-absent fills, negative stamping, and removal of DetachBranchCache. |
| db/state/execctx/statecache_readfill_test.go | New tests covering: no false assert during in-flight unwind, read-fill non-clobbering, and negative stamping/unwind invalidation. |
| db/state/execctx/statecache_readfill_bench_test.go | New benchmarks measuring domain progress stamping and cold-negative read costs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A total-miss negative carries no step, so the step-derived stamp pinned it near txNum 0 where no unwind could drop it. Stamp it with the domain's progress at observation time, as the SD read-fill does — covering the second fill site into the process-global StateCache.
yperbasis
changed the title
yperbasis
changed the title
…at step 0 A delete-shape unwind entry at step 0 was indistinguishable from a plain miss, so no bound was published and the ASSERT_STATE_CACHE guard compared a correct cached negative against the not-yet-deleted dying row. Plain misses now return kv.NoStepBound, letting a step-0 bound through — a young chain's whole state lives in step 0.
At ~20-30 ms each they are not long-running, and they carry the PR's core behavioral coverage.
yperbasis
changed the title
…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.
|
Flagging an overlap with #21414 (FCU semaphore decouple + bg-commit) for merge-order awareness — the two are independent, but edit the same read path. Same code. Both rewrite Same root. Finding 9's "embedded-RPC read straddling an FCU commit" is the publish→commit window #21414 routes around via the published SD — complementary layers (this PR = cache integrity; #21414 = reader sees authoritative in-flight state). #21414 will merge with bg-commit default OFF, so it does not ship the increased concurrency; adopting this PR's if-absent fill + NoStepBound on #21414's SD read path is deferred to the later default-flip PR, which will take #22466 → #22467 as prerequisites. Noting here so the rebase is expected. |
mh0lt
left a comment
mh0lt
left a comment
There was a problem hiding this comment.
Approving on the merits — the read-fill hardening (if-absent fills, progress-stamped negatives, NoStepBound sentinel, servableUnderBound gate, finding-4 assert guard) is sound and well-tested.
Two merge-order dependencies to note:
- Depends on #22466 (GenericCache fences): the if-absent read-fill is only race-safe once grow/Clear swaps are fenced — a mid-resize
PutIfAbsentcan otherwise install a stale snapshot value as live. Land #22466 first. - Conflicts with #21414 in
domain_shared.go getLatestMetered(multi-level parent-chain walk vsNoStepBound/servableUnderBound/PutIfAbsent). Whichever lands second re-merges; #21414 is being held until #22466, so the expected order is #22466 → #22467 → re-merge #21414.
|
Small non-blocking follow-up: the flush-apply doc at |
Resolves a semantic conflict with #22467, which deleted SharedDomains.DetachBranchCache on main: drop the getProof call site. Isolation from concurrent commits now comes from the shared branch cache's bound gating (servableUnderBound) instead of detaching; TestGetProofPinsReadSnapshot pins that the proof still resolves on the caller's RO snapshot.
Split from #22159 (the #22120 StateCache review findings): the
SharedDomainsread-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.
getLatestMeteredpopulates 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
LastKeycost 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_CACHEpanic during in-flight unwinds. The divergence assert runs only when the mem overlay publishes no per-keymaxStepbound: 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 returnskv.NoStepBoundinstead 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
DetachBranchCachedeleted. It advertised a fork-validation guard that was never wired; the actual isolation mechanism is the epoch-bumpingsd.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 thekv.NoStepBoundsignal)TestReadFill_DoesNotClobberLiveEntry(the fall-through read serves the maxStep-bounded row without replacing the live entry)TestReadFill_NegativeStampedWithProgressTestCachePopulatingGetterNegativeDroppedByUnwind(the warmBody fill site)BenchmarkDomainProgress/BenchmarkGetLatestColdNegativequantify the negative-stamp costThe read-fill regressions run in the short suite (no
testing.Shortguards) — 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, andexecution/execsuites, repeated cleanmake lint.Notes
maxStepfill-skip proposed there composes with this PR.PutIfFresh); merge order needs coordinating — whichever lands second carries a rebase that is mechanical except for preserving the negative progress-stamp through thePutIfAbsent→PutIfFreshswitch. Details in execution, db: bind StateCache fills to transaction views and reject stale fills #22444's "Interplay with db, execution: harden StateCache read-fills against stale overwrites and unwinds #22467" section.