fix(consensus): require multiplicity-correct MN payment matching after v24#7377
fix(consensus): require multiplicity-correct MN payment matching after v24#7377thepastaclaw wants to merge 3 commits into
Conversation
|
This pull request has conflicts, please rebase. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
The current |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectBlock as CChainState::ConnectBlock
participant IsBlockPayeeValid as CMNPaymentsProcessor::IsBlockPayeeValid
participant IsTransactionValid as CMNPaymentsProcessor::IsTransactionValid
participant Helper as FindUnmatchedMasternodePayment
ConnectBlock->>IsBlockPayeeValid: is_v24_active, check_superblock
IsBlockPayeeValid->>IsTransactionValid: strict_multiplicity
IsTransactionValid->>Helper: expected outputs, coinbase outputs, strict_multiplicity
Helper-->>IsTransactionValid: unmatched index or -1
IsTransactionValid-->>IsBlockPayeeValid: true/false
IsBlockPayeeValid-->>ConnectBlock: validation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
✅ Review complete (commit 828854a) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
|
Addressed the review suggestion in Local checks run: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
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)
|
CI diagnosis for the current red The upstream dependency jobs did build the depends packages successfully, but their cache-save step could not write the cache: The later source jobs then fail on the missing exact cache key: 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. |
|
@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. |
…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. Extract the matching into FindUnmatchedMasternodePayment so it can be unit-tested directly, and add focused regression coverage in masternode_payments_tests, including the duplicate-expected case that motivates the fix.
thepastaclaw
force-pushed
the
fix-v24-mn-payment-multiplicity
branch
from
4600b46 to
e541b35
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| // 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)}; |
There was a problem hiding this comment.
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (5)
src/Makefile.test.includesrc/masternode/payments.cppsrc/masternode/payments.hsrc/test/masternode_payments_tests.cpptest/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
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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._
| // 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)}; |
There was a problem hiding this comment.
🔴 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 |
There was a problem hiding this comment.
💬 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']
CMNPaymentsProcessor has no m_chainman member, so referencing it in IsTransactionValid broke the build. Compute v24 activation once in ConnectBlock (where it is already evaluated for the superblock check type) and thread it through IsBlockPayeeValid/IsTransactionValid as a strict_multiplicity parameter, keeping the payments module free of chain-state dependencies as done for MnRewardEra. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
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.
| @@ -198,19 +228,22 @@ CAmount GetMasternodePayment(int nHeight, CAmount blockValue, const Consensus::P | |||
| return true; | |||
| } | |||
|
|
|||
| for (const auto& txout : voutMasternodePayments) { | |||
| bool found = std::ranges::any_of(txNew.vout, [&txout](const auto& txout2) { return txout == txout2; }); | |||
| if (!found) { | |||
| std::string str_payout; | |||
| if (CTxDestination dest; ExtractDestination(txout.scriptPubKey, dest)) { | |||
| str_payout = "address=" + EncodeDestination(dest); | |||
| } else { | |||
| str_payout = "scriptPubKey=" + HexStr(txout.scriptPubKey); | |||
| } | |||
| LogPrintf("CMNPaymentsProcessor::%s -- ERROR! Failed to find expected payee %s amount=%lld height=%d\n", | |||
| __func__, str_payout, txout.nValue, nBlockHeight); | |||
| return false; | |||
| // With strict multiplicity (v24 active, computed by the caller) 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 int unmatched_idx = FindUnmatchedMasternodePayment(voutMasternodePayments, txNew.vout, strict_multiplicity); | |||
| if (unmatched_idx >= 0) { | |||
| const auto& txout = voutMasternodePayments[unmatched_idx]; | |||
| std::string str_payout; | |||
| if (CTxDestination dest; ExtractDestination(txout.scriptPubKey, dest)) { | |||
| str_payout = "address=" + EncodeDestination(dest); | |||
| } else { | |||
| str_payout = "scriptPubKey=" + HexStr(txout.scriptPubKey); | |||
| } | |||
| LogPrintf("CMNPaymentsProcessor::%s -- ERROR! Failed to find expected payee %s amount=%lld height=%d\n", | |||
| __func__, str_payout, txout.nValue, nBlockHeight); | |||
| return false; | |||
| } | |||
| return true; | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: Squash the two follow-up commits into df0dcae so the consensus change lands as a single bisectable commit
The stack is currently df0dcae (consensus tightening + new test file) → e541b35 (one-line non-backported.txt registration of that test file) → 828854a (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 df0dcae: 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 df0dcae 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. e541b35 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 e541b35 and 828854a into df0dcae 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.
source: ['claude', 'codex']
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
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_V24and pre-v24 validation keeps thelegacy behavior.
What was done?
existence-only mode or strict multiplicity-correct mode.
CMNPaymentsProcessor::IsTransactionValidto use strict matchingonly after
DEPLOYMENT_V24is active.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: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 instructionsthrough local Claude against
upstream/develop..HEAD; it returnedRecommendation: shipwith no significant findings.Breaking Changes
This is a consensus tightening after
DEPLOYMENT_V24: post-v24 blocks mustinclude a distinct coinbase output for each expected masternode payment output,
including duplicate expected outputs. Pre-v24 behavior is intentionally
unchanged.
Checklist: