LLMQ: Bound signature share verification batches

src/llmq/quorums_signing_shares.cpp

@@ -586,26 +586,21 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(NodeId nodeId, const CSigShare
 }
 
 void CSigSharesManager::CollectPendingSigSharesToVerify(
-        size_t maxUniqueSessions,
-        std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
-        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
+    size_t maxShares,
+    std::unordered_map<NodeId, std::vector<CSigShare> >& retSigShares,
+    std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums)
 {
     {
         LOCK(cs);
         if (nodeStates.empty()) {
             return;
         }
 
-        // This will iterate node states in random order and pick one sig share at a time. This avoids processing
-        // of large batches at once from the same node while other nodes also provided shares. If we wouldn't do this,
-        // other nodes would be able to poison us with a large batch with N-1 valid shares and the last one being
-        // invalid, making batch verification fail and revert to per-share verification, which in turn would slow down
-        // the whole verification process
-
-        std::unordered_set<std::pair<NodeId, uint256>, StaticSaltedHasher> uniqueSignHashes;
-        CLLMQUtils::IterateNodesRandom(nodeStates, [&]() {
-            return uniqueSignHashes.size() < maxUniqueSessions;
-        }, [&](NodeId nodeId, CSigSharesNodeState& ns) {
+        // Iterate node states in random order and pick one sig share at a time. This avoids processing large batches
+        // from one node while other nodes also provided shares. Bound the batch by actual share count so one session
+        // cannot inflate the batch and force expensive per-share fallback verification.
+        size_t sharesAdded{0};
+        CLLMQUtils::IterateNodesRandom(nodeStates, [&]() { return sharesAdded < maxShares; }, [&](NodeId nodeId, CSigSharesNodeState& ns) {
             if (ns.banned) {
                 ns.pendingIncomingSigShares.Clear();
                 return false;
@@ -617,12 +612,11 @@ void CSigSharesManager::CollectPendingSigSharesToVerify(
 
             bool alreadyHave = this->sigShares.Has(sigShare.GetKey());
             if (!alreadyHave) {
-                uniqueSignHashes.emplace(nodeId, sigShare.GetSignHash());
                 retSigShares[nodeId].emplace_back(sigShare);
+                ++sharesAdded;
             }
             ns.pendingIncomingSigShares.Erase(sigShare.GetKey());
-            return !ns.pendingIncomingSigShares.Empty();
-        }, rnd);
+            return !ns.pendingIncomingSigShares.Empty(); }, rnd);
 
         if (retSigShares.empty()) {
             return;

src/llmq/quorums_signing_shares.h

@@ -27,6 +27,8 @@ class CScheduler;
 
 namespace llmq
 {
+struct CSigSharesVerificationTestAccess;
+
 // <signHash, quorumMember>
 typedef std::pair<uint256, uint16_t> SigShareKey;
 
@@ -342,6 +344,8 @@ class CSigSharesNodeState
 
 class CSigSharesManager : public CRecoveredSigsListener
 {
+    friend struct CSigSharesVerificationTestAccess;
+
     static const int64_t SESSION_NEW_SHARES_TIMEOUT = 60;
     static const int64_t SIG_SHARE_REQUEST_TIMEOUT = 5;
 
@@ -405,9 +409,10 @@ class CSigSharesManager : public CRecoveredSigsListener
     bool VerifySigSharesInv(NodeId from, Consensus::LLMQType llmqType, const CSigSharesInv& inv);
     bool PreVerifyBatchedSigShares(NodeId nodeId, const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares, bool& retBan);
 
-    void CollectPendingSigSharesToVerify(size_t maxUniqueSessions,
-            std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
-            std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
+    // Collect at most maxShares actual sig shares in randomized peer order.
+    void CollectPendingSigSharesToVerify(size_t maxShares,
+        std::unordered_map<NodeId, std::vector<CSigShare> >& retSigShares,
+        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);
     bool ProcessPendingSigShares(CConnman& connman);
 
     void ProcessPendingSigSharesFromNode(NodeId nodeId,

src/test/CMakeLists.txt

@@ -96,6 +96,7 @@ add_executable(test_firo
   ${CMAKE_CURRENT_SOURCE_DIR}/evo_simplifiedmns_tests.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/progpow_tests.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/bls_tests.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/quorums_signing_share_tests.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/sparkmessage_tests.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/sparkname_tests.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/../libspark/test/ownership_test.cpp

src/test/quorums_signing_share_tests.cpp

@@ -0,0 +1,91 @@
+// Copyright (c) 2026 The Firo developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "llmq/quorums_signing.h"
+#include "llmq/quorums_signing_shares.h"
+#include "test/test_bitcoin.h"
+
+#include <boost/test/unit_test.hpp>
+
+namespace llmq
+{
+
+struct CSigSharesVerificationTestAccess {
+    static void AddPending(CSigSharesManager& manager, NodeId nodeId, const CSigShare& sigShare)
+    {
+        LOCK(manager.cs);
+        manager.nodeStates[nodeId].pendingIncomingSigShares.Add(sigShare.GetKey(), sigShare);
+    }
+
+    static size_t PendingCount(CSigSharesManager& manager, NodeId nodeId)
+    {
+        LOCK(manager.cs);
+        return manager.nodeStates.at(nodeId).pendingIncomingSigShares.Size();
+    }
+
+    static size_t Collect(CSigSharesManager& manager, size_t maxShares, Consensus::LLMQType llmqType, const uint256& quorumHash)
+    {
+        std::unordered_map<NodeId, std::vector<CSigShare> > sigSharesByNodes;
+        std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher> quorums;
+        quorums.emplace(std::make_pair(llmqType, quorumHash), CQuorumCPtr{});
+
+        manager.CollectPendingSigSharesToVerify(maxShares, sigSharesByNodes, quorums);
+
+        size_t count{0};
+        for (const auto& p : sigSharesByNodes) {
+            count += p.second.size();
+        }
+        return count;
+    }
+};
+
+} // namespace llmq
+
+namespace
+{
+
+llmq::CSigShare MakeSigShare(uint16_t quorumMember)
+{
+    llmq::CSigShare sigShare;
+    sigShare.llmqType = Consensus::LLMQ_400_60;
+    sigShare.quorumHash = uint256S("1");
+    sigShare.quorumMember = quorumMember;
+    sigShare.id = uint256S("2");
+    sigShare.msgHash = uint256S("3");
+    sigShare.UpdateKey();
+    return sigShare;
+}
+
+} // namespace
+
+BOOST_FIXTURE_TEST_SUITE(quorums_signing_share_tests, BasicTestingSetup)
+
+BOOST_AUTO_TEST_CASE(verification_batch_is_bounded_by_share_count)
+{
+    constexpr NodeId nodeId{1};
+    constexpr size_t maxShares{32};
+    constexpr size_t totalShares{400};
+
+    llmq::CSigSharesManager manager;
+    for (uint16_t member = 0; member < totalShares; ++member) {
+        llmq::CSigSharesVerificationTestAccess::AddPending(manager, nodeId, MakeSigShare(member));
+    }
+
+    const auto firstBatch = llmq::CSigSharesVerificationTestAccess::Collect(
+        manager, maxShares, Consensus::LLMQ_400_60, uint256S("1"));
+    BOOST_REQUIRE_EQUAL(firstBatch, maxShares);
+    BOOST_CHECK_EQUAL(llmq::CSigSharesVerificationTestAccess::PendingCount(manager, nodeId), totalShares - maxShares);
+
+    size_t collected{firstBatch};
+    while (llmq::CSigSharesVerificationTestAccess::PendingCount(manager, nodeId) != 0) {
+        const auto batch = llmq::CSigSharesVerificationTestAccess::Collect(
+            manager, maxShares, Consensus::LLMQ_400_60, uint256S("1"));
+        BOOST_REQUIRE_LE(batch, maxShares);
+        BOOST_REQUIRE_GT(batch, 0U);
+        collected += batch;
+    }
+    BOOST_CHECK_EQUAL(collected, totalShares);
+}
+
+BOOST_AUTO_TEST_SUITE_END()