diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt index 4bcb1ae8ea1..fb3ac974954 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt @@ -3,6 +3,17 @@ package org.dashfoundation.dashsdk.errors import org.dashfoundation.dashsdk.ffi.DashSDKException import org.json.JSONObject +// Display text for the persister failures, whose native message is a nested +// Rust error chain (operation, backend classification, the store's own +// phrasing) that no user can act on. One string per outcome a person can +// distinguish; a failed read and a failed write must not describe each other. +private const val PERSISTER_BUSY_USER_MESSAGE = + "The wallet database is busy. Try again in a moment." +private const val PERSISTER_UNREADABLE_USER_MESSAGE = + "The wallet data could not be read and may need to be restored." +private const val PERSISTER_UNSAVED_USER_MESSAGE = + "The wallet data could not be saved and may need to be restored." + /** * Public error hierarchy of the Kotlin SDK — the Android analog of the * Swift SDK's `UserFacingError`/`SDKError` split, keyed off the native @@ -19,6 +30,14 @@ sealed class DashSdkError( /** Whether retrying the same operation can plausibly succeed. */ open val isRetryable: Boolean get() = false + /** + * Text fit to show a person. Defaults to [message] — most native + * messages read as a sentence — but types whose message is a nested + * error chain override it, so a UI can display this unconditionally + * while logs keep [message]. + */ + open val userMessage: String get() = message.orEmpty() + class InvalidParameter(message: String, cause: Throwable? = null) : DashSdkError(message, cause) @@ -478,6 +497,93 @@ sealed class DashSdkError( cause, ) + /** + * `ErrorPersisterLoadTransient` (native code 49). Reading persisted + * wallet state failed on a store that reported the failure as + * retryable (`SQLITE_BUSY` and friends). Nothing was mutated — a + * load is a read — so this is retryable. The Android analog of + * Swift's `PlatformWalletError.persisterLoadTransient`. [message] is + * the diagnostic chain; display [userMessage]. + */ + class PersisterLoadTransient(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) { + override val isRetryable: Boolean get() = true + override val userMessage: String get() = PERSISTER_BUSY_USER_MESSAGE + } + + /** + * `ErrorPersisterLoadFatal` (native code 50). Reading persisted + * wallet state failed permanently — a corrupt or unreadable store, + * or a decode that will fail identically next time. Do NOT retry; + * the store needs repair or re-provisioning. Constraint-class read + * failures fold in here: a read cannot violate one, and neither is + * retryable. [message] is the diagnostic chain; display + * [userMessage]. + */ + class PersisterLoadFatal(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) { + override val userMessage: String get() = PERSISTER_UNREADABLE_USER_MESSAGE + } + + /** + * `ErrorPersisterStoreTransient` (native code 51). Writing wallet + * state failed on a busy or momentarily unavailable store. + * + * **Nothing was committed.** The native side only emits this when + * the persister guarantees the failed changeset round was rolled + * back whole, so re-issuing the operation cannot double-apply part + * of it — which is why this, uniquely among the store failures, is + * retryable. A wallet registration against a locked database + * produces it (dashpay/platform#4365); the retry decision is the + * host's, not the wallet's. [message] is the diagnostic chain; + * display [userMessage]. + */ + class PersisterStoreTransient(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) { + override val isRetryable: Boolean get() = true + override val userMessage: String get() = PERSISTER_BUSY_USER_MESSAGE + } + + /** + * `ErrorPersisterStoreFatal` (native code 52). Writing wallet state + * failed permanently — a full disk, a corrupt schema, an I/O error + * outside the retryable class. Do NOT retry; the wallet rolled its + * in-memory state back, so the operation may be re-attempted once + * the underlying fault is fixed. [message] is the diagnostic chain; + * display [userMessage]. + */ + class PersisterStoreFatal(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) { + override val userMessage: String get() = PERSISTER_UNSAVED_USER_MESSAGE + } + + /** + * `ErrorPersisterStoreConstraint` (native code 53). A write violated + * a constraint / foreign key / integrity rule. Deliberately distinct + * from [PersisterStoreFatal]: this is "the data is wrong" (a caller + * or schema-mapping bug) rather than "the storage engine is unhappy" + * (an operator problem), and the two route to different people. Do + * NOT retry unchanged. [message] is the diagnostic chain; display + * [userMessage]. + */ + class PersisterStoreConstraint(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) { + override val userMessage: String get() = PERSISTER_UNSAVED_USER_MESSAGE + } + + /** + * `ErrorPersisterRestore` (native code 54). Rehydrating persisted + * platform-address state into a freshly registered wallet failed. + * One code rather than three: it wraps a wallet error, not a store + * error, so it carries no retry classification. The wrapped error's + * rendering is in [message], which is diagnostic — display + * [userMessage]. + */ + class PersisterRestore(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) { + override val userMessage: String get() = PERSISTER_UNREADABLE_USER_MESSAGE + } + /** * Any other `PlatformWalletFFIResultCode` without a dedicated type. * Carries the platform-wallet [nativeCode] (already de-offset) and @@ -657,6 +763,17 @@ sealed class DashSdkError( // the deferred-token trio sits at 34-36 above. See // PlatformWalletFFIResultCode for the authoritative map.) 31 -> PlatformWallet.SigningKeyUnavailable(message, cause) + // Persister failures, operation x retry classification. These are + // exactly the "retry-semantics-bearing" codes this mapping exists + // for: only the two transients are retryable, and a constraint is + // kept apart from a fatal so hosts can route "your data is wrong" + // differently from "the storage engine is unhappy". + 49 -> PlatformWallet.PersisterLoadTransient(message, cause) + 50 -> PlatformWallet.PersisterLoadFatal(message, cause) + 51 -> PlatformWallet.PersisterStoreTransient(message, cause) + 52 -> PlatformWallet.PersisterStoreFatal(message, cause) + 53 -> PlatformWallet.PersisterStoreConstraint(message, cause) + 54 -> PlatformWallet.PersisterRestore(message, cause) else -> // @Deprecated fallback — see the code-6 arm; code 31 is the // real discriminator. diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt index d199a07ee36..1426fc60dea 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.kt @@ -32,9 +32,27 @@ package org.dashfoundation.dashsdk.ffi * [onWalletChangesetAccountBegin] / [onWalletChangesetAccountEnd]. * - Persist slots return `Int` (0 = ok, non-zero flips the round's * success flag so [onChangesetEnd] delivers the rollback). + * - A plain non-zero return means "failed, do not retry". A handler that + * can classify its own failure may instead return one of the two + * sentinels `platform-wallet-ffi` defines — [PERSIST_RC_TRANSIENT] for + * a retryable failure after which nothing was applied, or + * [PERSIST_RC_CONSTRAINT] for an integrity violation. The native side + * forwards the classification to its caller (surfacing as + * `DashSdkError.PlatformWallet.PersisterStoreTransient` and friends) and + * never retries on the handler's behalf. Returning the transient + * sentinel from a ROUND callback additionally asserts that a failed + * round is rolled back whole — see `PersistenceCallbacks` in + * `rs-platform-wallet-ffi/src/persistence.rs` for the exact contract. * - Load slots return flattened representations (`Array<...>` / typed * holder objects) that the trampoline re-packs into Rust-owned FFI - * structs; Kotlin never allocates native memory. + * structs; Kotlin never allocates native memory. **Only the + * `Int`-returning persist slots can carry a sentinel.** A load has no + * `Int` to put one in, so every load failure — a thrown exception + * included — reaches Rust as a fatal, unclassified error, and no load + * on this binding can report itself as transient or constraint-class. + * A subclass must therefore let a failed load THROW: returning an empty + * array reports a successful restore of nothing, which Rust reads as a + * fresh device, turning a store fault into apparent data loss. * * ## Threading * @@ -50,17 +68,29 @@ package org.dashfoundation.dashsdk.ffi */ abstract class NativePersistenceBridge { - /** - * Versioned semantic capability declaration consumed when JNI builds the - * native callback vtable. Defaults are deliberately zero: a no-op subclass - * must never gain capabilities merely because JNI supplies trampolines for - * every virtual method. - */ - open fun persistenceCapabilitiesVersion(): Int = 0 + companion object { + // Both values are the ABI defined by + // `packages/rs-platform-wallet-ffi/src/persistence.rs` and must + // change only together with it. - open fun persistenceCapabilitiesBits(): Long = 0L + /** + * A retryable failure after which nothing was applied. Returning it + * from a callback inside a changeset round also asserts that the + * failed round was rolled back whole. + * + * The round-end callback is the exception: failing it when the round + * had already failed means the rollback itself did not complete, so + * what reached the store is unknown. Rust classifies that as fatal and + * withholds the retry regardless of this value — re-issuing a + * changeset the store could neither apply nor undo risks merging it + * twice. This sentinel is honoured at round end only on a clean + * round, where the commit failed but the rollback succeeded. + */ + const val PERSIST_RC_TRANSIENT: Int = -2 + + /** A constraint / integrity violation — the data is wrong, not the store. */ + const val PERSIST_RC_CONSTRAINT: Int = -3 - companion object { /** * `PersistenceCapabilities::CORE_SWEEP_REMOVAL` (bit 11, `0x800`). * The one Kotlin home of this bit: `PlatformWalletPersistenceHandler` @@ -71,6 +101,16 @@ abstract class NativePersistenceBridge { const val CAPABILITY_CORE_SWEEP_REMOVAL: Long = 0x800 } + /** + * Versioned semantic capability declaration consumed when JNI builds the + * native callback vtable. Defaults are deliberately zero: a no-op subclass + * must never gain capabilities merely because JNI supplies trampolines for + * every virtual method. + */ + open fun persistenceCapabilitiesVersion(): Int = 0 + + open fun persistenceCapabilitiesBits(): Long = 0L + // ── Transactional bracketing ────────────────────────────────────── /** `on_changeset_begin_fn` — descriptor `([B)I`. */ @@ -729,6 +769,10 @@ abstract class NativePersistenceBridge { ): Int = 0 // ── Load callbacks ──────────────────────────────────────────────── + // + // These return objects rather than `Int`, so [PERSIST_RC_TRANSIENT] and + // [PERSIST_RC_CONSTRAINT] cannot be expressed here: a failing load + // reaches Rust as a fatal, unclassified error however it fails. /** * `on_load_wallet_list_fn`. Returns the persisted wallet list as an diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt index d68d1758f93..88a080308a0 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt @@ -96,6 +96,15 @@ import java.util.concurrent.Executors * `if !inChangeset { save() }` writers) commit immediately in their own * transaction when no round is open. * + * ## Load failure policy (mirrors the Swift `errored` return) + * + * A failed load FAILS the native load: the Room exception crosses into + * the trampoline, which returns a non-zero FFI code that reaches the host + * as [DashSdkError.PlatformWallet.PersisterLoadFatal]. Degrading to an + * empty result would report a successful restore of nothing, which Rust + * reads as a fresh device — a store fault would masquerade as data loss. + * [onGetCoreTxRecord] is the sole exception, by FFI contract; see its doc. + * * @param database the Room database to persist into. * @param dispatcher single-thread dispatcher confining all callback work; * `null` (the production default) creates a dedicated owned executor @@ -330,7 +339,7 @@ class PlatformWalletPersistenceHandler( /** * Serializes every persistence callback against compound external * sequences (wallet deletion's snapshot → secret delete → cascade). - * Each [guarded]/[guardedLoad] callback acquires it AT ENTRY on the + * Each [guarded]/[loadOrThrow] callback acquires it AT ENTRY on the * JNI caller thread — before any hop onto [dispatcher] — so a parked * callback never holds the persistence thread, and an exclusion * holder may safely run dispatcher-confined work. Callbacks fire @@ -2565,7 +2574,7 @@ class PlatformWalletPersistenceHandler( } } - override fun onLoadWalletList(): Array = guardedLoad(emptyArray()) { + override fun onLoadWalletList(): Array = loadOrThrow { runBlockingResult { healIdentityIsLocalFlags() // Restorable = wallet with ≥1 account carrying an xpub, @@ -2680,7 +2689,7 @@ class PlatformWalletPersistenceHandler( } } - override fun onLoadShieldedNotes(): Array = guardedLoad(emptyArray()) { + override fun onLoadShieldedNotes(): Array = loadOrThrow { runBlockingResult { // Shielded rows carry no wallet FK — read the whole table // directly (mirror of the Swift loader's fetch-all). @@ -2701,7 +2710,7 @@ class PlatformWalletPersistenceHandler( } override fun onLoadShieldedOutgoingNotes(): Array = - guardedLoad(emptyArray()) { + loadOrThrow { runBlockingResult { database.shieldedDao().getAllOutgoingNotes() .filter { it.recipient.size == 43 } @@ -2720,7 +2729,7 @@ class PlatformWalletPersistenceHandler( } override fun onLoadShieldedSyncStates(): Array = - guardedLoad(emptyArray()) { + loadOrThrow { runBlockingResult { database.shieldedDao().getAllSyncStates().map { s -> ShieldedSyncStateData( @@ -2732,7 +2741,7 @@ class PlatformWalletPersistenceHandler( } } - override fun onLoadShieldedActivity(): Array = guardedLoad(emptyArray()) { + override fun onLoadShieldedActivity(): Array = loadOrThrow { runBlockingResult { database.shieldedDao().getAllActivity().map { a -> ShieldedActivityData( @@ -2760,34 +2769,38 @@ class PlatformWalletPersistenceHandler( } /** - * Unlike best-effort cache loaders, a malformed persisted viewing key - * must fail the native load. Returning an empty array would masquerade as - * "no persisted key" and silently fall back to mnemonic resolution. - * Therefore validation/Room exceptions deliberately cross this virtual - * method into the JNI trampoline, which returns a non-zero FFI load code. - * The trampoline owns and frees its copied native restore array. + * A malformed persisted viewing key must fail the native load: an + * empty array would masquerade as "no persisted key" and silently + * fall back to mnemonic resolution. The trampoline owns and frees + * its copied native restore array. */ override fun onLoadShieldedViewingKeys(): Array = - runBlocking { - callbackExclusion.withLock { - runBlockingResult { - val keys = network?.let { lockedNetwork -> - database.walletDao().getByNetwork(lockedNetwork.ffiValue) - .flatMap { wallet -> - database.shieldedDao().getViewingKeysByWallet(wallet.walletId) - } - } ?: database.shieldedDao().getAllViewingKeys() - keys.map { key -> - ShieldedViewingKeyData( - walletId = key.walletId, - accountIndex = key.accountIndex, - fvkBytes = key.fvkBytes, - ) - }.toTypedArray() - } + loadOrThrow { + runBlockingResult { + val keys = network?.let { lockedNetwork -> + database.walletDao().getByNetwork(lockedNetwork.ffiValue) + .flatMap { wallet -> + database.shieldedDao().getViewingKeysByWallet(wallet.walletId) + } + } ?: database.shieldedDao().getAllViewingKeys() + keys.map { key -> + ShieldedViewingKeyData( + walletId = key.walletId, + accountIndex = key.accountIndex, + fvkBytes = key.fvkBytes, + ) + }.toTypedArray() } } + /** + * The one load slot allowed to contain its own failure: the FFI + * defines a non-zero return here as a transient miss surfaced to the + * asset-lock proof flow as `None`, which is exactly what a `null` + * answer produces. Both paths fall through to the SPV-event wait, so + * reporting a miss hides nothing (see `on_get_core_tx_record_fn` in + * `rs-platform-wallet-ffi/src/persistence.rs`). + */ override fun onGetCoreTxRecord(walletId: ByteArray, txid: ByteArray): CoreTxRecordData? = guardedLoad(null) { runBlockingResult { @@ -3044,8 +3057,7 @@ class PlatformWalletPersistenceHandler( /** * Whether the transaction [spendingTxid] funds an asset lock the - * network has already locked (`InstantSendLocked` or beyond), or - * `null` when the asset-lock table could not be read. + * network has already locked (`InstantSendLocked` or beyond). * * Keyed on the funding TXID alone, never on a single outpoint: * DIP-0027 lets one funding transaction carry several credit @@ -3054,23 +3066,17 @@ class PlatformWalletPersistenceHandler( * any vout. Finality belongs to the transaction, so any of its locks * reaching InstantSendLocked means the inputs are gone. * - * `null` is a deliberate third answer, not a swallowed error. This - * runs inside `guardedLoad(emptyArray())` and the Android load - * surface carries no error channel, so an escaping read failure would - * hand Rust a SUCCESSFUL EMPTY restore for every wallet — the - * strongest possible "this device has no coins". The fault is - * therefore contained to the single candidate it concerns and every - * unrelated wallet, account and TXO still restores. + * An unreadable asset-lock table fails the whole load rather than + * dropping the candidate: silently withholding an output the guard + * could not judge under-reports the wallet's funds, which is the + * apparent-data-loss this load path must never produce. Mirror of + * the Swift loader's `finalizedAssetLockFundingTxids` bail. */ - private suspend fun spendByFinalizedAssetLock(spendingTxid: ByteArray): Boolean? = - try { - val status = database.assetLockDao() - .maxStatusForTxid(spendingTxid.reversedArray().toHex()) - status != null && status >= ASSET_LOCK_STATUS_INSTANT_SEND_LOCKED - } catch (t: Throwable) { - Log.w(TAG, "load: asset-lock finality lookup failed; dropping the candidate UTXO", t) - null - } + private suspend fun spendByFinalizedAssetLock(spendingTxid: ByteArray): Boolean { + val status = database.assetLockDao() + .maxStatusForTxid(spendingTxid.reversedArray().toHex()) + return status != null && status >= ASSET_LOCK_STATUS_INSTANT_SEND_LOCKED + } /** * Assemble the [UtxoRestoreData] rows for one wallet: every unspent @@ -3126,30 +3132,18 @@ class PlatformWalletPersistenceHandler( // finality signal that provably arrives; from // InstantSendLocked on this output is gone. Skip it, and // heal the flag so isSpent-based readers stop counting it. - when (spendByFinalizedAssetLock(spendingTxid)) { - // Provably final. Heal opportunistically: excluding - // the row from THIS restore does not depend on the - // repair becoming durable, and the whole body of - // `onLoadWalletList` runs under - // `guardedLoad(emptyArray())` — an escaping write - // failure would discard every wallet's restore set + if (spendByFinalizedAssetLock(spendingTxid)) { + // Heal opportunistically: excluding the row from THIS + // restore does not depend on the repair becoming + // durable, so a failed write must not fail the load // over one unhealed row. Log and carry on instead, // the way `scrubAliases` treats its cleanup. - true -> { - try { - database.txoDao().markSpentByOutpoint(txo.outpoint, now()) - } catch (t: Throwable) { - Log.w(TAG, "load: failed to heal asset-lock-consumed TXO", t) - } - continue + try { + database.txoDao().markSpentByOutpoint(txo.outpoint, now()) + } catch (t: Throwable) { + Log.w(TAG, "load: failed to heal asset-lock-consumed TXO", t) } - // Unreadable (see the helper): drop this one candidate - // and never heal it. Under-reporting one output for a - // launch is recoverable; handing a consumed output back - // as spendable is what this guard exists to stop. - null -> continue - // Demonstrably not final — keep it in the restore set. - false -> Unit + continue } } val account = txo.accountId?.let { database.accountDao().getById(it) } @@ -3940,15 +3934,41 @@ class PlatformWalletPersistenceHandler( } /** - * Load-callback variant: on failure log and return [fallback]. Takes - * [callbackExclusion] like [guarded] — loads read the same state the - * deletion sequence mutates. + * Load-callback variant: log the failure and let it cross the JNI + * boundary, where the trampoline turns it into a non-zero FFI load + * code (surfacing as [DashSdkError.PlatformWallet.PersisterLoadFatal]). + * Takes [callbackExclusion] like [guarded] — loads read the same state + * the deletion sequence mutates. + * + * The log happens here because the trampoline only clears the pending + * exception; nothing downstream can still read its message or stack. + * + * Degrading to an empty result instead would report a successful + * restore of nothing, which Rust reads as a fresh device — a store + * fault would masquerade as data loss. Mirror of the Swift handler's + * `errored` return. + */ + private fun loadOrThrow(body: () -> T): T = + try { + runBlocking { callbackExclusion.withLock { body() } } + } catch (t: Throwable) { + Log.e(TAG, "persistence load callback failed; failing the native load", t) + throw t + } + + /** + * Load-callback variant for the ONE slot whose failure and whose + * empty answer are equivalent by contract: [onGetCoreTxRecord]. The + * FFI documents a non-zero return there as a transient backend miss + * surfaced to the proof flow as `None` — the same outcome [fallback] + * produces — so containing the fault here hides nothing. Every other + * load uses [loadOrThrow]. */ private fun guardedLoad(fallback: T, body: () -> T): T = try { runBlocking { callbackExclusion.withLock { body() } } } catch (t: Throwable) { - Log.e(TAG, "persistence load callback failed", t) + Log.e(TAG, "persistence record lookup failed; reporting a miss", t) fallback } diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt index aba0c4ceb2d..c4e859f3060 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt @@ -1097,6 +1097,9 @@ class PlatformWalletManager( * per restorable id to obtain a [ManagedPlatformWallet] handle. * * Idempotent: with no persisted state, leaves [wallets] untouched. + * + * On failure the manager is unchanged and still usable — fix the store + * and call again, or destroy the manager and rebuild it. */ suspend fun loadPersistedWallets(): List = withContext(Dispatchers.IO) { mapNativeErrors { WalletManagerNative.loadFromPersistor(managerHandle) } diff --git a/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt index 0889e6ba126..c5b288cf6b7 100644 --- a/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt +++ b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt @@ -217,6 +217,71 @@ class DashSdkErrorTest { ) } + @Test + fun persisterCodes49Through54MapTypedWithCorrectRetryability() { + // The whole point of the persister block: a host must be able to tell + // a busy store from a corrupt one WITHOUT parsing the message. Before + // these codes all three wallet variants flattened to ErrorUnknown and + // the classification died at the boundary. + val cases = listOf( + Triple(49, DashSdkError.PlatformWallet.PersisterLoadTransient::class.java, true), + Triple(50, DashSdkError.PlatformWallet.PersisterLoadFatal::class.java, false), + Triple(51, DashSdkError.PlatformWallet.PersisterStoreTransient::class.java, true), + Triple(52, DashSdkError.PlatformWallet.PersisterStoreFatal::class.java, false), + Triple(53, DashSdkError.PlatformWallet.PersisterStoreConstraint::class.java, false), + Triple(54, DashSdkError.PlatformWallet.PersisterRestore::class.java, false), + ) + + for ((code, type, retryable) in cases) { + val message = "persistence backend error from code $code" + val mapped = DashSdkError.fromNative( + DashSDKException(DashSdkError.PLATFORM_WALLET_CODE_OFFSET + code, message), + ) + + assertTrue( + "code $code must not fall through to Generic", + type.isInstance(mapped), + ) + assertEquals(message, mapped.message) + assertEquals( + "code $code retryability is part of its contract", + retryable, + mapped.isRetryable, + ) + } + } + + @Test + fun persisterCodesSplitUserMessageFromDiagnosticMessage() { + // The native message is a nested Rust error chain naming the + // operation, the backend classification and the store's phrasing. It + // must stay on `message` for logs and must never be what a UI shows; + // `userMessage` is the displayable half, and a failed write must not + // be described to a person as a failed read. + val chain = "failed to persist wallet registration changeset: " + + "persistence backend error (Transient): database is locked" + val busy = "The wallet database is busy. Try again in a moment." + val unreadable = "The wallet data could not be read and may need to be restored." + val unsaved = "The wallet data could not be saved and may need to be restored." + val expected = mapOf( + 49 to busy, + 50 to unreadable, + 51 to busy, + 52 to unsaved, + 53 to unsaved, + 54 to unreadable, + ) + + expected.forEach { (code, userMessage) -> + val mapped = DashSdkError.fromNative( + DashSDKException(DashSdkError.PLATFORM_WALLET_CODE_OFFSET + code, chain), + ) + + assertEquals("code $code user text", userMessage, mapped.userMessage) + assertEquals("code $code must keep the chain for logs", chain, mapped.message) + } + } + @Test fun assetLockInputConflictCode47MapsTyped() { // TERMINAL and RESERVED: no native path emits it today (that needs a diff --git a/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt index e9f941bcfd1..0aaacf099e1 100644 --- a/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt +++ b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.kt @@ -5419,9 +5419,8 @@ class PlatformWalletPersistenceHandlerTest { /** Hex prev-txids the restore hands back for [wallet], sorted. */ private fun restoredUtxoTxids(wallet: ByteArray): List { val entry = handler.onLoadWalletList().firstOrNull { it.walletId.contentEquals(wallet) } - // `guardedLoad` degrades to an empty array, which Rust reads as a - // fresh coinless device — name that failure rather than letting it - // surface as a NoSuchElementException. + // Name a missing wallet rather than letting it surface as a + // NoSuchElementException from the mapping below. assertNotNull("the restore must still carry this wallet", entry) return entry!!.utxos.map { it.prevTxid.toHex() }.sorted() } @@ -5503,25 +5502,14 @@ class PlatformWalletPersistenceHandlerTest { } /** - * Failure policy for the finality lookup the guard depends on. - * - * `onLoadWalletList` runs under `guardedLoad(emptyArray())` and the - * Android load surface is array-only — there is no error result — so - * an escaping read failure returns a SUCCESSFUL EMPTY restore, which - * Rust reads as a fresh, coinless device for EVERY wallet. The lookup - * must therefore contain its own failure: drop the one candidate it - * could not answer for (never healing it, since nothing was proven) - * and leave every unrelated wallet and TXO restoring normally. - * - * The fault is injected at the single prepared statement, not at the - * table, because the table is read by two other restore builders - * whose own failure modes are out of this guard's hands. + * Rebuild the fixture on a database whose statements starting with + * [failingSqlPrefix] can be faulted mid-test. The helper factory is + * fixed when the database is built, so this replaces the shared + * fixture rather than decorating it. The injector comes back + * disarmed — arm it once the seed data is in. */ - @Test - fun aFailingFinalizedLockLookupDropsOnlyItsOwnCandidate() = runTest { - // The helper factory is fixed when the database is built, so the - // shared fixture is replaced with one that can be faulted. - val faults = SingleStatementFaultInjector("SELECT MAX(statusRaw) FROM asset_locks") + private fun useFaultedDatabase(failingSqlPrefix: String): SingleStatementFaultInjector { + val faults = SingleStatementFaultInjector(failingSqlPrefix) db.close() db = Room.inMemoryDatabaseBuilder( ApplicationProvider.getApplicationContext(), @@ -5531,6 +5519,82 @@ class PlatformWalletPersistenceHandlerTest { .openHelperFactory(faults) .build() handler = newHandler() + return faults + } + + // ── Load failure policy ─────────────────────────────────────────── + // + // The load slots are array-shaped, so a load reports failure the only + // way it can: by throwing. The JNI trampoline turns the pending + // exception into a non-zero FFI load code, reaching the host as + // `PersisterLoadFatal` (50). Degrading to an empty array would instead + // report a SUCCESSFUL restore of nothing, which Rust reads as a fresh + // device — a store fault masquerading as data loss. Swift parity: + // `loadWalletList` returns `errored = true`. + + @Test + fun aFailingWalletFetchFailsTheLoadRatherThanRestoringNothing() = runTest { + val faults = useFaultedDatabase("SELECT * FROM wallets") + seedRestorableWallet(walletId, "yLoadFailFunder", ByteArray(32) { 81 }, 34) + + // Control: the wallet restores while the fetch is readable, so the + // failure below is the injected fault and not the fixture. + assertEquals(1, handler.onLoadWalletList().size) + + faults.armed = true + assertThrows( + "a failed wallet fetch must fail the load; an empty restore would " + + "report every persisted wallet as absent", + SQLiteException::class.java, + ) { handler.onLoadWalletList() } + } + + @Test + fun aFailingShieldedNoteFetchFailsTheLoad() = runTest { + val faults = useFaultedDatabase("SELECT * FROM shielded_notes") + handler.onChangesetBegin(walletId) + handler.onPersistShieldedNote( + walletId = walletId, + noteWalletId = walletId, + accountIndex = 0, + position = 3, + cmx = ByteArray(32) { 82 }, + nullifier = ByteArray(32) { 83 }, + blockHeight = 50, + isSpent = 0, + value = 100_000, + noteData = ByteArray(115) { 84 }, + ) + handler.onChangesetEnd(walletId, success = true) + + // Control, as above. + assertEquals(1, handler.onLoadShieldedNotes().size) + + faults.armed = true + assertThrows( + "a failed shielded-note fetch must fail the load; an empty restore " + + "would report the persisted notes as absent", + SQLiteException::class.java, + ) { handler.onLoadShieldedNotes() } + } + + /** + * Failure policy for the finality lookup the guard depends on. + * + * An unreadable asset-lock table cannot answer whether the output is + * gone. Withholding the one candidate it could not judge would + * under-report the wallet's funds — the same apparent data loss an + * empty restore produces, just quieter — so the read failure fails + * the whole load and the host retries. Swift parity: + * `finalizedAssetLockFundingTxids` bails with `errored = true`. + * + * The fault is injected at the single prepared statement, not at the + * table, because the table is read by two other restore builders + * whose own failure modes are out of this guard's hands. + */ + @Test + fun aFailingFinalizedLockLookupFailsTheLoad() = runTest { + val faults = useFaultedDatabase("SELECT MAX(statusRaw) FROM asset_locks") val fundingTxid = ByteArray(32) { 71 } val lockTxid = ByteArray(32) { 72 } @@ -5538,7 +5602,7 @@ class PlatformWalletPersistenceHandlerTest { walletId, "yLockFunderThrow", fundingTxid, lockTxid, 32, ) seedConsumedAssetLockRow(walletId, lockTxid, vout = 0) - // Sentinel 1: an ordinary unspent output on the SAME wallet, with + // Sentinel: an ordinary unspent output on the same wallet, with // no spender at all, so it never reaches the lookup. val sentinelTxid = ByteArray(32) { 73 } handler.onChangesetBegin(walletId) @@ -5548,15 +5612,10 @@ class PlatformWalletPersistenceHandlerTest { ) handler.onChangesetEnd(walletId, success = true) - // Sentinel 2: an unrelated wallet with its own restorable output. - val otherWallet = ByteArray(32) { 74 } - val otherTxid = ByteArray(32) { 75 } - seedRestorableWallet(otherWallet, "yOtherFunder", otherTxid, 33) - - // Readable lookup: the guard excludes the consumed output and - // keeps both sentinels. + // Control: with the lookup readable the load succeeds, excludes + // the consumed output and keeps the sentinel — so the failure + // below is the injected fault, not the fixture. assertEquals(listOf(sentinelTxid.toHex()), restoredUtxoTxids(walletId)) - assertEquals(listOf(otherTxid.toHex()), restoredUtxoTxids(otherWallet)) assertTrue(db.txoDao().getByOutpoint(makeOutpoint(fundingTxid, 0))!!.isSpent) // Re-stale the healed row so the unreadable pass faces the same @@ -5566,16 +5625,11 @@ class PlatformWalletPersistenceHandlerTest { ) faults.armed = true - assertEquals( - "only the unanswerable candidate is dropped; the unrelated output survives", - listOf(sentinelTxid.toHex()), - restoredUtxoTxids(walletId), - ) - assertEquals( - "and so does the unrelated wallet's — one bad lookup cannot empty the restore", - listOf(otherTxid.toHex()), - restoredUtxoTxids(otherWallet), - ) + assertThrows( + "an unanswerable finality lookup must fail the load, not silently " + + "withhold the output it could not judge", + SQLiteException::class.java, + ) { handler.onLoadWalletList() } assertFalse( "an unanswerable lookup proves nothing, so it must not heal the flag", db.txoDao().getByOutpoint(makeOutpoint(fundingTxid, 0))!!.isSpent, diff --git a/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md b/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md index 181eefe62f2..ad1fdf0b16e 100644 --- a/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md +++ b/packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md @@ -114,10 +114,11 @@ These are shipped ABI. Do not renumber. | 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 +**Next allocatable integer: 55** — 27–54 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, +rows below); 49–54 proposed by active #4586 (the persister +operation × kind block); 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,7 +126,8 @@ 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; use the next allocatable integer stated above. +(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.) @@ -158,6 +160,12 @@ Fork-era numbers remain in the collision history, which is immutable record. | 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 | `ErrorPersisterLoadTransient` | #4586 | Proposed — claimed from the frontier (48 at the time of the claim). Reading persisted state failed on a store that classified the failure retryable; nothing was mutated. First of a six-code `operation × kind` block: the wallet's `PersisterLoad` / `PersisterStore` / `PersisterRestore` variants each carry a typed `PersistenceError`, and before this block all three flattened to `ErrorUnknown` (99), so the retry classification died at the C boundary while the Rust API had carried it faithfully | +| 50 | `ErrorPersisterLoadFatal` | #4586 | Proposed — permanent read failure. `Fatal`, `Constraint` and `LockPoisoned` all fold here: a read cannot violate a constraint, and none of the three is retryable, so splitting them would spend codes hosts would handle identically | +| 51 | `ErrorPersisterStoreTransient` | #4586 | Proposed — a transient write failure from a backend that attests nothing was applied or retained. For FFI hosts this requires `ATOMIC_CHANGESETS`, both round brackets, and successful rollback of a failed round. A busy database produces 51 only with this attestation; the buffered `SqlitePersister` withholds it and maps to Fatal/52, requiring backend-aware `flush` recovery instead of reissuing `store` (refs #4365) | +| 52 | `ErrorPersisterStoreFatal` | #4586 | Proposed — permanent write failure, plus `LockPoisoned` (which carries no kind of its own) | +| 53 | `ErrorPersisterStoreConstraint` | #4586 | Proposed — integrity/foreign-key violation, kept apart from 52 so a host can route "your data is wrong" (caller or schema-mapping bug) differently from "the storage engine is unhappy" (operator/infrastructure). Not retryable either way | +| 54 | `ErrorPersisterRestore` | #4586 | Proposed — rehydrating persisted platform-address state into a freshly registered wallet failed. One code, not three: the variant wraps a `PlatformWalletError` rather than a `PersistenceError`, so there is no kind to split on | **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 @@ -242,6 +250,15 @@ that was always required was made — onto the wrong integers. | 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 | +**These two claims are now also redundant, not just misnumbered.** #4586's +49–54 block covers the same ground with finer granularity — it splits the +retry classification by *operation* as well as by kind, so +`ErrorPersisterTransient` / `ErrorPersisterFatal` have no meaning left that +49–52 do not already carry. If #3968 still needs codes it should adopt the +existing block rather than take two more integers from the frontier; a +second, coarser pair of persister codes would leave hosts with two ways to +learn the same thing and no rule for which one arrives. + 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**, which merged 27 into `v4.2-dev` on 2026-08-02. See the collision history below. @@ -259,8 +276,8 @@ 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, and as a later "48 and 49" copy did once #4356 took 48 and #4586 took the +49–54 persister block — read the frontier note, do not copy it). 26 and 27 need nothing: they are the merged base's own values, correctly inherited, and rule 3 keeps them where they are. diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index 451b4e72350..8295410e01c 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -1,4 +1,5 @@ use dpp::platform_value::string_encoding::Encoding; +use platform_wallet::changeset::PersistenceErrorKind; use platform_wallet::PlatformWalletError; use std::ffi::CString; use std::os::raw::c_char; @@ -293,6 +294,7 @@ pub enum PlatformWalletFFIResultCode { // 47 ErrorAssetLockInputConflict asset-lock double-spend detection // (terminal; RESERVED, no emitter yet) // 48 ErrorAssetLockInputContested asset-lock double-spend detection (provisional) + // 49-54 the persister operation x kind block below // // 38/39/40 carry a STABLE JSON detail object in the result `message` // instead of the typed `Display` rendering — see each variant's doc for @@ -504,6 +506,76 @@ pub enum PlatformWalletFFIResultCode { /// height, and says the verdict is provisional. ErrorAssetLockInputContested = 48, + // ----------------------------------------------------------------- + // Persister failures, operation x retry classification (49-54). + // + // The wallet's PersisterLoad / PersisterStore / PersisterRestore each + // carry a typed `PersistenceError` whose `kind` says whether a retry can + // help. One code per (operation, kind) pair keeps both halves: a host can + // tell a failed read from a failed write AND a retryable failure from a + // permanent one, without parsing the message. + // ----------------------------------------------------------------- + /// Maps `PlatformWalletError::PersisterLoad` classified + /// [`Transient`](platform_wallet::changeset::PersistenceErrorKind::Transient): + /// a retryable condition (`SQLITE_BUSY` and friends) while reading. + /// + /// Host action: retry later. Nothing was mutated — a load is a read. + ErrorPersisterLoadTransient = 49, + + /// Maps `PlatformWalletError::PersisterLoad` for every other + /// classification — `Fatal`, `Constraint`, and a poisoned persister lock: + /// a corrupt or unreadable store, or a decode that will fail identically + /// next time. + /// + /// Host action: do NOT retry; inspect the message and repair or + /// re-provision the store. `Constraint` folds in here because a read + /// cannot violate one — reported on a load it is a backend defect, not a + /// caller data error, and not retryable either way. + ErrorPersisterLoadFatal = 50, + + /// Maps `PlatformWalletError::PersisterStore` classified + /// [`Transient`](platform_wallet::changeset::PersistenceErrorKind::Transient): + /// a busy or momentarily unavailable store rejected the write. + /// + /// **Nothing was applied or retained**: the persister must attest that the + /// failed store is safe to reissue, including that it buffered nothing. + /// + /// Host action: retry later. A busy database produces this code only when + /// its backend provides that attestation. The buffered `SqlitePersister` + /// does not: its transient store failures map to `ErrorPersisterStoreFatal` + /// and require backend-aware recovery through `flush`, not another `store`. + ErrorPersisterStoreTransient = 51, + + /// Maps `PlatformWalletError::PersisterStore` classified `Fatal`, and a + /// poisoned persister lock: a full disk, a corrupt schema, an I/O error + /// outside the retryable class. + /// + /// Host action: do NOT retry; inspect the message. The wallet's in-memory + /// state was rolled back to before the operation, so the host may + /// re-attempt once the underlying fault is fixed. + ErrorPersisterStoreFatal = 52, + + /// Maps `PlatformWalletError::PersisterStore` classified + /// [`Constraint`](platform_wallet::changeset::PersistenceErrorKind::Constraint): + /// a SQL constraint / foreign-key / integrity violation. Distinct from + /// [`Self::ErrorPersisterStoreFatal`] so a host can separate "your data is + /// wrong" (caller or schema-mapping bug) from "the storage engine is + /// unhappy" (operator problem) — they route to different people. + /// + /// Host action: do NOT retry unchanged — fix the data, or the host-side + /// schema mapping that produced it. + ErrorPersisterStoreConstraint = 53, + + /// Maps `PlatformWalletError::PersisterRestore`: rehydrating persisted + /// platform-address state into a freshly registered wallet failed. One + /// code, not three — it wraps a `PlatformWalletError` rather than a + /// `PersistenceError`, so there is no retry classification to split on, + /// and the wrapped error's `Display` is the only detail channel. + /// + /// Host action: inspect the message; the wallet was registered but its + /// persisted address state did not come back. + ErrorPersisterRestore = 54, + /// The named thing does not exist. /// /// Originally (and still mostly) the code for every `Option` returned as an @@ -884,6 +956,28 @@ impl From for PlatformWalletFFIResult { // rides `NotFound` rather than spending a fifth marketplace // code hosts would handle identically. PlatformWalletError::DpnsNameNotFound { .. } => PlatformWalletFFIResultCode::NotFound, + // The persister trio, split by the store's own retry + // classification — flattened to ErrorUnknown a host could not tell + // a busy database from a corrupt one. `PersisterRestore` carries + // no kind to split on, so it takes a single code. + PlatformWalletError::PersisterLoad(source) => match source.kind() { + Some(PersistenceErrorKind::Transient) => { + PlatformWalletFFIResultCode::ErrorPersisterLoadTransient + } + _ => PlatformWalletFFIResultCode::ErrorPersisterLoadFatal, + }, + PlatformWalletError::PersisterStore(source) => match source.kind() { + Some(PersistenceErrorKind::Transient) => { + PlatformWalletFFIResultCode::ErrorPersisterStoreTransient + } + Some(PersistenceErrorKind::Constraint) => { + PlatformWalletFFIResultCode::ErrorPersisterStoreConstraint + } + _ => PlatformWalletFFIResultCode::ErrorPersisterStoreFatal, + }, + PlatformWalletError::PersisterRestore(..) => { + PlatformWalletFFIResultCode::ErrorPersisterRestore + } // NOTE: `MessageSigningFailed` is deliberately NOT matched, so it // falls to the `ErrorUnknown` catch-all below. Its causes are // internal invariant breaks (a public key that does not own the @@ -1972,6 +2066,215 @@ mod tests { assert_eq!(result.code, PlatformWalletFFIResultCode::ErrorUnknown); } + /// A `PersistenceError` of a chosen kind, as a backend would report it. + fn persistence_error( + kind: PersistenceErrorKind, + ) -> platform_wallet::changeset::PersistenceError { + platform_wallet::changeset::PersistenceError::backend_with_kind(kind, "database is locked") + } + + /// The one persister outcome a host may retry unchanged. + #[test] + fn persister_load_transient_maps_to_code_49() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterLoadTransient as i32, + 49 + ); + + let result: PlatformWalletFFIResult = PlatformWalletError::from_load_failure( + persistence_error(PersistenceErrorKind::Transient), + ) + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterLoadTransient + ); + assert!( + message_of(&result).contains("database is locked"), + "the typed Display must survive the conversion: {}", + message_of(&result) + ); + } + + /// None is retryable, and a read cannot violate a constraint. + #[test] + fn persister_load_non_transient_kinds_fold_onto_code_50() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterLoadFatal as i32, + 50 + ); + + for error in [ + persistence_error(PersistenceErrorKind::Fatal), + persistence_error(PersistenceErrorKind::Constraint), + platform_wallet::changeset::PersistenceError::LockPoisoned, + ] { + let rendered = error.to_string(); + let result: PlatformWalletFFIResult = + PlatformWalletError::from_load_failure(error).into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterLoadFatal, + "every non-transient load failure folds onto 50: {rendered}" + ); + } + } + + /// A persister attesting the atomic round contract, so the mapping tests + /// below exercise the code table rather than the re-issue gate. + fn atomic_persister() -> crate::persistence::FFIPersister { + extern "C" fn ok_begin(_ctx: *mut std::ffi::c_void, _wallet_id: *const u8) -> i32 { + 0 + } + extern "C" fn ok_end( + _ctx: *mut std::ffi::c_void, + _wallet_id: *const u8, + _success: bool, + ) -> i32 { + 0 + } + + crate::persistence::FFIPersister::new_with_persistence_capabilities( + crate::persistence::PersistenceCallbacks { + on_changeset_begin_fn: Some(ok_begin), + on_changeset_end_fn: Some(ok_end), + ..Default::default() + }, + platform_wallet::changeset::PersistenceCapabilities::ATOMIC_CHANGESETS, + ) + } + + /// The busy-database registration case (`dashpay/platform#4365`): the + /// wallet does not retry the write, the host learns it may. + #[test] + fn persister_store_transient_maps_to_code_51() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterStoreTransient as i32, + 51 + ); + + let result: PlatformWalletFFIResult = PlatformWalletError::from_store_failure( + &atomic_persister(), + persistence_error(PersistenceErrorKind::Transient), + ) + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterStoreTransient + ); + + // Code 51 promises the host nothing was committed and the changeset + // may be re-sent. A persister that does not attest that never reaches + // it — the promise is enforced before the code is chosen, not after. + let unattested: PlatformWalletFFIResult = PlatformWalletError::from_store_failure( + &crate::persistence::FFIPersister::new( + crate::persistence::PersistenceCallbacks::default(), + ), + persistence_error(PersistenceErrorKind::Transient), + ) + .into(); + assert_eq!( + unattested.code, + PlatformWalletFFIResultCode::ErrorPersisterStoreFatal, + "an unattested persister must not produce the re-issue invitation" + ); + } + + /// Permanent writes, plus the lock-poisoned case that has no kind. + #[test] + fn persister_store_fatal_maps_to_code_52() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterStoreFatal as i32, + 52 + ); + + for error in [ + persistence_error(PersistenceErrorKind::Fatal), + platform_wallet::changeset::PersistenceError::LockPoisoned, + ] { + let result: PlatformWalletFFIResult = + PlatformWalletError::from_store_failure(&atomic_persister(), error).into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterStoreFatal + ); + } + } + + /// "Your data is wrong" must not arrive as "the storage engine is unhappy". + #[test] + fn persister_store_constraint_maps_to_code_53() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterStoreConstraint as i32, + 53 + ); + + let result: PlatformWalletFFIResult = PlatformWalletError::from_store_failure( + &atomic_persister(), + persistence_error(PersistenceErrorKind::Constraint), + ) + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterStoreConstraint + ); + assert_ne!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterStoreFatal + ); + } + + /// One code, and the wrapped error's rendering still reaches the host. + #[test] + fn persister_restore_maps_to_code_54() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorPersisterRestore as i32, + 54 + ); + + let result: PlatformWalletFFIResult = PlatformWalletError::from_restore_failure( + PlatformWalletError::WalletCreation("no address pool".to_string()), + ) + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorPersisterRestore + ); + assert!( + message_of(&result).contains("no address pool"), + "the wrapped error's Display is the only detail channel: {}", + message_of(&result) + ); + } + + /// A host pins these integers, so a collision with an already-allocated + /// code silently re-labels a shipped meaning. + #[test] + fn persister_codes_occupy_their_own_slots() { + let persister = [ + PlatformWalletFFIResultCode::ErrorPersisterLoadTransient as i32, + PlatformWalletFFIResultCode::ErrorPersisterLoadFatal as i32, + PlatformWalletFFIResultCode::ErrorPersisterStoreTransient as i32, + PlatformWalletFFIResultCode::ErrorPersisterStoreFatal as i32, + PlatformWalletFFIResultCode::ErrorPersisterStoreConstraint as i32, + PlatformWalletFFIResultCode::ErrorPersisterRestore as i32, + ]; + assert_eq!(persister, [49, 50, 51, 52, 53, 54]); + + // The highest code allocated before this block, plus the terminal + // sentinels. + for taken in [ + PlatformWalletFFIResultCode::ErrorAssetLockInputContested as i32, + PlatformWalletFFIResultCode::NotFound as i32, + PlatformWalletFFIResultCode::ErrorUnknown as i32, + ] { + assert!( + !persister.contains(&taken), + "persister codes must not collide with {taken}" + ); + } + } + /// Read a result's message back as an owned `String`. Every /// marketplace assertion below inspects the message, and the raw /// `CStr::from_ptr` dance is noise at each site. diff --git a/packages/rs-platform-wallet-ffi/src/manager.rs b/packages/rs-platform-wallet-ffi/src/manager.rs index 84ef9bddac9..dfa716ff65c 100644 --- a/packages/rs-platform-wallet-ffi/src/manager.rs +++ b/packages/rs-platform-wallet-ffi/src/manager.rs @@ -644,6 +644,13 @@ pub unsafe extern "C" fn platform_wallet_manager_create_wallet_from_mnemonic_wit /// produce wallet handles — the caller should follow up with /// [`platform_wallet_manager_get_wallet`] per `wallet_id` it knows /// about. +/// +/// On error the handle stays valid and the manager is unchanged: fix the +/// store and call again, or destroy the manager and reconstruct it. Reopening +/// the same store path requires all persister references to be released. +/// [`platform_wallet_manager_destroy`] drops the manager's own references; +/// wallet handles, workers and in-flight operations can retain others after +/// it returns. #[no_mangle] pub unsafe extern "C" fn platform_wallet_manager_load_from_persistor( manager_handle: Handle, @@ -725,10 +732,8 @@ pub unsafe extern "C" fn platform_wallet_manager_destroy( release them on exit" ); } - // Dropping the manager here releases its persister/event-handler - // references; the host contexts are released (via `release_fn`) - // as soon as the last worker's reference drops — typically right - // now, or later if a straggler is still draining. + // Host contexts release with their last reference, which may outlive + // this manager through a wallet handle, worker or in-flight operation. } PlatformWalletFFIResult::ok() } diff --git a/packages/rs-platform-wallet-ffi/src/persistence.rs b/packages/rs-platform-wallet-ffi/src/persistence.rs index 2b0b23dbd49..398a3c88fa3 100644 --- a/packages/rs-platform-wallet-ffi/src/persistence.rs +++ b/packages/rs-platform-wallet-ffi/src/persistence.rs @@ -4,6 +4,10 @@ //! data is available (e.g., address balances), it is sent across FFI in //! C-compatible structs so the caller can persist it incrementally (e.g., via //! SwiftData on iOS). +//! +//! The negative callback return codes defined here are unrelated to +//! `rs-unified-sdk-jni`'s `RESOLVE_*` mnemonic-resolver codes, which reuse the +//! same integers on a different callback family. use bincode::config; use key_wallet::account::account_collection::AccountCollection; @@ -25,9 +29,9 @@ use std::str::FromStr; use crate::types::{FFINetwork, Network}; use platform_wallet::changeset::{ AccountAddressPoolEntry, AccountRegistrationEntry, ClientStartState, ClientWalletStartState, - ListedCoreTxid, PersistenceCapabilities, PersistenceError, PlatformWalletChangeSet, - PlatformWalletPersistence, ProviderKeyAccountEntry, ProviderKeyExtendedPubKey, - PERSISTENCE_CAPABILITIES_VERSION, + ListedCoreTxid, PersistenceCapabilities, PersistenceError, PersistenceErrorKind, + PlatformWalletChangeSet, PlatformWalletPersistence, ProviderKeyAccountEntry, + ProviderKeyExtendedPubKey, PERSISTENCE_CAPABILITIES_VERSION, }; use platform_wallet::wallet::platform_wallet::WalletId; use platform_wallet::wallet::{PerAccountPlatformAddressState, PerWalletPlatformAddressState}; @@ -346,6 +350,94 @@ pub struct PersistenceExtensionCallbacks { pub wallet_changeset_chain_lock_height: Option, } +/// Return value by which a persistence callback reports a **retryable** +/// failure after which nothing was applied (the host's own `SQLITE_BUSY` / +/// `SQLITE_FULL` / `SQLITE_IOERR` class). +/// +/// The host holds the storage handle and is the only party that can see the +/// native status code, so this is the only channel through which a retry +/// classification reaches Rust. Surfaces to the caller as +/// [`PersistenceErrorKind::Transient`]; the caller — never this crate — +/// decides whether to retry. +/// +/// Hosts use their binding's named constant rather than the literal: +/// `NativePersistenceBridge.PERSIST_RC_TRANSIENT` (Kotlin), +/// `PlatformWalletPersistRC.transient` (Swift). +pub const PLATFORM_WALLET_PERSIST_RC_TRANSIENT: i32 = -2; + +/// Return value by which a persistence callback reports a constraint / +/// foreign-key / integrity violation, surfacing as +/// [`PersistenceErrorKind::Constraint`] — "the data is wrong", as opposed to +/// "the storage engine is unhappy". Not retryable. +/// +/// Named on the host side as `NativePersistenceBridge.PERSIST_RC_CONSTRAINT` +/// (Kotlin) and `PlatformWalletPersistRC.constraint` (Swift). +pub const PLATFORM_WALLET_PERSIST_RC_CONSTRAINT: i32 = -3; + +/// Classify a non-zero persistence-callback return value. +/// +/// Only the two documented sentinels carry a classification; every other +/// non-zero value keeps the conservative [`PersistenceErrorKind::Fatal`] +/// reading, so hosts written against the plain `0` / non-zero contract +/// behave exactly as before. +fn persist_rc_kind(rc: i32) -> PersistenceErrorKind { + match rc { + PLATFORM_WALLET_PERSIST_RC_TRANSIENT => PersistenceErrorKind::Transient, + PLATFORM_WALLET_PERSIST_RC_CONSTRAINT => PersistenceErrorKind::Constraint, + _ => PersistenceErrorKind::Fatal, + } +} + +/// Build the error for a non-zero return from a **single-call** callback (a +/// load, a flush, a standalone persist), carrying the host's classification of +/// `rc`. Round callbacks instead accumulate into [`RoundOutcome`], which +/// classifies once for the whole round. +fn persist_callback_error(rc: i32, message: impl Into) -> PersistenceError { + PersistenceError::backend_with_kind(persist_rc_kind(rc), message.into()) +} + +/// The verdict of one `store` round's callbacks: fails if any callback failed, +/// reporting the MOST SEVERE kind any returned +/// (`Fatal` > `Constraint` > `Transient`) so one host-declared transient can +/// never mask a fatal sibling. +#[derive(Default)] +struct RoundOutcome { + worst: Option, +} + +impl RoundOutcome { + /// Record a non-zero return `rc` from a round callback. + fn record(&mut self, rc: i32) { + self.escalate(persist_rc_kind(rc)); + } + + /// Record a failure that can never be transient: a Rust-side encoding + /// failure (the same changeset will not encode later), or a rollback the + /// host could not complete. + fn record_fatal(&mut self) { + self.escalate(PersistenceErrorKind::Fatal); + } + + /// `PersistenceErrorKind` declares its variants in ascending severity and + /// derives `Ord` accordingly, so the ranking lives on the type. + fn escalate(&mut self, kind: PersistenceErrorKind) { + if self.worst.is_none_or(|worst| kind > worst) { + self.worst = Some(kind); + } + } + + /// `true` while every callback so far has succeeded — what + /// `on_changeset_end_fn` receives as its `success` argument. + fn is_success(&self) -> bool { + self.worst.is_none() + } + + /// The kind to report for the round, or `None` if it succeeded. + fn failure_kind(&self) -> Option { + self.worst + } +} + /// C callback vtable for wallet persistence. /// /// General-purpose notifications (`on_store_fn`, `on_flush_fn`) plus @@ -367,6 +459,49 @@ pub struct PersistenceExtensionCallbacks { /// callback returns and the lock is released.) Keep the work bounded; the call /// blocks every other wallet accessor while it runs. Mirrors the Rust-side /// `PlatformWalletPersistence::store` reentrancy contract. +/// +/// # Reporting a failure's retry classification +/// +/// Every callback below returns `0` for success and non-zero for failure. +/// A plain non-zero value means "failed, do not retry" — the conservative +/// reading Rust has always applied, so a host written against the original +/// contract needs no change. +/// +/// A host that can classify its own failure (it holds the storage handle and +/// sees the native status code) may instead return one of two sentinels, +/// which reach the Rust caller as a typed retry classification: +/// +/// * [`PLATFORM_WALLET_PERSIST_RC_TRANSIENT`] — a retryable failure after +/// which **nothing was applied** (`SQLITE_BUSY` and friends). +/// * [`PLATFORM_WALLET_PERSIST_RC_CONSTRAINT`] — a constraint / integrity +/// violation: the data is wrong, and retrying it unchanged will not help. +/// +/// Writes are never retried in-crate; the caller decides. Manager hydration +/// and wallet registration retry transient loads up to four attempts, with +/// 20/40/80 ms backoff. Direct trait reads follow their documented policy. +/// +/// ## What a transient verdict promises, and who must honour it +/// +/// A caller acting on "transient" re-issues the WHOLE changeset, and changeset +/// vectors merge by appending — so the verdict is only meaningful when the +/// failed round left nothing applied. That is exactly what +/// `ATOMIC_CHANGESETS` attests and what [`Self::on_changeset_end_fn`] with +/// `success = false` exists to drive, so a `store` round reports a transient +/// failure ONLY when both round brackets are wired AND the host declared +/// `ATOMIC_CHANGESETS`; otherwise Rust downgrades it to fatal, because a +/// partially applied round re-sent in full duplicates rows rather than +/// replacing them. **A host that does not roll a failed round back must not +/// return the transient sentinel from a round callback.** Single-call +/// callbacks (loads, flush, the changeset-begin abort) have no such +/// precondition: each is one operation that either happened or did not. +/// +/// ## Which slots can carry a sentinel at all +/// +/// Only slots that return `i32`. A binding whose load slots hand back holder +/// objects rather than a status code — the Kotlin bridge — has nowhere to put +/// one, so every load failure there, thrown exceptions included, reaches Rust +/// as fatal and unclassified. `on_get_core_tx_record_fn` is a further +/// exception in this vtable: see its own doc. #[repr(C)] #[allow(clippy::type_complexity)] pub struct PersistenceCallbacks { @@ -391,7 +526,10 @@ pub struct PersistenceCallbacks { /// itself failed (e.g. the atomic `save()` threw and the staged /// writes were rolled back); `store()` then returns `Err` so the /// caller does not advance state against data that never reached - /// durable storage. + /// durable storage. Failing while `success` was `false` reports a + /// failed ROLLBACK instead, leaving the round's disposition unknown: + /// `store()` then reports fatal whatever this returns, because a + /// changeset that may be half-applied must never be re-issued. pub on_changeset_end_fn: Option< unsafe extern "C" fn(context: *mut c_void, wallet_id: *const u8, success: bool) -> i32, >, @@ -1262,6 +1400,21 @@ impl FFIPersister { } } + /// Narrow a round's failure kind to what the caller may safely act on: + /// `Transient` survives only where + /// [`Self::store_transient_is_reissuable`] holds, since losing a retry + /// opportunity costs less than the rows a re-sent partial round would + /// duplicate. `Constraint` and `Fatal` invite no retry, so they pass + /// through. + fn reportable_round_kind(&self, reported: PersistenceErrorKind) -> PersistenceErrorKind { + match reported { + PersistenceErrorKind::Transient if !self.store_transient_is_reissuable() => { + PersistenceErrorKind::Fatal + } + kind => kind, + } + } + /// Compute the callback contracts that are structurally complete in this /// vtable. This mask is only an upper bound: the host must separately attest /// the semantics it actually implements. @@ -1376,6 +1529,16 @@ impl FFIPersister { } impl PlatformWalletPersistence for FFIPersister { + /// A host attests `ATOMIC_CHANGESETS` for a round it commits or rolls back + /// whole; nothing of a rolled-back round survives on this side either, so + /// the changeset is the caller's to re-issue. The capability is the + /// declaration intersected with the wired brackets, so an attestation + /// without an `end` callback to roll anything back does not count. + fn store_transient_is_reissuable(&self) -> bool { + self.persistence_capabilities() + .contains(PersistenceCapabilities::ATOMIC_CHANGESETS) + } + // Fan-out coverage note: `pending_contact_crypto_added` / // `pending_contact_crypto_cleared` have no vtable slots yet, so the // deferred contact-crypto queue is NOT durable on FFI hosts — the @@ -1442,9 +1605,10 @@ impl PlatformWalletPersistence for FFIPersister { ) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_persist_tracked_masternodes_fn returned error code {rc}" - ))); + return Err(persist_callback_error( + rc, + format!("on_persist_tracked_masternodes_fn returned error code {rc}"), + )); } Ok(()) } @@ -1481,9 +1645,10 @@ impl PlatformWalletPersistence for FFIPersister { ) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_tracked_masternodes_fn returned error code {rc}" - ))); + return Err(persist_callback_error( + rc, + format!("on_load_tracked_masternodes_fn returned error code {rc}"), + )); } let mut out = Vec::with_capacity(count); if !rows_ptr.is_null() && count > 0 { @@ -1560,19 +1725,21 @@ impl PlatformWalletPersistence for FFIPersister { // A nonzero begin means the client could NOT open its // transaction. Proceeding would run every per-kind // callback against no batch and then fire an unmatched - // `end`. Treat it as fatal: close the Rust-side round - // (so `in_round` doesn't wedge) and fail now, before any - // per-kind write. (Unlike the previous advisory-log - // behavior, the round is aborted so no state advances - // against an unopened batch.) + // `end`. Close the Rust-side round (so `in_round` doesn't + // wedge) and fail now, before any per-kind write — nothing + // was applied, so the host's own classification of `result` + // is reported as-is. let _ = round.end_round(); - return Err(PersistenceError::backend(format!( - "changeset-begin callback returned error code {result}; \ + return Err(persist_callback_error( + result, + format!( + "changeset-begin callback returned error code {result}; \ round aborted before any write" - ))); + ), + )); } } - let mut round_success = true; + let mut outcome = RoundOutcome::default(); // Wallet-registration metadata. Fires at most once per round // (registration emits the entry; subsequent rounds carry @@ -1593,7 +1760,7 @@ impl PlatformWalletPersistence for FFIPersister { "Wallet metadata persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1630,12 +1797,12 @@ impl PlatformWalletPersistence for FFIPersister { "Account registrations persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } Err(e) => { eprintln!("Failed to encode account registration specs: {}", e); - round_success = false; + outcome.record_fatal(); } } } @@ -1667,12 +1834,12 @@ impl PlatformWalletPersistence for FFIPersister { "Account address pools persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } Err(e) => { eprintln!("Failed to encode account address pool entries: {}", e); - round_success = false; + outcome.record_fatal(); } } } @@ -1707,7 +1874,7 @@ impl PlatformWalletPersistence for FFIPersister { "Address balance persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1747,12 +1914,12 @@ impl PlatformWalletPersistence for FFIPersister { "Derived-address persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } Err(e) => { eprintln!("Failed to encode derived address pool entries: {}", e); - round_success = false; + outcome.record_fatal(); } } } @@ -1790,12 +1957,12 @@ impl PlatformWalletPersistence for FFIPersister { "Marked-used address persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } Err(e) => { eprintln!("Failed to encode marked-used address pool entries: {}", e); - round_success = false; + outcome.record_fatal(); } } } @@ -1810,7 +1977,7 @@ impl PlatformWalletPersistence for FFIPersister { "Wallet changeset persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } @@ -1831,7 +1998,7 @@ impl PlatformWalletPersistence for FFIPersister { error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1869,7 +2036,7 @@ impl PlatformWalletPersistence for FFIPersister { "Wallet changeset sweeps persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1914,7 +2081,7 @@ impl PlatformWalletPersistence for FFIPersister { "Identity changeset persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1953,7 +2120,7 @@ impl PlatformWalletPersistence for FFIPersister { "DashPay payment persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -1999,7 +2166,7 @@ impl PlatformWalletPersistence for FFIPersister { "Identity keys changeset persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2050,7 +2217,7 @@ impl PlatformWalletPersistence for FFIPersister { "Token balance persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2095,7 +2262,7 @@ impl PlatformWalletPersistence for FFIPersister { "Asset lock persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2136,7 +2303,7 @@ impl PlatformWalletPersistence for FFIPersister { "Invitation persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2186,7 +2353,7 @@ impl PlatformWalletPersistence for FFIPersister { "DPNS name state persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2340,7 +2507,7 @@ impl PlatformWalletPersistence for FFIPersister { "Contact persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2367,7 +2534,7 @@ impl PlatformWalletPersistence for FFIPersister { "Sync state persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2420,7 +2587,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded notes persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2452,7 +2619,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded nullifier-spent persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2512,7 +2679,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded outgoing-notes persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2542,7 +2709,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded synced-index persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2588,7 +2755,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded viewing-key persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } } } @@ -2702,7 +2869,7 @@ impl PlatformWalletPersistence for FFIPersister { "Shielded activity persistence callback returned error code {}", result ); - round_success = false; + outcome.record(result); } // `rows` and `entries` drop here, after the callback // has copied everything it needs. @@ -2711,25 +2878,39 @@ impl PlatformWalletPersistence for FFIPersister { } } - // Close the round. Clients use this to commit (if - // `round_success == true`) or roll back (otherwise) the + // Close the round. Clients use this to commit (if the round + // succeeded) or roll back (otherwise) the // staged writes accumulated across the per-kind callbacks // above, making the whole store() call a single atomic // transaction from their perspective. if let Some(cb) = self.callbacks.on_changeset_end_fn { - let result = unsafe { cb(self.callbacks.context, wallet_id.as_ptr(), round_success) }; + let result = unsafe { + cb( + self.callbacks.context, + wallet_id.as_ptr(), + outcome.is_success(), + ) + }; if result != 0 { eprintln!("Changeset-end callback returned error code {}", result); - // The end callback is where the client COMMITS the round (e.g. - // the SwiftData atomic `save()`). A non-zero return means the - // commit failed and the staged writes were rolled back — the - // round never reached durable storage. Treat it as a - // persistence failure so `store()` returns `Err` and the caller - // does NOT advance / clear its in-memory state (pending queues, + // Either way `store()` must return `Err`, so the caller does + // NOT advance / clear its in-memory state (pending queues, // cleared drain entries, ignored-sender deltas) against data - // that was dropped. Otherwise the failure is silent and the - // dropped writes resurface or are lost with no signal. - round_success = false; + // that never reached durable storage. What differs is what the + // caller may then do about it. + if outcome.is_success() { + // A clean round: `end` is the COMMIT (the SwiftData atomic + // `save()`), and its failure applied nothing. The host's + // classification of that is legitimate. + outcome.record(result); + } else { + // A failing round: `end` fired with `success = false` to + // drive the ROLLBACK, and it failed. The round's + // disposition is now unknown, whatever the host classifies + // it as — and unknown is never retryable, because a + // re-issued changeset merges by appending. + outcome.record_fatal(); + } } } @@ -2742,8 +2923,9 @@ impl PlatformWalletPersistence for FFIPersister { // which cannot happen here since `begin_round` succeeded above.) round.end_round()?; - if !round_success { - return Err(PersistenceError::backend( + if let Some(kind) = outcome.failure_kind() { + return Err(PersistenceError::backend_with_kind( + self.reportable_round_kind(kind), "one or more persistence callbacks failed; changeset was rolled back", )); } @@ -2762,9 +2944,13 @@ impl PlatformWalletPersistence for FFIPersister { ignored" ); } else { - return Err(PersistenceError::backend(format!( - "Persistence store callback returned error code {result}" - ))); + // No end callback, so the per-kind writes already landed + // individually and the round is not all-or-nothing — + // `reportable_round_kind` withholds a retryable verdict. + return Err(PersistenceError::backend_with_kind( + self.reportable_round_kind(persist_rc_kind(result)), + format!("Persistence store callback returned error code {result}"), + )); } } } @@ -2773,19 +2959,16 @@ impl PlatformWalletPersistence for FFIPersister { } fn flush(&self, wallet_id: WalletId) -> Result<(), PersistenceError> { - // TODO: deferred — FFI callback failures are classified as - // `Fatal` (no transient-retry signal across the C ABI), and - // trailing-byte validation on decoded FFI payloads is not yet - // applied here. Both are tracked for a follow-up; no behavior - // change in this change. + // TODO: deferred — trailing-byte validation on decoded FFI + // payloads is not yet applied here. // Notify caller. if let Some(cb) = self.callbacks.on_flush_fn { let result = unsafe { cb(self.callbacks.context, wallet_id.as_ptr()) }; if result != 0 { - return Err(PersistenceError::backend(format!( - "Persistence flush callback returned error code {}", - result - ))); + return Err(persist_callback_error( + result, + format!("Persistence flush callback returned error code {}", result), + )); } } @@ -2807,10 +2990,10 @@ impl PlatformWalletPersistence for FFIPersister { let mut count: usize = 0; let rc = unsafe { load_cb(self.callbacks.context, &mut entries_ptr, &mut count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_wallet_list_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!("on_load_wallet_list_fn returned error code {}", rc), + )); } let _guard = LoadGuard { context: self.callbacks.context, @@ -2898,10 +3081,10 @@ impl PlatformWalletPersistence for FFIPersister { let rc = unsafe { load_notes(self.callbacks.context, &mut notes_ptr, &mut notes_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_notes_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!("on_load_shielded_notes_fn returned error code {}", rc), + )); } struct NotesGuard { context: *mut c_void, @@ -2963,10 +3146,13 @@ impl PlatformWalletPersistence for FFIPersister { let rc = unsafe { load_outgoing(self.callbacks.context, &mut out_ptr, &mut out_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_outgoing_notes_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!( + "on_load_shielded_outgoing_notes_fn returned error code {}", + rc + ), + )); } struct OutgoingGuard { context: *mut c_void, @@ -3027,10 +3213,10 @@ impl PlatformWalletPersistence for FFIPersister { load_states(self.callbacks.context, &mut states_ptr, &mut states_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_sync_states_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!("on_load_shielded_sync_states_fn returned error code {}", rc), + )); } struct StatesGuard { context: *mut c_void, @@ -3085,10 +3271,10 @@ impl PlatformWalletPersistence for FFIPersister { let rc = unsafe { load_activity(self.callbacks.context, &mut act_ptr, &mut act_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_activity_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!("on_load_shielded_activity_fn returned error code {}", rc), + )); } struct ActivityGuard { context: *mut c_void, @@ -3248,10 +3434,13 @@ impl PlatformWalletPersistence for FFIPersister { load_viewing_keys(self.callbacks.context, &mut vk_ptr, &mut vk_count) }; if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_load_shielded_viewing_keys_fn returned error code {}", - rc - ))); + return Err(persist_callback_error( + rc, + format!( + "on_load_shielded_viewing_keys_fn returned error code {}", + rc + ), + )); } struct ViewingKeysGuard { context: *mut c_void, @@ -3408,6 +3597,13 @@ impl PlatformWalletPersistence for FFIPersister { }; if rc != 0 { + // TODO(tx-record-read-sentinels): `on_get_core_tx_record_fn` + // collapses every non-zero return, sentinels included, into a + // miss, so the transient-vs-permanent read distinction is + // unreachable from FFI hosts. Deferred deliberately: converting it + // to `persist_callback_error` and letting + // `get_core_tx_record_or_transient_miss` do the collapsing is a + // behaviour change for every host on the current contract. tracing::debug!( txid = %txid, rc, @@ -3561,9 +3757,10 @@ impl PlatformWalletPersistence for FFIPersister { // free a buffer the host still owns on the failure path, which is a // double free for any host that cleans up its own failed allocation. if rc != 0 { - return Err(PersistenceError::backend(format!( - "on_list_wallet_core_txids_fn returned non-zero status {rc}" - ))); + return Err(persist_callback_error( + rc, + format!("on_list_wallet_core_txids_fn returned non-zero status {rc}"), + )); } // Success: ownership is ours now, and every return below must release @@ -4725,6 +4922,13 @@ fn build_wallet_start_state( // 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. + // + // INTENTIONAL(unmaintained-bincode-decoder): the three account-xpub + // decodes below run bincode 2.0.1 (RUSTSEC-2025-0141: development + // ceased, no CVE, no fix version) over host-supplied bytes. Accepted: + // no defect today, and the migration is tracked as its own + // supply-chain item, to be paired with the trailing-byte validation + // the `flush` decode boundary already defers. match account_type { AccountType::ProviderOperatorKeys => { let (bls_pubkey, _): (ExtendedBLSPubKey, usize) = @@ -5383,6 +5587,10 @@ fn build_unused_asset_locks( // SAFETY: Same lifetime contract as `transaction_bytes`. let proof_bytes = unsafe { slice::from_raw_parts(spec.proof_bytes, spec.proof_bytes_len) }; + // INTENTIONAL(unmaintained-bincode-decoder): host-supplied bytes + // through bincode 2.0.1 (RUSTSEC-2025-0141, unmaintained). Same + // accepted risk as the account-xpub decodes in + // `build_wallet_start_state`. let (proof, _) = dpp::bincode::decode_from_slice::( proof_bytes, config::standard(), @@ -8923,6 +9131,320 @@ mod tests { unsafe { free_contact_requests_ffi(rows.as_mut_ptr(), rows.len()) }; } + // ── Inbound retry classification from host return codes ── + + /// Returns the "retryable, nothing applied" sentinel. + extern "C" fn transient_metadata( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _network: FFINetwork, + _wallet_group_id: *const u8, + _birth_height: u32, + ) -> i32 { + PLATFORM_WALLET_PERSIST_RC_TRANSIENT + } + + /// Returns the constraint sentinel. + extern "C" fn constraint_metadata( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _network: FFINetwork, + _wallet_group_id: *const u8, + _birth_height: u32, + ) -> i32 { + PLATFORM_WALLET_PERSIST_RC_CONSTRAINT + } + + /// Returns a plain non-zero value, as a host on the original contract does. + extern "C" fn unclassified_metadata( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _network: FFINetwork, + _wallet_group_id: *const u8, + _birth_height: u32, + ) -> i32 { + 7 + } + + extern "C" fn ok_begin(_ctx: *mut TestCVoid, _wallet_id: *const u8) -> i32 { + 0 + } + + extern "C" fn ok_end(_ctx: *mut TestCVoid, _wallet_id: *const u8, _success: bool) -> i32 { + 0 + } + + /// Succeeds, so the round's only failure is the one the test drives. + extern "C" fn ok_metadata( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _network: FFINetwork, + _wallet_group_id: *const u8, + _birth_height: u32, + ) -> i32 { + 0 + } + + /// One payload: the metadata entry whose callback each test drives. + fn metadata_changeset() -> PlatformWalletChangeSet { + PlatformWalletChangeSet { + wallet_metadata: Some(platform_wallet::changeset::WalletMetadataEntry { + network: Network::Testnet, + wallet_group_id: [1u8; 32], + birth_height: 1, + }), + ..PlatformWalletChangeSet::default() + } + } + + /// Persister with metadata callback `metadata`, optionally bracketing + /// rounds and attesting atomicity. + fn store_failing_persister( + metadata: unsafe extern "C" fn( + *mut TestCVoid, + *const u8, + FFINetwork, + *const u8, + u32, + ) -> i32, + bracketed: bool, + capabilities: PersistenceCapabilities, + ) -> FFIPersister { + let callbacks = PersistenceCallbacks { + on_persist_wallet_metadata_fn: Some(metadata), + on_changeset_begin_fn: bracketed.then_some(ok_begin as _), + on_changeset_end_fn: bracketed.then_some(ok_end as _), + ..PersistenceCallbacks::default() + }; + FFIPersister::new_with_persistence_capabilities(callbacks, capabilities) + } + + fn store_error_kind(persister: &FFIPersister) -> Option { + persister + .store([1u8; 32], metadata_changeset()) + .expect_err("the metadata callback fails every round here") + .kind() + } + + /// The point of the inbound direction: a host that sees its own + /// `SQLITE_BUSY` can say so, and the caller learns it may retry. + #[test] + fn transient_sentinel_reaches_the_caller_from_an_atomic_round() { + let persister = store_failing_persister( + transient_metadata, + true, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Transient) + ); + } + + /// A transient verdict invites re-sending the WHOLE changeset, and + /// changeset vectors merge by appending — so without an all-or-nothing + /// round the re-send would duplicate rows. The verdict is withheld. + #[test] + fn transient_sentinel_is_withheld_when_the_round_is_not_atomic() { + // Brackets wired, but the host never attested atomicity. + let unattested = + store_failing_persister(transient_metadata, true, PersistenceCapabilities::NONE); + assert_eq!( + store_error_kind(&unattested), + Some(PersistenceErrorKind::Fatal), + "an unattested round must not invite a retry" + ); + + // Attested, but with no round brackets to roll anything back — the + // structural half of ATOMIC_CHANGESETS is missing. + let unbracketed = store_failing_persister( + transient_metadata, + false, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&unbracketed), + Some(PersistenceErrorKind::Fatal), + "an attestation without begin/end brackets must not invite a retry" + ); + } + + /// `Constraint` invites no retry, so it passes through either way. + #[test] + fn constraint_sentinel_survives_whether_or_not_the_round_is_atomic() { + for (bracketed, capabilities) in [ + (true, PersistenceCapabilities::ATOMIC_CHANGESETS), + (false, PersistenceCapabilities::NONE), + ] { + let persister = store_failing_persister(constraint_metadata, bracketed, capabilities); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Constraint) + ); + } + } + + /// Back-compatibility: a plain non-zero value keeps its conservative + /// reading. + #[test] + fn unclassified_non_zero_return_stays_fatal() { + let persister = store_failing_persister( + unclassified_metadata, + true, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Fatal) + ); + } + + /// The round reports the most severe kind any callback returned: here the + /// commit fails unclassified after a per-kind callback said transient. + #[test] + fn a_fatal_callback_masks_a_transient_sibling() { + extern "C" fn fatal_end( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _success: bool, + ) -> i32 { + 7 + } + + let callbacks = PersistenceCallbacks { + on_persist_wallet_metadata_fn: Some(transient_metadata), + on_changeset_begin_fn: Some(ok_begin), + on_changeset_end_fn: Some(fatal_end), + ..PersistenceCallbacks::default() + }; + let persister = FFIPersister::new_with_persistence_capabilities( + callbacks, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Fatal), + "a transient sibling must not soften the round's fatal verdict" + ); + } + + /// `end` fires with `success = false` to drive the rollback, so a failure + /// there is a failure to UNDO. The round's disposition is then unknown, + /// and unknown is never retryable — whatever the host classifies it as. + #[test] + fn a_failed_rollback_is_never_reported_as_retryable() { + extern "C" fn transient_end( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _success: bool, + ) -> i32 { + PLATFORM_WALLET_PERSIST_RC_TRANSIENT + } + + let callbacks = PersistenceCallbacks { + on_persist_wallet_metadata_fn: Some(transient_metadata), + on_changeset_begin_fn: Some(ok_begin), + on_changeset_end_fn: Some(transient_end), + ..PersistenceCallbacks::default() + }; + let persister = FFIPersister::new_with_persistence_capabilities( + callbacks, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Fatal), + "a rollback the host could not complete must not invite a re-send" + ); + } + + /// The other side of that coin: on an otherwise-clean round `end` is the + /// COMMIT, nothing was applied when it fails, and the host's transient + /// classification is legitimate. + #[test] + fn a_transient_commit_failure_on_a_clean_round_stays_retryable() { + extern "C" fn transient_end( + _ctx: *mut TestCVoid, + _wallet_id: *const u8, + _success: bool, + ) -> i32 { + PLATFORM_WALLET_PERSIST_RC_TRANSIENT + } + + let callbacks = PersistenceCallbacks { + on_persist_wallet_metadata_fn: Some(ok_metadata), + on_changeset_begin_fn: Some(ok_begin), + on_changeset_end_fn: Some(transient_end), + ..PersistenceCallbacks::default() + }; + let persister = FFIPersister::new_with_persistence_capabilities( + callbacks, + PersistenceCapabilities::ATOMIC_CHANGESETS, + ); + assert_eq!( + store_error_kind(&persister), + Some(PersistenceErrorKind::Transient), + "a failed commit that applied nothing keeps the host's classification" + ); + } + + /// A load either happened or did not, so it carries the host's + /// classification with no atomicity precondition. + #[test] + fn transient_sentinel_reaches_the_caller_from_a_load() { + extern "C" fn transient_load( + _ctx: *mut TestCVoid, + _out_entries: *mut *const WalletRestoreEntryFFI, + _out_count: *mut usize, + ) -> i32 { + PLATFORM_WALLET_PERSIST_RC_TRANSIENT + } + + let callbacks = PersistenceCallbacks { + on_load_wallet_list_fn: Some(transient_load), + ..PersistenceCallbacks::default() + }; + let err = FFIPersister::new(callbacks) + .load() + .expect_err("the load callback fails"); + assert_eq!(err.kind(), Some(PersistenceErrorKind::Transient)); + } + + /// The sentinels must stay off the values a host already returns. + #[test] + fn sentinel_values_are_pinned_including_the_accepted_resolver_overlap() { + // The persistence family's own established values: success, the + // generic failure hosts already return, and -1. + for taken in [0, 1, -1] { + assert_ne!(PLATFORM_WALLET_PERSIST_RC_TRANSIENT, taken); + assert_ne!(PLATFORM_WALLET_PERSIST_RC_CONSTRAINT, taken); + } + assert_ne!( + PLATFORM_WALLET_PERSIST_RC_TRANSIENT, + PLATFORM_WALLET_PERSIST_RC_CONSTRAINT + ); + + // The mnemonic-resolver callback family in `rs-unified-sdk-jni` + // (`src/mnemonic.rs`), copied because that crate is not a dependency + // here. The persistence sentinels deliberately reuse those integers: + // the two vtables share no call path, and renumbering a published C + // ABI to avoid a resemblance costs every host a migration. Hosts are + // kept off the literals by named constants on both sides + // (`NativePersistenceBridge.PERSIST_RC_*`, `PlatformWalletPersistRC`). + // These assertions are the tripwire: renumbering either family fires + // this test so the decision is re-read rather than re-derived. + const RESOLVE_NOT_FOUND: i32 = -1; + const RESOLVE_BUFFER_TOO_SMALL: i32 = -2; + const RESOLVE_OTHER: i32 = -3; + assert_eq!( + PLATFORM_WALLET_PERSIST_RC_TRANSIENT, + RESOLVE_BUFFER_TOO_SMALL + ); + assert_eq!(PLATFORM_WALLET_PERSIST_RC_CONSTRAINT, RESOLVE_OTHER); + assert_ne!(PLATFORM_WALLET_PERSIST_RC_TRANSIENT, RESOLVE_NOT_FOUND); + assert_ne!(PLATFORM_WALLET_PERSIST_RC_CONSTRAINT, RESOLVE_NOT_FOUND); + } + // ── Round serialization + defensive state machine (dashpay/platform#4069) ── use std::os::raw::c_void as TestCVoid; diff --git a/packages/rs-platform-wallet/src/changeset/core_bridge.rs b/packages/rs-platform-wallet/src/changeset/core_bridge.rs index 83421a9d831..9b20a1658dc 100644 --- a/packages/rs-platform-wallet/src/changeset/core_bridge.rs +++ b/packages/rs-platform-wallet/src/changeset/core_bridge.rs @@ -34,7 +34,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, Weak}; use dashcore::blockdata::transaction::{txout::TxOut, OutPoint}; use key_wallet::account::AccountType; @@ -224,7 +224,15 @@ impl std::fmt::Display for BatchDiagnostics { /// The `receiver` is the manager's lossless persistence receiver, taken once /// via `take_persistence_receiver()` before the manager is published to /// producers, and handed to this function. Exits when `cancel` fires or the -/// persistence channel's sender (the manager) is dropped. +/// persistence channel's sender (the manager) is dropped, in both cases after +/// committing what the exiting drain had consumed — never mid-batch. +/// +/// A drain commits the whole backlog only while the persister outlives it, +/// which is what [`PlatformWalletManager::shutdown`](crate::PlatformWalletManager::shutdown) +/// guarantees and a dirty drop does not: the task claims the persister when it +/// wakes, so a claim that finds it already released exits with the backlog +/// uncommitted (re-derived by the next SPV pass — the watermark rides the same +/// `store()` as the rows it implies). /// /// `sync_fault` is the host-visible hard-fault latch: the task sets it /// (and never clears it) the first time it freezes a durable watermark, so @@ -232,12 +240,17 @@ impl std::fmt::Display for BatchDiagnostics { /// than silently re-freezing on the next launch. /// /// Generic over `P` so the spawned task gets static-dispatch on -/// every `persister.store(...)` call. Pass the manager's own -/// `Arc

` (not the `Arc` -/// coercion) to actually realize the static-dispatch win. +/// every `persister.store(...)` call. Pass a `Weak` to the manager's own +/// `Arc

` (not to the `Arc` coercion) to +/// actually realize the static-dispatch win. +/// +/// The reference is **weak**: the task holds nothing while parked for the next +/// event, so the persister is released when its owner drops rather than when +/// this task next polls. It upgrades once per drain — before consuming +/// anything — and keeps that claim until the drain's backlog is committed. pub fn spawn_wallet_event_adapter

( wallet_manager: Arc>>, - persister: Arc

, + persister: Weak

, receiver: mpsc::UnboundedReceiver, sync_fault: Arc, cancel: CancellationToken, @@ -307,7 +320,7 @@ where /// show a hard "verification failed / rescan pending" state. async fn run_wallet_event_adapter

( wallet_manager: Arc>>, - persister: Arc

, + persister: Weak

, mut receiver: mpsc::UnboundedReceiver, sync_fault: Arc, cancel: CancellationToken, @@ -325,15 +338,36 @@ async fn run_wallet_event_adapter

( // One-shot latch so the hard "watermark frozen" line hits logcat exactly // once per session rather than once per faulted batch. let freeze_logged = Arc::new(AtomicBool::new(false)); + // The claim carried between the chunks of one cancellation drain. Empty + // while a commit is in flight — the claim rides into the blocking task and + // back out — and released before the task parks for the next event, since + // an idle adapter must hold nothing (issue #4133). + let mut drain_persister: Option> = None; loop { // Block for the first event of a batch. Everything already sitting in // the channel behind it is folded in below without another await, so a // burst costs one `store()` per wallet instead of one per event (see // [`ADAPTER_STORE_BATCH_LIMIT`]). - let first = tokio::select! { - recv = receiver.recv() => recv, - _ = cancel.cancelled() => break, + let first = if cancel.is_cancelled() { + // Shutting down: commit the backlog, never wait for more. The + // `select!` below would race the fired token against `recv` and + // drop it. The claim carries across these chunks, so a backlog + // larger than one batch cannot lose its tail to a chunk boundary. + match receiver.try_recv() { + Ok(event) => Some(event), + Err(_) => break, + } + } else { + // About to park with nothing consumed: hold no strong reference, + // or a dropped manager's store stays open until this task next + // polls (issue #4133). + drain_persister = None; + tokio::select! { + recv = receiver.recv() => recv, + // Re-enter above to drain the backlog before exiting. + _ = cancel.cancelled() => continue, + } }; // `recv()` on an mpsc returns `None` only when every sender (the @@ -345,6 +379,35 @@ async fn run_wallet_event_adapter

( break; }; + // Claim the persister before folding anything else off the channel, so + // everything this drain consumes is guaranteed a commit: an owner + // releasing its `Arc` mid-drain can no longer strand events this task + // has already taken. Claiming after the fold left a window as wide as + // the fold itself in which a whole batch became uncommittable. + // + // The one event already in hand is the irreducible remainder: an + // adapter that holds nothing while parked cannot claim before it wakes, + // and by then the persister may be gone. Nothing durable breaks — the + // watermark rides the same `store()` as the rows it implies, so the + // next SPV pass re-derives both. + // + // Taken, never cloned: the claim MOVES into the commit below and comes + // back out with the diagnostics, so a commit in flight is still the one + // and only strong reference a dropped manager has to wait on (#4133). + let persister_for_commit = match drain_persister.take() { + Some(claimed) => claimed, + None => match persister.upgrade() { + Some(claimed) => claimed, + None => { + tracing::warn!( + "persister already released when the wallet-event adapter woke; \ + exiting with the backlog uncommitted — the next scan re-derives it" + ); + break; + } + }, + }; + let mut batch: BTreeMap = BTreeMap::new(); let mut closed = false; { @@ -438,7 +501,6 @@ async fn run_wallet_event_adapter

( // accounted for" and "nobody knows". let settled: Arc>> = Arc::new(Mutex::new(Vec::new())); let settled_for_commit = Arc::clone(&settled); - let persister_for_commit = Arc::clone(&persister); let sync_fault_for_commit = Arc::clone(&sync_fault); let fault_for_commit = Arc::clone(&fault); let freeze_for_commit = Arc::clone(&freeze_logged); @@ -456,7 +518,7 @@ async fn run_wallet_event_adapter

( // `commit_batch` returns is lost when a later store in the same // batch panics, and the panic branch would then emit the one-shot // marker a second time for a freeze already announced. - commit_batch( + let diag = commit_batch( &*persister_for_commit, batch, folded, @@ -464,12 +526,20 @@ async fn run_wallet_event_adapter

( &sync_fault_for_commit, &freeze_for_commit, &mut settled, - ) + ); + // Hand the claim back out: the next chunk of a cancellation drain + // inherits it instead of racing a fresh upgrade against the owner's + // release. A panicking `commit_batch` drops it instead, and the + // next chunk re-claims. + (persister_for_commit, diag) }) .await; let diag = match committed { - Ok(diag) => diag, + Ok((claimed, diag)) => { + drain_persister = Some(claimed); + diag + } // The commit thread panicked, so `commit_batch` never reached the // `store()` rejection arm that would have frozen the affected // wallets. Freeze them here instead. @@ -3357,7 +3427,7 @@ mod tests { // lossless burst, a rejected `store()`, the per-wallet freeze, and // per-wallet batch folding. - use super::{run_wallet_event_adapter, AdapterFaultState}; + use super::{run_wallet_event_adapter, AdapterFaultState, ADAPTER_STORE_BATCH_LIMIT}; use crate::changeset::changeset::PlatformWalletChangeSet; use crate::changeset::client_start_state::ClientStartState; use crate::changeset::traits::{PersistenceError, PlatformWalletPersistence}; @@ -3551,7 +3621,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3587,6 +3657,132 @@ mod tests { ); } + /// A cancelled adapter commits the backlog already in the channel before + /// exiting, instead of racing the token against `recv` and discarding + /// whatever the producer had already handed to the lossless channel. + /// + /// The persister outlives the drain here, which is the `shutdown()` shape: + /// a joined shutdown holds the manager — and therefore the persister — + /// alive for as long as the drain it triggered. A dirty `Drop` gives no + /// such guarantee; see the `Drop` rustdoc on `PlatformWalletManager`. + #[tokio::test] + async fn cancellation_commits_the_events_already_buffered() { + let wallet_id = [11u8; 32]; + let (tx, rx) = unbounded_channel::(); + tx.send(sync_height_event(wallet_id, 41)).unwrap(); + tx.send(sync_height_event(wallet_id, 42)).unwrap(); + + let (obs_tx, mut obs_rx) = unbounded_channel(); + let persister = Arc::new(ProbePersister::new(obs_tx)); + let cancel = CancellationToken::new(); + // Already cancelled when the loop starts: the shape a cancelled + // manager leaves behind. + cancel.cancel(); + + run_wallet_event_adapter( + test_manager(), + Arc::downgrade(&persister), + rx, + Arc::new(AtomicBool::new(false)), + cancel, + ) + .await; + + let observed = obs_rx + .try_recv() + .expect("a cancelled adapter must still commit the buffered backlog"); + assert_eq!(observed.wallet_id, wallet_id); + assert_eq!( + observed.synced_height, + Some(42), + "both buffered events belong to the same drain" + ); + assert!( + obs_rx.try_recv().is_err(), + "the drain stops at the backlog it found, and never waits for more" + ); + // Held to the end so the exit is the cancel path, not a closed channel. + drop(tx); + } + + /// A drain owns the persister until its whole backlog is committed, so a + /// chunk boundary is not a loss boundary. + /// + /// A backlog larger than [`ADAPTER_STORE_BATCH_LIMIT`] is committed in + /// several chunks. Claiming the persister only after a chunk has folded + /// its events makes the first chunk's commit release the last strong + /// reference, and the next chunk then finds nothing to commit to — after + /// it has already taken its events off the lossless channel. The owner + /// releasing its `Arc` mid-drain (what `Drop` does) is exactly the + /// interleaving that exposes it. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_cancelled_drain_holds_the_persister_until_its_backlog_is_committed() { + use std::time::{Duration, Instant}; + + let wallet_id = [0x55u8; 32]; + // One past the limit: the tail event cannot ride the first chunk. + let backlog = ADAPTER_STORE_BATCH_LIMIT as u32 + 1; + let (tx, rx) = unbounded_channel::(); + for height in 1..=backlog { + tx.send(sync_height_event(wallet_id, height)).unwrap(); + } + + let (obs_tx, mut obs_rx) = unbounded_channel(); + let persister = Arc::new(ProbePersister::new(obs_tx)); + let (release, blocked) = persister.block_next(); + let probe = Arc::downgrade(&persister); + let cancel = CancellationToken::new(); + cancel.cancel(); + let handle = tokio::spawn(run_wallet_event_adapter( + test_manager(), + Arc::downgrade(&persister), + rx, + Arc::new(AtomicBool::new(false)), + cancel, + )); + + // Park inside the first chunk's `store()`, then release the only + // strong reference outside the adapter — the manager's own drop, + // landing while the drain is under way. + let deadline = Instant::now() + Duration::from_secs(5); + while !blocked.load(Ordering::Relaxed) { + assert!( + Instant::now() < deadline, + "the first chunk's store must park before the drop below means anything" + ); + tokio::time::sleep(Duration::from_millis(10)).await; + } + drop(persister); + drop(release); + + let first = obs_rx + .recv() + .await + .expect("the first chunk of the backlog must commit"); + assert_eq!( + first.synced_height, + Some(ADAPTER_STORE_BATCH_LIMIT as u32), + "the first chunk folds up to the batch limit" + ); + let second = obs_rx.recv().await.expect( + "the chunk after the first must still commit: a drain owns the \ + persister until its backlog is on disk", + ); + assert_eq!( + second.synced_height, + Some(backlog), + "the tail of the backlog must reach the store, not the warn log" + ); + + handle.await.unwrap(); + assert!( + probe.upgrade().is_none(), + "a finished drain must release the persister it claimed" + ); + // Held to the end so the exit is the cancel path, not a closed channel. + drop(tx); + } + /// (c) A rejected `store()` faults the wallet, and the very next /// watermark-only event is stripped and dropped (not delivered). #[tokio::test] @@ -3600,7 +3796,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3654,7 +3850,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3702,7 +3898,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3749,7 +3945,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3801,7 +3997,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3848,7 +4044,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3935,7 +4131,7 @@ mod tests { let handle = runtime.spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -3979,6 +4175,70 @@ mod tests { }); } + /// The adapter upgrades its weak persister reference for exactly the span + /// of a batch commit — the sole bound on the manager's synchronous release, + /// since a drop racing a commit reclaims the persister only when the parked + /// `store()` returns (issue #4133). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn an_in_flight_commit_holds_a_strong_persister_reference() { + use std::time::{Duration, Instant}; + + let wallet_id = [0x44u8; 32]; + let (tx, rx) = unbounded_channel::(); + let (obs_tx, mut obs_rx) = unbounded_channel(); + let persister = Arc::new(ProbePersister::new(obs_tx)); + let (release, blocked) = persister.block_next(); + let sync_fault = Arc::new(AtomicBool::new(false)); + let cancel = CancellationToken::new(); + let handle = tokio::spawn(run_wallet_event_adapter( + test_manager(), + Arc::downgrade(&persister), + rx, + Arc::clone(&sync_fault), + cancel.clone(), + )); + + assert_eq!( + Arc::strong_count(&persister), + 1, + "an idle adapter must hold the persister weakly — only this test's \ + own reference may be strong" + ); + + // Park the commit inside `store()`, and wait until the park is in + // effect so the count below is read during the commit, not before it. + tx.send(block_processed_event(wallet_id, 10)).unwrap(); + let deadline = Instant::now() + Duration::from_secs(5); + while !blocked.load(Ordering::Relaxed) { + assert!( + Instant::now() < deadline, + "the store must actually park before the assertion below means anything" + ); + tokio::time::sleep(Duration::from_millis(10)).await; + } + assert_eq!( + Arc::strong_count(&persister), + 2, + "a commit in flight must hold the upgraded reference for the whole \ + of its store()" + ); + + drop(release); + obs_rx + .recv() + .await + .expect("the released store must complete"); + cancel.cancel(); + drop(tx); + handle.await.unwrap(); + + assert_eq!( + Arc::strong_count(&persister), + 1, + "the upgraded reference must be released with the finished commit" + ); + } + /// (i) A commit panic must punish exactly the wallets whose outcome it /// left unknown — no more, no less. /// @@ -4015,7 +4275,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -4116,7 +4376,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -4204,7 +4464,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -4278,7 +4538,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -4327,7 +4587,7 @@ mod tests { let cancel = CancellationToken::new(); let handle = tokio::spawn(run_wallet_event_adapter( test_manager(), - Arc::clone(&persister), + Arc::downgrade(&persister), rx, Arc::clone(&sync_fault), cancel.clone(), @@ -4456,7 +4716,7 @@ mod tests { let sync_fault = Arc::new(AtomicBool::new(false)); let handle = spawn_wallet_event_adapter( Arc::clone(&wallet_manager), - Arc::clone(&persister), + Arc::downgrade(&persister), event_rx, Arc::clone(&sync_fault), cancel.clone(), @@ -4590,7 +4850,7 @@ mod tests { let sync_fault = Arc::new(AtomicBool::new(false)); let handle = spawn_wallet_event_adapter( Arc::clone(&wallet_manager), - Arc::clone(&persister), + Arc::downgrade(&persister), event_rx, Arc::clone(&sync_fault), cancel.clone(), @@ -4858,7 +5118,7 @@ mod tests { let sync_fault = Arc::new(AtomicBool::new(false)); let handle = spawn_wallet_event_adapter( Arc::clone(&wallet_manager), - Arc::clone(&persister), + Arc::downgrade(&persister), event_rx, Arc::clone(&sync_fault), cancel.clone(), diff --git a/packages/rs-platform-wallet/src/changeset/traits.rs b/packages/rs-platform-wallet/src/changeset/traits.rs index 647f4d7ed33..de88211e8e3 100644 --- a/packages/rs-platform-wallet/src/changeset/traits.rs +++ b/packages/rs-platform-wallet/src/changeset/traits.rs @@ -38,23 +38,35 @@ pub struct ListedCoreTxid { /// /// The enum is intentionally NOT `#[non_exhaustive]`: adding a new /// kind MUST force every consumer match to update explicitly. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +/// +/// **Variants are declared in ascending severity, and the derived [`Ord`] IS +/// that severity order.** Aggregators that reduce several failures to the one +/// they report (the FFI round accumulator) compare kinds directly, so a new +/// kind must be inserted at its severity position, not appended. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum PersistenceErrorKind { - /// The persister reports the write was not committed and the - /// buffered state is preserved (e.g. `SQLITE_BUSY`, `SQLITE_FULL`, - /// `SQLITE_IOERR`, `SQLITE_NOMEM`). Callers MAY retry with - /// exponential backoff. + /// The backend reports a retryable condition (e.g. `SQLITE_BUSY`, + /// `SQLITE_FULL`, `SQLITE_IOERR`, `SQLITE_NOMEM`). + /// + /// Whether and how to retry is the caller's decision, but what a retry + /// may safely be depends on the operation. From + /// [`flush`](PlatformWalletPersistence::flush) the implementation may + /// keep the buffered changeset, and the retry is another `flush`. From + /// [`store`](PlatformWalletPersistence::store) it invites re-issuing the + /// whole changeset, which is only safe under + /// [`store_transient_is_reissuable`](PlatformWalletPersistence::store_transient_is_reissuable) + /// — an implementation that keeps the failed changeset must leave that + /// attestation `false` rather than soften its classification here. Transient, - /// The persister reports an unrecoverable failure (schema - /// corruption, logic bug, I/O error not covered by the transient - /// class). Callers MUST NOT retry — the buffered changeset is - /// gone and the same call will keep failing. - Fatal, /// SQL constraint / foreign-key / integrity violation. Distinct /// from `Fatal` so callers can distinguish "your data is wrong" /// (caller bug) from "the storage engine is unhappy" (operator / - /// infrastructure problem). Treated as fatal for retry purposes. + /// infrastructure problem). Not retryable. Constraint, + /// The persister reports an unrecoverable failure (schema + /// corruption, logic bug, I/O error not covered by the transient + /// class). Not retryable — the same call will keep failing. + Fatal, } /// Errors returned by a [`PlatformWalletPersistence`] backend. @@ -121,6 +133,18 @@ impl PersistenceError { } } + /// The same error reclassified, preserving the `source` chain. + /// + /// For narrowing a backend's honest classification to what a caller may + /// safely act on. [`Self::LockPoisoned`] carries no kind and is returned + /// unchanged. + pub fn with_kind(self, kind: PersistenceErrorKind) -> Self { + match self { + Self::LockPoisoned => Self::LockPoisoned, + Self::Backend { source, .. } => Self::Backend { kind, source }, + } + } + /// `true` if the error is a `Backend` whose kind is /// [`PersistenceErrorKind::Transient`]. `LockPoisoned`, `Fatal`, /// and `Constraint` all read as non-transient. @@ -226,6 +250,27 @@ pub trait PlatformWalletPersistence: Send + Sync { PersistenceCapabilities::NONE } + /// Whether a [`store`](Self::store) that failed with + /// [`PersistenceErrorKind::Transient`] leaves the caller free to re-issue + /// the identical changeset. + /// + /// Two things must both hold: the failed round applied nothing, AND the + /// implementation retained nothing of it. Atomicity alone is not enough — + /// a backend that buffers the changeset, fails the write transactionally + /// and then restores the buffer for its own later `flush` satisfies + /// "nothing was applied" while still holding a copy. Re-issuing into that + /// copy merges the changeset twice, and changeset vectors merge by + /// appending. Such a backend leaves this `false` and expects its retry + /// through [`flush`](Self::flush) instead. + /// + /// **Fail-closed:** the default is `false`, so an implementation that has + /// not considered the question reports its transient `store` failures as + /// non-retryable. Withholding a retry costs one lost opportunity; granting + /// it wrongly costs duplicated rows. + fn store_transient_is_reissuable(&self) -> bool { + false + } + /// Compatibility summary for older invitation callers. It is true when the /// backend attests atomic changesets plus durably persisted invitation rows /// and asset-lock funding indices. This does not attest restart hydration; @@ -282,13 +327,17 @@ pub trait PlatformWalletPersistence: Send + Sync { /// [`PersistenceError::Backend`] so callers can drive retry policy /// off [`PersistenceError::is_transient`]: /// - /// - **[`PersistenceErrorKind::Transient`]** — for the canonical - /// SQLite backend that's `SQLITE_BUSY` / `SQLITE_LOCKED` plus the - /// I/O-class codes `SQLITE_FULL` / `SQLITE_IOERR` / - /// `SQLITE_NOMEM`: the buffered changeset is - /// preserved (re-merged via the buffer's `restore` path so any - /// `store` that landed during the failed flush wins on LWW - /// fields), and the caller MAY retry with exponential backoff. + /// - **[`PersistenceErrorKind::Transient`]** — a retryable condition; + /// for the canonical SQLite backend `SQLITE_BUSY` / `SQLITE_LOCKED` + /// plus the I/O-class codes `SQLITE_FULL` / `SQLITE_IOERR` / + /// `SQLITE_NOMEM`, where the buffered changeset is preserved + /// (re-merged via the buffer's `restore` path so any `store` that + /// landed during the failed flush wins on LWW fields). The retry is + /// another `flush`: an implementation that keeps the buffer must NOT + /// also attest + /// [`store_transient_is_reissuable`](Self::store_transient_is_reissuable), + /// or a caller re-issuing the changeset merges it into the copy the + /// implementation kept. /// - **[`PersistenceErrorKind::Constraint`]** — SQL /// constraint / FK / integrity violation. Caller bug; the data /// is rejected by the schema. MUST NOT retry without changing @@ -541,3 +590,57 @@ pub trait PlatformWalletPersistence: Send + Sync { // (consistent error/report semantics across SQLite, file, and FFI // backends) is agreed. } + +#[cfg(test)] +mod tests { + use super::*; + + /// The severity ranking is the enum's declaration order, and the FFI round + /// accumulator reduces a round's failures by comparing kinds directly. A + /// re-sort would silently let a transient verdict mask a fatal sibling. + #[test] + fn kind_ordering_is_ascending_severity() { + assert!(PersistenceErrorKind::Transient < PersistenceErrorKind::Constraint); + assert!(PersistenceErrorKind::Constraint < PersistenceErrorKind::Fatal); + } + + /// Fail-closed: an implementation that never considered re-issuability + /// must not have its transient `store` failures read as retryable. + #[test] + fn reissue_attestation_defaults_to_fail_closed() { + struct BareMinimum; + impl PlatformWalletPersistence for BareMinimum { + 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()) + } + } + assert!(!BareMinimum.store_transient_is_reissuable()); + } + + /// Narrowing a verdict must not cost the caller the detail it needs to + /// report or downcast. + #[test] + fn with_kind_reclassifies_and_keeps_the_source() { + let narrowed = PersistenceError::backend_with_kind( + PersistenceErrorKind::Transient, + "simulated SQLITE_BUSY", + ) + .with_kind(PersistenceErrorKind::Fatal); + assert_eq!(narrowed.kind(), Some(PersistenceErrorKind::Fatal)); + assert!(narrowed.to_string().contains("simulated SQLITE_BUSY")); + assert!(PersistenceError::LockPoisoned + .with_kind(PersistenceErrorKind::Fatal) + .kind() + .is_none()); + } +} diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index d3ab952c93d..7b4aca99c1f 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -14,6 +14,41 @@ pub enum PlatformWalletError { #[error("Wallet creation failed: {0}")] WalletCreation(String), + /// The persister failed to load the client start state during rehydration. + /// + /// Scope: emitted by manager rehydration (`load_from_persistor` and the + /// post-registration rehydration) and by the DashPay sent-payment + /// reconcile reads. The shielded-build reads still flatten their failure + /// into `ShieldedBuildError(String)`. + /// + /// This and the sibling `Persister*` variants carry their typed + /// [`PersistenceError`](crate::changeset::PersistenceError) rather than a + /// flattened string, so its retry classification survives — a transient + /// `SQLITE_BUSY` stays distinguishable from a permanent failure, in-crate + /// and across the C ABI (`platform-wallet-ffi` maps each variant and kind + /// to its own `PlatformWalletFFIResultCode`). They are separate variants + /// so a failed write is never reported as a failed read. + #[error("failed to load persisted client state: {0}")] + PersisterLoad(#[source] crate::changeset::PersistenceError), + + /// The persister failed to store the wallet-registration changeset. + /// See [`Self::PersisterLoad`] for why the typed cause is carried. + /// + /// Scope: wallet registration is the only write that reports this today. + /// A contact un-ignore flattens its failure into `Persistence(String)`, + /// the asset-lock pool write returns the raw `PersistenceError` on its own + /// signature, and the fire-and-forget writes (DPNS marketplace, platform + /// addresses, asset-lock tracking) log and swallow it. A host branching on + /// the classification gets it for registration and nowhere else yet. + #[error("failed to persist wallet registration changeset: {0}")] + PersisterStore(#[source] crate::changeset::PersistenceError), + + /// Restoring persisted platform-address state into a freshly registered + /// wallet failed. Boxed to break the recursion; the inner variant and its + /// `#[source]` chain survive intact. + #[error("failed to restore persisted platform-address state: {0}")] + PersisterRestore(#[source] Box), + #[error("Wallet not found: {0}")] WalletNotFound(String), @@ -907,6 +942,49 @@ pub enum PlatformWalletError { ShieldedNotBound, } +impl PlatformWalletError { + /// A persister `load` failed. + /// + /// There is deliberately no blanket `From`: the + /// conversion is undecidable from the value, because a `PersistenceError` + /// does not record whether a load, a store or a flush produced it, so an + /// inferred one would silently label failed writes as failed reads. Pick + /// the constructor naming the operation that actually failed. + pub fn from_load_failure(source: crate::changeset::PersistenceError) -> Self { + Self::PersisterLoad(source) + } + + /// A persister `store` failed. See [`Self::from_load_failure`] for why no + /// blanket conversion exists. + /// + /// `persister` is the one that failed: this is where the "transient means + /// nothing was committed, so re-issue it" promise is MADE — to the caller, + /// and across the C ABI as `ErrorPersisterStoreTransient` — so this is + /// where it is enforced. A `Transient` classification is narrowed to + /// `Fatal` unless the persister attests + /// [`store_transient_is_reissuable`](crate::changeset::PlatformWalletPersistence::store_transient_is_reissuable), + /// which is fail-closed. The `#[source]` chain survives the narrowing. + pub fn from_store_failure

(persister: &P, source: crate::changeset::PersistenceError) -> Self + where + P: crate::changeset::PlatformWalletPersistence + ?Sized, + { + use crate::changeset::PersistenceErrorKind; + let source = match source.kind() { + Some(PersistenceErrorKind::Transient) if !persister.store_transient_is_reissuable() => { + source.with_kind(PersistenceErrorKind::Fatal) + } + _ => source, + }; + Self::PersisterStore(source) + } + + /// Restoring persisted platform-address state failed. Boxes `source`, so + /// callers never write `Box::new`. + pub fn from_restore_failure(source: PlatformWalletError) -> Self { + Self::PersisterRestore(Box::new(source)) + } +} + /// Check whether an SDK error indicates that an InstantSend lock proof was /// rejected by Platform (e.g. the IS lock has expired). /// diff --git a/packages/rs-platform-wallet/src/manager/dashpay_sync.rs b/packages/rs-platform-wallet/src/manager/dashpay_sync.rs index ca35adb4dbc..2f08ef19e78 100644 --- a/packages/rs-platform-wallet/src/manager/dashpay_sync.rs +++ b/packages/rs-platform-wallet/src/manager/dashpay_sync.rs @@ -400,13 +400,16 @@ impl DashPaySyncManager { /// operation (each also has its own standalone on-demand FFI caller). /// /// The six steps run **independently** (log-and-continue) so a failure in - /// one does not skip the others. The two network *fetch* steps - /// (`sync_contact_requests`, `sync_profiles`) surface their first error so - /// the sweep can record this wallet as failed; the remaining steps - /// (contact profiles, contactInfo, the two payment reconciles) are - /// display- or local-only and never fail the pass. Contact requests run - /// first so freshly established contacts' accounts are registered before - /// the incoming-payment reconcile. + /// one does not skip the others. Four of them surface their first error + /// once every step has run, so the sweep can record this wallet as failed: + /// the two network *fetch* steps (`sync_contact_requests`, + /// `sync_profiles`), plus the two sent-payment reconciles, whose only + /// failure mode is a permanently unreadable persisted record — a store + /// fault the host has to hear about, not a display gap. The display-only + /// steps (contact profiles, contactInfo, the incoming reconcile, the + /// rescan backfill) never fail the pass. Contact requests run first so + /// freshly established contacts' accounts are registered before the + /// incoming-payment reconcile. async fn sync_wallet_dashpay( &self, wallet: &Arc, @@ -467,11 +470,11 @@ impl DashPaySyncManager { // wallet transaction history + the contact external-account // address pools. Runs after the incoming reconcile so an // existing received entry under the txid wins the dedup guard. - if let Err(e) = identity + let reconstruct_result = identity .dashpay() .reconcile_sent_payments_from_tx_history() - .await - { + .await; + if let Err(e) = &reconstruct_result { tracing::warn!( wallet_id = %hex::encode(wallet_id), error = %e, @@ -495,7 +498,8 @@ impl DashPaySyncManager { // Local-only: confirm `Pending` `Sent` payments the persisted core // record reports final (mined or InstantSend-locked). - if let Err(e) = identity.dashpay().reconcile_sent_payments().await { + let confirm_result = identity.dashpay().reconcile_sent_payments().await; + if let Err(e) = &confirm_result { tracing::warn!( wallet_id = %hex::encode(wallet_id), error = %e, @@ -503,9 +507,12 @@ impl DashPaySyncManager { ); } - // Surface the first fetch error (if any); both fetch steps have run. + // Surface the first error (if any); every step above has already run, + // so reporting one costs the others nothing. contact_result?; profile_result?; + reconstruct_result?; + confirm_result?; Ok(()) } } diff --git a/packages/rs-platform-wallet/src/manager/identity_sync.rs b/packages/rs-platform-wallet/src/manager/identity_sync.rs index e3b3a591dcd..54d2fd81af8 100644 --- a/packages/rs-platform-wallet/src/manager/identity_sync.rs +++ b/packages/rs-platform-wallet/src/manager/identity_sync.rs @@ -527,6 +527,13 @@ where drained } + /// Test-only: whether new sync passes are currently barred — a drain in + /// flight, a latched timeout, or the terminal seal `shutdown` applies. + #[cfg(test)] + pub(crate) fn sync_admission_closed(&self) -> bool { + self.quiescing.is_closed() + } + /// Run one sync pass across every registered identity. /// /// If a pass is already in flight, returns immediately without diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index c44d9cdb277..fcdbdcdd0dc 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::{retry_transient_load, PlatformWalletManager}; impl PlatformWalletManager

{ /// Load the full [`ClientStartState`] from the configured persister @@ -28,8 +28,40 @@ impl PlatformWalletManager

{ /// wallets missing from that slice get a fresh /// [`PlatformAddressWallet::initialize`](crate::wallet::platform_addresses::PlatformAddressWallet::initialize). /// + /// # Errors + /// + /// Returns [`PersisterLoad`](PlatformWalletError::PersisterLoad) when the + /// persister cannot produce the snapshot, and + /// [`PersisterRestore`](PlatformWalletError::PersisterRestore) when a + /// wallet in the snapshot cannot have its platform-address state rebuilt. + /// A persisted wallet whose id disagrees with its own key material, or one + /// the inner [`WalletManager`] refuses, is neither a read nor a restore + /// failure and stays + /// [`WalletCreation`](PlatformWalletError::WalletCreation). + /// + /// A transient read is retried in-crate, so a contended backend can block + /// this call for up to four times its busy timeout plus 140 ms of backoff + /// (≈20 s at SQLite's 5 s default). Call it off any UI thread. + /// + /// Any `Err` rolls back partial inserts and leaves the manager usable: fix + /// the store and call again, or reconstruct. Reconstructing over the same + /// path needs every strong persister reference released first. Dropping + /// the manager releases its own references; wallet handles, workers and + /// in-flight operations can retain others. [`shutdown`](Self::shutdown) + /// takes `&self`, so it cannot release the manager's own `Arc

`. + /// /// [`WalletManager`]: key_wallet_manager::WalletManager pub async fn load_from_persistor(&self) -> Result<(), PlatformWalletError> { + let persister = Arc::clone(&self.persister); + let start_state = match retry_transient_load(move || persister.load()).await { + Ok(state) => state, + Err(e) => { + // Debug, not Display: it carries the real cause (e.g. a + // bincode decode failure) rather than flattening the chain. + tracing::debug!(error = ?e, "persister load failed during rehydration"); + return Err(PlatformWalletError::from_load_failure(e)); + } + }; let ClientStartState { mut platform_addresses, wallets, @@ -37,12 +69,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 @@ -199,10 +226,10 @@ impl PlatformWalletManager

{ .initialize_from_persisted(persisted) .await { - load_error = Some(PlatformWalletError::WalletCreation(format!( - "Failed to restore platform address state: {}", - e - ))); + // Wrap the already-typed error rather than stringify it, so + // its concrete variant and source chain survive — the same + // shape `register_wallet` returns for this same failure. + load_error = Some(PlatformWalletError::from_restore_failure(e)); break 'load; } } else { @@ -371,7 +398,8 @@ mod idempotent_load_tests { ClientStartState, ClientWalletStartState, IdentityManagerStartState, PersistenceError, PlatformWalletChangeSet, PlatformWalletPersistence, }; - use crate::events::{EventHandler, PlatformEventHandler}; + use crate::events::PlatformEventHandler; + use crate::test_support::NoopTestEventHandler; use crate::wallet::platform_wallet::WalletId; use crate::PlatformWalletManager; @@ -458,15 +486,11 @@ mod idempotent_load_tests { } } - struct NoopEventHandler; - impl EventHandler for NoopEventHandler {} - impl PlatformEventHandler for NoopEventHandler {} - fn make_manager( persister: SingleWalletPersister, ) -> Arc> { let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); - let event_handler: Arc = Arc::new(NoopEventHandler); + let event_handler: Arc = Arc::new(NoopTestEventHandler); Arc::new(PlatformWalletManager::new( sdk, Arc::new(persister), @@ -583,7 +607,7 @@ mod idempotent_load_tests { let ctx = TestWalletContext::new_random(); let expected_id = ctx.wallet.compute_wallet_id(); let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); - let event_handler: Arc = Arc::new(NoopEventHandler); + let event_handler: Arc = Arc::new(NoopTestEventHandler); let manager = Arc::new(PlatformWalletManager::new( sdk, Arc::new(MismatchedSecondWalletPersister { @@ -614,3 +638,249 @@ mod idempotent_load_tests { ); } } + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + use dash_async::WorkerStatus; + + use super::*; + use crate::changeset::{PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet}; + use crate::events::PlatformEventHandler; + use crate::manager::WalletWorker; + use crate::test_support::NoopTestEventHandler; + + /// Strong `Arc

` clones a freshly built [`PlatformWalletManager`] holds: + /// its `persister` field, the `DashPayPaymentHandler`, and the + /// `IdentitySyncManager` — the wallet-event adapter deliberately excluded. + const MANAGER_PERSISTER_HOLDERS: usize = 3; + + /// Persister whose `load()` always fails. + 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()) + } + } + + /// Fails `load()` permanently once, then succeeds. + #[derive(Default)] + struct FatalOnceLoadPersister { + load_calls: AtomicUsize, + } + + impl PlatformWalletPersistence for FatalOnceLoadPersister { + 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("simulated fatal load failure")); + } + Ok(ClientStartState::default()) + } + } + + fn make_manager( + persister: Arc

, + ) -> PlatformWalletManager

{ + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let handler: Arc = Arc::new(NoopTestEventHandler); + PlatformWalletManager::new(sdk, persister, handler) + } + + #[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 manager = make_manager(persister); + + manager + .load_from_persistor() + .await + .expect("transient startup load failure must be retried"); + + assert_eq!(probe.load_calls.load(Ordering::SeqCst), 2); + } + + /// Isolating by construction: the count is read on a live, idle manager + /// with nothing dropped or aborted, so no teardown path can stand in for + /// the weak-reference property. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn adapter_holds_no_strong_persister_reference() { + let persister = Arc::new(FailingLoadPersister); + let probe = Arc::clone(&persister); + let _manager = make_manager(persister); + + assert_eq!( + Arc::strong_count(&probe), + MANAGER_PERSISTER_HOLDERS + 1, + "expected exactly {} strong persister references — the manager's \ + own `persister` field, the DashPayPaymentHandler on the event \ + fan-out, the IdentitySyncManager, and this test's probe. The idle \ + wallet-event adapter must not be among them: it holds a Weak

\ + and upgrades it per batch", + MANAGER_PERSISTER_HOLDERS + 1 + ); + } + + /// Running the manager-wide, one-way `shutdown()` on this failure path + /// seals every coordinator's admission gate and joins the wallet-event + /// adapter, so the retry returns `Ok(())` onto a manager that can never + /// sync or persist again (#4133). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn manager_stays_usable_after_a_failed_load() { + let manager = make_manager(Arc::new(FatalOnceLoadPersister::default())); + + let err = manager + .load_from_persistor() + .await + .expect_err("the first load must fail"); + assert!( + matches!(err, PlatformWalletError::PersisterLoad(_)), + "load failure must surface as the typed PersisterLoad variant, got {err:?}" + ); + + manager + .load_from_persistor() + .await + .expect("a load retried after a failed one must succeed"); + + assert!( + !manager.identity_sync_manager.sync_admission_closed(), + "a failed load must leave sync admission open — a sealed gate \ + makes every later `Ok(())` a lie" + ); + + // The adapter's receiver is taken exactly once, so a joined adapter + // cannot be respawned: `Ok` means the reused manager still persists. + let report = manager.shutdown().await; + assert_eq!( + report.per_worker.get(&WalletWorker::EventAdapter), + Some(&WorkerStatus::Ok), + "the wallet-event adapter must still have been running for \ + shutdown to join it: {report:?}" + ); + } + + /// Dropping the manager after a failed load releases the persister — the + /// precondition for reconstructing on the same path without a spurious + /// `WalletStorageError::AlreadyOpen` masking the real error (#4133). + /// + /// Isolates nothing: the count is the product of the whole teardown, so one + /// participant may regress while another still releases. + /// `adapter_holds_no_strong_persister_reference` pins the weak reference. + // TODO: cover the composed open -> failed load -> reopen from + // platform-wallet-storage; neither side asserts it today. + // 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 manager = make_manager(persister); + + 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:?}" + ); + assert_eq!( + Arc::strong_count(&probe), + MANAGER_PERSISTER_HOLDERS + 1, + "a failed load tears nothing down, so the manager's own references \ + must be exactly as they were before the call" + ); + + drop(manager); + assert_eq!( + Arc::strong_count(&probe), + 1, + "after a failed load and a drop nothing may still hold the persister" + ); + } + + /// A dirty drop releases the persister **synchronously**, bounded only by a + /// batch commit in flight (see + /// `an_in_flight_commit_holds_a_strong_persister_reference` in + /// `changeset::core_bridge`); the adapter is idle here. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn dropping_manager_releases_persister_synchronously_when_adapter_idle() { + let persister = Arc::new(FailingLoadPersister); + let probe = Arc::clone(&persister); + let manager = make_manager(persister); + assert_eq!( + Arc::strong_count(&probe), + MANAGER_PERSISTER_HOLDERS + 1, + "the manager must hold its persister before the drop for this to \ + mean anything" + ); + + // Dirty drop: `shutdown` is never called, so nothing joins the adapter. + drop(manager); + + assert_eq!( + Arc::strong_count(&probe), + 1, + "dropping the manager must release the persister immediately — an \ + idle adapter holds no strong reference to await" + ); + } +} diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index e95c712c338..4bdb13e0f5e 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -5,12 +5,15 @@ pub mod dashpay_sync; pub mod dpns_sync; pub mod identity_sync; mod load; +mod persist_retry; pub mod platform_address_sync; #[cfg(feature = "shielded")] pub mod shielded_sync; pub mod startup; mod wallet_lifecycle; +pub(crate) use persist_retry::retry_transient_load; + use std::sync::Arc; use std::time::Duration; @@ -504,7 +507,7 @@ impl PlatformWalletManager

{ let event_adapter_cancel = CancellationToken::new(); let event_adapter_join = spawn_wallet_event_adapter( Arc::clone(&wallet_manager), - Arc::clone(&persister), + Arc::downgrade(&persister), event_receiver, Arc::clone(&sync_fault), event_adapter_cancel.clone(), @@ -882,6 +885,10 @@ impl PlatformWalletManager

{ /// threads through the shared [`ThreadRegistry`] and finally drains the /// wallet-event adapter task. Idempotent. /// + /// Takes `&self`, so it cannot release the manager's own `Arc

` + /// persister — reopening the same store additionally needs the manager + /// dropped (see the [`Drop`] impl). + /// /// Ordering matters and is fourfold: /// 1. SPV is stopped and joined FIRST so it cannot dispatch more wallet /// events, then payment-task admission is closed and all admitted @@ -1051,6 +1058,36 @@ impl PlatformWalletManager

{ } } +/// Stops the wallet-event adapter task, which a dirty drop would otherwise +/// leave running against a torn-down manager. +/// +/// Dropping the manager releases its own persister references. The idle adapter +/// holds only a `Weak

`, but a drain, wallet handle, worker or in-flight read +/// can retain a strong reference. Reopening the same storage path must wait +/// until all such references are released; this drop does not guarantee it. +/// +/// **Buffered events are best-effort on this path.** Cancelling is all a `Drop` +/// can do: if dropping the fields releases the last `Arc

` before the adapter +/// claims it, the adapter exits with the backlog uncommitted. Nothing +/// durable breaks — a wallet's sync watermark rides the same `store()` as the +/// rows it implies, so the next SPV pass re-derives both. Use +/// [`shutdown`](PlatformWalletManager::shutdown) for a lossless drain: it holds +/// the manager, and with it the persister, alive while it joins the task, and +/// reports a status back. +/// +/// Having a `Drop` at all changes teardown for every holder of this public +/// type: a plain drop stops the adapter instead of detaching it, and the type's +/// fields can no longer be moved out. +impl Drop for PlatformWalletManager

{ + fn drop(&mut self) { + // Cancel and detach, never `abort`: the task observes the token at its + // next `recv` and exits after committing whatever its drain claimed the + // persister for. Aborting stops it at whatever await it is parked on, + // dropping a claimed batch mid-commit. + self.event_adapter_cancel.cancel(); + } +} + #[cfg(test)] mod tests { use super::*; @@ -1079,6 +1116,33 @@ mod tests { } } + /// Records the highest `synced_height` any `store()` carried, into state + /// held OUTSIDE the persister — so a test can read the outcome after the + /// persister itself has been released. + struct WatermarkPersister { + highest_synced_height: Arc, + } + + impl PlatformWalletPersistence for WatermarkPersister { + fn store( + &self, + _wallet_id: WalletId, + changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + if let Some(height) = changeset.core.as_ref().and_then(|core| core.synced_height) { + self.highest_synced_height + .fetch_max(height, std::sync::atomic::Ordering::SeqCst); + } + Ok(()) + } + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + fn load(&self) -> Result { + Ok(ClientStartState::default()) + } + } + struct NoopEventHandler; impl EventHandler for NoopEventHandler {} impl PlatformEventHandler for NoopEventHandler {} @@ -1219,6 +1283,74 @@ mod tests { assert!(again.all_clean(), "idempotent shutdown: {again:?}"); } + /// A joined `shutdown()` is the lossless drain: it keeps the manager — and + /// with it the persister — alive while the adapter finishes, so no + /// watermark buffered on the lossless channel is lost. A dirty `Drop` + /// promises nothing of the sort; see this type's `Drop` rustdoc. + /// + /// Drives the real manager and keeps NO strong `Arc

`: the outcome is + /// read from state that outlives the persister, so no reference the test + /// itself holds open can keep the drain's target alive for it. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_joined_shutdown_commits_the_watermarks_a_live_manager_buffered() { + use key_wallet::mnemonic::Mnemonic; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet_manager::WalletInterface; + + // Canonical all-`abandon` BIP-39 test vector. + const TEST_MNEMONIC: &str = "abandon abandon abandon abandon abandon abandon \ + abandon abandon abandon abandon abandon about"; + const TIP: u32 = 64; + + let highest_synced_height = Arc::new(std::sync::atomic::AtomicU32::new(0)); + let persister = Arc::new(WatermarkPersister { + highest_synced_height: Arc::clone(&highest_synced_height), + }); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let manager = PlatformWalletManager::new( + sdk, + persister, + Arc::new(NoopEventHandler) as Arc, + ); + + // `Some(0)` skips the SPV-tip birth-height lookup, so nothing here + // touches the network. + let seed = Mnemonic::from_phrase(TEST_MNEMONIC) + .expect("valid test mnemonic") + .to_seed(""); + let wallet_id = manager + .create_wallet_from_seed_bytes( + key_wallet::Network::Testnet, + &seed, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("register the test wallet") + .wallet_id(); + + // The upstream manager is the only producer on the lossless channel; + // a forward watermark advance is its cheapest event. + { + let mut wallet_manager = manager.wallet_manager.write().await; + for height in 1..=TIP { + wallet_manager.update_wallet_synced_height(&wallet_id, height); + } + } + + let report = manager.shutdown().await; + assert_eq!( + report.per_worker.get(&WalletWorker::EventAdapter), + Some(&WorkerStatus::Ok), + "the adapter must have been joined, not timed out: {report:?}" + ); + assert_eq!( + highest_synced_height.load(std::sync::atomic::Ordering::SeqCst), + TIP, + "a joined shutdown must commit every watermark the manager emitted" + ); + } + /// `reset_platform_address_sync_state` must fail closed when the /// in-flight pass does not drain: resetting watermarks and balances /// under a live pass would let that pass's tail re-persist the state @@ -1460,4 +1592,35 @@ mod tests { release_tx.send(()).expect("writer still parked"); writer.join().expect("writer thread completes"); } + + /// A dirty drop CANCELS the wallet-event adapter; it must never `abort` it. + /// An aborted task dies at whatever await it is parked on, taking with it + /// the events it had already pulled off the lossless channel — the loss the + /// channel exists to rule out. The parked sentinel below stands in for an + /// adapter mid-batch: it can only finish if the drop left it running. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn dropping_the_manager_does_not_abort_the_adapter_task() { + let manager = make_manager(); + + let (release_tx, release_rx) = tokio::sync::oneshot::channel::<()>(); + let (done_tx, done_rx) = tokio::sync::oneshot::channel::<()>(); + let sentinel = tokio::spawn(async move { + let _ = release_rx.await; + let _ = done_tx.send(()); + }); + // Park the sentinel where the real adapter's handle lives, so the drop + // path acts on it. The displaced adapter has its own cancel token and + // exits on its own. + manager.event_adapter_join.lock().await.replace(sentinel); + + drop(manager); + + release_tx + .send(()) + .expect("the adapter task was aborted on drop: its receiver is already gone"); + tokio::time::timeout(std::time::Duration::from_secs(5), done_rx) + .await + .expect("the parked adapter task never resumed after the manager was dropped") + .expect("the adapter task was aborted on drop: it never finished"); + } } diff --git a/packages/rs-platform-wallet/src/manager/persist_retry.rs b/packages/rs-platform-wallet/src/manager/persist_retry.rs new file mode 100644 index 00000000000..e990849a77c --- /dev/null +++ b/packages/rs-platform-wallet/src/manager/persist_retry.rs @@ -0,0 +1,79 @@ +//! Bounded retry for transient persister *reads*. +//! +//! Only `load` is retried in-crate: it is idempotent and the crate owns both +//! ends. A failed `store` propagates typed and kind-classified instead, and the +//! caller decides. +//! +//! Each attempt runs on the blocking pool; worst case per call is +//! `attempts × backend timeout + Σ backoff` (SQLite `busy_timeout` defaults to +//! 5 s). + +use std::sync::Arc; +use std::time::Duration; + +use crate::changeset::PersistenceError; + +/// Backoff before each retry of a transient `load` failure. Four total +/// attempts (the initial call plus one per entry). +pub(crate) const LOAD_RETRY_BACKOFF: [Duration; 3] = [ + Duration::from_millis(20), + Duration::from_millis(40), + Duration::from_millis(80), +]; + +/// Retry a synchronous persister `load` while it fails *transiently*, off the +/// async runtime, on the fixed [`LOAD_RETRY_BACKOFF`] schedule. +/// +/// `op` runs on the blocking pool once per attempt; success or a fatal error +/// returns immediately. A panic inside `op` propagates to the caller; a +/// cancelled attempt (runtime shutting down) surfaces as a backend error. +pub(crate) async fn retry_transient_load(op: F) -> Result +where + F: Fn() -> Result + Send + Sync + 'static, + T: Send + 'static, +{ + // The initial call, then one retry per backoff entry: the loop runs the + // schedule and the last attempt's result falls out of it, so there is no + // terminating sentinel and no escape hatch to panic through. + let op = Arc::new(op); + let mut outcome = load_attempt(&op).await; + for (retries_done, backoff) in LOAD_RETRY_BACKOFF.iter().enumerate() { + match outcome { + Ok(value) => return Ok(value), + Err(e) if e.is_transient() => { + tracing::debug!( + // 1-based, matching the schedule this module documents. + attempt = retries_done + 1, + backoff_ms = backoff.as_millis() as u64, + error = %e, + "transient persister load failure — retrying" + ); + tokio::time::sleep(*backoff).await; + outcome = load_attempt(&op).await; + } + Err(e) => return Err(e), + } + } + outcome +} + +/// Run one `load` attempt on the blocking pool. +// TODO(load-retry-holds-persister-strong-ref): an in-flight load retry holds a +// strong persister reference the caller cannot reclaim, contradicting the +// documented "only a batch commit holds one" relationship — `spawn_blocking` is +// uncancellable, so an abandoned caller's `Arc

` stays alive until the +// backend call returns. +async fn load_attempt(op: &Arc) -> Result +where + F: Fn() -> Result + Send + Sync + 'static, + T: Send + 'static, +{ + let call = Arc::clone(op); + match tokio::task::spawn_blocking(move || call()).await { + Ok(result) => result, + Err(join_err) if join_err.is_panic() => std::panic::resume_unwind(join_err.into_panic()), + Err(_cancelled) => Err(PersistenceError::backend( + "runtime shutting down before load retry", + )), + } +} diff --git a/packages/rs-platform-wallet/src/manager/startup.rs b/packages/rs-platform-wallet/src/manager/startup.rs index 7a83bc4787e..3ff36da277b 100644 --- a/packages/rs-platform-wallet/src/manager/startup.rs +++ b/packages/rs-platform-wallet/src/manager/startup.rs @@ -966,8 +966,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 — 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. @@ -985,12 +987,15 @@ impl PlatformWalletManager identity_scan_state: Some(recorded), ..Default::default() }; + // Single attempt, not retried — the outcome is logged and swallowed + // either way: an abandoned scan must not turn a shutdown into an error. if let Err(e) = self.persister.store(*wallet_id, changeset) { tracing::warn!( 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; 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 1eb70a75645..e79578df24a 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -22,6 +22,17 @@ use crate::wallet::PlatformWallet; use super::PlatformWalletManager; +/// The error a registration of an already-registered wallet returns. +/// +/// Built from the upstream variant `insert_wallet` would have produced, so the +/// pre-read check and the insert itself are indistinguishable to the caller — +/// which of the two answers first is an ordering detail, not a contract. +fn already_registered(wallet_id: WalletId) -> PlatformWalletError { + PlatformWalletError::WalletAlreadyExists( + key_wallet_manager::WalletError::WalletExists(wallet_id).to_string(), + ) +} + /// Parse a BIP-39 mnemonic in any supported language. /// /// Since rust-dashcore#981 `Mnemonic::from_phrase` IS the auto-detecting @@ -363,6 +374,62 @@ impl PlatformWalletManager

{ wallet.downgrade_to_external_signable(); + // Answer a duplicate registration before the read below, not after. + // Re-registering an existing wallet is a benign no-op the FFI / Swift + // call sites rely on, and it must stay one: a busy backend would + // otherwise turn it into a persister error that invites the caller to + // retry an operation that had nothing to do. `insert_wallet` stays the + // authority — this only decides which answer the caller gets first, so + // a wallet registered between the two checks still collides there. + { + let wm = self.wallet_manager.read().await; + if wm.get_wallet_info(®istration_wallet_id).is_some() { + return Err(already_registered(registration_wallet_id)); + } + } + + // Read the persisted state BEFORE the wallet exists anywhere the + // wallet-event producer can see it. + // + // The ordering is load-bearing, and the write it protects against is + // not one this function makes. An exhausted transient read reports + // `PersisterLoad(Transient)`, which tells the caller nothing was + // mutated and the registration is safe to re-issue. But + // `insert_wallet` publishes the wallet into the shared + // `WalletManager`, and the SPV filter loop scans under that same lock: + // a wallet whose `synced_height` is behind the chain is picked up by + // the next batch, whose matches and watermark advance travel the + // lossless channel to the wallet-event adapter and land in `store()`. + // Registering first therefore lets a durable write precede the promise + // that none happened — and the exhausting read is the slow case, so + // the producer has the most time to commit exactly when the promise is + // about to be made. Reading first leaves nothing for it to write. + // + // `load` is an idempotent read, so a transient blip is retried + // in-crate — unlike the `store` further down. + // + // The whole per-wallet map is carried across `insert_wallet` rather + // than sliced here: the authoritative id is the one that call returns, + // and it is deliberately not assumed equal to `registration_wallet_id` + // (see the divergence branch below). + let load_persister: Arc = Arc::clone(&self.persister) as _; + let mut persisted_platform_addresses = + match super::retry_transient_load(move || load_persister.load()).await { + Ok(crate::changeset::ClientStartState { + platform_addresses, .. + }) => platform_addresses, + Err(e) => { + tracing::error!( + wallet_id = %hex::encode(registration_wallet_id), + transient = e.is_transient(), + error = %e, + "failed to load persisted wallet state after retries; \ + registration aborted before the wallet was registered" + ); + return Err(PlatformWalletError::from_load_failure(e)); + } + }; + // Insert into WalletManager. A duplicate (same network-scoped // wallet id already registered) surfaces as the typed // `WalletAlreadyExists` so the create FFI / Swift call sites can @@ -371,16 +438,14 @@ impl PlatformWalletManager

{ // stays `WalletCreation`. let wallet_id = { let mut wm = self.wallet_manager.write().await; - wm.insert_wallet(wallet, platform_info).map_err(|e| { - if matches!(e, key_wallet_manager::WalletError::WalletExists(_)) { - PlatformWalletError::WalletAlreadyExists(e.to_string()) - } else { - PlatformWalletError::WalletCreation(format!( + wm.insert_wallet(wallet, platform_info) + .map_err(|e| match e { + key_wallet_manager::WalletError::WalletExists(id) => already_registered(id), + other => PlatformWalletError::WalletCreation(format!( "Failed to register wallet in WalletManager: {}", - e - )) - } - })? + other + )), + })? }; // `insert_wallet` recomputes the id from the (now external-signable) @@ -408,6 +473,10 @@ impl PlatformWalletManager

{ .insert(wallet_id, fences); } + // Now that the authoritative id is known, take this wallet's slice of + // the snapshot read above and drop the rest. + let persisted_platform_addresses = persisted_platform_addresses.remove(&wallet_id); + // Emit metadata + per-account xpubs + per-pool address // snapshots to the persister so the watch-only restore path // has everything it needs on next launch. The whole @@ -464,24 +533,24 @@ impl PlatformWalletManager

{ } } + // `store` is not retried here: the caller receives the typed, + // kind-classified `PersistenceError` and decides. if let Err(e) = self.persister.store(wallet_id, registration_changeset) { tracing::error!( wallet_id = %hex::encode(wallet_id), + transient = e.is_transient(), error = %e, "failed to persist wallet registration changeset" ); 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::from_store_failure(&*self.persister, e)); } // Build the PlatformWallet handle. @@ -500,58 +569,38 @@ impl PlatformWalletManager

{ broadcaster, ); - // Load persisted state. The only area wired up today is the - // platform-address provider — `from_persisted` skips the live - // `AddressPool` scan `initialize` would otherwise do. - // Per-wallet UTXOs / unused asset locks ship in the snapshot - // but don't have an active restore path yet. + // Restore the platform-address provider from the slice read above — + // the only area wired up today. `from_persisted` skips the live + // `AddressPool` scan `initialize` would otherwise do. Per-wallet + // UTXOs / unused asset locks ship in the snapshot but don't have an + // active restore path yet. // - // The two `?` returns below would otherwise leave the wallet - // half-registered (present in `wallet_manager` from the - // earlier `insert_wallet`, absent from `self.wallets`), - // poisoning every retry on `WalletAlreadyExists`. Roll back - // before bailing — same shape as `manager::load`. - let crate::changeset::ClientStartState { - mut platform_addresses, - wallets: _, - #[cfg(feature = "shielded")] - shielded: _, - } = match platform_wallet.load_persisted() { - Ok(state) => state, - Err(e) => { - let mut wm = self.wallet_manager.write().await; - if let Err(e) = wm.remove_wallet(&wallet_id) { - tracing::warn!( - wallet_id = %hex::encode(wallet_id), - error = %e, - "rollback: remove_wallet failed while unwinding a failed wallet setup" - ); - } - return Err(PlatformWalletError::WalletCreation(format!( - "Failed to load persisted wallet state: {}", - e - ))); - } - }; - - if let Some(persisted) = platform_addresses.remove(&wallet_id) { + // A bare `?` here would leave the wallet half-registered exactly as on + // the read path above, so this too rolls the insert back before + // bailing. Unlike that path, the registration write has already + // landed: `PersisterRestore` says so, and promises no re-issue. + if let Some(persisted) = persisted_platform_addresses { if let Err(e) = platform_wallet .platform() .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 - ))); + // Wrap the already-typed error rather than stringify it, so + // its concrete variant and source chain survive. + return Err(PlatformWalletError::from_restore_failure(e)); } } else { platform_wallet.platform().initialize().await; @@ -573,8 +622,11 @@ impl PlatformWalletManager

{ // // A wallet added while SPV is already synced (e.g. importing an // existing mnemonic with `birth_height = 0`) has its historical - // funds backfilled by the SPV rescan that `insert_wallet` above - // triggers. That rescan can complete — emitting the + // funds backfilled by an SPV rescan. `insert_wallet` does not + // request that rescan — it only publishes the wallet into the shared + // `WalletManager`; the SPV filter loop reads that same map, finds a + // wallet whose `synced_height` is behind the chain, and scans it on + // its next batch. That backfill can therefore complete — emitting the // `BlockProcessed` event that carries the post-backfill balance — // *before* this wallet lands in `self.wallets`, so // `BalanceUpdateHandler` drops those events (the wallet isn't in @@ -1273,6 +1325,705 @@ mod register_wallet_duplicate_tests { } } +#[cfg(test)] +mod persist_retry_tests { + //! Registration-path persistence: single-attempt `store`, bounded `load` + //! retry, typed error propagation, log-level policy. + + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + use std::time::Duration; + + use key_wallet::mnemonic::Mnemonic; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::Network; + use tracing::Level; + + use crate::changeset::{ + ClientStartState, PersistenceError, PersistenceErrorKind, PlatformWalletChangeSet, + PlatformWalletPersistence, + }; + use crate::error::PlatformWalletError; + use crate::events::PlatformEventHandler; + use crate::test_support::NoopTestEventHandler; + 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") + } + + use crate::test_support::tracing_capture::{RecordedEvents, RecordingGuard}; + + /// Persister with scripted `store` / `flush` / `load` outcomes. + /// + /// `store` counts registration and scan-verdict writes separately, + /// discriminated by the changeset: registration ends with a best-effort + /// `identity().sync()` that issues a SECOND `store`, and a single counter + /// would couple every registration-write assertion to discovery. + #[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, + /// Never scripted to fail: a `store` failure is never retried through + /// it, so every assertion expects 0. + flush_calls: AtomicUsize, + load_calls: AtomicUsize, + store_transient: bool, + store_fatal: bool, + /// Leading scan-verdict `store` calls that fail transiently. + scan_verdict_store_transient_failures: usize, + /// Leading `load` calls that fail transiently. + load_transient_failures: usize, + load_fatal: bool, + /// Fail fatally after `load_transient_failures`, instead of succeeding. + load_then_fatal: bool, + /// Model a buffering backend that keeps the failed changeset for its + /// own later retry, so re-issuing it would merge it twice. + retains_failed_changeset: bool, + } + + impl PlatformWalletPersistence for FaultyPersister { + fn store_transient_is_reissuable(&self) -> bool { + !self.retains_failed_changeset + } + + fn store( + &self, + _wallet_id: WalletId, + changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + // `merge` can fold a registration write and a scan verdict into + // one round, so both counters increment; letting the first match + // win would reintroduce the batching dependency. + 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: its failure + // aborts registration, a verdict's is swallowed. + if registration.is_some() { + if self.store_fatal { + return Err(fatal()); + } + if self.store_transient { + 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> { + self.flush_calls.fetch_add(1, Ordering::SeqCst); + 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()); + } + if self.load_then_fatal { + return Err(fatal()); + } + Ok(ClientStartState::default()) + } + } + + 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(NoopTestEventHandler); + Arc::new(PlatformWalletManager::new(sdk, persister, event_handler)) + } + + fn seed_bytes() -> [u8; 64] { + Mnemonic::from_phrase(TEST_MNEMONIC) + .expect("valid test mnemonic") + .to_seed("") + } + + /// `Some(0)` skips the SPV-tip birth-height lookup. + async fn register( + manager: &PlatformWalletManager, + ) -> Result<(), PlatformWalletError> { + manager + .create_wallet_from_seed_bytes( + Network::Testnet, + &seed_bytes(), + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .map(|_| ()) + } + + /// Surfaces on the first attempt, and rolls the in-memory insert back. + #[tokio::test] + async fn transient_store_failure_surfaces_as_persister_store_without_retry() { + let persister = Arc::new(FaultyPersister { + store_transient: true, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("a transient store failure must abort registration, not retry it"); + + match err { + PlatformWalletError::PersisterStore(pe) => assert!( + pe.is_transient(), + "a transient store failure must keep its 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, + "store is never retried via flush" + ); + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 0, + "an aborted registration never reaches the discovery scan" + ); + assert!( + manager.wallet_ids().await.is_empty(), + "a failed store must roll back the in-memory wallet insert" + ); + } + + /// A persister that keeps the failed changeset buffered for its own retry + /// (the canonical SQLite backend does exactly this) must never reach the + /// caller as retryable: re-issuing the changeset would merge it into the + /// retained copy, and changeset vectors merge by appending. + #[tokio::test] + async fn transient_store_failure_is_downgraded_without_a_reissue_attestation() { + let persister = Arc::new(FaultyPersister { + store_transient: true, + retains_failed_changeset: true, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("a transient store failure must abort registration"); + + match err { + PlatformWalletError::PersisterStore(pe) => assert!( + !pe.is_transient(), + "a persister that retains the failed changeset must not invite a re-issue" + ), + other => panic!("expected PersisterStore, got {other:?}"), + } + } + + /// A fatal `store` failure fails fast, keeping its classification. + #[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 transient `load` blip is retried — it is an idempotent read. + #[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" + ); + } + + #[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" + ); + } + + #[tokio::test] + async fn transient_then_fatal_load_surfaces_as_persister_load_fatal() { + let persister = Arc::new(FaultyPersister { + load_transient_failures: 1, + load_then_fatal: true, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("a load that turns fatal must abort registration"); + + match err { + PlatformWalletError::PersisterLoad(pe) => { + assert!( + !pe.is_transient(), + "the fatal outcome must win, not the earlier transient one" + ) + } + other => panic!("expected PersisterLoad, got {other:?}"), + } + assert_eq!(persister.load_calls.load(Ordering::SeqCst), 2); + } + + /// Nothing the wallet-event producer can persist may exist before the + /// registration read runs. + /// + /// The read is retried and can exhaust, and an exhausted transient read + /// reports `PersisterLoad(Transient)` — FFI code 49, which promises the + /// host nothing was mutated and the registration is safe to re-issue. + /// Registering the wallet in `WalletManager` first breaks that promise + /// without any code in this function writing anything: the SPV filter loop + /// reads the same lock, sees a wallet whose `synced_height` is behind the + /// chain, and its scan emits the events that the wallet-event adapter + /// commits through `store()`. The read exhausting is exactly the slow case + /// (a busy backend can take seconds per attempt), so the producer has the + /// most time to commit precisely when the promise is about to be made. + /// + /// Standing in for SPV from inside `load()` is what makes this + /// deterministic rather than a race: the probe plays the part of a filter + /// batch committing mid-read, then waits for the write it caused. Safe + /// because no caller of `retry_transient_load` holds the wallet-manager + /// lock across the call — verified at both call sites — and the probe + /// releases its own guard before waiting. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_registration_read_precedes_any_wallet_the_event_producer_can_see() { + use std::sync::atomic::AtomicBool; + use std::sync::{OnceLock, Weak}; + use tokio::sync::RwLock; + + use crate::wallet::platform_wallet::PlatformWalletInfo; + use key_wallet_manager::{WalletInterface, WalletManager}; + + /// Fails every `load` transiently, and while doing so acts as the SPV + /// filter loop: any wallet already visible in the manager gets a + /// watermark advance, which reaches the persister through the live + /// wallet-event adapter. + struct SpvRacingPersister { + wallet_manager: OnceLock>>>, + store_calls: AtomicUsize, + wallet_visible_during_load: AtomicBool, + /// Flipped for the second phase: the backend recovers and the + /// caller's retry must go through. + healthy: AtomicBool, + } + + impl PlatformWalletPersistence for SpvRacingPersister { + fn store( + &self, + _wallet_id: WalletId, + _changeset: PlatformWalletChangeSet, + ) -> Result<(), PersistenceError> { + self.store_calls.fetch_add(1, Ordering::SeqCst); + Ok(()) + } + + fn flush(&self, _wallet_id: WalletId) -> Result<(), PersistenceError> { + Ok(()) + } + + fn load(&self) -> Result { + let manager = self + .wallet_manager + .get() + .and_then(Weak::upgrade) + .expect("the probe is wired to the manager before any registration"); + // On the blocking pool (`retry_transient_load` spawns each + // attempt there), and no caller holds this lock across the + // call, so blocking on it here cannot deadlock. + let emitted = { + let mut wallet_manager = manager.blocking_write(); + match wallet_manager.get_all_wallet_infos().keys().next().copied() { + Some(wallet_id) => { + self.wallet_visible_during_load + .store(true, Ordering::SeqCst); + // What a committed filter batch does to a wallet + // that was behind the chain. + wallet_manager.update_wallet_synced_height(&wallet_id, 1_000); + true + } + None => false, + } + }; + + // Guard released above: give the write this read provoked time + // to actually land, so the assertions below observe a + // committed store rather than a lost race. + if emitted { + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while self.store_calls.load(Ordering::SeqCst) == 0 + && std::time::Instant::now() < deadline + { + std::thread::sleep(Duration::from_millis(10)); + } + } + + if self.healthy.load(Ordering::SeqCst) { + Ok(ClientStartState::default()) + } else { + Err(transient()) + } + } + } + + let persister = Arc::new(SpvRacingPersister { + wallet_manager: OnceLock::new(), + store_calls: AtomicUsize::new(0), + wallet_visible_during_load: AtomicBool::new(false), + healthy: AtomicBool::new(false), + }); + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let event_handler: Arc = Arc::new(NoopTestEventHandler); + let manager = PlatformWalletManager::new(sdk, Arc::clone(&persister), event_handler); + let _ = persister + .wallet_manager + .set(Arc::downgrade(&manager.wallet_manager)); + + let seed = Mnemonic::from_phrase(TEST_MNEMONIC) + .expect("valid test mnemonic") + .to_seed(""); + let err = manager + .create_wallet_from_seed_bytes( + Network::Testnet, + &seed, + WalletAccountCreationOptions::Default, + // `Some(0)` skips the SPV-tip lookup: the birth height is not + // what puts the wallet behind the chain here, the probe is. + Some(0), + ) + .await + .expect_err("an exhausted transient load must abort registration"); + match err { + PlatformWalletError::PersisterLoad(pe) => assert!( + pe.is_transient(), + "an exhausted transient load keeps its transient classification" + ), + other => panic!("expected PersisterLoad, got {other:?}"), + } + + assert_eq!( + persister.store_calls.load(Ordering::SeqCst), + 0, + "an exhausted read reports that nothing was mutated, so nothing may \ + have reached the persister before it — including writes this \ + function never makes itself" + ); + assert!( + !persister.wallet_visible_during_load.load(Ordering::SeqCst), + "the registration read must run before the wallet is visible in \ + WalletManager: the SPV filter loop reads that map, and a wallet \ + it can see is a wallet it can produce persistable events for" + ); + + // The caller takes the retry the error invited. It can only succeed if + // the aborted attempt left no wallet behind — a registration that + // returned before publishing one has nothing to collide with. + persister.healthy.store(true, Ordering::SeqCst); + manager + .create_wallet_from_seed_bytes( + Network::Testnet, + &seed, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("the retry a transient read invites must succeed"); + assert!( + !persister.wallet_visible_during_load.load(Ordering::SeqCst), + "the retry's read must also precede its own registration" + ); + } + + /// An exhausted transient `load` retry must leave nothing on disk to + /// double-write. + /// + /// `PersisterLoad(Transient)` crosses the C ABI as code 49, which tells the + /// host nothing was mutated and a later retry is safe. The only + /// caller-visible operation to retry is the registration itself, so with + /// the read ordered after the registration write that retry appends the + /// append-only changeset a second time. + #[tokio::test] + async fn an_exhausted_load_retry_leaves_the_registration_unwritten() { + // The whole schedule — the initial attempt plus one per backoff entry + // — so the first registration exhausts it and the retry's read + // succeeds. + let attempts = 1 + super::super::persist_retry::LOAD_RETRY_BACKOFF.len(); + let persister = Arc::new(FaultyPersister { + load_transient_failures: attempts, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let err = register(&manager) + .await + .expect_err("an exhausted transient load must abort registration"); + match err { + PlatformWalletError::PersisterLoad(pe) => assert!( + pe.is_transient(), + "an exhausted transient load keeps its transient classification" + ), + other => panic!("expected PersisterLoad, got {other:?}"), + } + assert_eq!(persister.load_calls.load(Ordering::SeqCst), attempts); + assert_eq!( + persister.registration_store_calls.load(Ordering::SeqCst), + 0, + "a read that can exhaust must run before the registration write: \ + its error promises the caller nothing was mutated, and the retry \ + it invites is the registration" + ); + + // The caller takes that invitation. + register(&manager) + .await + .expect("the retry a transient load failure invites must succeed"); + assert_eq!( + persister.registration_store_calls.load(Ordering::SeqCst), + 1, + "the retried registration must be the FIRST write of the changeset" + ); + } + + /// Virtual time, so the test itself doesn't wait the schedule's 140 ms. + #[tokio::test(start_paused = true)] + async fn transient_load_retry_follows_the_backoff_schedule() { + let calls = Arc::new(AtomicUsize::new(0)); + let op_calls = Arc::clone(&calls); + let start = tokio::time::Instant::now(); + + let result: Result<(), PersistenceError> = super::super::retry_transient_load(move || { + op_calls.fetch_add(1, Ordering::SeqCst); + Err(transient()) + }) + .await; + + assert!( + result.is_err(), + "an always-transient op exhausts the schedule" + ); + assert_eq!( + calls.load(Ordering::SeqCst), + 1 + super::super::persist_retry::LOAD_RETRY_BACKOFF.len(), + "one initial attempt plus one per scheduled backoff" + ); + let expected: Duration = super::super::persist_retry::LOAD_RETRY_BACKOFF.iter().sum(); + assert_eq!(tokio::time::Instant::now() - start, expected); + } + + /// A busy backend costs the scan verdict its durability this launch + /// (dashpay/platform#4365) rather than failing the registration that just + /// succeeded: logged and swallowed, never retried. + #[tokio::test] + async fn transient_scan_verdict_store_failure_is_logged_not_retried() { + let persister = Arc::new(FaultyPersister { + scan_verdict_store_transient_failures: 1, + ..Default::default() + }); + let manager = make_manager(Arc::clone(&persister)); + + let recorder = RecordedEvents::default(); + let _guard = RecordingGuard::install(recorder.clone()); + + register(&manager) + .await + .expect("a scan-verdict store failure must not disturb registration"); + + assert_eq!( + persister.scan_verdict_store_calls.load(Ordering::SeqCst), + 1, + "the verdict store is attempted once, never retried" + ); + let events = recorder.entries(); + assert!( + events.iter().any(|(level, msg)| *level == Level::WARN + && msg.contains("identity-scan verdict could not be persisted")), + "an unpersisted scan verdict must be logged at warn: {events:?}" + ); + assert!( + !events + .iter() + .any(|(level, msg)| *level == Level::ERROR && msg.contains("identity-scan verdict")), + "a scan-verdict store failure must not log at error: {events:?}" + ); + } + + #[tokio::test] + async fn unpersistable_scan_verdict_does_not_fail_registration() { + let persister = Arc::new(FaultyPersister { + scan_verdict_store_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"); + + assert_eq!(persister.scan_verdict_store_calls.load(Ordering::SeqCst), 1); + assert_eq!(persister.flush_calls.load(Ordering::SeqCst), 0); + } + + /// The typed variants preserve retry classification, allow structural + /// matching, and keep the `#[source]` chain. + /// + /// Also pins each constructor to the operation it names — the reason no + /// blanket `From` exists: only the call site knows + /// whether a load, a store or a flush produced the value, so an inferred + /// conversion would report failed writes as failed reads. + #[test] + fn typed_variants_preserve_classification_matching_and_source() { + use std::error::Error; + + let attesting = FaultyPersister { + retains_failed_changeset: false, + ..Default::default() + }; + let store_err = PlatformWalletError::from_store_failure(&attesting, 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" + ); + + // The narrowing keeps the chain: a caller that downcasts for detail + // still gets it, it is only told not to re-issue. + let retaining = FaultyPersister { + retains_failed_changeset: true, + ..Default::default() + }; + let narrowed = PlatformWalletError::from_store_failure(&retaining, transient()); + match &narrowed { + PlatformWalletError::PersisterStore(pe) => assert!(!pe.is_transient()), + other => panic!("expected PersisterStore, got {other:?}"), + } + assert!(narrowed.source().is_some()); + + let load_err = PlatformWalletError::from_load_failure(fatal()); + match &load_err { + PlatformWalletError::PersisterLoad(pe) => assert!(!pe.is_transient()), + other => panic!("expected PersisterLoad, got {other:?}"), + } + assert!(load_err.source().is_some()); + + // Structural matching must recover the concrete inner variant. + let restore_err = + PlatformWalletError::from_restore_failure(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:?}"), + } + + // Both take the SAME input type, so only the call site distinguishes + // them and a mix-up is silent. + assert!( + matches!( + PlatformWalletError::from_store_failure(&attesting, fatal()), + PlatformWalletError::PersisterStore(_) + ), + "a failed store must never be reported as a failed load" + ); + assert!( + matches!( + PlatformWalletError::from_load_failure(fatal()), + PlatformWalletError::PersisterLoad(_) + ), + "a failed load must never be reported as a failed store" + ); + } +} + /// Removal versus a same-id re-registration that lands *during* the removal. /// /// The invariant: `remove_wallet_with_teardown` removes, returns and tears down diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index 854d6c59d81..b0ec2af019c 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -650,7 +650,8 @@ impl crate::changeset::PlatformWalletPersistence for NoopTestPersister { } } -struct NoopTestEventHandler; +/// Event handler that ignores every event. +pub(crate) struct NoopTestEventHandler; impl crate::events::EventHandler for NoopTestEventHandler {} impl crate::events::PlatformEventHandler for NoopTestEventHandler {} @@ -781,3 +782,106 @@ pub(crate) async fn mnemonic_wallet_manager( receive_address, ) } + +/// Thread-scoped `tracing` event capture for tests that assert on log output. +/// +/// Every capturing test must route through the one globally-installed +/// subscriber here rather than installing its own — see [`RecorderRouter`]. +#[cfg(test)] +pub(crate) mod tracing_capture { + use std::cell::RefCell; + use std::sync::{Arc, Mutex, OnceLock}; + + use tracing::field::{Field, Visit}; + use tracing::Level; + use tracing_subscriber::layer::{Context, SubscriberExt}; + use tracing_subscriber::Layer; + + /// Level and message of every event recorded while registered as the + /// current thread's active recorder (see [`RecordingGuard`]). + #[derive(Clone, Default)] + pub(crate) struct RecordedEvents(Arc>>); + + impl RecordedEvents { + pub(crate) fn entries(&self) -> Vec<(Level, String)> { + self.0.lock().expect("recorded events mutex").clone() + } + + fn record(&self, event: &tracing::Event<'_>) { + struct MessageVisitor(String); + impl Visit for MessageVisitor { + fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) { + if field.name() == "message" { + self.0 = format!("{value:?}"); + } + } + } + let mut visitor = MessageVisitor(String::new()); + event.record(&mut visitor); + self.0 + .lock() + .expect("recorded events mutex") + .push((*event.metadata().level(), visitor.0)); + } + } + + thread_local! { + /// Where events from THIS thread go. Set only by [`RecordingGuard`]. + static ACTIVE_RECORDER: RefCell> = const { RefCell::new(None) }; + } + + /// Routes every event to whichever [`RecordedEvents`] the emitting thread + /// registered in [`ACTIVE_RECORDER`]. Installed as the process-wide + /// default exactly once — never per-test. + /// + /// A per-test `tracing::subscriber::set_default` swap is flaky under + /// `cargo test`'s parallel harness: tracing's per-callsite `Interest` cache + /// is process-global, so a concurrent test's swap/drop can race the + /// interest rebuild yours triggers and the event silently never reaches + /// your subscriber — even though dispatch stays correctly on your own + /// thread (confirmed: emitting and installing thread IDs matched on a + /// captured failure). Installing once, before any callsite is hit, + /// sidesteps the race: routing then goes through a thread-local this code + /// owns rather than tracing's default-swap machinery. + struct RecorderRouter; + + impl Layer for RecorderRouter { + fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) { + ACTIVE_RECORDER.with(|slot| { + if let Some(recorder) = slot.borrow().as_ref() { + recorder.record(event); + } + }); + } + } + + static GLOBAL_ROUTER_INIT: OnceLock<()> = OnceLock::new(); + + /// Scopes [`ACTIVE_RECORDER`] to `recorder` for this thread and lifetime. + pub(crate) struct RecordingGuard; + + impl RecordingGuard { + pub(crate) fn install(recorder: RecordedEvents) -> Self { + GLOBAL_ROUTER_INIT.get_or_init(|| { + let subscriber = tracing_subscriber::registry().with(RecorderRouter); + // `get_or_init` runs this exactly once, so the only way to + // fail is something outside it having installed a process-wide + // default first. Discarding that would leave the router + // uninstalled while this latch still reports success, and every + // guard below would capture nothing at all. + tracing::subscriber::set_global_default(subscriber).expect( + "the event-recording subscriber must become the process-wide default: \ + another global subscriber is already installed, so no test can capture", + ); + }); + ACTIVE_RECORDER.with(|slot| *slot.borrow_mut() = Some(recorder)); + Self + } + } + + impl Drop for RecordingGuard { + fn drop(&mut self) { + ACTIVE_RECORDER.with(|slot| *slot.borrow_mut() = None); + } + } +} 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 d606aca555f..7c4b504c2a4 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`) read every +/// failure as a miss and keep waiting on the live sync stream — see +/// [`record_or_persister_for_poll`]. /// - **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,29 +143,52 @@ 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. -pub(super) fn record_or_persister_or_log( +/// Variant of [`record_or_persister`] for poll loops: never aborts the wait, +/// whatever the persister does. +/// +/// This read is a FALLBACK for records the in-memory map evicted — the live +/// SPV stream can still deliver one — so any failure reads as a miss and the +/// loop keeps waiting, bounded by its own finality timeout. +/// +/// Both failure classes report once per wait, via `state`: per-iteration +/// logging would let a broken backend flood the log from inside an unbounded +/// poll loop, saying the same thing every time. +pub(super) fn record_or_persister_for_poll( in_memory: Option, persister: &crate::wallet::persister::WalletPersister, txid: &Txid, + state: &mut PollReadState, ) -> Option { - match record_or_persister(in_memory, persister, txid) { - Ok(opt) => opt, + if let Some(record) = in_memory { + return Some(record); + } + match persister.get_core_tx_record_or_transient_miss(txid, &mut state.transient_misses) { + Ok(found) => found, Err(e) => { - tracing::warn!( - txid = %txid, - error = %e, - "Persister fallback for core tx record failed; \ - treating as miss for this poll iteration" - ); + if !state.permanent_reported { + state.permanent_reported = true; + tracing::error!( + txid = %txid, + error = %e, + "Core tx-record fallback read is permanently failing; waiting on the \ + live sync stream instead until this wait's timeout" + ); + } None } } } +/// Read diagnostics for ONE wait, owned by the polling loop. +/// +/// The permanent-failure latch fires on the first `Err`; the transient tally +/// summarises itself when the wait ends, whichever way it ends. +#[derive(Debug, Default)] +pub(super) struct PollReadState { + permanent_reported: bool, + transient_misses: crate::wallet::persister::TransientMissTally, +} + impl AssetLockManager { /// Validate an IS-lock proof and upgrade it to a ChainLock proof if the /// transaction is old enough that the IS-lock may have expired. @@ -365,6 +388,8 @@ impl AssetLockManager { use key_wallet::transaction_checking::TransactionContext; let deadline = timeout.map(|t| tokio::time::Instant::now() + t); + // Once-per-wait read diagnostics; see `record_or_persister_for_poll`. + let mut read_state = PollReadState::default(); loop { // Arm the `Notify` future BEFORE the state check, closing @@ -391,9 +416,12 @@ impl AssetLockManager { funding_tx_record(&info.core_wallet.accounts, account_index, &out_point.txid) }) }; - if let Some(record) = - record_or_persister_or_log(in_memory, &self.persister, &out_point.txid) - { + if let Some(record) = record_or_persister_for_poll( + in_memory, + &self.persister, + &out_point.txid, + &mut read_state, + ) { if matches!(record.context, TransactionContext::InChainLockedBlock(_)) { if let Some(h) = record.height() { return Ok(h); @@ -453,6 +481,8 @@ impl AssetLockManager { tracing::info!(outpoint = %out_point, ?timeout, "wait_for_proof: entered"); let deadline = timeout.map(|t| tokio::time::Instant::now() + t); let mut iter: u32 = 0; + // Once-per-wait read diagnostics; see `record_or_persister_for_poll`. + let mut read_state = PollReadState::default(); // Read account_index and transaction from the tracked lock. let (account_index, tracked_tx) = { @@ -517,9 +547,12 @@ impl AssetLockManager { funding_tx_record(&info.core_wallet.accounts, account_index, &out_point.txid) }) }; - if let Some(record) = - record_or_persister_or_log(in_memory, &self.persister, &out_point.txid) - { + if let Some(record) = record_or_persister_for_poll( + in_memory, + &self.persister, + &out_point.txid, + &mut read_state, + ) { match &record.context { TransactionContext::InstantSend(instant_lock) => { return Ok(dpp::prelude::AssetLockProof::Instant( @@ -967,8 +1000,7 @@ mod tests { } } - /// Test persister that always errors out on `get_core_tx_record`, - /// to exercise the error-swallowing branch in `record_or_persister`. + /// Persister with a permanent `get_core_tx_record` failure. struct ErroringStore; impl PlatformWalletPersistence for ErroringStore { @@ -994,6 +1026,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) } @@ -1058,9 +1118,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)); @@ -1068,15 +1126,109 @@ mod tests { assert!(resolved.is_err()); } + /// A poll loop degrades on a permanent read failure rather than aborting: + /// the live SPV stream can still end the wait. #[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 poll_read_degrades_to_a_miss_on_permanent_backend_errors() { let unknown_txid = Txid::from([0xFF; 32]); let persister = wallet_persister(Arc::new(ErroringStore)); + let mut state = PollReadState::default(); - let resolved = record_or_persister_or_log(None, &persister, &unknown_txid); + let resolved = record_or_persister_for_poll(None, &persister, &unknown_txid, &mut state); + assert!( + resolved.is_none(), + "a permanent read failure must read as a miss, not abort the wait" + ); + assert!( + state.permanent_reported, + "the first permanent failure must be reported" + ); + + // Subsequent iterations of the SAME wait stay silent. + let resolved = record_or_persister_for_poll(None, &persister, &unknown_txid, &mut state); + assert!(resolved.is_none()); + assert!(state.permanent_reported); + } + + /// The report fires ONCE per wait: a poll loop spins many times against the + /// same broken backend, and per-iteration reporting buries the log. + /// + /// Counting is the point — an assertion that merely finds a report present + /// passes just as happily when every iteration emits one. + #[test] + fn poll_read_reports_a_permanent_failure_once_per_wait_not_once_per_iteration() { + use crate::test_support::tracing_capture::{RecordedEvents, RecordingGuard}; + use tracing::Level; + + let unknown_txid = Txid::from([0xFF; 32]); + let persister = wallet_persister(Arc::new(ErroringStore)); + let mut state = PollReadState::default(); + + let recorder = RecordedEvents::default(); + let _guard = RecordingGuard::install(recorder.clone()); + + // Three iterations of ONE wait, as a poll loop would. + for _ in 0..3 { + assert!( + record_or_persister_for_poll(None, &persister, &unknown_txid, &mut state).is_none() + ); + } + + let reports = recorder + .entries() + .into_iter() + .filter(|(level, msg)| { + *level == Level::ERROR && msg.contains("Core tx-record fallback read") + }) + .count(); + assert_eq!( + reports, 1, + "three iterations of one wait must produce exactly one report, got {reports}" + ); + } + + /// A transient failure must not consume the once-per-wait report. + #[test] + fn poll_read_treats_transient_backend_errors_as_a_silent_miss() { + let unknown_txid = Txid::from([0xFF; 32]); + let persister = wallet_persister(Arc::new(TransientErroringStore)); + let mut state = PollReadState::default(); + + let resolved = record_or_persister_for_poll(None, &persister, &unknown_txid, &mut state); assert!(resolved.is_none()); + assert!( + !state.permanent_reported, + "a transient failure must not consume the permanent-failure report" + ); + } + + /// The shared helper collapses transient failures, not permanent ones, and + /// only the collapsed ones are counted for the end-of-pass summary. + #[test] + fn transient_miss_read_helper_separates_transient_from_permanent() { + use crate::wallet::persister::TransientMissTally; + + let unknown_txid = Txid::from([0xFF; 32]); + let mut tally = TransientMissTally::default(); + + let transient = wallet_persister(Arc::new(TransientErroringStore)); + assert!(transient + .get_core_tx_record_or_transient_miss(&unknown_txid, &mut tally) + .expect("a transient failure must read as a miss") + .is_none()); + assert_eq!(tally.misses(), 1, "a collapsed failure must be counted"); + + let permanent = wallet_persister(Arc::new(ErroringStore)); + assert!( + permanent + .get_core_tx_record_or_transient_miss(&unknown_txid, &mut tally) + .is_err(), + "a permanent failure must stay visible to the caller" + ); + assert_eq!( + tally.misses(), + 1, + "a permanent failure is reported on its own, never counted as a miss" + ); } } 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 8c66dbd695d..76c02fc6999 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/discovery.rs @@ -641,9 +641,12 @@ 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. + /// + /// `store` is a single attempt, per the caller-decides persister-error + /// policy, so a merely busy backend (dashpay/platform#4365) costs the + /// verdict its durability this launch. Logged and swallowed either way. async fn publish_scan_verdict( &self, wallet_id: crate::wallet::platform_wallet::WalletId, @@ -676,9 +679,11 @@ impl IdentityWallet { if let Err(e) = self.persister.store(changeset) { tracing::warn!( 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; 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 4c455aba433..89f982f0db5 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -224,6 +224,20 @@ impl DashPayView<'_, B> { /// /// Local-only and idempotent: an existing payment entry under the /// txid is never overwritten. + /// + /// # Errors + /// + /// Every persister read here — the txid enumeration and each record — + /// reports as [`PlatformWalletError::PersisterLoad`], carrying the + /// backend's own retry classification. + /// + /// Transient tx-record read failures leave the scan incomplete, so the + /// guard stays unstamped and the next sweep retries. A permanent one is + /// reported rather than deferred — retrying it every sweep would never + /// succeed and never be reported — but only after the pass finishes: the + /// records that ARE readable are still reconstructed, and the first + /// permanent failure surfaces on the way out. A failed enumeration has no + /// pass to finish and returns immediately. pub async fn reconcile_sent_payments_from_tx_history( &self, ) -> Result { @@ -295,9 +309,10 @@ impl DashPayView<'_, B> { return Ok(0); } - let Some(listed) = self.persister.list_wallet_core_txids().map_err(|e| { - PlatformWalletError::Persistence(format!("failed to enumerate wallet txids: {e}")) - })? + let Some(listed) = self + .persister + .list_wallet_core_txids() + .map_err(PlatformWalletError::from_load_failure)? else { // The backend does not index wallet-scoped transaction history // (e.g. the Android vtable leaves the enumeration callbacks @@ -402,12 +417,22 @@ impl DashPayView<'_, B> { let mut incomplete_scan = false; let txid_count = listed.len(); let mut funded: Vec = Vec::new(); + // Reported once when this sweep ends, however it ends. + let mut transient_misses = crate::wallet::persister::TransientMissTally::default(); + // The first permanent read failure, surfaced only once the pass has + // finished. Aborting here would throw away every record already read. + let mut permanent_read_failure: Option = None; for entry in listed { if !entry.spends_wallet_input { continue; } let txid = entry.txid; - match self.persister.get_core_tx_record(&txid) { + // TODO(host-transient-read-classification): preserve host record-read errors; + // FFI currently maps every nonzero callback status to Ok(None). + match self + .persister + .get_core_tx_record_or_transient_miss(&txid, &mut transient_misses) + { Ok(Some(record)) => { // Walk the decoded transaction's outputs, NOT // `record.output_details`. Records handed back by @@ -427,6 +452,8 @@ impl DashPayView<'_, B> { .collect(), }); } + // Not readable yet, or a transient failure read as a miss. + // Both mean: retry on the next sweep. Ok(None) => { incomplete_scan = true; tracing::debug!( @@ -434,13 +461,13 @@ impl DashPayView<'_, B> { "reconcile_sent_payments_from_tx_history: listed tx record unavailable; will retry next sweep" ); } + // A permanent failure will not fix itself, so deferring it + // re-runs the whole sweep on every sync forever. Keep the first + // one and report it after the pass: one unreadable row must not + // void the reconstruction of every readable one. Err(e) => { incomplete_scan = true; - tracing::warn!( - error = %e, - %txid, - "reconcile_sent_payments_from_tx_history: tx-record read failed; will retry next sweep" - ); + permanent_read_failure.get_or_insert(e); } } } @@ -647,6 +674,9 @@ impl DashPayView<'_, B> { .insert(window.contact, table_digest); } } + if let Some(e) = permanent_read_failure { + return Err(PlatformWalletError::from_load_failure(e)); + } Ok(recorded) } @@ -670,6 +700,13 @@ 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 ones return [`PlatformWalletError::PersisterLoad`] once the + /// sweep has finished, so an unreadable record costs only its own + /// confirmation and not every other pending payment's. pub async fn reconcile_sent_payments(&self) -> Result { use crate::wallet::identity::types::dashpay::payment::{PaymentDirection, PaymentStatus}; @@ -697,19 +734,27 @@ impl DashPayView<'_, B> { }; let mut confirmed = 0usize; + // Reported once when this sweep ends, however it ends. + let mut transient_misses = crate::wallet::persister::TransientMissTally::default(); + // The first permanent read failure, surfaced after the sweep so the + // other pending payments still get their chance to confirm. + let mut permanent_read_failure: Option = None; for (_owner, txid_str) in pending { let Ok(txid) = txid_str.parse::() else { continue; }; - let record = match self.persister.get_core_tx_record(&txid) { + // A transient failure reads as a miss, so both are the same + // "not final yet, look again next sweep" outcome. + // TODO(host-transient-read-classification): preserve host record-read errors; + // FFI currently maps every nonzero callback status to Ok(None). + let record = match self + .persister + .get_core_tx_record_or_transient_miss(&txid, &mut transient_misses) + { Ok(Some(record)) => record, Ok(None) => continue, Err(e) => { - tracing::warn!( - error = %e, - txid = %txid_str, - "reconcile_sent_payments: tx-record read failed; will retry next sweep" - ); + permanent_read_failure.get_or_insert(e); continue; } }; @@ -731,6 +776,9 @@ impl DashPayView<'_, B> { .await; confirmed += 1; } + if let Some(e) = permanent_read_failure { + return Err(PlatformWalletError::from_load_failure(e)); + } Ok(confirmed) } } @@ -1644,7 +1692,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}; @@ -1694,6 +1743,11 @@ mod tests { key_wallet::managed_account::transaction_record::TransactionRecord, >, >, + /// `Some(kind)` fails every `get_core_tx_record` with that class. + read_error_kind: Mutex>, + /// Txids whose `get_core_tx_record` fails permanently while the rest + /// of the table stays readable — a single corrupt row. + permanently_unreadable: 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). @@ -1742,6 +1796,18 @@ 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.permanently_unreadable.lock().unwrap().contains(txid) { + return Err(PersistenceError::backend_with_kind( + PersistenceErrorKind::Fatal, + "simulated permanently unreadable tx record", + )); + } if self.listed_but_unavailable.lock().unwrap().contains(txid) { return Ok(None); } @@ -3576,6 +3642,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] @@ -3656,6 +3742,162 @@ mod tests { ); } + /// Only a transient failure folds into "incomplete, retry next time" — the + /// distinction stops a permanently unreadable store from silently + /// re-running the whole sweep on every dashpay sync. + #[tokio::test] + async fn reconcile_sent_payments_from_tx_history_surfaces_permanent_read_failures() { + use dashcore::hashes::Hash; + use dashcore::BlockHash; + use key_wallet::managed_account::transaction_record::OutputRole; + use key_wallet::transaction_checking::{BlockInfo, TransactionContext}; + + let persister = Arc::new(RecordStorePersister::default()); + let (manager, wallet_id) = make_wallet_with(Arc::clone(&persister)).await; + let owner = Identifier::from([0xAA; 32]); + let contact = Identifier::from([0xBB; 32]); + + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet.identity(); + let p = WalletPersister::new(wallet_id, Arc::clone(&persister) as _); + { + let mut wm = iw.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("info"); + info.identity_manager + .add_identity(bare_identity([0xAA; 32]), 0, wallet_id, &p) + .expect("add owner"); + } + let contact_addresses = install_external_account(&manager, wallet_id, owner, contact).await; + let change_address = first_standard_wallet_address(&manager, wallet_id).await; + let record = tx_record_with_outputs( + TransactionContext::InBlock(BlockInfo::new(123, BlockHash::all_zeros(), 0)), + vec![ + (contact_addresses[0].clone(), 25_000, OutputRole::Sent), + (change_address, 90_000, OutputRole::Change), + ], + ); + persister + .records + .lock() + .unwrap() + .insert(record.txid, record); + + // Transient: the sweep defers, exactly as before. + *persister.read_error_kind.lock().unwrap() = Some(PersistenceErrorKind::Transient); + assert_eq!( + iw.dashpay() + .reconcile_sent_payments_from_tx_history() + .await + .expect("a transient read failure must wait for the next sweep"), + 0 + ); + + // Permanent: the sweep reports it as a failed read. + *persister.read_error_kind.lock().unwrap() = Some(PersistenceErrorKind::Fatal); + let err = iw + .dashpay() + .reconcile_sent_payments_from_tx_history() + .await + .expect_err("a permanent read failure must surface, not loop forever"); + assert!( + matches!( + err, + PlatformWalletError::PersisterLoad(ref source) if !source.is_transient() + ), + "expected a permanent PersisterLoad, got {err:?}" + ); + } + + /// One permanently unreadable row costs only its own reconstruction. + /// + /// Returning from inside the collection loop threw away every record read + /// before it AND the whole matching phase, so a single corrupt row + /// suppressed the wallet's entire `Sent` history — on every sweep, forever. + #[tokio::test] + async fn reconcile_sent_payments_from_tx_history_reconstructs_around_an_unreadable_record() { + use dashcore::hashes::Hash; + use dashcore::BlockHash; + use key_wallet::managed_account::transaction_record::OutputRole; + use key_wallet::transaction_checking::{BlockInfo, TransactionContext}; + + let persister = Arc::new(RecordStorePersister::default()); + let (manager, wallet_id) = make_wallet_with(Arc::clone(&persister)).await; + let owner = Identifier::from([0xAA; 32]); + let contact = Identifier::from([0xBB; 32]); + + let wallet = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet.identity(); + let p = WalletPersister::new(wallet_id, Arc::clone(&persister) as _); + { + let mut wm = iw.wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("info"); + info.identity_manager + .add_identity(bare_identity([0xAA; 32]), 0, wallet_id, &p) + .expect("add owner"); + } + let contact_addresses = install_external_account(&manager, wallet_id, owner, contact).await; + let change_address = first_standard_wallet_address(&manager, wallet_id).await; + + let readable = tx_record_with_outputs( + TransactionContext::InBlock(BlockInfo::new(123, BlockHash::all_zeros(), 0)), + vec![ + (contact_addresses[0].clone(), 25_000, OutputRole::Sent), + (change_address.clone(), 90_000, OutputRole::Change), + ], + ); + let corrupt = tx_record_with_outputs( + TransactionContext::InBlock(BlockInfo::new(124, BlockHash::all_zeros(), 0)), + vec![ + (contact_addresses[1].clone(), 30_000, OutputRole::Sent), + (change_address, 80_000, OutputRole::Change), + ], + ); + let readable_txid = readable.txid; + let corrupt_txid = corrupt.txid; + assert_ne!(readable_txid, corrupt_txid, "the rows must be distinct"); + { + let mut records = persister.records.lock().unwrap(); + records.insert(readable_txid, readable); + records.insert(corrupt_txid, corrupt); + } + persister + .permanently_unreadable + .lock() + .unwrap() + .insert(corrupt_txid); + + let err = iw + .dashpay() + .reconcile_sent_payments_from_tx_history() + .await + .expect_err("a permanent read failure must still be reported"); + assert!( + matches!( + err, + PlatformWalletError::PersisterLoad(ref source) if !source.is_transient() + ), + "expected a permanent PersisterLoad, got {err:?}" + ); + + let wm = iw.wallet_manager.read().await; + let info = wm.get_wallet_info(&wallet_id).expect("info"); + let payments = &info + .identity_manager + .managed_identity(&owner) + .expect("managed") + .dashpay() + .payments; + assert!( + payments.contains_key(&readable_txid.to_string()), + "the readable record must still be reconstructed: one unreadable row \ + may not void the rest of the sweep" + ); + assert!( + !payments.contains_key(&corrupt_txid.to_string()), + "the unreadable record has nothing to reconstruct from" + ); + } + #[tokio::test] async fn reconcile_sent_payments_from_tx_history_does_not_overwrite_existing_entry() { use dashcore::hashes::Hash; diff --git a/packages/rs-platform-wallet/src/wallet/persister.rs b/packages/rs-platform-wallet/src/wallet/persister.rs index e6cc78affaa..fec0ce4638f 100644 --- a/packages/rs-platform-wallet/src/wallet/persister.rs +++ b/packages/rs-platform-wallet/src/wallet/persister.rs @@ -16,6 +16,36 @@ use crate::changeset::{ use crate::wallet::platform_wallet::WalletId; use dpp::prelude::Identifier; +/// Transient tx-record misses collapsed during one wait or sweep, reported as +/// a single line when it ends. +/// +/// Owned by the caller and dropped at its exit, so every path out — success, +/// timeout, early return — reports exactly once, and a backend that is merely +/// busy cannot flood the log from inside an unbounded poll loop. +#[derive(Debug, Default)] +pub(crate) struct TransientMissTally { + misses: usize, +} + +impl TransientMissTally { + #[cfg(test)] + pub(crate) fn misses(&self) -> usize { + self.misses + } +} + +impl Drop for TransientMissTally { + fn drop(&mut self) { + if self.misses > 0 { + tracing::debug!( + transient_misses = self.misses, + "Core tx-record reads hit transient backend failures during this pass; \ + each was read as a miss and will be retried" + ); + } + } +} + /// Per-wallet persistence handle. /// /// Thin wrapper around the shared [`PlatformWalletPersistence`] that binds @@ -64,6 +94,37 @@ impl WalletPersister { self.inner.get_core_tx_record(self.wallet_id, txid) } + /// [`Self::get_core_tx_record`] with the shared transient-as-miss policy. + /// + /// A busy store is indistinguishable in outcome from "the row is not + /// readable right now", and every caller here already retries a miss on its + /// next pass, so a transient failure collapses to `Ok(None)`. A permanent + /// one stays an `Err`: it will not fix itself, so swallowing it would + /// repeat the same doomed work forever with no signal. Use + /// [`Self::get_core_tx_record`] directly to tell the two apart. + /// + /// Every caller is a poll loop or a per-txid sweep, so the collapse is + /// counted into `tally` and reported once when that wait or sweep ends, + /// rather than logged per call. + pub(crate) fn get_core_tx_record_or_transient_miss( + &self, + txid: &Txid, + tally: &mut TransientMissTally, + ) -> Result, PersistenceError> { + match self.get_core_tx_record(txid) { + Err(e) if e.is_transient() => { + tally.misses += 1; + tracing::trace!( + %txid, + error = %e, + "Core tx-record read hit a transient backend failure; reading as a miss" + ); + Ok(None) + } + other => other, + } + } + /// Enumerate the persisted Core transaction ids scoped to this /// wallet, tagged with the host's wallet-funded verdict. Used by /// DashPay sent-payment reconstruction to fetch the full records diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs index dad43b97310..4f03bf52c65 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs @@ -1859,6 +1859,13 @@ impl PlatformWallet { } /// Load persisted state for this wallet. + /// + /// Calls the backend inline, with neither the transient retry nor the + /// `spawn_blocking` offload that + /// [`PlatformWalletManager::load_from_persistor`](crate::manager::PlatformWalletManager::load_from_persistor) + /// and wallet registration wrap their loads in: a transient failure + /// surfaces immediately instead of being retried, and a slow backend + /// blocks the calling thread — an async caller's runtime worker included. pub fn load_persisted(&self) -> Result { self.persister.load() } @@ -1928,6 +1935,10 @@ impl PlatformWallet { /// accounts that exist at that point; a second call after /// account bootstrap picks up the rest without regressing /// anything. + /// + /// Inherits [`load_persisted`](Self::load_persisted)'s inline read: no + /// transient retry, no offload. A host that wants either must wrap this + /// call itself. pub async fn load_and_apply_persisted( &self, ) -> Result<(), Box> { diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift index 49e97964602..74b7446411e 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift @@ -27,11 +27,41 @@ struct LiveModelFetcher: ModelFetching { } } +/// Return values by which a persistence callback classifies its own failure. +/// +/// The ABI is defined by `PLATFORM_WALLET_PERSIST_RC_*` in +/// `packages/rs-platform-wallet-ffi/src/persistence.rs` and must change only +/// together with it. Named here so no callback ever spells the literal — the +/// same integers mean unrelated things in other native callback families. +public enum PlatformWalletPersistRC { + /// A retryable failure after which **nothing was applied**. Returning it + /// from a callback inside a changeset round also asserts that the failed + /// round was rolled back whole. + public static let transient: Int32 = -2 + /// A constraint / integrity violation — the data is wrong, not the store. + public static let constraint: Int32 = -3 +} + /// Bridges FFI persistence callbacks to SwiftData storage. /// /// Allocated as a class so its pointer can be passed as the opaque `context` /// to the Rust persistence callbacks. Must be retained for the lifetime of /// the `PlatformWalletManager`. +/// +/// Callback return values: `0` succeeds and any non-zero value fails. A +/// plain non-zero failure means "do not retry". A callback that can +/// classify its own failure may instead return +/// `PlatformWalletPersistRC.transient` for a retryable failure +/// after which nothing was applied, or +/// `PlatformWalletPersistRC.constraint` for an integrity +/// violation; Rust forwards the classification to its caller (as +/// `PlatformWalletError.persisterStoreTransient` and friends) and never +/// retries on this handler's behalf. Returning the transient sentinel from +/// a callback inside a changeset round additionally asserts that a failed +/// round is rolled back whole — which this handler does, via +/// `endChangeset(success: false)`. The handlers below currently return +/// only `0` / `1` / `-1`, so they always read as fatal; opting in is a +/// per-callback change. // All mutable state (`backgroundContext`, caches) is confined to `serialQueue` // — the handler's de-facto actor — so it is safe to hand to a `@Sendable` // closure (e.g. the off-main `serialQueue.async` backfill dispatch). @@ -3191,6 +3221,14 @@ public final class PlatformWalletPersistenceHandler: @unchecked Sendable { /// the C shim so `store()` reports a persistence failure instead of /// silently advancing its in-memory state (pending queues, cleared drain /// entries, ignored-sender deltas) against writes that never reached disk. + /// + /// Failing this call when `success` is already `false` means the rollback + /// itself did not complete, so the round's disposition is unknown. Rust + /// classifies that as fatal and will not invite a re-send, regardless of + /// any retry sentinel returned here — re-issuing a changeset the store + /// could neither apply nor undo risks merging it twice. A retry sentinel + /// is only honoured on a *clean* round, where the commit failed but the + /// rollback succeeded and nothing was left behind. @discardableResult func endChangeset(walletId: Data, success: Bool) -> Bool { onQueue { diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift index 5b07dcbda8c..78f7ebf7e22 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift @@ -216,6 +216,36 @@ public enum PlatformWalletResultCode: Int32, Sendable { /// the Rust-side scan cannot see conflicts whose spender was already /// pruned. case errorAssetLockInputContested = 48 + /// Reading persisted wallet state failed on a store that reported the + /// failure as retryable (`SQLITE_BUSY` and friends). Nothing was + /// mutated — a load is a read. Retry later. + case errorPersisterLoadTransient = 49 + /// Reading persisted wallet state failed permanently — a corrupt or + /// unreadable store, or a decode that will fail identically next time. + /// Do NOT retry; inspect the message. Constraint-class failures fold in + /// here too: a read cannot violate one, and neither is retryable. + case errorPersisterLoadFatal = 50 + /// Writing wallet state failed on a busy or momentarily unavailable + /// store. **Nothing was committed** — the SDK only reports this when the + /// persister rolls a failed changeset round back whole, so re-issuing the + /// operation cannot double-apply part of it. Retry later. + case errorPersisterStoreTransient = 51 + /// Writing wallet state failed permanently — a full disk, a corrupt + /// schema, an I/O error outside the retryable class. Do NOT retry; + /// inspect the message. The wallet rolled its in-memory state back, so + /// the operation may be re-attempted once the fault is fixed. + case errorPersisterStoreFatal = 52 + /// A write violated a constraint / foreign key / integrity rule. + /// Deliberately distinct from `errorPersisterStoreFatal`: this is "the + /// data is wrong" (a caller or schema-mapping bug) rather than "the + /// storage engine is unhappy" (an operator problem), and the two route + /// to different people. Do NOT retry unchanged; fix the data. + case errorPersisterStoreConstraint = 53 + /// Rehydrating persisted platform-address state into a freshly + /// registered wallet failed. One code rather than three: it wraps a + /// wallet error, not a store error, so it carries no retry + /// classification. The wrapped error's rendering is in the message. + case errorPersisterRestore = 54 /// 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. @@ -323,6 +353,18 @@ public enum PlatformWalletResultCode: Int32, Sendable { self = .errorAssetLockInputConflict case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_ASSET_LOCK_INPUT_CONTESTED: self = .errorAssetLockInputContested + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_TRANSIENT: + self = .errorPersisterLoadTransient + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_FATAL: + self = .errorPersisterLoadFatal + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_TRANSIENT: + self = .errorPersisterStoreTransient + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_FATAL: + self = .errorPersisterStoreFatal + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_CONSTRAINT: + self = .errorPersisterStoreConstraint + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_RESTORE: + self = .errorPersisterRestore case PLATFORM_WALLET_FFI_RESULT_CODE_NOT_FOUND: self = .notFound case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_UNKNOWN: @@ -554,6 +596,28 @@ public enum PlatformWalletError: LocalizedError { /// confirmed spender is this wallet's own transaction, so the value /// behind the contested input lives on in it. case assetLockInputContested(String) + /// Reading persisted wallet state failed on a store that classified the + /// failure as retryable. Nothing was mutated — retry later. One of the + /// two retryable persister cases, alongside `persisterStoreTransient`. + case persisterLoadTransient(String) + /// Reading persisted wallet state failed permanently. Do NOT retry; + /// the store needs repair or re-provisioning. + case persisterLoadFatal(String) + /// Writing wallet state failed on a busy store, with the whole changeset + /// round rolled back — nothing was committed, so re-issuing the + /// operation is safe. Retry later. + case persisterStoreTransient(String) + /// Writing wallet state failed permanently. Do NOT retry until the + /// underlying fault is fixed; the wallet rolled its in-memory state back. + case persisterStoreFatal(String) + /// A write violated a constraint / integrity rule — the data is wrong, + /// as opposed to the storage engine being unhappy. Do NOT retry + /// unchanged. + case persisterStoreConstraint(String) + /// Rehydrating persisted platform-address state into a newly registered + /// wallet failed. Carries no retry classification: it wraps a wallet + /// error rather than a store error. + case persisterRestore(String) /// The named thing does not exist. For the deferred payment calls this is /// the wallet-was-REMOVED case: the token's wallet (or the wallet a payment /// was just signed against) is no longer registered in the manager, so there @@ -564,9 +628,13 @@ public enum PlatformWalletError: LocalizedError { case notFound(String) case unknown(String) - /// Diagnostic detail Rust attached to the originating - /// `PlatformWalletFFIResult`, or the context string a Swift-side - /// guard chose when constructing the error inline. + /// What to show a person. For most cases this is still the diagnostic + /// detail Rust attached to the originating `PlatformWalletFFIResult` (or + /// the context string a Swift-side guard chose when constructing the + /// error inline); the persister cases and the value-carrying marketplace + /// rejections compose their own text instead, because theirs is an error + /// chain or a JSON payload that reads as gibberish in an alert. The + /// persister chain stays available on `failureReason`. public var errorDescription: String? { switch self { case .nullPointer(let m), .invalidHandle(let m), .invalidParameter(let m), @@ -594,6 +662,20 @@ public enum PlatformWalletError: LocalizedError { .assetLockInputContested(let m), .notFound(let m), .unknown(let m): return m + // The persister messages are a nested Rust error chain naming the + // operation, the backend classification and the store's own phrasing + // ("… changeset: persistence backend error (Transient): database is + // locked"). That is log material, not alert material, so these six + // state what the person can do and leave the chain on + // `failureReason`. Which text applies is the CASE's meaning: a + // transient is worth retrying, a read failure and a write failure + // must not be described to a user as each other. + case .persisterLoadTransient, .persisterStoreTransient: + return "The wallet database is busy. Try again in a moment." + case .persisterLoadFatal, .persisterRestore: + return "The wallet data could not be read and may need to be restored." + case .persisterStoreFatal, .persisterStoreConstraint: + return "The wallet data could not be saved and may need to be restored." // The three value-carrying marketplace rejections compose their // description from the typed values, because their FFI message is // the machine-readable JSON detail — showing that raw would be @@ -613,6 +695,20 @@ public enum PlatformWalletError: LocalizedError { } } + /// The raw diagnostic chain behind a case whose `errorDescription` is + /// user-facing text — log it, do not display it. `nil` for every case + /// that already passes its detail through as the description. + public var failureReason: String? { + switch self { + case .persisterLoadTransient(let m), .persisterLoadFatal(let m), + .persisterStoreTransient(let m), .persisterStoreFatal(let m), + .persisterStoreConstraint(let m), .persisterRestore(let m): + return m + default: + return nil + } + } + init(result: PlatformWalletResult) { self.init(code: result.code, message: result.message) } @@ -718,6 +814,23 @@ public enum PlatformWalletError: LocalizedError { self = .assetLockInputConflict(detail) case .errorAssetLockInputContested: self = .assetLockInputContested(detail) + // The persister codes carry the wallet's typed `Display` as the + // message. Which operation failed and whether a retry can help is + // the CODE's meaning, not the string's — branch on the case, never + // on the text, and log the string rather than displaying it + // (`errorDescription` holds the user-facing wording). + case .errorPersisterLoadTransient: + self = .persisterLoadTransient(detail) + case .errorPersisterLoadFatal: + self = .persisterLoadFatal(detail) + case .errorPersisterStoreTransient: + self = .persisterStoreTransient(detail) + case .errorPersisterStoreFatal: + self = .persisterStoreFatal(detail) + case .errorPersisterStoreConstraint: + self = .persisterStoreConstraint(detail) + case .errorPersisterRestore: + self = .persisterRestore(detail) case .notFound: self = .notFound(detail) case .errorUnknown: self = .unknown(detail) } diff --git a/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift b/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift index 9b375b852d4..f629563cb73 100644 --- a/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift +++ b/packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift @@ -73,6 +73,127 @@ final class ErrorHandlingTests: XCTestCase { XCTAssertEqual(error.errorDescription, rendered) } + /// The persister block (49-54). Each code must decode from its + /// generated C constant, keep its own raw value, and reach a typed + /// `PlatformWalletError` case — the three edits a new code needs on + /// this side. Without the `init(ffi:)` arm a code compiles fine and + /// silently degrades to `.errorUnknown`, losing the classification the + /// Rust side went to the trouble of carrying across. + func testPersisterFFIResultMappings() { + let mappings: [(PlatformWalletFFIResultCode, PlatformWalletResultCode, Int32)] = [ + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_TRANSIENT, + .errorPersisterLoadTransient, 49 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_FATAL, + .errorPersisterLoadFatal, 50 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_TRANSIENT, + .errorPersisterStoreTransient, 51 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_FATAL, + .errorPersisterStoreFatal, 52 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_STORE_CONSTRAINT, + .errorPersisterStoreConstraint, 53 + ), + ( + PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_RESTORE, + .errorPersisterRestore, 54 + ), + ] + + for (ffi, expected, rawValue) in mappings { + XCTAssertEqual(PlatformWalletResultCode(ffi: ffi), expected) + XCTAssertNotEqual(PlatformWalletResultCode(ffi: ffi), .errorUnknown) + // Hand-mirrored ABI, not a derived ordinal. + XCTAssertEqual(expected.rawValue, rawValue) + } + } + + /// The two retryable persister codes must arrive as their own typed + /// cases carrying the Rust message, and must not be confused with the + /// non-retryable siblings that share an operation. + func testPersisterTypedErrorCases() { + let busy = "failed to persist wallet registration changeset: " + + "persistence backend error (Transient): database is locked" + let storeTransient = PlatformWalletError( + code: .errorPersisterStoreTransient, + message: busy + ) + guard case .persisterStoreTransient(let storeMessage) = storeTransient else { + return XCTFail("expected typed persisterStoreTransient error") + } + XCTAssertEqual(storeMessage, busy) + // The chain stays reachable for logs — on the associated value and + // on failureReason — but must never be the alert text. + XCTAssertEqual(storeTransient.failureReason, busy) + + guard case .persisterStoreConstraint = PlatformWalletError( + code: .errorPersisterStoreConstraint, + message: "constraint failed" + ) else { + return XCTFail("a constraint violation must not read as a transient or fatal store") + } + + guard case .persisterLoadTransient = PlatformWalletError( + code: .errorPersisterLoadTransient, + message: busy + ) else { + return XCTFail("expected typed persisterLoadTransient error") + } + + guard case .persisterRestore(let restoreMessage) = PlatformWalletError( + code: .errorPersisterRestore, + message: "failed to restore persisted platform-address state: wallet is locked" + ) else { + return XCTFail("expected typed persisterRestore error") + } + XCTAssertEqual( + restoreMessage, + "failed to restore persisted platform-address state: wallet is locked" + ) + } + + /// `errorDescription` is what a default SwiftUI alert renders, so the + /// persister cases must answer it with an instruction rather than the + /// Rust error chain — and must not describe a failed write as a failed + /// read. The chain belongs on `failureReason`. + func testPersisterErrorsSplitUserTextFromDiagnostics() { + let busy = "failed to persist wallet registration changeset: " + + "persistence backend error (Transient): database is locked" + let expected: [(PlatformWalletResultCode, String)] = [ + (.errorPersisterLoadTransient, "The wallet database is busy. Try again in a moment."), + (.errorPersisterStoreTransient, "The wallet database is busy. Try again in a moment."), + ( + .errorPersisterLoadFatal, + "The wallet data could not be read and may need to be restored." + ), + ( + .errorPersisterRestore, + "The wallet data could not be read and may need to be restored." + ), + ( + .errorPersisterStoreFatal, + "The wallet data could not be saved and may need to be restored." + ), + ( + .errorPersisterStoreConstraint, + "The wallet data could not be saved and may need to be restored." + ), + ] + + for (code, userText) in expected { + let error = PlatformWalletError(code: code, message: busy) + XCTAssertEqual(error.errorDescription, userText, "code \(code)") + XCTAssertEqual(error.failureReason, busy, "code \(code) must keep the chain for logs") + } + } + func testPlatformWalletNotFoundFFIResultMapping() { // Code 98 (the blanket Option→result miss) stays typed inside the // wallet-error family — the mapping Kotlin now converges on