fix: validate CbTx chainlock height diff by thepastaclaw · Pull Request #7363 · dashpay/dash

thepastaclaw · 2026-06-13T14:14:39Z

Issue being fixed or feature implemented

CheckCbTxBestChainlock derived an ancestor height from bestCLHeightDiff
without first ensuring the derived height was in range. This PR adds an
explicit consensus rejection for out-of-range CbTx chainlock height diffs before
ancestor lookup.

What was done?

Validate bestCLHeightDiff before deriving and using the referenced chainlock
ancestor height.
Return bad-cbtx-cldiff for out-of-range values.
Add a defensive null check around the ancestor lookup.
Add focused unit coverage for boundary values in a new evo_cbtx_tests suite.

How Has This Been Tested?

Tested locally on macOS arm64:

cd src && make -j6 test/test_dash
./test/test_dash --run_test=evo_cbtx_tests
./test/test_dash '--run_test=evo_*'

Results:

make -j6 test/test_dash: pass
./test/test_dash --run_test=evo_cbtx_tests: 1/1 pass
./test/test_dash '--run_test=evo_*': 38/38 pass

Pre-PR review gate:

code-review dashpay/dash upstream/develop review/fix-cbtx-chainlock-height-bound "Pre-PR review: validate CbTx bestCLHeightDiff range before ancestor lookup and add focused regression tests"

Result: ship.

Breaking Changes

None expected. This only rejects malformed CbTx chainlock metadata that
references an invalid ancestor height.

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

thepastaclaw · 2026-06-13T14:14:48Z

@coderabbitai review

coderabbitai · 2026-06-13T14:14:54Z

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

github-actions · 2026-06-13T14:15:23Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2026-06-13T14:19:59Z

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: d25648bf-47d8-4e84-9582-baeb8d8b6a7e

📥 Commits

Reviewing files that changed from the base of the PR and between 84ff99f and c93049f.

📒 Files selected for processing (4)

src/Makefile.test.include
src/evo/specialtxman.cpp
src/evo/specialtxman.h
src/test/evo_cbtx_tests.cpp

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

src/Makefile.test.include
src/evo/specialtxman.h
src/evo/specialtxman.cpp

Walkthrough

This PR exports CheckCbTxBestChainlock, adds bounds and null checks in its ChainLock validation path, adds a unit test for rejected bestCLHeightDiff values, and registers the new test file in the test build list.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

UdjinM6
PastaPastaPasta

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.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 names the main change: validating the CbTx chainlock height diff.
Description check	✅ Passed	The description matches the code changes and testing, including the new range check, null guard, and unit 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 · 2026-06-13T15:34:59Z

CI note: the only red item is predict_conflicts; the full build/test matrix is green. The predictor is flagging overlapping test/build files and Evo specialtx files against other open PRs (including #7340 and nearby queued work). I’m leaving this as a merge-sequencing/conflict-prediction blocker rather than an application/test failure for now.

thepastaclaw · 2026-06-15T12:03:04Z

✅ Review complete (commit c93049f)

thepastaclaw

Code Review

Small, well-scoped defensive fix that closes a remote crash vector in CheckCbTxBestChainlock by rejecting out-of-range bestCLHeightDiff before calling GetAncestor(), plus a defensive null check on the returned ancestor. The boundary math is correct and the change is strictly more-rejecting, so it cannot reject any historically valid block. Two minor non-blocking observations about test coverage and reject-reason granularity.

🟡 1 suggestion(s) | 💬 1 nitpick(s)

🤖 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/test/evo_cbtx_tests.cpp`:
- [SUGGESTION] src/test/evo_cbtx_tests.cpp:56-62: Defensive GetAncestor() null-check branch is not exercised
  Both test cases set bestCLHeightDiff to nHeight (5) and uint32_max, so both trip the range check at specialtxman.cpp:77 and return before the new `if (pAncestor == nullptr)` branch at specialtxman.cpp:89 is reached. The commit message highlights the null check as an intentional defense, but from the unit test's perspective that branch is currently unexercised. A case with `bestCLHeightDiff = nHeight - 1` on a standalone CBlockIndex (pprev == nullptr) would pass the range check, compute curBlockCoinbaseCLHeight = 0, and drive GetAncestor() down a chain it cannot walk, covering the defensive path.

thepastaclaw

Code Review

Small, well-scoped consensus-safety fix in CheckCbTxBestChainlock that rejects an out-of-range bestCLHeightDiff before it can produce a pre-genesis ancestor height, plus the defensive null check after GetAncestor() and a focused unit test for the boundary. Current head (dc82f29) addresses prior review nits by renaming the defensive reject reason to bad-cbtx-cldiff-ancestor and documenting why that branch is unreachable from a unit test. No in-scope code defects at this SHA; only a minor commit-history suggestion remains.

💬 1 nitpick(s)

Non-inline nitpick — Squash dc82f29 into 7312fff (or reword its subject)

dc82f29 only touches lines introduced one commit earlier by 7312fff: it renames the defensive reject reason from bad-cbtx-cldiff to bad-cbtx-cldiff-ancestor in src/evo/specialtxman.cpp and adds explanatory comments there and in the new unit test. Functionally this is the final shape of the original fix split across two commits, with the follow-up carrying a test: prefix even though it changes a production consensus-validation reject string in specialtxman.cpp. Since Dash Core preserves PR commits when merging via Merge #NNNN, leaving the split means git bisect/git blame for the reject string lands on a commit whose subject advertises only test changes. Recommend git rebase -i --autosquash (mark dc82f29 as fixup!) to fold it into 7312fff; if the split is intentionally preserved, reword its subject to something like evo: distinguish bad-cbtx-cldiff-ancestor reject reason and describe the rename in the body.

thepastaclaw · 2026-06-19T22:15:44Z

CI follow-up on the current red checks:

check_merge is failing in the workflow's git merge --ff-only base_branch path after checking out master, even though the PR branch merges cleanly into develop first and GitHub reports this PR as MERGEABLE.
The remaining predict_conflicts failures are the same conflict-prediction/sequencing signal already noted earlier; the actual Dash build/test matrix is not failing on this diff.

Leaving the branch unchanged: this does not appear to be an application/test regression in the CbTx height-diff validation change, and I am not going to force-push/no-op rebase just to churn CI.

PastaPastaPasta

utACK dc82f29

github-actions · 2026-06-24T02:25:25Z

This pull request has conflicts, please rebase.

thepastaclaw

Code Review

The CbTx bestCLHeightDiff range check is technically correct and well-tested. Both agents converge on the same single carried-forward nitpick: the follow-up commit 84ff99fd4c is prefixed test: but mutates a production consensus-validation reject string, which becomes misleading permanent history once merged. No new defects in the latest delta.

💬 1 nitpick(s)

thepastaclaw · 2026-06-24T04:51:23Z

+        if (pAncestor == nullptr) {
+            // Defense-in-depth: the range check above keeps curBlockCoinbaseCLHeight in
+            // [0, pindex->nHeight - 1], so GetAncestor() should never return nullptr here.
+            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff-ancestor");


💬 Nitpick: Follow-up commit titled test: actually renames a production reject string

Carried forward from the prior review at dc82f29959 and still valid at 84ff99fd4c. Commit 84ff99fd4c ("test: clarify CbTx chainlock diff ancestor guard") is not a pure test/comment change: it renames the production consensus-validation reject reason from bad-cbtx-cldiff to bad-cbtx-cldiff-ancestor in src/evo/specialtxman.cpp (the defensive GetAncestor() == nullptr branch). Functionally the result is fine and the differentiated reject reasons are actually clearer than collapsing them — the issue is purely commit hygiene: Dash Core preserves PR commits on merge, so a permanent commit titled test: ... that mutates a consensus-rejection string is misleading to future git log -- src/evo/specialtxman.cpp / git blame / git log -S 'bad-cbtx-cldiff-ancestor' readers. Recommend git rebase -i --autosquash to fold 84ff99fd4c into ebedd1ba5e, or at minimum reword its subject to something like fix: rename CbTx ancestor-guard reject reason so the permanent history accurately describes the production change.

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

github-actions · 2026-06-25T20:20:36Z

This pull request has conflicts, please rebase.

Reject an out-of-range bestCLHeightDiff that would otherwise produce a pre-genesis ancestor height when combined with pindex->nHeight, returning state.Invalid(BLOCK_CONSENSUS, "bad-cbtx-cldiff") before any GetAncestor() lookup. The GetAncestor() result is also null-checked as defense-in-depth. CheckCbTxBestChainlock is exposed in specialtxman.h alongside the existing Check* prototypes so a focused unit test can cover the new boundary in src/test/evo_cbtx_tests.cpp. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

thepastaclaw

Code Review

Reviewed current HEAD c93049f311. The CbTx bestCLHeightDiff hardening is clean and well-scoped: the range check bounds the derived chainlock height before cached/ancestor lookup, the defensive GetAncestor() branch is documented, and the unit test covers the rejected boundary values. No blocking issues.

Carried-forward prior findings:

STILL VALID: the top commit c93049f311 is titled test: clarify CbTx chainlock diff ancestor guard, but it also renames the production consensus-validation reject reason in src/evo/specialtxman.cpp from bad-cbtx-cldiff to bad-cbtx-cldiff-ancestor. Functionally the rename is fine; the issue is commit-history hygiene because Dash Core preserves PR commits on merge.

New findings in latest delta (84ff99fd..c93049f3): none.

CodeRabbit had 0 inline findings. Coverage note: claude-general and claude-dash-core-commit-history completed; both Codex ACP reviewers failed authentication and produced no findings.

💬 1 nitpick(s)

thepastaclaw · 2026-06-26T05:57:36Z

+        if (pAncestor == nullptr) {
+            // Defense-in-depth: the range check above keeps curBlockCoinbaseCLHeight in
+            // [0, pindex->nHeight - 1], so GetAncestor() should never return nullptr here.
+            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff-ancestor");


💬 Nitpick: test:-prefixed commit c93049f also renames a production reject string

Verified at HEAD c93049f: the commit titled test: clarify CbTx chainlock diff ancestor guard is not a pure test/comment change. Its diff against src/evo/specialtxman.cpp renames the production consensus reject reason returned at line 155 from bad-cbtx-cldiff to bad-cbtx-cldiff-ancestor (plus a new explanatory comment). Functionally the new name is an improvement — it disambiguates the unreachable defensive branch from the actively-used range-check rejection on line 141, which helps log triage. The issue is purely commit hygiene: dashpay/dash preserves PR commits on merge to develop, so a future git log --grep=cldiff or git blame on this line will land on a commit whose test: subject hides the user-visible reject-string change. Either fold the production rename into the prior fix: commit c892349 and keep only the test/comment additions here, or re-title this commit (e.g. refactor: distinguish CbTx chainlock ancestor reject reason) so the subject reflects the production rename. No code-correctness implication.

_{source: ['claude']}

UdjinM6

utACK c93049f

thepastaclaw marked this pull request as ready for review June 15, 2026 12:00

thepastaclaw commented Jun 15, 2026

View reviewed changes

Comment thread src/test/evo_cbtx_tests.cpp

Comment thread src/evo/specialtxman.cpp

github-actions Bot mentioned this pull request Jun 15, 2026

feat: add masternode reward shares #7340

Merged

5 tasks

thepastaclaw commented Jun 19, 2026

View reviewed changes

PastaPastaPasta previously approved these changes Jun 20, 2026

View reviewed changes

PastaPastaPasta requested a review from UdjinM6 June 20, 2026 09:10

PastaPastaPasta added this to the 24 milestone Jun 20, 2026

thepastaclaw force-pushed the fix-cbtx-chainlock-height-bound branch from dc82f29 to 84ff99f Compare June 24, 2026 02:24

github-actions Bot added the needs rebase label Jun 24, 2026

thepastaclaw commented Jun 24, 2026

View reviewed changes

github-actions Bot removed the needs rebase label Jun 25, 2026

github-actions Bot added the needs rebase label Jun 25, 2026

thepastaclaw and others added 2 commits June 26, 2026 00:41

test: clarify CbTx chainlock diff ancestor guard

c93049f

thepastaclaw dismissed PastaPastaPasta’s stale review via c93049f June 26, 2026 05:43

thepastaclaw force-pushed the fix-cbtx-chainlock-height-bound branch from 84ff99f to c93049f Compare June 26, 2026 05:43

github-actions Bot added needs rebase and removed needs rebase labels Jun 26, 2026

thepastaclaw commented Jun 26, 2026

View reviewed changes

github-actions Bot removed the needs rebase label Jun 26, 2026

UdjinM6 approved these changes Jun 26, 2026

View reviewed changes

Uh oh!

Conversation

thepastaclaw commented Jun 13, 2026

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist

Uh oh!

thepastaclaw commented Jun 13, 2026

Uh oh!

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

Uh oh!

Uh oh!

github-actions Bot commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

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

Uh oh!

Walkthrough

Estimated code review effort

Suggested reviewers

❌ Failed checks (1 warning)

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

thepastaclaw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw commented Jun 19, 2026

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 25, 2026

Uh oh!

thepastaclaw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jun 13, 2026 •

edited

Loading

github-actions Bot commented Jun 13, 2026 •

edited

Loading

coderabbitai Bot commented Jun 13, 2026 •

edited

Loading

thepastaclaw commented Jun 13, 2026 •

edited

Loading

thepastaclaw commented Jun 15, 2026 •

edited

Loading

thepastaclaw left a comment •

edited

Loading

thepastaclaw left a comment •

edited

Loading