fix(history): publish the pruning boundary before reclaiming behind it - #12954
Conversation
Adds IRangeRemovableKeyValueStore alongside the other capability interfaces, implemented over RocksDB's range tombstone and mirrored in MemDb so callers can be tested without a real database. The cost does not scale with how many keys the range holds, which only pays off where the keys are ordered along the dimension being dropped - the block stores are, being keyed by number then hash. The tombstone goes through a write batch rather than the database directly, so it reaches the WAL and a crash cannot resurrect a range its owner has already stopped serving. Nothing calls this yet. The tests pin the bounds, which is the whole safety story: the lower one inclusive, the upper one exclusive, neighbours on both sides untouched, an empty range a no-op, the tombstone surviving a reopen, and every hash at every covered height going - including the ones no level record lists, which a per-block loop leaves behind forever.
Pruning could not finish. Each pass read two full block bodies and wrote one tombstone per transaction, so clearing a 19M-block backlog meant reading roughly as much as it freed - and observed passes managed one to five blocks each, which does not terminate, it merely runs forever. The boundary a pruning node announces is now a policy decision rather than an outcome of disk throughput: it moves to the cutoff in one metadata write, and the disk is given back behind it in bounded steps. Removing a range costs the same whatever it spans, so a step is a handful of operations reading nothing at all. Splitting the announced boundary from a reclaim cursor is what makes an interrupted pass safe. The boundary can never be starved, and everything the reclaim touches has already been declared absent - so a reclaim that is slow, cancelled or lost to a crash leaves the node honest and merely fat, resuming from the persisted cursor. The gate and the resume point both read that cursor, not the boundary; they part company the moment a pass is cut short, and only the cursor knows where the disk stands. The transaction index is deliberately left alone. Enumerating it is what forced the body read per block, and its entries are an accelerator, not the record: one resolving to a block that is gone already answers not found. Reclaiming them wants a compaction filter, which is not here yet. Access list pruning no longer hangs off the block pass finishing. That gate meant it never ran at all, since the block pass was always cut short, so access lists grew without bound. Tests pin the boundary from both sides, that an interrupted pass leaves no gap behind, and that genesis and the sync pivot survive. The first two each caught a real defect: a gate and a resume point still reading the boundary instead of the cursor, which would have looked like working pruning on a disk that never shrank.
|
Claude finished @svlachakis's task in 8m 8s —— View job Review: PR #12954 — publish the pruning boundary before reclaiming behind it
The core design is right and the diagnosis is convincing. Replacing an O(blocks) read-then-delete walk with O(1) range tombstones is the correct fix for a pass that reads as much as it frees, and separating the announced boundary from the reclaim cursor — so an interrupted pass leaves the node honest-and-fat rather than dishonest-and-thin — is the load-bearing insight. The half-open bounds are correct in both directions: Two findings block merge, both in the seams between the new range paths and the deferred-write machinery that the old per-block paths handled explicitly. Critical1. That dictionary is not a cache; High2. The boundary is only announced when the boundary block's body is on disk —
Medium3. The column-family range delete is untested — 4. Deferred-overlay safety is documented, not enforced — 5. Dead code left in place — Low
Points I checked and found soundChain-level records were never deleted by the old path either, so leaving them is not a regression. · branch |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…rom riding on the reclaim Clearing the pending-canonical map alongside the receipts cache was a data-loss bug. That map is a cancellation ledger, not a cache: the deferred writer skips any entry missing from it, so clearing it dropped the transaction-index write of every block queued near the head, and those transactions would have stopped resolving by hash permanently. Nothing queued can be inside a pruned range, so there was nothing there to cancel in the first place. The boundary announcement hung on finding the block BODY at the boundary. Per-block that cost one skipped announcement out of thousands and healed on the next iteration; as a single jump it means a snap-synced node with no ancient bodies reclaims the range while its peers are never told - the exact inversion of the property this is built on. Headers are never pruned, so it announces off the header, and skips a body read and decode per boundary move. The access list pointer now keeps pace per chunk instead of after the loop, where a cancelling return skipped it and left the access list pass re-issuing tombstones over a range already reclaimed - and double-counting them. Range removal cannot know how many of its heights held a block, so the two counters say heights in their descriptions rather than quietly changing what they measure. Coverage follows the production path: the receipts store resolves a column, and no test exercised the column-family call. The simulate store no longer counts through a span it does not hold, and the legacy-key remark says why a hash colliding into a pruned range is accepted rather than guarded.
|
@claude review again |
|
Claude finished @svlachakis's task in 8m 12s —— View job Re-review: PR #12954 — publish the pruning boundary before reclaiming behind it
Both blocking findings from the last round are properly fixed, and the fixes are the right ones rather than the minimal ones.
Two deferrals I'm accepting as reasoned calls, not oversights: leaving One new finding blocks merge. It is in the seam the previous round didn't reach: the other implementations of the new interface members. High
Only So with Low
Verified sound this round
I could not run --- · branch |
benaadams
left a comment
benaadams
left a comment
There was a problem hiding this comment.
Two additional findings from local review; existing unresolved threads already cover the other requested items.
… the build The throwing default was reachable in production. NullReceiptStorage is what Receipt.StoreReceipts=false binds as the node's receipt storage, and the pruner runs regardless of it, so Rolling plus receipts-off would delete a chunk's blocks, throw on its receipts, leave the cursor where it was, and repeat the same chunk every pass - the non-terminating shape this branch exists to remove, on a configuration that works on master. Both non-persistent stores implement it now; the default still throws so the next store added has to decide rather than silently skip receipts, and a test over every implementation catches that at build time. The in-memory one is keyed by hash throughout, so it reads the height off the receipts themselves. The access list counter went one step too far and reported nothing at all on the common path, where the block pass claims the ground and the access list pass then finds none left. It counts what each pass newly claims, which double-counts neither. The reclaim limit is re-clamped against the live sync pivot rather than the one that happened to be current when the boundary was published, possibly in an earlier process: the boundary is durable, the pivot is not monotonic in config. Two comments claimed the access list pass publishes a boundary first. It does not, and does not need to - that pointer is local bookkeeping, announced to nobody - so they say what the code does. Also drops the LINQ that broke the build, and with it the assumption that a filter for 'error CS' sees every build error: style rules are errors here, so it never did.
…erlay holes Leaving the transaction index to the existing TxLookupLimit cleanup did not work, and the reason is structural: that path needs the block body to enumerate a block's transactions, and with default settings history pruning removes bodies at 1,056,768 blocks deep while the lookup horizon only reaches 2,350,000. By the time it arrives the body is gone, so it cleans nothing. Range reclaim had removed the pruner's own per-block cleanup, so entries would have accumulated without bound where they previously did not - a regression, not a deferred improvement. They are swept instead: the index is keyed by transaction hash so it cannot be dropped by range, but each value carries the block number, which is enough to decide staleness without reading anything else. A bounded slice per pass, resuming from a persisted cursor and starting over once the column has been walked, since the boundary moves on. No native callbacks, and the same shape as the rest of the reclaim. Entries in the legacy hash-valued form carry no number and are left rather than guessed at. The two metadata writes were also ordered wrongly. If the boundary reached disk first and the process died, a restart would find no cursor key, read that as level with the boundary, and treat an unreclaimed backlog as finished - disk never returned, silently, permanently. The cursor goes first now and the order is documented as load-bearing. The deferred-write overlay is drained for the range under its own lock rather than argued about in a comment. The margin reasoning held, but a public range method should not rest on what its only caller happens to do today.
Publishing a boundary and issuing range tombstones frees nothing: a tombstone does not count towards pending-compaction bytes, and the blocks and receipts column families are left at RocksDB's 30-day periodic compaction default, so the space can stay occupied for weeks. Measured on a 2.1GB column with half of it tombstoned: 0 bytes returned after 30 seconds. IRangeRemovableKeyValueStore gains ReclaimRange, implemented over rocksdb_delete_file_in_range_cf plus rocksdb_suggest_compact_range_cf. Block-numbered keys are written in ascending order, so a reclaimed range owns whole SST files and unlinking them is metadata-only; files straddling either bound are left to the compaction hint. It runs outside the deferred-write overlay lock, touching only keys already declared absent. Fixes a regression introduced earlier on this branch: with Receipt.CompactTxIndex=false every transaction-index value is a 32-byte block hash, which the sweep refused, and the per-block path that used to delete those regardless of value form is gone - so a column that was pruned became one that grows without bound. The hash form now resolves its height through the header. The sweep is also cancellable per entry, skipped while the boundary is still below the TxLookupLimit horizon (the shipping default, where the per-block path already covers it), and commits in 4096-key slices under the canonical writer's lock rather than one unbounded batch. Its budget rises to 500k, correcting a pass-rate estimate that was wrong by ~51x. Remaining review findings: volatile publication of the oldest-block header, and no null on lock timeout, so a pruned node cannot advertise full history; the cheap pruning gate trusted only once the pointers have loaded; InMemoryReceiptStorage records heights instead of reading them off receipts that never carry one; three copies of the range-key arithmetic collapsed into KeyValueStoreExtensions; the dead IBlockTree.DeleteOldBlock and the unreachable isFinalUpdate announcement throttle removed; BlocksPruned renamed to BlockHeightsReclaimed so a dashboard on the old name fails visibly rather than silently changing scale; and the [GaugeMetric] this branch had taken from PruningCutoffBlocknumber restored. Four regression tests, each proven red without its fix: the disk being returned, the CompactTxIndex=false sweep, a backlog surviving a restart, and a database holding a boundary but no cursor starting level with it rather than at genesis.
|
@claude review again |
|
Claude finished @svlachakis's task in 10m 4s —— View job Re-review: PR #12954 — publish the pruning boundary before reclaiming behind it
Everything outstanding from the last two rounds is fixed, and two of the fixes go further than what was asked — the deferred-overlay margin is now enforced rather than documented ( One finding blocks merge, in the new reclaim path. HighA failed best-effort reclaim aborts the pass and pins the cursor on the same chunk forever —
Note the asymmetry this exposes: Medium
Low
Verified sound this round
I could not run --- · branch |
|
@claude review again |
|
Claude finished @svlachakis's task in 6m 4s —— View job Re-review: PR #12954 — publish the pruning boundary before reclaiming behind it
The Nothing blocks merge. Eleven rounds in, every Critical and High raised across them is fixed, and every Medium is fixed or acknowledged with a reason I accepted at the time. Low1. The three reclaim tests now make incompatible claims about L0 —
The likelier reading is that the L0 line is the wrong rationale for the setup it explains: one flush puts 2. The new test pins one side of its property — Standing carry-oversUnchanged, listed so they stay a decision rather than an oversight:
Verified sound this round
I could not run · branch |
The endpoint test justified its compaction with "the unlink skips L0", which the two disk tests contradict: they flush with auto-compaction off, so every file they assert is unlinked is an L0 file, and they are green. The real reason that test needs the compaction is narrower and sufficient - one flush puts both keys in the same SST, and a file straddling the bound is never entirely inside the range whatever level it sits at. Also adds the assertion message the precondition line was missing.
|
Claude finished @svlachakis's task in 2m 28s —— View job Re-review: PR #12954 — publish the pruning boundary before reclaiming behind it
The part of your reply I'd single out is the experiment rather than the fix: you removed the compaction hint and observed both disk tests still green, which is what makes the L0 claim false as a measurement and not as a reading. That's the right standard, and it's also why the PR body's new caveat is the honest place to land — the unlink is demonstrated to free space on an L0 layout, not on the level-partitioned bottom level this targets. Stating that, plus naming the first-pass signal to watch (boundary jumping while Nothing blocks merge. Thirteen rounds in, every Critical, High and Medium raised across them is fixed or acknowledged with a reason I accepted at the time. CI is green on this commit — Standing carry-oversUnchanged, listed so they stay a decision rather than an oversight. None of them is a reason to hold this.
Verified sound this round
· branch |
0 means index forever and ulong.MaxValue means index nothing; under either the per-block path never removes an entry, so an operator on one of them has asked for the index to be left alone. The horizon check happened to stand the sweep down for 0 at any realistic head, since head - 0 is head and the pruning boundary is always below it, but ulong.MaxValue fell straight through and swept. Refusing both by name makes it a decision rather than arithmetic.
|
@claude eeview again |
|
Claude finished @svlachakis's task in 3m 15s —— View job Re-review: PR #12954 — publish the pruning boundary before reclaiming behind it
Refusing both by name rather than relying on the arithmetic is the right shape — the old form made "does the sweep run" depend on a subtraction that happened to work out, which is how the Nothing blocks merge. Fourteen rounds in, every Critical, High and Medium raised across them is fixed or acknowledged with a reason I accepted at the time. CI on this commit: Low1. The 2. 3. Standing carry-oversUnchanged, listed so they stay a decision rather than an oversight. None is a reason to hold this.
Verified sound this round
I could not run · branch |
The branch carried its own copy of the pruner redesign, which master has now squashed. Conflicts resolved by keeping master's version of everything the PR owns - the TxLookupLimit sentinel guard and its test cases most of all - and the branch's retention layer on top of it: IPrunedReceiptRetention, the self-describing receipt path, and RetainReceiptsAndReclaimTheRest with its density guard. Verified no line of master's HistoryPruner was dropped.
3 participants
Pruning does not finish today. On a node syncing mainnet from genesis with
History.Pruning=Rolling, observed passes look like this:One to five blocks per pass, against a backlog the node itself estimates at ~12M. That is not slow, it is non-terminating - and identical at
PruningTimeoutSecondsof 0, 2 and 60, because the timeout was never the binding constraint.Why
Two independent causes.
The work is enormous. Per block:
LoadLevel, a full body read and RLP decode viaFindBlock, a secondFindBlockinsideUpdateBlocksDeletePointeron every iteration (though the wire announcement is throttled to% 10000), one tombstone per transaction into the tx index, and a metadata write. Clearing 19M blocks means reading roughly as much as it frees. The body is read only to enumerate transactions for the index.The boundary is an outcome, not a decision.
_blocksDeletePointeradvances behind the deletion, one block at a time - and it is consensus-visible, feeding eth/69BlockRangeUpdateand the RPC capability floor. So a starved pruner leaves the node announcing blocks it means to delete while the disk grows.What this changes
The boundary moves first, in one metadata write. It is a policy decision from head and retention, so nothing can starve it. Raise-only, so a lower cutoff never walks it back onto data already gone.
The keys stop being visible in bounded steps.
DeleteRangeover the number-prefixed keys of blocks, receipts and access lists. A range tombstone costs the same whatever it spans, so a chunk is a handful of operations that read nothing.And the disk actually comes back. A tombstone frees nothing on its own: it does not count towards pending-compaction bytes, and these column families keep RocksDB's 30-day periodic compaction default. Measured on a 2.1 GB column with half of it tombstoned - 0 bytes returned after 30 seconds. So each chunk also unlinks the SST files lying entirely inside its range, which for keys written in ascending block-number order is nearly all of them, and hints for the boundary files. The unlink is metadata-only. It is best effort by contract: the keys are already gone durably, so a failure costs the timing of the space returning and nothing else - it must not abort a pass that has already published a boundary, still less take the node down.
The announced boundary and the reclaim cursor are separate, persisted values. This is the load-bearing part. Everything the reclaim touches has already been declared absent, so a reclaim that is slow, cancelled or lost to a crash leaves the node honest and merely fat - it resumes from the cursor. Both the gate and the resume point read the cursor, never the boundary.
Stale transaction-index entries are swept, not left. Range reclaim cannot address them - they are keyed by transaction hash - so a bounded, resumable, cancellable walk drops the ones naming blocks that are gone. It is skipped while the pruning boundary is still below the
Receipt.TxLookupLimithorizon, because there the existing per-block path still has the body it needs and does the work at no read cost.Which side of that horizon a node sits on changed with #12808, so concretely, at a mainnet head of ~25.9M against the default
TxLookupLimitof 2,350,000:Rolling, defaultRetentionEpochs=82125Rolling, mainnet floor33024UseAncientBarriers, mainnetIn the last two the body at
head − TxLookupLimithas already been reclaimed, soPruneOldTxIndexcannot enumerate it and the sweep is the only thing bounding the column. The gate self-corrects as the chain grows: once head passes 26,950,000 the fixed barrier falls back below the horizon and the sweep stands down again.Access list pruning no longer hangs off the block pass finishing. Worth being precise:
PruneBlockAccessListsalready checked the token per iteration, so the outer guard was redundant and removing it is a simplification, not a fix. Access lists were starved for the same reason blocks were - the whole pass never got to run.Progress cannot reach zero. The scheduler stamps its deadline at enqueue, so a pass that waited behind other work arrives already cancelled - on a busy node, potentially every pass. All three passes therefore honour the token only after a unit of work rather than before one, and the sweep runs last because it is the only one whose cost its range does not bound. A pass that arrives spent takes a reduced chunk instead of a full one, since the one case it exists for is also the one case it runs alongside block processing.
Operator-visible changes
Two metric series are renamed, because their unit changed and leaving the old names would have made them lie. A range is dropped in one operation and never learns how many of its heights held a block, so both now count heights reclaimed:
nethermind_blocks_prunednethermind_block_heights_reclaimednethermind_block_access_lists_prunednethermind_block_access_list_heights_reclaimedA dashboard or alert on an old name renders as "no data" rather than as an error, so this needs a changelog line.
nethermind_transaction_index_entries_prunedis new.Three members are removed from public interfaces, all of them with no production caller left:
IBlockTree.DeleteOldBlock,OnNewOldestBlockArgs.isFinalUpdateand theSyncServerannouncement throttle it fed, which could no longer be reached now the boundary moves in one jump.No configuration is added or changed. The chunk size and the sweep budget are constants.
Tests
The bounds are the whole safety story, so they are pinned from both sides: lower inclusive, upper exclusive, neighbours untouched, an empty range a no-op, the tombstone surviving a reopen, every hash at every covered height going - including ones no chain level lists, which the per-block loop leaves behind forever - and the column-family path, which is the one receipts actually take.
At the pruner: the block at the boundary survives while the one below it goes, genesis and the sync pivot are never touched, a reclaim backlog survives a restart, a database holding a boundary with no cursor starts level with it rather than at genesis, and both the reclaim and the sweep still move when the budget is already spent.
Seven were written by breaking the fix first and confirming the test went red - the disk being returned, the
Receipt.CompactTxIndex=falsesweep, the cursor reaching disk, the backlog surviving a restart, the no-cursor migration, and the two spent-budget guarantees. Several others caught real defects while being written: a gate and a resume point still reading the boundary instead of the cursor, either of which would have looked like working pruning on a disk that never shrank.On a real database
Since the above was written, this ran to completion on a mainnet archive node: a 1.9 TB database, syncing at ~95% and processing at ~1000 MGas/s throughout, so the pruner was competing for its budget the whole time rather than running on an idle box.
The boundary published on the first pass at 18:03:49. The reclaim cursor cleared the entire ~21.6M height backlog and reported
0 remainingat 18:18:10 - 14 minutes 21 seconds. Passes after that take 256 heights each, keeping pace with the boundary as it moves.blocks/receipts/Roughly 900 GB returned, and the net figure understates it:
flat/andflatHistory/grew by 7 GB over the same window because the node was still syncing.Some notes from the run.
The space comes back from the bottom level.
blocks/lost 76% of itself on a database large enough that its block column is overwhelmingly bottom-level, which answers in the aggregate what the caveat above could not. Whatdustill cannot separate is how much the file unlink returned directly versus how much the compaction hint returned shortly after - that would need SST-level counters. Either way the mechanism works end to end, and on master these tombstones would not exist to be compacted at all.The reduced chunk is not a corner case. Five passes in six took the reduced 100,000 rather than the full 1,000,000, meaning the token was already cancelled before the loop began - the same state in which the old path managed single-digit blocks per pass. The occasional full chunk landed when the scheduler had real time to give it. Fourteen minutes for the whole backlog is what that floor buys on a node that never stops processing.
Heights reclaimed are not bytes reclaimed. The first 11M heights predate December 2020 and gave up only ~200 GB between them; the remaining 10M gave up ~700 GB. An operator watching
duagainst the block counter early in a backlog will think it has stalled.blockAccessLists/is 37 MB on this node because access lists are recent, so its cursor spends its first twenty-odd million heights crossing empty space.A pass under load, and the last one. The count in parentheses on the first line is the heights added since the previous published boundary, not the outstanding backlog - the backlog is the
remainingfigure on the line below: