fix(consensus): require multiplicity-correct MN payment matching after v24#7377
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 |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (7)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds 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
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 0a40716) |
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. |
4600b46 to
e541b35
Compare
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.
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 👍 / 👎.
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
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']
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
828854a to
ef5fa2d
Compare
|
Addressed the commit-history review feedback by rewriting the branch to a The previous follow-ups Local validation: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
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..HEADreturns 1), and that commit includes the consensus change, the strict-multiplicity plumbing, the unit test registration insrc/Makefile.test.include, and thetest/util/data/non-backported.txtentry.
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
||
| // Check for correct masternode payment | ||
| if (IsTransactionValid(txNew, pindexPrev, blockSubsidy, feeReward, era)) { | ||
| if (IsTransactionValid(txNew, pindexPrev, blockSubsidy, feeReward, era, strict_multiplicity)) { |
There was a problem hiding this comment.
consider changing strict_multiplicity == true to check_superblock == SuperBlockCheckType::DisallowDuplicates for CMNPaymentsProcessor::IsBlockPayeeValid to reduce amount of arguments.
They are semantically the same meaning.
There was a problem hiding this comment.
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.
|
Verified the commit-history feedback from the Current head is a single coherent commit:
Fresh local validation in Push result was |
|
This pull request has conflicts, please rebase. |
|
Adopted Udjin's suggested test commit on top of the PR branch: This adds Local validation in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…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>
3d8c868 to
0a40716
Compare
|
Follow-up: after adopting the boundary test, GitHub reported the branch as conflicting with current Current PR commits: Rebased validation in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
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: