Skip to content

db/snapshotsync, rpc/jsonrpc: three review follow-ups from #23322 - #23812

Merged
AskAlexSharov merged 2 commits into
mainfrom
lupin012/prune_gates_frozen_blocks
Sep 6, 2026
Merged

db/snapshotsync, rpc/jsonrpc: three review follow-ups from #23322#23812
AskAlexSharov merged 2 commits into
mainfrom
lupin012/prune_gates_frozen_blocks

Conversation

@lupin012

@lupin012 lupin012 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #23322. #23690 closed seven of its deferred review notes and #23759 two more; this closes the three that are about RemoteBlockReader.FrozenBlocks and the mechanism behind it.

Note Site Change
r3885705116 block_reader.go a slow backend no longer costs every caller a full timeout
r3885704185 block_reader.go the zero handed out before the backend answers is no longer read as "no snapshots"
r3885706881 block_reader.go, eth_api.go one shared TTL-cache instead of two hand-rolled ones

Change

  • The refresh runs on a goroutine of its own; a caller that has a value to serve returns without waiting for the next one. A failed attempt is stamped like a successful one, so an unreachable backend costs one attempt per TTL instead of one per caller and per request.
  • FrozenBlocksObserved() (uint64, bool) reports the count together with whether the backend ever answered. receipts.PostStateCalculated and the eth_simulateV1 commitment path both read it; those are the two sentinel readers a remote reader can reach.
  • common/concurrent.CachedValue[T] holds what the getter and holdsPreMergeBlockData each kept by hand: one TTL, one dedup, one rule for what a failed pass leaves behind. Each site keeps its own waiting policy — Produce runs on the caller's goroutine, Go refreshes behind it.

For review

singleflight was tried and dropped. DoChan cannot join the pass in flight without possibly starting one of its own, and it runs the producer on a goroutine the caller does not own. That is unsafe for the pre-merge probe, which reads through the caller's kv.Tx: a caller released by ctx.Done() rolls that transaction back under a live reader. Produce runs the pass on the caller's goroutine instead. DoChan also turns a producer panic into go panic(e), which ends the process rather than the request.

An unobserved count now reads as "snapshots exist". For a pre-Byzantium block on a remote rpcdaemon whose backend has not answered yet, PostStateCalculated returns false, so the stored receipt is served — possibly without its root field — where before the block was re-executed and the gate could answer PrunedError for receipts that are on disk. That is the direction the note asks for, and the window is one TTL after a failed round trip.

Panics. Produce publishes the failed pass and re-panics, so the RPC server logs it as before; Go contains and logs it with the stack, since its goroutine has no caller to recover it.

Tests

common/concurrent/cached_value_test.go (11) pins the contract, including that a caller running the pass finishes it before returning while one that only waits is released by its own context. TestRemoteBlockReaderFrozenBlocks* cover the getter, rpc/jsonrpc/receipts/post_state_calculated_test.go the sentinel. The eth_simulateV1 leg has no dedicated test — the branch sits inside the commitment path.

make lint clean; ./common/concurrent/..., ./db/snapshotsync/freezeblocks/..., ./rpc/jsonrpc/..., ./execution/blockreplay/..., ./cmd/rpcdaemon/... green, with -race on the concurrency tests.

FrozenBlocks ran its refresh on the calling goroutine and never stamped a
failed one, so against an unresponsive backend every caller paid the full
timeout with a read transaction open. The refresh now runs on a goroutine of
its own, a caller with a value to serve returns without waiting, and a failed
attempt counts, so an unreachable backend costs one attempt per TTL.

Caching a failure is only safe once callers can tell a default from an
observation: FrozenBlocksObserved reports the count together with whether the
backend ever answered. PostStateCalculated and the eth_simulateV1 commitment
path read it, so an unobserved zero no longer stands for "no snapshots" and no
longer sends pre-Byzantium blocks down re-execution.

The bookkeeping the getter and the pre-merge probe each kept by hand moves to
common/concurrent.CachedValue: one TTL, one dedup, one rule for what a failed
pass leaves behind. Each site keeps its own waiting policy, because the probe
reads through the caller's transaction and cannot refresh behind it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The shared cache preserves caller-owned transaction lifetimes, deduplicates refreshes, handles failures safely, and has focused concurrency coverage.

Pull request overview

This PR unifies TTL-cached, deduplicated value production and distinguishes an unobserved remote frozen-block count from a confirmed zero.

Changes:

  • Adds generic concurrent.CachedValue[T] with synchronous and background refresh modes.
  • Migrates frozen-block fetching and pre-merge snapshot probing to the shared cache.
  • Updates receipt and simulation paths plus tests to use observation-aware frozen-block counts.
File summaries
File Description
common/concurrent/cached_value.go Adds the shared TTL cache and producer deduplication.
common/concurrent/cached_value_test.go Tests caching, failures, cancellation, deduplication, and panic handling.
db/dbservices/interfaces.go Extends block readers with observation-aware frozen counts.
db/snapshotsync/freezeblocks/block_reader.go Migrates local and remote readers to the new cache behavior.
db/snapshotsync/freezeblocks/block_reader_test.go Updates and expands remote-reader concurrency tests.
execution/blockreplay/memreader.go Implements the new reader method.
cmd/rpcdaemon/rpcservices/eth_backend.go Delegates the observation-aware method.
rpc/jsonrpc/eth_api.go Replaces the hand-rolled pre-merge probe cache.
rpc/jsonrpc/eth_simulation.go Avoids treating an unobserved count as zero frozen blocks.
rpc/jsonrpc/pre_merge_probe_test.go Adapts probe tests to CachedValue.
rpc/jsonrpc/check_prune_gates_test.go Adapts TTL-control tests to CachedValue.
rpc/jsonrpc/receipts/receipts_generator.go Uses observation-aware frozen counts for post-state decisions.
rpc/jsonrpc/receipts/post_state_calculated_test.go Covers the unobserved-count sentinel case.
Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@AskAlexSharov
AskAlexSharov added this pull request to the merge queue Sep 6, 2026
Merged via the queue into main with commit bff7e25 Sep 6, 2026
145 of 147 checks passed
@AskAlexSharov
AskAlexSharov deleted the lupin012/prune_gates_frozen_blocks branch September 6, 2026 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants