diff --git a/packages/rs-platform-wallet-ffi/src/dashpay.rs b/packages/rs-platform-wallet-ffi/src/dashpay.rs index f87050caec..868d4eb1bf 100644 --- a/packages/rs-platform-wallet-ffi/src/dashpay.rs +++ b/packages/rs-platform-wallet-ffi/src/dashpay.rs @@ -254,8 +254,11 @@ pub unsafe extern "C" fn platform_wallet_sync_contact_requests( block_on_worker(async move { identity.dashpay().sync_contact_requests().await }) }); let result = unwrap_option_or_return!(option); - let list = unwrap_result_or_return!(result); - unsafe { *out_array = ContactRequestHandleArray::from_requests(list) }; + let outcome = unwrap_result_or_return!(result); + // This on-demand FFI fetch surfaces only the ingested requests; the + // `fetch_complete` flag is consumed by the in-Rust ordered-startup gate + // (`manager::startup`), not by this C entry point. + unsafe { *out_array = ContactRequestHandleArray::from_requests(outcome.requests) }; PlatformWalletFFIResult::ok() } diff --git a/packages/rs-platform-wallet-ffi/src/persistence.rs b/packages/rs-platform-wallet-ffi/src/persistence.rs index 04a4e29ea1..76494e0e65 100644 --- a/packages/rs-platform-wallet-ffi/src/persistence.rs +++ b/packages/rs-platform-wallet-ffi/src/persistence.rs @@ -4786,6 +4786,13 @@ fn build_wallet_start_state( let identity_manager = IdentityManagerStartState { out_of_wallet_identities: BTreeMap::new(), wallet_identities, + // The FFI persister vtable has no slot for the identity-discovery + // completion flag (#4365) yet, so it is not restored here — it defaults + // to "not fully discovered", the safe direction: a warm launch re-runs + // discovery rather than shortcutting past an unprobed index. See the + // durability caveat on + // `PlatformWalletChangeSet::identity_discovery_complete`. + fully_discovered_wallets: std::collections::BTreeSet::new(), }; // Rehydrate tracked asset-locks (built / broadcast / IS-locked diff --git a/packages/rs-platform-wallet/src/changeset/changeset.rs b/packages/rs-platform-wallet/src/changeset/changeset.rs index fa425fbde5..95216ece52 100644 --- a/packages/rs-platform-wallet/src/changeset/changeset.rs +++ b/packages/rs-platform-wallet/src/changeset/changeset.rs @@ -1636,6 +1636,25 @@ pub struct PlatformWalletChangeSet { /// failed). Append-only delta; apply removes matching `(owner, contact, /// kind)` from the persisted queue. pub pending_contact_crypto_cleared: Vec, + /// Per-wallet identity-discovery completion flag (issue #4365), keyed by + /// the changeset's wallet id (the `store(wallet_id, changeset)` argument). + /// `Some(true)` when the wallet's most recent gap-limit identity scan + /// answered every probe (zero failed probes); `Some(false)` when a scan was + /// incomplete (a failed probe left an index unprobed). `None` means no + /// change in this delta. Merge policy: last-write-wins (a later `Some` + /// overrides an earlier one). + /// + /// It gates the ordered-startup warm-launch shortcut: only a wallet whose + /// last scan was complete may skip the network scan when a local identity + /// is already on file, so an incomplete initial scan re-runs discovery next + /// launch instead of shortcutting past a failed-probe index forever. + /// + /// Durability caveat (mirrors [`Self::pending_contact_crypto_added`]): the + /// SQLite backend persists this flag; the FFI persister vtable has no slot + /// for it yet, so on iOS/Android hosts it is process-lifetime only — a warm + /// launch there conservatively re-runs discovery (safe: never strands) + /// until the vtable slot and native handlers land. + pub identity_discovery_complete: Option, /// Shielded sub-wallet deltas: per-subwallet decrypted notes, /// spent marks, sync watermarks, nullifier checkpoints. The /// commitment tree itself is **not** in here — it lives on @@ -1757,6 +1776,12 @@ impl Merge for PlatformWalletChangeSet { .extend(other.pending_contact_crypto_added); self.pending_contact_crypto_cleared .extend(other.pending_contact_crypto_cleared); + // Identity-discovery completion: last-write-wins. A later delta's + // verdict (a scan just finished) supersedes an earlier one; `None` + // keeps the current value. + if other.identity_discovery_complete.is_some() { + self.identity_discovery_complete = other.identity_discovery_complete; + } #[cfg(feature = "shielded")] { self.shielded.merge(other.shielded); @@ -1783,7 +1808,8 @@ impl Merge for PlatformWalletChangeSet { && self.provider_key_account_registrations.is_empty() && self.account_address_pools.is_empty() && self.pending_contact_crypto_added.is_empty() - && self.pending_contact_crypto_cleared.is_empty(); + && self.pending_contact_crypto_cleared.is_empty() + && self.identity_discovery_complete.is_none(); #[cfg(feature = "shielded")] { core_empty && self.shielded.as_ref().is_none_or(|s| s.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 fbb42fa9e0..a32069fa0e 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 @@ -4,7 +4,7 @@ //! struct — no methods, no invariants, no live handles — so persisters //! can round-trip it without dragging in the manager's business logic. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use dpp::prelude::Identifier; @@ -26,4 +26,12 @@ pub struct IdentityManagerStartState { /// Wallet-owned identities, outer-keyed by wallet id and /// inner-keyed by BIP-9 registration index. pub wallet_identities: BTreeMap>, + /// Wallet ids whose most recent identity-discovery scan answered every + /// gap-limit probe (zero failed probes) — see + /// [`IdentityManager::is_wallet_fully_discovered`](crate::wallet::identity::IdentityManager::is_wallet_fully_discovered). + /// A backend that persists the `identity_discovery_complete` changeset flag + /// populates this so a genuinely-complete wallet keeps its startup + /// warm-launch shortcut across restart; a backend that does not leaves it + /// empty, which is the safe default (re-scan rather than strand, #4365). + pub fully_discovered_wallets: BTreeSet, } diff --git a/packages/rs-platform-wallet/src/manager/startup.rs b/packages/rs-platform-wallet/src/manager/startup.rs index 1b5ac7fa03..1cfcbb7059 100644 --- a/packages/rs-platform-wallet/src/manager/startup.rs +++ b/packages/rs-platform-wallet/src/manager/startup.rs @@ -87,6 +87,20 @@ async fn within_budget(deadline: Instant, future: F) -> tokio::time::timeout(remaining, future).await.ok() } +/// Whether the bring-up may take its warm-launch shortcut — skip the network +/// identity scan — for a wallet. +/// +/// True only when a local identity is already on file AND that wallet's last +/// discovery scan was COMPLETE (every gap-limit probe answered). The +/// completeness half is the fix for issue #4365: before it, a local identity +/// alone took the shortcut, so a wallet whose initial scan saw identity 0 but +/// got no answer for identity 1 shortcut past index 1 on every later launch and +/// stranded that identity + its contacts until a manual Discover. Pinned as a +/// pure predicate so the decision is tested without an SDK or a network. +fn may_take_warm_shortcut(has_local_identity: bool, discovery_complete: bool) -> bool { + has_local_identity && discovery_complete +} + /// Produces the master xpriv an identity scan needs, on demand. /// /// **Lazy is the point.** Which branches scan is this module's decision — a @@ -326,6 +340,23 @@ impl StartupTally { self.dashpay_sync_ran = true; } + /// Record the startup contact-request pass, gated on `fetch_complete`. + /// + /// Only a pass that reached Platform for EVERY identity marks the sync as + /// run. This is the F1 fix: `sync_contact_requests` returns an empty set + /// both when there genuinely are no contact requests AND when Platform was + /// unreachable for every identity, and the two must not be conflated. An + /// unreachable-Platform empty is NOT a proof of "no contacts", so it is + /// deliberately not recorded — leaving `dashpay_sync_ran` false keeps the + /// wallet out of `Ready` (via the `!dashpay_sync_ran` guard in + /// [`Self::status`]), so Core SPV stays gated / the pass re-runs rather than + /// scanning past a contact's funding height with DIP-15 addresses underived. + pub(crate) fn record_contact_sync_pass(&mut self, fetch_complete: bool) { + if fetch_complete { + self.record_sync_ran(); + } + } + pub(crate) fn record_drain(&mut self, drained: usize, pending: usize) { self.contact_accounts_drained = drained; self.contact_accounts_pending = pending; @@ -440,19 +471,32 @@ 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 — but only when the wallet's last scan was + // COMPLETE. A wallet with a local identity whose initial scan was + // incomplete (a failed probe left a later index unprobed) must NOT + // take the shortcut, or that later identity + its contacts stay + // stranded until a manual Discover (#4365). Re-running discovery + // resumes past the already-known identities, so the cost is bounded. + let local_identity = self.local_identity_id(wallet_id).await; + let take_shortcut = match local_identity { + Some(_) => { + may_take_warm_shortcut(true, self.wallet_discovery_is_complete(wallet_id).await) + } + None => false, + }; + match local_identity { + Some(known) if take_shortcut => tally.record_local_identity(known), + _ => { + 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 @@ -465,12 +509,24 @@ impl PlatformWalletManager // 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)) => { - tally.record_sync_ran(); + Some(Ok(outcome)) => { + // Gate on `fetch_complete` (F1): an empty result from a Platform + // that was unreachable for every identity must NOT be recorded + // as a completed sync, or `status()` would report `Ready` and + // start SPV promising contact addresses this pass never fetched. + tally.record_contact_sync_pass(outcome.fetch_complete); + if !outcome.fetch_complete { + tracing::warn!( + wallet_id = %hex::encode(wallet_id), + "startup: contact-request pass could not reach Platform for every \ + identity; not marking sync as run so the wallet stays out of Ready" + ); + } tracing::debug!( wallet_id = %hex::encode(wallet_id), - requests = requests.len(), - "startup: contact-request pass complete" + requests = outcome.requests.len(), + fetch_complete = outcome.fetch_complete, + "startup: contact-request pass finished" ); } Some(Err(e)) => { @@ -558,6 +614,18 @@ impl PlatformWalletManager .next() } + /// Whether this wallet's most recent identity-discovery scan was COMPLETE — + /// every gap-limit probe answered. Gates the warm-launch shortcut (#4365); + /// see [`crate::wallet::identity::IdentityManager::is_wallet_fully_discovered`]. + /// A wallet never scanned to completion (or one whose backend does not + /// persist the flag) reads `false` — the safe direction: re-scan. + async fn wallet_discovery_is_complete(&self, wallet_id: &WalletId) -> bool { + let wm = self.wallet_manager.read().await; + wm.get_wallet_info(wallet_id) + .map(|info| info.identity_manager.is_wallet_fully_discovered(wallet_id)) + .unwrap_or(false) + } + /// Scan for an identity, retrying only while Platform stays unreachable. /// /// An `Ok` result ends the loop whether or not it found anything: Platform @@ -627,6 +695,16 @@ impl PlatformWalletManager let Some(result) = within_budget(deadline, attempt_future).await else { // Sightings persist incrementally, so an abandoned scan may // still have folded an identity in before it was cut off. + // + // #4365 coverage: this budget-expiry path records the identity + // into the run's tally but the scan was NOT completed, so + // `discover_inner` never marked the wallet fully-discovered — + // the completion flag stays not-complete. We only reach + // discovery here because the warm-launch shortcut was declined + // (no local identity, or the flag was not complete), so the flag + // remains not-complete and the NEXT launch re-runs discovery + // rather than shortcutting past an index this abandoned scan + // never probed. if let Some(known) = self.local_identity_id(wallet_id).await { tally.record_local_identity(known); return; @@ -836,6 +914,57 @@ mod tests { assert!(!tally.has_identity()); } + /// F1: a contact-request pass that could not reach Platform for every + /// identity comes back with an EMPTY set, and that empty must NOT be + /// reported as `Ready` — an unreachable-Platform empty is not a proof of + /// "no contacts". Before the fix, `record_sync_ran()` fired unconditionally + /// on `Ok(_)`, so an all-identities-fetch-fail pass settled as `Ready` and + /// started SPV promising contact addresses it never fetched. + #[test] + fn contact_sync_that_could_not_reach_platform_is_not_ready() { + let mut tally = StartupTally::default(); + tally.record_discovered(identity()); + // fetch_complete == false: Platform unreachable for some/all identities. + tally.record_contact_sync_pass(false); + tally.record_drain(0, 0); + + assert!( + !tally.dashpay_sync_ran, + "an incomplete fetch must not mark the sync as run" + ); + assert_ne!(tally.status(), WalletStartupStatus::Ready); + assert_eq!(tally.status(), WalletStartupStatus::PartialAccountsPending); + } + + /// F1 fast path: a pass that reached Platform for every identity and found + /// no requests is a genuine empty, so it settles as `Ready`. + #[test] + fn contact_sync_genuine_empty_is_ready() { + let mut tally = StartupTally::default(); + tally.record_discovered(identity()); + // fetch_complete == true, empty result: genuinely no contact requests. + tally.record_contact_sync_pass(true); + tally.record_drain(0, 0); + + assert!(tally.dashpay_sync_ran); + assert_eq!(tally.status(), WalletStartupStatus::Ready); + } + + /// #4365: the warm-launch shortcut (skip the network identity scan when a + /// local identity is on file) is taken ONLY when the wallet's last scan was + /// complete. A local identity whose initial scan was incomplete must re-run + /// discovery next launch instead of shortcutting past the unprobed index. + #[test] + fn warm_shortcut_requires_a_complete_prior_scan() { + // Local identity + complete prior scan → shortcut (the optimization). + assert!(may_take_warm_shortcut(true, true)); + // Local identity but INCOMPLETE prior scan → must re-scan (was the bug). + assert!(!may_take_warm_shortcut(true, false)); + // No local identity → always scan, regardless of the completion flag. + assert!(!may_take_warm_shortcut(false, true)); + assert!(!may_take_warm_shortcut(false, false)); + } + /// 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 diff --git a/packages/rs-platform-wallet/src/wallet/apply.rs b/packages/rs-platform-wallet/src/wallet/apply.rs index 4390740640..f03593bb9f 100644 --- a/packages/rs-platform-wallet/src/wallet/apply.rs +++ b/packages/rs-platform-wallet/src/wallet/apply.rs @@ -122,6 +122,13 @@ impl PlatformWalletInfo { // start-state path. No changeset-replay hook in apply. pending_contact_crypto_added: _, pending_contact_crypto_cleared: _, + // The identity-discovery completion flag (#4365) is persistence-only + // here for the same reason: the in-memory set is mutated directly at + // the discovery site (`IdentityManager::set_wallet_discovery_complete`) + // and restored at load via the start-state path + // (`IdentityManagerStartState::fully_discovered_wallets`). No + // changeset-replay hook in apply. + identity_discovery_complete: _, // Shielded deltas are owned by `ShieldedWallet` (which // mutates its store directly during sync / spend); the // canonical in-memory state lives there and the 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 c99018792c..7ef354b47c 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 @@ -20,6 +20,31 @@ use crate::error::PlatformWalletError; use crate::wallet::identity::types::dashpay::contact_request::ContactRequest; use crate::wallet::identity::types::dashpay::established_contact::EstablishedContact; +/// Result of one contact-request sync pass: the requests it ingested, plus +/// whether every identity's fetch reached Platform. +/// +/// `fetch_complete` is the load-bearing distinction the ordered-startup path +/// (`crate::manager::startup`) depends on. An empty `requests` with +/// `fetch_complete == true` is a proof there are no incoming contact requests, +/// so the caller may record the sync pass as having run and let Core SPV +/// proceed. An empty `requests` with `fetch_complete == false` means Platform +/// could not be reached for one or more identities: the ingested set is +/// known-incomplete, and treating it as "no contacts" would start SPV past a +/// contact's funding height with that contact's DIP-15 addresses underived — +/// producing born-wrong / missing payment records. The recurring background +/// sweep ignores this flag (it retries every interval regardless); startup must +/// not, which is why the value is surfaced rather than swallowed. +#[derive(Debug, Clone)] +pub struct ContactRequestSyncOutcome { + /// Newly discovered incoming contact requests ingested this pass. + pub requests: Vec, + /// True iff EVERY identity's received AND sent fetch reached Platform this + /// pass. False when any per-identity fetch failed — including the + /// per-identity skips this method swallows so one identity's transient DAPI + /// error does not abort the sweep for the others. + pub fetch_complete: bool, +} + // --------------------------------------------------------------------------- // Deferred-crypto drain provider // --------------------------------------------------------------------------- @@ -1118,8 +1143,14 @@ impl DashPayView<'_, B> { /// register functions called — mirroring the accept path. Calling /// them inline under the guard would deadlock on first execution. /// - /// Returns all newly discovered incoming contact requests. - pub async fn sync_contact_requests(&self) -> Result, PlatformWalletError> { + /// Returns the newly discovered incoming contact requests together with a + /// `fetch_complete` flag — see [`ContactRequestSyncOutcome`]. An empty list + /// is only a proof of "no contacts" when `fetch_complete` is `true`; + /// otherwise Platform could not be reached for at least one identity and the + /// result is known-incomplete. + pub async fn sync_contact_requests( + &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)> = { @@ -1148,6 +1179,14 @@ impl DashPayView<'_, B> { }; let mut all_requests = Vec::new(); + // Cleared by ANY per-identity fetch failure below (a hard DAPI error on + // the received or sent fetch). A pass that could not reach Platform for + // some identity must not report an empty result as "no contacts" — that + // is the F1 regression this flag closes: with every identity's fetch + // failing, `all_requests` is empty and, without this, the caller read + // it as a settled empty and started SPV with contact addresses + // underived. Stays `true` for the genuine-empty fast path. + let mut fetch_complete = true; for (identity_id, hw_received, hw_sent) in identities { // --- Fetch (no guard held during the awaits). --- @@ -1169,6 +1208,8 @@ impl DashPayView<'_, B> { error = %e, "Failed to fetch received contact requests; skipping this identity" ); + // Known-incomplete: this identity was not synced this pass. + fetch_complete = false; continue; } }; @@ -1191,6 +1232,9 @@ impl DashPayView<'_, B> { "Failed to fetch sent contact requests; reconciling received side only" ); sent_ok = false; + // Known-incomplete: the sent side did not reach Platform, so + // this pass cannot be reported as a settled empty either. + fetch_complete = false; Default::default() } }; @@ -1460,7 +1504,10 @@ impl DashPayView<'_, B> { self.enqueue_pending_auto_accepts(&identity_id).await; } - Ok(all_requests) + Ok(ContactRequestSyncOutcome { + requests: all_requests, + fetch_complete, + }) } /// Parse a received `contactRequest` document into a [`ContactRequest`], 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 ba68afb0c2..21d69fe891 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -460,18 +460,40 @@ impl IdentityWallet { } if tally.is_trustworthy() { - // Found something despite a failed probe: the discovered - // identities are already persisted, so return them rather than - // discarding the work. The gap is still worth a line — an identity - // at the failed index would be missed until the next scan. - if tally.failed_probes > 0 { + // A trustworthy scan reached Platform. Record whether it was + // COMPLETE — every probe answered, zero failed — so the ordered + // startup path preserves its warm-launch shortcut only for wallets + // we fully scanned. A scan that saw an identity but had a failed + // probe is trustworthy (its finds are already persisted) yet + // INCOMPLETE: an identity at the failed index is still unaccounted + // for. Marking such a wallet not-fully-discovered is the #4365 fix — + // the next launch re-runs discovery (resuming past the known + // identities) instead of shortcutting past that index forever. + let fully_scanned = tally.failed_probes == 0; + if !fully_scanned { tracing::warn!( "Identity discovery completed with {} unanswered probe(s); an identity at \ a failed index may be missing until the next scan", tally.failed_probes ); } + { + let mut wm_guard = self.wallet_manager.write().await; + if let Some(info) = wm_guard.get_wallet_info_mut(&self.wallet_id) { + info.identity_manager.set_wallet_discovery_complete( + self.wallet_id, + fully_scanned, + &self.persister, + ); + } + } } else { + // Untrustworthy: no identity seen and at least one probe failed, so + // we cannot tell empty-from-unreachable. Return the retryable error + // WITHOUT touching the completion flag — an all-failure rescan does + // not invalidate a prior clean scan, and a wallet still awaiting its + // first clean scan stays not-fully-discovered and re-scans next + // launch. return Err(tally.into_incomplete_error(start_index, identity_index)); } @@ -931,6 +953,54 @@ mod tests { assert!(tally.is_trustworthy()); } + /// #4365: the completion verdict that gates the startup warm-launch + /// shortcut is "every probe answered" (`failed_probes == 0`), NOT merely + /// "trustworthy". A scan that saw an identity but had a failed probe is + /// trustworthy — its finds are already persisted — yet INCOMPLETE: an + /// identity at the failed index is unaccounted for, so `discover_inner` must + /// mark the wallet NOT fully discovered and let the next launch re-scan + /// rather than shortcut past that index forever. + #[test] + fn completion_requires_every_probe_answered_not_just_trustworthiness() { + // Found identity 0, then a failed probe at index 1: trustworthy, incomplete. + let incomplete = run_scan( + 5, + [ + Ok(Some(())), + Err(()), + Ok(None), + Ok(None), + Ok(None), + Ok(None), + ], + ); + assert!(incomplete.is_trustworthy()); + // This is exactly `discover_inner`'s `fully_scanned` expression. + let fully_scanned = incomplete.failed_probes == 0; + assert!( + !fully_scanned, + "a trustworthy-with-failures scan must NOT be marked fully discovered (#4365)" + ); + + // Every probe answered: complete. + let complete = run_scan( + 5, + [ + Ok(Some(())), + Ok(None), + Ok(None), + Ok(None), + Ok(None), + Ok(None), + ], + ); + assert!(complete.is_trustworthy()); + assert_eq!( + complete.failed_probes, 0, + "a clean scan is fully discovered" + ); + } + /// A rescan re-confirms identities the manager already holds, which never /// reach the returned `discovered` list. Judging by that list called a /// scan that plainly reached Platform "incomplete" as soon as a later diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index c084ea667a..407886cb77 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -1098,7 +1098,7 @@ impl DashPayView<'_, B> { // re-acquires that (non-reentrant) lock internally. self.drain_pending_contact_crypto(provider).await; - let (payment_address, used_flip_changeset, tx, fee, funding_accounts) = { + let (payment_address, used_flip_changeset, tx, fee, funding_accounts, reservation_token) = { let mut wm = self.wallet_manager.write().await; // Resolve the external account's xpub so we can derive addresses. @@ -1269,13 +1269,24 @@ impl DashPayView<'_, B> { // `impl TransactionSigner for S`) rather than the // resident `wallet`, so funding-input signatures are produced // from Keychain-derived keys without a resident seed. - // `build_signed` returns the fee the transaction actually + // `build_signed_reserved` returns the fee the transaction actually // pays — Σ(selected input values) − Σ(output values), a // dropped sub-dust change remainder included — since // rust-dashcore#872 (pinned above). No caller-side // recomputation needed. - let (tx, fee) = match builder - .build_signed(signer, |addr| funding_paths.get(&addr).cloned()) + // + // `build_signed_reserved` (not `build_signed`) so we keep the + // `ReservationToken` this build stamped onto the selected inputs. + // `build_signed` reserves identically but drops the token, which + // forces every later release to be unconditional — and both later + // releases here (the store-failure abort and a rejected broadcast) + // run after `.await`s during which a TTL sweep could reclaim this + // reservation and a concurrent build re-reserve the same outpoints. + // An unconditional release would then free that newer build's + // inputs (the double-spend window of `dashpay/platform#4185`), so + // we thread the token through and release owner-guarded. + let (tx, fee, reservation_token) = match builder + .build_signed_reserved(signer, |addr| funding_paths.get(&addr).cloned()) .await { Ok(built) => built, @@ -1313,6 +1324,7 @@ impl DashPayView<'_, B> { tx, fee, offered_accounts, + reservation_token, ) }; @@ -1327,11 +1339,26 @@ impl DashPayView<'_, B> { // leaves a one-address gap that the pool's gap window absorbs on // retry — bounded, because a signed transaction exists here, unlike // the unbounded build-failure case rolled back above. - self.persister.store(used_flip_changeset).map_err(|e| { - PlatformWalletError::Persistence(format!( + if let Err(e) = self.persister.store(used_flip_changeset) { + // The used flip did not reach disk, so nothing will be broadcast on + // this path (the `?` return below is now this branch). Release the + // inputs this build reserved instead of stranding them until the TTL + // backstop — owner-guarded via the build's token so we never free a + // newer build's re-reservation of the same outpoints. Previously the + // `?` returned here WITHOUT releasing, leaving every input the build + // pooled (now the whole spendable set, per #4373) reserved until TTL. + crate::wallet::reservations::release_funding_reservations( + &self.wallet_manager, + &self.wallet_id, + &funding_accounts, + &tx, + reservation_token, + ) + .await; + return Err(PlatformWalletError::Persistence(format!( "failed to persist payment-address used flip: {e}" - )) - })?; + ))); + } // --- 3. Broadcast the transaction, releasing the build's UTXO // reservation if the broadcast is definitively rejected pre-send. --- @@ -1347,9 +1374,10 @@ impl DashPayView<'_, B> { &self.wallet_id, &funding_accounts, &tx, - // This path does not thread the build's reservation token - // either; keep the historical unconditional release. - None, + // Owner-guarded: pass the build's token so a TTL sweep + + // re-build that re-reserved these outpoints on a possibly- + // sent tx is not clobbered (`release_reservation_if_owner`). + reservation_token, ) .await; Err(e) @@ -1519,6 +1547,11 @@ mod tests { #[derive(Default)] struct RecordingPersister { stores: Mutex>, + /// When armed, `store` returns a backend error instead of recording. + /// Lets a test drive the pre-broadcast store-failure abort in + /// `send_payment`, which must release the build's UTXO reservation. + /// Defaults to disarmed, so existing tests are unaffected. + fail_stores: std::sync::atomic::AtomicBool, } impl PlatformWalletPersistence for RecordingPersister { @@ -1527,6 +1560,9 @@ mod tests { wallet_id: WalletId, changeset: PlatformWalletChangeSet, ) -> Result<(), PersistenceError> { + if self.fail_stores.load(std::sync::atomic::Ordering::SeqCst) { + return Err(PersistenceError::backend("store armed to fail")); + } self.stores.lock().unwrap().push((wallet_id, changeset)); Ok(()) } @@ -6342,6 +6378,61 @@ mod tests { ); } + /// #4373 (F3, consequence a): a pre-broadcast used-flip STORE FAILURE must + /// release the build's UTXO reservation. Before the fix the `?` returned + /// without releasing, so every input the build reserved — now the whole + /// spendable set, since #4373 widened the funding pool — stayed reserved + /// until the TTL backstop and an immediate retry failed with a spurious + /// insufficient-funds. + #[tokio::test] + async fn send_payment_store_failure_releases_the_reservation() { + use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; + use std::sync::atomic::Ordering; + + let (manager, persister, wallet_id, owner_id, contact_id) = + register_sender_and_external_account().await; + let wallet_arc = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet_arc.identity(); + + // A SINGLE funded UTXO: the retry can only succeed if the first send's + // store-failure released it — otherwise it is still reserved and coin + // selection refuses for lack of funds. + fund_bip44_account_0(&manager, wallet_id, 0xD1, 120_000).await; + + let seed = Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid mnemonic") + .to_seed(""); + let provider = SeedCryptoProvider::from_seed(seed, Network::Testnet); + let signer = SeedSigner::new(seed, Network::Testnet); + + // Arm the persister so the pre-broadcast used-flip store fails. The send + // builds + signs (reserving the UTXO), then the store fails and the + // send must release the reservation before returning. + persister.fail_stores.store(true, Ordering::SeqCst); + let accepting = with_accepting_broadcaster(iw); + let err = accepting + .dashpay() + .send_payment(&owner_id, &contact_id, 50_000, None, &signer, &provider) + .await + .expect_err("an armed store failure must abort the send"); + assert!( + matches!(err, PlatformWalletError::Persistence(_)), + "the send must fail at the used-flip persist, got: {err:?}" + ); + + // Disarm and retry: the retry can only reselect the single UTXO if the + // failed send released its reservation. + persister.fail_stores.store(false, Ordering::SeqCst); + accepting + .dashpay() + .send_payment(&owner_id, &contact_id, 50_000, None, &signer, &provider) + .await + .expect( + "an immediate retry must reselect the freed input — a reservation left held \ + after the store failure strands funds until the TTL backstop", + ); + } + /// A definitively rejected broadcast must return the consumed payment /// address to the pool AND persist the revert — unlike a failed build, /// the used flip was already persisted before the broadcast attempt, so diff --git a/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs b/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs index 33bbe80e4b..263a7bc8c5 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/state/manager/lifecycle.rs @@ -92,6 +92,59 @@ impl IdentityManager { Ok(()) } + /// Whether `wallet_id`'s most recent identity-discovery scan was COMPLETE — + /// every gap-limit probe answered, zero failed probes. + /// + /// The ordered-startup path (`crate::manager::startup`) consults this to + /// decide whether it may take its warm-launch shortcut (skip the network + /// scan when a local identity is already on file). Defaults to `false` for + /// a wallet never scanned to completion — the safe direction: re-scan + /// rather than shortcut past a not-yet-probed index (#4365). + pub fn is_wallet_fully_discovered(&self, wallet_id: &WalletId) -> bool { + self.fully_discovered_wallets.contains(wallet_id) + } + + /// Record whether `wallet_id`'s most recent identity-discovery scan was + /// COMPLETE, and persist the verdict. + /// + /// `complete == true` enters the wallet into `fully_discovered_wallets` + /// (enabling the startup warm-launch shortcut); `false` removes it, so the + /// next launch re-runs discovery instead of shortcutting past an index a + /// failed probe skipped (#4365). Persists the `identity_discovery_complete` + /// changeset flag via `persister` so the verdict survives restart on + /// backends that carry it; a persist failure is logged, not fatal (mirrors + /// [`Self::add_identity`]) — the worst case is one unnecessary rescan next + /// launch, which is safe. A no-op (no persist) when the verdict is + /// unchanged, so a clean rescan of an already-complete wallet costs no host + /// write. + pub fn set_wallet_discovery_complete( + &mut self, + wallet_id: WalletId, + complete: bool, + persister: &WalletPersister, + ) { + let changed = if complete { + self.fully_discovered_wallets.insert(wallet_id) + } else { + self.fully_discovered_wallets.remove(&wallet_id) + }; + if !changed { + return; + } + + let cs = PlatformWalletChangeSet { + identity_discovery_complete: Some(complete), + ..Default::default() + }; + if let Err(e) = persister.store(cs) { + tracing::error!( + wallet_id = %hex::encode(wallet_id), + error = %e, + "failed to persist identity-discovery completion flag" + ); + } + } + /// Add an identity to the out-of-wallet (observed read-only) bucket. /// /// Replaces the previous `add_watched_identity` — we no longer keep 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 4b13ae5c4c..920a829878 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 @@ -35,7 +35,7 @@ use crate::changeset::IdentityManagerStartState; use crate::wallet::platform_wallet::WalletId; use dpp::identity::accessors::IdentityGettersV0; use dpp::prelude::Identifier; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; /// Plain alias for the BIP-9 HD identity index used as the inner-bucket /// key for wallet-owned identities. Keeps the type signatures readable @@ -95,6 +95,24 @@ 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, + + /// Wallet ids whose most recent identity-discovery scan answered + /// EVERY gap-limit probe — a scan with zero failed probes. The + /// ordered-startup path (`crate::manager::startup`) takes its + /// warm-launch shortcut — skip the network scan when a local identity + /// is already on file — ONLY for wallets in this set. A wallet whose + /// last scan was incomplete (a failed probe left an index unprobed, or + /// the scan was abandoned at the startup budget) is absent, so startup + /// re-runs discovery on the next launch instead of shortcutting past an + /// index a failed probe skipped and stranding the identity there + /// (issue #4365). + /// + /// Round-trips through + /// [`IdentityManagerStartState`](crate::changeset::IdentityManagerStartState) + /// so a genuinely-complete wallet keeps its shortcut across restart on + /// backends that persist it. Absent-on-load defaults to "not complete", + /// the safe direction: re-scan rather than strand. + fully_discovered_wallets: BTreeSet, } impl From for IdentityManager { @@ -102,6 +120,7 @@ impl From for IdentityManager { let IdentityManagerStartState { out_of_wallet_identities, wallet_identities, + fully_discovered_wallets, } = state; // Rebuild the side-index from the two buckets — `IdentityManagerStartState` @@ -127,6 +146,7 @@ impl From for IdentityManager { out_of_wallet_identities, wallet_identities, location_index, + fully_discovered_wallets, } } } @@ -478,6 +498,89 @@ mod tests { assert_eq!(manager.identity_count(), 4); } + /// #4365: the identity-discovery completion flag round-trips through the + /// start state so a genuinely-complete wallet keeps its startup warm-launch + /// shortcut across a reload, while a wallet absent from the set defaults to + /// not-fully-discovered (re-scan, never strand). + #[test] + fn discovery_complete_flag_round_trips_through_start_state() { + let complete: WalletId = [21u8; 32]; + let other: WalletId = [22u8; 32]; + + let mut start = IdentityManagerStartState::default(); + start.fully_discovered_wallets.insert(complete); + + let manager: IdentityManager = start.into(); + assert!( + manager.is_wallet_fully_discovered(&complete), + "a wallet marked fully-discovered in the start state stays so after load" + ); + assert!( + !manager.is_wallet_fully_discovered(&other), + "a wallet absent from the set defaults to not-fully-discovered" + ); + } + + /// The setter flips membership both ways and persists each real transition + /// via a changeset carrying the verdict, so the flag survives restart on + /// backends that read it back. A no-op transition costs no persist. + #[test] + fn set_wallet_discovery_complete_toggles_and_persists() { + use crate::changeset::{ + ClientStartState, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, + }; + use std::sync::Mutex; + + #[derive(Default)] + struct CapturingPersister { + stored: Mutex>>, + } + impl PlatformWalletPersistence for CapturingPersister { + fn store( + &self, + _wallet_id: WalletId, + changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + self.stored + .lock() + .unwrap() + .push(changeset.identity_discovery_complete); + Ok(()) + } + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + fn load(&self) -> Result { + Ok(ClientStartState::default()) + } + } + + let wallet_id: WalletId = [7u8; 32]; + let capture = Arc::new(CapturingPersister::default()); + let persister = WalletPersister::new(wallet_id, capture.clone()); + + let mut manager = IdentityManager::new(); + assert!(!manager.is_wallet_fully_discovered(&wallet_id)); + + // Mark complete → membership set. + manager.set_wallet_discovery_complete(wallet_id, true, &persister); + assert!(manager.is_wallet_fully_discovered(&wallet_id)); + + // Re-marking complete is a no-op: no membership change, no extra persist. + manager.set_wallet_discovery_complete(wallet_id, true, &persister); + + // Mark incomplete → membership cleared. + manager.set_wallet_discovery_complete(wallet_id, false, &persister); + assert!(!manager.is_wallet_fully_discovered(&wallet_id)); + + let stored = capture.stored.lock().unwrap(); + assert_eq!( + *stored, + vec![Some(true), Some(false)], + "only the two real transitions persist, each carrying its verdict" + ); + } + #[test] fn lookup_by_id_finds_in_either_bucket() { let mut manager = IdentityManager::new(); diff --git a/packages/rs-platform-wallet/src/wallet/reservations.rs b/packages/rs-platform-wallet/src/wallet/reservations.rs index dc95dce484..a2cc002208 100644 --- a/packages/rs-platform-wallet/src/wallet/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/reservations.rs @@ -74,8 +74,9 @@ pub(crate) async fn broadcast_releasing_on_rejection>, wallet_id: &WalletId, funding_accounts: &[AccountType], @@ -104,19 +107,12 @@ pub(crate) async fn release_reservation_after_rejected_broadcast( tracing::warn!( wallet_id = %hex::encode(wallet_id), ?funding_accounts, - "could not release UTXO reservation after rejected broadcast: wallet not found" + "could not release UTXO reservation: wallet not found" ); return; }; for funding_account in funding_accounts { match info.core_wallet.accounts.funds_account(funding_account) { - // Owner-guarded when the build's `ReservationToken` is available: - // this cleanup always runs after `.await`s (build → broadcast), so - // the original reservation may have been swept and the same - // outpoints re-reserved by a NEWER build — an unconditional release - // would clobber that newer owner and make its inputs re-selectable - // by a conflicting transaction. Callers without a token (paths that - // predate token plumbing) keep the historical unconditional release. Some(account) => match reservation_token { Some(token) => account.release_reservation_if_owner(tx, token), None => account.release_reservation(tx), @@ -124,9 +120,35 @@ pub(crate) async fn release_reservation_after_rejected_broadcast( None => tracing::warn!( wallet_id = %hex::encode(wallet_id), ?funding_account, - "could not release UTXO reservation after rejected broadcast: \ - funds account not found" + "could not release UTXO reservation: funds account not found" ), } } } + +/// Release the funding accounts' UTXO reservations for `tx` after its +/// broadcast came back [`BroadcastError::Rejected`]. Thin alias over +/// [`release_funding_reservations`] — the name the broadcast-side callers read +/// against — see it for the owner-guard rationale. +/// +/// Callers that pair the release with other rejection cleanup must order +/// that cleanup **before** this call when it removes state a concurrent +/// flow could act on — while the reservation is still held the inputs +/// cannot be re-selected by a new build, so the pre-release window is +/// safe. +pub(crate) async fn release_reservation_after_rejected_broadcast( + wallet_manager: &RwLock>, + wallet_id: &WalletId, + funding_accounts: &[AccountType], + tx: &Transaction, + reservation_token: Option, +) { + release_funding_reservations( + wallet_manager, + wallet_id, + funding_accounts, + tx, + reservation_token, + ) + .await; +}