From 25457a3272ff3b4ea6d30503f725675a8d16b987 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Tue, 26 May 2026 23:29:22 +0000 Subject: [PATCH] test: Tighten getblocktxn checks in parallel cb reconstruction test. Clear the getblocktxn message so we're not checking existing messages, and check that the hash in the getblocktxn match the cmpctblock being announced. Without this tightening of checks, a later commit that ignores CMPCTBLOCK messages that are unsolicited will succeed these tests while silently failing in reality. Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com> --- test/functional/p2p_compactblocks.py | 68 ++++++++++++++-------------- 1 file changed, 33 insertions(+), 35 deletions(-) diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index f83aa1bf25b..bf305cbd825 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -57,6 +57,7 @@ from test_framework.script import ( OP_RETURN, ) from test_framework.test_framework import BitcoinTestFramework +from test_framework.test_node import TestNode from test_framework.util import ( assert_not_equal, assert_equal, @@ -150,6 +151,17 @@ class CompactBlocksTest(BitcoinTestFramework): ]] self.utxos = [] + def getblocktxn_expected(self, peer, blockhash, indices=None): + with p2p_lock: + assert "getblocktxn" in peer.last_message + gbt = peer.last_message["getblocktxn"].block_txn_request + + assert_equal(gbt.blockhash, blockhash) + if indices is not None: + assert_equal(gbt.to_absolute(), indices) + if isinstance(peer, TestNode): + assert_not_equal(peer.getbestblockhash(), blockhash) + def build_block_on_tip(self, node): block = create_block(tmpl=node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)) block.solve() @@ -184,9 +196,10 @@ class CompactBlocksTest(BitcoinTestFramework): cmpct_block = HeaderAndShortIDs() cmpct_block.initialize_from_block(block) msg = msg_cmpctblock(cmpct_block.to_p2p()) + + peer.clear_getblocktxn() peer.send_and_ping(msg) - with p2p_lock: - assert "getblocktxn" in peer.last_message + self.getblocktxn_expected(peer, block.hash_int) return block, cmpct_block # Test "sendcmpct" (between peers preferring the same version): @@ -405,13 +418,11 @@ class CompactBlocksTest(BitcoinTestFramework): [k0, k1] = comp_block.get_siphash_keys() coinbase_hash = block.vtx[0].wtxid_int comp_block.shortids = [calculate_shortid(k0, k1, coinbase_hash)] + test_node.clear_getblocktxn() test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p())) assert_equal(int(node.getbestblockhash(), 16), block.hashPrevBlock) - # Expect a getblocktxn message. - with p2p_lock: - assert "getblocktxn" in test_node.last_message - absolute_indexes = test_node.last_message["getblocktxn"].block_txn_request.to_absolute() - assert_equal(absolute_indexes, [0]) # should be a coinbase request + # Expect a getblocktxn message that requests the coinbase. + self.getblocktxn_expected(test_node, block.hash_int, indices=[0]) # Send the coinbase, and verify that the tip advances. msg = msg_blocktxn() @@ -443,11 +454,9 @@ class CompactBlocksTest(BitcoinTestFramework): def test_getblocktxn_response(compact_block, peer, expected_result): msg = msg_cmpctblock(compact_block.to_p2p()) + peer.clear_getblocktxn() peer.send_and_ping(msg) - with p2p_lock: - assert "getblocktxn" in peer.last_message - absolute_indexes = peer.last_message["getblocktxn"].block_txn_request.to_absolute() - assert_equal(absolute_indexes, expected_result) + self.getblocktxn_expected(peer, compact_block.header.hash_int, expected_result) def test_tip_after_message(node, peer, msg, tip): peer.send_and_ping(msg) @@ -508,8 +517,7 @@ class CompactBlocksTest(BitcoinTestFramework): assert tx.txid_hex in mempool # Clear out last request. - with p2p_lock: - test_node.last_message.pop("getblocktxn", None) + test_node.clear_getblocktxn() # Send compact block comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True) @@ -538,12 +546,10 @@ class CompactBlocksTest(BitcoinTestFramework): # Send compact block comp_block = HeaderAndShortIDs() comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True) + test_node.clear_getblocktxn() test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p())) - absolute_indexes = [] - with p2p_lock: - assert "getblocktxn" in test_node.last_message - absolute_indexes = test_node.last_message["getblocktxn"].block_txn_request.to_absolute() - assert_equal(absolute_indexes, [6, 7, 8, 9, 10]) + expected_indices = [6, 7, 8, 9, 10] + self.getblocktxn_expected(test_node, block.hash_int, expected_indices) # Now give an incorrect response. # Note that it's possible for bitcoind to be smart enough to know we're @@ -578,12 +584,9 @@ class CompactBlocksTest(BitcoinTestFramework): # Send compact block comp_block = HeaderAndShortIDs() comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True) + test_node.clear_getblocktxn() test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p())) - absolute_indexes = [] - with p2p_lock: - assert "getblocktxn" in test_node.last_message - absolute_indexes = test_node.last_message["getblocktxn"].block_txn_request.to_absolute() - assert_equal(absolute_indexes, [1, 2]) + self.getblocktxn_expected(test_node, block.hash_int, indices=[1,2]) # Send a blocktxn that does not succeed in reconstruction, triggering # getdata fallback. @@ -896,6 +899,9 @@ class CompactBlocksTest(BitcoinTestFramework): # Test the simple parallel download case... for num_missing in [1, 5, 20]: + delivery_peer.clear_getblocktxn() + inbound_peer.clear_getblocktxn() + outbound_peer.clear_getblocktxn() # Remaining low-bandwidth peer is stalling_peer, who announces first assert_equal([peer['bip152_hb_to'] for peer in node.getpeerinfo()], [False, True, True, True]) @@ -903,10 +909,8 @@ class CompactBlocksTest(BitcoinTestFramework): block, cmpct_block = self.announce_cmpct_block(node, stalling_peer, num_missing) delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p())) - with p2p_lock: - # The second peer to announce should still get a getblocktxn - assert "getblocktxn" in delivery_peer.last_message - assert_not_equal(node.getbestblockhash(), block.hash_hex) + # The second peer to announce should still get a getblocktxn + self.getblocktxn_expected(delivery_peer, block.hash_int) inbound_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p())) with p2p_lock: @@ -915,10 +919,8 @@ class CompactBlocksTest(BitcoinTestFramework): assert_not_equal(node.getbestblockhash(), block.hash_hex) outbound_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p())) - with p2p_lock: - # The third peer to announce should get a getblocktxn if outbound - assert "getblocktxn" in outbound_peer.last_message - assert_not_equal(node.getbestblockhash(), block.hash_hex) + # The third peer to announce should get a getblocktxn if outbound + self.getblocktxn_expected(outbound_peer, block.hash_int) # Second peer completes the compact block first msg = msg_blocktxn() @@ -931,10 +933,6 @@ class CompactBlocksTest(BitcoinTestFramework): stalling_peer.send_and_ping(msg) self.utxos.append([block.vtx[-1].txid_int, 0, block.vtx[-1].vout[0].nValue]) - delivery_peer.clear_getblocktxn() - inbound_peer.clear_getblocktxn() - outbound_peer.clear_getblocktxn() - def run_test(self): self.wallet = MiniWallet(self.nodes[0])