Merge #2882: [DMNs] Quorums optimizations

0d320e5 Speed up ScanQuorums (Alessandro Rezzi)
6dad072 Don't join the quorum thread if called internally (Alessandro Rezzi)
8063624 Remove useless lock of cs_main (Alessandro Rezzi)
e79bec7 Refactor: Clean up code (Alessandro Rezzi)

Pull request description:

  Cleanup and various optimizations for the quorum class in particular:
  - Cleanup unused code/ function parameters
  - Don't lock cs_main when it is not needed
  - Huge speed up on the ScanQuorums function

ACKs for top commit: 0d320e5
  Liquid369:
    tACK 0d320e5
  Fuzzbawls:
    ACK 0d320e5

Tree-SHA512: d6183ea31f9461f7e63b4e7dfdc4963dd0eb92ee0a1b21b8a3459c77161b277a10a605fd2a88d7f55ef720c17f01d5062634980068be80ba0cd5a42fd2cb5ded

src/evo/evonotificationinterface.cpp

@@ -27,7 +27,7 @@ void EvoNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con
     // background thread updates
     llmq::chainLocksHandler->UpdatedBlockTip(pindexNew, pindexFork);
     llmq::quorumDKGSessionManager->UpdatedBlockTip(pindexNew, fInitialDownload);
-    llmq::quorumManager->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload);
+    llmq::quorumManager->UpdatedBlockTip(pindexNew, fInitialDownload);
 }
 
 void EvoNotificationInterface::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& oldMNList, const CDeterministicMNListDiff& diff)

src/llmq/quorums.cpp

@@ -43,7 +43,9 @@ CQuorum::~CQuorum()
 {
     // most likely the thread is already done
     stopCachePopulatorThread = true;
-    if (cachePopulatorThread.joinable()) {
+    // watch out to not join the thread when we're called from inside the thread, which might happen on shutdown. This
+    // is because on shutdown the thread is the last owner of the shared CQuorum instance and thus the destroyer of it.
+    if (cachePopulatorThread.joinable() && cachePopulatorThread.get_id() != std::this_thread::get_id()) {
         cachePopulatorThread.join();
     }
 }
@@ -158,18 +160,16 @@ CQuorumManager::CQuorumManager(CEvoDB& _evoDb, CBLSWorker& _blsWorker, CDKGSessi
 {
 }
 
-void CQuorumManager::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload)
+void CQuorumManager::UpdatedBlockTip(const CBlockIndex* pindexNew, bool fInitialDownload)
 {
     if (fInitialDownload || !activeMasternodeManager || !deterministicMNManager->IsDIP3Enforced(pindexNew->nHeight)) {
         return;
     }
 
-    LOCK(cs_main);
-
     for (auto& p : Params().GetConsensus().llmqs) {
         const auto& params = Params().GetConsensus().llmqs.at(p.first);
 
-        auto lastQuorums = ScanQuorums(p.first, (size_t)params.keepOldConnections);
+        auto lastQuorums = ScanQuorums(p.first, pindexNew, (size_t)params.keepOldConnections);
 
         llmq::EnsureLatestQuorumConnections(p.first, pindexNew, activeMasternodeManager->GetProTx(), lastQuorums);
     }
@@ -178,14 +178,9 @@ void CQuorumManager::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockI
 
 bool CQuorumManager::BuildQuorumFromCommitment(const CFinalCommitment& qc, const CBlockIndex* pindexQuorum, const uint256& minedBlockHash, 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 = deterministicMNManager->GetAllQuorumMembers((Consensus::LLMQType)qc.llmqType, pindexQuorum);
     quorum->Init(minedBlockHash, pindexQuorum, members, qc.validMembers, qc.quorumPublicKey);
 
@@ -197,7 +192,7 @@ bool CQuorumManager::BuildQuorumFromCommitment(const CFinalCommitment& qc, const
             quorum->WriteContributions(evoDb);
             hasValidVvec = true;
         } else {
-            LogPrintf("CQuorumManager::%s -- quorum.ReadContributions and BuildQuorumContributions for block %s failed", __func__, qc.quorumHash.ToString());
+            LogPrintf("CQuorumManager::%s -- quorum.ReadContributions and BuildQuorumContributions for block %s failed\n", __func__, qc.quorumHash.ToString());
         }
     }
 
@@ -254,33 +249,22 @@ bool CQuorumManager::HasQuorum(Consensus::LLMQType llmqType, const uint256& quor
 
 std::vector<CQuorumCPtr> CQuorumManager::ScanQuorums(Consensus::LLMQType llmqType, size_t maxCount)
 {
-    LOCK(cs_main);
-    return ScanQuorums(llmqType, chainActive.Tip()->GetBlockHash(), maxCount);
+    const CBlockIndex* pindex = WITH_LOCK(cs_main, return 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)
 {
+    // TODO: speed up even more by using a cache
+    auto& params = Params().GetConsensus().llmqs.at(llmqType);
+    auto quorumIndexes = quorumBlockProcessor->GetMinedCommitmentsUntilBlock(params.type, pindexStart, maxCount);
     std::vector<CQuorumCPtr> result;
-
-    LOCK(cs_main);
-    if (!mapBlockIndex.count(startBlock)) {
-        return result;
-    }
-
-    result.reserve(maxCount);
-
-    CBlockIndex* pindex = mapBlockIndex[startBlock];
-
-    while (pindex != NULL && result.size() < maxCount && deterministicMNManager->IsDIP3Enforced(pindex->nHeight)) {
-        if (HasQuorum(llmqType, pindex->GetBlockHash())) {
-            auto quorum = GetQuorum(llmqType, pindex->GetBlockHash());
-            if (quorum) {
-                result.emplace_back(quorum);
-            }
-        }
-
-        // TODO speedup (skip blocks where no quorums could have been mined)
-        pindex = pindex->pprev;
+    result.reserve(quorumIndexes.size());
+    for (auto& quorumIndex : quorumIndexes) {
+        assert(quorumIndex);
+        auto quorum = GetQuorum(params.type, quorumIndex);
+        assert(quorum != nullptr);
+        result.emplace_back(quorum);
     }
 
     return result;
@@ -291,13 +275,12 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const uint25
     CBlockIndex* pindexQuorum;
     {
         LOCK(cs_main);
-        auto quorumIt = mapBlockIndex.find(quorumHash);
+        pindexQuorum = LookupBlockIndex(quorumHash);
 
-        if (quorumIt == mapBlockIndex.end()) {
+        if (!pindexQuorum) {
             LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- block %s not found", __func__, quorumHash.ToString());
             return nullptr;
         }
-        pindexQuorum = quorumIt->second;
     }
     return GetQuorum(llmqType, pindexQuorum);
 }
@@ -338,13 +321,4 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, const CBlock
     return quorum;
 }
 
-CQuorumCPtr CQuorumManager::GetNewestQuorum(Consensus::LLMQType llmqType)
-{
-    auto quorums = ScanQuorums(llmqType, 1);
-    if (quorums.empty()) {
-        return nullptr;
-    }
-    return quorums.front();
-}
-
 } // namespace llmq

src/llmq/quorums.h

@@ -89,24 +89,27 @@ class CQuorumManager
 public:
     CQuorumManager(CEvoDB& _evoDb, CBLSWorker& _blsWorker, CDKGSessionManager& _dkgManager);
 
-    void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload);
+    void UpdatedBlockTip(const CBlockIndex* pindexNew, bool fInitialDownload);
 
 public:
     bool HasQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash);
 
     // all these methods will lock cs_main
     CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const uint256& quorumHash);
-    CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const CBlockIndex* pindexQuorum);
 
-    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 method will not lock cs_main
+    std::vector<CQuorumCPtr> ScanQuorums(Consensus::LLMQType llmqType, const CBlockIndex* pindexStart, size_t maxCount);
 
 private:
+    // all these methods will not lock cs_main
     void EnsureQuorumConnections(Consensus::LLMQType llmqType, const CBlockIndex* pindexNew);
 
     bool BuildQuorumFromCommitment(const CFinalCommitment& qc, const CBlockIndex* pindexQuorum, const uint256& minedBlockHash, std::shared_ptr<CQuorum>& quorum) const;
     bool BuildQuorumContributions(const CFinalCommitment& fqc, std::shared_ptr<CQuorum>& quorum) const;
+
+    CQuorumCPtr GetQuorum(Consensus::LLMQType llmqType, const CBlockIndex* pindexQuorum);
 };
 
 extern std::unique_ptr<CQuorumManager> quorumManager;

src/llmq/quorums_connections.cpp

@@ -109,16 +109,14 @@ std::set<size_t> CalcDeterministicWatchConnections(Consensus::LLMQType llmqType,
 // ensure connection to a given list of quorums
 void EnsureLatestQuorumConnections(Consensus::LLMQType llmqType, const CBlockIndex* pindexNew, const uint256& myProTxHash, std::vector<CQuorumCPtr>& lastQuorums)
 {
-    AssertLockHeld(cs_main);
-
     const auto& params = Params().GetConsensus().llmqs.at(llmqType);
     auto connman = g_connman->GetTierTwoConnMan();
 
     auto connmanQuorumsToDelete = connman->getQuorumNodes(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) {

src/llmq/quorums_signing.cpp

@@ -550,16 +550,16 @@ 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);
         if (signHeight > chainActive.Height()) {
             return nullptr;
         }
-        startBlock = chainActive[signHeight - SIGN_HEIGHT_OFFSET]->GetBlockHash();
+        pindexStart = chainActive[signHeight - SIGN_HEIGHT_OFFSET];
     }
 
-    auto quorums = quorumManager->ScanQuorums(llmqType, startBlock, poolSize);
+    auto quorums = quorumManager->ScanQuorums(llmqType, pindexStart, poolSize);
     if (quorums.empty()) {
         return nullptr;
     }

src/llmq/quorums_utils.cpp

@@ -50,7 +50,6 @@ std::string ToHexStr(const std::vector<bool>& vBits)
 
 bool IsQuorumActive(Consensus::LLMQType llmqType, const uint256& quorumHash)
 {
-    AssertLockHeld(cs_main);
 
     auto& params = Params().GetConsensus().llmqs.at(llmqType);
 

src/rpc/rpcquorums.cpp

@@ -138,7 +138,7 @@ UniValue listquorums(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->pindexQuorum->GetBlockHash().ToString());
         }

test/functional/tiertwo_chainlocks.py

@@ -35,6 +35,11 @@ def run_test(self):
         (qfc, badmembers) = self.mine_quorum()
         assert_equal(171, miner.getblockcount())
 
+        # in order to avoid sync issues signing sessions quorum selection looks for quorums mined at most at chaintip - 8 blocks.
+        # let's round it and mine 10 blocks
+        miner.generate(10)
+        self.sync_all()
+
         # mine single block, wait for chainlock
         self.nodes[0].generate(1)
         self.wait_for_chainlock_tip_all_nodes()

test/functional/tiertwo_signing_session.py

@@ -30,6 +30,12 @@ def run_test(self):
         self.log.info("----------------------------------")
         (qfc, badmembers) = self.mine_quorum()
         assert_equal(171, miner.getblockcount())
+
+        # in order to avoid sync issues signing sessions quorum selection looks for quorums mined at most at chaintip - 8 blocks.
+        # let's round it and mine 10 blocks
+        miner.generate(10)
+        self.sync_all()
+
         # Quorum members
         members = self.get_quorum_members(qfc['quorumHash'])