Merge bitcoin/bitcoin#33296: net: check for empty header before calling FillBlock

achow101 · Tom Trevethan · commit 53a5bd0790d6 · 2025-11-25T12:58:39.000Z
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 bitcoin/bitcoin@8b62647 fjahr: tACK 8b6264768030db1840041abeeaeefd6c227a2644 achow101: ACK 8b6264768030db1840041abeeaeefd6c227a2644 mzumsande: Code Review ACK 8b6264768030db1840041abeeaeefd6c227a2644 Tree-SHA512: d43a6f652161d4f7e6137f207a3e95259fc51509279d20347b1698c91179c39c8fcb75d2668b13a6b220f478a03578573208a415804be1d8843acb057fa1a73a
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -3810,13 +3810,27 @@ 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
                 Misbehaving(pfrom.GetId(), 100, "invalid compact block/non-matching block transactions");
                 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<CInv> 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
@@ -285,7 +285,7 @@ def test_invalid_cmpctblock_message(self):
         # 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 @@ def test_incorrect_blocktxn_response(self, test_node):
             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 @@ def run_test(self):
         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()
