From a616e7c7de35914efb6c681ba6ea45e017a90a32 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 22 Oct 2025 15:41:59 -0500 Subject: [PATCH 1/5] refactor: update PreVerifyBatchedSigShares to return structured results --- src/llmq/signing_shares.cpp | 28 ++++++++++++---------------- src/llmq/signing_shares.h | 21 +++++++++++++++++++-- 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index e61a12283a70..67a306a02687 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -443,8 +443,9 @@ bool CSigSharesManager::ProcessMessageBatchedSigShares(const CNode& pfrom, const return true; } - if (bool ban{false}; !PreVerifyBatchedSigShares(m_mn_activeman, qman, sessionInfo, batchedSigShares, ban)) { - return !ban; + auto verifyResult = PreVerifyBatchedSigShares(m_mn_activeman, qman, sessionInfo, batchedSigShares); + if (!verifyResult.IsSuccess()) { + return !verifyResult.should_ban; } std::vector sigSharesToProcess; @@ -550,46 +551,41 @@ void CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s signHash.ToString(), sigShare.getId().ToString(), sigShare.getMsgHash().ToString(), sigShare.getQuorumMember(), fromId); } -bool CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, - const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares, bool& retBan) +PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, + const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares) { - retBan = false; - if (!IsQuorumActive(session.llmqType, quorum_manager, session.quorum->qc->quorumHash)) { // quorum is too old - return false; + return {PreVerifyResult::QuorumTooOld, false}; } if (!session.quorum->IsMember(mn_activeman.GetProTxHash())) { // we're not a member so we can't verify it (we actually shouldn't have received it) - return false; + return {PreVerifyResult::NotAMember, false}; } if (!session.quorum->HasVerificationVector()) { // TODO we should allow to ask other nodes for the quorum vvec if we missed it in the DKG LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- we don't have the quorum vvec for %s, no verification possible.\n", __func__, session.quorumHash.ToString()); - return false; + return {PreVerifyResult::MissingVerificationVector, false}; } std::unordered_set dupMembers; for (const auto& [quorumMember, _] : batchedSigShares.sigShares) { if (!dupMembers.emplace(quorumMember).second) { - retBan = true; - return false; + return {PreVerifyResult::DuplicateMember, true}; } if (quorumMember >= session.quorum->members.size()) { LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__); - retBan = true; - return false; + return {PreVerifyResult::QuorumMemberOutOfBounds, true}; } if (!session.quorum->qc->validMembers[quorumMember]) { LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__); - retBan = true; - return false; + return {PreVerifyResult::QuorumMemberNotValid, true}; } } - return true; + return {PreVerifyResult::Success, false}; } bool CSigSharesManager::CollectPendingSigSharesToVerify( diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 8570c3c11435..b5bfc7602525 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -362,6 +362,23 @@ class CSignedSession int attempt{0}; }; +enum class PreVerifyResult { + Success, + QuorumTooOld, + NotAMember, + MissingVerificationVector, + DuplicateMember, + QuorumMemberOutOfBounds, + QuorumMemberNotValid +}; + +struct PreVerifyBatchedResult { + PreVerifyResult result; + bool should_ban; + + [[nodiscard]] bool IsSuccess() const { return result == PreVerifyResult::Success; } +}; + class CSigSharesManager : public llmq::CRecoveredSigsListener { private: @@ -467,8 +484,8 @@ class CSigSharesManager : public llmq::CRecoveredSigsListener void ProcessMessageSigShare(NodeId fromId, const CSigShare& sigShare) EXCLUSIVE_LOCKS_REQUIRED(!cs); static bool VerifySigSharesInv(Consensus::LLMQType llmqType, const CSigSharesInv& inv); - static bool PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, - const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares, bool& retBan); + static PreVerifyBatchedResult PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, + const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares); bool CollectPendingSigSharesToVerify( size_t maxUniqueSessions, std::unordered_map>& retSigShares, From 93cc792ccee77609b967d46bb001c9b33b5ac455 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 22 Oct 2025 15:42:23 -0500 Subject: [PATCH 2/5] test: add unit tests for LLMQ signing shares validation --- src/Makefile.test.include | 1 + src/test/llmq_signing_shares_tests.cpp | 321 +++++++++++++++++++++++++ 2 files changed, 322 insertions(+) create mode 100644 src/test/llmq_signing_shares_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index dd6dda7178c3..0ca58e453ae2 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -137,6 +137,7 @@ BITCOIN_TESTS =\ test/llmq_commitment_tests.cpp \ test/llmq_hash_tests.cpp \ test/llmq_params_tests.cpp \ + test/llmq_signing_shares_tests.cpp \ test/llmq_snapshot_tests.cpp \ test/llmq_utils_tests.cpp \ test/logging_tests.cpp \ diff --git a/src/test/llmq_signing_shares_tests.cpp b/src/test/llmq_signing_shares_tests.cpp new file mode 100644 index 000000000000..44a5cc748114 --- /dev/null +++ b/src/test/llmq_signing_shares_tests.cpp @@ -0,0 +1,321 @@ +// Copyright (c) 2025 The Dash Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +using namespace llmq; +using namespace llmq::testutils; + +// Test fixture with helper functions +struct LLMQSigningSharesTestFixture : public TestingSetup +{ + std::unique_ptr blsWorker; + + LLMQSigningSharesTestFixture() : TestingSetup() + { + blsWorker = std::make_unique(); + } + + // Helper to create a minimal test quorum + CQuorumCPtr CreateMinimalTestQuorum(int size, bool hasVerificationVector = true, + const std::vector& validMembers = {}) + { + const auto& params = GetLLMQParams(Consensus::LLMQType::LLMQ_TEST_V17); + + auto quorum = std::make_shared(params, *blsWorker); + + // Create commitment + auto qc_ptr = std::make_unique(); + qc_ptr->llmqType = params.type; + qc_ptr->quorumHash = InsecureRand256(); + + // Set valid members + if (!validMembers.empty()) { + qc_ptr->validMembers = validMembers; + } else { + qc_ptr->validMembers.resize(size, true); + } + + // Create members (empty DMN pointers are fine for our tests) + std::vector members(size, nullptr); + + quorum->Init(std::move(qc_ptr), nullptr, InsecureRand256(), members); + + // Set verification vector if requested + if (hasVerificationVector) { + std::vector vvec; + for (int i = 0; i < size; ++i) { + CBLSSecretKey sk; + sk.MakeNewKey(); + vvec.push_back(sk.GetPublicKey()); + } + quorum->SetVerificationVector(vvec); + } + + return quorum; + } + + // Helper to create test SessionInfo + CSigSharesNodeState::SessionInfo CreateTestSessionInfo(CQuorumCPtr quorum) + { + CSigSharesNodeState::SessionInfo session; + session.llmqType = quorum->params.type; + session.quorumHash = quorum->qc->quorumHash; + session.id = InsecureRand256(); + session.msgHash = InsecureRand256(); + session.quorum = quorum; + return session; + } + + // Helper to create test BatchedSigShares + CBatchedSigShares CreateTestBatchedSigShares(const std::vector& members) + { + CBatchedSigShares batched; + batched.sessionId = 1; + + for (uint16_t member : members) { + CBLSLazySignature lazySig; + batched.sigShares.emplace_back(member, lazySig); + } + + return batched; + } +}; + +BOOST_FIXTURE_TEST_SUITE(llmq_signing_shares_tests, LLMQSigningSharesTestFixture) + +// Test: Missing verification vector +BOOST_AUTO_TEST_CASE(preverify_missing_verification_vector) +{ + // Create quorum WITHOUT verification vector + auto quorum = CreateMinimalTestQuorum(3, false); + auto sessionInfo = CreateTestSessionInfo(quorum); + auto batchedSigShares = CreateTestBatchedSigShares({0, 1}); + + // Note: We can't easily test the full function because IsQuorumActive and IsMember + // require complex setup. This test verifies the data structures are created correctly. + // The missing verification vector check will be hit if the earlier checks pass. + + BOOST_CHECK(!quorum->HasVerificationVector()); + BOOST_CHECK_EQUAL(quorum->members.size(), 3); +} + +// Test: Duplicate member detection +BOOST_AUTO_TEST_CASE(preverify_duplicate_member) +{ + // Create a valid quorum + auto quorum = CreateMinimalTestQuorum(5, true); + auto sessionInfo = CreateTestSessionInfo(quorum); + + // Create batch with duplicate member (0 appears twice) + auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 0, 2}); + + // We can test the duplicate detection logic by checking the batch structure + std::unordered_set seen; + bool hasDuplicate = false; + for (const auto& [member, _] : batchedSigShares.sigShares) { + if (!seen.insert(member).second) { + hasDuplicate = true; + break; + } + } + + BOOST_CHECK(hasDuplicate); +} + +// Test: Quorum member out of bounds +BOOST_AUTO_TEST_CASE(preverify_member_out_of_bounds) +{ + // Create quorum with 5 members + auto quorum = CreateMinimalTestQuorum(5, true); + auto sessionInfo = CreateTestSessionInfo(quorum); + + // Create batch with member index out of bounds (>= 5) + auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 10}); + + // Verify that we have an out-of-bounds member + bool hasOutOfBounds = false; + for (const auto& [member, _] : batchedSigShares.sigShares) { + if (member >= quorum->members.size()) { + hasOutOfBounds = true; + break; + } + } + + BOOST_CHECK(hasOutOfBounds); + BOOST_CHECK_EQUAL(quorum->members.size(), 5); +} + +// Test: Invalid quorum member +BOOST_AUTO_TEST_CASE(preverify_invalid_quorum_member) +{ + // Create quorum with specific valid members pattern + std::vector validMembers = {true, false, true, true, false}; + auto quorum = CreateMinimalTestQuorum(5, true, validMembers); + auto sessionInfo = CreateTestSessionInfo(quorum); + + // Create batch including an invalid member (member 1 is invalid) + auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2}); + + // Verify that member 1 is marked invalid + BOOST_CHECK_EQUAL(quorum->qc->validMembers[0], true); + BOOST_CHECK_EQUAL(quorum->qc->validMembers[1], false); // Invalid! + BOOST_CHECK_EQUAL(quorum->qc->validMembers[2], true); + + // Check that we can detect the invalid member + bool hasInvalidMember = false; + for (const auto& [member, _] : batchedSigShares.sigShares) { + if (member < quorum->qc->validMembers.size() && + !quorum->qc->validMembers[member]) { + hasInvalidMember = true; + break; + } + } + + BOOST_CHECK(hasInvalidMember); +} + +// Test: Valid batch structure +BOOST_AUTO_TEST_CASE(preverify_valid_batch_structure) +{ + // Create a valid quorum + auto quorum = CreateMinimalTestQuorum(5, true); + auto sessionInfo = CreateTestSessionInfo(quorum); + + // Create a valid batch (all members exist and are unique) + auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2, 3, 4}); + + // Verify no duplicates + std::unordered_set seen; + bool hasDuplicate = false; + for (const auto& [member, _] : batchedSigShares.sigShares) { + if (!seen.insert(member).second) { + hasDuplicate = true; + break; + } + } + BOOST_CHECK(!hasDuplicate); + + // Verify all members are in bounds + bool allInBounds = true; + for (const auto& [member, _] : batchedSigShares.sigShares) { + if (member >= quorum->members.size()) { + allInBounds = false; + break; + } + } + BOOST_CHECK(allInBounds); + + // Verify all members are valid + bool allValid = true; + for (const auto& [member, _] : batchedSigShares.sigShares) { + if (member >= quorum->qc->validMembers.size() || + !quorum->qc->validMembers[member]) { + allValid = false; + break; + } + } + BOOST_CHECK(allValid); +} + +// Test: Empty batch +BOOST_AUTO_TEST_CASE(preverify_empty_batch) +{ + // Create a valid quorum + auto quorum = CreateMinimalTestQuorum(5, true); + auto sessionInfo = CreateTestSessionInfo(quorum); + + // Create an empty batch + auto batchedSigShares = CreateTestBatchedSigShares({}); + + // Empty batch should have no shares + BOOST_CHECK(batchedSigShares.sigShares.empty()); + + // Empty batch should pass all validation checks (nothing to validate) + for (const auto& share : batchedSigShares.sigShares) { + // This loop shouldn't execute + (void)share; // Suppress unused variable warning + BOOST_CHECK(false); + } +} + +// Test: Multiple duplicates +BOOST_AUTO_TEST_CASE(preverify_multiple_duplicates) +{ + auto quorum = CreateMinimalTestQuorum(10, true); + auto sessionInfo = CreateTestSessionInfo(quorum); + + // Create batch with multiple duplicates + auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2, 1, 3, 2, 4}); + + // Count duplicates + std::unordered_set seen; + int duplicateCount = 0; + for (const auto& [member, _] : batchedSigShares.sigShares) { + if (!seen.insert(member).second) { + duplicateCount++; + } + } + + BOOST_CHECK_EQUAL(duplicateCount, 2); // Members 1 and 2 appear twice each +} + +// Test: Boundary case - maximum member index +BOOST_AUTO_TEST_CASE(preverify_boundary_max_member) +{ + const int quorum_size = 10; + auto quorum = CreateMinimalTestQuorum(quorum_size, true); + auto sessionInfo = CreateTestSessionInfo(quorum); + + // Create batch with last valid member (size - 1) + auto batchedSigShares = CreateTestBatchedSigShares({0, static_cast(quorum_size - 1)}); + + // Verify max member is in bounds + for (const auto& [member, _] : batchedSigShares.sigShares) { + BOOST_CHECK(member < quorum->members.size()); + } +} + +// Test: All members invalid scenario +BOOST_AUTO_TEST_CASE(preverify_all_members_invalid) +{ + // Create quorum where all members are invalid + std::vector validMembers(5, false); + auto quorum = CreateMinimalTestQuorum(5, true, validMembers); + auto sessionInfo = CreateTestSessionInfo(quorum); + + // Create batch with any members + auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2}); + + // Verify all members are marked invalid + for (size_t i = 0; i < quorum->qc->validMembers.size(); ++i) { + BOOST_CHECK_EQUAL(quorum->qc->validMembers[i], false); + } + + // Check that all shares reference invalid members + bool allInvalid = true; + for (const auto& [member, _] : batchedSigShares.sigShares) { + if (member < quorum->qc->validMembers.size() && + quorum->qc->validMembers[member]) { + allInvalid = false; + break; + } + } + BOOST_CHECK(allInvalid); +} + +BOOST_AUTO_TEST_SUITE_END() From 9016da272f2b7abd096ee8fafa13b9620a3bbe51 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 24 Oct 2025 12:05:20 -0500 Subject: [PATCH 3/5] test: add better unit tests for PreVerifyBatchedSigShares validation logic --- src/test/llmq_signing_shares_tests.cpp | 340 +++++++++++++++---------- 1 file changed, 199 insertions(+), 141 deletions(-) diff --git a/src/test/llmq_signing_shares_tests.cpp b/src/test/llmq_signing_shares_tests.cpp index 44a5cc748114..3ea81217d198 100644 --- a/src/test/llmq_signing_shares_tests.cpp +++ b/src/test/llmq_signing_shares_tests.cpp @@ -24,37 +24,49 @@ using namespace llmq::testutils; struct LLMQSigningSharesTestFixture : public TestingSetup { std::unique_ptr blsWorker; - + CBLSSecretKey sk; + std::unique_ptr mn_activeman; + LLMQSigningSharesTestFixture() : TestingSetup() { blsWorker = std::make_unique(); + + // Create active masternode manager with a test secret key + sk.MakeNewKey(); + mn_activeman = std::make_unique(sk, *Assert(m_node.connman), m_node.dmnman); } - + // Helper to create a minimal test quorum CQuorumCPtr CreateMinimalTestQuorum(int size, bool hasVerificationVector = true, - const std::vector& validMembers = {}) + const std::vector& validMembers = {}, + bool includeOurProTxHash = false) { const auto& params = GetLLMQParams(Consensus::LLMQType::LLMQ_TEST_V17); - + auto quorum = std::make_shared(params, *blsWorker); - + // Create commitment auto qc_ptr = std::make_unique(); qc_ptr->llmqType = params.type; qc_ptr->quorumHash = InsecureRand256(); - + // Set valid members if (!validMembers.empty()) { qc_ptr->validMembers = validMembers; } else { qc_ptr->validMembers.resize(size, true); } - - // Create members (empty DMN pointers are fine for our tests) - std::vector members(size, nullptr); - + + // Create members + std::vector members; + for (int i = 0; i < size; ++i) { + // For simplicity, use nullptr members since we're testing pre-verification logic + // The actual member checking happens in IsMember() which we can't fully mock + members.push_back(nullptr); + } + quorum->Init(std::move(qc_ptr), nullptr, InsecureRand256(), members); - + // Set verification vector if requested if (hasVerificationVector) { std::vector vvec; @@ -65,10 +77,10 @@ struct LLMQSigningSharesTestFixture : public TestingSetup } quorum->SetVerificationVector(vvec); } - + return quorum; } - + // Helper to create test SessionInfo CSigSharesNodeState::SessionInfo CreateTestSessionInfo(CQuorumCPtr quorum) { @@ -80,18 +92,18 @@ struct LLMQSigningSharesTestFixture : public TestingSetup session.quorum = quorum; return session; } - + // Helper to create test BatchedSigShares CBatchedSigShares CreateTestBatchedSigShares(const std::vector& members) { CBatchedSigShares batched; batched.sessionId = 1; - + for (uint16_t member : members) { CBLSLazySignature lazySig; batched.sigShares.emplace_back(member, lazySig); } - + return batched; } }; @@ -99,42 +111,59 @@ struct LLMQSigningSharesTestFixture : public TestingSetup BOOST_FIXTURE_TEST_SUITE(llmq_signing_shares_tests, LLMQSigningSharesTestFixture) // Test: Missing verification vector +// Note: This test will likely return QuorumTooOld or NotAMember because we can't fully mock +// IsQuorumActive and IsMember. However, we can still verify the function is callable and +// returns a proper PreVerifyBatchedResult. BOOST_AUTO_TEST_CASE(preverify_missing_verification_vector) { // Create quorum WITHOUT verification vector auto quorum = CreateMinimalTestQuorum(3, false); auto sessionInfo = CreateTestSessionInfo(quorum); auto batchedSigShares = CreateTestBatchedSigShares({0, 1}); - - // Note: We can't easily test the full function because IsQuorumActive and IsMember - // require complex setup. This test verifies the data structures are created correctly. - // The missing verification vector check will be hit if the earlier checks pass. - - BOOST_CHECK(!quorum->HasVerificationVector()); - BOOST_CHECK_EQUAL(quorum->members.size(), 3); + + // Call PreVerifyBatchedSigShares - it should detect missing verification vector + // (if it gets past the earlier checks for quorum activity and membership) + auto result = CSigSharesManager::PreVerifyBatchedSigShares( + *mn_activeman, + *Assert(m_node.llmq_ctx->qman), + sessionInfo, + batchedSigShares + ); + + // We expect it to fail (not be successful) + BOOST_CHECK(!result.IsSuccess()); + // The actual result will likely be QuorumTooOld, NotAMember, or MissingVerificationVector + // depending on which check fails first } // Test: Duplicate member detection +// Since we control the batched sig shares structure, we can test this validation +// even if earlier checks might fail BOOST_AUTO_TEST_CASE(preverify_duplicate_member) { // Create a valid quorum auto quorum = CreateMinimalTestQuorum(5, true); auto sessionInfo = CreateTestSessionInfo(quorum); - + // Create batch with duplicate member (0 appears twice) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 0, 2}); - - // We can test the duplicate detection logic by checking the batch structure - std::unordered_set seen; - bool hasDuplicate = false; - for (const auto& [member, _] : batchedSigShares.sigShares) { - if (!seen.insert(member).second) { - hasDuplicate = true; - break; - } + + // Call PreVerifyBatchedSigShares + auto result = CSigSharesManager::PreVerifyBatchedSigShares( + *mn_activeman, + *Assert(m_node.llmq_ctx->qman), + sessionInfo, + batchedSigShares + ); + + // We expect failure + BOOST_CHECK(!result.IsSuccess()); + + // If we get to the duplicate check (past QuorumTooOld and NotAMember checks), + // we should get DuplicateMember with should_ban=true + if (result.result == PreVerifyResult::DuplicateMember) { + BOOST_CHECK(result.should_ban); } - - BOOST_CHECK(hasDuplicate); } // Test: Quorum member out of bounds @@ -143,21 +172,25 @@ BOOST_AUTO_TEST_CASE(preverify_member_out_of_bounds) // Create quorum with 5 members auto quorum = CreateMinimalTestQuorum(5, true); auto sessionInfo = CreateTestSessionInfo(quorum); - + // Create batch with member index out of bounds (>= 5) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 10}); - - // Verify that we have an out-of-bounds member - bool hasOutOfBounds = false; - for (const auto& [member, _] : batchedSigShares.sigShares) { - if (member >= quorum->members.size()) { - hasOutOfBounds = true; - break; - } + + // Call PreVerifyBatchedSigShares + auto result = CSigSharesManager::PreVerifyBatchedSigShares( + *mn_activeman, + *Assert(m_node.llmq_ctx->qman), + sessionInfo, + batchedSigShares + ); + + // We expect failure + BOOST_CHECK(!result.IsSuccess()); + + // If we get to the bounds check, we should get QuorumMemberOutOfBounds with should_ban=true + if (result.result == PreVerifyResult::QuorumMemberOutOfBounds) { + BOOST_CHECK(result.should_ban); } - - BOOST_CHECK(hasOutOfBounds); - BOOST_CHECK_EQUAL(quorum->members.size(), 5); } // Test: Invalid quorum member @@ -167,69 +200,57 @@ BOOST_AUTO_TEST_CASE(preverify_invalid_quorum_member) std::vector validMembers = {true, false, true, true, false}; auto quorum = CreateMinimalTestQuorum(5, true, validMembers); auto sessionInfo = CreateTestSessionInfo(quorum); - + // Create batch including an invalid member (member 1 is invalid) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2}); - - // Verify that member 1 is marked invalid - BOOST_CHECK_EQUAL(quorum->qc->validMembers[0], true); - BOOST_CHECK_EQUAL(quorum->qc->validMembers[1], false); // Invalid! - BOOST_CHECK_EQUAL(quorum->qc->validMembers[2], true); - - // Check that we can detect the invalid member - bool hasInvalidMember = false; - for (const auto& [member, _] : batchedSigShares.sigShares) { - if (member < quorum->qc->validMembers.size() && - !quorum->qc->validMembers[member]) { - hasInvalidMember = true; - break; - } + + // Call PreVerifyBatchedSigShares + auto result = CSigSharesManager::PreVerifyBatchedSigShares( + *mn_activeman, + *Assert(m_node.llmq_ctx->qman), + sessionInfo, + batchedSigShares + ); + + // We expect failure + BOOST_CHECK(!result.IsSuccess()); + + // If we get to the valid member check, we should get QuorumMemberNotValid with should_ban=true + if (result.result == PreVerifyResult::QuorumMemberNotValid) { + BOOST_CHECK(result.should_ban); } - - BOOST_CHECK(hasInvalidMember); } -// Test: Valid batch structure +// Test: Valid batch structure (though may fail early checks) BOOST_AUTO_TEST_CASE(preverify_valid_batch_structure) { // Create a valid quorum auto quorum = CreateMinimalTestQuorum(5, true); auto sessionInfo = CreateTestSessionInfo(quorum); - + // Create a valid batch (all members exist and are unique) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2, 3, 4}); - - // Verify no duplicates - std::unordered_set seen; - bool hasDuplicate = false; - for (const auto& [member, _] : batchedSigShares.sigShares) { - if (!seen.insert(member).second) { - hasDuplicate = true; - break; - } - } - BOOST_CHECK(!hasDuplicate); - - // Verify all members are in bounds - bool allInBounds = true; - for (const auto& [member, _] : batchedSigShares.sigShares) { - if (member >= quorum->members.size()) { - allInBounds = false; - break; - } - } - BOOST_CHECK(allInBounds); - - // Verify all members are valid - bool allValid = true; - for (const auto& [member, _] : batchedSigShares.sigShares) { - if (member >= quorum->qc->validMembers.size() || - !quorum->qc->validMembers[member]) { - allValid = false; - break; - } + + // Call PreVerifyBatchedSigShares + auto result = CSigSharesManager::PreVerifyBatchedSigShares( + *mn_activeman, + *Assert(m_node.llmq_ctx->qman), + sessionInfo, + batchedSigShares + ); + + // The batch structure is valid, but we may fail on QuorumTooOld or NotAMember + // This test ensures that valid batch structure doesn't cause crashes + // and that the function returns a proper result + + // Verify that at least we get a proper result type + // (not a ban-worthy failure from structure validation) + if (!result.IsSuccess()) { + // If not successful, it should be due to quorum checks, not structure validation + BOOST_CHECK(result.result == PreVerifyResult::QuorumTooOld || + result.result == PreVerifyResult::NotAMember || + result.result == PreVerifyResult::MissingVerificationVector); } - BOOST_CHECK(allValid); } // Test: Empty batch @@ -238,18 +259,25 @@ BOOST_AUTO_TEST_CASE(preverify_empty_batch) // Create a valid quorum auto quorum = CreateMinimalTestQuorum(5, true); auto sessionInfo = CreateTestSessionInfo(quorum); - + // Create an empty batch auto batchedSigShares = CreateTestBatchedSigShares({}); - - // Empty batch should have no shares - BOOST_CHECK(batchedSigShares.sigShares.empty()); - - // Empty batch should pass all validation checks (nothing to validate) - for (const auto& share : batchedSigShares.sigShares) { - // This loop shouldn't execute - (void)share; // Suppress unused variable warning - BOOST_CHECK(false); + + // Call PreVerifyBatchedSigShares + auto result = CSigSharesManager::PreVerifyBatchedSigShares( + *mn_activeman, + *Assert(m_node.llmq_ctx->qman), + sessionInfo, + batchedSigShares + ); + + // Empty batch should not trigger ban-worthy errors related to member validation + // It may fail early checks (QuorumTooOld, NotAMember), but shouldn't trigger + // DuplicateMember, QuorumMemberOutOfBounds, or QuorumMemberNotValid + if (!result.IsSuccess()) { + BOOST_CHECK(result.result != PreVerifyResult::DuplicateMember); + BOOST_CHECK(result.result != PreVerifyResult::QuorumMemberOutOfBounds); + BOOST_CHECK(result.result != PreVerifyResult::QuorumMemberNotValid); } } @@ -258,20 +286,25 @@ BOOST_AUTO_TEST_CASE(preverify_multiple_duplicates) { auto quorum = CreateMinimalTestQuorum(10, true); auto sessionInfo = CreateTestSessionInfo(quorum); - - // Create batch with multiple duplicates + + // Create batch with multiple duplicates - should fail on first duplicate found auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2, 1, 3, 2, 4}); - - // Count duplicates - std::unordered_set seen; - int duplicateCount = 0; - for (const auto& [member, _] : batchedSigShares.sigShares) { - if (!seen.insert(member).second) { - duplicateCount++; - } + + // Call PreVerifyBatchedSigShares + auto result = CSigSharesManager::PreVerifyBatchedSigShares( + *mn_activeman, + *Assert(m_node.llmq_ctx->qman), + sessionInfo, + batchedSigShares + ); + + // We expect failure + BOOST_CHECK(!result.IsSuccess()); + + // If we get to the duplicate check, it should trigger DuplicateMember with ban + if (result.result == PreVerifyResult::DuplicateMember) { + BOOST_CHECK(result.should_ban); } - - BOOST_CHECK_EQUAL(duplicateCount, 2); // Members 1 and 2 appear twice each } // Test: Boundary case - maximum member index @@ -280,13 +313,21 @@ BOOST_AUTO_TEST_CASE(preverify_boundary_max_member) const int quorum_size = 10; auto quorum = CreateMinimalTestQuorum(quorum_size, true); auto sessionInfo = CreateTestSessionInfo(quorum); - + // Create batch with last valid member (size - 1) auto batchedSigShares = CreateTestBatchedSigShares({0, static_cast(quorum_size - 1)}); - - // Verify max member is in bounds - for (const auto& [member, _] : batchedSigShares.sigShares) { - BOOST_CHECK(member < quorum->members.size()); + + // Call PreVerifyBatchedSigShares + auto result = CSigSharesManager::PreVerifyBatchedSigShares( + *mn_activeman, + *Assert(m_node.llmq_ctx->qman), + sessionInfo, + batchedSigShares + ); + + // This should not trigger QuorumMemberOutOfBounds since the max index is valid + if (!result.IsSuccess()) { + BOOST_CHECK(result.result != PreVerifyResult::QuorumMemberOutOfBounds); } } @@ -297,25 +338,42 @@ BOOST_AUTO_TEST_CASE(preverify_all_members_invalid) std::vector validMembers(5, false); auto quorum = CreateMinimalTestQuorum(5, true, validMembers); auto sessionInfo = CreateTestSessionInfo(quorum); - - // Create batch with any members + + // Create batch with any members - they're all invalid auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2}); - - // Verify all members are marked invalid - for (size_t i = 0; i < quorum->qc->validMembers.size(); ++i) { - BOOST_CHECK_EQUAL(quorum->qc->validMembers[i], false); - } - - // Check that all shares reference invalid members - bool allInvalid = true; - for (const auto& [member, _] : batchedSigShares.sigShares) { - if (member < quorum->qc->validMembers.size() && - quorum->qc->validMembers[member]) { - allInvalid = false; - break; - } + + // Call PreVerifyBatchedSigShares + auto result = CSigSharesManager::PreVerifyBatchedSigShares( + *mn_activeman, + *Assert(m_node.llmq_ctx->qman), + sessionInfo, + batchedSigShares + ); + + // We expect failure + BOOST_CHECK(!result.IsSuccess()); + + // If we get to the valid member check, should trigger QuorumMemberNotValid with ban + if (result.result == PreVerifyResult::QuorumMemberNotValid) { + BOOST_CHECK(result.should_ban); } - BOOST_CHECK(allInvalid); +} + +// Test: Result enum values and should_ban flag +BOOST_AUTO_TEST_CASE(preverify_result_structure) +{ + // Test that PreVerifyBatchedResult works correctly + PreVerifyBatchedResult success_result{PreVerifyResult::Success, false}; + BOOST_CHECK(success_result.IsSuccess()); + BOOST_CHECK(!success_result.should_ban); + + PreVerifyBatchedResult ban_result{PreVerifyResult::DuplicateMember, true}; + BOOST_CHECK(!ban_result.IsSuccess()); + BOOST_CHECK(ban_result.should_ban); + + PreVerifyBatchedResult no_ban_result{PreVerifyResult::QuorumTooOld, false}; + BOOST_CHECK(!no_ban_result.IsSuccess()); + BOOST_CHECK(!no_ban_result.should_ban); } BOOST_AUTO_TEST_SUITE_END() From 5defa8dd105a371060379c290d639c718bd4c829 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 24 Oct 2025 18:16:38 -0500 Subject: [PATCH 4/5] chore: apply-clang-format; fix test failures --- src/llmq/signing_shares.cpp | 6 +- src/llmq/signing_shares.h | 10 +-- src/test/llmq_signing_shares_tests.cpp | 87 ++++++++------------------ 3 files changed, 35 insertions(+), 68 deletions(-) diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 67a306a02687..7db7e24d8dda 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -551,8 +551,10 @@ void CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s signHash.ToString(), sigShare.getId().ToString(), sigShare.getMsgHash().ToString(), sigShare.getQuorumMember(), fromId); } -PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, - const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares) +PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, + const CQuorumManager& quorum_manager, + const CSigSharesNodeState::SessionInfo& session, + const CBatchedSigShares& batchedSigShares) { if (!IsQuorumActive(session.llmqType, quorum_manager, session.quorum->qc->quorumHash)) { // quorum is too old diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index b5bfc7602525..fad21ef4f013 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -472,6 +472,12 @@ class CSigSharesManager : public llmq::CRecoveredSigsListener void NotifyRecoveredSig(const std::shared_ptr& sig, bool proactive_relay) const EXCLUSIVE_LOCKS_REQUIRED(!cs); + static bool VerifySigSharesInv(Consensus::LLMQType llmqType, const CSigSharesInv& inv); + static PreVerifyBatchedResult PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, + const CQuorumManager& quorum_manager, + const CSigSharesNodeState::SessionInfo& session, + const CBatchedSigShares& batchedSigShares); + private: std::optional CreateSigShareForSingleMember(const CQuorum& quorum, const uint256& id, const uint256& msgHash) const; @@ -483,10 +489,6 @@ class CSigSharesManager : public llmq::CRecoveredSigsListener EXCLUSIVE_LOCKS_REQUIRED(!cs); void ProcessMessageSigShare(NodeId fromId, const CSigShare& sigShare) EXCLUSIVE_LOCKS_REQUIRED(!cs); - static bool VerifySigSharesInv(Consensus::LLMQType llmqType, const CSigSharesInv& inv); - static PreVerifyBatchedResult PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, - const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares); - bool CollectPendingSigSharesToVerify( size_t maxUniqueSessions, std::unordered_map>& retSigShares, std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums) diff --git a/src/test/llmq_signing_shares_tests.cpp b/src/test/llmq_signing_shares_tests.cpp index 3ea81217d198..efd0d0b12242 100644 --- a/src/test/llmq_signing_shares_tests.cpp +++ b/src/test/llmq_signing_shares_tests.cpp @@ -2,13 +2,14 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include #include +#include #include #include #include #include +#include #include #include #include @@ -21,13 +22,13 @@ using namespace llmq; using namespace llmq::testutils; // Test fixture with helper functions -struct LLMQSigningSharesTestFixture : public TestingSetup -{ +struct LLMQSigningSharesTestFixture : public TestingSetup { std::unique_ptr blsWorker; CBLSSecretKey sk; std::unique_ptr mn_activeman; - LLMQSigningSharesTestFixture() : TestingSetup() + LLMQSigningSharesTestFixture() : + TestingSetup(CBaseChainParams::REGTEST) { blsWorker = std::make_unique(); @@ -38,8 +39,7 @@ struct LLMQSigningSharesTestFixture : public TestingSetup // Helper to create a minimal test quorum CQuorumCPtr CreateMinimalTestQuorum(int size, bool hasVerificationVector = true, - const std::vector& validMembers = {}, - bool includeOurProTxHash = false) + const std::vector& validMembers = {}, bool includeOurProTxHash = false) { const auto& params = GetLLMQParams(Consensus::LLMQType::LLMQ_TEST_V17); @@ -123,12 +123,8 @@ BOOST_AUTO_TEST_CASE(preverify_missing_verification_vector) // Call PreVerifyBatchedSigShares - it should detect missing verification vector // (if it gets past the earlier checks for quorum activity and membership) - auto result = CSigSharesManager::PreVerifyBatchedSigShares( - *mn_activeman, - *Assert(m_node.llmq_ctx->qman), - sessionInfo, - batchedSigShares - ); + auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), + sessionInfo, batchedSigShares); // We expect it to fail (not be successful) BOOST_CHECK(!result.IsSuccess()); @@ -149,12 +145,8 @@ BOOST_AUTO_TEST_CASE(preverify_duplicate_member) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 0, 2}); // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares( - *mn_activeman, - *Assert(m_node.llmq_ctx->qman), - sessionInfo, - batchedSigShares - ); + auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), + sessionInfo, batchedSigShares); // We expect failure BOOST_CHECK(!result.IsSuccess()); @@ -177,12 +169,8 @@ BOOST_AUTO_TEST_CASE(preverify_member_out_of_bounds) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 10}); // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares( - *mn_activeman, - *Assert(m_node.llmq_ctx->qman), - sessionInfo, - batchedSigShares - ); + auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), + sessionInfo, batchedSigShares); // We expect failure BOOST_CHECK(!result.IsSuccess()); @@ -205,12 +193,8 @@ BOOST_AUTO_TEST_CASE(preverify_invalid_quorum_member) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2}); // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares( - *mn_activeman, - *Assert(m_node.llmq_ctx->qman), - sessionInfo, - batchedSigShares - ); + auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), + sessionInfo, batchedSigShares); // We expect failure BOOST_CHECK(!result.IsSuccess()); @@ -232,12 +216,8 @@ BOOST_AUTO_TEST_CASE(preverify_valid_batch_structure) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2, 3, 4}); // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares( - *mn_activeman, - *Assert(m_node.llmq_ctx->qman), - sessionInfo, - batchedSigShares - ); + auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), + sessionInfo, batchedSigShares); // The batch structure is valid, but we may fail on QuorumTooOld or NotAMember // This test ensures that valid batch structure doesn't cause crashes @@ -247,8 +227,7 @@ BOOST_AUTO_TEST_CASE(preverify_valid_batch_structure) // (not a ban-worthy failure from structure validation) if (!result.IsSuccess()) { // If not successful, it should be due to quorum checks, not structure validation - BOOST_CHECK(result.result == PreVerifyResult::QuorumTooOld || - result.result == PreVerifyResult::NotAMember || + BOOST_CHECK(result.result == PreVerifyResult::QuorumTooOld || result.result == PreVerifyResult::NotAMember || result.result == PreVerifyResult::MissingVerificationVector); } } @@ -264,12 +243,8 @@ BOOST_AUTO_TEST_CASE(preverify_empty_batch) auto batchedSigShares = CreateTestBatchedSigShares({}); // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares( - *mn_activeman, - *Assert(m_node.llmq_ctx->qman), - sessionInfo, - batchedSigShares - ); + auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), + sessionInfo, batchedSigShares); // Empty batch should not trigger ban-worthy errors related to member validation // It may fail early checks (QuorumTooOld, NotAMember), but shouldn't trigger @@ -291,12 +266,8 @@ BOOST_AUTO_TEST_CASE(preverify_multiple_duplicates) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2, 1, 3, 2, 4}); // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares( - *mn_activeman, - *Assert(m_node.llmq_ctx->qman), - sessionInfo, - batchedSigShares - ); + auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), + sessionInfo, batchedSigShares); // We expect failure BOOST_CHECK(!result.IsSuccess()); @@ -318,12 +289,8 @@ BOOST_AUTO_TEST_CASE(preverify_boundary_max_member) auto batchedSigShares = CreateTestBatchedSigShares({0, static_cast(quorum_size - 1)}); // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares( - *mn_activeman, - *Assert(m_node.llmq_ctx->qman), - sessionInfo, - batchedSigShares - ); + auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), + sessionInfo, batchedSigShares); // This should not trigger QuorumMemberOutOfBounds since the max index is valid if (!result.IsSuccess()) { @@ -343,12 +310,8 @@ BOOST_AUTO_TEST_CASE(preverify_all_members_invalid) auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2}); // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares( - *mn_activeman, - *Assert(m_node.llmq_ctx->qman), - sessionInfo, - batchedSigShares - ); + auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), + sessionInfo, batchedSigShares); // We expect failure BOOST_CHECK(!result.IsSuccess()); From 87b2e47e53c926ddae3aea93df6b4e4f4ed40084 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sun, 26 Oct 2025 22:00:38 +0300 Subject: [PATCH 5/5] refactor: extract validation logic and add proper unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the pure validation logic from PreVerifyBatchedSigShares into a new ValidateBatchedSigSharesStructure function that can be properly unit tested without external dependencies. Changes: - Add ValidateBatchedSigSharesStructure() - validates duplicates, bounds, and member validity without requiring IsQuorumActive, IsMember, or HasVerificationVector - Refactor PreVerifyBatchedSigShares() to use the extracted function - Rewrite unit tests to actually test the extracted function instead of reimplementing the logic manually Test coverage (14 tests, all passing): - Result structure tests (3): success, ban errors, non-ban errors - Validation logic tests (11): success case, empty batch, duplicate detection, bounds checking, member validity, error priority These tests provide real value by exercising the actual validation code and will catch regressions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/llmq/signing_shares.cpp | 40 ++- src/llmq/signing_shares.h | 5 + src/test/llmq_signing_shares_tests.cpp | 439 ++++++++++++------------- 3 files changed, 232 insertions(+), 252 deletions(-) diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 7db7e24d8dda..9c43fe8df0ef 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -551,6 +551,28 @@ void CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s signHash.ToString(), sigShare.getId().ToString(), sigShare.getMsgHash().ToString(), sigShare.getQuorumMember(), fromId); } +PreVerifyBatchedResult CSigSharesManager::ValidateBatchedSigSharesStructure(const CQuorum& quorum, + const CBatchedSigShares& batchedSigShares) +{ + std::unordered_set dupMembers; + + for (const auto& [quorumMember, _] : batchedSigShares.sigShares) { + if (!dupMembers.emplace(quorumMember).second) { + return {PreVerifyResult::DuplicateMember, true}; + } + + if (quorumMember >= quorum.members.size()) { + LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__); + return {PreVerifyResult::QuorumMemberOutOfBounds, true}; + } + if (!quorum.qc->validMembers[quorumMember]) { + LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__); + return {PreVerifyResult::QuorumMemberNotValid, true}; + } + } + return {PreVerifyResult::Success, false}; +} + PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, const CSigSharesNodeState::SessionInfo& session, @@ -571,23 +593,7 @@ PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiv return {PreVerifyResult::MissingVerificationVector, false}; } - std::unordered_set dupMembers; - - for (const auto& [quorumMember, _] : batchedSigShares.sigShares) { - if (!dupMembers.emplace(quorumMember).second) { - return {PreVerifyResult::DuplicateMember, true}; - } - - if (quorumMember >= session.quorum->members.size()) { - LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__); - return {PreVerifyResult::QuorumMemberOutOfBounds, true}; - } - if (!session.quorum->qc->validMembers[quorumMember]) { - LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__); - return {PreVerifyResult::QuorumMemberNotValid, true}; - } - } - return {PreVerifyResult::Success, false}; + return ValidateBatchedSigSharesStructure(*session.quorum, batchedSigShares); } bool CSigSharesManager::CollectPendingSigSharesToVerify( diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index fad21ef4f013..1cddba6b6a93 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -478,6 +478,11 @@ class CSigSharesManager : public llmq::CRecoveredSigsListener const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares); + // Validates the structure of batched sig shares (duplicates, bounds, member validity) + // This is extracted for testability - it's the pure validation logic without external dependencies + static PreVerifyBatchedResult ValidateBatchedSigSharesStructure(const CQuorum& quorum, + const CBatchedSigShares& batchedSigShares); + private: std::optional CreateSigShareForSingleMember(const CQuorum& quorum, const uint256& id, const uint256& msgHash) const; diff --git a/src/test/llmq_signing_shares_tests.cpp b/src/test/llmq_signing_shares_tests.cpp index efd0d0b12242..bdf05f4aaa0c 100644 --- a/src/test/llmq_signing_shares_tests.cpp +++ b/src/test/llmq_signing_shares_tests.cpp @@ -9,46 +9,44 @@ #include #include #include -#include #include #include -#include #include -#include #include using namespace llmq; using namespace llmq::testutils; -// Test fixture with helper functions -struct LLMQSigningSharesTestFixture : public TestingSetup { +/** + * Unit tests for LLMQ signature share validation logic + * + * These tests verify ValidateBatchedSigSharesStructure(), which contains the pure + * validation logic extracted from PreVerifyBatchedSigShares() for testability. + * + * Tests cover: + * - Duplicate member detection + * - Member index bounds checking + * - Invalid member validation + * - Result structure correctness + */ + +// Helper to create a mock quorum for testing +struct MockQuorumBuilder { + std::shared_ptr quorum; std::unique_ptr blsWorker; - CBLSSecretKey sk; - std::unique_ptr mn_activeman; - LLMQSigningSharesTestFixture() : - TestingSetup(CBaseChainParams::REGTEST) + MockQuorumBuilder(int size, const std::vector& validMembers = {}) { blsWorker = std::make_unique(); - - // Create active masternode manager with a test secret key - sk.MakeNewKey(); - mn_activeman = std::make_unique(sk, *Assert(m_node.connman), m_node.dmnman); - } - - // Helper to create a minimal test quorum - CQuorumCPtr CreateMinimalTestQuorum(int size, bool hasVerificationVector = true, - const std::vector& validMembers = {}, bool includeOurProTxHash = false) - { const auto& params = GetLLMQParams(Consensus::LLMQType::LLMQ_TEST_V17); - auto quorum = std::make_shared(params, *blsWorker); + quorum = std::make_shared(params, *blsWorker); // Create commitment auto qc_ptr = std::make_unique(); qc_ptr->llmqType = params.type; - qc_ptr->quorumHash = InsecureRand256(); + qc_ptr->quorumHash = GetTestQuorumHash(1); // Set valid members if (!validMembers.empty()) { @@ -57,286 +55,257 @@ struct LLMQSigningSharesTestFixture : public TestingSetup { qc_ptr->validMembers.resize(size, true); } - // Create members - std::vector members; + // Create placeholder member list (needed for size checks) + std::vector members(size, nullptr); + + quorum->Init(std::move(qc_ptr), nullptr, GetTestBlockHash(1), members); + + // Add verification vector + std::vector vvec; for (int i = 0; i < size; ++i) { - // For simplicity, use nullptr members since we're testing pre-verification logic - // The actual member checking happens in IsMember() which we can't fully mock - members.push_back(nullptr); + vvec.push_back(CreateRandomBLSPublicKey()); } + quorum->SetVerificationVector(vvec); + } - quorum->Init(std::move(qc_ptr), nullptr, InsecureRand256(), members); - - // Set verification vector if requested - if (hasVerificationVector) { - std::vector vvec; - for (int i = 0; i < size; ++i) { - CBLSSecretKey sk; - sk.MakeNewKey(); - vvec.push_back(sk.GetPublicKey()); - } - quorum->SetVerificationVector(vvec); - } + CQuorum& GetQuorum() const { return *quorum; } +}; - return quorum; - } +// Helper to create batched sig shares +CBatchedSigShares CreateBatchedSigShares(const std::vector& members) +{ + CBatchedSigShares batched; + batched.sessionId = 1; - // Helper to create test SessionInfo - CSigSharesNodeState::SessionInfo CreateTestSessionInfo(CQuorumCPtr quorum) - { - CSigSharesNodeState::SessionInfo session; - session.llmqType = quorum->params.type; - session.quorumHash = quorum->qc->quorumHash; - session.id = InsecureRand256(); - session.msgHash = InsecureRand256(); - session.quorum = quorum; - return session; + for (uint16_t member : members) { + CBLSLazySignature lazySig; + batched.sigShares.emplace_back(member, lazySig); } - // Helper to create test BatchedSigShares - CBatchedSigShares CreateTestBatchedSigShares(const std::vector& members) - { - CBatchedSigShares batched; - batched.sessionId = 1; - - for (uint16_t member : members) { - CBLSLazySignature lazySig; - batched.sigShares.emplace_back(member, lazySig); - } + return batched; +} - return batched; - } -}; +BOOST_FIXTURE_TEST_SUITE(llmq_signing_shares_tests, BasicTestingSetup) -BOOST_FIXTURE_TEST_SUITE(llmq_signing_shares_tests, LLMQSigningSharesTestFixture) +// +// Test the PreVerifyBatchedResult structure +// -// Test: Missing verification vector -// Note: This test will likely return QuorumTooOld or NotAMember because we can't fully mock -// IsQuorumActive and IsMember. However, we can still verify the function is callable and -// returns a proper PreVerifyBatchedResult. -BOOST_AUTO_TEST_CASE(preverify_missing_verification_vector) +BOOST_AUTO_TEST_CASE(result_structure_success) { - // Create quorum WITHOUT verification vector - auto quorum = CreateMinimalTestQuorum(3, false); - auto sessionInfo = CreateTestSessionInfo(quorum); - auto batchedSigShares = CreateTestBatchedSigShares({0, 1}); + PreVerifyBatchedResult result{PreVerifyResult::Success, false}; - // Call PreVerifyBatchedSigShares - it should detect missing verification vector - // (if it gets past the earlier checks for quorum activity and membership) - auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), - sessionInfo, batchedSigShares); + BOOST_CHECK(result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::Success); + BOOST_CHECK_EQUAL(result.should_ban, false); +} - // We expect it to fail (not be successful) - BOOST_CHECK(!result.IsSuccess()); - // The actual result will likely be QuorumTooOld, NotAMember, or MissingVerificationVector - // depending on which check fails first +BOOST_AUTO_TEST_CASE(result_structure_ban_errors) +{ + // Test ban-worthy errors + PreVerifyBatchedResult dup{PreVerifyResult::DuplicateMember, true}; + BOOST_CHECK(!dup.IsSuccess()); + BOOST_CHECK(dup.should_ban); + + PreVerifyBatchedResult bounds{PreVerifyResult::QuorumMemberOutOfBounds, true}; + BOOST_CHECK(!bounds.IsSuccess()); + BOOST_CHECK(bounds.should_ban); + + PreVerifyBatchedResult invalid{PreVerifyResult::QuorumMemberNotValid, true}; + BOOST_CHECK(!invalid.IsSuccess()); + BOOST_CHECK(invalid.should_ban); } -// Test: Duplicate member detection -// Since we control the batched sig shares structure, we can test this validation -// even if earlier checks might fail -BOOST_AUTO_TEST_CASE(preverify_duplicate_member) +BOOST_AUTO_TEST_CASE(result_structure_non_ban_errors) { - // Create a valid quorum - auto quorum = CreateMinimalTestQuorum(5, true); - auto sessionInfo = CreateTestSessionInfo(quorum); + // Test non-ban errors + PreVerifyBatchedResult old{PreVerifyResult::QuorumTooOld, false}; + BOOST_CHECK(!old.IsSuccess()); + BOOST_CHECK(!old.should_ban); + + PreVerifyBatchedResult not_member{PreVerifyResult::NotAMember, false}; + BOOST_CHECK(!not_member.IsSuccess()); + BOOST_CHECK(!not_member.should_ban); + + PreVerifyBatchedResult no_vvec{PreVerifyResult::MissingVerificationVector, false}; + BOOST_CHECK(!no_vvec.IsSuccess()); + BOOST_CHECK(!no_vvec.should_ban); +} - // Create batch with duplicate member (0 appears twice) - auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 0, 2}); +// +// Test ValidateBatchedSigSharesStructure - the extracted validation logic +// - // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), - sessionInfo, batchedSigShares); +BOOST_AUTO_TEST_CASE(validate_success) +{ + MockQuorumBuilder builder(5); + auto& quorum = builder.GetQuorum(); - // We expect failure - BOOST_CHECK(!result.IsSuccess()); + // Valid batch with no duplicates, all members in bounds and valid + auto batched = CreateBatchedSigShares({0, 1, 2, 3, 4}); - // If we get to the duplicate check (past QuorumTooOld and NotAMember checks), - // we should get DuplicateMember with should_ban=true - if (result.result == PreVerifyResult::DuplicateMember) { - BOOST_CHECK(result.should_ban); - } + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::Success); + BOOST_CHECK_EQUAL(result.should_ban, false); } -// Test: Quorum member out of bounds -BOOST_AUTO_TEST_CASE(preverify_member_out_of_bounds) +BOOST_AUTO_TEST_CASE(validate_empty_batch) { - // Create quorum with 5 members - auto quorum = CreateMinimalTestQuorum(5, true); - auto sessionInfo = CreateTestSessionInfo(quorum); + MockQuorumBuilder builder(5); + auto& quorum = builder.GetQuorum(); - // Create batch with member index out of bounds (>= 5) - auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 10}); + // Empty batch should succeed + auto batched = CreateBatchedSigShares({}); - // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), - sessionInfo, batchedSigShares); + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); - // We expect failure - BOOST_CHECK(!result.IsSuccess()); - - // If we get to the bounds check, we should get QuorumMemberOutOfBounds with should_ban=true - if (result.result == PreVerifyResult::QuorumMemberOutOfBounds) { - BOOST_CHECK(result.should_ban); - } + BOOST_CHECK(result.IsSuccess()); } -// Test: Invalid quorum member -BOOST_AUTO_TEST_CASE(preverify_invalid_quorum_member) +BOOST_AUTO_TEST_CASE(validate_duplicate_member_first_occurrence) { - // Create quorum with specific valid members pattern - std::vector validMembers = {true, false, true, true, false}; - auto quorum = CreateMinimalTestQuorum(5, true, validMembers); - auto sessionInfo = CreateTestSessionInfo(quorum); + MockQuorumBuilder builder(5); + auto& quorum = builder.GetQuorum(); - // Create batch including an invalid member (member 1 is invalid) - auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2}); + // Duplicate member (0 appears twice) + auto batched = CreateBatchedSigShares({0, 1, 0, 2}); - // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), - sessionInfo, batchedSigShares); + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); - // We expect failure BOOST_CHECK(!result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::DuplicateMember); + BOOST_CHECK(result.should_ban); +} - // If we get to the valid member check, we should get QuorumMemberNotValid with should_ban=true - if (result.result == PreVerifyResult::QuorumMemberNotValid) { - BOOST_CHECK(result.should_ban); - } +BOOST_AUTO_TEST_CASE(validate_duplicate_member_multiple) +{ + MockQuorumBuilder builder(10); + auto& quorum = builder.GetQuorum(); + + // Multiple duplicates - should catch first one (member 1) + auto batched = CreateBatchedSigShares({0, 1, 2, 1, 3, 2, 4}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(!result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::DuplicateMember); + BOOST_CHECK(result.should_ban); } -// Test: Valid batch structure (though may fail early checks) -BOOST_AUTO_TEST_CASE(preverify_valid_batch_structure) +BOOST_AUTO_TEST_CASE(validate_member_out_of_bounds) { - // Create a valid quorum - auto quorum = CreateMinimalTestQuorum(5, true); - auto sessionInfo = CreateTestSessionInfo(quorum); - - // Create a valid batch (all members exist and are unique) - auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2, 3, 4}); - - // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), - sessionInfo, batchedSigShares); - - // The batch structure is valid, but we may fail on QuorumTooOld or NotAMember - // This test ensures that valid batch structure doesn't cause crashes - // and that the function returns a proper result - - // Verify that at least we get a proper result type - // (not a ban-worthy failure from structure validation) - if (!result.IsSuccess()) { - // If not successful, it should be due to quorum checks, not structure validation - BOOST_CHECK(result.result == PreVerifyResult::QuorumTooOld || result.result == PreVerifyResult::NotAMember || - result.result == PreVerifyResult::MissingVerificationVector); - } + const int quorum_size = 5; + MockQuorumBuilder builder(quorum_size); + auto& quorum = builder.GetQuorum(); + + // Member 10 is out of bounds (>= 5) + auto batched = CreateBatchedSigShares({0, 1, 10}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(!result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::QuorumMemberOutOfBounds); + BOOST_CHECK(result.should_ban); } -// Test: Empty batch -BOOST_AUTO_TEST_CASE(preverify_empty_batch) +BOOST_AUTO_TEST_CASE(validate_member_at_max_valid_index) { - // Create a valid quorum - auto quorum = CreateMinimalTestQuorum(5, true); - auto sessionInfo = CreateTestSessionInfo(quorum); - - // Create an empty batch - auto batchedSigShares = CreateTestBatchedSigShares({}); - - // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), - sessionInfo, batchedSigShares); - - // Empty batch should not trigger ban-worthy errors related to member validation - // It may fail early checks (QuorumTooOld, NotAMember), but shouldn't trigger - // DuplicateMember, QuorumMemberOutOfBounds, or QuorumMemberNotValid - if (!result.IsSuccess()) { - BOOST_CHECK(result.result != PreVerifyResult::DuplicateMember); - BOOST_CHECK(result.result != PreVerifyResult::QuorumMemberOutOfBounds); - BOOST_CHECK(result.result != PreVerifyResult::QuorumMemberNotValid); - } + const int quorum_size = 10; + MockQuorumBuilder builder(quorum_size); + auto& quorum = builder.GetQuorum(); + + // Max valid index is size - 1, which should succeed + auto batched = CreateBatchedSigShares({0, static_cast(quorum_size - 1)}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(result.IsSuccess()); } -// Test: Multiple duplicates -BOOST_AUTO_TEST_CASE(preverify_multiple_duplicates) +BOOST_AUTO_TEST_CASE(validate_invalid_member) { - auto quorum = CreateMinimalTestQuorum(10, true); - auto sessionInfo = CreateTestSessionInfo(quorum); + // Create quorum with specific valid members pattern + std::vector validMembers = {true, false, true, true, false}; + MockQuorumBuilder builder(5, validMembers); + auto& quorum = builder.GetQuorum(); - // Create batch with multiple duplicates - should fail on first duplicate found - auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2, 1, 3, 2, 4}); + // Member 1 is invalid (marked false in validMembers) + auto batched = CreateBatchedSigShares({0, 1, 2}); - // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), - sessionInfo, batchedSigShares); + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); - // We expect failure BOOST_CHECK(!result.IsSuccess()); - - // If we get to the duplicate check, it should trigger DuplicateMember with ban - if (result.result == PreVerifyResult::DuplicateMember) { - BOOST_CHECK(result.should_ban); - } + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::QuorumMemberNotValid); + BOOST_CHECK(result.should_ban); } -// Test: Boundary case - maximum member index -BOOST_AUTO_TEST_CASE(preverify_boundary_max_member) +BOOST_AUTO_TEST_CASE(validate_all_members_valid) { - const int quorum_size = 10; - auto quorum = CreateMinimalTestQuorum(quorum_size, true); - auto sessionInfo = CreateTestSessionInfo(quorum); + // Create quorum with specific valid members pattern + std::vector validMembers = {true, false, true, true, false}; + MockQuorumBuilder builder(5, validMembers); + auto& quorum = builder.GetQuorum(); - // Create batch with last valid member (size - 1) - auto batchedSigShares = CreateTestBatchedSigShares({0, static_cast(quorum_size - 1)}); + // Only use valid members (0, 2, 3) + auto batched = CreateBatchedSigShares({0, 2, 3}); - // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), - sessionInfo, batchedSigShares); + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); - // This should not trigger QuorumMemberOutOfBounds since the max index is valid - if (!result.IsSuccess()) { - BOOST_CHECK(result.result != PreVerifyResult::QuorumMemberOutOfBounds); - } + BOOST_CHECK(result.IsSuccess()); } -// Test: All members invalid scenario -BOOST_AUTO_TEST_CASE(preverify_all_members_invalid) +BOOST_AUTO_TEST_CASE(validate_all_members_invalid) { // Create quorum where all members are invalid std::vector validMembers(5, false); - auto quorum = CreateMinimalTestQuorum(5, true, validMembers); - auto sessionInfo = CreateTestSessionInfo(quorum); + MockQuorumBuilder builder(5, validMembers); + auto& quorum = builder.GetQuorum(); - // Create batch with any members - they're all invalid - auto batchedSigShares = CreateTestBatchedSigShares({0, 1, 2}); + // All members are invalid + auto batched = CreateBatchedSigShares({0, 1, 2}); - // Call PreVerifyBatchedSigShares - auto result = CSigSharesManager::PreVerifyBatchedSigShares(*mn_activeman, *Assert(m_node.llmq_ctx->qman), - sessionInfo, batchedSigShares); + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); - // We expect failure BOOST_CHECK(!result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::QuorumMemberNotValid); + BOOST_CHECK(result.should_ban); +} - // If we get to the valid member check, should trigger QuorumMemberNotValid with ban - if (result.result == PreVerifyResult::QuorumMemberNotValid) { - BOOST_CHECK(result.should_ban); - } +BOOST_AUTO_TEST_CASE(validate_error_priority_duplicate_before_invalid) +{ + // Verify that duplicate check happens before validity check in the same iteration + std::vector validMembers = {true, false, true, false, true}; + MockQuorumBuilder builder(5, validMembers); + auto& quorum = builder.GetQuorum(); + + // Member 2 appears twice (at positions 0 and 1) + // This ensures duplicate is detected before we process any invalid members + auto batched = CreateBatchedSigShares({2, 2, 1}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(!result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::DuplicateMember); + BOOST_CHECK(result.should_ban); } -// Test: Result enum values and should_ban flag -BOOST_AUTO_TEST_CASE(preverify_result_structure) +BOOST_AUTO_TEST_CASE(validate_error_priority_bounds_before_invalid) { - // Test that PreVerifyBatchedResult works correctly - PreVerifyBatchedResult success_result{PreVerifyResult::Success, false}; - BOOST_CHECK(success_result.IsSuccess()); - BOOST_CHECK(!success_result.should_ban); - - PreVerifyBatchedResult ban_result{PreVerifyResult::DuplicateMember, true}; - BOOST_CHECK(!ban_result.IsSuccess()); - BOOST_CHECK(ban_result.should_ban); - - PreVerifyBatchedResult no_ban_result{PreVerifyResult::QuorumTooOld, false}; - BOOST_CHECK(!no_ban_result.IsSuccess()); - BOOST_CHECK(!no_ban_result.should_ban); + // Verify that bounds check happens before validity check + std::vector validMembers = {true, false, true}; + MockQuorumBuilder builder(3, validMembers); + auto& quorum = builder.GetQuorum(); + + // Member 10 is out of bounds, comes before member 1 which is invalid + auto batched = CreateBatchedSigShares({0, 10}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(!result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::QuorumMemberOutOfBounds); + BOOST_CHECK(result.should_ban); } BOOST_AUTO_TEST_SUITE_END()