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

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 · 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

⚠️ 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 · 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: 31f55f2e-117d-4bcf-be01-e2fb1bf61564

📥 Commits

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

📒 Files selected for processing (4)

src/Makefile.test.include
src/evo/specialtxman.cpp
src/evo/specialtxman.h
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 'fix: validate CbTx chainlock height diff' directly and concisely summarizes the main change: adding validation for the chainlock height difference field in CbTx.
Description check	✅ Passed	The pull request description comprehensively details the issue being fixed, specific changes made, testing performed, and breaking change assessment, all directly related to the changeset.
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

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

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 7312fff)

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 · 2026-06-15T12:37:43Z

+
+    // Upper boundary: uint32_t max.
+    cbTx.bestCLHeightDiff = std::numeric_limits<uint32_t>::max();
+    BlockValidationState state_big;
+    BOOST_CHECK(!CheckCbTxBestChainlock(cbTx, &pindex, consensus_params, chain, qman, chainlocks, state_big));
+    BOOST_CHECK_EQUAL(state_big.GetRejectReason(), "bad-cbtx-cldiff");
+}


🟡 Suggestion: 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.

_{source: ['claude']}

thepastaclaw · 2026-06-15T12:37:43Z

@@ -81,7 +85,11 @@ static bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex,
            cached_pindex = pindex;
            return true;
        }
-        uint256 curBlockCoinbaseCLBlockHash = pindex->GetAncestor(curBlockCoinbaseCLHeight)->GetBlockHash();
+        const CBlockIndex* pAncestor = pindex->GetAncestor(curBlockCoinbaseCLHeight);
+        if (pAncestor == nullptr) {
+            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff");
+        }


💬 Nitpick: Both new rejection paths reuse 'bad-cbtx-cldiff'

The range check at line 78 and the defensive null check at line 90 both return state.Invalid(..., "bad-cbtx-cldiff"). They describe distinct conditions (out-of-range diff vs. missing ancestor on an in-range diff), so collapsing them into one reason makes peer-misbehavior triage and log-based diagnostics slightly less precise. Distinguishing them (e.g. bad-cbtx-cldiff-ancestor for the null case) wouldn't change consensus outcomes but would improve operability. Optional.

_{source: ['claude']}

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

thepastaclaw commented Jun 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: validate CbTx chainlock height diff#7363

fix: validate CbTx chainlock height diff#7363
thepastaclaw wants to merge 1 commit into
dashpay:developfrom
thepastaclaw:fix-cbtx-chainlock-height-bound

thepastaclaw commented Jun 13, 2026

Uh oh!

thepastaclaw commented Jun 13, 2026

Uh oh!

coderabbitai Bot commented Jun 13, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 13, 2026

Uh oh!

coderabbitai Bot commented Jun 13, 2026

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw commented Jun 13, 2026 •

edited

Loading

Uh oh!

thepastaclaw commented Jun 15, 2026 •

edited

Loading

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Jun 15, 2026

Uh oh!

thepastaclaw Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

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

⚠️ Potential Merge Conflicts Detected

Uh oh!

coderabbitai Bot commented Jun 13, 2026

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!

thepastaclaw Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw Jun 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jun 13, 2026 •

edited

Loading

thepastaclaw commented Jun 13, 2026 •

edited

Loading

thepastaclaw commented Jun 15, 2026 •

edited

Loading