fix: use deployment status for protx revoke/update_service BLS scheme #7094
Conversation
Fix "bad-protx-sig" error when revoking masternodes registered with legacy BLS on V19-active networks. The bug was introduced when protx_revoke was changed to determine ptx.nVersion based on the masternode's state version rather than network deployment status. This caused a mismatch: signatures were created with the legacy BLS scheme (based on MN state) but verified using the basic BLS scheme (based on the global flag after V19). Changes: - protx_revoke: Use V19 deployment status for version and signing scheme determination, matching v22.1.3 behavior - protx_update_service: Remove is_basic_override parameter to use deployment status consistently 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add functional test to verify that protx update_service and protx revoke work correctly for masternodes registered with legacy BLS keys after V19 activation. This test covers the regression where the RPC used the masternode's state version to determine the BLS signing scheme, causing "bad-protx-sig" errors when the validator used the global scheme flag instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis pull request modifies the protx RPC operations ( 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/evo.cpp (1)
1254-1266: Fix clang-format issues flagged by CI.The pipeline is failing due to clang-format differences in this region (lines 1251-1253). The semicolon placement inside the
WITH_LOCKlambda may be causing the issue.🔧 Suggested fix for formatting
- const bool isV19active{DeploymentActiveAfter( - WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip();), - chainman.GetConsensus(), Consensus::DEPLOYMENT_V19)}; + const bool isV19active{ + DeploymentActiveAfter(WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip();), + chainman.GetConsensus(), Consensus::DEPLOYMENT_V19)};Run
clang-format-diff.pyto verify the expected formatting.
🤖 Fix all issues with AI agents
In @src/rpc/evo.cpp:
- Around line 1012-1013: Clang-format is out-of-sync for the ptx.nVersion
assignment; run the repo's clang-format (or clang-format-diff.py) and reformat
the expression using the project's style so the call to
ProTxVersion::GetMaxFromDeployment<CProUpServTx>(WITH_LOCK(::cs_main, return
chainman.ActiveChain().Tip()), chainman) is wrapped/indented exactly as other
similar long calls in the file (preserving WITH_LOCK and ::cs_main usage) and
commit the formatting-only change.
In @test/functional/rpc_protx_legacy_bls.py:
- Around line 1-5: Update the copyright year in the file header from 2024 to
2026 by editing the copyright comment line that currently reads "Copyright (c)
2024 The Dash Core developers" so it becomes "Copyright (c) 2026 The Dash Core
developers"; ensure the updated year appears in the same copyright comment and
leave the rest of the header (shebang and license reference) unchanged.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/rpc/evo.cpptest/functional/rpc_protx_legacy_bls.pytest/functional/test_runner.py
🧰 Additional context used
📓 Path-based instructions (2)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/test_runner.pytest/functional/rpc_protx_legacy_bls.py
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 (8)
📓 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.
📚 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/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
test/functional/rpc_protx_legacy_bls.py
📚 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:
test/functional/rpc_protx_legacy_bls.pysrc/rpc/evo.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
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.
Applied to files:
test/functional/rpc_protx_legacy_bls.py
📚 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-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-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
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[error] 1009-1010: Clang format differences found. Run 'clang-format-diff.py' to fix formatting differences in this region.
[error] 1251-1253: Clang format differences found. Run 'clang-format-diff.py' to fix formatting differences in this region.
⏰ 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: arm-linux-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (7)
src/rpc/evo.cpp (2)
1079-1081: LGTM: Deployment-based signing forupdate_service.Using
isV19activefrom deployment status rather than the masternode's stored version correctly aligns the RPC signing scheme with what the validator expects. This resolves the mismatch for legacy BLS masternodes after V19 activation.
1310-1311: LGTM: Deployment-based signing forrevoke.The signing now correctly uses
!isV19activeto determine the legacy scheme flag, which aligns with the validator's behavior. This is the core fix for the "bad-protx-sig" error described in the PR objectives.test/functional/test_runner.py (1)
352-352: LGTM: New regression test added to base test suite.Adding this to
BASE_SCRIPTSensures the legacy BLS regression test runs by default, which is appropriate for catching future breakage in this critical path.Note: The AI summary states the test was added to
EXTENDED_SCRIPTS, but it's actually added toBASE_SCRIPTS.test/functional/rpc_protx_legacy_bls.py (4)
6-25: Excellent documentation of the regression scenario.The docstring clearly explains the bug scenario, the root cause (MN state version vs. global scheme flag), and what the fix ensures. This is valuable for future maintainers.
31-50: LGTM: Test setup correctly configures V19 activation and legacy MNs.The setup with V19 activating at block 200 and 3 pre-V19 masternodes correctly establishes the regression test conditions. The assertions verify the preconditions are met.
81-107: LGTM:update_servicetest flow is correct.The test properly funds the transaction, calls
update_service, and verifies confirmation. This exercises the fix inprotx_update_service_common_wrapper.
109-153: LGTM:revoketest flow correctly handles node disconnection.The test appropriately handles the edge case where the revoked masternode's node gets disconnected. Using
sync_fun=self.no_opon line 134 avoids sync failures, then manually reconnects and syncs before verification.The verification of
PoSeBanHeight != -1correctly confirms the revocation was processed.
| ptx.nVersion = ProTxVersion::GetMaxFromDeployment<CProUpServTx>( | ||
| WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()), chainman, | ||
| /*is_basic_override=*/dmn->pdmnState->nVersion > ProTxVersion::LegacyBLS); | ||
| WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()), chainman); |
There was a problem hiding this comment.
Fix clang-format issues flagged by CI.
The pipeline is failing due to clang-format differences in this region (lines 1009-1010). Run clang-format-diff.py to resolve the formatting.
🤖 Prompt for AI Agents
In @src/rpc/evo.cpp around lines 1012 - 1013, Clang-format is out-of-sync for
the ptx.nVersion assignment; run the repo's clang-format (or
clang-format-diff.py) and reformat the expression using the project's style so
the call to
ProTxVersion::GetMaxFromDeployment<CProUpServTx>(WITH_LOCK(::cs_main, return
chainman.ActiveChain().Tip()), chainman) is wrapped/indented exactly as other
similar long calls in the file (preserving WITH_LOCK and ::cs_main usage) and
commit the formatting-only change.
| #!/usr/bin/env python3 | ||
| # Copyright (c) 2024 The Dash Core developers | ||
| # Distributed under the MIT software license, see the accompanying | ||
| # file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
There was a problem hiding this comment.
Update copyright year to 2026.
The copyright year is set to 2024, but the current year is 2026.
🔧 Suggested fix
-# Copyright (c) 2024 The Dash Core developers
+# Copyright (c) 2026 The Dash Core developers📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/usr/bin/env python3 | |
| # Copyright (c) 2024 The Dash Core developers | |
| # Distributed under the MIT software license, see the accompanying | |
| # file COPYING or http://www.opensource.org/licenses/mit-license.php. | |
| #!/usr/bin/env python3 | |
| # Copyright (c) 2026 The Dash Core developers | |
| # Distributed under the MIT software license, see the accompanying | |
| # file COPYING or http://www.opensource.org/licenses/mit-license.php. |
🤖 Prompt for AI Agents
In @test/functional/rpc_protx_legacy_bls.py around lines 1 - 5, Update the
copyright year in the file header from 2024 to 2026 by editing the copyright
comment line that currently reads "Copyright (c) 2024 The Dash Core developers"
so it becomes "Copyright (c) 2026 The Dash Core developers"; ensure the updated
year appears in the same copyright comment and leave the rest of the header
(shebang and license reference) unchanged.
| from test_framework.util import assert_equal | ||
|
|
||
|
|
||
| class ProtxLegacyBLSTest(DashTestFramework): |
There was a problem hiding this comment.
feature_dip3_v19.py already have some limited tests for v19 activation and combination of legacy / basic RPCs.
It includes even tests for protx revoke:
self.log.info(f"Trying to revoke proTx:{self.mninfo[-1].proTxHash}")
self.test_revoke_protx(evo_info_3.nodeIdx, self.mninfo[-1])
Could feature_dip3_v19.py be improved by adding this particular corner case instead creating a brand new functional test just for one corner case?
3 participants
Summary
Fix "bad-protx-sig" error when revoking or updating service for masternodes registered with legacy BLS keys on V19-active networks.
Root Cause: The RPC was changed to determine
ptx.nVersionand signing scheme based on the masternode's state version. However, the validator uses the global BLS scheme flag for verification. This caused a mismatch for legacy BLS masternodes on V19-active networks:ptx.nVersion!isV19active(→ BasicBLS)ptx.nVersion == LegacyBLS(→ LegacyBLS)Fix: Restore v22.1.3 behavior - determine version and signing scheme based on network deployment status, not masternode state.
Changes
src/rpc/evo.cpp:protx_revoke: Use V19 deployment status for version and signing schemeprotx_update_service: Removeis_basic_overrideparameter to use deployment statusTest plan
test/functional/rpc_protx_legacy_bls.pyprotx update_serviceworks for legacy BLS MNs after V19protx revokeworks for legacy BLS MNs after V19🤖 Generated with Claude Code