Use lazy BLS signatures more often and don't always verify self-recovered sigs

src/bls/bls.cpp

@@ -423,36 +423,35 @@ bool CBLSSignature::Recover(const std::vector<CBLSSignature>& sigs, const std::v
     return true;
 }
 
-CBLSLazySignature::CBLSLazySignature(CBLSSignature& _sig) :
-    bufValid(false),
-    sigInitialized(true),
-    sig(_sig)
-{
-
-}
-
+#ifndef BUILD_BITCOIN_INTERNAL
 void CBLSLazySignature::SetSig(const CBLSSignature& _sig)
 {
+    std::unique_lock<std::mutex> l(mutex);
     bufValid = false;
     sigInitialized = true;
     sig = _sig;
 }
 
 const CBLSSignature& CBLSLazySignature::GetSig() const
 {
+    std::unique_lock<std::mutex> l(mutex);
+    static CBLSSignature invalidSig;
     if (!bufValid && !sigInitialized) {
-        static CBLSSignature invalidSig;
         return invalidSig;
     }
     if (!sigInitialized) {
         sig.SetBuf(buf, sizeof(buf));
-        sigInitialized = true;
+        if (!sig.CheckMalleable(buf, sizeof(buf))) {
+            bufValid = false;
+            sigInitialized = false;
+            sig = invalidSig;
+        } else {
+            sigInitialized = true;
+        }
     }
     return sig;
 }
 
-#ifndef BUILD_BITCOIN_INTERNAL
-
 static std::once_flag init_flag;
 static mt_pooled_secure_allocator<uint8_t>* secure_allocator_instance;
 static void create_secure_allocator()

src/bls/bls.h

@@ -18,6 +18,7 @@
 #undef DOUBLE
 
 #include <array>
+#include <mutex>
 #include <unistd.h>
 
 // reversed BLS12-381
@@ -175,10 +176,6 @@ class CBLSWrapper
         char buf[SerSize] = {0};
         GetBuf(buf, SerSize);
         s.write((const char*)buf, SerSize);
-
-        // if (s.GetType() != SER_GETHASH) {
-        //    CheckMalleable(buf, SerSize);
-        // }
     }
     template <typename Stream>
     inline void Unserialize(Stream& s, bool checkMalleable = true)
@@ -187,23 +184,22 @@ class CBLSWrapper
         s.read((char*)buf, SerSize);
         SetBuf(buf, SerSize);
 
-        if (checkMalleable) {
-            CheckMalleable(buf, SerSize);
+        if (checkMalleable && !CheckMalleable(buf, SerSize)) {
+            throw std::ios_base::failure("malleable BLS object");
         }
     }
 
-    inline void CheckMalleable(void* buf, size_t size) const
+    inline bool CheckMalleable(void* buf, size_t size) const
     {
         char buf2[SerSize];
-        C tmp;
-        tmp.SetBuf(buf, SerSize);
-        tmp.GetBuf(buf2, SerSize);
+        GetBuf(buf2, SerSize);
         if (memcmp(buf, buf2, SerSize)) {
             // TODO not sure if this is actually possible with the BLS libs. I'm assuming here that somewhere deep inside
             // these libs masking might happen, so that 2 different binary representations could result in the same object
             // representation
-            throw std::ios_base::failure("malleable BLS object");
+            return false;
         }
+        return true;
     }
 
     inline std::string ToString() const
@@ -313,19 +309,51 @@ class CBLSSignature : public CBLSWrapper<bls::InsecureSignature, BLS_CURVE_SIG_S
     bool InternalGetBuf(void* buf) const;
 };
 
+#ifndef BUILD_BITCOIN_INTERNAL
 class CBLSLazySignature
 {
 private:
+    mutable std::mutex mutex;
+
     mutable char buf[BLS_CURVE_SIG_SIZE];
     mutable bool bufValid{false};
 
     mutable CBLSSignature sig;
     mutable bool sigInitialized{false};
 
 public:
+    CBLSLazySignature()
+    {
+        memset(buf, 0, sizeof(buf));
+    }
+
+    CBLSLazySignature(const CBLSLazySignature& r)
+    {
+        *this = r;
+    }
+
+    CBLSLazySignature& operator=(const CBLSLazySignature& r)
+    {
+        std::unique_lock<std::mutex> l(r.mutex);
+        bufValid = r.bufValid;
+        if (r.bufValid) {
+            memcpy(buf, r.buf, sizeof(buf));
+        } else {
+            memset(buf, 0, sizeof(buf));
+        }
+        sigInitialized = r.sigInitialized;
+        if (r.sigInitialized) {
+            sig = r.sig;
+        } else {
+            sig.Reset();
+        }
+        return *this;
+    }
+
     template<typename Stream>
     inline void Serialize(Stream& s) const
     {
+        std::unique_lock<std::mutex> l(mutex);
         if (!sigInitialized && !bufValid) {
             throw std::ios_base::failure("sig and buf not initialized");
         }
@@ -339,18 +367,16 @@ class CBLSLazySignature
     template<typename Stream>
     inline void Unserialize(Stream& s)
     {
+        std::unique_lock<std::mutex> l(mutex);
         s.read(buf, sizeof(buf));
         bufValid = true;
         sigInitialized = false;
     }
 
-public:
-    CBLSLazySignature() = default;
-    CBLSLazySignature(CBLSSignature& _sig);
-
     void SetSig(const CBLSSignature& _sig);
     const CBLSSignature& GetSig() const;
 };
+#endif
 
 typedef std::vector<CBLSId> BLSIdVector;
 typedef std::vector<CBLSPublicKey> BLSVerificationVector;

src/llmq/quorums_chainlocks.cpp

@@ -499,7 +499,7 @@ void CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recove
 
         clsig.nHeight = lastSignedHeight;
         clsig.blockHash = lastSignedMsgHash;
-        clsig.sig = recoveredSig.sig;
+        clsig.sig = recoveredSig.sig.GetSig();
     }
     ProcessNewChainLock(-1, clsig, ::SerializeHash(clsig));
 }

src/llmq/quorums_instantsend.cpp

@@ -579,7 +579,7 @@ bool CInstantSendManager::PreVerifyInstantSendLock(NodeId nodeId, const llmq::CI
 {
     retBan = false;
 
-    if (islock.txid.IsNull() || !islock.sig.IsValid() || islock.inputs.empty()) {
+    if (islock.txid.IsNull() || islock.inputs.empty()) {
         retBan = true;
         return false;
     }
@@ -628,6 +628,15 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
         auto nodeId = p.second.first;
         auto& islock = p.second.second;
 
+        if (batchVerifier.badSources.count(nodeId)) {
+            continue;
+        }
+
+        if (!islock.sig.GetSig().IsValid()) {
+            batchVerifier.badSources.emplace(nodeId);
+            continue;
+        }
+
         auto id = islock.GetRequestId();
 
         // no need to verify an ISLOCK if we already have verified the recovered sig that belongs to it
@@ -641,7 +650,7 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks()
             return false;
         }
         uint256 signHash = CLLMQUtils::BuildSignHash(llmqType, quorum->qc.quorumHash, id, islock.txid);
-        batchVerifier.PushMessage(nodeId, hash, signHash, islock.sig, quorum->qc.quorumPublicKey);
+        batchVerifier.PushMessage(nodeId, hash, signHash, islock.sig.GetSig(), quorum->qc.quorumPublicKey);
 
         // 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

src/llmq/quorums_instantsend.h

@@ -22,7 +22,7 @@ class CInstantSendLock
 public:
     std::vector<COutPoint> inputs;
     uint256 txid;
-    CBLSSignature sig;
+    CBLSLazySignature sig;
 
 public:
     ADD_SERIALIZE_METHODS

src/llmq/quorums_signing.cpp

@@ -101,7 +101,7 @@ bool CRecoveredSigsDb::ReadRecoveredSig(Consensus::LLMQType llmqType, const uint
     }
 
     try {
-        ret.Unserialize(ds, false);
+        ret.Unserialize(ds);
         return true;
     } catch (std::exception&) {
         return false;
@@ -328,11 +328,6 @@ bool CSigningManager::PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig&
         return false;
     }
 
-    if (!recoveredSig.sig.IsValid()) {
-        retBan = true;
-        return false;
-    }
-
     return true;
 }
 
@@ -436,8 +431,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.GetSig().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(), CLLMQUtils::BuildSignHash(recSig), recSig.sig, quorum->qc.quorumPublicKey);
+            batchVerifier.PushMessage(nodeId, recSig.GetHash(), CLLMQUtils::BuildSignHash(recSig), recSig.sig.GetSig(), quorum->qc.quorumPublicKey);
             verifyCount++;
         }
     }

src/llmq/quorums_signing.h

@@ -24,34 +24,25 @@ 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
-    {
-        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)
+    ADD_SERIALIZE_METHODS;
+
+    template <typename Stream, typename Operation>
+    inline void SerializationOp(Stream& s, Operation ser_action)
     {
-        s >> llmqType;
-        s >> quorumHash;
-        s >> id;
-        s >> msgHash;
-        if (!skipSig) {
-            sig.Unserialize(s, checkMalleable);
-            if (updateHash) {
-                UpdateHash();
-            }
+        READWRITE(llmqType);
+        READWRITE(quorumHash);
+        READWRITE(id);
+        READWRITE(msgHash);
+        READWRITE(sig);
+        if (ser_action.ForRead()) {
+            UpdateHash();
         }
     }
 

src/llmq/quorums_signing_shares.cpp

@@ -762,16 +762,21 @@ void CSigSharesManager::TryRecoverSig(const CQuorumCPtr& quorum, const uint256&
     rs.quorumHash = quorum->qc.quorumHash;
     rs.id = id;
     rs.msgHash = msgHash;
-    rs.sig = recoveredSig;
+    rs.sig.SetSig(recoveredSig);
     rs.UpdateHash();
 
-    auto signHash = CLLMQUtils::BuildSignHash(rs);
-    bool valid = rs.sig.VerifyInsecure(quorum->qc.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 = CLLMQUtils::BuildSignHash(rs);
+        bool valid = recoveredSig.VerifyInsecure(quorum->qc.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);

src/llmq/quorums_signing_shares.h

@@ -361,6 +361,7 @@ class CSigSharesManager : public CRecoveredSigsListener
     FastRandomContext rnd;
 
     int64_t lastCleanupTime{0};
+    std::atomic<uint32_t> recoveredSigsCounter{0};
 
 public:
     CSigSharesManager();