From 77d6d89d43cc5969c98d9b4b56a1e877b473e731 Mon Sep 17 00:00:00 2001 From: brunoerg Date: Wed, 25 Jan 2023 07:27:11 -0300 Subject: [PATCH] net: net_processing, add `ProcessCompactBlockTxns` When processing `CMPCTBLOCK` message, at some moments we can need to process cmpct block txns, since all messages are handled by ProcessMessage, we call ProcessMessage all over again. For this reason, it creates a function called `ProcessCompactBlockTxns` to process it. --- src/net_processing.cpp | 187 +++++++++++++++++++++-------------------- 1 file changed, 95 insertions(+), 92 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6597019797..6ced5aec04 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -918,6 +918,10 @@ private: /** Process a new block. Perform any post-processing housekeeping */ void ProcessBlock(CNode& node, const std::shared_ptr& block, bool force_processing, bool min_pow_checked); + /** 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); + /** * When a peer sends us a valid block, instruct it to announce blocks to us * using CMPCTBLOCK if possible by adding its nodeid to the end of @@ -3204,6 +3208,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 | GetFetchFlags(peer), 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, /*min_pow_checked=*/true); + } + return; +} + void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) @@ -4276,12 +4367,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 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. @@ -4382,10 +4468,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 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, @@ -4440,7 +4522,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } // cs_main if (fProcessBLOCKTXN) { - return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, time_received, interruptMsgProc); + BlockTransactions txn; + txn.blockhash = blockhash; + return ProcessCompactBlockTxns(pfrom, *peer, txn); } if (fRevertToHeaderProcessing) { @@ -4492,88 +4576,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 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 | GetFetchFlags(*peer), 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, 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, /*min_pow_checked=*/true); - } - return; + return ProcessCompactBlockTxns(pfrom, *peer, resp); } if (msg_type == NetMsgType::HEADERS)