fix(llmq): guard VerifyContributionShares against an empty verification-vector set - #7486
fix(llmq): guard VerifyContributionShares against an empty verification-vector set#7486PastaPastaPasta wants to merge 3 commits into
Conversation
Adds a bls_tests case that calls CBLSWorker::VerifyContributionShares() with a valid id and empty verification-vector / secret-key-share spans and asserts it returns an empty result. Without the accompanying fix this throws std::future_error(broken_promise) when ContributionVerifier::Start schedules zero work and the promise is destroyed; with it the call returns cleanly for both aggregated and non-aggregated modes. Pre-fix repro documented in the test comment.
AsyncVerifyContributionShares treated an empty verification-vector span as valid (VerifyVerificationVectors returns true when its loop never runs) and then ContributionVerifier::Start scheduled zero work, destroying the doneCallback promise without set_value. Callers that .get() the future therefore threw std::future_error(broken_promise); on the DKG phase-handler thread that is uncaught and std::terminate aborts dashd. Short-circuit empty input to an empty result, matching AsyncBuildQuorumVerificationVector and AsyncAggregateHelper. Also return early from ActiveDKGSession::VerifyPendingContributions when every pending member has been filtered out as bad.
The empty-input guard must not alter which contributions are accepted: accepting an invalid share would corrupt the recovered quorum key, rejecting an honest one would stall DKG. That property was only covered by src/bench/bls_dkg.cpp, which does not run under 'make check', so a regression was invisible to CI. Add a positive control over real GenerateContributions output. Quorum size 10 exceeds ContributionVerifier's batch size of 8, so the aggregated path builds two batches. Corrupting one share in the first batch must fail that batch, fall back to per-contribution verification, blame exactly the corrupted index, and leave the second batch accepted wholesale -- the property batch verification could silently break by masking an invalid share among valid ones. Both legacy and basic schemes, aggregated and non-aggregated paths.
|
✅ Final review complete — no blockers (commit 732417b) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The empty-input short-circuit correctly completes callback- and future-based callers, and the DKG call-site guard safely clears a fully filtered pending batch. The only reported concern—placing the regression test before its fix—is explicitly permitted by current develop-branch guidance when the failure is documented, as it is here. No in-scope defects remain.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); claude/general=claude-sonnet-5(completed); verifier=codex/final-verifier=gpt-5.6-sol(completed) fallback_for_sonnet_verifier=true; coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
Issue being fixed or feature implemented
CBLSWorker::AsyncVerifyContributionShares()terminates the node when handed an emptyverification-vector set.
The path is degenerate rather than obviously wrong, which is why it survives review:
VerifyVerificationVectors()returnstruefor an empty span — its loop body never runs.ContributionVerifier::Start()then schedules zero work (batchCount == 0when aggregated,or a zero-length one-by-one batch when not).
doneCallback. No scheduled lambda capturedshared_from_this(), sothe
ContributionVerifieris released as soon asStart()returns, the promise held by thefuture-based wrapper is destroyed without
set_value(), and.get()throwsstd::future_error(broken_promise).The DKG phase handler in
net_dkg.cpponly catchesAbortPhaseException, so that throwpropagates out of the phase-handler thread and
std::terminateaborts the node.Reachability
ActiveDKGSession::VerifyPendingContributions()is the only caller that can produce an emptyinput. It early-returns on
pendingContributionVerifications.empty(), but then filters thepending set by
m->bad || m->weComplain. If every pending member was marked bad betweenenqueue and flush — a duplicate contribution triggering
MarkBadMember()is the way in — thefiltered
vvecsis empty while the pre-filter check passed.Getting there is hard, and the constraints are worth spelling out so the severity is not
over-estimated:
(
BatchVerifyMessageSigs) beforeReceiveMessage()runs, so the duplicate contribution thattriggers
MarkBadMember()must be validly signed by that member's own operator key. Honestmembers cannot be poisoned into the bad set.
EnqueueOwn()feeds the localcontribution through the same
ReceiveMessage()path at the start of the contribute phase,and we never mark ourselves bad. The filtered set can therefore only be empty if the
32-entry flush inside
ReceiveMessage()already cleared our own entry — i.e. the node musthave accepted at least 33 contributions.
llmq_25_67cannot reach this code path at all. Only sizes 50/60/100/400 are candidates,and there an attacker additionally has to place its members entirely in the post-flush tail
(
N - t ≡ 0 mod 32→ 18/50, 28/60, 4/100, 16/400 members) while winning an ordering raceagainst state it cannot directly observe.
most once, and the flush fires synchronously on the 32nd enqueue, so the just-enqueued
member is never bad at that instant. Only the
VerifyAndComplain()call can observe anall-bad pending set.
The practical reading: this is defensive hardening of a crash path, not a readily exploitable
DoS. It is still a
std::terminatereachable from network input, the guard costs three lines,and the same degenerate input will bite the next caller that shows up.
Note that the reachability constraints above are also why no existing test caught this — the
functional-test quorums are far too small to ever flush the pending set.
What was done?
Two commits, at two layers:
CBLSWorker::AsyncVerifyContributionShares()short-circuits on empty input andinvokes
doneCallbackwith an empty result. This mirrors the empty-input handling thatAsyncBuildQuorumVerificationVector()andAsyncAggregateHelper()already have, so theworker API is now consistent about degenerate input.
ActiveDKGSession::VerifyPendingContributions()skips the worker call entirely whenthe post-filter member set is empty, rather than relying on the guard above. This avoids a
pointless async round-trip and keeps the call site correct on its own terms.
The first commit is the fix; the second is defence in depth.
How Has This Been Tested?
Unit tests in
src/test/bls_tests.cpp:bls_verify_contribution_shares_empty_vvecs_tests— without the fix this fails withstd::future_error: The associated promise has been destructed prior to the associated state becoming ready., the exact production failure. With the fix it returns an emptyresult on both the aggregated and non-aggregated paths.
bls_verify_contribution_shares_honest_tests— asserts that acceptance behaviour fornon-empty input is unchanged, so the guard cannot mask a real verification failure. The
quorum size (10) is deliberately larger than
ContributionVerifier's batch size of 8, socorrupting one share in the first batch must fail that batch, fall back to per-contribution
verification, blame exactly the corrupted index, and leave the second batch accepted
wholesale. That property previously had no coverage under
make checkat all — onlysrc/bench/bls_dkg.cppexercised it, and benches do not run in CI.Full
bls_testssuite passes. Built and run on macOS/arm64 against currentdevelop.Breaking Changes
None. The change only affects an input shape that previously terminated the process.
Checklist: