feat(debug-trace-server): bounded in-memory BlockData cache keyed by block hash

[flyq]

Summary

Adds a bounded memory tier between the HTTP response cache and the DB / RPC tiers so requests that miss the response cache — other tracer variants of a block, and especially debug_traceTransaction calls, which are never response-cached — reuse one witness fetch and contract resolution instead of repeating it per request. Stacked on #161; retarget to main after it merges.

Motivation

3.6 days of production logs show 73% of tx-driven block fetches (9,955 of 13,716) re-fetched a block already resolved moments earlier for a different transaction of the same block, plus ~345k repeated DB reads (redb read + witness decode each) for recent blocks; repeat-same-tx is ~0.4%, so a tx-level response cache would absorb nothing — the reusable unit is the block's resolved BlockData.

Design

New BlockDataCache (bin/debug-trace-server/src/block_data_cache.rs) backed by quick_cache, keyed by block hash: entries are immutable facts about that hash, and by-number lookups resolve number → hash first (via #161's window → memo → upstream chain), so canonicality is never cached and no reorg invalidation is needed. Lookup order is memory → DB → RPC; DB hits are written back, and the RPC-path insert runs inside the shared single-flight future so each fetch inserts exactly once even if the primary caller is cancelled. Memory hits record their block-distance sample from a tip memoized on DB-backed serves, so a pure cache hit opens no redb transaction.

Entry weights are computed once at insert (block_data_weight) and stored with the entry, so the weigher quick_cache re-invokes under its shard locks only reads a number; the weight charges the witness KV/level payload (stateless_common::witness_size::light_witness_memory_bytes, per-KV constant now size_of-derived so a salt layout change propagates), contract bytecodes (via the shared stateless_db::bytecode_weight formula), and per-transaction envelope overhead plus calldata, access lists, and EIP-7702 authorization lists. The cache pins 4 shards so the admission ceiling for a single entry is max_bytes / 4 on every host (quick_cache's default shard count scales with the core count, which would shrink the largest admissible block on big machines); startup logs the effective per-shard budget, and an insert the cache does not retain is counted (cache_admission_rejects_total) and logged instead of masquerading as a miss.

Trace failures are typed (TraceError in tracing_executor.rs) and cache hygiene keys off the discriminant: data-attributable failures — a witness that cannot replay the block; run_transaction and prestate-frame failures now abort the trace instead of degrading into cacheable TraceResult::Error entries — evict the block's entry and are counted per method (block_data_evictions_total), while request-attributable failures (mux/JS config parse, inspector construction, tracer output) never evict, so a client cannot drop hot entries with a cheap invalid request. A visible behavior change rides on this: debug_traceBlock* on a block whose witness cannot replay now returns -32000 instead of 200 with per-tx error entries — consistent with parity trace_block and geth's abort behavior, and it keeps the response cache from storing poisoned callTracer responses.

New knob --block-data-cache-max-size / DEBUG_TRACE_SERVER_BLOCK_DATA_CACHE_MAX_SIZE (default 1GB, "0" disables). debug_getCacheStatus always reports both responseCache and blockDataCache sections, and the cache exports hits/misses/entries/bytes/admission-rejects under type="block_data" plus a memory data-source label. Combined default memory across the response, block-data, and contract caches is ~2.5 GB, now stated in the README.

Testing

Unit tests on real fixture data: weigher terms including exact access-list/authorization arithmetic, hit/miss/stats counters, deterministic single-shard budget eviction (exactly two of three same-weight entries retained — the earlier form passed on a cache that admitted nothing), oversized-entry rejection counting, DB write-back with Arc::ptr_eq sharing plus a memoized-tip assertion (the memory-served read performs zero tip reads), memory short-circuit ahead of a hanging upstream, disabled-cache DB re-reads, executor-level Data/Request error discrimination, handler-level evicts-only-on-data-errors, both debug_getCacheStatus JSON shapes, and flag default/env/disable parsing. Full suite: 99 bin tests + 39 stateless-common + 17 stateless-db, cargo fmt --check / clippy --workspace --all-targets --all-features / cargo sort --check all clean.

Notes

Deployment is config-free by default (cache on at 1GB); operators can raise, lower, or disable via the env var. Known follow-up: a deterministically bad witness still costs an evict → refetch → fail cycle per request (visible via the eviction counter); negative caching with backoff is deferred. Merge flow per repo convention: after #161 merges, retarget this PR to main, "Update branch" (merge, no rebase), then squash-merge.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fadd97b4c3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Account for variable transaction fields in cache weights

For blocks containing EIP-2930/1559 access lists or EIP-7702 authorization lists, this charges only a fixed 512 bytes plus calldata even though those lists own variable-size heap allocations. Because quick_cache enforces the configured limit using this reported weight, blocks with large lists can make the supposedly bounded cache retain substantially more memory than --block-data-cache-max-size; include these variable transaction fields in the weight calculation.

AGENTS.md reference: AGENTS.md:L111-L111

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Evict fetched data when its witness fails execution

When an upstream returns a syntactically decodable but incomplete or incorrect witness, the fetch and contract-resolution stages can succeed and this inserts the data before any trace attempts to read state. The first trace then fails, but every later request hits the same cached witness instead of retrying the endpoint, so a transient bad response poisons that block until eviction or restart; only promote data after successful execution or remove the entry when execution fails.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 18a127ad74

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Evict entries for embedded block execution failures

When a decodable but incomplete witness is used with fourByteTracer, muxTracer, or a JS tracer, trace_block converts run_transaction failures into TraceResult::Error elements and still returns Ok (tracing_executor.rs:507-523, with equivalent handling in the other tracer branches). Consequently, this inspect_err never runs, the handler caches the error response, and the bad BlockData remains available to other tracer variants and transaction requests instead of being refetched. Fresh evidence in the final tree is that the added eviction observes only the outer result, not these embedded execution failures; propagate or inspect those failures before retaining the entry.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve valid cache entries on tracer-specific errors

When a client supplies an invalid muxTracer configuration or JavaScript tracer, compute_debug_trace_block can fail while constructing the inspector before reading any EVM state (tracing_executor.rs:529-537 and 595-608), but this unconditional callback evicts otherwise valid block data. Repeating such requests across blocks can force subsequent callers back through DB decoding or upstream witness and contract fetching, effectively allowing request-specific errors to defeat the cache; eviction should be limited to failures that actually indicate invalid block data or witness state.

Useful? React with 👍 / 👎.

[vincent-k2026]

Review

In one line: the skeleton is right — keying by block hash, never caching canonicality, inserting exactly once inside the single-flight future all check out. The problems are concentrated in two places: the "evict on execution failure" hook added in the last commit catches none of the cases it was written for while letting any client drop other people's cache entries, and the cache advertises a byte bound that can't be computed from the flag, isn't covered by its test, and depends on the host's CPU count.

What's genuinely good

1. Keying by hash is the right answer. An entry is an immutable fact about that hash, by-number lookups resolve number→hash first, and therefore reorgs need no invalidation hook at all — an orphaned hash's entry simply becomes unreachable and ages out. This is exactly the discipline the response cache in #161 should adopt; I've said so over there.
2. The "inserts exactly once even if the primary caller is cancelled" claim holds. I walked it: the insert sits inside the shared future after a successful fetch and is guarded by ? (data_provider.rs:641), so failures never insert; a coalesced waiter still drives it to completion if the primary is cancelled; if everyone cancels, the future is dropped and InFlightGuard unconditionally clears the in-flight entry, leaving nothing poisoned.
3. Three of the new tests actually discriminate: DB hit populating memory and the second read skipping the DB (with Arc::ptr_eq proving one allocation), a memory hit short-circuiting ahead of a hanging upstream, and the disabled-cache path re-reading the DB. Those fail if the feature breaks.

Blocking

• rpc_service.rs:491 — the eviction hook's discrimination signal is wrong in both directions.

The "bounded" claim has three holes

Details are in the inline threads; the summary:

1. The admission threshold is a function of the host's core count (block_data_cache.rs:138). quick_cache shards, an entry must fit in a single shard's budget, and the default shard count is available_parallelism() * 4. Measured on a 12-core box: one entry needs max_bytes >= 33x its own weight to be admitted at all. At the 1 GB default that's ~31 MB max cacheable block on 12 cores, ~6 MB on 64 cores, ~4 MB on 96 cores — more cores means a smaller admissible block, and a trace server runs on big machines. Rejected inserts are indistinguishable from ordinary misses.
2. The per-KV weight constant is a hand-copied approximation of an external crate's type layout (witness_size.rs:85). SALT_KV_BYTES is 103; size_of::<(SaltKey, Option<SaltValue>)>() measures 104. The 1-byte gap is noise — the point is that this PR promotes an observability-grade estimate into the enforcer of a memory bound, so a salt bump can silently make the cache over-retain with no signal.
3. Contract bytecode is charged twice (block_data_cache.rs:72). The Bytecode in BlockData and the one in ContractCache are the same refcounted allocation, so evicting a BlockData entry can free zero bytes. Conservative for RSS, but "1 GB" does not mean "1 GB of block data".

Why the tests didn't catch any of this

• eviction_respects_byte_budget is vacuous — measured len=0, weight=0 (see the inline thread).
• The access-list / authorization-list weight terms added in the last commit have zero coverage — the fixture block has 29 transactions with 0 access-list and 0 authorization-list items, so both new branches always take the map_or(0, ..) side.
• None of the five evict_block_data call sites has handler-level coverage; nothing asserts when eviction should and should not happen, which is why both directions of the bug survived.

Operational

• Default memory across the three caches is ~2.5 GB (response 1 GB + block-data 1 GB + contract 512 MB — ContractCache::new uses ContractCacheConfig::default()). The description says "+1GB RSS"; the combined footprint isn't documented anywhere.
• evict_block_data has no metric, so the amplification vector above would be invisible.
• --block-data-cache-max-size has no lower bound; combined with hole 1, a too-small value silently disables the cache while warn! only fires on exactly 0.
• BLOCK_DATA_CACHE_ESTIMATED_ITEMS is hard-coded at 1024 while max_bytes is configurable — inconsistent with the response cache, which exposes estimated_items as a flag.

Pre-existing, just noting

get_block_data:378-382 uses if let Ok(Some(hash)) = db.get_block_hash(..), silently dropping a DB Err and falling through to upstream, while its sibling get_block_data_from_db:559-568 warns explicitly and documents why. Untouched by this PR, so not a request here.

Things I checked that are fine

Listing these so it's clear they were looked at:

• get_block_data_for_tx does reach the memory tier (:495), so the PR's headline benefit holds. It still pays one upstream eth_getTransactionByHash per request, so what's saved is the witness fetch plus contract resolution, not the whole round trip.
• light_witness_memory_bytes is not missing a parent-commitments term — a LightWitness never materializes them (witness_size.rs:57-60), so kvs + levels is the correct set.
• I suspected the number→hash resolution in get_block_data duplicated the one in is_canonical and should be a shared memoized resolver. It shouldn't: the two treat "DB has no hash for this number" in opposite ways (fall through to upstream vs. treat as canonical below the retained range) and use different deadlines. Merging them would break the canonicality guard.

Suggested landing order

1. Required before merge: the discrimination rule at rpc_service.rs:491.
2. Cheap and worth doing together: shard/rejection observability, the size_of-derived constant, an eviction counter.
3. Tests: make the budget test discriminate, cover the two new weight terms, and add handler-level tests for the eviction rule (that last one is really the acceptance criterion for item 1).
4. Docs: combined memory footprint, and what "a cache shard's budget" actually resolves to.

How I verified

cargo nextest run -p debug-trace-server @ 18a127a in a separate target dir: 96 tests, all green. The numbers above come from three instrumented probes in a scratch worktree (entry weight 95,589 B; binary search for the admission threshold; size_of of the salt KV pair; access-list/authorization counts in the fixture), reverted afterwards. I also read every per-transaction failure path in tracing_executor.rs, confirmed ContractCacheConfig::default() is 512 MB, and checked quick_cache 0.6.16's Cache::with / OptionsBuilder::build to confirm the shard default is never overridden here.

Note on stacking: base is still #161's branch, and #161 has open changes requested — worth settling that one's direction before retargeting.

Happy to be corrected anywhere, especially if the eviction hook was aimed at a failure mode I haven't reconstructed.

[github-actions]

Claude review status

Living comment — rewritten in place. The review workflow keeps this single comment up to date instead of posting a new one each round, so it always describes the latest reviewed commit and the earlier text is intentionally gone. No reply is needed here; answer findings and questions in the review threads it links to.

🛠️ Review did not finish

Attempted head 563b3979 · updated 2026-07-30T02:14:26+00:00

This round did not publish: MODEL_ACTION_FAILED in phase review_retry. Anything listed below is from the last round that did. Re-run the workflow or push a new commit to try again.

[vincent-k2026]

Approving — verified against 0ab67b1

Everything from my round-1 review is addressed, and the central item is solved more cleanly than what I asked for.

The eviction rule now keys off a typed discriminant, and it's right in both directions. TraceError::{Request, Data} plus matches!(error, TraceError::Data(_)) at the call site means:

• The case the hook was written for is now actually caught: every tracer's run_transaction failure returns Err(BlockReplayFailed) instead of degrading into an Ok full of TraceResult::Error entries. The justification in the comment holds up — a canonical block's transactions all execute (reverts included) against correct state, so a replay failure really does mean the data is bad.
• The case that shouldn't have triggered it no longer does: mux/JS config parse and inspector construction map to TraceError::Request, so a client can't drop hot entries with a cheap invalid request.
• trace_failure_evicts_block_data_only_for_data_errors pins the rule at the handler layer, in both directions — the coverage whose absence let both directions of the bug survive.
• The client-visible consequence (debug_traceBlock* on unusable data now returns -32000 rather than 200 with per-tx error entries) is called out in the description with its rationale. Agreed it's the right trade: the old shape was a partial wrong answer dressed as success.

A false alarm I chased and am retracting, so nobody "fixes" something that is already correct: I suspected mux and JS still degraded per-tx execution failures, because three TraceResult::Error sites remain in their branches. They're at try_into_mux_frame, the per-tx JsInspector::with_transaction_context, and json_result — tracer-output and per-tx construction failures, none of them data-attributable. Both tracers' run_transaction arms do abort (tracing_executor.rs:603 and :695). The rule is applied consistently; no change wanted.

The rest, checked rather than assumed

• Shard count pinned to 4 with the reasoning written down, so the admission ceiling is max_bytes / 4 (~256 MB) on every host instead of shrinking as machines get bigger. Exactly the fix.
• Rejected inserts are now visible: peek after insert → counter + metric + debug log, and oversized_entry_is_rejected_and_counted pins the path.
• The budget test discriminates now. Single shard so the arithmetic is deterministic, and it asserts exact values (entry_count == 2, total_bytes == one * 2) instead of inequalities that a cache admitting nothing would also satisfy.
• The access-list / authorization terms have real coverage: a synthetic transaction with 3 storage keys and 2 authorizations, asserting the exact weight delta against the constants — it fails if either term is dropped.
• SALT_KV_BYTES is now size_of::<(SaltKey, Option<SaltValue>)>(), so the bound tracks the type instead of a hand-copied number.
• block_data_evictions_total per method; estimated_items derived from the byte budget rather than hard-coded; README documents the weigher's under-count and the bytecode double-charge.
• The StaticHashStore → StubBlockStore refactor with read counters is a good call — "the memory-served read performs zero tip reads" is a much better assertion than "it returned something".
• cargo nextest run -p debug-trace-server -p stateless-db @ 0ab67b1: 116 passed, 0 failed.

Non-blocking follow-ups

1. README.md:126 — the combined-memory figure predates the #161 merge and now omits a fourth cache. Details inline.
2. block_data_cache.rs:49 — where does EXPECTED_BLOCK_DATA_BYTES come from? Details inline.
3. block_data_cache.rs:194 — the peek-based reject detection can miscount a concurrent legitimate eviction; worth a clause in the comment. Details inline.

None of these gate the merge. Good round.