Avoid unnecessary processing/verification of reconstructed recovered signatures

src/bls/bls_batchverifier.h

@@ -63,6 +63,11 @@ class CBLSBatchVerifier
         messagesBySource.clear();
     }
 
+    size_t GetUniqueSourceCount() const
+    {
+        return messagesBySource.size();
+    }
+
     void Verify()
     {
         std::map<uint256, std::vector<MessageMapIterator>> byMessageHash;

src/llmq/quorums_instantsend.cpp

@@ -19,6 +19,8 @@
 #include <wallet/wallet.h>
 #endif
 
+#include <cxxtimer.hpp>
+
 #include <boost/algorithm/string/replace.hpp>
 #include <boost/thread.hpp>
 
@@ -729,7 +731,18 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
 
     {
         LOCK(cs);
-        pend = std::move(pendingInstantSendLocks);
+        // only process a max 32 locks at a time to avoid duplicate verification of recovered signatures which have been
+        // verified by CSigningManager in parallel
+        const size_t maxCount = 32;
+        if (pendingInstantSendLocks.size() <= maxCount) {
+            pend = std::move(pendingInstantSendLocks);
+        } else {
+            while (pend.size() < maxCount) {
+                auto it = pendingInstantSendLocks.begin();
+                pend.emplace(it->first, std::move(it->second));
+                pendingInstantSendLocks.erase(it);
+            }
+        }
     }
 
     if (pend.empty()) {
@@ -780,13 +793,15 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
     return true;
 }
 
-std::unordered_set<uint256> CInstantSendManager::ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>>& pend, bool ban)
+std::unordered_set<uint256> CInstantSendManager::ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>, StaticSaltedHasher>& pend, bool ban)
 {
     auto llmqType = Params().GetConsensus().llmqTypeInstantSend;
 
     CBLSBatchVerifier<NodeId, uint256> batchVerifier(false, true, 8);
     std::unordered_map<uint256, std::pair<CQuorumCPtr, CRecoveredSig>> recSigs;
 
+    size_t verifyCount = 0;
+    size_t alreadyVerified = 0;
     for (const auto& p : pend) {
         auto& hash = p.first;
         auto nodeId = p.second.first;
@@ -805,6 +820,7 @@ std::unordered_set<uint256> CInstantSendManager::ProcessPendingInstantSendLocks(
 
         // no need to verify an ISLOCK if we already have verified the recovered sig that belongs to it
         if (quorumSigningManager->HasRecoveredSig(llmqType, id, islock.txid)) {
+            alreadyVerified++;
             continue;
         }
 
@@ -815,6 +831,7 @@ std::unordered_set<uint256> CInstantSendManager::ProcessPendingInstantSendLocks(
         }
         uint256 signHash = CLLMQUtils::BuildSignHash(llmqType, quorum->qc.quorumHash, id, islock.txid);
         batchVerifier.PushMessage(nodeId, hash, signHash, islock.sig.Get(), quorum->qc.quorumPublicKey);
+        verifyCount++;
 
         // We can reconstruct the CRecoveredSig objects from the islock and pass it to the signing manager, which
         // avoids unnecessary double-verification of the signature. We however only do this when verification here
@@ -832,7 +849,12 @@ std::unordered_set<uint256> CInstantSendManager::ProcessPendingInstantSendLocks(
         }
     }
 
+    cxxtimer::Timer verifyTimer(true);
     batchVerifier.Verify();
+    verifyTimer.stop();
+
+    LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- verified locks. count=%d, alreadyVerified=%d, vt=%d, nodes=%d\n", __func__,
+            verifyCount, alreadyVerified, verifyTimer.count(), batchVerifier.GetUniqueSourceCount());
 
     std::unordered_set<uint256> badISLocks;
 

src/llmq/quorums_instantsend.h

@@ -97,7 +97,7 @@ class CInstantSendManager : public CRecoveredSigsListener
     std::unordered_map<uint256, CInstantSendLock*, StaticSaltedHasher> txToCreatingInstantSendLocks;
 
     // Incoming and not verified yet
-    std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>> pendingInstantSendLocks;
+    std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>, StaticSaltedHasher> pendingInstantSendLocks;
 
     // TXs which are neither IS locked nor ChainLocked. We use this to determine for which TXs we need to retry IS locking
     // of child TXs
@@ -137,7 +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);
+    std::unordered_set<uint256> ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>, StaticSaltedHasher>& 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

@@ -446,8 +446,17 @@ CSigningManager::CSigningManager(CDBWrapper& llmqDb, bool fMemory) :
 
 bool CSigningManager::AlreadyHave(const CInv& inv)
 {
-    LOCK(cs);
-    return inv.type == MSG_QUORUM_RECOVERED_SIG && db.HasRecoveredSigForHash(inv.hash);
+    if (inv.type != MSG_QUORUM_RECOVERED_SIG) {
+        return false;
+    }
+    {
+        LOCK(cs);
+        if (pendingReconstructedRecoveredSigs.count(inv.hash)) {
+            return true;
+        }
+    }
+
+    return db.HasRecoveredSigForHash(inv.hash);
 }
 
 bool CSigningManager::GetRecoveredSigForGetData(const uint256& hash, CRecoveredSig& ret)
@@ -492,6 +501,12 @@ void CSigningManager::ProcessMessageRecoveredSig(CNode* pfrom, const CRecoveredS
             CLLMQUtils::BuildSignHash(recoveredSig).ToString(), recoveredSig.id.ToString(), recoveredSig.msgHash.ToString(), pfrom->GetId());
 
     LOCK(cs);
+    if (pendingReconstructedRecoveredSigs.count(recoveredSig.GetHash())) {
+        // no need to perform full verification
+        LogPrint(BCLog::LLMQ, "CSigningManager::%s -- already pending reconstructed sig, signHash=%s, id=%s, msgHash=%s, node=%d\n", __func__,
+                 CLLMQUtils::BuildSignHash(recoveredSig).ToString(), recoveredSig.id.ToString(), recoveredSig.msgHash.ToString(), pfrom->GetId());
+        return;
+    }
     pendingRecoveredSigs[pfrom->GetId()].emplace_back(recoveredSig);
 }
 
@@ -587,13 +602,13 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify(
 
 void CSigningManager::ProcessPendingReconstructedRecoveredSigs()
 {
-    decltype(pendingReconstructedRecoveredSigs) l;
+    decltype(pendingReconstructedRecoveredSigs) m;
     {
         LOCK(cs);
-        l = std::move(pendingReconstructedRecoveredSigs);
+        m = std::move(pendingReconstructedRecoveredSigs);
     }
-    for (auto& p : l) {
-        ProcessRecoveredSig(-1, p.first, p.second, *g_connman);
+    for (auto& p : m) {
+        ProcessRecoveredSig(-1, p.second.first, p.second.second, *g_connman);
     }
 }
 
@@ -709,6 +724,8 @@ void CSigningManager::ProcessRecoveredSig(NodeId nodeId, const CRecoveredSig& re
         }
 
         db.WriteRecoveredSig(recoveredSig);
+
+        pendingReconstructedRecoveredSigs.erase(recoveredSig.GetHash());
     }
 
     CInv inv(MSG_QUORUM_RECOVERED_SIG, recoveredSig.GetHash());
@@ -726,7 +743,9 @@ void CSigningManager::ProcessRecoveredSig(NodeId nodeId, const CRecoveredSig& re
 void CSigningManager::PushReconstructedRecoveredSig(const llmq::CRecoveredSig& recoveredSig, const llmq::CQuorumCPtr& quorum)
 {
     LOCK(cs);
-    pendingReconstructedRecoveredSigs.emplace_back(recoveredSig, quorum);
+    pendingReconstructedRecoveredSigs.emplace(std::piecewise_construct,
+            std::forward_as_tuple(recoveredSig.GetHash()),
+            std::forward_as_tuple(recoveredSig, quorum));
 }
 
 void CSigningManager::TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)

src/llmq/quorums_signing.h

@@ -126,7 +126,7 @@ class CSigningManager
 
     // Incoming and not verified yet
     std::unordered_map<NodeId, std::list<CRecoveredSig>> pendingRecoveredSigs;
-    std::list<std::pair<CRecoveredSig, CQuorumCPtr>> pendingReconstructedRecoveredSigs;
+    std::unordered_map<uint256, std::pair<CRecoveredSig, CQuorumCPtr>, StaticSaltedHasher> pendingReconstructedRecoveredSigs;
 
     // must be protected by cs
     FastRandomContext rnd;

src/llmq/quorums_signing_shares.cpp

@@ -652,6 +652,7 @@ bool CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
     // which are not craftable by individual entities, making the rogue public key attack impossible
     CBLSBatchVerifier<NodeId, SigShareKey> batchVerifier(false, true);
 
+    cxxtimer::Timer prepareTimer(true);
     size_t verifyCount = 0;
     for (auto& p : sigSharesByNodes) {
         auto nodeId = p.first;
@@ -684,12 +685,13 @@ bool CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
             verifyCount++;
         }
     }
+    prepareTimer.stop();
 
     cxxtimer::Timer verifyTimer(true);
     batchVerifier.Verify();
     verifyTimer.stop();
 
-    LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- verified sig shares. count=%d, vt=%d, nodes=%d\n", __func__, verifyCount, verifyTimer.count(), sigSharesByNodes.size());
+    LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- verified sig shares. count=%d, pt=%d, vt=%d, nodes=%d\n", __func__, verifyCount, prepareTimer.count(), verifyTimer.count(), sigSharesByNodes.size());
 
     for (auto& p : sigSharesByNodes) {
         auto nodeId = p.first;
@@ -1036,10 +1038,11 @@ void CSigSharesManager::CollectSigSharesToSend(std::unordered_map<NodeId, std::v
         if (curTime >= p.second.nextAttemptTime) {
             p.second.nextAttemptTime = curTime + SEND_FOR_RECOVERY_TIMEOUT;
             auto dmn = SelectMemberForRecovery(p.second.quorum, p.second.sigShare.id, p.second.attempt);
-            LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- sending to %s, signHash=%s\n", __func__,
-                     dmn->proTxHash.ToString(), p.second.sigShare.GetSignHash().ToString());
             p.second.attempt++;
 
+            LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- signHash=%s, sending to %s, attempt=%d\n", __func__,
+                     p.second.sigShare.GetSignHash().ToString(), dmn->proTxHash.ToString(), p.second.attempt);
+
             auto it = proTxToNode.find(dmn->proTxHash);
             if (it == proTxToNode.end()) {
                 continue;