chore: deprecate protx register{,_fund,_prepare}_legacy and protx update_registrar_legacy, specifying scheme in bls generate
#6723
Conversation
da8893c to
bf89a94
Compare
32362b5 to
a466add
Compare
protx register{,_fund,_prepare}_legacy and protx update_registrar_legacy, specifying scheme in bls generate, add restrictions on ProTx version changesprotx register{,_fund,_prepare}_legacy and protx update_registrar_legacy, specifying scheme in bls generate
WalkthroughThe changes introduce deprecation notices and enforcement for several legacy masternode-related RPC commands in Dash Core. Documentation is updated to note the deprecation of the legacy variants of ✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/rpc/evo.cpp (1)
370-386: Clang-format violations block CI.
clang-diffreports style errors in this range.
Runclang-format -i src/rpc/evo.cpp(project style) and re-commit to unblock the pipeline.
🧹 Nitpick comments (4)
src/test/rpc_tests.cpp (1)
44-46: Minor: favourconstexpr std::arrayover braced-list to avoid small heap.Micro-nit – the
{ "-deprecatedrpc=legacy_mn" }literal is copied into astd::vectorinsideTestingSetup.
Replacing it with astatic constexpr std::array<const char*,1>would avoid a tiny allocation at start-up.
Purely optional.doc/release-notes-6723.md (1)
4-9: Fix typo & punctuation in release notes.
- Missing leading dash in the runtime flag on L9.
- Sentence on L5–6 is missing a full stop before “They”.
Apply:- They can be re-enabled with the runtime argument `-deprecatedrpc=legacy_mn`. + They can be re-enabled with the runtime argument `-deprecatedrpc=legacy_mn`. @@ - It can be re-enabled with the runtime argument `deprecatedrpc=legacy_mn`. + It can be re-enabled with the runtime argument `-deprecatedrpc=legacy_mn`.src/rpc/evo.cpp (2)
404-410: Duplicated deprecation check string – consider central helper.The literal
"DEPRECATED: Pass config option -deprecatedrpc=legacy_mn to enable this RPC"
is hard-coded in every wrapper. Extracting into a smallstatic constexpr char kLegacyDeprecationMsg[]would avoid future drift.
1716-1733: Error message references internal var name.
ParseBoolV(request.params[0], "bls_legacy_scheme");
If users supply an invalid value they’ll see “Error parsing parameter bls_legacy_scheme …” even though the public arg is namedlegacy.
Pass"legacy"instead to keep UX consistent.- bls_legacy_scheme = ParseBoolV(request.params[0], "bls_legacy_scheme"); + bls_legacy_scheme = ParseBoolV(request.params[0], "legacy");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
doc/release-notes-6723.md(1 hunks)src/rpc/evo.cpp(13 hunks)src/test/rpc_tests.cpp(1 hunks)test/functional/feature_dip3_v19.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[error] 373-480: Clang format differences found. The file does not conform to the required code style. Please run clang-format to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: linux64_multiprocess-test / Test source
- GitHub Check: linux64_tsan-test / Test source
🔇 Additional comments (2)
test/functional/feature_dip3_v19.py (1)
49-52: Flag consistently enables legacy RPCs in CI – looks good.The added
-deprecatedrpc=legacy_mnswitch mirrors the new gating logic and keeps the functional test alive. No further action needed.src/test/rpc_tests.cpp (1)
44-46: Constructor overload may shadow existing one – double-check ODR & init-order.If another non-default
RPCTestingSetupctor already exists, this new no-arg ctor is fine.
But if a different no-arg ctor is also present in another translation unit, you’ll get an ODR violation at link time.
Please run a quick search to make sure this is now the single default ctor.
| They can be re-enabled with the runtime argument `-deprecatedrpc=legacy_mn`. | ||
|
|
||
| * The argument `legacy` in `bls generate` has been deprecated in Dash Core v23 and may be ignored in a future version. | ||
| It can be re-enabled with the runtime argument `deprecatedrpc=legacy_mn`. |
There was a problem hiding this comment.
nit: could maybe split into 2 more explicit/self-explanatory ones
| They can be re-enabled with the runtime argument `-deprecatedrpc=legacy_mn`. | |
| * The argument `legacy` in `bls generate` has been deprecated in Dash Core v23 and may be ignored in a future version. | |
| It can be re-enabled with the runtime argument `deprecatedrpc=legacy_mn`. | |
| They can be re-enabled with the runtime argument `-deprecatedrpc=protx_legacy`. | |
| * The argument `legacy` in `bls generate` has been deprecated in Dash Core v23 and may be ignored in a future version. | |
| It can be re-enabled with the runtime argument `deprecatedrpc=bls_generate_legacy`. |
…roTx version changes with `DEPLOYMENT_V23` af10784 doc: add release notes for version restrictions (Kittywhiskers Van Gogh) 991f14d evo: prohibit new legacy scheme masternode registrations (Kittywhiskers Van Gogh) abf96b5 evo: prohibit extended address versioning on unsupported transactions (Kittywhiskers Van Gogh) de7f928 evo: prohibit upgrading from LegacyBLS to ExtAddr support directly (Kittywhiskers Van Gogh) e430e6e evo: prohibit any ProTx with legacy BLS version after basic BLS upgrade (Kittywhiskers Van Gogh) dc687a9 rpc: constrain ProTx version for ProUpRevTx based on legacy status (Kittywhiskers Van Gogh) 23c5152 trivial: use consistent variable name, use brackets (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6665 * Depends on #6723 * Please refer to comments from [dash#6665](#6665) for prior discussion on the contents of the pull request ([comment](#6665 (comment)), [comment](#6665 (comment)), [comment](#6665 (comment))) * Complementing the deprecation in [dash#6723](#6723), after `DEPLOYMENT_V23` is activated * Registration 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](#6665), which reserves a new version for extended addresses (`ExtAddr`), affecting `ProRegTx` and `ProUpServTx`, applicable after `DEPLOYMENT_V23` is activated * Masternodes **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 `ProRegTx` or `ProUpServTx` may not bear the version reserved for extended addresses (`ExtAddr`). Note that `IsVersionChangeValid()` does not extend to `ProRegTx` as 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 in `IsVersionChangeValid()` only _de facto_ applies to `ProUpServTx`. * Future version updates must be conscious of updates to the masternode state ([source](https://github.com/dashpay/dash/blob/d9f52acecb94d57be91b5e17d478d5c909df3633/src/evo/deterministicmns.cpp#L887-L888)), example code for what changes may be required are available [here](#6665 (comment)). ## Breaking Changes * `protx revoke` will no longer default to using the highest possible version of `ProUpRevTx` and will now _clamp_ the version to `LegacyBLS` if the masternode uses the legacy scheme. ## 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 **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK af10784 UdjinM6: utACK af10784 Tree-SHA512: fe788c33397f9e351377777946d5c0498569f68f22d308cce03f903fb3e149bd594ce2caafc50fd62611029563e2841bc42151579b21ba42fdf87cbf7762f716
Breaking Changes
The RPCs
protx register_legacy,protx register_fund_legacy,protx register_prepare_legacyandprotx update_registrar_legacyhave been deprecated in Dash Core v23 and may be removed in a future versionThey can be re-enabled with the runtime argument
-deprecatedrpc=legacy_mn.The argument
legacyinbls generatehas been deprecated in Dash Core v23 and may be ignored in a future version.It can be re-enabled with the runtime argument
deprecatedrpc=legacy_mn.Checklist: