Do not hold cs_vNodes in CSigSharesManager::SendMessages() for too long (#2758)

UdjinM6 · panleone · commit ed463a64b251 · 2024-11-05T21:46:23.000+01:00
diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp
@@ -1048,7 +1048,9 @@ bool CSigSharesManager::SendMessages()
 
     bool didSend = false;
 
-    g_connman->ForEachNode([&](CNode* pnode) {
+    std::vector<CNode*> vNodesCopy = g_connman->CopyNodeVector(CConnman::FullyConnectedOnly);
+
+    for (auto& pnode : vNodesCopy) {
         CNetMsgMaker msgMaker(pnode->GetSendVersion());
 
         auto it1 = sigSessionAnnouncements.find(pnode->GetId());
@@ -1133,9 +1135,10 @@ bool CSigSharesManager::SendMessages()
                 didSend = true;
             }
         }
+    }
 
-        return true;
-    });
+    // looped through all nodes, release them
+    g_connman->ReleaseNodeVector(vNodesCopy);
 
     return didSend;
 }
diff --git a/src/net.cpp b/src/net.cpp
@@ -2791,6 +2791,33 @@ int64_t PoissonNextSend(int64_t nNow, int average_interval_seconds) {
     return nNow + (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * average_interval_seconds * -1000000.0 + 0.5);
 }
 
+std::vector<CNode*> CConnman::CopyNodeVector(std::function<bool(const CNode* pnode)> cond)
+{
+    std::vector<CNode*> vecNodesCopy;
+    LOCK(cs_vNodes);
+    for (size_t i = 0; i < vNodes.size(); ++i) {
+        CNode* pnode = vNodes[i];
+        if (!cond(pnode))
+            continue;
+        pnode->AddRef();
+        vecNodesCopy.push_back(pnode);
+    }
+    return vecNodesCopy;
+}
+
+std::vector<CNode*> CConnman::CopyNodeVector()
+{
+    return CopyNodeVector(AllNodes);
+}
+
+void CConnman::ReleaseNodeVector(const std::vector<CNode*>& vecNodes)
+{
+    for (size_t i = 0; i < vecNodes.size(); ++i) {
+        CNode* pnode = vecNodes[i];
+        pnode->Release();
+    }
+}
+
 CSipHasher CConnman::GetDeterministicRandomizer(uint64_t id)
 {
     return CSipHasher(nSeed0, nSeed1).Write(id);
diff --git a/src/net.h b/src/net.h
@@ -280,6 +280,10 @@ class CConnman
         post();
     };
 
+    std::vector<CNode*> CopyNodeVector(std::function<bool(const CNode* pnode)> cond);
+    std::vector<CNode*> CopyNodeVector();
+    void ReleaseNodeVector(const std::vector<CNode*>& vecNodes);
+
     // Clears AskFor requests for every known peer
     void RemoveAskFor(const uint256& invHash, int invType);
 

Original file line number	Diff line number	Diff line change
`@@ -1048,7 +1048,9 @@ bool CSigSharesManager::SendMessages()`
`1048`	`1048`
`1049`	`1049`	`bool didSend = false;`
`1050`	`1050`
`1051`		`- g_connman->ForEachNode([&](CNode* pnode) {`
	`1051`	`+ std::vector<CNode*> vNodesCopy = g_connman->CopyNodeVector(CConnman::FullyConnectedOnly);`
	`1052`	`+`
	`1053`	`+ for (auto& pnode : vNodesCopy) {`
`1052`	`1054`	`CNetMsgMaker msgMaker(pnode->GetSendVersion());`
`1053`	`1055`
`1054`	`1056`	`auto it1 = sigSessionAnnouncements.find(pnode->GetId());`
`@@ -1133,9 +1135,10 @@ bool CSigSharesManager::SendMessages()`
`1133`	`1135`	`didSend = true;`
`1134`	`1136`	`}`
`1135`	`1137`	`}`
	`1138`	`+ }`
`1136`	`1139`
`1137`		`- return true;`
`1138`		`- });`
	`1140`	`+ // looped through all nodes, release them`
	`1141`	`+ g_connman->ReleaseNodeVector(vNodesCopy);`
`1139`	`1142`
`1140`	`1143`	`return didSend;`
`1141`	`1144`	`}`