diff --git a/src/Makefile.test.include b/src/Makefile.test.include index e98111bed99c..3e673a9e36ba 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -140,6 +140,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 bec02769c7ab..a265a8c910ce 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -331,47 +331,52 @@ bool CSigSharesManager::ProcessMessageSigShares(const CNode& pfrom, const CSigSh return true; } -// Failure is not issue, we should not ban node -static bool PreVerifySigShareQuorum(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, - const CQuorumCPtr& quorum, Consensus::LLMQType llmqType) +// Only returns benign failures (never ban): we simply can't verify sig shares for this quorum +static PreVerifyResult PreVerifySigShareQuorum(const uint256& our_protx_hash, bool is_quorum_active, const CQuorum& quorum) { - if (!IsQuorumActive(llmqType, quorum_manager, quorum->qc->quorumHash)) { + if (!is_quorum_active) { // quorum is too old - return false; + return PreVerifyResult::QuorumTooOld; } - if (!quorum->IsMember(mn_activeman.GetProTxHash())) { + if (!quorum.IsMember(our_protx_hash)) { // we're not a member so we can't verify it (we actually shouldn't have received it) - return false; + return PreVerifyResult::NotAMember; } - if (!quorum->HasVerificationVector()) { + if (!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, "%s -- we don't have the quorum vvec for %s, no verification possible.\n", __func__, - quorum->qc->quorumHash.ToString()); - return false; + quorum.qc->quorumHash.ToString()); + return PreVerifyResult::MissingVerificationVector; } - return true; + return PreVerifyResult::Success; } -// Ban node if PreVerifyBatchedSigShares failed -bool PreVerifyBatchedSigShares(const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares) +PreVerifyResult PreVerifyBatchedSigShares(const uint256& our_protx_hash, bool is_quorum_active, const CQuorum& quorum, + const CBatchedSigShares& batchedSigShares) { + if (const PreVerifyResult res = PreVerifySigShareQuorum(our_protx_hash, is_quorum_active, quorum); + res != PreVerifyResult::Success) { + return res; + } + std::unordered_set dupMembers; for (const auto& [quorumMember, _] : batchedSigShares.sigShares) { if (!dupMembers.emplace(quorumMember).second) { - return false; + LogPrint(BCLog::LLMQ_SIGS, "%s -- duplicate quorumMember\n", __func__); + return PreVerifyResult::DuplicateMember; } - if (quorumMember >= session.quorum->members.size()) { + if (quorumMember >= quorum.members.size()) { LogPrint(BCLog::LLMQ_SIGS, "%s -- quorumMember out of bounds\n", __func__); - return false; + return PreVerifyResult::MemberOutOfBounds; } - if (!session.quorum->qc->validMembers[quorumMember]) { + if (!quorum.qc->validMembers[quorumMember]) { LogPrint(BCLog::LLMQ_SIGS, "%s -- quorumMember not valid\n", __func__); - return false; + return PreVerifyResult::MemberNotValid; } } - return true; + return PreVerifyResult::Success; } bool CSigSharesManager::ProcessMessageBatchedSigShares(const CNode& pfrom, const CBatchedSigShares& batchedSigShares) @@ -381,12 +386,13 @@ 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 + const PreVerifyResult preVerifyResult = PreVerifyBatchedSigShares(m_mn_activeman.GetProTxHash(), + IsQuorumActive(sessionInfo.llmqType, qman, + sessionInfo.quorum->qc->quorumHash), + *sessionInfo.quorum, batchedSigShares); + if (preVerifyResult != PreVerifyResult::Success) { + // benign failures are ignored, malformed peer data leads to a ban + return !ShouldBan(preVerifyResult); } std::vector sigSharesToProcess; @@ -438,8 +444,10 @@ bool CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s if (!quorum) { return true; } - if (!PreVerifySigShareQuorum(m_mn_activeman, qman, quorum, sigShare.getLlmqType())) { - return true; + if (const PreVerifyResult res = PreVerifySigShareQuorum( + m_mn_activeman.GetProTxHash(), IsQuorumActive(sigShare.getLlmqType(), qman, quorum->qc->quorumHash), *quorum); + res != PreVerifyResult::Success) { + return !ShouldBan(res); } if (sigShare.getQuorumMember() >= quorum->members.size()) { diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 332f553fc895..c1db54b7de23 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -159,6 +159,55 @@ class CBatchedSigShares [[nodiscard]] std::string ToInvString() const; }; +/** + * Outcome of pre-verifying sig shares received from a peer before scheduling + * the expensive BLS verification. + * + * The first group of failures is benign: the local node simply cannot verify + * shares for this quorum (e.g. the quorum is not active anymore, the node is + * not a member or it missed the quorum verification vector in the DKG). The + * peer did nothing provably wrong, so these must never lead to a ban. + * + * The second group means the peer sent structurally malformed data that no + * honest node would produce, so the peer should be banned (see ShouldBan()). + */ +enum class PreVerifyResult { + Success, + // benign: we can't verify shares for this quorum, never ban + QuorumTooOld, + NotAMember, + MissingVerificationVector, + // malformed peer data, ban + DuplicateMember, + MemberOutOfBounds, + MemberNotValid, +}; + +[[nodiscard]] constexpr bool ShouldBan(PreVerifyResult result) +{ + switch (result) { + case PreVerifyResult::DuplicateMember: + case PreVerifyResult::MemberOutOfBounds: + case PreVerifyResult::MemberNotValid: + return true; + case PreVerifyResult::Success: + case PreVerifyResult::QuorumTooOld: + case PreVerifyResult::NotAMember: + case PreVerifyResult::MissingVerificationVector: + return false; + } // no default case, so the compiler can warn about missing cases + return false; +} + +/** + * Pre-verify a batch of sig shares (QBSIGSHARES) before scheduling the + * expensive BLS verification. our_protx_hash is the local masternode's proTx + * hash and is_quorum_active is resolved by the caller (see IsQuorumActive()); + * both are passed as plain values to keep this function unit-testable. + */ +[[nodiscard]] PreVerifyResult PreVerifyBatchedSigShares(const uint256& our_protx_hash, bool is_quorum_active, + const CQuorum& quorum, const CBatchedSigShares& batchedSigShares); + /** * Two-level (signHash -> quorumMember) map with a running entry count, so Size() is O(1) * instead of a fold over all sign hash buckets. All structural mutations go through the diff --git a/src/test/llmq_signing_shares_tests.cpp b/src/test/llmq_signing_shares_tests.cpp new file mode 100644 index 000000000000..8e12413b3e77 --- /dev/null +++ b/src/test/llmq_signing_shares_tests.cpp @@ -0,0 +1,180 @@ +// Copyright (c) 2026 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 + +#include + +#include +#include + +using namespace llmq; +using namespace llmq::testutils; + +// The ShouldBan() mapping is part of the API contract: only structurally +// malformed peer data may lead to a ban, benign inability to verify never does. +static_assert(!ShouldBan(PreVerifyResult::Success)); +static_assert(!ShouldBan(PreVerifyResult::QuorumTooOld)); +static_assert(!ShouldBan(PreVerifyResult::NotAMember)); +static_assert(!ShouldBan(PreVerifyResult::MissingVerificationVector)); +static_assert(ShouldBan(PreVerifyResult::DuplicateMember)); +static_assert(ShouldBan(PreVerifyResult::MemberOutOfBounds)); +static_assert(ShouldBan(PreVerifyResult::MemberNotValid)); + +namespace { + +struct SigSharesPreVerifyFixture : public BasicTestingSetup { + const Consensus::LLMQParams& llmq_params{GetLLMQParams(Consensus::LLMQType::LLMQ_TEST)}; + + // Never started: preverification must not perform any BLS work + CBLSWorker bls_worker; + CBlockIndex quorum_base_block_index; + + // proTxHash values are kept disjoint from GetTestQuorumHash() values + static uint256 ProTxHashOfMember(size_t member_idx) { return ArithToUint256(arith_uint256(1000 + member_idx)); } + static uint256 NonMemberProTxHash() { return ArithToUint256(arith_uint256(999)); } + + // Builds a quorum of llmq_params.size members whose proTxHashes are + // ProTxHashOfMember(0..size-1), all valid unless listed in invalid_members + CQuorumCPtr BuildQuorum(bool with_verification_vector = true, const std::vector& invalid_members = {}) + { + auto qc = std::make_unique(); + qc->llmqType = llmq_params.type; + qc->quorumHash = GetTestQuorumHash(1); + qc->validMembers.assign(llmq_params.size, true); + for (size_t idx : invalid_members) { + qc->validMembers.at(idx) = false; + } + + std::vector members; + for (size_t i = 0; i < static_cast(llmq_params.size); i++) { + auto dmn = std::make_shared(/*_internalId=*/i + 1); + dmn->proTxHash = ProTxHashOfMember(i); + members.push_back(std::move(dmn)); + } + + auto quorum = std::make_shared(llmq_params, bls_worker, std::move(qc), &quorum_base_block_index, + /*_minedBlockHash=*/uint256(), members); + if (with_verification_vector) { + // The pointer overload installs the vvec as-is; HasVerificationVector() only + // null-checks it, so an empty vector is enough here. The value overload would + // reject it against qc->quorumVvecHash. + quorum->SetVerificationVector(std::make_shared>()); + } + return quorum; + } + + // Preverification only looks at the member indexes, the signatures are never inspected + static CBatchedSigShares MakeBatch(const std::vector& member_indexes) + { + CBatchedSigShares batch; + for (uint16_t idx : member_indexes) { + batch.sigShares.emplace_back(idx, CBLSLazySignature()); + } + return batch; + } +}; + +} // namespace + +BOOST_FIXTURE_TEST_SUITE(llmq_signing_shares_tests, SigSharesPreVerifyFixture) + +BOOST_AUTO_TEST_CASE(preverify_success) +{ + const auto quorum = BuildQuorum(); + const auto res = PreVerifyBatchedSigShares(ProTxHashOfMember(0), /*is_quorum_active=*/true, *quorum, + MakeBatch({0, 1, 2})); + BOOST_CHECK(res == PreVerifyResult::Success); + BOOST_CHECK(!ShouldBan(res)); +} + +BOOST_AUTO_TEST_CASE(preverify_empty_batch_is_not_malformed) +{ + const auto quorum = BuildQuorum(); + const auto res = PreVerifyBatchedSigShares(ProTxHashOfMember(0), /*is_quorum_active=*/true, *quorum, MakeBatch({})); + BOOST_CHECK(res == PreVerifyResult::Success); + BOOST_CHECK(!ShouldBan(res)); +} + +BOOST_AUTO_TEST_CASE(preverify_quorum_too_old_no_ban) +{ + const auto quorum = BuildQuorum(); + const auto res = PreVerifyBatchedSigShares(ProTxHashOfMember(0), /*is_quorum_active=*/false, *quorum, + MakeBatch({0, 1, 2})); + BOOST_CHECK(res == PreVerifyResult::QuorumTooOld); + BOOST_CHECK(!ShouldBan(res)); + + // Benign prechecks run before the structural checks: a batch that would + // otherwise be ban-worthy must still not lead to a ban if we can't verify it + const auto res_malformed = PreVerifyBatchedSigShares(ProTxHashOfMember(0), /*is_quorum_active=*/false, *quorum, + MakeBatch({0, 0})); + BOOST_CHECK(res_malformed == PreVerifyResult::QuorumTooOld); + BOOST_CHECK(!ShouldBan(res_malformed)); +} + +BOOST_AUTO_TEST_CASE(preverify_not_a_member_no_ban) +{ + const auto quorum = BuildQuorum(); + const auto res = PreVerifyBatchedSigShares(NonMemberProTxHash(), /*is_quorum_active=*/true, *quorum, + MakeBatch({0, 1, 2})); + BOOST_CHECK(res == PreVerifyResult::NotAMember); + BOOST_CHECK(!ShouldBan(res)); +} + +BOOST_AUTO_TEST_CASE(preverify_missing_verification_vector_no_ban) +{ + const auto quorum = BuildQuorum(/*with_verification_vector=*/false); + const auto res = PreVerifyBatchedSigShares(ProTxHashOfMember(0), /*is_quorum_active=*/true, *quorum, + MakeBatch({0, 1, 2})); + BOOST_CHECK(res == PreVerifyResult::MissingVerificationVector); + BOOST_CHECK(!ShouldBan(res)); +} + +BOOST_AUTO_TEST_CASE(preverify_duplicate_member_bans) +{ + const auto quorum = BuildQuorum(); + const auto res = PreVerifyBatchedSigShares(ProTxHashOfMember(0), /*is_quorum_active=*/true, *quorum, + MakeBatch({0, 1, 0})); + BOOST_CHECK(res == PreVerifyResult::DuplicateMember); + BOOST_CHECK(ShouldBan(res)); +} + +BOOST_AUTO_TEST_CASE(preverify_member_out_of_bounds_bans) +{ + const auto quorum = BuildQuorum(); + const auto oob_idx = static_cast(llmq_params.size); // one past the last member + + const auto res = PreVerifyBatchedSigShares(ProTxHashOfMember(0), /*is_quorum_active=*/true, *quorum, + MakeBatch({oob_idx})); + BOOST_CHECK(res == PreVerifyResult::MemberOutOfBounds); + BOOST_CHECK(ShouldBan(res)); + + // A single bad entry poisons an otherwise valid batch + const auto res_mixed = PreVerifyBatchedSigShares(ProTxHashOfMember(0), /*is_quorum_active=*/true, *quorum, + MakeBatch({0, oob_idx})); + BOOST_CHECK(res_mixed == PreVerifyResult::MemberOutOfBounds); + BOOST_CHECK(ShouldBan(res_mixed)); +} + +BOOST_AUTO_TEST_CASE(preverify_member_not_valid_bans) +{ + const auto quorum = BuildQuorum(/*with_verification_vector=*/true, /*invalid_members=*/{1}); + const auto res = PreVerifyBatchedSigShares(ProTxHashOfMember(0), /*is_quorum_active=*/true, *quorum, MakeBatch({0, 1})); + BOOST_CHECK(res == PreVerifyResult::MemberNotValid); + BOOST_CHECK(ShouldBan(res)); +} + +BOOST_AUTO_TEST_SUITE_END()