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

1. 
   mzumsande commented at 2:33 PM on September 2, 2026: contributor

   m_blocks_unlinked serves two purposes:

   1. finding blocks to setBlockIndexCandidates once their parents are received
   2. setting m_chain_tx_count once the parents are received

   In case the parent block of a previously pruned block is received, 2. is currently not being done, because pruning removed the entries from m_blocks_unlinked. This can result in CheckBlockIndex failures (#31512, #36021) and incorrect rpc results, plus we are in a temporarily inconsistent state (m_chain_tx_count is currently not set, but would get set if we restarted).

   This scenario is unlikely to happen during normal node operation unless there are huge reorgs, but it can happen, for example, in getblockfrompeer scenarios. Fix this by leaving pruned blocks in m_chain_tx_count, and also adding them on restart.

   This was one of two reasons, the fuzz target block_index_tree couldn't cover pruning of blocks not in the main chain.

   The other one is similar to #34521 and #35070 - if we receive a block a second time (after pruning), we could change nSequenceId while the block is in setBlockIndexCandidates - fix this by attempting to remove it from the set before re-adding it.

   After the two issues are fixed, the restriction from block_index_tree fuzz target is removed.

   Fixes #36021 Fixes #31512

2. DrahtBot added the label Validation on Sep 2, 2026
3. 
   DrahtBot commented at 2:33 PM on September 2, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36149.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   ACK	marcofleon
   Concept ACK	l0rinc, jeanpablojp

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

4. validation: keep pruned blocks in m_blocks_unlinked

   m_blocks_unlinked serves two purposes:
   1. finding blocks to SetBlockIndexCandidates once their parents are
      received
   2. Setting m_chain_tx_count once the parents are received
   
   In case that a parent of a pruned block is received,
   2. is currently not being done, because pruning removed the entries.
   This can result in CheckBlockIndex failures, and an inconsistent state
   (because in case of a restart, m_chain_tx would be set).
   This is unlikely to happen during normal node operation unless
   there are huge reorgs, but it can happen, for example, in getblockfrompeer scenarios.
   
   Fix this by keeping pruned blocks in m_blocks_unlinked and also removing
   the BLOCK_HAVE_DATA requirement during startup.

   7c7ccc9cc0
5. test: add functional test for parents of pruned blocks

   This test would lead to a CheckBlockIndex assertion fail without
   the changes of the previous commit.

   5d5af40fcb
6. validation: don't change nSequenceId of a block that is a chain candidate

   nSequenceId is one of the sort keys of setBlockIndexCandidates, so changing it
   for a block that currently is in one of these sets corrupts their ordering,
   after which lookups for other entries can fail.
   
   This is also necessary for the block_index_tree fuzz test extension in the
   following commit, which would otherwise trip on this.

   80477ba978
7. fuzz: allow to prune out-of-chain blocks in block_index_tree

   After the issues with m_blocks_unlinked and
   setBlockIndexCandidates have been resolved in previous commits,
   the previous restriction to the fuzz test does no longer apply,
   and we can also prune out-of-chain blocks.

   40a31d97ab
8. mzumsande force-pushed on Sep 2, 2026
9. DrahtBot added the label CI failed on Sep 2, 2026
10. DrahtBot removed the label CI failed on Sep 2, 2026
11. 
    l0rinc commented at 5:29 PM on September 2, 2026: contributor

    Concept ACK, thanks for fixing it. I will try to review it a bit later.

12. mzumsande marked this as ready for review on Sep 2, 2026
13. 
    jeanpablojp commented at 12:32 AM on September 8, 2026: contributor

    Concept ACK

    Taking out the setBlockIndexCandidates erase, the extended block_index_tree target breaks on an input that is already in the qa-assets corpus.

14. in src/node/blockstorage.h:355 in 40a31d97ab

     350 | @@ -351,7 +351,16 @@ class BlockManager
     351 |      std::vector<CBlockIndex*> GetAllBlockIndices() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     352 |  
     353 |      /**
     354 | -     * All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions.
     355 | +     * Pairs A->B, where the transactions of B have been received at some point,
     356 | +     * but B is not a candidate for connection, because for A or one of its predecessors
    

    ---

    ---

    

    jeanpablojp commented at 12:32 AM on September 8, 2026:

    nit (feel free to ignore): predecessors appears only here in src/, and ancestors sounds to be the preference, including the comment this line replaces.

         * but B is not a candidate for connection, because for A or one of its ancestors
    

15. in test/functional/feature_prune_stale_fork.py:64 in 40a31d97ab

      59 | +        node.submitblock(side_parent.serialize().hex())
      60 | +        assert_equal(node.getblockheader(side_parent.hash_hex)["nTx"], 1)
      61 | +        # The child's block data is still gone, but its transactions were received at some
      62 | +        # point, so the grandchild is no longer waiting for transactions the node has.
      63 | +        assert_raises_rpc_error(-1, "Block not available (pruned data)", node.getblock, side_child.hash_hex)
      64 | +        assert_equal(self.tip_status(node, side_grandchild.hash_hex), "valid-headers")
    

    ---

    ---

    

    jeanpablojp commented at 12:32 AM on September 8, 2026:

    Pre-existing, so feel free to ignore. This assertion is what made me look.

    The getchaintips help calls valid-headers "All blocks are available for this branch", but the only availability test behind the status is HaveNumChainTxs(), whose own doc says it "Does not imply the transactions are still stored on disk". I get the same on master with a stale fork received in full and then pruned, where getblock on those blocks fails with "Block not available (pruned data)".

    Would "received at some point" be closer than "are available" there?

16. in test/functional/feature_prune_stale_fork.py:48 in 5d5af40fcb

      47 | @@ -34,6 +48,26 @@ def run_test(self):
      48 |          self.restart_node(0)
    

    ---

    ---

    

    marcofleon commented at 11:01 AM on September 9, 2026:

    We could also test submitting a missing parent of a pruned child before restarting, as in #36021. This test currently passes with the old behavior of removing a pruned block from m_blocks_unlinked, because it gets added back on start up. Could add a separate 2-block fork to cover the no restart case?

17. 
    marcofleon commented at 11:24 AM on September 9, 2026: contributor

    ACK 40a31d97ab118cb030af7d560aedbade3ae4cd36

    The main fix in 7c7ccc9cc0939e00f060b49ec476a761f2c1c85e looks correct to me. Ran the fuzz test for a while, no issues. Commit 80477ba97820485c8371f71d0b46b7bb23325cf2 fixes previously existing (potential) UB in setBlockIndexCandidates that is now caught by the fuzz target. Nice.

    In the PR description,

    Fix this by leaving pruned blocks in m_chain_tx_count

    should be m_blocks_unlinked instead.

    Also, left a non-blocking comment on the functional test.

    Thanks for fixing this cleanly!

18. DrahtBot requested review from jeanpablojp on Sep 9, 2026
19. DrahtBot requested review from l0rinc on Sep 9, 2026
20. in src/node/blockstorage.cpp:284 in 7c7ccc9cc0

     291 | -                range.first++;
     292 | -                if (_it->second == pindex) {
     293 | -                    m_blocks_unlinked.erase(_it);
     294 | -                }
     295 | -            }
     296 | +            // Note that the block is deliberately left in m_blocks_unlinked:
    

    ---

    ---

    

    mzumsande commented at 9:49 PM on September 9, 2026:

    A slightly different approach here (suggested by @l0rinc) would be to remove conditionally on m_chain_tx being set. I will explore this next week.

21. 
    mzumsande commented at 9:50 PM on September 9, 2026: contributor

    Thanks for the reviews! I will address the comments (and also most likely rework the PR a little bit, see other comment) next week, putting into draft until then.

22. mzumsande marked this as a draft on Sep 9, 2026

Review Requested
l0rincjeanpablojp