refactor: PreVerifyBatchedSigShares structured return and unit tests #6914
Conversation
|
WalkthroughThis PR replaces the boolean out-parameter verification path for batched signature shares with a structured return type. It introduces Sequence Diagram(s)sequenceDiagram
participant Net as Network / Message Receiver
participant Proc as ProcessMessageBatchedSigShares
participant Pre as PreVerifyBatchedSigShares
participant Struct as ValidateBatchedSigSharesStructure
participant QM as QuorumManager / Node State
rect rgba(220,240,255,0.5)
Net->>Proc: Receive BatchedSigShares message
end
Proc->>Pre: PreVerifyBatchedSigShares(session, batchedSigShares)
Pre->>Struct: Validate structure (duplicates, bounds, membership)
Struct-->>Pre: PreVerifyBatchedResult{result: <r>, should_ban: <b>}
Pre->>QM: Check quorum, verification vector, member validity
Pre-->>Proc: PreVerifyBatchedResult{result: <r>, should_ban: <b>}
alt Validation success
Proc->>Proc: IsSuccess() == true
Proc-->>Net: Accept / process batch
else Validation failure
Proc->>Proc: IsSuccess() == false
alt should_ban == true
Proc-->>Net: Reject and mark peer for ban
else should_ban == false
Proc-->>Net: Reject without ban
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/llmq/signing_shares.h (1)
371-376: Make result type hard to ignore and document ban semantics.Mark the struct [[nodiscard]] and add a brief comment clarifying should_ban semantics.
-struct PreVerifyBatchedResult { +// Preverification outcome for QBSIGSHARES. When should_ban==true, callers should +// abort processing and ban the peer at the higher-level message loop. +struct [[nodiscard]] PreVerifyBatchedResult { PreVerifyResult result; bool should_ban; - [[nodiscard]] bool IsSuccess() const { return result == PreVerifyResult::Success; } + [[nodiscard]] bool IsSuccess() const noexcept { return result == PreVerifyResult::Success; } };src/test/llmq_signing_shares_tests.cpp (3)
5-17: Include what you use.Add <unordered_set> for std::unordered_set.
#include <test/util/llmq_tests.h> +#include <unordered_set> #include <bls/bls.h>
101-191: These tests don’t exercise the preverification function.They validate structure locally but don’t assert PreVerifyBatchedSigShares outcomes (Success/should_ban). Consider:
- Adding an integration-style test that feeds a QBSIGSHARES message through ProcessMessage and observes ban/no‑ban.
- Or, add a test seam (e.g., expose a test-only helper that runs the structural checks used by PreVerifyBatchedSigShares) so unit tests can assert DuplicateMember/OutOfBounds/NotValid map to should_ban=true.
I can draft either an integration test or a small test-only helper to cover these branches.
236-254: Empty batch: assert expected Success explicitly (if a seam is added).Once a seam or integration path exists, assert result.result==Success and should_ban==false for empty batches.
src/llmq/signing_shares.cpp (1)
514-549: Add a log for duplicate-member bans and pre-size the set.Helps diagnose bans and avoids a couple of rehashes for larger batches.
- std::unordered_set<uint16_t> dupMembers; + std::unordered_set<uint16_t> dupMembers; + dupMembers.reserve(batchedSigShares.sigShares.size()); 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}; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Makefile.test.include(1 hunks)src/llmq/signing_shares.cpp(2 hunks)src/llmq/signing_shares.h(2 hunks)src/test/llmq_signing_shares_tests.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/test/llmq_signing_shares_tests.cppsrc/llmq/signing_shares.hsrc/llmq/signing_shares.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/llmq_signing_shares_tests.cpp
🧬 Code graph analysis (3)
src/test/llmq_signing_shares_tests.cpp (2)
src/test/util/llmq_tests.h (1)
testutils(26-35)src/test/util/setup_common.h (1)
InsecureRand256(80-80)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (2)
PreVerifyBatchedSigShares(514-549)PreVerifyBatchedSigShares(514-515)
src/llmq/signing_shares.cpp (1)
src/llmq/signing.cpp (2)
IsQuorumActive(831-840)IsQuorumActive(831-831)
🪛 GitHub Actions: Clang Diff Format Check
src/test/llmq_signing_shares_tests.cpp
[error] 1-1: Clang format differences found in src/test/llmq_signing_shares_tests.cpp. Run './contrib/devtools/clang-format-diff.py -p1' to fix formatting. Command failed with exit code 1.
src/llmq/signing_shares.h
[error] 1-1: Clang format differences found in src/llmq/signing_shares.h. Run './contrib/devtools/clang-format-diff.py -p1' to fix formatting. Command failed with exit code 1.
src/llmq/signing_shares.cpp
[error] 1-1: Clang format differences found in src/llmq/signing_shares.cpp. Run './contrib/devtools/clang-format-diff.py -p1' to fix formatting. Command failed with exit code 1.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (7)
src/Makefile.test.include (1)
140-140: Test added to suite — good.The new llmq_signing_shares test is properly wired into BITCOIN_TESTS.
src/llmq/signing_shares.h (3)
361-369: Result taxonomy looks sane.Enum covers expected branches (success, age, membership, vvec, dup, OOB, invalid).
471-473: PreVerifyBatchedSigShares: no stale call sites: All usages now return PreVerifyBatchedResult; no bool return or retBan out-param remains.
361-377: No formatting issues detected. clang-format-diff reports no differences for this change.src/test/llmq_signing_shares_tests.cpp (2)
23-97: Good fixture scaffolding.Fixture builds minimal quorums and batches cleanly; helpful for edge cases.
1-4: Run clang‐format and commit fixes
CI reports formatting issues in this file—please run:./contrib/devtools/clang-format-diff.py -p1and commit the resulting changes.
src/llmq/signing_shares.cpp (1)
413-416: Call-site semantics are correct.Returning !should_ban integrates with the all_of() + BanNode flow as intended.
Add a negative-path test later to confirm DuplicateMember triggers a ban via QBSIGSHARES (integration).
|
| // We can test the duplicate detection logic by checking the batch structure | ||
| std::unordered_set<uint16_t> seen; | ||
| bool hasDuplicate = false; | ||
| for (const auto& [member, _] : batchedSigShares.sigShares) { | ||
| if (!seen.insert(member).second) { | ||
| hasDuplicate = true; | ||
| break; |
There was a problem hiding this comment.
it looks for me as unit test preverify_duplicate_member instead of testing llmq's code tests validates that helper CreateTestBatchedSigShares works.
Becase this helper changes member of CBatchedSigShares:
for (uint16_t member : members) {
CBLSLazySignature lazySig;
batched.sigShares.emplace_back(member, lazySig);
}
(that's not the only regression test here that does something similar in this PR)
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
force-pushed
the
refac/PreVerifyBatchedSigShares
branch
from
99df68b to
4f5a823
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/llmq/signing_shares.cpp (1)
1-1: Fix clang-format diff failure for this file.Run the project’s formatting script locally and commit the result.
#!/bin/bash set -euo pipefail # From repo root git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i || true # Show any remaining format diffs (should be empty) git diff --exit-code || true
🧹 Nitpick comments (5)
src/llmq/signing_shares.h (2)
372-377: Unify naming style and make the API return [[nodiscard]].
- Field name should follow project’s lowerCamelCase (e.g., recvSessionId). Rename should_ban → shouldBan.
- Mark PreVerifyBatchedSigShares return as [[nodiscard]] to enforce handling.
Apply in this header:
struct PreVerifyBatchedResult { PreVerifyResult result; - bool should_ban; + bool shouldBan; [[nodiscard]] bool IsSuccess() const { return result == PreVerifyResult::Success; } }; - static PreVerifyBatchedResult PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, - const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares); + [[nodiscard]] static PreVerifyBatchedResult PreVerifyBatchedSigShares(const CActiveMasternodeManager& mn_activeman, const CQuorumManager& quorum_manager, + const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares);Also update usages in src/llmq/signing_shares.cpp and tests (see inline comments in those files for minimal diffs).
Also applies to: 475-478
362-370: Enum looks good; consider brief docs for each value.Add 1-line comments per enum value to clarify ban/no-ban intent for future maintainers. Keeps tests and call sites aligned.
src/llmq/signing_shares.cpp (1)
427-429: If renaming should_ban → shouldBan (consistency), update uses here.@@ - if (!verifyResult.IsSuccess()) { - return !verifyResult.should_ban; + if (!verifyResult.IsSuccess()) { + return !verifyResult.shouldBan; } @@ - return {PreVerifyResult::QuorumTooOld, false}; + return {PreVerifyResult::QuorumTooOld, false}; @@ - return {PreVerifyResult::NotAMember, false}; + return {PreVerifyResult::NotAMember, false}; @@ - return {PreVerifyResult::MissingVerificationVector, false}; + return {PreVerifyResult::MissingVerificationVector, false}; @@ - return {PreVerifyResult::DuplicateMember, true}; + return {PreVerifyResult::DuplicateMember, true}; @@ - return {PreVerifyResult::QuorumMemberOutOfBounds, true}; + return {PreVerifyResult::QuorumMemberOutOfBounds, true}; @@ - return {PreVerifyResult::QuorumMemberNotValid, true}; + return {PreVerifyResult::QuorumMemberNotValid, true}; @@ - return {PreVerifyResult::Success, false}; + return {PreVerifyResult::Success, false};Note: Only uses of the field change (shouldBan); the initializer brace order remains the same.
Also applies to: 531-558, 560-560
src/test/llmq_signing_shares_tests.cpp (2)
143-167: Strengthen assertions to verify exact outcomes (not just !IsSuccess).Once IsMember() and IsQuorumActive() are satisfied, assert specific PreVerifyResult and ban flag for each scenario (DuplicateMember, QuorumMemberOutOfBounds, QuorumMemberNotValid, Success/empty batch).
Example for duplicate:
- BOOST_CHECK(!result.IsSuccess()); - if (result.result == PreVerifyResult::DuplicateMember) { - BOOST_CHECK(result.should_ban); - } + BOOST_CHECK_MESSAGE(!result.IsSuccess(), "Expected failure for duplicate member"); + BOOST_CHECK_EQUAL(result.result, PreVerifyResult::DuplicateMember); + BOOST_CHECK(result.shouldBan);Apply similar tightening for:
- preverify_member_out_of_bounds → expect QuorumMemberOutOfBounds + shouldBan
- preverify_invalid_quorum_member → expect QuorumMemberNotValid + shouldBan
- preverify_empty_batch → expect Success (or at least not a ban-worthy structure failure)
- preverify_boundary_max_member → expect Success when early checks are satisfied
Note: if you keep the original field name should_ban, adjust accordingly; if you accept the rename, use shouldBan.
Also applies to: 171-194, 197-222, 225-254, 257-282, 285-308, 311-332, 335-360
165-166: Adjust field access if should_ban → shouldBan.- BOOST_CHECK(result.should_ban); + BOOST_CHECK(result.shouldBan); @@ - BOOST_CHECK(result.should_ban); + BOOST_CHECK(result.shouldBan); @@ - BOOST_CHECK(result.should_ban); + BOOST_CHECK(result.shouldBan); @@ - BOOST_CHECK(ban_result.should_ban); + BOOST_CHECK(ban_result.shouldBan); @@ - BOOST_CHECK(!no_ban_result.should_ban); + BOOST_CHECK(!no_ban_result.shouldBan);Also applies to: 191-193, 219-221, 305-307, 367-377
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Makefile.test.include(1 hunks)src/llmq/signing_shares.cpp(2 hunks)src/llmq/signing_shares.h(2 hunks)src/test/llmq_signing_shares_tests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Makefile.test.include
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/llmq/signing_shares.cppsrc/llmq/signing_shares.hsrc/test/llmq_signing_shares_tests.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/llmq_signing_shares_tests.cpp
🧬 Code graph analysis (3)
src/llmq/signing_shares.cpp (1)
src/llmq/signing.cpp (2)
IsQuorumActive(751-760)IsQuorumActive(751-751)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (2)
PreVerifyBatchedSigShares(526-561)PreVerifyBatchedSigShares(526-527)
src/test/llmq_signing_shares_tests.cpp (2)
src/test/util/llmq_tests.h (1)
testutils(26-35)src/test/util/setup_common.h (1)
InsecureRand256(80-80)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/signing_shares.cpp
[error] 1-1: Clang-format differences found. Run the formatting script to fix: git diff ... | ./contrib/devtools/clang-format-diff.py -p1 > diff_output.txt and apply clang-format.
src/llmq/signing_shares.h
[error] 1-1: Clang-format differences found. Run the formatting script to fix: git diff ... | ./contrib/devtools/clang-format-diff.py -p1 > diff_output.txt and apply clang-format.
src/test/llmq_signing_shares_tests.cpp
[error] 1-1: Clang-format differences found. Run the formatting script to fix: git diff ... | ./contrib/devtools/clang-format-diff.py -p1 > diff_output.txt and apply clang-format.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (3)
src/llmq/signing_shares.cpp (1)
427-429: Return semantics are correct against caller’s ban flow.Returning !verifyResult.should_ban ensures ranges::all_of short-circuits to BanNode() only on ban-worthy failures.
Please run unit tests covering both ban and non-ban paths once the tests are strengthened (see test file comments).
src/test/llmq_signing_shares_tests.cpp (2)
1-1: Cannot verify clang-format compliance in this environment.The sandbox lacks the necessary tools (clang-format, python3) to execute the original verification script. However, git shows that the working tree is clean with no pending changes for this file, suggesting the formatting concern may already be addressed or not currently applicable.
Manual verification of formatting compliance is recommended using the project's formatting script locally:
git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i
24-37: Make the quorum “active” in tests to bypass QuorumTooOld.PreVerifyBatchedSigShares first calls IsQuorumActive(), which consults CQuorumManager::ScanQuorums. Seed the test quorum into the manager so IsQuorumActive() returns true.
Search for existing helpers you can reuse:
If a helper like AddAndCacheQuorumForTesting exists, call it with your constructed quorum and its hash so IsQuorumActive() and GetQuorum() can find it. Otherwise, we can introduce a small test-only hook in CQuorumManager guarded by UNIT_TESTS to register synthetic quorums.
Also applies to: 84-109
src/llmq/signing_shares.cpp
Outdated
| 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}; | ||
| } |
There was a problem hiding this comment.
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.
| CQuorumCPtr CreateMinimalTestQuorum(int size, bool hasVerificationVector = true, | ||
| const std::vector<bool>& validMembers = {}, | ||
| bool includeOurProTxHash = false) | ||
| { |
There was a problem hiding this comment.
includeOurProTxHash is unused; tests won’t reach structure checks. Implement it to satisfy IsMember().
Without adding our ProTxHash to quorum->members, PreVerifyBatchedSigShares exits early with NotAMember, so duplicate/out-of-bounds/invalid tests don’t actually test structure validation.
Sketch (adjust to available constructors/utilities in-tree):
@@
- CQuorumCPtr CreateMinimalTestQuorum(int size, bool hasVerificationVector = true,
- const std::vector<bool>& validMembers = {},
- bool includeOurProTxHash = false)
+ CQuorumCPtr CreateMinimalTestQuorum(int size, bool hasVerificationVector = true,
+ const std::vector<bool>& validMembers = {},
+ bool includeOurProTxHash = false)
@@
- // Create members
- std::vector<CDeterministicMNCPtr> members;
- for (int i = 0; i < size; ++i) {
- // For simplicity, use nullptr members since we're testing pre-verification logic
- // The actual member checking happens in IsMember() which we can't fully mock
- members.push_back(nullptr);
- }
+ // Create members. To satisfy IsMember(), optionally inject our MN at index 0.
+ std::vector<CDeterministicMNCPtr> members;
+ members.resize(size, nullptr);
+ if (includeOurProTxHash) {
+ // Use a minimal deterministic MN with our proTxHash.
+ // Prefer an existing test helper if available (see verification script below).
+ auto state = std::make_shared<CDeterministicMNState>();
+ state->pubKeyOperator = bls::LegacySchemeKeyPair::FromSecret(sk).first; // or appropriate pubkey
+ auto dmn = std::make_shared<CDeterministicMN>();
+ dmn->proTxHash = mn_activeman->GetProTxHash();
+ dmn->pdmnState = state;
+ members[0] = dmn;
+ if (qc_ptr->validMembers.size() < static_cast<size_t>(size)) {
+ qc_ptr->validMembers.resize(size, true);
+ }
+ qc_ptr->validMembers[0] = true;
+ }If constructors differ, I can adapt to the codebase’s actual APIs once you confirm which test helpers exist.
Also applies to: 60-67, 68-82
🤖 Prompt for AI Agents
In src/test/llmq_signing_shares_tests.cpp around lines 40-43 (also apply similar
changes at 60-67 and 68-82), the includeOurProTxHash parameter is unused causing
PreVerifyBatchedSigShares to early-return NotAMember; update
CreateMinimalTestQuorum to, when includeOurProTxHash is true, insert the test's
local proTxHash into the quorum->members (or the appropriate members container)
so IsMember() returns true for the test node; ensure this uses existing test
helpers/constructors to build a CQuorumCPtr with members containing that
proTxHash and preserves hasVerificationVector/validMembers behavior so
subsequent structure/duplicate/out-of-bounds/invalid tests actually exercise the
validation logic.
|
|
||
| // Helper to create a minimal test quorum | ||
| CQuorumCPtr CreateMinimalTestQuorum(int size, bool hasVerificationVector = true, | ||
| const std::vector<bool>& validMembers = {}, bool includeOurProTxHash = false) |
There was a problem hiding this comment.
don't see how includeOurProTxHash is used
| const std::vector<bool>& validMembers = {}, bool includeOurProTxHash = false) | |
| const std::vector<bool>& validMembers = {}) |
| // The actual result will likely be QuorumTooOld, NotAMember, or MissingVerificationVector | ||
| // depending on which check fails first |
There was a problem hiding this comment.
Shouldn't it be deterministic? Why having a test like that if we can't get the expected MissingVerificationVector error?
| // If we get to the duplicate check (past QuorumTooOld and NotAMember checks), | ||
| // we should get DuplicateMember with should_ban=true |
There was a problem hiding this comment.
Same, I don't get this "If"... Same for all the tests below basically.
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 <[email protected]>
|
applied 89f4a4d |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/llmq/signing_shares.cpp (1)
526-546: Add LogPrint for DuplicateMember to match other validation branches.For consistency with the other validation error paths (lines 537, 541), add a LogPrint statement when detecting duplicate members. This aids debugging and provides parity in observability across all ban-worthy structural violations.
Apply this diff:
for (const auto& [quorumMember, _] : batchedSigShares.sigShares) { if (!dupMembers.emplace(quorumMember).second) { + LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- duplicate quorumMember in batch\n", __func__); return {PreVerifyResult::DuplicateMember, true}; }
🧹 Nitpick comments (1)
src/test/llmq_signing_shares_tests.cpp (1)
139-309: Consider adding integration tests for the full PreVerifyBatchedSigShares path.While these unit tests effectively validate the pure structural validation logic in
ValidateBatchedSigSharesStructure, they don't exercise the fullPreVerifyBatchedSigSharesfunction which includes checks for:
- Quorum activity (QuorumTooOld)
- Membership validation (NotAMember)
- Verification vector presence (MissingVerificationVector)
Per the PR objectives comments, reviewers expected tests that "call PreVerifyBatchedSigShares() and verify it produces the expected results." Consider adding integration tests that mock
SessionInfoand verify the complete validation flow, ensuring early-exit conditions work correctly before structural validation occurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/llmq/signing_shares.cpp(2 hunks)src/llmq/signing_shares.h(2 hunks)src/test/llmq_signing_shares_tests.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/test/llmq_signing_shares_tests.cppsrc/llmq/signing_shares.cppsrc/llmq/signing_shares.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/llmq_signing_shares_tests.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
src/test/llmq_signing_shares_tests.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/test/llmq_signing_shares_tests.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/test/llmq_signing_shares_tests.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/test/llmq_signing_shares_tests.cppsrc/llmq/signing_shares.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/llmq/signing_shares.cpp
🧬 Code graph analysis (3)
src/test/llmq_signing_shares_tests.cpp (2)
src/test/util/llmq_tests.h (4)
testutils(26-35)GetTestQuorumHash(107-107)GetTestBlockHash(109-109)CreateRandomBLSPublicKey(40-45)src/llmq/signing_shares.cpp (2)
ValidateBatchedSigSharesStructure(526-546)ValidateBatchedSigSharesStructure(526-527)
src/llmq/signing_shares.cpp (1)
src/llmq/signing.cpp (2)
IsQuorumActive(751-760)IsQuorumActive(751-751)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (6)
VerifySigSharesInv(347-351)VerifySigSharesInv(347-347)PreVerifyBatchedSigShares(548-569)PreVerifyBatchedSigShares(548-551)ValidateBatchedSigSharesStructure(526-546)ValidateBatchedSigSharesStructure(526-527)
🔇 Additional comments (4)
src/llmq/signing_shares.h (2)
362-377: LGTM! Clean refactoring to structured return type.The introduction of
PreVerifyResultenum andPreVerifyBatchedResultstruct with theIsSuccess()helper provides a clear, type-safe API that eliminates the error-prone out-parameter pattern. The enum values comprehensively cover all validation outcomes, and theshould_banfield properly distinguishes between ban-worthy structural violations and non-ban conditions.
467-476: Good extraction of validation logic for testability.Making
VerifySigSharesInvand the newValidateBatchedSigSharesStructurepublic static methods enables focused unit testing of the pure validation logic without requiring full dynamic quorum state. This separation of concerns improves maintainability.src/llmq/signing_shares.cpp (2)
427-430: LGTM! Proper usage of structured return type.The updated code correctly uses
IsSuccess()to check validation results and invertsshould_banfor the return value (returningtrueto continue processing when ban is not needed). This matches the expected semantics wherefalseaborts message processing.
548-569: LGTM! Refactored validation logic is clear and maintainable.The separation of concerns is well-executed: early-exit conditions (quorum activity, membership, verification vector) are checked first, then delegated to
ValidateBatchedSigSharesStructurefor structural validation. Each return provides appropriateshould_bansemantics.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
3 participants
Issue being fixed or feature implemented
See commits
What was done?
How Has This Been Tested?
ran unit tests
Breaking Changes
None
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.