fix(net): bound quorum data response vectors

[thepastaclaw]

Depends on #7439. Please review only the final command-specific commit here.

Issue being fixed or feature implemented

Quorum-data responses carry verification-vector and encrypted-contribution vectors whose expected sizes are known from the requested quorum. This hardens QDATA processing by checking the serialized vector count before allocating, deserializing, or processing those vectors.

This path is MNAuth/request-gated and is intentionally handled separately from unauthenticated public vector intake.

What was done?

• Read QDATA verification vectors with the requested quorum threshold as the maximum.
• Read encrypted contributions with the number of valid quorum members as the maximum.
• Require exact semantic counts and penalize mismatches before decrypt/aggregate work.
• Extend functional coverage for undersized, oversized, and allocation-amplifying CompactSize declarations.

The reusable bounded-vector serialization primitives are introduced separately in #7439.

How Has This Been Tested?

• test/functional/p2p_quorum_data.py

Breaking Changes

None.

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 _(for repository code-owners and collaborators only)

[coderabbitai]

Caution

Review failed

The pull request is closed.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 522cd80f-ec4c-40c6-a381-399bab405811

📥 Commits

Reviewing files that changed from the base of the PR and between ecf49885bcbb40fd09bd81de95edf54d3996e2fe and a286e0e.

📒 Files selected for processing (2)

• src/llmq/net_quorum.cpp
• test/functional/p2p_quorum_data.py

---

Walkthrough

QDATA processing now bounds verification-vector and encrypted-contribution deserialization using quorum-derived expected counts, rejects mismatches, and marks the peer misbehaving. Functional tests isolate malformed-payload cases, cover invalid vector and contribution lengths, and craft inconsistent CompactSize declarations for both sections.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Peer
  participant NetQuorum
  participant QuorumState

  Peer->>NetQuorum: Send malformed QDATA
  NetQuorum->>QuorumState: Read expected vector counts
  NetQuorum->>NetQuorum: Decode and validate sections
  NetQuorum->>Peer: Apply misbehavior score and disconnect

Loading

Suggested reviewers: PastaPastaPasta, knst, UdjinM6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main change: bounding quorum data response vectors.
Description check	✅ Passed	The description is directly related to the changeset and matches the implemented QDATA vector bounds and test coverage.
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.

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

[thepastaclaw]

✅ Review complete (commit a286e0e)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[thepastaclaw]

Code Review

Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5

The PR matches its stated goal of rejecting quorum-data vectors with unexpected element counts before deserialization work continues. I verified one in-scope portability regression in the new helper: the braced initialization from ReadCompactSize() is ill-formed on 32-bit size_t builds, and this repository still defines i686 depends hosts.

Verified Findings

• [BLOCKING] src/llmq/net_quorum.cpp:34: Fix narrowing initialization on 32-bit builds
  ReadCompactSize() returns uint64_t, but this new helper list-initializes a size_t from that value. On targets where size_t is 32-bit, list initialization rejects the uint64_t-to-size_t conversion as narrowing at compile time, so this breaks supported i686-style builds even though ReadCompactSize() range-checks vector sizes against MAX_SIZE. Use an explicit cast after the range-checked read.

Note: posted as a COMMENT review because GitHub does not allow approving/requesting changes on my own PR via the normal review event.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

Addressed the self-review finding about 32-bit size_t portability by making the ReadCompactSize() conversion explicit before comparing the expected QDATA vector size.

Commit: 9df78f0c6a4 (fix: avoid qdata vector size narrowing)

Validation:

• git diff --check upstream/develop..HEAD
• COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
• ./autogen.sh
• ./configure --without-gui
• make -C src dashd -j4
• make -C src dash-cli -j4
• test/functional/p2p_quorum_data.py

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

Code Review

Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5, claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5; verifier=gpt-5.5

Prior findings: prior-1 is fixed; src/llmq/net_quorum.cpp:34 now explicitly casts the ReadCompactSize() result to size_t, removing the 32-bit narrowing build failure. New findings in latest delta: one commit-history hygiene issue remains because the latest commit is a one-line fixup for code introduced by the previous PR commit.

Prior Finding Reconciliation

• FIXED: Fix narrowing initialization on 32-bit builds — Fixed in commit 9df78f0c6a4; ReadCompactSize(stream) is now wrapped in static_cast<size_t>() before initialization at src/llmq/net_quorum.cpp:34.

Carried-Forward Prior Findings

None.

New Findings In Latest Delta

• [SUGGESTION] <commit:9df78f0c6a4>: Squash the one-line corrective commit
  Commit 9df78f0c6a4 only changes the helper introduced by d452f3db84c from the narrowing size_t initialization to the corrected explicit cast. This is a review-fix commit for the same logical change, and Dash PR commits land in permanent history without squash merging, so leaving it separate records an intermediate unshipped version in git log and git blame. Squash 9df78f0c6a4 into d452f3db84c so the QDATA hardening lands as one corrected logical commit.

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 `<commit:9df78f0c6a4>`:
- [SUGGESTION] <commit:9df78f0c6a4>:1: Squash the one-line corrective commit
  Commit 9df78f0c6a4 only changes the helper introduced by d452f3db84c from the narrowing `size_t` initialization to the corrected explicit cast. This is a review-fix commit for the same logical change, and Dash PR commits land in permanent history without squash merging, so leaving it separate records an intermediate unshipped version in `git log` and `git blame`. Squash 9df78f0c6a4 into d452f3db84c so the QDATA hardening lands as one corrected logical commit.

[thepastaclaw]

@coderabbitai review

[thepastaclaw]

Squashed the one-line corrective commit into the original QDATA vector-bound commit. The new head is 4157ce53e6a and the tree is identical to the previously validated 9df78f0c6a4 head.

Validation after rewrite:

• tree equality: 9df78f0c6a4^{tree} == 4157ce53e6a^{tree}
• git diff --check upstream/develop..HEAD
• COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

Code Review

Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5

Prior reconciliation: prior-commit-history-1 is FIXED because the current stack at 4157ce53e6ae2b41ced3d1217bca0a0970d151d2 contains one non-merge commit, fix: bound quorum data response vectors, and the separate one-line corrective commit is no longer in the current branch history. Carried-forward prior findings: none. New findings in latest delta 9df78f0c6a477bd9a3d92fbd3e07e79c39d72537..4157ce53e6ae2b41ced3d1217bca0a0970d151d2: none; the tree delta is empty, and the cumulative PR diff's exact-size QDATA checks match the sender-side quorum vector shapes.

GitHub does not allow PastaClaw to approve their own PR, so this is posted as a COMMENT review rather than an APPROVE review.

Prior Reconciliation:

• FIXED: Squash the one-line corrective commit — Fixed in the new head: the current PR stack is a single non-merge commit, and the prior corrective commit 9df78f0c6a4 fix: avoid qdata vector size narrowing is not present in the current branch history.

Carried-forward prior findings: None.

New findings in latest delta:
None.

[thepastaclaw]

CI failure triage for the old d452f3db84ca run: the red linux64-test / Test source job failed in feature_governance.py --legacy-wallet, not in this PR's QDATA path. The failure was the known intermittent governance vote-count assertion (node.gobject("count")["votes"] observed 24 instead of 25), which is already tracked in #7328; I added this occurrence there.

This PR's current head is 4157ce53e6ae, and the current visible CI matrix is green, including linux64-test / Test source. No branch change, rebase, empty commit, or rerun is needed for this stale/unrelated failure.

[PastaPastaPasta]

utACK 4157ce53e6ae2b41ced3d1217bca0a0970d151d2

[UdjinM6]

utACK 4157ce53e6ae2b41ced3d1217bca0a0970d151d2

[knst]

over-complicated ; see clarification below

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

@kwvg Yes—the defensive effect does not depend on accepting a payload larger than MAX_PROTOCOL_MESSAGE_LENGTH.

The peer controls the CompactSize element count independently of the bytes that follow it. A small QDATA payload can therefore declare, for example, 1,000 BLS elements and omit the elements. With ordinary vector deserialization, the receiver starts element decoding and reaches a stream-underrun exception; that exception is logged by the outer message loop and does not reach the QDATA score-100 path. With the bounded helper, the declared count is compared with the known quorum threshold/member count first, so it is rejected and scored before any element decode.

The amended head a286e0e7eb4 removes the separate > MAX_SIZE serializer behavior that prompted your question. The functional regressions now use a declared count of 1,000—well below MAX_SIZE, above both QDATA semantic limits, and possible within the normal message cap—and cover both the verification-vector and contribution paths. The full functional test passes on this head.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a286e0e7eb

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[kwvg]

Code near identical to past utACK (see diff)

---

utACK a286e0e

[knst]

LGTM a286e0e

[thepastaclaw]

@coderabbitai review

Follow-up status: #7416 merged at a286e0e before the corrected branch head f2e577a was pushed. The fix is preserved on the source branch but is not in merge commit 5260466 and needs a separate follow-up.

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr view 7416 --repo dashpay/dash --json state,mergeCommit,mergedAt,baseRefName,headRefName,commits --jq '{state, mergeCommit, mergedAt, baseRefName, headRefName}'
echo "---"
gh api repos/dashpay/dash/pulls/7416/commits --jq '.[].sha' | tail -5
echo "---merge commit parents---"
gh api repos/dashpay/dash/commits/52604668ed7 --jq '.parents[].sha' 2>/dev/null || echo "not found"

Length of output: 882

---

@thepastaclaw Confirmed via the GitHub API: PR #7416 merged as 52604668ed7, whose merged parent is a286e0e7eb4fd63eb65f05f5deefd5a2c4c43f55 — matching the last commit listed on the PR before merge. Since f2e577a0951 was pushed after that merge, it's correct that the corrected head only exists on the source branch and isn't part of the merge commit on develop.

Since the merge already happened, there's nothing further for me to review on this closed PR. If you'd like, I can open a follow-up GitHub issue on dashpay/dash documenting that the MAX_PROTOCOL_MESSAGE_LENGTH/declared-count clarification from f2e577a0951 still needs to land via a separate PR — just confirm and I'll file it referencing this PR/comment as backlinks.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

Code Review

Source: orchestrator openai/gpt-5.6-sol/high (orchestration-only); reviewers codex/general=gpt-5.6-sol, sonnet5/general=claude-sonnet-5, codex/dash-core-commit-history=gpt-5.6-sol, sonnet5/dash-core-commit-history=claude-sonnet-5; verifier=claude-sonnet-5 (verifier-sonnet5-7416-1783709714).

PR #7416 at head a286e0e introduces pre-decode bounds checks for the two QDATA vectors, but the intended full-ban protection is bypassed for wire counts above MAX_SIZE. The carried-forward prior finding is STILL VALID: it was fixed at unpublished checkpoint 50e177d9 and then reverted along with its boundary coverage. No distinct new latest-delta or additional cumulative findings were identified; commit-history hygiene is clean.

🔴 1 blocking

🤖 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_quorum.cpp`:
- [BLOCKING] src/llmq/net_quorum.cpp:205-295: Oversized QDATA vector counts throw past the intended full-ban path at both call sites
  At current head, UnserializeVectorWithMaxSize (src/serialize.h:978-988) calls ReadCompactSize(s) with default range_check=true. Counts above MAX_SIZE throw before either QDATA expected-size check and its PeerMisbehaving(..., 100) full-ban branch. The exception reaches src/net_processing.cpp:5752, whose catch only logs it, leaving the authenticated peer connected even though ValidateDataResponse already consumed the request. Both data-mask fields are affected. The unpublished 50e177d9 checkpoint fixed this with range_check=false and a full-width comparison, but a286e0e7 removes that fix and weakens the MAX_SIZE+1 functional cases to count 1000, so the bypass is again untested. Impact is MNAuth-gated and element decoding remains bounded, but the PR's documented full-ban policy is bypassed.