Skip to content

Commit f5f6a7e

Browse files
UdjinM6claude
authored 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 c2ad362 commit f5f6a7e

3 files changed

Lines changed: 232 additions & 252 deletions

File tree

src/llmq/signing_shares.cpp

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -429,6 +429,28 @@ bool CSigSharesManager::ProcessMessageSigShare(NodeId fromId, const CSigShare& s
429429
return true;
430430
}
431431

432+
PreVerifyBatchedResult CSigSharesManager::ValidateBatchedSigSharesStructure(const CQuorum& quorum,
433+
const CBatchedSigShares& batchedSigShares)
434+
{
435+
std::unordered_set<uint16_t> dupMembers;
436+
437+
for (const auto& [quorumMember, _] : batchedSigShares.sigShares) {
438+
if (!dupMembers.emplace(quorumMember).second) {
439+
return {PreVerifyResult::DuplicateMember, true};
440+
}
441+
442+
if (quorumMember >= quorum.members.size()) {
443+
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__);
444+
return {PreVerifyResult::QuorumMemberOutOfBounds, true};
445+
}
446+
if (!quorum.qc->validMembers[quorumMember]) {
447+
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__);
448+
return {PreVerifyResult::QuorumMemberNotValid, true};
449+
}
450+
}
451+
return {PreVerifyResult::Success, false};
452+
}
453+
432454
PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman,
433455
const CQuorumManager& quorum_manager,
434456
const CSigSharesNodeState::SessionInfo& session,
@@ -449,23 +471,7 @@ PreVerifyBatchedResult CSigSharesManager::PreVerifyBatchedSigShares(const CActiv
449471
return {PreVerifyResult::MissingVerificationVector, false};
450472
}
451473

452-
std::unordered_set<uint16_t> dupMembers;
453-
454-
for (const auto& [quorumMember, _] : batchedSigShares.sigShares) {
455-
if (!dupMembers.emplace(quorumMember).second) {
456-
return {PreVerifyResult::DuplicateMember, true};
457-
}
458-
459-
if (quorumMember >= session.quorum->members.size()) {
460-
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__);
461-
return {PreVerifyResult::QuorumMemberOutOfBounds, true};
462-
}
463-
if (!session.quorum->qc->validMembers[quorumMember]) {
464-
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember not valid\n", __func__);
465-
return {PreVerifyResult::QuorumMemberNotValid, true};
466-
}
467-
}
468-
return {PreVerifyResult::Success, false};
474+
return ValidateBatchedSigSharesStructure(*session.quorum, batchedSigShares);
469475
}
470476

471477
bool CSigSharesManager::CollectPendingSigSharesToVerify(

src/llmq/signing_shares.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,11 @@ class CSigSharesManager : public llmq::CRecoveredSigsListener
467467
const CSigSharesNodeState::SessionInfo& session,
468468
const CBatchedSigShares& batchedSigShares);
469469

470+
// Validates the structure of batched sig shares (duplicates, bounds, member validity)
471+
// This is extracted for testability - it's the pure validation logic without external dependencies
472+
static PreVerifyBatchedResult ValidateBatchedSigSharesStructure(const CQuorum& quorum,
473+
const CBatchedSigShares& batchedSigShares);
474+
470475
private:
471476
std::optional<CSigShare> CreateSigShareForSingleMember(const CQuorum& quorum, const uint256& id, const uint256& msgHash) const;
472477

0 commit comments

Comments
 (0)