refactor: extract validation logic and add proper unit tests

UdjinM6 · claude · PastaPastaPasta · commit 87b2e47e53c9 · 2025-12-09T09:11:56.000-06:00
Extract the pure validation logic from PreVerifyBatchedSigShares into a new ValidateBatchedSigSharesStructure function that can be properly unit tested without external dependencies. Changes: - Add ValidateBatchedSigSharesStructure() - validates duplicates, bounds, and member validity without requiring IsQuorumActive, IsMember, or HasVerificationVector - Refactor PreVerifyBatchedSigShares() to use the extracted function - Rewrite unit tests to actually test the extracted function instead of reimplementing the logic manually Test coverage (14 tests, all passing): - Result structure tests (3): success, ban errors, non-ban errors - Validation logic tests (11): success case, empty batch, duplicate detection, bounds checking, member validity, error priority These tests provide real value by exercising the actual validation code and will catch regressions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp
@@ -551,6 +551,28 @@ void CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s
              signHash.ToString(), sigShare.getId().ToString(), sigShare.getMsgHash().ToString(), sigShare.getQuorumMember(), fromId);
 }
 
+PreVerifyBatchedResult CSigSharesManager::ValidateBatchedSigSharesStructure(const CQuorum& quorum,
+                                                                            const CBatchedSigShares& batchedSigShares)
+{
+    std::unordered_set<uint16_t> 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,
@@ -571,23 +593,7 @@ PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiv
         return {PreVerifyResult::MissingVerificationVector, false};
     }
 
-    std::unordered_set<uint16_t> dupMembers;
-
-    for (const auto& [quorumMember, _] : batchedSigShares.sigShares) {
-        if (!dupMembers.emplace(quorumMember).second) {
-            return {PreVerifyResult::DuplicateMember, true};
-        }
-
-        if (quorumMember >= session.quorum->members.size()) {
-            LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__);
-            return {PreVerifyResult::QuorumMemberOutOfBounds, true};
-        }
-        if (!session.quorum->qc->validMembers[quorumMember]) {
-            LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__);
-            return {PreVerifyResult::QuorumMemberNotValid, true};
-        }
-    }
-    return {PreVerifyResult::Success, false};
+    return ValidateBatchedSigSharesStructure(*session.quorum, batchedSigShares);
 }
 
 bool CSigSharesManager::CollectPendingSigSharesToVerify(
diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h
@@ -478,6 +478,11 @@ class CSigSharesManager : public llmq::CRecoveredSigsListener
                                                             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<CSigShare> 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
