Skip to content

fix(llmq): guard VerifyContributionShares against an empty verification-vector set - #7486

Open
PastaPastaPasta wants to merge 3 commits into
dashpay:developfrom
PastaPastaPasta:sec/v022
Open

fix(llmq): guard VerifyContributionShares against an empty verification-vector set#7486
PastaPastaPasta wants to merge 3 commits into
dashpay:developfrom
PastaPastaPasta:sec/v022

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Jul 27, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

CBLSWorker::AsyncVerifyContributionShares() terminates the node when handed an empty
verification-vector set.

The path is degenerate rather than obviously wrong, which is why it survives review:

  • VerifyVerificationVectors() returns true for an empty span — its loop body never runs.
  • ContributionVerifier::Start() then schedules zero work (batchCount == 0 when aggregated,
    or a zero-length one-by-one batch when not).
  • Nothing ever invokes doneCallback. No scheduled lambda captured shared_from_this(), so
    the ContributionVerifier is released as soon as Start() returns, the promise held by the
    future-based wrapper is destroyed without set_value(), and .get() throws
    std::future_error(broken_promise).

The DKG phase handler in net_dkg.cpp only catches AbortPhaseException, so that throw
propagates out of the phase-handler thread and std::terminate aborts the node.

Reachability

ActiveDKGSession::VerifyPendingContributions() is the only caller that can produce an empty
input. It early-returns on pendingContributionVerifications.empty(), but then filters the
pending set by m->bad || m->weComplain. If every pending member was marked bad between
enqueue and flush — a duplicate contribution triggering MarkBadMember() is the way in — the
filtered vvecs is 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:

  • An attacker can only mark its own masternodes bad. Message signatures are batch-verified
    (BatchVerifyMessageSigs) before ReceiveMessage() runs, so the duplicate contribution that
    triggers MarkBadMember() must be validly signed by that member's own operator key. Honest
    members cannot be poisoned into the bad set.
  • Our own contribution is always in the pending list. EnqueueOwn() feeds the local
    contribution 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 must
    have accepted at least 33 contributions.
  • That rules out every quorum smaller than 33 outright: all test/devnet params and
    llmq_25_67 cannot 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 race
    against state it cannot directly observe.
  • The 32-entry flush path cannot produce the empty case by itself: each member enqueues at
    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 an
    all-bad pending set.

The practical reading: this is defensive hardening of a crash path, not a readily exploitable
DoS. It is still a std::terminate reachable 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:

  1. CBLSWorker::AsyncVerifyContributionShares() short-circuits on empty input and
    invokes doneCallback with an empty result. This mirrors the empty-input handling that
    AsyncBuildQuorumVerificationVector() and AsyncAggregateHelper() already have, so the
    worker API is now consistent about degenerate input.

  2. ActiveDKGSession::VerifyPendingContributions() skips the worker call entirely when
    the 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 with
    std::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 empty
    result on both the aggregated and non-aggregated paths.
  • bls_verify_contribution_shares_honest_tests — asserts that acceptance behaviour for
    non-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, so
    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. That property previously had no coverage under make check at all — only
    src/bench/bls_dkg.cpp exercised it, and benches do not run in CI.

Full bls_tests suite passes. Built and run on macOS/arm64 against current develop.

Breaking Changes

None. The change only affects an input shape that previously terminated the process.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

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.
@PastaPastaPasta
PastaPastaPasta marked this pull request as ready for review July 28, 2026 22:33
@thepastaclaw

thepastaclaw commented Jul 28, 2026

Copy link
Copy Markdown

✅ Final review complete — no blockers (commit 732417b)

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants