From 16afd79b7a26a528e8b3a62e72c39211f752a468 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:37:32 +0000 Subject: [PATCH 1/5] fix(platform-wallet): typed persister errors with bounded transient retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Persistence failures on the wallet rehydration and registration paths were flattened into `PlatformWalletError::WalletCreation(String)`, destroying the transient/fatal classification callers need and severing the `#[source]` chain. Adds typed `PersisterLoad` / `PersisterStore` / `PersisterRestore` variants carrying the `PersistenceError` (boxed for the recursive restore case) and routes every persister boundary through them. On top of that, `retry_transient` (4 attempts, 20 -> 200 ms doubling backoff) now wraps persister `store` / `flush` / `load` on the registration, startup and identity-discovery paths, so a transient `SQLITE_BUSY` no longer aborts wallet registration outright or costs the identity-scan verdict its durability (#4365). Fatal errors still fail fast. The retry re-drives a failed `store` via a bare `flush`, which `PlatformWalletPersistence::store` now documents as a backend contract. Also fixes the persister leak behind #4133: a failed `load_from_persistor` left the wallet-event adapter holding an `Arc

` clone, so re-opening the same path returned a spurious `AlreadyOpen` masking the real error. `load_from_persistor` now shuts the manager down on both failure paths, with a `Drop` backstop cancelling and aborting the adapter task. `record_or_persister_or_log` and `reconcile_sent_payments` stop swallowing permanent read failures as "not found": transient errors still defer to the next sweep, permanent ones propagate as `PersisterLoad` instead of stalling an unbounded poll loop with no explanation. 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent --- .../src/changeset/traits.rs | 13 + packages/rs-platform-wallet/src/error.rs | 34 ++ .../rs-platform-wallet/src/manager/load.rs | 214 ++++++- .../rs-platform-wallet/src/manager/mod.rs | 32 ++ .../rs-platform-wallet/src/manager/startup.rs | 26 +- .../src/manager/wallet_lifecycle.rs | 523 +++++++++++++++++- .../src/wallet/asset_lock/sync/proof.rs | 79 ++- .../src/wallet/identity/network/discovery.rs | 30 +- .../src/wallet/identity/network/payments.rs | 43 +- 9 files changed, 926 insertions(+), 68 deletions(-) diff --git a/packages/rs-platform-wallet/src/changeset/traits.rs b/packages/rs-platform-wallet/src/changeset/traits.rs index 60d98195ba9..16e8ac8d217 100644 --- a/packages/rs-platform-wallet/src/changeset/traits.rs +++ b/packages/rs-platform-wallet/src/changeset/traits.rs @@ -266,6 +266,19 @@ pub trait PlatformWalletPersistence: Send + Sync { /// wallet accessor (readers and writers) for its duration. Keep the /// per-call work bounded; if the backend does inline I/O (see the type /// doc), size it accordingly. + /// + /// # Transient-failure retry contract + /// + /// An implementation that returns a [`PersistenceError`] classified + /// [`PersistenceErrorKind::Transient`] from `store` **MUST** have already + /// buffered/preserved the changeset so that a subsequent bare + /// [`flush`](Self::flush) — with no re-supplied changeset — completes the + /// write (mirroring `flush`'s own transient contract). This is what lets a + /// caller retry a transient `store` failure via `flush` alone; re-calling + /// `store` with the same changeset would double-merge it. An + /// implementation that cannot preserve the changeset on failure MUST + /// classify that failure [`PersistenceErrorKind::Fatal`] (or + /// [`Constraint`](PersistenceErrorKind::Constraint)), never `Transient`. fn store( &self, wallet_id: WalletId, diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index d24412b3007..a4c24264a2d 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -14,6 +14,40 @@ pub enum PlatformWalletError { #[error("Wallet creation failed: {0}")] WalletCreation(String), + /// The persister failed to load the client start state during + /// rehydration. Carries the typed [`PersistenceError`] so callers keep + /// its retry classification (`is_transient()` / + /// [`PersistenceErrorKind`]) instead of a flattened string — a + /// transient backend hiccup (e.g. `SQLITE_BUSY`) stays distinguishable + /// from a permanent failure and can be retried. + /// + /// [`PersistenceError`]: crate::changeset::PersistenceError + /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind + #[error("failed to load persisted client state: {0}")] + PersisterLoad(#[from] crate::changeset::PersistenceError), + + /// The persister failed to store the wallet-registration changeset. + /// Like [`Self::PersisterLoad`], it carries the typed + /// [`PersistenceError`] so the retry classification (`is_transient()` + /// / [`PersistenceErrorKind`]) survives the boundary — a transient + /// `SQLITE_BUSY` stays distinguishable from a permanent failure. + /// Distinct from [`Self::PersisterLoad`] so callers can tell a failed + /// registration write from a failed rehydration read; not `#[from]` + /// because that conversion is already claimed by [`Self::PersisterLoad`]. + /// + /// [`PersistenceError`]: crate::changeset::PersistenceError + /// [`PersistenceErrorKind`]: crate::changeset::PersistenceErrorKind + #[error("failed to persist wallet registration changeset: {0}")] + PersisterStore(#[source] crate::changeset::PersistenceError), + + /// Restoring the persisted platform-address state into the freshly + /// registered wallet failed. Wraps the underlying + /// [`PlatformWalletError`](Self) (boxed to break the recursive type) so + /// its concrete variant and `#[source]` chain survive instead of being + /// flattened into a string. + #[error("failed to restore persisted platform-address state: {0}")] + PersisterRestore(#[source] Box), + #[error("Wallet not found: {0}")] WalletNotFound(String), diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index 3588aef2b54..ed65cdd0283 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -10,7 +10,7 @@ use crate::wallet::identity::IdentityManager; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use crate::wallet::PlatformWallet; -use super::PlatformWalletManager; +use super::{wallet_lifecycle::retry_transient, PlatformWalletManager}; impl PlatformWalletManager

{ /// Load the full [`ClientStartState`] from the configured persister @@ -30,6 +30,22 @@ impl PlatformWalletManager

{ /// /// [`WalletManager`]: key_wallet_manager::WalletManager pub async fn load_from_persistor(&self) -> Result<(), PlatformWalletError> { + let start_state = match retry_transient(|| self.persister.load()).await { + Ok(state) => state, + Err(e) => { + // Preserve the typed source chain (Debug carries the real + // cause — e.g. a bincode decode failure) instead of flattening + // it to a Display string, and release the wallet-event adapter + // so a reconstruct on the same path doesn't hit `AlreadyOpen` + // masking this error. + tracing::debug!(error = ?e, "persister load failed during rehydration"); + let report = self.shutdown().await; + if !report.all_clean() { + tracing::warn!(?report, "wallet workers unclean after aborting rehydration"); + } + return Err(PlatformWalletError::PersisterLoad(e)); + } + }; let ClientStartState { mut platform_addresses, wallets, @@ -37,12 +53,7 @@ impl PlatformWalletManager

{ // not here — drop the snapshot at this entry point. #[cfg(feature = "shielded")] shielded: _, - } = self.persister.load().map_err(|e| { - PlatformWalletError::WalletCreation(format!( - "Failed to load persisted client state: {}", - e - )) - })?; + } = start_state; // Tracked (wallet-independent) masternodes ride the same startup // hydration; a failure logs and starts empty rather than failing @@ -237,6 +248,16 @@ impl PlatformWalletManager

{ } } } + // Release the wallet-event adapter so a reconstruct on the same + // persister path doesn't hit `AlreadyOpen` (see the early-return + // path above). + let report = self.shutdown().await; + if !report.all_clean() { + tracing::warn!( + ?report, + "wallet workers left unclean after rolling back a failed rehydration" + ); + } return Err(err); } @@ -363,3 +384,182 @@ mod idempotent_load_tests { ); } } + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + use super::*; + use crate::changeset::{PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet}; + use crate::events::{EventHandler, PlatformEventHandler}; + + /// Persister whose `load()` always fails — the failure path under test. + struct FailingLoadPersister; + + impl PlatformWalletPersistence for FailingLoadPersister { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + + fn load(&self) -> Result { + Err(PersistenceError::backend("simulated load failure")) + } + } + + struct TransientOnceLoadPersister { + load_calls: AtomicUsize, + } + + impl PlatformWalletPersistence for TransientOnceLoadPersister { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + + fn load(&self) -> Result { + if self.load_calls.fetch_add(1, Ordering::SeqCst) == 0 { + return Err(PersistenceError::backend_with_kind( + PersistenceErrorKind::Transient, + "simulated transient load failure", + )); + } + Ok(ClientStartState::default()) + } + } + + struct NoopEventHandler; + impl EventHandler for NoopEventHandler {} + impl PlatformEventHandler for NoopEventHandler {} + + #[tokio::test] + async fn transient_load_failure_during_startup_rehydration_is_retried() { + let persister = Arc::new(TransientOnceLoadPersister { + load_calls: AtomicUsize::new(0), + }); + let probe = Arc::clone(&persister); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let handler: Arc = Arc::new(NoopEventHandler); + let manager = PlatformWalletManager::new(sdk, persister, handler); + + manager + .load_from_persistor() + .await + .expect("transient startup load failure must be retried"); + + assert_eq!(probe.load_calls.load(Ordering::SeqCst), 2); + } + + /// A failed `load_from_persistor` must (a) surface the typed `PersisterLoad` + /// error preserving the source chain, and (b) release the wallet-event + /// adapter's `Arc` clone so a reconstruct on the same path + /// doesn't hit `WalletStorageError::AlreadyOpen` masking the real error + /// (issue #4133). + /// + /// This is a **manager-side proxy**, not a full end-to-end proof: it asserts + /// the persister's strong count returns to 1 (the test's own probe) after a + /// failed load + teardown — a lingering adapter clone would keep it above 1 + /// — which is the necessary precondition for a clean re-open. It does not + /// itself open a real `SqlitePersister`, fail, and re-open on the same path; + /// the platform-wallet ⇄ platform-wallet-storage dev-dependency cycle + /// precludes using the concrete persister here. That end-to-end + /// open → fail → reopen is covered by the storage crate's own round-trip + /// coverage test. + // Multi-thread: dropping the manager runs upstream's `Drop`, whose + // `ThreadRegistry::shutdown()` asserts a multi-thread runtime. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn failed_load_releases_persister_for_reconstruct() { + let persister = Arc::new(FailingLoadPersister); + let probe = Arc::clone(&persister); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let handler: Arc = Arc::new(NoopEventHandler); + + let manager = PlatformWalletManager::new(sdk, persister, handler); + + let err = manager + .load_from_persistor() + .await + .expect_err("load must fail"); + assert!( + matches!(err, PlatformWalletError::PersisterLoad(_)), + "load failure must surface as the typed PersisterLoad variant, got {err:?}" + ); + + drop(manager); + // Asserted directly, never polled: the failure path awaits the + // adapter's `JoinHandle` inside `shutdown`, so the task's clone is + // already released before `drop` runs. Release on THIS path is + // synchronous, which is the stronger guarantee — a poll loop (or a + // `yield_now`, which cedes nothing to another worker) would only + // hide a regression into eventual release. + assert_eq!( + Arc::strong_count(&probe), + 1, + "after a failed load + teardown nothing may still hold the persister" + ); + } + + /// The `Drop` backstop alone (no `shutdown` first) must *eventually* release + /// the adapter's `Arc` clone. Unlike the graceful path this is + /// not synchronous: `Drop::drop` calls `abort()`, which only *requests* + /// cancellation — the runtime drops the aborted task (and its clone) at its + /// next poll. So the strong count is polled, not asserted immediately, which + /// is exactly the "eventual, not synchronous" contract the `Drop` impl's + /// doc-comment describes. This is the branch the graceful-path test above + /// never exercises (there `shutdown` has already taken the join handle, so + /// `Drop`'s `abort` sees `None`). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn drop_backstop_eventually_releases_persister_without_shutdown() { + let persister = Arc::new(FailingLoadPersister); + let probe = Arc::clone(&persister); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let handler: Arc = Arc::new(NoopEventHandler); + + let manager = PlatformWalletManager::new(sdk, persister, handler); + // The adapter task spawned in `new()` holds a clone, so the count is + // above the probe before any teardown. + assert!( + Arc::strong_count(&probe) > 1, + "the spawned adapter task must hold an Arc clone" + ); + + // Dirty drop: never call `shutdown`, so `Drop`'s `abort` is the only + // thing that can reclaim the adapter's clone. + drop(manager); + + // Release is eventual: poll until the aborted task is dropped by the + // runtime rather than asserting immediately. The wait must be a timed + // sleep, not `yield_now`: the aborted task is reclaimed by whichever + // worker thread owns it, and yielding this thread never forces that + // one to run — the whole budget can burn in microseconds while the + // clone is still live. Breaks on the first observation, so the 2s + // ceiling is only ever paid by a genuine regression. + let mut count = Arc::strong_count(&probe); + for _ in 0..2_000 { + if count == 1 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(1)).await; + count = Arc::strong_count(&probe); + } + assert_eq!( + count, 1, + "the Drop backstop must eventually release the persister after aborting the adapter" + ); + } +} diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index 9192dfae148..cc484031aca 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -11,6 +11,11 @@ pub mod shielded_sync; pub mod startup; mod wallet_lifecycle; +/// Re-exported so the identity-scan verdict publishers under `wallet::` +/// retry on the same policy the registration path uses. The module itself +/// stays private — this is the only item it owes the rest of the crate. +pub(crate) use wallet_lifecycle::retry_transient; + use std::sync::Arc; use std::time::Duration; @@ -1038,6 +1043,33 @@ impl PlatformWalletManager

{ } } +/// Drop backstop for the wallet-event adapter task. +/// +/// The graceful teardown is [`shutdown`](PlatformWalletManager::shutdown) +/// (cancel + await the join). A dirty drop that skips it would otherwise merely +/// detach the `JoinHandle`, leaving the adapter task running and holding its +/// `Arc

` clone — which keeps the persister "open" and turns a later re-open +/// on the same path into a spurious `WalletStorageError::AlreadyOpen` that +/// masks the real error (issue #4133). Cancelling the token and aborting the +/// task here starts that release — but note it is *eventual*, not synchronous: +/// `abort()` only requests cancellation, so the runtime drops the task (and its +/// `Arc

` clone) at the task's next poll, not inside this `drop`. In practice +/// the adapter loop parks on an `.await` almost every iteration, so the clone is +/// reclaimed promptly. Only the graceful +/// [`shutdown`](PlatformWalletManager::shutdown) path *guarantees* the reference +/// is gone before it returns (it awaits the join); this backstop guarantees +/// eventual reclamation, not synchronous. +impl Drop for PlatformWalletManager

{ + fn drop(&mut self) { + self.event_adapter_cancel.cancel(); + // `get_mut` needs no runtime (we hold `&mut self`); `abort` is + // non-blocking. `None` when `shutdown` already took the handle. + if let Some(handle) = self.event_adapter_join.get_mut().take() { + handle.abort(); + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/packages/rs-platform-wallet/src/manager/startup.rs b/packages/rs-platform-wallet/src/manager/startup.rs index 168512f4d40..187d269d433 100644 --- a/packages/rs-platform-wallet/src/manager/startup.rs +++ b/packages/rs-platform-wallet/src/manager/startup.rs @@ -969,8 +969,10 @@ impl PlatformWalletManager /// 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. + /// Mirrors what `discover` publishes for itself, retry policy included; + /// needed separately because a scan dropped mid-await never reaches its own + /// bookkeeping. This is the verdict least affordable to lose — it is the + /// one that re-opens the identity question on the next launch. async fn record_identity_scan_cut_off(&self, wallet_id: &WalletId) { // Coverage of nothing: the scan was dropped mid-await, so it answered // no index and may not clear one an earlier scan left open. @@ -988,12 +990,24 @@ impl PlatformWalletManager identity_scan_state: Some(recorded), ..Default::default() }; - if let Err(e) = self.persister.store(*wallet_id, changeset) { - tracing::warn!( + // Transient failures are ridden out on the registration path's bounded + // policy; the buffer preserves the changeset, so the retries re-drive + // it through `flush`. The final outcome is still swallowed — an + // abandoned scan must not turn a shutdown into an error. + let mut changeset_slot = Some(changeset); + let outcome = crate::manager::retry_transient(|| match changeset_slot.take() { + Some(cs) => self.persister.store(*wallet_id, cs), + None => self.persister.flush(*wallet_id), + }) + .await; + if let Err(e) = outcome { + tracing::error!( wallet_id = %hex::encode(wallet_id), + transient = e.is_transient(), error = %e, - "failed to persist an abandoned scan's verdict; the next launch may take the \ - warm shortcut over an incomplete identity set" + "abandoned scan's verdict could not be persisted after retries; the next \ + launch will take the warm shortcut over an identity set nothing proved \ + complete" ); } } diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index 71bef57d723..4cc2dce78da 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -12,7 +12,7 @@ use key_wallet::Network; #[cfg(any(feature = "bls", feature = "eddsa"))] use crate::changeset::ProviderKeyExtendedPubKey; use crate::changeset::{ - AccountAddressPoolEntry, AccountRegistrationEntry, PlatformWalletChangeSet, + AccountAddressPoolEntry, AccountRegistrationEntry, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, ProviderKeyAccountEntry, WalletMetadataEntry, }; use crate::error::PlatformWalletError; @@ -51,6 +51,56 @@ fn parse_mnemonic_any_language(phrase: &str) -> Result { Err("phrase does not match any supported BIP-39 wordlist") } +/// Total attempts (initial + retries) for a transient-classified persister +/// operation on the wallet-registration path. Small on purpose: this runs +/// inline while creating a wallet, not as a background job — a lock blip +/// should be ridden out in well under a second, and a genuinely stuck +/// backend must still surface promptly. +const PERSIST_RETRY_MAX_ATTEMPTS: u32 = 4; + +/// Backoff before the first retry; doubles on each subsequent attempt. +const PERSIST_RETRY_INITIAL_BACKOFF: std::time::Duration = std::time::Duration::from_millis(20); + +/// Ceiling for the doubling backoff so registration latency stays bounded +/// (worst case with the constants above: 20 + 40 + 80 ≈ 140 ms). +const PERSIST_RETRY_MAX_BACKOFF: std::time::Duration = std::time::Duration::from_millis(200); + +/// Retry a synchronous persister operation while it fails *transiently*, +/// using bounded exponential backoff. +/// +/// `op` runs once, then re-runs after a backoff sleep for as long as it +/// returns a [`PersistenceError`] whose +/// [`is_transient()`](PersistenceError::is_transient) is true, up to +/// [`PERSIST_RETRY_MAX_ATTEMPTS`]. A fatal error (or success) returns +/// immediately — a fatal failure never retries. The sleep is async so it +/// yields the Tokio worker instead of spinning the CPU, which is exactly +/// what the storage layer's `FlushRetryable` contract asks callers to do. +pub(crate) async fn retry_transient(mut op: F) -> Result +where + F: FnMut() -> Result, +{ + let mut backoff = PERSIST_RETRY_INITIAL_BACKOFF; + let mut attempt: u32 = 1; + loop { + match op() { + Ok(value) => return Ok(value), + Err(e) if e.is_transient() && attempt < PERSIST_RETRY_MAX_ATTEMPTS => { + tracing::debug!( + attempt, + max_attempts = PERSIST_RETRY_MAX_ATTEMPTS, + backoff_ms = backoff.as_millis() as u64, + error = %e, + "transient persister failure — backing off before retry" + ); + tokio::time::sleep(backoff).await; + backoff = backoff.saturating_mul(2).min(PERSIST_RETRY_MAX_BACKOFF); + attempt += 1; + } + Err(e) => return Err(e), + } + } +} + /// Test-only rendezvous fired inside [`PlatformWalletManager::remove_wallet_with_teardown`], /// between the inner-manager removal and the public-map removal. /// @@ -484,24 +534,38 @@ impl PlatformWalletManager

{ } } - if let Err(e) = self.persister.store(wallet_id, registration_changeset) { + // Persist the registration changeset, riding out a *transient* + // backend blip (e.g. `SQLITE_BUSY`) with bounded exponential backoff + // before giving up. On a transient `store` failure the persister + // restores the buffered changeset (its documented contract), so the + // retries re-drive that same write via `flush` — no re-merge, no + // double-count: the first attempt hands the changeset over, later + // attempts flush what the buffer preserved. A fatal error is not + // retried and fails fast. Either way the typed `PersistenceError` + // (and its transient/fatal classification) is preserved for the + // caller instead of being flattened to a string. + let mut changeset_slot = Some(registration_changeset); + let store_result = retry_transient(|| match changeset_slot.take() { + Some(cs) => self.persister.store(wallet_id, cs), + None => self.persister.flush(wallet_id), + }) + .await; + if let Err(e) = store_result { tracing::error!( wallet_id = %hex::encode(wallet_id), + transient = e.is_transient(), error = %e, - "failed to persist wallet registration changeset" + "failed to persist wallet registration changeset after retries" ); let mut wm = self.wallet_manager.write().await; - if let Err(e) = wm.remove_wallet(&wallet_id) { + if let Err(remove_err) = wm.remove_wallet(&wallet_id) { tracing::warn!( wallet_id = %hex::encode(wallet_id), - error = %e, + error = %remove_err, "rollback: remove_wallet failed while unwinding a failed wallet registration" ); } - return Err(PlatformWalletError::WalletCreation(format!( - "Failed to persist wallet registration changeset: {}", - e - ))); + return Err(PlatformWalletError::PersisterStore(e)); } // Build the PlatformWallet handle. @@ -531,26 +595,36 @@ impl PlatformWalletManager

{ // earlier `insert_wallet`, absent from `self.wallets`), // poisoning every retry on `WalletAlreadyExists`. Roll back // before bailing — same shape as `manager::load`. + // Retry a transient load blip the same way as the store above; a + // load is an idempotent read, so re-reading after a lock blip is + // safe. `load_persisted()` returns the typed `PersistenceError` this + // rehydration boundary is built around, routed through the + // dedicated `PersisterLoad` variant so its retry classification + // survives to the caller. + let load_result = retry_transient(|| platform_wallet.load_persisted()).await; let crate::changeset::ClientStartState { mut platform_addresses, wallets: _, #[cfg(feature = "shielded")] shielded: _, - } = match platform_wallet.load_persisted() { + } = match load_result { Ok(state) => state, Err(e) => { + tracing::error!( + wallet_id = %hex::encode(wallet_id), + transient = e.is_transient(), + error = %e, + "failed to load persisted wallet state after retries" + ); let mut wm = self.wallet_manager.write().await; - if let Err(e) = wm.remove_wallet(&wallet_id) { + if let Err(remove_err) = wm.remove_wallet(&wallet_id) { tracing::warn!( wallet_id = %hex::encode(wallet_id), - error = %e, + error = %remove_err, "rollback: remove_wallet failed while unwinding a failed wallet setup" ); } - return Err(PlatformWalletError::WalletCreation(format!( - "Failed to load persisted wallet state: {}", - e - ))); + return Err(PlatformWalletError::PersisterLoad(e)); } }; @@ -560,18 +634,23 @@ impl PlatformWalletManager

{ .initialize_from_persisted(persisted) .await { + tracing::error!( + wallet_id = %hex::encode(wallet_id), + error = %e, + "failed to restore persisted platform-address state" + ); let mut wm = self.wallet_manager.write().await; - if let Err(e) = wm.remove_wallet(&wallet_id) { + if let Err(remove_err) = wm.remove_wallet(&wallet_id) { tracing::warn!( wallet_id = %hex::encode(wallet_id), - error = %e, + error = %remove_err, "rollback: remove_wallet failed while unwinding a failed wallet setup" ); } - return Err(PlatformWalletError::WalletCreation(format!( - "Failed to restore persisted platform address state: {}", - e - ))); + // `initialize_from_persisted` already returns a typed + // `PlatformWalletError`; wrap (boxed) rather than stringify so + // its concrete variant and source chain survive. + return Err(PlatformWalletError::PersisterRestore(Box::new(e))); } } else { platform_wallet.platform().initialize().await; @@ -1275,6 +1354,406 @@ mod register_wallet_duplicate_tests { } } +#[cfg(test)] +mod persist_retry_tests { + //! Registration-path persistence: transient-error retry and typed + //! error classification across the boundary. + + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + use key_wallet::mnemonic::{Language, Mnemonic}; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::Network; + + use crate::changeset::{ + ClientStartState, PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet, + PlatformWalletPersistence, + }; + use crate::error::PlatformWalletError; + use crate::events::{EventHandler, PlatformEventHandler}; + use crate::wallet::platform_wallet::WalletId; + use crate::PlatformWalletManager; + + // Canonical all-`abandon` BIP-39 test vector. + const TEST_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon \ + abandon abandon abandon abandon abandon about"; + + fn transient() -> PersistenceError { + PersistenceError::backend_with_kind( + PersistenceErrorKind::Transient, + "simulated SQLITE_BUSY", + ) + } + + fn fatal() -> PersistenceError { + PersistenceError::backend_with_kind(PersistenceErrorKind::Fatal, "simulated corruption") + } + + /// Persister whose `store` / `flush` / `load` outcomes are scripted so + /// the registration retry path can be driven deterministically. Models + /// the real contract: a transient `store` failure preserves the + /// changeset in the buffer, so the retry re-drives the write through + /// `flush`. + /// + /// `store` counts registration and identity-scan-verdict writes + /// separately. Registration ends with a best-effort `identity().sync()`, + /// so a successful registration issues a SECOND `store` carrying the scan + /// verdict; a single counter would make every assertion about the + /// registration write depend on unrelated discovery behaviour. The + /// changeset itself is the discriminator. + #[derive(Default)] + struct FaultyPersister { + /// Stores of the registration changeset. + registration_store_calls: AtomicUsize, + /// Stores of the identity-scan verdict published by `identity().sync()`. + scan_verdict_store_calls: AtomicUsize, + flush_calls: AtomicUsize, + load_calls: AtomicUsize, + /// The first registration `store` fails transiently (buffer preserved + /// for retry). + store_transient_first: bool, + /// Every registration `store` fails fatally (must NOT retry). + store_fatal: bool, + /// Number of leading scan-verdict `store` calls that fail transiently. + scan_verdict_store_transient_failures: usize, + /// Number of leading `flush` calls that fail transiently before Ok. + flush_transient_failures: usize, + /// Number of leading `load` calls that fail transiently before Ok. + load_transient_failures: usize, + /// Every `load` fails fatally (must NOT retry). + load_fatal: bool, + } + + impl PlatformWalletPersistence for FaultyPersister { + fn store( + &self, + _wallet_id: WalletId, + changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + // One changeset can carry both: `merge` folds a buffered + // registration write and a scan verdict into a single round. Each + // counter answers only its own question — "was this changeset + // handed over?" — so both increment. Letting the first match win + // would make an assertion about the registration write depend on + // whether discovery happened to be batched with it, which is the + // coupling these separate counters exist to remove. + let registration = changeset + .wallet_metadata + .is_some() + .then(|| self.registration_store_calls.fetch_add(1, Ordering::SeqCst)); + let verdict = changeset + .identity_scan_state + .is_some() + .then(|| self.scan_verdict_store_calls.fetch_add(1, Ordering::SeqCst)); + + // The registration half decides a combined round's outcome: its + // failure aborts the whole registration, while a verdict's is + // swallowed. + if let Some(n) = registration { + if self.store_fatal { + return Err(fatal()); + } + if self.store_transient_first && n == 0 { + return Err(transient()); + } + } + if let Some(n) = verdict { + if n < self.scan_verdict_store_transient_failures { + return Err(transient()); + } + } + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + let n = self.flush_calls.fetch_add(1, Ordering::SeqCst); + if n < self.flush_transient_failures { + Err(transient()) + } else { + Ok(()) + } + } + + fn load(&self) -> Result { + let n = self.load_calls.fetch_add(1, Ordering::SeqCst); + if self.load_fatal { + return Err(fatal()); + } + if n < self.load_transient_failures { + return Err(transient()); + } + Ok(ClientStartState::default()) + } + } + + struct NoopEventHandler; + impl EventHandler for NoopEventHandler {} + impl PlatformEventHandler for NoopEventHandler {} + + fn make_manager( + persister: Arc, + ) -> Arc> { + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let event_handler: Arc = Arc::new(NoopEventHandler); + Arc::new(PlatformWalletManager::new(sdk, persister, event_handler)) + } + + fn seed_bytes() -> [u8; 64] { + Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid test mnemonic") + .to_seed("") + } + + /// `Some(0)` skips the SPV-tip birth-height lookup so the test never + /// consults SPV. + async fn register( + manager: &PlatformWalletManager, + ) -> Result<(), PlatformWalletError> { + manager + .create_wallet_from_seed_bytes( + Network::Testnet, + &seed_bytes(), + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .map(|_| ()) + } + + /// A transient `store` failure is ridden out — the persister + /// buffers the changeset, the retry re-drives it via `flush`, and + /// registration succeeds instead of aborting. + #[tokio::test] + async fn transient_store_failure_is_retried_and_succeeds() { + let persister = Arc::new(FaultyPersister { + store_transient_first: true, + flush_transient_failures: 1, // one transient flush, then Ok + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + register(&manager) + .await + .expect("registration must succeed after retrying the transient store"); + + // store attempted once; flush retried twice (fail, then succeed). + assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 2); + // Registration ends in `identity().sync()`, whose scan publishes its + // verdict — the write that makes a partial scan survive a restart. + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 1, + "a completed registration must publish the identity-scan verdict" + ); + } + + /// A fatal `store` failure fails fast — no retry — and + /// surfaces as the typed `PersisterStore` whose inner classification is + /// non-transient. + #[tokio::test] + async fn fatal_store_failure_fails_fast_without_retry() { + let persister = Arc::new(FaultyPersister { + store_fatal: true, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("a fatal store must abort registration"); + + match err { + PlatformWalletError::PersisterStore(pe) => assert!( + !pe.is_transient(), + "a fatal store must carry non-transient classification" + ), + other => panic!("expected PersisterStore, got {other:?}"), + } + assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); + assert_eq!( + persister.flush_calls.load(Ordering::SeqCst), + 0, + "a fatal store must not be retried via flush" + ); + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 0, + "an aborted registration never reaches the discovery scan" + ); + } + + /// A store that stays transient exhausts the + /// bounded retry budget and returns the typed `PersisterStore` still + /// carrying transient classification (distinguishable from the fatal + /// case above). + #[tokio::test] + async fn persistently_transient_store_exhausts_bounded_retries() { + let persister = Arc::new(FaultyPersister { + store_transient_first: true, + flush_transient_failures: usize::MAX, // never recovers + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("registration must fail once the retry budget is spent"); + + match err { + PlatformWalletError::PersisterStore(pe) => assert!( + pe.is_transient(), + "an exhausted-but-transient store must stay classified transient" + ), + other => panic!("expected PersisterStore, got {other:?}"), + } + // 1 store + 3 flush retries == 4 total attempts (the budget). + assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 3); + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 0, + "an aborted registration never reaches the discovery scan" + ); + } + + /// A transient `load` blip during rehydration is retried (an + /// idempotent read), so registration succeeds. + #[tokio::test] + async fn transient_load_failure_is_retried_and_succeeds() { + let persister = Arc::new(FaultyPersister { + load_transient_failures: 1, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + register(&manager) + .await + .expect("registration must succeed after retrying the transient load"); + + assert_eq!(persister.registration_store_calls.load(Ordering::SeqCst), 1); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 0); + assert_eq!(persister.load_calls.load(Ordering::SeqCst), 2); + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 1, + "a completed registration must publish the identity-scan verdict" + ); + } + + /// A fatal `load` fails fast and surfaces as the typed + /// `PersisterLoad` — never the flattened `WalletCreation(String)`. + #[tokio::test] + async fn fatal_load_failure_surfaces_as_persister_load() { + let persister = Arc::new(FaultyPersister { + load_fatal: true, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("a fatal load must abort registration"); + + match err { + PlatformWalletError::PersisterLoad(pe) => assert!(!pe.is_transient()), + other => panic!("expected PersisterLoad, got {other:?}"), + } + assert_eq!( + persister.load_calls.load(Ordering::SeqCst), + 1, + "a fatal load must not be retried" + ); + } + + /// A transient failure persisting the identity-scan verdict is ridden out + /// on the same bounded policy the registration write uses, so a merely + /// busy backend does not cost the verdict its survival across a restart + /// (dashpay/platform#4365). + #[tokio::test] + async fn should_retry_a_transient_scan_verdict_store() { + let persister = Arc::new(FaultyPersister { + scan_verdict_store_transient_failures: 1, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + register(&manager) + .await + .expect("a retried scan-verdict store must not disturb registration"); + + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 1, + "the verdict is handed over once; the retry re-drives it via flush" + ); + assert_eq!( + persister.flush_calls.load(Ordering::SeqCst), + 1, + "the transient verdict store must be retried through flush" + ); + } + + /// Retrying the verdict never escalates into failing the scan that just + /// succeeded: once the budget is spent the outcome is logged and dropped, + /// and registration still returns Ok. + #[tokio::test] + async fn should_not_fail_registration_when_the_scan_verdict_never_persists() { + let persister = Arc::new(FaultyPersister { + scan_verdict_store_transient_failures: usize::MAX, + flush_transient_failures: usize::MAX, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + register(&manager) + .await + .expect("an unpersistable verdict must never fail wallet registration"); + + // 1 store + 3 flush retries == the shared 4-attempt budget. + assert_eq!(persister.scan_verdict_store_calls.load(Ordering::SeqCst), 1); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 3); + } + + /// The typed persister-phase variants preserve retry + /// classification, enable structural matching, and keep the `#[source]` + /// chain instead of flattening to a string. + #[test] + fn typed_variants_preserve_classification_matching_and_source() { + use std::error::Error; + + let store_err = PlatformWalletError::PersisterStore(transient()); + match &store_err { + PlatformWalletError::PersisterStore(pe) => assert!(pe.is_transient()), + other => panic!("expected PersisterStore, got {other:?}"), + } + assert!( + store_err.source().is_some(), + "PersisterStore must expose its PersistenceError source" + ); + + let load_err = PlatformWalletError::PersisterLoad(fatal()); + match &load_err { + PlatformWalletError::PersisterLoad(pe) => assert!(!pe.is_transient()), + other => panic!("expected PersisterLoad, got {other:?}"), + } + assert!(load_err.source().is_some()); + + // The restore variant wraps a typed inner error; structural matching + // must recover the concrete inner variant, not an opaque string. + let restore_err = + PlatformWalletError::PersisterRestore(Box::new(PlatformWalletError::WalletLocked)); + assert!(restore_err.source().is_some()); + match restore_err { + PlatformWalletError::PersisterRestore(inner) => { + assert!(matches!(*inner, PlatformWalletError::WalletLocked)); + } + other => panic!("expected PersisterRestore, got {other:?}"), + } + } +} + /// Removal versus a same-id re-registration that lands *during* the removal /// (`dashpay/platform#4185` review). /// diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs index 6d4b674d965..49f34530e22 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs @@ -27,9 +27,9 @@ use super::super::manager::AssetLockManager; /// Persister errors are surfaced as `Err(PersistenceError)` so call /// sites can choose their own policy: /// -/// - **Poll loops** (`wait_for_chain_lock`, `wait_for_proof`) typically -/// downgrade to `None` for the current iteration so the next tick -/// retries — see [`record_or_persister_or_log`] for that policy. +/// - **Poll loops** (`wait_for_chain_lock`, `wait_for_proof`) downgrade +/// transient failures to `None` for the current iteration and surface +/// permanent failures — see [`record_or_persister_or_log`]. /// - **One-shot recovery / fast-fail call sites** want the error /// visible so a transient backend failure isn't silently classified /// as "tx not found" — they handle the `Err` arm explicitly. @@ -143,26 +143,27 @@ pub(in crate::wallet::asset_lock) fn record_holds_local_finality( } } -/// Variant of [`record_or_persister`] that swallows persister errors -/// as `None` after a `warn`-level log. Use this from poll loops where -/// the next iteration retries — a hard error from a single tick would -/// abort the whole poll prematurely. +/// Variant of [`record_or_persister`] that retries transient failures as a miss. +/// +/// Use this from poll loops where the next iteration retries. Permanent +/// failures remain errors so an unbounded poll cannot hide them. pub(super) fn record_or_persister_or_log( in_memory: Option, persister: &crate::wallet::persister::WalletPersister, txid: &Txid, -) -> Option { +) -> Result, crate::changeset::PersistenceError> { match record_or_persister(in_memory, persister, txid) { - Ok(opt) => opt, - Err(e) => { + Ok(opt) => Ok(opt), + Err(e) if e.is_transient() => { tracing::warn!( txid = %txid, error = %e, - "Persister fallback for core tx record failed; \ + "Transient persister fallback for core tx record failed; \ treating as miss for this poll iteration" ); - None + Ok(None) } + Err(e) => Err(e), } } @@ -393,7 +394,7 @@ impl AssetLockManager { }) }; if let Some(record) = - record_or_persister_or_log(in_memory, &self.persister, &out_point.txid) + record_or_persister_or_log(in_memory, &self.persister, &out_point.txid)? { if matches!(record.context, TransactionContext::InChainLockedBlock(_)) { if let Some(h) = record.height() { @@ -520,7 +521,7 @@ impl AssetLockManager { }) }; if let Some(record) = - record_or_persister_or_log(in_memory, &self.persister, &out_point.txid) + record_or_persister_or_log(in_memory, &self.persister, &out_point.txid)? { match &record.context { TransactionContext::InstantSend(instant_lock) => { @@ -969,8 +970,7 @@ mod tests { } } - /// Test persister that always errors out on `get_core_tx_record`, - /// to exercise the error-swallowing branch in `record_or_persister`. + /// Test persister that returns a permanent `get_core_tx_record` error. struct ErroringStore; impl PlatformWalletPersistence for ErroringStore { @@ -996,6 +996,34 @@ mod tests { } } + struct TransientErroringStore; + + impl PlatformWalletPersistence for TransientErroringStore { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + Ok(()) + } + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + fn load(&self) -> Result { + Ok(ClientStartState::default()) + } + fn get_core_tx_record( + &self, + _wallet_id: WalletId, + _txid: &Txid, + ) -> Result, PersistenceError> { + Err(PersistenceError::backend_with_kind( + crate::changeset::PersistenceErrorKind::Transient, + "simulated transient backend failure", + )) + } + } + fn wallet_persister(inner: Arc) -> WalletPersister { WalletPersister::new([0u8; 32], inner) } @@ -1060,9 +1088,7 @@ mod tests { #[test] fn record_or_persister_propagates_backend_errors() { // Backend errors surface as `Err` so call sites can choose - // their own policy (one-shot recovery logs at error and - // degrades; poll loops downgrade to None for one tick via - // `record_or_persister_or_log`). + // their own policy; poll loops only downgrade transient errors. let unknown_txid = Txid::from([0xFF; 32]); let persister = wallet_persister(Arc::new(ErroringStore)); @@ -1071,14 +1097,21 @@ mod tests { } #[test] - fn record_or_persister_or_log_swallows_backend_errors_as_none() { - // The poll-loop variant downgrades errors to `None` (after a - // `warn` log) so a transient backend failure on one tick - // doesn't abort the whole poll. + fn record_or_persister_or_log_surfaces_permanent_backend_errors() { let unknown_txid = Txid::from([0xFF; 32]); let persister = wallet_persister(Arc::new(ErroringStore)); let resolved = record_or_persister_or_log(None, &persister, &unknown_txid); + assert!(resolved.is_err()); + } + + #[test] + fn record_or_persister_or_log_retries_transient_backend_errors() { + let unknown_txid = Txid::from([0xFF; 32]); + let persister = wallet_persister(Arc::new(TransientErroringStore)); + + let resolved = record_or_persister_or_log(None, &persister, &unknown_txid) + .expect("transient poll error must be downgraded for retry"); assert!(resolved.is_none()); } } 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 93dbfe1ff5b..50731a63871 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -640,9 +640,14 @@ impl IdentityWallet { /// 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. + /// survival across a restart, and it must not be allowed to fail the scan + /// that just succeeded. + /// + /// Best-effort is not one-shot, though. A backend that is merely busy + /// would otherwise cost the verdict its durability outright, which is the + /// gap the verdict exists to close (dashpay/platform#4365), so a transient + /// failure is ridden out on the same bounded policy the registration path + /// uses before the outcome is swallowed. async fn publish_scan_verdict( &self, wallet_id: crate::wallet::platform_wallet::WalletId, @@ -672,12 +677,23 @@ impl IdentityWallet { identity_scan_state: Some(recorded), ..Default::default() }; - if let Err(e) = self.persister.store(changeset) { - tracing::warn!( + // On a transient `store` failure the persister keeps the changeset + // buffered (its documented contract), so the retries re-drive that + // same write through `flush` rather than handing it over twice. + let mut changeset_slot = Some(changeset); + let outcome = crate::manager::retry_transient(|| match changeset_slot.take() { + Some(cs) => self.persister.store(cs), + None => self.persister.flush(), + }) + .await; + if let Err(e) = outcome { + tracing::error!( wallet_id = %hex::encode(wallet_id), + transient = e.is_transient(), error = %e, - "failed to persist the identity-scan verdict; a partial scan may not be \ - retried after a restart" + "identity-scan verdict could not be persisted after retries; a partial scan \ + will not be retried after a restart, so an identity at an unanswered index \ + stays hidden until a later scan publishes a verdict that lands" ); } } 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 d59c3250390..659a430ffe3 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -670,6 +670,11 @@ impl DashPayView<'_, B> { /// retried on the next sweep. /// /// Returns the number of entries confirmed this pass. + /// + /// # Errors + /// + /// Transient persistence read failures are deferred to the next sweep; + /// permanent failures return [`PlatformWalletError::PersisterLoad`]. pub async fn reconcile_sent_payments(&self) -> Result { use crate::wallet::identity::types::dashpay::payment::{PaymentDirection, PaymentStatus}; @@ -704,14 +709,16 @@ impl DashPayView<'_, B> { let record = match self.persister.get_core_tx_record(&txid) { Ok(Some(record)) => record, Ok(None) => continue, - Err(e) => { + Err(e) if e.is_transient() => { tracing::warn!( error = %e, txid = %txid_str, - "reconcile_sent_payments: tx-record read failed; will retry next sweep" + "reconcile_sent_payments: transient tx-record read failed; \ + will retry next sweep" ); continue; } + Err(e) => return Err(PlatformWalletError::PersisterLoad(e)), }; // An InstantSend lock is final for DashPay display, same as a // mined block — one definition of "final", shared with the @@ -1648,7 +1655,8 @@ mod tests { use key_wallet::Network; use crate::changeset::{ - ClientStartState, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, + ClientStartState, PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet, + PlatformWalletPersistence, }; use crate::error::PlatformWalletError; use crate::events::{EventHandler, PlatformEventHandler}; @@ -1698,6 +1706,9 @@ mod tests { key_wallet::managed_account::transaction_record::TransactionRecord, >, >, + /// `Some(kind)` makes every `get_core_tx_record` fail with that + /// error class instead of answering from `records`. + read_error_kind: Mutex>, /// Txids the enumeration lists but `get_core_tx_record` answers /// `Ok(None)` for — the FFI shape for "row exists, record not /// available yet" (missing bytes, undecodable, pending InstantSend). @@ -1746,6 +1757,12 @@ mod tests { PersistenceError, > { *self.get_core_tx_record_calls.lock().unwrap() += 1; + if let Some(kind) = *self.read_error_kind.lock().unwrap() { + return Err(PersistenceError::backend_with_kind( + kind, + "simulated tx-record read failure", + )); + } if self.listed_but_unavailable.lock().unwrap().contains(txid) { return Ok(None); } @@ -3584,6 +3601,26 @@ mod tests { 0, "reconcile must be idempotent" ); + + *persister.read_error_kind.lock().unwrap() = Some(PersistenceErrorKind::Transient); + assert_eq!( + iw.dashpay() + .reconcile_sent_payments() + .await + .expect("transient read failure must wait for the next sweep"), + 0 + ); + + *persister.read_error_kind.lock().unwrap() = Some(PersistenceErrorKind::Fatal); + let err = iw + .dashpay() + .reconcile_sent_payments() + .await + .expect_err("permanent read failure must abort the reconcile sweep"); + assert!(matches!( + err, + PlatformWalletError::PersisterLoad(ref source) if !source.is_transient() + )); } #[tokio::test] From f93aa3ff9f5f9edee49539154507890eb49dfa65 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:38:40 +0000 Subject: [PATCH 2/5] feat(platform-wallet-ffi): distinguishable persister result codes 49/50 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `From` flattened every persistence failure into the generic `ErrorWalletOperation` (6), so hosts lost the transient/fatal classification the wallet layer now preserves. Adds `ErrorPersisterFatal = 49` and `ErrorPersisterTransient = 50`, claimed from the registry's allocation frontier, and de-flattens the conversion: `PersisterLoad` / `PersisterStore` map on `is_transient()`, `PersisterRestore` unwraps to its typed inner error. Ships the full three-layer parity the registry mandates — Rust enum with a discriminant pin test, `ERROR_CODE_REGISTRY.md` rows 47-50 with the frontier moved to 51, the Swift `PlatformWalletResultCode` / `PlatformWalletError` mirrors, and Swift raw-value pins. The variants are declared in ascending discriminant order (49 then 50), the order both enums otherwise keep; the comment records why 50 is not 48. 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent --- .../ERROR_CODE_REGISTRY.md | 44 +++--- packages/rs-platform-wallet-ffi/src/error.rs | 128 +++++++++++++++++- .../PlatformWallet/PlatformWalletResult.swift | 21 +++ .../ErrorHandlingTests.swift | 44 ++++++ 4 files changed, 215 insertions(+), 22 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md b/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md index 181eefe62f2..b941e94fef4 100644 --- a/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md +++ b/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md @@ -98,7 +98,7 @@ These are shipped ABI. Do not renumber. | 24 | `ErrorAssetLockAlreadyConsumed` | | | 25 | `ErrorAssetLockFundingMismatch` | | | 26 | `ErrorTransactionBroadcastRejected` | Merged in `9302c62e8b`; took a number several open branches had been treating as free | -| 27 | `ErrorShutdownIncomplete` | Merged 2026-08-02 by **#4268** (`429667e723`). A quiesce/drain barrier missed its budget. **Took the number #4185 had held since before this file existed** — see the collision history below | +| 27 | `ErrorShutdownIncomplete` | Merged 2026-08-02 by **#4268** (`1e29d2f34c`). A quiesce/drain barrier missed its budget. **Took the number #4185 had held since before this file existed** — see the collision history below | | 29 | `ErrorAssetLockInsufficientFunds` | Merged 2026-08-26 by **#4361** (`699d25704a`) — the typed asset-lock shortfall, landed alongside the coinjoin-drain shielded funding binding. Lineage: fork-era #4184 → #4316 (closed unmerged) → #4361. Complete across all three layers at merge: the Rust discriminant with its raw-value test pin, Swift's `errorAssetLockInsufficientFunds = 29` raw case *and* its `init(ffi:)` arm *and* the typed `PlatformWalletError.assetLockInsufficientFunds` case with its arm in `init(code:message:)` (which `init(result:)` delegates to) and an `ErrorHandlingTests` pin, and Kotlin's typed `PlatformWallet.AssetLockInsufficientFunds` with the `29 ->` arm in `fromPlatformWalletNative` and a `DashSdkErrorTest` pin. **Rule 5 was satisfied only as of `15aa2caea1`, late in review.** Kotlin had mirrored 29 since the branch's binding commit `a711c55eca`; Swift carried none of rule 5's three edits until `15aa2caea1`, so 29 fell to `init(ffi:)`'s `default:` and lost its identity as `.errorUnknown` — the rule-5 failure this file exists to catch, one host typed and the other blind. Its Rust sibling `2eac8a897e` is what lets the code reach either host on the exact-amount funding path, which flattened the variant to `ErrorWalletOperation` (6) in `map_asset_lock_funding_result` before the blanket `From` arm could run | | 31 | `ErrorSigningKeyUnavailable` | Merged 2026-08-04 by **#4183** (merge commit `189a3abb1c`, stacked on #4191). The signer holds no usable private key for a requested public key. Landed complete in that one commit: the Rust C-facing discriminant, Swift's `errorSigningKeyUnavailable = 31` raw case *and* its `init(ffi:)` arm *and* the typed `PlatformWalletError` case with its `init(result:)` arm, and Kotlin's `31 -> PlatformWallet.SigningKeyUnavailable`. Rule 3 now protects it — see the 31-vs-33 note below | | 34 | `ErrorStaleReservationToken` | Merged 2026-08-06 by **#4308** (`438153da39`) — the reservation trio landed with the split build/broadcast surface (successor of fork-era #4185's claim) | @@ -110,14 +110,17 @@ These are shipped ABI. Do not renumber. | 40 | `ErrorContestedNameNotTradable` | Merged 2026-08-09 by **#4348** (`6373e00f0c`) | | 41 | `ErrorShieldedInsufficientBalance` | Merged 2026-08-11 by **#4360** (`e0b8baa850`) | | 42 | `ErrorMasternodeWithdrawalUnconfirmed` | Merged 2026-08-22 by **#4451** (masternode-credit claiming). **Took the number active #4356 had claimed** for `ErrorAssetLockInputConflict` — see the proposed table's 42 note; #4356 renumbers via the frontier | -| 46 | `ErrorMasternodeListUnavailable` | Merged 2026-08-24 by **#4465** (`8dd964277`). Initially minted as 43 (already held by active #4313's `ErrorShieldedInviteAlreadyClaimed` across all three layers) — collision flagged in review and renumbered to the then-frontier same day, Rust and Swift together | +| 46 | `ErrorMasternodeListUnavailable` | Merged 2026-08-24 by **#4465** (`3cf5e665fc`). Initially minted as 43 (already held by active #4313's `ErrorShieldedInviteAlreadyClaimed` across all three layers) — collision flagged in review and renumbered to the then-frontier same day, Rust and Swift together | +| 47 | `ErrorAssetLockInputConflict` | Merged 2026-08-31 by **#4356** (`2cd515b5b6`). **Reserved-with-no-emitter**: the wallet currently constructs only 48 — 47 is the terminal discard-licensing verdict, held for a future finalized-ancestry proof the SPV layer does not yet expose. Renumbered from 42 (taken by merged #4451's `ErrorMasternodeWithdrawalUnconfirmed`) via the frontier; three-layer renumber (Rust value + pin test, Swift raw case, Kotlin arm + test) was complete before merge | +| 48 | `ErrorAssetLockInputContested` | Merged 2026-08-31 by **#4356** (`2cd515b5b6`). The double-spend screen's only currently-emitted verdict — PROVISIONAL, not a discard licence; see the typed rustdoc. Renumbered from 43 (which active #4313 held) via the frontier, alongside 47's move. **This is the code that pushed #3968's `ErrorPersisterTransient` off 48 a second time — see row 50** | | 98 | `NotFound` | Sentinel — `Option` returned as an error | | 99 | `ErrorUnknown` | Sentinel — unmapped/flattened errors | -**Next allocatable integer: 49** — 27–48 are all claimed (27, 29, 31, 34–42 -and 46 merged; 43–45 proposed by active #4313 at head `0302b188ab`; 47 and -48 proposed by active #4356 (47 renumbered from 42, 48 from 43 — see their -rows below); 28, 30, +**Next allocatable integer: 51** — 27–50 are all claimed (27, 29, 31, 34–42, +46, 47 and 48 merged; 43–45 proposed by active #4313 at head `0302b188ab`; +49 proposed by active #3968 (`ErrorPersisterFatal`, unaffected by the +2026-08-31 merge); 50 proposed by active #3968 (`ErrorPersisterTransient`, +renumbered a second time — see its row below); 28, 30, 32 and 33 reserved). **28, 30, 32 and 33 are RESERVED, not free**: 28 and 30 were vacated when the reservation trio moved to 34–36; 32 and 33 lapsed when their in-repo owners @@ -125,10 +128,14 @@ reservation trio moved to 34–36; 32 and 33 lapsed when their in-repo owners unclaimed rather than back-filled, so no number is reused within a single review cycle. Rule 1's "do not reuse a gap unless this file marks it free" applies — this file does **not** mark any of them free, so the frontier is -the only allocation source and a new code takes 49. (42 is a cautionary tale: +the only allocation source and a new code takes 51. (42 is a cautionary tale: merged #4451 minted it while active #4356 held the claim — merged ABI wins, the open PR renumbers. 46's near-miss went the other way: caught in review, -renumbered before merge.) +renumbered before merge. 48 repeats the lesson a third time, on the other +side of the table: #3968 picked it from the frontier on 2026-08-27, and four +days later merged #4356 took it too, for `ErrorAssetLockInputContested` — +a claim recorded on one day is not a claim held forever, and this file's own +`v4.2-dev` merge on 2026-08-31 is what caught it, not a compiler.) ## Proposed allocations (open PRs) @@ -150,14 +157,14 @@ Fork-era numbers remain in the collision history, which is immutable record. | Code | Name | Owning PR | Status | | ---: | --- | --- | --- | | 28 | *(reserved — vacated)* | — | Vacated by #4185/#4256 on 2026-08-02; RESERVED, not reissuable — the next-free frontier is the only allocation source | -| 47 | `ErrorAssetLockInputConflict` | #4356 | Proposed — three-layer renumber from 42 **complete** on the branch (Rust value + pin test, Swift raw case, Kotlin arm + test all at 47). Merged #4451 had taken 42 for `ErrorMasternodeWithdrawalUnconfirmed` on 2026-08-22; merged ABI won and #4356 moved. **Reserved-with-no-emitter**: the wallet currently constructs only the provisional 48 — 47 is the terminal discard-licensing verdict, held for a future finalized-ancestry proof the SPV layer does not yet expose. The number is claimed ABI either way; Rule 1 makes 47 unavailable to any other contributor while #4356 is active | -| 48 | `ErrorAssetLockInputContested` | #4356 | Proposed — renumbered from 43 (which active #4313 holds) alongside 47's move. The provisional double-spend verdict the conflict screen always emits: the sighting BOUNDS the proof wait rather than replacing it, so the lock is still (re-)broadcast and waited on, and 48 is emitted only when that bounded wait expires with the conflict still standing; carries no discard licence. Rust value + Swift raw case + Kotlin typed arm and tests all at 48 on the branch | | 30 | *(reserved — vacated)* | — | Vacated by #4185/#4256 on 2026-08-02; RESERVED, not reissuable — the next-free frontier is the only allocation source | | 32 | *(reserved — lapsed)* | — | Owner #4310 (successor of fork-era #4247) closed without merging; RESERVED, not reissuable | | 33 | *(reserved — lapsed)* | — | Owner #4311 (successor of fork-era #4256) closed without merging; RESERVED, not reissuable | | 43 | `ErrorShieldedInviteAlreadyClaimed` | #4313 | In review — **ACTIVE; the former "on hold — holds no number" row is obsolete.** The branch revived and renumbered to the frontier exactly as that row prescribed. Lineage: fork-era #4204's 32 → 37 move, then 37 **taken by merged #4348** (`ErrorDocumentNotForSale = 37`, ABI since 2026-08-09), then 37 → 43 on revival. `ErrorShieldedInviteAlreadyClaimed = 43` at head `0302b188ab`. **Rule 5 is satisfied at that head**: Swift carries all three edits — the raw case, the `init(ffi:)` arm, and the typed `PlatformWalletError.shieldedInviteAlreadyClaimed` case with its arm in `init(code:message:)` (which `init(result:)` delegates to) — plus `errorDescription`; Kotlin has the typed terminal `PlatformWallet.ShieldedInviteAlreadyClaimed`, the `43 ->` arm in `fromPlatformWalletNative`, and a `DashSdkErrorTest` pin on 43. Swift's 43 mirror predates `0302b188ab` on the branch; the raw-value test pin for 43 is Kotlin's (Swift's `ErrorHandlingTests` pins 44 and 45 only) | | 44 | `ErrorShieldedScanBudgetExhausted` | #4313 | In review — claimed from the frontier; carries the #4306 scan-budget semantics (retryable — progress is checkpointed). **Rule 5 is satisfied as of `0302b188ab`, and was not before it.** At that commit's parent Kotlin already mirrored 44 (typed `ShieldedScanBudgetExhausted`, the `fromPlatformWalletNative` arm, a `DashSdkErrorTest` pin) while Swift carried none of rule 5's three edits, so 44 fell to `init(ffi:)`'s `default:` and lost its identity as `.errorUnknown` — one host typed, the other blind, the same failure shape as merged row 29's. `0302b188ab` adds the raw case, the `init(ffi:)` arm, the typed case with its `init(code:message:)` arm and `errorDescription`, and an `ErrorHandlingTests` pin of raw value 44 | | 45 | `ErrorShieldedLifecycleBusy` | #4313 | In review — claimed from the frontier. A shielded lifecycle operation refused because teardown/clear holds the wallet (retryable — nothing consumed); the FFI remove path passes the refusal through as 45 instead of flattening it to `ErrorWalletOperation` (6). Same rule-5 history as 44: Kotlin mirrored 45 at the parent commit already; Swift's three edits and an `ErrorHandlingTests` pin of raw value 45 landed in `0302b188ab`. **Rule 5 is satisfied at that head** | +| 49 | `ErrorPersisterFatal` | #3968 | Proposed — claimed from the frontier on 2026-08-27, in the same change that took 48 for `ErrorPersisterTransient` (since renumbered again — see 50). Unaffected by the 2026-08-31 `v4.2-dev` merge: 49 was never claimed by any other branch. Replaces the non-conforming 43, which collided silently with active #4313's `ErrorShieldedInviteAlreadyClaimed = 43`; neither tree carried both variants, so only this file showed it | +| 50 | `ErrorPersisterTransient` | #3968 | Proposed — **second renumber.** Claimed 48 from the frontier on 2026-08-27 (replacing the non-conforming 42), then merged #4356 took 48 too, for `ErrorAssetLockInputContested`, on 2026-08-31 — see row 48. Caught reconciling this file against the `v4.2-dev` merge, not by a compiler: neither tree carried both variants before the merge, so the collision was silent, same shape as merged row 29's and proposed row 43's. Moved to 50, the frontier's next free integer after 49 (#3968's own `ErrorPersisterFatal`). Renumbered across all layers together — the Rust discriminant, its `result_code_discriminants_remain_stable` pin, Swift's `errorPersisterTransient` raw case, and its `ErrorHandlingTests` pin. No Kotlin mirror: the persister pair is host-agnostic and falls through to `PlatformWallet.Generic`, which preserves the integer | **Code 31 left this table on 2026-08-04.** `ErrorSigningKeyUnavailable` sat here as #4183's proposal until #4183 merged (`189a3abb1c`); it is now in the merged @@ -239,8 +246,8 @@ that was always required was made — onto the wrong integers. | Code | Name | Owning PR | Conflict | | ---: | --- | --- | --- | -| 42 | `ErrorPersisterTransient` | #3968 | Contradicts **merged ABI** — 42 is #4451's `ErrorMasternodeWithdrawalUnconfirmed` (merged 2026-08-22). Not a paper conflict: since the 2026-08-25 base merges, #3968's **own tree** carries both variants — a hard E0081 in `error.rs` (`= 42` at both variants) and a duplicate raw value 42 in Swift's `PlatformWalletResultCode` — so the branch does not compile as-is | -| 43 | `ErrorPersisterFatal` | #3968 | Collides with **active #4313**, whose recorded claim is `ErrorShieldedInviteAlreadyClaimed = 43` (see its proposed row). The silent shape: nothing conflicts textually and neither tree carries both variants, so only this file shows it | +| 42 | `ErrorPersisterTransient` | #3968 | **RESOLVED 2026-08-27 — withdrawn and reissued as 48; 48 itself was reissued again to 50 on 2026-08-31 — see row 50.** Contradicted **merged ABI**: 42 is #4451's `ErrorMasternodeWithdrawalUnconfirmed` (merged 2026-08-22). Not a paper conflict — after the 2026-08-25 base merges #3968's **own tree** carried both variants, a hard E0081 in `error.rs` (`= 42` at both) plus a duplicate raw value 42 in Swift's `PlatformWalletResultCode`, so the branch did not compile as-is. Cleared when the branch next merged `v4.2-dev`, in the same change that imported this file | +| 43 | `ErrorPersisterFatal` | #3968 | **RESOLVED 2026-08-27 — withdrawn and reissued as 49.** Collided with **active #4313**, whose recorded claim is `ErrorShieldedInviteAlreadyClaimed = 43`. The silent shape: nothing conflicted textually and neither tree carried both variants, so only this file showed it — which is how it was caught | PR `#3954`'s `ErrorShutdownIncomplete = 27` used to sit in this table. It is gone because that claim **won**: #3954 was closed and superseded by **#4268**, @@ -259,8 +266,9 @@ been challenged on day one. Both persister codes must now take fresh integers **from the frontier note above, which is the single canonical source; no number is copied here because any copy goes stale the moment another PR merges** (as the original "46+" copy in this paragraph did when #4465 shipped -46 — the frontier note reads 48 as of 2026-08-26, so a pair claimed today -takes 48 and 49, recording the claim there and here in the same PR). 26 and +46). That is what happened: on 2026-08-27 the pair took **48 and 49** from the +frontier, and the claim is recorded in the proposed table and the frontier +note above. 26 and 27 need nothing: they are the merged base's own values, correctly inherited, and rule 3 keeps them where they are. @@ -440,9 +448,9 @@ it by #4451) and 43 was held in-tree by #4313. So the branch now collides at E0081 plus a duplicate Swift raw value in its own tree, since the 08-25 base merges imported merged 42) and at 43 (silent, against an active proposal). The full account, timeline included, is in **Non-conforming allocations** -above. Both persister codes still owe fresh integers from the frontier note — -the single canonical source; 48 as of 2026-08-26 — and a registry row in the -same PR (rule 2). +above. Both debts were settled on 2026-08-27: the pair took 48 and 49 from the +frontier and filed its rows under rule 2, in the same change that merged +`v4.2-dev` into the branch. ### 26 — RESOLVED: #4196 restacked onto #4185 and is on 34 / 35 / 36 @@ -595,7 +603,7 @@ the same thing in both enums. Compiled 2026-08-01 against `v4.2-dev` at `ed4116b26c`, re-verified 2026-08-02 against `v4.2-dev` at `5d68612a45` (where `ErrorShutdownIncomplete = 27`, -PR #4268 `429667e723`, entered the merged table), re-verified 2026-08-03 against +PR #4268 `1e29d2f34c`, entered the merged table), re-verified 2026-08-03 against that same base, and **re-verified again 2026-08-04 against `v4.2-dev` at `97904ed2fc`** (the head on that date; the branch has advanced substantially since — this whole section is the dated survey, and its merged/frontier diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index 63cb50a152c..2cff1fbed00 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -504,6 +504,20 @@ pub enum PlatformWalletFFIResultCode { /// height, and says the verdict is provisional. ErrorAssetLockInputContested = 48, + // ----------------------------------------------------------------- + // Embedded-persister failures (49-50), claimed from the allocation + // frontier in the error-code registry (#4318). 28, 30, 32 and 33 are + // reserved-not-free, so the frontier is the only allocation source. + // ErrorPersisterFatal was never contested and keeps 49. + // ErrorPersisterTransient was originally minted at 48 on 2026-08-27; + // merged #4356 took 48 for ErrorAssetLockInputContested on 2026-08-31, + // so it moved to 50 — see the registry's row 50 for the full account. + // ----------------------------------------------------------------- + /// A persister operation failed permanently; callers must not retry. + ErrorPersisterFatal = 49, + /// A persister operation failed transiently; callers may retry. + ErrorPersisterTransient = 50, + /// The named thing does not exist. /// /// Originally (and still mostly) the code for every `Option` returned as an @@ -525,6 +539,31 @@ pub enum PlatformWalletFFIResultCode { ErrorUnknown = 99, } +fn persistence_result_code( + error: &platform_wallet::changeset::PersistenceError, +) -> PlatformWalletFFIResultCode { + if error.is_transient() { + PlatformWalletFFIResultCode::ErrorPersisterTransient + } else { + PlatformWalletFFIResultCode::ErrorPersisterFatal + } +} + +fn platform_wallet_persister_result_code( + mut error: &PlatformWalletError, +) -> PlatformWalletFFIResultCode { + loop { + match error { + PlatformWalletError::PersisterLoad(error) + | PlatformWalletError::PersisterStore(error) => { + return persistence_result_code(error); + } + PlatformWalletError::PersisterRestore(inner) => error = inner, + _ => return PlatformWalletFFIResultCode::ErrorPersisterFatal, + } + } +} + /// Must be freed with ['platform_wallet_ffi_result_free'] #[repr(C)] #[derive(Debug)] @@ -815,6 +854,11 @@ impl From for PlatformWalletFFIResult { PlatformWalletError::AssetLockInsufficientFunds { .. } => { PlatformWalletFFIResultCode::ErrorAssetLockInsufficientFunds } + PlatformWalletError::PersisterLoad(..) + | PlatformWalletError::PersisterStore(..) + | PlatformWalletError::PersisterRestore(..) => { + platform_wallet_persister_result_code(&error) + } // A quiesce/drain barrier that did not complete within budget // (clear/reset paths). The host must fail closed: keep its // callback context alive and skip any paired persistence wipe. @@ -1082,10 +1126,8 @@ impl From for PlatformWalletFFIResult { impl From for PlatformWalletFFIResult { fn from(e: platform_wallet::changeset::PersistenceError) -> Self { - Self::err( - PlatformWalletFFIResultCode::ErrorWalletOperation, - format!("persistence error: {e}"), - ) + let code = persistence_result_code(&e); + Self::err(code, format!("persistence error: {e}")) } } @@ -1110,6 +1152,19 @@ impl From for PlatformWalletFFIResult { #[cfg(test)] mod tests { use super::*; + + #[test] + fn result_code_discriminants_remain_stable() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterTransient as i32, + 50 + ); + assert_eq!(PlatformWalletFFIResultCode::ErrorPersisterFatal as i32, 49); + assert_eq!( + PlatformWalletFFIResultCode::ErrorTransactionBroadcastRejected as i32, + 26 + ); + } use key_wallet::account::StandardAccountType; use key_wallet::wallet::managed_wallet_info::transaction_building::AccountTypePreference; @@ -1149,6 +1204,71 @@ mod tests { assert!(!r.message.is_null()); } + fn persistence_error( + kind: platform_wallet::changeset::PersistenceErrorKind, + ) -> platform_wallet::changeset::PersistenceError { + platform_wallet::changeset::PersistenceError::backend_with_kind(kind, "test failure") + } + + #[test] + fn should_map_persistence_errors_by_retry_classification() { + use platform_wallet::changeset::{PersistenceError, PersistenceErrorKind}; + + let transient: PlatformWalletFFIResult = + persistence_error(PersistenceErrorKind::Transient).into(); + assert_eq!( + transient.code, + PlatformWalletFFIResultCode::ErrorPersisterTransient + ); + + for error in [ + persistence_error(PersistenceErrorKind::Fatal), + persistence_error(PersistenceErrorKind::Constraint), + PersistenceError::LockPoisoned, + ] { + let result: PlatformWalletFFIResult = error.into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterFatal + ); + } + } + + #[test] + fn should_map_platform_wallet_persister_errors_by_retry_classification() { + use platform_wallet::changeset::PersistenceErrorKind; + + let cases = [ + ( + PlatformWalletError::PersisterLoad(persistence_error( + PersistenceErrorKind::Transient, + )), + PlatformWalletFFIResultCode::ErrorPersisterTransient, + ), + ( + PlatformWalletError::PersisterStore(persistence_error(PersistenceErrorKind::Fatal)), + PlatformWalletFFIResultCode::ErrorPersisterFatal, + ), + ( + PlatformWalletError::PersisterRestore(Box::new( + PlatformWalletError::PersisterStore(persistence_error( + PersistenceErrorKind::Transient, + )), + )), + PlatformWalletFFIResultCode::ErrorPersisterTransient, + ), + ( + PlatformWalletError::PersisterRestore(Box::new(PlatformWalletError::WalletLocked)), + PlatformWalletFFIResultCode::ErrorPersisterFatal, + ), + ]; + + for (error, expected) in cases { + let result: PlatformWalletFFIResult = error.into(); + assert_eq!(result.code, expected); + } + } + /// The three "can't-select-inputs" wallet variants (`NoSpendableInputs`, /// `OnlyOutputAddressesFunded`, `OnlyDustInputs`) all map to the dedicated /// `ErrorNoSelectableInputs` FFI code rather than flattening to diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift index 5b07dcbda8c..e3fc425633e 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift @@ -216,6 +216,16 @@ public enum PlatformWalletResultCode: Int32, Sendable { /// the Rust-side scan cannot see conflicts whose spender was already /// pruned. case errorAssetLockInputContested = 48 + // Persister failure classes (49-50) — claimed from the error-code + // registry's allocation frontier; these raw values are hand-mirrored ABI. + // errorPersisterFatal was never contested and keeps 49. + // errorPersisterTransient was originally 48; merged #4356 took 48 for + // errorAssetLockInputContested on 2026-08-31, so it moved to 50 — see + // the registry's row 50. + /// A persister operation failed permanently; callers must not retry. + case errorPersisterFatal = 49 + /// A persister operation failed transiently; callers may retry. + case errorPersisterTransient = 50 /// The named thing does not exist. Besides the handle/lookup failures this /// has always covered, BOTH deferred-send paths report the /// wallet-was-REMOVED case here. @@ -297,6 +307,10 @@ public enum PlatformWalletResultCode: Int32, Sendable { self = .errorAssetLockFundingMismatch case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_ASSET_LOCK_INSUFFICIENT_FUNDS: self = .errorAssetLockInsufficientFunds + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_TRANSIENT: + self = .errorPersisterTransient + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_FATAL: + self = .errorPersisterFatal case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_BROADCAST_REJECTED: self = .errorTransactionBroadcastRejected case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHUTDOWN_INCOMPLETE: @@ -416,6 +430,10 @@ public enum PlatformWalletError: LocalizedError { /// The `available` / `required` duff figures are in the message. Kotlin /// parity: `DashSdkError.PlatformWallet.AssetLockInsufficientFunds`. case assetLockInsufficientFunds(String) + /// A persister operation failed transiently; callers may retry. + case persisterTransient(String) + /// A persister operation failed permanently; callers must not retry. + case persisterFatal(String) case walletAlreadyExists(String) /// Definitive shielded-broadcast failure: the shielded transition /// (identity-create or a spend — unshield / transfer / withdrawal) was @@ -577,6 +595,7 @@ public enum PlatformWalletError: LocalizedError { .coreInsufficientFunds(let m), .assetLockNotTracked(let m), .assetLockAlreadyConsumed(let m), .assetLockFundingMismatch(let m), .assetLockInsufficientFunds(let m), + .persisterTransient(let m), .persisterFatal(let m), .walletAlreadyExists(let m), .shieldedBroadcastFailed(let m), .shieldedBroadcastUnconfirmed(let m), .shieldedSpendUnconfirmed(let m), .shieldedNoRecordedAnchor(let m), .shieldedInsufficientBalance(let m), @@ -646,6 +665,8 @@ public enum PlatformWalletError: LocalizedError { case .errorAssetLockAlreadyConsumed: self = .assetLockAlreadyConsumed(detail) case .errorAssetLockFundingMismatch: self = .assetLockFundingMismatch(detail) case .errorAssetLockInsufficientFunds: self = .assetLockInsufficientFunds(detail) + case .errorPersisterTransient: self = .persisterTransient(detail) + case .errorPersisterFatal: self = .persisterFatal(detail) case .errorWalletAlreadyExists: self = .walletAlreadyExists(detail) case .errorShieldedBroadcastFailed: self = .shieldedBroadcastFailed(detail) case .errorShieldedBroadcastUnconfirmed: self = .shieldedBroadcastUnconfirmed(detail) diff --git a/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift b/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift index 9b375b852d4..6f8108550b5 100644 --- a/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift +++ b/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift @@ -33,6 +33,43 @@ final class ErrorHandlingTests: XCTestCase { ) } + func testPersisterFFIResultMappingsRemainDistinguishable() { + XCTAssertEqual(PlatformWalletResultCode.errorPersisterTransient.rawValue, 50) + XCTAssertEqual(PlatformWalletResultCode.errorPersisterFatal.rawValue, 49) + XCTAssertEqual( + PlatformWalletResultCode( + ffi: PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_TRANSIENT + ), + .errorPersisterTransient + ) + XCTAssertEqual( + PlatformWalletResultCode( + ffi: PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_FATAL + ), + .errorPersisterFatal + ) + + let transientResult = PlatformWalletResult( + PlatformWalletFFIResult( + code: PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_TRANSIENT, + message: nil + ) + ) + let fatalResult = PlatformWalletResult( + PlatformWalletFFIResult( + code: PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_FATAL, + message: nil + ) + ) + + guard case .persisterTransient = PlatformWalletError(result: transientResult) else { + return XCTFail("transient persister code must map to persisterTransient") + } + guard case .persisterFatal = PlatformWalletError(result: fatalResult) else { + return XCTFail("fatal persister code must map to persisterFatal") + } + } + func testAssetLockInsufficientFundsFFIResultMapping() { // The asset-lock coin-selection shortfall (dashpay/platform#4073). // Swift could not decode code 29 at all before this mirror existed — @@ -529,6 +566,13 @@ final class ErrorHandlingTests: XCTestCase { // MARK: - Core broadcast outcome mapping func testCoreBroadcastOutcomeMapping() throws { + XCTAssertEqual(PlatformWalletResultCode.errorTransactionBroadcastRejected.rawValue, 26) + XCTAssertEqual( + PlatformWalletResultCode( + ffi: PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_TRANSACTION_BROADCAST_REJECTED + ), + .errorTransactionBroadcastRejected + ) XCTAssertEqual( try CoreTransactionBroadcastOutcome( resultCode: .success, From 5adbe629297bb62191201fea1929ccaf85c8235a Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:39:29 +0000 Subject: [PATCH 3/5] fix(platform-wallet): invalidate filter-scan generation on contact accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Contact-account registration went through `accounts.insert_funds_bearing_account`, which does not bump the wallet's `account_generation`. The running filter scan therefore never picked up the new account's addresses — a contact's incoming payments stayed invisible until something else happened to invalidate the scan. Registration now goes through `ManagedAccountOperations::add_managed_account`, and `PlatformWalletInfo` forwards `account_generation()` to the core wallet so the invalidation is observable. Tests assert generation `1` after registering both a contact and an external account. `reconcile_dashpay_rescan` no longer bails on `synced_height == 0`. A zero checkpoint already means "scan from genesis", but bailing left candidates unmarked, so once that scan advanced the very same contacts triggered a redundant funding-height rewind. Candidates are now marked as covered and the height is left alone. 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent --- .../src/wallet/identity/network/contacts.rs | 22 ++++--- .../src/wallet/identity/network/payments.rs | 59 +++++++++++++++---- .../src/wallet/platform_wallet_traits.rs | 4 ++ 3 files changed, 62 insertions(+), 23 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs b/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs index 821bfe9e4d9..fb9a141dbc2 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs @@ -5,6 +5,7 @@ use dpp::identity::Identity; use dpp::prelude::Identifier; use key_wallet::account::AccountType; use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; +use key_wallet::wallet::managed_wallet_info::managed_account_operations::ManagedAccountOperations; use super::*; use crate::broadcaster::TransactionBroadcaster; @@ -203,9 +204,9 @@ impl DashPayView<'_, B> { is_watch_only: false, }; - // DashPay accounts are funds-bearing; use the typed - // `insert_funds_bearing_account` API exposed by the post-split - // collection rather than wrapping in `OwnedManagedCoreAccount`. + // Build the initial funds-bearing state for persistence. The live + // insertion below goes through `ManagedAccountOperations` so upstream + // also invalidates the wallet's prior filter-scan generation. let managed = key_wallet::managed_account::ManagedCoreFundsAccount::from_account(&account); // Persist the registration BEFORE the in-memory inserts: a store @@ -239,9 +240,7 @@ impl DashPayView<'_, B> { "Failed to add contact account to wallet: {e}" )) })?; - info.core_wallet - .accounts - .insert_funds_bearing_account(managed) + info.add_managed_account(wallet, account_type) .map_err(|e| { PlatformWalletError::InvalidIdentityData(format!( "Failed to register contact account: {e}" @@ -535,8 +534,9 @@ impl DashPayView<'_, B> { is_watch_only: true, }; - // DashpayExternalAccount is funds-bearing; insert via the - // typed `insert_funds` API after the upstream split. + // Build the initial funds-bearing state for persistence. The live + // insertion below goes through `ManagedAccountOperations` so upstream + // also invalidates the wallet's prior filter-scan generation. let managed = key_wallet::managed_account::ManagedCoreFundsAccount::from_account(&account); // Persist the registration BEFORE the in-memory inserts (same @@ -575,10 +575,8 @@ impl DashPayView<'_, B> { ))) })?; - // (b) Insert ManagedCoreFundsAccount for address-pool tracking. - info.core_wallet - .accounts - .insert_funds_bearing_account(managed) + // (b) Insert the managed account and invalidate prior filter coverage. + info.add_managed_account(wallet, account_type) .map_err(|e| { Transient(PlatformWalletError::InvalidIdentityData(format!( "Failed to register external contact account: {}", 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 659a430ffe3..5637ba11f00 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -108,12 +108,10 @@ impl DashPayView<'_, B> { return Ok(None); }; + // A zero checkpoint already requests a scan from genesis; candidates are + // still processed so they are marked as covered and do not trigger a + // redundant funding-height rewind once that scan advances. let synced_height = info.core_wallet.synced_height(); - // 0 means "scan from genesis / not yet started" — already a full - // historical scan, nothing to backfill toward. - if synced_height == 0 { - return Ok(None); - } // (owner, contact) pairs that have a receival account — we can only // watch a contact's incoming addresses once its receival account exists. @@ -1652,6 +1650,7 @@ mod tests { use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; use key_wallet::mnemonic::{Language, Mnemonic}; use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use key_wallet::Network; use crate::changeset::{ @@ -2301,6 +2300,18 @@ mod tests { .expect("register_contact_account"); } + { + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + let wm = wallet.identity().wallet_manager.read().await; + assert_eq!( + wm.get_wallet_info(&wallet_id) + .expect("info") + .account_generation(), + 1, + "registering a contact account must invalidate the prior filter-scan generation" + ); + } + { let stores = persister.stores.lock().unwrap(); let registered = stores.iter().any(|(_, cs)| { @@ -2911,8 +2922,16 @@ mod tests { "all candidates marked -> no re-trigger" ); - // A newly discovered, older-funded contact re-lowers exactly once... + // Adding another account now invalidates upstream filter coverage and + // rewinds directly to the wallet birth floor. Reconcile recognizes that + // this full-history scan already covers the contact and marks it without + // a second, shallower rewind. establish_receival_contact(&manager, &persister, wallet_id, owner, c_c, 50, 50).await; + assert_eq!( + synced_height(&manager, wallet_id).await, + 0, + "new account insertion rewinds filter coverage to the wallet birth floor" + ); assert_eq!( iw_wallet .identity() @@ -2920,10 +2939,10 @@ mod tests { .reconcile_dashpay_rescan() .await .expect("rescan 3"), - Some(50), - "a new older contact re-lowers to its funding height" + None, + "the already-scheduled full-history scan needs no second rewind" ); - // ...then settles. + // The contact was marked while the checkpoint was zero, so it settles. assert_eq!( iw_wallet .identity() @@ -2937,8 +2956,8 @@ mod tests { } /// `synced_height == 0` means "scan from genesis / not started" — already a - /// full historical scan, so the rescan is a no-op (the masking path the spec - /// warns about). + /// full historical scan. Reconcile leaves the height alone but marks the + /// contact so advancing that scan does not cause a redundant rewind. #[tokio::test] async fn rescan_is_a_noop_when_synced_height_is_zero() { let (manager, persister, wallet_id) = make_wallet().await; @@ -2960,6 +2979,19 @@ mod tests { "synced_height 0 -> no rescan" ); assert_eq!(synced_height(&manager, wallet_id).await, 0); + + set_synced_height(&manager, wallet_id, 200).await; + assert_eq!( + iw_wallet + .identity() + .dashpay() + .reconcile_dashpay_rescan() + .await + .expect("rescan after forward progress"), + None, + "genesis-covered contact must stay settled after the scan advances" + ); + assert_eq!(synced_height(&manager, wallet_id).await, 200); } /// A `Sent` payment must advance `Pending → Confirmed` once its @@ -4853,6 +4885,11 @@ mod tests { let wm = iw.wallet_manager.read().await; let info = wm.get_wallet_info(&wallet_id).expect("info"); + assert_eq!( + info.account_generation(), + 1, + "registering an external account must invalidate the prior filter-scan generation" + ); use key_wallet::account::account_collection::DashpayAccountKey; let key = DashpayAccountKey { index: 0, diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs index 49ed828d228..86f707d6901 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs @@ -149,6 +149,10 @@ impl WalletInfoInterface for PlatformWalletInfo { self.core_wallet.synced_height() } + fn account_generation(&self) -> u64 { + self.core_wallet.account_generation() + } + fn update_last_processed_height(&mut self, current_height: u32) { self.core_wallet .update_last_processed_height(current_height); From 881dbd0bc4160193ff96a88714a2b1e0247c40ef Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:40:11 +0000 Subject: [PATCH 4/5] fix(platform-wallet): drop the local SPV timeout, fix Swift funding index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `broadcaster.rs` deleted its hardcoded 30 s `SPV_ACCEPTANCE_TIMEOUT` and now passes `None` to `broadcast_and_wait`. The local override was shorter than dash-spv's own budget, so sends were reported `MaybeSent`/uncertain while the SPV layer was still legitimately waiting. `SendTransactionView` non-platform -> platform flows use `senderAccountIndex = 0` instead of "first key-class-0 account with a positive balance": that search returned a key-class Platform-Payment account index, which was then fed to `CoreTransactionBuilder.setFunding(accountType: .bip44, ...)` — a different namespace, so core -> core sends could draw on the wrong funding account. Also in this batch: `run_tests.sh` tolerates a CI runner with no user default keychain under `set -euo pipefail`; `now_secs()` moves to `util.rs` as `pub(crate)`; the shield-input-selection regression test re-seeds off `reserve()` rather than hardcoded balances so it survives fee-schedule changes; and doc comments are corrected (`Wallet::new_watch_only` -> `new_external_signable`, `derive_spent_utxos` defaults, restore-loop skip behaviour). 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent --- .../rs-platform-wallet/src/broadcaster.rs | 19 ++----- .../src/changeset/core_bridge.rs | 7 +-- .../src/manager/wallet_lifecycle.rs | 2 +- packages/rs-platform-wallet/src/util.rs | 10 ++++ .../identity/network/contact_requests.rs | 5 +- .../src/wallet/platform_wallet.rs | 25 +++++---- .../KeyWallet/WalletManager.swift | 3 ++ .../PlatformWalletManager.swift | 4 ++ .../PlatformWalletPersistenceHandler.swift | 2 +- .../Core/Views/SendTransactionView.swift | 51 ++++++++++--------- packages/swift-sdk/run_tests.sh | 10 +++- 11 files changed, 78 insertions(+), 60 deletions(-) diff --git a/packages/rs-platform-wallet/src/broadcaster.rs b/packages/rs-platform-wallet/src/broadcaster.rs index 633af12ae25..c9e5f4cd593 100644 --- a/packages/rs-platform-wallet/src/broadcaster.rs +++ b/packages/rs-platform-wallet/src/broadcaster.rs @@ -124,13 +124,6 @@ impl TransactionBroadcaster for DapiBroadcaster { } } -/// How long the SPV broadcast waits for a network-acceptance verdict before -/// reporting the outcome as unknown. Shorter than dash-spv's own default so a -/// user-facing send does not hang for a full minute. On live Dash networks -/// acceptance usually resolves in seconds via the InstantSend lock or the -/// withheld-peer echo, well inside this bound. -const SPV_ACCEPTANCE_TIMEOUT: Duration = Duration::from_secs(30); - /// The SPV broadcast channel: send through P2P peers and await dash-spv's /// network-acceptance verdict (rust-dashcore#913). #[async_trait] @@ -188,11 +181,7 @@ impl SpvBroadcaster { impl TransactionBroadcaster for SpvBroadcaster { async fn broadcast(&self, transaction: &Transaction) -> Result { let txid = transaction.txid(); - match self - .spv - .broadcast_and_wait(transaction, Some(SPV_ACCEPTANCE_TIMEOUT)) - .await - { + match self.spv.broadcast_and_wait(transaction, None).await { Ok(BroadcastResult::Accepted { relayed_by }) => { tracing::info!( txid = %txid, @@ -208,9 +197,9 @@ impl TransactionBroadcaster for SpvBroadcaster { // later echo/IS-lock/confirmation or the reservation-TTL // backstop reconciles the reservation. Ok(BroadcastResult::Uncertain) => Err(BroadcastError::MaybeSent { - reason: format!( - "SPV broadcast saw no acceptance signal within {SPV_ACCEPTANCE_TIMEOUT:?}" - ), + reason: + "SPV broadcast saw no acceptance signal before dash-spv's acceptance timeout" + .to_string(), }), // Provably never sent (per the SpvChannel error contract): no // bytes reached the network, so the reservation is safe to diff --git a/packages/rs-platform-wallet/src/changeset/core_bridge.rs b/packages/rs-platform-wallet/src/changeset/core_bridge.rs index f9b7f491977..f3ddc86f484 100644 --- a/packages/rs-platform-wallet/src/changeset/core_bridge.rs +++ b/packages/rs-platform-wallet/src/changeset/core_bridge.rs @@ -1336,9 +1336,10 @@ fn derive_new_utxos(record: &TransactionRecord) -> Vec { /// the script and the address as independent parameters and validates /// neither. /// -/// Height and the confirmation flags describe the *previous* transaction and -/// aren't carried in `InputDetail`, so they remain defaulted on this synthetic -/// spent record (height 0, all flags false). +/// Height and the confirmation flags describe the *previous* output and +/// aren't carried in `InputDetail`, so they default (height 0, flags +/// false); `core_utxos` has no column for either, so those defaults never +/// become durable state. fn derive_spent_utxos(record: &TransactionRecord) -> Vec { record .input_details diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index 4cc2dce78da..6059cc5df04 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -279,7 +279,7 @@ impl PlatformWalletManager

{ // Snapshot per-account xpubs and address-pool entries BEFORE // the wallet / managed-info are moved into insert_wallet. The // persister sees everything needed to rebuild the wallet - // watch-only (via `Wallet::new_watch_only`) plus populate + // external-signable (via `Wallet::new_external_signable`) plus populate // SwiftData's address table on next launch. let account_specs: Vec<( key_wallet::account::AccountType, diff --git a/packages/rs-platform-wallet/src/util.rs b/packages/rs-platform-wallet/src/util.rs index 32dfde95325..8cce108d660 100644 --- a/packages/rs-platform-wallet/src/util.rs +++ b/packages/rs-platform-wallet/src/util.rs @@ -11,3 +11,13 @@ pub(crate) fn now_ms() -> u64 { .map(|d| d.as_millis() as u64) .unwrap_or(0) } + +/// Current wall-clock time in seconds since the Unix epoch. +/// +/// Pre-epoch reads return `0`, which upstream never expires. +pub(crate) fn now_secs() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0) +} 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 a9a6fa64e6c..ba3f82542a4 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 @@ -2882,10 +2882,7 @@ impl DashPayView<'_, B> { return 0; } - let now_secs = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map(|d| d.as_secs()) - .unwrap_or(0); + let now_secs = crate::util::now_secs(); let mut cleared: Vec = Vec::new(); // Permanent verify failures to mark so the sync sweep's enqueue gate diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs index c1ae06ba180..2b316334032 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs @@ -2303,23 +2303,24 @@ mod shield_input_selection_tests { #[test] fn regression_reports_max_from_usable_suffix_not_total_account_balance() { - // Real account snapshot: the leading address is below the reserve, so + // Account snapshot whose leading address cannot pay the fee, so // capacity must come from the usable suffix, not the account total. - assert!( - 297_264_780 <= reserve(), - "regression shape requires the leading address to stay below the reserve; \ - re-seed the balances if the versioned reserve drops under 297_264_780" - ); + // The leading balance is derived from the reserve — one credit below + // the strict `> reserve` viability threshold, the largest balance that + // must still be rejected as input 0 — so the shape holds whatever the + // versioned fee schedule does next. + let dust = reserve() - 1; + let usable = 3_623_849_220; let candidates = vec![ - (addr(1), 297_264_780), + (addr(1), dust), (addr(2), 2_000_000_000), (addr(3), 1_623_849_220), ]; let plan = plan(candidates).unwrap(); - let expected_max = 3_623_849_220 - reserve(); + let expected_max = usable - reserve(); - assert_eq!(plan.preflight.account_balance_credits, 3_921_114_000); - assert_eq!(plan.preflight.usable_balance_credits, 3_623_849_220); + assert_eq!(plan.preflight.account_balance_credits, dust + usable); + assert_eq!(plan.preflight.usable_balance_credits, usable); assert_eq!(plan.preflight.fee_reserve_credits, reserve()); assert_eq!(plan.preflight.max_shieldable_credits, expected_max); assert!(plan.preflight.can_shield); @@ -2329,11 +2330,13 @@ mod shield_input_selection_tests { assert!(!chosen.contains_key(&addr(1))); assert_eq!(chosen.values().sum::(), expected_max); + // `available` reports the usable suffix, never the account total — + // the whole point of the regression. let err = plan.select_inputs(expected_max + 1).unwrap_err(); assert!(matches!( err, PlatformWalletError::PlatformShieldCapacityExceeded { available, required } - if available == 3_623_849_220 && required == 3_623_849_221 + if available == usable && required == usable + 1 )); } diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift b/packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift index 540cf0adb8c..9363dbb02d4 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/WalletManager.swift @@ -651,6 +651,9 @@ public class WalletManager { /// Import a wallet from serialized bytes /// - Parameters: /// - walletBytes: The serialized wallet data + /// - birthHeight: Block height to start scanning from. Defaults to 0 + /// (genesis), a safe full rescan; pass the wallet's known birth + /// height to skip pre-birth blocks. /// - Returns: The wallet ID of the imported wallet public func importWallet(from walletBytes: Data, birthHeight: UInt32 = 0) throws -> Data { guard !walletBytes.isEmpty else { diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift index 8b7025e72b3..b4cfd5dbb4f 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift @@ -1329,6 +1329,10 @@ public class PlatformWalletManager: ObservableObject { for walletId in walletIds { guard walletId.count == 32 else { continue } + // A wallet Rust declined to register (corrupt/skipped row) is + // still listed by SwiftData; `get_wallet` returns NotFound for + // it, which the do/catch below logs to `lastError` and skips — + // one bad row never fails the whole restore. var walletHandle: Handle = NULL_HANDLE do { try walletId.withUnsafeBytes { idPtr in diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift index 01c5cfd5867..6785ed3fd1f 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift @@ -4988,7 +4988,7 @@ public final class PlatformWalletPersistenceHandler: @unchecked Sendable { /// A wallet is "restorable" when it has at least one /// `PersistentAccount` row with non-empty /// `accountExtendedPubKeyBytes`. The Rust side reconstructs the - /// watch-only `Wallet` via `Wallet::new_watch_only(network, + /// external-signable `Wallet` via `Wallet::new_external_signable(network, /// wallet_id, accounts)`; accounts come directly from the spec /// array, wallet id from the top-level struct. /// diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift index de48fdf33bb..b5f49500180 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift @@ -265,26 +265,34 @@ struct SendTransactionView: View { // be the one that was last created. let managed = walletManager.wallet(for: wallet.walletId) let platformAddressWallet = try? managed?.platformAddressWallet() - // Pick the account that will FUND a platform → - // platform transfer. The Rust Auto selector - // resolves the source via - // `platform_payment_managed_account_at_index` - // (key class 0) and selects its inputs WITHIN - // that single account — it does not span - // accounts. `canSend` only gates on the - // aggregate platform balance, so with multiple - // key-class-0 Platform Payment accounts we must - // choose an account whose OWN balance covers the - // requested amount + fee; otherwise we'd enable a - // send Rust rejects. The selection is factored - // into the pure, unit-tested - // `PlatformPaymentAccountSelection` helper. + // Resolve the account that FUNDS the send. Two + // consumers read `senderAccountIndex`, in two + // DISTINCT account namespaces: // - // Only the platform → platform path needs this - // coverage-aware pick; every other flow ignores - // `senderAccountIndex`, so the prior - // "first key-class-0 positive balance, else 0" - // behaviour is preserved for them. + // • platform → platform: a key-class-0 Platform + // Payment account. The Rust Auto selector + // resolves the source via + // `platform_payment_managed_account_at_index` + // and selects inputs WITHIN that single account + // (it does not span accounts). `canSend` gates + // only on the aggregate platform balance, so we + // must pick an account whose OWN balance covers + // amount + fee, else Rust rejects the send — + // done by the unit-tested + // `PlatformPaymentAccountSelection` helper. + // + // • core → core: a BIP44 Core account index, fed + // into `CoreTransactionBuilder.setFunding( + // accountType: .bip44, ...)`. That namespace is + // SEPARATE from key-class Platform Payment + // accounts — a Platform-Payment index must never + // leak into it. The Core send UI has no account + // picker and funds the default BIP44 account, so + // resolve to account 0. + // + // Every other flow (shielded / platform → shielded + // / core → shielded) ignores this value and + // resolves its own funding, so 0 is harmless there. let senderAccountIndex: UInt32 if viewModel.detectedFlow == .platformToPlatform { guard let resolved = resolvePlatformSenderAccountIndex() else { @@ -293,10 +301,7 @@ struct SendTransactionView: View { } senderAccountIndex = resolved } else { - senderAccountIndex = addressBalances - .filter { $0.account?.keyClass == 0 } - .first(where: { $0.balance > 0 })? - .accountIndex ?? 0 + senderAccountIndex = 0 } // Input selection and surplus handling are owned // by the Rust Auto path (surplus stays on the diff --git a/packages/swift-sdk/run_tests.sh b/packages/swift-sdk/run_tests.sh index 47ca4b095d3..19e83a461e4 100755 --- a/packages/swift-sdk/run_tests.sh +++ b/packages/swift-sdk/run_tests.sh @@ -23,7 +23,10 @@ cd "$SCRIPT_DIR" || exit 1 # touches a developer's keychain configuration; the previous default and # search list are restored on exit. if [ -n "${CI:-}${GITHUB_ACTIONS:-}" ]; then - PREV_DEFAULT_KEYCHAIN="$(security default-keychain -d user | sed -E 's/^[[:space:]]*"?//;s/"?[[:space:]]*$//')" + # `security default-keychain -d user` exits non-zero on a runner with no + # user default keychain; tolerate it (the restore below skips an empty + # value) so `set -euo pipefail` doesn't abort the run before any build. + PREV_DEFAULT_KEYCHAIN="$(security default-keychain -d user 2>/dev/null | sed -E 's/^[[:space:]]*"?//;s/"?[[:space:]]*$//' || true)" PREV_USER_KEYCHAINS_OUTPUT="$(security list-keychains -d user)" PREV_USER_KEYCHAINS=() while IFS= read -r keychain_path; do @@ -48,7 +51,10 @@ if [ -n "${CI:-}${GITHUB_ACTIONS:-}" ]; then cleanup_status=0 trap - EXIT - if [ "${CI_DEFAULT_MAY_HAVE_CHANGED:-0}" -eq 1 ]; then + # An empty PREV_DEFAULT_KEYCHAIN means the runner had no user default to + # begin with, so there is nothing to restore and `security -s ""` would + # only fail the cleanup. + if [ "${CI_DEFAULT_MAY_HAVE_CHANGED:-0}" -eq 1 ] && [ -n "${PREV_DEFAULT_KEYCHAIN:-}" ]; then if ! security default-keychain -d user -s "$PREV_DEFAULT_KEYCHAIN"; then cleanup_status=1 fi From 40e04c29a5bb6b9291c3001a8105679e5eec1466 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:40:34 +0000 Subject: [PATCH 5/5] refactor(platform-wallet-ffi): reuse the shared provider-account rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The FFI restore path carried its own copy of the provider-key account reconstruction, which had already drifted from the SQLite backend's. Both now call `platform_wallet::changeset::rebuild_provider_key_account`, so FFI and SQLite restore provider accounts identically (-54/+30). Ordering: `rebuild_provider_key_account` ships with the wallet-storage PR's required bucket (`changeset/changeset.rs` + `changeset/mod.rs`). Until that lands on the base branch this commit does not compile — the sole error is the unresolved import. Land the storage PR first. 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent --- .../rs-platform-wallet-ffi/src/persistence.rs | 84 +++++++------------ 1 file changed, 30 insertions(+), 54 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/persistence.rs b/packages/rs-platform-wallet-ffi/src/persistence.rs index 3b540ffbf8d..fea5fb5d25a 100644 --- a/packages/rs-platform-wallet-ffi/src/persistence.rs +++ b/packages/rs-platform-wallet-ffi/src/persistence.rs @@ -7,7 +7,7 @@ use bincode::config; use key_wallet::account::account_collection::AccountCollection; -use key_wallet::account::{Account, AccountType, BLSAccount, EdDSAAccount, StandardAccountType}; +use key_wallet::account::{Account, AccountType, StandardAccountType}; use key_wallet::bip32::DerivationPath; use key_wallet::bip32::ExtendedPubKey; use key_wallet::derivation_bls_bip32::ExtendedBLSPubKey; @@ -24,10 +24,10 @@ use std::str::FromStr; use crate::types::{FFINetwork, Network}; use platform_wallet::changeset::{ - AccountAddressPoolEntry, AccountRegistrationEntry, ClientStartState, ClientWalletStartState, - ListedCoreTxid, Merge, PersistenceCapabilities, PersistenceError, PlatformWalletChangeSet, - PlatformWalletPersistence, ProviderKeyAccountEntry, ProviderKeyExtendedPubKey, - PERSISTENCE_CAPABILITIES_VERSION, + rebuild_provider_key_account, AccountAddressPoolEntry, AccountRegistrationEntry, + ClientStartState, ClientWalletStartState, ListedCoreTxid, Merge, PersistenceCapabilities, + PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, ProviderKeyAccountEntry, + ProviderKeyExtendedPubKey, PERSISTENCE_CAPABILITIES_VERSION, }; use platform_wallet::wallet::platform_wallet::WalletId; use platform_wallet::wallet::{PerAccountPlatformAddressState, PerWalletPlatformAddressState}; @@ -4499,20 +4499,18 @@ fn build_wallet_start_state( unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) }; // Provider key-material accounts (BLS operator keys / EdDSA - // platform node keys) live in dedicated `Option` fields on the - // collection and carry a non-secp256k1 extended public key in - // the same `account_xpub_bytes` slot. Rebuild them watch-only - // via the type-specific `new` + insert methods rather than the - // ECDSA `Account::from_xpub` / `insert` path (which would fail - // to decode the bytes and reject the provider `AccountType`). - // Provider xpubs are stored raw (`bincode(xpub)`), exactly like the - // ECDSA accounts. The derivation scheme is NOT versioned here: this - // app is pre-release and the pre-#879 (secp256k1-hybrid) derivation - // never shipped to production. A wallet whose provider accounts were + // platform node keys) carry a non-secp256k1 extended public key in + // the same `account_xpub_bytes` slot; `account_type` discriminates + // the decode. The rebuild itself is the shared helper every backend's + // restore path uses (the SQLite backend calls it too). Provider + // xpubs are stored raw (`bincode(xpub)`), exactly like the ECDSA + // accounts. The derivation scheme is NOT versioned here: this app is + // pre-release and the pre-#879 (secp256k1-hybrid) derivation never + // shipped to production. A wallet whose provider accounts were // persisted by a pre-#879 dev build will restore those (stale) xpubs // and show stale operator / platform-node keys until it's deleted // and re-imported — an accepted, transient dev-only state. - match account_type { + let provider_key = match account_type { AccountType::ProviderOperatorKeys => { let (bls_pubkey, _): (ExtendedBLSPubKey, usize) = bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| { @@ -4521,22 +4519,7 @@ fn build_wallet_start_state( e )) })?; - let bls_account = BLSAccount::new( - Some(entry.wallet_id.to_vec()), - account_type, - bls_pubkey, - network, - ) - .map_err(|e| { - PersistenceError::backend(format!("BLSAccount::new failed: {:?}", e)) - })?; - accounts.insert_bls_account(bls_account).map_err(|e| { - PersistenceError::backend(format!( - "AccountCollection::insert_bls_account failed: {}", - e - )) - })?; - continue; + Some(ProviderKeyExtendedPubKey::Bls(bls_pubkey)) } AccountType::ProviderPlatformKeys => { let (ed_pubkey, _): (ExtendedEd25519PubKey, usize) = @@ -4546,29 +4529,22 @@ fn build_wallet_start_state( e )) })?; - let eddsa_account = EdDSAAccount::new( - Some(entry.wallet_id.to_vec()), - account_type, - ed_pubkey, - network, - ) - .map_err(|e| { - PersistenceError::backend(format!("EdDSAAccount::new failed: {:?}", e)) - })?; - accounts.insert_eddsa_account(eddsa_account).map_err(|e| { - PersistenceError::backend(format!( - "AccountCollection::insert_eddsa_account failed: {}", - e - )) - })?; - // The platform-node (Ed25519) pool is rehydrated from the - // persisted core-address rows like every other pool — see - // `restore_core_address_pools`. Those rows now carry the - // typed EdDSA key + `KeyTypeTagFFI::EdDSA`, so no dedicated - // batch side-channel is needed here. - continue; + Some(ProviderKeyExtendedPubKey::EdDSA(ed_pubkey)) } - _ => {} + _ => None, + }; + if let Some(key) = provider_key { + rebuild_provider_key_account( + &mut accounts, + entry.wallet_id, + network, + account_type, + &key, + ) + .map_err(|e| { + PersistenceError::backend(format!("provider key account rebuild failed: {e}")) + })?; + continue; } let (account_xpub, _): (ExtendedPubKey, usize) =