refactor: PreVerifyBatchedSigShares structured return and unit tests

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 \

src/llmq/signing_shares.cpp

@@ -424,8 +424,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<CSigShare> sigSharesToProcess;
@@ -522,46 +523,43 @@ void CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s
              sigShare.GetSignHash().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::PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman,
+                                                                    const CQuorumManager& quorum_manager,
+                                                                    const CSigSharesNodeState::SessionInfo& session,
+                                                                    const CBatchedSigShares& batchedSigShares)
 {
-    retBan = false;
-
     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<uint16_t> dupMembers;
 
     for (const auto& [quorumMember, _] : batchedSigShares.sigShares) {
         if (!dupMembers.emplace(quorumMember).second) {
-            retBan = true;
-            return false;
+            return {PreVerifyResult::DuplicateMember, true};
         }
 
         if (quorumMember >= session.quorum->members.size()) {
             LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__);
-            retBan = true;
-            return false;
+            return {PreVerifyResult::QuorumMemberOutOfBounds, true};
         }
         if (!session.quorum->qc->validMembers[quorumMember]) {
             LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__);
-            retBan = true;
-            return false;
+            return {PreVerifyResult::QuorumMemberNotValid, true};
         }
     }
-    return true;
+    return {PreVerifyResult::Success, false};
 }
 
 bool CSigSharesManager::CollectPendingSigSharesToVerify(

src/llmq/signing_shares.h

@@ -359,6 +359,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 CRecoveredSigsListener
 {
 private:
@@ -447,6 +464,12 @@ class CSigSharesManager : public CRecoveredSigsListener
 
     void NotifyRecoveredSig(const std::shared_ptr<const CRecoveredSig>& sig) const;
 
+    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);
+
 private:
     // all of these return false when the currently processed message should be aborted (as each message actually contains multiple messages)
     bool ProcessMessageSigSesAnn(const CNode& pfrom, const CSigSesAnn& ann);
@@ -455,10 +478,6 @@ class CSigSharesManager : public CRecoveredSigsListener
     bool ProcessMessageBatchedSigShares(const CNode& pfrom, const CBatchedSigShares& batchedSigShares);
     void ProcessMessageSigShare(NodeId fromId, const CSigShare& sigShare);
 
-    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<NodeId, std::vector<CSigShare>>& retSigShares,
         std::unordered_map<std::pair<Consensus::LLMQType, uint256>, CQuorumCPtr, StaticSaltedHasher>& retQuorums);