Extend functional test for setBlockIndexCandidates UB
What the developer wrote
Authored by marcofleon
Extend functional test for setBlockIndexCandidates UB
Fix the from-disk subtest to use a separate node so it builds on a
clean genesis block, rather than the leftover chain from the
in-memory subtest.
Change from a two-way to a three-way block race. The UB in the old
LoadChainTip (mutating nSequenceId, a sort key, while the block is
in setBlockIndexCandidates) corrupts the internal tree structure,
resulting in a failed erase that leaves stale blocks in the set
alongside the tip. With only two competing blocks, this is caught
by libstdc++ but not by libc++. A three-way split triggers the bug
on both implementations.
To trigger CheckBlockIndex (where the crashing assertion is), replace
the restart loop with sending a new block after a single restart.
What changed, and why it matters
This commit only changes a test file. It improves an existing functional test that exercises a known undefined-behavior bug in Bitcoin Core's block index handling. The test now uses a separate node, creates a three-way block race instead of a two-way race, and triggers the bug more reliably across different C++ standard library implementations. It does not change production code, so it does not fix or introduce a vulnerability by itself.
No immediate action required for this commit. Treat it as a regression-test improvement. Ensure the underlying UB bug in setBlockIndexCandidates handling is tracked and already fixed or scheduled for fixing in production code; if not, prioritize a code fix because the test confirms it is reproducible.
Security signals we found
Test-only change extending coverage for known undefined-behavior bug
References stale blocks remaining in setBlockIndexCandidates after failed erase
References crashing assertion in CheckBlockIndex
No production code modification
Evidence from the diff
The patch modifies test/functional/feature_chain_tiebreaks.py to extend coverage for a previously identified undefined-behavior (UB) issue: mutating nSequenceId (a sort key) while a block is inside setBlockIndexCandidates, which can corrupt the std::set internal tree and leave stale blocks. The test changes are: (1) use nodes[1] with a clean genesis for the from-disk subtest, (2) switch from a two-way to a three-way equal-work split so libc++ also triggers the bug, and (3) replace the restart loop with a single restart followed by a new block to exercise CheckBlockIndex. No consensus or validation code is changed.
Changed components
test/functional/feature_chain_tiebreaks.pyInspect captured patch +31 / −18
diff --git a/test/functional/feature_chain_tiebreaks.py b/test/functional/feature_chain_tiebreaks.py
index 707c9947..1b4037e1 100755
--- a/test/functional/feature_chain_tiebreaks.py
+++ b/test/functional/feature_chain_tiebreaks.py
@@ -15,6 +15,9 @@ class ChainTiebreaksTest(BitcoinTestFramework):
self.num_nodes = 2
self.setup_clean_chain = True
+ def setup_network(self):
+ self.setup_nodes()
+
@staticmethod
def send_headers(node, blocks):
"""Submit headers for blocks to node."""
@@ -103,27 +106,29 @@ class ChainTiebreaksTest(BitcoinTestFramework):
node.invalidateblock(blocks[0].hash_hex)
def test_chain_split_from_disk(self):
- node = self.nodes[0]
+ node = self.nodes[1]
peer = node.add_p2p_connection(P2PDataStore())
+ self.generate(node, 1, sync_fun=self.no_op)
+
self.log.info('Precomputing blocks')
#
- # A1
- # /
- # G
- # \
- # A2
+ # /- A1
+ # /
+ # G -- B1 --- A2
+ # \
+ # \- A3
#
blocks = []
- # Construct two blocks building from genesis.
+ # Construct three equal-work blocks building from the tip.
start_height = node.getblockcount()
- genesis_block = node.getblock(node.getblockhash(start_height))
- prev_time = genesis_block["time"]
+ tip_block = node.getblock(node.getbestblockhash())
+ prev_time = tip_block["time"]
- for i in range(0, 2):
+ for i in range(0, 3):
blocks.append(create_block(
- hashprev=int(genesis_block["hash"], 16),
+ hashprev=int(tip_block["hash"], 16),
tmpl={"height": start_height + 1,
# Make sure each block has a different hash.
"curtime": prev_time + i + 1,
@@ -131,16 +136,24 @@ class ChainTiebreaksTest(BitcoinTestFramework):
))
blocks[-1].solve()
- # Send blocks and test the last one is not connected
- self.log.info('Send A1 and A2. Make sure that only the former connects')
+ # Send blocks and test that only the first one connects
+ self.log.info('Send A1, A2, and A3. Make sure that only the former connects')
peer.send_blocks_and_test([blocks[0]], node, success=True)
peer.send_blocks_and_test([blocks[1]], node, success=False)
+ peer.send_blocks_and_test([blocks[2]], node, success=False)
- self.log.info('Restart the node and check that the best tip before restarting matched the ones afterwards')
- # Restart and check enough times for this to eventually fail if the logic is broken
- for _ in range(10):
- self.restart_node(0)
- assert_equal(blocks[0].hash_hex, node.getbestblockhash())
+ # Restart and send a new block
+ self.restart_node(1)
+ assert_equal(blocks[0].hash_hex, node.getbestblockhash())
+ peer = node.add_p2p_connection(P2PDataStore())
+ next_block = create_block(
+ hashprev=blocks[0].hash_int,
+ tmpl={"height": start_height + 2,
+ "curtime": prev_time + 10,
+ }
+ )
+ next_block.solve()
+ peer.send_blocks_and_test([next_block], node, success=True)
def run_test(self):
self.test_chain_split_in_memory()
Why this scored 33/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.