Skip to content

Commit 89f4a4d

Browse files
UdjinM6claude
andcommitted
refactor: extract validation logic and add proper unit tests
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>
1 parent 1f6d085 commit 89f4a4d

File tree

3 files changed

+232
-252
lines changed

3 files changed

+232
-252
lines changed

src/llmq/signing_shares.cpp

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,28 @@ void CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s
523523
sigShare.GetSignHash().ToString(), sigShare.getId().ToString(), sigShare.getMsgHash().ToString(), sigShare.getQuorumMember(), fromId);
524524
}
525525

526+
PreVerifyBatchedResult CSigSharesManager::ValidateBatchedSigSharesStructure(const CQuorum& quorum,
527+
const CBatchedSigShares& batchedSigShares)
528+
{
529+
std::unordered_set<uint16_t> dupMembers;
530+
531+
for (const auto& [quorumMember, _] : batchedSigShares.sigShares) {
532+
if (!dupMembers.emplace(quorumMember).second) {
533+
return {PreVerifyResult::DuplicateMember, true};
534+
}
535+
536+
if (quorumMember >= quorum.members.size()) {
537+
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__);
538+
return {PreVerifyResult::QuorumMemberOutOfBounds, true};
539+
}
540+
if (!quorum.qc->validMembers[quorumMember]) {
541+
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__);
542+
return {PreVerifyResult::QuorumMemberNotValid, true};
543+
}
544+
}
545+
return {PreVerifyResult::Success, false};
546+
}
547+
526548
PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman,
527549
const CQuorumManager& quorum_manager,
528550
const CSigSharesNodeState::SessionInfo& session,
@@ -543,23 +565,7 @@ PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiv
543565
return {PreVerifyResult::MissingVerificationVector, false};
544566
}
545567

546-
std::unordered_set<uint16_t> dupMembers;
547-
548-
for (const auto& [quorumMember, _] : batchedSigShares.sigShares) {
549-
if (!dupMembers.emplace(quorumMember).second) {
550-
return {PreVerifyResult::DuplicateMember, true};
551-
}
552-
553-
if (quorumMember >= session.quorum->members.size()) {
554-
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__);
555-
return {PreVerifyResult::QuorumMemberOutOfBounds, true};
556-
}
557-
if (!session.quorum->qc->validMembers[quorumMember]) {
558-
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__);
559-
return {PreVerifyResult::QuorumMemberNotValid, true};
560-
}
561-
}
562-
return {PreVerifyResult::Success, false};
568+
return ValidateBatchedSigSharesStructure(*session.quorum, batchedSigShares);
563569
}
564570

565571
bool CSigSharesManager::CollectPendingSigSharesToVerify(

src/llmq/signing_shares.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -470,6 +470,11 @@ class CSigSharesManager : public CRecoveredSigsListener
470470
const CSigSharesNodeState::SessionInfo& session,
471471
const CBatchedSigShares& batchedSigShares);
472472

473+
// Validates the structure of batched sig shares (duplicates, bounds, member validity)
474+
// This is extracted for testability - it's the pure validation logic without external dependencies
475+
static PreVerifyBatchedResult ValidateBatchedSigSharesStructure(const CQuorum& quorum,
476+
const CBatchedSigShares& batchedSigShares);
477+
473478
private:
474479
// all of these return false when the currently processed message should be aborted (as each message actually contains multiple messages)
475480
bool ProcessMessageSigSesAnn(const CNode& pfrom, const CSigSesAnn& ann);

0 commit comments

Comments
 (0)