feat(platform-wallet): watch-only rehydration from persistor (seedless load)#3692
Open
Claudius-Maginificent wants to merge 14 commits into
Open
feat(platform-wallet): watch-only rehydration from persistor (seedless load)#3692Claudius-Maginificent wants to merge 14 commits into
Claudius-Maginificent wants to merge 14 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPersisted wallet startup is redesigned around seedless watch-only reconstruction. ChangesWatch-only wallet rehydration and load outcome
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
7 tasks
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
b573fca to
b7508a0
Compare
6 tasks
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-05-22T10:49:18.857Z |
Collaborator
Author
🔁 Seedless-load rework landed —
|
Claudius-Maginificent
changed the title
6 tasks
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
0e92cb4 to
f57b117
Compare
Claudius-Maginificent
changed the title
Base automatically changed from
feat/platform-wallet-storage-secrets
to
feat/platform-wallet-sqlite-persistor
May 29, 2026 11:28
Base automatically changed from
feat/platform-wallet-sqlite-persistor
to
v3.1-dev
June 9, 2026 08:13
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3692 +/- ##
==========================================
Coverage 87.17% 87.18%
==========================================
Files 2629 2632 +3
Lines 327221 327563 +342
==========================================
+ Hits 285265 285592 +327
- Misses 41956 41971 +15
🚀 New features to boost your workflow:
|
lklimek
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Contributor
|
@coderabbitai review all |
Collaborator
|
✅ Review complete (commit 61c9fd7) |
Contributor
|
✅ Action performedFull review finished. |
thepastaclaw
suggested changes
Jun 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
I verified the reported wallet rehydration findings against the PR head and surrounding call paths. Three correctness issues are in scope for this restore/storage work: the SQLite account-registration key is lossy, repeated loads collide with already-registered wallets, and the FFI load path still turns malformed account xpub rows into whole-load failures before the manager can skip them per wallet.
🔴 6 blocking | 🟡 1 suggestion(s)
Findings not posted inline (3)
These findings could not be anchored to the current diff, but they are still part of this review.
- [BLOCKING]
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:107-111: Account registration primary key collapses distinct account variants — The writer upsertsaccount_registrationson(wallet_id, account_type, account_index), and the migration declares the same primary key. That key is narrower thankey_wallet's account identity:PlatformPaymentis keyed by(account, key_class), and DashPay receiving/external accounts are k... - [BLOCKING]
packages/rs-platform-wallet/src/manager/load.rs:159-168: Repeated restore fails on wallets already in the manager —load_from_persistoralways callswm.insert_wallet(wallet, platform_info)for every persisted wallet. The underlyingkey-wallet-managerimplementation returnsWalletExistswhen the wallet id is already registered, and this code maps that to a hardWalletCreationerror that aborts the loa... - [BLOCKING]
packages/rs-platform-wallet-ffi/src/persistence.rs:1543-1545: Malformed FFI account xpub aborts the whole restore — The FFI persister builds eachWalletRestoreEntryFFIwithbuild_wallet_start_state(entry)?before returningClientStartStatetoPlatformWalletManager::load_from_persistor. Inside that helper, malformedaccount_xpub_bytesreturnsPersistenceErrorwhile decoding the account xpub, so one...
🤖 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-storage/src/sqlite/schema/accounts.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:107-111: Account registration primary key collapses distinct account variants
The writer upserts `account_registrations` on `(wallet_id, account_type, account_index)`, and the migration declares the same primary key. That key is narrower than `key_wallet`'s account identity: `PlatformPayment` is keyed by `(account, key_class)`, and DashPay receiving/external accounts are keyed by `(index, user_identity_id, friend_identity_id)`. The blob stores the full `AccountRegistrationEntry`, but inserting two such variants with the same label and numeric index overwrites the first row before `load_state` can reconstruct the manifest. A restored watch-only wallet can therefore lose accounts and their address/platform state. Persist enough discriminator columns, or an unambiguous serialized account-type key, and include it in the conflict key/order.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:107-111: Account registration primary key collapses distinct account variants
The writer upserts `account_registrations` on `(wallet_id, account_type, account_index)`, and the migration declares the same primary key. That key is narrower than `key_wallet`'s account identity: `PlatformPayment` is keyed by `(account, key_class)`, and DashPay receiving/external accounts are keyed by `(index, user_identity_id, friend_identity_id)`. The blob stores the full `AccountRegistrationEntry`, but inserting two such variants with the same label and numeric index overwrites the first row before `load_state` can reconstruct the manifest. A restored watch-only wallet can therefore lose accounts and their address/platform state. Persist enough discriminator columns, or an unambiguous serialized account-type key, and include it in the conflict key/order.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:159-168: Repeated restore fails on wallets already in the manager
`load_from_persistor` always calls `wm.insert_wallet(wallet, platform_info)` for every persisted wallet. The underlying `key-wallet-manager` implementation returns `WalletExists` when the wallet id is already registered, and this code maps that to a hard `WalletCreation` error that aborts the load batch. A second `load_from_persistor` call on the same manager, or a restore after the same wallet has already been registered in memory, fails instead of treating the persisted wallet as already satisfied. Handle `WalletExists` or pre-check the existing wallet id so repeated restore is idempotent and still proceeds to the remaining persisted wallets.
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:159-168: Repeated restore fails on wallets already in the manager
`load_from_persistor` always calls `wm.insert_wallet(wallet, platform_info)` for every persisted wallet. The underlying `key-wallet-manager` implementation returns `WalletExists` when the wallet id is already registered, and this code maps that to a hard `WalletCreation` error that aborts the load batch. A second `load_from_persistor` call on the same manager, or a restore after the same wallet has already been registered in memory, fails instead of treating the persisted wallet as already satisfied. Handle `WalletExists` or pre-check the existing wallet id so repeated restore is idempotent and still proceeds to the remaining persisted wallets.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:1543-1545: Malformed FFI account xpub aborts the whole restore
The FFI persister builds each `WalletRestoreEntryFFI` with `build_wallet_start_state(entry)?` before returning `ClientStartState` to `PlatformWalletManager::load_from_persistor`. Inside that helper, malformed `account_xpub_bytes` returns `PersistenceError` while decoding the account xpub, so one corrupt SwiftData account row makes the entire load callback fail. That bypasses the new per-wallet skip contract documented on the manager and FFI surfaces, and prevents otherwise valid persisted wallets from loading. Convert per-entry account decode failures into a skipped wallet entry or an equivalent row-local corruption marker that the manager can report through `LoadOutcome::skipped`.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:1543-1545: Malformed FFI account xpub aborts the whole restore
The FFI persister builds each `WalletRestoreEntryFFI` with `build_wallet_start_state(entry)?` before returning `ClientStartState` to `PlatformWalletManager::load_from_persistor`. Inside that helper, malformed `account_xpub_bytes` returns `PersistenceError` while decoding the account xpub, so one corrupt SwiftData account row makes the entire load callback fail. That bypasses the new per-wallet skip contract documented on the manager and FFI surfaces, and prevents otherwise valid persisted wallets from loading. Convert per-entry account decode failures into a skipped wallet entry or an equivalent row-local corruption marker that the manager can report through `LoadOutcome::skipped`.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API
`pub mod rehydrate` exposes `manager::rehydrate::apply_persisted_core_state` to downstream crates even though it mutates `ManagedWalletInfo` according to load-path-specific assumptions such as first-funds-account UTXO routing and bounded address-pool probing. The only external uses are storage integration tests, so making this part of the public SDK API hardens internal restore details that should be free to change. Keep the module/function crate-private and move those cross-crate assertions behind an internal test feature or into `rs-platform-wallet` tests.
lklimek
reviewed
Jun 29, 2026
…persistor [#3692, clean on v3.1-dev] Squashed net-diff of feat/platform-wallet-rehydration onto v3.1-dev base (1653b89). Includes all merged commits: • changeset: CoreChangeSet, ClientWalletStartState, addresses_derived wiring • rehydrate: seedless watch-only wallet rebuild + apply_persisted_core_state • load_outcome: LoadOutcome / SkipReason / CorruptKind • manager/load: load_from_persistor implementation • manager/mod: PlatformWalletManager wiring • events: PlatformEvent + on_wallet_skipped_on_load concrete handler • error: RehydrateRowError relocated from manager::rehydrate • core_bridge: warn_if_non_default_account generalised to &[T] slice • FFI: persistence + manager bindings • Swift: PlatformWalletManager load() bridging • tests: rehydration_load integration suite • misc: .cargo/audit.toml, .gitignore fmt + clippy (-D warnings) + cargo test: all pass. Tree verified byte-for-byte identical to feat/platform-wallet-rehydration HEAD. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
52cdad9 to
83f7d4f
Compare
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-platform-wallet/src/manager/identity_sync.rs (1)
148-208: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winDo not let an exiting old loop erase the new loop’s cancel token.
Line 422 can execute after
stop()has taken the old token and a subsequentstart()has stored a new one. Clearing the slot unconditionally makes the live identity-sync loop unobservable/uncancellable and can allow duplicate background loops.Proposed fix
pub struct IdentitySyncManager<P> where P: PlatformWalletPersistence + 'static, { @@ /// Cancel token for the background loop, if running. background_cancel: StdMutex<Option<CancellationToken>>, + background_generation: AtomicU64, interval_secs: AtomicU64, @@ sdk, persister, background_cancel: StdMutex::new(None), + background_generation: AtomicU64::new(0), interval_secs: AtomicU64::new(DEFAULT_SYNC_INTERVAL_SECS), @@ } let cancel = CancellationToken::new(); + let my_gen = self + .background_generation + .fetch_add(1, Ordering::AcqRel) + + 1; *guard = Some(cancel.clone()); @@ - if let Ok(mut guard) = this.background_cancel.lock() { - *guard = None; + if let Ok(mut guard) = this.background_cancel.lock() { + if this.background_generation.load(Ordering::Acquire) == my_gen { + *guard = None; + } }Also applies to: 392-422
🤖 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/manager/identity_sync.rs` around lines 148 - 208, The background loop teardown in IdentitySyncManager::start/stop is clearing the shared cancel-token slot unconditionally, which can wipe out a newer token stored by a subsequent start(). Update the loop-exit cleanup near the stop handling so it only clears background_cancel when it still matches the token owned by that loop, leaving a newer loop’s token intact. Use the existing stop/start/background_cancel/CancellationToken flow in IdentitySyncManager to guard the compare-and-clear logic.packages/rs-platform-wallet/src/manager/shielded_sync.rs (1)
132-169: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winKeep cleanup from clearing a newer background loop’s cancel token.
Line 251 can run after
stop()returns and a laterstart()has already stored a fresh token. Unconditionally settingbackground_cancel = Nonethen loses the new token, so laterstop()cannot cancel the running loop and anotherstart()can spawn duplicates. Restore a generation/token ownership guard and check it while holding the mutex.Proposed fix
pub struct ShieldedSyncManager { @@ /// Cancel token for the background loop, if running. background_cancel: StdMutex<Option<CancellationToken>>, + background_generation: AtomicU64, interval_secs: AtomicU64, @@ event_manager, coordinator_slot, background_cancel: StdMutex::new(None), + background_generation: AtomicU64::new(0), interval_secs: AtomicU64::new(DEFAULT_SYNC_INTERVAL_SECS), @@ } let cancel = CancellationToken::new(); + let my_gen = self + .background_generation + .fetch_add(1, Ordering::AcqRel) + + 1; *guard = Some(cancel.clone()); @@ - if let Ok(mut guard) = this.background_cancel.lock() { - *guard = None; + if let Ok(mut guard) = this.background_cancel.lock() { + if this.background_generation.load(Ordering::Acquire) == my_gen { + *guard = None; + } }Also applies to: 215-252
🤖 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/manager/shielded_sync.rs` around lines 132 - 169, The background-loop cleanup in `ShieldedSyncManager::stop`/`cleanup` is clearing `background_cancel` even when a newer `start` has already installed a fresh `CancellationToken`. Update the token lifecycle so cleanup only removes the token it created, using a generation or token-identity guard while holding the `StdMutex` around `background_cancel`. Ensure the `start`, `stop`, and cleanup paths all compare ownership before setting the slot to `None`, so a later loop’s token is preserved and can still be canceled.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/manager/rehydrate.rs (1)
174-178: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPre-index spent outpoints before filtering.
This is currently
new_utxos × spent_utxos; persisted wallets with larger histories can make startup restore unnecessarily quadratic. Build a set of spent outpoints once, then filter against it.🤖 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/manager/rehydrate.rs` around lines 174 - 178, The unspent UTXO selection in rehydrate_wallet currently scans core.new_utxos against core.spent_utxos for each item, making startup restore quadratic for large histories. Update the filtering logic in rehydrate_wallet to precompute a set of spent outpoints once, then use that set when building the unspent Vec<&key_wallet::Utxo> so the lookup is O(1) per UTXO.
🤖 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/rs-platform-wallet/src/manager/rehydrate.rs`:
- Around line 8-10: The module docs in rehydrate.rs still describe wrong-seed
validation as deferred to separate FFI work, which is now outdated. Update the
comment near the top of the file to match the current contract by stating that
wrong-seed validation is handled in resolver-backed signing entrypoints, while
keeping the note that load/rehydration itself does not touch the seed; use the
existing rehydrate module documentation text as the place to align this wording.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/manager/identity_sync.rs`:
- Around line 148-208: The background loop teardown in
IdentitySyncManager::start/stop is clearing the shared cancel-token slot
unconditionally, which can wipe out a newer token stored by a subsequent
start(). Update the loop-exit cleanup near the stop handling so it only clears
background_cancel when it still matches the token owned by that loop, leaving a
newer loop’s token intact. Use the existing
stop/start/background_cancel/CancellationToken flow in IdentitySyncManager to
guard the compare-and-clear logic.
In `@packages/rs-platform-wallet/src/manager/shielded_sync.rs`:
- Around line 132-169: The background-loop cleanup in
`ShieldedSyncManager::stop`/`cleanup` is clearing `background_cancel` even when
a newer `start` has already installed a fresh `CancellationToken`. Update the
token lifecycle so cleanup only removes the token it created, using a generation
or token-identity guard while holding the `StdMutex` around `background_cancel`.
Ensure the `start`, `stop`, and cleanup paths all compare ownership before
setting the slot to `None`, so a later loop’s token is preserved and can still
be canceled.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/manager/rehydrate.rs`:
- Around line 174-178: The unspent UTXO selection in rehydrate_wallet currently
scans core.new_utxos against core.spent_utxos for each item, making startup
restore quadratic for large histories. Update the filtering logic in
rehydrate_wallet to precompute a set of spent outpoints once, then use that set
when building the unspent Vec<&key_wallet::Utxo> so the lookup is O(1) per UTXO.
🪄 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: 6ad8195d-696b-4fae-83cc-2844ae7e9047
📒 Files selected for processing (20)
.cargo/audit.toml.gitignorepackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/client_wallet_start_state.rspackages/rs-platform-wallet/src/changeset/core_bridge.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/events.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/manager/identity_sync.rspackages/rs-platform-wallet/src/manager/load.rspackages/rs-platform-wallet/src/manager/load_outcome.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/manager/rehydrate.rspackages/rs-platform-wallet/src/manager/shielded_sync.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rspackages/rs-platform-wallet/tests/rehydration_load.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
✅ Files skipped from review due to trivial changes (3)
- .cargo/audit.toml
- .gitignore
- packages/rs-platform-wallet/src/changeset/changeset.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/rs-platform-wallet/src/lib.rs
- packages/rs-platform-wallet/src/manager/load_outcome.rs
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
- packages/rs-platform-wallet/src/events.rs
- packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs
- packages/rs-platform-wallet/src/wallet/apply.rs
- packages/rs-platform-wallet/src/manager/mod.rs
- packages/rs-platform-wallet/tests/rehydration_load.rs
- packages/rs-platform-wallet/src/manager/load.rs
- packages/rs-platform-wallet/src/changeset/core_bridge.rs
- packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs
- packages/rs-platform-wallet-ffi/src/persistence.rs
- packages/rs-platform-wallet-ffi/src/manager.rs
thepastaclaw
suggested changes
Jun 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: all three prior author-facing findings are STILL VALID at 83f7d4f: duplicate restore still hard-fails, malformed FFI account xpubs still abort the whole load before skipped-wallet handling, and manager::rehydrate remains public. New in-scope finding in the latest cumulative verification: the keyless FFI projection now drops restored core address-pool snapshots, so used spent addresses and derived empty gap-window state are not rehydrated. One additional agent finding about a missing wallet-list free callback is a real pre-existing hardening issue, but it is out of scope for this PR because Swift wires both callbacks and the current delta did not introduce or worsen that partial-configuration path.
🔴 5 blocking | 🟡 1 suggestion(s)
Findings not posted inline (2)
These findings could not be anchored to the current diff, but they are still part of this review.
- [BLOCKING]
packages/rs-platform-wallet/src/manager/load.rs:155-164: Repeated restore fails on wallets already in the manager —load_from_persistorstill callswm.insert_wallet(wallet, platform_info)for every persisted wallet and converts every insertion error into a hardWalletCreationfailure. The normal registration path mapskey_wallet_manager::WalletError::WalletExists(_)specially, and Swift documents launc... - [BLOCKING]
packages/rs-platform-wallet-ffi/src/persistence.rs:1543-1545: Malformed FFI account xpub aborts the whole restore —FFIPersister::loadstill builds each Swift-provided restore entry withbuild_wallet_start_state(entry)?before returningClientStartStateto the manager. Inside that helper, malformedaccount_xpub_bytesis decoded with?, so one corrupt SwiftData account row becomes a whole-load `Persis...
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [SUGGESTION] (deduped existing open thread)
packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API —pub mod rehydrateexposes load-path helpers such asmanager::rehydrate::apply_persisted_core_stateto downstream crates. These functions mutateManagedWalletInfounder restore-specific assumptions and should remain behindPlatformWalletManager::load_from_persistor; otherwise the SDK freez...
🤖 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/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:155-164: Repeated restore fails on wallets already in the manager
`load_from_persistor` still calls `wm.insert_wallet(wallet, platform_info)` for every persisted wallet and converts every insertion error into a hard `WalletCreation` failure. The normal registration path maps `key_wallet_manager::WalletError::WalletExists(_)` specially, and Swift documents launch restore as idempotent, but this load path does not handle a wallet that is already registered. Calling restore twice on the same manager, or restoring when one persisted wallet is already loaded, aborts the batch instead of treating that wallet as already present and continuing with the remaining rows.
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:155-164: Repeated restore fails on wallets already in the manager
`load_from_persistor` still calls `wm.insert_wallet(wallet, platform_info)` for every persisted wallet and converts every insertion error into a hard `WalletCreation` failure. The normal registration path maps `key_wallet_manager::WalletError::WalletExists(_)` specially, and Swift documents launch restore as idempotent, but this load path does not handle a wallet that is already registered. Calling restore twice on the same manager, or restoring when one persisted wallet is already loaded, aborts the batch instead of treating that wallet as already present and continuing with the remaining rows.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:1543-1545: Malformed FFI account xpub aborts the whole restore
`FFIPersister::load` still builds each Swift-provided restore entry with `build_wallet_start_state(entry)?` before returning `ClientStartState` to the manager. Inside that helper, malformed `account_xpub_bytes` is decoded with `?`, so one corrupt SwiftData account row becomes a whole-load `PersistenceError` before `PlatformWalletManager::load_from_persistor` can classify the wallet as `LoadOutcome::skipped`. This violates the PR's per-wallet corrupt-row contract and prevents otherwise valid wallets from loading.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3400-3415: Core address-pool restore is dropped by the keyless projection
`build_wallet_start_state` restores `entry.core_address_pools` into the temporary `wallet_info`, including used flags and derived addresses, but the returned `ClientWalletStartState` only forwards `new_utxos`, height watermarks, and `last_applied_chain_lock` through `CoreChangeSet`. The manager then rebuilds a fresh watch-only wallet and `apply_persisted_core_state` only marks addresses that currently have restored UTXOs. Any previously used address whose funds were spent, or any derived gap-window address without an unspent UTXO, comes back as unused after seedless launch and can be handed out again.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:1543-1545: Malformed FFI account xpub aborts the whole restore
`FFIPersister::load` still builds each Swift-provided restore entry with `build_wallet_start_state(entry)?` before returning `ClientStartState` to the manager. Inside that helper, malformed `account_xpub_bytes` is decoded with `?`, so one corrupt SwiftData account row becomes a whole-load `PersistenceError` before `PlatformWalletManager::load_from_persistor` can classify the wallet as `LoadOutcome::skipped`. This violates the PR's per-wallet corrupt-row contract and prevents otherwise valid wallets from loading.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API
`pub mod rehydrate` exposes load-path helpers such as `manager::rehydrate::apply_persisted_core_state` to downstream crates. These functions mutate `ManagedWalletInfo` under restore-specific assumptions and should remain behind `PlatformWalletManager::load_from_persistor`; otherwise the SDK freezes reconstruction details as public API instead of keeping them internal to the load boundary.
Contributor
|
@coderabbitai review all |
Contributor
|
✅ Action performedFull review finished. |
…ncrete handlers only (#3692 review) Remove `PlatformEventHandler::on_platform_event` (the generic backward-compat escape hatch) and `PlatformEventManager::on_platform_event` entirely. `on_wallet_skipped_on_load` now has a plain no-op default, matching the pattern used by every other concrete handler on the trait. `PlatformEvent` is kept: it is `pub`, re-exported from `lib.rs`, and not present in the FFI or Swift layer — no dead-code warning applies to public items, and removing it would be a needless churn of the public API. Not a breaking change vs v3.1-dev: `on_platform_event` was only ever on this branch (absent from origin/v3.1-dev). Doc comments in manager/load.rs and manager/mod.rs updated to point to `on_wallet_skipped_on_load` instead of the removed method/event wrapper. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…g-seed gate (#3692 review) The rehydrate module header and the rehydration_load test header both claimed the wrong-seed gate was "deferred to separate FFI work and is not part of this path." That gate now exists on the resolver-backed signing entrypoints (sign_with_mnemonic_resolver + the FFI resolver sign path). Reword to say wrong-seed validation lives there; the seedless load path never sees the seed. Docs-only, no behaviour change. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nt HashSet (#3692 review) apply_persisted_core_state filtered new_utxos against spent_utxos with a nested `any()`, making the unspent projection O(new × spent). Collect the spent outpoints into a HashSet once and do O(1) membership lookups — behaviour is identical (Copy OutPoint, Hash + Eq), just linear. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ehydrate (#3692 review) The FFI build_wallet_start_state decoded the persisted core address pools (used flags + derived addresses) into a temp wallet_info, but the keyless ClientWalletStartState forwarded only the account manifest + UTXO/height projection — the pool used-state was dropped. apply_persisted_core_state then marked addresses used ONLY from currently-unspent UTXOs, so a previously-used address whose funds were since spent came back marked unused and could be handed out again as a fresh receive address: an address-reuse privacy leak. Carry the used-state through: - Add ClientWalletStartState::used_core_addresses (Vec<Address>, empty default) — a flat snapshot of every pool-marked-used address. - Populate it in the FFI projection from the already-decoded pools. - apply_persisted_core_state now marks used the UNION of unspent-UTXO addresses + used_core_addresses (new param), deriving deep slots via the existing horizon walk. Renamed extend_pools_for_restored_utxos -> extend_pools_for_restored_addresses since it now resolves both sources. Empty used_core_addresses preserves the prior unspent-only behaviour, so the native/SQLite path is unchanged until #3968 wires its pool readers to populate this field (cross-PR follow-up; no regression). Also fixes the O(new x spent) unspent filter via an outpoint HashSet. Test: rehydration_restores_persisted_used_state_for_spent_out_address asserts an in-window and a deep spent-out address come back used, and that the empty-snapshot baseline does NOT mark them. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t wallet on load (#3692 review) load_from_persistor mapped EVERY insert_wallet error (including key_wallet_manager::WalletError::WalletExists) to a fatal WalletCreation + 'load break + full rollback. So a second load_from_persistor — or a load run while the wallet is already in memory — aborted the whole batch instead of being a no-op. Match WalletExists specifically and treat it as already-satisfied: record the wallet as loaded and `continue` to the next row. It was not inserted by this pass, so it stays out of the rollback set and a later hard-fail never evicts the pre-existing wallet. Mirrors the create-path idempotent handling in wallet_lifecycle. Test: rt_idempotent_repeat_restore loads the same persister twice and asserts the second call returns Ok with the wallet still present. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ting the batch (#3692 review) FFIPersister::load looped `build_wallet_start_state(entry)?`, so ONE corrupt SwiftData row (e.g. a malformed account_xpub that aborts decode) failed the ENTIRE load() — every wallet, every launch. The manager already documents per-wallet skip (LoadOutcome::skipped + on_wallet_skipped_on_load, returns Ok), but the FFI never reached it. Make the FFI loop per-entry resilient: on a per-row build failure record the wallet as skipped and continue. Errors from build_wallet_start_state are inherently per-row (decode/projection of THAT entry), so this never swallows a whole-load failure. The skip travels to the manager through a new ClientStartState::skipped channel (Vec<(WalletId, SkipReason)>, empty default); load_from_persistor folds it into LoadOutcome::skipped and fires on_wallet_skipped_on_load. Reason is CorruptPersistedRow{DecodeError} — PersistenceError's Display is structural (no row bytes / key material). Cross-PR: ClientStartState derives Default so #3968's `::default()` build still compiles; a destructure there needs `skipped: _` (follow-up). Test: rt_persister_skipped_folds_into_outcome asserts a persister-rejected row surfaces in LoadOutcome::skipped + fires the event while the healthy wallet still loads and the call returns Ok. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…stive (#3692 review) Semver hygiene for the new, unreleased load surface so future variants don't break downstream matches: add #[non_exhaustive] to SkipReason, CorruptKind, LoadOutcome (load_outcome.rs) and PlatformEvent (events.rs). Consequence: the FFI skip_reason_code match (a downstream crate) is no longer exhaustive over the now-non_exhaustive SkipReason/CorruptKind, so add catch-all arms mapping future variants to generic codes (199 corrupt kind, 200 skip reason) until the mapping is extended. matches!() in tests is unaffected (it carries an implicit wildcard). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dex pool extension on rehydrate (#3692 review) QA flagged that the existing real-manager rehydration test defaults the address-pool payload and is structurally blind to #1, and that the pool-DEPTH fix (dash-evo-tool#829 Bug 2 / PR #830) had no regression guard. Add two distinct, focused tests through apply_persisted_core_state: - rehydration_used_state_survives_spent_utxo (#1, address-reuse): builds a ClientWalletStartState whose in-window address received funds that were then SPENT (new_utxos cancelled by spent_utxos → zero balance) and routes used_core_addresses through the field. Asserts the in-window + a deep (idx 30) address come back marked USED even with zero balance, and that the empty-snapshot baseline does NOT mark them. Replaces the weaker no-UTXO variant so the used flag is proven independent of a live UTXO. - rt_deep_index_utxos_extend_pools_on_rehydration (DEPTH): unspent UTXOs on walkable ladders past the eager 0..=gap_limit window (external -> idx 84, internal -> idx 90). Asserts the deep slots are derived into their pools and Sum(per-address visible) == balance.total == Sum(persisted) — no deep-index undercount. Test-only; the production fix already exists. Also: drop the stale "(from wallet_metadata)" table reference on the ClientWalletStartState::network doc (backend-agnostic now). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
review) Remove rt_deep_index_utxos_extend_pools_on_rehydration: the deep-index pool-extension scenario is already guarded by the pre-existing rehydration_extends_pools_to_cover_deep_index_utxos and rehydration_coinjoin_single_pool_deep_index. The existing 30->60 horizon extension already exercises the recursive walk, so a deeper ladder added no new code path — pure duplication. Keeps the #1 address-reuse test (rehydration_used_state_survives_spent_utxo). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
Review-fix summary (pushed in
|
…eview) After the on_platform_event removal, the `PlatformEvent` enum had zero references repo-wide — events flow through the concrete `PlatformEventHandler` methods (`on_wallet_skipped_on_load`, etc.), not a dispatched enum. Remove the enum (and the `#[non_exhaustive]` just added to it) plus its `lib.rs` re-export. Its only variant, `WalletSkippedOnLoad`, went with it; the `on_wallet_skipped_on_load(wallet_id, &SkipReason)` handler and `SkipReason` itself stay. No imports orphaned — `SkipReason` and `WalletId` are still used by `PlatformEventHandler` / `PlatformEventManager`. Verified: `git grep PlatformEvent` over rs-platform-wallet, -ffi and swift-sdk is empty (only `PlatformEventHandler` / `PlatformEventManager` remain). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/manager/rehydrate.rs (1)
204-244: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve account attribution during core-state rehydration.
This always restores UTXOs and used-address state into the first funds-bearing account. For wallets with multiple funds accounts, rows belonging to later accounts are derived/marked against the wrong xpub, so their pool visibility and used flags can be lost. Carry account identity in the keyless start state and apply each UTXO/used address to its matching account; or fail closed for multi-funds-account snapshots until that mapping exists.
🤖 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/manager/rehydrate.rs` around lines 204 - 244, The rehydration logic in the core-state restore path is always applying UTXOs and used-address markings to the first funding account, which breaks multi-funds-account wallets. Update the `rehydrate` flow to preserve account identity from the snapshot/start state and route each restored UTXO and marked address to the matching account instead of using `all_funding_accounts_mut().next()`. If that mapping is not available yet, make `rehydrate` fail closed for multi-account snapshots rather than silently restoring into the wrong account.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/tests/rehydration_load.rs (1)
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename new tests to start with
should_.These test names are descriptive, but the current test guideline requires names to start with “should …”. As per coding guidelines, "
**/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'".Also applies to: 223-223, 270-270, 339-339
🤖 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/tests/rehydration_load.rs` at line 182, Rename the newly added tests so they follow the test naming guideline by starting with should_, updating the async test functions in rehydration_load.rs such as rt_idempotent_repeat_restore and the other mentioned test cases to more descriptive names that begin with should; keep the rest of each test body unchanged and ensure the renamed symbols remain consistent wherever they are referenced.Source: Coding guidelines
🤖 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/rs-platform-wallet/src/changeset/client_start_state.rs`:
- Around line 36-42: The `ClientStartState::is_empty()` logic is missing the
`skipped` field, so a state with only persister-rejected rows is treated as
empty even though `skipped` still needs to be reported. Update `is_empty()` in
`ClientStartState` to consider `skipped` alongside the other load-result
collections, using the `skipped: Vec<(WalletId, SkipReason)>` field as part of
the emptiness check. Also make the corresponding documentation/comment and any
related `LoadOutcome` handling consistent so a skipped-only result is never
collapsed into an empty load.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/manager/rehydrate.rs`:
- Around line 204-244: The rehydration logic in the core-state restore path is
always applying UTXOs and used-address markings to the first funding account,
which breaks multi-funds-account wallets. Update the `rehydrate` flow to
preserve account identity from the snapshot/start state and route each restored
UTXO and marked address to the matching account instead of using
`all_funding_accounts_mut().next()`. If that mapping is not available yet, make
`rehydrate` fail closed for multi-account snapshots rather than silently
restoring into the wrong account.
---
Nitpick comments:
In `@packages/rs-platform-wallet/tests/rehydration_load.rs`:
- Line 182: Rename the newly added tests so they follow the test naming
guideline by starting with should_, updating the async test functions in
rehydration_load.rs such as rt_idempotent_repeat_restore and the other mentioned
test cases to more descriptive names that begin with should; keep the rest of
each test body unchanged and ensure the renamed symbols remain consistent
wherever they are referenced.
🪄 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: 3241630a-897e-4cdf-b079-e2698f300be0
📒 Files selected for processing (12)
packages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/client_start_state.rspackages/rs-platform-wallet/src/changeset/client_wallet_start_state.rspackages/rs-platform-wallet/src/events.rspackages/rs-platform-wallet/src/manager/load.rspackages/rs-platform-wallet/src/manager/load_outcome.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/manager/rehydrate.rspackages/rs-platform-wallet/src/manager/wallet_lifecycle.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/tests/rehydration_load.rs
✅ Files skipped from review due to trivial changes (1)
- packages/rs-platform-wallet/src/wallet/platform_wallet.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs
- packages/rs-platform-wallet/src/manager/load_outcome.rs
- packages/rs-platform-wallet/src/manager/mod.rs
- packages/rs-platform-wallet/src/manager/load.rs
- packages/rs-platform-wallet-ffi/src/manager.rs
thepastaclaw
suggested changes
Jun 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: prior-1, prior-2, and prior-3 are fixed in the current source; prior-4 remains valid because manager::rehydrate is still exported as public API. New findings in the latest delta: the PR removes the generation guard from both identity and shielded background sync loops, reintroducing a stale-thread cleanup race; the FFI malformed-xpub row is now skipped correctly but is still surfaced with the generic reason code despite the ABI documenting a dedicated malformed-xpub code.
🔴 2 blocking | 🟡 2 suggestion(s)
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [SUGGESTION] (deduped existing open thread)
packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API —pub mod rehydrateexposes restore-path helpers such asmanager::rehydrate::apply_persisted_core_stateto downstream crates even though the only in-tree non-test caller is the manager load path. These helpers mutateManagedWalletInfounder seedless reconstruction assumptions and should stay...
🤖 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/identity_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/identity_sync.rs:421-423: Old identity-sync thread can clear a newer cancellation token
The base branch used `background_generation` so an exiting background thread only cleared `background_cancel` if it was still the active generation. This PR removes that guard and now clears the slot unconditionally. A `stop()` followed quickly by `start()` can install a new token while the old thread is still unwinding; when the old thread reaches this cleanup block, it erases the new token. After that `is_running()` reports false, `stop()`/`quiesce()` cannot cancel the still-live new loop, and shutdown can return while the loop later calls the FFI-backed persister through host-owned callback context.
In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/shielded_sync.rs:251-253: Old shielded-sync thread can make a new loop untrackable
The base branch guarded this cleanup with a generation check, but the current code clears `background_cancel` unconditionally when any shielded-sync thread exits. A tight `stop()` -> `start()` overlap lets the old thread erase the new loop's cancellation token. The FFI stop path relies on `quiesce()` as the barrier before clear/unregister/rebind operations, but once the token is lost `quiesce()` can return while the replacement loop is still alive and can later persist shielded state or dispatch callbacks after the host believes sync is stopped.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:1568-1572: Malformed Swift xpub rows use the generic skip code
`SkippedWalletFFI.reason_code` documents `101` for malformed account xpubs, and `skip_reason_code` only emits that value for `CorruptKind::MalformedXpub`. The FFI loader now correctly skips a bad row instead of aborting the whole restore, but every `build_wallet_start_state` error, including `failed to decode account xpub`, is converted to `CorruptKind::DecodeError`, so Swift receives reason code `102`. That makes the newly exposed outcome API unable to distinguish malformed xpub rows from unrelated structural decode failures despite advertising a dedicated code.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API
`pub mod rehydrate` exposes restore-path helpers such as `manager::rehydrate::apply_persisted_core_state` to downstream crates even though the only in-tree non-test caller is the manager load path. These helpers mutate `ManagedWalletInfo` under seedless reconstruction assumptions and should stay behind `PlatformWalletManager::load_from_persistor`; leaving the module public freezes internal recovery mechanics as SDK API.
thepastaclaw
suggested changes
Jun 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: prior-1, prior-2, prior-3, and prior-4 are still valid in the current source at 8d88d49. New findings in the latest delta: none beyond the carried-forward issues; the latest three-file delta does not address the existing cancellation-token races, FFI skip-code mismatch, or public rehydration module exposure.
🔴 2 blocking | 🟡 2 suggestion(s)
Carried-forward findings already raised (4)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [BLOCKING] (deduped existing open thread)
packages/rs-platform-wallet/src/manager/identity_sync.rs:421-423: Old identity-sync thread can clear a newer cancellation token — This PR removed thebackground_generationguard that previously let an exiting thread clearbackground_cancelonly when its token was still current. The cleanup now unconditionally writesNone, so astop()followed quickly bystart()can install a replacement token while the old OS thre... - [BLOCKING] (deduped existing open thread)
packages/rs-platform-wallet/src/manager/shielded_sync.rs:251-253: Old shielded-sync thread can make a new loop untrackable — This cleanup has the same stale-thread race: the PR removedbackground_generation, and any exiting shielded-sync thread now clearsbackground_cancelunconditionally. A tightstop()orquiesce()followed bystart()can leave the previous thread racing with the new start, causing the prev... - [SUGGESTION] (deduped existing open thread)
packages/rs-platform-wallet-ffi/src/persistence.rs:1568-1572: Malformed Swift xpub rows use the generic skip code —SkippedWalletFFI.reason_codedocuments101for malformed account xpubs, andskip_reason_codeonly returns that value forCorruptKind::MalformedXpub. However,FFIPersister::loadmaps everybuild_wallet_start_statefailure toCorruptKind::DecodeError, including the explicit `failed to... - [SUGGESTION] (deduped existing open thread)
packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API —pub mod rehydrateexports restore-path internals such asmanager::rehydrate::apply_persisted_core_stateto downstream crates, while the only in-tree non-test caller is the manager load path. These helpers mutateManagedWalletInfounder the PR's seedless reconstruction assumptions and should...
🤖 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/identity_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/identity_sync.rs:421-423: Old identity-sync thread can clear a newer cancellation token
This PR removed the `background_generation` guard that previously let an exiting thread clear `background_cancel` only when its token was still current. The cleanup now unconditionally writes `None`, so a `stop()` followed quickly by `start()` can install a replacement token while the old OS thread is still unwinding, then have the old thread erase that new token. Once that happens, `is_running()` reports false and later `stop()` or `shutdown()` cannot cancel the still-live replacement loop; after `quiesce()` reopens its gate, that loop can run again and call the FFI-backed persister through host-owned callback context after the host believes shutdown is complete.
In `packages/rs-platform-wallet/src/manager/shielded_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/shielded_sync.rs:251-253: Old shielded-sync thread can make a new loop untrackable
This cleanup has the same stale-thread race: the PR removed `background_generation`, and any exiting shielded-sync thread now clears `background_cancel` unconditionally. A tight `stop()` or `quiesce()` followed by `start()` can leave the previous thread racing with the new start, causing the previous thread to erase the new loop's cancellation token. The FFI stop, clear, and destroy paths rely on quiescence before the host clears or releases Swift-owned persistence and event callback contexts; after the token is lost, a later quiesce can return while the replacement loop is still alive and able to persist shielded state or dispatch callbacks.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:1568-1572: Malformed Swift xpub rows use the generic skip code
`SkippedWalletFFI.reason_code` documents `101` for malformed account xpubs, and `skip_reason_code` only returns that value for `CorruptKind::MalformedXpub`. However, `FFIPersister::load` maps every `build_wallet_start_state` failure to `CorruptKind::DecodeError`, including the explicit `failed to decode account xpub` error from decoding `AccountSpecFFI.account_xpub_bytes`. Restore now correctly skips the corrupt row instead of aborting the batch, but Swift receives reason code `102`, so the newly exposed outcome ABI cannot distinguish malformed xpub rows from unrelated structural decode failures despite advertising a dedicated code.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API
`pub mod rehydrate` exports restore-path internals such as `manager::rehydrate::apply_persisted_core_state` to downstream crates, while the only in-tree non-test caller is the manager load path. These helpers mutate `ManagedWalletInfo` under the PR's seedless reconstruction assumptions and should remain behind `PlatformWalletManager::load_from_persistor`; keeping the module public freezes the recovery mechanics as SDK API without an external contract.
Collaborator
|
Correction from PastaClaw: the earlier review-bot triage note at #3692 (comment) was generated by PastaClaw during heartbeat handling, but it was posted through the wrong GitHub identity. Please treat it as agent-authored review-bot triage, not as Pasta speaking. Reposting under the correct account for attribution: Processed the CodeRabbit batch against current head Most of the older inline CodeRabbit threads are already resolved. Two current CodeRabbit items still look live and are not covered by my latest CHANGES_REQUESTED review:
Separately, my current blocking review still stands on the identity/shielded sync cancellation-token cleanup races. |
thepastaclaw
pushed a commit
to thepastaclaw/platform
that referenced
this pull request
Jun 29, 2026
… join [dashpay#3954, independent on v3.1-dev] Introduce a shared `ThreadRegistry<WalletWorker>` lifecycle engine in `dash-async` (weight-ordered graceful shutdown: lower weights drain first, equal weights concurrently; reap-or-park orphan handling; cancellation-safe `RefcountedFlagGuard`). Migrate the three periodic sync coordinators (identity / platform-address / shielded) onto a shared `CoordinatorLifecycle` that drives them through the registry and gates passes via an `is_syncing` / `quiescing` handshake. `PlatformWalletManager::shutdown()` now holds every coordinator's quiescing gate across the whole teardown, quiesces the weight-0 coordinators concurrently, then drains the weight-10 wallet-event adapter and any parked orphans, returning a per-worker `ShutdownReport`. `clear_shielded` refuses (leaving the commitment-tree store intact) when the in-flight pass does not drain cleanly, surfaced as `ShieldedShutdownIncomplete` / `ErrorShutdownIncomplete = 19`. FFI `destroy` reports the incomplete-shutdown code so the Swift host defers freeing its callback context. Reconstructed independently on a clean v3.1-dev base: only the graceful- shutdown contribution is included; the drifted dashpay#3692 rehydration/storage work merged into the source branch is excluded. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d on rehydrate (#3692 review) #3692 inadvertently deleted the per-manager `background_generation` guard from `identity_sync` and `shielded_sync` (the change was curated from the stacked branch where it was "superseded by #3954's ThreadRegistry"). On the *independent* #3692 there is no ThreadRegistry, so the deletion reintroduces the race thepastaclaw flagged: a tight stop() -> start() lets an exiting old thread's cleanup unconditionally clear `background_cancel`, wiping the *new* loop's token. is_running() then reports false and stop()/quiesce() can no longer cancel the still-live new loop. Fix: revert both files to v3.1-dev base, so #3692 no longer touches the sync managers at all and the base guard stands. The centralized lifecycle replacement (ThreadRegistry / CoordinatorLifecycle) remains #3954's scope; in the integration branch #3954's version supersedes this. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
#3692 review) `is_empty()` omitted the `skipped` set, so a load that reconstructed no wallets but rejected one or more rows reported empty — which would let a skipped-only result short-circuit `LoadOutcome::skipped` and the `on_wallet_skipped_on_load` notifications. Count `skipped` so a skipped-only state is non-empty. Adds a regression test. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ount xpub (#3692 review) The FFI load path mapped every `build_wallet_start_state` failure to `CorruptKind::DecodeError` (reason_code 102), so the dedicated `MalformedXpub` family (code 101 — already wired in `skip_reason_code` and documented on `WalletSkipInfoFFI.reason_code`) was dead: Swift could never distinguish an unrecoverable malformed-xpub row from any other decode failure. Classify the error via `corrupt_kind_from_build_err`, keyed off the producer's stable `MALFORMED_XPUB_ERR_PREFIX` constant (shared between the decode site and the classifier so the two can't drift). Adds a regression test. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jun 30, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The latest push fixed the identity-sync generation cleanup and the malformed-xpub skip-code classification, including a regression test for the latter. One blocking shielded-sync race remains because the generation check is still performed before acquiring the cancel-token mutex, and the rehydration helper module is still exposed as public Rust API despite only being used internally by the manager load path.
🔴 2 blocking | 🟡 1 suggestion(s)
Findings not posted inline (1)
These findings could not be anchored to the current diff, but they are still part of this review.
- [BLOCKING]
packages/rs-platform-wallet/src/manager/shielded_sync.rs:270-273: Shielded sync can still clear a newer cancellation token — The cleanup checksbackground_generationbefore lockingbackground_cancel. An old loop can read its own generation, then a stop/start sequence can install a new token and increment the generation while holding the mutex; when the old loop later acquires the mutex, it still executes the alread...
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [SUGGESTION] (deduped existing open thread)
packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API —pub mod rehydratemakes restore internals such asmanager::rehydrate::apply_persisted_core_stateavailable to downstream crates, while the only in-tree non-test caller isPlatformWalletManager::load_from_persistor. These helpers mutateManagedWalletInfounder seedless reconstruction assum...
🤖 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/shielded_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/shielded_sync.rs:270-273: Shielded sync can still clear a newer cancellation token
The cleanup checks `background_generation` before locking `background_cancel`. An old loop can read its own generation, then a stop/start sequence can install a new token and increment the generation while holding the mutex; when the old loop later acquires the mutex, it still executes the already-passed branch and clears the new token. After that `is_running()`, `stop()`, `quiesce()`, and shutdown lose track of the replacement loop, so the Rust background thread can continue using persistence/event callback contexts after Swift believes the loop has stopped or the manager has been destroyed.
- [BLOCKING] packages/rs-platform-wallet/src/manager/shielded_sync.rs:270-273: Shielded sync can still clear a newer cancellation token
The cleanup checks `background_generation` before locking `background_cancel`. An old loop can read its own generation, then a stop/start sequence can install a new token and increment the generation while holding the mutex; when the old loop later acquires the mutex, it still executes the already-passed branch and clears the new token. After that `is_running()`, `stop()`, `quiesce()`, and shutdown lose track of the replacement loop, so the Rust background thread can continue using persistence/event callback contexts after Swift believes the loop has stopped or the manager has been destroyed.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API
`pub mod rehydrate` makes restore internals such as `manager::rehydrate::apply_persisted_core_state` available to downstream crates, while the only in-tree non-test caller is `PlatformWalletManager::load_from_persistor`. These helpers mutate `ManagedWalletInfo` under seedless reconstruction assumptions and do not define a stable external contract, so exposing the module freezes implementation details as SDK API unnecessarily.
Collaborator
Author
|
Re: thepastaclaw's 2026-06-30 re-review — confirming the two resolved items (identity-sync generation cleanup + malformed-xpub skip-code 101 with its regression test), and dispositioning the remaining body-only finding. 🔴
🟡 🤖 Co-authored by Claudius the Magnificent AI Agent |
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.
Why this PR exists
load_from_persistor()was a stub, forcing a full re-scan from birth height on every launch.v3.1-dev— reshapesClientWalletStartStateand rewrites every in-tree consumer in the same PR. The storage-reader half is feat(platform-wallet): persistence readers + seedless load() wiring (split from #3692) #3968 (also independent); the two combine on thedash-evo-toolintegration branch.What was done (
rs-platform-wallet+-ffi+ swift)PlatformWalletManager::load_from_persistor()reconstructs every persisted wallet from a keyless snapshot:Wallet::new_watch_only(...)from stored account xpubs — no seed, no signing-key derivation on the load path.LoadOutcome.skipped+PlatformEvent::WalletSkippedOnLoad.ClientWalletStartState,CoreChangeSet, load-result types).warn_if_non_default_account,RehydrateRowError→error.rs, concreteon_wallet_skipped_on_loadhandler; carrieslast_applied_chain_lockthrough the seedless load.Testing
cargo fmt --all --check;cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets -- -D warnings;cargo test -p platform-wallet(225 passed). Swift file needs a separate iOS build check.Breaking changes
None against
v3.1-dev(every change is additive or on an unreleased shape).Checklist
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes