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/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index e61a12283a70..9c43fe8df0ef 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,49 @@ 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::ValidateBatchedSigSharesStructure(const CQuorum& quorum, + const CBatchedSigShares& batchedSigShares) { - retBan = false; + 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, + const CBatchedSigShares& batchedSigShares) +{ 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; - } - - if (quorumMember >= session.quorum->members.size()) { - LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__); - retBan = true; - return false; - } - if (!session.quorum->qc->validMembers[quorumMember]) { - LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__); - retBan = true; - return false; - } - } - return true; + return ValidateBatchedSigSharesStructure(*session.quorum, batchedSigShares); } bool CSigSharesManager::CollectPendingSigSharesToVerify( diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 8570c3c11435..1cddba6b6a93 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: @@ -455,6 +472,17 @@ 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); + + // 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; @@ -466,10 +494,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 bool PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, - const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares, bool& retBan); - 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 new file mode 100644 index 000000000000..bdf05f4aaa0c --- /dev/null +++ b/src/test/llmq_signing_shares_tests.cpp @@ -0,0 +1,311 @@ +// 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 + +using namespace llmq; +using namespace llmq::testutils; + +/** + * 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; + + MockQuorumBuilder(int size, const std::vector& validMembers = {}) + { + blsWorker = std::make_unique(); + const auto& params = GetLLMQParams(Consensus::LLMQType::LLMQ_TEST_V17); + + quorum = std::make_shared(params, *blsWorker); + + // Create commitment + auto qc_ptr = std::make_unique(); + qc_ptr->llmqType = params.type; + qc_ptr->quorumHash = GetTestQuorumHash(1); + + // Set valid members + if (!validMembers.empty()) { + qc_ptr->validMembers = validMembers; + } else { + qc_ptr->validMembers.resize(size, true); + } + + // 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) { + vvec.push_back(CreateRandomBLSPublicKey()); + } + quorum->SetVerificationVector(vvec); + } + + CQuorum& GetQuorum() const { return *quorum; } +}; + +// Helper to create batched sig shares +CBatchedSigShares CreateBatchedSigShares(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, BasicTestingSetup) + +// +// Test the PreVerifyBatchedResult structure +// + +BOOST_AUTO_TEST_CASE(result_structure_success) +{ + PreVerifyBatchedResult result{PreVerifyResult::Success, false}; + + BOOST_CHECK(result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::Success); + BOOST_CHECK_EQUAL(result.should_ban, false); +} + +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); +} + +BOOST_AUTO_TEST_CASE(result_structure_non_ban_errors) +{ + // 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); +} + +// +// Test ValidateBatchedSigSharesStructure - the extracted validation logic +// + +BOOST_AUTO_TEST_CASE(validate_success) +{ + MockQuorumBuilder builder(5); + auto& quorum = builder.GetQuorum(); + + // Valid batch with no duplicates, all members in bounds and valid + auto batched = CreateBatchedSigShares({0, 1, 2, 3, 4}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::Success); + BOOST_CHECK_EQUAL(result.should_ban, false); +} + +BOOST_AUTO_TEST_CASE(validate_empty_batch) +{ + MockQuorumBuilder builder(5); + auto& quorum = builder.GetQuorum(); + + // Empty batch should succeed + auto batched = CreateBatchedSigShares({}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(result.IsSuccess()); +} + +BOOST_AUTO_TEST_CASE(validate_duplicate_member_first_occurrence) +{ + MockQuorumBuilder builder(5); + auto& quorum = builder.GetQuorum(); + + // Duplicate member (0 appears twice) + auto batched = CreateBatchedSigShares({0, 1, 0, 2}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(!result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::DuplicateMember); + 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); +} + +BOOST_AUTO_TEST_CASE(validate_member_out_of_bounds) +{ + 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); +} + +BOOST_AUTO_TEST_CASE(validate_member_at_max_valid_index) +{ + 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()); +} + +BOOST_AUTO_TEST_CASE(validate_invalid_member) +{ + // Create quorum with specific valid members pattern + std::vector validMembers = {true, false, true, true, false}; + MockQuorumBuilder builder(5, validMembers); + auto& quorum = builder.GetQuorum(); + + // Member 1 is invalid (marked false in validMembers) + auto batched = CreateBatchedSigShares({0, 1, 2}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(!result.IsSuccess()); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::QuorumMemberNotValid); + BOOST_CHECK(result.should_ban); +} + +BOOST_AUTO_TEST_CASE(validate_all_members_valid) +{ + // Create quorum with specific valid members pattern + std::vector validMembers = {true, false, true, true, false}; + MockQuorumBuilder builder(5, validMembers); + auto& quorum = builder.GetQuorum(); + + // Only use valid members (0, 2, 3) + auto batched = CreateBatchedSigShares({0, 2, 3}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(result.IsSuccess()); +} + +BOOST_AUTO_TEST_CASE(validate_all_members_invalid) +{ + // Create quorum where all members are invalid + std::vector validMembers(5, false); + MockQuorumBuilder builder(5, validMembers); + auto& quorum = builder.GetQuorum(); + + // All members are invalid + auto batched = CreateBatchedSigShares({0, 1, 2}); + + auto result = CSigSharesManager::ValidateBatchedSigSharesStructure(quorum, batched); + + BOOST_CHECK(!result.IsSuccess()); + BOOST_CHECK_EQUAL(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); +} + +BOOST_AUTO_TEST_CASE(validate_error_priority_bounds_before_invalid) +{ + // 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()