Merge #2925: [BUG] Fix more thread issues

f5ba264 fix another possible deadlock (ale)
f162f76 fix: fix data race and remove circular dependency (ale)
9207b8f fix data race: LOCK cs before accessing nodeState (ale)
58e5c8c fix: Solve possible deadlock (ale)

Pull request description:

  This PR fixes more thread issues that ThreadSanitizer showed:

  first commit:
  Solve a possible 3-threads deadlock fixed by locking `mempool.cs` before `cs_inventory` (see the comment for the log)

  second commit:
  solve the trivial data race where `nodeState` is accessed without locking the corresponding mutex

  third commit:
  solve all the issues that the function `getQuorumNodes` had:

  - `it` iterator was accessed without having the lock on `cs_vPendingMasternodes` which was causing a possible data race
  - `connman->vNodes`  was accessed without locking the mutex `cs_vNodes` (another possible data race)

   Solved by using the function `connman->ForEachNode` and removing the useless getter.

  fourth commit:
  Solve a possible deadlock: lock `cs_vPendingMasternodes` before calling `connmann->ForEachNode(...)`

ACKs for top commit: f5ba264
  Liquid369:
    tACK f5ba264
  Fuzzbawls:
    ACK f5ba264

Tree-SHA512: 2fa61ec6b3ad395cbe968874a90f1ec16eb5ddec85d48ddeb438fc92f4ae281d4ff7c5b297b147c4b3bf60d6a4849c0619567e3beccb930a76f61d0815bea1d5

src/llmq/quorums_signing_shares.cpp

@@ -526,6 +526,7 @@ void CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
 // It's ensured that no duplicates are passed to this method
 void CSigSharesManager::ProcessPendingSigSharesFromNode(NodeId nodeId, const std::vector<CSigShare>& sigShares, const std::map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr>& quorums, CConnman& connman)
 {
+    LOCK(cs);
     auto& nodeState = nodeStates[nodeId];
 
     cxxtimer::Timer t(true);

src/net.cpp

@@ -2463,11 +2463,6 @@ size_t CConnman::GetMaxOutboundNodeCount()
     return nMaxOutbound;
 }
 
-std::vector<CNode*> CConnman::GetvNodes()
-{
-    return vNodes;
-};
-
 size_t CConnman::GetNodeCount(NumConnections flags)
 {
     LOCK(cs_vNodes);

src/net.h

@@ -330,7 +330,6 @@ class CConnman
     bool RemoveAddedNode(const std::string& node);
     std::vector<AddedNodeInfo> GetAddedNodeInfo();
 
-    std::vector<CNode*> GetvNodes();
     size_t GetNodeCount(NumConnections num);
     size_t GetMaxOutboundNodeCount();
     void GetNodeStats(std::vector<CNodeStats>& vstats);

src/net_processing.cpp

@@ -2489,7 +2489,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic<bool>& interruptM
         std::vector<CInv> vInv;
         std::vector<CInv> vInvWait;
         {
-            LOCK(pto->cs_inventory);
+            LOCK2(mempool.cs, pto->cs_inventory);
             vInv.reserve(std::max<size_t>(pto->vInventoryBlockToSend.size() + pto->vInventoryTierTwoToSend.size(), INVENTORY_BROADCAST_MAX));
 
             // Add blocks

src/tiertwo/net_masternodes.cpp

@@ -7,7 +7,6 @@
 
 #include "chainparams.h"
 #include "evo/deterministicmns.h"
-#include "llmq/quorums.h"
 #include "netmessagemaker.h"
 #include "scheduler.h"
 #include "tiertwo/masternode_meta_manager.h" // for g_mmetaman
@@ -42,27 +41,21 @@ std::set<uint256> TierTwoConnMan::getQuorumNodes(Consensus::LLMQType llmqType)
 
 std::set<NodeId> TierTwoConnMan::getQuorumNodes(Consensus::LLMQType llmqType, uint256 quorumHash)
 {
+    LOCK(cs_vPendingMasternodes);
     std::set<NodeId> result;
-    auto it = WITH_LOCK(cs_vPendingMasternodes, return masternodeQuorumRelayMembers.find(std::make_pair(llmqType, quorumHash)));
-    if (WITH_LOCK(cs_vPendingMasternodes, return it == masternodeQuorumRelayMembers.end())) {
+    auto it = masternodeQuorumRelayMembers.find(std::make_pair(llmqType, quorumHash));
+    if (it == masternodeQuorumRelayMembers.end()) {
         return {};
     }
-    for (const auto pnode : connman->GetvNodes()) {
+    connman->ForEachNode([&](CNode* pnode) {
         if (pnode->fDisconnect) {
-            continue;
+            return;
         }
         if (!it->second.count(pnode->verifiedProRegTxHash)) {
-            continue;
-        }
-        // is it a valid member?
-        if (!llmq::quorumManager->GetQuorum(llmqType, quorumHash)) {
-            continue;
-        }
-        if (!llmq::quorumManager->GetQuorum(llmqType, quorumHash)->IsValidMember(pnode->verifiedProRegTxHash)) {
-            continue;
+            return;
         }
         result.emplace(pnode->GetId());
-    }
+    });
     return result;
 }
 
@@ -80,12 +73,10 @@ void TierTwoConnMan::removeQuorumNodes(Consensus::LLMQType llmqType, const uint2
 
 void TierTwoConnMan::setMasternodeQuorumRelayMembers(Consensus::LLMQType llmqType, const uint256& quorumHash, const std::set<uint256>& proTxHashes)
 {
-    {
-        LOCK(cs_vPendingMasternodes);
-        auto it = masternodeQuorumRelayMembers.emplace(std::make_pair(llmqType, quorumHash), proTxHashes);
-        if (!it.second) {
-            it.first->second = proTxHashes;
-        }
+    LOCK(cs_vPendingMasternodes);
+    auto it = masternodeQuorumRelayMembers.emplace(std::make_pair(llmqType, quorumHash), proTxHashes);
+    if (!it.second) {
+        it.first->second = proTxHashes;
     }
 
     // Update existing connections

test/lint/lint-circular-dependencies.sh

@@ -44,10 +44,8 @@ EXPECTED_CIRCULAR_DEPENDENCIES=(
     "llmq/quorums_dkgsession -> llmq/quorums_dkgsessionmgr -> llmq/quorums_dkgsessionhandler -> llmq/quorums_dkgsession"
     "llmq/quorums_dkgsessionhandler -> net_processing -> llmq/quorums_dkgsessionmgr -> llmq/quorums_dkgsessionhandler"
     "llmq/quorums_signing -> net_processing -> llmq/quorums_signing"
-    "llmq/quorums -> llmq/quorums_connections -> tiertwo/net_masternodes -> llmq/quorums"
     "llmq/quorums_chainlocks -> net_processing -> llmq/quorums_chainlocks"
     "llmq/quorums_chainlocks -> validation -> llmq/quorums_chainlocks"
-    "activemasternode -> tiertwo/net_masternodes -> llmq/quorums -> activemasternode"
     "chain -> legacy/stakemodifier -> validation -> validationinterface -> chain"
     "chain -> legacy/stakemodifier -> stakeinput -> txdb -> chain"
     "chain -> legacy/stakemodifier -> validation -> checkpoints -> chain"