From 8366cc7ab330961314117215aeca6e04fa2c63a7 Mon Sep 17 00:00:00 2001 From: Navid Rahimi Date: Mon, 24 Aug 2026 09:20:12 +0000 Subject: [PATCH] LLMQ: Bound signature share verification batches --- src/llmq/quorums_signing_shares.cpp | 26 +++---- src/llmq/quorums_signing_shares.h | 11 ++- src/test/CMakeLists.txt | 1 + src/test/quorums_signing_share_tests.cpp | 91 ++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 19 deletions(-) create mode 100644 src/test/quorums_signing_share_tests.cpp diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp index 0fb2b0fc1d..c1b2b8f0ed 100644 --- a/src/llmq/quorums_signing_shares.cpp +++ b/src/llmq/quorums_signing_shares.cpp @@ -586,9 +586,9 @@ bool CSigSharesManager::PreVerifyBatchedSigShares(NodeId nodeId, const CSigShare } void CSigSharesManager::CollectPendingSigSharesToVerify( - size_t maxUniqueSessions, - std::unordered_map>& retSigShares, - std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums) + size_t maxShares, + std::unordered_map >& retSigShares, + std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums) { { LOCK(cs); @@ -596,16 +596,11 @@ void CSigSharesManager::CollectPendingSigSharesToVerify( 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, 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; diff --git a/src/llmq/quorums_signing_shares.h b/src/llmq/quorums_signing_shares.h index 53e0e1ab48..765d745b75 100644 --- a/src/llmq/quorums_signing_shares.h +++ b/src/llmq/quorums_signing_shares.h @@ -27,6 +27,8 @@ class CScheduler; namespace llmq { +struct CSigSharesVerificationTestAccess; + // typedef std::pair 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>& retSigShares, - std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums); + // Collect at most maxShares actual sig shares in randomized peer order. + void CollectPendingSigSharesToVerify(size_t maxShares, + std::unordered_map >& retSigShares, + std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums); bool ProcessPendingSigShares(CConnman& connman); void ProcessPendingSigSharesFromNode(NodeId nodeId, diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index 6408280c81..3ab3e71bf4 100644 --- a/src/test/CMakeLists.txt +++ b/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 diff --git a/src/test/quorums_signing_share_tests.cpp b/src/test/quorums_signing_share_tests.cpp new file mode 100644 index 0000000000..e16c5790d7 --- /dev/null +++ b/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 + +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 > sigSharesByNodes; + std::unordered_map, 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()