feat: head state cache for fast RPC reads on top of head#12150
feat: head state cache for fast RPC reads on top of head#12150LukaszRozmej wants to merge 3 commits into
Conversation
Adds an opt-in, cross-block coherent cache of state at the canonical head (and the last few ancestors) to accelerate read-only RPC calls (eth_call, debug_trace*, trace_*) that execute transactions on top of the tip. Account and storage reads on the RPC tx-processing path become O(1) lookups instead of trie traversals. The cache is kept coherent with the head by applying each block's changed keys (refreshed values), and flushes on reorg. The per-block changed-storage set is captured from the processing world state's write batches (works on any node, independent of EIP-7928), with the consensus/generated Block Access List as fallbacks; changed accounts come from Block.AccountChanges. Disabled by default (Blocks.EnableHeadStateCache). RPC-only: block processing is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@claude review |
|
Claude finished @LukaszRozmej's task in 6m 21s —— View job Review: head state cache for fast RPC readsNice, well-documented design — the generation-seqlock + per-block changed-key ring is a clean way to get cross-block coherence, and confining it to the shareable RPC source keeps the blast radius small. I focused on correctness/concurrency, the processing hot path, and the coherence invariants. Findings are inline; summary below. 🔴 Critical
🟠 High
🟡 Medium
🔵 Low
Notes
Verdict: not mergeable as-is — the Critical (pooled-buffer race) and High (backfill coherence) should be fixed first, and the Medium findings addressed or explicitly acknowledged. |
github-actions
Bot
added
configuration
new feature
performance is good
state+storage
labels
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| private static FrozenSet<AddressAsKey> ToAccountSet(ArrayPoolList<AddressAsKey> addresses) | ||
| { | ||
| HashSet<AddressAsKey> set = new(addresses.Count); | ||
| foreach (AddressAsKey address in addresses.AsSpan()) set.Add(address); | ||
| return set.ToFrozenSet(); | ||
| } |
There was a problem hiding this comment.
Critical — shared ownership / data race on the pooled block.AccountChanges.
block.AccountChanges is an ArrayPoolList<AddressAsKey> that the TxPool already owns and disposes. TxPool.OnHeadChange enqueues the same BlockReplacementEventArgs to _headBlocksChannel, and ProcessNewHeadLoop (running on a separate task thread) calls block.DisposeAccountChanges() — which returns the backing array to ArrayPool and nulls the field — at TxPool.cs:290 (and synchronously at :240 on the syncing path).
Both TxPool (via ChainHeadInfoProvider.OnHeadChanged) and this updater subscribe to the same IBlockTree.BlockAddedToMain. There is no coordination on the pooled buffer's lifetime, so ToAccountSet(accountChanges) here can iterate AsSpan() over an array that is concurrently being returned to the pool (and potentially re-rented/mutated elsewhere). That is a use-after-free of pooled memory: it can cache/refresh arbitrary addresses (silent state corruption) or fault.
This fires on essentially every canonical head advance when the cache is enabled. It needs an independent source of changed accounts (e.g. copy them out under guaranteed-alive ownership, or derive accounts from the journal-captured delta the same way storage is) rather than borrowing TxPool's pooled list.
| // Miss (or raced an advance): load at the pinned header root and backfill while still current. | ||
| Account? loaded = baseScope.Get(address); | ||
| if (snapshot.IsCurrent) | ||
| { | ||
| _accounts.Set(in key, loaded); | ||
| } | ||
| return loaded; |
There was a problem hiding this comment.
High — backfill is not fenced against Advance; a stale value can be published into the head-coherent cache and persist.
The IsCurrent check and the _accounts.Set are not atomic w.r.t. HeadStateCache.Advance:
reader: updater (Advance):
loaded = baseScope.Get(addr) // value @ old head N
if (snapshot.IsCurrent) // true, gen==N
gen -> N+1 (odd)
refresh(addr) -> writes value @ N+1
gen -> N+2 (even)
_accounts.Set(addr, loaded) // overwrites with stale value @ N
The whole point of the generation seqlock is logical coherence, but the backfill write can land after Advance has already refreshed that key to the new head value, leaving a stale entry that is then served to all subsequent head readers until the key changes again or is evicted. The race window is the (potentially disk-bound) base trie read, and one lost race poisons the entry indefinitely. Same pattern in CachingStorageTree.Get (lines 99–104).
A re-check of snapshot.IsCurrent after Set cannot un-publish the write — the cache needs a write path that is atomic against the generation (or Advance must invalidate concurrently-backfilling entries).
| public void Commit(ulong blockNumber) | ||
| { | ||
| baseScope.Commit(blockNumber); | ||
|
|
||
| // RootHash now reflects the committed state; the block's final commit stores under its | ||
| // final state root. Snapshot so later commits don't mutate the stored set. | ||
| buffer.Store(baseScope.RootHash, new HeadStateBlockDelta(_slots.ToFrozenSet(), _requiresFlush)); | ||
| } |
There was a problem hiding this comment.
Medium — hot-path cost on block processing, and the "block processing is unaffected" claim is inaccurate when the flag is on.
When the cache is enabled this decorator wraps the main processing world state (MainProcessingContext.cs:49), so every storage write goes through CaptureStorageWriteBatch.Set → HashSet<StorageCell>.Add, and every Commit builds a fresh _slots.ToFrozenSet().
Two concerns:
- Per-block
ToFrozenSetallocation/CPU on the processing path for every committed block (including during sync, where the head cache provides no value). - Because
_slotsis accumulated per scope (not reset per commit), a long sync scope spanning many blocks makes eachCommitsnapshot the ever-growing set → O(n²)ToFrozenSetwork and aHashSetthat grows towardMaxTrackedSlots(2^21).
The PR body states "block processing is unaffected" — that's only true with the flag off. Worth measuring the on-flag processing overhead and reflecting it in the description.
| // 1) Journal capture: works on any node, regardless of EIP-7928 / Block Access Lists. | ||
| if (block.AccountChanges is { } accountChanges | ||
| && block.Header.StateRoot is { } stateRoot | ||
| && _deltaBuffer is not null | ||
| && _deltaBuffer.TryGet(stateRoot, out HeadStateBlockDelta delta)) | ||
| { | ||
| if (delta.RequiresFlush) return false; // self-destruct: can't enumerate cleared slots | ||
| changedAccounts = ToAccountSet(accountChanges); | ||
| changedSlots = delta.ChangedSlots; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Medium — the journal-captured delta is trusted as authoritative even when it may be incomplete.
If a present delta is found it short-circuits the BAL fallbacks. Coherence at head-X depends on the recorded changed-slot set being complete: any slot whose head value differs from its head-X value must appear in the window, otherwise ChangedInWindow returns false and a stale value gets served/backfilled.
The capture only records slots that flow through IStorageWriteBatch.Set on the decorated processing scope. The PR explicitly lists "coverage for non-standard processing paths (AuRa/Optimism/Taiko)" and parallel BAL execution as not-yet-verified. If any of those write paths (e.g. the Flat/BAL parallel state) bypass the intercepted write batch, the stored delta is silently under-inclusive and is preferred over the (complete) BAL — producing wrong RPC reads rather than just falling back. Please confirm every canonical processing path routes storage writes through this batch, or add an assertion/guard.
Related: this relies on block.AccountChanges including accounts that changed only storage (so their moved storage root is reflected). Worth documenting/asserting that invariant.
| public void Store(Hash256 stateRoot, HeadStateBlockDelta delta) | ||
| { | ||
| lock (_lock) | ||
| { | ||
| if (!_byRoot.ContainsKey(stateRoot)) | ||
| { | ||
| _order.Enqueue(stateRoot); | ||
| while (_order.Count > capacity) | ||
| { | ||
| _byRoot.Remove(_order.Dequeue()); | ||
| } | ||
| } | ||
| _byRoot[stateRoot] = delta; | ||
| } | ||
| } |
There was a problem hiding this comment.
Low/Medium — keyed solely by post-state root: distinct blocks sharing a state root collide.
Two blocks with different parents (or a side branch) that reach the same post-state root will share a key here; Store overwrites, so the updater may apply the wrong block's changed-slot set on a sequential advance. The state being identical does not imply the changed-relative-to-parent set is identical, so this can produce an under-inclusive delta → stale head-X reads. Rare, but a silent coherency hole. Consider keying by (parentRoot, stateRoot) or block hash, or at least documenting the assumption that post-state roots are unique within the buffer window.
| builder | ||
| .AddSingleton(new HeadStateCache( | ||
| blocksConfig.HeadStateCacheDepth, | ||
| blocksConfig.HeadStateCacheAccountSetsBits, | ||
| blocksConfig.HeadStateCacheStorageSetsBits)) |
There was a problem hiding this comment.
Low — unvalidated config can fail node startup.
HeadStateCacheAccountSetsBits/HeadStateCacheStorageSetsBits outside [1, 20] (and HeadStateCacheDepth < 0) throw ArgumentOutOfRangeException from the SeqlockCache/HeadStateCache constructors, which would abort startup with an opaque error rather than a config-validation message. Since these are user-facing (if hidden) config items, consider clamping or validating with a clear message.
|
Claude finished @LukaszRozmej's task in 6m 54s —— View job Review: head state cache for fast RPC readsNice, self-contained, opt-in design with good XML docs and a sensible coherence model (head-coherent caches + per-block changed-key window + generation seqlock). My review focused on the concurrency/coherence guarantees and the hot-path impact. Findings below; I left inline comments on the specific lines. 🔴 High
🟡 Medium
🟢 Low / notes
Verdict: the two High findings concern the core concurrency correctness of the feature, so I'd resolve or explicitly accept them before merge. Since the feature is opt-in and off by default, production blast radius is limited, but the coherence hole (#1) means it can't yet be safely recommended to enable. |
| HeadStateDeltaBuffer? headStateDeltaBuffer = rootLifetimeScope.ResolveOptional<HeadStateDeltaBuffer>(); | ||
| if (headStateDeltaBuffer is not null) | ||
| { | ||
| worldState = new HeadStateDeltaCaptureScopeProvider(worldState, headStateDeltaBuffer); | ||
| } |
There was a problem hiding this comment.
Medium — when the flag is on, block processing is affected (contrary to the PR description).
The PR states "block processing is unaffected" / "only the shareable tx-processing source is decorated", but HeadStateDeltaCaptureScopeProvider wraps the main processing GlobalWorldState here. With the feature enabled, every storage write on the processing hot path now goes through CaptureStorageWriteBatch.Set → allocates a StorageCell and does a HashSet<StorageCell>.Add, and every block Commit does a _slots.ToFrozenSet() (an allocation proportional to the number of cells the block touched). For a dense block this is non-trivial work/allocation on the critical block-processing path.
This is acceptable as an opt-in/off-by-default experimental cost, but the description should be corrected and the per-block capture overhead measured. Worth a benchmark with the flag on.
| private void OnBlockAddedToMain(object? sender, BlockReplacementEventArgs e) | ||
| { | ||
| Block block = e.Block; | ||
| if (block.Hash is null) return; | ||
|
|
||
| lock (_lock) | ||
| { | ||
| try | ||
| { | ||
| bool sequential = e.PreviousBlock is null | ||
| && _cache.HeadHash is not null | ||
| && _cache.HeadHash.Equals(block.ParentHash); | ||
|
|
||
| if (!sequential || !TryCollectChangedKeys(block, out FrozenSet<AddressAsKey> changedAccounts, out FrozenSet<StorageCell> changedSlots)) | ||
| { | ||
| _cache.Flush(block.Hash); | ||
| return; | ||
| } | ||
|
|
||
| _cache.Advance(block.Hash, changedAccounts, changedSlots, new Refresher(_stateReader, block.Header)); |
There was a problem hiding this comment.
Medium (perf) — the head refresh runs synchronously on the BlockAddedToMain handler (main processing thread).
OnBlockAddedToMain → Advance re-reads, via IStateReader (trie traversal), every changed key that is currently cached — and changedAccounts comes from Block.AccountChanges, which is over-inclusive (it contains every account touched, read or written, not just mutated — see StateProvider.ChangedAddresses() returning all _blockChanges.Keys). For a busy block with a warm cache this can be many trie reads, all on the block-processing thread, and all while the generation is odd (so concurrent RPC readers are degraded to passthrough for that window).
Consider doing the refresh off the processing thread, or bounding the number of keys refreshed per advance. At minimum, please measure the added per-block latency with the flag enabled.
(Also note: during fast/snap sync, non-executed blocks won't have a delta and will hit the Flush path on every BlockAddedToMain — wasteful but not incorrect.)
| // Miss (or raced an advance): load at the pinned header root and backfill while still current. | ||
| Account? loaded = baseScope.Get(address); | ||
| if (snapshot.IsCurrent) | ||
| { | ||
| _accounts.Set(in key, loaded); | ||
| } | ||
| return loaded; |
There was a problem hiding this comment.
High — backfill can publish a stale value for a key the new head changed (coherence hole).
The if (snapshot.IsCurrent) check and the subsequent _accounts.Set(...) are not atomic with respect to HeadStateCache.Advance. Consider:
- Reader pinned at head
H0(depth 0, soChangedInWindowis alwaysfalse).TryGetValuemisses,loaded = baseScope.Get(address)returns theH0valuev0. - Reader evaluates
snapshot.IsCurrent→true(gen still evenG). - Reader thread is preempted.
Advance(H0→H1)runs to completion: the block changed this account, but it is not currently cached, soAdvanceskips it (it only refreshes already-cached changed keys, relying on backfill). Gen ends even atG+2, head is nowH1. - Reader resumes and executes
_accounts.Set(key, v0)— publishing the staleH0value. - A new depth-0 reader pinned at
H1(snapshot.Generation == G+2) doesTryGetValue→ hit → returnsv0. Wrong. At depth 0 there is no window to filter it out, and nothing re-refreshes it until the key is changed again or evicted, so the stale value can persist for many blocks.
The whole scheme assumes a backfilled value always equals the current head value, but a backfill that lands just after an Advance for a key that block changed violates that. Because Advance deliberately does not refresh uncached changed keys, backfill is the only thing that populates them — and it is racy.
A fix needs the backfill to be validated against the generation atomically (e.g. re-check generation after Set and evict/clear the entry if it moved, or stamp entries with the generation they were written under and reject on read when it differs), rather than the current check-then-act. At minimum this should be explicitly acknowledged given it undermines the core coherence guarantee.
| Interlocked.Increment(ref _generation); // -> odd: readers pass through while we mutate | ||
|
|
||
| // Refresh only changed keys that are currently cached; uncached keys get the fresh head value | ||
| // lazily on the next read (backfill), so there is no stale entry to fix. | ||
| foreach (AddressAsKey address in accountSet) | ||
| { | ||
| if (_accounts.TryGetValue(in address, out _)) | ||
| { | ||
| _accounts.Set(in address, refresh.GetAccount(address.Value)); | ||
| } | ||
| } | ||
| foreach (StorageCell cell in slotSet) | ||
| { | ||
| StorageCell key = cell; | ||
| if (_storage.TryGetValue(in key, out _)) | ||
| { | ||
| _storage.Set(in key, refresh.GetStorage(in key)); | ||
| } | ||
| } | ||
|
|
||
| ShiftRings(newHeadHash, accountSet, slotSet); | ||
|
|
||
| Interlocked.Increment(ref _generation); // -> even: publish the new head | ||
| } |
There was a problem hiding this comment.
High — an exception inside Advance leaves the generation seqlock permanently odd, and the recovery Flush corrupts parity further.
Advance increments _generation to odd (line 132), then calls refresh.GetAccount / refresh.GetStorage, which do trie reads via IStateReader and can throw (e.g. MissingTrieNode/TrieException under pruning). If that happens, the second increment (line 154) never runs, so _generation stays odd.
The caller's recovery is HeadStateCacheUpdater.OnBlockAddedToMain's catch → _cache.Flush(...). But Flush assumes it's entered while even: it does Increment (odd→even here), Clear(), Increment (even→odd here). So:
- the
Clear()runs while the generation is even, meaning a concurrent reader canTrySnapshotmid-flush and observe a half-cleared cache, and Flushexits odd, soTrySnapshotpermanently returnsfalseand the cache silently degrades to passthrough until restart.
Recommend making Advance exception-safe (wrap the mutation in try { ... } finally { Interlocked.Increment(ref _generation); } so it always returns to even) and/or making Flush force the generation to a known-even value rather than blind-incrementing.
- Fix data race / use-after-free: stop reading the pooled Block.AccountChanges (TxPool owns and disposes it concurrently). Capture changed accounts in the journal delta via the write batch instead — precise (mutated, not merely touched) and independently owned. - Fix backfill-vs-Advance race: serialize cache writes (backfill, Advance, Flush) under a write lock; backfill is generation-checked and non-blocking (TryEnter), so a value loaded at the old head is dropped, never published stale. Reads stay lock-free. - Make Advance exception-safe: gather refreshed values before touching the generation, and only flip generation in a no-throw phase under try/finally, so a trie read failure can't leave the seqlock stuck odd. Flush is parity-safe. - Capture changed storage cells thread-safely (parallel storage-root workers): per-batch buffering + locked merge. - Key the delta buffer by (blockNumber, stateRoot) to reduce collisions. - Clamp head-cache config to valid ranges so misconfig can't abort startup. - Tests: parallel-capture thread-safety; updater journal-delta advance/flush. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review — addressed in 🔴 Critical — pooled 🟠 High — backfill vs 🟠 High — exception in 🟡 Medium — buffer keyed only by state root: now keyed by 🟡 Medium — journal trusted even if incomplete: the capture decorator is only installed on the main processing context, whose authoritative final commit routes every storage write through the intercepted 🔵 Low — unvalidated config: clamped to valid ranges in the module. 🟡 Medium — "block processing is unaffected" is inaccurate with the flag on: correct — when enabled, the capture decorator adds a Also fixed the original cause of your run-time crash (the Deliberately deferred (follow-ups, noted in the PR): moving the synchronous head refresh off the block-processing thread; benchmarking the on-flag capture overhead (incl. the O(n²) accumulation across a long sync scope, currently bounded by the cap→flush); extending the cache to the direct |
Follow-ups from review: - Move the head-cache refresh off the BlockAddedToMain thread onto a single background worker (bounded queue; dropped events self-heal via the parent-mismatch flush), so the trie-bound refresh no longer delays block processing or other event subscribers. - Capture the per-block changed set per block (per-commit buffering folded into a per-block set, reset on block-number change) instead of accumulating across a whole processing scope — removes the O(n²) snapshot cost on long sync branches and makes the delta per-block-accurate. - Extend the cache to the direct IStateReader RPCs (eth_getBalance, eth_getStorageAt, …) via HeadCachedStateReader decorating the shared reader. The updater is wired with the raw GlobalStateReader to avoid a refresh-reads-the-cache cycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Didn't provide desired improvements |
1 participant
Changes
Adds an opt-in, cross-block coherent cache of state at the canonical head (and the last few ancestors, "head-X") to speed up read-only RPC calls on top of the tip — both the execution-path calls (
eth_call,eth_estimateGas,trace_*,debug_trace*) and the direct state reads (eth_getBalance,eth_getStorageAt,eth_getTransactionCount, …).Today the RPC read paths walk the trie on every call (served only by the trie-node cache + RocksDB block cache); there is no direct
address→Account/(address,slot)→valuecache for RPC. This PR adds one, kept coherent across blocks by applying each block's changed keys.HeadStateCache— twoSeqlockCaches (accounts + storage) coherent at head, a head-hash ring + per-block changed-key ring for head-X. Writes (backfill / advance / flush) are serialized under a write lock; reads stay lock-free and are guarded by a generation seqlock (fall back to the trie if the head moves mid-call).HeadStateCacheScopeProvider— decorates the read-only world state for the execution-path RPCs: O(1) head lookups (generation-guarded backfill on miss), head-X served for keys unchanged in the window (changed keys deferred to the trie at the head-X root), pure passthrough otherwise.HeadCachedStateReader— decorates the sharedIStateReadersoeth_getBalance/eth_getStorageAt/… hit the cache for head/head-X reads.HeadStateCacheUpdater— applies head changes on a single background worker (off theBlockAddedToMainthread): refreshes the keys a block changed, flushes on reorg/gap/self-destruct. Bounded queue; a dropped event self-heals via the next non-sequential flush.HeadStateDeltaCaptureScopeProvider+HeadStateDeltaBuffer— capture each block's changed accounts and storage cells from the processing world state's write batches (works on any node, independent of EIP-7928), keyed by(blockNumber, stateRoot). The consensus / generated Block Access List are fallbacks when no capture is present.IBlocksConfig):EnableHeadStateCache(default false),HeadStateCacheDepth(default 2), cache sizing (clamped to valid ranges).Scope / cost: when the flag is on, the capture decorator wraps the main processing world state, so each storage write does a
HashSet.Addand each block commit a per-blockToFrozenSet— non-trivial but bounded per block. With the flag off (default) there is zero processing overhead. Reads on the RPC path become O(1) hash lookups instead of trie traversals.Types of changes
Testing
Requires testing
If yes, did you write tests?
Notes on testing
HeadStateCacheTests(State.Test): decorated reads at depths 0..X match undecorated trie reads (changed + unchanged keys, hit + backfill); reorg-flush; passthrough; per-block capture keyed by(number, root); self-destruct → flush; parallel-write capture thread-safety (mirrors the parallel storage-root workers).HeadStateCacheUpdaterTests(Blockchain.Test): journal-delta advance refreshes cached account/slot off-thread; self-destruct flush; non-sequential flush.Documentation
Requires documentation update
Config reference for the new
Blocks.EnableHeadStateCacheoptions (once defaults are finalized).Requires explanation in Release Notes
Remarks
Opt-in, default off. Concurrency model (review-hardened): a write lock serializes cache writes so a backfill that races an
Advanceis dropped rather than published stale;Advanceis exception-safe (generation only flips in a no-throw phase); the updater never reads the pooledBlock.AccountChanges(TxPool owns/disposes it) — changed accounts come from the journal capture instead.Remaining follow-ups: hit/miss metrics for observability; coverage for non-standard processing paths (AuRa/Optimism/Taiko — currently they produce no capture delta and fall back to BAL/flush, so they're correct but not journal-accelerated); on-flag processing-overhead benchmark. A documented future phase is reusing the same coherent-delta mechanism to speed up consecutive-block processing (deferred until the cache is proven, since a stale value there is an invalid block, not just a bad RPC response).
🤖 Generated with Claude Code