rpc/jsonrpc, db/snapshotsync: seven review follow-ups from #23322 - #23690
Conversation
- erigon_getLogs resolves a block hash through the shared resolver, so an unknown or non-canonical hash reports "block not found" instead of [] - resolveLogsRange returns the PrunedError it computes instead of leaving the refusal to every caller - the receipts gate and eth_capabilities follow history for any retention that is not a window of its own, which is the rule historyRetireCutoffs applies - a transaction-free block is cached, and answered before the exec semaphore, which is now taken where execution starts - the fork check is read once, from receipts.PostStateCalculated - the FrozenBlocks wait test asserts backend calls instead of wall-clock time - trims three comments to the invariant they carry
|
Review pass for over-engineering only — correctness, security and perf are out of scope here. One line per finding: where, what to cut, what replaces it.
net: about -95 of the 255 added lines. |
…ontech#23725) Follow-up to erigontech#23690, from an over-engineering review pass on it. No behavior change. | Site | Change | | --- | --- | | `eth_api.go`, `eth_system.go` | `receiptsRetiredWithHistory` inlined as `!amount.Enabled()` — both call sites are the second arm of a switch whose first arm already handles `KeepAllReceiptsPruneMode` | | `check_prune_gates_test.go`, `prune_gating_test.go` | `TestCapabilitiesFollowHistoryForASentinelRetention` → a `pruneGatingConfigs` row; `TestCapabilitiesAgreeWithGates` pins it against the gate more tightly | | `receipts/` | the two new test files → one test next to the helpers it uses, pinning the pre-semaphore answer and the cache together | | `check_prune_gates_test.go` | the `resolveLogsRange` leg dropped — nothing reaches the resolver except the two legs above it | | `eth_api.go`, `erigon_receipts_test.go` | three docstrings trimmed to what the name does not carry | Net −110 lines. No new test: pure inlining and test consolidation, covered by the existing suite.
…23759) Follow-up to erigontech#23322. erigontech#23690 closed seven of its deferred review notes; this closes the two P3 ones it left, one per commit. | Note | Site | Change | | --- | --- | --- | | [r3880873445](erigontech#23322 (comment)) | `eth_api.go` | a log query no longer inherits the post-state requirement of a full receipt | | [r3880873497](erigontech#23322 (comment)) | `prune_gating_test.go` | the fixtures persist the receipts their retention promises | ## Logs `checkLogsAvailable` reused the full-receipt gate, which refuses a pre-Byzantium block below 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 — `getLogsV3` asks `GetReceipt` for a receipt *without* a post state, which is exactly why the generator serves the cache. 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; the rest moves unchanged into `checkReceiptSourceAvailable`. `checkLogsAvailable` composes the blocks leg with that one and keeps the history leg behind `usesLogIndex`, since an indexed filter still searches `LogAddrIdx`/`LogTopicIdx`. Filtered queries are unaffected, and `overlay_getLogs` gates on `checkBlockHistoryAvailable` and is untouched. `eth_capabilities` is unchanged on purpose: `caps.Logs` is already `stricterRetention(receipts, history)`, so the post-state clamp was a no-op for it. `TestCapabilitiesAgreeWithTheLogsGatePreByzantium` pins that the advertised boundary does not move — the gate now reaches further back than advertised, never short of it. ## Fixtures They force-wrote `PersistReceipts` after `InsertChain` and never enabled `RCacheDomain`, 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 `requirePersistedReceipts` asserts 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, `-race` green on the gate tests, `make lint` clean. ## 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 right route, but it reverses this fixture's premise of inserting without physical pruning, so it wants its own PR.
…#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.
Follow-up to #23322, which merged as approved with thirteen inline review notes deferred.
This closes the seven that are self-contained; the other six are grouped at the end.
erigon_receipts.goerigon_getLogsno longer answers[]for an unknown or non-canonical block hasheth_receipts.goresolveLogsRangereturns thePrunedErrorit computeseth_api.go,eth_system.goeth_capabilitiesfollow history for a sentinel retentionreceipts_generator.goeth_api.goreceipts.PostStateCalculatedblock_reader_test.goblock_reader.go,eth_api.goThree of them are worth a line:
By-hash logs.
resolveLogsRange's hash branch is extracted intoresolveLogsBlockHashand
erigon_getLogsgoes through it, so all three by-hash endpoints answer alike. The samefunction no longer discards the refusal it derives; error class and message are unchanged.
Sentinel retention.
Distance.Enabled()is false for three values and the switchspecial-cased two, so
KeepPostMergeBlocksPruneModeproduced no gate whilehistoryRetireCutoffsretires the RCache with history for it. Both switches now ask onepredicate, which also drops the ordering dependency. It is an unexported helper rather than a
second exported method on
prune.Mode, since the existing narrowReceiptsFollowHistory()still drives RCache download policy — see below.
Exec semaphore. It is now taken where execution starts, immediately before
PrepareEnv,so neither a transaction-free block nor the persisted-receipts fast path queues behind real
re-executions. The per-block mutex still covers the whole function.
Tests
Five new, one changed.
TestErigonGetLogsByBlockHashRequiresACanonicalBlock;a
resolveLogsRangeleg onTestLogsByBlockHashNamesThePruneBoundary; acache_sentinel_retentionrow onTestReceiptsGateFollowsRetentionplusTestCapabilitiesFollowHistoryForASentinelRetention;TestGetReceiptsCachesAnEmptyBlockandTestGetReceiptsAnswersAnEmptyBlockWithoutAnExecSlot(semaphore full and context alreadycancelled, so any path reaching the semaphore reports the cancellation).
make lintandmake test-shortare clean.One deviation
The flaky-assert note suggested
< 2*timeoutor a channel signal; this assertsclient.calls.Load() <= 2instead — each caller reaches the backend at most once, and everyfetch it runs is bounded by that caller's own deadline. Deterministic, but it pins the timing
property less tightly;
require.Less(t, time.Since(started), 2*timeout)can go back alongsideit if preferred.
Not in this PR
Each is a behaviour change wanting its own test:
erigon_getLatestLogsis the third by-hash log path and still hand-rollsHeaderByHash.Mode.ReceiptsFollowHistory()(narrow) still drives RCache download policy indb/snapshotsync, so for the sentinel retention download and retirement disagree.Generator.GetReceipt(singular) takes no exec slot at all, though it executes.