feat: prohibit new legacy scheme masternodes, restrict ProTx version changes with DEPLOYMENT_V23
#6729
Conversation
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
WalkthroughThis change introduces new version enforcement rules for masternode transactions following the v23 fork activation in Dash Core. It adds a function to validate permissible version upgrades and downgrades for masternode special transactions, ensuring that legacy scheme registrations are blocked after v23 and preventing downgrades from the basic to legacy scheme. The 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/evo/deterministicmns.cpp (8)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
doc/release-notes-6729.md (1)
11-12: Fix indentation consistency.The bullet point has an extra space compared to the previous bullet points.
-* `protx revoke` will now use the legacy scheme version for legacy masternodes instead of the defaulting to the - highest `ProUpRevTx` version. +* `protx revoke` will now use the legacy scheme version for legacy masternodes instead of the defaulting to the + highest `ProUpRevTx` version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
doc/release-notes-6729.md(1 hunks)src/evo/deterministicmns.cpp(7 hunks)src/rpc/evo.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
src/rpc/evo.cpp (5)
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
doc/release-notes-6729.md (2)
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
src/evo/deterministicmns.cpp (5)
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[error] 1277-1278: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix the formatting issues.
🪛 LanguageTool
doc/release-notes-6729.md
[grammar] ~1-~1: Use correct spacing
Context: Notable Changes --------------- * Dash Core will no longer permit the reg...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~4-~4: Use correct spacing
Context: ...masternodes after the deployment of the v23 fork. Existing basic scheme masternodes will ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~5-~5: Use correct spacing
Context: ... downgrading to the legacy scheme after the deployment is active. Updated RPCs --------------...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~6-~6: Use correct spacing
Context: ...legacy scheme after the deployment is active. Updated RPCs ---------------- * `protx...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~8-~8: Use correct spacing
Context: ...er the deployment is active. Updated RPCs ---------------- * protx revoke will now use the legacy scheme version...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
🔇 Additional comments (5)
src/evo/deterministicmns.cpp (4)
1329-1335: LGTM! Legacy scheme registration prevention implemented correctly.The check properly disallows new masternode registrations using the legacy scheme after v23 deployment, as specified in the PR objectives.
1470-1478: Good variable naming improvement and version validation integration.The renaming from
mntodmnimproves code clarity, and the version validation is properly integrated.Also applies to: 1500-1500, 1514-1514
1542-1545: Version validation properly integrated.The version change validation is correctly added to the registrar update transaction checks.
1602-1609: Clamping logic for legacy BLS confirmed inprotx_revokeRPCThe handler sets
ptx.nVersionviaProTxVersion::GetMaxFromDeployment, passing
/*is_basic_override=*/dmn->pdmnState->nVersion > ProTxVersion::LegacyBLS,
and then callsSignSpecialTxPayloadByHash(tx, ptx, keyOperator, /*use_legacy=*/ptx.nVersion == ProTxVersion::LegacyBLS);which ensures the payload is signed with the LegacyBLS scheme when appropriate. No further changes are needed here.
src/rpc/evo.cpp (1)
1279-1281: LGTM! Version selection logic correctly respects masternode scheme.The change properly defers the version assignment until after retrieving the masternode, then uses the masternode's current version to determine the
is_basic_overrideparameter. This ensures that the revoke transaction uses the appropriate version based on the masternode's current scheme rather than defaulting to the highest available version.This aligns perfectly with the PR objectives to prevent version downgrades and respect the distinction between legacy and basic BLS schemes.
Even though only ProUpRegTx updates the operator key field, it is not good policy to allow transactions that don't use the version that defines the masternode's currently active key scheme or higher.
As ExtAddr > BasicBLS > LegacyBLS, an upgrade to ExtAddr *assumes* the node is already *at least* at BasicBLS. We should not allow upgrades if this assumption is not met.
This is a more explicit check that builds on the implicit maximum version check.
This does not affect existing nodes but the basic scheme was activated in v19, since then we've had three major releases and enough time has passed, we should now prohibit registration of such masternodes.
Additional Information
Depends on refactor: model interface after
MnNetInfoand support switching impls, add new ProTx version #6665Depends on chore: deprecate
protx register{,_fund,_prepare}_legacyandprotx update_registrar_legacy, specifying scheme inbls generate#6723Please refer to comments from dash#6665 for prior discussion on the contents of the pull request (comment, comment, comment)
Complementing the deprecation in dash#6723, after
DEPLOYMENT_V23is activatedRegistration of new masternodes (i.e.
ProRegTx) with the legacy scheme (LegacyBLS) will no longer be allowed. Existing masternodes are not affected and can continue to operate and participate.Masternodes that are already using the basic scheme (
BasicBLS) or higher may no longer downgrade to the legacy scheme.Additional guardrails have been introduced to complement dash#6665, which reserves a new version for extended addresses (
ExtAddr), affectingProRegTxandProUpServTx, applicable afterDEPLOYMENT_V23is activatedMasternodes must migrate to the basic scheme (
BasicBLS) before attempting to utilize extended addresses (ExtAddr), legacy scheme nodes may not attempt a direct upgrade.Special transactions other than
ProRegTxorProUpServTxmay not bear the version reserved for extended addresses (ExtAddr). Note thatIsVersionChangeValid()does not extend toProRegTxas it creates a new entry and therefore doesn't have a masternode state version to compare against (i.e. there's no version to change), so the restriction inIsVersionChangeValid()only de facto applies toProUpServTx.Future version updates must be conscious of updates to the masternode state (source), example code for what changes may be required are available here.
Breaking Changes
protx revokewill no longer default to using the highest possible version ofProUpRevTxand will now clamp the version toLegacyBLSif the masternode uses the legacy scheme.Checklist