feat(platform-wallet): fixed provider key derivation via rust-dashcore bump + legacy BLS display#4120
Merged
Merged
Conversation
…on; show legacy BLS form Bumps the pin to 56a40788 picking up dashpay/rust-dashcore#879 (BLS operator / Ed25519 platform-node derivation now matches DashSync/dashbls: no spurious 0x00 in hardened children, legacy G1 serialization in the HD chain, bip39 seed fed directly instead of the secp hybrid), #877 (mainnet sync floored at HD activation height) and #874 (seed refresh). The provider key detail screens now show both BLS serializations — "BLS Public Key" (modern) and "BLS Public Key (Legacy)" — emitted from Rust via the new legacy serializer. Existing wallets need a delete + re-import for provider accounts to re-derive under the fixed scheme. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProvider key derivation now uses raw BIP-39 seeds, supports modern and legacy BLS representations, computes Tenderdash platform-node IDs, restores provider pools, and propagates these changes through Rust FFI, Swift persistence, indexing, and UI behavior. ChangesProvider wallet derivation and restoration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MnemonicResolver
participant RustWallet
participant WalletPersistence
participant SwiftWallet
participant SwiftUI
MnemonicResolver->>RustWallet: resolve zeroizing raw seed
RustWallet->>RustWallet: derive provider keys and platform-node IDs
WalletPersistence->>RustWallet: restore provider key pools
RustWallet->>SwiftWallet: return derived keys and ownership data
SwiftWallet->>SwiftUI: provide legacy key and masternode state
SwiftUI->>SwiftUI: render key rows and gated claim state
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Collaborator
|
⛔ Blockers found — Sonnet deferred (commit 4ac9108) |
thepastaclaw
suggested changes
Jul 13, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
All three Codex findings are confirmed. The rust-dashcore #879 bump changes provider-account derivation to use the BIP39 seed directly, but Platform still duplicates the obsolete secp-hybrid derivation, restores incompatible persisted provider xpubs without an enforced migration, and indexes only the modern serialization when matching historical legacy BLS operator keys.
Source: Codex reviewer gpt-5.6-sol (general); verifier gpt-5.6-sol.
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 (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
3 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 `packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs:268-296: Provider derivation still uses the obsolete secp-hybrid seed
The pinned rust-dashcore #879 change makes provider accounts derive from the BIP39 seed directly, but this path still derives a secp256k1 child at the provider account path and passes its 32-byte scalar to the BLS or Ed25519 master. The registration helper repeats that obsolete derivation at lines 96-113, and the FFI resolver reduces the mnemonic to an ExtendedPrivKey before reaching this method. Fresh accounts created by Wallet::from_mnemonic therefore use different account keys from private operator reveals, platform-node derivations, and the platform-node batch persisted during registration. Debug builds can hit the existing account-xpub assertions; release builds can silently return or persist a different valid key. Preserve the raw BIP39 seed through the resolver and share the same provider derivation routine used by upstream account creation, with cross-path tests against Wallet::from_mnemonic.
In `packages/rs-platform-wallet/src/manager/accessors.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/accessors.rs:897-900: Legacy BLS operator keys are omitted from ownership matching
ProviderDerivedKey now returns modern and legacy serializations of the same BLS point, but the ownership map records only public_key_bytes. aggregate_masternodes preserves the exact 48 bytes carried by ProRegTx or ProUpRegTx, and masternode_entry_ffi performs an exact lookup against this map. Wallet-owned masternodes whose payload contains the legacy serialization therefore remain operator_in_wallet=false. Index both serializations under the same derivation index.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3042-3065: Pre-fix provider xpubs are restored without migration or rejection
The restore path decodes and reinstalls persisted BLS and Ed25519 provider xpubs without a derivation-version marker or compatibility check. Consequently, upgraded wallets continue using pre-#879 provider account points while fresh imports use the corrected direct-seed scheme. The commit message notes that existing wallets require deletion and re-import, but the implementation neither enforces nor communicates that requirement at load time, so stale provider keys remain silently usable. Rederive or migrate these accounts through the mnemonic resolver, version the persisted representation, or reject incompatible rows with an explicit re-import error.
…on persisted provider accounts Deletes platform-wallet's duplicated pre-#879 hybrid derivation: all operator/platform-node key derivation now delegates to the corrected upstream key-wallet primitives (gate-free variants usable on resident and watch-only wallets), and the FFI mnemonic resolver preserves the raw BIP39 seed instead of collapsing it to a secp256k1 master. The operator ownership map indexes both BLS serializations (v1 ProRegTx payloads carry legacy bytes). Persisted provider account xpubs are stamped with a derivation-version marker; unstamped (pre-fix) rows are skipped at load with a warning so stale keys are never shown or matched — a wallet re-import regenerates them correctly. Golden-vector tests pin the derivation to upstream's dashbls reference values, and a cross-path test asserts both derivation entry points agree; user-verified against dashwallet-ios (DashSync) with a shared mnemonic on mainnet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pre-#879 derivation never shipped, so the version-magic machinery guarded a dev-only transient state that delete+re-import cures. Provider xpubs persist as raw bincode like every other account; a comment records the accepted consequence for pre-fix dev wallets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs (2)
463-519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate platform-node derivation logic vs.
derive_platform_node_public_keys.This branch re-implements the same seed → Ed25519 master → DIP-3 account path → hardened child → pubkey/node-id sequence already implemented in
derive_platform_node_public_keys(lines 108-192) in this same file. Given the PR's premise is exactly that duplicated/hybrid derivation logic caused a divergence bug, consider extracting a shared private helper both call so a future fix only needs to land once.// Shared by `derive_platform_node_public_keys` and the // `ProviderKeyKind::PlatformNode` branch below. fn platform_node_xpriv_at( seed: &[u8], network: key_wallet::Network, index: u32, ) -> Result<ExtendedEd25519PrivKey, PlatformWalletError> { let account_path = AccountType::ProviderPlatformKeys .derivation_path(network) .map_err(|e| PlatformWalletError::KeyDerivation(format!( "failed to build provider platform-node account path: {e}" )))?; let master = ExtendedEd25519PrivKey::new_master(network, seed).map_err(|e| { PlatformWalletError::KeyDerivation(format!("failed to build Ed25519 master: {e}")) })?; let account_xpriv = master.derive_priv(&account_path).map_err(|e| { PlatformWalletError::KeyDerivation(format!("failed to derive account key: {e}")) })?; let child = ChildNumber::from_hardened_idx(index).map_err(|e| { PlatformWalletError::KeyDerivation(format!("invalid platform-node key index {index}: {e}")) })?; account_xpriv.derive_priv(&[child]).map_err(|e| { PlatformWalletError::KeyDerivation(format!("failed to derive key at index {index}: {e}")) }) }🤖 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 `@packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs` around lines 463 - 519, Duplicate platform-node derivation should be centralized to prevent future divergence. Extract a shared private helper such as platform_node_xpriv_at for the seed-to-child ExtendedEd25519PrivKey sequence, then update both derive_platform_node_public_keys and the ProviderKeyKind::PlatformNode branch to use it while preserving their existing public-key, node-id, and private-key handling.
347-440: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSeed-vs-xpub operator key cross-check only runs in debug builds.
The
debug_assert_eq!(lines 396-404) verifying the seed-derived operator pubkey matches the account-xpub derivation is compiled out under#[cfg(debug_assertions)]in release builds. Since this exact failure mode (seed-derived vs. xpub-derived key mismatch) is what this PR was created to fix, a future regression here would silently hand out a masternode operator key that doesn't match what's registered on-chain, with no safeguard in production.🛡️ Promote the debug-only check to a real error in all builds
- #[cfg(debug_assertions)] - if let Ok(xpub) = account.bls_public_key.derive_pub_legacy(child) { - debug_assert_eq!( - xpub.to_bytes(), - xpriv.public_key_bytes(), - "BLS operator seed-derived pubkey diverged from account-xpub \ - derivation" - ); - } + let xpub = account.bls_public_key.derive_pub_legacy(child).map_err(|e| { + PlatformWalletError::KeyDerivation(format!( + "failed to cross-check BLS operator key at index {index}: {e}" + )) + })?; + if xpub.to_bytes() != xpriv.public_key_bytes() { + return Err(PlatformWalletError::KeyDerivation( + "BLS operator seed-derived pubkey diverged from account-xpub \ + derivation".to_string(), + )); + }🤖 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 `@packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs` around lines 347 - 440, Update the seed-handling branch in the operator key derivation flow to always compare the seed-derived public key with account.bls_public_key.derive_pub_legacy(child), removing the debug_assert_eq! and its debug-only gating. On mismatch, return a PlatformWalletError::KeyDerivation with clear context instead of producing ProviderDerivedKey; preserve successful derivation behavior when the keys match.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs`:
- Around line 463-519: Duplicate platform-node derivation should be centralized
to prevent future divergence. Extract a shared private helper such as
platform_node_xpriv_at for the seed-to-child ExtendedEd25519PrivKey sequence,
then update both derive_platform_node_public_keys and the
ProviderKeyKind::PlatformNode branch to use it while preserving their existing
public-key, node-id, and private-key handling.
- Around line 347-440: Update the seed-handling branch in the operator key
derivation flow to always compare the seed-derived public key with
account.bls_public_key.derive_pub_legacy(child), removing the debug_assert_eq!
and its debug-only gating. On mismatch, return a
PlatformWalletError::KeyDerivation with clear context instead of producing
ProviderDerivedKey; preserve successful derivation behavior when the keys match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef52e23a-eaf6-4109-95aa-6f1101854474
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlpackages/rs-platform-wallet-ffi/src/identity_keys_from_mnemonic.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/provider_key_at_index.rspackages/rs-platform-wallet-ffi/src/wallet_restore_types.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/wallet/provider_key_at_index.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift
…ways-on operator key cross-check Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Both CodeRabbit nitpicks addressed in 8b68f98:
🤖 Addressed by Claude Code |
QuantumExplorer
added a commit
to dashpay/rust-dashcore
that referenced
this pull request
Jul 14, 2026
…tform-node at index) (#881) * feat(key-wallet): gate-free provider-key derivation entry points (#880) Add wallet-state-agnostic entry points for deriving provider keys at an index, with no is_watch_only gate in either direction: - BLSAccount::operator_public_key_at(index): legacy-scheme non-hardened public derivation off the stored account xpub; works on resident AND watch-only accounts. Returns the extended key so both the legacy and modern/IETF serializations are obtainable. Rejects non-operator accounts so a mixed-up lookup fails loudly. - BLSAccount::operator_private_key_at(seed, network, index): re-derives the full DIP-3 operator path from the raw BIP39 seed in the legacy BLS scheme, then the non-hardened child; needs no account state. - EdDSAAccount::platform_node_key_at(seed, network, index): SLIP-0010 hardened derivation of the platform node key from the raw seed. The existing convenience wrappers are gated on is_watch_only in opposite directions (public derivation requires watch-only, seed derivation requires non-watch-only), so no single upstream API served both resident and restored/external-signable wallets — forcing downstream consumers to re-compose low-level primitives (see dashpay/platform#4120 and the stale derivation bug behind #878/#879). New tests pin the entry points to the existing dashbls / SLIP-0010 golden vectors and assert they behave identically on resident and watch-only accounts. Closes #880 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: pin sanitizer jobs to nightly-2026-07-13 (rustc ICE on latest nightly) The 2026-07-14 nightly (rustc daf2e5e18) ICEs while expanding the libtest harness for dash-spv-ffi tests ("attribute is missing tokens: rustc_test_entrypoint_marker" in rustc_ast/src/attr/mod.rs), breaking the Address Sanitizer job for every PR. Pin ASAN/TSAN to the last known-good nightly until a fixed nightly ships. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Contributor
✅ Action performedReview finished.
|
…er via rust-dashcore#881 Bumps the pin to bc6cbf96 picking up the gate-free provider-key API (dashpay/rust-dashcore#881, from our #880): operator_public_key_at / operator_private_key_at / platform_node_key_at. All derivation composition in provider_key_at_index.rs is deleted — platform now resolves wallets, calls upstream, and maps types. The private-reveal keeps an always-on consistency check (secret's pubkey must equal the xpub-derived one; the two upstream paths derive from independent sources and don't verify each other). Provider key accounts are also routed through the address-pool restore so their persisted rows rehydrate the in-memory pools at load (used-flags and beyond-gap indices survive restarts). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-platform-wallet-ffi/src/persistence.rs (1)
3229-3327: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSolid fix for the funds-only gap — consider adding restore-path coverage.
Unifying funds and provider-key accounts via
managed_account_type_mut()correctly closes the gap where a persisted address pool targetingProviderOwnerKeys/ProviderVotingKeys/ProviderOperatorKeys/ProviderPlatformKeyswas previously dropped. The single-match structure (vs. a funds-then-provider fallback) is a reasonable way to satisfy the borrow checker here, as the comment explains.If not already covered elsewhere in this test module (lines 4601-5076 weren't in the provided context), consider adding a
build_wallet_start_statetest that persists acore_address_poolsrow targeting one of the four provider account types and asserts the used/highest-index state round-trips intowallet_info.accounts.provider_*.🤖 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 `@packages/rs-platform-wallet-ffi/src/persistence.rs` around lines 3229 - 3327, Add restore-path coverage in the build_wallet_start_state test suite by persisting a core_address_pools entry targeting a provider account type such as ProviderOwnerKeys, then assert that its used and highest-index state is restored to the corresponding wallet_info.accounts.provider_* field. Reuse the existing setup and assertions for the other provider variants if practical, while preserving current funds-account coverage.packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs (1)
508-582: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a negative-path test for the seed↔xpub cross-check.
This test (and the guard it exercises at lines 340-355) only asserts the matching case (
sk0.public_key().to_bytes() == xpub0.public_key.to_bytes(), and same for testnet). The guard's entire purpose is to reject a mismatched pair with aKeyDerivationerror — that path is untested, so a future change that weakens or removes the check wouldn't be caught here.Consider adding a case that feeds a seed from a different mnemonic/account (or a hand-crafted mismatched xpub) into
derive_provider_key_at_index(Operator, .., include_private: true)and asserts it returnsErr(PlatformWalletError::KeyDerivation(_)).🤖 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 `@packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs` around lines 508 - 582, Extend operator_keys_match_dashbls_reference_and_cross_check with a mismatched seed/xpub case that invokes derive_provider_key_at_index for Operator with include_private enabled, using a seed from a different mnemonic or otherwise incompatible account data. Assert the call returns Err(PlatformWalletError::KeyDerivation(_)), while preserving the existing matching-path assertions for mainnet and testnet.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/persistence.rs`:
- Around line 3229-3327: Add restore-path coverage in the
build_wallet_start_state test suite by persisting a core_address_pools entry
targeting a provider account type such as ProviderOwnerKeys, then assert that
its used and highest-index state is restored to the corresponding
wallet_info.accounts.provider_* field. Reuse the existing setup and assertions
for the other provider variants if practical, while preserving current
funds-account coverage.
In `@packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs`:
- Around line 508-582: Extend
operator_keys_match_dashbls_reference_and_cross_check with a mismatched
seed/xpub case that invokes derive_provider_key_at_index for Operator with
include_private enabled, using a seed from a different mnemonic or otherwise
incompatible account data. Assert the call returns
Err(PlatformWalletError::KeyDerivation(_)), while preserving the existing
matching-path assertions for mainnet and testnet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2d6d2ec-4ca6-43da-8193-29e78f6990d9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlpackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/wallet/provider_key_at_index.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
…ve path Extracts restore_core_address_pools and the operator cross-check into testable helpers (behavior unchanged) and adds: a round-trip test proving a persisted ProviderOwnerKeys pool row rehydrates used-flags and beyond-gap indices at load, and a mismatched-seed test proving the private-reveal guard returns KeyDerivation instead of a key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Both CodeRabbit test nitpicks addressed in the latest commit:
All 430 tests green. 🤖 Addressed by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4120 +/- ##
=========================================
Coverage 87.42% 87.42%
=========================================
Files 2643 2643
Lines 333632 333632
=========================================
Hits 291684 291684
Misses 41948 41948
🚀 New features to boost your workflow:
|
Every derived-key value on the operator and platform-node account screens now renders as paired copyable "(Hex)" / "(Base64)" rows (same bytes, second encoding) so keys can be cross-checked against wallets that display base64, e.g. dashwallet-ios. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
✅ Action performedReview finished.
|
thepastaclaw
suggested changes
Jul 14, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The raw-seed provider derivation and legacy BLS ownership fixes are correct, and the five targeted provider-key tests pass. The current head still has five blocking lifecycle and persistence defects affecting restored provider keys, masternode ownership, claims, and wallet deletion, plus one C-ABI output-contract issue.
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— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 5 blocking | 🟡 1 suggestion(s)
5 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 `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:2913-2922: Preserve typed provider keys when restoring address pools
The new provider-account arms restore BLS operator and Ed25519 platform-node pools through `CoreAddressEntryFFI`, whose 33-byte public-key field can represent only compressed ECDSA keys. `build_core_address_entry_ffi` consequently emits `has_public_key = false` for BLS and EdDSA entries, and `address_info_from_ffi` restores them as `public_key: None`. `restore_address_pool` then overwrites any prederived entry at that index. The pinned key-wallet requires `PublicKeyType::BLS` or `PublicKeyType::EdDSA` when matching and consuming these keys, so restored provider entries fail after relaunch. Extend the ABI with a key-type discriminator and enough storage for 48-byte BLS keys, or preserve/rederive the correctly typed key during restore. The new round-trip test covers only the ECDSA-based provider-owner account and does not exercise either affected type.
In `packages/rs-platform-wallet/src/manager/accessors.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/accessors.rs:922-927: Build platform-node ownership from the persisted key batch
This loop calls platform-node derivation without a resolved seed. Registration persists a 20-key platform-node batch and then downgrades the wallet to external-signable, while restored wallets are also seedless. The first derivation therefore fails and leaves `platform_index` empty during the normal lifecycle. Swift subsequently overwrites `PersistentMasternode.platformInWallet` with this false result instead of matching the durable `derivedPlatformNodeKeys` batch, so wallet-owned platform node IDs are never recognized. Build the ownership index from the persisted batch or provide the same resolver-backed seed path used by explicit provider-key derivation.
- [BLOCKING] packages/rs-platform-wallet/src/manager/accessors.rs:888-890: Scan operator ownership through the pool's high-water mark
The ownership index treats 20 as a permanent maximum, although BLS operator children are non-hardened and can be derived beyond the initial window from the account xpub. This PR also restores provider pools and their beyond-gap indices. Any operator key at index 20 or later is omitted from `operator_index`, so an otherwise valid wallet-owned masternode is reported as external. Derive through the restored/generated pool's highest index or build the index directly from its typed entries.
In `packages/rs-platform-wallet-ffi/src/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/wallet.rs:374-382: Keep Claim unavailable while withdrawal is a stub
The Swift masternode view presents an enabled Claim action for eligible balances and sends confirmed claims through this function, but this implementation unconditionally returns `ErrorWalletOperation` without fetching, signing, or broadcasting anything. Every user-confirmed claim is therefore guaranteed to fail. Hide or disable the action until the verified signer path is implemented, or complete the withdrawal orchestration before exposing it.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/wallet.rs:220-231: Initialize masternode outputs before fallible lookup
The public C function documents null and zero outputs when the wallet is not found, but invalid-handle and unknown-wallet paths return through `unwrap_option_or_return!` before writing either output. Swift initializes its locals, but other C callers can retain stale pointer and count values despite the documented contract. Establish the empty result immediately after validating the pointers.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3703-3731: Delete persisted masternodes with their wallet
`PersistentMasternode` stores only a raw `walletId` and has no relationship through which deleting `PersistentWallet` could cascade. This deletion path explicitly purges other raw-ID tables but never fetches or deletes masternode rows, and `MasternodeSync` intentionally declines to prune when a fresh aggregate is empty. Deleting and reimporting the deterministic wallet ID can therefore resurrect stale masternode and ownership state indefinitely. Purge `PersistentMasternode` rows for the deleted wallet ID alongside the other raw-ID records.
…hip, node id, restore & lifecycle fixes Bumps the pin for rust-dashcore #882 (operator gap-limit extension), #884 (platform node id = Tenderdash SHA256[0..20], fixing our #883) and #885 (dedicated PlatformNodeId newtype). Addresses the review blockers on the masternode feature: - Ownership maps are rebuilt from the managed provider pools: operator scans through the pool's high-water mark (not a hardcoded 20-window) so #882/restored beyond-gap keys match; platform node ownership reads the persisted EdDSA batch with node ids recomputed under #884 (the lifecycle is seedless, so seed-derivation never fired). - Platform-node key batch is threaded restore→FFI→pool as typed EdDSA entries; MasternodeSync never downgrades a true ownership flag on a transient absent signal. - Address-pool restore keeps prederived typed BLS/EdDSA keys instead of clobbering them with the ECDSA-shaped FFI row. - Claim button gated off (withdrawal is still a stub); wallet deletion purges PersistentMasternode; list_masternodes writes empty outputs before fallible lookups. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Addressed the remaining review blockers (the body-only findings) in c2797b7, alongside the pin bump to rust-dashcore 2045cd29 (#882/#884/#885):
Verified: 🤖 Addressed by Claude Code |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 3770-3783: Wrap the PersistentMasternode fetch-and-delete block in
the existing isLastNetworkRow condition, matching the sibling TXO, PendingInput,
and AssetLock cleanup. Keep the walletId predicate and deletion behavior
unchanged when this is the final network row, while preserving shared masternode
rows when another network row remains.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d804e647-6476-4faa-9d59-333a48681535
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlpackages/rs-platform-wallet-ffi/src/core_wallet_types.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/wallet.rspackages/rs-platform-wallet-ffi/src/wallet_restore_types.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/spv/peers.rspackages/rs-platform-wallet/src/wallet/provider_key_at_index.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/MasternodeSync.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/MasternodeDetailView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
…hex-only The platform-node private reveal now offers "Tenderdash Node Key (Base64)" — base64(priv32 ‖ pub32), the exact blob dashmate's "Enter Ed25519 node key" prompt validates — and the node id row drops its base64 sibling (node ids are conventionally hex). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 14, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
All six indexed prior findings are fixed at the exact head. Two in-scope blocking defects remain: same-block provider updates are aggregated in txid order instead of block order, and persisted platform ownership cannot be cleared after an on-chain key rotation. Static formatting and diff checks passed; targeted Rust tests could not start because the pinned rust-dashcore revision was unavailable offline.
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— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
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 `packages/rs-platform-wallet/src/manager/accessors.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/accessors.rs:853-866: Preserve block order when aggregating provider updates
The BTreeMap deduplicates provider records by txid and `into_values()` subsequently feeds them to `aggregate_masternodes` in txid order. That aggregator resolves mutable fields with `height >= previous_height`, so equal-height updates overwrite one another according to txid ordering. Dash Core's `RebuildListFromBlock` applies provider transactions sequentially in `block.vtx` order and does not prohibit multiple updates for the same proTxHash in one block. Consequently, two valid same-block ProUpServ or ProUpReg transactions can leave this wallet showing an earlier service address, operator key, voting key, payout script, or platform node id as current. Retain transaction position alongside height and compare `(height, position)` when selecting the latest update. This behavior entered the cumulative PR range in ccaca2818d5ebf11a96a309662e7abe8a8ba873b.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/MasternodeSync.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Services/MasternodeSync.swift:101-105: Clear platform ownership after an on-chain key rotation
Once an existing row has `platformInWallet == true`, this condition rejects every later definitive `false`. A valid ProUpServ transaction can rotate the evonode's platform node id from a wallet-owned Ed25519 key to an external key; Rust then reports the new node id and `platformInWallet == false`, but Swift updates `platformNodeId` while retaining the old ownership flag and derivation index. The UI therefore claims ownership of a key the wallet does not hold. Preserve transient restore behavior by exposing a checked/unknown state over the FFI, or distinguish an unchanged-key transient miss from a changed-key definitive non-match before retaining the prior value. This behavior entered the cumulative PR range in c2797b7a9b352319e374f0382770c81aac44d67f.
…latform ownership Review round for the two blocking findings: - aggregate_masternodes now processes provider txs in ascending (height, in-block position) so per-field latest-wins matches Core's block.vtx application order instead of the caller's BTreeMap txid order. ProviderMasternodeTxs threads the position through; upstream retained records don't expose an in-block index yet (rust-dashcore#890), so the live caller passes 0 and same-block ties fall back to feed order until that lands. Both-feed-orders test pins the mechanism. - Platform-node ownership is now a tri-state: a new platform_ownership_checked FFI flag reports whether the derived platform-node index had entries to compare against. When checked, MasternodeSync writes platformInWallet verbatim (true OR false), so an on-chain rotation to an external key clears stale ownership; when unchecked (seedless restore before key-batch rehydration), the prior value is retained. A changed platformNodeId also overwrites, as definitive rotation evidence. Node-id byte order stays as decoded wire bytes on purpose — the canonicalization fix belongs upstream (rust-dashcore#887); reversing platform-side would double-reverse once that lands via a pin bump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Both blockers addressed in cc4cbcb: Same-block provider-update ordering ( One honest caveat: upstream retained records don't expose an in-block index — Platform-ownership rotation clearing ( Verification: |
…byte order rust-dashcore#889 (fixes #887): PlatformNodeId::consensus_decode now normalizes the wire's reversed uint160-internal bytes to the canonical Tenderdash SHA256(pubkey)[..20] order at the consensus boundary (and consensus_encode reverses back). Payload-sourced node ids therefore display forward and match the wallet's derived platform-node ownership index directly — the platform-side reversal this PR deliberately avoided is now unnecessary by construction. Comments at the aggregate arms updated accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
rust-dashcore#887 is now fixed upstream (rust-dashcore#889) and pinned here in 4ac9108 (rev 7221d866): |
thepastaclaw
suggested changes
Jul 14, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes the platform-node byte-order blocker by pinning rust-dashcore 7221d866, whose consensus boundary converts wire-order IDs to canonical Tenderdash order. One carried-forward blocker remains: production records still lack in-block positions, so same-block provider updates resolve in txid order instead of block order. No new latest-delta finding was found; formatting, diff checks, 155 FFI tests, 421 wallet tests, and 12 Drive decoder tests passed.
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 (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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 `packages/rs-platform-wallet/src/manager/accessors.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/accessors.rs:870-883: Preserve block order when aggregating provider updates
Provider records are deduplicated in a txid-keyed BTreeMap and every live record receives `position = 0`. The stable `(height, position)` aggregation sort therefore leaves same-block ties in txid order rather than Dash Core's `block.vtx` order. Multiple same-block ProReg, ProUpServ, or ProUpReg transactions can consequently leave an earlier service address, operator key, voting key, payout script, or platform node ID as current. Thread the actual in-block transaction index from block processing into the retained transaction context.
…ernode aggregation Bump rust-dashcore to 38c689d9 (#891, fixes #890): block processing now stamps each retained record's BlockInfo with its block.vtx index, and chainlock promotion preserves it. - provider_masternode_txs_blocking reads block_info.position() instead of the previous position = 0 placeholder, so same-block provider updates aggregate in Core's apply order. - The position round-trips persistence: TransactionRecordFFI and PersistentTransaction carry block_position/has_block_position, and the provider special-tx staging path restores it onto the rebuilt record's BlockInfo. Rows persisted before the field existed restore as None and fall back to feed order among themselves. - Round-trip test covers both the flagged and pre-field cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Reviewed |
shumkov
added a commit
that referenced
this pull request
Jul 14, 2026
…y-invitations Both sides had bumped rust-dashcore past the PlatformNodeId newtype (#885): v4.1-dev's #4120 pinned 38c689d9 (a descendant of this branch's ac8828c9, adding #889 canonical byte order + #891 tx-record positions), so the dependency block resolves to v4.1-dev's newer rev — git's auto-merge had duplicated the whole [workspace.dependencies] table, so it was reconciled by hand back to a single block. peers.rs and core_wallet_types.rs take v4.1-dev's variants of the same PlatformNodeId adaptations both sides had made independently. persistence.rs: #4120 extracted the load-side pool-restore loop into restore_core_address_pools() with funds + provider-key routing, but the extraction was based on v4.1-dev's copy and so dropped this branch's asset-lock funding-account arms (identity registration / top-up / invitation / address top-up) — the voucher-key-reuse restore fix. The merge takes the helper and re-adds those six arms (with the security rationale comment); the FFIPersister durability attestation from the round-5 fix is preserved. Post-merge: workspace check + clippy --all-features + fmt clean; platform-wallet 460 and platform-wallet-ffi 169 tests green (both suites now include the base's new provider-key restore tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 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.
Issue being fixed or feature implemented
key-wallet derived wrong masternode operator BLS keys and platform node ids (dashpay/rust-dashcore#878) — they never matched DashSync/on-chain registrations, so #4116's operator/platform ownership matching could never succeed. The fix merged upstream as dashpay/rust-dashcore#879; this picks it up and adds the dual BLS serialization display users expect from dashwallet-ios.
What was done?
337f6ccc→56a40788: chore(wasm-dpp): proper identifier buffer inheritance #879 (BLS operator / Ed25519 platform-node derivation per DashSync/dashbls — drops the spurious0x00in hardened BLS children, uses legacy G1 serialization in the HD chain, feeds the bip39 seed directly instead of the secp256k1 hybrid with double path application), fix: possible overflow issues #877 (mainnet sync floors at the HD/BIP39 activation height), feat(wasm-dpp): state_transition_fee_validator binding and tests #874 (masternode seed refresh).ProviderDerivedKey(rs-platform-wallet) now carrieslegacy_public_key_bytesfor BLS operator keys (via chore(wasm-dpp): proper identifier buffer inheritance #879'sto_bytes_legacy()/public_key_bytes_legacy()— Rust-side only, no Swift crypto), threaded throughProviderKeyAtIndexFFIand the Swift DTO; the Provider Operator Keys account screen renders both "BLS Public Key" (modern,82…) and "BLS Public Key (Legacy)" (02…) as copyable rows. Ed25519 platform-node keys are unaffected (no legacy row).Note: wallets created before this bump persisted provider account roots derived under the broken scheme — a wallet delete + re-import is needed for the corrected keys (the old keys never corresponded to anything on-chain). Follow-up (deliberately excluded): the masternode detail page's raw on-chain operator key row could also show both forms, but that requires a version-aware re-encode (ProRegTx v1 payloads carry legacy encoding, v2+ modern) threaded through the aggregation.
Follow-up rounds on this PR:
provider_key_at_index.rsto a pure adapter over the gate-free upstream API (operator_public_key_at/operator_private_key_at/platform_node_key_at) — platform holds zero derivation knowledge now. An always-on consistency check guards the private-reveal (the upstream public and private paths derive from independent sources)._ => Nonearm).How Has This Been Tested?
cargo check --workspace --all-targetsandcargo test -p platform-wallet-ffi(152 tests) green against the new pin; clippy clean.build_ios.sh --target sim(warnings-as-errors) green, cbindgen headers regenerated with the new FFI field.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit