From 41319f6c985fbf1c5e2d4dd48e1780a8a558f0d5 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Mon, 10 Aug 2026 00:03:19 +0300 Subject: [PATCH 1/4] fix(platform-wallet): report an unanswered identity scan as incomplete, not empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `discover_inner` counted a failed `Identity::fetch` as a gap-limit miss, identical to a genuine "no identity at this index". With `IDENTITY_GAP_LIMIT = 5`, a few seconds of network trouble ended the scan with `Ok(vec![])` — which at the FFI boundary is indistinguishable from "this seed owns no identity". That is not a rare shape. Every probe verifies its GroveDB proof against quorum keys from a single HTTPS endpoint whose LRU cache is cold on each process start, so all five DAPI retries and all six index probes fail together when it is slow. dashwallet runs the scan once, immediately after restore-from-seed, and records an empty success as final for the session: the restored identity — and with it every DashPay contact and all contact payment history — stayed hidden until the app was relaunched. Count unanswered probes apart from real misses. They still advance the miss counter so a scan terminates when Platform is unreachable, but a scan that ends with nothing found and at least one unanswered probe now returns `IdentityDiscoveryIncomplete` instead of an empty success. Callers retry on error and cache an empty success, so this alone lets a transient failure heal on the next attempt. A scan that did find something keeps returning its identities — they are already persisted — with a warning that a failed index may still hide one. --- packages/rs-platform-wallet/src/error.rs | 21 +++++++++ .../src/wallet/identity/network/discovery.rs | 46 +++++++++++++++++-- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 4af28baf1a0..2c2664b4607 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -48,6 +48,27 @@ pub enum PlatformWalletError { #[error("Identity index not set for identity {0} — register or discover the identity first")] IdentityIndexNotSet(Identifier), + #[error( + "Identity discovery incomplete: {failed_probes} of {probed} index probe(s) from {start_index} \ + could not reach Platform and no identity was found; last error: {last_error}" + )] + /// A gap-limit scan ended empty with at least one index left unanswered. + /// Distinct from an empty success: it means "we do not know", so the + /// caller must retry rather than record that the seed owns no identity. + /// Both outcomes used to arrive as `Ok(vec![])`, which is how a transient + /// DAPI failure right after restore-from-seed became a whole session + /// without an identity. + IdentityDiscoveryIncomplete { + /// First index the scan probed. + start_index: u32, + /// How many indices were probed before the gap limit stopped the scan. + probed: u32, + /// How many of those probes failed to reach Platform. + failed_probes: u32, + /// Rendered last probe failure, for the log line at the FFI boundary. + last_error: String, + }, + #[error( "DashPay receiving account already exists for identity {identity} with contact {contact} on network {network:?} (account index {account_index})" )] 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 2731e5ed159..9b47e3e09be 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -339,6 +339,18 @@ impl IdentityWallet { let mut consecutive_misses = 0u32; let mut identity_index = start_index; let mut discovered: Vec = Vec::new(); + // A probe that never reached Platform is not evidence that the index is + // empty, so failures are counted apart from genuine misses. They still + // advance `consecutive_misses` — the scan has to terminate when the + // network is down — but a scan that ends with nothing found and at + // least one failed probe reports itself incomplete instead of + // returning `Ok(vec![])`. That empty-Ok was indistinguishable at the + // FFI boundary from "this seed owns no identity", and dashwallet + // cached it as final for the rest of the session: a few seconds of + // trouble right after restore-from-seed hid an existing identity (and + // with it every DashPay contact) until the app was relaunched. + let mut failed_probes = 0u32; + let mut last_probe_error: Option = None; while consecutive_misses < gap_limit { // Derive the MASTER auth pubkey hash for this identity index @@ -452,9 +464,12 @@ impl IdentityWallet { identity_index, e ); - // Treat a transient fetch error as a miss so the - // scan eventually terminates; the user can rerun - // to pick up anything we skipped over. + // Advance the miss counter so the scan still terminates + // when Platform is unreachable, but remember that this + // index was never actually answered — the result below + // depends on telling the two apart. + last_probe_error = Some(e.to_string()); + failed_probes += 1; consecutive_misses += 1; } } @@ -462,6 +477,31 @@ impl IdentityWallet { identity_index += 1; } + // Nothing found, and at least one index was never answered: the scan + // does not know whether an identity exists, so it must not claim there + // is none. Callers retry on error; they cache an empty success. + if discovered.is_empty() && failed_probes > 0 { + return Err(PlatformWalletError::IdentityDiscoveryIncomplete { + start_index, + probed: identity_index.saturating_sub(start_index), + failed_probes, + last_error: last_probe_error + .unwrap_or_else(|| "unknown probe failure".to_string()), + }); + } + + // 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 failed_probes > 0 { + tracing::warn!( + "Identity discovery completed with {} unanswered probe(s); an identity at a \ + failed index may be missing until the next scan", + failed_probes + ); + } + // --- DPNS lookup for all discovered identities --- for identity in &discovered { let identity_id = identity.id(); From 13067c961ba4d89dd102a42a1c909f0fdce4906d Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Mon, 10 Aug 2026 00:19:25 +0300 Subject: [PATCH 2/4] test(platform-wallet): pin the empty-vs-unanswered identity scan distinction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Name the rule the previous commit introduced and cover it. The whole defect was that two different endings — "no identity exists" and "no answer came back" — shared one counter and one return value, so the condition deserves to be a named predicate with the regression written down next to it rather than an inline `&&` nobody can grep for. --- .../src/wallet/identity/network/discovery.rs | 73 ++++++++++++++----- 1 file changed, 54 insertions(+), 19 deletions(-) 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 9b47e3e09be..61a195c63fe 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -477,31 +477,27 @@ impl IdentityWallet { identity_index += 1; } - // Nothing found, and at least one index was never answered: the scan - // does not know whether an identity exists, so it must not claim there - // is none. Callers retry on error; they cache an empty success. - if discovered.is_empty() && failed_probes > 0 { + if scan_result_is_trustworthy(discovered.len(), failed_probes) { + // 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 failed_probes > 0 { + tracing::warn!( + "Identity discovery completed with {} unanswered probe(s); an identity at \ + a failed index may be missing until the next scan", + failed_probes + ); + } + } else { return Err(PlatformWalletError::IdentityDiscoveryIncomplete { start_index, probed: identity_index.saturating_sub(start_index), failed_probes, - last_error: last_probe_error - .unwrap_or_else(|| "unknown probe failure".to_string()), + last_error: last_probe_error.unwrap_or_else(|| "unknown probe failure".to_string()), }); } - // 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 failed_probes > 0 { - tracing::warn!( - "Identity discovery completed with {} unanswered probe(s); an identity at a \ - failed index may be missing until the next scan", - failed_probes - ); - } - // --- DPNS lookup for all discovered identities --- for identity in &discovered { let identity_id = identity.id(); @@ -555,10 +551,27 @@ impl IdentityWallet { } } +/// Whether a finished gap-limit scan may be reported as its literal result. +/// +/// A scan answers "which identities does this seed own", and there are three +/// endings, not two: it found some, it confirmed there are none, or it never +/// got an answer. The third used to be reported as the second — a failed probe +/// incremented the same miss counter as an empty index, so a scan that reached +/// no one at all returned "this seed owns no identity". Callers cannot retry +/// what they were told is a definitive answer, so a few seconds of network +/// trouble after restore-from-seed cost a whole session's DashPay state. +/// +/// Emptiness is only trustworthy when every probe was actually answered. +/// A scan that found something is trustworthy either way: those identities +/// exist, and the caller re-scans later for anything a failed index hid. +fn scan_result_is_trustworthy(discovered: usize, failed_probes: u32) -> bool { + discovered > 0 || failed_probes == 0 +} + #[cfg(test)] mod tests { use super::super::identity_handle::derive_ecdsa_identity_auth_keypair_from_master; - use super::breadcrumb_decisions; + use super::{breadcrumb_decisions, scan_result_is_trustworthy}; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; use dpp::identity::v0::IdentityV0; @@ -812,4 +825,26 @@ mod tests { ); } } + + /// The regression this whole distinction exists for: a scan that found + /// nothing because it could not reach Platform must not be reported as a + /// scan that found nothing because there is nothing. + #[test] + fn empty_scan_with_unanswered_probes_is_not_trustworthy() { + assert!(!scan_result_is_trustworthy(0, 1)); + assert!(!scan_result_is_trustworthy(0, 5)); + } + + #[test] + fn empty_scan_with_every_probe_answered_is_trustworthy() { + assert!(scan_result_is_trustworthy(0, 0)); + } + + /// Discovered identities are already persisted, so a partial failure does + /// not discard them — it only means a later scan may find more. + #[test] + fn scan_that_found_something_is_trustworthy_despite_failures() { + assert!(scan_result_is_trustworthy(1, 0)); + assert!(scan_result_is_trustworthy(1, 3)); + } } From 359ff97454e03426553969a8e1944895013981ef Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Mon, 10 Aug 2026 01:05:57 +0300 Subject: [PATCH 3/4] fix(platform-wallet): judge scan reachability by sightings, not new identities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scan_result_is_trustworthy` was fed `discovered.len()`, but `discovered` collects only identities the manager did not already track — the push is guarded by `is_new`. A rescan from index 0 therefore re-confirms a known identity without adding anything to that list, so a single failed probe at a later index made the scan report itself incomplete even though Platform had plainly answered. That is the app's "Find identities" command: it always scans from 0, and for a wallet whose identity is already known it would have started returning an error instead of "no new identities were found". Count every index Platform answered with an identity and judge by that. Regression test asserts both directions — the sighting count passes where the `discovered` length would have failed. Reported by CodeRabbit on #4352. --- .../src/wallet/identity/network/discovery.rs | 40 ++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) 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 61a195c63fe..a314dc211a3 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -351,6 +351,10 @@ impl IdentityWallet { // with it every DashPay contact) until the app was relaunched. let mut failed_probes = 0u32; let mut last_probe_error: Option = None; + // Identities Platform answered with, including ones the manager + // already tracked. `discovered` holds only the NEW ones, so it is the + // wrong thing to judge reachability by — see `identities_seen += 1`. + let mut identities_seen = 0usize; while consecutive_misses < gap_limit { // Derive the MASTER auth pubkey hash for this identity index @@ -453,6 +457,11 @@ impl IdentityWallet { if is_new { discovered.push(identity.clone()); } + // Counted for every sighting, not just new ones: a rescan + // that re-confirms an identity the manager already holds + // still proves Platform answered, and `discovered` stays + // empty in that case. + identities_seen += 1; consecutive_misses = 0; } Ok(None) => { @@ -477,7 +486,7 @@ impl IdentityWallet { identity_index += 1; } - if scan_result_is_trustworthy(discovered.len(), failed_probes) { + if scan_result_is_trustworthy(identities_seen, failed_probes) { // 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 @@ -562,10 +571,18 @@ impl IdentityWallet { /// trouble after restore-from-seed cost a whole session's DashPay state. /// /// Emptiness is only trustworthy when every probe was actually answered. -/// A scan that found something is trustworthy either way: those identities -/// exist, and the caller re-scans later for anything a failed index hid. -fn scan_result_is_trustworthy(discovered: usize, failed_probes: u32) -> bool { - discovered > 0 || failed_probes == 0 +/// A scan that saw an identity is trustworthy either way: it reached Platform +/// and the seed demonstrably owns one, and the caller re-scans later for +/// anything a failed index hid. +/// +/// `identities_seen` counts every index Platform answered with an identity — +/// NOT the length of the returned `discovered` list, which holds only the ones +/// the manager did not already track. A rescan from index 0 (what the app's +/// "Find identities" command does) re-confirms known identities without adding +/// to `discovered`, so judging by that list would report a reachable scan as +/// incomplete the moment any later index failed. +fn scan_result_is_trustworthy(identities_seen: usize, failed_probes: u32) -> bool { + identities_seen > 0 || failed_probes == 0 } #[cfg(test)] @@ -847,4 +864,17 @@ mod tests { assert!(scan_result_is_trustworthy(1, 0)); assert!(scan_result_is_trustworthy(1, 3)); } + + /// A rescan from index 0 re-confirms identities the manager already holds, + /// which never reach the returned `discovered` list. Judging by that list + /// would call a scan that plainly reached Platform "incomplete" as soon as + /// a later index failed — turning the app's "Find identities" command into + /// an error for a wallet whose identity was found successfully. + #[test] + fn rescan_of_a_known_identity_is_trustworthy_despite_a_later_failure() { + let identities_seen = 1; // answered at index 0, but is_new == false + let discovered_len = 0; // …so nothing was added to `discovered` + assert!(scan_result_is_trustworthy(identities_seen, 4)); + assert!(!scan_result_is_trustworthy(discovered_len, 4)); + } } From 387647701cc512dd30c9441bc94cd3ea106f66f2 Mon Sep 17 00:00:00 2001 From: Roman <51091564+jeanpierreroma@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:02:19 +0300 Subject: [PATCH 4/4] refactor(platform-wallet): give the identity scan a tally its tests can drive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both suggestions on #4352. **Keep the probe failure typed.** `IdentityDiscoveryIncomplete` carried the last error as an already-rendered `String`, so a Rust caller could only parse the message back. The variant now holds `Box` as `#[source]`: `thiserror` still renders it into the message the FFI boundary logs, and callers get the variant. This matters because the error's contract is "retry", while a probe can also fail on configuration, protocol or proof errors that retrying will not fix — classifying that needs the typed error. **Test the bookkeeping, not just the verdict.** The counters were locals in `discover_inner` and the tests asserted on a bare predicate, so they kept passing no matter what the scan fed it — including the exact `discovered.len()` wiring defect the previous commit fixed. The counters and the verdict move into `ScanTally`, which the scan and the tests now both drive: `run_scan` replays a sequence of probe outcomes through the same methods and the same gap-limit loop condition production uses. Verified by mutation: dropping `identities_seen += 1` fails three tests, dropping `failed_probes += 1` fails three others. Under the old tests both mutations passed. Adds coverage for the gap-limit behaviour that had none: a sighting resets the miss run, unanswered probes still terminate the scan, and the error carries its source. --- packages/rs-platform-wallet/src/error.rs | 13 +- .../src/wallet/identity/network/discovery.rs | 265 +++++++++++++----- 2 files changed, 202 insertions(+), 76 deletions(-) diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 306c722bb4c..8dab7054699 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -50,7 +50,7 @@ pub enum PlatformWalletError { #[error( "Identity discovery incomplete: {failed_probes} of {probed} index probe(s) from {start_index} \ - could not reach Platform and no identity was found; last error: {last_error}" + could not reach Platform and no identity was found; last error: {source}" )] /// A gap-limit scan ended empty with at least one index left unanswered. /// Distinct from an empty success: it means "we do not know", so the @@ -58,6 +58,11 @@ pub enum PlatformWalletError { /// Both outcomes used to arrive as `Ok(vec![])`, which is how a transient /// DAPI failure right after restore-from-seed became a whole session /// without an identity. + /// + /// "Retry" is the contract, not a promise that the cause is transient — a + /// probe can also fail on configuration, protocol or proof errors. The + /// underlying failure is kept typed in `source` so a Rust caller can + /// classify it instead of parsing the rendered message. IdentityDiscoveryIncomplete { /// First index the scan probed. start_index: u32, @@ -65,8 +70,10 @@ pub enum PlatformWalletError { probed: u32, /// How many of those probes failed to reach Platform. failed_probes: u32, - /// Rendered last probe failure, for the log line at the FFI boundary. - last_error: String, + /// The last probe failure. Boxed to keep this variant from widening + /// the enum past the existing `Sdk` variant. + #[source] + source: Box, }, #[error( 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 a314dc211a3..ba68afb0c29 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -336,27 +336,11 @@ impl IdentityWallet { let start_index = opts.start_index.unwrap_or(cached_start_index); let gap_limit = opts.gap_limit.max(1); - let mut consecutive_misses = 0u32; let mut identity_index = start_index; let mut discovered: Vec = Vec::new(); - // A probe that never reached Platform is not evidence that the index is - // empty, so failures are counted apart from genuine misses. They still - // advance `consecutive_misses` — the scan has to terminate when the - // network is down — but a scan that ends with nothing found and at - // least one failed probe reports itself incomplete instead of - // returning `Ok(vec![])`. That empty-Ok was indistinguishable at the - // FFI boundary from "this seed owns no identity", and dashwallet - // cached it as final for the rest of the session: a few seconds of - // trouble right after restore-from-seed hid an existing identity (and - // with it every DashPay contact) until the app was relaunched. - let mut failed_probes = 0u32; - let mut last_probe_error: Option = None; - // Identities Platform answered with, including ones the manager - // already tracked. `discovered` holds only the NEW ones, so it is the - // wrong thing to judge reachability by — see `identities_seen += 1`. - let mut identities_seen = 0usize; - - while consecutive_misses < gap_limit { + 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 @@ -457,15 +441,10 @@ impl IdentityWallet { if is_new { discovered.push(identity.clone()); } - // Counted for every sighting, not just new ones: a rescan - // that re-confirms an identity the manager already holds - // still proves Platform answered, and `discovered` stays - // empty in that case. - identities_seen += 1; - consecutive_misses = 0; + tally.record_sighting(); } Ok(None) => { - consecutive_misses += 1; + tally.record_miss(); } Err(e) => { tracing::warn!( @@ -473,38 +452,27 @@ impl IdentityWallet { identity_index, e ); - // Advance the miss counter so the scan still terminates - // when Platform is unreachable, but remember that this - // index was never actually answered — the result below - // depends on telling the two apart. - last_probe_error = Some(e.to_string()); - failed_probes += 1; - consecutive_misses += 1; + tally.record_failure(e); } } identity_index += 1; } - if scan_result_is_trustworthy(identities_seen, failed_probes) { + 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 failed_probes > 0 { + if tally.failed_probes > 0 { tracing::warn!( "Identity discovery completed with {} unanswered probe(s); an identity at \ a failed index may be missing until the next scan", - failed_probes + tally.failed_probes ); } } else { - return Err(PlatformWalletError::IdentityDiscoveryIncomplete { - start_index, - probed: identity_index.saturating_sub(start_index), - failed_probes, - last_error: last_probe_error.unwrap_or_else(|| "unknown probe failure".to_string()), - }); + return Err(tally.into_incomplete_error(start_index, identity_index)); } // --- DPNS lookup for all discovered identities --- @@ -560,7 +528,7 @@ impl IdentityWallet { } } -/// Whether a finished gap-limit scan may be reported as its literal result. +/// Running bookkeeping for one gap-limit scan, and the verdict it produces. /// /// A scan answers "which identities does this seed own", and there are three /// endings, not two: it found some, it confirmed there are none, or it never @@ -570,25 +538,87 @@ impl IdentityWallet { /// what they were told is a definitive answer, so a few seconds of network /// trouble after restore-from-seed cost a whole session's DashPay state. /// -/// Emptiness is only trustworthy when every probe was actually answered. -/// A scan that saw an identity is trustworthy either way: it reached Platform -/// and the seed demonstrably owns one, and the caller re-scans later for -/// anything a failed index hid. -/// -/// `identities_seen` counts every index Platform answered with an identity — -/// NOT the length of the returned `discovered` list, which holds only the ones -/// the manager did not already track. A rescan from index 0 (what the app's -/// "Find identities" command does) re-confirms known identities without adding -/// to `discovered`, so judging by that list would report a reachable scan as -/// incomplete the moment any later index failed. -fn scan_result_is_trustworthy(identities_seen: usize, failed_probes: u32) -> bool { - identities_seen > 0 || failed_probes == 0 +/// The counters live here, rather than as locals in `discover_inner`, so tests +/// drive the same bookkeeping production does. Asserting on a bare predicate +/// proved too weak: it kept passing while the scan fed it the wrong number. +#[derive(Default)] +struct ScanTally { + /// Empty-or-unanswered indices since the last sighting. Terminates the + /// scan at the gap limit. + consecutive_misses: u32, + /// Probes that never reached Platform. + failed_probes: u32, + /// 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 + /// does) re-confirms known identities without adding to `discovered`, so + /// judging by that list reports a plainly reachable scan as incomplete the + /// moment any later index fails. + identities_seen: usize, + /// Last probe failure, kept typed so callers can inspect the variant + /// rather than parse a rendered string. + last_probe_error: Option, +} + +impl ScanTally { + /// Whether the scan should probe another index. + fn should_continue(&self, gap_limit: u32) -> bool { + self.consecutive_misses < gap_limit + } + + /// Platform answered with an identity at this index. + fn record_sighting(&mut self) { + self.identities_seen += 1; + self.consecutive_misses = 0; + } + + /// Platform answered, definitively, that this index holds no identity. + fn record_miss(&mut self) { + self.consecutive_misses += 1; + } + + /// 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) { + self.last_probe_error = Some(error); + self.failed_probes += 1; + self.consecutive_misses += 1; + } + + /// Whether the scan's literal result may be reported as-is. + /// + /// Emptiness is only trustworthy when every probe was answered. A scan + /// that saw an identity is trustworthy either way: it reached Platform and + /// the seed demonstrably owns one, and the caller re-scans later for + /// anything a failed index hid. + fn is_trustworthy(&self) -> bool { + self.identities_seen > 0 || self.failed_probes == 0 + } + + /// The error an untrustworthy scan returns, carrying the last probe + /// failure as its `source`. + fn into_incomplete_error(self, start_index: u32, next_index: u32) -> PlatformWalletError { + PlatformWalletError::IdentityDiscoveryIncomplete { + start_index, + probed: next_index.saturating_sub(start_index), + failed_probes: self.failed_probes, + // Unreachable by construction: `is_trustworthy` only returns false + // when `failed_probes > 0`, and every failure records its error. + // Named rather than papered over with a plausible-looking cause. + source: Box::new(self.last_probe_error.unwrap_or_else(|| { + dash_sdk::Error::Generic( + "identity discovery reported a failed probe with no recorded error".to_string(), + ) + })), + } + } } #[cfg(test)] mod tests { use super::super::identity_handle::derive_ecdsa_identity_auth_keypair_from_master; - use super::{breadcrumb_decisions, scan_result_is_trustworthy}; + use super::{breadcrumb_decisions, PlatformWalletError, ScanTally}; use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; use dpp::identity::v0::IdentityV0; @@ -843,38 +873,127 @@ mod tests { } } + fn probe_failure() -> dash_sdk::Error { + dash_sdk::Error::Generic("dapi unreachable".to_string()) + } + + /// Drive a whole scan through the same `ScanTally` production uses, one + /// probe outcome per element, stopping at the gap limit exactly as + /// `discover_inner`'s loop does. `Some(())` is a sighting, `None` a + /// confirmed miss, `Err` an unanswered probe. + fn run_scan( + gap_limit: u32, + outcomes: impl IntoIterator, ()>>, + ) -> ScanTally { + let mut tally = ScanTally::default(); + for outcome in outcomes { + if !tally.should_continue(gap_limit) { + break; + } + match outcome { + Ok(Some(())) => tally.record_sighting(), + Ok(None) => tally.record_miss(), + Err(()) => tally.record_failure(probe_failure()), + } + } + tally + } + /// The regression this whole distinction exists for: a scan that found /// nothing because it could not reach Platform must not be reported as a /// scan that found nothing because there is nothing. #[test] - fn empty_scan_with_unanswered_probes_is_not_trustworthy() { - assert!(!scan_result_is_trustworthy(0, 1)); - assert!(!scan_result_is_trustworthy(0, 5)); + fn scan_that_never_reached_platform_is_not_trustworthy() { + let tally = run_scan(5, [Err(()), Err(()), Err(()), Err(()), Err(())]); + assert_eq!(tally.failed_probes, 5); + assert_eq!(tally.identities_seen, 0); + assert!(!tally.is_trustworthy()); } + /// The genuinely-empty wallet: every probe answered, all of them "none". #[test] - fn empty_scan_with_every_probe_answered_is_trustworthy() { - assert!(scan_result_is_trustworthy(0, 0)); + fn scan_with_every_probe_answered_empty_is_trustworthy() { + let tally = run_scan(5, [Ok(None), Ok(None), Ok(None), Ok(None), Ok(None)]); + assert_eq!(tally.failed_probes, 0); + assert!(tally.is_trustworthy()); } /// Discovered identities are already persisted, so a partial failure does /// not discard them — it only means a later scan may find more. #[test] fn scan_that_found_something_is_trustworthy_despite_failures() { - assert!(scan_result_is_trustworthy(1, 0)); - assert!(scan_result_is_trustworthy(1, 3)); + let tally = run_scan( + 5, + [Ok(Some(())), Err(()), Ok(None), Err(()), Ok(None), Ok(None)], + ); + assert_eq!(tally.identities_seen, 1); + assert_eq!(tally.failed_probes, 2); + assert!(tally.is_trustworthy()); } - /// A rescan from index 0 re-confirms identities the manager already holds, - /// which never reach the returned `discovered` list. Judging by that list - /// would call a scan that plainly reached Platform "incomplete" as soon as - /// a later index failed — turning the app's "Find identities" command into - /// an error for a wallet whose identity was found successfully. + /// 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 + /// index failed — turning the app's "Find identities" command into an + /// error for a wallet whose identity was found successfully. + /// + /// `discovered` stays empty for the whole scan here, which is exactly the + /// value the fixed wiring must NOT be judging by. #[test] fn rescan_of_a_known_identity_is_trustworthy_despite_a_later_failure() { - let identities_seen = 1; // answered at index 0, but is_new == false - let discovered_len = 0; // …so nothing was added to `discovered` - assert!(scan_result_is_trustworthy(identities_seen, 4)); - assert!(!scan_result_is_trustworthy(discovered_len, 4)); + let tally = run_scan( + 5, + [Ok(Some(())), Err(()), Err(()), Err(()), Err(()), Err(())], + ); + assert_eq!(tally.identities_seen, 1, "the known identity was seen"); + assert!(tally.failed_probes > 0); + assert!(tally.is_trustworthy()); + } + + /// A sighting resets the gap, so an identity past a run of empties is + /// still reachable — and the scan does not stop early. + #[test] + fn a_sighting_resets_the_consecutive_miss_run() { + let tally = run_scan(3, [Ok(None), Ok(None), Ok(Some(())), Ok(None), Ok(None)]); + assert_eq!(tally.identities_seen, 1); + assert_eq!(tally.consecutive_misses, 2); + assert!(tally.should_continue(3)); + } + + /// An unanswered probe still has to stop the scan, or an offline device + /// would walk indices forever. + #[test] + fn unanswered_probes_still_terminate_the_scan() { + let tally = run_scan(3, [Err(()), Err(()), Err(()), Err(()), Err(())]); + assert!(!tally.should_continue(3)); + assert_eq!( + tally.failed_probes, 3, + "stopped at the gap limit, not after 5" + ); + } + + /// The failure reaches the caller typed, not flattened to a string. + #[test] + fn incomplete_error_carries_the_probe_failure_as_its_source() { + use std::error::Error as _; + + let tally = run_scan(2, [Err(()), Err(())]); + let error = tally.into_incomplete_error(0, 2); + match &error { + PlatformWalletError::IdentityDiscoveryIncomplete { + start_index, + probed, + failed_probes, + source, + } => { + assert_eq!(*start_index, 0); + assert_eq!(*probed, 2); + assert_eq!(*failed_probes, 2); + assert!(matches!(**source, dash_sdk::Error::Generic(_))); + } + other => panic!("expected IdentityDiscoveryIncomplete, got {other:?}"), + } + assert!(error.source().is_some(), "source must survive for callers"); + assert!(error.to_string().contains("dapi unreachable")); } }