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); 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()