Repository navigation
fix: reject malformed detached quorum share batches - #7822
PastaPastaPasta merged 1 commit into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 24 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (1)
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 |
|
✅ Final review complete — no blockers (commit 7a9d1b0) · triage: low · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied Phase-2 claims against exact head 9fe144a; no in-scope defects were found. Malformed-source rejection survives batch verification, and the caller bans and skips the entire detached source vector before dispatch; the regression checks malformed-only batches, both mixed-share orderings, an independent valid source, and all-valid controls. Validation was static only: the supplied CI snapshot shows successful container-build and formatting checks, two queued manifest jobs, and no completed project-test results; the PR's reported local test runs were not independently reproduced.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff makes a small, contained rejection-set fix in src/llmq/net_signing.cpp, extracts the existing verification loop without changing cryptographic verification, and adds focused regression coverage. - Phase 1 reviewers: not run (skipped for throughput: 13 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
When a detached sig share's lazy BLS signature failed to deserialize, ProcessPendingSigShares banned the node and stopped collecting its shares, but never marked it as a bad source. The dispatch loop then handed that node's whole vector, including the malformed share and every unverified share after it, to CSigSharesManager::ProcessPendingSigShares, which assumes its input is already verified. Record the node in batchVerifier.badSources instead, so the existing dispatch loop bans it once and skips its shares. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
9fe144a to
7a9d1b0
Compare
thepastaclaw
removed
the
pastaclaw:approved
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The complete diff correctly records malformed-signature sources in badSources, which survives batch verification and causes the existing dispatch loop to ban the source and skip its entire detached share vector. Valid-source processing is unchanged, and no in-scope defects or commit-history issues were found. Validation was static only: the supplied CI snapshot has queued checks and a failed check_merge job whose log could not be retrieved, so successful CI is not established.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The one-line change in NetSigning::ProcessPendingSigShares routes malformed sources through the existing ban-and-skip path, making this a small, contained behavior fix despite its signature-processing context. - Phase 1 reviewers: not run (skipped for throughput: 16 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
2 participants
Issue being fixed or feature implemented
When a detached sig share's lazy BLS signature fails to deserialize,
NetSigning::ProcessPendingSigSharesbans the source and stops collecting its shares, but never adds it tobatchVerifier.badSources. The dispatch loop then passes that source's whole vector toCSigSharesManager::ProcessPendingSigShares, including the malformed share and every unverified share after it. That method assumes its input is already verified, so unverified shares can reach the local share manager and interfere with recovery.What was done?
In the malformed-signature branch, replace
BanNode(nodeId)withbatchVerifier.badSources.emplace(nodeId). The existing dispatch loop already bans every node inbadSourcesonce (which also re-requests its shares) and skips its vector, so the malformed source's shares are no longer dispatched. Valid sources go through the same verification path as before.#7840 changes the same
ProcessPendingSigSharesblock (per-signature BLS authentication) and should be rebased over whichever of the two merges first.How Has This Been Tested?
Verified by inspection and build. No regression test is added, because the one-line change is self-evident from the diff:
badSourcesnow drives the existing ban-and-skip dispatch loop, andCBLSBatchVerifier::Verify()only adds tobadSourcesand never clears it.make -C src dashd test/test_dash dash-clibuilt cleanly../src/test/test_dash --run_test=llmq_utils_testspassed.test/functional/test_runner.py feature_llmq_signing.py "feature_llmq_signing.py --spork21"passed.Breaking Changes
None to consensus rules, historical commitment validation, wire formats, or valid signatures. Detached batches from a source with a malformed share are now dropped instead of dispatched.
Checklist:
No user-facing documentation change is needed for this internal validation repair.
🤖 Generated with Claude Code