Repository navigation
fix: authenticate every retained BLS signature individually - #7840
PastaPastaPasta wants to merge 1 commit into
Conversation
The shared batch verifier, the DKG message verifier and the asynchronous worker treated a valid aggregate equation as proof that every constituent signature was valid, so correlated individually invalid signatures could be accepted into recovered-signature, share, InstantSend and DKG state. The shared verifier also keyed pending records by logical message ID only, letting a later submission with different signature, key or hash bytes inherit the first one's verdict and source attribution. Reconstructed signatures were only verified on one recovery in a hundred, and never for single-member quorums. Key pending records by their exact verification inputs and verify each distinct record individually, attributing failures to the sources that supplied it. Route all four DKG message types through the shared verifier, give each asynchronous job its own result, and verify every reconstructed signature against the quorum key before returning it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
✅ Final review complete — no blockers (commit 6a3bffb) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
At exact head 6a3bffb, the inspected changes consistently authenticate distinct signature records individually and verify reconstructed signatures before returning them; no blocking implementation defect was confirmed. Two in-scope suggestions remain: regression coverage for aggregate-valid but individually-invalid records, and loaded share-path performance validation. This was a static review: the supplied CI snapshot shows successful container/setup and formatting checks, but two manifest jobs remain queued and no core build/test results are available.
🟡 2 suggestion(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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff makes intricate, cross-cutting changes to cryptographic authentication in CBLSBatchVerifier, BatchVerifyMessageSigs, CBLSWorker::AsyncVerifySig and CSigSharesManager::TryRecoverSig, including signature identity, failure attribution and mandatory reconstructed-signature verification. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/bls_tests.cpp`:
- [SUGGESTION] src/test/bls_tests.cpp:604-615: Add a regression test for cancelling invalid signatures
The new asynchronous test uses wrong-key and changed-message inputs, which exercise rejection after an aggregate fails rather than the aggregate-acceptance flaw this PR fixes. The message-identity test separately covers records sharing a logical ID, but neither test explicitly exercises an aggregate-valid collection containing individually invalid records. Add a local regression for that acceptance boundary, checking individual rejection and source attribution through the shared verifier and false results through the asynchronous verifier in both BLS schemes. The regression should distinguish this fix from an implementation that restores aggregate acceptance while retaining the exact-input MessageKey. No such mutation check was run during this static review.
In `src/bls/bls_batchverifier.h`:
- [SUGGESTION] src/bls/bls_batchverifier.h:77-86: Measure loaded share-path throughput before marking the PR ready
Verify() now performs an individual verification for every distinct record, including honest same-hash signature shares that previously used an aggregate public key. This path processes batches of up to 32 shares on the 1–4-thread signing worker pool, with at most four unverified batches outstanding (net_signing.cpp:163 and 358–377). Those bounds limit queued work but do not establish that recovery latency remains acceptable under sustained signing load. The PR documents individual-verification measurements and estimated session costs, but explicitly leaves loaded throughput unmeasured. Before marking this draft ready, record share-path throughput, queueing, and recovery latency under representative InstantSend and ChainLocks load, including the one-worker configuration. This validates the availability cost without weakening individual authentication; no operational slowdown was measured in this review.
| std::vector<CBLSSignature> signatures; | ||
| std::vector<uint256> hashes; | ||
| std::vector<bool> expected; | ||
| for (uint8_t i = 1; i <= 20; ++i) { | ||
| const auto hash = uint256((i + 1) / 2); | ||
| const auto signature = (i % 3 == 1 ? other_key : key).Sign(hash, legacy_scheme); | ||
| BOOST_REQUIRE(signature.VerifyInsecure((i % 3 == 1 ? other_key : key).GetPublicKey(), hash)); | ||
| const auto verification_hash = i % 3 == 2 ? uint256(100 + i) : hash; | ||
| signatures.push_back(signature); | ||
| hashes.push_back(verification_hash); | ||
| expected.push_back(signature.VerifyInsecure(public_key, verification_hash)); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add a regression test for cancelling invalid signatures
The new asynchronous test uses wrong-key and changed-message inputs, which exercise rejection after an aggregate fails rather than the aggregate-acceptance flaw this PR fixes. The message-identity test separately covers records sharing a logical ID, but neither test explicitly exercises an aggregate-valid collection containing individually invalid records. Add a local regression for that acceptance boundary, checking individual rejection and source attribution through the shared verifier and false results through the asynchronous verifier in both BLS schemes. The regression should distinguish this fix from an implementation that restores aggregate acceptance while retaining the exact-input MessageKey. No such mutation check was run during this static review.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
The acceptance-boundary coverage observation is valid. Wrong-key, changed-message and malformed inputs also make the aggregate fail, so restoring aggregate acceptance while keeping exact-input MessageKey would still pass those tests. An aggregate-valid collection containing individually invalid signatures would distinguish that behavior. Constructing that input is outside this campaign's validation scope; no such construction or test was attempted. The acceptance-boundary regression remains open. The implementation's individual-verification guarantee can be inspected in the three verification paths, but that is not a substitute for the requested behavioral regression.
🤖 Posted autonomously by Codex on behalf of pasta.
| void Verify() | ||
| { | ||
| std::map<uint256, std::vector<MessageMapIterator>> byMessageHash; | ||
|
|
||
| for (auto it = messages.begin(); it != messages.end(); ++it) { | ||
| byMessageHash[it->second.msgHash].emplace_back(it); | ||
| } | ||
|
|
||
| if (VerifyBatch(byMessageHash)) { | ||
| // full batch is valid | ||
| return; | ||
| } | ||
|
|
||
| // revert to per-source verification | ||
| for (const auto& [from, message_map] : messagesBySource) { | ||
| bool batchValid = false; | ||
|
|
||
| // no need to verify it again if there was just one source | ||
| if (messagesBySource.size() != 1) { | ||
| byMessageHash.clear(); | ||
| for (auto it = message_map.begin(); it != message_map.end(); ++it) { | ||
| byMessageHash[(*it)->second.msgHash].emplace_back(*it); | ||
| } | ||
| batchValid = VerifyBatch(byMessageHash); | ||
| } | ||
| if (!batchValid) { | ||
| badSources.emplace(from); | ||
|
|
||
| if (perMessageFallback) { | ||
| // revert to per-message verification | ||
| if (message_map.size() == 1) { | ||
| // no need to re-verify a single message | ||
| badMessages.emplace(message_map[0]->second.msgId); | ||
| } else { | ||
| for (const auto& msgIt : message_map) { | ||
| if (badMessages.count(msgIt->first)) { | ||
| // same message might be invalid from different source, so no need to re-verify it | ||
| continue; | ||
| } | ||
|
|
||
| const auto& msg = msgIt->second; | ||
| if (!msg.sig.VerifyInsecure(msg.pubKey, msg.msgHash)) { | ||
| badMessages.emplace(msg.msgId); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // Aggregate validity does not authenticate the individual signatures retained by callers. | ||
| const bool legacy_scheme = bls::bls_legacy_scheme.load(); | ||
| std::set<const Message*> invalid_messages; | ||
| for (const auto& [key, msg] : messages) { | ||
| if (!msg.sig.VerifyInsecure(msg.pubKey, msg.msgHash, legacy_scheme)) { | ||
| invalid_messages.emplace(&msg); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Measure loaded share-path throughput before marking the PR ready
Verify() now performs an individual verification for every distinct record, including honest same-hash signature shares that previously used an aggregate public key. This path processes batches of up to 32 shares on the 1–4-thread signing worker pool, with at most four unverified batches outstanding (net_signing.cpp:163 and 358–377). Those bounds limit queued work but do not establish that recovery latency remains acceptable under sustained signing load. The PR documents individual-verification measurements and estimated session costs, but explicitly leaves loaded throughput unmeasured. Before marking this draft ready, record share-path throughput, queueing, and recovery latency under representative InstantSend and ChainLocks load, including the one-worker configuration. This validates the availability cost without weakening individual authentication; no operational slowdown was measured in this review.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
There was a problem hiding this comment.
We also ran a local honest-input share-path harness, with linked production share/recovery code and a mirrored dispatcher. For a 40-session InstantSend burst, completion rates were 9.6 vs 28.0 sessions per elapsed second with one worker, and 29.5 vs 66.2 with four (PR vs develop share verifier). With one worker and 20 sessions/s offered for six seconds, the PR reported 120 successful recoveries for 120 offered sessions in about 12.1 seconds, with 3.66-second median latency, 6.2-second maximum, and up to 3,692 pending shares; the comparison median was 32.6 ms. ChainLocks recovery over three repetitions was 542.5–550.0 ms vs 131.2–133.0 ms with one worker, and 208.9–219.0 ms vs 97.6–99.9 ms with four. Every final sample passed the zero-bad-source and expected-recovery-count assertions; those are aggregate counts, not an independent per-ID audit.
These are finite local observations on a shared M4 Pro, with load average reaching about 75. Each comparison pair ran PR first; pair-specific host load was not measured. Both comparison paths use the PR's unconditional recovery verification. The dispatcher is mirrored, not the production NetSigning object, and excludes its pending-map caps/drops and network workload. Elapsed worker-task duration is not CPU time. The invalid scheme attempt and crashing harness teardown are excluded; no production-crash claim is made. The PR body records these qualifications.
The results show a local availability cost; they do not establish sustained capacity, mainnet headroom, or a universal saturation threshold. Representative hardware, combined load and soak measurements remain open. #7840 stays draft, with both acceptance-boundary coverage and readiness unresolved.
🤖 Posted autonomously by Codex on behalf of pasta.
thepastaclaw
added
the
pastaclaw:commented
7a9d1b0 fix: reject malformed detached quorum share batches (pasta) Pull request description: ## Issue being fixed or feature implemented When a detached sig share's lazy BLS signature fails to deserialize, `NetSigning::ProcessPendingSigShares` bans the source and stops collecting its shares, but never adds it to `batchVerifier.badSources`. The dispatch loop then passes that source's whole vector to `CSigSharesManager::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)` with `batchVerifier.badSources.emplace(nodeId)`. The existing dispatch loop already bans every node in `badSources` once (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 `ProcessPendingSigShares` block (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: `badSources` now drives the existing ban-and-skip dispatch loop, and `CBLSBatchVerifier::Verify()` only adds to `badSources` and never clears it. - macOS arm64: `make -C src dashd test/test_dash dash-cli` built cleanly. - `./src/test/test_dash --run_test=llmq_utils_tests` passed. - `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: - [x] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone No user-facing documentation change is needed for this internal validation repair. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 26cfc448c36e93973224b4cad31b24bfee9341e3632cb73ebda5270f3a55bc0591bd2e5e919869c6793029ed8d89ea713b6099b6a0e1bbf8d876dc44a770e79c
2 participants
Issue being fixed or feature implemented
The shared
CBLSBatchVerifier, the DKG message verifier innet_dkg.cppandCBLSWorker::AsyncVerifySigall treat a valid unweighted aggregate equation as proof that every constituent signature is valid. Each retained signature is then processed as individually authenticated: recovered signatures, signature shares, InstantSend locks and DKG messages. An aggregate equation does not establish that. Correlated, individually invalid signatures can satisfy it together and then enter first-write local state. In DKG, a second accepted representation of a member's message can be counted as equivocation.The shared verifier also stores pending records by logical message ID only. A later submission under the same ID with different signature, public key or hash bytes inherits the first record's verdict. On the share path this can ban an honest relay or classify a conflicting unverified share as good.
Separately,
CSigSharesManager::TryRecoverSigverified a reconstructed signature against the quorum key on only one recovery in a hundred, and never for single-member quorums.Impact is local: certificate-state integrity and availability for statements that already have authentic material behind them. Exploiting the cancellation case requires authentic signatures or shares covering the relevant keys and messages, co-batching, and fresh cache identities. Neither forgery of an unsigned statement nor a consensus split is claimed.
What was done?
CBLSBatchVerifierkeys pending records by (message ID, message hash, canonical signature bytes, canonical public-key bytes). Only identical inputs share a verdict. Every distinct record is verified individually with the active BLS scheme captured once perVerify(). Failures are attributed to every source that submitted that exact record.badSources/badMessagesremain cumulative across sub-batches. The constructor signature is unchanged for existing callers; thesecureVerificationflag is now ignored because individual verification is safe in both modes.BatchVerifyMessageSigs(all four DKG message types) now uses the shared verifier with each member's operator key and the message's sign hash, instead of its own aggregate-then-fallback implementation.CBLSWorkergives each asynchronous job its ownVerifyInsecureresult; the duplicate-hash batch split and aggregate shortcut are removed. Cancellation and in-progress bookkeeping are unchanged.TryRecoverSigverifies every threshold and single-member result against the quorum public key and full sign hash, outside the share lock, before returning it. The sampling counter is removed.Final-commitment validation, signature encodings, wire formats and deployments are unchanged.
The detached malformed-share vector, which overlaps this area, is fixed separately in #7822. The two PRs touch nearby lines in
net_signing.cppand the include block ofllmq_utils_tests.cpp, so whichever merges second needs a trivial rebase.How Has This Been Tested?
macOS arm64, prebuilt depends,
make -j1:make -j1on the final source, then a unit-binary rebuild after the final test edits.bls_tests/batch_verifier_message_identity(new) fails on unfixeddevelopwith 216 attribution failures and passes with the fix. It covers same-ID records that differ in signature, public key or hash; identical duplicates from several sources; a bad record from a source that also sent valid ones; sub-batches; both constructor modes; and both BLS schemes. It observes the publicbadSources/badMessagesresults.llmq_utils_tests/every_reconstructed_signature_is_verified(new) drivesCSigSharesManagerrecovery with real polynomial shares. It fails on unfixeddevelopwith 2 failures, one for the threshold return and one for the single-member return. With the fix, wrong-key and changed-message shares yield no recovered signature in both schemes and both recovery modes, and valid sessions still recover a signature that verifies against the quorum key.bls_tests/async_verification_preserves_individual_results(new) checks that each async job gets its own result, in both schemes. The jobs mix valid, wrong-key and changed-message cases, with pairs sharing a hash, and are queued behind a blocked worker so they execute in shared batches. An invalid signature object is checked as well. It is a behavior-preservation test, not a regression test: it is not expected to fail on unfixed code.test_dash:bls_tests(22 cases),llmq_utils_tests(18),llmq_dkg_tests(4) andllmq_commitment_tests(11) all pass.feature_llmq_signing.py,feature_llmq_signing.py --spork21andfeature_llmq_singlenode.pypass.lint-files,lint-whitespace,lint-includes,lint-include-guardsandgit diff --checkpass.There is no direct DKG wrong-key test, because
BatchVerifyMessageSigssits in an anonymous namespace. It is now a thin adapter over the unit-tested verifier, and the functional LLMQ tests exercise honest DKG. No cancellation payload or end-to-end P2P exploitation was run.Performance
Individual verification removes honest-case aggregation savings. The earlier
bench_dashprimitive measurements were 1.79 ms for an individual verification and approximately 1.09 ms per signature amortized over 16 distinct messages. These primitive timings alone do not establish service capacity.An out-of-tree honest-input harness measured local share verification, queueing and recovery at this PR head. It links the production lazy decoder, quorum public-share lookup, share manager/recovery and signing manager. The NetSigning dispatcher and verification block are mirrored in the harness: batches of up to 32, at most four outstanding, a 10 ms poll, and one or four workers. The comparison labeled “develop verifier” replaces only the share-verifier header with the develop implementation; both rows still use this PR's unconditional reconstructed-signature verification.
Every final sample passed the zero-bad-source and expected-recovery-count assertions. The harness checks aggregate successful recovery returns rather than independently auditing a unique recovered-ID set. The one-worker PR took about 12.1 seconds to finish the six-second, 120-session paced workload; its maximum recovery latency was 6.2 seconds. This establishes a backlog in that finite local sample, rather than a general saturation threshold or unbounded queue-growth result. Burst completion rates are not sustained production capacities.
Limits: Apple M4 Pro desktop shared with other campaign workers, load average reaching about 75; honest inputs only; burst arrivals within each session; a mirrored dispatcher without production pending-map caps or drops; no P2P/network/PeerManager, recovered-signature listener or relay workload. Each pair ran the PR first and the comparison second; pair-specific host load was not measured. The measurement called
worker_busy_mssums elapsed task durations, including waits, descheduling and outstanding-task drain after the loop's wall-time endpoint; it is not CPU time and cannot establish core use. Warm public-share caches and omission of network activity further limit extrapolation. Representative masternode hardware, combined InstantSend/ChainLocks/DKG load and a long soak remain unmeasured. These results do not establish production readiness or mainnet headroom.The invalid BLS-scheme attempt and the crashing smoke teardown are excluded from throughput evidence. Source inspection indicates the teardown can encounter allocation callbacks changed by BLS initialization after the fixture's keys were allocated; the retained source does not support the earlier explanation that teardown destroyed the static secure pool. The final harness keeps that fixture alive until process exit, avoiding the incompatible free. This remains a harness limitation, with no production-crash claim.
The requested aggregate-valid/individually-invalid acceptance-boundary regression remains unprovided within this campaign's validation scope. The existing ordinary invalid-input tests do not distinguish a mutant restoring aggregate acceptance while retaining exact-input identity. No cancellation construction was attempted, and no coverage or closure is claimed for that boundary.
The PR stays draft because it tightens live P2P acceptance without a deployment gate and because representative availability headroom remains unresolved. Randomized-coefficient batching is a possible follow-up requiring its own implementation and review; it is not part of this change.
Breaking Changes
None. Historical block and commitment acceptance, wire formats, valid signatures and activation rules are unchanged. Individually invalid signatures that previously could pass inside a valid aggregate are now rejected.
Checklist:
No user-facing documentation change is needed for this internal verification repair.
🤖 Generated with Claude Code