#36149 validation: Leave pruned blocks in `m_blocks_unlinked`, fix another `nSequenceId` issue

full analysis

https://github.com/bitcoin/bitcoin/pull/36149 · mzumsande · +69/-37 in 5 files, 4 commits · labels: Validation · draft

Goal

  • Prevent debug crashes and block candidate corruption when handling pruned stale forks
  • Ensure late-arriving ancestor blocks can properly propagate transaction counts
  • Allow fuzz testing to safely exercise pruning on out-of-chain blocks

This pull request retains pruned blocks in `m_blocks_unlinked` so that ancestor transactions arriving later still update transaction counts across the fork. It also removes blocks from `setBlockIndexCandidates` before updating their `nSequenceId` to prevent set order corruption when pruned blocks are re-downloaded. In addition, it updates the functional test suite and extends the `block_index_tree` fuzzer to prune out-of-chain blocks.

Problem: When the parent of a previously pruned block arrives, the parent is unable to propagate transaction counts down the chain because pruning prematurely removed entries from `m_blocks_unlinked`. This causes `CheckBlockIndex` assertion failures (#31512, #36021) and corrupts candidate set ordering if a block is re-downloaded.

Category: Validation (#4 of 48)

P2 · bug fix

  • P2 because it fixes assertion crashes and candidate corruption during block redownloads
  • Addresses rare crashes triggered by debug checks when stale forks are pruned
  • Enables fuzzers to explore out-of-chain pruning paths without hitting false crashes

Fixes assertion failures in `CheckBlockIndex` and candidate set ordering corruption when handling pruned stale forks. As the author notes, while rare under normal operation without large reorgs, this occurs during `getblockfrompeer` flows and triggers crashes in debug builds (#31512, #36021). It also unblocks the `block_index_tree` fuzzer from exercising out-of-chain block pruning.

Membership: Modifies chain candidate tracking and block storage structures in src/validation.cpp, src/node/blockstorage.cpp, and src/node/blockstorage.h.

Factors: security/stability 2, bug 2, performance 0, user value 1, leverage 2

Reviewability: Paused: Author reworking

  • Wait for author to finish exploring an alternative conditional removal approach before reviewing

The author moved the PR to draft to explore an alternative conditional removal approach suggested during review before pushing updates.

Author status: said wait while exploring an alternative conditional removal approach

Open concerns:

  • The author is investigating an alternative condition suggested by l0rinc to remove unlinked entries only after `m_chain_tx` is set.
  • marcofleon suggested an additional test case for submitting missing parents before node restart.

Resolved concerns:

  • Fuzz verification confirmed that erasing candidates before updating `nSequenceId` prevents corpus crashes on re-downloaded blocks.

Agreement: Strong

  • Strong support with full review and fuzz reproduction
  • Verified the fixes for unlinked chain tracking and candidate set ordering (marcofleon)
  • Verified fuzz reproduction of candidate corruption on existing seeds (jeanpablojp)
  • Concept approval without objections (l0rinc)

Strong approval with full review and fuzz reproduction; author exploring a minor rework.

marcofleon tested and ACKed the fix for both the unlinked chain tracking and the `nSequenceId` sorting bug. l0rinc and jeanpablojp provided Concept ACKs with no objections raised.

  • marcofleon gave a tested ACK, noting the main fix looks correct and the sequence ID change fixes potential UB in setBlockIndexCandidates caught by fuzzing.
  • jeanpablojp confirmed that without the setBlockIndexCandidates erase, the extended fuzz target fails on existing qa-assets seeds.
  • l0rinc gave a Concept ACK.

Review verdicts (DrahtBot): 1

Files

56 lines under test/bench/ci.

  • test/functional/feature_prune_stale_fork.py +37/-3
  • src/node/blockstorage.cpp +6/-16
  • src/validation.cpp +14/-3
  • src/test/fuzz/block_index_tree.cpp +2/-14
  • src/node/blockstorage.h +10/-1

Card

This PR fixes two validation bugs that occur when receiving missing ancestors of pruned blocks. By keeping pruned blocks in m_blocks_unlinked, transaction counts correctly propagate down the fork when parents arrive, preventing CheckBlockIndex assertion failures (#31512, #36021). It also avoids undefined set ordering by removing candidate blocks before reassigning nSequenceId on re-downloaded blocks, allowing the block_index_tree fuzzer to prune out-of-chain blocks. The change is supported by reviewer ACKs and fuzz verification, but the author has paused review to explore a minor conditional-removal refinement.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T16:23, confidence high, input hash ee43223efb6c3c73