fix: handle null pprev in IsQuorumTypeEnabled instead of terminating - #7520
fix: handle null pprev in IsQuorumTypeEnabled instead of terminating#7520PastaPastaPasta wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
|
⛔ Blockers found — Sonnet deferred (commit 2204e00) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The null handling correctly prevents the reported genesis-triggered termination, and the NetDKG guard is consistent with that behavior. Two blocking issues remain: block processing ignores the new empty-member verification failure, which permits invalid commitments under accepted zero-threshold test/devnet configurations, and the first commit in the series does not compile against the pre-fix gsl::not_null API. The added tests do not exercise the asynchronous zero-threshold path.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
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/blockprocessor.cpp`:
- [BLOCKING] src/llmq/blockprocessor.cpp:231: Propagate the empty-member signature verification failure
`VerifySignatureAsync()` now returns `false` without adding a `BlsCheck` when `GetAllQuorumMembers()` returns an empty set, but this caller discards that result and relies only on `queue_control.Wait()`. Both `-llmqtestparams` and `-llmqdevnetparams` accept a zero threshold and copy it to `minSize`, so a positive-size commitment with all-false signer/member bitsets and otherwise valid non-null BLS fields can pass `Verify(..., false)` with no selected members. On devnet, the first mining window can legitimately expect the genesis hash as the quorum base. Before this change, the empty public-key `BlsCheck` was queued and failed deterministically; now no check is queued, `Wait()` succeeds, and `ProcessCommitment()` repeats verification without signatures and can store the invalid commitment. Check the return value here, or reject an empty member set during the non-signature portion of `CFinalCommitment::Verify()`. The new genesis test uses a positive `minSize` and is rejected by the bitset validation loop, so it does not cover this path.
In `src/test/evo_utils_tests.cpp`:
- [BLOCKING] src/test/evo_utils_tests.cpp:95-96: Rewrite the non-buildable regression-test commit
Commit `78f6283a4ca` adds this literal `nullptr` call while `IsQuorumTypeEnabled()` still accepts `gsl::not_null<const CBlockIndex*>`. The `not_null(std::nullptr_t)` constructor is explicitly deleted in `src/gsl/pointers.h`, so that commit fails at compilation rather than reproducing the claimed runtime termination. The later commit changes the API and makes the line compile, but it leaves a broken and misleading bisect point in the permanent series. Squash the test into the null-handling fix, or rewrite the test-first commit to pass a runtime pointer variable initialized from `genesis->pprev` so it compiles against the old signature and reaches the intended `Expects()` failure.
| BOOST_CHECK(!m_node.chainman->IsQuorumTypeEnabled(llmq_type, nullptr)); | ||
|
|
There was a problem hiding this comment.
🔴 Blocking: Rewrite the non-buildable regression-test commit
Commit 78f6283a4ca adds this literal nullptr call while IsQuorumTypeEnabled() still accepts gsl::not_null<const CBlockIndex*>. The not_null(std::nullptr_t) constructor is explicitly deleted in src/gsl/pointers.h, so that commit fails at compilation rather than reproducing the claimed runtime termination. The later commit changes the API and makes the line compile, but it leaves a broken and misleading bisect point in the permanent series. Squash the test into the null-handling fix, or rewrite the test-first commit to pass a runtime pointer variable initialized from genesis->pprev so it compiles against the old signature and reaches the intended Expects() failure.
source: ['codex']
There was a problem hiding this comment.
Correction — Rewrite the non-buildable regression-test commit remains STILL VALID at 2204e009e3cbef16445ce799134cfd88a2bd2416.
The rebase preserved the same patch: commit 38804a2c75e still passes a literal nullptr while its parent API requires gsl::not_null<const CBlockIndex*>; the raw-pointer signature is introduced only by the following commit. The earlier auto-resolution text was generated incorrectly because the finding's category/hash changed between review rounds. The Codex verifier kept this as a blocking finding, and the conversation remains unresolved.
Add a regression: when a quorum base is the genesis block, base_index->pprev is nullptr and was previously fed into IsQuorumTypeEnabled's gsl::not_null parameter, causing std::terminate(). The new case drives both IsQuorumTypeEnabled(nullptr) and GetAllQuorumMembers(genesis) and documents the verified pre-fix Abort trap (EXIT=134) failure.
Genesis quorumHash yields CBlockIndex::pprev == nullptr. That pointer was passed into IsQuorumTypeEnabled's gsl::not_null parameter, so Expects() called std::terminate() before commitment/DKG validation could reject the input (via GetAllQuorumMembers/CheckLLMQCommitment/qfcommit, and via NetDKG QCONTRIB/etc.). Accept a plain const CBlockIndex* and return false for null, matching pre-hardening DeploymentActiveAfter(nullptr, ...) semantics. Also guard the two attacker-reachable call sites that pass ->pprev so genesis is rejected with misbehavior scoring / empty members rather than an abort.
…to-end GetAllQuorumMembers() can legitimately return an empty member set (disabled LLMQ type, out-of-range quorumIndex, and now a parentless quorum base). CFinalCommitment::VerifySignatureAsync indexed members[0] unconditionally on the is_single_member() branch, so an empty set is an out-of-bounds read. No built-in LLMQ type has size == 1, but regtest -llmqtestparams and devnet -llmqdevnetparams both allow it. Reject an empty member set explicitly instead. Add llmq_commitment_tests/commitment_genesis_quorum_hash_rejected_test, which drives the real consensus entry point CheckLLMQCommitment with a mined commitment naming the genesis block. Verified that removing the IsQuorumTypeEnabled null guard makes this test abort the binary (EXIT=134), which the previous direct-call test did not. Also correct the stale reproduction comment in evo_utils_tests: the nullptr literal there was a compile error pre-fix, not the abort site.
7f7b427 to
2204e00
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2204e009e3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (members.empty()) { | ||
| LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] no quorum members\n", quorumHash.ToString()); | ||
| return false; |
There was a problem hiding this comment.
When an empty member set reaches block verification, this new return false does not reject the block because CQuorumBlockProcessor::ProcessBlock ignores VerifySignatureAsync's return value and then treats an empty check queue as successful. This is reachable with the accepted zero-threshold regtest/devnet quorum overrides: the count checks accept all-false member bitsets, while a genesis quorum base produces no members, so the commitment can be processed without either BLS signature being verified. Have the queued caller reject a false return (and cover that caller in the regression test) rather than relying solely on the return here.
AGENTS.md reference: AGENTS.md:L164-L164
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Both carried-forward blocking findings remain valid at exact head 2204e00: asynchronous block verification still discards the empty-member failure, and the first commit remains independently non-buildable. The latest delta contains no genuinely new findings; the three PR patches are unchanged by the rebase, and no author reply or subsequent code change resolves either blocker.
Source: reviewers codex/general=gpt-5.6-sol and codex/dash-core-commit-history=gpt-5.6-sol; verifier codex/verifier=gpt-5.6-sol; openclaw-agent coordinator is orchestration-only.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/blockprocessor.cpp`:
- [BLOCKING] src/llmq/blockprocessor.cpp:231: Propagate the empty-member signature verification failure
`CFinalCommitment::VerifySignatureAsync()` now returns `false` at `src/llmq/commitment.cpp:60-62` before adding any queued BLS checks when member selection is empty, but this caller discards that result and relies only on `queue_control.Wait()`. The accepted `-llmqtestparams` and `-llmqdevnetparams` parsers permit a zero threshold and copy it into `minSize`, so a positive-size commitment with all-false signer/member bitsets and structurally valid non-null BLS fields can pass `Verify(..., false)` when no members are selected. Before this PR, the queued `BlsCheck` with no public keys returned `false`; after the early return, the queue can be empty, `Wait()` succeeds, and `ProcessCommitment()` repeats verification without signatures and can accept the commitment. Reject a false asynchronous-verification result here, or move the empty-member rejection into the non-signature portion of `CFinalCommitment::Verify()`. The added genesis test uses a positive `minSize` and fails in the bitset validation loop, so it does not cover this bypass.
2 participants
Issue being fixed or feature implemented
ChainstateManager::IsQuorumTypeEnabledtookgsl::not_null<const CBlockIndex*>. Three call sites pass apprevthat is null when the quorum base index is genesis:llmq::utils::GetAllQuorumMembers,NetDKG::ProcessMessage, and indirectlyCFinalCommitment::Verify/VerifySignatureAsync.The
not_nullconverting constructor callsExpects(), which routes togsl::details::terminate(). That function is[[noreturn]] noexcept, so this is a process abort and not an exception - thetry/catch(std::exception&)inCheckSpecialTxcannot contain it. It is reached before any signature check, sinceGetAllQuorumMembersruns ahead of thecheckSigsblock inVerify.Three entry points, with different gates:
qfcommitfrom an unauthenticated peer.CQuorumBlockProcessor::ProcessMessageis dispatched with no MNAuth gate. An attacker setsquorumHashto the genesis hash. All pre-Verifygates pass except the "too old" height check, so this requires the victim's height to be at or below the DKG interval - i.e. any node in the first minutes of a fresh sync, and trivially on regtest/devnet.What was done?
IsQuorumTypeEnabledto accept a raw pointer and returnfalseon null, fixing the sink itself.GetAllQuorumMembersand in the DKG message path, covering all three entry points includingProcessSpecialTxsInBlock.members[0]in the single-member branch.On consensus safety of the empty-members check: it has no consensus implication. An empty member set already fails
Verify'svalidMembers/signersbitset loop withbad-qc-invalid, and the aggregate BLS path with an empty pubkey vector can never verify true, so nothing currently accepted becomes rejected.Reviewer note: relaxing the signature from
not_nullto a raw pointer weakens the contract for all other callers. If reviewers prefer, the alternative is to keepnot_nulland null-check at every call site instead; that was judged noisier for the same guarantee.How Has This Been Tested?
The first commit adds a regression test covering the genesis/null-pprev terminate path, ordered before the fix.
Full build and test validation is delegated to CI on this PR; the changes were not built locally.
Breaking Changes
None.
Checklist: