From bad20931d32a8009dbe15223f1680cd02cb841c2 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:32:08 -0400 Subject: [PATCH 1/2] fix(platform-wallet): stop the startup sequence reporting integrity it did not establish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects on the bring-up path introduced or left open by #4359, all of which end the same way: `start_wallet_subsystems` returns a status that promises a contact's DIP-15 addresses exist before Core SPV starts, when they do not. An address the wallet is not watching when the compact-filter scan passes its funding height produces no transaction at all, so each of these is a silent data gap rather than a cosmetic mislabel. 1. A contact pass that reached nobody was recorded as a completed sync. `sync_contact_requests` is log-and-continue per identity — correct for the recurring sweep, and it collapsed "Platform answered, nothing new" into the same `Ok(vec![])` as "Platform answered nobody". With DAPI unreachable the sweep returned an empty success, startup called `record_sync_ran`, and `status()` reported `Ready`. The pass now reports what it reached. `sync_contact_requests_reporting` returns a `ContactSyncReport` carrying the per-identity failure set; `sync_contact_requests` keeps its shape and raises `ContactSyncUnreachable` when nothing at all was read. Startup records the sync only on a complete pass, so a degraded one stays `PartialAccountsPending`. Failures retry themselves: a failed fetch leaves that direction's high-water cursor unadvanced, so the next sweep re-requests exactly the range it missed. 2. A partial identity scan was never retried once any identity was on file. `ScanTally::is_trustworthy` is `identities_seen > 0 || failed_probes == 0`, so a scan that saw index 0 and got no answer at index 1 returns `Ok`; the warm-launch shortcut then skipped discovery on every later launch, and nothing recorded that the scan had been partial. The second identity, and all of its contacts, stayed invisible for the life of the installation. A scan now publishes a verdict — complete, or the indices it could not answer — which the shortcut consults. Two things follow: a partial scan is retried inside its own launch, with the scan key already resolved and within the budget the caller granted; and the verdict rides the changeset so a host that persists it re-opens the question on the next launch. Absence of a verdict deliberately reads as "unknown", not "incomplete", so hosts that have not adopted the field keep the shortcut instead of paying for a scan plus a Keychain round trip before every Core SPV start. The budget-expiry path records the same verdict, since a scan dropped mid-await never reaches its own bookkeeping. Refs #4365. Not closed: no persister vtable carries the field yet, so cross-launch retention still needs the host slot. 3. The seed-binding gate existed only in the Swift wrapper. Everything the drain does with key material is unauthenticated, and `register_contact_account` keys its existence check on `(index, us, them)` rather than on the xpub — so a provider resolving the wrong seed writes contact receiving addresses once, and every later correct-seed pass no-ops. The corruption is permanent and its only symptom is payments that never arrive. iOS gated this in Swift; a JNI binding added later would have inherited the ungated path. The shared sequence now verifies the binding itself, immediately before the drain and only when something is actually queued, so a warm launch with an empty queue still resolves no key material. It fails closed on any error — a provider that cannot answer has not been shown to own the wallet — and skipping costs nothing unrecoverable, because the queue is left intact for the next signer-present drain. Reported as the new `SeedBindingUnverified` status rather than raised, since Core sync must start regardless. Also fixes a latent misreport the rescan path exposed: the discovery-failure statuses claim the identity question is open, and a rescan can now reach them with an identity already on file. They are gated on `identity_id.is_none()`. 20 new tests, including three that drive the real `start_wallet_subsystems` over a mock SDK: a wrong-seed provider registers no contact account and leaves the queue intact, the owning seed drains, and an empty queue never consults the gate at all. The mock's failing fetches reproduce the DAPI-unreachable case for (1) end to end. Co-Authored-By: Claude Opus 4.8 --- .../rs-platform-wallet-ffi/src/persistence.rs | 5 + .../src/wallet_startup.rs | 10 +- .../src/changeset/changeset.rs | 81 ++ .../changeset/identity_manager_start_state.rs | 10 + .../rs-platform-wallet/src/changeset/mod.rs | 12 +- packages/rs-platform-wallet/src/error.rs | 18 + .../rs-platform-wallet/src/manager/startup.rs | 745 +++++++++++++++++- .../rs-platform-wallet/src/wallet/apply.rs | 13 + .../identity/network/contact_requests.rs | 197 ++++- .../src/wallet/identity/network/discovery.rs | 149 +++- .../src/wallet/identity/network/mod.rs | 6 + .../identity/state/manager/accessors.rs | 36 + .../src/wallet/identity/state/manager/mod.rs | 93 ++- .../PlatformWalletManagerStartup.swift | 32 +- 14 files changed, 1350 insertions(+), 57 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/persistence.rs b/packages/rs-platform-wallet-ffi/src/persistence.rs index 04a4e29ea1d..459c1bf314a 100644 --- a/packages/rs-platform-wallet-ffi/src/persistence.rs +++ b/packages/rs-platform-wallet-ffi/src/persistence.rs @@ -4786,6 +4786,11 @@ fn build_wallet_start_state( let identity_manager = IdentityManagerStartState { out_of_wallet_identities: BTreeMap::new(), wallet_identities, + // No vtable slot carries the identity-scan verdict yet, so nothing is + // restored here. Empty reads as "unknown", which preserves the + // warm-launch shortcut rather than forcing a scan every launch — see + // `IdentityManagerStartState::scan_states`. + scan_states: BTreeMap::new(), }; // Rehydrate tracked asset-locks (built / broadcast / IS-locked diff --git a/packages/rs-platform-wallet-ffi/src/wallet_startup.rs b/packages/rs-platform-wallet-ffi/src/wallet_startup.rs index 583dfc11bb4..8bc2de6d9fa 100644 --- a/packages/rs-platform-wallet-ffi/src/wallet_startup.rs +++ b/packages/rs-platform-wallet-ffi/src/wallet_startup.rs @@ -31,6 +31,7 @@ pub enum WalletStartupStatusFFI { PartialNoIdentity = 2, PartialAccountsPending = 3, DiscoveryFailed = 4, + SeedBindingUnverified = 5, } impl From for WalletStartupStatusFFI { @@ -41,6 +42,7 @@ impl From for WalletStartupStatusFFI { WalletStartupStatus::PartialNoIdentity => Self::PartialNoIdentity, WalletStartupStatus::PartialAccountsPending => Self::PartialAccountsPending, WalletStartupStatus::DiscoveryFailed => Self::DiscoveryFailed, + WalletStartupStatus::SeedBindingUnverified => Self::SeedBindingUnverified, } } } @@ -56,8 +58,13 @@ pub struct WalletStartupOutcomeFFI { pub identity_id: [u8; 32], /// Discovery scans performed; `0` when a local identity was already known. pub discovery_attempts: u32, - /// Whether the inline contact-request pass ran. + /// Whether the inline contact-request pass ran **to completion**. `false` + /// when it came back degraded — some identities' contact documents could + /// not be read, so their account builds were never enqueued. pub dashpay_sync_ran: bool, + /// The drain was skipped because the supplied contact-crypto provider does + /// not resolve this wallet's seed. Nothing was derived or written. + pub seed_binding_unverified: bool, /// Contact-crypto entries the drain completed. pub contact_accounts_drained: u32, /// Contact-account builds still queued on return. @@ -78,6 +85,7 @@ impl From for WalletStartupOutcomeFFI { identity_id, discovery_attempts: outcome.discovery_attempts, dashpay_sync_ran: outcome.dashpay_sync_ran, + seed_binding_unverified: outcome.seed_binding_unverified, contact_accounts_drained: outcome.contact_accounts_drained as u32, contact_accounts_pending: outcome.contact_accounts_pending as u32, elapsed_ms: outcome.elapsed.as_millis() as u64, diff --git a/packages/rs-platform-wallet/src/changeset/changeset.rs b/packages/rs-platform-wallet/src/changeset/changeset.rs index fa425fbde56..a100837083b 100644 --- a/packages/rs-platform-wallet/src/changeset/changeset.rs +++ b/packages/rs-platform-wallet/src/changeset/changeset.rs @@ -1255,6 +1255,67 @@ pub struct WalletMetadataEntry { pub birth_height: u32, } +/// Whether the last gap-limit identity scan for this wallet answered every +/// index it probed. +/// +/// A scan has three endings, and only two of them are visible in what it +/// returns. It can find identities, it can prove there are none, or it can +/// find *some* while one of its probes goes unanswered — and that third +/// ending returns `Ok` with the identities it did find, because discarding +/// them would be worse. `ScanTally::is_trustworthy` is +/// `identities_seen > 0 || failed_probes == 0`, so a scan that saw index 0 +/// and got no answer at index 1 is reported as a success. +/// +/// That is survivable only if something scans again. Nothing did: the +/// warm-launch shortcut skips discovery whenever any identity is on file, and +/// the fact that the scan behind that identity was partial existed nowhere +/// once the process exited. An identity at the unanswered index then stayed +/// invisible for the life of the installation, along with all of its contacts +/// — a silent, permanent gap whose only symptom is a missing identity and +/// DPNS name after a restore. See dashpay/platform#4365. +/// +/// This is that missing fact. `complete` is stored rather than derived from +/// `failed_indices` because the two ways a scan can end early are different: +/// unanswered probes leave indices behind, while a scan abandoned at the +/// startup budget leaves none and is no more complete for it. +/// +/// Carried as `Option` — at most one scan verdict per +/// persist round, last-write-wins, which is correct because a later scan's +/// verdict wholly supersedes an earlier one's. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct IdentityScanStateEntry { + /// Every index the scan probed was answered. Only a `true` here may let a + /// later launch skip discovery. + pub complete: bool, + /// One past the highest index the scan probed. + pub probed_through: u32, + /// Indices whose probe never got an answer, ascending. Empty for a scan + /// that was cut off before it could fail anything. + pub failed_indices: Vec, +} + +impl IdentityScanStateEntry { + /// A scan that answered every index it probed. + pub fn completed(probed_through: u32) -> Self { + Self { + complete: true, + probed_through, + failed_indices: Vec::new(), + } + } + + /// A scan that left at least one index unanswered, or was abandoned + /// before it could finish. + pub fn incomplete(probed_through: u32, failed_indices: Vec) -> Self { + Self { + complete: false, + probed_through, + failed_indices, + } + } +} + /// One entry per registered account. Captures the per-account xpub /// + type so a future load path can rebuild the wallet watch-only /// via `Account::from_xpub`. Hardened derivation at the account @@ -1606,6 +1667,18 @@ pub struct PlatformWalletChangeSet { /// Per-wallet metadata emitted once at registration. See /// [`WalletMetadataEntry`] for the merge policy. pub wallet_metadata: Option, + /// Verdict of the most recent gap-limit identity scan. Emitted by + /// discovery and by the startup sequence when it abandons a scan; read on + /// the next launch to decide whether the warm-launch shortcut may skip + /// discovery. See [`IdentityScanStateEntry`]. + /// + /// Durability caveat, the same one [`Self::pending_contact_crypto_added`] + /// carries: no persister vtable has a slot for this field yet, so on + /// hosts that have not adopted it the verdict is process-lifetime only. + /// Within a process it still redirects a second bring-up, and a partial + /// scan is now retried inside its own launch — but closing + /// dashpay/platform#4365 across launches needs the host slot. + pub identity_scan_state: Option, /// Per-account registration entries emitted at registration / on /// later `add_account` calls. See [`AccountRegistrationEntry`] for /// the merge policy (plain `Vec::extend`, dedup is the apply-side @@ -1741,6 +1814,13 @@ impl Merge for PlatformWalletChangeSet { if let Some(meta) = other.wallet_metadata { self.wallet_metadata = Some(meta); } + // Identity-scan verdict: last-write-wins. A later scan's verdict + // wholly supersedes an earlier one's — merging two would have to + // invent a rule for combining a complete scan with an incomplete one, + // and either answer would be wrong for one of them. + if let Some(scan) = other.identity_scan_state { + self.identity_scan_state = Some(scan); + } // Per-account specs and address-pool snapshots: append-only. // See the type docstrings for the rationale (registration // round emits each key once; snapshots are whole-pool, so @@ -1779,6 +1859,7 @@ impl Merge for PlatformWalletChangeSet { .as_ref() .is_none_or(|m| m.is_empty()) && self.wallet_metadata.is_none() + && self.identity_scan_state.is_none() && self.account_registrations.is_empty() && self.provider_key_account_registrations.is_empty() && self.account_address_pools.is_empty() diff --git a/packages/rs-platform-wallet/src/changeset/identity_manager_start_state.rs b/packages/rs-platform-wallet/src/changeset/identity_manager_start_state.rs index fbb42fa9e09..a4072b61848 100644 --- a/packages/rs-platform-wallet/src/changeset/identity_manager_start_state.rs +++ b/packages/rs-platform-wallet/src/changeset/identity_manager_start_state.rs @@ -8,6 +8,7 @@ use std::collections::BTreeMap; use dpp::prelude::Identifier; +use crate::changeset::IdentityScanStateEntry; use crate::wallet::identity::ManagedIdentity; use crate::wallet::identity::RegistrationIndex; use crate::wallet::platform_wallet::WalletId; @@ -26,4 +27,13 @@ pub struct IdentityManagerStartState { /// Wallet-owned identities, outer-keyed by wallet id and /// inner-keyed by BIP-9 registration index. pub wallet_identities: BTreeMap>, + /// Per-wallet verdict of the last gap-limit identity scan. + /// + /// An absent entry means "no verdict was restored", which is NOT the same + /// as a complete scan — a host that does not persist the verdict yet, and + /// a wallet whose first scan has not run, both land here. Absence + /// therefore preserves the existing warm-launch behaviour rather than + /// claiming a guarantee nobody made; only a restored `complete: false` + /// forces a rescan. See [`IdentityScanStateEntry`]. + pub scan_states: BTreeMap, } diff --git a/packages/rs-platform-wallet/src/changeset/mod.rs b/packages/rs-platform-wallet/src/changeset/mod.rs index e87cc14aeec..4ecb036faec 100644 --- a/packages/rs-platform-wallet/src/changeset/mod.rs +++ b/packages/rs-platform-wallet/src/changeset/mod.rs @@ -31,12 +31,12 @@ pub use changeset::{ AssetLockChangeSet, AssetLockEntry, ContactChangeSet, ContactRequestEntry, CoreChangeSet, DpnsNameSaleStatus, DpnsNameStateChangeSet, DpnsNameStateEntry, HighestUsedIndexes, IdentityChangeSet, IdentityEntry, IdentityKeyDerivationIndices, IdentityKeyEntry, - IdentityKeysChangeSet, InvitationChangeSet, InvitationEntry, InvitationStatus, - KeyDerivationBreadcrumb, KeyWithBreadcrumb, PendingContactCrypto, PendingContactCryptoKey, - PendingContactCryptoKind, PendingContactCryptoOp, PlatformAddressBalanceEntry, - PlatformAddressChangeSet, PlatformWalletChangeSet, ProviderKeyAccountEntry, - ProviderKeyExtendedPubKey, ProviderPlatformNodePubKey, ReceivedContactRequestKey, - SentContactRequestKey, TokenBalanceChangeSet, WalletMetadataEntry, + IdentityKeysChangeSet, IdentityScanStateEntry, InvitationChangeSet, InvitationEntry, + InvitationStatus, KeyDerivationBreadcrumb, KeyWithBreadcrumb, PendingContactCrypto, + PendingContactCryptoKey, PendingContactCryptoKind, PendingContactCryptoOp, + PlatformAddressBalanceEntry, PlatformAddressChangeSet, PlatformWalletChangeSet, + ProviderKeyAccountEntry, ProviderKeyExtendedPubKey, ProviderPlatformNodePubKey, + ReceivedContactRequestKey, SentContactRequestKey, TokenBalanceChangeSet, WalletMetadataEntry, }; pub use client_start_state::ClientStartState; pub use client_wallet_start_state::ClientWalletStartState; diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 8349eb1df21..6451048d4fb 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -453,6 +453,24 @@ pub enum PlatformWalletError { wallet_id: String, }, + #[error( + "Contact-request sync reached none of the wallet's {identities} identities \ + (Platform unreachable) — the pass did not complete" + )] + /// A contact-request pass had identities to fetch for and could not read a + /// single one of them. Distinct from an empty success, which means + /// "Platform answered, and there is nothing new": this one means we do not + /// know, so the caller must not record the pass as completed. + /// + /// The sweep's per-identity log-and-continue collapsed the two, so a DAPI + /// outage returned `Ok(vec![])` and a startup sequence recorded a + /// successful contact sync — then reported `Ready`, promising that every + /// contact's DIP-15 addresses existed before Core SPV started. + ContactSyncUnreachable { + /// Identities the pass tried, and failed, to fetch for. + identities: usize, + }, + #[error("SPV is already running — stop it before starting again")] SpvAlreadyRunning, diff --git a/packages/rs-platform-wallet/src/manager/startup.rs b/packages/rs-platform-wallet/src/manager/startup.rs index 1b5ac7fa031..51d092ac676 100644 --- a/packages/rs-platform-wallet/src/manager/startup.rs +++ b/packages/rs-platform-wallet/src/manager/startup.rs @@ -202,6 +202,19 @@ pub enum WalletStartupStatus { /// is certain, not the reason. Either way those contacts' payments wait on /// the DIP-15 rescan. PartialAccountsPending, + /// The contact-crypto provider does not resolve the seed that owns this + /// wallet, so the drain was skipped without deriving anything. + /// + /// Not a slow-Platform outcome like the other partials — it says the host + /// handed this call a signer for a different wallet, and the only safe + /// response was to do nothing. Deriving anyway would write contact + /// receiving xpubs from the wrong seed, and because + /// `register_contact_account` keys its existence check on the contact pair + /// rather than on the xpub, those wrong addresses would be written once + /// and never revisited by a later correct-seed pass. The wallet would then + /// watch addresses nobody pays to, with no symptom but payments that never + /// arrive. + SeedBindingUnverified, } impl WalletStartupStatus { @@ -240,9 +253,16 @@ pub struct WalletStartupOutcome { /// Discovery scans performed. `0` when a local identity was already known /// and no network scan was needed. pub discovery_attempts: u32, - /// Whether the inline DashPay sync pass ran (skipped when there is no - /// identity to sync for). + /// Whether the inline DashPay sync pass ran **to completion**. `false` + /// when it was skipped, failed, ran out of budget, or came back degraded — + /// a pass that could not read some identities' contact documents leaves + /// their account builds unenqueued, so it is not a pass the caller may + /// rely on. pub dashpay_sync_ran: bool, + /// The contact-account drain was skipped because the supplied + /// contact-crypto provider does not resolve this wallet's seed. Nothing + /// was derived and nothing was written; the queue is intact. + pub seed_binding_unverified: bool, /// Contact-crypto entries completed by the drain. pub contact_accounts_drained: usize, /// Contact-account builds still queued when this returned. @@ -273,6 +293,9 @@ pub(crate) struct StartupTally { pub discovery_failed_locally: bool, pub discovery_attempts: u32, pub dashpay_sync_ran: bool, + /// The drain was skipped because the contact-crypto provider could not be + /// shown to resolve this wallet's seed. + pub seed_binding_unverified: bool, pub contact_accounts_drained: usize, pub contact_accounts_pending: usize, } @@ -326,6 +349,12 @@ impl StartupTally { self.dashpay_sync_ran = true; } + /// The seed behind the contact-crypto provider could not be shown to own + /// this wallet, so the drain never ran. + pub(crate) fn record_seed_binding_unverified(&mut self) { + self.seed_binding_unverified = true; + } + pub(crate) fn record_drain(&mut self, drained: usize, pending: usize) { self.contact_accounts_drained = drained; self.contact_accounts_pending = pending; @@ -338,17 +367,33 @@ impl StartupTally { /// absence outranks the drain counters for the same reason — with no /// identity there is nothing to have drained. pub(crate) fn status(&self) -> WalletStartupStatus { + // Both of these say "the identity question is still open", so neither + // may decide the verdict once an identity is known. That used to be + // structurally impossible — discovery ran only when nothing was on + // file, and every branch that found something returned early — but a + // rescan forced by an incomplete prior scan reaches them with an + // identity already recorded, and reporting *that* launch as + // `DiscoveryFailed` would hide a sync and drain that both ran. + // // A local fault outranks unreachability: both leave the question open, // but only this one tells the client not to bother asking again. - if self.discovery_failed_locally { + if self.discovery_failed_locally && self.identity_id.is_none() { return WalletStartupStatus::DiscoveryFailed; } - if self.discovery_unreachable { + if self.discovery_unreachable && self.identity_id.is_none() { return WalletStartupStatus::PartialNoIdentity; } if self.proven_no_identity && self.identity_id.is_none() { return WalletStartupStatus::NoIdentity; } + // Outranks the queue counters, and must: a wrong-seed provider is why + // the queue was not drained, and it is the one ending here that points + // at a host misconfiguration rather than at Platform being slow. It + // also has to outrank `Ready` — with an empty queue every other signal + // would read as a clean run. + if self.seed_binding_unverified { + return WalletStartupStatus::SeedBindingUnverified; + } if self.contact_accounts_pending > 0 { return WalletStartupStatus::PartialAccountsPending; } @@ -368,6 +413,7 @@ impl StartupTally { identity_id: self.identity_id, discovery_attempts: self.discovery_attempts, dashpay_sync_ran: self.dashpay_sync_ran, + seed_binding_unverified: self.seed_binding_unverified, contact_accounts_drained: self.contact_accounts_drained, contact_accounts_pending: self.contact_accounts_pending, elapsed, @@ -440,19 +486,49 @@ impl PlatformWalletManager let identity_wallet = wallet.identity(); // 1. Local identities first. A warm launch must not pay for a network - // scan it does not need. - if let Some(known) = self.local_identity_id(wallet_id).await { - tally.record_local_identity(known); - } else { - self.discover_identity_with_backoff( - wallet_id, - identity_wallet, - scan_key, - opts.gap_limit, - deadline, - &mut tally, - ) - .await; + // scan it does not need — unless the scan that produced those + // identities is on record as having left indices unanswered, in + // which case "we already have one" is not evidence that we have + // them all. A wallet whose second identity was hidden by a failed + // probe used to stay that way for the life of the installation, + // because this shortcut is the only thing that would have looked + // again (dashpay/platform#4365). + // + // Only a recorded incomplete scan re-opens the question. An absent + // verdict keeps the shortcut, so hosts that do not persist it are + // exactly where they were rather than paying for a scan every + // launch. + let scan_incomplete = self.identity_scan_is_incomplete(wallet_id).await; + match self.local_identity_id(wallet_id).await { + Some(known) if !scan_incomplete => tally.record_local_identity(known), + Some(known) => { + tracing::info!( + wallet_id = %hex::encode(wallet_id), + "startup: the last identity scan left indices unanswered; rescanning \ + rather than trusting the identities already on file" + ); + tally.record_local_identity(known); + self.discover_identity_with_backoff( + wallet_id, + identity_wallet, + scan_key, + opts.gap_limit, + deadline, + &mut tally, + ) + .await; + } + None => { + self.discover_identity_with_backoff( + wallet_id, + identity_wallet, + scan_key, + opts.gap_limit, + deadline, + &mut tally, + ) + .await; + } } // With no identity there is nothing to sync and nothing to drain, and @@ -464,15 +540,44 @@ impl PlatformWalletManager // 2. One contact-request pass, so the deferred builds exist to drain. // Log-and-continue: a prior session may already have queued work // that this call can still complete. - match within_budget(deadline, identity_wallet.dashpay().sync_contact_requests()).await { - Some(Ok(requests)) => { + match within_budget( + deadline, + identity_wallet.dashpay().sync_contact_requests_reporting(), + ) + .await + { + Some(Ok(report)) if report.is_complete() => { tally.record_sync_ran(); tracing::debug!( wallet_id = %hex::encode(wallet_id), - requests = requests.len(), + requests = report.requests.len(), + identities = report.identities_attempted, "startup: contact-request pass complete" ); } + // Reached Platform for some identities and not others (or for none + // at all). The requests it did fetch are real and already + // persisted, but the identities it missed have contact requests + // nobody has looked at, whose account builds were therefore never + // enqueued — so the queue being empty below proves nothing. Not + // recording the pass keeps `status()` off `Ready`, which is the + // promise that every contact's DIP-15 addresses exist before Core + // SPV starts. + // + // The failures retry themselves: a fetch that errored leaves that + // direction's high-water cursor unadvanced, so the next sweep + // re-requests exactly the range this pass missed. + Some(Ok(report)) => { + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + requests = report.requests.len(), + identities = report.identities_attempted, + failed = report.failed_identities.len(), + degraded = report.degraded_identities.len(), + "startup: contact-request pass was degraded; not recording it as a \ + completed sync" + ); + } Some(Err(e)) => { tracing::warn!( wallet_id = %hex::encode(wallet_id), @@ -504,22 +609,70 @@ impl PlatformWalletManager // `contact_accounts_pending`, rather than a drain that reports zero // because every crypto operation failed. let (drained, accepted) = match contact_crypto { - Some(contact_crypto) => { - let drained = identity_wallet + // Nothing queued means the drain would derive nothing, so there is + // no wrong-seed write to prevent and no reason to pay for the check + // below. Covers both drains: auto-accepts ride the same queue. + // Keeping the gate's cost proportional to its risk is what lets it + // live here — a warm launch with an empty queue still resolves no + // key material at all. + Some(contact_crypto) + if identity_wallet .dashpay() - .drain_pending_contact_crypto_until(contact_crypto, Some(deadline)) - .await; - let accepted = match identity_signer { - Some(signer) => { - identity_wallet - .dashpay() - .drain_auto_accepts_until(signer, contact_crypto, Some(deadline)) - .await - } - None => 0, - }; - (drained, accepted) + .drainable_contact_crypto_count() + .await + > 0 => + { + // Everything past this point derives from whatever seed the + // provider resolves, and none of it is authenticated. A + // provider mapped to the wrong wallet derives contact receiving + // xpubs from the wrong seed, and `register_contact_account` + // keys its existence check on `(index, us, them)` — not on the + // xpub — so the wrong addresses are written once and every + // later correct-seed pass no-ops. The corruption is permanent + // and its only symptom is payments that never arrive. + // + // The gate belongs here rather than in each client for the + // same reason the ordering does: iOS enforces it in its Swift + // wrapper today, and a client that has to remember to gate this + // call is a client that will eventually forget. A JNI binding + // added later inherits the gate instead of the bug. + // + // Fail closed on every error, not only on a mismatch. A + // provider that cannot answer has not been shown to own this + // wallet, and skipping costs nothing that is not recoverable: + // the queue is untouched, so the next signer-present drain + // completes exactly the work this one declined to guess at. + if let Err(e) = wallet.verify_seed_binds(contact_crypto).await { + tally.record_seed_binding_unverified(); + tracing::error!( + wallet_id = %hex::encode(wallet_id), + error = %e, + "startup: the contact-crypto provider does not bind to this wallet's \ + seed; skipping the drain rather than deriving contact addresses that \ + could never be corrected" + ); + (0, 0) + } else { + let drained = identity_wallet + .dashpay() + .drain_pending_contact_crypto_until(contact_crypto, Some(deadline)) + .await; + let accepted = match identity_signer { + Some(signer) => { + identity_wallet + .dashpay() + .drain_auto_accepts_until(signer, contact_crypto, Some(deadline)) + .await + } + None => 0, + }; + (drained, accepted) + } } + // A provider was supplied and the queue is empty — the ordinary + // warm launch. Nothing to drain, nothing to verify, nothing to + // report beyond the pending count read below. + Some(_) => (0, 0), None => { tracing::info!( wallet_id = %hex::encode(wallet_id), @@ -548,6 +701,20 @@ impl PlatformWalletManager Ok(tally.into_outcome(started.elapsed())) } + /// Whether this wallet's last gap-limit scan is on record as having left + /// indices unanswered. + /// + /// `false` when no verdict is known — see + /// [`IdentityManager::identity_scan_is_incomplete`] for why "unknown" must + /// not read as "incomplete". + /// + /// [`IdentityManager::identity_scan_is_incomplete`]: crate::wallet::identity::IdentityManager::identity_scan_is_incomplete + async fn identity_scan_is_incomplete(&self, wallet_id: &WalletId) -> bool { + let wm = self.wallet_manager.read().await; + wm.get_wallet_info(wallet_id) + .is_some_and(|info| info.identity_manager.identity_scan_is_incomplete(wallet_id)) + } + /// The first identity this wallet already owns locally, if any. async fn local_identity_id(&self, wallet_id: &WalletId) -> Option { let wm = self.wallet_manager.read().await; @@ -625,6 +792,15 @@ impl PlatformWalletManager } }; let Some(result) = within_budget(deadline, attempt_future).await else { + // Dropped mid-await, so the scan recorded no verdict of its + // own. Record one here: an abandoned scan probed an unknown + // prefix of the index space and answered the rest of it not at + // all, which is exactly the state a later launch must not + // mistake for a settled identity set. Without this the + // budget-expiry path reproduces #4365 in its own right — it + // consults local state, finds the sighting that was persisted + // before cancellation, and records a warm launch. + self.record_identity_scan_cut_off(wallet_id).await; // Sightings persist incrementally, so an abandoned scan may // still have folded an identity in before it was cut off. if let Some(known) = self.local_identity_id(wallet_id).await { @@ -636,19 +812,44 @@ impl PlatformWalletManager match result { Ok(found) => { - match found.first() { - Some(identity) => tally.record_discovered(identity.id()), + let identity = match found.first() { + Some(identity) => Some(identity.id()), // An empty return is not proof on its own: `discover` // reports only identities THIS call inserted, so a // concurrent startup that inserted one first leaves us // seeing it as already-managed and returning nothing. // Consult local state before calling it absence. - None => match self.local_identity_id(wallet_id).await { - Some(known) => tally.record_discovered(known), - None => tally.record_proven_absent(), - }, + None => self.local_identity_id(wallet_id).await, + }; + let Some(identity) = identity else { + tally.record_proven_absent(); + return; + }; + tally.record_discovered(identity); + // `Ok` does not mean "every index was answered": a scan + // that saw an identity is reported as trustworthy even + // when a later probe went unanswered, and an identity + // hiding at that index is invisible until something scans + // again. Retry it here, inside the budget the caller + // already granted and with the scan key already resolved, + // rather than leaving it to a launch that may never come. + if !self.identity_scan_is_incomplete(wallet_id).await { + return; } - return; + if backoff.is_none() { + // Out of attempts. The verdict stays on record, so the + // next launch re-opens the question instead of taking + // the warm shortcut. + tracing::warn!( + "startup: identity discovery still has unanswered indices after \ + every attempt; the recorded verdict will force a rescan" + ); + return; + } + tracing::info!( + attempt = attempt + 1, + "startup: identity discovery left indices unanswered; rescanning" + ); } Err(PlatformWalletError::IdentityDiscoveryIncomplete { .. }) => { tally.record_unreachable(); @@ -678,7 +879,46 @@ impl PlatformWalletManager tokio::time::sleep((*backoff).min(remaining)).await; } - tally.record_discovery_gave_up(); + // Only meaningful while the identity question is still open. The + // partial-scan retry above can exhaust the loop with an identity + // already recorded, and that launch is not an unreachable-Platform + // launch — it found something, it just could not prove it found + // everything. + if !tally.has_identity() { + tally.record_discovery_gave_up(); + } + } + + /// Record that a scan was abandoned before it could answer every index. + /// + /// Mirrors what `discover` publishes for itself; needed separately because + /// a scan dropped mid-await never reaches its own bookkeeping. + async fn record_identity_scan_cut_off(&self, wallet_id: &WalletId) { + { + let mut wm = self.wallet_manager.write().await; + match wm.get_wallet_info_mut(wallet_id) { + Some(info) => info.identity_manager.record_identity_scan( + *wallet_id, + crate::changeset::IdentityScanStateEntry::incomplete(0, Vec::new()), + ), + None => return, + } + } + let changeset = crate::changeset::PlatformWalletChangeSet { + identity_scan_state: Some(crate::changeset::IdentityScanStateEntry::incomplete( + 0, + Vec::new(), + )), + ..Default::default() + }; + if let Err(e) = self.persister.store(*wallet_id, changeset) { + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + error = %e, + "failed to persist an abandoned scan's verdict; the next launch may take the \ + warm shortcut over an incomplete identity set" + ); + } } } @@ -752,6 +992,7 @@ mod tests { WalletStartupStatus::NoIdentity, WalletStartupStatus::PartialAccountsPending, WalletStartupStatus::DiscoveryFailed, + WalletStartupStatus::SeedBindingUnverified, ] { assert!( !terminal.discovery_worth_retrying(), @@ -836,6 +1077,83 @@ mod tests { assert!(!tally.has_identity()); } + /// A contact pass that could not read some identities' documents is not a + /// completed pass, and the whole point of tracking that is to keep it off + /// `Ready`. `Ready` promises the DIP-15 addresses exist before Core SPV + /// starts; a degraded pass never enqueued the account builds for the + /// identities it missed, so the queue being empty proves nothing. + #[test] + fn a_degraded_contact_pass_is_not_ready_even_with_an_empty_queue() { + let mut tally = StartupTally::default(); + tally.record_discovered(identity()); + // Deliberately no `record_sync_ran` — this is what startup does when + // the report comes back degraded. + tally.record_drain(0, 0); + + assert_eq!(tally.status(), WalletStartupStatus::PartialAccountsPending); + assert!(!tally.dashpay_sync_ran); + } + + /// The wrong-seed ending outranks every other non-discovery verdict, + /// including a clean-looking drain. With an empty queue the run is + /// otherwise indistinguishable from a healthy one, and reporting it as + /// `Ready` would hide the single condition here that points at a host + /// misconfiguration rather than at Platform being slow. + #[test] + fn an_unverified_seed_binding_outranks_a_clean_drain() { + let mut tally = StartupTally::default(); + tally.record_discovered(identity()); + tally.record_sync_ran(); + tally.record_seed_binding_unverified(); + tally.record_drain(0, 0); + + assert_eq!(tally.status(), WalletStartupStatus::SeedBindingUnverified); + assert!( + tally.status().identity_is_settled(), + "the identity was found; it is the drain that did not run" + ); + assert!(!tally.status().discovery_worth_retrying()); + } + + /// The outcome carries the flag so a client can tell "nothing was queued" + /// from "we refused to derive". + #[test] + fn an_unverified_seed_binding_reaches_the_outcome() { + let mut tally = StartupTally::default(); + tally.record_discovered(identity()); + tally.record_sync_ran(); + tally.record_seed_binding_unverified(); + + let outcome = tally.into_outcome(Duration::from_secs(1)); + assert!(outcome.seed_binding_unverified); + assert_eq!(outcome.status, WalletStartupStatus::SeedBindingUnverified); + } + + /// A rescan forced by an incomplete prior scan can now reach the + /// discovery-failure branches with an identity already on file. Those + /// statuses say "the identity question is still open", which would be a + /// lie here — and it would also hide a sync and drain that both ran. + #[test] + fn a_failed_rescan_does_not_reopen_a_settled_identity() { + let mut unreachable = StartupTally::default(); + unreachable.record_local_identity(identity()); + unreachable.record_unreachable(); + unreachable.record_discovery_gave_up(); + unreachable.record_sync_ran(); + unreachable.record_drain(1, 0); + assert_eq!(unreachable.status(), WalletStartupStatus::Ready); + + let mut local_fault = StartupTally::default(); + local_fault.record_local_identity(identity()); + local_fault.record_discovery_failed_locally(); + local_fault.record_sync_ran(); + local_fault.record_drain(0, 2); + assert_eq!( + local_fault.status(), + WalletStartupStatus::PartialAccountsPending + ); + } + /// Every network step is abandonable, so `within_budget` must return /// `None` rather than run a future past the deadline. This is the guard for /// the gap review found: bounding only the discovery retries let a stalled @@ -868,6 +1186,349 @@ mod tests { assert_eq!(within_budget(deadline, async { "ran" }).await, None); } + // --------------------------------------------------------------------- + // End-to-end: the seed-binding gate in front of the drain. + // + // Driven through the real `start_wallet_subsystems` over a mock SDK, so + // what is asserted is the sequence's actual behaviour rather than a + // restatement of the tally rules above. + // --------------------------------------------------------------------- + + /// Canonical all-`abandon` BIP-39 vector — the seed + /// `test_platform_wallet_manager` builds its wallet from. + const OWNING_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon \ + abandon abandon abandon abandon abandon about"; + /// A different valid BIP-39 vector: the mis-mapped Keychain slot. + const FOREIGN_MNEMONIC: &str = + "legal winner thank year wave sausage worth useful legal winner thank yellow"; + + /// Only ever passed as `None`, so the sequence skips the DIP-15 + /// auto-accept pass — but the generic still has to be named. + #[derive(Debug)] + struct UnusedSigner; + + #[async_trait::async_trait] + impl Signer for UnusedSigner { + async fn sign( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + unreachable!("the auto-accept pass is never reached with a None signer") + } + + async fn sign_create_witness( + &self, + _key: &IdentityPublicKey, + _data: &[u8], + ) -> Result { + unreachable!("the auto-accept pass is never reached with a None signer") + } + + fn can_sign_with(&self, _key: &IdentityPublicKey) -> bool { + false + } + } + + fn seed_for(phrase: &str) -> [u8; 64] { + use key_wallet::mnemonic::{Language, Mnemonic}; + Mnemonic::from_phrase(phrase, Language::English) + .expect("valid test mnemonic") + .to_seed("") + } + + fn test_identity(id_byte: u8) -> dpp::identity::Identity { + use dpp::identity::v0::IdentityV0; + dpp::identity::Identity::V0(IdentityV0 { + id: Identifier::from([id_byte; 32]), + public_keys: std::collections::BTreeMap::new(), + balance: 0, + revision: 0, + }) + } + + /// A manager holding one wallet that owns one identity with a single + /// queued `RegisterReceiving` op — the smallest state in which the drain + /// has real work, and the op that derives a contact receiving xpub + /// straight from the provider with no network round trip. + async fn manager_with_queued_contact_crypto() -> ( + std::sync::Arc>, + WalletId, + ) { + use crate::changeset::{ + upsert_pending_contact_crypto, PendingContactCrypto, PendingContactCryptoOp, + }; + use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; + + let (manager, wallet_id) = crate::test_support::test_platform_wallet_manager().await; + let persister = WalletPersister::new(wallet_id, std::sync::Arc::new(NoPlatformPersistence)); + + let mut wm = manager.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("wallet info"); + info.identity_manager + .add_identity(test_identity(1), 0, wallet_id, &persister) + .expect("add identity"); + let managed = info + .identity_manager + .managed_identity_mut(&Identifier::from([1u8; 32])) + .expect("managed identity"); + upsert_pending_contact_crypto( + managed.dashpay_pending_contact_crypto_mut(), + PendingContactCrypto { + owner_identity_id: Identifier::from([1u8; 32]), + contact_id: Identifier::from([2u8; 32]), + op: PendingContactCryptoOp::RegisterReceiving, + enqueued_at_ms: 0, + }, + ); + drop(wm); + + (manager, wallet_id) + } + + /// Count the DashPay receiving accounts the wallet is watching. The thing + /// a wrong-seed drain would corrupt: `register_contact_account` keys its + /// existence check on `(index, us, them)` and NOT on the xpub, so an + /// account written from the wrong seed is never revisited. + async fn receiving_account_count( + manager: &crate::PlatformWalletManager, + wallet_id: &WalletId, + ) -> usize { + let wm = manager.wallet_manager.read().await; + wm.get_wallet_info(wallet_id) + .map(|info| info.core_wallet.accounts.dashpay_receival_accounts.len()) + .unwrap_or(0) + } + + async fn drainable( + manager: &crate::PlatformWalletManager, + wallet_id: &WalletId, + ) -> usize { + let wallet = manager.get_wallet(wallet_id).await.expect("wallet"); + wallet + .identity() + .dashpay() + .drainable_contact_crypto_count() + .await + } + + /// The defect this gate closes: a provider resolving someone else's seed + /// derives contact receiving xpubs that are written once and never + /// corrected, so the wallet watches addresses nobody pays to. The drain + /// must not run at all, and the queue must survive intact for the next + /// signer-present attempt. + #[tokio::test] + async fn a_wrong_seed_provider_never_reaches_the_drain() { + use crate::wallet::identity::network::SeedCryptoProvider; + + let (manager, wallet_id) = manager_with_queued_contact_crypto().await; + assert_eq!(receiving_account_count(&manager, &wallet_id).await, 0); + assert_eq!(drainable(&manager, &wallet_id).await, 1); + + let foreign = + SeedCryptoProvider::from_seed(seed_for(FOREIGN_MNEMONIC), key_wallet::Network::Testnet); + let outcome = manager + .start_wallet_subsystems( + &wallet_id, + None, + Some(&foreign), + None::<&UnusedSigner>, + WalletStartupOptions::default(), + ) + .await + .expect("a wrong seed is reported, not raised"); + + assert_eq!(outcome.status, WalletStartupStatus::SeedBindingUnverified); + assert!(outcome.seed_binding_unverified); + assert_eq!( + outcome.contact_accounts_drained, 0, + "nothing may be drained with a provider that does not own the wallet" + ); + assert_eq!( + receiving_account_count(&manager, &wallet_id).await, + 0, + "not one contact account may be registered from the wrong seed" + ); + assert_eq!( + drainable(&manager, &wallet_id).await, + 1, + "the queue must survive so the next signer-present drain can do the work" + ); + } + + /// The other half: the wallet's own seed passes the gate and the drain + /// runs. Without this the test above would also pass if the gate simply + /// refused everything. + #[tokio::test] + async fn the_owning_seed_passes_the_gate_and_the_drain_runs() { + use crate::wallet::identity::network::SeedCryptoProvider; + + let (manager, wallet_id) = manager_with_queued_contact_crypto().await; + let owning = + SeedCryptoProvider::from_seed(seed_for(OWNING_MNEMONIC), key_wallet::Network::Testnet); + + let outcome = manager + .start_wallet_subsystems( + &wallet_id, + None, + Some(&owning), + None::<&UnusedSigner>, + WalletStartupOptions::default(), + ) + .await + .expect("bring-up reports rather than raises"); + + assert!( + !outcome.seed_binding_unverified, + "the wallet's own seed must bind" + ); + assert_ne!(outcome.status, WalletStartupStatus::SeedBindingUnverified); + assert_eq!( + outcome.contact_accounts_drained, 1, + "the queued RegisterReceiving op must have been completed" + ); + assert_eq!( + receiving_account_count(&manager, &wallet_id).await, + 1, + "the contact receiving account must exist after a verified drain" + ); + } + + /// The gate is paid for only when there is something to protect. An empty + /// queue means the drain would derive nothing, so no key material is + /// resolved — which is what keeps this affordable on a warm launch. + /// Proven with a provider that would FAIL the check: reaching a status + /// other than `SeedBindingUnverified` shows it was never consulted. + #[tokio::test] + async fn an_empty_queue_skips_the_gate_entirely() { + use crate::changeset::{PendingContactCryptoKey, PendingContactCryptoKind}; + use crate::wallet::identity::network::SeedCryptoProvider; + + let (manager, wallet_id) = manager_with_queued_contact_crypto().await; + // Empty the queue so the drain has nothing to do. + { + let mut wm = manager.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("wallet info"); + let managed = info + .identity_manager + .managed_identity_mut(&Identifier::from([1u8; 32])) + .expect("managed identity"); + let key = PendingContactCryptoKey { + owner_identity_id: Identifier::from([1u8; 32]), + contact_id: Identifier::from([2u8; 32]), + kind: PendingContactCryptoKind::RegisterReceiving, + }; + managed + .dashpay_pending_contact_crypto_mut() + .retain(|e| e.key() != key); + } + assert_eq!(drainable(&manager, &wallet_id).await, 0); + + let foreign = + SeedCryptoProvider::from_seed(seed_for(FOREIGN_MNEMONIC), key_wallet::Network::Testnet); + let outcome = manager + .start_wallet_subsystems( + &wallet_id, + None, + Some(&foreign), + None::<&UnusedSigner>, + WalletStartupOptions::default(), + ) + .await + .expect("bring-up reports rather than raises"); + + assert!( + !outcome.seed_binding_unverified, + "with nothing to drain the binding check must not run at all" + ); + } + + /// The F1 regression, end to end and against a Platform that answers + /// nothing (the mock SDK fails every contact fetch, which is exactly the + /// DAPI-unreachable shape). + /// + /// Before the fix this pass returned `Ok(vec![])`, startup called + /// `record_sync_ran`, and a wallet whose contacts had never been read + /// reported `Ready` — the status that promises every contact's DIP-15 + /// addresses exist before Core SPV starts. + #[tokio::test] + async fn a_contact_pass_that_reached_nobody_is_not_a_completed_sync() { + use crate::wallet::identity::network::SeedCryptoProvider; + + let (manager, wallet_id) = manager_with_queued_contact_crypto().await; + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + + // The pass itself: one identity attempted, none reached. + let report = wallet + .identity() + .dashpay() + .sync_contact_requests_reporting() + .await + .expect("the pass returns a report"); + assert_eq!(report.identities_attempted, 1); + assert_eq!(report.failed_identities.len(), 1); + assert!(!report.is_complete()); + assert!(report.is_fully_degraded()); + + // The back-compat return shape can no longer render this as success. + let err = wallet + .identity() + .dashpay() + .sync_contact_requests() + .await + .expect_err("reaching nobody must not look like an empty result"); + assert!( + matches!( + err, + PlatformWalletError::ContactSyncUnreachable { identities: 1 } + ), + "expected ContactSyncUnreachable, got: {err:?}" + ); + + // The cursors stayed put, so the next sweep re-requests the same + // range — this is what makes the failure retried rather than buried. + { + let wm = manager.wallet_manager.read().await; + let managed = wm + .get_wallet_info(&wallet_id) + .expect("wallet info") + .identity_manager + .managed_identity(&Identifier::from([1u8; 32])) + .expect("managed identity"); + assert_eq!( + managed.dashpay().high_water_received_ms(), + None, + "a failed fetch must not advance the cursor past requests it never read" + ); + assert_eq!(managed.dashpay().high_water_sent_ms(), None); + } + + // And the sequence must not record it as a sync that ran. + let owning = + SeedCryptoProvider::from_seed(seed_for(OWNING_MNEMONIC), key_wallet::Network::Testnet); + let outcome = manager + .start_wallet_subsystems( + &wallet_id, + None, + Some(&owning), + None::<&UnusedSigner>, + WalletStartupOptions::default(), + ) + .await + .expect("bring-up reports rather than raises"); + + assert!( + !outcome.dashpay_sync_ran, + "a pass that read none of the wallet's identities is not a completed sync" + ); + assert_ne!( + outcome.status, + WalletStartupStatus::Ready, + "Ready promises contact addresses this call never prepared" + ); + assert_eq!(outcome.status, WalletStartupStatus::PartialAccountsPending); + } + #[test] fn outcome_carries_the_tally_through() { let mut tally = StartupTally::default(); diff --git a/packages/rs-platform-wallet/src/wallet/apply.rs b/packages/rs-platform-wallet/src/wallet/apply.rs index 4390740640c..8e36af5768d 100644 --- a/packages/rs-platform-wallet/src/wallet/apply.rs +++ b/packages/rs-platform-wallet/src/wallet/apply.rs @@ -106,6 +106,7 @@ impl PlatformWalletInfo { // replay hook. invitations: _, dpns_name_states, + identity_scan_state, // Registration-round metadata / per-account specs / // per-pool snapshots are persistence-only — the // canonical in-memory wallet state is built up at @@ -162,6 +163,18 @@ impl PlatformWalletInfo { } } + // 2a'. Identity-scan verdict. Replayed rather than dropped: unlike the + // registration metadata below it, this one has live in-memory + // state on the identity manager, and it is read on the next + // bring-up to decide whether the identity set may be treated as + // settled. A verdict that survived to persistence and then got + // dropped on the way back in would leave a partial scan looking + // complete — the exact failure the verdict exists to prevent. + if let Some(scan) = identity_scan_state { + self.identity_manager + .record_identity_scan(wallet.wallet_id, scan); + } + // 2a. DPNS name states (username marketplace): upserts land // first, then tombstones, into the in-memory working set — // same LWW-then-remove discipline as the rest of this diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs index c99018792cf..b8f6d7bbd09 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs @@ -1089,6 +1089,63 @@ fn count_account_build_ops(queue: &[crate::changeset::PendingContactCrypto]) -> .count() } +/// What one contact-request pass actually reached, as opposed to what it +/// returned. +/// +/// The sweep is deliberately log-and-continue per identity: one identity's +/// transient DAPI error must not stall DashPay sync for every other identity +/// on the wallet. That is right for a recurring background sweep and wrong for +/// anything that treats the pass as a precondition, because the two endings it +/// collapses are opposites — "Platform answered, and there is nothing new" and +/// "Platform answered nobody, so we do not know". Both used to arrive as +/// `Ok(vec![])`. +/// +/// The distinction matters most at startup, where a completed pass is the +/// promise that a contact's DIP-15 addresses exist before the compact-filter +/// scan passes their funding height. An address the wallet is not watching by +/// then produces no transaction at all, so recording an unreachable pass as a +/// successful one does not merely mislabel a status — it starts Core SPV +/// against an address set that is silently short. +#[derive(Debug, Default, Clone)] +pub struct ContactSyncReport { + /// Newly discovered incoming contact requests. Real whatever else failed: + /// they were fetched, ingested and persisted. + pub requests: Vec, + /// Identities the pass tried to fetch for. + pub identities_attempted: usize, + /// Identities nothing was ingested for — the received-side fetch failed, + /// or their local state was gone when the write guard was taken. Their + /// high-water cursors are deliberately left unadvanced, so the next sweep + /// re-fetches exactly the range this one missed. + pub failed_identities: Vec, + /// Identities whose received side ingested but whose **sent**-side fetch + /// failed. Their incoming requests are real; what is missing is the + /// reciprocal reconciliation that establishes contacts, and the sent + /// cursor stays unadvanced so the next sweep retries it. + pub degraded_identities: Vec, +} + +impl ContactSyncReport { + /// Every identity's fetches, both directions, were answered. + /// + /// The only state in which the pass may be recorded as a completed one. A + /// wallet with no identities is complete by this rule — there was nothing + /// to fetch, which is an answer rather than a degradation. + pub fn is_complete(&self) -> bool { + self.failed_identities.is_empty() && self.degraded_identities.is_empty() + } + + /// Not one identity's contact documents could be read. + /// + /// The signature of an unreachable Platform rather than of an empty + /// wallet, and the ending that must never be mistaken for a clean pass. A + /// wallet with no identities is NOT fully degraded: nothing was attempted, + /// so nothing failed. + pub fn is_fully_degraded(&self) -> bool { + self.identities_attempted > 0 && self.failed_identities.len() == self.identities_attempted + } +} + impl DashPayView<'_, B> { /// Fetch and process contact requests from the platform for all local identities. /// @@ -1119,7 +1176,36 @@ impl DashPayView<'_, B> { /// them inline under the guard would deadlock on first execution. /// /// Returns all newly discovered incoming contact requests. + /// + /// # Errors + /// + /// [`PlatformWalletError::ContactSyncUnreachable`] when the pass had + /// identities to fetch for and not one of them could be read. That ending + /// is indistinguishable from a clean empty result in the return value + /// alone, and reporting it as success is what let a startup sequence + /// record an unreachable Platform as a completed contact pass. Callers + /// that need to tell a partial pass from a complete one — rather than only + /// a total failure from everything else — should call + /// [`Self::sync_contact_requests_reporting`] instead. pub async fn sync_contact_requests(&self) -> Result, PlatformWalletError> { + let report = self.sync_contact_requests_reporting().await?; + if report.is_fully_degraded() { + return Err(PlatformWalletError::ContactSyncUnreachable { + identities: report.identities_attempted, + }); + } + Ok(report.requests) + } + + /// [`Self::sync_contact_requests`], reporting what the pass reached. + /// + /// Same work, same side effects; the difference is only that the caller + /// gets the failure set rather than a `Vec` that cannot express it. Use + /// this wherever a *complete* pass is a precondition — a partial one is + /// still `Ok`, and still leaves some contacts' account builds unenqueued. + pub async fn sync_contact_requests_reporting( + &self, + ) -> Result { // Snapshot each identity's high-water cursors up front so the // incremental query bound is read before any mutation this sweep. let identities: Vec<(Identifier, Option, Option)> = { @@ -1147,6 +1233,10 @@ impl DashPayView<'_, B> { .collect() }; + let mut report = ContactSyncReport { + identities_attempted: identities.len(), + ..Default::default() + }; let mut all_requests = Vec::new(); for (identity_id, hw_received, hw_sent) in identities { @@ -1169,6 +1259,12 @@ impl DashPayView<'_, B> { error = %e, "Failed to fetch received contact requests; skipping this identity" ); + // Nothing of this identity's is ingested this pass, and its + // cursors stay where they were. Recorded rather than only + // logged so a caller that treats the pass as a precondition + // can tell this from a clean empty result — see + // `ContactSyncReport`. + report.failed_identities.push(identity_id); continue; } }; @@ -1191,6 +1287,7 @@ impl DashPayView<'_, B> { "Failed to fetch sent contact requests; reconciling received side only" ); sent_ok = false; + report.degraded_identities.push(identity_id); Default::default() } }; @@ -1218,11 +1315,18 @@ impl DashPayView<'_, B> { let candidates = { let mut wm = self.wallet_manager.write().await; let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&self.wallet_id) else { + // Fetched, but there is no longer anywhere to put it. Same + // outcome for this identity as a failed fetch — nothing + // ingested — so it is reported the same way. + report.failed_identities.push(identity_id); continue; }; let managed = match info.identity_manager.managed_identity_mut(&identity_id) { Some(m) => m, - None => continue, + None => { + report.failed_identities.push(identity_id); + continue; + } }; // Established contacts re-keyed by a rotation request in // this pass — their stale external accounts are torn down @@ -1460,7 +1564,8 @@ impl DashPayView<'_, B> { self.enqueue_pending_auto_accepts(&identity_id).await; } - Ok(all_requests) + report.requests = all_requests; + Ok(report) } /// Parse a received `contactRequest` document into a [`ContactRequest`], @@ -3795,6 +3900,94 @@ mod cursor_tests { } } +#[cfg(test)] +mod contact_sync_report_tests { + use super::ContactSyncReport; + use dpp::prelude::Identifier; + + fn id(b: u8) -> Identifier { + Identifier::from([b; 32]) + } + + /// The clean pass: every identity answered, nothing new to report. This + /// must stay distinguishable from the unreachable case below, because it + /// is the only one that entitles a caller to say the contact set is + /// current. + #[test] + fn an_answered_pass_with_no_new_requests_is_complete() { + let report = ContactSyncReport { + identities_attempted: 2, + ..Default::default() + }; + + assert!(report.is_complete()); + assert!(!report.is_fully_degraded()); + } + + /// A wallet with no identities had nothing to fetch. That is an answer, + /// not a degradation — and specifically not a *total* one, or an empty + /// wallet would report the same thing as a total outage. + #[test] + fn a_wallet_with_no_identities_is_complete_and_not_degraded() { + let report = ContactSyncReport::default(); + + assert!(report.is_complete()); + assert!( + !report.is_fully_degraded(), + "nothing was attempted, so nothing failed" + ); + } + + /// Not one identity could be read: the DAPI-unreachable ending that used + /// to arrive as `Ok(vec![])`. + #[test] + fn a_pass_that_read_no_identity_is_fully_degraded() { + let report = ContactSyncReport { + identities_attempted: 2, + failed_identities: vec![id(1), id(2)], + ..Default::default() + }; + + assert!(!report.is_complete()); + assert!(report.is_fully_degraded()); + } + + /// The partial pass. What it fetched is real, so it is not a total + /// failure — and it is still not complete, because the identities it + /// missed have contact requests nobody looked at and account builds + /// nobody enqueued. Treating this as a completed sync is the same bug as + /// the total case, one identity at a time. + #[test] + fn a_partial_pass_is_neither_complete_nor_fully_degraded() { + let report = ContactSyncReport { + identities_attempted: 3, + failed_identities: vec![id(1)], + ..Default::default() + }; + + assert!(!report.is_complete()); + assert!(!report.is_fully_degraded()); + } + + /// A sent-side failure ingests the received side, so nothing is lost — + /// but the reciprocal reconciliation that establishes contacts did not + /// happen, so the pass still may not be recorded as complete. + #[test] + fn a_sent_side_failure_alone_still_degrades_the_pass() { + let report = ContactSyncReport { + identities_attempted: 1, + degraded_identities: vec![id(1)], + ..Default::default() + }; + + assert!(!report.is_complete()); + assert!( + !report.is_fully_degraded(), + "the received side was read; this is not a total failure" + ); + } +} + #[cfg(test)] mod sweep_tests { use super::*; diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs index ba68afb0c29..1dd9e5f048c 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -452,13 +452,22 @@ impl IdentityWallet { identity_index, e ); - tally.record_failure(e); + tally.record_failure(identity_index, e); } } identity_index += 1; } + // Record what this scan could and could not answer, before the verdict + // on whether its *result* is usable. The two are independent: a scan + // that found an identity despite an unanswered probe returns `Ok` and + // is still not a scan anybody may build a "nothing left to find" + // conclusion on. Published on the error path too — a scan that reached + // nobody is the strongest possible reason to scan again. + self.publish_scan_verdict(wallet_id, tally.verdict(identity_index)) + .await; + if tally.is_trustworthy() { // Found something despite a failed probe: the discovered // identities are already persisted, so return them rather than @@ -526,6 +535,50 @@ impl IdentityWallet { Ok(discovered) } + + /// Record and persist what a gap-limit scan managed to probe. + /// + /// Best-effort by design, and on the persist half only: the in-memory + /// record always lands, so a second bring-up in this process already sees + /// an incomplete scan and rescans. A failed persist costs the verdict its + /// survival across a restart, which is the same exposure a host that has + /// no slot for the field already has — it must not be allowed to fail the + /// scan that just succeeded. + async fn publish_scan_verdict( + &self, + wallet_id: crate::wallet::platform_wallet::WalletId, + verdict: crate::changeset::IdentityScanStateEntry, + ) { + { + let mut wm = self.wallet_manager.write().await; + match wm.get_wallet_info_mut(&wallet_id) { + Some(info) => info + .identity_manager + .record_identity_scan(wallet_id, verdict.clone()), + None => { + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + "identity scan finished for a wallet that is no longer managed; \ + dropping its verdict" + ); + return; + } + } + } + + let changeset = crate::changeset::PlatformWalletChangeSet { + identity_scan_state: Some(verdict), + ..Default::default() + }; + if let Err(e) = self.persister.store(changeset) { + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + error = %e, + "failed to persist the identity-scan verdict; a partial scan may not be \ + retried after a restart" + ); + } + } } /// Running bookkeeping for one gap-limit scan, and the verdict it produces. @@ -548,6 +601,13 @@ struct ScanTally { consecutive_misses: u32, /// Probes that never reached Platform. failed_probes: u32, + /// The indices behind [`Self::failed_probes`], ascending. + /// + /// The count alone says a scan was partial; the indices say *where*, which + /// is what makes the verdict actionable — a later launch knows exactly + /// which slots were never answered, and a reader of the persisted verdict + /// can tell an unanswered probe from a scan that was simply cut short. + failed_indices: Vec, /// Every index Platform answered with an identity — including ones the /// manager already tracked, which never reach the returned `discovered` /// list. A rescan from index 0 (what the app's "Find identities" command @@ -580,12 +640,31 @@ impl ScanTally { /// The probe never got an answer. It still advances the miss counter — the /// scan has to terminate when the network is down — but it is remembered /// separately, because the verdict depends on telling the two apart. - fn record_failure(&mut self, error: dash_sdk::Error) { + fn record_failure(&mut self, index: u32, error: dash_sdk::Error) { self.last_probe_error = Some(error); self.failed_probes += 1; + self.failed_indices.push(index); self.consecutive_misses += 1; } + /// The verdict to persist for this scan. + /// + /// Separate from [`Self::is_trustworthy`] and not its mirror: a scan that + /// found an identity despite an unanswered probe IS trustworthy — its + /// findings are real and worth keeping — and is still not complete. That + /// gap is precisely where an identity goes missing for the life of an + /// installation, so the two questions get two methods. + fn verdict(&self, probed_through: u32) -> crate::changeset::IdentityScanStateEntry { + if self.failed_indices.is_empty() { + crate::changeset::IdentityScanStateEntry::completed(probed_through) + } else { + crate::changeset::IdentityScanStateEntry::incomplete( + probed_through, + self.failed_indices.clone(), + ) + } + } + /// Whether the scan's literal result may be reported as-is. /// /// Emptiness is only trustworthy when every probe was answered. A scan @@ -886,14 +965,18 @@ mod tests { outcomes: impl IntoIterator, ()>>, ) -> ScanTally { let mut tally = ScanTally::default(); - for outcome in outcomes { + // Index-carrying like the production loop, which probes from + // `start_index` upward — the harness scans from 0, so the element + // position IS the index. + for (index, outcome) in outcomes.into_iter().enumerate() { if !tally.should_continue(gap_limit) { break; } + let index = index as u32; match outcome { Ok(Some(())) => tally.record_sighting(), Ok(None) => tally.record_miss(), - Err(()) => tally.record_failure(probe_failure()), + Err(()) => tally.record_failure(index, probe_failure()), } } tally @@ -910,6 +993,64 @@ mod tests { assert!(!tally.is_trustworthy()); } + /// The #4365 shape: an identity at index 0, no answer at index 1. The + /// scan is trustworthy — its findings are real — and it is NOT complete, + /// and those are different questions. Reporting only the first is what let + /// an identity at the unanswered index stay hidden for the life of an + /// installation. + #[test] + fn a_scan_that_found_something_despite_a_failed_probe_is_trustworthy_but_incomplete() { + let tally = run_scan(5, [Ok(Some(())), Err(()), Ok(None), Ok(None), Ok(None)]); + + assert!( + tally.is_trustworthy(), + "the identity it found is real and must not be discarded" + ); + let verdict = tally.verdict(5); + assert!( + !verdict.complete, + "an unanswered index means the identity set is not settled" + ); + assert_eq!( + verdict.failed_indices, + vec![1], + "the verdict names which index went unanswered" + ); + assert_eq!(verdict.probed_through, 5); + } + + /// A scan that answered everything is the only one that may let a later + /// launch skip discovery. + #[test] + fn a_fully_answered_scan_produces_a_complete_verdict() { + let tally = run_scan( + 5, + [ + Ok(Some(())), + Ok(None), + Ok(None), + Ok(None), + Ok(None), + Ok(None), + ], + ); + + let verdict = tally.verdict(6); + assert!(verdict.complete); + assert!(verdict.failed_indices.is_empty()); + } + + /// Every probe unanswered: the verdict records all of them, so a rescan + /// knows the whole range is open. + #[test] + fn a_scan_that_reached_nobody_records_every_failed_index() { + let tally = run_scan(3, [Err(()), Err(()), Err(())]); + + let verdict = tally.verdict(3); + assert!(!verdict.complete); + assert_eq!(verdict.failed_indices, vec![0, 1, 2]); + } + /// The genuinely-empty wallet: every probe answered, all of them "none". #[test] fn scan_with_every_probe_answered_empty_is_trustworthy() { diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/mod.rs b/packages/rs-platform-wallet/src/wallet/identity/network/mod.rs index 752fee202ce..3eb1753449c 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/mod.rs @@ -67,8 +67,14 @@ pub use seed_binding::SeedBindingVerification; mod tokens; pub use contact_info::ContactInfoPublishOutcome; +/// Seed-backed [`ContactCryptoProvider`] for tests. Lives behind the private +/// `contact_requests` module, so sibling modules reach it directly and the +/// manager's tests reach it through here. +#[cfg(test)] +pub(crate) use contact_requests::SeedCryptoProvider; pub use contact_requests::{ AutoAcceptProofSource, ContactCryptoProvider, ContactInfoOpened, ContactInfoSealed, + ContactSyncReport, }; pub use dashpay_view::DashPayView; pub use discovery::IdentityDiscoveryOptions; diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/manager/accessors.rs b/packages/rs-platform-wallet/src/wallet/identity/state/manager/accessors.rs index ea80ea19ae0..fcabcc3fdc7 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/manager/accessors.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/manager/accessors.rs @@ -212,4 +212,40 @@ impl IdentityManager { .get(wallet_id) .and_then(|m| m.keys().last().copied()) } + + /// Verdict of the last gap-limit identity scan for `wallet_id`, if one is + /// known. + pub fn identity_scan_state( + &self, + wallet_id: &WalletId, + ) -> Option<&crate::changeset::IdentityScanStateEntry> { + self.identity_scan_states.get(wallet_id) + } + + /// Whether a scan is known to have left indices unanswered. + /// + /// The question the warm-launch shortcut asks, phrased so that only + /// positive evidence of an incomplete scan can force a rescan. Deliberately + /// **not** `!is_complete()`: an absent verdict means nobody recorded one — + /// a host that does not persist it, or a wallet whose identities predate + /// this bookkeeping — and treating "unknown" as "incomplete" would make + /// every launch on such a host pay for a full scan plus its Keychain round + /// trip, which is the cost the warm-launch shortcut exists to avoid. + pub fn identity_scan_is_incomplete(&self, wallet_id: &WalletId) -> bool { + self.identity_scan_states + .get(wallet_id) + .is_some_and(|state| !state.complete) + } + + /// Record the verdict of a gap-limit scan for `wallet_id`. + /// + /// In-memory only — the caller emits the matching changeset entry, because + /// only it holds the persister. + pub fn record_identity_scan( + &mut self, + wallet_id: WalletId, + state: crate::changeset::IdentityScanStateEntry, + ) { + self.identity_scan_states.insert(wallet_id, state); + } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/manager/mod.rs b/packages/rs-platform-wallet/src/wallet/identity/state/manager/mod.rs index 4b13ae5c4c4..4c20f791dc7 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/manager/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/manager/mod.rs @@ -31,7 +31,7 @@ mod apply; mod lifecycle; use super::managed_identity::ManagedIdentity; -use crate::changeset::IdentityManagerStartState; +use crate::changeset::{IdentityManagerStartState, IdentityScanStateEntry}; use crate::wallet::platform_wallet::WalletId; use dpp::identity::accessors::IdentityGettersV0; use dpp::prelude::Identifier; @@ -95,6 +95,18 @@ pub struct IdentityManager { /// callers that need to drop an identity reach the buckets through /// `remove_for_apply` so the index stays in sync. location_index: BTreeMap, + + /// Per-wallet verdict of the last gap-limit identity scan, keyed by wallet + /// id because a scan is a wallet-scoped act even though its result is a + /// set of identities. + /// + /// Consulted by the startup sequence before it takes the warm-launch + /// shortcut: a scan that could not answer every index must not let a + /// later launch conclude the identity set is settled. An absent entry + /// means no verdict is known — see + /// [`IdentityManagerStartState::scan_states`] for why that is deliberately + /// not read as "complete". + identity_scan_states: BTreeMap, } impl From for IdentityManager { @@ -102,6 +114,7 @@ impl From for IdentityManager { let IdentityManagerStartState { out_of_wallet_identities, wallet_identities, + scan_states, } = state; // Rebuild the side-index from the two buckets — `IdentityManagerStartState` @@ -127,6 +140,7 @@ impl From for IdentityManager { out_of_wallet_identities, wallet_identities, location_index, + identity_scan_states: scan_states, } } } @@ -405,6 +419,83 @@ mod tests { assert!(manager.location_index().is_empty()); } + /// The cross-launch half of dashpay/platform#4365: an incomplete scan + /// verdict restored from the start state must still say "incomplete", or + /// the next launch takes the warm shortcut over an identity set that was + /// never fully probed. + #[test] + fn an_incomplete_scan_verdict_survives_a_restore() { + use crate::changeset::{IdentityManagerStartState, IdentityScanStateEntry}; + + let wallet: WalletId = [10u8; 32]; + let mut state = IdentityManagerStartState::default(); + state + .scan_states + .insert(wallet, IdentityScanStateEntry::incomplete(5, vec![1])); + + let manager = IdentityManager::from(state); + + assert!( + manager.identity_scan_is_incomplete(&wallet), + "a restored partial scan must still force a rescan" + ); + assert_eq!( + manager + .identity_scan_state(&wallet) + .expect("verdict restored") + .failed_indices, + vec![1] + ); + } + + /// The other side of it: a scan that answered everything restores as + /// complete, so the warm-launch shortcut keeps working and a healthy + /// wallet pays for no probes. + #[test] + fn a_complete_scan_verdict_permits_the_warm_shortcut() { + use crate::changeset::{IdentityManagerStartState, IdentityScanStateEntry}; + + let wallet: WalletId = [10u8; 32]; + let mut state = IdentityManagerStartState::default(); + state + .scan_states + .insert(wallet, IdentityScanStateEntry::completed(6)); + + let manager = IdentityManager::from(state); + + assert!(!manager.identity_scan_is_incomplete(&wallet)); + } + + /// "No verdict" is not "incomplete". Every wallet that predates this + /// bookkeeping, and every host that does not persist the verdict yet, + /// lands here — and forcing them all to rescan on every launch would cost + /// a full gap-limit scan plus a Keychain round trip before every Core SPV + /// start, which is the cost the warm shortcut exists to avoid. + #[test] + fn an_unknown_scan_verdict_does_not_force_a_rescan() { + let manager = IdentityManager::new(); + + assert!(!manager.identity_scan_is_incomplete(&[42u8; 32])); + assert!(manager.identity_scan_state(&[42u8; 32]).is_none()); + } + + /// A later scan's verdict wholly supersedes an earlier one's — that is + /// what lets a clean rescan clear a prior partial scan and hand the + /// shortcut back. + #[test] + fn a_clean_rescan_clears_an_earlier_partial_verdict() { + use crate::changeset::IdentityScanStateEntry; + + let wallet: WalletId = [10u8; 32]; + let mut manager = IdentityManager::new(); + + manager.record_identity_scan(wallet, IdentityScanStateEntry::incomplete(5, vec![1])); + assert!(manager.identity_scan_is_incomplete(&wallet)); + + manager.record_identity_scan(wallet, IdentityScanStateEntry::completed(6)); + assert!(!manager.identity_scan_is_incomplete(&wallet)); + } + #[test] fn from_start_state_rebuilds_location_index() { use crate::changeset::IdentityManagerStartState; diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift index 9e65f891cb2..96c9ead2d1e 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift @@ -30,6 +30,18 @@ public enum WalletStartupStatus: UInt8, Sendable { /// reachability problem. The identity question is unanswered, and another /// scan will not answer it: the same fault is still there. case discoveryFailed = 4 + /// The contact-crypto provider does not resolve the seed that owns this + /// wallet, so the contact-account drain was skipped without deriving + /// anything. + /// + /// Unlike the other partial cases this one is not about Platform being + /// slow — it says the signer handed to the call belongs to a different + /// wallet. Deriving anyway would write contact receiving addresses from + /// the wrong seed that no later correct-seed pass would ever revisit, so + /// doing nothing is the only safe response. Check the Keychain mapping for + /// this wallet; a rerun with the right signer completes the work, which is + /// still queued. + case seedBindingUnverified = 5 /// Whether another discovery scan could change the answer. /// @@ -57,8 +69,16 @@ public struct WalletStartupOutcome: Sendable, Equatable { /// Discovery scans performed. `0` when a local identity was already known /// and no network scan was needed. public let discoveryAttempts: UInt32 - /// Whether the inline contact-request pass ran. + /// Whether the inline contact-request pass ran **to completion**. `false` + /// when it was skipped, failed, ran out of budget, or came back degraded: + /// a pass that could not read some identities' contact documents left + /// their account builds unenqueued, so an empty pending count does not + /// mean their addresses are ready. public let dashPaySyncRan: Bool + /// The contact-account drain was skipped because the contact-crypto + /// provider does not resolve this wallet's seed. Nothing was derived and + /// nothing was written; the queued work is intact. + public let seedBindingUnverified: Bool /// Contact-crypto entries the drain completed. public let contactAccountsDrained: UInt32 /// Contact-account builds still queued on return. Non-zero means the @@ -142,6 +162,15 @@ extension PlatformWalletManager { // races this call. The verify is marker-cached, so the common path // costs a string comparison. // + // The shared Rust sequence now runs the same check of its own, just + // before the drain and only when something is actually queued, so a + // future JNI client inherits the gate instead of the bug. The two are + // not redundant: this one throws, refusing the call outright, while + // the Rust one fails closed and reports `seedBindingUnverified` — it + // has to let Core SPV start regardless. Keeping this here is what + // turns a wrong-seed pairing into a loud error on iOS rather than a + // silently degraded launch. + // // Against `storage`, not a default one: the resolver below reads that // store, and verifying a different Keychain than the work will use // would approve one mnemonic while another derives the accounts. @@ -233,6 +262,7 @@ extension WalletStartupOutcome { : nil self.discoveryAttempts = ffi.discovery_attempts self.dashPaySyncRan = ffi.dashpay_sync_ran + self.seedBindingUnverified = ffi.seed_binding_unverified self.contactAccountsDrained = ffi.contact_accounts_drained self.contactAccountsPending = ffi.contact_accounts_pending self.elapsed = TimeInterval(ffi.elapsed_ms) / 1000 From c968b303adda4335d21e06848d0b837eff108512 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:09:22 -0400 Subject: [PATCH 2/2] fix(platform-wallet): close three gaps review found in the bring-up hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to bad2093 on this branch. Each of these is the same failure the original commit set out to fix, surviving on a path it did not cover. 1. The seed-binding gate still had an ungated entry point. The gate landed in `start_wallet_subsystems`, but `platform_wallet_drain_pending_contact_crypto` — the FFI the JNI binding calls — went straight to `drain_pending_contact_crypto` and `drain_auto_accepts` with no check at all. A JNI client draining with a wrong-seed resolver therefore wrote contact receiving accounts from the wrong seed, permanently: `register_contact_account` keys its existence check on `(index, us, them)` and not on the xpub, so no later correct-seed pass revisits them. That is the exact defect the commit message said was closed, still reachable from the entry point it named as the reason to close it. The gate moves into `PlatformWallet::drain_pending_contact_crypto_verified` — one primitive that verifies, then runs both drains — and the startup sequence and the FFI now both drain through it. Behaviour is unchanged on each: the check is still skipped when nothing is queued (a warm launch resolves no key material), still fails closed on every verification error and not only on a mismatch, and still leaves the queue intact. The FFI reports a refusal as `ErrorInvalidParameter` for `SeedMismatch` — the same code the standalone verify already returns, so a host recognises the wrong-seed condition identically however it arrives — and `ErrorWalletOperation` for a provider that simply could not answer. 2. A known-incomplete identity scan could still report `Ready`. `StartupTally` had no way to say "an identity is known and the set it belongs to is not". The discovery signals are gated on `identity_id.is_none()` (correctly — a rescan reaches them with an identity on file), so a launch whose rescan was forced by an incomplete verdict and then failed fell through every check to `Ready`: the status that promises the identity set is settled, on the one launch that knows it is not. `identity_scan_incomplete` is recorded from the verdict on record once discovery is done, and `status()` returns the new `IdentityScanIncomplete` rather than `Ready`. Reading the verdict rather than this call's discovery counters is what makes it correct on the launches that have no counters to read — a warm shortcut, or a rescan abandoned before it started — and it catches the same defect reached from the other side, a first scan that came back partial. The check is ranked last, so the only run whose status changes is the one that used to lie; every other run keeps a status clients already handle and reads the flag on the outcome. `discovery_worth_retrying` covers the new status: the unanswered indices are exactly what another scan could answer. The test that pinned this, `a_failed_rescan_does_not_reopen_a_settled_ identity`, asserted `Ready` for precisely the incomplete-rescan case. Its real subject — a failed rescan must not re-open an identity already on file — is preserved and now sits alongside an assertion that the scan gap IS reported, under a name that says so. 3. A local fault mid-scan published no verdict at all. `publish_scan_verdict` has one call site, below four `?` early returns in `discover_inner` (breadcrumb derivation, the wallet-info lookup, `add_identity`, `add_keys`). A persistence write that failed, or a wallet that left the manager, therefore abandoned the walk part-way through the index space and recorded nothing — and "unknown" is what keeps the warm-launch shortcut armed. Worse, when a previous scan had recorded a COMPLETE verdict, that stale verdict survived the abandoned scan. #4365's shape, on the local-fault path. The scan body now runs in a block whose result is carried out, so no `?` inside it can skip the publish — including any added later. The index the walk died on is recorded as unanswered first, because a verdict built from the probe bookkeeping alone would see an empty failed-index list and publish an abandoned scan as complete, which is strictly worse than publishing nothing. The abort index is tracked apart from `failed_indices` so `failed_probes` and `IdentityDiscoveryIncomplete` keep meaning "probes Platform never answered"; the verdict merges the two, since to a later launch they are the same fact. Note the scan loop is re-indented by one level and not otherwise touched — `git diff -w` shows the real change. The FFI outcome struct gains `identity_scan_incomplete` and the status enum appends `IdentityScanIncomplete = 6`; the Swift mirror follows both, as the `SeedBindingUnverified` append did. 11 new tests. The gate: a wrong-seed provider is refused with the typed error and registers zero contact accounts while the queue survives, the owning seed drains, an empty queue never consults the provider. The scan signal: both directions at the tally level, plus the wire-up driven through the real `start_wallet_subsystems`. The verdict: a real local fault injected mid-scan (a seedless wallet against the resident-key derive) replaces a stale complete verdict with an incomplete one, so the next launch re-scans. Co-Authored-By: Claude Opus 4.8 --- .../rs-platform-wallet-ffi/src/dashpay.rs | 72 ++- .../src/wallet_startup.rs | 7 + .../rs-platform-wallet/src/manager/startup.rs | 363 ++++++++++++--- .../src/wallet/identity/network/discovery.rs | 439 +++++++++++++----- .../wallet/identity/network/seed_binding.rs | 293 ++++++++++++ .../PlatformWalletManagerStartup.swift | 37 +- 6 files changed, 999 insertions(+), 212 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/dashpay.rs b/packages/rs-platform-wallet-ffi/src/dashpay.rs index f87050caec6..f9b749c51bc 100644 --- a/packages/rs-platform-wallet-ffi/src/dashpay.rs +++ b/packages/rs-platform-wallet-ffi/src/dashpay.rs @@ -839,6 +839,24 @@ impl platform_wallet::ContactCryptoProvider for ResolverContactCryptoProvider { /// identity `signer_handle` to send the reciprocal). Writes the total number of /// completed entries (drained + auto-accepted) to `out_drained`. /// +/// # Seed binding +/// +/// Whenever there is drainable work, the resolver behind `core_signer_handle` +/// is first checked against this wallet's persisted BIP44 account-0 xpub +/// (`PlatformWallet::drain_pending_contact_crypto_verified`, the same gate the +/// startup sequence drains through). A resolver mapped to a different wallet +/// fails the call with `ErrorInvalidParameter` and derives NOTHING — the queue +/// is left intact for the next correct-seed drain. This is not advisory: a +/// wrong-seed drain writes contact receiving accounts that no later +/// correct-seed pass revisits (`register_contact_account` keys its existence +/// check on the contact pair, not on the xpub), so the corruption would be +/// permanent and its only symptom payments that never arrive. Any other +/// verification failure — a resolver that simply cannot answer — fails closed +/// the same way, with `ErrorWalletOperation`. +/// +/// An empty queue skips the check entirely, so a poll with nothing to do still +/// costs no key material. +/// /// # Safety /// - `signer_handle` (the identity document signer) is **optional**: pass null to /// run only the provider-derived ops (account build / contactInfo decrypt) and @@ -856,6 +874,10 @@ pub unsafe extern "C" fn platform_wallet_drain_pending_contact_crypto( ) -> PlatformWalletFFIResult { check_ptr!(core_signer_handle); check_ptr!(out_drained); + // Zero-init before any fallible work so a refused drain leaves a truthful + // count rather than whatever the caller's stack held — same discipline as + // the cached seed-binding verify. + unsafe { *out_drained = 0 }; // The identity signer is optional — null means "provider-only drain". let signer_addr = if signer_handle.is_null() { @@ -866,7 +888,6 @@ pub unsafe extern "C" fn platform_wallet_drain_pending_contact_crypto( let core_signer_addr = core_signer_handle as usize; let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { - let identity = wallet.identity().clone(); let wallet_id = wallet.wallet_id(); let network = wallet.network(); // SAFETY: same lifetime contract as platform_wallet_send_dashpay_payment — @@ -878,30 +899,45 @@ pub unsafe extern "C" fn platform_wallet_drain_pending_contact_crypto( network, ) }; + let wallet = wallet.clone(); block_on_worker(async move { - let drained = identity - .dashpay() - .drain_pending_contact_crypto(&provider) - .await; // The auto-accept pass needs the identity signer for the reciprocal; - // skip it when no identity signer was supplied. - let accepted = if signer_addr != 0 { - let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); - identity - .dashpay() - .drain_auto_accepts(signer, &provider) - .await + // `None` skips it, matching a null `signer_handle`. + let signer: Option<&VTableSigner> = if signer_addr != 0 { + Some(&*(signer_addr as *const VTableSigner)) } else { - 0 + None }; - drained + accepted + // Unbounded, as this entry point has always been: it is called off + // the main thread by a host that decided the work is worth waiting + // for, not from the Core-SPV-gating startup path that owns a budget. + wallet + .drain_pending_contact_crypto_verified(&provider, signer, None) + .await }) }); - let total = unwrap_option_or_return!(option); - unsafe { - *out_drained = total as u32; + let result = unwrap_option_or_return!(option); + match result { + Ok(total) => { + unsafe { + *out_drained = total as u32; + } + PlatformWalletFFIResult::ok() + } + // Same code the standalone verify reports for a mis-mapped resolver, so + // a host recognizes the wrong-seed condition identically whether it + // checked up front or was refused at the drain. + Err(e @ platform_wallet::PlatformWalletError::SeedMismatch { .. }) => { + PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + e.to_string(), + ) + } + Err(e) => PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorWalletOperation, + e.to_string(), + ), } - PlatformWalletFFIResult::ok() } /// Number of deferred **account-build** contact-crypto ops queued for this diff --git a/packages/rs-platform-wallet-ffi/src/wallet_startup.rs b/packages/rs-platform-wallet-ffi/src/wallet_startup.rs index 8bc2de6d9fa..bb1b1840858 100644 --- a/packages/rs-platform-wallet-ffi/src/wallet_startup.rs +++ b/packages/rs-platform-wallet-ffi/src/wallet_startup.rs @@ -32,6 +32,7 @@ pub enum WalletStartupStatusFFI { PartialAccountsPending = 3, DiscoveryFailed = 4, SeedBindingUnverified = 5, + IdentityScanIncomplete = 6, } impl From for WalletStartupStatusFFI { @@ -43,6 +44,7 @@ impl From for WalletStartupStatusFFI { WalletStartupStatus::PartialAccountsPending => Self::PartialAccountsPending, WalletStartupStatus::DiscoveryFailed => Self::DiscoveryFailed, WalletStartupStatus::SeedBindingUnverified => Self::SeedBindingUnverified, + WalletStartupStatus::IdentityScanIncomplete => Self::IdentityScanIncomplete, } } } @@ -65,6 +67,10 @@ pub struct WalletStartupOutcomeFFI { /// The drain was skipped because the supplied contact-crypto provider does /// not resolve this wallet's seed. Nothing was derived or written. pub seed_binding_unverified: bool, + /// The wallet's identity scan is on record as having left indices + /// unanswered and this launch did not close the gap. Any identity reported + /// here is real; it may not be the only one. + pub identity_scan_incomplete: bool, /// Contact-crypto entries the drain completed. pub contact_accounts_drained: u32, /// Contact-account builds still queued on return. @@ -86,6 +92,7 @@ impl From for WalletStartupOutcomeFFI { discovery_attempts: outcome.discovery_attempts, dashpay_sync_ran: outcome.dashpay_sync_ran, seed_binding_unverified: outcome.seed_binding_unverified, + identity_scan_incomplete: outcome.identity_scan_incomplete, contact_accounts_drained: outcome.contact_accounts_drained as u32, contact_accounts_pending: outcome.contact_accounts_pending as u32, elapsed_ms: outcome.elapsed.as_millis() as u64, diff --git a/packages/rs-platform-wallet/src/manager/startup.rs b/packages/rs-platform-wallet/src/manager/startup.rs index 51d092ac676..e1170f00996 100644 --- a/packages/rs-platform-wallet/src/manager/startup.rs +++ b/packages/rs-platform-wallet/src/manager/startup.rs @@ -215,29 +215,48 @@ pub enum WalletStartupStatus { /// watch addresses nobody pays to, with no symptom but payments that never /// arrive. SeedBindingUnverified, + /// An identity is known and every later step ran, but the wallet's + /// gap-limit identity scan is still on record as having left indices + /// unanswered — the rescan this launch forced did not close the gap. + /// + /// The distinction from [`Self::Ready`] is the whole point: an identity + /// hiding at an unanswered index is invisible to everything that consults + /// local state, so calling this launch `Ready` promises an identity set + /// that was never established. That is #4365's exact shape, one level up — + /// the wallet has *an* identity, so the warm-launch shortcut and every + /// tally signal read clean while a second identity stays lost. + /// + /// Not terminal: the verdict stays on record, so the next launch re-opens + /// the question instead of taking the shortcut. Nothing about the contact + /// state is in doubt here — the sync and the drain both ran for the + /// identity that *is* known. + IdentityScanIncomplete, } impl WalletStartupStatus { /// Whether another discovery scan could change the answer. /// - /// True only for [`Self::PartialNoIdentity`]. The other three are terminal - /// for different reasons — an identity was found, absence was proven, or - /// the failure is local and will still be there next time — and only an - /// unreachable Platform is worth asking again. + /// True for [`Self::PartialNoIdentity`] (Platform was never reached) and + /// [`Self::IdentityScanIncomplete`] (it was reached, but some indices were + /// not). The rest are terminal for different reasons — absence was proven, + /// the failure is local and will still be there next time, or the scan + /// answered everything it probed. /// /// This is the distinction platform#4352 made expressible: before it, "no /// identity exists" and "we never got through" both arrived as an empty /// success, so clients either retried a proven-empty scan forever or cached /// a network failure as fact. pub fn discovery_worth_retrying(self) -> bool { - matches!(self, Self::PartialNoIdentity) + matches!(self, Self::PartialNoIdentity | Self::IdentityScanIncomplete) } /// Whether the identity question has an answer. /// /// Note this is NOT the inverse of [`Self::discovery_worth_retrying`]: /// [`Self::DiscoveryFailed`] leaves the question open *and* is not worth - /// retrying. Use this to decide what to display, and + /// retrying, while [`Self::IdentityScanIncomplete`] has an answer that is + /// merely known to be partial — an identity was found, so there is + /// something to display. Use this to decide what to display, and /// `discovery_worth_retrying` to decide whether to scan again. pub fn identity_is_settled(self) -> bool { !matches!(self, Self::PartialNoIdentity | Self::DiscoveryFailed) @@ -263,6 +282,14 @@ pub struct WalletStartupOutcome { /// contact-crypto provider does not resolve this wallet's seed. Nothing /// was derived and nothing was written; the queue is intact. pub seed_binding_unverified: bool, + /// The wallet's gap-limit identity scan is on record as having left + /// indices unanswered, and this launch's scan did not close the gap. The + /// identities reported here are real; they may not be all of them. + /// + /// Carried separately from `status` because the status can only report one + /// thing and a pending contact queue outranks this — a client that wants + /// to surface "still looking for your other identities" reads the flag. + pub identity_scan_incomplete: bool, /// Contact-crypto entries completed by the drain. pub contact_accounts_drained: usize, /// Contact-account builds still queued when this returned. @@ -296,6 +323,10 @@ pub(crate) struct StartupTally { /// The drain was skipped because the contact-crypto provider could not be /// shown to resolve this wallet's seed. pub seed_binding_unverified: bool, + /// The recorded identity-scan verdict still says indices were left + /// unanswered once discovery was done for this launch. Independent of + /// `identity_id`: the gap is about the identities that were NOT found. + pub identity_scan_incomplete: bool, pub contact_accounts_drained: usize, pub contact_accounts_pending: usize, } @@ -355,6 +386,19 @@ impl StartupTally { self.seed_binding_unverified = true; } + /// Discovery is done for this launch and the recorded scan verdict still + /// says indices went unanswered. + /// + /// Read from the persisted verdict rather than inferred from the + /// discovery counters, because the two are not the same question. The + /// counters describe what *this* call did; the verdict describes what the + /// wallet's identity set is known to be missing, and it survives a launch + /// that never scanned at all. Only positive evidence sets it — an absent + /// verdict is "unknown", never "incomplete". + pub(crate) fn record_identity_scan_incomplete(&mut self) { + self.identity_scan_incomplete = true; + } + pub(crate) fn record_drain(&mut self, drained: usize, pending: usize) { self.contact_accounts_drained = drained; self.contact_accounts_pending = pending; @@ -404,6 +448,22 @@ impl StartupTally { if !self.dashpay_sync_ran { return WalletStartupStatus::PartialAccountsPending; } + // Last, and deliberately so: every check above describes work this + // launch did, while this one describes an identity set the wallet is + // on record as not having fully established. Ranking it here is what + // makes the fix additive — the only run whose status changes is the + // one that used to come back `Ready`, which is precisely the run that + // was lying. Everything else keeps the status a client already + // handles, and reads `identity_scan_incomplete` on the outcome if it + // cares. + // + // `Ready` is the promise that a contact payment has everything it + // needs. An unanswered index can hide a whole identity from every + // consumer of local state, so a launch that knows its scan was partial + // has not earned that word. + if self.identity_scan_incomplete { + return WalletStartupStatus::IdentityScanIncomplete; + } WalletStartupStatus::Ready } @@ -414,6 +474,7 @@ impl StartupTally { discovery_attempts: self.discovery_attempts, dashpay_sync_ran: self.dashpay_sync_ran, seed_binding_unverified: self.seed_binding_unverified, + identity_scan_incomplete: self.identity_scan_incomplete, contact_accounts_drained: self.contact_accounts_drained, contact_accounts_pending: self.contact_accounts_pending, elapsed, @@ -531,6 +592,28 @@ impl PlatformWalletManager } } + // Discovery is done for this launch; re-read the verdict it leaves + // behind. Re-reading rather than inferring from the branch above is + // what makes this correct in every ending: a rescan that closed the + // gap publishes a complete verdict and this reads `false`, a rescan + // that could not publishes (or leaves) an incomplete one, and a fresh + // scan that came back partial without ever having a prior verdict is + // caught too — it is the same defect, reached from the other side. + // + // Without this the tally has no way to express "an identity is known + // and the set it belongs to is not", so a launch whose rescan was + // unreachable arrived at `Ready`: the guard in `status()` requires + // `identity_id.is_none()` before a discovery signal may decide the + // verdict, and here an identity IS on file. + if self.identity_scan_is_incomplete(wallet_id).await { + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + "startup: the identity scan is still on record as incomplete; this launch \ + cannot report a settled identity set" + ); + tally.record_identity_scan_incomplete(); + } + // With no identity there is nothing to sync and nothing to drain, and // that is true whether Platform proved absence or never answered. if !tally.has_identity() { @@ -608,77 +691,45 @@ impl PlatformWalletManager // passes `None` gets the sequence's other steps and an honest // `contact_accounts_pending`, rather than a drain that reports zero // because every crypto operation failed. - let (drained, accepted) = match contact_crypto { - // Nothing queued means the drain would derive nothing, so there is - // no wrong-seed write to prevent and no reason to pay for the check - // below. Covers both drains: auto-accepts ride the same queue. - // Keeping the gate's cost proportional to its risk is what lets it - // live here — a warm launch with an empty queue still resolves no - // key material at all. - Some(contact_crypto) - if identity_wallet - .dashpay() - .drainable_contact_crypto_count() - .await - > 0 => + // + // The seed-binding gate in front of both drains is NOT applied here: + // it lives inside + // [`PlatformWallet::drain_pending_contact_crypto_verified`], the one + // primitive this sequence and the FFI drain entry point share. Keeping + // it there rather than in each caller is the whole point — a client + // that has to remember to gate the call is a client that will + // eventually forget, which is exactly how the FFI entry point came to + // have no gate while iOS enforced one in its Swift wrapper. The only + // error it can return is a failed verification (the drains themselves + // report counts, never errors), so an `Err` here means precisely "the + // provider was not shown to own this wallet". + let drained = match contact_crypto { + Some(contact_crypto) => match wallet + .drain_pending_contact_crypto_verified( + contact_crypto, + identity_signer, + Some(deadline), + ) + .await { - // Everything past this point derives from whatever seed the - // provider resolves, and none of it is authenticated. A - // provider mapped to the wrong wallet derives contact receiving - // xpubs from the wrong seed, and `register_contact_account` - // keys its existence check on `(index, us, them)` — not on the - // xpub — so the wrong addresses are written once and every - // later correct-seed pass no-ops. The corruption is permanent - // and its only symptom is payments that never arrive. - // - // The gate belongs here rather than in each client for the - // same reason the ordering does: iOS enforces it in its Swift - // wrapper today, and a client that has to remember to gate this - // call is a client that will eventually forget. A JNI binding - // added later inherits the gate instead of the bug. - // - // Fail closed on every error, not only on a mismatch. A - // provider that cannot answer has not been shown to own this - // wallet, and skipping costs nothing that is not recoverable: - // the queue is untouched, so the next signer-present drain - // completes exactly the work this one declined to guess at. - if let Err(e) = wallet.verify_seed_binds(contact_crypto).await { + Ok(drained) => drained, + Err(e) => { tally.record_seed_binding_unverified(); tracing::error!( wallet_id = %hex::encode(wallet_id), error = %e, - "startup: the contact-crypto provider does not bind to this wallet's \ - seed; skipping the drain rather than deriving contact addresses that \ - could never be corrected" + "startup: the contact-crypto drain was refused; the supplied provider \ + does not bind to this wallet's seed" ); - (0, 0) - } else { - let drained = identity_wallet - .dashpay() - .drain_pending_contact_crypto_until(contact_crypto, Some(deadline)) - .await; - let accepted = match identity_signer { - Some(signer) => { - identity_wallet - .dashpay() - .drain_auto_accepts_until(signer, contact_crypto, Some(deadline)) - .await - } - None => 0, - }; - (drained, accepted) + 0 } - } - // A provider was supplied and the queue is empty — the ordinary - // warm launch. Nothing to drain, nothing to verify, nothing to - // report beyond the pending count read below. - Some(_) => (0, 0), + }, None => { tracing::info!( wallet_id = %hex::encode(wallet_id), "startup: no contact-crypto provider; skipping the drain" ); - (0, 0) + 0 } }; // Not budgeted: a local queue-length read with no I/O. Leaving it @@ -688,7 +739,7 @@ impl PlatformWalletManager .dashpay() .pending_contact_crypto_count() .await; - tally.record_drain(drained + accepted, pending); + tally.record_drain(drained, pending); if pending > 0 { tracing::warn!( @@ -1129,29 +1180,99 @@ mod tests { assert_eq!(outcome.status, WalletStartupStatus::SeedBindingUnverified); } - /// A rescan forced by an incomplete prior scan can now reach the + /// A rescan forced by an incomplete prior scan can reach the /// discovery-failure branches with an identity already on file. Those /// statuses say "the identity question is still open", which would be a - /// lie here — and it would also hide a sync and drain that both ran. + /// lie here — and it would also hide a sync and drain that both ran. But + /// the rescan failing is not nothing either: it means the scan gap that + /// forced it is still there. + /// + /// This test previously asserted `Ready` for the unreachable half, pinning + /// the very defect the `identity_scan_incomplete` signal exists to close — + /// a launch that knows its identity set is partial reporting the status + /// that promises it is complete. Both halves keep their real subject (the + /// identity must not be re-opened) and now assert the gap is reported. #[test] - fn a_failed_rescan_does_not_reopen_a_settled_identity() { + fn a_failed_rescan_reports_the_scan_gap_without_reopening_the_identity() { + // The scenario the name describes: the prior verdict said incomplete, + // which is the only reason a rescan ran at all, and it is still + // incomplete afterwards. let mut unreachable = StartupTally::default(); unreachable.record_local_identity(identity()); unreachable.record_unreachable(); unreachable.record_discovery_gave_up(); + unreachable.record_identity_scan_incomplete(); unreachable.record_sync_ran(); unreachable.record_drain(1, 0); - assert_eq!(unreachable.status(), WalletStartupStatus::Ready); + assert_eq!( + unreachable.status(), + WalletStartupStatus::IdentityScanIncomplete, + "a launch whose rescan never closed the gap has not established the identity set" + ); + assert!( + unreachable.status().identity_is_settled(), + "the identity that WAS found is real; only the set around it is open" + ); + assert!( + unreachable.status().discovery_worth_retrying(), + "the unanswered indices are exactly what another scan could answer" + ); let mut local_fault = StartupTally::default(); local_fault.record_local_identity(identity()); local_fault.record_discovery_failed_locally(); + local_fault.record_identity_scan_incomplete(); local_fault.record_sync_ran(); local_fault.record_drain(0, 2); assert_eq!( local_fault.status(), - WalletStartupStatus::PartialAccountsPending + WalletStartupStatus::PartialAccountsPending, + "a pending contact queue still outranks the scan gap in the status" ); + assert!( + local_fault.status().identity_is_settled(), + "a local discovery fault must not re-open an identity that is on file" + ); + } + + /// The corrected verdict, isolated: an otherwise perfectly clean run — an + /// identity, a completed contact pass, an empty queue — is still not + /// `Ready` while the scan that produced that identity is on record as + /// having left indices unanswered. `Ready` promises a settled identity + /// set, and this run cannot promise one. + #[test] + fn an_incomplete_scan_keeps_an_otherwise_clean_run_off_ready() { + let mut tally = StartupTally::default(); + tally.record_local_identity(identity()); + tally.record_sync_ran(); + tally.record_identity_scan_incomplete(); + tally.record_drain(1, 0); + + assert_eq!(tally.status(), WalletStartupStatus::IdentityScanIncomplete); + + let outcome = tally.into_outcome(Duration::from_secs(1)); + assert!( + outcome.identity_scan_incomplete, + "the flag must reach the client even where the status is outranked" + ); + assert_eq!(outcome.identity_id, Some(identity())); + } + + /// The other direction, and the reason the check reads the recorded + /// verdict rather than the discovery counters: the identical run with a + /// scan that answered every index it probed IS `Ready`. Without this the + /// test above would keep passing if the signal were stuck on. + #[test] + fn a_complete_scan_reaches_ready() { + let mut tally = StartupTally::default(); + tally.record_local_identity(identity()); + tally.record_sync_ran(); + tally.record_drain(1, 0); + + assert_eq!(tally.status(), WalletStartupStatus::Ready); + + let outcome = tally.into_outcome(Duration::from_secs(1)); + assert!(!outcome.identity_scan_incomplete); } /// Every network step is abandonable, so `within_budget` must return @@ -1529,6 +1650,106 @@ mod tests { assert_eq!(outcome.status, WalletStartupStatus::PartialAccountsPending); } + /// The wire-up, end to end: the sequence reads the wallet's RECORDED scan + /// verdict once discovery is done and carries it out on the outcome. + /// + /// Reading the verdict rather than inferring from this call's discovery + /// counters is the point — a launch that took the warm shortcut, or whose + /// rescan was abandoned before it started, still has to report the gap the + /// wallet is on record as having, and neither of those launches has a + /// discovery counter to infer it from. + /// + /// Driven with a zero budget so no branch depends on network timing: every + /// step is abandoned at its deadline and what is asserted is purely which + /// verdict came out. ("No verdict at all" is not reachable here — the + /// harness's mock SDK answers no probe, so creating the wallet already + /// leaves one — and it is the accessor's own documented contract that an + /// absent verdict reads as unknown rather than incomplete.) + #[tokio::test] + async fn a_recorded_incomplete_scan_reaches_the_outcome() { + use crate::changeset::IdentityScanStateEntry; + use crate::wallet::identity::network::SeedCryptoProvider; + + let no_time = WalletStartupOptions { + budget: Duration::ZERO, + gap_limit: None, + }; + + let (manager, wallet_id) = manager_with_queued_contact_crypto().await; + let owning = + SeedCryptoProvider::from_seed(seed_for(OWNING_MNEMONIC), key_wallet::Network::Testnet); + + // The wallet arrives with an unanswered index on record — a real + // incomplete scan, produced by the mock SDK refusing every probe + // during wallet creation, not a hand-planted flag. + { + let wm = manager.wallet_manager.read().await; + let verdict = wm + .get_wallet_info(&wallet_id) + .expect("wallet info") + .identity_manager + .identity_scan_state(&wallet_id) + .cloned() + .expect("precondition: the creation scan recorded a verdict"); + assert!( + !verdict.complete, + "precondition: that verdict must be the incomplete one" + ); + } + + let outcome = manager + .start_wallet_subsystems( + &wallet_id, + None, + Some(&owning), + None::<&UnusedSigner>, + no_time, + ) + .await + .expect("bring-up reports rather than raises"); + + assert!( + outcome.identity_scan_incomplete, + "the recorded gap must reach the client: {outcome:?}" + ); + assert_ne!( + outcome.status, + WalletStartupStatus::Ready, + "Ready promises an identity set this launch did not establish" + ); + assert!( + outcome.identity_id.is_some(), + "the identity that IS known must still be reported" + ); + + // The other direction, through the same sequence: once the scan is on + // record as having answered everything it probed, the signal clears. + // Without this the assertion above would keep passing if the flag were + // simply stuck on. + { + let mut wm = manager.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("wallet info"); + info.identity_manager + .record_identity_scan(wallet_id, IdentityScanStateEntry::completed(4)); + } + + let outcome = manager + .start_wallet_subsystems( + &wallet_id, + None, + Some(&owning), + None::<&UnusedSigner>, + no_time, + ) + .await + .expect("bring-up reports rather than raises"); + + assert!( + !outcome.identity_scan_incomplete, + "a complete verdict leaves nothing to report: {outcome:?}" + ); + } + #[test] fn outcome_carries_the_tally_through() { let mut tally = StartupTally::default(); diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs index 1dd9e5f048c..4de41bf3f7c 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -340,134 +340,174 @@ impl IdentityWallet { let mut discovered: Vec = Vec::new(); let mut tally = ScanTally::default(); - while tally.should_continue(gap_limit) { - // Derive the MASTER auth pubkey hash for this identity index - // from whichever source the caller picked. The per-index read - // lock is only needed for the wallet-internal derive (it reads - // the resident key material); the master derive is a pure, - // lock-free secp256k1 pass. - let key_hash_array = match source { - KeyHashSource::ResidentWallet => { - let wm = self.wallet_manager.read().await; - let wallet = wm.get_wallet(&self.wallet_id).ok_or_else(|| { - crate::error::PlatformWalletError::WalletNotFound( - "Wallet not found in wallet manager".to_string(), - ) - })?; - derive_identity_auth_key_hash( - wallet, + // The scan runs inside its own block so its early returns cannot skip + // the verdict below. Every `?` in here is a LOCAL fault — a wallet that + // left the manager, a persistence write that failed — not a probe that + // went unanswered, and each of them abandons the scan part-way through + // the index space. Returning straight out left no verdict at all, and + // "unknown" is what keeps the warm-launch shortcut: the next launch saw + // the identities this scan had already folded in, took the shortcut, + // and never looked at the indices it never reached. That is #4365's + // shape on the local-fault path, so the error is carried out to the + // publish below rather than thrown from the middle of the walk. + let scan_outcome: Result<(), PlatformWalletError> = async { + while tally.should_continue(gap_limit) { + // Derive the MASTER auth pubkey hash for this identity index + // from whichever source the caller picked. The per-index read + // lock is only needed for the wallet-internal derive (it reads + // the resident key material); the master derive is a pure, + // lock-free secp256k1 pass. + let key_hash_array = match source { + KeyHashSource::ResidentWallet => { + let wm = self.wallet_manager.read().await; + let wallet = wm.get_wallet(&self.wallet_id).ok_or_else(|| { + crate::error::PlatformWalletError::WalletNotFound( + "Wallet not found in wallet manager".to_string(), + ) + })?; + derive_identity_auth_key_hash( + wallet, + network, + identity_index, + MASTER_KEY_INDEX, + )? + } + KeyHashSource::Master(master) => derive_identity_auth_key_hash_from_master( + master, network, identity_index, MASTER_KEY_INDEX, - )? - } - KeyHashSource::Master(master) => derive_identity_auth_key_hash_from_master( - master, - network, - identity_index, - MASTER_KEY_INDEX, - )?, - }; + )?, + }; + + // Query Platform for an identity registered with this key + // hash. No locks are held during this network call. + let fetch_result = Identity::fetch(&self.sdk, PublicKeyHash(key_hash_array)).await; + + match fetch_result { + Ok(Some(identity)) => { + let identity_id = identity.id(); + + // Derive + verify a candidate for every on-chain key + // (shared with the index-load path) BEFORE taking the write + // lock — candidate derivation borrows the resident wallet / + // master xpriv, while breadcrumb emission needs `&mut info`. + let key_decisions = self + .derive_key_breadcrumbs( + &identity, + identity_index, + network, + match source { + KeyHashSource::Master(master) => Some(master), + KeyHashSource::ResidentWallet => None, + }, + ) + .await?; + + // Acquire write lock to add/enrich the identity, then emit + // every per-key breadcrumb in one batched changeset. + let mut wm_guard = self.wallet_manager.write().await; + let info_guard = + wm_guard + .get_wallet_info_mut(&self.wallet_id) + .ok_or_else(|| { + crate::error::PlatformWalletError::WalletNotFound( + "Wallet info not found in wallet manager".to_string(), + ) + })?; + let is_new = info_guard.identity_manager.identity(&identity_id).is_none(); + if is_new { + info_guard.identity_manager.add_identity( + identity.clone(), + identity_index, + wallet_id, + &self.persister, + )?; + } - // Query Platform for an identity registered with this key - // hash. No locks are held during this network call. - let fetch_result = Identity::fetch(&self.sdk, PublicKeyHash(key_hash_array)).await; - - match fetch_result { - Ok(Some(identity)) => { - let identity_id = identity.id(); - - // Derive + verify a candidate for every on-chain key - // (shared with the index-load path) BEFORE taking the write - // lock — candidate derivation borrows the resident wallet / - // master xpriv, while breadcrumb emission needs `&mut info`. - let key_decisions = self - .derive_key_breadcrumbs( - &identity, - identity_index, - network, - match source { - KeyHashSource::Master(master) => Some(master), - KeyHashSource::ResidentWallet => None, - }, - ) - .await?; - - // Acquire write lock to add/enrich the identity, then emit - // every per-key breadcrumb in one batched changeset. - let mut wm_guard = self.wallet_manager.write().await; - let info_guard = - wm_guard - .get_wallet_info_mut(&self.wallet_id) - .ok_or_else(|| { - crate::error::PlatformWalletError::WalletNotFound( - "Wallet info not found in wallet manager".to_string(), - ) - })?; - let is_new = info_guard.identity_manager.identity(&identity_id).is_none(); - if is_new { - info_guard.identity_manager.add_identity( - identity.clone(), - identity_index, - wallet_id, - &self.persister, - )?; - } + if let Some(managed) = info_guard + .identity_manager + .managed_identity_mut(&identity_id) + { + managed.set_status(IdentityStatus::Active, &self.persister); + managed.wallet_id = Some(wallet_id); + // Breadcrumbs for every re-derivable key (not just the + // MASTER key) so the client (iOS Keychain) can + // re-derive each signing key's private key — without + // this only the master key is materialized and the + // imported identity cannot sign with its HIGH / + // CRITICAL authentication keys. A failed persist here + // would silently leave the identity watch-only after + // restart, so surface it (matching `add_identity` above). + managed + .add_keys(key_decisions, &self.persister) + .map_err(|e| { + PlatformWalletError::Persistence(format!( + "identity keys not persisted during discovery: {e}" + )) + })?; + } + drop(wm_guard); - if let Some(managed) = info_guard - .identity_manager - .managed_identity_mut(&identity_id) - { - managed.set_status(IdentityStatus::Active, &self.persister); - managed.wallet_id = Some(wallet_id); - // Breadcrumbs for every re-derivable key (not just the - // MASTER key) so the client (iOS Keychain) can - // re-derive each signing key's private key — without - // this only the master key is materialized and the - // imported identity cannot sign with its HIGH / - // CRITICAL authentication keys. A failed persist here - // would silently leave the identity watch-only after - // restart, so surface it (matching `add_identity` above). - managed - .add_keys(key_decisions, &self.persister) - .map_err(|e| { - PlatformWalletError::Persistence(format!( - "identity keys not persisted during discovery: {e}" - )) - })?; + if is_new { + discovered.push(identity.clone()); + } + tally.record_sighting(); } - drop(wm_guard); - - if is_new { - discovered.push(identity.clone()); + Ok(None) => { + tally.record_miss(); + } + Err(e) => { + tracing::warn!( + "Failed to query identity at index {}: {}", + identity_index, + e + ); + tally.record_failure(identity_index, e); } - tally.record_sighting(); - } - Ok(None) => { - tally.record_miss(); - } - Err(e) => { - tracing::warn!( - "Failed to query identity at index {}: {}", - identity_index, - e - ); - tally.record_failure(identity_index, e); } - } - identity_index += 1; + identity_index += 1; + } + Ok(()) } + .await; + + // A local fault stopped the walk at `identity_index`, so that index and + // everything above it went unanswered. Record the index it died on + // before the verdict is built: without it `verdict` sees an empty + // failed-index list and would publish this abandoned scan as COMPLETE — + // strictly worse than the missing verdict this fixes, since a complete + // verdict actively re-arms the shortcut. + let probed_through = match &scan_outcome { + Ok(()) => identity_index, + Err(e) => { + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + index = identity_index, + error = %e, + "identity discovery hit a local fault mid-scan; recording the index it \ + stopped at as unanswered so a later launch rescans instead of trusting \ + a walk that never finished" + ); + tally.record_local_fault(identity_index); + identity_index.saturating_add(1) + } + }; // Record what this scan could and could not answer, before the verdict // on whether its *result* is usable. The two are independent: a scan // that found an identity despite an unanswered probe returns `Ok` and // is still not a scan anybody may build a "nothing left to find" - // conclusion on. Published on the error path too — a scan that reached - // nobody is the strongest possible reason to scan again. - self.publish_scan_verdict(wallet_id, tally.verdict(identity_index)) + // conclusion on. Published on every ending — an unreachable Platform is + // the strongest possible reason to scan again, and so is a scan that + // was cut short by a fault on this device. + self.publish_scan_verdict(wallet_id, tally.verdict(probed_through)) .await; + // Only now, with the verdict on record either way. + scan_outcome?; + if tally.is_trustworthy() { // Found something despite a failed probe: the discovered // identities are already persisted, so return them rather than @@ -618,6 +658,15 @@ struct ScanTally { /// Last probe failure, kept typed so callers can inspect the variant /// rather than parse a rendered string. last_probe_error: Option, + /// The index a LOCAL fault stopped the scan at, if one did. + /// + /// Held apart from [`Self::failed_indices`] so [`Self::failed_probes`] and + /// the incomplete-scan error keep meaning exactly "probes Platform never + /// answered" — a persistence write that failed is not a network condition + /// and must not be reported as one. [`Self::verdict`] folds the two + /// together, because to a later launch they are the same fact: an index + /// nobody answered. + aborted_at_index: Option, } impl ScanTally { @@ -647,6 +696,30 @@ impl ScanTally { self.consecutive_misses += 1; } + /// A local fault abandoned the scan at `index` — the walk stopped there, + /// so that index and every one above it went unanswered. + /// + /// Recorded so [`Self::verdict`] cannot call an abandoned scan complete. + /// It does not touch the probe counters: the scan did not fail to REACH + /// Platform, it failed on this device, and conflating the two would make + /// the incomplete-scan error claim a network cause it has no evidence for. + fn record_local_fault(&mut self, index: u32) { + self.aborted_at_index = Some(index); + } + + /// Every index this scan did not answer, ascending — unanswered probes + /// plus the index a local fault abandoned it at. + fn unanswered_indices(&self) -> Vec { + let mut indices = self.failed_indices.clone(); + if let Some(index) = self.aborted_at_index { + if !indices.contains(&index) { + indices.push(index); + indices.sort_unstable(); + } + } + indices + } + /// The verdict to persist for this scan. /// /// Separate from [`Self::is_trustworthy`] and not its mirror: a scan that @@ -655,13 +728,11 @@ impl ScanTally { /// gap is precisely where an identity goes missing for the life of an /// installation, so the two questions get two methods. fn verdict(&self, probed_through: u32) -> crate::changeset::IdentityScanStateEntry { - if self.failed_indices.is_empty() { + let unanswered = self.unanswered_indices(); + if unanswered.is_empty() { crate::changeset::IdentityScanStateEntry::completed(probed_through) } else { - crate::changeset::IdentityScanStateEntry::incomplete( - probed_through, - self.failed_indices.clone(), - ) + crate::changeset::IdentityScanStateEntry::incomplete(probed_through, unanswered) } } @@ -1137,4 +1208,140 @@ mod tests { assert!(error.source().is_some(), "source must survive for callers"); assert!(error.to_string().contains("dapi unreachable")); } + + // ----------------------------------------------------------------------- + // Local faults: the scan aborted by something on THIS device rather than + // by an unanswered probe. + // ----------------------------------------------------------------------- + + /// A scan abandoned part-way through must never be published as complete. + /// + /// This is the trap in the fix: a local fault records no failed *probe*, + /// so a verdict built from the probe bookkeeping alone sees an empty + /// failed-index list and calls the abandoned walk clean. That is worse + /// than the missing verdict it replaces — a complete verdict actively + /// re-arms the warm-launch shortcut over an index space nobody finished. + #[test] + fn a_locally_aborted_scan_is_never_complete() { + let mut tally = run_scan(5, [Ok(Some(())), Ok(None)]); + // Fault on the index the walk stopped at, exactly as `discover_inner` + // records it. + tally.record_local_fault(2); + + let verdict = tally.verdict(3); + assert!( + !verdict.complete, + "a scan that stopped early cannot claim it answered everything" + ); + assert_eq!( + verdict.failed_indices, + vec![2], + "the verdict names the index the walk died on" + ); + assert_eq!(verdict.probed_through, 3); + } + + /// The two kinds of gap are merged, ascending, for the reader: to a later + /// launch an unanswered probe and an abandoned index are the same fact. + #[test] + fn an_abort_is_merged_with_the_unanswered_probes() { + let mut tally = run_scan(5, [Ok(Some(())), Err(()), Ok(None)]); + tally.record_local_fault(3); + + let verdict = tally.verdict(4); + assert!(!verdict.complete); + assert_eq!(verdict.failed_indices, vec![1, 3]); + assert_eq!( + tally.failed_probes, 1, + "a device-side fault is not a probe Platform failed to answer" + ); + } + + /// A local fault at an index that ALSO went unanswered is recorded once. + #[test] + fn an_abort_at_an_already_unanswered_index_is_not_duplicated() { + let mut tally = run_scan(5, [Err(())]); + tally.record_local_fault(0); + + assert_eq!(tally.verdict(1).failed_indices, vec![0]); + } + + /// End to end, with a real local fault injected mid-scan: a stale + /// **complete** verdict must not survive it. + /// + /// The fault is genuine rather than mocked — `discover()` derives each + /// probe hash from resident key material, and this wallet is + /// external-signable (its seed lives outside the manager), so the derive + /// fails on the first index. That is one of the `?` early returns above + /// `publish_scan_verdict`, and before this fix every one of them returned + /// without publishing anything at all: the previous verdict stood, and a + /// verdict that says "complete" is exactly what keeps the warm-launch + /// shortcut armed. The wallet would then trust an index space this scan + /// abandoned — #4365's shape reached from the local-fault side. + #[tokio::test] + async fn a_local_fault_mid_scan_replaces_a_stale_complete_verdict() { + use crate::changeset::IdentityScanStateEntry; + use crate::wallet::identity::network::IdentityDiscoveryOptions; + + let (manager, wallet_id) = crate::test_support::test_platform_wallet_manager().await; + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + + assert!( + !wallet.state().await.wallet().has_seed(), + "precondition: the resident derive must be the thing that faults" + ); + + // The state the defect preserves: a wallet whose last scan answered + // everything, so the next launch takes the shortcut. + { + let mut wm = manager.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("wallet info"); + info.identity_manager + .record_identity_scan(wallet_id, IdentityScanStateEntry::completed(9)); + } + { + let wm = manager.wallet_manager.read().await; + assert!( + !wm.get_wallet_info(&wallet_id) + .expect("wallet info") + .identity_manager + .identity_scan_is_incomplete(&wallet_id), + "precondition: the warm shortcut is armed" + ); + } + + let err = wallet + .identity() + .discover(IdentityDiscoveryOptions { + start_index: Some(0), + gap_limit: 5, + }) + .await + .expect_err("the resident derive cannot work for a seedless wallet"); + assert!( + !matches!(err, PlatformWalletError::IdentityDiscoveryIncomplete { .. }), + "precondition: this must be a LOCAL fault, not an unanswered probe: {err:?}" + ); + + let wm = manager.wallet_manager.read().await; + let info = wm.get_wallet_info(&wallet_id).expect("wallet info"); + let verdict = info + .identity_manager + .identity_scan_state(&wallet_id) + .expect("a verdict must have been published on the fault path"); + assert!( + !verdict.complete, + "the stale complete verdict must not have survived an abandoned scan" + ); + assert_eq!( + verdict.failed_indices, + vec![0], + "the index the walk died on is on record as unanswered" + ); + assert!( + info.identity_manager + .identity_scan_is_incomplete(&wallet_id), + "the next launch must re-scan instead of taking the warm shortcut" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs b/packages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs index 8ba700b191c..8d1b94795af 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs @@ -9,6 +9,13 @@ //! Keychain slot — the signer resolving some other wallet's mnemonic — derives //! a different xpub and is refused, so it can never sign for the wrong wallet. //! This is the wrong-seed detection without ever holding a resident seed. +//! +//! The check is also the gate in front of the deferred contact-crypto drain — +//! see [`PlatformWallet::drain_pending_contact_crypto_verified`], the primitive +//! every client drains through so none of them can forget it. + +use dpp::identity::signer::Signer; +use dpp::identity::IdentityPublicKey; use crate::error::PlatformWalletError; use crate::wallet::identity::network::contact_requests::ContactCryptoProvider; @@ -114,6 +121,88 @@ impl PlatformWallet { }) } } + + /// Drain the deferred contact-crypto queue, but only through a provider + /// that has been shown to resolve this wallet's seed. + /// + /// Runs the provider-only ops + /// ([`drain_pending_contact_crypto_until`]) and, when an identity signer is + /// supplied, the DIP-15 auto-accept pass + /// ([`drain_auto_accepts_until`]) — the same pair every drain entry point + /// runs — and returns their combined completed count. `deadline` bounds + /// both from the inside; `None` is unbounded. + /// + /// # Why the gate lives here + /// + /// Everything the drain derives comes from whatever seed the provider + /// resolves, and none of it is authenticated. A provider mapped to the + /// wrong wallet derives contact receiving xpubs from the wrong seed, and + /// `register_contact_account` keys its existence check on `(index, us, + /// them)` rather than on the xpub — so the wrong addresses are written + /// once and every later correct-seed pass no-ops. The corruption is + /// permanent and its only symptom is payments that never arrive. + /// + /// Putting the check in each client is what lets a client forget it: iOS + /// enforced it in its Swift wrapper while the FFI drain entry point had no + /// gate at all, so a JNI binding written against that entry point + /// inherited the bug rather than the rule. This is the one primitive both + /// the startup sequence and the FFI drain call, so there is a single place + /// the gate can be removed from and none where it can be omitted. + /// + /// # Cost + /// + /// Proportional to the risk: an empty queue would derive nothing, so there + /// is no wrong-seed write to prevent and the check is skipped entirely — + /// a warm launch with nothing queued resolves no key material at all. Both + /// drains ride the same queue, so one count covers both. + /// + /// # Errors + /// + /// Fails closed on **every** verification error, not only on + /// [`PlatformWalletError::SeedMismatch`]: a provider that cannot answer has + /// not been shown to own this wallet. Skipping costs nothing that is not + /// recoverable — the queue is untouched, so the next signer-present drain + /// completes exactly the work this one declined to guess at. + /// + /// [`drain_pending_contact_crypto_until`]: crate::wallet::identity::network::DashPayView::drain_pending_contact_crypto_until + /// [`drain_auto_accepts_until`]: crate::wallet::identity::network::DashPayView::drain_auto_accepts_until + pub async fn drain_pending_contact_crypto_verified( + &self, + crypto: &C, + identity_signer: Option<&S>, + deadline: Option, + ) -> Result + where + C: ContactCryptoProvider + Sync, + S: Signer + Send + Sync, + { + let dashpay = self.identity().dashpay(); + if dashpay.drainable_contact_crypto_count().await == 0 { + return Ok(0); + } + + self.verify_seed_binds(crypto).await.inspect_err(|e| { + tracing::error!( + wallet_id = %hex::encode(self.wallet_id()), + error = %e, + "the contact-crypto provider does not bind to this wallet's seed; skipping \ + the drain rather than deriving contact addresses that could never be corrected" + ); + })?; + + let drained = dashpay + .drain_pending_contact_crypto_until(crypto, deadline) + .await; + let accepted = match identity_signer { + Some(signer) => { + dashpay + .drain_auto_accepts_until(signer, crypto, deadline) + .await + } + None => 0, + }; + Ok(drained + accepted) + } } #[cfg(test)] @@ -506,4 +595,208 @@ mod tests { "expected InvalidIdentityData, got: {err:?}" ); } + + // ----------------------------------------------------------------------- + // The gate in front of the drain. + // + // `verify_seed_binds` above proves the check itself is right. These prove + // the drain cannot run without it — the property that matters, because the + // FFI entry point every JNI client binds to used to call the drains + // directly and skip the check entirely. + // ----------------------------------------------------------------------- + + /// A different valid BIP-39 vector: the mis-mapped Keychain slot. + const FOREIGN_MNEMONIC: &str = + "legal winner thank year wave sausage worth useful legal winner thank yellow"; + + /// Only ever passed as `None`, so the auto-accept pass is skipped — but the + /// generic still has to be named. + #[derive(Debug)] + struct UnusedSigner; + + #[async_trait::async_trait] + impl dpp::identity::signer::Signer for UnusedSigner { + async fn sign( + &self, + _key: &dpp::identity::IdentityPublicKey, + _data: &[u8], + ) -> Result { + unreachable!("the auto-accept pass is never reached with a None signer") + } + + async fn sign_create_witness( + &self, + _key: &dpp::identity::IdentityPublicKey, + _data: &[u8], + ) -> Result { + unreachable!("the auto-accept pass is never reached with a None signer") + } + + fn can_sign_with(&self, _key: &dpp::identity::IdentityPublicKey) -> bool { + false + } + } + + /// A wallet owning one identity with a single queued `RegisterReceiving` + /// op — the smallest state in which the drain has real work, and the op + /// that derives a contact receiving xpub straight from the provider with + /// no network round trip. + async fn wallet_with_queued_contact_crypto() -> ( + Arc>, + Arc, + WalletId, + ) { + use crate::changeset::{ + upsert_pending_contact_crypto, PendingContactCrypto, PendingContactCryptoOp, + }; + use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; + use dpp::identity::v0::IdentityV0; + use dpp::prelude::Identifier; + + let manager = make_manager(); + let wallet = manager + .create_wallet_from_seed_bytes( + Network::Testnet, + &seed_for(TEST_MNEMONIC), + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("wallet creation"); + let wallet_id = wallet.wallet_id(); + let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); + + let mut wm = manager.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("wallet info"); + info.identity_manager + .add_identity( + dpp::identity::Identity::V0(IdentityV0 { + id: Identifier::from([1u8; 32]), + public_keys: std::collections::BTreeMap::new(), + balance: 0, + revision: 0, + }), + 0, + wallet_id, + &persister, + ) + .expect("add identity"); + let managed = info + .identity_manager + .managed_identity_mut(&Identifier::from([1u8; 32])) + .expect("managed identity"); + upsert_pending_contact_crypto( + managed.dashpay_pending_contact_crypto_mut(), + PendingContactCrypto { + owner_identity_id: Identifier::from([1u8; 32]), + contact_id: Identifier::from([2u8; 32]), + op: PendingContactCryptoOp::RegisterReceiving, + enqueued_at_ms: 0, + }, + ); + drop(wm); + + (manager, wallet, wallet_id) + } + + /// The DashPay receiving accounts the wallet is watching — the thing a + /// wrong-seed drain corrupts. `register_contact_account` keys its + /// existence check on `(index, us, them)` and NOT on the xpub, so an + /// account written from the wrong seed is never revisited. + async fn receiving_account_count( + manager: &PlatformWalletManager, + wallet_id: &WalletId, + ) -> usize { + let wm = manager.wallet_manager.read().await; + wm.get_wallet_info(wallet_id) + .map(|info| info.core_wallet.accounts.dashpay_receival_accounts.len()) + .unwrap_or(0) + } + + async fn drainable(wallet: &crate::PlatformWallet) -> usize { + wallet + .identity() + .dashpay() + .drainable_contact_crypto_count() + .await + } + + /// The defect: a provider resolving someone else's seed derives contact + /// receiving xpubs that are written once and never corrected, so the + /// wallet watches addresses nobody pays to. The drain must not run at all, + /// and the queue must survive intact for the next signer-present attempt. + #[tokio::test] + async fn a_wrong_seed_provider_is_refused_before_the_drain() { + let (manager, wallet, wallet_id) = wallet_with_queued_contact_crypto().await; + assert_eq!(receiving_account_count(&manager, &wallet_id).await, 0); + assert_eq!(drainable(&wallet).await, 1); + + let foreign = SeedCryptoProvider::from_seed(seed_for(FOREIGN_MNEMONIC), Network::Testnet); + let err = wallet + .drain_pending_contact_crypto_verified(&foreign, None::<&UnusedSigner>, None) + .await + .expect_err("a provider that does not own the wallet must be refused"); + + assert!( + matches!(err, PlatformWalletError::SeedMismatch { .. }), + "the refusal must be the typed wrong-seed error, got: {err:?}" + ); + assert_eq!( + receiving_account_count(&manager, &wallet_id).await, + 0, + "not one contact account may be registered from the wrong seed" + ); + assert_eq!( + drainable(&wallet).await, + 1, + "the queue must survive so the next correct-seed drain can do the work" + ); + } + + /// The other half: the wallet's own seed passes the gate and the drain + /// runs. Without this the test above would also pass if the gate simply + /// refused everything. + #[tokio::test] + async fn the_owning_seed_passes_the_gate_and_the_drain_runs() { + let (manager, wallet, wallet_id) = wallet_with_queued_contact_crypto().await; + + let owning = SeedCryptoProvider::from_seed(seed_for(TEST_MNEMONIC), Network::Testnet); + let drained = wallet + .drain_pending_contact_crypto_verified(&owning, None::<&UnusedSigner>, None) + .await + .expect("the wallet's own seed must bind"); + + assert_eq!(drained, 1, "the queued RegisterReceiving op must complete"); + assert_eq!( + receiving_account_count(&manager, &wallet_id).await, + 1, + "the contact receiving account must exist after a verified drain" + ); + } + + /// The gate is paid for only when there is something to protect. An empty + /// queue would derive nothing, so no key material is resolved — which is + /// what keeps this affordable on a warm launch. Proven with a provider + /// that would FAIL the check: an `Ok` shows it was never consulted. + #[tokio::test] + async fn an_empty_queue_never_consults_the_provider() { + let manager = make_manager(); + let wallet = manager + .create_wallet_from_seed_bytes( + Network::Testnet, + &seed_for(TEST_MNEMONIC), + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("wallet creation"); + assert_eq!(drainable(&wallet).await, 0); + + let foreign = SeedCryptoProvider::from_seed(seed_for(FOREIGN_MNEMONIC), Network::Testnet); + let drained = wallet + .drain_pending_contact_crypto_verified(&foreign, None::<&UnusedSigner>, None) + .await + .expect("with nothing to drain the binding check must not run at all"); + assert_eq!(drained, 0); + } } diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift index 96c9ead2d1e..c8df1c185d5 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift @@ -42,20 +42,37 @@ public enum WalletStartupStatus: UInt8, Sendable { /// this wallet; a rerun with the right signer completes the work, which is /// still queued. case seedBindingUnverified = 5 + /// An identity is known and every later step ran, but the gap-limit + /// identity scan is still on record as having left indices unanswered. + /// + /// Not a failure of this launch's work — the contact sync and the drain + /// both ran for the identity that *is* known. It says the identity SET is + /// not established: an identity sitting at an unanswered index is + /// invisible to everything that reads local state, and reporting ``ready`` + /// would promise a set this launch never proved. The verdict stays on + /// record, so the next launch re-scans instead of taking the warm + /// shortcut. + case identityScanIncomplete = 6 /// Whether another discovery scan could change the answer. /// - /// True only for ``partialNoIdentity``. The others are terminal for this - /// launch — an identity was found, absence was proven, or the failure is - /// local and will still be there next time. - public var discoveryWorthRetrying: Bool { self == .partialNoIdentity } + /// True for ``partialNoIdentity`` (Platform was never reached) and + /// ``identityScanIncomplete`` (it was reached, but not for every index). + /// The others are terminal for this launch — absence was proven, the + /// failure is local and will still be there next time, or the scan + /// answered everything it probed. + public var discoveryWorthRetrying: Bool { + self == .partialNoIdentity || self == .identityScanIncomplete + } /// Whether the identity question has an answer. /// /// Not the inverse of ``discoveryWorthRetrying``: ``discoveryFailed`` - /// leaves the question open *and* is not worth retrying. Use this to decide - /// what to show, and ``discoveryWorthRetrying`` to decide whether to scan - /// again. + /// leaves the question open *and* is not worth retrying, while + /// ``identityScanIncomplete`` has an answer that is merely known to be + /// partial — an identity was found, so there is something to show. Use + /// this to decide what to show, and ``discoveryWorthRetrying`` to decide + /// whether to scan again. public var identityIsSettled: Bool { self != .partialNoIdentity && self != .discoveryFailed } @@ -79,6 +96,11 @@ public struct WalletStartupOutcome: Sendable, Equatable { /// provider does not resolve this wallet's seed. Nothing was derived and /// nothing was written; the queued work is intact. public let seedBindingUnverified: Bool + /// The wallet's identity scan is on record as having left indices + /// unanswered and this launch did not close the gap. Any identity reported + /// here is real; it may not be the only one. Carried separately from + /// ``status`` because a pending contact queue outranks it there. + public let identityScanIncomplete: Bool /// Contact-crypto entries the drain completed. public let contactAccountsDrained: UInt32 /// Contact-account builds still queued on return. Non-zero means the @@ -263,6 +285,7 @@ extension WalletStartupOutcome { self.discoveryAttempts = ffi.discovery_attempts self.dashPaySyncRan = ffi.dashpay_sync_ran self.seedBindingUnverified = ffi.seed_binding_unverified + self.identityScanIncomplete = ffi.identity_scan_incomplete self.contactAccountsDrained = ffi.contact_accounts_drained self.contactAccountsPending = ffi.contact_accounts_pending self.elapsed = TimeInterval(ffi.elapsed_ms) / 1000