Skip to content

refactor: PreVerifyBatchedSigShares structured return and unit tests #6914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
+380 −20
Open

refactor: PreVerifyBatchedSigShares structured return and unit tests #6914

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b7614d8
refactor: update PreVerifyBatchedSigShares to return structured results
PastaPastaPasta Oct 22, 2025
8e2674f
test: add unit tests for LLMQ signing shares validation
PastaPastaPasta Oct 22, 2025
4f5a823
test: add better unit tests for PreVerifyBatchedSigShares validation …
PastaPastaPasta Oct 24, 2025
b8c344b
chore: apply-clang-format; fix test failures
PastaPastaPasta Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Makefile.test.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
28 changes: 12 additions & 16 deletions src/llmq/signing_shares.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -522,46 +523,41 @@ 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};
}
Copy link

@coderabbitai coderabbitai bot Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add missing include for std::unordered_set and log DuplicateMember for parity.

  • std::unordered_set is used but not included here; rely on explicit include to avoid transitive-dependency breakage.
  • Emit a LogPrint on DuplicateMember like the other branches.

Apply:

@@
-#include <util/time.h>
+#include <util/time.h>
+#include <unordered_set>
@@
-    for (const auto& [quorumMember, _] : batchedSigShares.sigShares) {
+    for (const auto& [quorumMember, _] : batchedSigShares.sigShares) {
         if (!dupMembers.emplace(quorumMember).second) {
-            return {PreVerifyResult::DuplicateMember, true};
+            LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- duplicate quorumMember in batch\n", __func__);
+            return {PreVerifyResult::DuplicateMember, true};
         }

If you applied the header rename, update this function’s returns and call sites accordingly (see next comment).

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/llmq/signing_shares.cpp around lines 526 to 561, the function uses
std::unordered_set but the header is not included and the DuplicateMember branch
lacks a log call for parity with other error branches; add #include
<unordered_set> at the top of this file and insert a LogPrint call immediately
before the DuplicateMember return (matching the style/format used in the other
branches, including context such as __func__ and session.quorumHash or
quorumMember as appropriate), then return {PreVerifyResult::DuplicateMember,
true}; and if you have applied the header rename mentioned elsewhere, update
this function’s return type/values and all its call sites accordingly.


bool CSigSharesManager::CollectPendingSigSharesToVerify(
Expand Down
21 changes: 19 additions & 2 deletions src/llmq/signing_shares.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -456,8 +473,8 @@ class CSigSharesManager : public CRecoveredSigsListener
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);
static PreVerifyBatchedResult PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager,
const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares);

bool CollectPendingSigSharesToVerify(
size_t maxUniqueSessions, std::unordered_map<NodeId, std::vector<CSigShare>>& retSigShares,
Expand Down
Loading
Loading