From 53a5bd0790d6b32cb0c814c92a2ddf467719f33d Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 8 Sep 2025 17:16:28 -0700 Subject: [PATCH] Merge bitcoin/bitcoin#33296: net: check for empty header before calling FillBlock 8b6264768030db1840041abeeaeefd6c227a2644 test: send duplicate blocktxn message in p2p_compactblocks.py (Eugene Siegel) 5e585a0fc4fd68dd7b4982054b34deae2e7aeb89 net: check for empty header before calling FillBlock (Eugene Siegel) Pull request description: This avoids an Assume crash if multiple blocktxn messages are received. The first call to `FillBlock` would make the header empty via `SetNull` and the call right before the second `FillBlock` would crash [here](https://github.com/bitcoin/bitcoin/blob/689a32197638e92995dd8eb071425715f5fdc3a4/src/net_processing.cpp#L3333) since `LookupBlockIndex` won't find anything. Fix that by checking for an empty header before the Assume. ACKs for top commit: instagibbs: reACK https://github.com/bitcoin/bitcoin/pull/33296/commits/8b6264768030db1840041abeeaeefd6c227a2644 fjahr: tACK 8b6264768030db1840041abeeaeefd6c227a2644 achow101: ACK 8b6264768030db1840041abeeaeefd6c227a2644 mzumsande: Code Review ACK 8b6264768030db1840041abeeaeefd6c227a2644 Tree-SHA512: d43a6f652161d4f7e6137f207a3e95259fc51509279d20347b1698c91179c39c8fcb75d2668b13a6b220f478a03578573208a415804be1d8843acb057fa1a73a --- src/net_processing.cpp | 14 +++++++++ test/functional/p2p_compactblocks.py | 44 ++++++++++++++++++++++++++-- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b1e6e8bf85..dc869c9a87 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3810,6 +3810,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock; + + if (partialBlock.header.IsNull()) { + // It is possible for the header to be empty if a previous call to FillBlock wiped the header, but left + // the PartiallyDownloadedBlock pointer around (i.e. did not call RemoveBlockRequest). In this case, we + // should not call LookupBlockIndex below. + RemoveBlockRequest(resp.blockhash, pfrom.GetId()); + Misbehaving(pfrom.GetId(), 100, "previous compact block reconstruction attempt failed"); + LogPrint(BCLog::NET, "Peer %d sent compact block transactions multiple times", pfrom.GetId()); + return; + } + ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn); if (status == READ_STATUS_INVALID) { RemoveBlockRequest(resp.blockhash, pfrom.GetId()); // Reset in-flight state in case Misbehaving does not result in a disconnect @@ -3817,6 +3828,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; } else if (status == READ_STATUS_FAILED) { // Might have collided, fall back to getdata now :( + // We keep the failed partialBlock to disallow processing another compact block announcement from the same + // peer for the same block. We let the full block download below continue under the same m_downloading_since + // timer. std::vector invs; invs.push_back(CInv(MSG_BLOCK | GetFetchFlags(pfrom), resp.blockhash)); m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, invs)); diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index 76944ec046..221c09c177 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -285,7 +285,7 @@ class CompactBlocksTest(BitcoinTestFramework): # This index will be too high prefilled_txn = PrefilledTransaction(1, block.vtx[0]) cmpct_block.prefilled_txn = [prefilled_txn] - self.segwit_node.send_await_disconnect(msg_cmpctblock(cmpct_block)) + self.additional_segwit_node.send_await_disconnect(msg_cmpctblock(cmpct_block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock) # Compare the generated shortids to what we expect based on BIP 152, given @@ -603,6 +603,42 @@ class CompactBlocksTest(BitcoinTestFramework): test_node.send_and_ping(msg_no_witness_block(block)) assert_equal(int(node.getbestblockhash(), 16), block.sha256) + # Multiple blocktxn responses will cause a node to get disconnected. + def test_multiple_blocktxn_response(self, test_node): + node = self.nodes[0] + utxo = self.utxos[0] + + block = self.build_block_with_transactions(node, utxo, 2) + + # Send compact block + comp_block = HeaderAndShortIDs() + comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True) + 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]) + + # Send a blocktxn that does not succeed in reconstruction, triggering + # getdata fallback. + msg = msg_blocktxn() + msg.block_transactions = BlockTransactions(block.sha256, [block.vtx[2]] + [block.vtx[1]]) + test_node.send_and_ping(msg) + + # Tip should not have updated + assert_equal(int(node.getbestblockhash(), 16), block.hashPrevBlock) + + # We should receive a getdata request + test_node.wait_for_getdata([block.sha256], timeout=10) + assert test_node.last_message["getdata"].inv[0].type == MSG_BLOCK or \ + test_node.last_message["getdata"].inv[0].type == MSG_BLOCK | MSG_WITNESS_FLAG + + # Send the same blocktxn and assert the sender gets disconnected. + with node.assert_debug_log(['previous compact block reconstruction attempt failed']): + test_node.send_message(msg) + test_node.wait_for_disconnect() + def test_getblocktxn_handler(self, test_node): version = test_node.cmpct_version node = self.nodes[0] @@ -897,12 +933,16 @@ class CompactBlocksTest(BitcoinTestFramework): self.test_invalid_tx_in_compactblock(self.segwit_node) self.test_invalid_tx_in_compactblock(self.old_node) + self.log.info("Testing handling of multiple blocktxn responses...") + self.test_multiple_blocktxn_response(self.segwit_node) + + self.segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(cmpct_version=2)) + self.log.info("Testing invalid index in cmpctblock message...") self.test_invalid_cmpctblock_message() self.log.info("Testing high-bandwidth mode states via getpeerinfo...") self.test_highbandwidth_mode_states_via_getpeerinfo() - if __name__ == '__main__': CompactBlocksTest().main()