fix: treat unregistered types as inactive in IsQuorumActive by PastaPastaPasta · Pull Request #7544 · dashpay/dash

PastaPastaPasta · 2026-08-04T20:11:54Z

Issue being fixed or feature implemented

IsQuorumActive previously aborted via assert when given an LLMQ type that is not registered in the active chain's consensus params. Call sites are currently gated or reach this helper only with validated types, so this is defense-in-depth rather than a live remote crash today. If a future caller reorders checks or omits the net-layer type gate, an unregistered wire-derived type would still abort the node.

What was done?

Return false from IsQuorumActive when Params().GetLLMQ(llmqType) is empty instead of asserting

How Has This Been Tested?

Built src/test/test_dash against prebuilt depends (aarch64-apple-darwin25.3.0)
Ran ./src/test/test_dash --run_test=llmq_invalid_type_tests (pass)
No new dedicated unit case: an unknown-type-only assertion would only restate the early return, and a meaningful active/inactive fixture needs full mined-quorum setup not warranted for this one-liner

Breaking Changes

None. Unregistered types were never valid active quorums; callers that asserted-failed now simply treat them as inactive.

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)

This pull request was created by Codex.

IsQuorumActive previously asserted that Params().GetLLMQ(llmqType) was present. Call sites are currently gated, but an invalid wire-derived type that reaches this helper would abort the node. Return false instead and cover the path in llmq_invalid_type_tests.

thepastaclaw · 2026-08-04T20:12:07Z

✅ Final review complete — no blockers (commit e42e666)

coderabbitai · 2026-08-04T20:13:43Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b004ea6a-b7d7-42a9-b087-d470f3b67a5a

📥 Commits

Reviewing files that changed from the base of the PR and between f8d3c09 and e42e666.

📒 Files selected for processing (1)

src/llmq/signing.cpp

💤 Files with no reviewable changes (1)

src/llmq/signing.cpp

Walkthrough

IsQuorumActive now returns false when the requested LLMQ type is unknown or unregistered. The change replaces the previous assertion failure during parameter lookup.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

dashpay/dash#7516: Both changes handle unknown or unregistered LLMQType values without assertion failures.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely states that unregistered quorum types are treated as inactive in IsQuorumActive.
Description check	✅ Passed	The description directly explains the IsQuorumActive change, its rationale, testing, and impact.
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.

✨ 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 · 2026-08-04T20:23:37Z

@PastaPastaPasta CodeRabbit found no actionable review comments. One separate blocker remains: Validate PR title rejects the llmq scope because it is not in the repository allowlist. Please retitle this to fix: treat unregistered types as inactive in IsQuorumActive (or use an allowed scope such as consensus) so the check can rerun.

The unknown-type early-return is self-explanatory. A dedicated unknown-type-only unit test only restates that guard and is not worth keeping without a full active-quorum fixture.

PastaPastaPasta

LGTM; nice simple PR. This fixes no bug atm, BUT in the future, I could very easily see us or some agent misusing this function, passing an llmqType received from the network, and crashing

thepastaclaw

Final validation — Codex + Sonnet

The new guard correctly treats unregistered LLMQ types as inactive and prevents them from reaching ScanQuorums; no blocking correctness issue is present. There are no carried-forward findings because this is the first substantive automated review; the full current PR has two non-blocking issues: the direct regression test was removed, and the add-then-remove commit pair should be squashed before merge.
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 contract attempts, fresh retry completed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=codex/final-verifier=gpt-5.6-sol(completed); 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 (failed), claude-sonnet-5 — general (failed), claude-sonnet-5 — general (completed)

🟡 2 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 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/signing.cpp`:
- [SUGGESTION] src/llmq/signing.cpp:651-654: Restore the direct unknown-type regression test
  No test at the current head calls IsQuorumActive with an LLMQ type for which GetLLMQ returns nullopt, so the new early-return branch is not directly covered. The test removed by e42e666 reached this guard before ScanQuorums for both an unknown value and a real but unregistered enum value; it passed with this change and would abort on the previous assertion. Restore that focused test so a future refactor cannot silently reintroduce the assertion or otherwise break the defensive behavior this PR adds.

In `<commit:e42e666>`:
- [SUGGESTION] <commit:e42e666>:1: Squash the add-then-remove follow-up into the original fix
  Commit e42e66634b5 only removes the explanatory comment, include, and regression test introduced by the immediately preceding f8d3c094e25; it adds no independent behavior. Dash's merge commits preserve the PR's individual commits, so leaving this stack intact permanently records an avoidable add-then-remove seam in git log and blame. Squash the follow-up into the original fix and make the resulting single commit reflect the final decision on the regression test.

thepastaclaw · 2026-08-05T01:55:05Z

    const auto& llmq_params_opt = Params().GetLLMQ(llmqType);
-    assert(llmq_params_opt.has_value());
+    if (!llmq_params_opt.has_value()) {
+        return false;
+    }


🟡 Suggestion: Restore the direct unknown-type regression test

No test at the current head calls IsQuorumActive with an LLMQ type for which GetLLMQ returns nullopt, so the new early-return branch is not directly covered. The test removed by e42e666 reached this guard before ScanQuorums for both an unknown value and a real but unregistered enum value; it passed with this change and would abort on the previous assertion. Restore that focused test so a future refactor cannot silently reintroduce the assertion or otherwise break the defensive behavior this PR adds.

_{source: ['claude', 'codex']}

Adding a test for this seems not needed

PastaPastaPasta · 2026-08-05T18:11:51Z

LGTM

knst

utACK e42e666

github-actions Bot mentioned this pull request Aug 4, 2026

refactor: route per-LLMQ-type caches through PerLlmqTypeCache #7516

Open

5 tasks

PastaPastaPasta changed the title ~~fix(llmq): treat unregistered types as inactive in IsQuorumActive~~ fix: treat unregistered types as inactive in IsQuorumActive Aug 4, 2026

chore: drop low-value IsQuorumActive comments and test

e42e666

The unknown-type early-return is self-explanatory. A dedicated unknown-type-only unit test only restates that guard and is not worth keeping without a full active-quorum fixture.

PastaPastaPasta commented Aug 4, 2026

View reviewed changes

thepastaclaw reviewed Aug 5, 2026

View reviewed changes

knst approved these changes Aug 5, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix: treat unregistered types as inactive in IsQuorumActive - #7544

fix: treat unregistered types as inactive in IsQuorumActive#7544
PastaPastaPasta wants to merge 2 commits into
dashpay:developfrom
PastaPastaPasta:fix/is-quorum-active-invalid-type

PastaPastaPasta commented Aug 4, 2026 •

edited

Loading

Uh oh!

thepastaclaw commented Aug 4, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

thepastaclaw commented Aug 4, 2026

Uh oh!

PastaPastaPasta left a comment

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Aug 5, 2026

Uh oh!

PastaPastaPasta Aug 5, 2026

Uh oh!

PastaPastaPasta commented Aug 5, 2026

Uh oh!

knst left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

PastaPastaPasta commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

thepastaclaw commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

thepastaclaw commented Aug 4, 2026

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Final validation — Codex + Sonnet

Review provenance

Uh oh!

thepastaclaw Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Aug 5, 2026

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PastaPastaPasta commented Aug 4, 2026 •

edited

Loading

thepastaclaw commented Aug 4, 2026 •

edited

Loading

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading