feat: add masternode reward shares#7340
Conversation
|
|
✅ Review complete (commit ec013c5) |
![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: 46c5fb4df7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const auto owner_payouts = GetOwnerPayouts(dmnPayee->pdmnState->nVersion, dmnPayee->pdmnState->scriptPayout, | ||
| dmnPayee->pdmnState->payouts); |
There was a problem hiding this comment.
When a v4 masternode later sends any ProUpServTx, RebuildListFromBlock stores the service payload version in pdmnState->nVersion (and CProUpServTx is capped at v3), while the v4 state still keeps the owner shares in payouts and has no serialized legacy scriptPayout. This call then falls back to LegacyPayoutAsList(scriptPayout), so the next selected payment is validated against a single empty/legacy script instead of the configured payout shares. The payout selection needs to remain tied to whether shares are present (or preserve v4 for payout state) rather than the service-update version.
Useful? React with 👍 / 👎.
| constexpr bool is_multipayout_eligible{std::is_same_v<std::decay_t<T>, CProRegTx> || std::is_same_v<std::decay_t<T>, CProUpRegTx>}; | ||
| const bool is_v24_active{DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24)}; | ||
| return ProTxVersion::GetMax( | ||
| is_basic_override ? *is_basic_override | ||
| : DeploymentActiveAfter(pindexPrev, chainman.GetConsensus(), Consensus::DEPLOYMENT_V19), | ||
| is_extaddr_eligible ? DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_V24) : false); | ||
| is_extaddr_eligible ? is_v24_active : false, | ||
| is_multipayout_eligible ? is_v24_active : false); |
There was a problem hiding this comment.
Including CProUpRegTx in the v24 multipayout max makes the normal protx update_registrar RPC build v4 registrar updates after v24. For masternodes that are still LegacyBLS, CheckProUpRegTx rejects that as a legacy-to->BasicBLS upgrade (bad-protx-version-upgrade), so the previously valid path of supplying a basic BLS operator key to upgrade the registrar state now fails unless the RPC clamps the version to v2 for legacy entries.
Useful? React with 👍 / 👎.
|
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:
WalkthroughThis PR implements multi-payout support for masternodes by introducing a CMasternodePayoutShare type and ProTxVersion::MultiPayout, adding payouts fields to ProTx/state/simplified types with conditional serialization, centralizing payout-list utilities (selection, validation, formatting, key-safety), wiring owner payout lists into bloom/filter extraction and block payment splitting, extending RPC input/output and wallet ownership checks, updating node/Qt view accessors, and adding unit and functional tests exercising multipayout flows. Sequence Diagram(s)sequenceDiagram
participant RPC_Client
participant RPC_Server
participant GetOwnerPayouts
participant PaymentsProcessor
RPC_Client->>RPC_Server: protx register / update_registrar (payouts array)
RPC_Server->>GetOwnerPayouts: derive owner payouts for ProTx/state
GetOwnerPayouts->>RPC_Server: return payout list
RPC_Server->>PaymentsProcessor: include payouts in state and block template
PaymentsProcessor->>PaymentsProcessor: split masternodeReward across payouts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/masternode.cpp (1)
641-645:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
masternodeliststill collapses multi-payout owners to a single payee.Using
GetOwnerPayouts(...).front()here meanspayee,full,info, andjsononly expose the first owner payout address after v4, even though the masternode actually pays multiple owners. That makes the RPC output and filtering incomplete for multi-payout nodes; iterate all owner payouts here, likeGetRequiredPaymentsString()already does above.🤖 Prompt for 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. In `@src/rpc/masternode.cpp` around lines 641 - 645, The code uses GetOwnerPayouts(...).front() so payee/full/info/json only show the first owner payout; change the logic to iterate all entries returned by GetOwnerPayouts(dmn.pdmnState->nVersion, dmn.pdmnState->scriptPayout, dmn.pdmnState->payouts) (similar to GetRequiredPaymentsString()) and concatenate each entry's scriptPayout -> ExtractDestination -> EncodeDestination into the payee string(s); update the places that set payeeStr/payeeScript (and the values used for full/info/json) so they include every owner payout address instead of just the front() element.
🧹 Nitpick comments (2)
test/functional/feature_masternode_payout_shares.py (1)
112-124: 💤 Low valueMaximum payout count boundary tested.
The test exercises the 8-payout maximum and confirms registrar updates work correctly with multi-destination payouts.
Optional: Consider testing operator reward interaction
The current test uses
operator_reward=0throughout (line 59, 72). Consider adding a separate test scenario with a non-zero operator reward to verify that the owner reward split correctly accounts for the operator share being deducted first (as described in the PR summary: "subtract operator reward first, then split the owner reward").This is not critical since the payment distribution logic is independently tested in unit tests, but would provide end-to-end verification of the operator + multi-payout interaction.
🤖 Prompt for 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. In `@test/functional/feature_masternode_payout_shares.py` around lines 112 - 124, Test currently exercises max 8-payouts with operator_reward left at 0; add an additional scenario that sets a non-zero operator_reward and verifies payouts account for operator share being subtracted before splitting owner rewards. Update the test in feature_masternode_payout_shares.py to create a registrant/update via mn.update_registrar with operator_reward > 0 (and suitable fundsAddr and updated_payouts), then assert via payout_address_rewards(update_raw["proUpRegTx"]["payouts"]) and node.protx("info", protx_hash)["state"]["payouts"] that the resulting payouts reflect the operator deduction-first logic (use the existing payout_address_rewards helper and the updated_payouts variable to compute expected amounts). Ensure you include a case for the 8-payout boundary combined with a non-zero operator_reward so the end-to-end distribution is validated.src/test/evo_trivialvalidation.cpp (1)
167-187: 💤 Low valueComprehensive validation coverage.
The test cases exercise all error paths from
IsPayoutListTriviallyValidandIsPayoutListKeySafe, matching the validation logic in the referenced context snippets.Optional: Add explicit MAX_REWARD boundary test
Consider adding a test for a single payout with reward > MAX_REWARD (10000) to explicitly verify the upper bound check:
CheckPayouts({{99, payout1}, {9901, payout2}}, owner_id, voting_id, "bad-protx-payout-reward"); +CheckPayouts({{10001, payout1}}, owner_id, voting_id, "bad-protx-payout-reward"); CheckPayouts({{5000, payout1}, {4999, payout2}}, owner_id, voting_id, "bad-protx-payout-reward-sum");This would explicitly test the
payout.reward > CMasternodePayoutShare::MAX_REWARDcondition, though the sum check would also catch this case.🤖 Prompt for 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. In `@src/test/evo_trivialvalidation.cpp` around lines 167 - 187, Add an explicit unit test case that verifies the upper-bound check for a single payout reward exceeding CMasternodePayoutShare::MAX_REWARD: call CheckPayouts with a MasternodePayoutShares containing one entry whose reward is CMasternodePayoutShare::MAX_REWARD + 1 (or otherwise > MAX_REWARD) and expect the same rejection string used for reward violations (e.g., "bad-protx-payout-reward" or the appropriate reject reason), so IsPayoutListTriviallyValid's payout.reward > CMasternodePayoutShare::MAX_REWARD path is exercised; also consider mirroring that check with IsPayoutListKeySafe if applicable.
🤖 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/evo/providertx.cpp`:
- Around line 101-121: IsPayoutListKeySafe currently narrows the payout reuse
check to PKHash only, letting a P2SH collateral script be reused in payouts; fix
by comparing the extracted payout destination against the full collateral_dest
(not against *collateral_pkhash). Keep the initial owner/voting key check using
CTxDestination(PKHash(keyIDOwner)) and CTxDestination(PKHash(keyIDVoting))
as-is, then in the loop call ExtractDestination(payout.scriptPayout,
payout_dest) and if (payout_dest == collateral_dest) return state.Invalid(...,
"bad-protx-payee-reuse"); remove the special-case reliance on collateral_pkhash
so all collateral destination types (P2PKH, P2SH, etc.) are checked.
In `@src/evo/specialtxman.cpp`:
- Around line 399-406: When updating newState->nVersion from opt_proTx->nVersion
in the ProUpRegTx handling, also update/convert newState->netInfo so it stays in
the same encoding family as nVersion: when switching to a MultiPayout-capable
version replace or reconfigure netInfo to the platform-capable representation
(so netInfo->CanStorePlatform() returns true), and when switching to a
legacy/BasicBLS version convert or clear platform fields so it uses the legacy
encoding; do the same fix in the registrar-update branch that moves
ExtAddr/BasicBLS/MultiPayout states (the registrar update logic that currently
allows transitions to BasicBLS or straight to MultiPayout) so netInfo is
adjusted whenever nVersion is changed, ensuring NetInfoSerWrapper selection
during serialization remains consistent with nVersion.
In `@src/rpc/evo.cpp`:
- Around line 789-793: The RPC layer is still rejecting payout arrays because
the argument schemas for protx register* and protx update_registrar* (symbols
payoutAddress_register and payoutAddress_update declared in GetRpcArg()) are
typed as RPCArg::Type::STR; update those RPCArg definitions to accept an array
form (e.g., RPCArg::Type::ARR or a union/variant allowing both STR and ARR) and
adjust their help/validation text accordingly so the handler code (ParsePayouts,
the protx register*/protx update_registrar* RPC methods) can receive an array
and hit the multi-payout branches; repeat the same change for the other
occurrence noted around the 1215-1223 region.
---
Outside diff comments:
In `@src/rpc/masternode.cpp`:
- Around line 641-645: The code uses GetOwnerPayouts(...).front() so
payee/full/info/json only show the first owner payout; change the logic to
iterate all entries returned by GetOwnerPayouts(dmn.pdmnState->nVersion,
dmn.pdmnState->scriptPayout, dmn.pdmnState->payouts) (similar to
GetRequiredPaymentsString()) and concatenate each entry's scriptPayout ->
ExtractDestination -> EncodeDestination into the payee string(s); update the
places that set payeeStr/payeeScript (and the values used for full/info/json) so
they include every owner payout address instead of just the front() element.
---
Nitpick comments:
In `@src/test/evo_trivialvalidation.cpp`:
- Around line 167-187: Add an explicit unit test case that verifies the
upper-bound check for a single payout reward exceeding
CMasternodePayoutShare::MAX_REWARD: call CheckPayouts with a
MasternodePayoutShares containing one entry whose reward is
CMasternodePayoutShare::MAX_REWARD + 1 (or otherwise > MAX_REWARD) and expect
the same rejection string used for reward violations (e.g.,
"bad-protx-payout-reward" or the appropriate reject reason), so
IsPayoutListTriviallyValid's payout.reward > CMasternodePayoutShare::MAX_REWARD
path is exercised; also consider mirroring that check with IsPayoutListKeySafe
if applicable.
In `@test/functional/feature_masternode_payout_shares.py`:
- Around line 112-124: Test currently exercises max 8-payouts with
operator_reward left at 0; add an additional scenario that sets a non-zero
operator_reward and verifies payouts account for operator share being subtracted
before splitting owner rewards. Update the test in
feature_masternode_payout_shares.py to create a registrant/update via
mn.update_registrar with operator_reward > 0 (and suitable fundsAddr and
updated_payouts), then assert via
payout_address_rewards(update_raw["proUpRegTx"]["payouts"]) and
node.protx("info", protx_hash)["state"]["payouts"] that the resulting payouts
reflect the operator deduction-first logic (use the existing
payout_address_rewards helper and the updated_payouts variable to compute
expected amounts). Ensure you include a case for the 8-payout boundary combined
with a non-zero operator_reward so the end-to-end distribution is validated.
🪄 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: 411dbdaf-dfaf-4431-8d34-20d4582a1a1b
📒 Files selected for processing (20)
src/common/bloom.cppsrc/evo/core_write.cppsrc/evo/deterministicmns.cppsrc/evo/dmnstate.cppsrc/evo/dmnstate.hsrc/evo/providertx.cppsrc/evo/providertx.hsrc/evo/simplifiedmns.cppsrc/evo/simplifiedmns.hsrc/evo/smldiff.cppsrc/evo/specialtx_filter.cppsrc/evo/specialtxman.cppsrc/masternode/payments.cppsrc/node/interfaces.cppsrc/rpc/evo.cppsrc/rpc/masternode.cppsrc/test/evo_trivialvalidation.cpptest/functional/feature_masternode_payout_shares.pytest/functional/test_framework/test_framework.pytest/functional/test_runner.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified all seven findings against the worktree at 46c5fb4. Three independent blocking bugs in the new multi-payout plumbing are confirmed: MakeSignString rewrites the message format for legacy (pre-v4) ProRegTx and breaks historical external-collateral signature verification; the unconditional newState->nVersion = opt_proTx->nVersion in the ProUpServTx branch demotes v4 masternodes to v3 on every service update (their payouts list is then silently dropped); and the same unconditional overwrite in the ProUpRegTx branch lets a v2 update demote an ExtAddr state. Suggestions about a missing CLI conversion entry, a lost revoke-source error path, the mutable cache in getScriptPayout, and a bloom short-circuit are also valid.
🔴 3 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
3 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 `src/evo/specialtxman.cpp`:
- [BLOCKING] src/evo/specialtxman.cpp:348-350: ProUpServTx unconditionally overwrites state nVersion, silently demoting v4 masternodes
In the TRANSACTION_PROVIDER_UPDATE_SERVICE branch of `RebuildListFromBlock`, the v24-gated block does `newState->nVersion = opt_proTx->nVersion;` with no anti-downgrade clamp. `GetMaxFromDeployment<CProUpServTx>` is *not* multipayout-eligible (`is_multipayout_eligible` in providertx.cpp:26 is false for CProUpServTx), so `protx update_service` always emits a ProUpServTx at version 3 (ExtAddr) at most. Apply this to a state already at MultiPayout (4) and the on-chain state is demoted to 3. Because `GetOwnerPayouts(state.nVersion, …)` switches on the state version (providertx.cpp:53-56), the demoted state stops reading `state.payouts` and falls back to `LegacyPayoutAsList(state.scriptPayout)` — and for a v4 state created via the ProUpRegTx branch `scriptPayout` was cleared (specialtxman.cpp:402). The result is that any v4 MN that issues a normal service update loses its payout-share list and miners will produce/validate an empty/legacy single-payout output instead of the configured shares. Preserve the higher version rather than blindly copying the tx version.
- [BLOCKING] src/evo/specialtxman.cpp:399-406: ProUpRegTx unconditionally overwrites state nVersion and can downgrade an ExtAddr state
`newState->nVersion = opt_proTx->nVersion;` at line 399 runs unconditionally, defeating the explicit anti-downgrade clamp added at line 394 in the pubKey-change branch. `IsVersionChangeValid` (specialtxman.cpp:898-926) only rejects `tx_version == ExtAddr` for non-service updates and `tx_version == MultiPayout` for non-registrar updates; it does *not* reject a v2 (BasicBLS) ProUpRegTx against a state already at v3 (ExtAddr). After this PR such a tx will silently demote `state.nVersion` from 3 → 2. `CDeterministicMNState` keys its on-disk netInfo serialization off `obj.nVersion >= ProTxVersion::ExtAddr` (dmnstate.h around line 100), so the downgrade also corrupts how `netInfo` is (de)serialized for Evo nodes. Clamp the version like the existing pubkey branch does, or gate the new payout switch on `opt_proTx->nVersion >= MultiPayout` without touching `nVersion` otherwise.
In `src/evo/providertx.cpp`:
- [BLOCKING] src/evo/providertx.cpp:219-236: MakeSignString changes the signed message format for all ProRegTx versions, breaking legacy external-collateral signatures
Previously `MakeSignString()` emitted just the encoded payout address (e.g. `Xj7…`). It now always calls `PayoutListToString(GetOwnerPayouts(nVersion, scriptPayout, payouts))`, which for pre-v4 transactions produces `LegacyPayoutAsList(scriptPayout)` and formats it as `"10000:Xj7…"`. That string is what `CheckStringSig` feeds to `CMessageSigner::VerifyMessage` for ProRegTx with an external collateral (specialtxman.cpp:1046-1051). The change is not gated on `ProTxVersion::MultiPayout`, so every historical v2/v3 ProRegTx with external collateral — whose `vchSig` was produced against the original format — will now fail verification with `bad-protx-sig` whenever `check_sigs` is on (e.g. reindex with `-assumevalid=0`, or any chain segment past assumevalid). It also breaks any externally prepared but not-yet-broadcast legacy ProRegTx. Gate the new format on `nVersion >= MultiPayout` so legacy payloads keep producing/verifying their original message.
In `src/rpc/client.cpp`:
- [SUGGESTION] src/rpc/client.cpp:280-310: dash-cli never converts the payout array argument for protx register/update_registrar
`ParsePayouts` in src/rpc/evo.cpp:259 only takes the multi-payout array path when the UniValue is an array. Through dash-cli all positional arguments default to strings unless listed in `vRPCConvertParams` here, and the new payout parameter is not registered for any of `protx register`, `register_legacy`, `register_evo`, `register_fund`, `register_fund_legacy`, `register_fund_evo`, `register_prepare`, `register_prepare_legacy`, `register_prepare_evo`, `update_registrar`, or `update_registrar_legacy`. As a result, a CLI invocation like `dash-cli protx register … '[{"address":"…","reward":10000}]'` reaches `ParsePayouts` as a string and is rejected as an invalid address. The Python functional test exercises the RPC path with native objects so it doesn't catch this. Add entries for the payout positional argument on each of these commands (the index is `paramIdx + 5` for register variants and `3` for update_registrar variants).
In `src/rpc/evo.cpp`:
- [SUGGESTION] src/rpc/evo.cpp:1349-1359: protx revoke silently calls FundSpecialTx with CNoDestination when no payout source exists
Pre-PR, the final `else` branch in `protx_revoke` threw `"No payout or fee source addresses found, can't revoke"` when `dmn->pdmnState->scriptPayout == CScript()`. The new code drops the empty-script check and always calls `ExtractDestination(owner_payouts.front().scriptPayout, txDest)`. For a v2/v3 state with an empty `scriptPayout`, `GetOwnerPayouts` returns `LegacyPayoutAsList(CScript())` so the front element is the empty script, `ExtractDestination` fails leaving `txDest` as `CNoDestination`, and `FundSpecialTx` then attempts to use it. Users get an opaque downstream wallet failure instead of the previous explicit message. Restore the guard.
In `src/node/interfaces.cpp`:
- [SUGGESTION] src/node/interfaces.cpp:126-131: MnEntryImpl::getScriptPayout collapses multi-payout MNs to the first share via a non-thread-safe mutable cache
`getScriptPayout()` is declared `const` but writes to `mutable CScript m_script_payout` on every call and returns a reference to it, so concurrent callers on the same `MnEntryImpl` race on that buffer. It also silently discards every payout share after the first for v4 MNs — any caller that still uses the legacy single-script accessor (GUI/wallet integrations counting rewards) will under-report or misattribute payouts. Either expose the full `MasternodePayoutShares` through the `MnEntry` interface and migrate callers, or at minimum return by value and document that the legacy accessor exposes only the first share.
| newState->nVersion = opt_proTx->nVersion; | ||
| if (opt_proTx->nVersion >= ProTxVersion::MultiPayout) { | ||
| newState->payouts = opt_proTx->payouts; | ||
| newState->scriptPayout.clear(); | ||
| } else { | ||
| newState->scriptPayout = opt_proTx->scriptPayout; | ||
| newState->payouts.clear(); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: ProUpRegTx unconditionally overwrites state nVersion and can downgrade an ExtAddr state
newState->nVersion = opt_proTx->nVersion; at line 399 runs unconditionally, defeating the explicit anti-downgrade clamp added at line 394 in the pubKey-change branch. IsVersionChangeValid (specialtxman.cpp:898-926) only rejects tx_version == ExtAddr for non-service updates and tx_version == MultiPayout for non-registrar updates; it does not reject a v2 (BasicBLS) ProUpRegTx against a state already at v3 (ExtAddr). After this PR such a tx will silently demote state.nVersion from 3 → 2. CDeterministicMNState keys its on-disk netInfo serialization off obj.nVersion >= ProTxVersion::ExtAddr (dmnstate.h around line 100), so the downgrade also corrupts how netInfo is (de)serialized for Evo nodes. Clamp the version like the existing pubkey branch does, or gate the new payout switch on opt_proTx->nVersion >= MultiPayout without touching nVersion otherwise.
| newState->nVersion = opt_proTx->nVersion; | |
| if (opt_proTx->nVersion >= ProTxVersion::MultiPayout) { | |
| newState->payouts = opt_proTx->payouts; | |
| newState->scriptPayout.clear(); | |
| } else { | |
| newState->scriptPayout = opt_proTx->scriptPayout; | |
| newState->payouts.clear(); | |
| } | |
| newState->keyIDVoting = opt_proTx->keyIDVoting; | |
| if (opt_proTx->nVersion >= ProTxVersion::MultiPayout) { | |
| newState->nVersion = opt_proTx->nVersion; | |
| newState->payouts = opt_proTx->payouts; | |
| newState->scriptPayout.clear(); | |
| } else { | |
| // Never downgrade an already-upgraded state (e.g. ExtAddr -> BasicBLS would break netInfo serialization). | |
| newState->nVersion = std::max<uint16_t>(newState->nVersion, opt_proTx->nVersion); | |
| newState->scriptPayout = opt_proTx->scriptPayout; | |
| newState->payouts.clear(); | |
| } |
source: ['claude']
There was a problem hiding this comment.
Resolved in f977194 — ProUpRegTx unconditionally overwrites state nVersion and can downgrade an ExtAddr state no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| CTxDestination txDest; | ||
| ExtractDestination(dmn->pdmnState->scriptOperatorPayout, txDest); | ||
| FundSpecialTx(*pwallet, tx, ptx, txDest); | ||
| } else if (dmn->pdmnState->scriptPayout != CScript()) { | ||
| } else { | ||
| // Using funds from previousely specified masternode payout address | ||
| CTxDestination txDest; | ||
| ExtractDestination(dmn->pdmnState->scriptPayout, txDest); | ||
| const auto owner_payouts = GetOwnerPayouts(dmn->pdmnState->nVersion, dmn->pdmnState->scriptPayout, dmn->pdmnState->payouts); | ||
| ExtractDestination(owner_payouts.front().scriptPayout, txDest); | ||
| FundSpecialTx(*pwallet, tx, ptx, txDest); |
There was a problem hiding this comment.
🟡 Suggestion: protx revoke silently calls FundSpecialTx with CNoDestination when no payout source exists
Pre-PR, the final else branch in protx_revoke threw "No payout or fee source addresses found, can't revoke" when dmn->pdmnState->scriptPayout == CScript(). The new code drops the empty-script check and always calls ExtractDestination(owner_payouts.front().scriptPayout, txDest). For a v2/v3 state with an empty scriptPayout, GetOwnerPayouts returns LegacyPayoutAsList(CScript()) so the front element is the empty script, ExtractDestination fails leaving txDest as CNoDestination, and FundSpecialTx then attempts to use it. Users get an opaque downstream wallet failure instead of the previous explicit message. Restore the guard.
| CTxDestination txDest; | |
| ExtractDestination(dmn->pdmnState->scriptOperatorPayout, txDest); | |
| FundSpecialTx(*pwallet, tx, ptx, txDest); | |
| } else if (dmn->pdmnState->scriptPayout != CScript()) { | |
| } else { | |
| // Using funds from previousely specified masternode payout address | |
| CTxDestination txDest; | |
| ExtractDestination(dmn->pdmnState->scriptPayout, txDest); | |
| const auto owner_payouts = GetOwnerPayouts(dmn->pdmnState->nVersion, dmn->pdmnState->scriptPayout, dmn->pdmnState->payouts); | |
| ExtractDestination(owner_payouts.front().scriptPayout, txDest); | |
| FundSpecialTx(*pwallet, tx, ptx, txDest); | |
| } else { | |
| // Using funds from previousely specified masternode payout address | |
| CTxDestination txDest; | |
| const auto owner_payouts = GetOwnerPayouts(dmn->pdmnState->nVersion, dmn->pdmnState->scriptPayout, dmn->pdmnState->payouts); | |
| if (!ExtractDestination(owner_payouts.front().scriptPayout, txDest)) { | |
| throw JSONRPCError(RPC_INTERNAL_ERROR, "No payout or fee source addresses found, can't revoke"); | |
| } | |
| FundSpecialTx(*pwallet, tx, ptx, txDest); | |
| } |
source: ['claude']
There was a problem hiding this comment.
Resolved in f977194 — protx revoke silently calls FundSpecialTx with CNoDestination when no payout source exists no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| const CScript& getScriptPayout() const override | ||
| { | ||
| m_script_payout = GetOwnerPayouts(m_dmn->pdmnState->nVersion, m_dmn->pdmnState->scriptPayout, | ||
| m_dmn->pdmnState->payouts).front().scriptPayout; | ||
| return m_script_payout; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: MnEntryImpl::getScriptPayout collapses multi-payout MNs to the first share via a non-thread-safe mutable cache
getScriptPayout() is declared const but writes to mutable CScript m_script_payout on every call and returns a reference to it, so concurrent callers on the same MnEntryImpl race on that buffer. It also silently discards every payout share after the first for v4 MNs — any caller that still uses the legacy single-script accessor (GUI/wallet integrations counting rewards) will under-report or misattribute payouts. Either expose the full MasternodePayoutShares through the MnEntry interface and migrate callers, or at minimum return by value and document that the legacy accessor exposes only the first share.
source: ['claude']
There was a problem hiding this comment.
Resolved in f977194 — MnEntryImpl::getScriptPayout collapses multi-payout MNs to the first share via a non-thread-safe mutable cache no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| bool found_payout = false; | ||
| for (const auto& payout : GetOwnerPayouts(opt_proTx->nVersion, opt_proTx->scriptPayout, opt_proTx->payouts)) { | ||
| found_payout = found_payout || CheckScript(payout.scriptPayout); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Bloom-match payout loop never short-circuits
found_payout = found_payout || CheckScript(payout.scriptPayout); keeps scanning every share after a match. CheckScript is const and side-effect-free, so the trailing iterations are wasted work — relevant when v4 ProTx can carry up to 8 owner shares. Same pattern in the TRANSACTION_PROVIDER_UPDATE_REGISTRAR arm at line 168.
| bool found_payout = false; | |
| for (const auto& payout : GetOwnerPayouts(opt_proTx->nVersion, opt_proTx->scriptPayout, opt_proTx->payouts)) { | |
| found_payout = found_payout || CheckScript(payout.scriptPayout); | |
| } | |
| bool found_payout = false; | |
| for (const auto& payout : GetOwnerPayouts(opt_proTx->nVersion, opt_proTx->scriptPayout, opt_proTx->payouts)) { | |
| if (CheckScript(payout.scriptPayout)) { | |
| found_payout = true; | |
| break; | |
| } | |
| } |
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — Bloom-match payout loop never short-circuits no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
PastaPastaPasta
force-pushed
the
feat/multi-party-payout
branch
from
1a00247 to
f977194
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/evo/specialtxman.cpp (1)
411-428: 💤 Low valueVersion assignment before
SetStateVersionis redundant.Line 415 sets
newState->nVersion = opt_proTx->nVersion, butSetStateVersionat line 426 will overwrite it withtarget_versionanyway (which may differ whenopt_proTx->nVersion < current_version). The ProUpRegTx handling below (lines 465-467) correctly omits the direct nVersion assignment, lettingSetStateVersionhandle it.Consider removing line 415 to match the cleaner pattern used in ProUpRegTx.
♻️ Suggested simplification
auto newState = std::make_shared<CDeterministicMNState>(*dmn->pdmnState); const uint16_t current_version{static_cast<uint16_t>(newState->nVersion)}; const uint16_t target_version{is_v24_deployed ? std::max<uint16_t>(current_version, opt_proTx->nVersion) : current_version}; - if (is_v24_deployed) { - // Extended addresses support in v24 means that the version can be updated - newState->nVersion = opt_proTx->nVersion; - } newState->netInfo = opt_proTx->netInfo; newState->scriptOperatorPayout = opt_proTx->scriptOperatorPayout;🤖 Prompt for 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. In `@src/evo/specialtxman.cpp` around lines 411 - 428, Remove the redundant direct assignment newState->nVersion = opt_proTx->nVersion when is_v24_deployed and instead rely on SetStateVersion to set the final version; specifically, delete the line that assigns opt_proTx->nVersion to newState->nVersion (while keeping the existing computation of current_version and target_version and the subsequent call to SetStateVersion(target_version, dmn->nType, state)), so version resolution is consistently handled by SetStateVersion the same way as in the ProUpRegTx path.src/rpc/masternode.cpp (1)
230-247: 💤 Low valueConsider extracting common payout iteration logic.
Both
GetRequiredPaymentsString(lines 232-240) andGetOwnerPayoutsString(lines 272-279) have nearly identical iteration and destination extraction logic. This is minor duplication, but if you anticipate more call sites, a shared helper could reduce maintenance burden.Current code is functionally correct.
Also applies to: 269-281
🤖 Prompt for 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. In `@src/rpc/masternode.cpp` around lines 230 - 247, Extract the duplicated loop that iterates payouts, ExtractDestination checks, and EncodeDestination concatenation into a single helper (e.g., BuildPayoutsString or EncodePayoutDestinations) and call it from both GetRequiredPaymentsString and GetOwnerPayoutsString; the helper should accept the vector of payout structs (as returned by GetOwnerPayouts or payee->pdmnState->payouts) and an optional operator payout script+flag, perform the ExtractDestination() + NONFATAL_UNREACHABLE() checks, join with ", " and return the final string so you can replace the repeated loops in GetRequiredPaymentsString and GetOwnerPayoutsString with a single call to that helper.
🤖 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.
Nitpick comments:
In `@src/evo/specialtxman.cpp`:
- Around line 411-428: Remove the redundant direct assignment newState->nVersion
= opt_proTx->nVersion when is_v24_deployed and instead rely on SetStateVersion
to set the final version; specifically, delete the line that assigns
opt_proTx->nVersion to newState->nVersion (while keeping the existing
computation of current_version and target_version and the subsequent call to
SetStateVersion(target_version, dmn->nType, state)), so version resolution is
consistently handled by SetStateVersion the same way as in the ProUpRegTx path.
In `@src/rpc/masternode.cpp`:
- Around line 230-247: Extract the duplicated loop that iterates payouts,
ExtractDestination checks, and EncodeDestination concatenation into a single
helper (e.g., BuildPayoutsString or EncodePayoutDestinations) and call it from
both GetRequiredPaymentsString and GetOwnerPayoutsString; the helper should
accept the vector of payout structs (as returned by GetOwnerPayouts or
payee->pdmnState->payouts) and an optional operator payout script+flag, perform
the ExtractDestination() + NONFATAL_UNREACHABLE() checks, join with ", " and
return the final string so you can replace the repeated loops in
GetRequiredPaymentsString and GetOwnerPayoutsString with a single call to that
helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2160d2a3-8198-4e47-9c39-e3873596d0c3
📒 Files selected for processing (8)
src/evo/providertx.cppsrc/evo/specialtxman.cppsrc/node/interfaces.cppsrc/rpc/client.cppsrc/rpc/evo.cppsrc/rpc/masternode.cppsrc/test/evo_trivialvalidation.cpptest/functional/feature_masternode_payout_shares.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/node/interfaces.cpp
- src/test/evo_trivialvalidation.cpp
- src/evo/providertx.cpp
- src/rpc/evo.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f977194004
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| CKeyID keyIDVoting; | ||
| std::shared_ptr<NetInfoInterface> netInfo{nullptr}; | ||
| CScript scriptPayout; | ||
| MasternodePayoutShares payouts; |
There was a problem hiding this comment.
For a v4 masternode registered with payout shares, a later ProUpRevTx still calls ResetOperatorFields(), which downgrades nVersion to LegacyBLS; since v4 registrations do not serialize a legacy scriptPayout, the added payouts state becomes ignored by GetOwnerPayouts() and is lost on state serialization. In that revoked state, RPC flows such as protx update_registrar with an empty payout can no longer reuse the existing payout shares and instead try to reuse an empty legacy script, so revocation unexpectedly discards the configured owner payouts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/functional/feature_masternode_payout_shares.py (1)
146-160: ⚡ Quick winAdd
masternode("payments")amount assertions in the operator-payout branch.At Line 146 onward, you validate split math via
getblocktemplate(), but not themasternode("payments")RPC path after operator payout activation. Adding parallel amount checks there would harden RPC-coverage for the same payout split.Suggested test extension
gbt_payees = [p for p in node.getblocktemplate()["masternode"] if p["script"] != "6a"] operator_payees = [p for p in gbt_payees if p["payee"] == operator_payout] assert_equal(len(operator_payees), 1) owner_payees = [p for p in gbt_payees if p["payee"] != operator_payout] assert_equal([p["payee"] for p in owner_payees], [p["address"] for p in updated_payouts]) masternode_total = sum(p["amount"] for p in gbt_payees) operator_amount = masternode_total * 2500 // 10000 assert_equal(operator_payees[0]["amount"], operator_amount) owner_total = masternode_total - operator_amount paid_owner_total = 0 for i, payee in enumerate(owner_payees): expected_amount = owner_total - paid_owner_total if i == len(owner_payees) - 1 else owner_total * 1250 // 10000 assert_equal(payee["amount"], expected_amount) paid_owner_total += payee["amount"] + + self.generate(node, 1, sync_fun=self.no_op) + rpc_payees = [p for p in node.masternode("payments")[0]["masternodes"][0]["payees"] if p["script"] != "6a"] + rpc_operator = [p for p in rpc_payees if p["address"] == operator_payout] + assert_equal(len(rpc_operator), 1) + rpc_owner = [p for p in rpc_payees if p["address"] != operator_payout] + assert_equal([p["address"] for p in rpc_owner], [p["address"] for p in updated_payouts]) + assert_equal(sum(p["amount"] for p in rpc_owner), owner_total) + assert_equal(rpc_operator[0]["amount"], operator_amount)🤖 Prompt for 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. In `@test/functional/feature_masternode_payout_shares.py` around lines 146 - 160, The test currently verifies payout splits from getblocktemplate() but misses asserting the same amounts via the masternode("payments") RPC; call node.masternode("payments") after operator payout activation, filter out OP_RETURN entries, build payments list (analogous to gbt_payees), locate operator entry by operator_payout, compute masternode_total, operator_amount = masternode_total * 2500 // 10000 and owner_total = masternode_total - operator_amount, then assert the operator entry's "amount" equals operator_amount and iterate owner entries to assert each owner "amount" matches the same per-payee math used for owner_payees (use expected_amount = owner_total * 1250 // 10000 for non-last entries and remainder for the last) ensuring paid_owner_total sums match.
🤖 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.
Nitpick comments:
In `@test/functional/feature_masternode_payout_shares.py`:
- Around line 146-160: The test currently verifies payout splits from
getblocktemplate() but misses asserting the same amounts via the
masternode("payments") RPC; call node.masternode("payments") after operator
payout activation, filter out OP_RETURN entries, build payments list (analogous
to gbt_payees), locate operator entry by operator_payout, compute
masternode_total, operator_amount = masternode_total * 2500 // 10000 and
owner_total = masternode_total - operator_amount, then assert the operator
entry's "amount" equals operator_amount and iterate owner entries to assert each
owner "amount" matches the same per-payee math used for owner_payees (use
expected_amount = owner_total * 1250 // 10000 for non-last entries and remainder
for the last) ensuring paid_owner_total sums match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c86c00c0-7b31-4f88-b5b5-4af06ee60f21
📒 Files selected for processing (12)
src/evo/providertx.cppsrc/evo/specialtxman.cppsrc/interfaces/node.hsrc/node/interfaces.cppsrc/qt/masternodelist.cppsrc/qt/masternodemodel.cppsrc/qt/masternodemodel.hsrc/rpc/client.cppsrc/rpc/evo.cppsrc/rpc/masternode.cppsrc/test/evo_trivialvalidation.cpptest/functional/feature_masternode_payout_shares.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/test/evo_trivialvalidation.cpp
- src/rpc/masternode.cpp
- src/rpc/client.cpp
- src/evo/specialtxman.cpp
- src/rpc/evo.cpp
- src/evo/providertx.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c04460fee
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ExtractDestination(payout.scriptPayout, payout_dest) && payout_dest == collateral_dest) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-payee-reuse"); |
There was a problem hiding this comment.
When this helper is reached from CheckProRegTx/CheckProUpRegTx for legacy v1-v3 payloads, GetOwnerPayouts wraps the single legacy scriptPayout, so this new comparison now rejects any payout address that matches the collateral address. That was not part of the old consensus checks (they only rejected collateral reuse with owner/voting keys), and the call is not gated on v24 or MultiPayout, so previously valid pre-v4 registrations/registrar updates that pay rewards to the collateral address will fail validation/IBD with bad-protx-payee-reuse. Keep the new payout-vs-collateral restriction limited to v4 payout-share transactions or otherwise preserve legacy behavior.
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/Makefile.am`:
- Line 1211: Remove the unused networking source from the dash-chainstate build:
delete the governance/net_governance.cpp entry from dash_chainstate_SOURCES in
src/Makefile.am so the validation-only binary no longer pulls in NetGovernance
networking code; verify that no references to NetGovernance remain in the
dash-chainstate entry points (bitcoin-chainstate.cpp / node/chainstate.cpp)
before committing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental verification at head 1a00247: five of the seven prior findings (#1 ProUpServTx version demotion, #2 ProUpRegTx version demotion, #3 MakeSignString legacy break, #4 vRPCConvertParams gaps, #5 protx_revoke empty-payout) are properly fixed by the new SetStateVersion helper, legacy-format gating, vRPCConvertParams additions, and restored empty-payout guard. Two prior items remain: the Qt MnEntry interface still collapses v4 multi-payouts to the first share (race fixed, semantic loss not), and the bloom payout loops still iterate without short-circuiting. No new in-scope blocking issues introduced by the latest delta.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/node/interfaces.cpp`:
- [SUGGESTION] src/node/interfaces.cpp:108-128: MnEntry interface still collapses v4 multi-payouts to the first share
The const-mutable race from the prior review is fixed (m_script_payout is now an immutable member initialized in the constructor), but the interface still exposes only `GetOwnerPayouts(...).front().scriptPayout` through `getScriptPayout()`. v4 ProTx can carry up to eight owner payout shares. The downstream Qt callers `masternodemodel.cpp:59` (`ExtractDestination(dmn->getScriptPayout(), ...)`) and `masternodelist.cpp:226` (`walletModel->wallet().isSpendable(entry->scriptPayoutRaw())`) use this single script to display the payout address and to decide whether the wallet owns the masternode. A wallet that owns one of the non-first payout shares will not be recognized through the payout path, and the GUI will display only the first payee even though the masternode pays multiple owners. This is the central UX/wallet-recognition contract added by this PR, so the interface should return the full payout list (and callers should iterate it for spendability/display).
| @@ -122,7 +125,7 @@ class MnEntryImpl : public MnEntry | |||
| const CKeyID& getKeyIdOwner() const override { return m_dmn->pdmnState->keyIDOwner; } | |||
| const CKeyID& getKeyIdVoting() const override { return m_dmn->pdmnState->keyIDVoting; } | |||
| const COutPoint& getCollateralOutpoint() const override { return m_dmn->collateralOutpoint; } | |||
| const CScript& getScriptPayout() const override { return m_dmn->pdmnState->scriptPayout; } | |||
| const CScript& getScriptPayout() const override { return m_script_payout; } | |||
There was a problem hiding this comment.
🟡 Suggestion: MnEntry interface still collapses v4 multi-payouts to the first share
The const-mutable race from the prior review is fixed (m_script_payout is now an immutable member initialized in the constructor), but the interface still exposes only GetOwnerPayouts(...).front().scriptPayout through getScriptPayout(). v4 ProTx can carry up to eight owner payout shares. The downstream Qt callers masternodemodel.cpp:59 (ExtractDestination(dmn->getScriptPayout(), ...)) and masternodelist.cpp:226 (walletModel->wallet().isSpendable(entry->scriptPayoutRaw())) use this single script to display the payout address and to decide whether the wallet owns the masternode. A wallet that owns one of the non-first payout shares will not be recognized through the payout path, and the GUI will display only the first payee even though the masternode pays multiple owners. This is the central UX/wallet-recognition contract added by this PR, so the interface should return the full payout list (and callers should iterate it for spendability/display).
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — MnEntry interface still collapses v4 multi-payouts to the first share no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| @@ -160,8 +164,12 @@ bool CBloomFilter::CheckSpecialTransactionMatchesAndUpdate(const CTransaction &t | |||
| if (const auto opt_proTx = GetTxPayload<CProUpRegTx>(tx)) { | |||
| if(contains(opt_proTx->proTxHash)) | |||
| return true; | |||
| bool found_payout = false; | |||
| for (const auto& payout : GetOwnerPayouts(opt_proTx->nVersion, opt_proTx->scriptPayout, opt_proTx->payouts)) { | |||
| found_payout = found_payout || CheckScript(payout.scriptPayout); | |||
| } | |||
There was a problem hiding this comment.
💬 Nitpick: Bloom payout-match loops do not short-circuit after a hit
Both TRANSACTION_PROVIDER_REGISTER (lines 135-138) and TRANSACTION_PROVIDER_UPDATE_REGISTRAR (lines 167-170) use found_payout = found_payout || CheckScript(payout.scriptPayout); inside a range-for. The || short-circuits the CheckScript call once found_payout is true, but the loop itself continues iterating. v4 ProTx can carry up to eight payout shares, so trailing iterations are needless work. Break out as soon as a match is observed (or use std::any_of).
| bool found_payout = false; | |
| for (const auto& payout : GetOwnerPayouts(opt_proTx->nVersion, opt_proTx->scriptPayout, opt_proTx->payouts)) { | |
| if (CheckScript(payout.scriptPayout)) { found_payout = true; break; } | |
| } |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Bloom payout-match loops do not short-circuit after a hit no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of PR #7340 (multi-payout owner reward lists for ProTx v4) confirms the implementation is sound. Prior finding #1 (interface collapsing v4 multi-payouts) was FIXED in commit f977194 — getScriptPayouts() now exposes the full list and Qt callers were updated. Prior finding #2 (bloom payout-match loops not short-circuiting) remains a valid nitpick and is carried forward. No new in-scope issues identified.
💬 1 nitpick(s)
| @@ -160,8 +164,12 @@ bool CBloomFilter::CheckSpecialTransactionMatchesAndUpdate(const CTransaction &t | |||
| if (const auto opt_proTx = GetTxPayload<CProUpRegTx>(tx)) { | |||
| if(contains(opt_proTx->proTxHash)) | |||
| return true; | |||
| bool found_payout = false; | |||
| for (const auto& payout : GetOwnerPayouts(opt_proTx->nVersion, opt_proTx->scriptPayout, opt_proTx->payouts)) { | |||
| found_payout = found_payout || CheckScript(payout.scriptPayout); | |||
| } | |||
There was a problem hiding this comment.
💬 Nitpick: Bloom payout-match loops still iterate after a hit
Carried forward from the prior review — STILL VALID. Both the TRANSACTION_PROVIDER_REGISTER branch (lines 135-138) and the TRANSACTION_PROVIDER_UPDATE_REGISTRAR branch (lines 167-170) use found_payout = found_payout || CheckScript(payout.scriptPayout). The || short-circuit skips additional CheckScript calls after the first match, but the for-loop still walks every remaining share. With v4 lists of up to eight payouts this is minor, but a break on first match is clearer and avoids unnecessary iteration. No correctness impact.
| bool found_payout = false; | |
| for (const auto& payout : GetOwnerPayouts(opt_proTx->nVersion, opt_proTx->scriptPayout, opt_proTx->payouts)) { | |
| if (CheckScript(payout.scriptPayout)) { | |
| found_payout = true; | |
| break; | |
| } | |
| } |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in this update — Bloom payout-match loops still iterate after a hit no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
pls see https://github.com/UdjinM6/dash/commits/pr-7340/ (this branch assumes suggestions from dashpay/dips#184 (comment) were accepted) |
Match the updated DIP-0026 layout: each owner payout entry serializes scriptPayout before reward, and the external-collateral sign string uses <payoutStr>:<reward>. Reorder the struct members, constructor and SERIALIZE_METHODS accordingly and update all construction sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
DIP-0026 requires every payout script to be P2PKH or P2SH, so a destination can always be extracted. Emit "address" unconditionally and document it as non-optional, and order each entry as address/script/reward. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Use CMasternodePayoutShare::MAX_REWARD instead of a bare 10000 literal in the owner reward split, and replace the manual payout-match loops with short-circuiting std::any_of in the bloom filter special-tx handling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add DIP-0026 cases 13 and 14: a block that omits an expected owner payout output or underpays one must be rejected with bad-cb-payee, while a faithful coinbase built from the same template is accepted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The bloom filter and JSON writers in libbitcoin_common call GetOwnerPayouts and PayoutListToJson, but these were defined in providertx.cpp which lives in libbitcoin_node, so dash-tx (links common, not node) failed to link. Move the layer-agnostic owner payout helpers (LegacyPayoutAsList, GetOwnerPayouts, PayoutListToString, PayoutListToJson) into a new common source file evo/providertx_util.cpp and add it to the dash-chainstate source list. The trivial-validation helpers stay in providertx.cpp as node-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PastaPastaPasta
force-pushed
the
feat/multi-party-payout
branch
from
b72132f to
9091989
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental polish pass: prior bloom payout-loop nitpick is resolved by the std::any_of refactor in commit 215828e. The only new flag was a low-confidence missing- include in qt/masternodelist.cpp, which is a false positive (the same pattern in bloom.cpp also lacks a direct include and builds clean across the project's std/Qt header chains). No in-scope blocking or suggestion findings remain.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 9091989. Carried-forward prior findings: none (the b72132f review had zero findings). New findings in the current cumulative review: none. The one Codex 'blocking' finding about IsPayoutListTriviallyValid being called with CKeyID{} from CProUpRegTx::IsTriviallyValid is a theoretical-only concern — a payout to PKHash(0) would require a hash160 preimage of all zeros, which is cryptographically infeasible, and the real owner-key reuse check still runs in CheckProUpRegTx against the actual masternode owner key. CodeRabbit posted no inline findings.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 4401017. Carried-forward prior findings (9091989): none — prior review posted nothing. One new blocking consensus issue confirmed in the current cumulative state: the new IsPayoutListKeySafe collateral-vs-payout reuse rule is applied to all ProRegTx/ProUpRegTx versions rather than being gated to MultiPayout (v4). Pre-PR validation only rejected collateral reuse with owner/voting keys; the new rule also rejects payout==collateral, and is unconditionally evaluated for legacy v1–v3 payloads (GetOwnerPayouts wraps the legacy scriptPayout into a single-entry list). The latest delta even patches an existing v3 test (block_reward_reallocation_tests.cpp) to avoid the new rule, confirming the behavior change on legacy payloads.
🔴 1 blocking
🤖 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/evo/specialtxman.cpp`:
- [BLOCKING] src/evo/specialtxman.cpp:1067-1070: New collateral-vs-payout reuse rule tightens consensus for legacy ProRegTx/ProUpRegTx versions
`IsPayoutListKeySafe` is called unconditionally for every ProRegTx version here (and for ProUpRegTx at line 1246). For pre-MultiPayout payloads, `GetOwnerPayouts(nVersion, scriptPayout, payouts)` returns a one-entry legacy list built from the existing `scriptPayout`, so the new check `payout_dest == collateral_dest` (providertx.cpp:98–103) is enforced on v1–v3 payloads. Pre-PR behavior (see commit 317917a89e:src/evo/specialtxman.cpp lines 987–992) only rejected collateral reuse with `keyIDOwner` / `keyIDVoting`; payout-script reuse against the collateral destination was permitted. The latest delta in `block_reward_reallocation_tests.cpp` had to rewrite an `nVersion = 3` ProRegTx to fund from a random address rather than `scriptPayout`, which directly demonstrates the consensus rule is now stricter for legacy payloads. This is a soft-fork–shaped change with no deployment gating: legacy ProRegTx/ProUpRegTx that were previously valid (including any already in chain history) would be rejected, risking IBD/reorg divergence and a chain-split on any legacy registration that happens to reuse the payout script as collateral. Gate the new `payout_dest == collateral_dest` check to `ProTxVersion::MultiPayout` (e.g., only call `IsPayoutListKeySafe` for v4+ payouts, or keep the owner/voting-key checks unconditional but skip the payout-destination loop for legacy versions). Either way, leave legacy v1–v3 validation byte-for-byte equivalent to the prior consensus rule.
| // don't allow reuse of collateral key for other keys (don't allow people to put the collateral key onto an online server) | ||
| // this check applies to internal and external collateral, but internal collaterals are not necessarily a P2PKH | ||
| if (collateralTxDest == CTxDestination(PKHash(opt_ptx->keyIDOwner)) || | ||
| collateralTxDest == CTxDestination(PKHash(opt_ptx->keyIDVoting))) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-collateral-reuse"); | ||
| } | ||
| if (!IsPayoutListKeySafe(GetOwnerPayouts(opt_ptx->nVersion, opt_ptx->scriptPayout, opt_ptx->payouts), | ||
| collateralTxDest, opt_ptx->keyIDOwner, opt_ptx->keyIDVoting, state)) return false; |
There was a problem hiding this comment.
🔴 Blocking: New collateral-vs-payout reuse rule tightens consensus for legacy ProRegTx/ProUpRegTx versions
IsPayoutListKeySafe is called unconditionally for every ProRegTx version here (and for ProUpRegTx at line 1246). For pre-MultiPayout payloads, GetOwnerPayouts(nVersion, scriptPayout, payouts) returns a one-entry legacy list built from the existing scriptPayout, so the new check payout_dest == collateral_dest (providertx.cpp:98–103) is enforced on v1–v3 payloads. Pre-PR behavior (see commit 317917a:src/evo/specialtxman.cpp lines 987–992) only rejected collateral reuse with keyIDOwner / keyIDVoting; payout-script reuse against the collateral destination was permitted. The latest delta in block_reward_reallocation_tests.cpp had to rewrite an nVersion = 3 ProRegTx to fund from a random address rather than scriptPayout, which directly demonstrates the consensus rule is now stricter for legacy payloads. This is a soft-fork–shaped change with no deployment gating: legacy ProRegTx/ProUpRegTx that were previously valid (including any already in chain history) would be rejected, risking IBD/reorg divergence and a chain-split on any legacy registration that happens to reuse the payout script as collateral. Gate the new payout_dest == collateral_dest check to ProTxVersion::MultiPayout (e.g., only call IsPayoutListKeySafe for v4+ payouts, or keep the owner/voting-key checks unconditional but skip the payout-destination loop for legacy versions). Either way, leave legacy v1–v3 validation byte-for-byte equivalent to the prior consensus rule.
source: ['codex']
There was a problem hiding this comment.
Resolved in 57a70f2 — New collateral-vs-payout reuse rule tightens consensus for legacy ProRegTx/ProUpRegTx versions no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior collateral-vs-payout reuse finding is FIXED — the new commit gates that check on nVersion >= ProTxVersion::MultiPayout at both ProRegTx and ProUpRegTx sites and adds a regression test. One legacy-consensus concern remains in CProUpRegTx::IsTriviallyValid, which still passes a placeholder CKeyID{} as the owner into IsPayoutListTriviallyValid and would reject a v1-v3 payout whose destination happens to be PKHash(0). Practically unreachable but consensus-shaped, and parallel to the issue the author just fixed.
🔴 1 blocking
🤖 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/evo/providertx.cpp`:
- [BLOCKING] src/evo/providertx.cpp:275: ProUpRegTx trivial validation tightens v1-v3 consensus via placeholder owner key
`CProUpRegTx::IsTriviallyValid()` now invokes `IsPayoutListTriviallyValid(GetOwnerPayouts(...), CKeyID{}, keyIDVoting, state)` (providertx.cpp:275). Inside the helper (providertx.cpp:79), each payout destination is compared against `PKHash(keyIDOwner)`. With the placeholder `CKeyID{}`, that comparison rejects any payout whose destination is the all-zeros PKHash. Pre-PR, trivial validation only required the payout script to be P2PKH/P2SH; the owner-reuse check was the contextual one in `CheckProUpRegTx`, which used the real `dmn->pdmnState->keyIDOwner`. The current PR keeps that contextual call (specialtxman.cpp:1233-1234), so the trivial-level owner comparison is both redundant and a new legacy-consensus rule for v1-v3 ProUpRegTx payloads.
This is the same shape of issue the author just fixed for collateral-vs-payout reuse: applying a new MultiPayout-only constraint unconditionally to legacy payloads. Reachability is admittedly tiny — no realistic key hashes to 0 — but a v1-v3 ProUpRegTx with a P2PKH(0) payout that pre-PR nodes accept and post-PR nodes reject is still a split-shaped behavior change to legacy consensus.
Fix options: (1) gate the trivial owner-key comparison to `nVersion >= ProTxVersion::MultiPayout` (the contextual check still enforces it correctly for all versions), or (2) make `IsPayoutListTriviallyValid` skip the owner comparison when `keyIDOwner.IsNull()`. Either keeps the v1-v3 trivial check byte-equivalent to the pre-PR behavior.
| if (!scriptPayout.IsPayToPublicKeyHash() && !scriptPayout.IsPayToScriptHash()) { | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-payee"); | ||
| } | ||
| if (!IsPayoutListTriviallyValid(GetOwnerPayouts(nVersion, scriptPayout, payouts), CKeyID{}, keyIDVoting, state)) return false; |
There was a problem hiding this comment.
🔴 Blocking: ProUpRegTx trivial validation tightens v1-v3 consensus via placeholder owner key
CProUpRegTx::IsTriviallyValid() now invokes IsPayoutListTriviallyValid(GetOwnerPayouts(...), CKeyID{}, keyIDVoting, state) (providertx.cpp:275). Inside the helper (providertx.cpp:79), each payout destination is compared against PKHash(keyIDOwner). With the placeholder CKeyID{}, that comparison rejects any payout whose destination is the all-zeros PKHash. Pre-PR, trivial validation only required the payout script to be P2PKH/P2SH; the owner-reuse check was the contextual one in CheckProUpRegTx, which used the real dmn->pdmnState->keyIDOwner. The current PR keeps that contextual call (specialtxman.cpp:1233-1234), so the trivial-level owner comparison is both redundant and a new legacy-consensus rule for v1-v3 ProUpRegTx payloads.
This is the same shape of issue the author just fixed for collateral-vs-payout reuse: applying a new MultiPayout-only constraint unconditionally to legacy payloads. Reachability is admittedly tiny — no realistic key hashes to 0 — but a v1-v3 ProUpRegTx with a P2PKH(0) payout that pre-PR nodes accept and post-PR nodes reject is still a split-shaped behavior change to legacy consensus.
Fix options: (1) gate the trivial owner-key comparison to nVersion >= ProTxVersion::MultiPayout (the contextual check still enforces it correctly for all versions), or (2) make IsPayoutListTriviallyValid skip the owner comparison when keyIDOwner.IsNull(). Either keeps the v1-v3 trivial check byte-equivalent to the pre-PR behavior.
source: ['codex']
There was a problem hiding this comment.
Good catch! I think option 2 sounds right.
There was a problem hiding this comment.
Resolved in this update — ProUpRegTx trivial validation tightens v1-v3 consensus via placeholder owner key no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f6565316a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!IsPayoutListKeySafe(owner_payouts, collateralTxDest, dmn->pdmnState->keyIDOwner, opt_ptx->keyIDVoting, | ||
| opt_ptx->nVersion >= ProTxVersion::MultiPayout, state)) return false; |
There was a problem hiding this comment.
When v24 is active and an existing masternode is already at MultiPayout, a crafted v2 ProUpRegTx can set its legacy scriptPayout to the collateral address: this guard keys check_payout_collateral_reuse off opt_ptx->nVersion, so the payout-vs-collateral check is skipped. RebuildListFromBlock then uses target_version = max(old_version, opt_proTx->nVersion) and converts that legacy script into newState->payouts, leaving a v4 payout-share state that violates the new collateral-reuse restriction.
Useful? React with 👍 / 👎.
PastaPastaPasta
force-pushed
the
feat/multi-party-payout
branch
from
5f65653 to
9f5400b
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified the reviewers' shared consensus finding at exact head 5fca893: pre-v24 registrar updates can still lower an existing BasicBLS masternode state to LegacyBLS when the operator key changes. The new payout/collateral-reuse delta did not add a separate issue, and the prior null-owner payout blocker is resolved.
🔴 1 blocking
🤖 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/evo/specialtxman.cpp`:
- [BLOCKING] src/evo/specialtxman.cpp:458-459: Pre-v24 operator updates can downgrade BasicBLS masternode state
For pre-v24 blocks this branch still sets `target_version` to the registrar transaction version whenever the operator key changes. Contextual validation does not stop that downgrade before v24 (`IsVersionChangeValid` returns true while DEPLOYMENT_V24 is inactive), and `CProUpRegTx::IsTriviallyValid` permits a legacy v1 registrar update with a legacy operator key as long as the version is within the deployment maximum. As a result, a masternode whose persisted state is already `BasicBLS` can submit an operator-changing legacy ProUpRegTx; block connection then resets the operator fields and calls `SetStateVersion(..., LegacyBLS)`, lowering the deterministic masternode state version. This reintroduces legacy operator-state after the basic scheme is active and makes consensus state downgradeable.
| const uint16_t target_version{is_v24_deployed ? std::max<uint16_t>(old_version, opt_proTx->nVersion) | ||
| : (operator_changed ? opt_proTx->nVersion : old_version)}; |
There was a problem hiding this comment.
🔴 Blocking: Pre-v24 operator updates can downgrade BasicBLS masternode state
For pre-v24 blocks this branch still sets target_version to the registrar transaction version whenever the operator key changes. Contextual validation does not stop that downgrade before v24 (IsVersionChangeValid returns true while DEPLOYMENT_V24 is inactive), and CProUpRegTx::IsTriviallyValid permits a legacy v1 registrar update with a legacy operator key as long as the version is within the deployment maximum. As a result, a masternode whose persisted state is already BasicBLS can submit an operator-changing legacy ProUpRegTx; block connection then resets the operator fields and calls SetStateVersion(..., LegacyBLS), lowering the deterministic masternode state version. This reintroduces legacy operator-state after the basic scheme is active and makes consensus state downgradeable.
Suggested direction: do not let pre-v24 operator-changing registrar updates lower the existing state version. For example, keep the higher state version when the operator key changes:
operator_changed ? std::max<uint16_t>(old_version, opt_proTx->nVersion) : old_version
Alternatively, reject contextual registrar downgrades explicitly. Please also add a regression test that starts with a pre-v24 BasicBLS state, submits a legacy v1 operator-key update, and verifies the state remains BasicBLS or the transaction is rejected.
source: reviewer-a, reviewer-b
There was a problem hiding this comment.
Resolved in 43dc7bd — Pre-v24 operator updates can downgrade BasicBLS masternode state no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
Thank you, this is quite interesting. Is it also possible to enable multi-party collateral funding but so that a masternode could be destroyed and everyone get their share back at the protocol level? I was thinking about it in the past, you can achieve multi party funding (and a registration) via partially signed transactions right now, but there is missing a mechanism that guarantee you can destroy a node any time and everyone get their share back. |
Later; I want to do 1 thing at a time :D |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #7340 introduces v4 ProRegTx/ProUpRegTx multi-payout (gated to v24). The latest delta (78b42ff) is a small test adjustment in rpc_netinfo.py updating the expected default ProTx version from v3 (ExtAddr) to v4 (MultiPayout), which matches the new ProTxVersion::GetMax behavior. Cumulative review of providertx, dmnstate gating, RebuildListFromBlock transitions, payment splitting, bloom/filter matching, validation (count/reward/duplicate/key reuse), JSON/RPC, SML diff, and tests surfaces no in-scope blocking or suggestion-level issues. Codex flagged a multiset-matching weakness in IsTransactionValid; verification confirms it is a pre-existing weakness not introduced by this PR — recorded as out-of-scope.
Extracts the pre-v24 (legacy v1-v3) registrar version-handling regression test from PR dashpay#7340 onto develop, so it validates already-deployed consensus independent of the DIP-0026 changes. proupreg_version_handling_before_v24 covers: an unchanged-operator registrar update keeps the stored MN state version; an operator-key change adopts the tx version; and a v1 update that changes the operator key of a v2 masternode downgrades the state to LegacyBLS and re-encodes the operator key with the legacy BLS scheme. Passes against unmodified develop, confirming it describes deployed behavior. The v4/MultiPayout-dependent cases from the PR are intentionally omitted since those types do not exist on develop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Commit 43dc7bd applied the version-bump max() to pre-v24 registrar updates whenever the operator key changes, not only under v24. Pre-v24 a masternode is only ever v1/v2, so a v1 (LegacyBLS) ProUpRegTx that changes the operator key of a v2 (BasicBLS) masternode now keeps BasicBLS instead of adopting the tx version (LegacyBLS) as deployed consensus does. The state version drives the CSimplifiedMNListEntry serialization (operator-key scheme and nType/platform presence), so this diverges merkleRootMNList from already-deployed nodes and can split the chain. Gate the downgrade-prevention behind DEPLOYMENT_V24, restoring the legacy behavior (adopt the tx version on operator change) before v24. Correct proupreg_version_handling_before_v24, which asserted the divergent result, and extend its downgrade step to also verify the submitted operator key is stored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at ec013c5 confirms the prior 78b42ff clean review remains accurate. No carried-forward prior findings need to gate this PR. The latest delta (single commit restoring pre-v24 ProUpRegTx state-version semantics) is correct and matches the updated unit test. Codex's finding about duplicate-payee validation is a real pre-existing consensus bug that exists identically without this PR (any_of-based loop was not touched by PR #7340), so it's recorded as an out-of-scope maintainer follow-up rather than a blocker. CodeRabbit had zero inline findings; no reactions needed.
|
Nice work getting this up. I independently implemented DIP-0026 as well (#7353), so rather than open a competing PR I would rather help review and converge here. Two substantive points came out of comparing the two implementations. 1. The duff-remainder rounding rule is unspecified, and our two implementations already diverge on it. When 2. The coinbase validator is not multiplicity-correct. |
| const auto& arr = value.get_array(); | ||
| if (arr.empty()) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must contain at least one entry", paramName)); | ||
| } |
There was a problem hiding this comment.
Could reject oversized payout arrays at RPC-parse time with a clear error, before any serialization — so the user gets immediate feedback instead of building a tx that consensus would reject:
| } | |
| } | |
| if (arr.size() > 8) { | |
| throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must not contain more than 8 entries", paramName)); | |
| } |
5 participants
Issue being fixed or feature implemented
Implements the masternode reward shares DIP by adding provider transaction payload version 4 for multi-payout owner reward lists.
What was done?
How Has This Been Tested?
./src/test/test_dash --run_test=evo_trivialvalidationtest/functional/feature_masternode_payout_shares.pymake -j4 src/dashdgit diff --checktest/lint/lint-whitespace.pytest/lint/lint-python.py test/functional/feature_masternode_payout_shares.py test/functional/test_framework/test_framework.pyskipped becauseflake8is not installed.Breaking Changes
No breaking changes for legacy provider transaction payload versions. New unified payout lists are gated to provider transaction payload version 4 under the existing v24 deployment.
Checklist:
This pull request was created by Codex.