rpc/jsonrpc, db/snapshotsync: serve blocks from the oldest one the datadir holds - #23939
Conversation
MinimumBlockAvailable took the highest per-type minimum but skipped a type whose segments are not visible, so a datadir whose transaction segments all start above the aligned height reported genesis: the lowest block it serves in full, the value eth_capabilities advertises and the sentry status message carries, while no transaction below that height is readable. SegmentsMin replaces the per-type SegmentsMinByType, its only caller. It walks the snapshot set's own types rather than a list rebuilt in the reader, and answers that no block is complete when one of them covers nothing; the reader then falls back to the database, as it already did for a type missing altogether.
Under chain-history expiry the boundary was the merge height whatever the datadir held, because the probe read MinimumBlockAvailable and kept only the boolean. Both directions were wrong. Segments are indivisible, so the one spanning the merge is downloaded whole and reaches below it: on mainnet the transactions start at 15_500_000 and the gate refused the 37_394 blocks up to the merge that the node serves. A datadir pruned by distance and reopened with the expiry sentinel starts above the merge instead, and the gate then promised blocks that are gone while eth_capabilities advertised a floor with holes above it. The verdict now carries the block the datadir serves from: the value read when availability settles it, the merge height when only a readable pre-merge transaction can. eth_capabilities follows without a change of its own, since it resolves the boundary through the same call. max(mergeHeight, oldest) does not close this: under expiry oldest lands below the merge, so the maximum stays the merge height and the refusal with it.
The search for a pre-merge user transaction restarted from the last block on every pass, discarding the bounds it had just descended through, and the sampling loop above it dropped the counts it read. Both are cumulative counts on bodies that were already fetched, each of them a gRPC round trip on a remote rpcdaemon. A bound holds until the search excludes as many transactions as it records, so the blocks a pass reads bracket the next one, and a sampled count of zero says there is no transaction below to look for. On a chain inflated block after block the walk drops from 133 body reads to 32; with the inflation near genesis, from 25 to 14.
An undecided probe is not stored, so every gated request repeated the whole walk: MinimumBlockAvailable, the sampling loop, and up to 256 body reads. Two different things were undecided, though. A search that ran out of budget read everything it asked for, and the chain shape that spent the budget is what a second walk reads again; a datadir whose bodies are not there has not answered at all, and does as soon as blocks arrive. The first is now a verdict worth keeping for the cache TTL, the second stays unremembered. Caching both would also swallow a failed probe, which a caller must retry on its own transaction.
|
Checked out the branch and ran it. The stated scope is green, and I fuzzed the probe: 4000 random chains across four transaction densities, each run twice, once with transactions readable and once with the bodies kept but the transactions taken away. With them readable the probe never reported expiry; with them unreadable it never reported archive while a pre-merge transaction was really there. The carried bounds and the Two things came out of it. Correctness
The invariant does hold as written, and I convinced myself of it. But it now lives across iterations in a slice the search carries, rather than in the per-iteration reset the old shape had, and it is the kind of thing the next edit to this function breaks silently. This runs in an RPC handler under a read transaction, so a spin is a goroutine pinning an MDBX reader rather than a slow request. One Worth recording
Finding 1 aside, the boundary change reads correct to me: the mainnet expiry shape resolves through the |
A database with nothing beyond genesis and one serving from genesis both read as zero, so a datadir frozen mid-chain with an empty database reported genesis as its oldest block while it advertised blocks above it. findFirstCompleteBlock now says whether it found a body, and SegmentsMin reports the minimum the visible types reach next to the completeness flag, so the fall-through has an answer to keep.
The outer walk terminates because the pruned bounds always leave one the search has not excluded yet. Assert it where the bound is taken, so a later edit costs an open question rather than a loop held under a read transaction.
if high.txns <= excludedTxns { |
There was a problem hiding this comment.
🟡 Changes recommended
An incomplete snapshot set can still be reported as a servable boundary when the database contains no blocks.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR aligns block pruning gates and capabilities with the oldest block the datadir can serve.
Changes:
- Tracks the resolved oldest block in the pre-merge probe cache.
- Improves sparse transaction search bounds and caching.
- Adds snapshot minimum handling and regression tests.
File summaries
| File | Description |
|---|---|
rpc/jsonrpc/pre_merge_probe_test.go |
Tests probe caching and search efficiency. |
rpc/jsonrpc/eth_system.go |
Clarifies capability boundary behavior. |
rpc/jsonrpc/eth_api.go |
Resolves and caches the actual block boundary. |
rpc/jsonrpc/check_prune_gates_test.go |
Tests gates and advertised boundaries. |
db/snapshotsync/snapshots.go |
Adds aggregate segment minimum reporting. |
db/snapshotsync/snapshots_test.go |
Tests segment minimum completeness. |
db/snapshotsync/freezeblocks/block_reader.go |
Combines snapshot and database minima. |
db/snapshotsync/freezeblocks/block_reader_test.go |
Tests minimum-block resolution scenarios. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A search that spends its read budget is now remembered, so the note about leaving the question open described the behaviour before that change.
…ntech#24218) Review follow-up from erigontech#23322: erigontech#23322 (comment) `_preMergeData` is stored only when the probe decides, so a datadir whose block data cannot be read was never remembered and the walk ran again on every gated RPC: `MinimumBlockAvailable` plus about log2(mergeHeight) `CanonicalBodyForStorage` reads, each one a gRPC round trip on a remote rpcdaemon. erigontech#23939 closed the `earlyTxnSpent` branch of the same note; this closes the remaining one. An undecided answer is now held for a TTL of its own, one second, separate from the 30s verdict TTL. A fresh verdict is still checked first, and only a probe that neither decided nor failed is recorded, so a failed read is not inherited by another caller. The answer is held, never stored as an observation, so a datadir whose blocks have not arrived is still not read as an archive one. Cost: up to one second before arriving blocks are noticed, inside the window where the gate rejected them anyway. Tests: `TestPreMergeProbeHoldsAnUnansweredWalkForItsTTL` pins the reads (20 across 5 requests before, 4 after); `TestPreMergeProbeWalksAgainOnceTheUnansweredTTLPasses` pins the other direction. `TestBlocksGateDoesNotSettleExpiryBeforeBlocksArrive` sets the new TTL to zero, so it keeps observing one walk per call.
Follow-up to #23322, the last group of its deferred review notes.
Boundary. Segments are indivisible, so the one spanning the merge is downloaded whole and
its transactions reach below it: on mainnet they start at 15,500,000 against a
MergeHeightof15,537,394, and those 37,394 blocks were refused although the node serves them. A datadir pruned
by distance and reopened with the expiry sentinel starts above the merge, and the same fixed
boundary promised blocks that are gone. The probe already read
MinimumBlockAvailableand keptonly the boolean; its verdict now carries the block served from, and
eth_capabilitiesfollowsthrough the same call.
max(mergeHeight, oldest)does not close it — under expiryoldestlandsbelow the merge — and short-circuiting only at
oldest >= mergeHeightreads the distance-prunedshape as an archive and drops the gate.
Undecided verdict. A search that ran out of budget read everything it asked for and a second
walk reaches the same place, so it is kept for the cache TTL; a datadir whose bodies are not there
has not answered and stays unremembered, which leaves
TestBlocksGateDoesNotSettleExpiryBeforeBlocksArriveuntouched and needs no second TTL. That halfof the note remains: the "no block data" shape still pays about 24 body reads per request rather
than 280.
Not from review.
MinimumBlockAvailableskipped a segment type with nothing visible, so adatadir whose transaction segments start above the aligned height reported genesis — the value the
boundary is built on;
SegmentsMinreplaces the per-type accessor. And the search now carries itsbounds across passes and brackets them with the sampled counts: 133 body reads to 32, 25 to 14 with
the inflation near genesis.
Tests.
TestBlocksGateAppliesExpiryWhenOldestIsMidChainandTestPreMergeSearchStopsAtItsReadBudgetchange expectation — they pinned the rules this PR corrects — plus six new tests and a
expiry_starting_mid_chaincapabilities case.go test ./rpc/jsonrpc/ ./db/snapshotsync/... ./p2p/sentry/green,
make lintclean.