merge bitcoin#21943: Dedup and RAII-fy the creation of a copy of CConnman::vNodes

Author: kwvg

src/governance/governance.cpp

@@ -1223,11 +1223,11 @@ void CGovernanceManager::RequestGovernanceObject(CNode* pfrom, const uint256& nH
 
 int CGovernanceManager::RequestGovernanceObjectVotes(CNode& peer, CConnman& connman) const
 {
-    std::array<CNode*, 1> nodeCopy{&peer};
-    return RequestGovernanceObjectVotes(nodeCopy, connman);
+    const std::vector<CNode*> vNodeCopy{&peer};
+    return RequestGovernanceObjectVotes(vNodeCopy, connman);
 }
 
-int CGovernanceManager::RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CConnman& connman) const
+int CGovernanceManager::RequestGovernanceObjectVotes(const std::vector<CNode*>& vNodesCopy, CConnman& connman) const
 {
     static std::map<uint256, std::map<CService, int64_t> > mapAskedRecently;
 
@@ -1501,7 +1501,7 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex, CConnman& co
 
 void CGovernanceManager::RequestOrphanObjects(CConnman& connman)
 {
-    std::vector<CNode*> vNodesCopy = connman.CopyNodeVector(CConnman::FullyConnectedOnly);
+    const CConnman::NodesSnapshot snap{connman, /* filter = */ CConnman::FullyConnectedOnly};
 
     std::vector<uint256> vecHashesFiltered;
     {
@@ -1517,15 +1517,13 @@ void CGovernanceManager::RequestOrphanObjects(CConnman& connman)
 
     LogPrint(BCLog::GOBJECT, "CGovernanceObject::RequestOrphanObjects -- number objects = %d\n", vecHashesFiltered.size());
     for (const uint256& nHash : vecHashesFiltered) {
-        for (CNode* pnode : vNodesCopy) {
+        for (CNode* pnode : snap.Nodes()) {
             if (!pnode->CanRelay()) {
                 continue;
             }
             RequestGovernanceObject(pnode, nHash, connman);
         }
     }
-
-    connman.ReleaseNodeVector(vNodesCopy);
 }
 
 void CGovernanceManager::CleanOrphanObjects()

src/governance/governance.h

@@ -358,7 +358,7 @@ class CGovernanceManager : public GovernanceStore
     void InitOnLoad();
 
     int RequestGovernanceObjectVotes(CNode& peer, CConnman& connman) const;
-    int RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CConnman& connman) const;
+    int RequestGovernanceObjectVotes(const std::vector<CNode*>& vNodesCopy, CConnman& connman) const;
 
     /*
      * Trigger Management (formerly CGovernanceTriggerManager)

src/llmq/signing_shares.cpp

@@ -1092,14 +1092,13 @@ bool CSigSharesManager::SendMessages()
         return session->sendSessionId;
     };
 
-    std::vector<CNode*> vNodesCopy = connman.CopyNodeVector(CConnman::FullyConnectedOnly);
-
+    const CConnman::NodesSnapshot snap{connman, /* filter = */ CConnman::FullyConnectedOnly};
     {
         LOCK(cs);
         CollectSigSharesToRequest(sigSharesToRequest);
         CollectSigSharesToSend(sigShareBatchesToSend);
         CollectSigSharesToAnnounce(sigSharesToAnnounce);
-        CollectSigSharesToSendConcentrated(sigSharesToSend, vNodesCopy);
+        CollectSigSharesToSendConcentrated(sigSharesToSend, snap.Nodes());
 
         for (auto& [nodeId, sigShareMap] : sigSharesToRequest) {
             for (auto& [hash, sigShareInv] : sigShareMap) {
@@ -1120,7 +1119,7 @@ bool CSigSharesManager::SendMessages()
 
     bool didSend = false;
 
-    for (auto& pnode : vNodesCopy) {
+    for (auto& pnode : snap.Nodes()) {
         CNetMsgMaker msgMaker(pnode->GetCommonVersion());
 
         if (const auto it1 = sigSessionAnnouncements.find(pnode->GetId()); it1 != sigSessionAnnouncements.end()) {
@@ -1222,9 +1221,6 @@ bool CSigSharesManager::SendMessages()
         }
     }
 
-    // looped through all nodes, release them
-    connman.ReleaseNodeVector(vNodesCopy);
-
     return didSend;
 }
 

src/masternode/sync.cpp

@@ -140,12 +140,11 @@ void CMasternodeSync::ProcessTick()
     }
 
     nTimeLastProcess = GetTime();
-    std::vector<CNode*> vNodesCopy = connman.CopyNodeVector(CConnman::FullyConnectedOnly);
+    const CConnman::NodesSnapshot snap{connman, /* filter = */ CConnman::FullyConnectedOnly};
 
     // gradually request the rest of the votes after sync finished
     if(IsSynced()) {
-        m_govman.RequestGovernanceObjectVotes(vNodesCopy, connman);
-        connman.ReleaseNodeVector(vNodesCopy);
+        m_govman.RequestGovernanceObjectVotes(snap.Nodes(), connman);
         return;
     }
 
@@ -154,7 +153,7 @@ void CMasternodeSync::ProcessTick()
     LogPrint(BCLog::MNSYNC, "CMasternodeSync::ProcessTick -- nTick %d nCurrentAsset %d nTriedPeerCount %d nSyncProgress %f\n", nTick, nCurrentAsset, nTriedPeerCount, nSyncProgress);
     uiInterface.NotifyAdditionalDataSyncProgressChanged(nSyncProgress);
 
-    for (auto& pnode : vNodesCopy)
+    for (auto& pnode : snap.Nodes())
     {
         CNetMsgMaker msgMaker(pnode->GetCommonVersion());
 
@@ -189,7 +188,7 @@ void CMasternodeSync::ProcessTick()
             }
 
             if (nCurrentAsset == MASTERNODE_SYNC_BLOCKCHAIN) {
-                int64_t nTimeSyncTimeout = vNodesCopy.size() > 3 ? MASTERNODE_SYNC_TICK_SECONDS : MASTERNODE_SYNC_TIMEOUT_SECONDS;
+                int64_t nTimeSyncTimeout = snap.Nodes().size() > 3 ? MASTERNODE_SYNC_TICK_SECONDS : MASTERNODE_SYNC_TIMEOUT_SECONDS;
                 if (fReachedBestHeader && (GetTime() - nTimeLastBumped > nTimeSyncTimeout)) {
                     // At this point we know that:
                     // a) there are peers (because we are looping on at least one of them);
@@ -205,7 +204,7 @@ void CMasternodeSync::ProcessTick()
 
                     if (gArgs.GetBoolArg("-syncmempool", DEFAULT_SYNC_MEMPOOL)) {
                         // Now that the blockchain is synced request the mempool from the connected outbound nodes if possible
-                        for (auto pNodeTmp : vNodesCopy) {
+                        for (auto pNodeTmp : snap.Nodes()) {
                             bool fRequestedEarlier = m_netfulfilledman.HasFulfilledRequest(pNodeTmp->addr, "mempool-sync");
                             if (pNodeTmp->nVersion >= 70216 && !pNodeTmp->IsInboundConn() && !fRequestedEarlier && !pNodeTmp->IsBlockRelayOnly()) {
                                 m_netfulfilledman.AddFulfilledRequest(pNodeTmp->addr, "mempool-sync");
@@ -222,7 +221,6 @@ void CMasternodeSync::ProcessTick()
             if(nCurrentAsset == MASTERNODE_SYNC_GOVERNANCE) {
                 if (!m_govman.IsValid()) {
                     SwitchToNextAsset();
-                    connman.ReleaseNodeVector(vNodesCopy);
                     return;
                 }
                 LogPrint(BCLog::GOBJECT, "CMasternodeSync::ProcessTick -- nTick %d nCurrentAsset %d nTimeLastBumped %lld GetTime() %lld diff %lld\n", nTick, nCurrentAsset, nTimeLastBumped, GetTime(), GetTime() - nTimeLastBumped);
@@ -235,7 +233,6 @@ void CMasternodeSync::ProcessTick()
                         // it's kind of ok to skip this for now, hopefully we'll catch up later?
                     }
                     SwitchToNextAsset();
-                    connman.ReleaseNodeVector(vNodesCopy);
                     return;
                 }
 
@@ -259,12 +256,11 @@ void CMasternodeSync::ProcessTick()
 
     if (nCurrentAsset != MASTERNODE_SYNC_GOVERNANCE) {
         // looped through all nodes and not syncing governance yet/already, release them
-        connman.ReleaseNodeVector(vNodesCopy);
         return;
     }
 
     // request votes on per-obj basis from each node
-    for (const auto& pnode : vNodesCopy) {
+    for (const auto& pnode : snap.Nodes()) {
         if(!m_netfulfilledman.HasFulfilledRequest(pnode->addr, "governance-sync")) {
             continue; // to early for this node
         }
@@ -291,16 +287,12 @@ void CMasternodeSync::ProcessTick()
                 // reset nTimeNoObjectsLeft to be able to use the same condition on resync
                 nTimeNoObjectsLeft = 0;
                 SwitchToNextAsset();
-                connman.ReleaseNodeVector(vNodesCopy);
                 return;
             }
             nLastTick = nTick;
             nLastVotes = m_govman.GetVoteCount();
         }
     }
-
-    // looped through all nodes, release them
-    connman.ReleaseNodeVector(vNodesCopy);
 }
 
 void CMasternodeSync::SendGovernanceSyncRequest(CNode* pnode) const