diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 61d50f0a0b08..e2458d277c7f 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -136,6 +136,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 f337d63ff19e..eed11ac36092 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -333,12 +333,9 @@ bool CSigSharesManager::ProcessMessageBatchedSigShares(const CNode& pfrom, const return true; } - if (!PreVerifySigShareQuorum(m_mn_activeman, qman, sessionInfo.quorum, sessionInfo.llmqType)) { - return true; - } - - if (!PreVerifyBatchedSigShares(sessionInfo, batchedSigShares)) { - return false; // ban node + auto verifyResult = PreVerifyBatchedSigShares(m_mn_activeman, qman, sessionInfo, batchedSigShares); + if (!verifyResult.IsSuccess()) { + return !verifyResult.should_ban; } std::vector sigSharesToProcess; @@ -432,6 +429,51 @@ bool CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s return true; } +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, + const CBatchedSigShares& batchedSigShares) +{ + if (!IsQuorumActive(session.llmqType, quorum_manager, session.quorum->qc->quorumHash)) { + // quorum is too old + 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 {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 {PreVerifyResult::MissingVerificationVector, false}; + } + + return ValidateBatchedSigSharesStructure(*session.quorum, batchedSigShares); +} + bool CSigSharesManager::CollectPendingSigSharesToVerify( size_t maxUniqueSessions, std::unordered_map>& retSigShares, std::unordered_map, CQuorumCPtr, StaticSaltedHasher>& retQuorums) diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 82c1e88de585..c1ce45607a46 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -369,6 +369,23 @@ struct PendingSignatureData { } }; +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: @@ -441,6 +458,20 @@ class CSigSharesManager : public llmq::CRecoveredSigsListener bool AsyncSignIfMember(Consensus::LLMQType llmqType, CSigningManager& sigman, const uint256& id, const uint256& msgHash, const uint256& quorumHash = uint256(), bool allowReSign = false, bool allowDiffMsgHashSigning = false) EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingSigns, !cs); + + 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; 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()