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

pull mzumsande wants to merge 5 commits into bitcoin:master from mzumsande:202609_fix_cbi_pruning_unlinked changing 6 files +106 −40

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_blocks_unlinked that don't have m_chain_tx_count set, 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
DrahtBot added the label Validation on Sep 2, 2026
DrahtBot commented at 2:33 PM on September 2, 2026: contributor


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



External sites
- Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36149
- Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36149.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36149#author-ralph


Reviews

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

Type Reviewers

Concept ACK l0rinc, jeanpablojp

Stale ACK marcofleon

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #36621 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36621.svg"></sub> (node: batch block file pruning in a single pass over block index by mutatrum)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
mzumsande force-pushed on Sep 2, 2026
DrahtBot added the label CI failed on Sep 2, 2026
DrahtBot removed the label CI failed on Sep 2, 2026
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.
mzumsande marked this as ready for review on Sep 2, 2026
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.

Type	Reviewers
Concept ACK	l0rinc, jeanpablojp
Stale ACK	marcofleon

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

mzumsande commented at 5:31 PM on November 2, 2026:

done

in test/functional/feature_prune_stale_fork.py:64 in 40a31d97ab outdated
```
  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?

mzumsande commented at 5:34 PM on November 2, 2026:

Makes sense - since there were two other spots in the vicinity that had a similar issue I added a separate commit that changes these.
in test/functional/feature_prune_stale_fork.py:48 in 5d5af40fcb outdated
```
  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?

mzumsande commented at 5:40 PM on November 2, 2026:

I added a generate() call before the restart, so that CheckBlockIndex is invoked. I think that should cover the situation, too.
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!
DrahtBot requested review from jeanpablojp on Sep 9, 2026
DrahtBot requested review from l0rinc on Sep 9, 2026

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.

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.
mzumsande marked this as a draft on Sep 9, 2026

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 as long as they
don't have their m_chain_tx_count yet, and also removing the
BLOCK_HAVE_DATA requirement during startup. Also don't add pruned blocks
to setBlockIndexCandidates when linking them, since they can't be
connected before being downloaded again.

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

12208a02ff

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.

0522e7beba

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.

92c02d4221

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.

07ee4b98c6

doc: clarify getchaintips status descriptions

Whether a branch is "headers-only" or "valid-headers" depends on whether
the transactions of all its blocks have been received at some point, not
on whether the block data is currently available - blocks may have been
pruned since.

532ffaa26d

mzumsande force-pushed on Nov 2, 2026
mzumsande commented at 5:59 PM on November 2, 2026: contributor

40a31d9 to 532ffaa: In addition to addressing review comments, I changed the approach of the first commit, taking some things of a branch from @l0rinc (thanks, added you as coauthor).

In the previous PR branch, pruning never removed anything from m_blocks_unlinked. Now, a pruned block is only kept there if it doesn't have its m_chain_tx_count yet, i.e. if it is still waiting for an ancestor's transactions. Blocks that are already linked are removed on pruning, as on master. Also, blocks without data are no longer added to setBlockIndexCandidates when they get linked.

The approaches are very similar, the main functional difference is for the (admittedly far-fetched) case that a block is pruned, downloaded again (e.g. via getblockfrompeer) and then pruned again. Another advantage is that we can now have slightly stricter checks in CheckBlockIndex().
mzumsande marked this as ready for review on Nov 2, 2026
mzumsande renamed this:
~~validation: Leave pruned blocks in `m_blocks_unlinked`, fix another `nSequenceId` issue~~
validation: Leave some pruned blocks in `m_blocks_unlinked`, fix another `nSequenceId` issue
on Nov 2, 2026

Contributors

Review Requested
l0rinc jeanpablojp

Labels

Validation

Linked (view graph)

#31512 validation: CheckBlockIndex can fail in combination with pruning #34521 validation: fix UB in `LoadChainTip`#35070 validation: prevent FindMostWorkChain from causing UB #36021 validation: `Assertion failed: ((pindexFirstNeverProcessed == nullptr || pindex == snap_base) == pindex->HaveNumChainTxs())`#36702 validation: don't treat snapshot block as linked on startup

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

External sites

Reviews

Conflicts