From eeaf6a5cf748dc22f7d9edf7cb8bc16de953d3bb Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 9 Jul 2026 16:53:22 -0500 Subject: [PATCH 1/2] refactor(llmq): return explicit PreVerifyResult from sig share preverification Replace boolean-only sig share preverification outcomes with an explicit PreVerifyResult enum and constexpr ShouldBan() mapping. The shared PreVerifySigShareQuorum helper remains the single source for benign quorum checks used by both message paths, while PreVerifyBatchedSigShares adds the structural malformed-peer outcomes. This makes the distinction between benign inability to verify and ban-worthy peer data explicit in the type system. Resolve quorum activity and the local proTxHash at the call sites and pass them as plain values, keeping the production preverification function independent of CQuorumManager and CActiveMasternodeManager and directly unit-testable. Add a duplicate-member log line for parity with the other malformed-batch paths; runtime behavior is otherwise unchanged. Co-Authored-By: Claude Fable 5 --- src/llmq/signing_shares.cpp | 62 +++++++++++++++++++++---------------- src/llmq/signing_shares.h | 49 +++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 27 deletions(-) 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 From db76fb23986774f450aa816ba461d69a5182daf6 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Thu, 9 Jul 2026 16:55:34 -0500 Subject: [PATCH 2/2] test: add unit tests for batched sig share preverification Exercise llmq::PreVerifyBatchedSigShares directly against real CQuorum objects built from a CFinalCommitment and deterministic mock masternodes, covering every result disposition: the three benign non-ban prechecks (QuorumTooOld, NotAMember, MissingVerificationVector) and the three structural ban outcomes (DuplicateMember, MemberOutOfBounds, MemberNotValid), plus empty-batch acceptance, precedence of benign prechecks over structural checks, and a single bad entry poisoning an otherwise valid batch. The ShouldBan() result-to-ban mapping is pinned with static_asserts. Co-Authored-By: Claude Fable 5 --- src/Makefile.test.include | 1 + src/test/llmq_signing_shares_tests.cpp | 180 +++++++++++++++++++++++++ 2 files changed, 181 insertions(+) create mode 100644 src/test/llmq_signing_shares_tests.cpp 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/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()