fix(core): clean up dangling/orphan blocks on SetHead#2442
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR ports upstream geth HeaderChain.SetHead cleanup behavior into XDPoSChain to ensure rewinds fully delete orphan/dangling block data above the rollback target, preventing stale hash-keyed block data from misleading the downloader’s ancestor search after restarts.
Changes:
- Add
rawdb.ReadAllHashesto enumerate all block hashes stored at a given height. - Extend
core/HeaderChain.SetHeadto also delete data for all hashes at each rewound height and to additionally wipe “leftover” heights above the current head on the first iteration. - Add
core/blockchain_sethead_test.goto verify dangling blocks above head are fully wiped bySetHead.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| core/rawdb/accessors_chain.go | Adds ReadAllHashes helper to enumerate hashes stored at a specific block height. |
| core/headerchain.go | Updates HeaderChain.SetHead to remove side-fork/orphan data and leftover heights above head during rewinds. |
| core/blockchain_sethead_test.go | Adds regression test ensuring dangling block data above head is removed after SetHead. |
2d3c33e to
c14dd35
Compare
SetHead rewinds the chain to a lower block height. Without comprehensive cleanup, the downloader can jump to stale blocks instead of resyncing intermediate ranges. SetHead previously only deleted the canonical chain segment, leaving orphaned/fork blocks intact. The downloader's HasBlock() check would hit these orphaned entries, incorrectly reporting ancestors found, skipping intermediate resync. Root cause: Original implementation used a contiguous upward scan (checking head+1, head+2, ...) that stopped at the first height with no header, leaving orphaned segments above gaps intact. Real-world data showed orphaned fork segments can sit ABOVE gaps (e.g., head at 83,531,000 with no markers at 83,531,501–83,532,999 but orphaned data at 83,533,000–83,599,999). Solution: Replace contiguous scan with ReadAllDanglingHashes(), which walks the header keyspace directly via iterator and returns ALL headers above head regardless of gaps. SetHead now performs post-loop sweep deleting all header/td/ body/receipt/canonical data above the new head. For bad blocks: Only delete records strictly above the new head, preserving debugging data for blocks at/below head for operator review (respects DeleteAllBadBlocks config). Optimization: Use two-phase batch commits to avoid duplicate deletes. Flush rewind deletions to database before scanning for dangling data, preventing ReadAllDanglingHashes from re-observing already-deleted segments. Changes: - Add ReadAllDanglingHashes(db, head) for iterator-based orphan detection - Add DeleteBadBlocksAbove(db, head) for selective bad block cleanup - Refactor SetHead() with two-phase batch writes and iterator-based sweep - Add TestSetHeadCleansDanglingBlocksAcrossGap for gap-handling validation - Add TestSetHeadCleansOnlyBadBlocksAboveHead for bad block preservation Verification: - 5/5 SetHead unit tests pass - Remote testnet: rollback 83,531,500→83,531,000 cleaned 69,000 heights, sequential block import without bad block jumps - Full core test suite passes
Proposed changes
SetHead rewinds the chain to a lower block height. Without comprehensive cleanup,
the downloader can jump to stale blocks instead of resyncing intermediate ranges.
SetHead previously only deleted the canonical chain segment, leaving orphaned/fork
blocks intact. The downloader's HasBlock() check would hit these orphaned entries,
incorrectly reporting ancestors found, skipping intermediate resync.
Root cause: Original implementation used a contiguous upward scan (checking
head+1, head+2, ...) that stopped at the first height with no header, leaving
orphaned segments above gaps intact.
Solution: Replace contiguous scan with ReadAllDanglingHashes(), which walks
the header keyspace directly via iterator and returns ALL headers above head
regardless of gaps. SetHead now performs post-loop sweep deleting all header/td/
body/receipt/canonical data above the new head.
For bad blocks: Only delete records strictly above the new head, preserving
debugging data for blocks at/below head for operator review (respects
DeleteAllBadBlocks config).
Optimization: Use two-phase batch commits to avoid duplicate deletes.
Changes:
Verification: 5/5 unit tests pass, remote testnet verified
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that