fix: bound DKG contribution blob intake

[PastaPastaPasta]

Issue being fixed or feature implemented

The DKG intake hardening on develop pre-validates pushed qcontrib messages before retaining them. The valid configured envelope is params.minSize <= actual members <= params.size; the later DKG session worker still checks the exact members.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 NetDKG so encrypted contribution blob counts must be within the configured quorum bounds:

• at least params.minSize
• at most params.size

The 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:

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

This pull request was created by Codex.

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[thepastaclaw]

✅ Review complete (commit 439fb7d)

[coderabbitai]

Walkthrough

The change tightens QCONTRIB structural validation in CheckDKGMessageStructure by requiring the encrypted contribution blob count to fall within params.minSize and params.size. The functional intake test now constructs a targeted undersized qcontrib payload and verifies that sending it results in a malformed DKG message log and a banscore of 100.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• dashpay/dash#7397: Similar QCONTRIB/DKG intake hardening, including validation changes and adversarial intake coverage.

Suggested reviewers: kwvg, UdjinM6

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: tightening DKG contribution intake bounds.
Description check	✅ Passed	The description matches the code and test changes by explaining the lower-bound intake validation and added coverage.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/llmq/net_dkg.cpp (1)

84-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor style inconsistency: params.minSize cast without the same negative-guard as size/threshold.

Lines 84-85 guard params.size/params.threshold with param > 0 ? static_cast<size_t>(param) : 0 before use, but the new check at line 93 casts params.minSize directly. If minSize were ever negative, this would wrap to a huge size_t and always reject QCONTRIB for that quorum, whereas the guarded locals fail safe to 0. Given minSize is 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.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 650680e3-c0be-4021-a55a-44508aa34d43

📥 Commits

Reviewing files that changed from the base of the PR and between 4af395d and 10d7a5806d6109fad3c08dd593239eff379ca227.

📒 Files selected for processing (1)

• src/llmq/net_dkg.cpp

[thepastaclaw]

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.

[thepastaclaw]

💬 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']}

[thepastaclaw]

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.}

[thepastaclaw]

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=3 and minSize=2, while the test sends blob_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 production src/llmq/net_dkg.cpp hunk that introduces the clamped min_size local.

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`.

[thepastaclaw]

🟡 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']}

[knst]

tACK 439fb7d

[UdjinM6]

utACK 439fb7d