fix(rpc): use deployment state for BLS scheme in protx revoke/update_service, improve feature_dip3_v19.py
#7096
Conversation
…service Commit dc687a9 introduced is_basic_override to set ptx.nVersion based on masternode state rather than deployment status. This caused a scheme mismatch for legacy masternodes after V19 activation: - ptx.nVersion was set to LegacyBLS (based on MN state) - Signing used legacy scheme (based on ptx.nVersion) - Verification used basic scheme (global bls_legacy_scheme) Fix by removing the is_basic_override parameter, letting GetMaxFromDeployment use V19 deployment state for version determination. Also align update_service signing to use ptx.nVersion consistently. This ensures version, signing, and verification all use the same deployment-based scheme. NOTE: `protx update_service` now always uses the deployment max version (`ExtAddr` after v24) without the state override, which means legacy MNs will hit `bad-protx-version-upgrade` and be blocked from `update_service` until they run `update_registrar`.
Add a masternode registered before V19 activation and test that it can be successfully revoked after V19 activates. This validates the fix for the BLS scheme mismatch in protx_revoke.
Add test_update_service_protx to verify that update_service works correctly for masternodes registered before V19 activation. This validates the fix applies to both revoke and update_service RPCs.
|
WalkthroughCalls to ProTxVersion::GetMaxFromDeployment for CProUpServTx and CProUpRevTx are simplified by removing the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
How can we make it more clear to users as to what is needed? |
After V24 activation, GetMaxFromDeployment returns ExtAddr (version 3) for CProUpServTx. However, CheckVersionConsistency() in deterministicmns.cpp enforces that legacy masternodes (LegacyBLS state) cannot skip directly to ExtAddr - they must first upgrade to BasicBLS via update_registrar. Without this fix, legacy masternodes calling protx update_service or protx revoke after V24 activation would receive "bad-protx-version-upgrade" validation errors. Clamp the transaction version to BasicBLS when the masternode state is still using LegacyBLS, allowing these RPCs to work while respecting the version upgrade constraints.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/rpc/evo.cpp (2)
1268-1275: Good symmetry with update_service; consider deduplicating the clamp logic.Same clamp pattern appears in multiple RPCs; a tiny helper (e.g.,
ClampProTxVersionForLegacyMN(dmn_state_version, ptx_version)) would reduce drift risk if rules change again.
1012-1020: Consider documenting the legacy masternode version clamping behavior in help text.The
update_serviceandupdate_service_evohelp text doesn't mention that legacy masternodes are silently clamped toBasicBLSversion to prevent invalid version jumps. Users may expect the latest deployed version to be used and could be confused when their legacy masternode doesn't receive it. A brief note in the help text explaining that legacy masternodes must upgrade sequentially (and why) would clarify the behavior.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/rpc/evo.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/rpc/evo.cpp
🧠 Learnings (7)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 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
Repo: dashpay/dash PR: 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
Repo: dashpay/dash PR: 6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
Applied to files:
src/rpc/evo.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/rpc/evo.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/rpc/evo.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/rpc/evo.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/rpc/evo.cpp
📚 Learning: 2025-06-16T17:59:55.669Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
Applied to files:
src/rpc/evo.cpp
🧬 Code graph analysis (1)
src/rpc/evo.cpp (1)
src/evo/providertx.cpp (5)
GetMaxFromDeployment(20-21)GetMaxFromDeployment(29-31)GetMaxFromDeployment(32-34)GetMaxFromDeployment(35-37)GetMaxFromDeployment(38-40)
⏰ 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). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (1)
src/rpc/evo.cpp (1)
1087-1087: Good: signing scheme now derived fromptx.nVersion, matching the payload version.
This should prevent “tx says legacy, verify expects basic” style mismatches.
Shouldn't be an issue anymore, see 5a7a991 |
3 participants
Issue being fixed or feature implemented
Commit UdjinM6@dc687a9 introduced
is_basic_overridetoset ptx.nVersionbased on masternode state rather than deployment status. This caused a scheme mismatch for legacy masternodes after V19 activation:ptx.nVersionwas set to LegacyBLS (based on MN state)ptx.nVersion)bls_legacy_scheme)Alternative to #7094
What was done?
Fix by removing the
is_basic_overrideparameter, lettingGetMaxFromDeploymentuse V19 deployment state for version determination. Also alignupdate_servicesigning to useptx.nVersionconsistently and add functional tests for both.Clamp the transaction version to BasicBLS when the masternode state is still using LegacyBLS, allowing these RPCs to work while respecting the version upgrade constraints.
How Has This Been Tested?
Run
feature_dip3_v19.pyBreaking Changes
protx update_servicenow always uses the deployment max version (ExtAddrafter v24) without the state override, which means legacy MNs will hitbad-protx-version-upgradeand be blocked fromupdate_serviceuntil they runupdate_registrar.Checklist: