From 5e585a0fc4fd68dd7b4982054b34deae2e7aeb89 Mon Sep 17 00:00:00 2001 From: Eugene Siegel Date: Wed, 3 Sep 2025 12:44:23 -0400 Subject: [PATCH 1/2] net: check for empty header before calling FillBlock Previously in debug builds, this would cause an Assume crash if FillBlock had been called previously. This could happen when multiple blocktxn messages were received. Co-Authored-By: Greg Sanders --- src/net_processing.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e1674b6d514..336669a8545 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3329,6 +3329,16 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl PartiallyDownloadedBlock& partialBlock = *range_flight.first->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(block_transactions.blockhash, pfrom.GetId()); + Misbehaving(peer, "previous compact block reconstruction attempt failed"); + LogDebug(BCLog::NET, "Peer %d sent compact block transactions multiple times", pfrom.GetId()); + return; + } + // We should not have gotten this far in compact block processing unless it's attached to a known header const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(partialBlock.header.hashPrevBlock))}; ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn, @@ -3340,6 +3350,9 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl } else if (status == READ_STATUS_FAILED) { if (first_in_flight) { // 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.emplace_back(MSG_BLOCK | GetFetchFlags(peer), block_transactions.blockhash); MakeAndPushMessage(pfrom, NetMsgType::GETDATA, invs); From 8b6264768030db1840041abeeaeefd6c227a2644 Mon Sep 17 00:00:00 2001 From: Eugene Siegel Date: Wed, 3 Sep 2025 12:44:52 -0400 Subject: [PATCH 2/2] test: send duplicate blocktxn message in p2p_compactblocks.py Add test_multiple_blocktxn_response that checks that the peer is disconnected. --- test/functional/p2p_compactblocks.py | 42 ++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index 56186bc1eb4..a2a3ffe2d82 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -558,6 +558,42 @@ class CompactBlocksTest(BitcoinTestFramework): test_node.send_and_ping(msg_block(block)) assert_equal(node.getbestblockhash(), block.hash_hex) + # 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.hash_int, [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.hash_int], 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_without_ping(msg) + test_node.wait_for_disconnect() + def test_getblocktxn_handler(self, test_node): node = self.nodes[0] # bitcoind will not send blocktxn responses for blocks whose height is @@ -977,6 +1013,12 @@ class CompactBlocksTest(BitcoinTestFramework): # 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 handling of multiple blocktxn responses...") + self.test_multiple_blocktxn_response(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()