From d6c37b28a7820fd48ec61e959becd9269d144628 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 28 Jul 2025 17:16:20 -0400 Subject: [PATCH 1/4] qa: remove unnecessary tx removal from compact block The error being checked here is BLOCK_MUTATED, as returned by IsBlockMutated() in FillBlock(). Dropping the fourth transaction from the block is unnecessary and would make testing of other block validation failures in following commits more verbose. --- test/functional/p2p_compactblocks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index 72890233037..7671a6fdd84 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -709,7 +709,6 @@ class CompactBlocksTest(BitcoinTestFramework): utxo = self.utxos[0] block = self.build_block_with_transactions(node, utxo, 5) - del block.vtx[3] block.hashMerkleRoot = block.calc_merkle_root() # Drop the coinbase witness but include the witness commitment. add_witness_commitment(block) From f12d8b104e0e6b7f3a01036c8abee8444485e013 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 28 Jul 2025 17:28:02 -0400 Subject: [PATCH 2/4] qa: test a compact block with an invalid transaction The current test to exercise a block with an invalid transaction actually creates a block with an invalid coinbase witness, which is checked early and for which MaybePunishNodeForBlock() is not called. Add a test case with an invalid regular transaction, which will lead CheckInputScripts to return a CONSENSUS error and MaybePunishNodeForBlock() to be called, appropriately not disconnecting upon an invalid compact block. This was until now untested as can be checked with the following diff: ```diff diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0c4a89c44cb..d243fb88d4b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1805,10 +1805,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati // The node is providing invalid data: case BlockValidationResult::BLOCK_CONSENSUS: case BlockValidationResult::BLOCK_MUTATED: - if (!via_compact_block) { + //if (!via_compact_block) { if (peer) Misbehaving(*peer, message); return; - } + //} break; case BlockValidationResult::BLOCK_CACHED_INVALID: { ``` Finally, note this failure is cached (unlike the malleated witness failure), which will be used in the following commits. --- test/functional/p2p_compactblocks.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index 7671a6fdd84..0d44943d6f3 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -54,6 +54,7 @@ from test_framework.script import ( CScript, OP_DROP, OP_TRUE, + OP_RETURN, ) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( @@ -726,6 +727,23 @@ class CompactBlocksTest(BitcoinTestFramework): assert_not_equal(node.getbestblockhash(), block.hash_hex) test_node.sync_with_ping() + # Re-establish a proper witness commitment with the coinbase witness, but + # invalidate the last tx in the block. + block.vtx[4].vin[0].scriptSig = CScript([OP_RETURN]) + block.hashMerkleRoot = block.calc_merkle_root() + add_witness_commitment(block) + block.solve() + + # This will lead to a consensus failure for which we also won't be disconnected but which + # will be cached. + comp_block.initialize_from_block(block, prefill_list=list(range(len(block.vtx))), use_witness=True) + msg = msg_cmpctblock(comp_block.to_p2p()) + test_node.send_and_ping(msg) + + # The tip still didn't advance. + assert_not_equal(node.getbestblockhash(), block.hash_hex) + test_node.sync_with_ping() + # Helper for enabling cb announcements # Send the sendcmpct request and sync headers def request_cb_announcements(self, peer): From fb2dcbb160bd7d61d53d8150df7f2aaabc32f78c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 28 Jul 2025 17:40:19 -0400 Subject: [PATCH 3/4] qa: test cached failure for compact block Submit the block with an invalid transaction Script again, leading to CACHED_INVALID being returned by AcceptBlockHeader(). Ensure that also this code path does not lead to a disconnection. This was previously untested, as can be checked with the following diff: ```diff diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0c4a89c44cb..e8e0c805367 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1814,10 +1814,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati { // Discourage outbound (but not inbound) peers if on an invalid chain. // Exempt HB compact block peers. Manual connections are always protected from discouragement. - if (peer && !via_compact_block && !peer->m_is_inbound) { + //if (peer && !via_compact_block && !peer->m_is_inbound) { if (peer) Misbehaving(*peer, message); return; - } + //} break; } case BlockValidationResult::BLOCK_INVALID_HEADER: ``` --- test/functional/p2p_compactblocks.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index 0d44943d6f3..724a5874373 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -744,6 +744,13 @@ class CompactBlocksTest(BitcoinTestFramework): assert_not_equal(node.getbestblockhash(), block.hash_hex) test_node.sync_with_ping() + # The failure above was cached. Submitting the compact block again will returned a cached + # consensus error (the code path is different) and still not get us disconnected (nor + # advance the tip). + test_node.send_and_ping(msg) + assert_not_equal(node.getbestblockhash(), block.hash_hex) + test_node.sync_with_ping() + # Helper for enabling cb announcements # Send the sendcmpct request and sync headers def request_cb_announcements(self, peer): From c1574381168573c561ebddf1945d2debefb340f7 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 29 Jul 2025 09:52:32 -0400 Subject: [PATCH 4/4] qa: test that we do disconnect upon a second invalid compact block being announced This was in fact untested until now. This can be checked with the following diff. ```diff diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0c4a89c44cb..f8b9adf910a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1822,7 +1822,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati } case BlockValidationResult::BLOCK_INVALID_HEADER: case BlockValidationResult::BLOCK_INVALID_PREV: - if (peer) Misbehaving(*peer, message); + if (!via_compact_block && peer) Misbehaving(*peer, message); return; // Conflicting (but not necessarily invalid) data or different policy: case BlockValidationResult::BLOCK_MISSING_PREV: ``` --- test/functional/p2p_compactblocks.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index 724a5874373..80041e2b59a 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -751,6 +751,13 @@ class CompactBlocksTest(BitcoinTestFramework): assert_not_equal(node.getbestblockhash(), block.hash_hex) test_node.sync_with_ping() + # Now, announcing a second block building on top of the invalid one will get us disconnected. + block.hashPrevBlock = block.hash_int + block.solve() + comp_block.initialize_from_block(block, prefill_list=list(range(len(block.vtx))), use_witness=True) + msg = msg_cmpctblock(comp_block.to_p2p()) + test_node.send_await_disconnect(msg) + # Helper for enabling cb announcements # Send the sendcmpct request and sync headers def request_cb_announcements(self, peer): @@ -967,6 +974,9 @@ class CompactBlocksTest(BitcoinTestFramework): self.log.info("Testing handling of invalid compact blocks...") self.test_invalid_tx_in_compactblock(self.segwit_node) + # The previous test will lead to a disconnection. Reconnect before continuing. + self.segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn()) + self.log.info("Testing invalid index in cmpctblock message...") self.test_invalid_cmpctblock_message()