Multiple fixes and optimizations for LLMQs and ChainLocks (#2724)

* Indicate success when signing was unnecessary

* Fix typo in name of LLMQ_400_60

* Move RemoveAskFor call for CLSIGs into ProcessNewChainLock

In case we got INV items for the same CLSIG that we recreated through
HandleNewRecoveredSig, (re-)requesting of the CLSIG from other peers
becomes unnecessary.

* Move Cleanup() call in CChainLocksHandler::UpdatedBlockTip up

We bail out early in a few situations from this method, so that Cleanup()
might not be called while its at the bottom.

* Bail out from CChainLocksHandler::UpdatedBlockTip if we already got the CLSIG

* Call RemoveAskFor when QFCOMMITMENT was received

Otherwise we might end up re-requesting it for a very long time when the
commitment INV was received shortly before it got mined.

* Call RemoveSigSharesForSession when a recovered sig is received

Otherwise we end up with session data in node states lingering around until
a fake "timeout" occurs (can be seen in the logs).

* Better handling of false-positive conflicts in CSigningManager

The old code was emitting a lot of messages in logs as it treated sigs
for exactly the same session as a conflict. This commit fixes this by
looking at the signHash before logging.

Also handle a corner-case where a recovered sig might be deleted between
the HasRecoveredSigForId and GetRecoveredSigById call.

* Don't run into session timeout when sig shares come in slow

Instead of just tracking when the first share was received, we now also
track when the last (non-duplicate) share was received. Sessios will now
timeout 5 minutes after the first share arrives, or 1 minute after the last
one arrived.

Author: codablock

src/llmq/quorums_chainlocks.cpp

@@ -77,17 +77,17 @@ 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) {
@@ -190,6 +190,8 @@ void CChainLocksHandler::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBl
         return;
     }
 
+    Cleanup();
+
     // DIP8 defines a process called "Signing attempts" which should run before the CLSIG is finalized
     // To simplify the initial implementation, we skip this process and directly try to create a CLSIG
     // This will fail when multiple blocks compete, but we accept this for the initial implementation.
@@ -201,6 +203,12 @@ void CChainLocksHandler::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBl
     {
         LOCK(cs);
 
+        if (bestChainLockBlockIndex == pindexNew) {
+            // we first got the CLSIG, then the header, and then the block was connected.
+            // In this case there is no need to continue here.
+            return;
+        }
+
         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.
@@ -226,8 +234,6 @@ void CChainLocksHandler::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBl
     }
 
     quorumSigningManager->AsyncSignIfMember(Params().GetConsensus().llmqChainLocks, requestId, msgHash);
-
-    Cleanup();
 }
 
 // WARNING: cs_main and cs should not be held!

src/llmq/quorums_init.cpp

@@ -54,6 +54,7 @@ void StartLLMQSystem()
         quorumDKGSessionManager->StartThreads();
     }
     if (quorumSigSharesManager) {
+        quorumSigSharesManager->RegisterAsRecoveredSigsListener();
         quorumSigSharesManager->StartWorkerThread();
     }
     if (chainLocksHandler) {
@@ -68,6 +69,7 @@ void StopLLMQSystem()
     }
     if (quorumSigSharesManager) {
         quorumSigSharesManager->StopWorkerThread();
+        quorumSigSharesManager->UnregisterAsRecoveredSigsListener();
     }
     if (quorumDKGSessionManager) {
         quorumDKGSessionManager->StopThreads();

src/llmq/quorums_signing.cpp

@@ -494,11 +494,25 @@ void CSigningManager::ProcessRecoveredSig(NodeId nodeId, const CRecoveredSig& re
             signHash.ToString(), recoveredSig.id.ToString(), recoveredSig.msgHash.ToString(), nodeId);
 
         if (db.HasRecoveredSigForId(llmqType, recoveredSig.id)) {
-            // this should really not happen, as each masternode is participating in only one vote,
-            // even if it's a member of multiple quorums. so a majority is only possible on one quorum and one msgHash per id
-            LogPrintf("CSigningManager::%s -- conflicting recoveredSig for id=%s, msgHash=%s\n", __func__,
-                recoveredSig.id.ToString(), recoveredSig.msgHash.ToString());
-            return;
+            CRecoveredSig otherRecoveredSig;
+            if (db.GetRecoveredSigById(llmqType, recoveredSig.id, otherRecoveredSig)) {
+                auto otherSignHash = llmq::utils::BuildSignHash(recoveredSig);
+                if (signHash != otherSignHash) {
+                    // this should really not happen, as each masternode is participating in only one vote,
+                    // even if it's a member of multiple quorums. so a majority is only possible on one quorum and one msgHash per id
+                    LogPrintf("CSigningManager::%s -- conflicting recoveredSig for signHash=%s, id=%s, msgHash=%s, otherSignHash=%s\n", __func__,
+                        signHash.ToString(), recoveredSig.id.ToString(), recoveredSig.msgHash.ToString(), otherSignHash.ToString());
+                } else {
+                    // Looks like we're trying to process a recSig that is already known. This might happen if the same
+                    // recSig comes in through regular QRECSIG messages and at the same time through some other message
+                    // which allowed to reconstruct a recSig (e.g. IXLOCK). In this case, just bail out.
+                }
+                return;
+            } else {
+                // This case is very unlikely. It can only happen when cleanup caused this specific recSig to vanish
+                // between the HasRecoveredSigForId and GetRecoveredSigById call. If that happens, treat it as if we
+                // never had that recSig
+            }
         }
 
         db.WriteRecoveredSig(recoveredSig);
@@ -552,14 +566,19 @@ bool CSigningManager::AsyncSignIfMember(Consensus::LLMQType llmqType, const uint
         if (db.HasVotedOnId(llmqType, id)) {
             uint256 prevMsgHash;
             db.GetVoteForId(llmqType, id, prevMsgHash);
-            LogPrintf("CSigningManager::%s -- already voted for id=%s and msgHash=%s. Not voting on conflicting msgHash=%s\n", __func__,
-                id.ToString(), prevMsgHash.ToString(), msgHash.ToString());
+            if (msgHash != prevMsgHash) {
+                LogPrintf("CSigningManager::%s -- already voted for id=%s and msgHash=%s. Not voting on conflicting msgHash=%s\n", __func__,
+                    id.ToString(), prevMsgHash.ToString(), msgHash.ToString());
+            } else {
+                LogPrintf("CSigningManager::%s -- already voted for id=%s and msgHash=%s. Not voting again.\n", __func__,
+                    id.ToString(), prevMsgHash.ToString());
+            }
             return false;
         }
 
         if (db.HasRecoveredSigForId(llmqType, id)) {
             // no need to sign it if we already have a recovered sig
-            return false;
+            return true;
         }
         db.WriteVoteForId(llmqType, id, msgHash);
     }

src/llmq/quorums_signing_shares.cpp

@@ -3,15 +3,16 @@
 // Distributed under the MIT/X11 software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include "quorums_signing_shares.h"
+#include "quorums_signing.h"
+
 #include "activemasternode.h"
 #include "bls/bls_batchverifier.h"
 #include "cxxtimer.h"
 #include "init.h"
 #include "net.h"
 #include "net_processing.h"
 #include "netmessagemaker.h"
-#include "quorums_signing.h"
+#include "quorums_signing_shares.h"
 #include "quorums_utils.h"
 #include "random.h"
 #include "shutdown.h"
@@ -171,6 +172,16 @@ void CSigSharesManager::StopWorkerThread()
     }
 }
 
+void CSigSharesManager::RegisterAsRecoveredSigsListener()
+{
+    quorumSigningManager->RegisterRecoveredSigsListener(this);
+}
+
+void CSigSharesManager::UnregisterAsRecoveredSigsListener()
+{
+    quorumSigningManager->UnregisterRecoveredSigsListener(this);
+}
+
 void CSigSharesManager::Interrupt()
 {
     interruptSigningShare();
@@ -548,7 +559,16 @@ void CSigSharesManager::ProcessSigShare(NodeId nodeId, const CSigShare& sigShare
         }
 
         sigSharesToAnnounce.Add(sigShare.GetKey(), true);
-        firstSeenForSessions.emplace(sigShare.GetSignHash(), GetTimeMillis());
+
+        auto it = timeSeenForSessions.find(sigShare.GetSignHash());
+        if (it == timeSeenForSessions.end()) {
+            auto t = GetTimeMillis();
+            // insert first-seen and last-seen time
+            timeSeenForSessions.emplace(sigShare.GetSignHash(), std::make_pair(t, t));
+        } else {
+            // update last-seen time
+            it->second.second = GetTimeMillis();
+        }
 
         if (!quorumNodes.empty()) {
             // don't announce and wait for other nodes to request this share and directly send it to them
@@ -946,11 +966,12 @@ void CSigSharesManager::Cleanup()
 
         // Remove sessions which timed out
         std::unordered_set<uint256> timeoutSessions;
-        for (auto& p : firstSeenForSessions) {
+        for (auto& p : timeSeenForSessions) {
             auto& signHash = p.first;
-            int64_t time = p.second;
+            int64_t firstSeenTime = p.second.first;
+            int64_t lastSeenTime = p.second.second;
 
-            if (now - time >= SIGNING_SESSION_TIMEOUT) {
+            if (now - firstSeenTime >= SESSION_TOTAL_TIMEOUT || now - lastSeenTime >= SESSION_NEW_SHARES_TIMEOUT) {
                 timeoutSessions.emplace(signHash);
             }
         }
@@ -1032,7 +1053,7 @@ void CSigSharesManager::RemoveSigSharesForSession(const uint256& signHash)
     sigSharesRequested.EraseAllForSignHash(signHash);
     sigSharesToAnnounce.EraseAllForSignHash(signHash);
     sigShares.EraseAllForSignHash(signHash);
-    firstSeenForSessions.erase(signHash);
+    timeSeenForSessions.erase(signHash);
 }
 
 void CSigSharesManager::RemoveBannedNodeStates()
@@ -1171,4 +1192,11 @@ void CSigSharesManager::Sign(const CQuorumCPtr& quorum, const uint256& id, const
         sigShare.id.ToString(), sigShare.msgHash.ToString(), t.count());
     ProcessSigShare(-1, sigShare, *g_connman, quorum);
 }
+
+void CSigSharesManager::HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig)
+{
+    LOCK(cs);
+    RemoveSigSharesForSession(llmq::utils::BuildSignHash(recoveredSig));
+}
+
 } // namespace llmq

src/llmq/quorums_signing_shares.h

@@ -325,9 +325,10 @@ class CSigSharesNodeState
     void RemoveSession(const uint256& signHash);
 };
 
-class CSigSharesManager
+class CSigSharesManager : public CRecoveredSigsListener
 {
-    static const int64_t SIGNING_SESSION_TIMEOUT = 60 * 1000;
+    static const int64_t SESSION_NEW_SHARES_TIMEOUT = 60 * 1000;
+    static const int64_t SESSION_TOTAL_TIMEOUT = 5 * 60 * 1000;
     static const int64_t SIG_SHARE_REQUEST_TIMEOUT = 5 * 1000;
 
 private:
@@ -337,7 +338,9 @@ class CSigSharesManager
     CThreadInterrupt interruptSigningShare;
 
     SigShareMap<CSigShare> sigShares;
-    std::unordered_map<uint256, int64_t> firstSeenForSessions;
+
+    // stores time of first and last receivedSigShare. Used to detect timeouts
+    std::unordered_map<uint256, std::pair<int64_t, int64_t>> timeSeenForSessions;
 
     std::unordered_map<NodeId, CSigSharesNodeState> nodeStates;
     SigShareMap<std::pair<NodeId, int64_t>> sigSharesRequested;
@@ -357,13 +360,17 @@ class CSigSharesManager
     void StartWorkerThread();
     void StopWorkerThread();
     void Interrupt();
+    void RegisterAsRecoveredSigsListener();
+    void UnregisterAsRecoveredSigsListener();
 
 public:
     void ProcessMessage(CNode* pnode, const std::string& strCommand, CDataStream& vRecv, CConnman& connman);
 
     void AsyncSign(const CQuorumCPtr& quorum, const uint256& id, const uint256& msgHash);
     void Sign(const CQuorumCPtr& quorum, const uint256& id, const uint256& msgHash);
 
+    void HandleNewRecoveredSig(const CRecoveredSig& recoveredSig);
+
 private:
     void ProcessMessageSigSharesInv(CNode* pfrom, const CSigSharesInv& inv, CConnman& connman);
     void ProcessMessageGetSigShares(CNode* pfrom, const CSigSharesInv& inv, CConnman& connman);