Re-verify invalid IS sigs when the active quorum set rotated (#3052)

* Split ProcessPendingInstantSendLocks into two methods

* Split SelectQuorumForSigning into SelectQuorumForSigning and GetActiveQuorumSet

* Implement retrying of IS lock verification when the LLMQ active set rotates

Author: codablock

src/llmq/quorums_instantsend.cpp

@@ -708,8 +708,6 @@ bool CInstantSendManager::PreVerifyInstantSendLock(NodeId nodeId, const llmq::CI
 
 bool CInstantSendManager::ProcessPendingInstantSendLocks()
 {
-    auto llmqType = Params().GetConsensus().llmqForInstantSend;
-
     decltype(pendingInstantSendLocks) pend;
 
     {
@@ -731,6 +729,44 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
         tipHeight = chainActive.Height();
     }
 
+    auto llmqType = Params().GetConsensus().llmqForInstantSend;
+
+    // Every time a new quorum enters the active set, an older one is removed. This means that between two blocks, the
+    // active set can be different, leading to different selection of the signing quorum. When we detect such rotation
+    // of the active set, we must re-check invalid sigs against the previous active set and only ban nodes when this also
+    // fails.
+    auto quorums1 = quorumSigningManager->GetActiveQuorumSet(llmqType, tipHeight);
+    auto quorums2 = quorumSigningManager->GetActiveQuorumSet(llmqType, tipHeight - 1);
+    bool quorumsRotated = quorums1 != quorums2;
+
+    if (quorumsRotated) {
+        // first check against the current active set and don't ban
+        auto badISLocks = ProcessPendingInstantSendLocks(tipHeight, pend, false);
+        if (!badISLocks.empty()) {
+            LogPrintf("CInstantSendManager::%s -- detected LLMQ active set rotation, redoing verification on old active set\n", __func__);
+
+            // filter out valid IS locks from "pend"
+            for (auto it = pend.begin(); it != pend.end(); ) {
+                if (!badISLocks.count(it->first)) {
+                    it = pend.erase(it);
+                } else {
+                    ++it;
+                }
+            }
+            // now check against the previous active set and perform banning if this fails
+            ProcessPendingInstantSendLocks(tipHeight - 1, pend, true);
+        }
+    } else {
+        ProcessPendingInstantSendLocks(tipHeight, pend, true);
+    }
+
+    return true;
+}
+
+std::unordered_set<uint256> CInstantSendManager::ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>>& pend, bool ban)
+{
+    auto llmqType = Params().GetConsensus().llmqForInstantSend;
+
     CBLSBatchVerifier<NodeId, uint256> batchVerifier(false, true, 8);
     std::unordered_map<uint256, std::pair<CQuorumCPtr, CRecoveredSig>> recSigs;
 
@@ -755,10 +791,10 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
             continue;
         }
 
-        auto quorum = quorumSigningManager->SelectQuorumForSigning(llmqType, tipHeight, id);
+        auto quorum = quorumSigningManager->SelectQuorumForSigning(llmqType, signHeight, id);
         if (!quorum) {
             // should not happen, but if one fails to select, all others will also fail to select
-            return false;
+            return {};
         }
         uint256 signHash = CLLMQUtils::BuildSignHash(llmqType, quorum->qc.quorumHash, id, islock.txid);
         batchVerifier.PushMessage(nodeId, hash, signHash, islock.sig.Get(), quorum->qc.quorumPublicKey);
@@ -781,7 +817,9 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
 
     batchVerifier.Verify();
 
-    if (!batchVerifier.badSources.empty()) {
+    std::unordered_set<uint256> badISLocks;
+
+    if (ban && !batchVerifier.badSources.empty()) {
         LOCK(cs_main);
         for (auto& nodeId : batchVerifier.badSources) {
             // Let's not be too harsh, as the peer might simply be unlucky and might have sent us an old lock which
@@ -797,6 +835,7 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
         if (batchVerifier.badMessages.count(hash)) {
             LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: invalid sig in islock, peer=%d\n", __func__,
                      islock.txid.ToString(), hash.ToString(), nodeId);
+            badISLocks.emplace(hash);
             continue;
         }
 
@@ -817,7 +856,7 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
         }
     }
 
-    return true;
+    return badISLocks;
 }
 
 void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& hash, const CInstantSendLock& islock)

src/llmq/quorums_instantsend.h

@@ -137,6 +137,7 @@ class CInstantSendManager : public CRecoveredSigsListener
     void ProcessMessageInstantSendLock(CNode* pfrom, const CInstantSendLock& islock, CConnman& connman);
     bool PreVerifyInstantSendLock(NodeId nodeId, const CInstantSendLock& islock, bool& retBan);
     bool ProcessPendingInstantSendLocks();
+    std::unordered_set<uint256> ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>>& pend, bool ban);
     void ProcessInstantSendLock(NodeId from, const uint256& hash, const CInstantSendLock& islock);
     void UpdateWalletTransaction(const CTransactionRef& tx, const CInstantSendLock& islock);
 

src/llmq/quorums_signing.cpp

@@ -849,7 +849,7 @@ bool CSigningManager::GetVoteForId(Consensus::LLMQType llmqType, const uint256&
     return db.GetVoteForId(llmqType, id, msgHashRet);
 }
 
-CQuorumCPtr CSigningManager::SelectQuorumForSigning(Consensus::LLMQType llmqType, int signHeight, const uint256& selectionHash)
+std::vector<CQuorumCPtr> CSigningManager::GetActiveQuorumSet(Consensus::LLMQType llmqType, int signHeight)
 {
     auto& llmqParams = Params().GetConsensus().llmqs.at(llmqType);
     size_t poolSize = (size_t)llmqParams.signingActiveQuorumCount;
@@ -859,12 +859,17 @@ CQuorumCPtr CSigningManager::SelectQuorumForSigning(Consensus::LLMQType llmqType
         LOCK(cs_main);
         int startBlockHeight = signHeight - SIGN_HEIGHT_OFFSET;
         if (startBlockHeight > chainActive.Height()) {
-            return nullptr;
+            return {};
         }
         pindexStart = chainActive[startBlockHeight];
     }
 
-    auto quorums = quorumManager->ScanQuorums(llmqType, pindexStart, poolSize);
+    return quorumManager->ScanQuorums(llmqType, pindexStart, poolSize);
+}
+
+CQuorumCPtr CSigningManager::SelectQuorumForSigning(Consensus::LLMQType llmqType, int signHeight, const uint256& selectionHash)
+{
+    auto quorums = GetActiveQuorumSet(llmqType, signHeight);
     if (quorums.empty()) {
         return nullptr;
     }

src/llmq/quorums_signing.h

@@ -177,6 +177,7 @@ class CSigningManager
     bool HasVotedOnId(Consensus::LLMQType llmqType, const uint256& id);
     bool GetVoteForId(Consensus::LLMQType llmqType, const uint256& id, uint256& msgHashRet);
 
+    std::vector<CQuorumCPtr> GetActiveQuorumSet(Consensus::LLMQType llmqType, int signHeight);
     CQuorumCPtr SelectQuorumForSigning(Consensus::LLMQType llmqType, int signHeight, const uint256& selectionHash);
 
     // Verifies a recovered sig that was signed while the chain tip was at signedAtTip