fix(consensus): require multiplicity-correct MN payment matching after v24

[thepastaclaw]

Issue being fixed or feature implemented

Masternode payment validation currently checks each expected coinbase output with
existence-only matching. If the expected masternode payments contain duplicate
CTxOuts, one actual coinbase output can satisfy multiple expected payments.

This matters after the v24 multi-payout reward-share changes because duplicate
expected outputs can be valid when multiple owner shares resolve to the same
amount and script. Since this tightens consensus validation, the stricter
matching is gated behind DEPLOYMENT_V24 and pre-v24 validation keeps the
legacy behavior.

What was done?

• Added a masternode payment matching helper that can run in legacy
  existence-only mode or strict multiplicity-correct mode.
• Switched CMNPaymentsProcessor::IsTransactionValid to use strict matching
  only after DEPLOYMENT_V24 is active.
• Added focused unit tests covering duplicate expected payments, legacy
  behavior, empty expected payments, missing outputs, and exact amount matching.

How Has This Been Tested?

Ran locally on macOS in
/Users/claw/Projects/dash/worktrees/fix-v24-mn-payment-multiplicity:

./src/test/test_dash --run_test=masternode_payments_tests
./src/test/test_dash --run_test=governance_superblock_tests

Both passed.

Also ran the pre-PR review gate command with the explicit branch ref:

code-review dashpay/dash upstream/develop fix-v24-mn-payment-multiplicity "v24-gated multiplicity-correct masternode payment matching"

The wrapper built the correct 4-file diff, but the Codex reviewer backend failed
with 401 Unauthorized. As a fallback, I ran the same code-reviewer instructions
through local Claude against upstream/develop..HEAD; it returned
Recommendation: ship with no significant findings.

Breaking Changes

This is a consensus tightening after DEPLOYMENT_V24: post-v24 blocks must
include a distinct coinbase output for each expected masternode payment output,
including duplicate expected outputs. Pre-v24 behavior is intentionally
unchanged.

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)

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

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

[thepastaclaw]

The current check_merge failure appears unrelated to this PR branch. The job fast-forwarded PR #7377 onto develop successfully:\n\ntext\nUpdating 8c9f166a3..93be20423\nFast-forward\n\n\nIt failed afterward while checking whether develop can fast-forward into master:\n\ntext\ngit checkout master\ngit merge --ff-only base_branch\nfatal: Not possible to fast-forward, aborting.\n\n\nSo the branch itself is mergeable into develop; this looks like the current repo-level develop → master relationship, not a conflict introduced by this PR.

[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: e3bc87a9-5e3f-402e-8f15-936d822c4be6

📥 Commits

Reviewing files that changed from the base of the PR and between 93be204 and 4600b46.

📒 Files selected for processing (1)

• test/util/data/non-backported.txt

✅ Files skipped from review due to trivial changes (1)

• test/util/data/non-backported.txt

---

Walkthrough

Added FindUnmatchedMasternodePayment to compare expected and actual masternode payout outputs with optional strict multiplicity. CMNPaymentsProcessor::IsTransactionValid now uses strict matching after Consensus::DEPLOYMENT_V24 activation and logs the unmatched payout on failure. Added a unit test file and included it in BITCOIN_TESTS.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TransactionValidation as CMNPaymentsProcessor::IsTransactionValid
  participant DeploymentState as Consensus::DEPLOYMENT_V24
  participant MatchingHelper as FindUnmatchedMasternodePayment
  participant Logger as LogPrintf

  TransactionValidation->>DeploymentState: DeploymentActiveAfter(...)
  DeploymentState-->>TransactionValidation: strict_match flag
  TransactionValidation->>MatchingHelper: compare expected payouts vs txNew.vout
  MatchingHelper-->>TransactionValidation: unmatched index or -1
  alt unmatched payout
    TransactionValidation->>Logger: log expected payout and failure
  end

Loading

Possibly related PRs

• dashpay/dash#7339: Changes the same masternode payment validation path in src/masternode/payments.cpp/payments.h with V24-related output matching behavior.
• dashpay/dash#7362: Also threads is_v24 into payment validation and tightens duplicate-output handling for payment checks.

Suggested reviewers: UdjinM6, PastaPastaPasta

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 27.27% 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 summarizes the main change: stricter masternode payment matching after v24.
Description check	✅ Passed	The description matches the code changes and testing around v24-gated multiplicity-correct payment matching.
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

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

✅ Review complete (commit 4600b46)

[thepastaclaw]

Code Review

Verified one non-blocking in-scope issue: the PR adds a Dash-specific unit test but does not register it in the Dash-only non-backported file list used by local lint gates. Coverage was degraded because the required ACP Claude/Codex reviewer sessions failed authentication; this review is based on native Codex reviewer outputs plus direct source inspection.

🟡 1 suggestion(s)

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 `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:68: Register the new masternode payment test for Dash-only linting
  This PR adds `src/test/masternode_payments_tests.cpp`, but none of the existing patterns in `test/util/data/non-backported.txt` match that file. Both `.github/workflows/clang-diff-format.yml` and `test/lint/lint-cppcheck-dash.py` build their Dash-specific file lists from this data file, so the new test is skipped by those Dash-only format/cppcheck gates unless it is listed here.

[thepastaclaw]

Addressed the review suggestion in 4600b46ebf: added src/test/masternode_payments_tests.cpp to test/util/data/non-backported.txt, so the new Dash-only unit test is included by the Dash-specific clang-format/cppcheck file expansion.

Local checks run:

git ls-files -- $(cat test/util/data/non-backported.txt) | grep '^src/test/masternode_payments_tests.cpp$'
python3 test/lint/lint-cppcheck-dash.py
git diff --check

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

[thepastaclaw]

Code Review

The prior finding about registering src/test/masternode_payments_tests.cpp in test/util/data/non-backported.txt is fixed at line 67 by commit 4600b46. No new in-scope defects were introduced. The only remaining observation is a stylistic nit about squashing the one-line follow-up commit into its parent for a cleaner permanent history; split among the commit-history agents and not a defect.

💬 1 nitpick(s)

[thepastaclaw]

CI diagnosis for the current red Build source jobs: this is not a compile failure from PR #7377. The source-build jobs all stop before configuring/building Dash because the required depends cache cannot be restored with fail-on-cache-miss.

The upstream dependency jobs did build the depends packages successfully, but their cache-save step could not write the cache:

Failed to save: Unable to reserve cache with key ... More details: cache write denied: token has no writable scopes

The later source jobs then fail on the missing exact cache key:

Failed to restore cache entry. Exiting as fail-on-cache-miss is set.

So the PR branch is proper as-is; these failures are a GitHub Actions cache-permission/cache-availability issue for this fork run, not an issue in the masternode payment matching changes. I am leaving the branch untouched.

[thepastaclaw]

@PastaPastaPasta @UdjinM6 this is ready for human Core review from the code side. I diagnosed the red source-build jobs: they fail before compiling Dash because the depends cache could not be saved/restored for the fork run, while the PR branch itself remains mergeable and the change has completed self-review/CodeRabbit. Leaving the branch untouched unless review finds an actual code issue.