[DMNs] Chainlock optimizations

src/evo/evonotificationinterface.cpp

@@ -25,7 +25,7 @@ void EvoNotificationInterface::AcceptedBlockHeader(const CBlockIndex* pindexNew)
 void EvoNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload)
 {
     // background thread updates
-    llmq::chainLocksHandler->UpdatedBlockTip(pindexNew, pindexFork);
+    llmq::chainLocksHandler->UpdatedBlockTip(pindexNew);
     llmq::quorumDKGSessionManager->UpdatedBlockTip(pindexNew, fInitialDownload);
     llmq::quorumManager->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload);
 }

src/init.cpp

@@ -1541,7 +1541,7 @@ bool AppInitMain()
                 // The on-disk coinsdb is now in a good state, create the cache
                 pcoinsTip.reset(new CCoinsViewCache(pcoinscatcher.get()));
 
-                InitTierTwoPostCoinsCacheLoad(&scheduler);
+                InitTierTwoPostCoinsCacheLoad();
 
                 bool is_coinsview_empty = fReset || fReindexChainState || pcoinsTip->GetBestBlock().IsNull();
                 if (!is_coinsview_empty) {

src/llmq/quorums_chainlocks.cpp

@@ -27,14 +27,33 @@ std::string CChainLockSig::ToString() const
     return strprintf("CChainLockSig(nHeight=%d, blockHash=%s)", nHeight, blockHash.ToString());
 }
 
-CChainLocksHandler::CChainLocksHandler(CScheduler* _scheduler) :
-    scheduler(_scheduler)
+CChainLocksHandler::CChainLocksHandler()
 {
-    quorumSigningManager->RegisterRecoveredSigsListener(this);
+    scheduler = new CScheduler();
+    CScheduler::Function serviceLoop = boost::bind(&CScheduler::serviceQueue, scheduler);
+    scheduler_thread = new boost::thread(boost::bind(&TraceThread<CScheduler::Function>, "chainlock-scheduler", serviceLoop));
 }
 
 CChainLocksHandler::~CChainLocksHandler()
 {
+    scheduler_thread->interrupt();
+    scheduler_thread->join();
+    delete scheduler_thread;
+    delete scheduler;
+}
+
+void CChainLocksHandler::Start()
+{
+    quorumSigningManager->RegisterRecoveredSigsListener(this);
+    scheduler->scheduleEvery([&]() {
+        EnforceBestChainLock();
+    },
+        5000);
+}
+
+void CChainLocksHandler::Stop()
+{
+    scheduler->stop();
     quorumSigningManager->UnregisterRecoveredSigsListener(this);
 }
 
@@ -57,7 +76,7 @@ bool CChainLocksHandler::GetChainLockByHash(const uint256& hash, llmq::CChainLoc
     return true;
 }
 
-void CChainLocksHandler::ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, CConnman& connman)
+void CChainLocksHandler::ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv)
 {
     if (!sporkManager.IsSporkActive(SPORK_23_CHAINLOCKS_ENFORCEMENT)) {
         return;
@@ -69,17 +88,16 @@ void CChainLocksHandler::ProcessMessage(CNode* pfrom, const std::string& strComm
 
         auto hash = ::SerializeHash(clsig);
 
-        {
-            LOCK(cs_main);
-            connman.RemoveAskFor(hash, MSG_CLSIG);
-        }
-
         ProcessNewChainLock(pfrom->GetId(), clsig, hash);
     }
 }
 
 void CChainLocksHandler::ProcessNewChainLock(NodeId from, const llmq::CChainLockSig& clsig, const uint256& hash)
 {
+    {
+        LOCK(cs_main);
+        g_connman->RemoveAskFor(hash, MSG_CLSIG);
+    }
     {
         LOCK(cs);
         if (!seenChainLocks.emplace(hash, GetTimeMillis()).second) {
@@ -93,89 +111,114 @@ void CChainLocksHandler::ProcessNewChainLock(NodeId from, const llmq::CChainLock
     }
 
     uint256 requestId = ::SerializeHash(std::make_pair(CLSIG_REQUESTID_PREFIX, clsig.nHeight));
-    uint256 msgHash = clsig.blockHash;
-    if (!quorumSigningManager->VerifyRecoveredSig(Params().GetConsensus().llmqChainLocks, clsig.nHeight, requestId, msgHash, clsig.sig)) {
+    if (!quorumSigningManager->VerifyRecoveredSig(Params().GetConsensus().llmqChainLocks, clsig.nHeight, requestId, clsig.blockHash, clsig.sig)) {
         LogPrintf("CChainLocksHandler::%s -- invalid CLSIG (%s), peer=%d\n", __func__, clsig.ToString(), from);
         if (from != -1) {
             LOCK(cs_main);
-            Misbehaving(from, 100);
+            Misbehaving(from, 10);
         }
         return;
     }
 
+    CBlockIndex* pindex;
     {
-        LOCK2(cs_main, cs);
-
-        if (InternalHasConflictingChainLock(clsig.nHeight, clsig.blockHash)) {
-            // This should not happen. If it happens, it means that a malicious entity controls a large part of the MN
-            // network. In this case, we don't allow him to reorg older chainlocks.
-            LogPrintf("CChainLocksHandler::%s -- new CLSIG (%s) tries to reorg previous CLSIG (%s), peer=%d\n",
-                      __func__, clsig.ToString(), bestChainLock.ToString(), from);
-            return;
-        }
+        LOCK(cs_main);
+        pindex = LookupBlockIndex(clsig.blockHash);
+    }
 
+    {
+        LOCK(cs);
         bestChainLockHash = hash;
         bestChainLock = clsig;
 
-        CInv inv(MSG_CLSIG, hash);
-        g_connman->RelayInv(inv);
-
-        auto blockIt = mapBlockIndex.find(clsig.blockHash);
-        if (blockIt == mapBlockIndex.end()) {
-            // we don't know the block/header for this CLSIG yet, so bail out for now
-            // when the block or the header later comes in, we will enforce the correct chain
-            return;
+        if (pindex != nullptr) {
+            if (pindex->nHeight != clsig.nHeight) {
+                // Should not happen, same as the conflict check from above.
+                LogPrintf("CChainLocksHandler::%s -- height of CLSIG (%s) does not match the specified block's height (%d)\n",
+                    __func__, clsig.ToString(), pindex->nHeight);
+                // Note: not relaying clsig here
+                return;
+            }
+            bestChainLockWithKnownBlock = bestChainLock;
+            bestChainLockBlockIndex = pindex;
         }
+    }
 
-        if (blockIt->second->nHeight != clsig.nHeight) {
-            // Should not happen, same as the conflict check from above.
-            LogPrintf("CChainLocksHandler::%s -- height of CLSIG (%s) does not match the specified block's height (%d)\n",
-                    __func__, clsig.ToString(), blockIt->second->nHeight);
-            return;
-        }
+    // Do not hold cs while calling RelayInv
+    AssertLockNotHeld(cs);
+    CInv clsigInv(MSG_CLSIG, hash);
+    g_connman->RelayInv(clsigInv);
 
-        const CBlockIndex* pindex = blockIt->second;
-        bestChainLockWithKnownBlock = bestChainLock;
-        bestChainLockBlockIndex = pindex;
+    if (pindex == nullptr) {
+        // we don't know the block/header for this CLSIG yet, so bail out for now
+        // when the block or the header later comes in, we will enforce the correct chain
+        return;
     }
 
-    EnforceBestChainLock();
+    scheduler->scheduleFromNow([&]() {
+        EnforceBestChainLock();
+    },
+        0);
 
     LogPrintf("CChainLocksHandler::%s -- processed new CLSIG (%s), peer=%d\n",
               __func__, clsig.ToString(), from);
 }
 
 void CChainLocksHandler::AcceptedBlockHeader(const CBlockIndex* pindexNew)
 {
-    bool doEnforce = false;
-    {
-        LOCK2(cs_main, cs);
-
-        if (pindexNew->GetBlockHash() == bestChainLock.blockHash) {
-            LogPrintf("CChainLocksHandler::%s -- block header %s came in late, updating and enforcing\n", __func__, pindexNew->GetBlockHash().ToString());
+    LOCK(cs);
 
-            if (bestChainLock.nHeight != pindexNew->nHeight) {
-                // Should not happen, same as the conflict check from ProcessNewChainLock.
-                LogPrintf("CChainLocksHandler::%s -- height of CLSIG (%s) does not match the specified block's height (%d)\n",
-                          __func__, bestChainLock.ToString(), pindexNew->nHeight);
-                return;
-            }
+    if (pindexNew->GetBlockHash() == bestChainLock.blockHash) {
+        LogPrintf("CChainLocksHandler::%s -- block header %s came in late, updating and enforcing\n", __func__, pindexNew->GetBlockHash().ToString());
 
-            bestChainLockBlockIndex = pindexNew;
-            doEnforce = true;
+        if (bestChainLock.nHeight != pindexNew->nHeight) {
+            // Should not happen, same as the conflict check from ProcessNewChainLock.
+            LogPrintf("CChainLocksHandler::%s -- height of CLSIG (%s) does not match the specified block's height (%d)\n",
+                __func__, bestChainLock.ToString(), pindexNew->nHeight);
+            return;
         }
+
+        // when EnforceBestChainLock is called later, it might end up invalidating other chains but not activating the
+        // CLSIG locked chain. This happens when only the header is known but the block is still missing yet. The usual
+        // block processing logic will handle this when the block arrives
+        bestChainLockWithKnownBlock = bestChainLock;
+        bestChainLockBlockIndex = pindexNew;
     }
-    if (doEnforce) {
-        EnforceBestChainLock();
+}
+
+void CChainLocksHandler::UpdatedBlockTip(const CBlockIndex* pindexNew)
+{
+    // don't call TrySignChainTip directly but instead let the scheduler call it. This way we ensure that cs_main is
+    // never locked and TrySignChainTip is not called twice in parallel
+    // EnforceBestChainLock switching chains.
+    LOCK(cs);
+    if (tryLockChainTipScheduled) {
+        return;
     }
+    tryLockChainTipScheduled = true;
+    scheduler->scheduleFromNow([&]() {
+        EnforceBestChainLock();
+        TrySignChainTip();
+        LOCK(cs);
+        tryLockChainTipScheduled = false;
+    },
+        0);
 }
 
-void CChainLocksHandler::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork)
+void CChainLocksHandler::TrySignChainTip()
 {
+    Cleanup();
+
+    const CBlockIndex* pindex;
+    {
+        LOCK(cs_main);
+        pindex = chainActive.Tip();
+    }
+
     if (!fMasterNode) {
         return;
     }
-    if (!pindexNew->pprev) {
+    if (!pindex->pprev) {
         return;
     }
     if (!sporkManager.IsSporkActive(SPORK_23_CHAINLOCKS_ENFORCEMENT)) {
@@ -187,59 +230,68 @@ void CChainLocksHandler::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBl
     // This will fail when multiple blocks compete, but we accept this for the initial implementation.
     // Later, we'll add the multiple attempts process.
 
-    uint256 requestId = ::SerializeHash(std::make_pair(CLSIG_REQUESTID_PREFIX, pindexNew->nHeight));
-    uint256 msgHash = pindexNew->GetBlockHash();
-
     {
         LOCK(cs);
 
-        if (InternalHasConflictingChainLock(pindexNew->nHeight, pindexNew->GetBlockHash())) {
-            if (!inEnforceBestChainLock) {
-                // we accepted this block when there was no lock yet, but now a conflicting lock appeared. Invalidate it.
-                LogPrintf("CChainLocksHandler::%s -- conflicting lock after block was accepted, invalidating now\n",
-                          __func__);
-                ScheduleInvalidateBlock(pindexNew);
-            }
+        if (pindex->nHeight == lastSignedHeight) {
+            // already signed this one
             return;
         }
 
-        if (bestChainLock.nHeight >= pindexNew->nHeight) {
+        if (bestChainLock.nHeight >= pindex->nHeight) {
             // already got the same CLSIG or a better one
             return;
         }
 
-        if (pindexNew->nHeight == lastSignedHeight) {
-            // already signed this one
+        if (InternalHasConflictingChainLock(pindex->nHeight, pindex->GetBlockHash())) {
+            // don't sign if another conflicting CLSIG is already present. EnforceBestChainLock will later enforce
+            // the correct chain.
             return;
         }
-        lastSignedHeight = pindexNew->nHeight;
+    }
+    uint256 requestId = ::SerializeHash(std::make_pair(CLSIG_REQUESTID_PREFIX, pindex->nHeight));
+    uint256 msgHash = pindex->GetBlockHash();
+
+    {
+        LOCK(cs);
+        if (bestChainLock.nHeight >= pindex->nHeight) {
+            // might have happened while we didn't hold cs
+            return;
+        }
+        lastSignedHeight = pindex->nHeight;
         lastSignedRequestId = requestId;
         lastSignedMsgHash = msgHash;
     }
 
     quorumSigningManager->AsyncSignIfMember(Params().GetConsensus().llmqChainLocks, requestId, msgHash);
-
-    Cleanup();
 }
 
 // WARNING: cs_main and cs should not be held!
+// This should also not be called from validation signals, as this might result in recursive calls
 void CChainLocksHandler::EnforceBestChainLock()
 {
+    AssertLockNotHeld(cs);
+    AssertLockNotHeld(cs_main);
+
     CChainLockSig clsig;
     const CBlockIndex* pindex;
+    const CBlockIndex* currentBestChainLockBlockIndex;
     {
         LOCK(cs);
         clsig = bestChainLockWithKnownBlock;
-        pindex = bestChainLockBlockIndex;
+        pindex = currentBestChainLockBlockIndex = this->bestChainLockBlockIndex;
+        if (!currentBestChainLockBlockIndex) {
+            // we don't have the header/block, so we can't do anything right now
+            return;
+        }
     }
-
+    bool activateNeeded;
     {
         LOCK(cs_main);
 
         // Go backwards through the chain referenced by clsig until we find a block that is part of the main chain.
         // For each of these blocks, check if there are children that are NOT part of the chain referenced by clsig
         // and invalidate each of them.
-        inEnforceBestChainLock = true; // avoid unnecessary ScheduleInvalidateBlock calls inside UpdatedBlockTip
         while (pindex && !chainActive.Contains(pindex)) {
             // Invalidate all blocks that have the same prevBlockHash but are not equal to blockHash
             auto itp = mapPrevBlockIndex.equal_range(pindex->pprev->GetBlockHash());
@@ -248,20 +300,27 @@ void CChainLocksHandler::EnforceBestChainLock()
                     continue;
                 }
                 LogPrintf("CChainLocksHandler::%s -- CLSIG (%s) invalidates block %s\n",
-                          __func__, bestChainLockWithKnownBlock.ToString(), jt->second->GetBlockHash().ToString());
+                    __func__, clsig.ToString(), jt->second->GetBlockHash().ToString());
                 DoInvalidateBlock(jt->second, false);
             }
 
             pindex = pindex->pprev;
         }
-        inEnforceBestChainLock = false;
+        // In case blocks from the correct chain are invalid at the moment, reconsider them. The only case where this
+        // can happen right now is when missing superblock triggers caused the main chain to be dismissed first. When
+        // the trigger later appears, this should bring us to the correct chain eventually. Please note that this does
+        // NOT enforce invalid blocks in any way, it just causes re-validation.
+        if (!currentBestChainLockBlockIndex->IsValid()) {
+            CValidationState state;
+            ReconsiderBlock(state, LookupBlockIndex(currentBestChainLockBlockIndex->GetBlockHash()));
+        }
+
+        activateNeeded = chainActive.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) != currentBestChainLockBlockIndex;
     }
 
     CValidationState state;
-    if (!ActivateBestChain(state)) {
-        LogPrintf("CChainLocksHandler::UpdatedBlockTip -- ActivateBestChain failed: %s\n", state.GetRejectReason());
-        // This should not have happened and we are in a state were it's not safe to continue anymore
-        assert(false);
+    if (activateNeeded && !ActivateBestChain(state)) {
+        LogPrintf("CChainLocksHandler::%s -- ActivateBestChain failed: %s\n", __func__, state.GetRejectReason());
     }
 }
 
@@ -291,16 +350,6 @@ void CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recove
     ProcessNewChainLock(-1, clsig, ::SerializeHash(clsig));
 }
 
-void CChainLocksHandler::ScheduleInvalidateBlock(const CBlockIndex* pindex)
-{
-    // Calls to InvalidateBlock and ActivateBestChain might result in re-invocation of the UpdatedBlockTip and other
-    // signals, so we can't directly call it from signal handlers. We solve this by doing the call from the scheduler
-
-    scheduler->scheduleFromNow([this, pindex]() {
-        DoInvalidateBlock(pindex, true);
-    }, 0);
-}
-
 // WARNING, do not hold cs while calling this method as we'll otherwise run into a deadlock
 void CChainLocksHandler::DoInvalidateBlock(const CBlockIndex* pindex, bool activateBestChain)
 {
@@ -310,7 +359,7 @@ void CChainLocksHandler::DoInvalidateBlock(const CBlockIndex* pindex, bool activ
         LOCK(cs_main);
 
         // get the non-const pointer
-        CBlockIndex* pindex2 = mapBlockIndex[pindex->GetBlockHash()];
+        CBlockIndex* pindex2 = LookupBlockIndex(pindex->GetBlockHash());
 
         CValidationState state;
         if (!InvalidateBlock(state, params, pindex2)) {