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..f12b45b49354 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()); @@ -198,15 +203,10 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< if (vtx_missing.size() != tx_missing_offset) return READ_STATUS_INVALID; - BlockValidationState state; - if (!CheckBlock(block, state, Params().GetConsensus())) { - // 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 a19db7db2dd3..61210ef9b78f 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. @@ -79,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 { @@ -129,6 +132,11 @@ class PartiallyDownloadedBlock { const CTxMemPool* pool; public: CBlockHeader header; + + // Can be overriden for testing + using IsBlockMutatedFn = std::function; + IsBlockMutatedFn m_check_block_mutated_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/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..a0fb53e99651 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}; @@ -1071,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); @@ -1240,40 +1248,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; - state->nBlocksInFlight--; - if (state->nBlocksInFlight == 0) { - // Last validated block on the queue was received. - m_peers_downloading_from--; + if (from_peer && *from_peer != node_id) { + range.first++; + continue; + } + + CNodeState& state = *Assert(State(node_id)); + + 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 +1306,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 +1529,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 +1816,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 +2065,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 +3163,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; } @@ -3594,6 +3627,91 @@ 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; + + 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 + 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 :( + // 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)); + } 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 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 + // 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) { @@ -4816,12 +4934,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. @@ -4846,15 +4959,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 +4990,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,10 +5017,15 @@ 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)); + 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)); + } else { + // Give up for this peer and wait for other peer(s) + RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); + } return; } @@ -4904,14 +5035,25 @@ 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 { + } 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 +5074,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); @@ -4946,8 +5088,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 @@ -4998,68 +5143,7 @@ void PeerManagerImpl::ProcessMessage( BlockTransactions resp; vRecv >> resp; - std::shared_ptr pblock = std::make_shared(); - bool fBlockRead = false; - { - 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()) { - 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; - 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)); - } 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, 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(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) { @@ -5114,6 +5198,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()); { @@ -5730,14 +5825,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 +5883,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 +6595,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/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/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/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp new file mode 100644 index 000000000000..7e523113e7e4 --- /dev/null +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -0,0 +1,120 @@ +#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::IsBlockMutatedFn FuzzedIsBlockMutated(bool result) +{ + return [result](const CBlock& block) { + return result; + }; +} + +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 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_block_mutated); + assert(block->GetHash() == reconstructed_block.GetHash()); + break; + case READ_STATUS_FAILED: + assert(fail_block_mutated); + break; + case READ_STATUS_INVALID: + break; + } +} 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_compactblocks.py b/test/functional/p2p_compactblocks.py index b0ee1e2a09d6..f90a264a05c3 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 @@ -552,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 @@ -708,7 +751,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 +788,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 +835,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 +921,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 +939,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...") @@ -831,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() 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',