refactor: make sig share preverification results explicit

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 \

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<uint16_t> 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<CSigShare> 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()) {

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

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 <test/util/llmq_tests.h>
+#include <test/util/setup_common.h>
+
+#include <arith_uint256.h>
+#include <bls/bls.h>
+#include <bls/bls_worker.h>
+#include <chain.h>
+#include <evo/deterministicmns.h>
+#include <llmq/commitment.h>
+#include <llmq/params.h>
+#include <llmq/quorums.h>
+#include <llmq/signing_shares.h>
+#include <uint256.h>
+
+#include <boost/test/unit_test.hpp>
+
+#include <memory>
+#include <vector>
+
+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<size_t>& invalid_members = {})
+    {
+        auto qc = std::make_unique<CFinalCommitment>();
+        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<CDeterministicMNCPtr> members;
+        for (size_t i = 0; i < static_cast<size_t>(llmq_params.size); i++) {
+            auto dmn = std::make_shared<CDeterministicMN>(/*_internalId=*/i + 1);
+            dmn->proTxHash = ProTxHashOfMember(i);
+            members.push_back(std::move(dmn));
+        }
+
+        auto quorum = std::make_shared<CQuorum>(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<std::vector<CBLSPublicKey>>());
+        }
+        return quorum;
+    }
+
+    // Preverification only looks at the member indexes, the signatures are never inspected
+    static CBatchedSigShares MakeBatch(const std::vector<uint16_t>& 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<uint16_t>(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()