fix: proper migration for legacy bls keys to the protx v>=2 - #7472
Draft
knst wants to merge 19 commits into
Draft
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
5 tasks
5 tasks
|
⛔ 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
Collaborator
Author
|
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 |
Merged
5 tasks
thepastaclaw
suggested changes
Jul 18, 2026
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.
Comment on lines
+61
to
+63
| 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']
UdjinM6
added a commit
to UdjinM6/dash
that referenced
this pull request
Jul 18, 2026
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>
knst
marked this pull request as draft
July 21, 2026 07:12
Collaborator
Author
|
Likely #7473 to merge instead |
UdjinM6
added a commit
to UdjinM6/dash
that referenced
this pull request
Jul 25, 2026
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>
|
This pull request has conflicts, please rebase. |
UdjinM6
added a commit
to UdjinM6/dash
that referenced
this pull request
Jul 28, 2026
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>
PastaPastaPasta
added a commit
that referenced
this pull request
Aug 7, 2026
… (desync + cross-scheme uniqueness) 4963125 test: deduplicate BLS migration regression fixtures (PastaClaw) b6722c2 doc: add release notes for #7473 (UdjinM6) d62a031 test: cover BLS scheme migration and cross-scheme operator-key uniqueness (UdjinM6) 71e7980 fix: allow legacy->basic operator key migration without key rotation (UdjinM6) f094101 fix: enforce operator key uniqueness across BLS schemes (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Two independent bugs in how a masternode's operator BLS key relates to its state version. Both exist on `develop` today and are reproduced by test; neither is introduced by this change. > **Rebased on #7302.** This branch now sits on top of the merged #7302 (uniform > protx versioning: v4 merged into v3, RPC version selection via > `DeploymentToProtxVersion`, and the v1→v3 update restriction removed). The fixes > below are unchanged in substance; the migration now lands a legacy masternode at > **v3** directly (rather than clamping to v2) because #7302 allows v1→v3, and the > operator-key re-encoding continues to make that safe. **(A) Post-v24 operator-key scheme desync → consensus split.** `CDeterministicMNState` derives its operator key's serialization from `nVersion` (`CBLSLazyPublicKeyVersionWrapper(key, nVersion == LegacyBLS)`). But after v24 a masternode's `nVersion` could rise out of LegacyBLS (v1) to a basic-scheme version (v2/v3) while the stored key kept the legacy scheme flag: - `ProUpServTx` carries no operator key, yet set `newState->nVersion` from the payload. - `ProUpRegTx` re-submitting the *same* key basic-encoded left `operator_changed` false, because `CBLSLazyPublicKey::operator==` compares the public key and ignores the scheme, so the re-encode was skipped while the version still rose. Once a state has a basic-scheme `nVersion` with a legacy-flagged key, a list rebuilt from block diffs and the same list reloaded from disk hash that key differently, so `mnUniquePropertyMap` diverges. Nodes then disagree on `HasUniqueProperty()` and therefore on `bad-protx-dup-key` — a chain split determined purely by restart history. `update_service` is the routine operation that plants this: any legacy masternode updating its service address after v24 would trigger it, no attacker required. **(B) Operator-key uniqueness enforced per encoding, not per key (live today).** `mnUniquePropertyMap` is keyed by `GetUniquePropertyHash()`, which serializes its argument, and a BLS key serializes differently under the two schemes, so `H(K, legacy) != H(K, basic)`. `CheckProRegTx`'s duplicate check consults that map, so it does not see an operator key an existing masternode holds under the *other* encoding. A `ProRegTx` never proves ownership of the operator key, so anyone can re-register an existing masternode's operator public key for the price of a collateral. This is exploitable on mainnet now; it is closed here for post-v24. ## What was done? The approach is **migrate, don't force rotation** (adopting the maintainer's preferred model from #7472), while keeping the cross-scheme uniqueness guards that make the re-encode safe. Every new consensus rule is gated on `DEPLOYMENT_V24`. A legacy masternode keeps the same operator private key across the migration; only the serialized encoding of the public key changes. On a version bump, `SetStateVersion()` re-encodes the stored key to the scheme its version implies (`Set(Get(), …)`, not `SetLegacy()`, so the cached serialization actually changes), and `UpdateUniqueProperty()` re-keys the scheme-dependent unique-property map when the encoding changes (it now compares `GetUniquePropertyHash()` rather than the scheme-blind `operator==`). Re-encoding is only safe if the target slot is free: `UpdateMN()` reports a duplicate by *throwing*, and that throw escapes `BlockAssembler::CreateNewBlock`, stalling block production. Because bug B leaves the per-encoding registration hole open, a squatter can already hold the key under the other encoding — so the migration is guarded at every layer, and only when the key actually changes or the version crosses the legacy→basic boundary (a grandfathered cross-scheme pair's non-migrating routine update is not blocked). The unique-property map is deliberately **not** made canonical. It is derived, not serialized: `CDeterministicMNList::Unserialize` clears it and rebuilds via `AddMN`. A canonical hash would apply retroactively to all history and, if any cross-scheme duplicate pair already exists, make `AddMN` throw and nodes fail to sync past it. Cross-scheme pairs created before activation are therefore tolerated (nothing rehashes the map) and remain usable for non-migrating routine updates; a same-key legacy→basic migration by the still-legacy member is rejected `bad-protx-dup-key` until the conflicting key is rotated. The change is split into two source commits, a test commit, a release-notes commit, and a follow-up test-fixture dedup commit; each source commit builds on its own. **Commit 1 — enforce uniqueness across schemes.** A helper `HasOperatorKeyUnderAnyScheme` probes both encodings (two O(1) lookups, not a scan), wired in at every point a key can be claimed: - `CheckProRegTx` and `CheckProUpRegTx`, against the previous block's list. - `RebuildListFromBlock`, against the list as rebuilt so far (same-block pairs). - `AcceptToMemoryPool`. This probe is deliberately **not** v24-gated: block assembly does not revalidate special transactions cumulatively, so a pair admitted before activation is never evicted and would poison every template afterwards. Keeping the pair out of the mempool is what actually closes that, and mempool policy is allowed to be stricter than consensus (before v24 a node rejecting the second transaction would still accept a consensus-valid block containing the pair; after v24 consensus rejects the pair too, so the policy difference cannot split the chain in either regime). The registrar probes run only when the operator key is actually changing: an update that keeps its own key cannot create a duplicate, and probing it anyway would let a pre-activation cross-scheme pair permanently block the affected masternode's registrar updates. **Commit 2 — allow legacy→basic migration without key rotation.** Handles bug A by migrating in place rather than forcing a rotation (adopting #7472's re-encode model): - `SetStateVersion()` re-encodes the stored operator key to the scheme its version implies (`Set(Get(), …)`, not `SetLegacy()`, so the cached serialization actually changes), keeping the invariant that a stored key's encoding always matches its `nVersion`; `UpdateUniqueProperty()` re-keys the scheme-dependent map on an encoding change (comparing `GetUniquePropertyHash()` rather than the scheme-blind `operator==`). - The migration is guarded against collisions via `HasOperatorKeyUnderAnyScheme` in `CheckProUpServTx`, `CheckProUpRegTx` and `RebuildListFromBlock` — the last against the list as rebuilt so far, since per-transaction checks run against `pindexPrev` and are blind to an earlier transaction in the same block — so a re-encode that would collide with another masternode's key cannot throw out of block assembly. The guard fires only when the operator key changes or the version crosses the legacy→basic boundary (a shared `IsSchemeMigration` predicate), so a grandfathered cross-scheme pair's non-migrating update is not blocked. - `CDeterministicMNStateDiff` now compares the scheme-dependent hash for the operator key field. Its comparison used `operator==` (scheme-blind), so a same-key migration produced a diff that omitted the key — a node reconstructing the list from evoDB diffs kept the old encoding while an online-built list had the new one, a reconstruction split that full-snapshot serialization does not reveal. - RPC (`rpc/evo.cpp`): `protx update_service` / `protx update_registrar` migrate a legacy masternode to the basic scheme (keeping the same key, or supplying a new one), instead of erroring or forcing a rotation. Version selection flows through #7302's `DeploymentToProtxVersion`, so post-v24 the migration payload is **v3** (ExtAddr) — the legacy→BasicBLS clamp #7302 removed is not reintroduced; the stored/reused operator key is re-encoded to the basic scheme so it matches the target version. The legacy-BLS RPC variants keep the masternode on the legacy scheme. **Commit 3 — tests.** Unit tests in `evo_deterministicmns_tests.cpp` and the functional-test extension in `feature_protx_version.py` (renamed from `feature_dip3_v19.py` by #7302) covering both fixes (enumerated below). **Commit 4 — release notes.** `doc/release-notes-7473.md` documenting the post-v24 per-key uniqueness enforcement and the in-place migration behavior of `protx update_service` / `protx update_registrar`. **Commit 5 — test fixture dedup.** Deduplicates the v24-activation scaffolding in `evo_deterministicmns_tests.cpp` into a shared `TestMNChainSetup` fixture; no behavior change (test-only refactor, contributed by a reviewer). ## How Has This Been Tested? Built with `--enable-werror`; the full unit suite (`./src/test/test_dash`, 778 cases) and the touched functional test (`feature_protx_version.py`) both pass, and lint is clean for the changed files. Each source commit was rebuilt independently and compiles on its own under `--enable-werror`; the test commit adds the coverage below, and the full `evo_dip3_activation_tests` suite passes. Unit tests (`src/test/evo_deterministicmns_tests.cpp`) — each rejection test was watched fail first (by flipping the v24 activation height): - `proupserv_migrates_legacy`, `proupreg_migrates_legacy_same_key` — the happy-path migrations: a legacy masternode raises its version keeping the same operator key, the stored key re-encodes to basic, and the masternode is *not* PoSe-banned. - `migration_rejected_when_key_squatted` — a squatter already holds the key under the other encoding; the migration is rejected `bad-protx-dup-key` at the per-tx layer rather than throwing. - `same_mn_same_block_migration_consistent`, `same_mn_same_block_version_crossing_key_rotation` — same-masternode transitions within one block reconstruct identically. - `same_block_cross_scheme_key_pair_rejected`, `mempool_rejects_cross_scheme_key_race`, `pre_v24_cross_scheme_pair_cannot_become_resident` — the same-block, mempool, and activation-boundary pairings that motivated the rebuild-time and ungated mempool probes. - `proregtx_rejects_cross_scheme_key_reuse`, `proupreg_rejects_cross_scheme_key_reuse` — bug B at the registration and registrar paths, both directions, with fresh-key and self-key non-false-positive cases. - `statediff_captures_operator_key_reencoding` — the evoDB-diff reconstruction path: a same-key migration must emit the re-encoded key in the diff. - `has_operator_key_under_any_scheme` — the two-encoding lookup helper. - `stale_special_tx_does_not_poison_template` — a resident-but-invalid special tx seeded via `addUnchecked`; without the package recheck `CreateNewBlock` throws. - `pre_v24_behaviour_unchanged` — the non-retroactivity guard: asserts the new rules are inert before v24, so nothing on live mainnet/testnet changes. Functional test (`test/functional/feature_protx_version.py`): extended to assert, after v24 activation, that a legacy masternode's `update_service` migrates it to the basic scheme in place (v3 payload, `state.version == 3`, same operator key re-encoded to a different hex, `PoSeBanHeight == -1`), and that the list reloads from disk identically after the migration. The same-key migration runs alongside #7302's rotated-key `update_registrar` migration and is sequenced last (with a reconnect after the key re-encoding, which churns the migrated node's masternode connections) so it does not destabilise the surrounding checks. ## Breaking Changes All consensus rules here are gated on `DEPLOYMENT_V24`, which is `NEVER_ACTIVE` on mainnet and testnet, so there is no consensus change on any live network; `pre_v24_behaviour_unchanged` asserts this. After v24 activates: - Absent a cross-scheme key collision, a LegacyBLS masternode migrates to the basic scheme in place on its next version bump (`update_service` or `update_registrar`), keeping the same operator key; the stored public key is re-encoded legacy→basic. No forced key rotation, no PoSe ban. - Registering or updating to an operator public key already held by another masternode is rejected regardless of which BLS encoding either side uses, and a migration that would collide with such a key is rejected `bad-protx-dup-key` rather than stalling block assembly. The `AcceptToMemoryPool` cross-scheme check is mempool policy (ungated) rather than consensus, so a node may reject a second in-flight transaction that another node's mempool accepted; both would still accept a consensus-valid block containing it before v24, and after v24 consensus rejects the pair, so the policy difference cannot split the chain. Not addressed here, and worth stating: cross-scheme duplicate pairs created *before* activation are tolerated, not resolved; and bug B is only closed post-v24, so the registration hole remains open on mainnet until v24 activates. ## 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 - [x] I have made corresponding changes to the documentation (release notes) - [x] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: 257bb3de1c69d3291b833f325dc6dca59bfc4da87539c55509e3d1bc7d227072c018bca49649e2028c73748e9371da74bcf32b370ce3511a05f9c3cc518c129d
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.