Fixes #35050
Probably a rare occurrence, but it's possible to have pruned blocks without BLOCK_HAVE_DATA be inserted into m_blocks_unlinked when starting up a node. In CheckBlockIndex(), there's an assertion that blocks without data on disk shouldn't be in m_blocks_unlinked.
Fix by gating on BLOCK_HAVE_DATA in BlockManager::LoadBlockIndex() before adding to m_blocks_unlinked.
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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ViniciusCestarii, sedited |
| Concept ACK | mzumsande |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
A pruned stale-fork block whose parent is header-only shouldn't be
re-added to m_blocks_unlinked when starting up a node.
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)
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.
Thanks, fixed.
tACK 0cc9be25f65eb82009f6f0e4b242faca2a7728cf ran the new functional test, also verified the assert fires without the fix
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()
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)
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)
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)
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.
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):
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.
ACK for the fix - less sure about the test.