Merge #2885: [BLS] [TEST] Update bls batchverifier

fbada5b Unit test coverage (Alessandro Rezzi)
115aa08 Update bls_batchverifier to upstream version (Alessandro Rezzi)

Pull request description:

  The first commit updates `bls_batchverifier.h` to the most recent upstream version
  The second commit adds unit tests  for the batchverifier

ACKs for top commit: fbada5b
  Fuzzbawls:
    ACK fbada5b
  Liquid369:
    tACK fbada5b

Tree-SHA512: 6bdf56f2935970797b90d7f428d0a9bf7ce2a62d1d4e9d812d6acd0fa71ed74f1715208001b31c9a76946e21349129a29ef39224eaa7e383d8040675b2c2c5f3

src/bls/bls_batchverifier.h

@@ -11,8 +11,8 @@
 #include <map>
 #include <vector>
 
-template<typename SourceId, typename MessageId>
-class CBLSInsecureBatchVerifier
+template <typename SourceId, typename MessageId>
+class CBLSBatchVerifier
 {
 private:
     struct Message {
@@ -22,9 +22,13 @@ class CBLSInsecureBatchVerifier
         CBLSPublicKey pubKey;
     };
 
-    typedef std::map<MessageId, Message> MessageMap;
-    typedef typename MessageMap::iterator MessageMapIterator;
-    typedef std::map<SourceId, std::vector<MessageMapIterator>> MessagesBySourceMap;
+    using MessageMap = std::map<MessageId, Message>;
+    using MessageMapIterator = typename MessageMap::iterator;
+    using MessagesBySourceMap = std::map<SourceId, std::vector<MessageMapIterator>>;
+
+    bool secureVerification;
+    bool perMessageFallback;
+    size_t subBatchSize;
 
     MessageMap messages;
     MessagesBySourceMap messagesBySource;
@@ -34,15 +38,37 @@ class CBLSInsecureBatchVerifier
     std::set<MessageId> badMessages;
 
 public:
+    CBLSBatchVerifier(bool _secureVerification, bool _perMessageFallback, size_t _subBatchSize = 0) : secureVerification(_secureVerification),
+                                                                                                      perMessageFallback(_perMessageFallback),
+                                                                                                      subBatchSize(_subBatchSize)
+    {
+    }
+
     void PushMessage(const SourceId& sourceId, const MessageId& msgId, const uint256& msgHash, const CBLSSignature& sig, const CBLSPublicKey& pubKey)
     {
         assert(sig.IsValid() && pubKey.IsValid());
 
         auto it = messages.emplace(msgId, Message{msgId, msgHash, sig, pubKey}).first;
         messagesBySource[sourceId].emplace_back(it);
+
+        if (subBatchSize != 0 && messages.size() >= subBatchSize) {
+            Verify();
+            ClearMessages();
+        }
+    }
+
+    void ClearMessages()
+    {
+        messages.clear();
+        messagesBySource.clear();
+    }
+
+    size_t GetUniqueSourceCount() const
+    {
+        return messagesBySource.size();
     }
 
-    void Verify(bool perMessageFallback)
+    void Verify()
     {
         std::map<uint256, std::vector<MessageMapIterator>> byMessageHash;
 
@@ -94,7 +120,19 @@ class CBLSInsecureBatchVerifier
     }
 
 private:
-    bool VerifyBatch(const std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
+    // All Verify methods take ownership of the passed byMessageHash map and thus might modify the map. This is to avoid
+    // unnecessary copies
+
+    bool VerifyBatch(std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
+    {
+        if (secureVerification) {
+            return VerifyBatchSecure(byMessageHash);
+        } else {
+            return VerifyBatchInsecure(byMessageHash);
+        }
+    }
+
+    bool VerifyBatchInsecure(const std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
     {
         CBLSSignature aggSig;
         std::vector<uint256> msgHashes;
@@ -129,7 +167,7 @@ class CBLSInsecureBatchVerifier
                 }
             }
 
-            if (!aggSig.IsValid()) {
+            if (!aggPubKey.IsValid()) {
                 // only duplicates for this msgHash
                 continue;
             }
@@ -144,6 +182,59 @@ class CBLSInsecureBatchVerifier
 
         return aggSig.VerifyInsecureAggregated(pubKeys, msgHashes);
     }
+
+    bool VerifyBatchSecure(std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
+    {
+        // Loop until the byMessageHash map is empty, which means that all messages were verified
+        // The secure form of verification will only aggregate one message for the same message hash, even if multiple
+        // exist (signed with different keys). This avoids the rogue public key attack.
+        // This is slower than the insecure form as it requires more pairings
+        while (!byMessageHash.empty()) {
+            if (!VerifyBatchSecureStep(byMessageHash)) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    bool VerifyBatchSecureStep(std::map<uint256, std::vector<MessageMapIterator>>& byMessageHash)
+    {
+        CBLSSignature aggSig;
+        std::vector<uint256> msgHashes;
+        std::vector<CBLSPublicKey> pubKeys;
+        std::set<MessageId> dups;
+
+        msgHashes.reserve(messages.size());
+        pubKeys.reserve(messages.size());
+
+        for (auto it = byMessageHash.begin(); it != byMessageHash.end();) {
+            const auto& msgHash = it->first;
+            auto& messageIts = it->second;
+            const auto& msg = messageIts.back()->second;
+
+            if (dups.emplace(msg.msgId).second) {
+                msgHashes.emplace_back(msgHash);
+                pubKeys.emplace_back(msg.pubKey);
+
+                if (!aggSig.IsValid()) {
+                    aggSig = msg.sig;
+                } else {
+                    aggSig.AggregateInsecure(msg.sig);
+                }
+            }
+
+            messageIts.pop_back();
+            if (messageIts.empty()) {
+                it = byMessageHash.erase(it);
+            } else {
+                ++it;
+            }
+        }
+
+        assert(!msgHashes.empty());
+
+        return aggSig.VerifyInsecureAggregated(pubKeys, msgHashes);
+    }
 };
 
-#endif //PIVX_CRYPTO_BLS_BATCHVERIFIER_H
+#endif // PIVX_CRYPTO_BLS_BATCHVERIFIER_H

src/llmq/quorums_signing.cpp

@@ -354,7 +354,9 @@ void CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman)
         return;
     }
 
-    CBLSInsecureBatchVerifier<NodeId, uint256> batchVerifier;
+    // It's ok to perform insecure batched verification here as we verify against the quorum public keys, which are not
+    // craftable by individual entities, making the rogue public key attack impossible
+    CBLSBatchVerifier<NodeId, uint256> batchVerifier(false, false);
 
     size_t verifyCount = 0;
     for (auto& p : recSigsByNode) {
@@ -369,7 +371,7 @@ void CSigningManager::ProcessPendingRecoveredSigs(CConnman& connman)
     }
 
     cxxtimer::Timer verifyTimer;
-    batchVerifier.Verify(false);
+    batchVerifier.Verify();
     verifyTimer.stop();
 
     LogPrintf("llmq", "CSigningManager::%s -- verified recovered sig(s). count=%d, vt=%d, nodes=%d\n", __func__, verifyCount, verifyTimer.count(), recSigsByNode.size());

src/llmq/quorums_signing_shares.cpp

@@ -474,7 +474,9 @@ void CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
         return;
     }
 
-    CBLSInsecureBatchVerifier<NodeId, SigShareKey> batchVerifier;
+    // It's ok to perform insecure batched verification here as we verify against the quorum public key shares,
+    // which are not craftable by individual entities, making the rogue public key attack impossible
+    CBLSBatchVerifier<NodeId, SigShareKey> batchVerifier(false, true);
 
     size_t verifyCount = 0;
     for (auto& p : sigSharesByNodes) {
@@ -502,7 +504,7 @@ void CSigSharesManager::ProcessPendingSigShares(CConnman& connman)
     }
 
     cxxtimer::Timer verifyTimer;
-    batchVerifier.Verify(true);
+    batchVerifier.Verify();
     verifyTimer.stop();
 
     LogPrintf("llmq", "CSigSharesManager::%s -- verified sig shares. count=%d, vt=%d, nodes=%d\n", __func__, verifyCount, verifyTimer.count(), sigSharesByNodes.size());

src/test/bls_tests.cpp

@@ -3,10 +3,12 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include "test/test_pivx.h"
+
+#include "bls/bls_batchverifier.h"
 #include "bls/bls_ies.h"
-#include "bls/key_io.h"
 #include "bls/bls_worker.h"
 #include "bls/bls_wrapper.h"
+#include "bls/key_io.h"
 #include "random.h"
 
 #include <boost/test/unit_test.hpp>
@@ -293,4 +295,111 @@ BOOST_AUTO_TEST_CASE(bls_pk_io_tests)
     BOOST_CHECK(oppk4 == nullopt);
 }
 
+struct Message {
+    uint32_t sourceId;
+    uint32_t msgId;
+    uint256 msgHash;
+    CBLSSecretKey sk;
+    CBLSPublicKey pk;
+    CBLSSignature sig;
+    bool valid;
+};
+
+static void AddMessage(std::vector<Message>& vec, uint32_t sourceId, uint32_t msgId, uint32_t msgHash, bool valid)
+{
+    Message m;
+    m.sourceId = sourceId;
+    m.msgId = msgId;
+    *((uint32_t*)m.msgHash.begin()) = msgHash;
+    m.sk.MakeNewKey();
+    m.pk = m.sk.GetPublicKey();
+    m.sig = m.sk.Sign(m.msgHash);
+    m.valid = valid;
+
+    if (!valid) {
+        CBLSSecretKey tmp;
+        tmp.MakeNewKey();
+        m.sig = tmp.Sign(m.msgHash);
+    }
+
+    vec.emplace_back(m);
+}
+
+static void Verify(std::vector<Message>& vec, bool secureVerification, bool perMessageFallback)
+{
+    CBLSBatchVerifier<uint32_t, uint32_t> batchVerifier(secureVerification, perMessageFallback);
+
+    std::set<uint32_t> expectedBadMessages;
+    std::set<uint32_t> expectedBadSources;
+    for (auto& m : vec) {
+        if (!m.valid) {
+            expectedBadMessages.emplace(m.msgId);
+            expectedBadSources.emplace(m.sourceId);
+        }
+
+        batchVerifier.PushMessage(m.sourceId, m.msgId, m.msgHash, m.sig, m.pk);
+    }
+
+    batchVerifier.Verify();
+
+    BOOST_CHECK(batchVerifier.badSources == expectedBadSources);
+
+    if (perMessageFallback) {
+        BOOST_CHECK(batchVerifier.badMessages == expectedBadMessages);
+    } else {
+        BOOST_CHECK(batchVerifier.badMessages.empty());
+    }
+}
+
+static void Verify(std::vector<Message>& vec)
+{
+    Verify(vec, false, false);
+    Verify(vec, true, false);
+    Verify(vec, false, true);
+    Verify(vec, true, true);
+}
+
+BOOST_AUTO_TEST_CASE(batch_verifier_tests)
+{
+    std::vector<Message> msgs;
+
+    // distinct messages from distinct sources
+    AddMessage(msgs, 1, 1, 1, true);
+    AddMessage(msgs, 2, 2, 2, true);
+    AddMessage(msgs, 3, 3, 3, true);
+    Verify(msgs);
+
+    // distinct messages from same source
+    AddMessage(msgs, 4, 4, 4, true);
+    AddMessage(msgs, 4, 5, 5, true);
+    AddMessage(msgs, 4, 6, 6, true);
+    Verify(msgs);
+
+    // invalid sig
+    AddMessage(msgs, 7, 7, 7, false);
+    Verify(msgs);
+
+    // same message as before, but from another source and with valid sig
+    AddMessage(msgs, 8, 8, 7, true);
+    Verify(msgs);
+
+    // same message as before, but from another source and signed with another key
+    AddMessage(msgs, 9, 9, 7, true);
+    Verify(msgs);
+
+    msgs.clear();
+    // same message, signed by multiple keys
+    AddMessage(msgs, 1, 1, 1, true);
+    AddMessage(msgs, 1, 2, 1, true);
+    AddMessage(msgs, 1, 3, 1, true);
+    AddMessage(msgs, 2, 4, 1, true);
+    AddMessage(msgs, 2, 5, 1, true);
+    AddMessage(msgs, 2, 6, 1, true);
+    Verify(msgs);
+
+    // last message invalid from one source
+    AddMessage(msgs, 1, 7, 1, false);
+    Verify(msgs);
+}
+
 BOOST_AUTO_TEST_SUITE_END()