fix!: use the same version along all protx special transactions - #7302
Conversation
|
⛔ Blockers found — Sonnet deferred (commit b6260a5) |
✅ 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 👍 / 👎.
|
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 restructures ProTx version handling by introducing a shared Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant RPC as RPC (protx_register/update)
participant Validation as validation.cpp
participant SpecialTxMan as specialtxman.cpp
participant ProviderTx as CProRegTx/CProUpRegTx/CProUpServTx/CProUpRevTx
RPC->>Validation: DeploymentToProtxVersion(pindexPrev, chainman, override)
Validation-->>RPC: nVersion
RPC->>ProviderTx: construct payload with nVersion
RPC->>SpecialTxMan: submit transaction
SpecialTxMan->>SpecialTxMan: GetValidatedPayload<ProTx>(tx, pindexPrev, chainman, state)
SpecialTxMan->>Validation: DeploymentToProtxVersion(pindexPrev, chainman)
Validation-->>SpecialTxMan: max allowed version
SpecialTxMan->>ProviderTx: IsTriviallyValid(state)
ProviderTx-->>SpecialTxMan: valid/invalid + reject reason
SpecialTxMan-->>RPC: accepted payload or bad-protx-version error
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 |
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
📥 Commits
Reviewing files that changed from the base of the PR and between ed4d9c63ea85c22204447f2dfd0a7d8ec9fd2af3 and f94f7ccb3b3afe2f744e5af0c75d6a831f7fd7ff.
📒 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: f94f7ccb
🔴 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 d86d5716a126b19adec84daf6bc600b502ae045a
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).
| @@ -907,11 +905,6 @@ static bool IsVersionChangeValid(gsl::not_null<const CBlockIndex*> pindexPrev, c | |||
| 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
| @@ -93,6 +93,15 @@ class CSpecialTxProcessor | |||
| }; | |||
|
|
|||
|
|
|||
| /** | |||
| * 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. |
f94f7cc to
a016390
Compare
|
This pull request has conflicts, please rebase. |
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 👍 / 👎.
|
@thepastaclaw ignore devnet deployments, they are temporary |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes the stale unit-test name, but the resulting one-line fixup commit should be autosquashed into the commit that changed the expected behavior. Across the cumulative PR, two carried-forward correctness blockers remain: version 3 is assigned an incompatible serialization layout despite already being released, and v1-to-v3 state promotion leaves the operator key and uniqueness index in the legacy encoding. Three carried-forward commit-history findings also remain open.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
5 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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.h`:
- [BLOCKING] src/evo/providertx.h:105-114: Do not reinterpret existing version-3 ProRegTx payloads
Dash v23.1.3 already defines `ExtAddr = 3` and serializes one `scriptPayout` for version-3 `CProRegTx` and `CProUpRegTx` payloads; its `CDeterministicMNState` uses the same single-script representation. The released devnet parameters also allow V24 activation, so version-3 registrations and persisted states can already exist. This branch instead treats version 3 as the former version-4 payout-vector format: the serialized script length becomes `payouts_count`, and subsequent script or transaction bytes are consumed as payout records. The equivalent changes at `src/evo/providertx.h:245-254` and `src/evo/dmnstate.h:104-107` affect registrar decoding and deterministic-list snapshots. Preserve the released version-3 representation, or retain a distinct payload version and provide an explicit persistent-state migration.
In `<commit:8b9c2e80532>`:
- [SUGGESTION] <commit:8b9c2e80532>:1: Fold the payout migration fix and regression test into the version merge
Commit `8b9c2e80532` directly repairs behavior introduced by `c01a1ed19a7`: without migrating `scriptPayout` before `SetStateVersion()` can return, promotion to version 3 can lose the owner payout. Commit `9a86b0871a2` later adds the matching regression coverage. Fold the production fix and its test into `c01a1ed19a7` so every retained commit has valid state-migration behavior.
In `<commit:1c41191d363>`:
- [SUGGESTION] <commit:1c41191d363>:1: Squash or reword the review-feedback test commit
Commit `1c41191d363` is still titled `test: address review comments about preserving v2 / v3 version for dmnstate`. That subject records transient review conversation rather than the durable V24 state-version behavior covered by the functional test. Fold it into the relevant versioning change or reword it to describe V24 ProTx state-version preservation.
In `<commit:f43dbc09f1b>`:
- [SUGGESTION] <commit:f43dbc09f1b>:1: Autosquash the one-line regression-test fixup
The latest commit, `f43dbc09f1b` (`fix: regression test case fixup`), correctly renames `proupreg_v3_on_legacy_rejected` to `proupreg_v3_on_legacy_valid`. The stale name was left by `8cb7d3a7de2`, which changed the helper and assertion from rejected to valid behavior. Fold this one-line fixup into `8cb7d3a7de2` so the retained history does not contain a generic fixup commit or an intermediate test whose name contradicts its assertion.
…rdingly code style
…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
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
f43dbc0 to
b6260a5
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The force-rebased stack is logically unchanged: all six carried-forward findings remain valid, comprising two correctness blockers and four commit-history cleanup items. No genuinely new latest-delta finding was identified; the current equivalents still reinterpret released version-3 data and leave v1-to-v3 operator-key scheme and uniqueness-index migration incomplete. Changes are required before merge.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
5 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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.h`:
- [BLOCKING] src/evo/providertx.h:105-114: Do not reinterpret existing version-3 ProRegTx payloads
Dash v23.1.3 already defines `ExtAddr = 3` and serializes one `scriptPayout` for version-3 `CProRegTx` and `CProUpRegTx` payloads; `CDeterministicMNState` uses the same single-script representation. That release also permits devnet V24 signaling from July 1, 2025, so version-3 transactions and persisted states can already exist. This branch instead selects the former version-4 payout-vector format for version 3: the serialized script length is interpreted as `payouts_count`, and subsequent bytes are consumed as payout records. The equivalent thresholds at `src/evo/providertx.h:245-254` and `src/evo/dmnstate.h:104-107` also change registrar decoding and persisted deterministic-list state. Preserve the released version-3 representation, or retain a distinct payload version and provide an explicit persistent-state migration.
In `<commit:bcd24cd6ddc>`:
- [SUGGESTION] <commit:bcd24cd6ddc>:1: Fold the payout migration fix and regression test into the version merge
Commit `bcd24cd6ddc` directly repairs behavior introduced by `dbf0f1f35de`: without moving `scriptPayout` before `SetStateVersion()` can return early, promotion to version 3 can lose the owner payout. Commit `de9461bf91d` later adds the matching regression coverage. Fold the production correction and its test into `dbf0f1f35de` so the retained version-merge commit has valid state-migration behavior.
In `<commit:c864cbe98df>`:
- [SUGGESTION] <commit:c864cbe98df>:1: Squash or reword the review-feedback test commit
Commit `c864cbe98df` remains titled `test: address review comments about preserving v2 / v3 version for dmnstate`, which records transient review conversation rather than the durable behavior covered by the functional test. Fold it into the relevant versioning change or reword it to describe V24 ProTx state-version preservation.
In `<commit:b6260a521ed>`:
- [SUGGESTION] <commit:b6260a521ed>:1: Autosquash the one-line regression-test fixup
The current tip is a one-line commit titled `fix: regression test case fixup`, renaming `proupreg_v3_on_legacy_rejected` to `proupreg_v3_on_legacy_valid`. The stale name was left by `cd6475774c5`, which changed the helper and assertion from rejected to valid behavior. Fold this fixup into `cd6475774c5` so permanent history does not retain a generic corrective commit or an intermediate test whose name contradicts its assertion.
… (desync + cross-scheme uniqueness) 4963125 test: deduplicate BLS migration regression fixtures (PastaClaw) b6722c2 doc: add release notes for #7473 (UdjinM6) d62a031 test: cover BLS scheme migration and cross-scheme operator-key uniqueness (UdjinM6) 71e7980 fix: allow legacy->basic operator key migration without key rotation (UdjinM6) f094101 fix: enforce operator key uniqueness across BLS schemes (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Two independent bugs in how a masternode's operator BLS key relates to its state version. Both exist on `develop` today and are reproduced by test; neither is introduced by this change. > **Rebased on #7302.** This branch now sits on top of the merged #7302 (uniform > protx versioning: v4 merged into v3, RPC version selection via > `DeploymentToProtxVersion`, and the v1→v3 update restriction removed). The fixes > below are unchanged in substance; the migration now lands a legacy masternode at > **v3** directly (rather than clamping to v2) because #7302 allows v1→v3, and the > operator-key re-encoding continues to make that safe. **(A) Post-v24 operator-key scheme desync → consensus split.** `CDeterministicMNState` derives its operator key's serialization from `nVersion` (`CBLSLazyPublicKeyVersionWrapper(key, nVersion == LegacyBLS)`). But after v24 a masternode's `nVersion` could rise out of LegacyBLS (v1) to a basic-scheme version (v2/v3) while the stored key kept the legacy scheme flag: - `ProUpServTx` carries no operator key, yet set `newState->nVersion` from the payload. - `ProUpRegTx` re-submitting the *same* key basic-encoded left `operator_changed` false, because `CBLSLazyPublicKey::operator==` compares the public key and ignores the scheme, so the re-encode was skipped while the version still rose. Once a state has a basic-scheme `nVersion` with a legacy-flagged key, a list rebuilt from block diffs and the same list reloaded from disk hash that key differently, so `mnUniquePropertyMap` diverges. Nodes then disagree on `HasUniqueProperty()` and therefore on `bad-protx-dup-key` — a chain split determined purely by restart history. `update_service` is the routine operation that plants this: any legacy masternode updating its service address after v24 would trigger it, no attacker required. **(B) Operator-key uniqueness enforced per encoding, not per key (live today).** `mnUniquePropertyMap` is keyed by `GetUniquePropertyHash()`, which serializes its argument, and a BLS key serializes differently under the two schemes, so `H(K, legacy) != H(K, basic)`. `CheckProRegTx`'s duplicate check consults that map, so it does not see an operator key an existing masternode holds under the *other* encoding. A `ProRegTx` never proves ownership of the operator key, so anyone can re-register an existing masternode's operator public key for the price of a collateral. This is exploitable on mainnet now; it is closed here for post-v24. ## What was done? The approach is **migrate, don't force rotation** (adopting the maintainer's preferred model from #7472), while keeping the cross-scheme uniqueness guards that make the re-encode safe. Every new consensus rule is gated on `DEPLOYMENT_V24`. A legacy masternode keeps the same operator private key across the migration; only the serialized encoding of the public key changes. On a version bump, `SetStateVersion()` re-encodes the stored key to the scheme its version implies (`Set(Get(), …)`, not `SetLegacy()`, so the cached serialization actually changes), and `UpdateUniqueProperty()` re-keys the scheme-dependent unique-property map when the encoding changes (it now compares `GetUniquePropertyHash()` rather than the scheme-blind `operator==`). Re-encoding is only safe if the target slot is free: `UpdateMN()` reports a duplicate by *throwing*, and that throw escapes `BlockAssembler::CreateNewBlock`, stalling block production. Because bug B leaves the per-encoding registration hole open, a squatter can already hold the key under the other encoding — so the migration is guarded at every layer, and only when the key actually changes or the version crosses the legacy→basic boundary (a grandfathered cross-scheme pair's non-migrating routine update is not blocked). The unique-property map is deliberately **not** made canonical. It is derived, not serialized: `CDeterministicMNList::Unserialize` clears it and rebuilds via `AddMN`. A canonical hash would apply retroactively to all history and, if any cross-scheme duplicate pair already exists, make `AddMN` throw and nodes fail to sync past it. Cross-scheme pairs created before activation are therefore tolerated (nothing rehashes the map) and remain usable for non-migrating routine updates; a same-key legacy→basic migration by the still-legacy member is rejected `bad-protx-dup-key` until the conflicting key is rotated. The change is split into two source commits, a test commit, a release-notes commit, and a follow-up test-fixture dedup commit; each source commit builds on its own. **Commit 1 — enforce uniqueness across schemes.** A helper `HasOperatorKeyUnderAnyScheme` probes both encodings (two O(1) lookups, not a scan), wired in at every point a key can be claimed: - `CheckProRegTx` and `CheckProUpRegTx`, against the previous block's list. - `RebuildListFromBlock`, against the list as rebuilt so far (same-block pairs). - `AcceptToMemoryPool`. This probe is deliberately **not** v24-gated: block assembly does not revalidate special transactions cumulatively, so a pair admitted before activation is never evicted and would poison every template afterwards. Keeping the pair out of the mempool is what actually closes that, and mempool policy is allowed to be stricter than consensus (before v24 a node rejecting the second transaction would still accept a consensus-valid block containing the pair; after v24 consensus rejects the pair too, so the policy difference cannot split the chain in either regime). The registrar probes run only when the operator key is actually changing: an update that keeps its own key cannot create a duplicate, and probing it anyway would let a pre-activation cross-scheme pair permanently block the affected masternode's registrar updates. **Commit 2 — allow legacy→basic migration without key rotation.** Handles bug A by migrating in place rather than forcing a rotation (adopting #7472's re-encode model): - `SetStateVersion()` re-encodes the stored operator key to the scheme its version implies (`Set(Get(), …)`, not `SetLegacy()`, so the cached serialization actually changes), keeping the invariant that a stored key's encoding always matches its `nVersion`; `UpdateUniqueProperty()` re-keys the scheme-dependent map on an encoding change (comparing `GetUniquePropertyHash()` rather than the scheme-blind `operator==`). - The migration is guarded against collisions via `HasOperatorKeyUnderAnyScheme` in `CheckProUpServTx`, `CheckProUpRegTx` and `RebuildListFromBlock` — the last against the list as rebuilt so far, since per-transaction checks run against `pindexPrev` and are blind to an earlier transaction in the same block — so a re-encode that would collide with another masternode's key cannot throw out of block assembly. The guard fires only when the operator key changes or the version crosses the legacy→basic boundary (a shared `IsSchemeMigration` predicate), so a grandfathered cross-scheme pair's non-migrating update is not blocked. - `CDeterministicMNStateDiff` now compares the scheme-dependent hash for the operator key field. Its comparison used `operator==` (scheme-blind), so a same-key migration produced a diff that omitted the key — a node reconstructing the list from evoDB diffs kept the old encoding while an online-built list had the new one, a reconstruction split that full-snapshot serialization does not reveal. - RPC (`rpc/evo.cpp`): `protx update_service` / `protx update_registrar` migrate a legacy masternode to the basic scheme (keeping the same key, or supplying a new one), instead of erroring or forcing a rotation. Version selection flows through #7302's `DeploymentToProtxVersion`, so post-v24 the migration payload is **v3** (ExtAddr) — the legacy→BasicBLS clamp #7302 removed is not reintroduced; the stored/reused operator key is re-encoded to the basic scheme so it matches the target version. The legacy-BLS RPC variants keep the masternode on the legacy scheme. **Commit 3 — tests.** Unit tests in `evo_deterministicmns_tests.cpp` and the functional-test extension in `feature_protx_version.py` (renamed from `feature_dip3_v19.py` by #7302) covering both fixes (enumerated below). **Commit 4 — release notes.** `doc/release-notes-7473.md` documenting the post-v24 per-key uniqueness enforcement and the in-place migration behavior of `protx update_service` / `protx update_registrar`. **Commit 5 — test fixture dedup.** Deduplicates the v24-activation scaffolding in `evo_deterministicmns_tests.cpp` into a shared `TestMNChainSetup` fixture; no behavior change (test-only refactor, contributed by a reviewer). ## How Has This Been Tested? Built with `--enable-werror`; the full unit suite (`./src/test/test_dash`, 778 cases) and the touched functional test (`feature_protx_version.py`) both pass, and lint is clean for the changed files. Each source commit was rebuilt independently and compiles on its own under `--enable-werror`; the test commit adds the coverage below, and the full `evo_dip3_activation_tests` suite passes. Unit tests (`src/test/evo_deterministicmns_tests.cpp`) — each rejection test was watched fail first (by flipping the v24 activation height): - `proupserv_migrates_legacy`, `proupreg_migrates_legacy_same_key` — the happy-path migrations: a legacy masternode raises its version keeping the same operator key, the stored key re-encodes to basic, and the masternode is *not* PoSe-banned. - `migration_rejected_when_key_squatted` — a squatter already holds the key under the other encoding; the migration is rejected `bad-protx-dup-key` at the per-tx layer rather than throwing. - `same_mn_same_block_migration_consistent`, `same_mn_same_block_version_crossing_key_rotation` — same-masternode transitions within one block reconstruct identically. - `same_block_cross_scheme_key_pair_rejected`, `mempool_rejects_cross_scheme_key_race`, `pre_v24_cross_scheme_pair_cannot_become_resident` — the same-block, mempool, and activation-boundary pairings that motivated the rebuild-time and ungated mempool probes. - `proregtx_rejects_cross_scheme_key_reuse`, `proupreg_rejects_cross_scheme_key_reuse` — bug B at the registration and registrar paths, both directions, with fresh-key and self-key non-false-positive cases. - `statediff_captures_operator_key_reencoding` — the evoDB-diff reconstruction path: a same-key migration must emit the re-encoded key in the diff. - `has_operator_key_under_any_scheme` — the two-encoding lookup helper. - `stale_special_tx_does_not_poison_template` — a resident-but-invalid special tx seeded via `addUnchecked`; without the package recheck `CreateNewBlock` throws. - `pre_v24_behaviour_unchanged` — the non-retroactivity guard: asserts the new rules are inert before v24, so nothing on live mainnet/testnet changes. Functional test (`test/functional/feature_protx_version.py`): extended to assert, after v24 activation, that a legacy masternode's `update_service` migrates it to the basic scheme in place (v3 payload, `state.version == 3`, same operator key re-encoded to a different hex, `PoSeBanHeight == -1`), and that the list reloads from disk identically after the migration. The same-key migration runs alongside #7302's rotated-key `update_registrar` migration and is sequenced last (with a reconnect after the key re-encoding, which churns the migrated node's masternode connections) so it does not destabilise the surrounding checks. ## Breaking Changes All consensus rules here are gated on `DEPLOYMENT_V24`, which is `NEVER_ACTIVE` on mainnet and testnet, so there is no consensus change on any live network; `pre_v24_behaviour_unchanged` asserts this. After v24 activates: - Absent a cross-scheme key collision, a LegacyBLS masternode migrates to the basic scheme in place on its next version bump (`update_service` or `update_registrar`), keeping the same operator key; the stored public key is re-encoded legacy→basic. No forced key rotation, no PoSe ban. - Registering or updating to an operator public key already held by another masternode is rejected regardless of which BLS encoding either side uses, and a migration that would collide with such a key is rejected `bad-protx-dup-key` rather than stalling block assembly. The `AcceptToMemoryPool` cross-scheme check is mempool policy (ungated) rather than consensus, so a node may reject a second in-flight transaction that another node's mempool accepted; both would still accept a consensus-valid block containing it before v24, and after v24 consensus rejects the pair, so the policy difference cannot split the chain. Not addressed here, and worth stating: cross-scheme duplicate pairs created *before* activation are tolerated, not resolved; and bug B is only closed post-v24, so the registration hole remains open on mainnet until v24 activates. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation (release notes) - [x] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: 257bb3de1c69d3291b833f325dc6dca59bfc4da87539c55509e3d1bc7d227072c018bca49649e2028c73748e9371da74bcf32b370ce3511a05f9c3cc518c129d
5 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: