From 613a45cd4b5482aedbdc7c61c839ea05996935c6 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 4 May 2024 15:33:36 -0400 Subject: [PATCH 1/2] net: reduce LOCK(cs_main) scope in GETBLOCKTXN Also adds a static assertion that MAX_BLOCKTXN_DEPTH <= MIN_BLOCKS_TO_KEEP --- src/net_processing.cpp | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9b5983b9d0769..84f9a5dc8b23e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -118,6 +118,7 @@ static const unsigned int MAX_HEADERS_RESULTS = 2000; static const int MAX_CMPCTBLOCK_DEPTH = 5; /** Maximum depth of blocks we're willing to respond to GETBLOCKTXN requests for. */ static const int MAX_BLOCKTXN_DEPTH = 10; +static_assert(MAX_BLOCKTXN_DEPTH <= MIN_BLOCKS_TO_KEEP, "MAX_BLOCKTXN_DEPTH too high"); /** Size of the "block download window": how far ahead of our current height do we fetch? * Larger windows tolerate larger download speed differences between peer, but increase the potential * degree of disordering of blocks on disk (which make reindexing and pruning harder). We'll probably @@ -4366,6 +4367,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, return; } + FlatFilePos block_pos{}; { LOCK(cs_main); @@ -4376,15 +4378,21 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } if (pindex->nHeight >= m_chainman.ActiveChain().Height() - MAX_BLOCKTXN_DEPTH) { - CBlock block; - const bool ret{m_chainman.m_blockman.ReadBlockFromDisk(block, *pindex)}; - assert(ret); - - SendBlockTransactions(pfrom, *peer, block, req); - return; + block_pos = pindex->GetBlockPos(); } } + if (!block_pos.IsNull()) { + CBlock block; + const bool ret{m_chainman.m_blockman.ReadBlockFromDisk(block, block_pos)}; + // If height is above MAX_BLOCKTXN_DEPTH then this block cannot get + // pruned after we release cs_main above, so this read should never fail. + assert(ret); + + SendBlockTransactions(pfrom, *peer, block, req); + return; + } + // If an older block is requested (should never happen in practice, // but can happen in tests) send a block response instead of a // blocktxn response. Sending a full block response instead of a From 75d27fefc7a04ebdda7be5724a014b6a896e7325 Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Sat, 4 May 2024 15:38:39 -0400 Subject: [PATCH 2/2] net: reduce LOCK(cs_main) scope in ProcessGetBlockData This also changes behavior if ReadBlockFromDisk or ReadRawBlockFromDisk fails. Previously, the node would crash due to an assert. This has been replaced with logging the error, disconnecting the peer, and returning early. --- src/net_processing.cpp | 96 ++++++++++++++++++++++++++---------------- 1 file changed, 59 insertions(+), 37 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 84f9a5dc8b23e..57e2f7409b272 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2421,38 +2421,48 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } } - LOCK(cs_main); - const CBlockIndex* pindex = m_chainman.m_blockman.LookupBlockIndex(inv.hash); - if (!pindex) { - return; - } - if (!BlockRequestAllowed(pindex)) { - LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom.GetId()); - return; - } - // disconnect node in case we have reached the outbound limit for serving historical blocks - if (m_connman.OutboundTargetReached(true) && - (((m_chainman.m_best_header != nullptr) && (m_chainman.m_best_header->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.IsMsgFilteredBlk()) && - !pfrom.HasPermission(NetPermissionFlags::Download) // nodes with the download permission may exceed target - ) { - LogPrint(BCLog::NET, "historical block serving limit reached, disconnect peer=%d\n", pfrom.GetId()); - pfrom.fDisconnect = true; - return; - } - // Avoid leaking prune-height by never sending blocks below the NODE_NETWORK_LIMITED threshold - if (!pfrom.HasPermission(NetPermissionFlags::NoBan) && ( - (((peer.m_our_services & NODE_NETWORK_LIMITED) == NODE_NETWORK_LIMITED) && ((peer.m_our_services & NODE_NETWORK) != NODE_NETWORK) && (m_chainman.ActiveChain().Tip()->nHeight - pindex->nHeight > (int)NODE_NETWORK_LIMITED_MIN_BLOCKS + 2 /* add two blocks buffer extension for possible races */) ) - )) { - LogPrint(BCLog::NET, "Ignore block request below NODE_NETWORK_LIMITED threshold, disconnect peer=%d\n", pfrom.GetId()); - //disconnect node and prevent it from stalling (would otherwise wait for the missing block) - pfrom.fDisconnect = true; - return; - } - // Pruned nodes may have deleted the block, so check whether - // it's available before trying to send. - if (!(pindex->nStatus & BLOCK_HAVE_DATA)) { - return; + const CBlockIndex* pindex{nullptr}; + const CBlockIndex* tip{nullptr}; + bool can_direct_fetch{false}; + FlatFilePos block_pos{}; + { + LOCK(cs_main); + pindex = m_chainman.m_blockman.LookupBlockIndex(inv.hash); + if (!pindex) { + return; + } + if (!BlockRequestAllowed(pindex)) { + LogPrint(BCLog::NET, "%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom.GetId()); + return; + } + // disconnect node in case we have reached the outbound limit for serving historical blocks + if (m_connman.OutboundTargetReached(true) && + (((m_chainman.m_best_header != nullptr) && (m_chainman.m_best_header->GetBlockTime() - pindex->GetBlockTime() > HISTORICAL_BLOCK_AGE)) || inv.IsMsgFilteredBlk()) && + !pfrom.HasPermission(NetPermissionFlags::Download) // nodes with the download permission may exceed target + ) { + LogPrint(BCLog::NET, "historical block serving limit reached, disconnect peer=%d\n", pfrom.GetId()); + pfrom.fDisconnect = true; + return; + } + tip = m_chainman.ActiveChain().Tip(); + // Avoid leaking prune-height by never sending blocks below the NODE_NETWORK_LIMITED threshold + if (!pfrom.HasPermission(NetPermissionFlags::NoBan) && ( + (((peer.m_our_services & NODE_NETWORK_LIMITED) == NODE_NETWORK_LIMITED) && ((peer.m_our_services & NODE_NETWORK) != NODE_NETWORK) && (tip->nHeight - pindex->nHeight > (int)NODE_NETWORK_LIMITED_MIN_BLOCKS + 2 /* add two blocks buffer extension for possible races */) ) + )) { + LogPrint(BCLog::NET, "Ignore block request below NODE_NETWORK_LIMITED threshold, disconnect peer=%d\n", pfrom.GetId()); + //disconnect node and prevent it from stalling (would otherwise wait for the missing block) + pfrom.fDisconnect = true; + return; + } + // Pruned nodes may have deleted the block, so check whether + // it's available before trying to send. + if (!(pindex->nStatus & BLOCK_HAVE_DATA)) { + return; + } + can_direct_fetch = CanDirectFetch(); + block_pos = pindex->GetBlockPos(); } + std::shared_ptr pblock; if (a_recent_block && a_recent_block->GetHash() == pindex->GetBlockHash()) { pblock = a_recent_block; @@ -2460,16 +2470,28 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // Fast-path: in this case it is possible to serve the block directly from disk, // as the network format matches the format on disk std::vector block_data; - if (!m_chainman.m_blockman.ReadRawBlockFromDisk(block_data, pindex->GetBlockPos())) { - assert(!"cannot load block from disk"); + if (!m_chainman.m_blockman.ReadRawBlockFromDisk(block_data, block_pos)) { + if (WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.IsBlockPruned(*pindex))) { + LogPrint(BCLog::NET, "Block was pruned before it could be read, disconnect peer=%s\n", pfrom.GetId()); + } else { + LogError("Cannot load block from disk, disconnect peer=%d\n", pfrom.GetId()); + } + pfrom.fDisconnect = true; + return; } MakeAndPushMessage(pfrom, NetMsgType::BLOCK, Span{block_data}); // Don't set pblock as we've sent the block } else { // Send block from disk std::shared_ptr pblockRead = std::make_shared(); - if (!m_chainman.m_blockman.ReadBlockFromDisk(*pblockRead, *pindex)) { - assert(!"cannot load block from disk"); + if (!m_chainman.m_blockman.ReadBlockFromDisk(*pblockRead, block_pos)) { + if (WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.IsBlockPruned(*pindex))) { + LogPrint(BCLog::NET, "Block was pruned before it could be read, disconnect peer=%s\n", pfrom.GetId()); + } else { + LogError("Cannot load block from disk, disconnect peer=%d\n", pfrom.GetId()); + } + pfrom.fDisconnect = true; + return; } pblock = pblockRead; } @@ -2507,7 +2529,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // they won't have a useful mempool to match against a compact block, // and we don't feel like constructing the object for them, so // instead we respond with the full, non-compact block. - if (CanDirectFetch() && pindex->nHeight >= m_chainman.ActiveChain().Height() - MAX_CMPCTBLOCK_DEPTH) { + if (can_direct_fetch && pindex->nHeight >= tip->nHeight - MAX_CMPCTBLOCK_DEPTH) { if (a_recent_compact_block && a_recent_compact_block->header.GetHash() == pindex->GetBlockHash()) { MakeAndPushMessage(pfrom, NetMsgType::CMPCTBLOCK, *a_recent_compact_block); } else { @@ -2528,7 +2550,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& // and we want it right after the last block so they don't // wait for other stuff first. std::vector vInv; - vInv.emplace_back(MSG_BLOCK, m_chainman.ActiveChain().Tip()->GetBlockHash()); + vInv.emplace_back(MSG_BLOCK, tip->GetBlockHash()); MakeAndPushMessage(pfrom, NetMsgType::INV, vInv); peer.m_continuation_block.SetNull(); }