fix: proper migration for legacy bls keys to the protx v>=2#7472
Conversation
…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
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
|
🕓 Ready for review — 10 ahead in queue (commit 5f66192) |
![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: 94f3129802
ℹ️ 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".
| // v24-gated version transition, so pre-v24 blocks are untouched. | ||
| if (state_mn.pubKeyOperator != CBLSLazyPublicKey()) { | ||
| const auto pubkey{state_mn.pubKeyOperator.Get()}; | ||
| state_mn.pubKeyOperator.Set(pubkey, nVersion == ProTxVersion::LegacyBLS); |
There was a problem hiding this comment.
When a v24 update keeps the same operator key, this line changes only the CBLSLazyPublicKey encoding; however CDeterministicMNStateDiff still decides whether to include Field_pubKeyOperator via operator!=, which treats legacy/basic encodings of the same point as equal. BuildDiff therefore omits the key, so nodes reconstructing the MN list from stored/P2P diffs set nVersion to v3 but keep the legacy-encoded key and legacy unique-property hash, reintroducing the reconstruction-dependent duplicate-key behavior this patch is trying to remove.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR centralizes ProTx version selection and owner-payout extraction around Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RPC
participant GetValidatedPayload
participant ProTxState
participant PayoutConsumers
RPC->>GetValidatedPayload: construct and validate ProTx payload
GetValidatedPayload->>ProTxState: apply version and payout state
ProTxState->>PayoutConsumers: expose state-based owner payouts
PayoutConsumers-->>RPC: serialize or display payout destinations
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 |
![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)
src/validation.cpp (1)
5831-5838: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the
ChainstateManageroverload here. It keeps both deployment checks on the same path and avoids mixingGetConsensus()withchainman.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validation.cpp` around lines 5831 - 5838, Update DeploymentToProtxVersion to use the ChainstateManager overload of DeploymentActiveAfter for the DEPLOYMENT_V19 check, passing chainman consistently as already done for DEPLOYMENT_V24; preserve the existing is_basic_override behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rpc/evo.cpp`:
- Line 1106: Update the ptx.nVersion assignment in the relevant RPC flow to
retain BasicBLS for legacy service updates after v24 instead of unconditionally
using DeploymentToProtxVersion(). Preserve the higher-version selection for
applicable non-legacy updates so SignAndSendSpecialTx() receives a valid
transaction version.
- Around line 1228-1239: Update the empty operator-key handling in the RPC
method using nVersion so a reused legacy operator key is decoded and re-encoded
with the target version when nVersion is ExtAddr, while preserving the same key.
Do not copy the legacy encoding unchanged; ensure the resulting key satisfies
the existing version invariant.
---
Nitpick comments:
In `@src/validation.cpp`:
- Around line 5831-5838: Update DeploymentToProtxVersion to use the
ChainstateManager overload of DeploymentActiveAfter for the DEPLOYMENT_V19
check, passing chainman consistently as already done for DEPLOYMENT_V24;
preserve the existing is_basic_override behavior.
🪄 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: 150955fa-ddc8-4072-95fa-5096a05accef
📒 Files selected for processing (28)
src/common/bloom.cppsrc/evo/core_write.cppsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/dmnstate.cppsrc/evo/dmnstate.hsrc/evo/netinfo.cppsrc/evo/providertx.cppsrc/evo/providertx.hsrc/evo/providertx_util.cppsrc/evo/specialtx_filter.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/types.hsrc/masternode/payments.cppsrc/node/interfaces.cppsrc/rpc/evo.cppsrc/rpc/masternode.cppsrc/test/data/trivially_invalid.jsonsrc/test/evo_deterministicmns_tests.cppsrc/test/evo_trivialvalidation.cppsrc/validation.cppsrc/validation.htest/functional/feature_masternode_payout_shares.pytest/functional/feature_protx_version.pytest/functional/rpc_netinfo.pytest/functional/test_runner.pytest/lint/lint-circular-dependencies.py
|
Extra python script to reproduce issue [it is reproducible on top of develop as well]: repro_protx_dupkey_reload_divergence.py not an artefact to merge so far as replaced by regression tests |
3 participants
depends on #7302 to prevent conflicts ;
but changes are appliable for
develop, issue is reproduced on develop too.Issue being fixed or feature implemented
Issue has been spotted during reviewing #7302, see original comment:
What was done?
Keep the operator key's BLS encoding a deterministic function of nVersion, matching the SML and
on-disk serialization, so the unique-property index is re-keyed to the same scheme on every node
whether the list was built online or reloaded from a snapshot. Done before the early return because
callers pre-set nVersion, which would otherwise skip a needed re-encode. Runs only under the
v24-gated version transition, so pre-v24 blocks are untouched.
How Has This Been Tested?
Firstly, added new regression tests.
Secondly, there's mockup python script that reproduces issue (first comment on PR). This script is not meant to be merged, because its functionality is covered by regression tests
Breaking Changes
Fixes Dash Core v24 introduced issue, has not been released yet.
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.