fix(history): create the history pruner before the block processor starts - #13285
kamilchodola wants to merge 5 commits into
Conversation
…arts HistoryPruner schedules a pass on IBlockProcessingQueue.ProcessingQueueEmpty, which BlockchainProcessor raises once when its loop starts and then after every processed block. Nothing resolved the pruner before InitializeNetwork or RegisterRpcModules, both of which run after StartBlockProcessor, so the startup signal was raised with no subscriber. A node that receives no blocks (no CL attached, offline maintenance run with History.Pruning enabled) therefore never pruned its backlog. Resolving the pruner from StartBlockProcessor makes it exist before the first signal.
|
Claude finished @kamilchodola's task in 6m 0s —— View job Review:
|
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Medium | StartBlockProcessor.cs:13-18 |
The change moves the first pruning pass into the middle of startup, not just the subscription. It can now run concurrently with StartupBlockTreeFixer (which deletes chain levels) and with EraStep/EraEStep history import — both previously always completed before the pruner existed. An additive StartHistoryPruner step calling the existing IHistoryPruner.SchedulePruneHistory() after ReviewBlockTree avoids the question entirely, touches no existing file, and doesn't depend on the queue-empty edge. (detail) |
| 2 | Medium | EthereumStepsManagerTests.cs:114-135 |
The test asserts the constructor signature, not the invariant. Autofac resolves ctor args before the ctor body, so it stays green if _ = historyPruner; — the line most likely to be removed by an IDE cleanup — is deleted. Also misplaced: it never uses EthereumStepsManager, and it splits Warmup_selects_head_before_pivot from its CreateWarmupEnvironment helper. (detail) |
| 3 | Low | StartBlockProcessor.cs:15 |
CancellationToken _ renamed to cancellationToken while still unused — unrelated diff that now implies the step honours cancellation. (detail) |
| 4 | Low | HistoryPruner.cs:283 |
Worth confirming the reported symptom is fully fixed: TryPruneHistory bails on _blockTree.SyncPivot.BlockNumber == 0. Your repro used UseAncientBarriers on a snapshot with a pivot set, but an offline pass over a DB synced from genesis with no SyncConfig.PivotNumber would still no-op at startup. |
The added comment on the discard is a genuine why comment stating the invariant — that one is good and should stay whichever shape the fix takes.
Not verified
I could not run dotnet build or dotnet test in this environment (the commands need approval), so compilation and the new test are unverified by me. Everything above is from reading the code.
Verdict: the bug is real and correctly diagnosed, but I'd resolve finding 1 (or record why the startup overlap is safe) and strengthen the test in finding 2 before merging.
· fix/history-pruner-startup-trigger
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Replace the IHistoryPruner injection into StartBlockProcessor with an additive StartHistoryPruner step that calls SchedulePruneHistory after InitializeNetwork. That keeps the first pass after the block-tree fixer and the Era import, where the pruner used to be constructed, instead of racing them from the processor's startup signal. StartBlockProcessor is back to its master form.
|
@claude re-review |
|
Claude finished @kamilchodola's task in 5m 10s —— View job Re-review:
|
| # | Prior finding | Status |
|---|---|---|
| 1 | First pruning pass moved into the middle of startup, overlapping StartupBlockTreeFixer / Era import |
Resolved. StartBlockProcessor is back to its master form; the additive step runs after InitializeNetwork, i.e. after EraStep/EraEStep (dependents: [InitializeNetwork]) and therefore after ReviewBlockTree. Verified in EraStep.cs:11-14. |
| 2 | Test asserted the constructor signature, not the invariant | Resolved. The step now calls SchedulePruneHistory() directly, so Received(1) is the invariant; EthereumStepsLoaderTests.BuildInSteps_IsCorrect genuinely pins registration — it diffs the assembly's non-optional steps against BuiltInStepsModule, so omitting the step would fail it (EthereumStepsLoaderTests.cs:36-56). |
| 3 | Unrelated CancellationToken _ → cancellationToken rename |
Resolved. File untouched. Note the new step's unused cancellationToken matches its closest sibling StartHistoryWalkVerification.cs:16, so it reads as house style there. |
The shape is right: additive step, no existing file modified other than the registration list, IHistoryPruner is registered unconditionally in BlockTreeModule (via NethermindModule.cs:67) so the step resolves in every client, Execute returns immediately because SchedulePruneHistory is fire-and-forget, and the double trigger is safe (_currentlyPruning CAS).
New findings
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Medium | StartHistoryPrunerTests.cs / behaviour |
One scheduled pass ≠ a drained backlog. The pass is bounded by PruningTimeoutSeconds (default 2 s) and stops at a 100k–1M-height chunk boundary with "the next pass resumes from here"; on the very node this PR targets (no CL, no blocks) there is no next trigger, so an offline snapshot trim publishes the whole boundary but reclaims only the first chunk. PruningTimeoutSeconds=0 removes the deadline and the single pass then completes — that resolves it, but it needs saying in the PR body, because "start once with --History.Pruning UseAncientBarriers" reads as sufficient. (detail) |
| 2 | Low | StartHistoryPruner.cs:20 |
The step calls SchedulePruneHistory() unconditionally, while the pruner's own trigger is gated on historyConfig.Enabled() and Pruning defaults to Disabled. The first pass does not short-circuit (the fast bail needs _hasLoadedDeletePointers && _stampsValidated), so every default-config node now runs pointer discovery + a metadata write + retention stamping at startup. Nothing is deleted, and the discovery path is already reachable via OldestBlockHeader — but mirroring the gate is one line. (detail) |
| 3 | Low | StartHistoryPruner.cs:11-16 |
The summary promises ordering "past the block-tree fixer and the Era import", but the attribute only names InitializeNetwork, which does not depend on ReviewBlockTree. The guarantee is contingent on EraStep's dependents list. Adding typeof(ReviewBlockTree) to dependencies is free (already satisfied) and makes the doc true by construction. (detail) |
Carried over for the record, unchanged by this PR: TryPruneHistory still returns early on _blockTree.SyncPivot.BlockNumber == 0 (HistoryPruner.cs:284), so a database synced from genesis with no pivot still won't prune from this trigger. Pre-existing, not a regression.
Comment quality: the XML <summary>/<remarks> on the new step state why the step exists and what would otherwise break — that's the good kind and should stay.
Not verified
dotnet build / dotnet test need approval in this environment, so compilation and the new test are unverified by me — though both new files use types already referenced from their projects (Nethermind.Init → Nethermind.History via BlockTreeModule; Nethermind.Runner.Test → via Module/PrunedReceiptRetentionModuleTests.cs), so nothing suggests a reference gap.
Verdict: the diagnosis and the new shape are both correct, and this is a clear improvement over the previous revision. I'd add the offline-run caveat (finding 1) to the PR body and, optionally, take the two one-line hardening suggestions.
· fix/history-pruner-startup-trigger
…BlockTree Mirror the pruner's own constructor gate so a default-config node schedules no pass, and declare the ReviewBlockTree dependency directly instead of relying on the Era steps' dependent list.
|
@claude re-review |
|
Claude finished @kamilchodola's task in 4m 1s —— View job Re-review:
|
| # | Prior finding | Status |
|---|---|---|
| L-2 | Step called SchedulePruneHistory() unconditionally while the pruner's own trigger is gated |
Resolved. if (historyConfig.Enabled()) now mirrors HistoryPruner.cs:127,145, so a default-config (Disabled) node does exactly what it does on master — no pointer discovery, no metadata write, no retention stamping at startup. |
| L-3 | Documented ordering held only transitively through EraStep |
Resolved. typeof(ReviewBlockTree) is now declared directly, so the "past the block-tree fixer" half of the summary holds by construction. The Era half is still transitive (EraStep/EraEStep: dependencies: [ReviewBlockTree], dependents: [InitializeNetwork], EraStep.cs:11-14) — that's fine, InitializeNetwork is the right thing to name for it. |
| M-1 | One scheduled pass ≠ a drained backlog on a node that never processes a block | Still open — see below. |
Checks on the new dependency edge, all clean:
ReviewBlockTreeis a non-optional built-in step (BuiltInStepsModule.cs:28), soEthereumStepsManager's missing-dependency throw (EthereumStepsManager.cs:100) can't fire; dependency matching is byStepBaseType, so a plugin subclass still satisfies it.- No cycle:
ReviewBlockTree → LoadGenesisBlock,InitializeNetwork → {LoadGenesisBlock, SetupKeyStore, InitializeBlockchain}; nothing depends onStartHistoryPruner. - No deadlock via
ReviewBlockTree's internalProcessingQueueEmptywait (ReviewBlockTree.cs:71-76) —StartBlockProcessordoesn't depend on the new step, so the processor still starts and releases it. EthereumStepsLoaderTests.BuildInSteps_IsCorrectgenuinely pins the registration: it diffs theNethermind.Initassembly's non-optional steps againstBuiltInStepsModule, andStepInfoKeyincludes the dependency list, so both dropping the step and quietly changing its dependencies fail the test.- Test shape is right for the repo rules: input→expected pairs belong in
[TestCase], not[Values], andReceived(0)/Received(1)on a substitute is the actual invariant now that the step calls the method directly.
Remaining finding
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Medium (doc-only) | PR body / testing notes | The motivating offline run is started by this step, not completed. SchedulePruneHistory enqueues one pass with a PruningTimeoutSeconds deadline (default 2, HistoryConfig.cs:12), and PruneBlocksAndReceipts returns at the first chunk boundary once the token trips (HistoryPruner.cs:610-615, chunk = 100k–1M heights) leaving "the next pass resumes from here". I re-confirmed there is no timer and no other caller of SchedulePruneHistory in the tree, so on the node this PR targets — no CL, no blocks — there is no next pass: the boundary is published in full (UpdateBlocksDeletePointer(target) at :572, before the reclaim loop) while only the first chunk is reclaimed, i.e. the node reports the history gone with the disk not yet freed. --History.PruningTimeoutSeconds 0 removes the deadline and the single pass runs to completion. Worth one line in the testing notes, because "start the node once with --History.Pruning UseAncientBarriers" reads as sufficient. |
Small upside of this PR that's worth recording alongside it: because the reclaim cursor is persisted, a restart now resumes the reclaim one chunk at a time, which master couldn't do at all on an idle node.
Two things unchanged by this PR, for the record: TryPruneHistory still returns early when _blockTree.SyncPivot.BlockNumber == 0 (HistoryPruner.cs:284), so a DB synced from genesis with no pivot still won't prune from this trigger; and the Enabled() check now lives in two places (step + pruner ctor) — acceptable, since the point is to avoid the startup work, but a future pruning mode has to remember both.
Comment quality: the <summary>/<remarks> on the new step explain why it exists and what breaks without it — the good kind, keep them.
Not verified
dotnet build / dotnet test need approval in this environment, so compilation and the new test are unverified by me. Nothing suggests a gap: Nethermind.Runner.Test already compiles using Nethermind.History in Module/PrunedReceiptRetentionModuleTests.cs, and the [RunnerStepDependencies(dependencies: [...])] form matches StartHistoryWalkVerification.cs:13.
Verdict: the fix is correct and the shape is now right — additive step, one-line registration change, no existing behaviour touched, disabled nodes byte-for-byte as before. Only the offline-run caveat (finding 1) is outstanding, and it's a PR-body edit rather than a code change.
· fix/history-pruner-startup-trigger
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
Review report
Scope: all three files against merge-base 221c144bff — Nethermind.Init/Modules/BuiltInStepsModule.cs (+1), Nethermind.Init/Steps/StartHistoryPruner.cs (new, 24 lines), Nethermind.Runner.Test/Ethereum/Steps/StartHistoryPrunerTests.cs (new). Read for verification only: HistoryPruner, IHistoryConfig/HistoryConfig, EthereumStepsManager, ReviewBlockTree, BlockTreeModule, and the RunnerStepDependencies attributes of the neighbouring steps.
The DI and ordering mechanics are sound. The step resolves lazily inside RunStep (after WaitForDependencies), so nothing is constructed early and no cycle is introduced; IHistoryPruner is a BlockTreeModule singleton already built by the RPC and sync graphs, so eager resolution here changes nothing. Ordering after ReviewBlockTree is genuinely guaranteed twice over — explicitly, and transitively via InitializeNetwork, which EraStep/EraEStep make a dependent of ReviewBlockTree; the explicit entry is redundant but reads as fair self-documentation. The Enabled() gate is necessary and correct (without it TryPruneHistory would load pointers and fire NewOldestBlock on a pruning-disabled node), and fresh/empty DBs are safe (SyncPivot == 0 and the ancient-bodies hold both short-circuit). No new race with sync or the block processor: the pass clamps to SyncPivot, _currentlyPruning serialises, and the semantics match the pre-existing ProcessingQueueEmpty trigger. Adding a step to BuiltInSteps is the established registration path (cf. StartHistoryWindowPruner via FlatHistoryModule), so the one-line edit to existing code is appropriate.
I'd approve once F1 is addressed or consciously accepted with the <remarks> reworded.
|
@claude review |
|
Claude finished @benaadams's task in 2m 30s —— View job Review:
|
| step | code |
|---|---|
_pruningInterval = PruningInterval * SlotsPerEpoch = 8 * 32 = 256 |
HistoryPruner.cs:128, HistoryConfig.cs:11 |
PruningIntervalHasElapsed() => _pruningInterval == 0 || Head!.Number % _pruningInterval == 0 |
HistoryPruner.cs:533 |
ShouldPruneHistory() returns false on that, before looking at any owed work |
HistoryPruner.cs:514 |
| consulted after pointer load + retention stamping, before every delete path | HistoryPruner.cs:314 |
Head.Number is precisely the quantity that never moves on a node with no CL and no peers — the exact scenario in the PR body. git grep SchedulePruneHistory confirms the only two production triggers are OnBlockProcessorQueueEmpty and this new step, so there is no retry. Net effect on the target scenario: the pass loads and publishes the delete pointer and stamps receipt retention, then bails without deleting anything, unless the stored head happens to sit on a multiple of 256 (~0.4%).
That also makes the <remarks> — "a node that receives no blocks would never prune its backlog" — still true after the change in 255/256 cases. Details and a suggested one-line fix (_firstPassDone bypassing the gate for the process's first pass, no new public surface) are in the inline comment.
Worth noting the manual repro in the PR body ("with the timeout disabled the single pass runs to completion") is consistent with a head that happened to land on the boundary — it doesn't contradict this, and it's the kind of thing that only a head % 256 != 0 test would have caught.
Everything else re-checked and clean
- Registration —
BuiltInStepsModule.cs:32insertion is the established path;EthereumStepsLoaderTests.BuildInSteps_IsCorrectdiffs assembly steps against this list andStepInfoKeyincludes dependencies, so both omission and a silent dependency change fail it. - Ordering —
[RunnerStepDependencies(dependencies: [typeof(InitializeNetwork), typeof(ReviewBlockTree)])]makes the "past the block-tree fixer" half hold by construction; the Era half is transitive viaEraStep'sdependents: [InitializeNetwork], which is fine. No cycle, and no deadlock againstReviewBlockTree's internalProcessingQueueEmptywait sinceStartBlockProcessordoesn't depend on the new step. - Config gate —
if (historyConfig.Enabled())mirrorsHistoryPruner.cs:127/145exactly (Enabled() => Pruning != PruningModes.Disabled,IHistoryConfig.cs:41), so a default-config node is byte-for-byte as on master. This resolves my earlier L-2. - Additivity — one line of existing code touched,
StartBlockProcessoruntouched. Matches the AGENTS.md preference ordering. - Test —
[TestCase]over the threePruningModeswith anexpectedPassescolumn is the right attribute choice per the repo test rules (input→expected pairs, not[Values]), andReceived(n)on the substitute is the real invariant now that the step calls the method directly. Its blind spot is exactly F1: substitutingIHistoryPrunermeans it cannot see that the scheduled pass is a no-op. - Comments — the
<summary>/<remarks>explain why the step exists and what breaks without it; that's the good kind and should stay, but the<remarks>needs to match reality once F1 is settled (either fixed, or reworded to promise only what it delivers).
Unchanged by this PR, for the record: TryPruneHistory still returns early on _blockTree.SyncPivot.BlockNumber == 0 (HistoryPruner.cs:284), so a DB synced from genesis with no pivot still won't prune from this trigger. Pre-existing.
Findings
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | High | StartHistoryPruner.cs:21 → HistoryPruner.cs:533 |
The scheduled pass is gated on Head.Number % 256 == 0, which never becomes true on the idle node this PR targets. The stated goal is not achieved and the <remarks> overstates the guarantee. (detail) |
My earlier M-1 (offline runs need --History.PruningTimeoutSeconds 0) is resolved — it's now in the PR body's testing notes. L-2 and L-3 are resolved in code.
Not verified
dotnet build / dotnet test need approval in this environment, so compilation and the new test are unverified by me. Nothing suggests a gap — Nethermind.Runner.Test already compiles using Nethermind.History, and the attribute form matches StartHistoryWalkVerification.cs.
Verdict: the diagnosis, the shape, and the DI wiring are all right, and this is strictly better than master. But as it stands the fix is inert on its own motivating scenario. I'd land the interval-gate bypass (here or in a follow-up) before merging, or reword the <remarks> so the step doesn't claim a guarantee it can't keep.
· fix/history-pruner-startup-trigger
|
Addressed the startup interval finding in 3e67b48. The first eligible pruning pass bypasses the interval once; missing pivot and ancient-backfill holds preserve that pending pass. Later passes retain the existing interval gate. Validation: History build passed with 0 warnings/errors; 55 HistoryPrunerTests and 3 StartHistoryPrunerTests passed, including the startup and hold regressions. @claude re-review |
|
Claude finished @kamilchodola's task in 3m 29s —— View job Re-review:
|
| path | flag preserved? | correct? |
|---|---|---|
Head is null / SyncPivot.BlockNumber == 0 (:286-288) |
yes — bail is upstream of any write | ✅ |
fast-path skip when nothing is owed (:288) |
yes — ShouldPruneHistory returns false on the work clauses, never clears |
✅ |
TryLoadDeletePointers() false — the ancient-backfill hold (:300) |
yes | ✅ |
ShouldPruneHistory(pending) false under the lock (:318) |
yes | ✅ |
pass actually eligible (:324) |
cleared, then the work runs | ✅ |
So the bypass is consumed by the first pass that has something to do, not merely by the first pass that is attempted — which is the distinction that makes it survive a node whose pointers can't load yet. ShouldPruneHistory still requires _enabled and still requires owed work; only PruningIntervalHasElapsed() is skipped (:520). A disabled node is unchanged.
Walking the motivating scenario: offline snapshot, --History.Pruning UseAncientBarriers --History.PruningTimeoutSeconds 0, no CL → StartHistoryPruner schedules → head non-null, pivot set → pointers load (SynchronizationEnabled: false takes the branch at :379-385) → ShouldPruneHistory(true) bypasses 10_000 % 256 != 0 → prunes. That was inert before this commit; it works now. The <remarks> on StartHistoryPruner is accurate as written again, and the updated SchedulePruneHistory XML doc matches the new contract.
Memory model is fine: the plain write at :324 is published by Monitor.Exit, and the Volatile.Read at :288 only ever errs toward entering the lock, where the value is re-read accurately. The field comment says exactly that, which is the good kind of comment — keep it.
Also re-checked and clean: no new lock ordering (the flag is read/written only inside _pruneLock or on the advisory fast path), no new allocation or hot-path cost, _currentlyPruning CAS still serialises passes, plugin subclasses inherit the behaviour, the flag is per-process so a restart re-arms it, and PruningInterval = 0 configs are unaffected.
Initial_pruning_pass_bypasses_interval_once is a good test — head 100, _pruningInterval 256, so the first pass pruning to #36 is only explainable by the bypass, and the second pass (cutoff moved to #68 via RetentionEpochs = 1) leaving #50 in place is only explainable by the gate reasserting. That pins both halves. Initial_pruning_pass_remains_pending_after_missing_sync_pivot likewise asserts on an actual deletion, so it pins its path.
Findings
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Low | HistoryPrunerTests.cs:546-559 |
Initial_pruning_pass_remains_pending_while_ancient_backfill_is_held asserts only on OnPruningPassStarting, which HistoryPruner calls at :313 — before the interval gate at :318. The assertion passes regardless of whether the flag survived the hold, and the new pruningInterval: 8 argument has no effect on the outcome; reverting the production change leaves this test green. Assert on something gated by ShouldPruneHistory instead (e.g. the persisted delete pointer advancing), as the sibling sync-pivot test does. (detail) |
| 2 | Low | HistoryPruner.cs:93 |
private int _initialPruningPassPending = 1; sits directly under volatile bool _hasLoadedDeletePointers / _stampsValidated, which have identical access patterns. volatile bool would match them and drop the != 0 / Volatile.Read ceremony; the int form is only warranted for _currentlyPruning, which is actually CAS'd. Purely cosmetic — take it or leave it. |
For the record, not findings:
- The bypass is consumed before the work runs, so a first pass truncated by
PruningTimeoutSecondsdoes not get a second interval-free pass. That's the pre-existing continuation semantics and it's harmless in the target scenario (an idle node has no next trigger at all), and the PR body already documents--History.PruningTimeoutSeconds 0for it. Does_not_prune_when_disabledlosing its explicitPruningInterval = 0is safe — theDisabledcase short-circuits on_enabledbefore the interval is consulted.TryPruneHistorystill returns early onSyncPivot.BlockNumber == 0, so a DB synced from genesis with no pivot still won't prune. Pre-existing, unchanged, and now explicitly covered by a test.
Not verified
dotnet build / dotnet test need approval in this environment, so I could not independently reproduce the "0 warnings, 55 + 3 tests passing" run reported in the trigger comment — everything above is from reading the code. Nothing in the diff suggests a compile problem: Volatile is already in scope (used at :241), and the two new test-helper parameters are defaulted so existing call sites are unaffected.
Verdict: F1 is properly fixed and the mechanism is the minimal one — one field, no new public surface, no existing behaviour changed for disabled or interval-zero nodes. Both remaining findings are Low; only #1 is worth acting on, and it's test-only. No blockers from my side.
· fix/history-pruner-startup-trigger
|
Fixed the important regression-test gap in 71ce6df. The ancient-backfill test uses interval 256 (head 10000 is not a boundary), verifies the held pass leaves the delete pointer absent, and verifies the pointer actually advances after the hold is released. This checks work behind the pruning gate, not just the earlier retention callback. All 55 HistoryPrunerTests passed. @claude re-review |
|
Claude finished @kamilchodola's task in 2m 20s —— View job Re-review:
|
| pass | what happens | what the test asserts |
|---|---|---|
| 1 | TryLoadDeletePointers() returns false on the ancient-backfill hold → SkipLocalPruning at HistoryPruner.cs:300, flag untouched |
no OnPruningPassStarting, and HistoryPruningDeletePointer absent |
| 2 | marker set + pointer seeded to 1 → load succeeds, stamps taken, ShouldPruneHistory(pending: true) bypasses the interval → PruneBlocksAndReceipts → UpdateBlocksDeletePointer(target) (:578) |
pointer advanced to CutoffBlockNumber |
The second assertion is only reachable past ShouldPruneHistory (:319), and the interval genuinely has not elapsed on that pass: pruningInterval: 256 × SlotsPerEpoch = _pruningInterval 8192, head is 10_000, 10000 % 8192 = 1808 ≠ 0. So:
- revert
_initialPruningPassPending→ pass 2 bails at the gate, pointer stays1, test fails ✅ - move the
_initialPruningPassPending = 0clear above theTryLoadDeletePointersbail → pass 1 consumes it, pass 2 bails, test fails ✅
Both mutants that the previous version survived are now caught. Seeding the pointer to 1 is the right setup, too — without it TryLoadDeletePointers discovers the frontier at 9,000, which is already past the cutoff, so there'd be no owed work for ShouldPruneHistory to find and the assertion would be vacuous in the other direction.
Also re-checked and unchanged from my last pass: Does_not_prune_when_pruning_is_disabled_or_sync_pivot_is_missing actually gained teeth from the bypass (with the default PruningInterval = 8 and head 100 the interval never elapses, so pre-bypass the Disabled case would have been blocked by the gate rather than by _enabled); Initial_pruning_pass_bypasses_interval_once pins both halves via head 100 % 256 ≠ 0; the flag's memory model, lock discipline, and the Enabled()/ReviewBlockTree wiring in StartHistoryPruner are all as previously verified.
Findings
None at Medium or above. One carried-over cosmetic note, unchanged and entirely optional:
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Low | HistoryPruner.cs:93 |
private int _initialPruningPassPending = 1; sits next to volatile bool _hasLoadedDeletePointers / _stampsValidated, which have identical access patterns. A volatile bool would match them and drop the != 0 / Volatile.Read ceremony; the int form only earns its keep for _currentlyPruning, which is genuinely CAS'd. Take it or leave it. |
Two things I looked at and decided are not findings:
- The flag is cleared at
:324before the work runs, so a first pass aborted by an exception (e.g.VerifyReclaimSupportedon an unsupported layout) consumes the bypass. That exception is deterministic and would recur on every pass anyway, so nothing is lost. - A first pass truncated by
PruningTimeoutSecondslikewise doesn't get a second interval-free pass — pre-existing continuation semantics, and the PR body already documents--History.PruningTimeoutSeconds 0for the offline case.
Comment quality: the field comment at :92 and the updated SchedulePruneHistory <summary> both state the contract rather than restating the code — keep them.
Not verified
dotnet test needs approval in this environment, so I could not reproduce the reported "55 HistoryPrunerTests passing" run myself — everything above is from reading the code. Nothing in the diff suggests a compile problem: Rlp, RlpReader.DecodeULong(), and IPrunedReceiptRetention.RetainedHeights all exist with the signatures the test uses, and Nethermind.Serialization.Rlp / System.Collections.Generic are already imported in the file.
Verdict: the regression-test gap is properly closed and the mechanism is verified end-to-end. Nothing blocking from my side — the only open item is a one-word cosmetic suggestion.
· fix/history-pruner-startup-trigger
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
Re-reviewed all five changed files at 71ce6df. The previous interval-gate and ancient-backfill regression-test findings are resolved. Two remaining findings are inline. Verification was by source inspection; tests were not run.
| { | ||
| public Task Execute(CancellationToken cancellationToken) | ||
| { | ||
| if (historyConfig.Enabled()) historyPruner.SchedulePruneHistory(); |
There was a problem hiding this comment.
[P2] The documented offline workaround still times out. This schedules one pass, but History.PruningTimeoutSeconds=0 becomes timeout: null in HistoryPruner.SchedulePruneHistory, and BackgroundTaskScheduler.TryScheduleTask replaces null with its two-second DefaultTimeout; Activity.Do then cancels at that deadline. An idle node can therefore still leave its backlog unfinished with no next trigger. The timeout mismatch predates this PR, but invalidates the PR body's claim that disabling the timeout lets this single pass run to completion. Correct the timeout handling with a real-scheduler regression test, or correct the completion claim; the existing capture-only scheduler test does not exercise deadline cancellation.
There was a problem hiding this comment.
Confirmed, and it's sharper than "still times out": IHistoryConfig.cs:35 documents "Set to 0 to disable the timeout", but 0 → null → BackgroundTaskScheduler.cs:225 timeout ?? DefaultTimeout = 2 s, i.e. identical to the PruningTimeoutSeconds default. The documented option is a no-op, so the PR body's offline workaround changes nothing.
Argues for fixing it here rather than only correcting the body: map 0 to a bounded far-future deadline instead of null (Activity.Do calls cts.CancelAfter, so it has to stay under ~24 days).
Marchhill
left a comment
Marchhill
left a comment
There was a problem hiding this comment.
Mechanism verified: the startup ProcessingQueueEmpty (BlockchainProcessor.cs:349) really has no subscriber — every IHistoryPruner consumer sits in InitializeNetwork or later, and with SynchronizationEnabled=false SyncServer isn't built at all, so the pruner was never constructed. The one-shot bypass is correctly scoped (consumed only under _pruneLock after ShouldPruneHistory passes, so a backfill hold or missing pivot doesn't spend it) and both new tests positive-control it — head 100 % 256 != 0, so pass 1 runs only via the bypass and pass 2 is throttled. History/Runner lanes green on 71ce6df.
Holding off approval on @LukaszRozmej's P2, which is worse than stated: PruningTimeoutSeconds=0 is documented as "disable the timeout" but resolves to the scheduler's 2 s default, so the workaround this PR prescribes for its own motivating scenario is a no-op. Evidence on that thread.
Minor — the title ("create the history pruner before the block processor starts") no longer describes the change: StartBlockProcessor is untouched and the new step runs after InitializeNetwork.
4 participants
Changes
StartHistoryPrunerthat callsIHistoryPruner.SchedulePruneHistory()onceInitializeNetworkhas completed.StartHistoryPrunerTestsasserting the step schedules a pass.HistoryPruneronly ever schedules a pruning pass fromIBlockProcessingQueue.ProcessingQueueEmpty.BlockchainProcessorraises that once when its loop starts and afterwards only after a processed block. Nothing resolves the pruner untilInitializeNetwork(viaSyncServer) orRegisterRpcModules, both of which run afterStartBlockProcessor, so the startup signal has no subscriber. On a live node the next processed block hides this. A node that receives no blocks (no CL attached yet, or an offline maintenance run with--History.Pruningenabled to trim an existing database) never prunes, and the skip is only visible at Trace level.The step depends on
InitializeNetwork, which is where the pruner used to be constructed:EraStep/EraESteplistInitializeNetworkas a dependent and themselves depend onReviewBlockTree, so the first pass still starts after the block-tree fixer and any Era import, exactly as before. No existing step is modified.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Found while pruning a mainnet flat snapshot offline with
--History.Pruning UseAncientBarriersand networking disabled: the node started cleanly and never loggedPruning historical blocks up to.EthereumStepsLoaderTests.BuildInSteps_IsCorrectcovers the registration of the new step.For an offline pruning run (no CL, no peers) also pass
--History.PruningTimeoutSeconds 0: this step supplies the first pass only, and with the default 2 s deadline a pass stops at the next chunk boundary and waits for aProcessingQueueEmptysignal that an idle node never receives. The delete pointer is published in full before the reclaim loop, so without the timeout disabled the node reports pruned history while most of the disk is still occupied. With the timeout disabled the single pass runs to completion.Documentation
Requires documentation update
Requires explanation in Release Notes