feat(platform-wallet): watch-only rehydration from persistor (seedless load)#3692
feat(platform-wallet): watch-only rehydration from persistor (seedless load)#3692Claudius-Maginificent wants to merge 15 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 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 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 |
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
b573fca to
b7508a0
Compare
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-05-22T10:49:18.857Z |
🔁 Seedless-load rework landed —
|
Claudius-Maginificent
changed the title
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
0e92cb4 to
f57b117
Compare
Claudius-Maginificent
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3692 +/- ##
=========================================
Coverage 87.18% 87.18%
=========================================
Files 2632 2632
Lines 327563 327563
=========================================
Hits 285592 285592
Misses 41971 41971
🚀 New features to boost your workflow:
|
lklimek
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
@coderabbitai review all |
|
✅ Review complete (commit 2ab0bed) |
|
✅ Action performedFull review finished. |
thepastaclaw
left a comment
thepastaclaw
left a comment
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.
… 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
left a comment
thepastaclaw
left a comment
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.
|
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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Follow-up on current head 61c9fd757d06e59da6fd3dc68b4614011d33f120: verified the latest PR diff no longer includes identity_sync.rs or shielded_sync.rs, so the remaining body-only shielded-sync finding from my 2026-06-30 re-review is out of scope for #3692 and should be treated as withdrawn here. The concrete regressions I raised on this PR have either been fixed (identity_sync generation cleanup, malformed-xpub skip code, skipped-only ClientStartState::is_empty()) or have an accepted nonblocking/WONTFIX rationale (pub mod rehydrate).
No unresolved review threads remain from my side on this head.
Bring the PR branch up to date with the new base v4.1-dev (retarget from previous base). Clean merge, no conflicts. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
| network, | ||
| expected_wallet_id, | ||
| accounts, | ||
| )) |
There was a problem hiding this comment.
[SEC — HIGH/MEDIUM] Seedless rehydration trusts the persisted account manifest without binding it to wallet_id.
build_watch_only_wallet rebuilds each account from the persisted account_xpub via Account::from_xpub(...) and hands the collection to Wallet::new_watch_only(network, expected_wallet_id, accounts) — the wallet_id is stamped in verbatim, never re-derived or checked against the accounts.
Verified against the pinned key-wallet rev: compute_wallet_id() for WalletType::WatchOnly returns self.wallet_id unchanged (does not recompute from the account keys). The canonical id is a hash of the root extended pubkey, but only per-account (hardened-derived) xpubs are persisted — hardened derivation is one-way, so an account xpub can't be walked back to the root pubkey to verify. The only validation performed is structural (Account::from_xpub rejects a malformed xpub); a well-formed but wrong xpub (corrupted or substituted) passes silently and becomes the wallet's key material under the victim's original wallet_id. The existing seed→wallet_id gate on the spend path doesn't mitigate this — it authorizes spending, but receive addresses are derived from the (possibly tampered) account xpubs and never re-verified against the seed.
Impact: if the persisted store is corrupted or tampered, the rehydrated wallet presents under the legitimate wallet_id but derives receive addresses from the wrong/attacker xpub — funds the user subsequently receives could land on addresses the user's seed can't spend (or, in a tamper scenario, that an attacker controls).
Suggestion: authenticate the persisted wallet-defining material before trusting it on load — e.g. persist a MAC/commitment over {wallet_id, network, account manifest} keyed to a secret in the platform secure enclave, and fail closed (skip the row) on verification failure at load time. At minimum, document this trust boundary explicitly at load_from_persistor's entry point.
| }); | ||
| let result = unwrap_option_or_return!(option); | ||
| unwrap_result_or_return!(result); | ||
| let outcome = unwrap_result_or_return!(result); |
There was a problem hiding this comment.
[SEC — MEDIUM] out_outcome is left uninitialized on the error path — later platform_wallet_load_outcome_free call risks a garbage-free / double-free.
platform_wallet_manager_load_from_persistor only writes *out_outcome (via ptr::write) on the success path. The unwrap_result_or_return!(result) early-return here (whenever load_from_persistor() fails — e.g. this PR's own new RehydrationTopologyUnsupported fail-closed guard) leaves the caller's LoadOutcomeFFI buffer completely untouched. platform_wallet_load_outcome_free then unconditionally Box::from_raws whatever it finds in o.skipped — a caller that follows the documented "must release via _free" contract with a standard allocate-then-defer-free-on-error idiom would free an uninitialized/stale pointer.
Not live today (the in-tree Swift caller passes nil), but this is a public #[no_mangle] extern "C" fn, and the same crate has an established fix for exactly this pattern elsewhere (document.rs, "Initialize the JSON out-param to null up front so every early error return leaves it null" — repeated 5×) that this new FFI pair doesn't follow. Confirmed via cargo test -p platform-wallet-ffi --lib (100/100 passing) that no test in the crate touches either new FFI symbol, so a regression here wouldn't be caught.
Suggestion: write a safe empty LoadOutcomeFFI { loaded_count: 0, skipped_count: 0, skipped: null } into *out_outcome immediately after the null-check, before the fallible call, matching the crate's own established out-param convention.
| @@ -1,3 +1,5 @@ | |||
| [advisories] | |||
There was a problem hiding this comment.
[SEC — LOW] cargo audit reports a live HIGH advisory in the resolved dependency graph, and this PR's own change un-suppresses the gate.
This PR empties the ignore list here (dropping the prior RUSTSEC-2020-0071 entry). Running cargo audit against the resolved graph now reports:
- ERROR —
quinn-proto 0.11.14, RUSTSEC-2026-0185, severity 7.5 HIGH: remote memory exhaustion from unbounded out-of-order stream reassembly; fixed in>=0.11.15. Transitive viareqwest→quinn→quinn-proto. - WARN (unsound) —
anyhow 1.0.102, RUSTSEC-2026-0190. - WARN (unmaintained) —
bincode 2.0.1andatomic-polyfill 1.0.3.bincodeis directly on the untrusted-persisted-blob decode path this PR adds.
No Cargo.lock/Cargo.toml change in this PR, so the vulnerable version is pre-existing — but with the ignore list now empty, CI's cargo audit step will fail on this advisory.
Suggestion: bump the transitive quinn-proto to >=0.11.15 (cargo update -p quinn-proto, or raise the reqwest/quinn constraint) before/alongside this PR so CI audit passes, or add a dated, rationale-carrying ignore entry if the QUIC path is confirmed unreachable.
| let mut index: u32 = 0; | ||
|
|
||
| loop { | ||
| // Horizon: gap_limit past the deepest match, or the initial |
There was a problem hiding this comment.
[SEC — LOW] Rehydration derivation work is bounded per-chain but not per-manifest — a crafted/corrupted persisted manifest can force a large synchronous key-derivation load at launch.
extend_pools_for_restored_addresses caps each chain's discovery scan at MAX_REHYDRATION_DERIVATION_INDEX (10,000), reasoning that "a funds account exposes a fixed, small number of chains" — true per account, but the number of accounts comes from the persisted manifest, which is untrusted under the same threat model as the wallet_id/xpub finding above. A manifest with many accounts (or UTXO addresses spaced to walk the horizon toward the ceiling on each chain) multiplies secp256k1 derivations, all performed synchronously during load_from_persistor at app launch.
Bounded (no infinite loop/panic), but the per-chain cap gives a false sense of total boundedness since the multiplier (account count) is caller-supplied.
Suggestion: add an aggregate derivation/account-count guard for the whole rehydration pass, and log when the manifest size or cumulative derivation total crosses a threshold.
| /// never touches the seed, so it cannot skip for a wrong or unavailable | ||
| /// seed. Variants carry no key material (SECRETS.md SEC-REQ-2.0.1). | ||
| /// | ||
| /// [`MnemonicResolverHandle`]: rs_sdk_ffi::MnemonicResolverHandle |
There was a problem hiding this comment.
[Docs — LOW] Two doc-link targets are unreachable.
- This line (and
load.rs:59) define[MnemonicResolverHandle]: rs_sdk_ffi::MnemonicResolverHandle, butrs-platform-wallet/Cargo.tomlhas nors-sdk-ffidependency — the intra-doc path won't resolve. client_wallet_start_state.rs:12linkssign_with_mnemonic_resolverto the bare crate roothttps://docs.rs/rs-platform-wallet-ffi/rather than the item itself — an internal path-only FFI crate unlikely to be published there anyway.
Both provable from the manifest/URL alone, no doc build required — rendered docs will carry dead or crate-root-only links where a reader expects to land on the referenced item.
Suggestion: downgrade the MnemonicResolverHandle reference to plain inline code with a prose pointer, and replace the bare docs.rs link with an item-specific URL or plain code text.
| /// records, IS-locks, sync watermarks, `last_applied_chain_lock`). | ||
| /// The manager applies this onto a fresh | ||
| /// `ManagedWalletInfo::from_wallet` skeleton built from the | ||
| /// watch-only wallet. Rebuilt by the `core_state::load_state` reader |
There was a problem hiding this comment.
[Docs — LOW] Public rustdoc points at a non-existent core_state::load_state reader and leaks an internal "(item B)" work-item label.
There's no core_state::load_state reader in this crate (grepped fn load_state / mod core_state). The core_state projection is actually produced by the FFI persister's build_wallet_start_state for the iOS path (the SQLite reader lives in the storage crate) — the doc cross-reference resolves to nothing. Separately, "(item B)" is an internal implementation-plan label that means nothing to a reader of the shipped API docs.
Suggestion: point at the real producer(s) — e.g. "built by the persister's load path (build_wallet_start_state for the FFI/iOS path; the SQLite reader for the storage path)" — and drop the "(item B)" label.
| //! RT cases here: | ||
| //! - RT-WO: round-trip — watch-only wallet is registered after reload. | ||
| //! - RT-Corrupt: a row with an empty manifest is skipped with | ||
| //! `MissingManifest`, the other row loads, a `WalletSkippedOnLoad` |
There was a problem hiding this comment.
[Docs — LOW] References a PlatformEvent::WalletSkippedOnLoad event that doesn't exist.
This test-module doc (and the PR description) name a WalletSkippedOnLoad event, but there's no PlatformEvent enum in the crate and no such type (grep finds only these comments). The real mechanism is the PlatformEventHandler::on_wallet_skipped_on_load trait method (events.rs).
Suggestion: update the doc comment (and ideally the PR description) to name the real surface — the on_wallet_skipped_on_load handler callback carrying SkipReason::CorruptPersistedRow.
| pub struct SkippedWalletFFI { | ||
| /// The (public) 32-byte wallet id that was skipped. | ||
| pub wallet_id: [u8; 32], | ||
| /// Structural skip reason. `100` = missing account manifest, |
There was a problem hiding this comment.
[Project — LOW] FFI reason_code values 199/200 are emitted via the #[non_exhaustive] fallthrough arms but undocumented on the public struct, and all five codes are magic literals duplicated between the doc and the match.
SkippedWalletFFI.reason_code's doc documents only 100/101/102; a host switching on this field has no documented handling for the generic-corrupt (199) / generic-skip (200) fallback codes. Separately, every other status code crossing this FFI boundary is a named #[repr(C)] enum variant (PlatformWalletFFIResultCode) — this one is bare integer literals with no cbindgen-exported constant, so a future reorder of the match arms has nothing to catch it at compile time.
Suggestion: document 199/200 alongside 100/101/102, and lift the five codes into named consts referenced by both the match and the doc so they can't silently drift apart.
| // `iter_mut()` over `Vec<&mut AddressPool>` yields `&mut &mut _`; | ||
| // reborrow once so the pool flows into `ensure_derived` cleanly. | ||
| let pool: &mut AddressPool = pool; | ||
| debug_assert_eq!( |
There was a problem hiding this comment.
[Code quality — LOW] Probe/pool chain-order invariant guarded only by debug_assert_eq!, compiled out in release.
extend_pools_for_restored_addresses applies discovered derivation depths back to the real pools by position (zip). The doc comment explicitly claims this is fail-closed ("verify that invariant before zipping by position"), but at runtime only the pool/probe count is checked; the per-slot chain-type match that actually guarantees correct-pool attribution is a debug_assert_eq! — compiled out in release. Currently low-likelihood since both sides are built from the same enumeration in the same order today, but the guard is exactly what protects future refactors of that enumeration, and a release build would silently misattribute derivation depth to the wrong pool if that ordering ever diverged.
Suggestion: promote the per-slot check to a runtime guard returning PlatformWalletError::RehydrationPoolMismatch (or a dedicated variant) when pool.pool_type != probe.pool_type, matching the fail-closed intent already applied to the length check.
| /// mandate). An empty UTXO set is always `Ok`. | ||
| /// | ||
| /// This never touches key material. | ||
| pub fn apply_persisted_core_state( |
There was a problem hiding this comment.
[Code quality — LOW] apply_persisted_core_state is exported pub (real public crate API) though only used internally, inconsistent with its sibling.
Its only non-test caller is load.rs, same crate; the FFI crate references it only in comments, never calls it. Its sibling in the same file, build_watch_only_wallet, is correctly scoped pub(super). The visibility asymmetry between these two closely-related rehydration entry points reads as accidental rather than intentional, and commits the crate to a public API surface (taking internal key_wallet types) with no external consumer.
Suggestion: narrow to pub(super) (or pub(crate)) to match build_watch_only_wallet, unless deliberately intended as public API — in which case re-export from lib.rs and document that intent.
| /// String-matched because `PersistenceError` carries no typed discriminator. | ||
| fn corrupt_kind_from_build_err(e: &PersistenceError) -> CorruptKind { | ||
| let msg = e.to_string(); | ||
| if msg.contains(MALFORMED_XPUB_ERR_PREFIX) { |
There was a problem hiding this comment.
[Code quality — LOW] corrupt_kind_from_build_err classifies errors by substring-matching the Display text of PersistenceError.
The malformed-xpub vs. generic-decode classification (which drives FFI reason_code 101 vs 102) is decided by e.to_string().contains(MALFORMED_XPUB_ERR_PREFIX). The PR does mitigate the worst of this by sharing the prefix constant between producer and matcher, but the underlying design still routes a structural decision through human-readable text — a reworded or translated upstream error message would silently reclassify the failure with no compiler signal. (Note: today this specific call site is unreachable anyway — see the separate MalformedXpub dead-code comment on rehydrate.rs — which makes the string-matching risk latent rather than active, but it's the same fragile pattern that will bite whenever Account::from_xpub becomes fallible.)
Suggestion: add a typed discriminator to PersistenceError (e.g. a MalformedXpub variant or a kind() method) and match on that instead of the Display string.
| // (SECRETS.md: no `Wallet`/seed crosses `load()`). The manager | ||
| // rebuilds a watch-only wallet from this manifest via | ||
| // `Wallet::new_watch_only` and applies this `core_state` projection. | ||
| // Signing happens later via the on-demand |
There was a problem hiding this comment.
[Code quality — LOW] Build→project→rebuild round-trip duplicates pool-derivation and used-address marking across the FFI reader and the manager.
build_wallet_start_state constructs a full Wallet + ManagedWalletInfo, merges persisted pool used flags into it, then extracts new_utxos/used_core_addresses back out and drops the wallet; the manager then rebuilds a fresh watch-only ManagedWalletInfo and re-derives pools / re-marks used addresses independently in apply_persisted_core_state + extend_pools_for_restored_addresses. The "derive pools, mark used addresses" concern is expressed twice on either side of the keyless boundary — a change to gap-limit or used-address semantics on one side that isn't mirrored on the other risks reintroducing the address-reuse hazard this PR sets out to close.
Suggestion: consider having the FFI reader project directly to the keyless ClientWalletStartState without first materializing and discarding a full Wallet/ManagedWalletInfo, so pool derivation/used-marking live only in the manager's apply_persisted_core_state.
| // each probe from index 0 is an accepted, bounded one-time-load cost | ||
| // (per chain capped at MAX_REHYDRATION_DERIVATION_INDEX); rehydration | ||
| // runs once per wallet at startup, never on a hot path. | ||
| let mut probes: Vec<(AddressPool, BTreeSet<u32>)> = account |
There was a problem hiding this comment.
[Code quality — LOW] Per-probe BTreeSet<u32> is redundant — only its max is ever read; Option<u32> would do.
probes is typed Vec<(AddressPool, BTreeSet<u32>)> and the set is populated with every resolved index during discovery, but it's only ever read via .iter().next_back() — i.e. only its maximum. The discovery loop already tracks that same maximum in a loop-local variable, so the full BTreeSet allocates a tree per chain for no functional benefit beyond what's already tracked.
Suggestion: change probes to Vec<(AddressPool, Option<u32>)> storing the deepest resolved index directly — removes a per-chain allocation and a piece of duplicated state.
| } | ||
| let mut accounts = AccountCollection::new(); | ||
| for entry in manifest { | ||
| let account = Account::from_xpub( |
There was a problem hiding this comment.
[Code quality — LOW] CorruptKind::MalformedXpub is unreachable dead code via this call site.
Account::from_xpub in the exact pinned key-wallet dependency this PR builds against is an unconditional Ok(...) — no fallible branch at all. Since the only input here (entry.account_xpub) is already a well-typed ExtendedPubKey (any actually-malformed bytes were rejected earlier at the bincode-decode step), there's no value this call site could ever receive that makes Account::from_xpub return Err. .map_err(|_| RehydrateRowError::MalformedXpub) is therefore dead code today, and the rustdoc'd Errors section describes a corruption-detection capability the code cannot currently deliver. The same infallibility applies to the second, independent Account::from_xpub call in the FFI's own build_wallet_start_state (persistence.rs), which is presently mapped to the generic DecodeError code instead — also unreachable for the same reason, and will silently misclassify once key-wallet eventually makes this fallible (its Result signature is clear scaffolding for that).
Suggestion: either remove the dead branch/variant until Account::from_xpub is actually fallible (with a TODO referencing the upstream change that will make it so), or add a test that actually forces the failure (impossible today, which is itself proof of the gap).
| } | ||
|
|
||
| /// Regression: after restart-in-place the watch-only pools eagerly | ||
| /// cover only `0..=gap_limit`, but persisted UTXOs can sit at deeper |
There was a problem hiding this comment.
[Code quality — LOW] Self-contradicting eager-derivation-window doc in a module that calls itself "safety-critical-correct".
This comment's opening sentence states the eager window is 0..=gap_limit (inclusive range notation), but two lines later the same comment's own "Index layout" table says index gap_limit is the "first index past eager window" — i.e. outside the window, which is only consistent with the eager window being 0..gap_limit (exclusive). Confirmed by actually running the referenced test: the real runtime boundary is 0..gap_limit (exclusive) — the code's behavior is correct, only this sentence's notation is wrong. Easy to copy the wrong notation into new code without noticing the internal inconsistency in the same comment block.
Suggestion: fix this sentence to 0..gap_limit to match the "Index layout" table immediately below it and the verified test behavior.
| // bytes Swift handed back onto the local `wallet_info`. It is then | ||
| // carried into the keyless `CoreChangeSet` below and re-applied by | ||
| // `apply_persisted_core_state`, so the asset-lock-resume | ||
| // CL-from-metadata fallback (`proof.rs`) fires at app launch on any |
There was a problem hiding this comment.
[SEC — LOW/MEDIUM] Persisted last_applied_chain_lock is restored without re-verifying its BLS/quorum signature.
The persisted chainlock is bincode-decoded and written straight onto wallet metadata, then carried into CoreChangeSet and re-applied during rehydration. Only bincode shape is validated — the signature is never re-checked against the quorum. This value drives the asset-lock-resume fallback, which fires at launch on any tracked lock whose funding height is <= cl.block_height; a corrupted or forged blob could inject an arbitrary height, causing the wallet to treat asset locks as chain-locked prematurely.
Bounded by network-side verification (Platform re-verifies the chainlock when the asset-lock proof is submitted, so a forged local chainlock produces a proof the network rejects rather than accepting a forgery on-chain) — still a data-integrity gap on locally-trusted crypto material restored from an unauthenticated store.
Suggestion: re-verify the restored chainlock's signature before trusting it for the asset-lock-resume fallback, or mark a restored-from-persistence chainlock explicitly unverified pending SPV re-confirmation.
| @@ -2758,6 +2784,24 @@ impl Drop for LoadGuard { | |||
| /// the configured signer surface (see | |||
| /// `Wallet::new_external_signable`). Earlier revisions of this code | |||
There was a problem hiding this comment.
[Docs — LOW] This comment documents an "external-signable"/host-keychain-signer reconstruction model that contradicts the "watch-only" model every other artifact in this PR describes.
This doc block states the reconstruction is "for external-signable reconstruction ... built via Wallet::new_external_signable so the signer surface routes back to the host's keychain." But rehydrate.rs::build_watch_only_wallet builds Wallet::new_watch_only, and load.rs/client_wallet_start_state.rs/the Swift binding all agree the manager registers a watch-only wallet with signing deferred to the on-demand resolver path. The new_external_signable wallet this FFI doc describes is actually built and then dropped (see the wallet field removed from ClientWalletStartState a bit further down in this same function) — it never governs signing. A host reading the generated header can't tell which signing model is authoritative.
Suggestion: rewrite this doc to state the locally-built wallet here is a throwaway used only to shape the account/UTXO projection, that it's dropped, and that the manager registers a watch-only wallet whose signing is resolver-gated — matching the wording in client_wallet_start_state.rs and the Swift doc.
| /// | ||
| /// Returns [`RehydrateRowError`] when the row is structurally unusable | ||
| /// (caller maps it onto a per-row [`SkipReason`]). | ||
| pub(super) fn build_watch_only_wallet( |
There was a problem hiding this comment.
Automated review — 18 findings from an independent grumpy-review pass
Posted 17 inline comments above (security, docs, and code-quality) against the head commit. One additional finding couldn't be anchored inline since it points at pre-existing code this PR's new logic reads from rather than a line changed by the diff itself:
[SEC — LOW] Persisted UTXO values are restored with unchecked u64 addition during balance recompute. persistence.rs:3158 (value: u.value_duffs) takes the persisted value verbatim into the restored UTXO — pre-existing code, unchanged here — and the downstream balance recompute (in key_wallet) sums per-account/per-wallet totals with plain +=, no checked_add/saturating_add. Under the same untrusted-persisted-store threat model as the wallet_id/xpub finding above, a crafted blob could supply values whose sum overflows u64: release builds silently wrap to a wrong balance, debug builds panic. Suggestion: validate/clamp persisted UTXO values on load, and/or use checked arithmetic in the balance accumulation.
Overall assessment: solid, well-tested feature (clippy clean, 16-test suite asserting real substance — exact derivation indices, exact balance totals, boundary conditions). The one item worth landing before this touches production wallets holding real funds is the wallet_id/xpub authentication gap (first comment above) and the FFI out-param initialization fix (second comment) — everything else here is non-blocking polish.
🤖 Co-authored by Claudius the Magnificent AI Agent
…5.0.0) (#55) * feat(crew)!: Bilby exits code review, Marvin/Adams split its scope (v5.0.0) An internal Opus-vs-Sonnet-5 grumpy-review experiment showed Sonnet 5 matching Opus on review depth while winning on independent-verification behavior, and measured redundancy showed project-reviewer-adams and developer-bilby overlapping 18-27% of the time on the same code-quality ground. Restructure the review crew around evidence type instead: - developer-bilby exits code review entirely — implementation-only from now on (still writes code, tests, and self-reviews its own work). - qa-engineer-marvin absorbs the adversarial/execution half (run tests and lints, edge cases, panics, independent verification against git history and live repo state — never trust a diff or a report at face value). Model flips opus -> sonnet. - project-reviewer-adams absorbs the structural/idiom half (naming, DRY, cross-file consistency, readable-without-running). Model flips sonnet -> opus. - security-engineer-smythe is unchanged (opus, unconditional). grumpy-review's agent mix becomes a fixed 3-agent trio (Smythe/Adams/ Marvin) for every non-trivial review, replacing the old per-language conditional-agent table. Trivial-review fallback picks the opposite tier from whichever tier authored the code, for maximum independence. workflow-feature/-simplified narrow Marvin's QA-phase remit to Tests-only, moving Docs-review/Dedup-audit to Adams; workflow-trivial drops both passes outright at its scope. workflow-simplified's security pass is now unconditional, matching grumpy-review's trio treatment. Every new agent-scope section is self-contained (states its own boundary directly rather than deferring to a sibling agent's file) — there is no runtime mechanism for one agent to look up another's prompt. BREAKING CHANGE: developer-bilby's description/frontmatter and grumpy-review's agent-selection contract change shape; qa-engineer-marvin and project-reviewer-adams swap model-tier defaults everywhere declared (agent frontmatter, grand-admiral Token Economy, grumpy-review Core agents table). See CHANGELOG.md [5.0.0] for the full list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vd7EkGmndUCNYHR7gMqJx * fix(agents): re-home Adams' coverage-gap flag, name over-engineered structures A real-PR A/B validation run (old crew vs new crew on dashpay/platform#3692) showed the Test Depth narrowing in the crew restructure had an unintended side effect: it also dropped Adams' general "flag zero test coverage on an untested surface" instinct, with no new owner (too coverage-abstract for Marvin's execution-only scope). Re-home a reading-only version of it on Adams, scoped to public/cross-boundary surfaces specifically so it doesn't reintroduce the deep-assertion-quality overlap with Marvin the restructure was meant to remove. Also name "redundant/over-engineered data structures" as an explicit example category, since a similar finding class (an old Bilby catch) wasn't reproduced without a concrete prompt or a data structure that a simpler type would do. Amends the still-unreleased [5.0.0] entry (PR #55 not yet merged) rather than bumping the version again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vd7EkGmndUCNYHR7gMqJx --------- Co-authored-by: Claudius Agent <noreply@anthropic.com>
3 participants
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