net processing: Move block inventory state to net_processing #19829
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -791,6 +791,11 @@ void PeerManager::FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) { | |
| LOCK(cs_main); | ||
| int misbehavior{0}; | ||
| { | ||
| // We remove the PeerRef from g_peer_map here, but we don't always | ||
| // destruct the Peer. Sometimes another thread is still holding a | ||
| // PeerRef, so the refcount is >= 1. Be careful not to do any | ||
| // processing here that assumes Peer won't be changed before it's | ||
| // destructed. | ||
| PeerRef peer = RemovePeer(nodeid); | ||
| assert(peer != nullptr); | ||
jnewbery marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| misbehavior = WITH_LOCK(peer->m_misbehavior_mutex, return peer->m_misbehavior_score); | ||
|
|
@@ -870,6 +875,7 @@ bool PeerManager::GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) { | |
| PeerRef peer = GetPeerRef(nodeid); | ||
| if (peer == nullptr) return false; | ||
| stats.m_misbehavior_score = WITH_LOCK(peer->m_misbehavior_mutex, return peer->m_misbehavior_score); | ||
| stats.m_starting_height = peer->m_starting_height; | ||
jnewbery marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return true; | ||
| } | ||
|
|
@@ -1309,13 +1315,17 @@ void PeerManager::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInde | |
| } | ||
| } | ||
|
|
||
| { | ||
| LOCK(m_peer_mutex); | ||
| for (auto& it : m_peer_map) { | ||
|
There was a problem hiding this comment. a50dee5 : Not sure if that matters much here. Could we prematurely send headers before There was a problem hiding this comment. This is an excellent observation. Block hashes could be pushed into bitcoin/src/net_processing.cpp Lines 4077 to 4078 in c434e2c I think this behaviour is ok. The version-verack handshake usually lasts no more than a second or two, and at the worst case times out after a minute, so we're never going to push many block hashes onto this vector. There was a problem hiding this comment. Any reason to not have a There was a problem hiding this comment. I originally had one but removed it because it didn't seem worth it. We can revisit that decision later if it seems useful. ForEachNode was useful because it allowed passing a lambda into CConnman which would be executed while the cs_vNodes lock was held. Here, the m_peer_mutex is in PeerMan, so it can be held directly. |
||
| Peer& peer = *it.second; | ||
| LOCK(peer.m_block_inv_mutex); | ||
|
There was a problem hiding this comment. I think this is the first place where we are acquiring a lock while already holding Or, would it be better to first copy all the Peer objects to a temporary place, release the There was a problem hiding this comment. Would a comment next to There was a problem hiding this comment. Added comments to the first commit. Let me know if you think more is required. |
||
| for (const uint256& hash : reverse_iterate(vHashes)) { | ||
| peer.m_blocks_for_headers_relay.push_back(hash); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| m_connman.WakeMessageHandler(); | ||
| } | ||
|
|
||
|
|
@@ -1465,7 +1475,7 @@ static void RelayAddress(const CNode& originator, | |
| connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc)); | ||
| } | ||
|
|
||
| void static ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChainParams& chainparams, const CInv& inv, CConnman& connman) | ||
| { | ||
| bool send = false; | ||
| std::shared_ptr<const CBlock> a_recent_block; | ||
|
|
@@ -1605,16 +1615,18 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c | |
| } | ||
| } | ||
|
|
||
| { | ||
| LOCK(peer.m_block_inv_mutex); | ||
| // Trigger the peer node to send a getblocks request for the next batch of inventory | ||
| if (inv.hash == peer.m_continuation_block) { | ||
| // Send immediately. This must send even if redundant, | ||
| // and we want it right after the last block so they don't | ||
| // wait for other stuff first. | ||
| std::vector<CInv> vInv; | ||
| vInv.push_back(CInv(MSG_BLOCK, ::ChainActive().Tip()->GetBlockHash())); | ||
| connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::INV, vInv)); | ||
| peer.m_continuation_block.SetNull(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1714,7 +1726,7 @@ void static ProcessGetData(CNode& pfrom, Peer& peer, const CChainParams& chainpa | |
| if (it != peer.m_getdata_requests.end() && !pfrom.fPauseSend) { | ||
| const CInv &inv = *it++; | ||
| if (inv.IsGenBlkMsg()) { | ||
| ProcessGetBlockData(pfrom, peer, chainparams, inv, connman); | ||
| } | ||
| // else: If the first item on the queue is an unknown type, we erase it | ||
| // and continue processing the queue on the next call. | ||
|
|
@@ -1764,7 +1776,9 @@ void PeerManager::SendBlockTransactions(CNode& pfrom, const CBlock& block, const | |
| m_connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp)); | ||
| } | ||
|
|
||
| void PeerManager::ProcessHeadersMessage(CNode& pfrom, const Peer& peer, | ||
| const std::vector<CBlockHeader>& headers, | ||
| bool via_compact_block) | ||
jnewbery marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| const CNetMsgMaker msgMaker(pfrom.GetCommonVersion()); | ||
| size_t nCount = headers.size(); | ||
|
|
@@ -1854,7 +1868,8 @@ void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector<CBlockHe | |
| // Headers message had its maximum size; the peer may have more headers. | ||
| // TODO: optimize: if pindexLast is an ancestor of ::ChainActive().Tip or pindexBestHeader, continue | ||
| // from there instead. | ||
| LogPrint(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n", | ||
| pindexLast->nHeight, pfrom.GetId(), peer.m_starting_height); | ||
jnewbery marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexLast), uint256())); | ||
| } | ||
|
|
||
|
|
@@ -2280,7 +2295,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat | |
| ServiceFlags nServices; | ||
| int nVersion; | ||
| std::string cleanSubVer; | ||
| int starting_height = -1; | ||
| bool fRelay = true; | ||
|
|
||
| vRecv >> nVersion >> nServiceInt >> nTime >> addrMe; | ||
|
|
@@ -2311,7 +2326,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat | |
| cleanSubVer = SanitizeString(strSubVer); | ||
| } | ||
| if (!vRecv.empty()) { | ||
| vRecv >> starting_height; | ||
| } | ||
| if (!vRecv.empty()) | ||
| vRecv >> fRelay; | ||
|
|
@@ -2360,7 +2375,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat | |
| LOCK(pfrom.cs_SubVer); | ||
| pfrom.cleanSubVer = cleanSubVer; | ||
| } | ||
| peer->m_starting_height = starting_height; | ||
|
|
||
| // set nodes not relaying blocks and tx and not serving (parts) of the historical blockchain as "clients" | ||
| pfrom.fClient = (!(nServices & NODE_NETWORK) && !(nServices & NODE_NETWORK_LIMITED)); | ||
|
|
@@ -2440,7 +2455,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat | |
|
|
||
| LogPrint(BCLog::NET, "receive version message: %s: version %d, blocks=%d, us=%s, peer=%d%s\n", | ||
| cleanSubVer, pfrom.nVersion, | ||
| peer->m_starting_height, addrMe.ToString(), pfrom.GetId(), | ||
| remoteAddr); | ||
|
|
||
| int64_t nTimeOffset = nTime - GetTime(); | ||
|
|
@@ -2474,7 +2489,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat | |
|
|
||
| if (!pfrom.IsInboundConn()) { | ||
| LogPrintf("New outbound peer connected: version: %d, blocks=%d, peer=%d%s (%s)\n", | ||
| pfrom.nVersion.load(), peer->m_starting_height, | ||
| pfrom.GetId(), (fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToString()) : ""), | ||
| pfrom.ConnectionTypeAsString()); | ||
| } | ||
|
|
@@ -2786,13 +2801,12 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat | |
| LogPrint(BCLog::NET, " getblocks stopping, pruned or too old block at %d %s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); | ||
| break; | ||
| } | ||
| WITH_LOCK(peer->m_block_inv_mutex, peer->m_blocks_for_inv_relay.push_back(pindex->GetBlockHash())); | ||
| if (--nLimit <= 0) { | ||
| // When this block is requested, we'll send an inv that'll | ||
| // trigger the peer to getblocks the next batch of inventory. | ||
| LogPrint(BCLog::NET, " getblocks stopping at limit %d %s\n", pindex->nHeight, pindex->GetBlockHash().ToString()); | ||
| WITH_LOCK(peer->m_block_inv_mutex, {peer->m_continuation_block = pindex->GetBlockHash();}); | ||
| break; | ||
| } | ||
| } | ||
|
|
@@ -3316,7 +3330,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat | |
| // the peer if the header turns out to be for an invalid block. | ||
| // Note that if a peer tries to build on an invalid chain, that | ||
| // will be detected and the peer will be disconnected/discouraged. | ||
| return ProcessHeadersMessage(pfrom, *peer, {cmpctblock.header}, /*via_compact_block=*/true); | ||
| } | ||
|
|
||
| if (fBlockReconstructed) { | ||
|
|
@@ -3459,7 +3473,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat | |
| ReadCompactSize(vRecv); // ignore tx count; assume it is 0. | ||
| } | ||
|
|
||
| return ProcessHeadersMessage(pfrom, *peer, headers, /*via_compact_block=*/false); | ||
| } | ||
|
|
||
| if (msg_type == NetMsgType::BLOCK) | ||
|
|
@@ -4067,6 +4081,7 @@ class CompareInvMempoolOrder | |
|
|
||
| bool PeerManager::SendMessages(CNode* pto) | ||
| { | ||
| PeerRef peer = GetPeerRef(pto->GetId()); | ||
| const Consensus::Params& consensusParams = m_chainparams.GetConsensus(); | ||
|
|
||
| // We must call MaybeDiscourageAndDisconnect first, to ensure that we'll | ||
|
|
@@ -4192,7 +4207,7 @@ bool PeerManager::SendMessages(CNode* pto) | |
| got back an empty response. */ | ||
| if (pindexStart->pprev) | ||
| pindexStart = pindexStart->pprev; | ||
| LogPrint(BCLog::NET, "initial getheaders (%d) to peer=%d (startheight:%d)\n", pindexStart->nHeight, pto->GetId(), peer->m_starting_height); | ||
| m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexStart), uint256())); | ||
| } | ||
| } | ||
|
|
@@ -4208,11 +4223,11 @@ bool PeerManager::SendMessages(CNode* pto) | |
| // If no header would connect, or if we have too many | ||
| // blocks, or if the peer doesn't want headers, just | ||
| // add all to the inv queue. | ||
| LOCK(peer->m_block_inv_mutex); | ||
| std::vector<CBlock> vHeaders; | ||
| bool fRevertToInv = ((!state.fPreferHeaders && | ||
| (!state.fPreferHeaderAndIDs || peer->m_blocks_for_headers_relay.size() > 1)) || | ||
| peer->m_blocks_for_headers_relay.size() > MAX_BLOCKS_TO_ANNOUNCE); | ||
| const CBlockIndex *pBestIndex = nullptr; // last header queued for delivery | ||
| ProcessBlockAvailability(pto->GetId()); // ensure pindexBestKnownBlock is up-to-date | ||
|
|
||
|
|
@@ -4221,7 +4236,7 @@ bool PeerManager::SendMessages(CNode* pto) | |
| // Try to find first header that our peer doesn't have, and | ||
| // then send all headers past that one. If we come across any | ||
| // headers that aren't on ::ChainActive(), give up. | ||
| for (const uint256& hash : peer->m_blocks_for_headers_relay) { | ||
| const CBlockIndex* pindex = LookupBlockIndex(hash); | ||
| assert(pindex); | ||
| if (::ChainActive()[pindex->nHeight] != pindex) { | ||
|
|
@@ -4238,7 +4253,7 @@ bool PeerManager::SendMessages(CNode* pto) | |
| // which should be caught by the prior check), but one | ||
| // way this could happen is by using invalidateblock / | ||
| // reconsiderblock repeatedly on the tip, causing it to | ||
| // be added multiple times to m_blocks_for_headers_relay. | ||
| // Robustly deal with this rare situation by reverting | ||
| // to an inv. | ||
| fRevertToInv = true; | ||
|
|
@@ -4310,10 +4325,10 @@ bool PeerManager::SendMessages(CNode* pto) | |
| } | ||
| if (fRevertToInv) { | ||
| // If falling back to using an inv, just try to inv the tip. | ||
| // The last entry in m_blocks_for_headers_relay was our tip at some point | ||
| // in the past. | ||
| if (!peer->m_blocks_for_headers_relay.empty()) { | ||
| const uint256& hashToAnnounce = peer->m_blocks_for_headers_relay.back(); | ||
| const CBlockIndex* pindex = LookupBlockIndex(hashToAnnounce); | ||
| assert(pindex); | ||
|
|
||
|
|
@@ -4327,32 +4342,32 @@ bool PeerManager::SendMessages(CNode* pto) | |
|
|
||
| // If the peer's chain has this block, don't inv it back. | ||
| if (!PeerHasHeader(&state, pindex)) { | ||
| peer->m_blocks_for_inv_relay.push_back(hashToAnnounce); | ||
| LogPrint(BCLog::NET, "%s: sending inv peer=%d hash=%s\n", __func__, | ||
| pto->GetId(), hashToAnnounce.ToString()); | ||
| } | ||
| } | ||
| } | ||
| peer->m_blocks_for_headers_relay.clear(); | ||
| } | ||
|
|
||
| // | ||
| // Message: inventory | ||
| // | ||
| std::vector<CInv> vInv; | ||
| { | ||
| LOCK(peer->m_block_inv_mutex); | ||
| vInv.reserve(std::max<size_t>(peer->m_blocks_for_inv_relay.size(), INVENTORY_BROADCAST_MAX)); | ||
|
|
||
| // Add blocks | ||
| for (const uint256& hash : peer->m_blocks_for_inv_relay) { | ||
| vInv.push_back(CInv(MSG_BLOCK, hash)); | ||
| if (vInv.size() == MAX_INV_SZ) { | ||
| m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); | ||
| vInv.clear(); | ||
| } | ||
| } | ||
| peer->m_blocks_for_inv_relay.clear(); | ||
|
|
||
| if (pto->m_tx_relay != nullptr) { | ||
| LOCK(pto->m_tx_relay->cs_tx_inventory); | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.