fix: validate CbTx chainlock height diff

[thepastaclaw]

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]

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

[github-actions]

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

• feat: add masternode reward shares #7340 - feat: add masternode reward shares

Please coordinate with the authors of these PRs to avoid merge conflicts.

[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: 32f988ad-9a7b-49e9-8907-18f2bfd0f6a4

📥 Commits

Reviewing files that changed from the base of the PR and between 7312fff and dc82f29.

📒 Files selected for processing (2)

• src/evo/specialtxman.cpp
• src/test/evo_cbtx_tests.cpp

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

• src/evo/specialtxman.cpp
• src/test/evo_cbtx_tests.cpp

---

Walkthrough

This PR introduces stricter validation for the bestCLHeightDiff field in Dash coinbase transactions. The changes export a validation function that checks both the height difference bounds (rejecting values that would imply pre-genesis ancestors) and safely handles missing block indices during ancestor lookups. A unit test verifies rejection at boundary conditions, and the test is integrated into the build system.

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 33.33% 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 specifically describes the main change: validating CbTx chainlock height diff, which is the core issue being fixed across all modified files.
Description check	✅ Passed	The description is directly related to the changeset, explaining the validation gap being fixed, what was done, testing results, and impact, with clear alignment to the code changes.
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]

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]

✅ Review complete (commit dc82f29)

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

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