validation: Don't add pruned blocks to `m_blocks_unlinked` on startup #35168

pull marcofleon wants to merge 2 commits into bitcoin:master from marcofleon:2026/04/loadblockindex-unlinked-fix changing 3 files +43 −1

marcofleon commented at 6:53 PM on April 27, 2026: contributor

Fixes #35050

The m_blocks_unlinked map keeps track of blocks that have transactions but whose parent (or any ancestor) does not. This happens when a block is received before its parent, or during a reorg, when FindMostWorkChain() encounters a block whose ancestors were pruned.

The bug this PR addresses is a rare interaction of these two cases, which happens on startup when BlockManager::LoadBlockIndex() rebuilds m_blocks_unlinked. The check there only considers whether a block has transactions, and pruned blocks keep nTx > 0 but clear BLOCK_HAVE_DATA. So if there's a pruned block on a stale fork whose parent has no transactions, that block is added to m_blocks_unlinked without having data on disk. This violates an assertion in CheckBlockIndex().

Get rid of this unintended case by gating on BLOCK_HAVE_DATA before adding to m_blocks_unlinked.

validation: Don't add pruned blocks to m_blocks_unlinked on startup

LoadBlockIndex() adds to m_blocks_unlinked based only on nTx > 0, without
checking BLOCK_HAVE_DATA. Pruning preserves nTx but clears BLOCK_HAVE_DATA,
so a pruned block whose parent was header-only gets re-added on every
restart, causing the CheckBlockIndex() assertion that entries must have
data on disk to fail.

Check that BLOCK_HAVE_DATA is set before inserting into m_blocks_unlinked.

Fixes #35050.

0e4b0bacec

DrahtBot added the label Validation on Apr 27, 2026
DrahtBot commented at 6:53 PM on April 27, 2026: contributor



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



Code Coverage & Benchmarks

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



Reviews

See the guideline for information on the review process.

Type Reviewers

Concept ACK mzumsande

Stale ACK ViniciusCestarii, sedited

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

Type	Reviewers
Concept ACK	mzumsande
Stale ACK	ViniciusCestarii, sedited

in test/functional/feature_prune_unlinked.py:24 in 546c27181e

  19 | +
  20 | +        self.log.info("Create a 2-block stale fork with a header-only parent and a full child")
  21 | +        fork_point = node.getblockhash(1)
  22 | +        tip_time = node.getblock(node.getbestblockhash())["time"] + 1
  23 | +
  24 | +        side_parent = create_block(int(fork_point, 16), create_coinbase(2), ntime=tip_time)

maflcko commented at 8:27 AM on April 28, 2026:

        side_parent = create_block(int(fork_point, 16), height=2, ntime=tip_time)

nit: Haven't tried this, but I think in new code you can use the suggestion, if it works.

marcofleon commented at 11:03 AM on April 28, 2026:

Thanks, fixed.

marcofleon force-pushed on Apr 28, 2026
ViniciusCestarii commented at 1:11 PM on April 28, 2026: contributor

tACK 0cc9be25f65eb82009f6f0e4b242faca2a7728cf ran the new functional test, also verified the assert fires without the fix

in test/functional/feature_prune_unlinked.py:27 in 0cc9be25f6

  22 | +        tip_time = node.getblock(node.getbestblockhash())["time"] + 1
  23 | +
  24 | +        side_parent = create_block(int(fork_point, 16), height=2, ntime=tip_time)
  25 | +        side_parent.solve()
  26 | +        side_child = create_block(side_parent.hash_int, height=3, ntime=tip_time + 1)
  27 | +        side_child.solve()

sedited commented at 8:46 PM on April 28, 2026:

Nit: Do we need this extra buffer instead of just forking off the current tip? This could be one line otherwise:

[side_parent, side_child] = create_empty_fork(node, 2)

marcofleon commented at 12:05 PM on May 12, 2026:

Fixed and tested that it works the same.

in test/functional/feature_prune_stale_fork.py:36 in 0cc9be25f6 outdated

  31 | +        assert_equal(node.getblockheader(side_parent.hash_hex)["nTx"], 0)
  32 | +        assert_equal(node.getblockheader(side_child.hash_hex)["nTx"], 1)
  33 | +
  34 | +        self.log.info("Advance and prune so the stale-fork child loses BLOCK_HAVE_DATA")
  35 | +        self.generate(node, 500)
  36 | +        node.pruneblockchain(node.getblockcount() - 100)

sedited commented at 8:53 PM on April 28, 2026:

Nit: Maybe check that the block is actually pruned here:

assert_raises_rpc_error(-1, "Block not available (pruned data)", node.getblock, side_child.hash_hex)

marcofleon commented at 12:05 PM on May 12, 2026:

done

sedited approved
sedited commented at 8:58 PM on April 28, 2026: contributor

ACK 0cc9be25f65eb82009f6f0e4b242faca2a7728cf

Can you add something along the lines of stratospher's explanation from the issue to the pull request description? It reads a bit dry at the moment.
in test/functional/feature_prune_unlinked.py:12 in 0cc9be25f6
```
   7 | +from test_framework.blocktools import create_block
   8 | +from test_framework.test_framework import BitcoinTestFramework
   9 | +from test_framework.util import assert_equal
  10 | +
  11 | +
  12 | +class FeaturePruneUnlinkedTest(BitcoinTestFramework):
```
mzumsande commented at 4:13 PM on April 29, 2026:

Not sure about naming functional tests after internal structures that are not exposed to users. In my opinion functional test should test externally observable behavior that would also happen in non-debug builds where CheckBlockIndex doesn't run.

While it's great to have the test as a proof that this is really an issue, I am a bit undecided if a permanent functional test for this rather theoretical constellation is really needed. It seems that we could have hundreds of functional tests for similar situations. Ideally, a fuzz target that catches this (and many similar situations at the same time) would be my preference.

marcofleon commented at 4:55 PM on May 11, 2026:

I mainly included it as a way to test the bug, and I'm fine with dropping it if others are too, or renaming it if it's kept. I agree the bug is quite specific and may not be worth its own functional test. I'm not entirely sure how to make it more general or where a test like this could fit nicely.

As for a fuzz test, I think Antithesis-style deterministic testing is the most effective way we have (for now) to surface situations like this one. Eventually, I can see fuzzamoto finding these types of bugs the more it's improved. I haven't given it very deep thought, but maybe extending block_index_tree could find this bug and others like it? Or a more general harness for pruning + restart behavior. Either way, feels like a larger follow-up to this fix.

marcofleon commented at 12:03 PM on May 12, 2026:

Renamed and cleaned up the functional test, in case we decide to keep it.
mzumsande commented at 4:18 PM on April 29, 2026: contributor

ACK for the fix - less sure about the test.

test: Add coverage for m_blocks_unlinked invariant in LoadBlockIndex

A pruned stale-fork block whose parent doesn't have any transactions
shouldn't be added to m_blocks_unlinked when starting up a node.

3f44f9aef7

marcofleon force-pushed on May 12, 2026

Contributors

Labels

Linked (view graph)

#35050 validation: `Assertion failed: (!foundInUnlinked)`

validation: Don't add pruned blocks to `m_blocks_unlinked` on startup #35168

Code Coverage & Benchmarks

Reviews