Less cs_main locks in quorums (#2702)

* Drop cs_main from CQuorumManager::UpdatedBlockTip()

* CLLMQUtils::IsQuorumActive() shouldn't require cs_main to be held

* Revert comment deletion

src/llmq/quorums.cpp

@@ -166,8 +166,6 @@ void CQuorumManager::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockI
         return;
     }
 
-    LOCK(cs_main);
-
     for (auto& p : Params().GetConsensus().llmqs) {
         EnsureQuorumConnections(p.first, pindexNew);
     }
@@ -175,18 +173,16 @@ void CQuorumManager::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockI
 
 void CQuorumManager::EnsureQuorumConnections(Consensus::LLMQType llmqType, const CBlockIndex* pindexNew)
 {
-    AssertLockHeld(cs_main);
-
     const auto& params = Params().GetConsensus().llmqs.at(llmqType);
 
     auto myProTxHash = activeMasternodeInfo.proTxHash;
-    auto lastQuorums = ScanQuorums(llmqType, (size_t)params.keepOldConnections);
+    auto lastQuorums = ScanQuorums(llmqType, pindexNew, (size_t)params.keepOldConnections);
 
     auto connmanQuorumsToDelete = g_connman->GetMasternodeQuorums(llmqType);
 
     // don't remove connections for the currently in-progress DKG round
     int curDkgHeight = pindexNew->nHeight - (pindexNew->nHeight % params.dkgInterval);
-    auto curDkgBlock = chainActive[curDkgHeight]->GetBlockHash();
+    auto curDkgBlock = pindexNew->GetAncestor(curDkgHeight)->GetBlockHash();
     connmanQuorumsToDelete.erase(curDkgBlock);
 
     for (auto& quorum : lastQuorums) {
@@ -222,19 +218,14 @@ void CQuorumManager::EnsureQuorumConnections(Consensus::LLMQType llmqType, const
     }
 }
 
-bool CQuorumManager::BuildQuorumFromCommitment(const CFinalCommitment& qc, std::shared_ptr<CQuorum>& quorum) const
+bool CQuorumManager::BuildQuorumFromCommitment(const CFinalCommitment& qc, const CBlockIndex* pindexQuorum, std::shared_ptr<CQuorum>& quorum) const
 {
-    AssertLockHeld(cs_main);
+    assert(pindexQuorum);
+    assert(qc.quorumHash == pindexQuorum->GetBlockHash());
 
-    if (!mapBlockIndex.count(qc.quorumHash)) {
-        LogPrintf("CQuorumManager::%s -- block %s not found", __func__, qc.quorumHash.ToString());
-        return false;
-    }
-    auto& params = Params().GetConsensus().llmqs.at((Consensus::LLMQType)qc.llmqType);
-    auto quorumIndex = mapBlockIndex[qc.quorumHash];
     auto members = CLLMQUtils::GetAllQuorumMembers((Consensus::LLMQType)qc.llmqType, qc.quorumHash);
 
-    quorum->Init(qc.quorumHash, quorumIndex->nHeight, members, qc.validMembers, qc.quorumPublicKey);
+    quorum->Init(qc.quorumHash, pindexQuorum->nHeight, members, qc.validMembers, qc.quorumPublicKey);
 
     bool hasValidVvec = false;
     if (quorum->ReadContributions(evoDb)) {
@@ -300,12 +291,16 @@ bool CQuorumManager::HasQuorum(Consensus::LLMQType llmqType, const uint256& quor
 }
 
 std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqType, size_t maxCount)
+{
+    const CBlockIndex* pindex;
     {
         LOCK(cs_main);
-    return ScanQuorums(llmqType, chainActive.Tip()->GetBlockHash(), maxCount);
+        pindex = chainActive.Tip();
+    }
+    return ScanQuorums(llmqType, pindex, maxCount);
 }
 
-std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqType, const uint256& startBlock, size_t maxCount)
+std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqType, const CBlockIndex* pindexStart, size_t maxCount)
 {
     std::vector<CQuorumCPtr> result;
     result.reserve(maxCount);
@@ -318,19 +313,13 @@ std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp
         return result;
     }
 
-    LOCK(cs_main);
+    auto pindex = pindexStart->GetAncestor(pindexStart->nHeight - (pindexStart->nHeight % params.dkgInterval));
-    if (!mapBlockIndex.count(startBlock)) {
-        return result;
-    }
-
-    CBlockIndex* pindex = mapBlockIndex[startBlock];
-    pindex = chainActive[pindex->nHeight - (pindex->nHeight % params.dkgInterval)];
 
     while (pindex != nullptr
             && pindex->nHeight >= params.dkgInterval
             && result.size() < maxCount
             && deterministicMNManager->IsDIP3Enforced(pindex->nHeight)) {
-        auto quorum = GetQuorum(llmqType, pindex->GetBlockHash());
+        auto quorum = GetQuorum(llmqType, pindex);
         if (quorum) {
             result.emplace_back(quorum);
         }
@@ -348,7 +337,25 @@ std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqTyp
 
 CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash)
 {
-    AssertLockHeld(cs_main);
+    CBlockIndex* pindexQuorum;
+    {
+        LOCK(cs_main);
+        auto quorumIt = mapBlockIndex.find(quorumHash);
+
+        if (quorumIt == mapBlockIndex.end()) {
+            LogPrintf("CQuorumManager::%s -- block %s not found", __func__, quorumHash.ToString());
+            return nullptr;
+        }
+        pindexQuorum = quorumIt->second;
+    }
+    return GetQuorum(llmqType, pindexQuorum);
+}
+
+CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const CBlockIndex* pindexQuorum)
+{
+    assert(pindexQuorum);
+
+    auto quorumHash = pindexQuorum->GetBlockHash();
 
     // we must check this before we look into the cache. Reorgs might have happened which would mean we might have
     // cached quorums which are not in the active chain anymore
@@ -371,7 +378,8 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint25
     auto& params = Params().GetConsensus().llmqs.at(llmqType);
 
     auto quorum = std::make_shared<CQuorum>(params, blsWorker);
-    if (!BuildQuorumFromCommitment(qc, quorum)) {
+
+    if (!BuildQuorumFromCommitment(qc, pindexQuorum, quorum)) {
         return nullptr;
     }
 

src/llmq/quorums.h

@@ -91,20 +91,24 @@ public:
 
     void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload);
 
-public:
     bool HasQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash);
 
-    // all these methods will lock cs_main
+    // all these methods will lock cs_main for a short period of time
     CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash);
     CQuorumCPtr GetNewestQuorum(Consensus::LLMQType llmqType);
     std::vector<CQuorumCPtr> ScanQuorums(Consensus::LLMQType llmqType, size_t maxCount);
-    std::vector<CQuorumCPtr> ScanQuorums(Consensus::LLMQType llmqType, const uint256& startBlock, size_t maxCount);
+
+    // this one is cs_main-free
+    std::vector<CQuorumCPtr> ScanQuorums(Consensus::LLMQType llmqType, const CBlockIndex* pindexStart, size_t maxCount);
 
 private:
+    // all private methods here are cs_main-free
     void EnsureQuorumConnections(Consensus::LLMQType llmqType, const CBlockIndex *pindexNew);
 
-    bool BuildQuorumFromCommitment(const CFinalCommitment& qc, std::shared_ptr<CQuorum>& quorum) const;
+    bool BuildQuorumFromCommitment(const CFinalCommitment& qc, const CBlockIndex* pindexQuorum, std::shared_ptr<CQuorum>& quorum) const;
     bool BuildQuorumContributions(const CFinalCommitment& fqc, std::shared_ptr<CQuorum>& quorum) const;
+
+    CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const CBlockIndex* pindex);
 };
 
 extern CQuorumManager* quorumManager;

src/llmq/quorums_signing.cpp

@@ -259,11 +259,8 @@ bool CSigningManager::PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig&
         return false;
     }
 
-    CQuorumCPtr quorum;
+    CQuorumCPtr quorum = quorumManager->GetQuorum(llmqType, recoveredSig.quorumHash);
-    {
-        LOCK(cs_main);
 
-        quorum = quorumManager->GetQuorum(llmqType, recoveredSig.quorumHash);
     if (!quorum) {
         LogPrintf("CSigningManager::%s -- quorum %s not found, node=%d\n", __func__,
                   recoveredSig.quorumHash.ToString(), nodeId);
@@ -272,7 +269,6 @@ bool CSigningManager::PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig&
     if (!CLLMQUtils::IsQuorumActive(llmqType, quorum->quorumHash)) {
         return false;
     }
-    }
 
     if (!recoveredSig.sig.IsValid()) {
         retBan = true;
@@ -316,8 +312,6 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify(
         }
     }
 
-    {
-        LOCK(cs_main);
     for (auto& p : retSigShares) {
         NodeId nodeId = p.first;
         auto& v = p.second;
@@ -349,7 +343,6 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify(
         }
     }
 }
-}
 
 void CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman)
 {
@@ -561,17 +554,17 @@ CQuorumCPtr CSigningManager::SelectQuorumForSigning(Consensus::LLMQType llmqType
     auto& llmqParams = Params().GetConsensus().llmqs.at(llmqType);
     size_t poolSize = (size_t)llmqParams.signingActiveQuorumCount;
 
-    uint256 startBlock;
+    CBlockIndex* pindexStart;
     {
         LOCK(cs_main);
         int startBlockHeight = signHeight - SIGN_HEIGHT_OFFSET;
         if (startBlockHeight > chainActive.Height()) {
             return nullptr;
         }
-        startBlock = chainActive[startBlockHeight]->GetBlockHash();
+        pindexStart = chainActive[startBlockHeight];
     }
 
-    auto quorums = quorumManager->ScanQuorums(llmqType, startBlock, poolSize);
+    auto quorums = quorumManager->ScanQuorums(llmqType, pindexStart, poolSize);
     if (quorums.empty()) {
         return nullptr;
     }

src/llmq/quorums_signing_shares.cpp

@@ -347,11 +347,7 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(NodeId nodeId, const CBatchedS
         return false;
     }
 
-    CQuorumCPtr quorum;
+    CQuorumCPtr quorum = quorumManager->GetQuorum(llmqType, batchedSigShares.quorumHash);
-    {
-        LOCK(cs_main);
-
-        quorum = quorumManager->GetQuorum(llmqType, batchedSigShares.quorumHash);
     if (!quorum) {
         // TODO should we ban here?
         LogPrintf("CSigSharesManager::%s -- quorum %s not found, node=%d\n", __func__,
@@ -372,7 +368,6 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(NodeId nodeId, const CBatchedS
                   batchedSigShares.quorumHash.ToString(), nodeId);
         return false;
     }
-    }
 
     std::set<uint16_t> dupMembers;
 
@@ -994,9 +989,7 @@ void CSigSharesManager::Cleanup()
         }
     }
 
-    {
     // Find quorums which became inactive
-        LOCK(cs_main);
     for (auto it = quorumsToCheck.begin(); it != quorumsToCheck.end(); ) {
         if (CLLMQUtils::IsQuorumActive(it->first, it->second)) {
             it = quorumsToCheck.erase(it);
@@ -1004,7 +997,7 @@ void CSigSharesManager::Cleanup()
             ++it;
         }
     }
-    }
+
     {
         // Now delete sessions which are for inactive quorums
         LOCK(cs);

src/llmq/quorums_utils.cpp

@@ -99,8 +99,6 @@ std::set<size_t> CLLMQUtils::CalcDeterministicWatchConnections(Consensus::LLMQTy
 
 bool CLLMQUtils::IsQuorumActive(Consensus::LLMQType llmqType, const uint256& quorumHash)
 {
-    AssertLockHeld(cs_main);
-
     auto& params = Params().GetConsensus().llmqs.at(llmqType);
 
     // sig shares and recovered sigs are only accepted from recent/active quorums

src/rpc/rpcquorums.cpp

@@ -38,7 +38,7 @@ UniValue quorum_list(const JSONRPCRequest& request)
     for (auto& p : Params().GetConsensus().llmqs) {
         UniValue v(UniValue::VARR);
 
-        auto quorums = llmq::quorumManager->ScanQuorums(p.first, chainActive.Tip()->GetBlockHash(), count);
+        auto quorums = llmq::quorumManager->ScanQuorums(p.first, chainActive.Tip(), count);
         for (auto& q : quorums) {
             v.push_back(q->quorumHash.ToString());
         }