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
|
⛔ Blockers found — Sonnet deferred (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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The reused-key RPC path is fixed at this head, but the BLS re-encoding migration is not preserved in deterministic masternode state diffs. Reconstructing a list from a snapshot and persisted diffs can therefore produce a different operator-key uniqueness index and different block-validation results. The three migration commits should also be made atomic before merge.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/evo/specialtxman.cpp`:
- [BLOCKING] src/evo/specialtxman.cpp:61-63: Persist the encoding-only operator-key change in state diffs
These lines change a legacy BLS wrapper to basic encoding while preserving the decoded point. `CDeterministicMNStateDiff` uses `operator!=`, which treats wrappers containing the same point as equal, so `BuildDiff()` omits `Field_pubKeyOperator` even though the unique-property hash changed. Replaying the persisted diff from the preceding snapshot updates `nVersion` and `netInfo` but retains the legacy wrapper and legacy uniqueness-map entry. A basic-encoded ProRegTx or ProUpRegTx reusing that point can then pass `HasUniqueProperty()` on the reconstructed node while a node retaining the live list rejects it, making consensus validation depend on cache or restart history. Record encoding changes in the state diff and add a test that reconstructs the migrated list by applying the serialized block diff to a pre-migration snapshot.
In `<commit:48d3735>`:
- [SUGGESTION] <commit:48d3735>:1: Do not place failing regression tests before their fix
Commit 48d373562f7 adds tests that fail at that commit because operator-key re-encoding and uniqueness-map re-keying arrive only in 94f31298021. The subsequent 5f661926e17 `fixup` commit then completes the same change in the registrar RPC path. Reorder the implementation before the tests or squash these three commits into one atomic migration fix, with the explicit fixup folded into the substantive implementation commit.
| 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.
🔴 Blocking: Persist the encoding-only operator-key change in state diffs
These lines change a legacy BLS wrapper to basic encoding while preserving the decoded point. CDeterministicMNStateDiff uses operator!=, which treats wrappers containing the same point as equal, so BuildDiff() omits Field_pubKeyOperator even though the unique-property hash changed. Replaying the persisted diff from the preceding snapshot updates nVersion and netInfo but retains the legacy wrapper and legacy uniqueness-map entry. A basic-encoded ProRegTx or ProUpRegTx reusing that point can then pass HasUniqueProperty() on the reconstructed node while a node retaining the live list rejects it, making consensus validation depend on cache or restart history. Record encoding changes in the state diff and add a test that reconstructs the migrated list by applying the serialized block diff to a pre-migration snapshot.
source: ['codex']
Adopts the maintainer's preferred approach from dashpay#7472 (re-encode the operator key on a version change) instead of forcing a key rotation to leave LegacyBLS, while keeping the cross-scheme uniqueness guards this branch added so the re-key cannot collide and stall block production. A masternode operator can keep the same BLS private key across the legacy->basic migration; only the serialized encoding of the public key changes. Rather than rejecting a same-key ProUpServTx/ProUpRegTx and forcing a rotation (which also PoSe-bans the masternode), SetStateVersion() now re-encodes the stored key to the scheme its version implies, and UpdateUniqueProperty() re-keys the scheme-dependent unique-property map when the encoding changes. The RPCs build a BasicBLS migration payload for a legacy masternode instead of erroring. Because dashpay#7472's re-encode collides -- and UpdateMN() throws out of block assembly -- when a squatter already holds the same key under the other encoding (the live per-encoding registration hole), migration is guarded: CheckProUpServTx, CheckProUpRegTx and RebuildListFromBlock reject a migration that would collide with another masternode's key under either scheme (bad-protx-dup-key), and only when the key actually changes or the version crosses the scheme boundary, so a grandfathered cross-scheme pair's non-migrating routine update is not blocked. CDeterministicMNStateDiff also has to capture the re-encoding: its field comparison used CBLSLazyPublicKey::operator==, which ignores the scheme, so a same-key migration produced a diff that omitted the key. A node reconstructing the list from evoDB diffs then kept the old encoding while an online-built list had the new one -- a reconstruction split that full-snapshot serialization does not reveal. The diff now compares the scheme-dependent hash. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Likely #7473 to merge instead |
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.