fix: treat unregistered types as inactive in IsQuorumActive - #7544
Conversation
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.
|
✅ Final review complete — no blockers (commit e42e666) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Walkthrough
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
🚥 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 |
|
@PastaPastaPasta CodeRabbit found no actionable review comments. One separate blocker remains: |
PastaPastaPasta
changed the title
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
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
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
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
| const auto& llmq_params_opt = Params().GetLLMQ(llmqType); | ||
| assert(llmq_params_opt.has_value()); | ||
| if (!llmq_params_opt.has_value()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🟡 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']
There was a problem hiding this comment.
Adding a test for this seems not needed
|
LGTM |
3 participants
Issue being fixed or feature implemented
IsQuorumActivepreviously aborted viaassertwhen 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?
falsefromIsQuorumActivewhenParams().GetLLMQ(llmqType)is empty instead of assertingHow Has This Been Tested?
src/test/test_dashagainst prebuilt depends (aarch64-apple-darwin25.3.0)./src/test/test_dash --run_test=llmq_invalid_type_tests(pass)Breaking Changes
None. Unregistered types were never valid active quorums; callers that asserted-failed now simply treat them as inactive.
Checklist:
This pull request was created by Codex.