From 85e058b19145b5068f2f71a90c1182bf2a93c473 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 3 Jun 2021 12:33:16 +0100 Subject: [PATCH 1/8] [net processing] Remove unnecessary hash arg from MarkBlockAsInFlight() MarkBlockAsInFlight is always called with a non-null pindex. Just get the block hash from that pindex inside the function. --- src/net_processing.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 65224b4259..9a6c62a81f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -472,7 +472,7 @@ private: * Returns false, still setting pit, if the block was already in flight from the same peer * pit will only be valid as long as the same cs_main lock is being held */ - bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pindex, std::list::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -782,8 +782,11 @@ bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) return false; } -bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex, std::list::iterator** pit) +bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pindex, std::list::iterator** pit) { + assert(pindex); + const uint256& hash{pindex->GetBlockHash()}; + CNodeState *state = State(nodeid); assert(state != nullptr); @@ -807,7 +810,7 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, co // We're starting a block download (batch) from this peer. state->m_downloading_since = GetTime(); } - if (state->nBlocksInFlightValidHeaders == 1 && pindex != nullptr) { + if (state->nBlocksInFlightValidHeaders == 1) { nPeersWithValidatedDownloads++; } itInFlight = mapBlocksInFlight.insert(std::make_pair(hash, std::make_pair(nodeid, it))).first; @@ -2081,7 +2084,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer, } uint32_t nFetchFlags = GetFetchFlags(pfrom); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); - MarkBlockAsInFlight(pfrom.GetId(), pindex->GetBlockHash(), pindex); + MarkBlockAsInFlight(pfrom.GetId(), pindex); LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n", pindex->GetBlockHash().ToString(), pfrom.GetId()); } @@ -3384,7 +3387,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if ((!fAlreadyInFlight && nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) || (fAlreadyInFlight && blockInFlightIt->second.first == pfrom.GetId())) { std::list::iterator* queuedBlockIt = nullptr; - if (!MarkBlockAsInFlight(pfrom.GetId(), pindex->GetBlockHash(), pindex, &queuedBlockIt)) { + if (!MarkBlockAsInFlight(pfrom.GetId(), pindex, &queuedBlockIt)) { if (!(*queuedBlockIt)->partialBlock) (*queuedBlockIt)->partialBlock.reset(new PartiallyDownloadedBlock(&m_mempool)); else { @@ -4767,7 +4770,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) for (const CBlockIndex *pindex : vToDownload) { uint32_t nFetchFlags = GetFetchFlags(*pto); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); - MarkBlockAsInFlight(pto->GetId(), pindex->GetBlockHash(), pindex); + MarkBlockAsInFlight(pto->GetId(), pindex); LogPrint(BCLog::NET, "Requesting block %s (%d) peer=%d\n", pindex->GetBlockHash().ToString(), pindex->nHeight, pto->GetId()); } From b4e29f2436943c131dd25b123d13a25ce09bab58 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 3 Jun 2021 12:38:44 +0100 Subject: [PATCH 2/8] [net processing] Remove QueuedBlock.fValidatedHeaders Since headers-first syncing, we only ever request a block if we've already validated its headers. Therefore QueuedBlock.fValidatedHeaders is always set to true. Remove it. --- src/net_processing.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9a6c62a81f..bb60222e5f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -159,10 +159,12 @@ static constexpr size_t MAX_ADDR_TO_SEND{1000}; namespace { /** Blocks that are in flight, and that are in the queue to be downloaded. */ struct QueuedBlock { + /** Block hash */ uint256 hash; - const CBlockIndex* pindex; //!< Optional. - bool fValidatedHeaders; //!< Whether this block has validated headers at the time of request. - std::unique_ptr partialBlock; //!< Optional, used for CMPCTBLOCK downloads + /** BlockIndex. We must have this since we only request blocks when we've already validated the header. */ + const CBlockIndex* pindex; + /** Optional, used for CMPCTBLOCK downloads */ + std::unique_ptr partialBlock; }; /** @@ -764,8 +766,8 @@ bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) if (itInFlight != mapBlocksInFlight.end()) { CNodeState *state = State(itInFlight->second.first); assert(state != nullptr); - state->nBlocksInFlightValidHeaders -= itInFlight->second.second->fValidatedHeaders; - if (state->nBlocksInFlightValidHeaders == 0 && itInFlight->second.second->fValidatedHeaders) { + state->nBlocksInFlightValidHeaders -= 1; + if (state->nBlocksInFlightValidHeaders == 0) { // Last validated block on the queue was received. nPeersWithValidatedDownloads--; } @@ -803,9 +805,9 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pind MarkBlockAsReceived(hash); std::list::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(), - {hash, pindex, pindex != nullptr, std::unique_ptr(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)}); + {hash, pindex, std::unique_ptr(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)}); state->nBlocksInFlight++; - state->nBlocksInFlightValidHeaders += it->fValidatedHeaders; + state->nBlocksInFlightValidHeaders += 1; if (state->nBlocksInFlight == 1) { // We're starting a block download (batch) from this peer. state->m_downloading_since = GetTime(); From b03de9c7538d85b12929a62b9ec966fd3910e660 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 3 Jun 2021 12:45:15 +0100 Subject: [PATCH 3/8] [net processing] Remove CNodeState.nBlocksInFlightValidHeaders nBlocksInFlightValidHeaders always has the same value as nBlocksInFlight, since we only download blocks with valid headers. --- src/net_processing.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index bb60222e5f..07cfd6f4e6 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -629,7 +629,6 @@ struct CNodeState { //! 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}; - int nBlocksInFlightValidHeaders{0}; //! Whether we consider this a preferred download peer. bool fPreferredDownload{false}; //! Whether this peer wants invs or headers (when possible) for block announcements. @@ -766,17 +765,16 @@ bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) if (itInFlight != mapBlocksInFlight.end()) { CNodeState *state = State(itInFlight->second.first); assert(state != nullptr); - state->nBlocksInFlightValidHeaders -= 1; - if (state->nBlocksInFlightValidHeaders == 0) { - // Last validated block on the queue was received. - nPeersWithValidatedDownloads--; - } if (state->vBlocksInFlight.begin() == itInFlight->second.second) { // 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(itInFlight->second.second); state->nBlocksInFlight--; + if (state->nBlocksInFlight == 0) { + // Last validated block on the queue was received. + nPeersWithValidatedDownloads--; + } state->m_stalling_since = 0us; mapBlocksInFlight.erase(itInFlight); return true; @@ -807,12 +805,9 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pind std::list::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(), {hash, pindex, std::unique_ptr(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)}); state->nBlocksInFlight++; - state->nBlocksInFlightValidHeaders += 1; if (state->nBlocksInFlight == 1) { // We're starting a block download (batch) from this peer. state->m_downloading_since = GetTime(); - } - if (state->nBlocksInFlightValidHeaders == 1) { nPeersWithValidatedDownloads++; } itInFlight = mapBlocksInFlight.insert(std::make_pair(hash, std::make_pair(nodeid, it))).first; @@ -1139,7 +1134,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) WITH_LOCK(g_cs_orphans, m_orphanage.EraseForPeer(nodeid)); m_txrequest.DisconnectedPeer(nodeid); nPreferredDownload -= state->fPreferredDownload; - nPeersWithValidatedDownloads -= (state->nBlocksInFlightValidHeaders != 0); + nPeersWithValidatedDownloads -= (state->nBlocksInFlight != 0); assert(nPeersWithValidatedDownloads >= 0); m_outbound_peers_with_protect_from_disconnect -= state->m_chain_sync.m_protect; assert(m_outbound_peers_with_protect_from_disconnect >= 0); @@ -4717,7 +4712,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // to unreasonably increase our timeout. if (state.vBlocksInFlight.size() > 0) { QueuedBlock &queuedBlock = state.vBlocksInFlight.front(); - int nOtherPeersWithValidatedDownloads = nPeersWithValidatedDownloads - (state.nBlocksInFlightValidHeaders > 0); + int nOtherPeersWithValidatedDownloads = nPeersWithValidatedDownloads - 1; if (current_time > state.m_downloading_since + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) { LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.hash.ToString(), pto->GetId()); pto->fDisconnect = true; From 156a19ee6a22789adedcb6ab067c2eca2d3bfdfe Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 3 Jun 2021 12:49:27 +0100 Subject: [PATCH 4/8] scripted-diff: rename nPeersWithValidatedDownloads -BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren nPeersWithValidatedDownloads m_peers_downloading_from -END VERIFY SCRIPT- --- src/net_processing.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 07cfd6f4e6..ed579b2c07 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -514,7 +514,7 @@ private: std::list lNodesAnnouncingHeaderAndIDs GUARDED_BY(cs_main); /** Number of peers from which we're downloading blocks. */ - int nPeersWithValidatedDownloads GUARDED_BY(cs_main) = 0; + int m_peers_downloading_from GUARDED_BY(cs_main) = 0; /** Storage for orphan information */ TxOrphanage m_orphanage; @@ -773,7 +773,7 @@ bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) state->nBlocksInFlight--; if (state->nBlocksInFlight == 0) { // Last validated block on the queue was received. - nPeersWithValidatedDownloads--; + m_peers_downloading_from--; } state->m_stalling_since = 0us; mapBlocksInFlight.erase(itInFlight); @@ -808,7 +808,7 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pind if (state->nBlocksInFlight == 1) { // We're starting a block download (batch) from this peer. state->m_downloading_since = GetTime(); - nPeersWithValidatedDownloads++; + m_peers_downloading_from++; } itInFlight = mapBlocksInFlight.insert(std::make_pair(hash, std::make_pair(nodeid, it))).first; if (pit) @@ -1134,8 +1134,8 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) WITH_LOCK(g_cs_orphans, m_orphanage.EraseForPeer(nodeid)); m_txrequest.DisconnectedPeer(nodeid); nPreferredDownload -= state->fPreferredDownload; - nPeersWithValidatedDownloads -= (state->nBlocksInFlight != 0); - assert(nPeersWithValidatedDownloads >= 0); + m_peers_downloading_from -= (state->nBlocksInFlight != 0); + 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); m_wtxid_relay_peers -= state->m_wtxid_relay; @@ -1147,7 +1147,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) // Do a consistency check after the last peer is removed. assert(mapBlocksInFlight.empty()); assert(nPreferredDownload == 0); - assert(nPeersWithValidatedDownloads == 0); + assert(m_peers_downloading_from == 0); assert(m_outbound_peers_with_protect_from_disconnect == 0); assert(m_wtxid_relay_peers == 0); assert(m_txrequest.Size() == 0); @@ -4712,7 +4712,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // to unreasonably increase our timeout. if (state.vBlocksInFlight.size() > 0) { QueuedBlock &queuedBlock = state.vBlocksInFlight.front(); - int nOtherPeersWithValidatedDownloads = nPeersWithValidatedDownloads - 1; + int nOtherPeersWithValidatedDownloads = m_peers_downloading_from - 1; if (current_time > state.m_downloading_since + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) { LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.hash.ToString(), pto->GetId()); pto->fDisconnect = true; From 4e90d2dd0e91e7eb560f2c1b430f13c7a047804f Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 3 Jun 2021 12:57:43 +0100 Subject: [PATCH 5/8] [net processing] Remove QueuedBlock.hash It's redundant with CBlockIndex::GetBlockHash() --- src/net_processing.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ed579b2c07..be09bde39f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -159,8 +159,6 @@ static constexpr size_t MAX_ADDR_TO_SEND{1000}; namespace { /** Blocks that are in flight, and that are in the queue to be downloaded. */ struct QueuedBlock { - /** Block hash */ - uint256 hash; /** BlockIndex. We must have this since we only request blocks when we've already validated the header. */ const CBlockIndex* pindex; /** Optional, used for CMPCTBLOCK downloads */ @@ -803,7 +801,7 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pind MarkBlockAsReceived(hash); std::list::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(), - {hash, pindex, std::unique_ptr(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)}); + {pindex, std::unique_ptr(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)}); state->nBlocksInFlight++; if (state->nBlocksInFlight == 1) { // We're starting a block download (batch) from this peer. @@ -1129,7 +1127,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) nSyncStarted--; for (const QueuedBlock& entry : state->vBlocksInFlight) { - mapBlocksInFlight.erase(entry.hash); + mapBlocksInFlight.erase(entry.pindex->GetBlockHash()); } WITH_LOCK(g_cs_orphans, m_orphanage.EraseForPeer(nodeid)); m_txrequest.DisconnectedPeer(nodeid); @@ -4714,7 +4712,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) QueuedBlock &queuedBlock = state.vBlocksInFlight.front(); int nOtherPeersWithValidatedDownloads = m_peers_downloading_from - 1; if (current_time > state.m_downloading_since + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) { - LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.hash.ToString(), pto->GetId()); + LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.pindex->GetBlockHash().ToString(), pto->GetId()); pto->fDisconnect = true; return true; } From 62993507336be06490b202b3955d4830a99e9e34 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 3 Jun 2021 13:11:22 +0100 Subject: [PATCH 6/8] [net processing] Add IsBlockRequested() function MarkBlockAsReceived() should not be used for both removing the block from mapBlocksInFlight and checking whether it was in the map. --- src/net_processing.cpp | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index be09bde39f..0cfb91e353 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -463,10 +463,14 @@ private: Mutex m_recent_confirmed_transactions_mutex; std::unique_ptr m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex); - /* Returns a bool indicating whether we requested this block. - * Also used if a block was /not/ received and timed out or started with another peer + /** Have we requested this block from a peer */ + bool IsBlockRequested(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 request for the block has timed out */ - bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /* Mark a block as in flight * Returns false, still setting pit, if the block was already in flight from the same peer @@ -757,7 +761,12 @@ static void UpdatePreferredDownload(const CNode& node, CNodeState* state) EXCLUS nPreferredDownload += state->fPreferredDownload; } -bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) +bool PeerManagerImpl::IsBlockRequested(const uint256& hash) +{ + return mapBlocksInFlight.find(hash) != mapBlocksInFlight.end(); +} + +void PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) { std::map::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash); if (itInFlight != mapBlocksInFlight.end()) { @@ -775,9 +784,7 @@ bool PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) } state->m_stalling_since = 0us; mapBlocksInFlight.erase(itInFlight); - return true; } - return false; } bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pindex, std::list::iterator** pit) @@ -976,7 +983,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(NodeId nodeid, unsigned int count if (pindex->nStatus & BLOCK_HAVE_DATA || m_chainman.ActiveChain().Contains(pindex)) { if (pindex->HaveTxsDownloaded()) state->pindexLastCommonBlock = pindex; - } else if (mapBlocksInFlight.count(pindex->GetBlockHash()) == 0) { + } else if (!IsBlockRequested(pindex->GetBlockHash())) { // The block is not already downloaded, and not yet in flight. if (pindex->nHeight > nWindowEnd) { // We reached the end of the window. @@ -2054,7 +2061,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer, // Calculate all the blocks we'd need to switch to pindexLast, up to a limit. while (pindexWalk && !m_chainman.ActiveChain().Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) && - !mapBlocksInFlight.count(pindexWalk->GetBlockHash()) && + !IsBlockRequested(pindexWalk->GetBlockHash()) && (!IsWitnessEnabled(pindexWalk->pprev, m_chainparams.GetConsensus()) || State(pfrom.GetId())->fHaveWitness)) { // We don't have this block, and it's not yet in flight. vToFetch.push_back(pindexWalk); @@ -2825,7 +2832,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); UpdateBlockAvailability(pfrom.GetId(), inv.hash); - if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) { + if (!fAlreadyHave && !fImporting && !fReindex && !IsBlockRequested(inv.hash)) { // Headers-first is the primary method of announcement on // the network. If a node fell back to sending blocks by inv, // it's probably for a re-org. The final block hash @@ -3613,9 +3620,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, const uint256 hash(pblock->GetHash()); { LOCK(cs_main); - // Also always process if we requested the block explicitly, as we may - // need it even though it is not a candidate for a new best tip. - forceProcessing |= MarkBlockAsReceived(hash); + // Always process the block if we requested it, since we may + // need it even when it's not a candidate for a new best tip. + forceProcessing = IsBlockRequested(hash); + MarkBlockAsReceived(hash); // mapBlockSource is only used for punishing peers and setting // which peers send us compact blocks, so the race between here and // cs_main in ProcessNewBlock is fine. From 2c45f832e87acd11fbd144cc0bb8e49816933c70 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 3 Jun 2021 13:55:31 +0100 Subject: [PATCH 7/8] [net processing] Tidy up MarkBlockAsReceived() --- src/net_processing.cpp | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0cfb91e353..b016bab710 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -768,23 +768,29 @@ bool PeerManagerImpl::IsBlockRequested(const uint256& hash) void PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) { - std::map::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash); - if (itInFlight != mapBlocksInFlight.end()) { - CNodeState *state = State(itInFlight->second.first); - assert(state != nullptr); - if (state->vBlocksInFlight.begin() == itInFlight->second.second) { - // 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(itInFlight->second.second); - state->nBlocksInFlight--; - if (state->nBlocksInFlight == 0) { - // Last validated block on the queue was received. - m_peers_downloading_from--; - } - state->m_stalling_since = 0us; - mapBlocksInFlight.erase(itInFlight); + auto it = mapBlocksInFlight.find(hash); + if (it == mapBlocksInFlight.end()) { + // Block was not requested + return; } + + auto [node_id, list_it] = it->second; + CNodeState *state = State(node_id); + assert(state != nullptr); + + 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); + + state->nBlocksInFlight--; + if (state->nBlocksInFlight == 0) { + // Last validated block on the queue was received. + m_peers_downloading_from--; + } + state->m_stalling_since = 0us; + mapBlocksInFlight.erase(it); } bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pindex, std::list::iterator** pit) From 2f4ad6b7efa408b8a858e87499bf6cfcdf936d73 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 3 Jun 2021 12:49:27 +0100 Subject: [PATCH 8/8] scripted-diff: rename MarkBlockAs functions -BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); } ren MarkBlockAsInFlight BlockRequested ren MarkBlockAsReceived RemoveBlockRequest -END VERIFY SCRIPT- --- src/net_processing.cpp | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b016bab710..c5a389f228 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -470,13 +470,13 @@ private: * - the block has been recieved from a peer * - the request for the block has timed out */ - void MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + void RemoveBlockRequest(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /* Mark a block as in flight * Returns false, still setting pit, if the block was already in flight from the same peer * pit will only be valid as long as the same cs_main lock is being held */ - bool MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pindex, std::list::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool BlockRequested(NodeId nodeid, const CBlockIndex* pindex, std::list::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool TipMayBeStale() EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -766,7 +766,7 @@ bool PeerManagerImpl::IsBlockRequested(const uint256& hash) return mapBlocksInFlight.find(hash) != mapBlocksInFlight.end(); } -void PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) +void PeerManagerImpl::RemoveBlockRequest(const uint256& hash) { auto it = mapBlocksInFlight.find(hash); if (it == mapBlocksInFlight.end()) { @@ -793,7 +793,7 @@ void PeerManagerImpl::MarkBlockAsReceived(const uint256& hash) mapBlocksInFlight.erase(it); } -bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pindex, std::list::iterator** pit) +bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex* pindex, std::list::iterator** pit) { assert(pindex); const uint256& hash{pindex->GetBlockHash()}; @@ -811,7 +811,7 @@ bool PeerManagerImpl::MarkBlockAsInFlight(NodeId nodeid, const CBlockIndex* pind } // Make sure it's not listed somewhere already. - MarkBlockAsReceived(hash); + RemoveBlockRequest(hash); std::list::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(), {pindex, std::unique_ptr(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)}); @@ -2092,7 +2092,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer, } uint32_t nFetchFlags = GetFetchFlags(pfrom); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); - MarkBlockAsInFlight(pfrom.GetId(), pindex); + BlockRequested(pfrom.GetId(), pindex); LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n", pindex->GetBlockHash().ToString(), pfrom.GetId()); } @@ -3395,7 +3395,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if ((!fAlreadyInFlight && nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) || (fAlreadyInFlight && blockInFlightIt->second.first == pfrom.GetId())) { std::list::iterator* queuedBlockIt = nullptr; - if (!MarkBlockAsInFlight(pfrom.GetId(), pindex, &queuedBlockIt)) { + if (!BlockRequested(pfrom.GetId(), pindex, &queuedBlockIt)) { if (!(*queuedBlockIt)->partialBlock) (*queuedBlockIt)->partialBlock.reset(new PartiallyDownloadedBlock(&m_mempool)); else { @@ -3408,7 +3408,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, PartiallyDownloadedBlock& partialBlock = *(*queuedBlockIt)->partialBlock; ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact); if (status == READ_STATUS_INVALID) { - MarkBlockAsReceived(pindex->GetBlockHash()); // Reset in-flight state in case Misbehaving does not result in a disconnect + RemoveBlockRequest(pindex->GetBlockHash()); // Reset in-flight state in case Misbehaving does not result in a disconnect Misbehaving(pfrom.GetId(), 100, "invalid compact block"); return; } else if (status == READ_STATUS_FAILED) { @@ -3503,7 +3503,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // process from some other peer. We do this after calling // ProcessNewBlock so that a malleated cmpctblock announcement // can't be used to interfere with block relay. - MarkBlockAsReceived(pblock->GetHash()); + RemoveBlockRequest(pblock->GetHash()); } } return; @@ -3535,7 +3535,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock; ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn); if (status == READ_STATUS_INVALID) { - MarkBlockAsReceived(resp.blockhash); // Reset in-flight state in case Misbehaving does not result in a disconnect + RemoveBlockRequest(resp.blockhash); // Reset in-flight state in case Misbehaving does not result in a disconnect Misbehaving(pfrom.GetId(), 100, "invalid compact block/non-matching block transactions"); return; } else if (status == READ_STATUS_FAILED) { @@ -3561,7 +3561,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // though the block was successfully read, and rely on the // handling in ProcessNewBlock to ensure the block index is // updated, etc. - MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer + RemoveBlockRequest(resp.blockhash); // 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 @@ -3629,7 +3629,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // Always process the block if we requested it, since we may // need it even when it's not a candidate for a new best tip. forceProcessing = IsBlockRequested(hash); - MarkBlockAsReceived(hash); + RemoveBlockRequest(hash); // mapBlockSource is only used for punishing peers and setting // which peers send us compact blocks, so the race between here and // cs_main in ProcessNewBlock is fine. @@ -4779,7 +4779,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) for (const CBlockIndex *pindex : vToDownload) { uint32_t nFetchFlags = GetFetchFlags(*pto); vGetData.push_back(CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())); - MarkBlockAsInFlight(pto->GetId(), pindex); + BlockRequested(pto->GetId(), pindex); LogPrint(BCLog::NET, "Requesting block %s (%d) peer=%d\n", pindex->GetBlockHash().ToString(), pindex->nHeight, pto->GetId()); }