Use lazy BLS signatures more often and don't always verify self-recovered sigs (dashpay#2860)

codablock · panleone · commit 965a4a79154f · 2024-11-14T10:09:59.000+01:00
* Make CBLSLazySignature thread safe

* Perform malleability check in CBLSLazySignature

* Use CBLSLazySignature in CRecoveredSig and CInstantSendLock

* Only sporadically verify self-recovered signatures

* test
diff --git a/src/llmq/quorums_chainlocks.cpp b/src/llmq/quorums_chainlocks.cpp
@@ -345,7 +345,7 @@ void CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recove
 
         clsig.nHeight = lastSignedHeight;
         clsig.blockHash = lastSignedMsgHash;
-        clsig.sig = recoveredSig.sig;
+        clsig.sig = recoveredSig.sig.Get();
     }
     ProcessNewChainLock(-1, clsig, ::SerializeHash(clsig));
 }
diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp
@@ -100,7 +100,7 @@ bool CRecoveredSigsDb::ReadRecoveredSig(Consensus::LLMQType llmqType, const uint
     }
 
     try {
-        ret.Unserialize(ds, false);
+        ret.Unserialize(ds);
         return true;
     } catch (std::exception&) {
         return false;
@@ -326,11 +326,6 @@ bool CSigningManager::PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig&
         return false;
     }
 
-    if (!recoveredSig.sig.IsValid()) {
-        retBan = true;
-        return false;
-    }
-
     return true;
 }
 
@@ -417,8 +412,14 @@ bool CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman)
         auto& v = p.second;
 
         for (auto& recSig : v) {
+            // we didn't verify the lazy signature until now
+            if (!recSig.sig.Get().IsValid()) {
+                batchVerifier.badSources.emplace(nodeId);
+                break;
+            }
+
             const auto& quorum = quorums.at(std::make_pair((Consensus::LLMQType)recSig.llmqType, recSig.quorumHash));
-            batchVerifier.PushMessage(nodeId, recSig.GetHash(), llmq::utils::BuildSignHash(recSig), recSig.sig, quorum->quorumPublicKey);
+            batchVerifier.PushMessage(nodeId, recSig.GetHash(), llmq::utils::BuildSignHash(recSig), recSig.sig.Get(), quorum->quorumPublicKey);
             verifyCount++;
         }
     }
diff --git a/src/llmq/quorums_signing.h b/src/llmq/quorums_signing.h
@@ -26,34 +26,20 @@ class CRecoveredSig
     uint256 quorumHash;
     uint256 id;
     uint256 msgHash;
-    CBLSSignature sig;
+    CBLSLazySignature sig;
 
     // only in-memory
     uint256 hash;
 
 public:
-    template <typename Stream>
-    inline void Serialize(Stream& s) const
+    SERIALIZE_METHODS(CRecoveredSig, obj)
     {
-        s << llmqType;
-        s << quorumHash;
-        s << id;
-        s << msgHash;
-        s << sig;
-    }
-    template <typename Stream>
-    inline void Unserialize(Stream& s, bool checkMalleable = true, bool updateHash = true, bool skipSig = false)
-    {
-        s >> llmqType;
-        s >> quorumHash;
-        s >> id;
-        s >> msgHash;
-        if (!skipSig) {
-            sig.Unserialize(s, checkMalleable);
-            if (updateHash) {
-                UpdateHash();
-            }
-        }
+        READWRITE(obj.llmqType);
+        READWRITE(obj.quorumHash);
+        READWRITE(obj.id);
+        READWRITE(obj.msgHash);
+        READWRITE(obj.sig);
+        SER_READ(obj, obj.UpdateHash());
     }
 
     void UpdateHash()
diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp
@@ -753,16 +753,21 @@ void CSigSharesManager::TryRecoverSig(const CQuorumCPtr& quorum, const uint256&
     rs.quorumHash = quorum->pindexQuorum->GetBlockHash();
     rs.id = id;
     rs.msgHash = msgHash;
-    rs.sig = recoveredSig;
+    rs.sig.Set(recoveredSig);
     rs.UpdateHash();
 
-    auto signHash = llmq::utils::BuildSignHash(rs);
-    bool valid = rs.sig.VerifyInsecure(quorum->quorumPublicKey, signHash);
-    if (!valid) {
-        // this should really not happen as we have verified all signature shares before
-        LogPrintf("CSigSharesManager::%s -- own recovered signature is invalid. id=%s, msgHash=%s\n", __func__,
-            id.ToString(), msgHash.ToString());
-        return;
+    // There should actually be no need to verify the self-recovered signatures as it should always succeed. Let's
+    // however still verify it from time to time, so that we have a chance to catch bugs. We do only this sporadic
+    // verification because this is unbatched and thus slow verification that happens here.
+    if (((recoveredSigsCounter++) % 100) == 0) {
+        auto signHash = llmq::utils::BuildSignHash(rs);
+        bool valid = recoveredSig.VerifyInsecure(quorum->quorumPublicKey, signHash);
+        if (!valid) {
+            // this should really not happen as we have verified all signature shares before
+            LogPrintf("CSigSharesManager::%s -- own recovered signature is invalid. id=%s, msgHash=%s\n", __func__,
+                id.ToString(), msgHash.ToString());
+            return;
+        }
     }
 
     quorumSigningManager->ProcessRecoveredSig(-1, rs, quorum, connman);
diff --git a/src/llmq/quorums_signing_shares.h b/src/llmq/quorums_signing_shares.h
@@ -353,6 +353,7 @@ class CSigSharesManager : public CRecoveredSigsListener
     FastRandomContext rnd;
 
     int64_t lastCleanupTime{0};
+    std::atomic<uint32_t> recoveredSigsCounter{0};
 
 public:
     CSigSharesManager();

Original file line number	Diff line number	Diff line change
`@@ -345,7 +345,7 @@ void CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recove`
`345`	`345`
`346`	`346`	`clsig.nHeight = lastSignedHeight;`
`347`	`347`	`clsig.blockHash = lastSignedMsgHash;`
`348`		`- clsig.sig = recoveredSig.sig;`
	`348`	`+ clsig.sig = recoveredSig.sig.Get();`
`349`	`349`	`}`
`350`	`350`	`ProcessNewChainLock(-1, clsig, ::SerializeHash(clsig));`
`351`	`351`	`}`