rpc/jsonrpc: two review follow-ups from #23322 - #23759
Merged
Merged
Conversation
The fixtures force-wrote PersistReceipts after InsertChain and never enabled RCacheDomain, so a disabled domain dropped everything execution wrote. The cells asserting that a keep-all retention serves an old block passed by re-executing against physically unpruned history, which the retention promises nothing about, and a regression in the cache path went unnoticed. The domain is now enabled before the chain runs, and requirePersistedReceipts asserts the receipts really are on disk. TestReceiptEndpointsCloseWhenTheCacheIsNotServed adds the endpoint counterpart of the gate-level test: the retention opens the block only while the cache is served. Still open from the same review note: an endpoint test with history physically unavailable, so a re-execution cannot answer at all. It needs step-sized aggregation and a prune mode applied at insert time, which reverses this fixture's premise of inserting without physical pruning.
checkLogsAvailable reused the full-receipt gate, which refuses a pre-Byzantium block below the history cutoff because the post state a full receipt carries is not stored and has to be recomputed by re-executing. A log query never reads that field: getLogsV3 asks GetReceipt for a receipt without a post state, and the generator serves the persistent cache exactly because of it. So with receipts kept beyond history an unfiltered pre-Byzantium query was refused although every byte it reads was retained. checkReceiptsAvailable keeps the post-state leg and the rest moves unchanged into checkReceiptSourceAvailable, which gates on where the receipts come from whatever fields the caller reads. checkLogsAvailable composes the blocks leg with that one, and keeps the history leg behind usesLogIndex: an indexed filter still searches LogAddrIdx and LogTopicIdx, retired at the history cutoff. eth_capabilities is unchanged. caps.Logs is already the stricter form describing an indexed query, so the post-state clamp was a no-op for it: the advertised boundary stays at the history cutoff and the gate now reaches further back than advertised, never short of it.
AskAlexSharov
approved these changes
Sep 3, 2026
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches generator behavior and includes focused regression coverage.
Pull request overview
Separates log availability from full-receipt post-state requirements and strengthens receipt-cache fixtures.
Changes:
- Adds a receipt-source gate for log queries.
- Enables and verifies persisted receipt fixtures before execution.
- Adds pre-Byzantium and cache-disabled regression tests.
File summaries
| File | Description |
|---|---|
rpc/jsonrpc/eth_api.go |
Separates receipt-source and full-receipt gating. |
rpc/jsonrpc/prune_gating_test.go |
Persists and verifies fixture receipts. |
rpc/jsonrpc/check_prune_gates_test.go |
Covers cache and pre-Byzantium boundaries. |
Review details
- Files reviewed: 3/3 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.
lupin012
marked this pull request as ready for review
September 3, 2026 17:17
AskAlexSharov
deleted the
lupin012/logs_gate_and_prune_gating_fixtures
branch
September 4, 2026 03:20
This was referenced Sep 4, 2026
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Sep 6, 2026
…#23322 (erigontech#23812) Follow-up to erigontech#23322. erigontech#23690 closed seven of its deferred review notes and erigontech#23759 two more; this closes the three that are about `RemoteBlockReader.FrozenBlocks` and the mechanism behind it. | Note | Site | Change | | --- | --- | --- | | [r3885705116](erigontech#23322 (comment)) | `block_reader.go` | a slow backend no longer costs every caller a full timeout | | [r3885704185](erigontech#23322 (comment)) | `block_reader.go` | the zero handed out before the backend answers is no longer read as "no snapshots" | | [r3885706881](erigontech#23322 (comment)) | `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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #23322. #23690 closed seven of its deferred review notes; this closes the
two P3 ones it left, one per commit.
eth_api.goprune_gating_test.goLogs
checkLogsAvailablereused the full-receipt gate, which refuses a pre-Byzantium blockbelow the history cutoff: a full receipt carries a post state the cache does not store
and only a re-execution can recompute. A log query never reads that field —
getLogsV3asks
GetReceiptfor a receipt without a post state, which is exactly why thegenerator serves the cache. So with receipts kept beyond history, an unfiltered
pre-Byzantium query was refused although every byte it reads was retained.
checkReceiptsAvailablekeeps the post-state leg; the rest moves unchanged intocheckReceiptSourceAvailable.checkLogsAvailablecomposes the blocks leg with thatone and keeps the history leg behind
usesLogIndex, since an indexed filter stillsearches
LogAddrIdx/LogTopicIdx. Filtered queries are unaffected, andoverlay_getLogsgates oncheckBlockHistoryAvailableand is untouched.eth_capabilitiesis unchanged on purpose:caps.Logsis alreadystricterRetention(receipts, history), so the post-state clamp was a no-op for it.TestCapabilitiesAgreeWithTheLogsGatePreByzantiumpins that the advertised boundarydoes not move — the gate now reaches further back than advertised, never short of it.
Fixtures
They force-wrote
PersistReceiptsafterInsertChainand never enabledRCacheDomain,so a disabled domain dropped everything execution wrote: the keep-all cells passed by
re-executing against unpruned history. The domain is now enabled before the chain runs,
and
requirePersistedReceiptsasserts the receipts really are on disk.Tests
Both commits are TDD; the fixture one first failed with
"[]" should have 1 item(s), but has 0. New:TestLogsGateSkipsThePostStateLegPreByzantium(one block, three answers),TestCapabilitiesAgreeWithTheLogsGatePreByzantium,TestReceiptEndpointsCloseWhenTheCacheIsNotServed.go test -count=1 ./rpc/jsonrpc/...green,
-racegreen on the gate tests,make lintclean.Not in this PR
The fixture note also asked for an endpoint test with history physically unavailable.
A negative control confirms the gap is real: the "endpoint answers" leg passes even with
zero persisted receipts. Real pruning (
WithStepSize+WithPruneMode) is the rightroute, but it reverses this fixture's premise of inserting without physical pruning, so
it wants its own PR.