From 622299e6f2708881fe2b7025320d297cdfa6bc99 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 24 Jan 2023 12:35:16 +0100 Subject: [PATCH 1/7] Merge bitcoin/bitcoin#26898: fuzz: Add PartiallyDownloadedBlock target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backport of bitcoin/bitcoin#26898 (fuzz harness + assert->READ_STATUS_INVALID hardening in PartiallyDownloadedBlock). Dash adaptations to the fuzz harness: use tx->GetHash() (no segwit/GetWitnessHash), FUZZ_TARGET(..., .init=) instead of FUZZ_TARGET_INIT, drop nonexistent test/fuzz/util/mempool.h include (symbols come from test/util/txmempool.h + test/fuzz/util.h), and swap the absent BLOCK_HEADER_LOW_WORK for Dash's BLOCK_CHAINLOCK. (cherry picked from knst commit 2ea984f473d — bitcoin/bitcoin#26898) --- src/Makefile.test.include | 1 + src/blockencodings.cpp | 18 ++- src/blockencodings.h | 10 ++ src/test/fuzz/partially_downloaded_block.cpp | 141 +++++++++++++++++++ 4 files changed, 164 insertions(+), 6 deletions(-) create mode 100644 src/test/fuzz/partially_downloaded_block.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 2885c5fccb25..cc7ca8088ccc 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -337,6 +337,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/parse_numbers.cpp \ test/fuzz/parse_script.cpp \ test/fuzz/parse_univalue.cpp \ + test/fuzz/partially_downloaded_block.cpp \ test/fuzz/policy_estimator.cpp \ test/fuzz/policy_estimator_io.cpp \ test/fuzz/poolresource.cpp \ diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index b66c98e8e80c..f1f54d61ab21 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -54,7 +54,8 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c if (cmpctblock.shorttxids.size() + cmpctblock.prefilledtxn.size() > MaxBlockSize() / MIN_TRANSACTION_SIZE) return READ_STATUS_INVALID; - assert(header.IsNull() && txn_available.empty()); + if (!header.IsNull() || !txn_available.empty()) return READ_STATUS_INVALID; + header = cmpctblock.header; txn_available.resize(cmpctblock.BlockTxCount()); @@ -169,14 +170,18 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c return READ_STATUS_OK; } -bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const { - assert(!header.IsNull()); +bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const +{ + if (header.IsNull()) return false; + assert(index < txn_available.size()); return txn_available[index] != nullptr; } -ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector& vtx_missing) { - assert(!header.IsNull()); +ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector& vtx_missing) +{ + if (header.IsNull()) return READ_STATUS_INVALID; + uint256 hash = header.GetHash(); block = header; block.vtx.resize(txn_available.size()); @@ -199,7 +204,8 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< return READ_STATUS_INVALID; BlockValidationState state; - if (!CheckBlock(block, state, Params().GetConsensus())) { + CheckBlockFn check_block = m_check_block_mock ? m_check_block_mock : CheckBlock; + if (!check_block(block, state, Params().GetConsensus(), /*fCheckPoW=*/true, /*fCheckMerkleRoot=*/true)) { // TODO: We really want to just check merkle tree manually here, // but that is expensive, and CheckBlock caches a block's // "checked-status" (in the CBlock?). CBlock should be able to diff --git a/src/blockencodings.h b/src/blockencodings.h index a19db7db2dd3..29a3d3025b39 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -7,8 +7,13 @@ #include +#include class CTxMemPool; +class BlockValidationState; +namespace Consensus { +struct Params; +}; // Transaction compression schemes for compact block relay can be introduced by writing // an actual formatter here. @@ -129,6 +134,11 @@ class PartiallyDownloadedBlock { const CTxMemPool* pool; public: CBlockHeader header; + + // Can be overriden for testing + using CheckBlockFn = std::function; + CheckBlockFn m_check_block_mock{nullptr}; + explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {} // extra_txn is a list of extra transactions to look at, in form diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp new file mode 100644 index 000000000000..be3d9adbdf11 --- /dev/null +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -0,0 +1,141 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +namespace { +const TestingSetup* g_setup; +} // namespace + +void initialize_pdb() +{ + static const auto testing_setup = MakeNoLogFileContext(); + g_setup = testing_setup.get(); +} + +PartiallyDownloadedBlock::CheckBlockFn FuzzedCheckBlock(std::optional result) +{ + return [result](const CBlock&, BlockValidationState& state, const Consensus::Params&, bool, bool) { + if (result) { + return state.Invalid(*result); + } + + return true; + }; +} + +FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) +{ + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + + auto block{ConsumeDeserializable(fuzzed_data_provider)}; + if (!block || block->vtx.size() == 0 || + block->vtx.size() >= std::numeric_limits::max()) { + return; + } + + CBlockHeaderAndShortTxIDs cmpctblock{*block}; + + CTxMemPool pool{}; + PartiallyDownloadedBlock pdb{&pool}; + + // Set of available transactions (mempool or extra_txn) + std::set available; + // The coinbase is always available + available.insert(0); + + std::vector> extra_txn; + for (size_t i = 1; i < block->vtx.size(); ++i) { + auto tx{block->vtx[i]}; + + bool add_to_extra_txn{fuzzed_data_provider.ConsumeBool()}; + bool add_to_mempool{fuzzed_data_provider.ConsumeBool()}; + + if (add_to_extra_txn) { + extra_txn.emplace_back(tx->GetHash(), tx); + available.insert(i); + } + + if (add_to_mempool) { + LOCK2(cs_main, pool.cs); + pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, *tx)); + available.insert(i); + } + } + + auto init_status{pdb.InitData(cmpctblock, extra_txn)}; + + std::vector missing; + // Whether we skipped a transaction that should be included in `missing`. + // FillBlock should never return READ_STATUS_OK if that is the case. + bool skipped_missing{false}; + for (size_t i = 0; i < cmpctblock.BlockTxCount(); i++) { + // If init_status == READ_STATUS_OK then a available transaction in the + // compact block (i.e. IsTxAvailable(i) == true) implies that we marked + // that transaction as available above (i.e. available.count(i) > 0). + // The reverse is not true, due to possible compact block short id + // collisions (i.e. available.count(i) > 0 does not imply + // IsTxAvailable(i) == true). + if (init_status == READ_STATUS_OK) { + assert(!pdb.IsTxAvailable(i) || available.count(i) > 0); + } + + bool skip{fuzzed_data_provider.ConsumeBool()}; + if (!pdb.IsTxAvailable(i) && !skip) { + missing.push_back(block->vtx[i]); + } + + skipped_missing |= (!pdb.IsTxAvailable(i) && skip); + } + + // Mock CheckBlock + bool fail_check_block{fuzzed_data_provider.ConsumeBool()}; + auto validation_result = + fuzzed_data_provider.PickValueInArray( + {BlockValidationResult::BLOCK_RESULT_UNSET, + BlockValidationResult::BLOCK_CONSENSUS, + BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE, + BlockValidationResult::BLOCK_CACHED_INVALID, + BlockValidationResult::BLOCK_INVALID_HEADER, + BlockValidationResult::BLOCK_MUTATED, + BlockValidationResult::BLOCK_MISSING_PREV, + BlockValidationResult::BLOCK_INVALID_PREV, + BlockValidationResult::BLOCK_TIME_FUTURE, + BlockValidationResult::BLOCK_CHECKPOINT, + BlockValidationResult::BLOCK_CHAINLOCK}); + pdb.m_check_block_mock = FuzzedCheckBlock( + fail_check_block ? + std::optional{validation_result} : + std::nullopt); + + CBlock reconstructed_block; + auto fill_status{pdb.FillBlock(reconstructed_block, missing)}; + switch (fill_status) { + case READ_STATUS_OK: + assert(!skipped_missing); + assert(!fail_check_block); + assert(block->GetHash() == reconstructed_block.GetHash()); + break; + case READ_STATUS_CHECKBLOCK_FAILED: [[fallthrough]]; + case READ_STATUS_FAILED: + assert(fail_check_block); + break; + case READ_STATUS_INVALID: + break; + } +} From cc87ed2d1edad61fd30001df167d08dfa761cdd5 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Wed, 3 Dec 2025 11:05:25 -0600 Subject: [PATCH 2/7] Merge bitcoin/bitcoin#27626: Parallel compact block downloads, take 3 This is a backport of bitcoin/bitcoin#27626 This PR attempts to mitigate a single case, where high bandwidth peers can bail us out of a flakey peer not completing blocks for us. We allow up to 2 additional getblocktxns requests per unique block. This would hopefully allow the chance for an honest high bandwidth peer to hand us the transactions even if the first in flight peer stalls out. Changes: - Convert mapBlocksInFlight from std::map to std::multimap to allow multiple peers to be downloading the same block - Add IsBlockRequestedFromOutbound() to check if block is being fetched from an outbound peer - Update RemoveBlockRequest() to take optional from_peer parameter to selectively remove only that peers request - Add MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3 constant - Remove nBlocksInFlight counter, use vBlocksInFlight.size() instead - Add parallel download test in p2p_compactblocks.py Generated with Claude Code Co-Authored-By: Claude (cherry picked from commit 7b8553f630a3254a34f09c15420ba831dfcfbbb0) --- src/net.h | 2 + src/net_processing.cpp | 230 ++++++++++++++++++--------- src/net_processing.h | 2 + src/rpc/blockchain.cpp | 2 +- test/functional/p2p_compactblocks.py | 98 +++++++++++- 5 files changed, 255 insertions(+), 79 deletions(-) diff --git a/src/net.h b/src/net.h index 0cf55621919c..6b41bd15c454 100644 --- a/src/net.h +++ b/src/net.h @@ -224,7 +224,9 @@ class CNodeStats int nVersion; std::string cleanSubVer; bool fInbound; + // We requested high bandwidth connection to peer bool m_bip152_highbandwidth_to; + // Peer requested high bandwidth connection bool m_bip152_highbandwidth_from; int m_starting_height; uint64_t nSendBytes; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d99bcc71a399..e7a73823cd15 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -457,7 +457,6 @@ struct CNodeState { std::list vBlocksInFlight; //! When the first entry in vBlocksInFlight started downloading. Don't care when vBlocksInFlight is empty. std::chrono::microseconds m_downloading_since{0us}; - int nBlocksInFlight{0}; //! Whether we consider this a preferred download peer. bool fPreferredDownload{false}; //! Whether this peer wants invs or headers (when possible) for block announcements. @@ -1035,8 +1034,11 @@ class PeerManagerImpl final : public PeerManager /** Have we requested this block from a peer */ bool IsBlockRequested(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + /** Have we requested this block from an outbound peer */ + bool IsBlockRequestedFromOutbound(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + /** Remove this block from our tracked requested blocks. Called if: - * - the block has been recieved from a peer + * - the block has been received from a peer * - the request for the block has timed out * If "from_peer" is specified, then only remove the block if it is in * flight from that peer (to avoid one peer's network traffic from @@ -1057,7 +1059,9 @@ class PeerManagerImpl final : public PeerManager */ void FindNextBlocksToDownload(const Peer& peer, unsigned int count, std::vector& vBlocks, NodeId& nodeStaller) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - std::map::iterator> > mapBlocksInFlight GUARDED_BY(cs_main); + /* Multimap used to preserve insertion order */ + typedef std::multimap::iterator>> BlockDownloadMap; + BlockDownloadMap mapBlocksInFlight GUARDED_BY(cs_main); /** When our tip was last updated. */ std::atomic m_last_tip_update{0s}; @@ -1240,40 +1244,55 @@ std::chrono::microseconds PeerManagerImpl::NextInvToInbounds(std::chrono::micros bool PeerManagerImpl::IsBlockRequested(const uint256& hash) { - return mapBlocksInFlight.find(hash) != mapBlocksInFlight.end(); + return mapBlocksInFlight.count(hash); } -void PeerManagerImpl::RemoveBlockRequest(const uint256& hash, std::optional from_peer) +bool PeerManagerImpl::IsBlockRequestedFromOutbound(const uint256& hash) { - auto it = mapBlocksInFlight.find(hash); - if (it == mapBlocksInFlight.end()) { - // Block was not requested - return; + for (auto range = mapBlocksInFlight.equal_range(hash); range.first != range.second; range.first++) { + auto [nodeid, block_it] = range.first->second; + CNodeState& nodestate = *Assert(State(nodeid)); + if (!nodestate.m_is_inbound) return true; } - auto [node_id, list_it] = it->second; + return false; +} - if (from_peer && node_id != *from_peer) { - // Block was requested by another peer +void PeerManagerImpl::RemoveBlockRequest(const uint256& hash, std::optional from_peer) +{ + auto range = mapBlocksInFlight.equal_range(hash); + if (range.first == range.second) { + // Block was not requested from any peer return; } - CNodeState *state = State(node_id); - assert(state != nullptr); + // We should not have requested too many of this block + Assume(mapBlocksInFlight.count(hash) <= MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK); - if (state->vBlocksInFlight.begin() == list_it) { - // First block on the queue was received, update the start download time for the next one - state->m_downloading_since = std::max(state->m_downloading_since, GetTime()); - } - state->vBlocksInFlight.erase(list_it); + while (range.first != range.second) { + auto [node_id, list_it] = range.first->second; + + if (from_peer && *from_peer != node_id) { + range.first++; + continue; + } + + CNodeState& state = *Assert(State(node_id)); - state->nBlocksInFlight--; - if (state->nBlocksInFlight == 0) { - // Last validated block on the queue was received. - m_peers_downloading_from--; + if (state.vBlocksInFlight.begin() == list_it) { + // First block on the queue was received, update the start download time for the next one + state.m_downloading_since = std::max(state.m_downloading_since, GetTime()); + } + state.vBlocksInFlight.erase(list_it); + + if (state.vBlocksInFlight.empty()) { + // Last validated block on the queue for this peer was received. + m_peers_downloading_from--; + } + state.m_stalling_since = 0us; + + range.first = mapBlocksInFlight.erase(range.first); } - state->m_stalling_since = 0us; - mapBlocksInFlight.erase(it); } bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex& block, std::list::iterator **pit) @@ -1283,27 +1302,29 @@ bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex& block, st CNodeState *state = State(nodeid); assert(state != nullptr); + Assume(mapBlocksInFlight.count(hash) <= MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK); + // Short-circuit most stuff in case it is from the same node - std::map::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash); - if (itInFlight != mapBlocksInFlight.end() && itInFlight->second.first == nodeid) { - if (pit) { - *pit = &itInFlight->second.second; + for (auto range = mapBlocksInFlight.equal_range(hash); range.first != range.second; range.first++) { + if (range.first->second.first == nodeid) { + if (pit) { + *pit = &range.first->second.second; + } + return false; } - return false; } - // Make sure it's not listed somewhere already. - RemoveBlockRequest(hash, std::nullopt); + // Make sure it's not being fetched already from same peer. + RemoveBlockRequest(hash, nodeid); std::list::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(), {&block, std::unique_ptr(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)}); - state->nBlocksInFlight++; - if (state->nBlocksInFlight == 1) { + if (state->vBlocksInFlight.size() == 1) { // We're starting a block download (batch) from this peer. state->m_downloading_since = GetTime(); m_peers_downloading_from++; } - itInFlight = mapBlocksInFlight.insert(std::make_pair(hash, std::make_pair(nodeid, it))).first; + auto itInFlight = mapBlocksInFlight.insert(std::make_pair(hash, std::make_pair(nodeid, it))); if (pit) { *pit = &itInFlight->second.second; } @@ -1504,7 +1525,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co } } else if (waitingfor == -1) { // This is the first already-in-flight block. - waitingfor = mapBlocksInFlight[pindex->GetBlockHash()].first; + waitingfor = mapBlocksInFlight.lower_bound(pindex->GetBlockHash())->second.first; } } } @@ -1791,12 +1812,20 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) { nSyncStarted--; for (const QueuedBlock& entry : state->vBlocksInFlight) { - mapBlocksInFlight.erase(entry.pindex->GetBlockHash()); + auto range = mapBlocksInFlight.equal_range(entry.pindex->GetBlockHash()); + while (range.first != range.second) { + auto [node_id, list_it] = range.first->second; + if (node_id != nodeid) { + range.first++; + } else { + range.first = mapBlocksInFlight.erase(range.first); + } + } } m_orphanage.EraseForPeer(nodeid); if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid); m_num_preferred_download_peers -= state->fPreferredDownload; - m_peers_downloading_from -= (state->nBlocksInFlight != 0); + m_peers_downloading_from -= (!state->vBlocksInFlight.empty()); assert(m_peers_downloading_from >= 0); m_outbound_peers_with_protect_from_disconnect -= state->m_chain_sync.m_protect; assert(m_outbound_peers_with_protect_from_disconnect >= 0); @@ -2032,11 +2061,11 @@ std::optional PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl if (peer == nullptr) return "Peer does not exist"; LOCK(cs_main); - // Mark block as in-flight unless it already is (for this peer). - // If the peer does not send us a block, vBlocksInFlight remains non-empty, - // causing us to timeout and disconnect. - // If a block was already in-flight for a different peer, its BLOCKTXN - // response will be dropped. + + // Forget about all prior requests + RemoveBlockRequest(block_index.GetBlockHash(), std::nullopt); + + // Mark block as in-flight if (!BlockRequested(peer_id, block_index)) return "Already requested from this peer"; // Construct message to request the block @@ -3130,7 +3159,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c std::vector vGetData; // Download as much as possible, from earliest to latest. for (const CBlockIndex *pindex : vToFetch | std::views::reverse) { - if (nodestate->nBlocksInFlight >= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { + if (nodestate->vBlocksInFlight.size() >= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { // Can't download any more from this peer break; } @@ -4846,15 +4875,27 @@ void PeerManagerImpl::ProcessMessage( nodestate->m_last_block_announcement = GetTime(); } - std::map::iterator> >::iterator blockInFlightIt = mapBlocksInFlight.find(pindex->GetBlockHash()); - bool fAlreadyInFlight = blockInFlightIt != mapBlocksInFlight.end(); - if (pindex->nStatus & BLOCK_HAVE_DATA) // Nothing to do here return; + auto range_flight = mapBlocksInFlight.equal_range(pindex->GetBlockHash()); + size_t already_in_flight = std::distance(range_flight.first, range_flight.second); + bool requested_block_from_this_peer{false}; + + // Multimap ensures ordering of outstanding requests. It's either empty or first in line. + bool first_in_flight = already_in_flight == 0 || (range_flight.first->second.first == pfrom.GetId()); + + while (range_flight.first != range_flight.second) { + if (range_flight.first->second.first == pfrom.GetId()) { + requested_block_from_this_peer = true; + break; + } + range_flight.first++; + } + if (pindex->nChainWork <= m_chainman.ActiveChain().Tip()->nChainWork || // We know something better pindex->nTx != 0) { // We had this block at some point, but pruned it - if (fAlreadyInFlight) { + if (requested_block_from_this_peer) { // We requested this block for some reason, but our mempool will probably be useless // so we just grab the block via normal getdata std::vector vInv(1); @@ -4865,14 +4906,15 @@ void PeerManagerImpl::ProcessMessage( } // If we're not close to tip yet, give up and let parallel block fetch work its magic - if (!fAlreadyInFlight && !CanDirectFetch()) + if (!already_in_flight && !CanDirectFetch()) { return; + } // We want to be a bit conservative just to be extra careful about DoS // possibilities in compact block processing... if (pindex->nHeight <= m_chainman.ActiveChain().Height() + 2) { - if ((!fAlreadyInFlight && nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) || - (fAlreadyInFlight && blockInFlightIt->second.first == pfrom.GetId())) { + if ((already_in_flight < MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK && nodestate->vBlocksInFlight.size() < MAX_BLOCKS_IN_TRANSIT_PER_PEER) || + requested_block_from_this_peer) { std::list::iterator *queuedBlockIt = nullptr; if (!BlockRequested(pfrom.GetId(), *pindex, &queuedBlockIt)) { if (!(*queuedBlockIt)->partialBlock) @@ -4891,11 +4933,16 @@ void PeerManagerImpl::ProcessMessage( Misbehaving(*peer, 100, "invalid compact block"); return; } else if (status == READ_STATUS_FAILED) { - // Duplicate txindexes, the block is now in-flight, so just request it - std::vector vInv(1); - vInv[0] = CInv(MSG_BLOCK, blockhash); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); - return; + if (first_in_flight) { + // Duplicate txindexes, the block is now in-flight, so just request it + std::vector vInv(1); + vInv[0] = CInv(MSG_BLOCK, blockhash); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); + return; + } else { + // Give up for this peer and wait for other peer(s) + RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); + } } BlockTransactionsRequest req; @@ -4909,9 +4956,24 @@ void PeerManagerImpl::ProcessMessage( txn.blockhash = blockhash; blockTxnMsg << txn; fProcessBLOCKTXN = true; - } else { + } else if (first_in_flight) { + // We will try to round-trip any compact blocks we get on failure, + // as long as it's first... req.blockhash = pindex->GetBlockHash(); m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETBLOCKTXN, req)); + } else if (pfrom.m_bip152_highbandwidth_to && + (!pfrom.IsInboundConn() || + IsBlockRequestedFromOutbound(blockhash) || + already_in_flight < MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK - 1)) { + // ... or it's a hb relay peer and: + // - peer is outbound, or + // - we already have an outbound attempt in flight(so we'll take what we can get), or + // - it's not the final parallel download slot (which we may reserve for first outbound) + req.blockhash = pindex->GetBlockHash(); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETBLOCKTXN, req)); + } else { + // Give up for this peer and wait for other peer(s) + RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); } } else { // This block is either already in flight from a different @@ -4932,7 +4994,7 @@ void PeerManagerImpl::ProcessMessage( } } } else { - if (fAlreadyInFlight) { + if (requested_block_from_this_peer) { // We requested this block, but its far into the future, so our // mempool will probably be useless - request the block normally std::vector vInv(1); @@ -5003,24 +5065,44 @@ void PeerManagerImpl::ProcessMessage( { LOCK(cs_main); - std::map::iterator> >::iterator it = mapBlocksInFlight.find(resp.blockhash); - if (it == mapBlocksInFlight.end() || !it->second.second->partialBlock || - it->second.first != pfrom.GetId()) { + auto range_flight = mapBlocksInFlight.equal_range(resp.blockhash); + size_t already_in_flight = std::distance(range_flight.first, range_flight.second); + bool requested_block_from_this_peer{false}; + + // Multimap ensures ordering of outstanding requests. It's either empty or first in line. + bool first_in_flight = already_in_flight == 0 || (range_flight.first->second.first == pfrom.GetId()); + + while (range_flight.first != range_flight.second) { + auto [node_id, block_it] = range_flight.first->second; + if (node_id == pfrom.GetId() && block_it->partialBlock) { + requested_block_from_this_peer = true; + break; + } + range_flight.first++; + } + + if (!requested_block_from_this_peer) { LogPrint(BCLog::NET, "Peer %d sent us block transactions for block we weren't expecting\n", pfrom.GetId()); return; } - PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock; + PartiallyDownloadedBlock& partialBlock = *range_flight.first->second.second->partialBlock; 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(*peer, 100, "invalid compact block/non-matching block transactions"); return; } else if (status == READ_STATUS_FAILED) { - // Might have collided, fall back to getdata now :( - std::vector invs; - invs.push_back(CInv(MSG_BLOCK, resp.blockhash)); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, invs)); + if (first_in_flight) { + // Might have collided, fall back to getdata now :( + std::vector invs; + invs.push_back(CInv(MSG_BLOCK, resp.blockhash)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, invs)); + } else { + RemoveBlockRequest(resp.blockhash, pfrom.GetId()); + LogPrint(BCLog::NET, "Peer %d sent us a compact block but it failed to reconstruct, waiting on first download to complete\n", pfrom.GetId()); + return; + } } else { // Block is either okay, or possibly we received // READ_STATUS_CHECKBLOCK_FAILED. @@ -5039,7 +5121,7 @@ void PeerManagerImpl::ProcessMessage( // though the block was successfully read, and rely on the // handling in ProcessNewBlock to ensure the block index is // updated, etc. - RemoveBlockRequest(resp.blockhash, pfrom.GetId()); // it is now an empty pointer + RemoveBlockRequest(resp.blockhash, std::nullopt); // it is now an empty pointer fBlockRead = true; // mapBlockSource is used for potentially punishing peers and // updating which peers send us compact blocks, so the race @@ -5730,14 +5812,14 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) // valid headers chain with at least as much work as our tip. CNodeState *node_state = State(pnode->GetId()); if (node_state == nullptr || - (now - pnode->m_connected >= MINIMUM_CONNECT_TIME && node_state->nBlocksInFlight == 0)) { + (now - pnode->m_connected >= MINIMUM_CONNECT_TIME && node_state->vBlocksInFlight.empty())) { pnode->fDisconnect = true; LogPrint(BCLog::NET, "disconnecting extra block-relay-only peer=%d (last block received at time %d)\n", pnode->GetId(), count_seconds(pnode->m_last_block_time)); return true; } else { LogPrint(BCLog::NET, "keeping block-relay-only peer=%d chosen for eviction (connect time: %d, blocks_in_flight: %d)\n", - pnode->GetId(), count_seconds(pnode->m_connected), node_state->nBlocksInFlight); + pnode->GetId(), count_seconds(pnode->m_connected), node_state->vBlocksInFlight.size()); } return false; }); @@ -5788,13 +5870,13 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now) // Also don't disconnect any peer we're trying to download a // block from. CNodeState &state = *State(pnode->GetId()); - if (now - pnode->m_connected > MINIMUM_CONNECT_TIME && state.nBlocksInFlight == 0) { + if (now - pnode->m_connected > MINIMUM_CONNECT_TIME && state.vBlocksInFlight.empty()) { LogPrint(BCLog::NET, "disconnecting extra outbound peer=%d (last block announcement received at time %d)\n", pnode->GetId(), oldest_block_announcement); pnode->fDisconnect = true; return true; } else { LogPrint(BCLog::NET, "keeping outbound peer=%d chosen for eviction (connect time: %d, blocks_in_flight: %d)\n", - pnode->GetId(), count_seconds(pnode->m_connected), state.nBlocksInFlight); + pnode->GetId(), count_seconds(pnode->m_connected), state.vBlocksInFlight.size()); return false; } }); @@ -6500,17 +6582,17 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Message: getdata (blocks) // std::vector vGetData; - if (CanServeBlocks(*peer) && pto->CanRelay() && ((sync_blocks_and_headers_from_peer && !IsLimitedPeer(*peer)) || !m_chainman.ActiveChainstate().IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) { + if (CanServeBlocks(*peer) && pto->CanRelay() && ((sync_blocks_and_headers_from_peer && !IsLimitedPeer(*peer)) || !m_chainman.ActiveChainstate().IsInitialBlockDownload()) && state.vBlocksInFlight.size() < MAX_BLOCKS_IN_TRANSIT_PER_PEER) { std::vector vToDownload; NodeId staller = -1; - FindNextBlocksToDownload(*peer, MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.nBlocksInFlight, vToDownload, staller); + FindNextBlocksToDownload(*peer, MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.vBlocksInFlight.size(), vToDownload, staller); for (const CBlockIndex *pindex : vToDownload) { vGetData.emplace_back(MSG_BLOCK, pindex->GetBlockHash()); BlockRequested(pto->GetId(), *pindex); LogPrint(BCLog::NET, "Requesting block %s (%d) peer=%d\n", pindex->GetBlockHash().ToString(), pindex->nHeight, pto->GetId()); } - if (state.nBlocksInFlight == 0 && staller != -1) { + if (state.vBlocksInFlight.empty() && staller != -1) { if (State(staller)->m_stalling_since == 0us) { State(staller)->m_stalling_since = current_time; LogPrint(BCLog::NET, "Stall started peer=%d\n", staller); diff --git a/src/net_processing.h b/src/net_processing.h index 1491c06901b9..295349fdd548 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -42,6 +42,8 @@ static const bool DEFAULT_PEERBLOOMFILTERS = true; static const bool DEFAULT_PEERBLOCKFILTERS = false; /** Threshold for marking a node to be discouraged, e.g. disconnected and added to the discouragement filter. */ static const int DISCOURAGEMENT_THRESHOLD{100}; +/** Maximum number of outstanding CMPCTBLOCK requests for the same block. */ +static const unsigned int MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3; struct CNodeStateStats { int m_misbehavior_score = 0; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index eca003933174..8842cf15b36f 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -485,7 +485,7 @@ static RPCHelpMan getblockfrompeer() "getblockfrompeer", "Attempt to fetch block from a given peer.\n\n" "We must have the header for this block, e.g. using submitheader.\n" - "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n" + "Subsequent calls for the same block may cause the response from the previous peer to be ignored.\n" "Peers generally ignore requests for a stale block that they never fully verified, or one that is more than a month old.\n" "When a peer does not respond with a block, we will disconnect.\n\n" "Returns an empty JSON object if the request was successfully scheduled.", diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py index b0ee1e2a09d6..22cc1dd778c6 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -102,6 +102,10 @@ def clear_block_announcement(self): self.last_message.pop("headers", None) self.last_message.pop("cmpctblock", None) + def clear_getblocktxn(self): + with p2p_lock: + self.last_message.pop("getblocktxn", None) + def get_headers(self, locator, hashstop): msg = msg_getheaders() msg.locator.vHave = locator @@ -708,7 +712,7 @@ def request_cb_announcements(self, peer): peer.get_headers(locator=[int(tip, 16)], hashstop=0) peer.send_and_ping(msg_sendcmpct(announce=True, version=1)) - def test_compactblock_reconstruction_multiple_peers(self, stalling_peer, delivery_peer): + def test_compactblock_reconstruction_stalling_peer(self, stalling_peer, delivery_peer): node = self.nodes[0] assert len(self.utxos) @@ -745,7 +749,15 @@ def announce_cmpct_block(node, peer): delivery_peer.send_message(msg_tx(tx)) delivery_peer.sync_with_ping() - cmpct_block.prefilled_txn[0].tx = CTxIn() + # Corrupt the prefilled coinbase's content while keeping it a + # structurally valid coinbase, so the reconstructed block's merkle root + # no longer matches the announced header. Dash has no witness data to + # mutate the way upstream does here; this mismatch is caught by + # IsBlockMutated inside FillBlock (READ_STATUS_FAILED -> getdata + # fallback) rather than by InitData, so the peer is not disconnected and + # relay via the stalling peer still works. + cmpct_block.prefilled_txn[0].tx.vout[0].nValue += 1 + cmpct_block.prefilled_txn[0].tx.rehash() delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p())) assert int(node.getbestblockhash(), 16) != block.sha256 @@ -784,12 +796,85 @@ def assert_highbandwidth_states(node, hb_to, hb_from): hb_test_node.send_and_ping(msg_sendcmpct(announce=False, version=1)) assert_highbandwidth_states(self.nodes[0], hb_to=True, hb_from=False) + def test_compactblock_reconstruction_parallel_reconstruction(self, stalling_peer, delivery_peer, inbound_peer, outbound_peer): + """ All p2p connections are inbound except outbound_peer. We test that ultimate parallel slot + can only be taken by an outbound node unless prior attempts were done by an outbound + """ + node = self.nodes[0] + assert len(self.utxos) + + def announce_cmpct_block(node, peer, txn_count): + utxo = self.utxos.pop(0) + block = self.build_block_with_transactions(node, utxo, txn_count) + + cmpct_block = HeaderAndShortIDs() + cmpct_block.initialize_from_block(block) + msg = msg_cmpctblock(cmpct_block.to_p2p()) + peer.send_and_ping(msg) + with p2p_lock: + assert "getblocktxn" in peer.last_message + return block, cmpct_block + + for name, peer in [("delivery", delivery_peer), ("inbound", inbound_peer), ("outbound", outbound_peer)]: + self.log.info(f"Setting {name} as high bandwidth peer") + block, cmpct_block = announce_cmpct_block(node, peer, 1) + msg = msg_blocktxn() + msg.block_transactions.blockhash = block.sha256 + msg.block_transactions.transactions = block.vtx[1:] + peer.send_and_ping(msg) + assert_equal(int(node.getbestblockhash(), 16), block.sha256) + peer.clear_getblocktxn() + + # Test the simple parallel download case... + for num_missing in [1, 5, 20]: + + # Remaining low-bandwidth peer is stalling_peer, who announces first + assert_equal([peer['bip152_hb_to'] for peer in node.getpeerinfo()], [False, True, True, True]) + + block, cmpct_block = announce_cmpct_block(node, stalling_peer, num_missing) + + delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p())) + with p2p_lock: + # The second peer to announce should still get a getblocktxn + assert "getblocktxn" in delivery_peer.last_message + assert int(node.getbestblockhash(), 16) != block.sha256 + + inbound_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p())) + with p2p_lock: + # The third inbound peer to announce should *not* get a getblocktxn + assert "getblocktxn" not in inbound_peer.last_message + assert int(node.getbestblockhash(), 16) != block.sha256 + + outbound_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p())) + with p2p_lock: + # The third peer to announce should get a getblocktxn if outbound + assert "getblocktxn" in outbound_peer.last_message + assert int(node.getbestblockhash(), 16) != block.sha256 + + # Second peer completes the compact block first + msg = msg_blocktxn() + msg.block_transactions.blockhash = block.sha256 + msg.block_transactions.transactions = block.vtx[1:] + delivery_peer.send_and_ping(msg) + assert_equal(int(node.getbestblockhash(), 16), block.sha256) + + # Nothing bad should happen if we get a late fill from the first peer... + stalling_peer.send_and_ping(msg) + self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue]) + + delivery_peer.clear_getblocktxn() + inbound_peer.clear_getblocktxn() + outbound_peer.clear_getblocktxn() + + def run_test(self): self.wallet = MiniWallet(self.nodes[0]) # Setup the p2p connections self.test_node = self.nodes[0].add_p2p_connection(TestP2PConn()) self.additional_test_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_HEADERS_COMPRESSED) + self.onemore_inbound_node = self.nodes[0].add_p2p_connection(TestP2PConn()) + self.outbound_node = self.nodes[0].add_outbound_p2p_connection(TestP2PConn(), p2p_idx=3, connection_type="outbound-full-relay") # We will need UTXOs to construct transactions in later tests. self.make_utxos() @@ -797,6 +882,8 @@ def run_test(self): self.log.info("Testing SENDCMPCT p2p message... ") self.test_sendcmpct(self.test_node) self.test_sendcmpct(self.additional_test_node) + self.test_sendcmpct(self.onemore_inbound_node) + self.test_sendcmpct(self.outbound_node) self.log.info("Testing compactblock construction...") self.test_compactblock_construction(self.test_node) @@ -813,8 +900,11 @@ def run_test(self): self.log.info("Testing handling of incorrect blocktxn responses...") self.test_incorrect_blocktxn_response(self.test_node) - self.log.info("Testing reconstructing compact blocks from all peers...") - self.test_compactblock_reconstruction_multiple_peers(self.test_node, self.additional_test_node) + self.log.info("Testing reconstructing compact blocks with a stalling peer...") + self.test_compactblock_reconstruction_stalling_peer(self.test_node, self.additional_test_node) + + self.log.info("Testing reconstructing compact blocks from multiple peers...") + self.test_compactblock_reconstruction_parallel_reconstruction(stalling_peer=self.test_node, inbound_peer=self.onemore_inbound_node, delivery_peer=self.additional_test_node, outbound_peer=self.outbound_node) # End-to-end block relay tests self.log.info("Testing end-to-end block relay...") From 6668c2a8f8d28342f0a5b6d42b7c50e059bea9b6 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 1 Jul 2026 12:23:35 -0500 Subject: [PATCH 3/7] Merge bitcoin/bitcoin#27743: Unconditionally return when compact block status == READ_STATUS_FAILED Backport of bitcoin/bitcoin#27743 (commit d97269579). Applied manually onto the post-#27626 CMPCTBLOCK handler; Dash already uses plain CInv(MSG_BLOCK, ...) (no GetFetchFlags). --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e7a73823cd15..222632a319bc 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4938,11 +4938,11 @@ void PeerManagerImpl::ProcessMessage( std::vector vInv(1); vInv[0] = CInv(MSG_BLOCK, blockhash); m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); - return; } else { // Give up for this peer and wait for other peer(s) RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); } + return; } BlockTransactionsRequest req; From 5306dfae5fe5684cd143af89e994538000de7aa3 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 1 Jul 2026 12:35:21 -0500 Subject: [PATCH 4/7] Merge bitcoin/bitcoin#26969: net, refactor: net_processing, add ProcessCompactBlockTxns Backport of bitcoin/bitcoin#26969 (commit 77d6d89d43c). Applied manually onto Dash's post-#27626/#27743 tree because the auto-merge duplicated class members; the extraction uses Dash's own BLOCKTXN handler body. Dash adaptations: Misbehaving(pfrom.GetId(), ...) (NodeId API), plain CInv(MSG_BLOCK, ...) (no GetFetchFlags), ProcessBlock(..., /*force_processing=*/true) (no min_pow_checked), and the method's lock annotation adds !m_peer_mutex (required by Dash's Misbehaving). Also aligns the success-path RemoveBlockRequest to upstream's pfrom.GetId() (the #27626 auto-backport had used std::nullopt). --- src/net_processing.cpp | 190 +++++++++++++++++++++-------------------- 1 file changed, 97 insertions(+), 93 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 222632a319bc..9998ac903231 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1075,6 +1075,10 @@ class PeerManagerImpl final : public PeerManager /** Process a new block. Perform any post-processing housekeeping */ void ProcessBlock(CNode& from, const std::shared_ptr& pblock, bool force_processing); + /** Process compact block txns */ + void ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const BlockTransactions& block_transactions) + EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex, !m_most_recent_block_mutex); + /** Relay map (txid -> CTransactionRef) */ typedef std::map MapRelay; MapRelay mapRelay GUARDED_BY(cs_main); @@ -3623,6 +3627,93 @@ void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr pblock = std::make_shared(); + bool fBlockRead{false}; + const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); + { + LOCK(cs_main); + + auto range_flight = mapBlocksInFlight.equal_range(block_transactions.blockhash); + size_t already_in_flight = std::distance(range_flight.first, range_flight.second); + bool requested_block_from_this_peer{false}; + + // Multimap ensures ordering of outstanding requests. It's either empty or first in line. + bool first_in_flight = already_in_flight == 0 || (range_flight.first->second.first == pfrom.GetId()); + + while (range_flight.first != range_flight.second) { + auto [node_id, block_it] = range_flight.first->second; + if (node_id == pfrom.GetId() && block_it->partialBlock) { + requested_block_from_this_peer = true; + break; + } + range_flight.first++; + } + + if (!requested_block_from_this_peer) { + LogPrint(BCLog::NET, "Peer %d sent us block transactions for block we weren't expecting\n", pfrom.GetId()); + return; + } + + PartiallyDownloadedBlock& partialBlock = *range_flight.first->second.second->partialBlock; + ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn); + if (status == READ_STATUS_INVALID) { + RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // Reset in-flight state in case Misbehaving does not result in a disconnect + Misbehaving(peer, 100, "invalid compact block/non-matching block transactions"); + return; + } else if (status == READ_STATUS_FAILED) { + if (first_in_flight) { + // Might have collided, fall back to getdata now :( + std::vector invs; + invs.push_back(CInv(MSG_BLOCK, block_transactions.blockhash)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, invs)); + } else { + RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); + LogPrint(BCLog::NET, "Peer %d sent us a compact block but it failed to reconstruct, waiting on first download to complete\n", pfrom.GetId()); + return; + } + } else { + // Block is either okay, or possibly we received + // READ_STATUS_CHECKBLOCK_FAILED. + // Note that CheckBlock can only fail for one of a few reasons: + // 1. bad-proof-of-work (impossible here, because we've already + // accepted the header) + // 2. merkleroot doesn't match the transactions given (already + // caught in FillBlock with READ_STATUS_FAILED, so + // impossible here) + // 3. the block is otherwise invalid (eg invalid coinbase, + // block is too big, too many legacy sigops, etc). + // So if CheckBlock failed, #3 is the only possibility. + // Under BIP 152, we don't discourage the peer unless proof of work is + // invalid (we don't require all the stateless checks to have + // been run). This is handled below, so just treat this as + // though the block was successfully read, and rely on the + // handling in ProcessNewBlock to ensure the block index is + // updated, etc. + RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // it is now an empty pointer + fBlockRead = true; + // mapBlockSource is used for potentially punishing peers and + // updating which peers send us compact blocks, so the race + // between here and cs_main in ProcessNewBlock is fine. + // BIP 152 permits peers to relay compact blocks after validating + // the header only; we should not punish peers if the block turns + // out to be invalid. + mapBlockSource.emplace(block_transactions.blockhash, std::make_pair(pfrom.GetId(), false)); + } + } // Don't hold cs_main when we call into ProcessNewBlock + if (fBlockRead) { + // Since we requested this block (it was in mapBlocksInFlight), force it to be processed, + // even if it would not be a candidate for new tip (missing previous block, chain not long enough, etc) + // This bypasses some anti-DoS logic in AcceptBlock (eg to prevent + // disk-space attacks), but this should be safe due to the + // protections in the compact block handler -- see related comment + // in compact block optimistic reconstruction handling. + ProcessBlock(pfrom, pblock, /*force_processing=*/true); + } + return; +} + void PeerManagerImpl::PostProcessMessage(MessageProcessingResult&& result, NodeId node) { if (result.m_error) { @@ -4845,12 +4936,7 @@ void PeerManagerImpl::ProcessMessage( blockhash.ToString(), pfrom.GetId()); } - // When we succeed in decoding a block's txids from a cmpctblock - // message we typically jump to the BLOCKTXN handling code, with a - // dummy (empty) BLOCKTXN message, to re-use the logic there in - // completing processing of the putative block (without cs_main). bool fProcessBLOCKTXN = false; - CDataStream blockTxnMsg(SER_NETWORK, PROTOCOL_VERSION); // If we end up treating this as a plain headers message, call that as well // without cs_main. @@ -4951,10 +5037,6 @@ void PeerManagerImpl::ProcessMessage( req.indexes.push_back(i); } if (req.indexes.empty()) { - // Dirty hack to jump to BLOCKTXN code (TODO: move message handling into their own functions) - BlockTransactions txn; - txn.blockhash = blockhash; - blockTxnMsg << txn; fProcessBLOCKTXN = true; } else if (first_in_flight) { // We will try to round-trip any compact blocks we get on failure, @@ -5008,8 +5090,11 @@ void PeerManagerImpl::ProcessMessage( } } // cs_main - if (fProcessBLOCKTXN) - return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, time_received, interruptMsgProc); + if (fProcessBLOCKTXN) { + BlockTransactions txn; + txn.blockhash = blockhash; + return ProcessCompactBlockTxns(pfrom, *peer, txn); + } if (fRevertToHeaderProcessing) { // Headers received from HB compact block peers are permitted to be @@ -5060,88 +5145,7 @@ void PeerManagerImpl::ProcessMessage( BlockTransactions resp; vRecv >> resp; - std::shared_ptr pblock = std::make_shared(); - bool fBlockRead = false; - { - LOCK(cs_main); - - auto range_flight = mapBlocksInFlight.equal_range(resp.blockhash); - size_t already_in_flight = std::distance(range_flight.first, range_flight.second); - bool requested_block_from_this_peer{false}; - - // Multimap ensures ordering of outstanding requests. It's either empty or first in line. - bool first_in_flight = already_in_flight == 0 || (range_flight.first->second.first == pfrom.GetId()); - - while (range_flight.first != range_flight.second) { - auto [node_id, block_it] = range_flight.first->second; - if (node_id == pfrom.GetId() && block_it->partialBlock) { - requested_block_from_this_peer = true; - break; - } - range_flight.first++; - } - - if (!requested_block_from_this_peer) { - LogPrint(BCLog::NET, "Peer %d sent us block transactions for block we weren't expecting\n", pfrom.GetId()); - return; - } - - PartiallyDownloadedBlock& partialBlock = *range_flight.first->second.second->partialBlock; - 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(*peer, 100, "invalid compact block/non-matching block transactions"); - return; - } else if (status == READ_STATUS_FAILED) { - if (first_in_flight) { - // Might have collided, fall back to getdata now :( - std::vector invs; - invs.push_back(CInv(MSG_BLOCK, resp.blockhash)); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, invs)); - } else { - RemoveBlockRequest(resp.blockhash, pfrom.GetId()); - LogPrint(BCLog::NET, "Peer %d sent us a compact block but it failed to reconstruct, waiting on first download to complete\n", pfrom.GetId()); - return; - } - } else { - // Block is either okay, or possibly we received - // READ_STATUS_CHECKBLOCK_FAILED. - // Note that CheckBlock can only fail for one of a few reasons: - // 1. bad-proof-of-work (impossible here, because we've already - // accepted the header) - // 2. merkleroot doesn't match the transactions given (already - // caught in FillBlock with READ_STATUS_FAILED, so - // impossible here) - // 3. the block is otherwise invalid (eg invalid coinbase, - // block is too big, too many legacy sigops, etc). - // So if CheckBlock failed, #3 is the only possibility. - // Under BIP 152, we don't discourage the peer unless proof of work is - // invalid (we don't require all the stateless checks to have - // been run). This is handled below, so just treat this as - // though the block was successfully read, and rely on the - // handling in ProcessNewBlock to ensure the block index is - // updated, etc. - RemoveBlockRequest(resp.blockhash, std::nullopt); // it is now an empty pointer - fBlockRead = true; - // mapBlockSource is used for potentially punishing peers and - // updating which peers send us compact blocks, so the race - // between here and cs_main in ProcessNewBlock is fine. - // BIP 152 permits peers to relay compact blocks after validating - // the header only; we should not punish peers if the block turns - // out to be invalid. - mapBlockSource.emplace(resp.blockhash, std::make_pair(pfrom.GetId(), false)); - } - } // Don't hold cs_main when we call into ProcessNewBlock - if (fBlockRead) { - // Since we requested this block (it was in mapBlocksInFlight), force it to be processed, - // even if it would not be a candidate for new tip (missing previous block, chain not long enough, etc) - // This bypasses some anti-DoS logic in AcceptBlock (eg to prevent - // disk-space attacks), but this should be safe due to the - // protections in the compact block handler -- see related comment - // in compact block optimistic reconstruction handling. - ProcessBlock(pfrom, pblock, /*force_processing=*/true); - } - return; + return ProcessCompactBlockTxns(pfrom, *peer, resp); } if (msg_type == NetMsgType::HEADERS || msg_type == NetMsgType::HEADERS2) { From c9b3d19d71b7c76a8189b46376fe035c07785dd5 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 1 Jul 2026 12:43:49 -0500 Subject: [PATCH 5/7] Merge bitcoin/bitcoin#29412: p2p: Don't process mutated blocks Backport of bitcoin/bitcoin#29412, slimmed for Dash. Introduces IsBlockMutated() and isolates the merkle-root check into CheckMerkleRoot() (with an m_checked_merkle_root cache on CBlock), and rejects/discourages mutated blocks early in the BLOCK message handler as a defence-in-depth DoS mitigation. Dash adaptations: Dash is not a segwit chain and has none of the witness-commitment primitives (GetWitnessCommitmentIndex, BlockWitnessMerkleRoot, HasWitness, m_checked_witness_commitment). IsBlockMutated therefore drops the witness/check_witness_root machinery entirely and reduces to the merkle-root and CVE-2012-2459 / 64-byte-transaction malleation checks; it uses ::GetSerializeSize(*tx, PROTOCOL_VERSION) instead of TX_NO_WITNESS. The early net_processing check is unconditional (no prev_block/DEPLOYMENT_SEGWIT gating) and uses Misbehaving(pfrom.GetId(), 100, ...). --- src/net_processing.cpp | 11 +++ src/primitives/block.h | 6 +- src/test/validation_tests.cpp | 97 ++++++++++++++++++++++++++ src/validation.cpp | 66 +++++++++++++++--- src/validation.h | 4 ++ test/functional/p2p_mutated_blocks.py | 99 +++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 7 files changed, 271 insertions(+), 13 deletions(-) create mode 100755 test/functional/p2p_mutated_blocks.py diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9998ac903231..1232a9cfba77 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5200,6 +5200,17 @@ void PeerManagerImpl::ProcessMessage( LogPrint(BCLog::NET, "received block %s peer=%d\n", pblock->GetHash().ToString(), pfrom.GetId()); + // Check for possible mutation as a defence-in-depth mitigation against + // attacks that leverage mutated blocks. Dash has no witness commitment, + // so this reduces to the merkle-root / 64-byte-transaction malleation + // checks and can be performed unconditionally. + if (IsBlockMutated(/*block=*/*pblock)) { + LogPrint(BCLog::NET, "Received mutated block from peer=%d\n", pfrom.GetId()); + Misbehaving(*peer, 100, "mutated block"); + WITH_LOCK(cs_main, RemoveBlockRequest(pblock->GetHash(), pfrom.GetId())); + return; + } + bool forceProcessing = false; const uint256 hash(pblock->GetHash()); { diff --git a/src/primitives/block.h b/src/primitives/block.h index f33e44ad3904..92061e5d82bb 100644 --- a/src/primitives/block.h +++ b/src/primitives/block.h @@ -189,8 +189,9 @@ class CBlock : public CBlockHeader // network and disk std::vector vtx; - // memory only - mutable bool fChecked; + // Memory-only flags for caching expensive checks + mutable bool fChecked; // CheckBlock() + mutable bool m_checked_merkle_root{false}; // CheckMerkleRoot() CBlock() { @@ -214,6 +215,7 @@ class CBlock : public CBlockHeader CBlockHeader::SetNull(); vtx.clear(); fChecked = false; + m_checked_merkle_root = false; } CBlockHeader GetBlockHeader() const diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index e0503db6aa95..60263280834a 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -3,9 +3,13 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include +#include +#include #include #include +#include #include @@ -36,4 +40,97 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo) BOOST_CHECK_EQUAL(out210.nChainTx, 200U); } +//! Test the Dash (non-witness) IsBlockMutated() predicate directly. +BOOST_AUTO_TEST_CASE(block_malleation) +{ + // Calls IsBlockMutated and clears the CBlock validity-cache flags so the + // same block object can be re-tested. + auto is_mutated = [](CBlock& block) { + bool mutated{IsBlockMutated(block)}; + block.fChecked = false; + block.m_checked_merkle_root = false; + return mutated; + }; + auto is_not_mutated = [&is_mutated](CBlock& block) { + return !is_mutated(block); + }; + + auto create_coinbase_tx = []() { + CMutableTransaction coinbase; + coinbase.vin.resize(1); + coinbase.vout.resize(1); + coinbase.vout[0].scriptPubKey.resize(4); + auto tx = MakeTransactionRef(coinbase); + assert(tx->IsCoinBase()); + return tx; + }; + + CBlock block; + + // An empty block is expected to have a merkle root of 0x0. + BOOST_CHECK(block.vtx.empty()); + block.hashMerkleRoot = uint256::ONE; + BOOST_CHECK(is_mutated(block)); + block.hashMerkleRoot = uint256(); + BOOST_CHECK(is_not_mutated(block)); + + // A block with a single coinbase tx is mutated if the merkle root does not + // equal the coinbase tx's hash. + block.vtx.push_back(create_coinbase_tx()); + BOOST_CHECK(block.vtx[0]->GetHash() != block.hashMerkleRoot); + BOOST_CHECK(is_mutated(block)); + block.hashMerkleRoot = BlockMerkleRoot(block); + BOOST_CHECK(is_not_mutated(block)); + + // A block with two transactions is mutated if the merkle root does not + // match the transactions. + block.vtx.push_back(MakeTransactionRef(CMutableTransaction{})); + BOOST_CHECK(is_mutated(block)); + block.hashMerkleRoot = BlockMerkleRoot(block); + BOOST_CHECK(is_not_mutated(block)); + + // A block with a duplicated transaction (CVE-2012-2459) is mutated even + // when hashMerkleRoot is set to the value the malleable tree computes. + block.vtx[1] = block.vtx[0]; + bool mutated{false}; + block.hashMerkleRoot = BlockMerkleRoot(block, &mutated); + BOOST_CHECK(mutated); + BOOST_CHECK(is_mutated(block)); + + // A block with a single 64-byte coinbase transaction is NOT considered + // mutated: the 64-byte malleation guard is only applied when the first + // transaction is not a coinbase. + block.vtx.clear(); + { + CMutableTransaction mtx; + mtx.vin.resize(1); + mtx.vout.resize(1); + mtx.vout[0].scriptPubKey.resize(4); + auto tx = MakeTransactionRef(mtx); + assert(tx->IsCoinBase()); + assert(::GetSerializeSize(*tx, PROTOCOL_VERSION) == 64); + block.vtx.push_back(tx); + block.hashMerkleRoot = BlockMerkleRoot(block); + } + BOOST_CHECK(is_not_mutated(block)); + + // Conversely, a coinbase-less block that contains a 64-byte transaction is + // treated as mutated (the retained CVE-2012-2459 / 64-byte guard; see + // "Weaknesses in Bitcoin's Merkle Root Construction"). + block.vtx.clear(); + { + CMutableTransaction mtx; + mtx.vin.resize(1); + mtx.vin[0].prevout = COutPoint(uint256::ONE, 0); // non-null -> not a coinbase + mtx.vout.resize(1); + mtx.vout[0].scriptPubKey.resize(4); + auto tx = MakeTransactionRef(mtx); + assert(!tx->IsCoinBase()); + assert(::GetSerializeSize(*tx, PROTOCOL_VERSION) == 64); + block.vtx.push_back(tx); + block.hashMerkleRoot = BlockMerkleRoot(block); + } + BOOST_CHECK(is_mutated(block)); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index 0b1b5a99c3c2..a57796595dc6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3788,6 +3788,33 @@ static bool CheckBlockHeader(const CBlockHeader& block, const uint256& hash, Blo return true; } +static bool CheckMerkleRoot(const CBlock& block, BlockValidationState& state) +{ + if (block.m_checked_merkle_root) return true; + + bool mutated; + uint256 merkle_root = BlockMerkleRoot(block, &mutated); + if (block.hashMerkleRoot != merkle_root) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-txnmrklroot", + /*debug_message=*/"hashMerkleRoot mismatch"); + } + + // Check for merkle tree malleability (CVE-2012-2459): repeating sequences + // of transactions in a block without affecting the merkle root of a block, + // while still invalidating it. + if (mutated) { + return state.Invalid( + /*result=*/BlockValidationResult::BLOCK_MUTATED, + /*reject_reason=*/"bad-txns-duplicate", + /*debug_message=*/"duplicate transaction"); + } + + block.m_checked_merkle_root = true; + return true; +} + bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW, bool fCheckMerkleRoot, const uint256* known_hash) { // These are checks that are independent of context. @@ -3806,17 +3833,8 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu return false; // Check the merkle root. - if (fCheckMerkleRoot) { - bool mutated; - uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); - if (block.hashMerkleRoot != hashMerkleRoot2) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txnmrklroot", "hashMerkleRoot mismatch"); - - // Check for merkle tree malleability (CVE-2012-2459): repeating sequences - // of transactions in a block without affecting the merkle root of a block, - // while still invalidating it. - if (mutated) - return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-txns-duplicate", "duplicate transaction"); + if (fCheckMerkleRoot && !CheckMerkleRoot(block, state)) { + return false; } // All potential-corruption validation must be done before we do any @@ -3865,6 +3883,32 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu return true; } +bool IsBlockMutated(const CBlock& block) +{ + BlockValidationState state; + if (!CheckMerkleRoot(block, state)) { + LogPrint(BCLog::VALIDATION, "Block mutated: %s\n", state.ToString()); + return true; + } + + if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) { + // Consider the block mutated if any transaction is 64 bytes in size (see 3.1 + // in "Weaknesses in Bitcoin's Merkle Root Construction": + // https://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20190225/a27d8837/attachment-0001.pdf). + // + // Note: This is not a consensus change as this only applies to blocks that + // don't have a coinbase transaction and would therefore already be invalid. + return std::any_of(block.vtx.begin(), block.vtx.end(), + [](auto& tx) { return ::GetSerializeSize(*tx, PROTOCOL_VERSION) == 64; }); + } else { + // Theoretically it is still possible for a block with a 64 byte + // coinbase transaction to be mutated but we neglect that possibility + // here as it requires at least 224 bits of work. + } + + return false; +} + /** Context-dependent validity checks. * By "context", we mean only the previous block headers, but not the UTXO * set; UTXO-related validity checks are done in ConnectBlock(). diff --git a/src/validation.h b/src/validation.h index 35a489fe6272..627c241e9f6a 100644 --- a/src/validation.h +++ b/src/validation.h @@ -374,6 +374,10 @@ void InitScriptExecutionCache(); /** Context-independent validity checks */ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true, const uint256* known_hash = nullptr); +/** Check if a block has been mutated (with respect to its merkle root and, + * for defence-in-depth, CVE-2012-2459 / 64-byte-transaction malleation). */ +bool IsBlockMutated(const CBlock& block); + /** Check a block is completely valid from start to finish (only works on top of our current best block) */ bool TestBlockValidity(BlockValidationState& state, const chainlock::Chainlocks& chainlocks, diff --git a/test/functional/p2p_mutated_blocks.py b/test/functional/p2p_mutated_blocks.py new file mode 100755 index 000000000000..bf55131f613c --- /dev/null +++ b/test/functional/p2p_mutated_blocks.py @@ -0,0 +1,99 @@ +#!/usr/bin/env python3 +# Copyright (c) The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +""" +Test that an attacker can't degrade compact block relay by sending unsolicited +mutated blocks to clear in-flight blocktxn requests from other honest peers. +""" + +from test_framework.p2p import P2PInterface +from test_framework.messages import ( + BlockTransactions, + msg_cmpctblock, + msg_block, + msg_blocktxn, + HeaderAndShortIDs, +) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.blocktools import ( + COINBASE_MATURITY, + create_block, + NORMAL_GBT_REQUEST_PARAMS, +) +from test_framework.util import assert_equal +from test_framework.wallet import MiniWallet +import copy + +class MutatedBlocksTest(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 1 + + def run_test(self): + self.wallet = MiniWallet(self.nodes[0]) + self.generate(self.wallet, COINBASE_MATURITY) + + honest_relayer = self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="outbound-full-relay") + attacker = self.nodes[0].add_p2p_connection(P2PInterface()) + + # Create new block with two transactions (coinbase + 1 self-transfer). + # The self-transfer transaction is needed to trigger a compact block + # `getblocktxn` roundtrip. + tx = self.wallet.create_self_transfer()["tx"] + block = create_block(tmpl=self.nodes[0].getblocktemplate(NORMAL_GBT_REQUEST_PARAMS), txlist=[tx]) + block.solve() + + # Create mutated version of the block by changing the transaction + # version on the self-transfer. Dash has no witness data, so mutating a + # transaction (which changes its hash and hence the block's merkle root) + # is what trips the mutation check. + mutated_block = copy.deepcopy(block) + mutated_block.vtx[1].nVersion = 4 + + # Announce the new block via a compact block through the honest relayer + cmpctblock = HeaderAndShortIDs() + cmpctblock.initialize_from_block(block) + honest_relayer.send_message(msg_cmpctblock(cmpctblock.to_p2p())) + + # Wait for a `getblocktxn` that attempts to fetch the self-transfer + def self_transfer_requested(): + if not honest_relayer.last_message.get('getblocktxn'): + return False + + get_block_txn = honest_relayer.last_message['getblocktxn'] + return ( + get_block_txn.block_txn_request.blockhash == block.sha256 + and get_block_txn.block_txn_request.indexes == [1] + ) + honest_relayer.wait_until(self_transfer_requested, timeout=5) + + # Block at height 101 should be the only one in flight from peer 0 + peer_info_prior_to_attack = self.nodes[0].getpeerinfo() + assert_equal(peer_info_prior_to_attack[0]['id'], 0) + assert_equal([101], peer_info_prior_to_attack[0]["inflight"]) + + # Attempt to clear the honest relayer's download request by sending the + # mutated block (as the attacker). + with self.nodes[0].assert_debug_log(expected_msgs=["Block mutated: bad-txnmrklroot, hashMerkleRoot mismatch"]): + attacker.send_message(msg_block(mutated_block)) + # Attacker should get disconnected for sending a mutated block + attacker.wait_for_disconnect(timeout=5) + + # Block at height 101 should *still* be the only block in-flight from + # peer 0 + peer_info_after_attack = self.nodes[0].getpeerinfo() + assert_equal(peer_info_after_attack[0]['id'], 0) + assert_equal([101], peer_info_after_attack[0]["inflight"]) + + # The honest relayer should be able to complete relaying the block by + # sending the blocktxn that was requested. + block_txn = msg_blocktxn() + block_txn.block_transactions = BlockTransactions(blockhash=block.sha256, transactions=[tx]) + honest_relayer.send_and_ping(block_txn) + assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256) + + +if __name__ == '__main__': + MutatedBlocksTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index b398e6e55e47..f199d1f8e74d 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -332,6 +332,7 @@ 'p2p_leak.py', 'p2p_compactblocks.py', 'p2p_compactblocks_blocksonly.py', + 'p2p_mutated_blocks.py', # NOTE: needs dash_hash to pass 'p2p_connect_to_devnet.py', 'feature_sporks.py', 'rpc_getblockstats.py', From fe1e05d6dc34481d20591f3cfe6667ce3c66bf24 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 1 Jul 2026 12:47:30 -0500 Subject: [PATCH 6/7] Merge bitcoin/bitcoin#32646: p2p: Add witness mutation check inside FillBlock Backport of bitcoin/bitcoin#32646 (commits bac9ee48306, 28299ce7763), slimmed for Dash. PartiallyDownloadedBlock::FillBlock now calls IsBlockMutated() instead of CheckBlock() as a belt-and-suspenders mutation check on reconstructed compact blocks, and the vestigial READ_STATUS_CHECKBLOCK_FAILED status is removed. Dash adaptations: no segwit, so FillBlock keeps its 2-arg signature (no segwit_active parameter) and the mutation mock hook becomes IsBlockMutatedFn = std::function (m_check_block_mutated_mock). The fuzz harness is updated to mock IsBlockMutated instead of CheckBlock. --- src/blockencodings.cpp | 14 +++----- src/blockencodings.h | 6 ++-- src/net_processing.cpp | 18 +--------- src/test/fuzz/partially_downloaded_block.cpp | 37 +++++--------------- 4 files changed, 15 insertions(+), 60 deletions(-) diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index f1f54d61ab21..f12b45b49354 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -203,16 +203,10 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< if (vtx_missing.size() != tx_missing_offset) return READ_STATUS_INVALID; - BlockValidationState state; - CheckBlockFn check_block = m_check_block_mock ? m_check_block_mock : CheckBlock; - if (!check_block(block, state, Params().GetConsensus(), /*fCheckPoW=*/true, /*fCheckMerkleRoot=*/true)) { - // TODO: We really want to just check merkle tree manually here, - // but that is expensive, and CheckBlock caches a block's - // "checked-status" (in the CBlock?). CBlock should be able to - // check its own merkle root and cache that check. - if (state.GetResult() == BlockValidationResult::BLOCK_MUTATED) - return READ_STATUS_FAILED; // Possible Short ID collision - return READ_STATUS_CHECKBLOCK_FAILED; + // Check for possible mutations early now that we have a seemingly good block + IsBlockMutatedFn check_mutated{m_check_block_mutated_mock ? m_check_block_mutated_mock : IsBlockMutated}; + if (check_mutated(/*block=*/block)) { + return READ_STATUS_FAILED; // Possible Short ID collision } LogPrint(BCLog::CMPCTBLOCK, "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool (incl at least %lu from extra pool) and %lu txn requested\n", hash.ToString(), prefilled_count, mempool_count, extra_count, vtx_missing.size()); diff --git a/src/blockencodings.h b/src/blockencodings.h index 29a3d3025b39..61210ef9b78f 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -84,8 +84,6 @@ typedef enum ReadStatus_t READ_STATUS_OK, READ_STATUS_INVALID, // Invalid object, peer is sending bogus crap READ_STATUS_FAILED, // Failed to process object - READ_STATUS_CHECKBLOCK_FAILED, // Used only by FillBlock to indicate a - // failure in CheckBlock. } ReadStatus; class CBlockHeaderAndShortTxIDs { @@ -136,8 +134,8 @@ class PartiallyDownloadedBlock { CBlockHeader header; // Can be overriden for testing - using CheckBlockFn = std::function; - CheckBlockFn m_check_block_mock{nullptr}; + using IsBlockMutatedFn = std::function; + IsBlockMutatedFn m_check_block_mutated_mock{nullptr}; explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {} diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1232a9cfba77..157b78f9bf8f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3674,23 +3674,7 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl return; } } else { - // Block is either okay, or possibly we received - // READ_STATUS_CHECKBLOCK_FAILED. - // Note that CheckBlock can only fail for one of a few reasons: - // 1. bad-proof-of-work (impossible here, because we've already - // accepted the header) - // 2. merkleroot doesn't match the transactions given (already - // caught in FillBlock with READ_STATUS_FAILED, so - // impossible here) - // 3. the block is otherwise invalid (eg invalid coinbase, - // block is too big, too many legacy sigops, etc). - // So if CheckBlock failed, #3 is the only possibility. - // Under BIP 152, we don't discourage the peer unless proof of work is - // invalid (we don't require all the stateless checks to have - // been run). This is handled below, so just treat this as - // though the block was successfully read, and rely on the - // handling in ProcessNewBlock to ensure the block index is - // updated, etc. + // Block is okay for further processing RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // it is now an empty pointer fBlockRead = true; // mapBlockSource is used for potentially punishing peers and diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp index be3d9adbdf11..7e523113e7e4 100644 --- a/src/test/fuzz/partially_downloaded_block.cpp +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -28,14 +28,10 @@ void initialize_pdb() g_setup = testing_setup.get(); } -PartiallyDownloadedBlock::CheckBlockFn FuzzedCheckBlock(std::optional result) +PartiallyDownloadedBlock::IsBlockMutatedFn FuzzedIsBlockMutated(bool result) { - return [result](const CBlock&, BlockValidationState& state, const Consensus::Params&, bool, bool) { - if (result) { - return state.Invalid(*result); - } - - return true; + return [result](const CBlock& block) { + return result; }; } @@ -103,37 +99,20 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) skipped_missing |= (!pdb.IsTxAvailable(i) && skip); } - // Mock CheckBlock - bool fail_check_block{fuzzed_data_provider.ConsumeBool()}; - auto validation_result = - fuzzed_data_provider.PickValueInArray( - {BlockValidationResult::BLOCK_RESULT_UNSET, - BlockValidationResult::BLOCK_CONSENSUS, - BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE, - BlockValidationResult::BLOCK_CACHED_INVALID, - BlockValidationResult::BLOCK_INVALID_HEADER, - BlockValidationResult::BLOCK_MUTATED, - BlockValidationResult::BLOCK_MISSING_PREV, - BlockValidationResult::BLOCK_INVALID_PREV, - BlockValidationResult::BLOCK_TIME_FUTURE, - BlockValidationResult::BLOCK_CHECKPOINT, - BlockValidationResult::BLOCK_CHAINLOCK}); - pdb.m_check_block_mock = FuzzedCheckBlock( - fail_check_block ? - std::optional{validation_result} : - std::nullopt); + // Mock IsBlockMutated + bool fail_block_mutated{fuzzed_data_provider.ConsumeBool()}; + pdb.m_check_block_mutated_mock = FuzzedIsBlockMutated(fail_block_mutated); CBlock reconstructed_block; auto fill_status{pdb.FillBlock(reconstructed_block, missing)}; switch (fill_status) { case READ_STATUS_OK: assert(!skipped_missing); - assert(!fail_check_block); + assert(!fail_block_mutated); assert(block->GetHash() == reconstructed_block.GetHash()); break; - case READ_STATUS_CHECKBLOCK_FAILED: [[fallthrough]]; case READ_STATUS_FAILED: - assert(fail_check_block); + assert(fail_block_mutated); break; case READ_STATUS_INVALID: break; From 9d947f3a4f3d83cc6432431203a1065c120db902 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 1 Jul 2026 12:52:06 -0500 Subject: [PATCH 7/7] Merge bitcoin/bitcoin#33296: net: check for empty header before calling FillBlock Backport of bitcoin/bitcoin#33296 (commits 5e585a0fc4f, 8b626476803), slimmed for Dash. Guards ProcessCompactBlockTxns against re-entering PartiallyDownloadedBlock::FillBlock when a prior reconstruction attempt returned READ_STATUS_FAILED and left the (now header-less) partial block in flight; such a second blocktxn now discourages and disconnects the peer with a clear reason. Adds the test_multiple_blocktxn_response regression test to p2p_compactblocks.py. Dash adaptations: no LookupBlockIndex/DEPLOYMENT_SEGWIT (Dash's FillBlock takes no segwit_active parameter), and Misbehaving(pfrom.GetId(), 100, ...). Note on the double-blocktxn SIGABRT: on this branch that crash is already eliminated by #26898 (FillBlock returns READ_STATUS_INVALID instead of asserting on a null header, which the caller then handles by discouraging and removing the block request). Upstream's guard exists to avoid a LookupBlockIndex/Assume() on the wiped header, which Dash does not perform. This guard is therefore a faithful belt-and-suspenders port -- it rejects the duplicate earlier with a clearer message and skips a wasted reconstruction pass -- rather than the fix for a still-reachable crash. --- src/net_processing.cpp | 14 +++++++++ test/functional/p2p_compactblocks.py | 44 ++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 157b78f9bf8f..a0fb53e99651 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3657,6 +3657,17 @@ 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). Don't attempt to + // reconstruct again. + RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); + Misbehaving(peer, 100, "previous compact block reconstruction attempt failed"); + LogPrint(BCLog::NET, "Peer %d sent compact block transactions multiple times\n", pfrom.GetId()); + return; + } + ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn); if (status == READ_STATUS_INVALID) { RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // Reset in-flight state in case Misbehaving does not result in a disconnect @@ -3665,6 +3676,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.push_back(CInv(MSG_BLOCK, block_transactions.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 22cc1dd778c6..f90a264a05c3 100755 --- a/test/functional/p2p_compactblocks.py +++ b/test/functional/p2p_compactblocks.py @@ -556,6 +556,45 @@ def test_incorrect_blocktxn_response(self, test_node): test_node.send_and_ping(msg_block(block)) assert_equal(int(node.getbestblockhash(), 16), block.sha256) + # Multiple blocktxn responses for the same in-flight block must not cause a + # re-entrant FillBlock (which previously hit an assert); the second response + # should instead get the peer disconnected (see bitcoin#33296). + 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, prefilling only the coinbase so the node has to + # request the other two transactions via getblocktxn. + comp_block = HeaderAndShortIDs() + comp_block.initialize_from_block(block, prefill_list=[0]) + test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p())) + 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]) + + # Respond with the transactions in the wrong order so reconstruction + # fails (merkle root mismatch), triggering the getdata fallback while + # the PartiallyDownloadedBlock is left in flight. + 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 for the full block + test_node.wait_for_getdata([block.sha256], timeout=10) + assert test_node.last_message["getdata"].inv[0].type == MSG_BLOCK + + # Sending the same blocktxn again must get the peer disconnected rather + # than re-entering FillBlock on the (now header-less) partial block. + 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): node = self.nodes[0] # dashd will not send blocktxn responses for blocks whose height is @@ -921,5 +960,10 @@ def run_test(self): self.log.info("Testing high-bandwidth mode states via getpeerinfo...") self.test_highbandwidth_mode_states_via_getpeerinfo() + self.log.info("Testing handling of multiple blocktxn responses...") + # Earlier tests may have left self.test_node disconnected; use a fresh peer. + self.test_node = self.nodes[0].add_p2p_connection(TestP2PConn()) + self.test_multiple_blocktxn_response(self.test_node) + if __name__ == '__main__': CompactBlocksTest().main()