fix: guard ClearStaleMarkersAbove behind forceUpdateHeadBlock#11004
fix: guard ClearStaleMarkersAbove behind forceUpdateHeadBlock#11004
Conversation
…d hot-path scan PR #10876 added an unconditional upward scan in UpdateMainChain that clears stale canonical markers left by beacon sync. The scan runs on every call and walks until LoadLevel returns null, which is O(K) per block during forward sync (BlockDownloader) and O(K²) total when BlockchainProcessor falls behind — also incorrectly clearing valid beacon-synced markers ahead of the processing front. The scan is only needed during FCU reorgs, which is the only path that passes forceUpdateHeadBlock: true. Guard the call so forward sync and forward block processing skip the scan entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5f8057c to
9b2cbac
Compare
|
@claude review |
|
Claude finished @LukaszRozmej's task in 3m 2s —— View job Code Review
SummaryThe fix is correct and well-targeted. The one-line guard properly restores the pre-#10876 hot-path behavior for both
|
| Caller | forceUpdateHeadBlock |
Effect after fix |
|---|---|---|
ForkchoiceUpdatedHandler.cs:258 |
true |
Clears stale markers — correct |
BlockTreeOverlay.cs:28 |
true |
Clears stale markers at init — correct |
BlockchainProcessor.cs:502 |
false (default) |
Skips scan — correct |
BlockDownloader.cs:553 |
false (default) |
Skips scan — correct |
NonProcessingProducedBlockSuggester.cs:24 |
false (default) |
Skips scan — correct |
The complexity claims are accurate: BlockDownloader calling UpdateMainChain([block], false) per block was causing O(K) per-block scans (K = levels above head), and BlockchainProcessor processing blocks above a beacon-synced region was causing O(K²) total work plus temporarily clearing valid markers needed for canonical lookups.
Minor comment inaccuracy (cosmetic, not a bug):
"The scan is only needed during FCU reorgs — the only production path that passes
forceUpdateHeadBlock: true(ForkchoiceUpdatedHandler)."
BlockTreeOverlay.cs:28 also passes true. The claim in the PR description is slightly overstated, but it doesn't affect correctness since BlockTreeOverlay is an initialization step and behaves correctly with the guard.
BlockTreeTests.cs — New tests
Test 1: UpdateMainChain_WhenForwardProcessingWithBeaconSyncedDescendants_DoesNotClearMarkers
The test correctly models the regression scenario: FCU sets head to block 1, forward sync marks blocks 2–4, then forward processing of block 2 (forceUpdateHeadBlock: false) must not erase markers for blocks 3 and 4. Assertions on chain[2] and chain[3] are appropriate.
Test 2: UpdateMainChain_WhenFcuForwardReorgToLongerChain_ClearsStaleMarkersAboveNewHead
The test correctly verifies that FCU (forceUpdateHeadBlock: true) to chain B (height 3) clears the stale chain-A marker at height 4 via ClearStaleMarkersAbove(max(1, 3) = 3, batch). The math checks out.
Minor naming nit — WhenFcuForwardReorgToLongerChain is slightly misleading: chain B stops at height 3 while chain A reaches height 4, so B is not strictly longer. Something like WhenFcuReorgToDifferentChain would be more accurate. This is cosmetic only.
Overall
The fix is minimal, focused, and correct. The new tests cover both the "preservation" and "clearing" behaviours. All existing #10876 regression tests remain valid since they go through the FCU (forceUpdateHeadBlock: true) path. No concerns beyond the cosmetic items noted above.
fix: guard ClearStaleMarkersAbove behind forceUpdateHeadBlock to avoid hot-path scan PR #10876 added an unconditional upward scan in UpdateMainChain that clears stale canonical markers left by beacon sync. The scan runs on every call and walks until LoadLevel returns null, which is O(K) per block during forward sync (BlockDownloader) and O(K²) total when BlockchainProcessor falls behind — also incorrectly clearing valid beacon-synced markers ahead of the processing front. The scan is only needed during FCU reorgs, which is the only path that passes forceUpdateHeadBlock: true. Guard the call so forward sync and forward block processing skip the scan entirely. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes a performance regression from #10876 that significantly slows down forward sync on nodes that are far behind.
Summary
ClearStaleMarkersAbove) inUpdateMainChainto clear stale canonical markers left by beacon sync. The scan walks upward from head untilLoadLevelreturns null on everyUpdateMainChaincall.BlockDownloadercallsUpdateMainChain(block, false)per block. The scan traverses all pre-downloaded header levels above — O(K) per block, where K can be millions of levels on a node syncing from scratch.BlockchainProcessorfalls behind, it callsUpdateMainChain(blocks, true)per processed block. The scan clears valid beacon-synced markers ahead of the processing front, causing O(K²) total work and temporarily breaking canonical lookups for blocks above.forceUpdateHeadBlock: true(ForkchoiceUpdatedHandler).Fix: Guard the call with
if (forceUpdateHeadBlock)so forward sync and forward block processing skip the scan entirely.Changes
One line in
BlockTree.cs: wrapClearStaleMarkersAbove(...)withif (forceUpdateHeadBlock).All 15 existing #10876 regression tests pass unchanged (they use
forceUpdateHeadBlock: truefor the FCU step). Two new tests verify:Types of changes
Testing
🤖 Generated with Claude Code