Skip to content

fix: reject unsupported custom LLMQ sizes#7448

Open
thepastaclaw wants to merge 1 commit into
dashpay:developfrom
thepastaclaw:fix/validate-llmq-size-overrides
Open

fix: reject unsupported custom LLMQ sizes#7448
thepastaclaw wants to merge 1 commit into
dashpay:developfrom
thepastaclaw:fix/validate-llmq-size-overrides

Conversation

@thepastaclaw

Copy link
Copy Markdown

Issue being fixed

Devnet and regtest LLMQ size overrides accepted quorum sizes above 400 even though the signing protocol cannot carry them. Signing-session inventories and batched signature-share messages are capped at 400, so an oversized custom quorum could start successfully but fail later when peers exchange signing shares.

What was done

  • Define Consensus::MAX_LLMQ_SIZE as the authoritative 400-member protocol maximum.
  • Tie MAX_MSGS_TOTAL_BATCHED_SIGS to that shared maximum so the limits cannot drift.
  • Reject oversized -llmqdevnetparams, -llmqtestparams, -llmqtestinstantsendparams, and -llmqtestplatformparams values during chain-parameter construction.
  • Cover normal custom values, the inclusive 400-member boundary, and 401-member rejection for all four override paths.

This is a fail-fast configuration fix discovered while reviewing #7418. It does not change mainnet or testnet parameters, and accepted custom sizes at or below 400 retain their existing behavior.

Validation

  • Full unit suite: 744 test cases passed.
  • feature_llmq_signing.py: both variants passed.
  • Direct startup checks: 401-member devnet/regtest overrides fail with the intended error; 400-member overrides start normally.
  • git diff --check
  • whitespace lint
  • circular-dependency lint
  • Independent pre-PR review: ship, no findings.

Scope

This intentionally adds only the missing upper bound. Existing behavior for zero, negative, or otherwise inconsistent custom size/threshold pairs is unchanged.

The devnet `-llmqdevnetparams` and regtest `-llmqtestparams`,
`-llmqtestinstantsendparams` and `-llmqtestplatformparams` overrides parsed a
`<size>:<threshold>` pair and assigned the size to the LLMQ params without any
upper bound. Sizes above 400 were therefore accepted at startup even though the
signing protocol cannot carry them: signing session inventories and batched
sig-share messages are bounded at 400 entries (MAX_MSGS_TOTAL_BATCHED_SIGS), and
the bounded intake work extends that limit to the receive path. A node
configured with, say, a 500-member quorum would build inventories its peers
reject, so the quorum could never recover a signature - a confusing runtime
failure far away from the misconfiguration.

Introduce a single authoritative Consensus::MAX_LLMQ_SIZE (400) in the low-level
llmq/params.h, which chainparams already includes, and define the existing
message-specific MAX_MSGS_TOTAL_BATCHED_SIGS in terms of it so the two limits
cannot drift apart. Chain-parameter construction now fails fast with a clear
error when an override requests a size above the maximum. Sizes at or below 400,
including the small custom values used by the functional test framework, are
unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@PastaPastaPasta PastaPastaPasta marked this pull request as ready for review July 12, 2026 15:58
@thepastaclaw

thepastaclaw commented Jul 12, 2026

Copy link
Copy Markdown
Author

✅ Review complete (commit a54fcbe)

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b5231da5-8843-4d2b-b6ec-2261315ae4fc

📥 Commits

Reviewing files that changed from the base of the PR and between ecf2382 and a54fcbe.

📒 Files selected for processing (4)
  • src/chainparams.cpp
  • src/llmq/params.h
  • src/llmq/signing_shares.h
  • src/test/llmq_params_tests.cpp

Walkthrough

Adds Consensus::MAX_LLMQ_SIZE with a value of 400 and uses it for batched signature limits. Regtest and devnet LLMQ size overrides now throw std::runtime_error when exceeding this limit. Tests cover valid maximum-size overrides and rejection of larger values for both network types.

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

Possibly related PRs

Suggested reviewers: knst, kwvg, UdjinM6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% 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 summarizes the main change: rejecting oversized custom LLMQ sizes.
Description check ✅ Passed The description directly matches the changeset and explains the LLMQ size limit fix and tests.
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

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

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Code Review

Source: codex (gpt-5.6-sol, reasoning=high) — general & dash-core-commit-history lanes; sonnet5 (claude-sonnet-5, effort=high) — general & dash-core-commit-history lanes; opus (claude-opus-4-8) — general & dash-core-commit-history lanes. Final verifier: claude-sonnet-5 (verifier-sonnet5-7448-1783873850). Orchestrator (openai/gpt-5.6-sol) is orchestration-only, not a reviewer or finding source. Independently verified PR #7448 at head a54fcbe (base develop): the diff introduces Consensus::MAX_LLMQ_SIZE(400), ties MAX_MSGS_TOTAL_BATCHED_SIGS to it, and adds the upper-bound rejection to all four override paths (-llmqdevnetparams, -llmqtestparams, -llmqtestinstantsendparams, -llmqtestplatformparams) exactly as described. New unit test correctly exercises the 400-boundary and 401-rejection for all four paths. Single clean non-merge commit; message matches diff. All six native-token lanes found no defects, and independent verification confirms no in-scope issues.

Note: GitHub does not allow me to approve my own PR, so this verified clean review is posted as a COMMENT.

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.

1 participant