fix!: use the same version along all protx special transactions#7302
fix!: use the same version along all protx special transactions#7302knst wants to merge 14 commits into
Conversation
|
✅ Review complete (commit ff6151b) |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
![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: ed4d9c63ea
ℹ️ 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".
| ptx.nVersion = DeploymentToProtxVersion(WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()), chainman, | ||
| /*is_basic_override=*/!use_legacy); |
There was a problem hiding this comment.
When DEPLOYMENT_V24 is active, this assignment makes protx update_registrar default to version 3, but legacy masternodes (dmn->pdmnState->nVersion == LegacyBLS) are still required by IsVersionChangeValid in src/evo/specialtxman.cpp to move to version 2 before any higher version. As a result, the RPC now constructs transactions that fail with bad-protx-version-upgrade for legacy nodes, which breaks the legacy→basic upgrade path (and related registrar updates) via the standard RPC. update_service and revoke already avoid this with a BasicBLS clamp, so update_registrar is now inconsistent and regresses behavior introduced by this commit.
Useful? React with 👍 / 👎.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (25)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR introduces shared ProTx version constants and a deployment-based version helper, then refactors provider transaction validation to use a simpler Sequence Diagram(s)sequenceDiagram
participant RPC as src/rpc/evo.cpp
participant Validation as src/validation.cpp
participant SpecialTxMan as src/evo/specialtxman.cpp
participant ProTx as CProRegTx/CProUpServTx/CProUpRegTx/CProUpRevTx
RPC->>Validation: DeploymentToProtxVersion(pindexPrev, chainman, override)
Validation-->>RPC: allowed nVersion
RPC->>SpecialTxMan: build special tx with nVersion
SpecialTxMan->>ProTx: IsTriviallyValid(state)
ProTx-->>SpecialTxMan: accept or reject
SpecialTxMan-->>RPC: validated payload or error
Estimated code review effortEstimated code review effort: 4 (Complex) | ~60 minutes 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 |
knst
force-pushed
the
fix-providertx-versions
branch
from
ed4d9c6 to
f94f7cc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/evo/providertx.h (1)
95-99: ⚡ Quick win
GetValidatedPayload<T>is not full validation.These comments overstate the helper’s contract. As implemented in
src/evo/specialtxman.cpp,GetValidatedPayloadonly covers payload decoding, deployment-gated version bounds, andIsTriviallyValid(...); callers still needCheckPro*Txfor collateral, masternode-list, signature, input-hash, and version-transition checks. Please reword this so future call sites don’t treat the helper as sufficient consensus validation.Also applies to: 160-164, 211-215, 265-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/evo/providertx.h` around lines 95 - 99, The comment for IsTriviallyValid overstresses GetValidatedPayload<T> — update the wording to clearly state that GetValidatedPayload<T> only performs payload decoding, deployment-gated version bounds checks and calls IsTriviallyValid, and that callers must still run full consensus checks (e.g., CheckProRegTx / CheckProUpServTx or other CheckPro*Tx functions) to validate collateral, masternode-list state, signatures, input-hash and version-transition rules; change the docstring near IsTriviallyValid and the similar comments at the other three locations to explicitly list those remaining checks and not present GetValidatedPayload<T> as full validation.src/test/evo_trivialvalidation.cpp (1)
61-64: ⚡ Quick winAdd a post-v24 fixture path here.
This runner still collapses the matrix to
"legacy"vs"basic", so none of the new version-3 behavior introduced in this PR gets exercised. Please add an"extaddr"/post-v24 branch and vectors for at leastCProUpRegTxandCProUpRevTx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/evo_trivialvalidation.cpp` around lines 61 - 64, The test currently only selects between "basic" and "legacy" via test[2].get_str() when setting pindexPrev, so post-v24/extaddr vectors aren't exercised; update the selection logic (replace the two-way ternary around pindexPrev or add an if/else/switch) to handle a third value "extaddr" and pick an appropriate CBlockIndex from chainman.ActiveChain() for the post-v24 path, and add corresponding test vectors for CProUpRegTx and CProUpRevTx in the test matrix so the "extaddr" branch is executed during the trivial validation runner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/evo/specialtxman.h`:
- Around line 101-103: The template function GetValidatedPayload is defined in a
.cpp which will cause linker errors because the tests instantiate it for
concrete types; fix by either moving the full template definition into the
header where it's declared (so the compiler can instantiate for CProRegTx,
CProUpServTx, CProUpRegTx, CProUpRevTx) or, if you keep the definition in
specialtxman.cpp, add explicit template instantiations for
GetValidatedPayload<CProRegTx>, GetValidatedPayload<CProUpServTx>,
GetValidatedPayload<CProUpRegTx>, and GetValidatedPayload<CProUpRevTx> in that
.cpp so the linker sees the generated symbols.
---
Nitpick comments:
In `@src/evo/providertx.h`:
- Around line 95-99: The comment for IsTriviallyValid overstresses
GetValidatedPayload<T> — update the wording to clearly state that
GetValidatedPayload<T> only performs payload decoding, deployment-gated version
bounds checks and calls IsTriviallyValid, and that callers must still run full
consensus checks (e.g., CheckProRegTx / CheckProUpServTx or other CheckPro*Tx
functions) to validate collateral, masternode-list state, signatures, input-hash
and version-transition rules; change the docstring near IsTriviallyValid and the
similar comments at the other three locations to explicitly list those remaining
checks and not present GetValidatedPayload<T> as full validation.
In `@src/test/evo_trivialvalidation.cpp`:
- Around line 61-64: The test currently only selects between "basic" and
"legacy" via test[2].get_str() when setting pindexPrev, so post-v24/extaddr
vectors aren't exercised; update the selection logic (replace the two-way
ternary around pindexPrev or add an if/else/switch) to handle a third value
"extaddr" and pick an appropriate CBlockIndex from chainman.ActiveChain() for
the post-v24 path, and add corresponding test vectors for CProUpRegTx and
CProUpRevTx in the test matrix so the "extaddr" branch is executed during the
trivial validation runner.
🪄 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: e2a718c8-ebeb-4c10-8f6b-eedbc74713c3
📒 Files selected for processing (17)
src/evo/deterministicmns.hsrc/evo/dmnstate.hsrc/evo/netinfo.cppsrc/evo/providertx.cppsrc/evo/providertx.hsrc/evo/simplifiedmns.hsrc/evo/smldiff.hsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/types.hsrc/llmq/commitment.cppsrc/rpc/evo.cppsrc/test/data/trivially_invalid.jsonsrc/test/evo_trivialvalidation.cppsrc/validation.cppsrc/validation.htest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (3)
- src/evo/dmnstate.h
- src/evo/smldiff.h
- src/evo/deterministicmns.h
✅ Files skipped from review due to trivial changes (5)
- src/llmq/commitment.cpp
- src/evo/types.h
- src/test/data/trivially_invalid.json
- test/lint/lint-circular-dependencies.py
- src/evo/simplifiedmns.h
🚧 Files skipped from review as they are similar to previous changes (5)
- src/evo/netinfo.cpp
- src/validation.cpp
- src/validation.h
- src/evo/specialtxman.cpp
- src/evo/providertx.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The refactor is sound but introduces a regression in the non-legacy protx update_registrar RPC: after V24 activates, it produces a v3 ProUpRegTx that consensus rejects when the masternode is still in LegacyBLS state, and also trips a CHECK_NONFATAL when the caller reuses the existing legacy operator key. Sibling RPCs (update_service, revoke) have the correct BasicBLS clamp and update_registrar should match. Test coverage for the newly-permitted v3 ProUpRegTx/ProUpRevTx path is also missing.
Reviewed commit: f94f7cc
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: No regression coverage for the newly-permitted v3 ProUpRegTx/ProUpRevTx path
src/test/evo_trivialvalidation.cpp (lines 60-64)
This PR removes the bad-protx-version-tx-type consensus check that previously rejected CProUpRegTx/CProUpRevTx at version 3, and routes the test harness through the same GetValidatedPayload path consensus uses. However, the harness still only accepts "basic" and "legacy" vectors and leaves a TODO for extended addresses, so none of the new acceptance rules are actually exercised at a post-V24 height. Adding a v3 ProUpRegTx/ProUpRevTx vector validated against a post-V24 pindexPrev would lock in the new contract and protect against accidental re-introduction of the dropped check.
🤖 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/rpc/evo.cpp`:
- [BLOCKING] lines 1139-1140: `protx update_registrar` produces an invalid v3 payload for legacy-state masternodes after V24
After V24 activates, `DeploymentToProtxVersion(tip, chainman, /*is_basic_override=*/!use_legacy)` returns `ProTxVersion::ExtAddr` (3) for the non-legacy path. Previously, `GetMaxFromDeployment<CProUpRegTx>` capped this at `BasicBLS` (2) for registrar updates. Two consequences:
1. If the underlying masternode is still at state version `LegacyBLS` (1), `IsVersionChangeValid` (specialtxman.cpp:907-909) rejects the v3 jump with `bad-protx-version-upgrade`. There is no longer a working non-legacy RPC path to migrate a legacy masternode via `update_registrar`.
2. If the caller leaves the operator key unchanged, `ptx.pubKeyOperator` is reused from `dmn->pdmnState`, which is legacy. The `CHECK_NONFATAL(ptx.pubKeyOperator.IsLegacy() == (ptx.nVersion == ProTxVersion::LegacyBLS))` at line 1159 then trips (true == false).
The sibling wrappers `protx update_service` (1014-1018) and `protx revoke` (1268-1272) already clamp to BasicBLS in this case. `update_registrar` needs the same clamp. Real-world impact is limited because V24 is `NEVER_ACTIVE` on mainnet/testnet, but the regression is real and silently breaks devnet/regtest setups.
In `src/test/evo_trivialvalidation.cpp`:
- [SUGGESTION] lines 60-64: No regression coverage for the newly-permitted v3 ProUpRegTx/ProUpRevTx path
This PR removes the `bad-protx-version-tx-type` consensus check that previously rejected `CProUpRegTx`/`CProUpRevTx` at version 3, and routes the test harness through the same `GetValidatedPayload` path consensus uses. However, the harness still only accepts `"basic"` and `"legacy"` vectors and leaves a TODO for extended addresses, so none of the new acceptance rules are actually exercised at a post-V24 height. Adding a v3 ProUpRegTx/ProUpRevTx vector validated against a post-V24 `pindexPrev` would lock in the new contract and protect against accidental re-introduction of the dropped check.
| ptx.nVersion = DeploymentToProtxVersion(WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()), chainman, | ||
| /*is_basic_override=*/!use_legacy); |
There was a problem hiding this comment.
🔴 Blocking: protx update_registrar produces an invalid v3 payload for legacy-state masternodes after V24
After V24 activates, DeploymentToProtxVersion(tip, chainman, /*is_basic_override=*/!use_legacy) returns ProTxVersion::ExtAddr (3) for the non-legacy path. Previously, GetMaxFromDeployment<CProUpRegTx> capped this at BasicBLS (2) for registrar updates. Two consequences:
-
If the underlying masternode is still at state version
LegacyBLS(1),IsVersionChangeValid(specialtxman.cpp:907-909) rejects the v3 jump withbad-protx-version-upgrade. There is no longer a working non-legacy RPC path to migrate a legacy masternode viaupdate_registrar. -
If the caller leaves the operator key unchanged,
ptx.pubKeyOperatoris reused fromdmn->pdmnState, which is legacy. TheCHECK_NONFATAL(ptx.pubKeyOperator.IsLegacy() == (ptx.nVersion == ProTxVersion::LegacyBLS))at line 1159 then trips (true == false).
The sibling wrappers protx update_service (1014-1018) and protx revoke (1268-1272) already clamp to BasicBLS in this case. update_registrar needs the same clamp. Real-world impact is limited because V24 is NEVER_ACTIVE on mainnet/testnet, but the regression is real and silently breaks devnet/regtest setups.
source: ['claude', 'codex']
🤖 Fix this 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/rpc/evo.cpp`:
- [BLOCKING] lines 1139-1140: `protx update_registrar` produces an invalid v3 payload for legacy-state masternodes after V24
After V24 activates, `DeploymentToProtxVersion(tip, chainman, /*is_basic_override=*/!use_legacy)` returns `ProTxVersion::ExtAddr` (3) for the non-legacy path. Previously, `GetMaxFromDeployment<CProUpRegTx>` capped this at `BasicBLS` (2) for registrar updates. Two consequences:
1. If the underlying masternode is still at state version `LegacyBLS` (1), `IsVersionChangeValid` (specialtxman.cpp:907-909) rejects the v3 jump with `bad-protx-version-upgrade`. There is no longer a working non-legacy RPC path to migrate a legacy masternode via `update_registrar`.
2. If the caller leaves the operator key unchanged, `ptx.pubKeyOperator` is reused from `dmn->pdmnState`, which is legacy. The `CHECK_NONFATAL(ptx.pubKeyOperator.IsLegacy() == (ptx.nVersion == ProTxVersion::LegacyBLS))` at line 1159 then trips (true == false).
The sibling wrappers `protx update_service` (1014-1018) and `protx revoke` (1268-1272) already clamp to BasicBLS in this case. `update_registrar` needs the same clamp. Real-world impact is limited because V24 is `NEVER_ACTIVE` on mainnet/testnet, but the regression is real and silently breaks devnet/regtest setups.
kwvg
left a comment
kwvg
left a comment
There was a problem hiding this comment.
Concept NACK
From commit d86d571
CProRegTx and CProUpServTx used to be the only type of protx that have
a different version. It is theoretically acceptable in assumption that
there is no new features or version will ever be introduced for protx
special transaction.
The versioning system made an assumption that any change in ProTx structures will affect all fields uniformly, leaving certain fields v2 and others v3 allows us to prevent unexpected upgrades based on changes to fields that were never modified and thus prevents the propagation of serialisation directives based on fields never changed.
Even if the RPC sets the version we intend, removing a way to distinguish what txType is allowed to set the version at the consensus level means a patch in RPC could allow corruption of the masternode list as it'll see a v3 transaction, assume v3 serialization and then on client restart, cannot recognise the bytes on disk as it's v2 bytes but v3 version.
A theoretical v4 would follow the same pattern, set the version where the fields could be updated (always at creation OR for existing nodes, when the relevant ProTx is submitted) and then version-solve accordingly. The current system doesn't block that.
Let's assume that revocation is now at v4, future rules could force that you first upgrade to v3 by submitting a new ProUpRegTx (explicit user action that submits the new ser format) to then use v4 ProUpRevTx (to avoid getting a v2 to v4 not allowed error).
| return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-version-upgrade"); | ||
| } | ||
|
|
||
| if (tx_type != TRANSACTION_PROVIDER_UPDATE_SERVICE && tx_version == ProTxVersion::ExtAddr) { |
There was a problem hiding this comment.
The reason we have version transition rules is to prevent the node from being classed as ExtAddr without actually opting in to it at creation or by explicitly updating the service.
Especially since once the version is upgraded the serialisation format changes. By classifying all transactions ExtAddr, a revoke transaction could indicate that the node intends to follow ExtAddr serialization when that was a) not the user's intention and b) will cause serialization mismatches which are very dependent on the version (see below)
Lines 78 to 79 in 4a0fd85
|
|
||
|
|
||
| /** | ||
| * This helper does some trivial validations that doesn't depends on collateral and |
There was a problem hiding this comment.
These changes are valuable but must be decoupled from changes in consensus behavior
| /** Get highest permissible ProTx version based on deployment status | ||
| * Note: The override is needed because some RPCs need to use deployment status information for everything *except* | ||
| * the BLS version upgrade since they are specializations for a specific BLS version. This is a one-off. | ||
| * TODO: Resolve this oddity. Consider deprecating legacy BLS-only RPCs so we can remove them eventually. |
There was a problem hiding this comment.
| * TODO: Resolve this oddity. Consider deprecating legacy BLS-only RPCs so we can remove them eventually. |
We can probably drop this TODO since the legacy RPCs won't be going anywhere even after deprecation due to hard requirements in functional tests, we already have network rules to enforce the deprecation at a consensus level but the RPCs themselves remain indispensible.
No, they would not, because:
|
The issue isn't the new node creation path, it's the upgrade path, say
But without at a minimum restricting the upgrade path, a specially crafted transaction could update the expected serialization to v3 with a ProUpRevTx (despite holding no service data but now allowed to mark itself as v3) and then on restart, when reading the masternode list and encountering a v3, find the v2 bytes unrecognizable and emit an error. |
I will write some extra tests; PR is draft temporary |
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
fix-providertx-versions
branch
from
f94f7cc to
a016390
Compare
|
This pull request has conflicts, please rebase. |
…ated" This reverts commit 3594769e845b9b03cc2d08c6cf786b7df0f835e5.
…dation This commit replaces usage of helpers GetPayload+IsTrivialValid to GetValidatedPayload It unifies validation between regression tests and production code and useful for the next commits in PR
CProRegTx and CProUpServTx used to be the only type of protx that have a different version. It is theoretically acceptable in assumption that there is no new features or version will ever be introduced for protx special transaction. Though, for better compatibility for futher version, unification, simplicity of documentation and to reduce user's confusions for after-v24 version of CProUpRegTx and CProUpRevTx are allowed to be "ext addresses" even they don't have any network related fields _at the moment_ So, since now: - version 1: legacy BLS, extended addresses disallowed (pre v19 fork) - version 2: basic BLS, extended addresses disallowed (since v19 fork) - version 3: basic BLS, extended addresses allowed, multi-payouts allowed (since v24 fork) NOTE: there are also classes CSimplifiedMNListEntry and CDeterministicMNState use the same enum for its version; moreover CDeterministicMNState inherits version directly from CProRegTx. This refactoring doesn't contradict or conflict this behavior
It helps to drop multiple circular dependencies for providertx <-> validation.h as a side effect
knst
force-pushed
the
fix-providertx-versions
branch
from
a016390 to
ff6151b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff6151b1a7
ℹ️ 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".
| if (obj.nVersion >= ProTxVersion::ExtAddr) { | ||
| READWRITE(obj.payouts); |
There was a problem hiding this comment.
Because v3 states now take the payouts branch, a basic v2 masternode upgraded by a post-v24 ProUpServTx loses its owner payee: the service-update path in CSpecialTxProcessor::RebuildListFromBlock can raise nVersion from 2 to 3 while only converting netInfo, and v2 states have an empty payouts vector because they serialized scriptPayout. After that, GetOwnerPayouts(state) returns an empty list, so masternode payment construction omits the owner payout (and callers that use .front() can fail) until a registrar update backfills it. Please migrate scriptPayout to LegacyPayoutAsList when any non-registrar version bump crosses ExtAddr.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/evo/specialtxman.cpp (1)
966-983: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
GetValidatedPayload<T>visible to external callers
src/test/evo_trivialvalidation.cppalready usesGetValidatedPayload<T>(), butsrc/evo/specialtxman.cppdefines the template without any explicit instantiations. Move the definition to the header or add instantiations forCProRegTx,CProUpServTx,CProUpRegTx, andCProUpRevTxto avoid link failures.🤖 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 966 - 983, The templated GetValidatedPayload<T> helper is only defined in specialtxman.cpp, so external users like the test code can’t link against it. Make the template visible outside this translation unit by either moving the definition of GetValidatedPayload into the corresponding header or adding explicit instantiations for the ProTx types used by callers: CProRegTx, CProUpServTx, CProUpRegTx, and CProUpRevTx. Keep the existing validation logic in GetValidatedPayload and ensure the chosen approach preserves the current behavior while making the symbol available to external callers.
🧹 Nitpick comments (1)
src/rpc/masternode.cpp (1)
225-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating owner-payout string building.
GetRequiredPaymentsString's owner-payout loop (232-239) andGetOwnerPayoutsString(271-278) build an identical comma-separated destination string fromGetOwnerPayouts(...). Consider havingGetRequiredPaymentsStringcallGetOwnerPayoutsString(*payee->pdmnState)and append the operator-reward suffix separately, to avoid future divergence between the two implementations.♻️ Proposed dedup
std::string strPayments = "Unknown"; if (payee) { - strPayments.clear(); - for (const auto& payout : GetOwnerPayouts(*payee->pdmnState)) { - CTxDestination dest; - if (!ExtractDestination(payout.scriptPayout, dest)) { - NONFATAL_UNREACHABLE(); - } - if (!strPayments.empty()) strPayments += ", "; - strPayments += EncodeDestination(dest); - } + strPayments = GetOwnerPayoutsString(*payee->pdmnState); if (payee->nOperatorReward != 0 && payee->pdmnState->scriptOperatorPayout != CScript()) {Note:
GetOwnerPayoutsStringis defined afterGetRequiredPaymentsStringin this file, so a forward declaration (or reordering) would be needed.Also applies to: 268-280
🤖 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 225 - 266, `GetRequiredPaymentsString` duplicates the owner-payout comma-separated destination building already implemented in `GetOwnerPayoutsString`. Refactor `GetRequiredPaymentsString` to reuse `GetOwnerPayoutsString(*payee->pdmnState)` for the owner payout portion, then append the operator-reward destination separately if needed. Since `GetOwnerPayoutsString` is defined later in `masternode.cpp`, add a forward declaration or move the helper so `GetRequiredPaymentsString` can call it cleanly.
🤖 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/rpc/evo.cpp`:
- Line 31: The `register_prepare` path in `src/rpc/evo.cpp` returns a prepared
ProTx after `SetTxPayload()` without validating the payout-share array, so
malformed rewards can slip through. Add a check in the payout-share
parsing/validation logic to ensure the array totals exactly match
`MasternodePayoutShare::MAX_REWARD` before building the transaction, and reject
values outside the allowed `uint16_t` range or any sum mismatch before returning
from `register_prepare`.
---
Outside diff comments:
In `@src/evo/specialtxman.cpp`:
- Around line 966-983: The templated GetValidatedPayload<T> helper is only
defined in specialtxman.cpp, so external users like the test code can’t link
against it. Make the template visible outside this translation unit by either
moving the definition of GetValidatedPayload into the corresponding header or
adding explicit instantiations for the ProTx types used by callers: CProRegTx,
CProUpServTx, CProUpRegTx, and CProUpRevTx. Keep the existing validation logic
in GetValidatedPayload and ensure the chosen approach preserves the current
behavior while making the symbol available to external callers.
---
Nitpick comments:
In `@src/rpc/masternode.cpp`:
- Around line 225-266: `GetRequiredPaymentsString` duplicates the owner-payout
comma-separated destination building already implemented in
`GetOwnerPayoutsString`. Refactor `GetRequiredPaymentsString` to reuse
`GetOwnerPayoutsString(*payee->pdmnState)` for the owner payout portion, then
append the operator-reward destination separately if needed. Since
`GetOwnerPayoutsString` is defined later in `masternode.cpp`, add a forward
declaration or move the helper so `GetRequiredPaymentsString` can call it
cleanly.
🪄 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: 5188d641-e727-4ee4-93ca-c0efd12f24db
📒 Files selected for processing (25)
src/common/bloom.cppsrc/evo/core_write.cppsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/dmnstate.cppsrc/evo/dmnstate.hsrc/evo/netinfo.cppsrc/evo/providertx.cppsrc/evo/providertx.hsrc/evo/providertx_util.cppsrc/evo/simplifiedmns.cppsrc/evo/simplifiedmns.hsrc/evo/smldiff.hsrc/evo/specialtx_filter.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/types.hsrc/llmq/commitment.cppsrc/masternode/payments.cppsrc/node/interfaces.cppsrc/rpc/evo.cppsrc/rpc/masternode.cppsrc/test/data/trivially_invalid.jsonsrc/test/evo_deterministicmns_tests.cppsrc/test/evo_trivialvalidation.cpp
💤 Files with no reviewable changes (4)
- src/evo/smldiff.h
- src/test/evo_trivialvalidation.cpp
- src/evo/deterministicmns.h
- src/test/evo_deterministicmns_tests.cpp
✅ Files skipped from review due to trivial changes (4)
- src/llmq/commitment.cpp
- src/evo/netinfo.cpp
- src/evo/simplifiedmns.h
- src/test/data/trivially_invalid.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/evo/types.h
- src/evo/specialtxman.h
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/evo/specialtxman.cpp (1)
966-983: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
GetValidatedPayload<T>visible to external callers
src/test/evo_trivialvalidation.cppalready usesGetValidatedPayload<T>(), butsrc/evo/specialtxman.cppdefines the template without any explicit instantiations. Move the definition to the header or add instantiations forCProRegTx,CProUpServTx,CProUpRegTx, andCProUpRevTxto avoid link failures.🤖 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 966 - 983, The templated GetValidatedPayload<T> helper is only defined in specialtxman.cpp, so external users like the test code can’t link against it. Make the template visible outside this translation unit by either moving the definition of GetValidatedPayload into the corresponding header or adding explicit instantiations for the ProTx types used by callers: CProRegTx, CProUpServTx, CProUpRegTx, and CProUpRevTx. Keep the existing validation logic in GetValidatedPayload and ensure the chosen approach preserves the current behavior while making the symbol available to external callers.
🧹 Nitpick comments (1)
src/rpc/masternode.cpp (1)
225-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating owner-payout string building.
GetRequiredPaymentsString's owner-payout loop (232-239) andGetOwnerPayoutsString(271-278) build an identical comma-separated destination string fromGetOwnerPayouts(...). Consider havingGetRequiredPaymentsStringcallGetOwnerPayoutsString(*payee->pdmnState)and append the operator-reward suffix separately, to avoid future divergence between the two implementations.♻️ Proposed dedup
std::string strPayments = "Unknown"; if (payee) { - strPayments.clear(); - for (const auto& payout : GetOwnerPayouts(*payee->pdmnState)) { - CTxDestination dest; - if (!ExtractDestination(payout.scriptPayout, dest)) { - NONFATAL_UNREACHABLE(); - } - if (!strPayments.empty()) strPayments += ", "; - strPayments += EncodeDestination(dest); - } + strPayments = GetOwnerPayoutsString(*payee->pdmnState); if (payee->nOperatorReward != 0 && payee->pdmnState->scriptOperatorPayout != CScript()) {Note:
GetOwnerPayoutsStringis defined afterGetRequiredPaymentsStringin this file, so a forward declaration (or reordering) would be needed.Also applies to: 268-280
🤖 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 225 - 266, `GetRequiredPaymentsString` duplicates the owner-payout comma-separated destination building already implemented in `GetOwnerPayoutsString`. Refactor `GetRequiredPaymentsString` to reuse `GetOwnerPayoutsString(*payee->pdmnState)` for the owner payout portion, then append the operator-reward destination separately if needed. Since `GetOwnerPayoutsString` is defined later in `masternode.cpp`, add a forward declaration or move the helper so `GetRequiredPaymentsString` can call it cleanly.
🤖 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/rpc/evo.cpp`:
- Line 31: The `register_prepare` path in `src/rpc/evo.cpp` returns a prepared
ProTx after `SetTxPayload()` without validating the payout-share array, so
malformed rewards can slip through. Add a check in the payout-share
parsing/validation logic to ensure the array totals exactly match
`MasternodePayoutShare::MAX_REWARD` before building the transaction, and reject
values outside the allowed `uint16_t` range or any sum mismatch before returning
from `register_prepare`.
---
Outside diff comments:
In `@src/evo/specialtxman.cpp`:
- Around line 966-983: The templated GetValidatedPayload<T> helper is only
defined in specialtxman.cpp, so external users like the test code can’t link
against it. Make the template visible outside this translation unit by either
moving the definition of GetValidatedPayload into the corresponding header or
adding explicit instantiations for the ProTx types used by callers: CProRegTx,
CProUpServTx, CProUpRegTx, and CProUpRevTx. Keep the existing validation logic
in GetValidatedPayload and ensure the chosen approach preserves the current
behavior while making the symbol available to external callers.
---
Nitpick comments:
In `@src/rpc/masternode.cpp`:
- Around line 225-266: `GetRequiredPaymentsString` duplicates the owner-payout
comma-separated destination building already implemented in
`GetOwnerPayoutsString`. Refactor `GetRequiredPaymentsString` to reuse
`GetOwnerPayoutsString(*payee->pdmnState)` for the owner payout portion, then
append the operator-reward destination separately if needed. Since
`GetOwnerPayoutsString` is defined later in `masternode.cpp`, add a forward
declaration or move the helper so `GetRequiredPaymentsString` can call it
cleanly.
🪄 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: 5188d641-e727-4ee4-93ca-c0efd12f24db
📒 Files selected for processing (25)
src/common/bloom.cppsrc/evo/core_write.cppsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/dmnstate.cppsrc/evo/dmnstate.hsrc/evo/netinfo.cppsrc/evo/providertx.cppsrc/evo/providertx.hsrc/evo/providertx_util.cppsrc/evo/simplifiedmns.cppsrc/evo/simplifiedmns.hsrc/evo/smldiff.hsrc/evo/specialtx_filter.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/types.hsrc/llmq/commitment.cppsrc/masternode/payments.cppsrc/node/interfaces.cppsrc/rpc/evo.cppsrc/rpc/masternode.cppsrc/test/data/trivially_invalid.jsonsrc/test/evo_deterministicmns_tests.cppsrc/test/evo_trivialvalidation.cpp
💤 Files with no reviewable changes (4)
- src/evo/smldiff.h
- src/test/evo_trivialvalidation.cpp
- src/evo/deterministicmns.h
- src/test/evo_deterministicmns_tests.cpp
✅ Files skipped from review due to trivial changes (4)
- src/llmq/commitment.cpp
- src/evo/netinfo.cpp
- src/evo/simplifiedmns.h
- src/test/data/trivially_invalid.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/evo/types.h
- src/evo/specialtxman.h
🛑 Comments failed to post (1)
src/rpc/evo.cpp (1)
31-31: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate payout-share totals before returning prepared transactions.
Line 317 permits rewards up to
uint16_t::maxand the parser never checks that array rewards sum toMasternodePayoutShare::MAX_REWARD. Theregister_preparebranch returns afterSetTxPayload()withoutSignAndSendSpecialTx()/CheckSpecialTx(), so malformed payout arrays can be emitted as invalid unsigned ProTx payloads.Proposed fix
`#include` <limits> +#include <set> static MasternodePayoutShares ParsePayouts(const UniValue& value, const std::string& paramName, CTxDestination& first_dest) { MasternodePayoutShares payouts; if (value.isArray()) { + uint32_t total_reward{0}; + std::set<CScript> seen_scripts; const auto& arr = value.get_array(); if (arr.empty()) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must contain at least one entry", paramName)); } @@ if (payouts.empty()) { first_dest = dest; } const int64_t reward = reward_value.getInt<int64_t>(); - if (reward < 0 || reward > std::numeric_limits<uint16_t>::max()) { + if (reward < MasternodePayoutShare::MIN_REWARD || reward > MasternodePayoutShare::MAX_REWARD) { throw JSONRPCError(RPC_INVALID_PARAMETER, "payout reward out of range"); } - payouts.emplace_back(GetScriptForDestination(dest), static_cast<uint16_t>(reward)); + CScript payout_script = GetScriptForDestination(dest); + if (!seen_scripts.emplace(payout_script).second) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "duplicate payout address"); + } + total_reward += static_cast<uint32_t>(reward); + payouts.emplace_back(payout_script, static_cast<uint16_t>(reward)); + } + if (total_reward != MasternodePayoutShare::MAX_REWARD) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "payout rewards must sum to 10000"); } } else { first_dest = DecodeDestination(value.get_str()); if (!IsValidDestination(first_dest)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("invalid payout address: %s", value.get_str()));Also applies to: 317-328
🤖 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/evo.cpp` at line 31, The `register_prepare` path in `src/rpc/evo.cpp` returns a prepared ProTx after `SetTxPayload()` without validating the payout-share array, so malformed rewards can slip through. Add a check in the payout-share parsing/validation logic to ensure the array totals exactly match `MasternodePayoutShare::MAX_REWARD` before building the transaction, and reject values outside the allowed `uint16_t` range or any sum mismatch before returning from `register_prepare`.
There was a problem hiding this comment.
Code Review
The PR cleanly unifies ProTx versioning by folding v4 into v3 and centralizing max-version derivation. One remaining consensus-affecting bug: after V24, applying a v3 ProUpRevTx to a v2 (BasicBLS) masternode state silently downgrades the stored state to v1 (LegacyBLS) because ResetOperatorFields resets nVersion and the restore branch only fires for old_version>=ExtAddr. Two commit-history hygiene items should also be cleaned before merge.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
Findings not posted inline (3)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
<commit:d5ec8d1de3>:1: Drop the feat/Revert pair (2900455 + d5ec8d1) from the stack — Commit 2900455 (feat: output legacy 'payout' only if deprecated mode is activated) is immediately reverted by d5ec8d1. Their combined net effect on the tree is zero, so keeping them permanently indevelopadds two dead-end stops togit log/git bisectand forcesgit blamereaders to jump through an intentionally-undone change. Since Dash merges without squashing, please drop both commits via interactive rebase before merge. - [SUGGESTION]
<commit:93d764816b>:1: Squash or reword the review-feedback commit 93d7648 — Commit 93d7648 (test: address review comments about preserving v2 / v3 version for dmnstate) is a classic review-response fixup: the subject describes the review process rather than the durable behavior being added, and the diff logically belongs with dddec3c (fix!: use the same version along all protx special transactions) which introduced the dmnstate versioning behavior it patches. Since this stack lands unsquashed, eithergit rebase -i --autosquashthis into its parent, or rew... - [NITPICK]
<commit:75ca23bee8>:1: Commit subject 75ca23b still references pre-rename class name — 75ca23b (refactor: let compiler to auto-generate operator== and != for CMasternodePayoutShare) is applied after abffb92 renamed the type toMasternodePayoutShare, so the subject references a class name that no longer exists at that point in history. Consider updating toMasternodePayoutShare, or squashing 75ca23b into the rename/ctor-removal commits since all three are trivial tidying of the same type.
🤖 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:518: v3 ProUpRevTx silently downgrades a v2 masternode state to LegacyBLS after V24
Post-V24, `IsVersionChangeValid()` (specialtxman.cpp:1006-1013) accepts a version-3 `ProUpRevTx` against a v2 (BasicBLS) state — it only rejects legacy-scheme jumps above v2 and legacy downgrades. The `protx revoke` RPC (rpc/evo.cpp:1368-1374) already emits v3 for any non-legacy state, so this path is directly reachable from the wallet. When applied here, `ResetOperatorFields()` (dmnstate.h:120) unconditionally sets `newState->nVersion = ProTxVersion::LegacyBLS`, and this block only restores the previous version when `old_version >= ProTxVersion::ExtAddr`. Therefore a still-v2 masternode revoked by an accepted v3 `ProUpRevTx` ends up stored as version 1, contradicting the PR's post-V24 invariant that DMN state versions should be preserved/upgraded (as ProUpRegTx at line 476 already does via `std::max(old_version, opt_proTx->nVersion)`). The added functional test at feature_protx_version.py:151-159 does not catch this because it performs a v3 `update_registrar` first, so revoke is applied to an already-v3 state. Use the same max-based target-version model as the registrar path, and only reset to legacy when V24 is not deployed.
In `<commit:d5ec8d1de3>`:
- [SUGGESTION] <commit:d5ec8d1de3>:1: Drop the feat/Revert pair (2900455ed6 + d5ec8d1de3) from the stack
Commit 2900455ed6 (`feat: output legacy 'payout' only if deprecated mode is activated`) is immediately reverted by d5ec8d1de3. Their combined net effect on the tree is zero, so keeping them permanently in `develop` adds two dead-end stops to `git log`/`git bisect` and forces `git blame` readers to jump through an intentionally-undone change. Since Dash merges without squashing, please drop both commits via interactive rebase before merge.
In `<commit:93d764816b>`:
- [SUGGESTION] <commit:93d764816b>:1: Squash or reword the review-feedback commit 93d764816b
Commit 93d764816b (`test: address review comments about preserving v2 / v3 version for dmnstate`) is a classic review-response fixup: the subject describes the review process rather than the durable behavior being added, and the diff logically belongs with dddec3ccd8 (`fix!: use the same version along all protx special transactions`) which introduced the dmnstate versioning behavior it patches. Since this stack lands unsquashed, either `git rebase -i --autosquash` this into its parent, or reword it to describe the logical test change (e.g., `test: cover v24 ProTx dmnstate version preservation`).
| const uint16_t old_version{static_cast<uint16_t>(newState->nVersion)}; | ||
| newState->ResetOperatorFields(); | ||
| if (old_version >= ProTxVersion::MultiPayout && !SetStateVersion(*newState, old_version, dmn->nType, state)) { | ||
| if (old_version >= ProTxVersion::ExtAddr && !SetStateVersion(*newState, old_version, dmn->nType, state)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: v3 ProUpRevTx silently downgrades a v2 masternode state to LegacyBLS after V24
Post-V24, IsVersionChangeValid() (specialtxman.cpp:1006-1013) accepts a version-3 ProUpRevTx against a v2 (BasicBLS) state — it only rejects legacy-scheme jumps above v2 and legacy downgrades. The protx revoke RPC (rpc/evo.cpp:1368-1374) already emits v3 for any non-legacy state, so this path is directly reachable from the wallet. When applied here, ResetOperatorFields() (dmnstate.h:120) unconditionally sets newState->nVersion = ProTxVersion::LegacyBLS, and this block only restores the previous version when old_version >= ProTxVersion::ExtAddr. Therefore a still-v2 masternode revoked by an accepted v3 ProUpRevTx ends up stored as version 1, contradicting the PR's post-V24 invariant that DMN state versions should be preserved/upgraded (as ProUpRegTx at line 476 already does via std::max(old_version, opt_proTx->nVersion)). The added functional test at feature_protx_version.py:151-159 does not catch this because it performs a v3 update_registrar first, so revoke is applied to an already-v3 state. Use the same max-based target-version model as the registrar path, and only reset to legacy when V24 is not deployed.
| const uint16_t old_version{static_cast<uint16_t>(newState->nVersion)}; | |
| newState->ResetOperatorFields(); | |
| if (old_version >= ProTxVersion::MultiPayout && !SetStateVersion(*newState, old_version, dmn->nType, state)) { | |
| if (old_version >= ProTxVersion::ExtAddr && !SetStateVersion(*newState, old_version, dmn->nType, state)) { | |
| return false; | |
| } | |
| const uint16_t old_version{static_cast<uint16_t>(newState->nVersion)}; | |
| const uint16_t target_version{is_v24_deployed ? std::max<uint16_t>(old_version, opt_proTx->nVersion) | |
| : ProTxVersion::LegacyBLS}; | |
| newState->ResetOperatorFields(); | |
| if (target_version >= ProTxVersion::BasicBLS && !SetStateVersion(*newState, target_version, dmn->nType, state)) { | |
| return false; | |
| } |
source: ['codex']
3 participants
Issue being fixed or feature implemented
Until now, CProRegTx and CProUpServTx were the only ProTx subtypes that could carry a higher version (v3, ext-addresses). With v4 introduced with masternode payout this gap is even more strange.
What was done?
For better forward-compatibility, uniform documentation, and less user confusion, after v24 CProUpRegTx and CProUpRevTx are also allowed to use the "ext-addresses" version, even though they don't carry any network-related fields at the moment.
Functionality of "multi-payout" is merged to v3 protx; v4 protx are removed.
They both activated by v24 fork and that's possible that multiple-payout masternode is using external address; it's completely find situation and existing v4 separation is artificial.
From now on:
NOTE: CSimplifiedMNListEntry and CDeterministicMNState use the same enum for its version; moreover CDeterministicMNState inherits version directly from CProRegTx. This refactoring is possible due already existing versioning of state's object.
It also simplifies the implementation, drops the dependency of evo/providertx.h on validation.h and reduces the number of circular dependencies over evo/providertx.
The regression test now goes through the same GetValidatedPayload helper that consensus uses, instead of calling GetTxPayload + IsTriviallyValid directly.
How Has This Been Tested?
Run unit / functional tests.
Breaking Changes
After v24, CProUpRegTx and CProUpRevTx may now be serialized at version 3, which they were previously not eligible for. The bad-protx-version-tx-type consensus check is removed accordingly so these txes are accepted at version 3.
v4 protx is removed by merging functionality to v3. They are activated by the same fork and this diversion is not required.
Checklist: