Skip to content

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

Merged
PastaPastaPasta merged 2 commits into
dashpay:developfrom
thepastaclaw:fix-v24-mn-payment-multiplicity
Jul 8, 2026
Merged

fix(consensus): require multiplicity-correct MN payment matching after v24#7377
PastaPastaPasta merged 2 commits into
dashpay:developfrom
thepastaclaw:fix-v24-mn-payment-multiplicity

Conversation

@thepastaclaw

Copy link
Copy Markdown

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

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

@github-actions

Copy link
Copy Markdown

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
✅ 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

Copy link
Copy Markdown
Author

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 developmaster relationship, not a conflict introduced by this PR.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

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: 4d52b667-5a23-4387-a5f2-24bd72f9403e

📥 Commits

Reviewing files that changed from the base of the PR and between 3d8c868 and 0a40716.

📒 Files selected for processing (7)
  • src/Makefile.test.include
  • src/masternode/payments.cpp
  • src/masternode/payments.h
  • src/test/evo_deterministicmns_tests.cpp
  • src/test/masternode_payments_tests.cpp
  • src/validation.cpp
  • test/util/data/non-backported.txt
🚧 Files skipped from review as they are similar to previous changes (3)
  • test/util/data/non-backported.txt
  • src/Makefile.test.include
  • src/masternode/payments.h

Walkthrough

Adds FindUnmatchedMasternodePayment for comparing expected and actual masternode payout outputs with optional strict multiplicity. CMNPaymentsProcessor::IsTransactionValid now uses that helper, and CChainState::ConnectBlock passes the V24 activation flag into payee validation. New unit tests cover helper matching behavior and the V24 boundary case, and the new test file is added to the build and non-backported list.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ConnectBlock as CChainState::ConnectBlock
  participant IsBlockPayeeValid as CMNPaymentsProcessor::IsBlockPayeeValid
  participant IsTransactionValid as CMNPaymentsProcessor::IsTransactionValid
  participant FindUnmatchedMasternodePayment as FindUnmatchedMasternodePayment

  ConnectBlock->>IsBlockPayeeValid: is_v24_active, check_superblock
  IsBlockPayeeValid->>IsTransactionValid: strict_multiplicity
  IsTransactionValid->>FindUnmatchedMasternodePayment: expected outputs, coinbase outputs
  FindUnmatchedMasternodePayment-->>IsTransactionValid: unmatched index or -1
  IsTransactionValid-->>IsBlockPayeeValid: true/false
  IsBlockPayeeValid-->>ConnectBlock: validation result
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 changes masternode payout output handling and validation, touching duplicate-output handling in the same validation area.
  • dashpay/dash#7340: Related through masternode reward/output multiplicity and expected payout construction.

Suggested reviewers: PastaPastaPasta, UdjinM6

🚥 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 consensus change: stricter masternode payment matching after v24.
Description check ✅ Passed The description accurately describes the payment-matching fix, v24 gating, and the added 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 thepastaclaw marked this pull request as ready for review June 25, 2026 22:20
@thepastaclaw

thepastaclaw commented Jun 25, 2026

Copy link
Copy Markdown
Author

✅ Review complete (commit 0a40716)

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
✅ 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 thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Author

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

@thepastaclaw thepastaclaw force-pushed the fix-v24-mn-payment-multiplicity branch from 4600b46 to e541b35 Compare July 4, 2026 14:27

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e541b35696

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/masternode/payments.cpp Outdated
// After v24 activation each expected payment must be matched by a distinct coinbase output:
// duplicate expected outputs require duplicate coinbase outputs. Pre-v24 retains the legacy
// existence-only check to avoid tightening historical validation.
const bool strict_match{DeploymentActiveAfter(pindexPrev, m_chainman, Consensus::DEPLOYMENT_V24)};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Pass an available deployment context

This path now references m_chainman, but CMNPaymentsProcessor only stores the deterministic MN manager, superblock manager, and consensus params, so any build that compiles payments.cpp will fail with an unknown member/identifier here. Use an available v24 context (or plumb ChainstateManager into the processor) before calling the chainman overload of DeploymentActiveAfter.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/masternode/payments.cpp`:
- Line 234: The strict-match check in CMNPaymentsProcessor is calling
DeploymentActiveAfter with m_chainman even though the processor does not own
that member, so it will not compile. Update the relevant CMNPaymentsProcessor
method to either accept ChainstateManager directly or take a precomputed V24
activation flag from the caller, and then use that value in the strict_match
initialization instead of referencing m_chainman.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f4b89ab6-d256-4079-ac98-318403d91ff1

📥 Commits

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

📒 Files selected for processing (5)
  • src/Makefile.test.include
  • src/masternode/payments.cpp
  • src/masternode/payments.h
  • src/test/masternode_payments_tests.cpp
  • test/util/data/non-backported.txt
✅ Files skipped from review due to trivial changes (1)
  • test/util/data/non-backported.txt
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/masternode/payments.h
  • src/Makefile.test.include

Comment thread src/masternode/payments.cpp Outdated

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

New blocking finding at head e541b35: CMNPaymentsProcessor::IsTransactionValid references m_chainman, but the class in src/masternode/payments.h:74-97 has no such member and the constructor never receives one, and validation.h is not included, so this translation unit cannot compile. Carried-forward prior-1 (squash the one-line non-backported.txt follow-up commit into its parent) is STILL_VALID at the current head — the stack is still df0dcae + e541b35 with identical shape.

Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5 (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history.

🔴 1 blocking | 💬 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/masternode/payments.cpp`:
- [BLOCKING] src/masternode/payments.cpp:234: `m_chainman` is not a member of CMNPaymentsProcessor — this will not compile
  At `src/masternode/payments.cpp:234` the new gate is:

  ```cpp
  const bool strict_match{DeploymentActiveAfter(pindexPrev, m_chainman, Consensus::DEPLOYMENT_V24)};

But CMNPaymentsProcessor in src/masternode/payments.h:74-97 declares only three members — m_dmnman, m_superblocks, and m_consensus_params — and its explicit constructor takes only (CDeterministicMNManager&, governance::SuperblockManager&, const Consensus::Params&). There is no m_chainman member, and IsTransactionValid receives no chainman parameter either. DEPLOYMENT_V24 is a versionbits DeploymentPos, so the overload that would accept a ChainstateManager& is the template in src/validation.h — and src/masternode/payments.cpp does not include validation.h (its includes list stops at deploymentstatus.h, which alone does not expose that overload).

Net effect: this file will not compile as-is (unknown identifier m_chainman, and even if it were declared, the required overload of DeploymentActiveAfter is not visible in this translation unit). Because src/test/masternode_payments_tests.cpp is now linked into test_dash via src/Makefile.test.include, the whole test binary cannot be built either — contradicting the PR description's claim that the new tests were run locally at this SHA.

Resolution options: (a) thread ChainstateManager& m_chainman through the constructor from CChainstateHelper (which already receives a ChainstateManager& at src/evo/chainhelper.h:60 and constructs CMNPaymentsProcessor in chainhelper.cpp:30) and add #include <validation.h>; (b) have the caller precompute the v24-active flag and pass it into IsTransactionValid/IsBlockPayeeValid; or (c) pass both const Consensus::Params& and VersionBitsCache& and use the 4-arg overload. Option (b) is the smallest change and keeps the processor free of chain-state dependencies.

</details>

_Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the verified blocking finding._

Comment thread src/masternode/payments.cpp Outdated
// After v24 activation each expected payment must be matched by a distinct coinbase output:
// duplicate expected outputs require duplicate coinbase outputs. Pre-v24 retains the legacy
// existence-only check to avoid tightening historical validation.
const bool strict_match{DeploymentActiveAfter(pindexPrev, m_chainman, Consensus::DEPLOYMENT_V24)};

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: m_chainman is not a member of CMNPaymentsProcessor — this will not compile

At src/masternode/payments.cpp:234 the new gate is:

const bool strict_match{DeploymentActiveAfter(pindexPrev, m_chainman, Consensus::DEPLOYMENT_V24)};

But CMNPaymentsProcessor in src/masternode/payments.h:74-97 declares only three members — m_dmnman, m_superblocks, and m_consensus_params — and its explicit constructor takes only (CDeterministicMNManager&, governance::SuperblockManager&, const Consensus::Params&). There is no m_chainman member, and IsTransactionValid receives no chainman parameter either. DEPLOYMENT_V24 is a versionbits DeploymentPos, so the overload that would accept a ChainstateManager& is the template in src/validation.h — and src/masternode/payments.cpp does not include validation.h (its includes list stops at deploymentstatus.h, which alone does not expose that overload).

Net effect: this file will not compile as-is (unknown identifier m_chainman, and even if it were declared, the required overload of DeploymentActiveAfter is not visible in this translation unit). Because src/test/masternode_payments_tests.cpp is now linked into test_dash via src/Makefile.test.include, the whole test binary cannot be built either — contradicting the PR description's claim that the new tests were run locally at this SHA.

Resolution options: (a) thread ChainstateManager& m_chainman through the constructor from CChainstateHelper (which already receives a ChainstateManager& at src/evo/chainhelper.h:60 and constructs CMNPaymentsProcessor in chainhelper.cpp:30) and add #include <validation.h>; (b) have the caller precompute the v24-active flag and pass it into IsTransactionValid/IsBlockPayeeValid; or (c) pass both const Consensus::Params& and VersionBitsCache& and use the 4-arg overload. Option (b) is the smallest change and keeps the processor free of chain-state dependencies.

source: ['opus-general', 'sonnet5-general', 'codex-general']

src/test/dynamic_activation*.cpp
src/test/evo*.cpp
src/test/llmq*.cpp
src/test/masternode_payments_tests.cpp

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Consider squashing the test-registration follow-up into the parent fix commit (prior-1 STILL_VALID)

Carried forward from the prior review of 4600b46 (prior-1). At current head e541b35 the stack shape is unchanged after the rebase: df0dcae (fix(consensus): require multiplicity-correct MN payment matching after v24) introduces src/test/masternode_payments_tests.cpp and registers it in src/Makefile.test.include, and e541b35 (test: register MN payment test as Dash-only) is a one-line append of that same file's path to test/util/data/non-backported.txt. Because Dash merges PR stacks without squashing, df0dcae lands in git log in a state where the new Dash-only test file is not yet covered by the Dash-only clang-format/cppcheck lint gates until the very next commit — a mild bisect/blame hazard, and the review-fix rationale is not preserved in permanent history. Folding e541b35 into df0dcae via interactive rebase would land the fix, its tests, and the lint-list registration as one coherent unit. Non-blocking — the standalone commit is self-explanatory and reasonable to keep as-is.

source: ['opus-general', 'sonnet5-general', 'codex-general', 'opus-dash-core-commit-history', 'sonnet5-dash-core-commit-history', 'codex-dash-core-commit-history']

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Prior blocking m_chainman compile failure at df0dcae/e541b35696 is FIXED at head 828854a — the fix threads a caller-computed strict_multiplicity bool through IsBlockPayeeValid/IsTransactionValid, mirroring the existing MnRewardEra pattern, and validation.cpp computes is_v24_active in ConnectBlock where m_chainman is in scope. The only carried-forward concern is commit-history hygiene: df0dcae remains a non-compiling intermediate commit in a consensus-critical file, and both follow-ups (e541b35 lint registration, 828854a build fix) belong squashed into the parent so the stack lands as one coherent, bisectable commit. No new code defects.

Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5 (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history.

🟡 1 suggestion(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/masternode/payments.cpp`:
- [SUGGESTION] src/masternode/payments.cpp:216-249: Squash the two follow-up commits into df0dcae107 so the consensus change lands as a single bisectable commit
  The stack is currently df0dcae107 (consensus tightening + new test file) → e541b35696 (one-line non-backported.txt registration of that test file) → 828854a9e7 (build fix that replaces an invalid m_chainman reference with a threaded strict_multiplicity parameter). The build-fix commit's own message confirms it repairs a compile break introduced by df0dcae107: at that commit `src/masternode/payments.cpp` calls `DeploymentActiveAfter(pindexPrev, m_chainman, …)` but `CMNPaymentsProcessor` in `src/masternode/payments.h:74-97` has no `m_chainman` member and no such argument is passed to its constructor, so the payments TU (and therefore `test_dash`, since df0dcae107 also adds `src/test/masternode_payments_tests.cpp` to the build) cannot link at that commit. Because Dash merges PR stacks without squashing, this leaves a consensus-critical, non-compiling intermediate commit in permanent `develop` history — a real `git bisect` hazard for any future regression search across this range. e541b35696 is likewise a mechanical follow-up that has no independent rationale from its parent (the parent is what created the file being registered). Please interactive-rebase and fold e541b35696 and 828854a9e7 into df0dcae107 so the fix, test, lint registration, and threaded-parameter plumbing land as one coherent commit that builds on its own. The final tree state is correct — this is purely history cleanup, but it matters for consensus-critical bisectability.

Comment thread src/masternode/payments.cpp
@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
✅ 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 thepastaclaw force-pushed the fix-v24-mn-payment-multiplicity branch from 828854a to ef5fa2d Compare July 7, 2026 04:35
@thepastaclaw

Copy link
Copy Markdown
Author

Addressed the commit-history review feedback by rewriting the branch to a
single coherent commit:

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

The previous follow-ups e541b35696 and 828854a9e7 are now folded into the
original consensus/test change, so the PR no longer has a non-compiling
intermediate commit.

Local validation:

git diff --check upstream/develop..HEAD
make -C src test/test_dash
src/test/test_dash --run_test=masternode_payments_tests
COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
git ls-files -- $(cat test/util/data/non-backported.txt) | grep '^src/test/masternode_payments_tests.cpp$'
python3 test/lint/lint-cppcheck-dash.py

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
✅ 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 thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Carried-forward prior finding prior-1 is FIXED: the current stack is a single commit, ef5fa2d, on top of upstream/develop, so the earlier bisectability/history issue no longer applies. New latest-delta findings: none; the PR diff implements v24-gated multiplicity-correct masternode payment matching, updates the only call site, registers the unit test, and git diff --check upstream/develop...HEAD is clean. Reviewer coverage disclosure: codex-general and codex-dash-core-commit-history ran and produced parseable no-finding outputs; claude-general and claude-dash-core-commit-history did not produce parseable output because they failed with an extra-usage runtime error, and CodeRabbit had no actionable inline findings.

Source: reviewers: codex-general (gpt-5.5), codex-dash-core-commit-history (gpt-5.5); failed lanes: claude-general (opus, extra-usage limit), claude-dash-core-commit-history (opus, extra-usage limit); verifier: codex (gpt-5.5). Specialist: dash-core-commit-history.

Prior reconciliation:

  • prior-1 FIXED: FIXED. The current PR history contains exactly one commit over upstream/develop (git rev-list --count upstream/develop..HEAD returns 1), and that commit includes the consensus change, the strict-multiplicity plumbing, the unit test registration in src/Makefile.test.include, and the test/util/data/non-backported.txt entry.

Carried-forward prior findings: none.

New latest-delta findings: none.

Note: GitHub rejected the automated self-APPROVE event with HTTP 422, so this exact-SHA clean review is posted as COMMENT while preserving the verifier result.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
✅ 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.

@PastaPastaPasta PastaPastaPasta requested a review from knst July 7, 2026 14:15

// Check for correct masternode payment
if (IsTransactionValid(txNew, pindexPrev, blockSubsidy, feeReward, era)) {
if (IsTransactionValid(txNew, pindexPrev, blockSubsidy, feeReward, era, strict_multiplicity)) {

@knst knst Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider changing strict_multiplicity == true to check_superblock == SuperBlockCheckType::DisallowDuplicates for CMNPaymentsProcessor::IsBlockPayeeValid to reduce amount of arguments.

They are semantically the same meaning.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to keep these separate. check_superblock == DisallowDuplicates also encodes whether superblock validation is being run at all: NoCheck is used for chainlocked blocks / during sync, even when v24 is active. IsBlockPayeeValid() still validates the masternode payee before the later NoCheck return, so deriving strict multiplicity from check_superblock would disable the new v24 masternode-payment multiplicity rule in those skip-superblock cases. The extra bool is a little noisier, but it preserves the deployment-state distinction from the superblock-check mode.

@thepastaclaw

Copy link
Copy Markdown
Author

Verified the commit-history feedback from the 828854a9e7 self-review is already addressed on the current PR branch.

Current head is a single coherent commit:

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

df0dcae107, e541b35696, and 828854a9e7 are no longer ancestors of the branch, and git rev-list --count upstream/develop..HEAD returns 1, so the non-compiling intermediate history is gone.

Fresh local validation in worktrees/tracker-1839:

git diff --check upstream/develop..HEAD
COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
test/lint/lint-tests.py
./autogen.sh
./configure --without-gui
make -C src test/test_dash -j4
src/test/test_dash --run_test=masternode_payments_tests

Push result was Everything up-to-date; current PR CI is green and latest CodeRabbit command completed without actionable comments.

@UdjinM6 UdjinM6 added this to the 24 milestone Jul 8, 2026

@UdjinM6 UdjinM6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls consider cc62957

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

@thepastaclaw

Copy link
Copy Markdown
Author

Adopted Udjin's suggested test commit on top of the PR branch:

3d8c868f5e0 test: cover the v24 MN payment multiplicity boundary end-to-end

This adds evo_dip3_activation_tests/mn_payment_multiplicity_v24_boundary, covering the full coinbase construction/ConnectBlock path across the v24 boundary with the owner/operator duplicate-output case.

Local validation in worktrees/tracker-1586:

./autogen.sh
./configure --without-gui
make -C src test/test_dash -j4
src/test/test_dash --run_test=evo_dip3_activation_tests/mn_payment_multiplicity_v24_boundary
src/test/test_dash --run_test=evo_dip3_activation_tests
src/test/test_dash --run_test=masternode_payments_tests
git diff --check upstream/develop..HEAD
COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
test/lint/lint-tests.py

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
✅ 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 and others added 2 commits July 8, 2026 10:50
…r v24

CMNPaymentsProcessor::IsTransactionValid previously checked each expected masternode coinbase output with std::ranges::any_of, which made matching existence-only: a single actual output could satisfy multiple identical expected outputs. With multi-payout reward shares, expected outputs can legitimately include duplicates, so this mismatch is consensus-relevant.

Gate the stricter matching behind DEPLOYMENT_V24 to avoid tightening historical validation. After v24 activation, every expected output must be matched by a distinct actual output; pre-v24 retains the legacy existence-only behaviour.

Compute v24 activation in ConnectBlock, where chainman is already available, and thread the result through the masternode payment validation path as a strict_multiplicity flag. This keeps the payments processor free of chain-state dependencies.

Extract the matching into FindUnmatchedMasternodePayment so it can be unit-tested directly, add focused regression coverage in masternode_payments_tests, and register the Dash-only test in non-backported.txt for local lint expansion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add evo_dip3_activation_tests/mn_payment_multiplicity_v24_boundary, exercising
the full FillBlockPayments -> ConnectBlock -> IsBlockPayeeValid round-trip
across the v24 activation boundary with a single masternode.

A plain v2 (BasicBLS) masternode -- deliberately NOT multi-payout -- is
registered with a single owner payout and the operator payout pointing at the
same script, with a 50% operator reward, so whenever the masternode reward is
even the owner share and the operator share resolve to two identical coinbase
outputs. The test builds a "merge cheat" coinbase that folds those two
identical outputs into one, redirecting the freed value to the miner so the
total coinbase value is unchanged (IsBlockValueValid still passes) while the
masternode is underpaid, and asserts both sides of the boundary:

  - pre-v24, legacy existence-only matching ACCEPTS the merge cheat;
  - post-v24, strict multiplicity matching REJECTS it, and the faithful
    two-output block connects.

The duplicate is an owner-payout/operator-payout script collision, not a
multi-payout property: multi-payout owner shares are forced to distinct
scripts (bad-protx-payee-dup) and so can never collide. Keeping the same v2 MN
across the boundary makes explicit that strict multiplicity matching is gated
purely on v24 activation, independent of the registrant's ProTx version.

Also adds a TestChainV24PendingSetup fixture that activates v19/v20/mn_rr at
height 500 and stops with v24 signalled but still pending, so the test can
register a basic-BLS MN and then mine across v24 activation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@thepastaclaw thepastaclaw force-pushed the fix-v24-mn-payment-multiplicity branch from 3d8c868 to 0a40716 Compare July 8, 2026 15:55
@thepastaclaw

Copy link
Copy Markdown
Author

Follow-up: after adopting the boundary test, GitHub reported the branch as conflicting with current develop. I rebased the PR branch and resolved the same-file test conflict by keeping both the new upstream ExtNetInfo coverage and the v24 MN-payment multiplicity boundary test.

Current PR commits:

84a6ba162cb fix(consensus): require multiplicity-correct MN payment matching after v24
0a407162de5 test: cover the v24 MN payment multiplicity boundary end-to-end

Rebased validation in worktrees/tracker-1586:

make -C src test/test_dash -j4
src/test/test_dash --run_test=evo_dip3_activation_tests/mn_payment_multiplicity_v24_boundary
src/test/test_dash --run_test=evo_dip3_activation_tests
src/test/test_dash --run_test=masternode_payments_tests
git diff --check upstream/develop..HEAD
COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
test/lint/lint-tests.py

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
✅ 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 thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Source: reviewers: opus general (failed: extra-usage quota), gpt-5.5 general, opus dash-core-commit-history (failed: extra-usage quota), gpt-5.5 dash-core-commit-history; verifier: gpt-5.5.

Carried-forward prior findings: none. Prior 3d8c868f is OUTDATED/NO VERIFIED OUTPUT because that assigned run went stale before reviewer launch/public posting, so there were no verified findings to classify as STILL VALID, FIXED, INTENTIONALLY DEFERRED, or carry forward. The last substantive public review at ef5fa2d8 had already resolved the older commit-history finding and left no active findings.

New findings in latest delta: none. I reviewed both git diff 3d8c868f..0a407162de55e2ad8d2f52b64e01258ea11546bd and the cumulative PR diff from the merge-base; the v24-gated strict multiplicity matching and the boundary/unit coverage are consistent with the PR goal.

Note: posted as a COMMENT review because GitHub does not allow PastaClaw to approve their own PR.

@UdjinM6 UdjinM6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 0a40716

@knst knst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 0a40716

@PastaPastaPasta PastaPastaPasta merged commit cb32e9f into dashpay:develop Jul 8, 2026
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants