fix: bound DKG contribution blob intake#7408
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 439fb7d) |
WalkthroughThe change tightens QCONTRIB structural validation in Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
src/llmq/net_dkg.cpp (1)
84-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor style inconsistency:
params.minSizecast without the same negative-guard assize/threshold.Lines 84-85 guard
params.size/params.thresholdwithparam > 0 ? static_cast<size_t>(param) : 0before use, but the new check at line 93 castsparams.minSizedirectly. IfminSizewere ever negative, this would wrap to a hugesize_tand always reject QCONTRIB for that quorum, whereas the guarded locals fail safe to 0. GivenminSizeis a trusted, statically-defined config value this is unlikely to bite, but for consistency with the surrounding defensive style it may be worth applying the same guard.♻️ Optional consistency fix
const size_t size = params.size > 0 ? static_cast<size_t>(params.size) : 0; const size_t threshold = params.threshold > 0 ? static_cast<size_t>(params.threshold) : 0; + const size_t minSize = params.minSize > 0 ? static_cast<size_t>(params.minSize) : 0; try { CDataStream s(vRecv); // copy; deserialization does not advance the caller's stream if (msg_type == NetMsgType::QCONTRIB) { CDKGContribution qc; s >> qc; return qc.vvec != nullptr && qc.vvec->size() == threshold && qc.contributions != nullptr && - qc.contributions->blobs.size() >= static_cast<size_t>(params.minSize) && + qc.contributions->blobs.size() >= minSize && qc.contributions->blobs.size() <= size;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llmq/net_dkg.cpp` around lines 84 - 94, The QCONTRIB validation in the DKG message parsing path uses params.minSize without the same negative-value guard applied to params.size and params.threshold. Update the check in the CDKGContribution handling inside the NetMsgType::QCONTRIB branch to normalize params.minSize to a size_t with a non-negative guard, matching the defensive pattern used for size and threshold, so the comparison stays consistent and avoids unsigned wraparound.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/llmq/net_dkg.cpp`:
- Around line 84-94: The QCONTRIB validation in the DKG message parsing path
uses params.minSize without the same negative-value guard applied to params.size
and params.threshold. Update the check in the CDKGContribution handling inside
the NetMsgType::QCONTRIB branch to normalize params.minSize to a size_t with a
non-negative guard, matching the defensive pattern used for size and threshold,
so the comparison stays consistent and avoids unsigned wraparound.
There was a problem hiding this comment.
Code Review
Source: reviewers: claude-dash-core-commit-history (opus, ok), claude-general (opus, ok), codex-dash-core-commit-history (gpt-5.5, ok), codex-general (gpt-5.5, ok); verifier: claude (opus).
Small, correct hardening of the QCONTRIB intake structural check that adds a lower bound (>= params.minSize) on the encrypted contributions blob count, complementing the existing upper bound and matching the DIP-6 envelope enforced later in CDKGSession::PreVerifyMessage. No consensus behavior change; only per-peer intake rejection with misbehavior score 100. Both agents converge on a single test-coverage suggestion; no blocking issues.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
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 `test/functional/feature_llmq_dkg_intake.py`:
- [SUGGESTION] test/functional/feature_llmq_dkg_intake.py:134-145: Add functional coverage for under-minSize QCONTRIB rejection
The new `qc.contributions->blobs.size() >= params.minSize` bound in `CheckDKGMessageStructure` (src/llmq/net_dkg.cpp:93) is not exercised by any test. `test_malformed_rejected` only covers a truncated payload (which fails during deserialization) and `test_oversized_rejected` covers the >1 MiB path, so neither catches a regression that drops or weakens the lower bound. `feature_llmq_dkg_intake.py` is the natural home for a case that constructs a well-formed `CDKGContribution` with `vvec->size() == threshold` but `blobs.size()` in `[0, minSize)`, sends it via `msg_dkg_raw(b"qcontrib", ...)`, and asserts the `"malformed DKG message"` debug log plus a misbehavior score of 100. Without this, a future regression that reintroduces retention of under-minSize contribution blobs would go unnoticed.
In `src/llmq/net_dkg.cpp`:
- [NITPICK] src/llmq/net_dkg.cpp:91-94: Cast of signed `params.minSize` is inconsistent with adjacent clamping
Lines 84-85 of the same function defensively clamp `params.size`/`params.threshold` before casting to `size_t`, and `MaxDKGMessageSize` (lines 51-52) does the same. Line 93 instead bare-casts `params.minSize` to `size_t`. Every configured `minSize` in `src/llmq/params.h` is >= 2, so this is safe today, but the local style divergence stands out and a future misconfigured 0/negative value would silently accept anything (0) or become unreachable (huge). A `std::max(0, params.minSize)`-style clamp would match the surrounding pattern with no observable behavior change. Not required for this PR.
| return qc.vvec != nullptr && qc.vvec->size() == threshold && | ||
| qc.contributions != nullptr && qc.contributions->blobs.size() <= size; | ||
| qc.contributions != nullptr && | ||
| qc.contributions->blobs.size() >= static_cast<size_t>(params.minSize) && | ||
| qc.contributions->blobs.size() <= size; |
There was a problem hiding this comment.
💬 Nitpick: Cast of signed params.minSize is inconsistent with adjacent clamping
Lines 84-85 of the same function defensively clamp params.size/params.threshold before casting to size_t, and MaxDKGMessageSize (lines 51-52) does the same. Line 93 instead bare-casts params.minSize to size_t. Every configured minSize in src/llmq/params.h is >= 2, so this is safe today, but the local style divergence stands out and a future misconfigured 0/negative value would silently accept anything (0) or become unreachable (huge). A std::max(0, params.minSize)-style clamp would match the surrounding pattern with no observable behavior change. Not required for this PR.
source: ['claude-general']
There was a problem hiding this comment.
Resolved in this update — Cast of signed params.minSize is inconsistent with adjacent clamping no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
PastaPastaPasta
force-pushed
the
codex/fix-dkg-intake-partial-members
branch
from
a4664e4 to
439fb7d
Compare
There was a problem hiding this comment.
Code Review
Source: reviewers: claude-general (opus, ok), codex-general (gpt-5.5, ok), claude-dash-core-commit-history (opus, ok), codex-dash-core-commit-history (gpt-5.5, ok); verifier: claude (opus, ok).
Cumulative revalidation at head 439fb7d0. The latest delta from a4664e40 to 439fb7d0 has no tree changes, so this pass revalidated the current head and the prior findings rather than treating the empty delta as a clean slate.
Prior Findings Reconciliation:
- prior-1: FIXED. LLMQ_TEST uses
size=3andminSize=2, while the test sendsblob_count=1; the all-zero BLS bytes deserialize far enough to reach the blob-count return path, so removing the new lower bound makes the test fail instead of passing via a deserialization exception. - prior-2: STILL VALID. The current
test:commit still contains the productionsrc/llmq/net_dkg.cpphunk that introduces the clampedmin_sizelocal.
Carried-Forward Prior Findings:
- prior-2 is carried forward below as the only current-head finding.
New Findings In Latest Delta:
- None.
1 suggestion
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/llmq/net_dkg.cpp`:
- [SUGGESTION] src/llmq/net_dkg.cpp:85-94: Production min_size clamp is landing in a test-prefixed commit
Commit 439fb7d0a7 has subject `test: cover undersized DKG contribution intake` but its diff also edits `src/llmq/net_dkg.cpp`: it introduces `const size_t min_size = params.minSize > 0 ? static_cast<size_t>(params.minSize) : 0;` and rewrites the intake check to consume that local, replacing the raw `static_cast<size_t>(params.minSize)` that fix commit 35d4480367 introduced. That is a defensive production behavior change unrelated to the test being added. Because Dash keeps PR commits unsquashed on merge, `git log --oneline` and `git blame` on this consensus-adjacent intake path will report a `test:` commit for a real code change. Two clean options: squash the `net_dkg.cpp` hunk into 35d4480367 so the fix commit is self-contained with its final clamped form, or split it into its own commit (for example, `fix: clamp negative params.minSize in DKG intake check`) and leave the `test:` commit touching only `test/functional/feature_llmq_dkg_intake.py`.
| const size_t min_size = params.minSize > 0 ? static_cast<size_t>(params.minSize) : 0; | ||
| const size_t threshold = params.threshold > 0 ? static_cast<size_t>(params.threshold) : 0; | ||
| try { | ||
| CDataStream s(vRecv); // copy; deserialization does not advance the caller's stream | ||
| if (msg_type == NetMsgType::QCONTRIB) { | ||
| CDKGContribution qc; | ||
| s >> qc; | ||
| return qc.vvec != nullptr && qc.vvec->size() == threshold && | ||
| qc.contributions != nullptr && qc.contributions->blobs.size() <= size; | ||
| qc.contributions != nullptr && | ||
| qc.contributions->blobs.size() >= min_size && |
There was a problem hiding this comment.
🟡 Suggestion: Production min_size clamp is landing in a test-prefixed commit
Commit 439fb7d has subject test: cover undersized DKG contribution intake but its diff also edits src/llmq/net_dkg.cpp: it introduces const size_t min_size = params.minSize > 0 ? static_cast<size_t>(params.minSize) : 0; and rewrites the intake check to consume that local, replacing the raw static_cast<size_t>(params.minSize) that fix commit 35d4480 introduced. That is a defensive production behavior change (clamping negative/zero params.minSize) unrelated to the test being added. Because Dash keeps PR commits unsquashed on merge, git log --oneline and git blame on this consensus-adjacent intake path will report a test: commit for a real code change — a bisect/blame hazard. Two clean options: squash the net_dkg.cpp hunk into 35d4480 so the fix commit is self-contained with its final clamped form, or split it into its own commit (e.g. fix: clamp negative params.minSize in DKG intake check) and leave the test: commit touching only test/functional/feature_llmq_dkg_intake.py.
source: ['claude', 'codex']
4 participants
Issue being fixed or feature implemented
The DKG intake hardening on
developpre-validates pushedqcontribmessages before retaining them. The valid configured envelope isparams.minSize <= actual members <= params.size; the later DKG session worker still checks the exactmembers.size()once it has session context.Without the lower bound, structurally well-formed but undersized qcontrib payloads can pass the cheap intake check and reach retention before the worker rejects them.
What was done?
Tightened the cheap qcontrib structure check in
NetDKGso encrypted contribution blob counts must be within the configured quorum bounds:params.minSizeparams.sizeThe exact member-count validation remains in the DKG worker.
Also added functional coverage for a qcontrib payload that deserializes cleanly but has fewer encrypted contribution blobs than
params.minSize.How Has This Been Tested?
Run feature_asset_locks.py multiple times that has been faulty in the first place (by knst).
Breaking Changes
None.
Checklist:
This pull request was created by Codex.