fix(consensus): clamp commitment debug logging to bitset size

[thepastaclaw]

Issue being fixed or feature implemented

CheckLLMQCommitment() logs validMembers before
CFinalCommitment::Verify() validates the deserialized bitset sizes. A
malformed quorum commitment payload can provide a valid LLMQ type while
encoding an undersized validMembers bitset, causing the debug log loop to
index past the deserialized vector.

Practical exploitability is low because quorum-commitment transactions are not
accepted from the mempool, and this path requires both a mined malformed block
and non-default -debug=llmq logging. This is still consensus/security-adjacent
hardening because validation should reject the malformed payload without
invoking undefined behavior in debug logging.

What was done?

• Clamp the pre-validation CheckLLMQCommitment() debug-log loop to the smaller
  of the configured LLMQ size and the deserialized validMembers size.
• Leave the existing validation flow unchanged so malformed bitset sizes are
  still rejected by VerifySizes().
• Add a regression test that exercises CheckLLMQCommitment() with an
  undersized validMembers bitset while LLMQ debug logging is enabled.

How Has This Been Tested?

Tested on macOS arm64 in a local Dash Core depends build:

• make -j6 -C src test/test_dash
• src/test/test_dash --run_test=llmq_commitment_tests

The regression test was also checked against the old unclamped loop after
rebuilding the affected test binary; it crashes on the out-of-bounds debug-log
access before this fix and passes with the clamp.

Breaking Changes

None. Valid commitment validation is unchanged; malformed commitments continue
to be rejected by the existing size checks.

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)

[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.

[coderabbitai]

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: fb89a2ac-4ee4-4d51-a1a8-6e68d17aeef1

📥 Commits

Reviewing files that changed from the base of the PR and between b18e7e2 and e862f6b.

📒 Files selected for processing (2)

• src/llmq/commitment.cpp
• src/test/llmq_commitment_tests.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/llmq/commitment.cpp

---

Walkthrough

CheckLLMQCommitment in commitment.cpp now computes log_size as the minimum of the expected LLMQ member count and the actual validMembers bitset size before iterating over it for debug logging. This replaces the prior unconditional iteration over the expected size, which could read past the end of an undersized bitset in malformed payloads. The validation logic (VerifySizes()) is unchanged. A new unit test in llmq_commitment_tests.cpp adds the required headers, constructs a CFinalCommitmentTxPayload with intentionally undersized bitsets, round-trips it through serialization, calls CheckLLMQCommitment, and asserts both the rejection reason ("bad-qc-height") and that the captured debug log contains the clamped empty validMembers[] rendering without the "v[0]" out-of-bounds indicator.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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 clearly and concisely describes the main change: adding clamping to commitment debug logging to prevent out-of-bounds access.
Description check	✅ Passed	The description is directly related to the changeset, explaining the issue being fixed, the solution implemented, and testing performed.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 and usage tips.}

[thepastaclaw]

✅ Review complete (commit e862f6b)

[thepastaclaw]

Code Review

PR clamps the only pre-validation validMembers[i] access in CheckLLMQCommitment()'s debug-log loop to the deserialized bitset size, eliminating the OOB read on malformed commitments while leaving the existing VerifySizes() rejection path intact. The other debug-log loop in CFinalCommitment::Verify() is already protected by a preceding VerifySizes() check, so the PR's scope is complete. Regression test pins the fix.

✅ Verified clean. GitHub does not allow PastaClaw to approve its own PR, so this is posted as a comment review rather than an approval.

[PastaPastaPasta]

utACK b18e7e2

[UdjinM6]

utACK b18e7e2

[PastaPastaPasta]

utACK; assuming CI is happy

[UdjinM6]

utACK e862f6b

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[github-actions]

This pull request has conflicts, please rebase.

[thepastaclaw]

Code Review

Tightly scoped, correct fix: clamps the pre-validation debug-log loop in CheckLLMQCommitment to min(llmq_params.size, validMembers.size()), preventing an OOB read on malformed wire-format DYNBITSET payloads when -debug=llmq is enabled. Validation semantics are unchanged because Verify()/VerifyNull() still reject mismatched sizes via VerifySizes(). The regression test correctly exercises the OOB-prone path by forcing failure on bad-qc-height so the debug log is emitted, then asserts clamped output via a RAII-guarded log callback. Reconciliation: carried-forward prior findings: none (prior PastaClaw review at b18e7e2 found nothing); new findings in latest delta: 0 — the e862f6b delta only adds the #include <algorithm> required by std::min<size_t>. CodeRabbit reported 0 inline findings. GitHub does not allow PastaClaw to approve its own PR, so this verified-clean result is posted as a comment review rather than an approval.

[thepastaclaw]

CI note: the red check_merge jobs are failing in the workflow’s git merge --ff-only base_branch check from master, even though GitHub reports this PR as MERGEABLE and conflict prediction reports no PR conflicts. The diff itself is already approved and the Dash CI build/test jobs are green or still finishing; I’m not going to force-push/no-op rebase just to churn CI.