From c97ecf376fb297d835423df8997b640a19db5fdf Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:40:42 -0400 Subject: [PATCH 01/12] fix(wallet): surface RecoveredFromChain asset locks on host resume paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chain-locked enrichment promotes tracked asset locks to `AssetLockStatus::RecoveredFromChain` (discriminant 5) in `sync/reconstruction.rs`, but every host resume surface expressed "still recoverable" as the contiguous range `1..3`. Status 5 sits above the terminal `Consumed` (4) numerically while being decidedly non-terminal, so each of those filters silently dropped exactly the rows the restore scan had just rebuilt. User-visible effect: an address top-up that was funded and chain-locked before a wallet restore appears on no surface at all — not the Pending Platform Top Ups list, not the Resumable Registrations list — and the Swift status label rendered it as "Unknown(5)". The funds are intact and Rust will happily resume them, but nothing in the UI can reach them, so they read as lost. Changes: - `AssetLockDao.observeResumableAddressTopUps` admits `1..3 ∪ {5}`. `4` stays excluded: it is the terminal tombstone that `resume_asset_lock` rejects, and re-surfacing it would produce the perpetual-spinner row the #4347 guard exists to prevent. - New `AssetLockDao.observeResumableTopUpsByFundingType`. Shielded address top-ups (funding type 5) previously had no resumable query at all — the address query is pinned to funding type 4, and the identity-recovery surface behind `TrackedAssetLock.eligibleFromNative` deliberately admits only funding types 0..2 — so a stalled shielded top-up was invisible everywhere. - Swift `isVisibleAsResumable` / `canFundIdentity` accept 5, and `statusLabel` names it. A `5` carries a real `ChainAssetLockProof`, so it is as fundable as a `3`; what is unknown is Platform-side consumption, and Platform is the arbiter of that. - `IdentitiesContentView.crossWalletResumableLocks` now reuses `isVisibleAsResumable` instead of restating the range inline. `TrackedAssetLock.FundingType` is deliberately NOT widened to funding types 4/5. That enum is the identity-recovery eligibility filter, and its consumers assert on it (`IdentityRegistration` requires IDENTITY_REGISTRATION, `IdentityCredits` requires the two top-up variants). Admitting address/shielded locks there would push them into pickers whose `require(...)` then throws — a new crash path, not a fix. The address/shielded recovery surface is the DAO query above. Tests: 7 new Robolectric Room tests pinning both ends of the domain (5 in, 4 out, 0 out, funding-type and wallet scoping intact), plus a Swift case asserting status 5 is resumable. --- .../dashsdk/persistence/dao/AssetLockDao.kt | 48 ++++- .../persistence/entities/AssetLockEntity.kt | 10 +- .../persistence/AssetLockResumableDaoTest.kt | 182 ++++++++++++++++++ .../Core/Views/IdentitiesContentView.swift | 32 +-- .../Utils/PersistentAssetLockDisplay.swift | 49 +++-- .../CreateIdentityResumableTests.swift | 30 ++- 6 files changed, 315 insertions(+), 36 deletions(-) create mode 100644 packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/AssetLockResumableDaoTest.kt diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt index dddd64d56db..339f2b36bb9 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt @@ -59,18 +59,54 @@ interface AssetLockDao { /** * Resumable Platform-address top-up locks — `fundingTypeRaw == 4` - * (AssetLockAddressTopUp) and `statusRaw ∈ [1, 3]` (Broadcast through - * ChainLocked, excluding Built and Consumed). Backs the "Pending - * Platform Top Ups" orphan surface (← the SwiftData `@Query` behind - * `PendingPlatformFundFromAssetLocksList.swift`, whose Swift filter is - * `fundingTypeRaw == 4 && isVisibleAsResumable`). + * (AssetLockAddressTopUp) and a recoverable, non-terminal status. + * Backs the "Pending Platform Top Ups" orphan surface (← the SwiftData + * `@Query` behind `PendingPlatformFundFromAssetLocksList.swift`, whose + * Swift filter is `fundingTypeRaw == 4 && isVisibleAsResumable`). + * + * The recoverable set is `[1, 3] ∪ {5}` — Broadcast, InstantSendLocked, + * ChainLocked, and RecoveredFromChain. `0` (Built) is excluded because + * the funding transaction has not been broadcast; `4` (Consumed) is the + * terminal tombstone that Rust's `resume_asset_lock` rejects outright. + * + * `5` (RecoveredFromChain) is not a gap in the ordering — it is a + * distinct status written by the restore scan and by the chainlock + * promotion path for a lock whose Core finality is proven but whose + * Platform-side consumption is unknown (`sync/reconstruction.rs`). A + * range bounded at `3` dropped exactly those rows, so a chain-locked + * address top-up rebuilt from history appeared on no surface at all. */ @Query( "SELECT * FROM asset_locks WHERE walletId = :walletId " + - "AND fundingTypeRaw = 4 AND statusRaw >= 1 AND statusRaw <= 3" + "AND fundingTypeRaw = 4 " + + "AND ((statusRaw >= 1 AND statusRaw <= 3) OR statusRaw = 5)" ) fun observeResumableAddressTopUps(walletId: ByteArray): Flow> + /** + * Funding-type-scoped variant of [observeResumableAddressTopUps], using + * the identical recoverable-status predicate. + * + * Exists because shielded address top-ups (`fundingTypeRaw == 5`, + * `AssetLockShieldedAddressTopUp`) had no resumable query at all: the + * query above is pinned to `4`, and the identity-recovery surface fed by + * `TrackedAssetLock.eligibleFromNative` deliberately admits only funding + * types `0..2`. A stalled or chain-locked shielded top-up was therefore + * invisible on every host surface. + * + * Pass `4` (address) or `5` (shielded). Funding types `0..3` are + * identity-family locks, whose recovery surface is the identity screens. + */ + @Query( + "SELECT * FROM asset_locks WHERE walletId = :walletId " + + "AND fundingTypeRaw = :fundingTypeRaw " + + "AND ((statusRaw >= 1 AND statusRaw <= 3) OR statusRaw = 5)" + ) + fun observeResumableTopUpsByFundingType( + walletId: ByteArray, + fundingTypeRaw: Int, + ): Flow> + @Query("SELECT * FROM asset_locks WHERE outPointHex = :outPointHex") suspend fun getByOutPointHex(outPointHex: String): AssetLockEntity? diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/AssetLockEntity.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/AssetLockEntity.kt index 5181fe92545..c5505e3746b 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/AssetLockEntity.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/AssetLockEntity.kt @@ -40,7 +40,15 @@ data class AssetLockEntity( val amountDuffs: Long, /** * `AssetLockStatus` discriminant: 0 Built, 1 Broadcast, - * 2 InstantSendLocked, 3 ChainLocked, 4 Consumed. + * 2 InstantSendLocked, 3 ChainLocked, 4 Consumed, + * 5 RecoveredFromChain. + * + * `4` is terminal; `5` is NOT, its higher discriminant + * notwithstanding. `5` means Core finality is proven while + * Platform-side consumption is unknown — what the restore scan and the + * chainlock-promotion path write — so it belongs in every "still + * recoverable" predicate alongside `1..3`, and a contiguous `1..3` + * range silently drops it. */ val statusRaw: Int, /** diff --git a/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/AssetLockResumableDaoTest.kt b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/AssetLockResumableDaoTest.kt new file mode 100644 index 00000000000..0c5cdebcf0f --- /dev/null +++ b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/AssetLockResumableDaoTest.kt @@ -0,0 +1,182 @@ +package org.dashfoundation.dashsdk.persistence + +import androidx.test.core.app.ApplicationProvider +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +import org.dashfoundation.dashsdk.persistence.entities.AssetLockEntity +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * In-memory Room contract tests for the resumable asset-lock predicates on + * [org.dashfoundation.dashsdk.persistence.dao.AssetLockDao]. + * + * The behavior under test is the status domain. Rust's `AssetLockStatus` + * is `0 Built, 1 Broadcast, 2 InstantSendLocked, 3 ChainLocked, + * 4 Consumed, 5 RecoveredFromChain`, and the recoverable set is NOT a + * contiguous range: `4` is the terminal tombstone that must stay hidden, + * while `5` — written by the restore scan and the chainlock-promotion path + * for a lock with proven Core finality and unknown Platform-side + * consumption — must be visible. + * + * Expressing that as `statusRaw >= 1 AND statusRaw <= 3` dropped every + * recovered row, so a chain-locked top-up the user really funded appeared + * on no host surface at all. These tests pin both ends: `5` in, `4` out. + */ +@RunWith(RobolectricTestRunner::class) +class AssetLockResumableDaoTest { + + private lateinit var db: DashDatabase + + private val walletId = ByteArray(32) { 1 } + private val otherWalletId = ByteArray(32) { 2 } + + /** `AssetLockFundingType` discriminants. */ + private val fundingAddressTopUp = 4 + private val fundingShieldedTopUp = 5 + private val fundingIdentityRegistration = 0 + + @Before + fun setUp() { + db = DashDatabase.createInMemory(ApplicationProvider.getApplicationContext()) + } + + @After + fun tearDown() { + db.close() + } + + private suspend fun insert( + outPointHex: String, + statusRaw: Int, + fundingTypeRaw: Int = fundingAddressTopUp, + owner: ByteArray = walletId, + ) { + db.assetLockDao().upsert( + AssetLockEntity( + outPointHex = outPointHex, + walletId = owner, + transactionBytes = ByteArray(4), + fundingTypeRaw = fundingTypeRaw, + identityIndexRaw = 0, + amountDuffs = 10_000, + statusRaw = statusRaw, + ), + ) + } + + private suspend fun resumableAddressOutpoints(): List = + db.assetLockDao().observeResumableAddressTopUps(walletId).first() + .map { it.outPointHex } + .sorted() + + // ── observeResumableAddressTopUps ───────────────────────────────── + + /** + * The regression itself: status 5 must be returned. Before the fix the + * upper bound of `3` hid it, and the funds it represents were + * unreachable from the UI. + */ + @Test + fun resumableAddressTopUpsIncludeRecoveredFromChain() = runTest { + insert("aa:0", statusRaw = 5) + + assertEquals(listOf("aa:0"), resumableAddressOutpoints()) + } + + /** Every recoverable status, and only those. */ + @Test + fun resumableAddressTopUpsCoverTheWholeRecoverableDomain() = runTest { + insert("built:0", statusRaw = 0) + insert("broadcast:0", statusRaw = 1) + insert("islocked:0", statusRaw = 2) + insert("chainlocked:0", statusRaw = 3) + insert("consumed:0", statusRaw = 4) + insert("recovered:0", statusRaw = 5) + + assertEquals( + listOf("broadcast:0", "chainlocked:0", "islocked:0", "recovered:0"), + resumableAddressOutpoints(), + ) + } + + /** + * The terminal guard from #4347 must survive the widening. A Consumed + * row that re-surfaced would be a perpetual-spinner the underlying + * `resume_asset_lock` rejects with "already Consumed — nothing to + * resume". + */ + @Test + fun resumableAddressTopUpsStillExcludeConsumed() = runTest { + insert("consumed:0", statusRaw = 4) + + assertEquals(emptyList(), resumableAddressOutpoints()) + } + + /** Built (0) stays out: nothing has been broadcast yet. */ + @Test + fun resumableAddressTopUpsStillExcludeBuilt() = runTest { + insert("built:0", statusRaw = 0) + + assertEquals(emptyList(), resumableAddressOutpoints()) + } + + /** The funding-type and wallet scoping are unchanged by the widening. */ + @Test + fun resumableAddressTopUpsStayScopedToFundingTypeFourAndWallet() = runTest { + insert("identity:0", statusRaw = 5, fundingTypeRaw = fundingIdentityRegistration) + insert("shielded:0", statusRaw = 5, fundingTypeRaw = fundingShieldedTopUp) + insert("foreign:0", statusRaw = 5, owner = otherWalletId) + insert("mine:0", statusRaw = 5) + + assertEquals(listOf("mine:0"), resumableAddressOutpoints()) + } + + // ── observeResumableTopUpsByFundingType ─────────────────────────── + + /** + * Shielded address top-ups (funding type 5) previously had no resumable + * query at all — the address query is pinned to 4, and the + * identity-recovery surface (`TrackedAssetLock.eligibleFromNative`) + * deliberately admits only funding types 0..2 — so a stalled shielded + * top-up was invisible everywhere. + */ + @Test + fun resumableByFundingTypeCoversShieldedTopUps() = runTest { + insert("shielded-broadcast:0", statusRaw = 1, fundingTypeRaw = fundingShieldedTopUp) + insert("shielded-recovered:0", statusRaw = 5, fundingTypeRaw = fundingShieldedTopUp) + insert("shielded-consumed:0", statusRaw = 4, fundingTypeRaw = fundingShieldedTopUp) + insert("shielded-built:0", statusRaw = 0, fundingTypeRaw = fundingShieldedTopUp) + insert("address-recovered:0", statusRaw = 5, fundingTypeRaw = fundingAddressTopUp) + + val shielded = db.assetLockDao() + .observeResumableTopUpsByFundingType(walletId, fundingShieldedTopUp) + .first() + .map { it.outPointHex } + .sorted() + + assertEquals(listOf("shielded-broadcast:0", "shielded-recovered:0"), shielded) + } + + /** Parameterized with 4, it agrees with the dedicated address query. */ + @Test + fun resumableByFundingTypeAgreesWithTheAddressQuery() = runTest { + insert("a:0", statusRaw = 1) + insert("b:0", statusRaw = 5) + insert("c:0", statusRaw = 4) + insert("d:0", statusRaw = 0) + + val parameterized = db.assetLockDao() + .observeResumableTopUpsByFundingType(walletId, fundingAddressTopUp) + .first() + .map { it.outPointHex } + .sorted() + + assertEquals(resumableAddressOutpoints(), parameterized) + assertEquals(listOf("a:0", "b:0"), parameterized) + } +} diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift index 794331c1cf6..a5bca7088c5 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/IdentitiesContentView.swift @@ -426,17 +426,25 @@ struct IdentitiesContentView: View { /// out: a tight crash window between TX build and broadcast /// with no useful UX action to take. /// - /// The upper bound (`<= 3`, ChainLocked) excludes `statusRaw == 4` - /// (Consumed) — the terminal state set when a lock has already - /// funded an identity. In the happy path the anti-join against - /// `PersistentIdentity` would hide a Consumed lock anyway - /// (every successful registration writes both rows at the same - /// slot), but the local-only delete-identity action removes - /// the identity row WITHOUT the asset-lock row, which frees the - /// slot in the anti-join. Without this upper bound a Consumed - /// lock would re-surface as a Resume row that can't advance — - /// `resume_asset_lock` rejects Consumed entries with - /// "already Consumed — nothing to resume". + /// `statusRaw == 4` (Consumed) is excluded — the terminal state + /// set when a lock has already funded an identity. In the happy + /// path the anti-join against `PersistentIdentity` would hide a + /// Consumed lock anyway (every successful registration writes + /// both rows at the same slot), but the local-only + /// delete-identity action removes the identity row WITHOUT the + /// asset-lock row, which frees the slot in the anti-join. + /// Without excluding it, a Consumed lock would re-surface as a + /// Resume row that can't advance — `resume_asset_lock` rejects + /// Consumed entries with "already Consumed — nothing to resume". + /// + /// `statusRaw == 5` (RecoveredFromChain) IS admitted. It is not a + /// state beyond Consumed despite the higher discriminant: it is + /// what the restore scan / chainlock-promotion path writes for a + /// lock with proven Core finality and unknown Platform-side + /// consumption, and a user-driven Resume is precisely the surface + /// allowed to try consuming one. Expressing the predicate as the + /// contiguous range `1...3` hid every recovered lock, so an + /// identity funded before a restore had no resume row at all. /// /// Generic over `AssetLockResumeRow` so the pure filter is /// unit-testable without a SwiftData container. @@ -446,7 +454,7 @@ struct IdentitiesContentView: View { ) -> [R] { locks.filter { lock in guard lock.fundingTypeRaw >= 0 && lock.fundingTypeRaw <= 2 else { return false } - guard lock.statusRaw >= 1 && lock.statusRaw <= 3 else { return false } + guard lock.isVisibleAsResumable else { return false } let slot = UInt32(bitPattern: lock.identityIndexRaw) return !usedSlots.contains( UsedSlot(walletId: lock.walletId, slot: slot) diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift index 9c51496a217..c6a8c78e591 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Utils/PersistentAssetLockDisplay.swift @@ -26,7 +26,18 @@ extension AssetLockResumeRow { /// Platform layer. Consumed (4) was already used to fund an /// identity and cannot be reused; the persisted row survives /// only for historical lookup. - var canFundIdentity: Bool { statusRaw == 2 || statusRaw == 3 } + /// + /// RecoveredFromChain (5) also qualifies: the restore scan and the + /// chainlock-promotion path attach a real `ChainAssetLockProof` + /// before writing that status, so the lock is exactly as fundable + /// as a ChainLocked (3) one. What is unknown for a `5` is whether + /// Platform already consumed it — and Platform, not the client, is + /// the arbiter of that: it rejects an already-spent outpoint with a + /// typed error. A user-driven Resume is the surface allowed to ask. + /// (Do not feed `5` into an automatic retry sweep — blind retries + /// of historical locks are the failure mode the status exists to + /// prevent.) + var canFundIdentity: Bool { statusRaw == 2 || statusRaw == 3 || statusRaw == 5 } /// `true` when the lock should be surfaced on the Resumable /// Registrations section at all. Lower bar than `canFundIdentity` @@ -35,22 +46,31 @@ extension AssetLockResumeRow { /// in-flight crash-recovery situation has visible continuity /// through the IS-lock arrival. /// - /// Upper bound at `3` (ChainLocked) is load-bearing: status `4` - /// (Consumed) is the terminal state for a lock that already - /// funded an identity, and the underlying `resume_asset_lock` - /// rejects Consumed entries with "already Consumed — nothing - /// to resume". Without the upper bound, the "Delete identity - /// locally" action — which removes `PersistentIdentity` but - /// leaves the `PersistentAssetLock` alive — frees the - /// `(walletId, identityIndex)` slot in the anti-join and the - /// Consumed row would re-surface as a perpetual-spinner row - /// that can't be advanced. - var isVisibleAsResumable: Bool { statusRaw >= 1 && statusRaw <= 3 } + /// Excluding `4` (Consumed) is load-bearing: it is the terminal + /// state for a lock that already funded an identity, and the + /// underlying `resume_asset_lock` rejects Consumed entries with + /// "already Consumed — nothing to resume". Without that exclusion, + /// the "Delete identity locally" action — which removes + /// `PersistentIdentity` but leaves the `PersistentAssetLock` alive + /// — frees the `(walletId, identityIndex)` slot in the anti-join + /// and the Consumed row would re-surface as a perpetual-spinner + /// row that can't be advanced. + /// + /// `5` (RecoveredFromChain) IS included. It sits above the terminal + /// `4` numerically but is not terminal: it is what the restore scan + /// and the chainlock-promotion path write for a lock with proven + /// Core finality and unknown Platform-side consumption. Treating + /// this as a contiguous `1...3` range silently hid every such row, + /// which is the whole visible symptom — a chain-locked top-up the + /// user funded appears nowhere and reads as lost funds. + var isVisibleAsResumable: Bool { + (statusRaw >= 1 && statusRaw <= 3) || statusRaw == 5 + } } /// Human-readable label for `PersistentAssetLock.statusRaw`. Kept /// here (rather than as a `case` block re-implemented in every -/// view) so the 0/1/2/3 → label mapping has one home. Mirrors the +/// view) so the 0...5 → label mapping has one home. Mirrors the /// Rust-side `AssetLockStatus` enum. extension PersistentAssetLock { var statusLabel: String { @@ -60,6 +80,9 @@ extension PersistentAssetLock { case 2: return "InstantSendLocked" case 3: return "ChainLocked" case 4: return "Consumed" + // Core finality proven, Platform-side consumption unknown. + // Rendered as "Unknown(5)" before this case existed. + case 5: return "RecoveredFromChain" default: return "Unknown(\(statusRaw))" } } diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CreateIdentityResumableTests.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CreateIdentityResumableTests.swift index fffc94c2c49..5514b577eb6 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CreateIdentityResumableTests.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/CreateIdentityResumableTests.swift @@ -100,16 +100,38 @@ final class CreateIdentityResumableTests: XCTestCase { XCTAssertEqual(result, locks) } + /// Regression: `statusRaw == 5` (RecoveredFromChain) must be + /// ACCEPTED. It is what the restore scan and the chainlock-promotion + /// path write for a lock whose Core finality is proven (a real + /// `ChainAssetLockProof` is attached) but whose Platform-side + /// consumption is unknown. A user-driven Resume is exactly the + /// surface allowed to try consuming one — Platform is the arbiter + /// and rejects an already-spent outpoint with a typed error. + /// + /// The filter used to be the contiguous range `1...3`, which hid + /// every recovered lock: a chain-locked top-up the user had really + /// funded appeared on no surface at all and read as lost funds. + func testRecoveredFromChainLocksAreResumable() { + let lock = FakeAssetLockRow(walletId: walletA, statusRaw: 5, identityIndexRaw: 0) + let result = IdentitiesContentView.crossWalletResumableLocks( + in: [lock], + usedSlots: [] + ) + XCTAssertEqual(result, [lock]) + } + /// `statusRaw == 4` (Consumed) is the terminal state for a lock /// that already funded an identity. It must NOT surface on the /// Resumable Registrations list: in the happy path the anti-join /// against `PersistentIdentity` hides it, but the local-only /// delete-identity action removes the identity row WITHOUT the /// asset-lock row, which frees the slot in the anti-join. Without - /// the upper bound (`<= 3`) on the filter, a Consumed lock would - /// re-surface as a Resume row that can't advance - /// (`resume_asset_lock` rejects Consumed entries with - /// "already Consumed — nothing to resume"). + /// excluding `4`, a Consumed lock would re-surface as a Resume row + /// that can't advance (`resume_asset_lock` rejects Consumed entries + /// with "already Consumed — nothing to resume"). + /// + /// Note this is an exclusion of `4` specifically, NOT an upper + /// bound: `5` sits above it numerically and is resumable. func testConsumedLocksAreHiddenFromResumableList() { let lock = FakeAssetLockRow(walletId: walletA, statusRaw: 4, identityIndexRaw: 0) let result = IdentitiesContentView.crossWalletResumableLocks( From 1f06fc594ee59c31471e37df1f55e95befdd18ca Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:41:06 -0400 Subject: [PATCH 02/12] fix(wallet): bound the asset-lock recovery waits that could pin a host thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unbounded waits on the asset-lock recovery path could never terminate, and both are reached from FFI entry points that drive the future with `runtime().block_on(...)` — so neither merely delays a result, each pins the calling host thread for good. 1. Already-consumed reconciliation (#4357 regression) `reconcile_asset_lock_submit_result` upgrades an Instant proof via `upgrade_to_chain_lock_proof(out_point, chain_lock_timeout)`, and all three production call sites (`identity/network/registration.rs` x2, `platform_addresses/fund_from_asset_lock.rs`) pass `None`. The `None` arm of `wait_for_chain_lock` loops forever waiting on SPV lock events. The trigger is routine rather than exotic: an IS-locked lock consumed seconds after broadcast draws the unauthenticated "already consumed" report while its ChainLock is still ~2.5 minutes out — and never arrives at all when the device is offline or SPV is not connected. Pre-#4357 this path returned a typed error immediately. `None` now selects `RECONCILIATION_CHAIN_LOCK_TIMEOUT` (180s). The ChainLock here is wanted only as evidence to record alongside a report about an operation that has ALREADY terminated, so failing to get it degrades instead of propagating: the lock keeps its current status and the typed `AssetLockAlreadyConsumed` is still returned, preserving the code-24 signal hosts branch on. #4357's proof retention is unchanged whenever the ChainLock is reachable inside the bound. 2. Resume after an ambiguous re-broadcast (#4367 regression) A `MaybeSent` verdict on a `Built` lock advances it to `Broadcast` and waits for a proof. But `MaybeSent` is also the NORMAL verdict for a genuinely rejected transaction — `DapiBroadcaster` classifies every failure that way by construction, and the SPV broadcaster reaches `Rejected` only on `NotConnected` (no BIP61 in modern Dash). So the advance is not evidence the transaction is live, and the following `wait_for_proof(None)` at the `resume_asset_lock(.., None)` call sites turned a ~30s broadcast failure into a wait that never ends, because no proof can arrive for a transaction that was never accepted. The advance is kept (it is what stops each recovery pass repeating the same broadcast), but when the caller asked for an unbounded wait the proof wait is bounded by `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` and its expiry is translated back into the `TransactionBroadcastUnconfirmed` callers used to get promptly. Callers that supplied their own timeout are untouched, `FinalityTimeout` and all — the shielded seed pool treats that error as a pacing signal, so re-typing it for everyone would break a working flow to fix a different one. Also on the `Broadcast` arm: a definite `Rejected` is no longer swallowed. That arm logged every broadcast error and fell through to `wait_for_proof`, which is right for the ambiguous verdict but guarantees a dead wait for a verdict that means the send provably did not happen. It now surfaces the error and drops the row via the new `untrack_unproven_broadcast_asset_lock`, so cleanup is not lost and a later resume does not re-enter the same wait. That untrack is a separate method rather than a widening of `untrack_asset_lock`. The existing method's caller in `build.rs` uses "the row was removed" as its trigger to RELEASE the funding-input reservation, and deliberately spares rows that advanced to `Broadcast` concurrently because that is evidence the transaction reached the network. Teaching it to remove `Broadcast` rows would release reservations for inputs whose transaction may be live — a double-spend opening. The new method releases no reservation, and guards on `proof.is_none()` plus the `Consumed` terminal state from #4347. Tests: 5 new cases. The two hang regressions are pinned with `start_paused` runtimes and were confirmed to hang the test binary indefinitely when the fixes are reverted. --- .../src/wallet/asset_lock/orchestration.rs | 239 ++++++++++- .../src/wallet/asset_lock/sync/recovery.rs | 390 +++++++++++++++++- .../src/wallet/asset_lock/sync/tracking.rs | 57 +++ 3 files changed, 661 insertions(+), 25 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs index 4f3d415aa4b..d981737e575 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs @@ -67,6 +67,51 @@ use crate::wallet::asset_lock::manager::AssetLockManager; #[cfg(feature = "shielded")] pub(crate) const CL_FALLBACK_TIMEOUT: Duration = Duration::from_secs(180); +/// Bounded ChainLock wait for the **already-consumed reconciliation** path +/// ([`AssetLockManager::reconcile_asset_lock_submit_result`]). +/// +/// Reconciliation is not a funding flow, so the "a ChainLock always +/// eventually arrives, therefore wait forever" reasoning behind +/// `upgrade_to_chain_lock_proof(None)` does not transfer to it. The +/// operation Platform was asked to perform has already terminated with an +/// unauthenticated `already consumed` report; the ChainLock is wanted only +/// as *evidence to durably record alongside that report*, and the caller is +/// blocked the whole time it is being fetched. +/// +/// Every production call site reaches this through an FFI entry point that +/// drives the future with `runtime().block_on(...)`, so an unbounded wait +/// here does not merely delay a result — it pins the host thread that made +/// the call. The realistic trigger is routine: an IS-locked lock consumed +/// seconds after broadcast reports `already consumed` while its ChainLock +/// is still ~2.5 minutes out, and never arrives at all when the device is +/// offline or SPV is not connected. +/// +/// On expiry the reconciliation still returns the typed +/// [`PlatformWalletError::AssetLockAlreadyConsumed`] — the code-24 signal +/// hosts branch on — having simply failed to attach the chain proof. That +/// matches the pre-#4357 behavior (typed error, no proof retained) while +/// keeping #4357's proof retention whenever the ChainLock is reachable +/// inside the bound. +pub(crate) const RECONCILIATION_CHAIN_LOCK_TIMEOUT: Duration = Duration::from_secs(180); + +/// Bounded proof wait applied after a resume re-broadcast came back +/// `MaybeSent` (see `sync::recovery::resume_asset_lock`). +/// +/// The unbounded `wait_for_proof(None)` used by the funding flows is +/// justified by the transaction being *known* broadcast — finality is then +/// only a matter of time. A `MaybeSent` verdict does not establish that: +/// `DapiBroadcaster` classifies every failure as `MaybeSent`, and the SPV +/// broadcaster reports `Rejected` only for `NotConnected`, so a genuinely +/// rejected transaction is indistinguishable from an accepted one. Waiting +/// without a bound on that signal converts a ~30s broadcast failure into a +/// permanent hang at the `resume_asset_lock(.., None)` call sites. +/// +/// Sized to comfortably cover a ChainLock (~2.5 min) so a transaction that +/// really was accepted still resolves inside the bound; on expiry the +/// caller gets `TransactionBroadcastUnconfirmed`, which is what the +/// pre-#4367 code returned immediately. +pub(crate) const UNCONFIRMED_BROADCAST_PROOF_TIMEOUT: Duration = Duration::from_secs(180); + /// Delay between retries when Platform rejected with CL-height-too-low. /// Each retry bumps `PutSettings::user_fee_increase` so the ST hash /// changes (Tenderdash caches rejected ST hashes for ~24h on @@ -397,6 +442,21 @@ impl AssetLockManager { /// lock to an SPV-backed ChainLock proof, durably retain it as /// consumption-unknown, and preserve the typed host signal. Successful and /// unrelated results pass through unchanged. + /// + /// `chain_lock_timeout` bounds the IS→CL promotion. `None` does **not** + /// mean "wait forever" here: it selects + /// [`RECONCILIATION_CHAIN_LOCK_TIMEOUT`]. Reconciliation always + /// terminates, because the promotion is a best-effort attempt to attach + /// evidence to an operation that has *already* finished, and every + /// production caller reaches it under an FFI `block_on` that would + /// otherwise pin the host thread for as long as the ChainLock is + /// missing (offline / unconnected SPV: forever). + /// + /// Failing to obtain the proof therefore degrades rather than + /// propagates: the lock keeps its current status and the typed + /// [`PlatformWalletError::AssetLockAlreadyConsumed`] is still returned, + /// so the host's code-24 branch is reached either way and the caller + /// may retry to pick the proof up later. pub(crate) async fn reconcile_asset_lock_submit_result( &self, result: Result, @@ -413,18 +473,38 @@ impl AssetLockManager { } let chain_proof = match effective_proof { - AssetLockProof::Chain(_) => effective_proof.clone(), + AssetLockProof::Chain(_) => Some(effective_proof.clone()), AssetLockProof::Instant(_) => { - self.upgrade_to_chain_lock_proof(out_point, chain_lock_timeout) - .await? + let bounded = chain_lock_timeout.or(Some(RECONCILIATION_CHAIN_LOCK_TIMEOUT)); + match self.upgrade_to_chain_lock_proof(out_point, bounded).await { + Ok(proof) => Some(proof), + // Bounded, so this arm is reachable in normal operation + // (an IS-locked lock consumed seconds after broadcast has + // no ChainLock yet). Record nothing, keep the code-24 + // signal, let the caller retry. + Err(e) => { + tracing::warn!( + outpoint = %out_point, + error = %e, + timeout = ?bounded, + "could not obtain a ChainLock proof for an unauthenticated \ + already-consumed report within the bound; reporting the lock \ + as already consumed without retaining consumption-unknown state" + ); + None + } + } } }; - self.mark_asset_lock_consumption_unknown(out_point, chain_proof) - .await?; - tracing::warn!( - outpoint = %out_point, - "recorded unauthenticated already-consumed report as consumption unknown" - ); + + if let Some(chain_proof) = chain_proof { + self.mark_asset_lock_consumption_unknown(out_point, chain_proof) + .await?; + tracing::warn!( + outpoint = %out_point, + "recorded unauthenticated already-consumed report as consumption unknown" + ); + } Err(PlatformWalletError::AssetLockAlreadyConsumed(*out_point)) } @@ -865,4 +945,145 @@ mod tests { ); } } + + /// Regression: the already-consumed reconciliation must TERMINATE when + /// the ChainLock it wants never arrives. + /// + /// Shape: the funding transaction is present and tracked but its record + /// is not in a chain-locked block, the effective proof is an + /// InstantSend proof (so the IS→CL promotion runs), no SPV chainlock is + /// ever delivered, and `chain_lock_timeout` is `None` — exactly what all + /// three production call sites pass (`identity/network/registration.rs` + /// x2, `platform_addresses/fund_from_asset_lock.rs`). + /// + /// Before the fix `None` meant "wait forever" and this future never + /// resolved. Under FFI that is a permanently pinned host thread, since + /// every one of those call sites is reached through `runtime() + /// .block_on(...)`. The realistic trigger is ordinary: a lock consumed + /// seconds after broadcast is IS-locked but not yet chain-locked (~2.5 + /// min away), and never chain-locked at all when the device is offline. + /// + /// `start_paused` lets the runtime auto-advance the bounded sleep, so + /// the assertion is that the call resolves at all — and resolves as the + /// typed code-24 `AssetLockAlreadyConsumed` the hosts branch on, not as + /// the `FinalityTimeout` of the failed promotion. + #[tokio::test(start_paused = true)] + async fn already_consumed_reconciliation_terminates_without_a_chainlock() { + use crate::test_support::{ + funded_wallet_manager, AlwaysRejectedBroadcaster, NoopTestPersister, + }; + use crate::wallet::asset_lock::manager::AssetLockManager; + use crate::wallet::asset_lock::tracked::{AssetLockStatus, TrackedAssetLock}; + use crate::wallet::persister::WalletPersister; + use dashcore::{InstantLock, Network}; + use dpp::identity::state_transition::asset_lock_proof::InstantAssetLockProof; + use key_wallet::account::account_type::StandardAccountType; + use std::sync::Arc; + use tokio::sync::Notify; + + let (wallet_manager, wallet_id, _generation, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let sdk = Arc::new( + dash_sdk::SdkBuilder::new_mock() + .with_network(Network::Testnet) + .build() + .expect("mock sdk"), + ); + let manager = AssetLockManager::new( + sdk, + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(Notify::new()), + Arc::new(AlwaysRejectedBroadcaster), + WalletPersister::new(wallet_id, Arc::new(NoopTestPersister)), + ); + let (transaction, _path) = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await + .expect("build asset lock"); + let out_point = OutPoint::new(transaction.txid(), 0); + + // An INSTANT proof: this is what selects the `upgrade_to_chain_lock_proof` + // arm. A Chain proof would short-circuit and never exercise the wait. + let instant_proof = AssetLockProof::Instant(InstantAssetLockProof::new( + InstantLock::default(), + transaction.clone(), + 0, + )); + { + let mut wm = wallet_manager.write().await; + wm.get_wallet_info_mut(&wallet_id) + .expect("wallet must remain registered") + .tracked_asset_locks + .insert( + out_point, + TrackedAssetLock { + out_point, + transaction, + account_index: 0, + funding_type: AssetLockFundingType::IdentityRegistration, + identity_index: 0, + amount: 1_000_000, + status: AssetLockStatus::InstantSendLocked, + proof: Some(instant_proof.clone()), + }, + ); + } + + // The unauthenticated code-24 consensus response that puts + // `reconcile_asset_lock_submit_result` on the reconciliation path. + use dpp::consensus::basic::identity::IdentityAssetLockTransactionOutPointAlreadyConsumedError; + let already_consumed = + dash_sdk::Error::Protocol(dpp::ProtocolError::ConsensusError(Box::new( + IdentityAssetLockTransactionOutPointAlreadyConsumedError::new( + out_point.txid, + out_point.vout as usize, + ) + .into(), + ))); + + let error = manager + .reconcile_asset_lock_submit_result::<()>( + Err(already_consumed), + &out_point, + &instant_proof, + None, + ) + .await + .expect_err("an already-consumed report always ends as a typed error"); + + assert!( + matches!( + error, + PlatformWalletError::AssetLockAlreadyConsumed(actual) if actual == out_point + ), + "reconciliation must terminate carrying the code-24 signal even when the \ + ChainLock never arrives, got {error:?}" + ); + + // No ChainLock proof was obtainable, so nothing may claim + // consumption-unknown state: the row keeps what it had, and a later + // retry can still pick the proof up. + let status = wallet_manager + .read() + .await + .get_wallet_info(&wallet_id) + .expect("wallet") + .tracked_asset_locks + .get(&out_point) + .expect("lock stays tracked") + .status + .clone(); + assert_eq!( + status, + AssetLockStatus::InstantSendLocked, + "without a chain proof the lock must NOT be promoted to RecoveredFromChain" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs index fb3852dcef1..76fa6e303d4 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs @@ -17,6 +17,7 @@ use crate::changeset::changeset::AssetLockChangeSet; use crate::error::PlatformWalletError; use super::super::manager::AssetLockManager; +use super::super::orchestration::UNCONFIRMED_BROADCAST_PROOF_TIMEOUT; use super::super::tracked::{AssetLockStatus, TrackedAssetLock}; // --------------------------------------------------------------------------- @@ -264,24 +265,69 @@ impl AssetLockManager { // completed. Advancing to `Broadcast` and waiting matches // what the `Broadcast` arm below already does with the // identical signal. - match self.broadcaster.broadcast(&tx).await { - Ok(_) => {} + // + // `MaybeSent` is however ALSO what the broadcaster reports + // for a genuinely rejected transaction: `DapiBroadcaster` + // classifies every failure that way by construction, and the + // SPV broadcaster only reaches `Rejected` on `NotConnected`. + // So the advance above cannot be read as evidence the tx is + // live, and the proof wait that follows it must not be the + // unbounded one — at the `resume_asset_lock(.., None)` + // production call sites that would turn a prompt broadcast + // failure into a permanent hang. Bound it, and translate the + // expiry back into the `TransactionBroadcastUnconfirmed` the + // caller used to get immediately. + let maybe_sent_reason = match self.broadcaster.broadcast(&tx).await { + Ok(_) => None, Err(BroadcastError::MaybeSent { reason }) => { tracing::warn!( outpoint = %out_point, reason = %reason, "resume_asset_lock: re-broadcast of a Built lock returned an \ - unknown outcome (the network may already hold this tx); \ - advancing to Broadcast and waiting for proof" + unknown outcome (the network may already hold this tx, or may \ + have rejected it — the broadcaster cannot tell); advancing to \ + Broadcast and waiting for proof under a bounded timeout" ); + Some(reason) } Err(rejected) => return Err(rejected.into()), - } + }; let cs = self .advance_asset_lock_status(out_point, AssetLockStatus::Broadcast, None) .await?; self.queue_asset_lock_changeset(cs); - let proof = self.wait_for_proof(out_point, timeout).await?; + let proof = match (&maybe_sent_reason, timeout) { + // Ambiguous re-broadcast AND an unbounded wait: the only + // combination that can hang forever. Substitute the bound + // and translate its expiry back into the broadcast error + // the caller used to get immediately. + // + // Callers that passed their own timeout are left exactly + // as they were, `FinalityTimeout` and all — the shielded + // seed pool treats that error as a pacing signal and + // resumes the lock later, so re-typing it would break a + // working flow to fix an unrelated one. + (Some(reason), None) => { + match self + .wait_for_proof(out_point, Some(UNCONFIRMED_BROADCAST_PROOF_TIMEOUT)) + .await + { + Ok(proof) => proof, + Err(PlatformWalletError::FinalityTimeout(_)) => { + return Err(PlatformWalletError::TransactionBroadcastUnconfirmed( + format!( + "asset lock {} was re-broadcast with an unknown \ + outcome and no InstantSend/ChainLock proof arrived \ + within {:?}: {}", + out_point, UNCONFIRMED_BROADCAST_PROOF_TIMEOUT, reason + ), + )) + } + Err(e) => return Err(e), + } + } + _ => self.wait_for_proof(out_point, timeout).await?, + }; self.validate_or_upgrade_proof(proof, account_index, out_point) .await? } @@ -296,22 +342,42 @@ impl AssetLockManager { // the user-facing funding flows — would hang forever. A // re-broadcast revives an evicted/undelivered tx. // - // Best-effort: unlike the `Built` arm, this tx was already - // broadcast once (that's what `Broadcast` means), so it may - // still be in a mempool or already mined — in which case the - // network reports "already known" / "already in block - // chain". The broadcaster can't distinguish that from a real - // rejection (DAPI classifies every failure as `MaybeSent`), - // so we log and proceed to `wait_for_proof` regardless + // Best-effort for the AMBIGUOUS verdict only: unlike the + // `Built` arm, this tx was already broadcast once (that's + // what `Broadcast` means), so it may still be in a mempool or + // already mined — in which case the network reports "already + // known" / "already in block chain", which the broadcaster + // cannot distinguish from a real rejection and reports as + // `MaybeSent`. We log that and proceed to `wait_for_proof` // rather than failing the resume on a tx that is actually - // fine. If the tx really was mined, `wait_for_proof` - // resolves immediately from the SPV/persisted record. + // fine. If the tx really was mined, `wait_for_proof` resolves + // immediately from the SPV/persisted record. + // + // A DEFINITE `Rejected` is a different signal and must NOT be + // swallowed: the broadcaster only reaches it when the send + // provably did not happen (SPV `NotConnected`). Proceeding to + // an unbounded `wait_for_proof` on that verdict guarantees a + // hang, because nothing was sent and no proof can arrive. + // Surface it, and drop the unproven row so a later resume + // does not re-enter the same dead wait. if let Err(e) = self.broadcaster.broadcast(&tx).await { + if matches!(e, BroadcastError::Rejected { .. }) { + tracing::warn!( + outpoint = %out_point, + error = %e, + "resume_asset_lock: defensive re-broadcast of a \ + Broadcast-status lock was definitively rejected; \ + untracking the unproven row and failing the resume" + ); + let cs = self.untrack_unproven_broadcast_asset_lock(out_point).await; + self.queue_asset_lock_changeset(cs); + return Err(e.into()); + } tracing::debug!( outpoint = %out_point, error = %e, "resume_asset_lock: defensive re-broadcast of a \ - Broadcast-status lock returned an error (likely \ + Broadcast-status lock returned an unknown outcome (likely \ already in a mempool or mined); proceeding to wait \ for proof" ); @@ -969,4 +1035,296 @@ mod tests { "re-derived credit-output path must match the build-time path" ); } + + /// Builds a tracked lock at `status` on a funded wallet and resumes it + /// through `broadcaster` with the given `timeout`, returning the resume + /// error and the lock's tracked state afterwards (`None` = untracked). + async fn resume_lock_at( + broadcaster: Arc, + status: AssetLockStatus, + timeout: Option, + ) -> (PlatformWalletError, Option) { + let (wallet_manager, wallet_id, _balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let sdk = Arc::new( + dash_sdk::SdkBuilder::new_mock() + .with_network(Network::Testnet) + .build() + .expect("mock sdk"), + ); + let manager = AssetLockManager::new( + sdk, + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(Notify::new()), + broadcaster, + WalletPersister::new(wallet_id, Arc::new(RecordingPersistence::default())), + ); + let (transaction, _path) = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::AssetLockAddressTopUp, + 4, + &signer, + ) + .await + .expect("build asset lock"); + let out_point = OutPoint::new(transaction.txid(), 0); + { + let mut wm = wallet_manager.write().await; + wm.get_wallet_info_mut(&wallet_id) + .expect("wallet must remain registered") + .tracked_asset_locks + .insert( + out_point, + TrackedAssetLock { + out_point, + transaction, + account_index: 0, + funding_type: AssetLockFundingType::AssetLockAddressTopUp, + identity_index: 4, + amount: 1_000_000, + status, + proof: None, + }, + ); + } + + let error = manager + .resume_asset_lock(&out_point, timeout) + .await + .expect_err("no proof event is ever delivered in these cases"); + let tracked = wallet_manager + .read() + .await + .get_wallet_info(&wallet_id) + .expect("wallet") + .tracked_asset_locks + .get(&out_point) + .map(|lock| lock.status.clone()); + (error, tracked) + } + + /// Regression: an ambiguous re-broadcast on the UNBOUNDED resume path + /// must not hang. + /// + /// `MaybeSent` is the broadcaster's verdict for a genuinely rejected + /// transaction as much as for an accepted one — `DapiBroadcaster` + /// classifies every failure that way, and the SPV broadcaster reaches + /// `Rejected` only on `NotConnected`. So advancing to `Broadcast` and + /// then waiting with `wait_for_proof(None)` — which is what the three + /// `resume_asset_lock(.., None)` production call sites do — turned a + /// broadcast failure that used to surface in ~30s into a wait that never + /// ends, because no proof can arrive for a tx that was never accepted. + /// + /// `start_paused` auto-advances the substituted bound, so this asserts + /// termination *and* that the caller gets the pre-#4367 typed error back. + #[tokio::test(start_paused = true)] + async fn unbounded_resume_of_an_ambiguous_rebroadcast_terminates() { + let (error, status) = resume_lock_at( + Arc::new(AlwaysMaybeSentBroadcaster), + AssetLockStatus::Built, + None, + ) + .await; + + assert!( + matches!( + error, + PlatformWalletError::TransactionBroadcastUnconfirmed(_) + ), + "an unbounded resume whose re-broadcast was ambiguous must end as a \ + broadcast-unconfirmed failure rather than hang, got {error:?}" + ); + assert_eq!( + status, + Some(AssetLockStatus::Broadcast), + "the advance itself is still correct — the row stays resumable so a \ + later pass can pick up a proof that does eventually arrive" + ); + } + + /// The bounded callers must be untouched by the fix above. The shielded + /// seed pool passes its own timeout and treats `FinalityTimeout` as a + /// pacing signal (pause, resume the lock later), so re-typing the error + /// for every caller would have broken a working flow to fix a different + /// one. + #[tokio::test] + async fn bounded_resume_of_an_ambiguous_rebroadcast_still_reports_finality_timeout() { + let (error, status) = resume_lock_at( + Arc::new(AlwaysMaybeSentBroadcaster), + AssetLockStatus::Built, + Some(Duration::from_millis(10)), + ) + .await; + + assert!( + matches!(error, PlatformWalletError::FinalityTimeout(_)), + "a caller-supplied timeout must keep its FinalityTimeout semantics: {error:?}" + ); + assert_eq!(status, Some(AssetLockStatus::Broadcast)); + } + + /// Regression: a DEFINITE rejection on the `Broadcast` arm must be + /// surfaced, not swallowed. + /// + /// The arm logged every broadcast error and fell through to + /// `wait_for_proof` on the reasoning that the tx may already be in a + /// mempool. That reasoning holds for `MaybeSent`, but `Rejected` is only + /// reached when the send provably did not happen — so falling through + /// guaranteed a wait for a proof that cannot exist, unbounded at the + /// production call sites. The unproven row is dropped too, so a later + /// resume does not re-enter the same dead wait. + #[tokio::test] + async fn definite_rejection_on_a_broadcast_lock_surfaces_and_untracks() { + let (error, tracked) = resume_lock_at( + Arc::new(AlwaysRejectedBroadcaster), + AssetLockStatus::Broadcast, + None, + ) + .await; + + assert!( + matches!(error, PlatformWalletError::TransactionBroadcast(_)), + "a definite rejection must fail the resume instead of falling through \ + to an unbounded proof wait, got {error:?}" + ); + assert_eq!( + tracked, None, + "an unproven Broadcast row whose re-broadcast was definitively rejected \ + must be untracked so cleanup is not lost" + ); + } + + /// The untrack in the case above is narrowly scoped. A row carrying a + /// proof has authenticated on-chain evidence that outranks any broadcast + /// verdict, and the terminal `Consumed` tombstone must survive (#4347), + /// so neither may be removed. + #[tokio::test] + async fn unproven_broadcast_untrack_spares_proven_and_terminal_rows() { + use dashcore::InstantLock; + use dpp::identity::state_transition::asset_lock_proof::InstantAssetLockProof; + + let (wallet_manager, wallet_id, _balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let sdk = Arc::new( + dash_sdk::SdkBuilder::new_mock() + .with_network(Network::Testnet) + .build() + .expect("mock sdk"), + ); + let manager = AssetLockManager::new( + sdk, + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(Notify::new()), + Arc::new(AlwaysRejectedBroadcaster), + WalletPersister::new(wallet_id, Arc::new(RecordingPersistence::default())), + ); + let (transaction, _path) = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::AssetLockAddressTopUp, + 4, + &signer, + ) + .await + .expect("build asset lock"); + let out_point = OutPoint::new(transaction.txid(), 0); + let proof = dpp::prelude::AssetLockProof::Instant(InstantAssetLockProof::new( + InstantLock::default(), + transaction.clone(), + 0, + )); + + async fn insert( + wallet_manager: &Arc>>, + wallet_id: &crate::wallet::platform_wallet::WalletId, + out_point: OutPoint, + transaction: Transaction, + status: AssetLockStatus, + proof: Option, + ) { + let mut wm = wallet_manager.write().await; + wm.get_wallet_info_mut(wallet_id) + .expect("wallet") + .tracked_asset_locks + .insert( + out_point, + TrackedAssetLock { + out_point, + transaction, + account_index: 0, + funding_type: AssetLockFundingType::AssetLockAddressTopUp, + identity_index: 4, + amount: 1_000_000, + status, + proof, + }, + ); + } + + // Rows the untrack must SPARE. + for (status, row_proof, label) in [ + ( + AssetLockStatus::Broadcast, + Some(proof.clone()), + "Broadcast with a proof", + ), + (AssetLockStatus::Consumed, None, "Consumed tombstone"), + ( + AssetLockStatus::RecoveredFromChain, + None, + "RecoveredFromChain", + ), + (AssetLockStatus::Built, None, "Built"), + ] { + insert( + &wallet_manager, + &wallet_id, + out_point, + transaction.clone(), + status, + row_proof, + ) + .await; + let cs = manager + .untrack_unproven_broadcast_asset_lock(&out_point) + .await; + assert!( + cs.removed.is_empty(), + "{label} must not be removed by the unproven-Broadcast untrack" + ); + assert!( + wallet_manager + .read() + .await + .get_wallet_info(&wallet_id) + .expect("wallet") + .tracked_asset_locks + .contains_key(&out_point), + "{label} must stay tracked" + ); + } + + // The one row it does remove. + insert( + &wallet_manager, + &wallet_id, + out_point, + transaction.clone(), + AssetLockStatus::Broadcast, + None, + ) + .await; + let cs = manager + .untrack_unproven_broadcast_asset_lock(&out_point) + .await; + assert!( + cs.removed.contains(&out_point), + "an unproven Broadcast row must be removed" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs index 3fa36441b64..4a51cdfea49 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs @@ -85,6 +85,63 @@ impl AssetLockManager { cs } + /// Remove a tracked asset lock that is sitting at + /// [`Broadcast`](AssetLockStatus::Broadcast) **without a proof** after a + /// re-broadcast came back definitively [`Rejected`]. + /// + /// Companion to [`untrack_asset_lock`](Self::untrack_asset_lock), kept + /// separate rather than folded into it because the two have different + /// safety obligations. `untrack_asset_lock` is the build-time rejection + /// path, and its caller in `asset_lock/build.rs` uses "the row was + /// removed" as the trigger to RELEASE the funding-input reservation. + /// There, a row that advanced to `Broadcast` concurrently is treated as + /// positive evidence the transaction reached the network, so the guard + /// deliberately keeps it and holds the reservation. Widening that method + /// to remove `Broadcast` rows would release reservations for inputs + /// whose transaction may be live — a double-spend opening. + /// + /// This method is only reached from `resume_asset_lock` after the + /// broadcaster returned `Rejected`, which it does only when the send + /// provably did not happen. It releases no reservation. + /// + /// Guards, in addition to the status check: + /// + /// - `proof.is_none()` — a row carrying an IS/CL proof has authenticated + /// on-chain evidence that outranks any broadcast verdict. + /// - Only `Broadcast` is removed. [`Consumed`](AssetLockStatus::Consumed) + /// is a terminal tombstone that must survive (#4347), and + /// `InstantSendLocked` / `ChainLocked` / `RecoveredFromChain` all imply + /// finality that a broadcast rejection cannot contradict. + /// + /// Idempotent: an empty changeset when the outpoint is untracked or + /// fails a guard. + pub(crate) async fn untrack_unproven_broadcast_asset_lock( + &self, + out_point: &OutPoint, + ) -> AssetLockChangeSet { + let mut wm = self.wallet_manager.write().await; + let mut cs = AssetLockChangeSet::default(); + if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { + match info.tracked_asset_locks.get(out_point) { + Some(entry) + if entry.status == AssetLockStatus::Broadcast && entry.proof.is_none() => + { + info.tracked_asset_locks.remove(out_point); + cs.removed.insert(*out_point); + } + Some(entry) => tracing::warn!( + outpoint = %out_point, + status = ?entry.status, + has_proof = entry.proof.is_some(), + "untrack_unproven_broadcast_asset_lock: lock is not an unproven \ + Broadcast row — leaving it tracked" + ), + None => {} + } + } + cs + } + /// Mark a tracked asset lock as /// [`Consumed`](AssetLockStatus::Consumed) after a successful /// identity registration or top-up. From c017d88d1ed2e6e9bda5e46f59ccafd0fc95db05 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:01:01 -0400 Subject: [PATCH 03/12] fix(wallet): a rejected re-broadcast must not untrack, and bound the remaining resume waits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both behaviors this PR's first revision introduced on `resume_asset_lock`'s `Broadcast` arm were defective as shipped. 1. `Rejected` is not evidence about the row The arm dropped an unproven `Broadcast` row when the defensive re-broadcast returned `BroadcastError::Rejected`, on the premise that the verdict proves the transaction never reached the network. It does not. With the production `SpvBroadcaster`, `Rejected` is reachable from exactly two places — a client that was never started (`spv/runtime.rs:222`) and dash-spv's zero-connected-peers check (`:125`) — so it is a statement about the attempt that just failed, never about the ORIGINAL broadcast that moved the row to `Broadcast` in an earlier process. That made the untrack routinely destructive. `catchUpStuckAssetLocks` runs on every wallet load, selects `statusRaw < 2` (which includes `Broadcast` = 1) and has no SPV-connected gate, so an ordinary offline relaunch deleted the tracking row for an asset lock that may well be mined — with no way back, because reconstruction re-inserts only on a FRESH detection event, which an already-recorded mined transaction never produces again. The row is now left exactly as it was and the typed error is surfaced. No state on this path makes non-dispatch of the original send provable (a row can sit at `Built` after a successful broadcast too, when the app died between the send and the status advance), so `untrack_unproven_broadcast_asset_lock` has no justified caller and is removed rather than left loaded. 2. The `Broadcast` arm's proof wait was still unbounded The first revision bounded only the `Built` arm. Its own retained behavior — advance an ambiguous `Built` lock to `Broadcast` and leave the row there — routes exactly that lock into the `Broadcast` arm on the next resume pass, where a bare `wait_for_proof(out_point, timeout)` with `timeout = None` waits on `Notify` forever. The hang was deferred by one pass, not removed, and under the FFI's `runtime().block_on(...)` it pins the host thread for good. Both remaining waits now substitute `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` when the caller asked for an unbounded one: - `Broadcast`: expiry is re-typed to `TransactionBroadcastUnconfirmed` and the row is left at `Broadcast`. The bound costs nothing — a proof that lands after it is returned by the next resume on `wait_for_proof`'s first iteration, straight from the record. - `RecoveredFromChain`'s proof-less fallback: bounded for uniformity. Its "resolves immediately by construction" argument holds only while the chain-locked record is reachable, and the accident that loses a row's persisted proof can take the record with it. `FinalityTimeout` is kept there — nothing is broadcast on that path. Callers that supply their own timeout are unchanged in both arms (`or` is the identity on `Some`; the re-typing is gated on `timeout.is_none()`), so the shielded seed pool keeps reading `FinalityTimeout` as a pacing signal. The `Built` arm's `Ok`-verdict wait stays unbounded: `Ok` is the broadcaster's positive network-acceptance contract for a send that just happened, the same evidence the initial funding path waits on. Tests: 3 new cases, 1 rewritten, 1 removed. Each new case was confirmed against its defect — both bound regressions hang the test binary indefinitely when the bound is reverted, and the untrack case fails with `left: None, right: Some(Broadcast)` when the untrack is restored. Co-Authored-By: Claude Opus 4.8 --- .../src/wallet/asset_lock/sync/recovery.rs | 346 ++++++++++-------- .../src/wallet/asset_lock/sync/tracking.rs | 65 +--- 2 files changed, 204 insertions(+), 207 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs index 76fa6e303d4..194fa850c9e 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs @@ -209,9 +209,22 @@ impl AssetLockManager { /// build phase when the credit output is later consumed on Platform. /// /// `timeout` is `Option` and is only consulted when the lock - /// still needs a proof (`Built` / `Broadcast`): `None` waits - /// **indefinitely** for finality. For `InstantSendLocked` / `ChainLocked` - /// the proof already exists and no wait happens, so the value is moot. + /// still needs a proof (`Built` / `Broadcast`, or the defensive + /// proof-less `RecoveredFromChain` fallback). For `InstantSendLocked` / + /// `ChainLocked` the proof already exists and no wait happens, so the + /// value is moot. + /// + /// `None` requests an unbounded wait, and gets one **only** where this + /// call obtained positive evidence the transaction is on the network: + /// the `Built` arm whose re-broadcast returned `Ok`. Every other + /// proof-waiting path substitutes + /// [`UNCONFIRMED_BROADCAST_PROOF_TIMEOUT`], because the alternative is a + /// `Notify` loop that never terminates under the FFI's + /// `runtime().block_on(...)` — a permanently pinned host thread rather + /// than a late answer. Expiry leaves the tracked row untouched, so the + /// next resume picks up a proof that arrives later straight from the + /// record; on the `Broadcast` arm it is reported as + /// [`PlatformWalletError::TransactionBroadcastUnconfirmed`]. pub async fn resume_asset_lock( &self, out_point: &OutPoint, @@ -338,9 +351,9 @@ impl AssetLockManager { // default `-mempoolexpiry` is two weeks), or the original // broadcast may have reached no peers at all (SPV // connectivity gap). Once no node holds the tx, no IS/CL - // proof can ever arrive, and the wait — now unbounded for - // the user-facing funding flows — would hang forever. A - // re-broadcast revives an evicted/undelivered tx. + // proof can ever arrive and the wait below can only run out + // its bound. A re-broadcast revives an evicted/undelivered + // tx, so it is worth attempting before every wait. // // Best-effort for the AMBIGUOUS verdict only: unlike the // `Built` arm, this tx was already broadcast once (that's @@ -353,24 +366,46 @@ impl AssetLockManager { // fine. If the tx really was mined, `wait_for_proof` resolves // immediately from the SPV/persisted record. // - // A DEFINITE `Rejected` is a different signal and must NOT be - // swallowed: the broadcaster only reaches it when the send - // provably did not happen (SPV `NotConnected`). Proceeding to - // an unbounded `wait_for_proof` on that verdict guarantees a - // hang, because nothing was sent and no proof can arrive. - // Surface it, and drop the unproven row so a later resume - // does not re-enter the same dead wait. + // A DEFINITE `Rejected` ends the resume early — but it says + // NOTHING about the row, and must not be read as one. + // + // `Rejected` is scoped to the attempt that produced it. With + // the production `SpvBroadcaster` it is reachable from + // exactly two places, an unstarted client and dash-spv's + // zero-connected-peers check (`spv/runtime.rs`), so it means + // "*this* send never left the device" — not "the transaction + // is not on the network". The ORIGINAL broadcast that put + // this row at `Broadcast` happened in an earlier process, + // possibly days ago, and its outcome is untouched by a + // re-broadcast that never dispatched. + // + // So there is no untrack here. `catchUpStuckAssetLocks` runs + // on every wallet load, selects `statusRaw < 2` (which + // includes `Broadcast` = 1) and has no SPV-connected gate, so + // dropping the row on this verdict deleted tracking for a + // possibly-mined asset lock on an ordinary offline relaunch — + // and reconstruction only re-inserts on a FRESH detection + // event, which an already-recorded mined transaction never + // produces again. The row stays exactly as it was; a later + // resume, once SPV is connected, re-broadcasts and resolves + // it normally. + // + // Nor is there a state on this path where non-dispatch of the + // original send IS provable: a row can sit at `Built` after a + // successful broadcast too (app killed between the send and + // the status advance), which is precisely why the `Built` arm + // above also only surfaces the error and leaves its row alone. if let Err(e) = self.broadcaster.broadcast(&tx).await { if matches!(e, BroadcastError::Rejected { .. }) { tracing::warn!( outpoint = %out_point, error = %e, "resume_asset_lock: defensive re-broadcast of a \ - Broadcast-status lock was definitively rejected; \ - untracking the unproven row and failing the resume" + Broadcast-status lock was definitively rejected — \ + this attempt never left the device, which proves \ + nothing about the original broadcast; leaving the \ + row tracked at Broadcast and failing the resume" ); - let cs = self.untrack_unproven_broadcast_asset_lock(out_point).await; - self.queue_asset_lock_changeset(cs); return Err(e.into()); } tracing::debug!( @@ -382,7 +417,42 @@ impl AssetLockManager { for proof" ); } - let proof = self.wait_for_proof(out_point, timeout).await?; + // Bounded like the `Built` arm, and for the same reason. This + // arm is only entered on a RESUME, i.e. for a transaction + // whose earlier broadcast window already failed to produce a + // proof — so "finality is only a matter of time", the premise + // that justifies the unbounded `wait_for_proof(None)` on the + // initial funding path, does not hold here. The `None` this + // receives from `resume_asset_lock(.., None)` (and from the + // FFI's `timeout_secs == 0`) drove an unbounded `Notify` loop + // under `runtime().block_on(...)`, which pins the calling host + // thread permanently rather than merely delaying a result. + // + // The bound costs nothing: the row is left at `Broadcast`, so + // a proof that lands after the expiry is picked up by the very + // next resume — `wait_for_proof` returns it on its first + // iteration, straight from the record, without waiting at all. + // + // Callers that supplied their own timeout keep their exact + // semantics, `FinalityTimeout` and all: `or` is the identity + // on `Some`, and the re-typing below is gated on the caller + // having asked for an unbounded wait. The shielded seed pool + // reads `FinalityTimeout` as a pacing signal, so re-typing it + // for everyone would break a working flow to fix another. + let bounded = timeout.or(Some(UNCONFIRMED_BROADCAST_PROOF_TIMEOUT)); + let proof = match self.wait_for_proof(out_point, bounded).await { + Ok(proof) => proof, + Err(PlatformWalletError::FinalityTimeout(_)) if timeout.is_none() => { + let reason = format!( + "asset lock {} is tracked as broadcast but no \ + InstantSend/ChainLock proof arrived within {:?}; the \ + lock remains tracked and resumable", + out_point, UNCONFIRMED_BROADCAST_PROOF_TIMEOUT + ); + return Err(PlatformWalletError::TransactionBroadcastUnconfirmed(reason)); + } + Err(e) => return Err(e), + }; self.validate_or_upgrade_proof(proof, account_index, out_point) .await? } @@ -415,13 +485,31 @@ impl AssetLockManager { // was lost, and its wait resolves from the already // chain-locked record rather than blocking on new // network events. + // + // That last part is a "by construction" argument, and it + // holds only while the chain-locked record is still + // reachable — the same lost-state accident that produced a + // proof-less `RecoveredFromChain` row could equally have + // taken the record with it, and then the wait has nothing + // to resolve from and blocks forever on `Notify`. The bound + // is free where the argument holds (`wait_for_proof` returns + // from the record on its first iteration, before any deadline + // is consulted) and closes the case where it doesn't, so it + // is applied for uniformity with the two arms above. No + // re-typing here: nothing was broadcast on this path, so + // `FinalityTimeout` is the honest verdict. match existing_proof { Some(proof) => { self.validate_or_upgrade_proof(proof, account_index, out_point) .await? } None => { - let proof = self.wait_for_proof(out_point, timeout).await?; + let proof = self + .wait_for_proof( + out_point, + timeout.or(Some(UNCONFIRMED_BROADCAST_PROOF_TIMEOUT)), + ) + .await?; self.validate_or_upgrade_proof(proof, account_index, out_point) .await? } @@ -1166,18 +1254,21 @@ mod tests { assert_eq!(status, Some(AssetLockStatus::Broadcast)); } - /// Regression: a DEFINITE rejection on the `Broadcast` arm must be - /// surfaced, not swallowed. + /// A DEFINITE rejection on the `Broadcast` arm is surfaced rather than + /// swallowed — and the row SURVIVES it. /// - /// The arm logged every broadcast error and fell through to - /// `wait_for_proof` on the reasoning that the tx may already be in a - /// mempool. That reasoning holds for `MaybeSent`, but `Rejected` is only - /// reached when the send provably did not happen — so falling through - /// guaranteed a wait for a proof that cannot exist, unbounded at the - /// production call sites. The unproven row is dropped too, so a later - /// resume does not re-enter the same dead wait. + /// The first revision of this fix untracked the row here, reasoning that + /// `Rejected` proves the transaction never reached the network. It does + /// not: with the production `SpvBroadcaster` that verdict means an + /// unstarted client or zero connected peers, which is a fact about the + /// re-broadcast attempt, not about the ORIGINAL broadcast that put the + /// row at `Broadcast` in an earlier process. `catchUpStuckAssetLocks` + /// resumes every `statusRaw < 2` row on each wallet load with no + /// SPV-connected gate, so that untrack deleted tracking for + /// possibly-mined asset locks on ordinary offline relaunches, with no + /// path back (reconstruction re-inserts only on a fresh detection event). #[tokio::test] - async fn definite_rejection_on_a_broadcast_lock_surfaces_and_untracks() { + async fn definite_rejection_on_a_broadcast_lock_surfaces_without_untracking() { let (error, tracked) = resume_lock_at( Arc::new(AlwaysRejectedBroadcaster), AssetLockStatus::Broadcast, @@ -1188,143 +1279,96 @@ mod tests { assert!( matches!(error, PlatformWalletError::TransactionBroadcast(_)), "a definite rejection must fail the resume instead of falling through \ - to an unbounded proof wait, got {error:?}" + to a proof wait, got {error:?}" ); assert_eq!( - tracked, None, - "an unproven Broadcast row whose re-broadcast was definitively rejected \ - must be untracked so cleanup is not lost" + tracked, + Some(AssetLockStatus::Broadcast), + "a re-broadcast that never left the device says nothing about the \ + original send — the row must survive, unchanged, for a later resume" ); } - /// The untrack in the case above is narrowly scoped. A row carrying a - /// proof has authenticated on-chain evidence that outranks any broadcast - /// verdict, and the terminal `Consumed` tombstone must survive (#4347), - /// so neither may be removed. - #[tokio::test] - async fn unproven_broadcast_untrack_spares_proven_and_terminal_rows() { - use dashcore::InstantLock; - use dpp::identity::state_transition::asset_lock_proof::InstantAssetLockProof; + /// Regression: the `Broadcast` arm's proof wait must terminate on the + /// UNBOUNDED resume path too. + /// + /// The first revision of this fix bounded only the `Built` arm. Its own + /// retained behavior — advance an ambiguous `Built` lock to `Broadcast` + /// and leave the row there — routes exactly that lock into this arm on + /// the next resume pass, where a bare `wait_for_proof(None)` waits on + /// `Notify` forever. The hang was deferred by one pass, not removed, and + /// under the FFI's `runtime().block_on(...)` it pins a host thread. + /// + /// `start_paused` auto-advances the substituted bound, so this asserts + /// termination *and* the typed error the caller gets on expiry. + #[tokio::test(start_paused = true)] + async fn unbounded_resume_of_a_broadcast_row_terminates() { + let (error, status) = resume_lock_at( + Arc::new(AlwaysMaybeSentBroadcaster), + AssetLockStatus::Broadcast, + None, + ) + .await; - let (wallet_manager, wallet_id, _balance, signer) = - funded_wallet_manager(StandardAccountType::BIP44Account).await; - let sdk = Arc::new( - dash_sdk::SdkBuilder::new_mock() - .with_network(Network::Testnet) - .build() - .expect("mock sdk"), + assert!( + matches!( + error, + PlatformWalletError::TransactionBroadcastUnconfirmed(_) + ), + "an unbounded resume of a Broadcast row must end as a \ + broadcast-unconfirmed failure rather than hang, got {error:?}" ); - let manager = AssetLockManager::new( - sdk, - Arc::clone(&wallet_manager), - wallet_id, - Arc::new(Notify::new()), - Arc::new(AlwaysRejectedBroadcaster), - WalletPersister::new(wallet_id, Arc::new(RecordingPersistence::default())), + assert_eq!( + status, + Some(AssetLockStatus::Broadcast), + "the row must stay exactly where it was so a proof arriving after the \ + bound is picked up by the next resume" ); - let (transaction, _path) = manager - .build_asset_lock_transaction( - 1_000_000, - 0, - AssetLockFundingType::AssetLockAddressTopUp, - 4, - &signer, - ) - .await - .expect("build asset lock"); - let out_point = OutPoint::new(transaction.txid(), 0); - let proof = dpp::prelude::AssetLockProof::Instant(InstantAssetLockProof::new( - InstantLock::default(), - transaction.clone(), - 0, - )); + } - async fn insert( - wallet_manager: &Arc>>, - wallet_id: &crate::wallet::platform_wallet::WalletId, - out_point: OutPoint, - transaction: Transaction, - status: AssetLockStatus, - proof: Option, - ) { - let mut wm = wallet_manager.write().await; - wm.get_wallet_info_mut(wallet_id) - .expect("wallet") - .tracked_asset_locks - .insert( - out_point, - TrackedAssetLock { - out_point, - transaction, - account_index: 0, - funding_type: AssetLockFundingType::AssetLockAddressTopUp, - identity_index: 4, - amount: 1_000_000, - status, - proof, - }, - ); - } + /// The bounded callers of the `Broadcast` arm keep their semantics, the + /// same way the `Built` arm's do: `or` is the identity on `Some`, and the + /// re-typing is gated on the caller having asked for an unbounded wait. + #[tokio::test] + async fn bounded_resume_of_a_broadcast_row_still_reports_finality_timeout() { + let (error, status) = resume_lock_at( + Arc::new(AlwaysMaybeSentBroadcaster), + AssetLockStatus::Broadcast, + Some(Duration::from_millis(10)), + ) + .await; - // Rows the untrack must SPARE. - for (status, row_proof, label) in [ - ( - AssetLockStatus::Broadcast, - Some(proof.clone()), - "Broadcast with a proof", - ), - (AssetLockStatus::Consumed, None, "Consumed tombstone"), - ( - AssetLockStatus::RecoveredFromChain, - None, - "RecoveredFromChain", - ), - (AssetLockStatus::Built, None, "Built"), - ] { - insert( - &wallet_manager, - &wallet_id, - out_point, - transaction.clone(), - status, - row_proof, - ) - .await; - let cs = manager - .untrack_unproven_broadcast_asset_lock(&out_point) - .await; - assert!( - cs.removed.is_empty(), - "{label} must not be removed by the unproven-Broadcast untrack" - ); - assert!( - wallet_manager - .read() - .await - .get_wallet_info(&wallet_id) - .expect("wallet") - .tracked_asset_locks - .contains_key(&out_point), - "{label} must stay tracked" - ); - } + assert!( + matches!(error, PlatformWalletError::FinalityTimeout(_)), + "a caller-supplied timeout must keep its FinalityTimeout semantics: {error:?}" + ); + assert_eq!(status, Some(AssetLockStatus::Broadcast)); + } - // The one row it does remove. - insert( - &wallet_manager, - &wallet_id, - out_point, - transaction.clone(), - AssetLockStatus::Broadcast, + /// The `RecoveredFromChain` proof-less fallback is bounded for the same + /// reason, even though its wait resolves immediately whenever the + /// chain-locked record it reads is still present. The accident that + /// leaves a `RecoveredFromChain` row without its persisted proof can take + /// the record too, and then the "resolves immediately by construction" + /// argument yields an unbounded `Notify` loop. No re-typing: nothing is + /// broadcast on this arm, so `FinalityTimeout` is the honest verdict. + #[tokio::test(start_paused = true)] + async fn unbounded_resume_of_a_proofless_recovered_row_terminates() { + let (error, status) = resume_lock_at( + Arc::new(AlwaysRejectedBroadcaster), + AssetLockStatus::RecoveredFromChain, None, ) .await; - let cs = manager - .untrack_unproven_broadcast_asset_lock(&out_point) - .await; + assert!( - cs.removed.contains(&out_point), - "an unproven Broadcast row must be removed" + matches!(error, PlatformWalletError::FinalityTimeout(_)), + "a proof-less RecoveredFromChain resume must terminate, got {error:?}" + ); + assert_eq!( + status, + Some(AssetLockStatus::RecoveredFromChain), + "the row keeps its status — the resume proved nothing new about it" ); } } diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs index 4a51cdfea49..9a08a5930d9 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/tracking.rs @@ -85,62 +85,15 @@ impl AssetLockManager { cs } - /// Remove a tracked asset lock that is sitting at - /// [`Broadcast`](AssetLockStatus::Broadcast) **without a proof** after a - /// re-broadcast came back definitively [`Rejected`]. - /// - /// Companion to [`untrack_asset_lock`](Self::untrack_asset_lock), kept - /// separate rather than folded into it because the two have different - /// safety obligations. `untrack_asset_lock` is the build-time rejection - /// path, and its caller in `asset_lock/build.rs` uses "the row was - /// removed" as the trigger to RELEASE the funding-input reservation. - /// There, a row that advanced to `Broadcast` concurrently is treated as - /// positive evidence the transaction reached the network, so the guard - /// deliberately keeps it and holds the reservation. Widening that method - /// to remove `Broadcast` rows would release reservations for inputs - /// whose transaction may be live — a double-spend opening. - /// - /// This method is only reached from `resume_asset_lock` after the - /// broadcaster returned `Rejected`, which it does only when the send - /// provably did not happen. It releases no reservation. - /// - /// Guards, in addition to the status check: - /// - /// - `proof.is_none()` — a row carrying an IS/CL proof has authenticated - /// on-chain evidence that outranks any broadcast verdict. - /// - Only `Broadcast` is removed. [`Consumed`](AssetLockStatus::Consumed) - /// is a terminal tombstone that must survive (#4347), and - /// `InstantSendLocked` / `ChainLocked` / `RecoveredFromChain` all imply - /// finality that a broadcast rejection cannot contradict. - /// - /// Idempotent: an empty changeset when the outpoint is untracked or - /// fails a guard. - pub(crate) async fn untrack_unproven_broadcast_asset_lock( - &self, - out_point: &OutPoint, - ) -> AssetLockChangeSet { - let mut wm = self.wallet_manager.write().await; - let mut cs = AssetLockChangeSet::default(); - if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { - match info.tracked_asset_locks.get(out_point) { - Some(entry) - if entry.status == AssetLockStatus::Broadcast && entry.proof.is_none() => - { - info.tracked_asset_locks.remove(out_point); - cs.removed.insert(*out_point); - } - Some(entry) => tracing::warn!( - outpoint = %out_point, - status = ?entry.status, - has_proof = entry.proof.is_some(), - "untrack_unproven_broadcast_asset_lock: lock is not an unproven \ - Broadcast row — leaving it tracked" - ), - None => {} - } - } - cs - } + // NOTE: there is deliberately no `untrack_unproven_broadcast_asset_lock` + // companion here. A `Rejected` verdict from a re-broadcast describes only + // that attempt (with the production `SpvBroadcaster`: an unstarted client + // or zero connected peers), never the ORIGINAL broadcast that moved the + // row to `Broadcast` in an earlier process — so it is not evidence that + // the transaction is absent from the network, and removing the row on it + // deleted tracking for possibly-mined asset locks during ordinary offline + // relaunches. `resume_asset_lock` now surfaces the typed error and leaves + // the row untouched. /// Mark a tracked asset lock as /// [`Consumed`](AssetLockStatus::Consumed) after a successful From a21c55c008eeb90cc4bf84783edc7601768900e1 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 20 Aug 2026 13:07:23 -0400 Subject: [PATCH 04/12] fix(wallet): RecoveredFromChain is proven-final and fundable in the Kotlin display predicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AssetLockDisplay.kt` still described the status domain as `0/1/2/3/4` and treated it as an ordered scale, so status `5` (RecoveredFromChain) fell through every branch: * `statusLabel` rendered it as "Unknown(5)". * `canFundIdentity` was false, which routes the resume screen's copy into the "still awaiting InstantSend / ChainLock finality" branch — telling the user to wait for a finality that is already PROVEN. The restore scan and the chainlock-promotion path attach a real `ChainAssetLockProof` before writing `5`; what is unknown is Platform-side consumption, and Platform is the arbiter of that, rejecting an already-spent outpoint with a typed error. * `isVisibleAsResumable` was `1..3`, which disagreed with the DAO query's `[1,3] ∪ {5}` predicate — so a row the database was willing to return could still be dropped by the Kotlin surface reading it. Aligns all three with `PersistentAssetLockDisplay.swift`, which already made these three calls the same way. The Consumed (`4`) exclusion is now written by name rather than as an upper bound of `3`, since `5` sits above it numerically while being very much alive — the file header says so explicitly so the next reader doesn't "simplify" it back into a range. Co-Authored-By: Claude Opus 4.8 --- .../example/ui/funding/AssetLockDisplay.kt | 53 ++++++++++++++----- .../ui/funding/AssetLockDisplayTest.kt | 27 +++++++++- 2 files changed, 65 insertions(+), 15 deletions(-) diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/AssetLockDisplay.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/AssetLockDisplay.kt index 66ab6ed64c8..5c9cd464caf 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/AssetLockDisplay.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/AssetLockDisplay.kt @@ -4,34 +4,58 @@ import org.dashfoundation.dashsdk.persistence.entities.AssetLockEntity /** * Asset-lock display helpers — port of `PersistentAssetLockDisplay.swift`. - * Consolidates the 0/1/2/3/4 `AssetLockStatus` discriminants (a protocol- + * Consolidates the 0..5 `AssetLockStatus` discriminants (a protocol- * mirrored constant from the Rust side) into one place so the funding UIs * don't re-implement the same `when` blocks. Extensions on * [AssetLockEntity] so every list surface reads the status the same way. + * + * The status domain is NOT an ordered severity scale, and no predicate here + * may treat it as one. `4` (Consumed) is the terminal tombstone; `5` + * (RecoveredFromChain) sits above it numerically but is very much alive. + * Every predicate below therefore excludes `4` by name rather than by an + * upper bound. */ /** * `true` when the lock should appear on the resumable "Pending Platform Top - * Ups" orphan surface at all — `statusRaw ∈ [1, 3]` (Broadcast through - * ChainLocked). Lower bar than [canFundIdentity]: a Broadcast (1) lock isn't - * submittable yet but the user should still see it (as "Waiting for - * InstantSend / ChainLock…") so a crash-recovery situation has visible - * continuity through the IS-lock arrival. Upper bound at 3 is load-bearing: - * status 4 (Consumed) is terminal and `resume_asset_lock` rejects it, so a - * Consumed row must never resurface as a perpetual-spinner dead end. + * Ups" orphan surface at all — `statusRaw ∈ [1, 3] ∪ {5}`. Lower bar than + * [canFundIdentity]: a Broadcast (1) lock isn't submittable yet but the user + * should still see it (as "Waiting for InstantSend / ChainLock…") so a + * crash-recovery situation has visible continuity through the IS-lock + * arrival. + * + * Excluding `4` (Consumed) is load-bearing: it is terminal and + * `resume_asset_lock` rejects it, so a Consumed row must never resurface as + * a perpetual-spinner dead end. That exclusion is deliberately NOT written + * as an upper bound of `3` — `5` (RecoveredFromChain) is what the restore + * scan and the chainlock-promotion path write for a lock with proven Core + * finality and unknown Platform-side consumption, and a contiguous `1..3` + * range silently dropped every one of them: a chain-locked top-up the user + * really funded appeared on no surface at all and read as lost funds. * ← Swift `isVisibleAsResumable`. */ val AssetLockEntity.isVisibleAsResumable: Boolean - get() = statusRaw in 1..3 + get() = statusRaw in 1..3 || statusRaw == 5 /** * `true` when the lock has a usable IS-lock / chain-lock proof AND hasn't - * been consumed — `statusRaw == 2 || statusRaw == 3`. Only these can submit - * the funding ST immediately; Built (0) and Broadcast (1) still await - * finality. ← Swift `canFundIdentity`. + * been consumed — `statusRaw ∈ {2, 3, 5}`. Only these can submit the funding + * ST immediately; Built (0) and Broadcast (1) still await finality. + * + * `5` (RecoveredFromChain) qualifies: the restore scan and the + * chainlock-promotion path attach a real `ChainAssetLockProof` before + * writing that status, so Core-side finality is PROVEN and the lock is + * exactly as fundable as a ChainLocked (3) one. What is unknown for a `5` is + * whether Platform already consumed it — and Platform, not the client, is + * the arbiter of that: it rejects an already-spent outpoint with a typed + * error. A user-driven Resume is the surface allowed to ask. (Do not feed + * `5` into an automatic retry sweep — blind retries of historical locks are + * the failure mode the status exists to prevent.) Reading `5` as + * not-yet-final made the UI tell the user to wait for a finality that had + * already happened. ← Swift `canFundIdentity`. */ val AssetLockEntity.canFundIdentity: Boolean - get() = statusRaw == 2 || statusRaw == 3 + get() = statusRaw == 2 || statusRaw == 3 || statusRaw == 5 /** * Human-readable status label. Mirrors the Rust-side `AssetLockStatus` enum. @@ -44,6 +68,9 @@ val AssetLockEntity.statusLabel: String 2 -> "InstantSendLocked" 3 -> "ChainLocked" 4 -> "Consumed" + // Core finality proven, Platform-side consumption unknown. + // Rendered as "Unknown(5)" before this branch existed. + 5 -> "RecoveredFromChain" else -> "Unknown($statusRaw)" } diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/ui/funding/AssetLockDisplayTest.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/ui/funding/AssetLockDisplayTest.kt index 00f9c3ac01a..5bf52374030 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/ui/funding/AssetLockDisplayTest.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/ui/funding/AssetLockDisplayTest.kt @@ -33,21 +33,42 @@ class AssetLockDisplayTest { ) @Test - fun `isVisibleAsResumable is true only for statusRaw 1 through 3`() { + fun `isVisibleAsResumable covers 1 through 3 plus RecoveredFromChain`() { assertFalse(lock(0).isVisibleAsResumable) // Built assertTrue(lock(1).isVisibleAsResumable) // Broadcast assertTrue(lock(2).isVisibleAsResumable) // InstantSendLocked assertTrue(lock(3).isVisibleAsResumable) // ChainLocked assertFalse(lock(4).isVisibleAsResumable) // Consumed + assertTrue(lock(5).isVisibleAsResumable) // RecoveredFromChain } + /** + * The exclusion of `4` is by NAME, not by an upper bound — `5` sits + * above it numerically and is resumable. A `statusRaw <= 3` bound reads + * as equivalent and silently hides every restored lock. + */ @Test - fun `canFundIdentity is true only for statusRaw 2 or 3`() { + fun `isVisibleAsResumable excludes Consumed without bounding above it`() { + assertFalse(lock(4).isVisibleAsResumable) + assertTrue(lock(5).isVisibleAsResumable) + } + + /** + * RecoveredFromChain (5) is FUNDABLE. The restore scan and the + * chainlock-promotion path attach a real `ChainAssetLockProof` before + * writing it, so Core-side finality is proven — treating it as + * not-yet-final routed the row into the "waiting for InstantSend / + * ChainLock finality" copy and told the user to wait for something that + * had already happened. + */ + @Test + fun `canFundIdentity covers 2, 3 and RecoveredFromChain`() { assertFalse(lock(0).canFundIdentity) assertFalse(lock(1).canFundIdentity) assertTrue(lock(2).canFundIdentity) assertTrue(lock(3).canFundIdentity) assertFalse(lock(4).canFundIdentity) + assertTrue(lock(5).canFundIdentity) } @Test @@ -57,6 +78,8 @@ class AssetLockDisplayTest { assertEquals("InstantSendLocked", lock(2).statusLabel) assertEquals("ChainLocked", lock(3).statusLabel) assertEquals("Consumed", lock(4).statusLabel) + // Regression: rendered as "Unknown(5)" before the branch existed. + assertEquals("RecoveredFromChain", lock(5).statusLabel) assertEquals("Unknown(7)", lock(7).statusLabel) } From 776817483316c19678962efc240c776e76d1de40 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 20 Aug 2026 13:07:45 -0400 Subject: [PATCH 05/12] fix(wallet): surface and route resumable SHIELDED top-ups on both hosts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The funding-type-parameterized resumable query added earlier in this branch had no production caller on either host, so the gap it was meant to close stayed open: a stalled or RecoveredFromChain shielded top-up (`fundingTypeRaw == 5`) was still absent from every recovery surface in both apps. * Kotlin: `IdentitiesHomeScreen` called `observeResumableAddressTopUps`, which hardcodes `fundingTypeRaw = 4`. `ShieldedFundScreen` could only start a fresh shield. * Swift: `PendingPlatformFundFromAssetLocksList` filtered `== 4`, and `WalletDetailView` always presented the platform-ADDRESS resume view — `ShieldedFundFromAssetLockView.resumeFromLock` was fully wired to `shieldedResumeFundFromAssetLock` but never constructed with a lock. Neither type has any other home: the identity surfaces admit only funding types `0..2`, and `3` is an invitation voucher owned by the reclaim flow. So a type-5 row was unreachable from anywhere in either app, and read to the user as lost funds. Both halves are needed. Surfacing the row without routing it only moves the dead end one tap later: types 4 and 5 consume their locks through DIFFERENT transitions (`resumeFundFromAssetLock` vs. the Type 18 `shieldedResumeFundFromAssetLock`), so a shielded lock sent to the address screen would submit the wrong transition against it. Kotlin: * `ResumableTopUps.kt` — `resumableTopUpsAcrossWallets` fans the DAO out over both top-up funding types per wallet, and `resumeRouteFor` maps a row to its matching resume screen, fail-closed on anything else. Both are pure so the wiring is assertable without Room or a Compose runtime, which is exactly what the DAO-level test could not cover. * `ShieldedFundScreen` gains resume mode, mirroring `FundFromAssetLockScreen`: hides Amount, shows the tracked lock, and dispatches to `shieldedResumeFundFromAssetLock`. It shares the shielded coordinator with fresh shields on purpose — both consume the same per-wallet `shield_guard` Rust-side, so a resume racing a fresh shield has to hit the same gate. The outpoint parse happens before the coordinator claims the slot. Swift: * The list's funding-type + status predicate is extracted as a pure `nonisolated static` generic over `AssetLockResumeRow` (same shape as `IdentitiesContentView.crossWalletResumableLocks`) and widened to admit both top-up types. * `WalletDetailView`'s resume sheet branches on funding type, finally constructing `ShieldedFundFromAssetLockView(wallet:resumeFromLock:)`. Co-Authored-By: Claude Opus 4.8 --- .../example/navigation/AppNavHost.kt | 7 +- .../example/navigation/Routes.kt | 17 +- .../ui/funding/AddressFundProgressScreen.kt | 30 +-- .../example/ui/funding/ResumableTopUps.kt | 104 +++++++++ .../ui/identity/IdentitiesHomeScreen.kt | 33 ++- .../example/ui/shielded/ShieldedFundScreen.kt | 160 ++++++++++++-- .../example/ui/funding/ResumableTopUpsTest.kt | 207 ++++++++++++++++++ .../Core/Views/WalletDetailView.swift | 15 +- ...endingPlatformFundFromAssetLocksList.swift | 75 +++++-- .../PendingPlatformTopUpResumeTests.swift | 126 +++++++++++ 10 files changed, 695 insertions(+), 79 deletions(-) create mode 100644 packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/ResumableTopUps.kt create mode 100644 packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/ui/funding/ResumableTopUpsTest.kt create mode 100644 packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PendingPlatformTopUpResumeTests.swift diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/navigation/AppNavHost.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/navigation/AppNavHost.kt index d37ae44dd65..2c7c5005cf6 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/navigation/AppNavHost.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/navigation/AppNavHost.kt @@ -284,7 +284,12 @@ fun AppNavHost( } composable { entry -> - ShieldedFundScreen(entry.toRoute().walletIdHex, navController) + val route = entry.toRoute() + ShieldedFundScreen( + walletIdHex = route.walletIdHex, + navController = navController, + resumeOutPointHex = route.resumeOutPointHex.takeIf { it.isNotEmpty() }, + ) } composable { entry -> diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/navigation/Routes.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/navigation/Routes.kt index 1d0098c1ea9..0d025c16782 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/navigation/Routes.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/navigation/Routes.kt @@ -189,8 +189,21 @@ import kotlinx.serialization.Serializable /** Seed the shielded note pool (← `SeedShieldedPoolView.swift`). */ @Serializable data class SeedShieldedPool(val walletIdHex: String) -/** Shield funds from an asset lock (← `ShieldedFundFromAssetLockView.swift`). */ -@Serializable data class ShieldedFund(val walletIdHex: String) +/** + * Shield funds from an asset lock (← `ShieldedFundFromAssetLockView.swift`). + * + * [resumeOutPointHex] carries the `:` of an already- + * tracked orphan SHIELDED top-up lock (`fundingTypeRaw == 5`) when the + * screen is opened in RESUME mode — the shielded twin of + * [FundFromAssetLock.resumeOutPointHex], and the counterpart of the Swift + * view's `resumeFromLock` parameter. Empty means fresh-shield mode; nav args + * don't support a nullable `String`, so `""` stands in for "no lock to + * resume". + */ +@Serializable data class ShieldedFund( + val walletIdHex: String, + val resumeOutPointHex: String = "", +) /** * Live shielded-funding progress (← `ShieldedFundFromAssetLockProgressView.swift`). diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/AddressFundProgressScreen.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/AddressFundProgressScreen.kt index eac4e92aee2..38517aaf1c8 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/AddressFundProgressScreen.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/AddressFundProgressScreen.kt @@ -35,7 +35,6 @@ import androidx.navigation.NavHostController import org.dashfoundation.dashsdk.persistence.entities.AssetLockEntity import org.dashfoundation.example.di.LocalAppContainer import org.dashfoundation.example.navigation.AddressFundProgress -import org.dashfoundation.example.navigation.FundFromAssetLock import org.dashfoundation.example.services.assetlock.AddressFundFromAssetLockController import org.dashfoundation.example.services.assetlock.AddressFundFromAssetLockController.Phase import org.dashfoundation.example.util.hexToBytes @@ -83,9 +82,16 @@ fun PendingAssetLockRow( * IdentitiesHome list embeds. Port of * `PendingPlatformFundFromAssetLocksList.swift`: merges two row sources — * (1) [controllers], the live in-flight fundings from the coordinator, and - * (2) [resumableLocks], DB-backed orphan `AssetLockEntity` rows - * (`fundingTypeRaw == 4`, `statusRaw ∈ [1, 3]`) recovered after a crash - * between asset-lock broadcast and ST submission. + * (2) [resumableLocks], DB-backed orphan `AssetLockEntity` rows recovered + * after a crash between asset-lock broadcast and ST submission. + * + * The orphan half covers BOTH top-up funding types — `4` + * (AssetLockAddressTopUp) and `5` (AssetLockShieldedAddressTopUp) — because + * neither has any other recovery home: the identity screens admit only + * funding types `0..2`. Each row's Resume is routed by funding type through + * [resumeRouteFor]; a shielded lock has to reach the shielded resume FFI, + * not the platform-address one, so surfacing the row without routing it + * would only move the dead end one tap later. * * Anti-double-consume gate: if ANY controller is [Phase.InFlight] * ([hasActiveFunding]), Resume is suppressed on every orphan row — the @@ -110,17 +116,15 @@ fun PendingAssetLocksList( ) controllers.forEach { PendingAssetLockRow(it, navController) } resumableLocks.forEach { (walletIdHex, lock) -> + // Fail closed: a funding type with no resume flow on this + // surface keeps the non-interactive indicator rather than + // dispatching to a screen that would submit the wrong + // transition for it. + val route = resumeRouteFor(walletIdHex, lock) ResumablePlatformFundFromAssetLockRow( lock = lock, - hasActiveFunding = hasActiveFundingFor(walletIdHex), - onResume = { - navController.navigate( - FundFromAssetLock( - walletIdHex = walletIdHex, - resumeOutPointHex = lock.outPointHex, - ), - ) - }, + hasActiveFunding = hasActiveFundingFor(walletIdHex) || route == null, + onResume = { route?.let { navController.navigate(it) } }, ) } } diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/ResumableTopUps.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/ResumableTopUps.kt new file mode 100644 index 00000000000..ef98641032d --- /dev/null +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/funding/ResumableTopUps.kt @@ -0,0 +1,104 @@ +package org.dashfoundation.example.ui.funding + +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.flowOf +import org.dashfoundation.dashsdk.persistence.entities.AssetLockEntity +import org.dashfoundation.example.navigation.FundFromAssetLock +import org.dashfoundation.example.navigation.ShieldedFund +import org.dashfoundation.example.util.hexToBytes + +/** + * The two `AssetLockFundingType` discriminants that own the "Pending + * Platform Top Ups" recovery surface. + * + * The full domain is `0 IdentityRegistration, 1 IdentityTopUp, + * 2 IdentityTopUpNotBound, 3 IdentityInvitation, 4 AssetLockAddressTopUp, + * 5 AssetLockShieldedAddressTopUp`. Types `0..2` recover on the identity + * screens (`IdentitiesContentView.crossWalletResumableLocks` and its Kotlin + * counterpart, both of which admit only `0..2`); `3` is a bearer voucher + * consumed exclusively by the invitation reclaim flow. That leaves `4` and + * `5` with no recovery home other than this surface — and `5` had none at + * all until it was added here. + */ +internal const val FUNDING_TYPE_ADDRESS_TOP_UP = 4 +internal const val FUNDING_TYPE_SHIELDED_ADDRESS_TOP_UP = 5 + +/** + * Funding types the top-up recovery surface observes, in render order. + * + * This list is the whole point of `AssetLockDao.observeResumableTopUpsByFundingType` + * existing: the older `observeResumableAddressTopUps` hardcodes + * `fundingTypeRaw = 4`, so a stalled or `RecoveredFromChain` SHIELDED + * top-up (`5`) was returned by no query any screen ran. Widening the + * status predicate alone did not fix that — a row invisible on the + * funding-type axis stays invisible however generous the status axis is. + */ +internal val RESUMABLE_TOP_UP_FUNDING_TYPES = listOf( + FUNDING_TYPE_ADDRESS_TOP_UP, + FUNDING_TYPE_SHIELDED_ADDRESS_TOP_UP, +) + +/** + * Cross-wallet, cross-funding-type stream of resumable orphan top-up locks, + * each tagged with the hex wallet id that owns it (needed for the Resume + * navigation, which is wallet-scoped while the Identities tab is not). + * + * [observe] is the DAO seam — production passes + * `AssetLockDao::observeResumableTopUpsByFundingType`, which applies the + * recoverable-status predicate (`[1,3] ∪ {5}`) SQL-side. Taking it as a + * parameter keeps this function a pure combinator: the funding-type fan-out + * that the blocker was about is then assertable without a Room database or + * a Compose runtime. + * + * Emissions are ordered by (wallet, funding type) so the rendered list + * doesn't reshuffle between recompositions. + */ +internal fun resumableTopUpsAcrossWallets( + walletIdHexes: List, + observe: (walletId: ByteArray, fundingTypeRaw: Int) -> Flow>, +): Flow>> { + // `combine` over an empty source list never emits, which would leave the + // section stuck on its initial value instead of resolving to "nothing to + // recover". Short-circuit to an explicit empty emission. + if (walletIdHexes.isEmpty()) return flowOf(emptyList()) + + val slots: List> = walletIdHexes.flatMap { hex -> + RESUMABLE_TOP_UP_FUNDING_TYPES.map { fundingType -> hex to fundingType } + } + val flows = slots.map { (hex, fundingType) -> observe(hex.hexToBytes(), fundingType) } + return combine(flows) { emissions -> + slots.zip(emissions.toList()).flatMap { (slot, locks) -> + val (hex, _) = slot + locks.map { hex to it } + } + } +} + +/** + * The screen a Resume tap on an orphan top-up row must open, or `null` when + * the row's funding type has no resume flow on this surface. + * + * Routing on `fundingTypeRaw` is the second half of the fix: surfacing a + * type-5 row is useless if its Resume lands on + * [org.dashfoundation.example.ui.funding.FundFromAssetLockScreen], whose + * submit calls `resumeFundFromAssetLock` — the platform-ADDRESS resume. A + * shielded lock has to reach `shieldedResumeFundFromAssetLock` instead, + * which is what [ShieldedFund] in resume mode does. + * + * Fail-closed on anything else: identity-family types (`0..3`) recover on + * the identity screens, and an unknown discriminant must not be dispatched + * to a resume flow that would mis-handle it. + */ +internal fun resumeRouteFor(walletIdHex: String, lock: AssetLockEntity): Any? = + when (lock.fundingTypeRaw) { + FUNDING_TYPE_ADDRESS_TOP_UP -> FundFromAssetLock( + walletIdHex = walletIdHex, + resumeOutPointHex = lock.outPointHex, + ) + FUNDING_TYPE_SHIELDED_ADDRESS_TOP_UP -> ShieldedFund( + walletIdHex = walletIdHex, + resumeOutPointHex = lock.outPointHex, + ) + else -> null + } diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/IdentitiesHomeScreen.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/IdentitiesHomeScreen.kt index 08d0e4517e2..06c2065cf06 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/IdentitiesHomeScreen.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/IdentitiesHomeScreen.kt @@ -36,9 +36,6 @@ import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.navigation.NavHostController import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.combine -import kotlinx.coroutines.flow.flowOf -import org.dashfoundation.dashsdk.persistence.entities.AssetLockEntity import org.dashfoundation.example.di.LocalAppContainer import org.dashfoundation.example.di.LocalAppState import org.dashfoundation.example.navigation.CreateIdentity @@ -49,8 +46,8 @@ import org.dashfoundation.example.navigation.StateTransitions import org.dashfoundation.example.ui.components.EntityRow import org.dashfoundation.example.ui.components.SectionHeader import org.dashfoundation.example.ui.funding.PendingAssetLocksList +import org.dashfoundation.example.ui.funding.resumableTopUpsAcrossWallets import org.dashfoundation.example.ui.wallet.toHexString -import org.dashfoundation.example.util.hexToBytes import org.dashfoundation.example.util.toHex /** @@ -81,26 +78,24 @@ fun IdentitiesHomeScreen(navController: NavHostController) { // DB-backed resumable orphan asset locks (ADDR-03), merged into the // "Pending Platform Top Ups" surface below alongside the in-flight // controllers. The Identities tab is network-scoped (not wallet-scoped), - // so we observe resumable address-topup locks across EVERY loaded wallet - // and tag each with its owning wallet id for the Resume navigation. + // so we observe resumable top-up locks across EVERY loaded wallet and tag + // each with its owning wallet id for the Resume navigation. // ← the DB-backed orphan half of `PendingPlatformFundFromAssetLocksList.swift`. + // + // BOTH top-up funding types are observed (see + // `RESUMABLE_TOP_UP_FUNDING_TYPES`). This used to call + // `observeResumableAddressTopUps`, which pins `fundingTypeRaw = 4`, so a + // stalled or RecoveredFromChain SHIELDED top-up (`5`) was returned by no + // query this screen — or any other production surface — ran, and the + // funds it represents were unreachable from the UI entirely. val loadedWallets by (manager?.wallets ?: MutableStateFlow(emptyMap())).collectAsStateWithLifecycle() val walletIdHexes = loadedWallets.keys.sorted() val resumableLocks by remember(walletIdHexes) { - if (walletIdHexes.isEmpty()) { - flowOf(emptyList>()) - } else { - val flows = walletIdHexes.map { hex -> - val walletId = hex.hexToBytes() - container.database.assetLockDao().observeResumableAddressTopUps(walletId) - } - combine(flows) { arrays -> - walletIdHexes.zip(arrays.toList()).flatMap { (hex, locks) -> - locks.map { hex to it } - } - } - } + resumableTopUpsAcrossWallets( + walletIdHexes = walletIdHexes, + observe = container.database.assetLockDao()::observeResumableTopUpsByFundingType, + ) }.collectAsStateWithLifecycle(initialValue = emptyList()) Scaffold( diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt index bb7aeb6b18a..7eff42013de 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt @@ -33,6 +33,7 @@ import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.navigation.NavHostController import org.dashfoundation.dashsdk.funding.ShieldedProver +import org.dashfoundation.dashsdk.persistence.entities.AssetLockEntity import org.dashfoundation.example.di.LocalAppContainer import org.dashfoundation.example.navigation.ShieldedFundProgress import org.dashfoundation.example.services.shielded.ShieldedFundFromAssetLockCoordinator.StartFundingResult @@ -40,6 +41,10 @@ import org.dashfoundation.example.ui.components.ErrorAlertDialog import org.dashfoundation.example.ui.components.FormSection import org.dashfoundation.example.ui.components.LabeledContent import org.dashfoundation.example.ui.components.SubmitButton +import org.dashfoundation.example.ui.funding.canFundIdentity +import org.dashfoundation.example.ui.funding.parseOutPoint +import org.dashfoundation.example.ui.funding.shortOutPointDisplay +import org.dashfoundation.example.ui.funding.statusLabel import org.dashfoundation.example.util.hexToBytes import org.dashfoundation.example.util.toHex @@ -58,20 +63,53 @@ import org.dashfoundation.example.util.toHex * `platform_wallet_manager_shielded_fund_from_asset_lock`; the screen then * navigates to the dismissal-safe progress view. The Orchard note arrives on * the next shielded sync pass, not synchronously. + * + * RESUME mode (← the Swift view's `resumeFromLock` parameter): when + * [resumeOutPointHex] is non-null the screen hides the Amount section (the + * lock and its amount were fixed at original build time) and routes Submit to + * [org.dashfoundation.dashsdk.wallet.PlatformWalletManager.shieldedResumeFundFromAssetLock] + * instead of `shieldedFundFromAssetLock`, seeded with the parsed outpoint. + * The recipient is still chosen here — a shielded orphan lock carries no + * recipient stamp, because the Orchard recipient is an external address + * picked at ST-submit time, not allocated from the wallet. + * + * This is the resume path for `fundingTypeRaw == 5` + * (AssetLockShieldedAddressTopUp) rows on the "Pending Platform Top Ups" + * surface. Before it existed those rows had nowhere to go: the surface only + * queried funding type 4, and its Resume opened the platform-ADDRESS screen, + * whose submit calls the wrong FFI for a shielded lock. */ @OptIn(ExperimentalMaterial3Api::class) @Composable -fun ShieldedFundScreen(walletIdHex: String, navController: NavHostController) { +fun ShieldedFundScreen( + walletIdHex: String, + navController: NavHostController, + resumeOutPointHex: String? = null, +) { ShieldedGate(navController) { val container = LocalAppContainer.current val walletId = remember(walletIdHex) { walletIdHex.hexToBytes() } val manager by container.walletManagerStore.activeManager.collectAsStateWithLifecycle() + val isResume = resumeOutPointHex != null var amountText by rememberSaveable { mutableStateOf("") } var recipientHex by rememberSaveable { mutableStateOf("") } var error by remember { mutableStateOf(null) } var isSubmitting by remember { mutableStateOf(false) } + // In resume mode, load the tracked lock so we can show its amount + + // status. Keyed by `outPointHex` (primary key); a null result means + // the lock was swept between opening the list and this screen. + // ← FundFromAssetLockScreen's identical resume-mode load. + val resumeLock by produceState( + initialValue = null, + resumeOutPointHex, + ) { + value = resumeOutPointHex?.let { + container.database.assetLockDao().getByOutPointHex(it) + } + } + // Prover status + fee estimate (bridged, single-note-with-change = 2 actions). val proverReady by produceState(initialValue = false) { runCatching { ShieldedProver.warmUp() } @@ -102,13 +140,18 @@ fun ShieldedFundScreen(walletIdHex: String, navController: NavHostController) { val recipient = overrideRecipient ?: defaultRecipient val amount = amountText.toLongOrNull() - val canSubmit = - manager != null && recipient != null && amount != null && amount > 0 && !isSubmitting + // Resume only needs a recipient (+ the loaded lock): the shield value + // is derived Rust-side from the existing lock. Fresh needs an amount + // too. ← FundFromAssetLockScreen's `canSubmit`. + val canSubmit = manager != null && recipient != null && !isSubmitting && + if (isResume) resumeLock != null else (amount != null && amount > 0) Scaffold( topBar = { TopAppBar( - title = { Text("Shield from Asset Lock") }, + title = { + Text(if (isResume) "Resume Shield" else "Shield from Asset Lock") + }, navigationIcon = { IconButton(onClick = { navController.popBackStack() }) { Icon(Icons.AutoMirrored.Filled.ArrowBack, contentDescription = "Back") @@ -137,6 +180,47 @@ fun ShieldedFundScreen(walletIdHex: String, navController: NavHostController) { ) } + if (isResume) { + // Read-only summary of the lock being resumed — replaces + // the Amount section (the locked amount is fixed by the + // original build). ← Swift `resumeFromAssetLockSection`. + FormSection(title = "Resuming") { + val lock = resumeLock + if (lock == null) { + Text( + "This asset lock is no longer tracked. Return to the " + + "Pending Platform Top Ups list.", + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.error, + modifier = Modifier.testTag("shieldedFund.resume.missing"), + ) + } else { + Text( + "Asset Lock ${lock.shortOutPointDisplay}", + style = MaterialTheme.typography.bodyMedium, + modifier = Modifier.testTag("shieldedFund.resume.outpoint"), + ) + Text( + "${lock.amountDuffs} duffs · ${lock.statusLabel}", + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + Text( + if (lock.canFundIdentity) { + "The asset lock already reached a usable proof state. " + + "Pick a recipient to complete the shield." + } else { + "The asset lock is broadcast and still awaiting " + + "InstantSend / ChainLock finality. Resuming will wait " + + "for finality, then shield into the pool." + }, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } + } + } + FormSection(title = "Recipient") { OutlinedTextField( value = recipientHex, @@ -156,39 +240,71 @@ fun ShieldedFundScreen(walletIdHex: String, navController: NavHostController) { ) } - FormSection(title = "Amount") { - OutlinedTextField( - value = amountText, - onValueChange = { amountText = it.filter(Char::isDigit) }, - label = { Text("Amount (duffs)") }, - singleLine = true, - keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Number), - modifier = Modifier.fillMaxWidth().testTag("shieldedFund.amount"), - ) + if (!isResume) { + FormSection(title = "Amount") { + OutlinedTextField( + value = amountText, + onValueChange = { amountText = it.filter(Char::isDigit) }, + label = { Text("Amount (duffs)") }, + singleLine = true, + keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Number), + modifier = Modifier.fillMaxWidth().testTag("shieldedFund.amount"), + ) + } } SubmitButton( - text = "Shield", + text = if (isResume) "Resume Shield" else "Shield", isLoading = isSubmitting, enabled = canSubmit, modifier = Modifier.fillMaxWidth().testTag("shieldedFund.submit"), ) { val m = manager ?: return@SubmitButton val recipientBytes = recipient ?: return@SubmitButton - val amountDuffs = amount ?: return@SubmitButton + + // Resume mode dispatches to a different FFI than a fresh + // shield, so resolve the whole body up front — including + // the outpoint parse, which must not fail after the + // coordinator has already claimed the slot. + val submitBody: suspend () -> Unit = if (isResume) { + val lock = resumeLock ?: return@SubmitButton + val parsed = parseOutPoint(lock.outPointHex) + if (parsed == null) { + error = "Could not parse asset lock outpoint: ${lock.outPointHex}" + return@SubmitButton + } + val (txid, vout) = parsed + { + m.shieldedResumeFundFromAssetLock( + walletId = walletId, + outPointTxid = txid, + outPointVout = vout, + recipientRaw43 = recipientBytes, + ) + } + } else { + val amountDuffs = amount ?: return@SubmitButton + { + m.shieldedFundFromAssetLock( + walletId = walletId, + recipientRaw43 = recipientBytes, + amountDuffs = amountDuffs, + ) + } + } + isSubmitting = true // Start the funding through the coordinator (dismissal-safe, // per-wallet serialized). The body performs the shield FFI. + // Resume shares the coordinator with fresh shields on + // purpose: both consume the same per-wallet shield_guard + // Rust-side, so a resume racing a fresh shield on one + // wallet has to be blocked by the same gate. val result = container.shieldedFundCoordinator.startFunding( walletId = walletId, recipientRaw43 = recipientBytes, - ) { - m.shieldedFundFromAssetLock( - walletId = walletId, - recipientRaw43 = recipientBytes, - amountDuffs = amountDuffs, - ) - } + body = submitBody, + ) when (result) { is StartFundingResult.Started -> { navController.navigate( diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/ui/funding/ResumableTopUpsTest.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/ui/funding/ResumableTopUpsTest.kt new file mode 100644 index 00000000000..27373ad199f --- /dev/null +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/ui/funding/ResumableTopUpsTest.kt @@ -0,0 +1,207 @@ +package org.dashfoundation.example.ui.funding + +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.runTest +import org.dashfoundation.dashsdk.persistence.entities.AssetLockEntity +import org.dashfoundation.example.navigation.FundFromAssetLock +import org.dashfoundation.example.navigation.ShieldedFund +import org.dashfoundation.example.util.toHex +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * Caller-side tests for the "Pending Platform Top Ups" recovery surface. + * + * `AssetLockResumableDaoTest` (in `:sdk`) already pins the DAO query itself — + * that `observeResumableTopUpsByFundingType(walletId, 5)` returns the + * `[1,3] ∪ {5}` status set. What that cannot show is whether any production + * screen ASKS for funding type 5. It didn't: the only caller ran + * `observeResumableAddressTopUps`, which hardcodes `fundingTypeRaw = 4`, so a + * stalled or RecoveredFromChain shielded top-up was absent from every host + * surface no matter how correct the query was. + * + * These tests pin the two halves of that wiring: + * + * 1. [resumableTopUpsAcrossWallets] fans out over BOTH top-up funding + * types, for every loaded wallet. + * 2. [resumeRouteFor] sends each row to the resume flow that matches its + * funding type — a shielded lock must not land on the platform-address + * screen, whose submit calls the wrong FFI. + */ +class ResumableTopUpsTest { + + private val walletA = ByteArray(32) { 1 } + private val walletB = ByteArray(32) { 2 } + private val walletAHex = walletA.toHex() + private val walletBHex = walletB.toHex() + + private fun lock( + outPointHex: String, + fundingTypeRaw: Int, + statusRaw: Int = 5, + ): AssetLockEntity = AssetLockEntity( + outPointHex = outPointHex, + walletId = ByteArray(32), + transactionBytes = ByteArray(0), + fundingTypeRaw = fundingTypeRaw, + identityIndexRaw = 0, + amountDuffs = 100_000L, + statusRaw = statusRaw, + ) + + /** + * Records every `(walletIdHex, fundingTypeRaw)` the caller asks for, and + * serves whatever rows the test registered for that pair. + */ + private class RecordingObserver( + private val rows: Map, List> = emptyMap(), + ) { + val requested = mutableListOf>() + + fun observe(walletId: ByteArray, fundingTypeRaw: Int): Flow> { + val key = walletId.toHex() to fundingTypeRaw + requested += key + return flowOf(rows[key] ?: emptyList()) + } + } + + // ── funding-type fan-out ────────────────────────────────────────── + + /** + * The blocker itself. Funding type 5 must be among the types the screen + * observes — otherwise the parameterized DAO query has no production + * caller and the shielded rows it can return are never requested. + */ + @Test + fun `observes both address and shielded top-up funding types`() = runTest { + val observer = RecordingObserver() + + resumableTopUpsAcrossWallets(listOf(walletAHex), observer::observe).first() + + assertEquals( + listOf(walletAHex to 4, walletAHex to 5), + observer.requested, + ) + } + + /** + * The user-visible consequence: a shielded (`fundingTypeRaw == 5`) + * RecoveredFromChain lock reaches the rendered list, tagged with its + * owning wallet so Resume can navigate wallet-scoped. + */ + @Test + fun `surfaces a shielded RecoveredFromChain top-up`() = runTest { + val shielded = lock("shielded:0", fundingTypeRaw = 5, statusRaw = 5) + val observer = RecordingObserver(mapOf((walletAHex to 5) to listOf(shielded))) + + val rows = resumableTopUpsAcrossWallets(listOf(walletAHex), observer::observe).first() + + assertEquals(listOf(walletAHex to shielded), rows) + } + + /** Address rows are unaffected by the widening. */ + @Test + fun `still surfaces address top-ups alongside shielded ones`() = runTest { + val address = lock("address:0", fundingTypeRaw = 4, statusRaw = 1) + val shielded = lock("shielded:0", fundingTypeRaw = 5, statusRaw = 5) + val observer = RecordingObserver( + mapOf( + (walletAHex to 4) to listOf(address), + (walletAHex to 5) to listOf(shielded), + ), + ) + + val rows = resumableTopUpsAcrossWallets(listOf(walletAHex), observer::observe).first() + + assertEquals(listOf(walletAHex to address, walletAHex to shielded), rows) + } + + /** + * Each wallet is observed on both funding types, and every row keeps the + * hex of the wallet that owns it — the Identities tab is cross-wallet, + * so mis-tagging would send Resume to the wrong wallet's screen. + */ + @Test + fun `tags rows with their owning wallet across wallets`() = runTest { + val aLock = lock("a:0", fundingTypeRaw = 4, statusRaw = 2) + val bLock = lock("b:0", fundingTypeRaw = 5, statusRaw = 5) + val observer = RecordingObserver( + mapOf( + (walletAHex to 4) to listOf(aLock), + (walletBHex to 5) to listOf(bLock), + ), + ) + + val rows = resumableTopUpsAcrossWallets( + listOf(walletAHex, walletBHex), + observer::observe, + ).first() + + assertEquals(listOf(walletAHex to aLock, walletBHex to bLock), rows) + assertEquals( + listOf(walletAHex to 4, walletAHex to 5, walletBHex to 4, walletBHex to 5), + observer.requested, + ) + } + + /** + * With no wallets loaded the flow must still emit — `combine` over an + * empty source list never emits, which would leave the section pinned to + * its initial value instead of resolving to "nothing to recover". + */ + @Test + fun `emits an empty list when no wallets are loaded`() = runTest { + val observer = RecordingObserver() + + val rows = resumableTopUpsAcrossWallets(emptyList(), observer::observe).first() + + assertEquals(emptyList>(), rows) + assertEquals(emptyList>(), observer.requested) + } + + // ── resume routing ──────────────────────────────────────────────── + + /** + * The second half of the blocker: surfacing a shielded row is useless if + * its Resume opens the platform-address screen, whose submit calls + * `resumeFundFromAssetLock` rather than + * `shieldedResumeFundFromAssetLock`. + */ + @Test + fun `routes a shielded lock to the shielded resume screen`() { + val shielded = lock("shielded:0", fundingTypeRaw = 5) + + assertEquals( + ShieldedFund(walletIdHex = walletAHex, resumeOutPointHex = "shielded:0"), + resumeRouteFor(walletAHex, shielded), + ) + } + + @Test + fun `routes an address lock to the platform-address resume screen`() { + val address = lock("address:0", fundingTypeRaw = 4) + + assertEquals( + FundFromAssetLock(walletIdHex = walletAHex, resumeOutPointHex = "address:0"), + resumeRouteFor(walletAHex, address), + ) + } + + /** + * Fail closed. Identity-family types (0..3) recover on the identity + * screens; an unknown discriminant must not be dispatched into a resume + * flow that would submit the wrong transition for it. + */ + @Test + fun `refuses to route funding types that have no resume flow here`() { + for (fundingType in listOf(0, 1, 2, 3, 6, 99)) { + assertNull( + "fundingTypeRaw $fundingType must not route to a top-up resume screen", + resumeRouteFor(walletAHex, lock("x:0", fundingTypeRaw = fundingType)), + ) + } + } +} diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift index cd9affaee51..e5145830765 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift @@ -278,7 +278,20 @@ struct WalletDetailView: View { WithdrawPlatformAddressView(wallet: wallet) } .sheet(item: $resumingAssetLock) { lock in - FundFromAssetLockPlatformAddressView(wallet: wallet, resumeFromLock: lock) + // Route by funding type. Both top-up types reach this sheet from + // `PendingPlatformFundFromAssetLocksList`, and they consume their + // locks through DIFFERENT transitions: type 4 resumes via + // `resumeFundFromAssetLock` (credit a Platform address), type 5 + // via `shieldedResumeFundFromAssetLock` (Type 18 shield into the + // Orchard pool). Sending a shielded lock to the address view + // would submit the wrong transition against it, so surfacing the + // row without this branch would only move the dead end one tap + // later. + if lock.fundingTypeRaw == 5 { + ShieldedFundFromAssetLockView(wallet: wallet, resumeFromLock: lock) + } else { + FundFromAssetLockPlatformAddressView(wallet: wallet, resumeFromLock: lock) + } } .sheet(isPresented: $showShieldFromAssetLock) { ShieldedFundFromAssetLockView(wallet: wallet) diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundFromAssetLocksList.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundFromAssetLocksList.swift index 1bc59ca9478..72d08fdba21 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundFromAssetLocksList.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundFromAssetLocksList.swift @@ -7,10 +7,17 @@ // // 1. In-flight controllers from `AddressFundFromAssetLockCoordinator` — the // live submit-still-running case. -// 2. Orphaned `PersistentAssetLock` rows with -// `fundingTypeRaw == AssetLockAddressTopUp` (4) and -// `statusRaw ∈ [1, 3]` — the crash-recovery case where the user -// killed the app between asset-lock broadcast and ST submission. +// 2. Orphaned `PersistentAssetLock` rows on either top-up funding type — +// `AssetLockAddressTopUp` (4) or `AssetLockShieldedAddressTopUp` (5) — +// at a recoverable status (`isVisibleAsResumable`, i.e. +// `[1, 3] ∪ {5}`): the crash-recovery case where the user killed the +// app between asset-lock broadcast and ST submission. +// +// Both top-up funding types belong here because neither has any other +// recovery home: the identity-side surfaces admit only funding types +// `0...2`, and `3` is an invitation voucher owned by the reclaim flow. +// Filtering to `4` alone left every shielded top-up — stalled or +// RecoveredFromChain — invisible on every surface in the app. // // Anti-join: an orphaned lock is hidden if its outpoint is already // claimed by an in-flight controller. (We index by outpoint here @@ -63,11 +70,11 @@ struct PendingPlatformFundFromAssetLocksList: View { let orphans = resumableLocks(excludingControllerOutpoints: Set(walletControllers.compactMap { _ in // Controllers don't currently store the outpoint of the // asset lock they're driving. The de-dupe set therefore - // never has entries today — but the SwiftData status - // filter (`>=1, <=3`) already excludes locks that have - // been Consumed, so an in-flight controller whose lock - // is mid-transition lands at status 2/3 and would only - // briefly co-render. The plumbing is here so a future + // never has entries today — but the status filter + // (`isVisibleAsResumable`) already excludes locks that + // have been Consumed, so an in-flight controller whose + // lock is mid-transition lands at status 2/3 and would + // only briefly co-render. The plumbing is here so a future // tweak (controller exposes its outpoint after broadcast) // can de-dupe by returning a non-nil here. nil @@ -121,18 +128,44 @@ struct PendingPlatformFundFromAssetLocksList: View { coordinator.activeControllers().filter { $0.walletId == walletId } } - /// Resumable asset-lock rows for this wallet — fundingType 4 - /// (AssetLockAddressTopUp) and status in 1..3 (Broadcast through - /// ChainLocked, excluding Consumed). Excludes outpoints already - /// owned by an in-flight controller. + /// Resumable asset-lock rows for this wallet — either top-up funding + /// type at a recoverable status. Excludes outpoints already owned by + /// an in-flight controller. private func resumableLocks( excludingControllerOutpoints excluded: Set ) -> [PersistentAssetLock] { assetLocks - .filter { $0.fundingTypeRaw == 4 } - .filter { $0.isVisibleAsResumable } + .filter { Self.isResumableTopUp($0) } .filter { !excluded.contains($0.outPointHex) } } + + /// Funding-type + status predicate for this surface, extracted as a + /// pure `static` (generic over `AssetLockResumeRow`) so it is + /// unit-testable without a SwiftData container — same shape as + /// `IdentitiesContentView.crossWalletResumableLocks`. + /// + /// Admits BOTH top-up funding types: + /// + /// * `4` AssetLockAddressTopUp — resumes into + /// `FundFromAssetLockPlatformAddressView`. + /// * `5` AssetLockShieldedAddressTopUp — resumes into + /// `ShieldedFundFromAssetLockView`. + /// + /// `5` was previously filtered out here. Because the identity + /// surfaces admit only `0...2` and the reclaim flow owns `3`, that + /// left a stalled or RecoveredFromChain shielded top-up on NO surface + /// in the app — real recoverable value with no way to reach it. The + /// status half is delegated to `isVisibleAsResumable`, which excludes + /// the terminal Consumed (`4`) by name while keeping + /// RecoveredFromChain (`5`). + /// + /// Identity-family types are rejected fail-closed: they have their own + /// resume surface, and dispatching one here would offer a resume flow + /// that submits the wrong transition for it. + nonisolated static func isResumableTopUp(_ lock: R) -> Bool { + guard lock.fundingTypeRaw == 4 || lock.fundingTypeRaw == 5 else { return false } + return lock.isVisibleAsResumable + } } private extension AddressFundFromAssetLockController { @@ -293,13 +326,13 @@ struct ResumablePlatformFundFromAssetLockRow: View { waitingIndicator } else { // Nothing is driving a fund on this wallet, so every visible row - // (statusRaw 1/2/3, per the list's `isVisibleAsResumable` + // (statusRaw 1/2/3/5, per the list's `isVisibleAsResumable` // filter) is a genuinely-resumable orphan with no driver: a - // proof-ready (`canFundIdentity`, statusRaw 2/3) lock submits as - // soon as a recipient is picked; a Broadcast (statusRaw 1) lock - // re-enters the (now unbounded) finality wait via - // `resume_asset_lock`. Without this the Broadcast case was a - // permanent "Waiting…" dead end. + // proof-ready (`canFundIdentity`, statusRaw 2/3/5) lock submits + // as soon as a recipient is picked; a Broadcast (statusRaw 1) + // lock re-enters the finality wait via `resume_asset_lock`. + // Without this the Broadcast case was a permanent "Waiting…" + // dead end. resumeButton } } diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PendingPlatformTopUpResumeTests.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PendingPlatformTopUpResumeTests.swift new file mode 100644 index 00000000000..159a4ad4fb2 --- /dev/null +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/PendingPlatformTopUpResumeTests.swift @@ -0,0 +1,126 @@ +import XCTest +import SwiftDashSDK +@testable import SwiftExampleApp + +/// Tests the pure funding-type + status predicate behind the wallet +/// detail screen's "Pending Platform Top Ups" surface. +/// +/// This is the only surface in the app that can recover an orphaned +/// TOP-UP asset lock. The identity-side +/// `IdentitiesContentView.crossWalletResumableLocks` admits only +/// funding types `0...2`, and `3` (IdentityInvitation) is a bearer +/// voucher owned by the reclaim flow — so a lock at funding type `4` +/// or `5` that this filter rejects is unreachable from anywhere in the +/// app, and reads to the user as lost funds. +/// +/// Two invariants can silently regress: +/// +/// 1. Funding type `5` (AssetLockShieldedAddressTopUp) must be +/// admitted. The filter was `fundingTypeRaw == 4` alone, which +/// hid every shielded top-up — stalled or RecoveredFromChain — +/// on every surface. Widening the STATUS predicate did not help: +/// a row excluded on the funding-type axis stays excluded +/// however generous the status axis is. +/// 2. The status half must keep excluding the terminal Consumed +/// (`4`) while admitting RecoveredFromChain (`5`) — delegated to +/// `isVisibleAsResumable`, and asserted here because this +/// surface is the one that composes the two axes. +final class PendingPlatformTopUpResumeTests: XCTestCase { + + private struct FakeAssetLockRow: AssetLockResumeRow, Equatable { + let walletId: Data + let statusRaw: Int + var identityIndexRaw: Int32 = 0 + let fundingTypeRaw: Int + } + + private let wallet = Data(repeating: 0xA1, count: 8) + + private func row(status: Int, fundingType: Int) -> FakeAssetLockRow { + FakeAssetLockRow(walletId: wallet, statusRaw: status, fundingTypeRaw: fundingType) + } + + // MARK: - funding-type axis + + /// The regression: a shielded top-up must be admitted, at every + /// recoverable status. + func testAdmitsShieldedAddressTopUps() { + for status in [1, 2, 3, 5] { + XCTAssertTrue( + PendingPlatformFundFromAssetLocksList.isResumableTopUp( + row(status: status, fundingType: 5) + ), + "shielded top-up at status \(status) must be resumable" + ) + } + } + + /// Address top-ups are unaffected by the widening. + func testAdmitsAddressTopUps() { + for status in [1, 2, 3, 5] { + XCTAssertTrue( + PendingPlatformFundFromAssetLocksList.isResumableTopUp( + row(status: status, fundingType: 4) + ), + "address top-up at status \(status) must be resumable" + ) + } + } + + /// Identity-family funding types recover on the identity screens. + /// Admitting one here would offer a resume flow that submits the + /// wrong transition against it. + func testRejectsIdentityFamilyAndUnknownFundingTypes() { + for fundingType in [0, 1, 2, 3, 6, 99] { + XCTAssertFalse( + PendingPlatformFundFromAssetLocksList.isResumableTopUp( + row(status: 2, fundingType: fundingType) + ), + "fundingTypeRaw \(fundingType) must not reach the top-up surface" + ) + } + } + + // MARK: - status axis + + /// Built (0) has not been broadcast; Consumed (4) is the terminal + /// tombstone `resume_asset_lock` rejects outright. Both stay out on + /// either funding type. + func testRejectsBuiltAndConsumedOnBothTopUpTypes() { + for fundingType in [4, 5] { + for status in [0, 4] { + XCTAssertFalse( + PendingPlatformFundFromAssetLocksList.isResumableTopUp( + row(status: status, fundingType: fundingType) + ), + "status \(status) on fundingType \(fundingType) must stay hidden" + ) + } + } + } + + /// `4` is excluded by NAME, not by an upper bound: `5` sits above + /// it numerically and is resumable. A `statusRaw <= 3` bound reads + /// as equivalent and silently drops every restored lock. + func testConsumedExclusionIsNotAnUpperBound() { + XCTAssertFalse( + PendingPlatformFundFromAssetLocksList.isResumableTopUp( + row(status: 4, fundingType: 5) + ) + ) + XCTAssertTrue( + PendingPlatformFundFromAssetLocksList.isResumableTopUp( + row(status: 5, fundingType: 5) + ) + ) + } + + /// RecoveredFromChain carries a real `ChainAssetLockProof`, so it is + /// as fundable as a ChainLocked (3) lock — the row must offer Resume + /// rather than the "waiting for finality" indicator. + func testRecoveredFromChainIsFundableNotWaiting() { + XCTAssertTrue(row(status: 5, fundingType: 5).canFundIdentity) + XCTAssertTrue(row(status: 5, fundingType: 4).canFundIdentity) + XCTAssertFalse(row(status: 1, fundingType: 5).canFundIdentity) + } +} From 6f4506cb19442a5ebbdfbfb280a09ecd962714fd Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 20 Aug 2026 13:07:58 -0400 Subject: [PATCH 06/12] docs(wallet-ffi): a zero resume timeout selects the policy default, not an unbounded wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both asset-lock sync entry points still documented `timeout_secs == 0` as requesting an unbounded wait, justified by "a ChainLock is guaranteed finality; a broadcast lock is pending, never failed". That contract no longer holds, and the reasoning behind it was the bug this branch fixed: on a RESUME the broadcaster cannot establish that the transaction is live at all — `DapiBroadcaster` classifies every failure as `MaybeSent`, and the SPV broadcaster reaches `Rejected` only on `NotConnected` — so a rejected transaction is indistinguishable from an accepted one. `resume_asset_lock` now substitutes the 180s `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` on every arm that actually waits. Zero therefore means "decline to specify a bound; apply the recovery policy's state-dependent default", which is the opposite of what a caller reading these docs would plan for. `asset_lock_manager_catch_up_blocking` made the stale promise load-bearing: it explicitly told callers the thread parks "indefinitely" at zero, and that entry point is fanned out one call per stuck lock at launch. Documents the real contract at both entry points — which stages consult the timeout at all, what zero selects, that expiry is non-destructive (the row stays tracked, the next resume returns a late proof straight from the record), and that a non-zero timeout keeps its exact semantics. Docs only; no behavior change. Co-Authored-By: Claude Opus 4.8 --- .../src/asset_lock/sync.rs | 65 +++++++++++++++++-- 1 file changed, 59 insertions(+), 6 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs b/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs index 5b840f9d039..2c8d1424adb 100644 --- a/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs +++ b/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs @@ -42,6 +42,36 @@ fn parse_outpoint(txid: *const [u8; 32], vout: u32) -> dashcore::OutPoint { /// the already-tracked lock state; signing the consume transition is /// the next stage's responsibility (e.g. /// [`crate::platform_wallet_register_identity_with_funding_signer`]). +/// +/// # Timeouts +/// +/// `timeout_secs` bounds only the stages that still have to WAIT for a +/// proof (`Built` / `Broadcast`, plus the defensive proof-less +/// `RecoveredFromChain` fallback). `InstantSendLocked` / `ChainLocked` +/// already carry a proof and return without ever consulting it. +/// +/// `timeout_secs == 0` does **not** request an unbounded wait — it +/// declines to specify one, and `resume_asset_lock` then applies the +/// recovery policy's own state-dependent default. Today every +/// proof-waiting arm substitutes the same 180s +/// `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` bound (sized to comfortably +/// cover a ~2.5min ChainLock), because on a resume none of them can +/// establish that the transaction is live on the network: a resume +/// re-broadcast reports `MaybeSent` for an accepted and a rejected +/// transaction alike. Waiting without a bound on that signal is a +/// `Notify` loop with no terminating event, and under the +/// `runtime().block_on(...)` below it pins the calling host thread +/// permanently rather than merely delaying an answer. +/// +/// Expiry is non-destructive: the tracked row keeps its status, so a +/// proof arriving afterwards is returned by the very next resume +/// straight from the record, without waiting at all. On the `Built` / +/// `Broadcast` arms the expiry surfaces as +/// `TransactionBroadcastUnconfirmed`. +/// +/// A non-zero `timeout_secs` keeps its exact semantics, +/// `FinalityTimeout` included — the substitution above is gated on the +/// caller having declined to choose. #[no_mangle] pub unsafe extern "C" fn asset_lock_manager_resume( handle: Handle, @@ -58,8 +88,11 @@ pub unsafe extern "C" fn asset_lock_manager_resume( check_ptr!(out_derivation_path); let out_point = parse_outpoint(txid, vout); - // `timeout_secs == 0` requests an unbounded wait (a ChainLock is - // guaranteed finality; a broadcast lock is pending, never failed). + // `timeout_secs == 0` declines to specify a bound; it does NOT ask + // for an unbounded wait. `resume_asset_lock` reads the resulting + // `None` as "apply the recovery policy's default", which on every + // proof-waiting arm is the 180s `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT`. + // See this function's `# Timeouts` section. let timeout = (timeout_secs != 0).then(|| Duration::from_secs(timeout_secs)); let option = ASSET_LOCK_MANAGER_STORAGE.with_item(handle, |manager| { @@ -94,8 +127,25 @@ pub unsafe extern "C" fn asset_lock_manager_resume( /// Returns `ok` on a successful proof resolution, an error on /// timeout / wait failure. The Swift caller is expected to schedule /// this on a background queue — `runtime().block_on(...)` parks the -/// calling thread for up to `timeout_secs` (or **indefinitely** when -/// `timeout_secs == 0`, since a ChainLock is guaranteed finality). +/// calling thread for the duration of the wait. +/// +/// # Timeouts +/// +/// Identical contract to [`asset_lock_manager_resume`], which this +/// delegates to: `timeout_secs == 0` selects the recovery policy's +/// state-dependent default rather than an unbounded wait, and that +/// default is the 180s `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` on every +/// arm that actually waits for a proof. So the thread this parks is +/// parked for a bounded time in all cases — `timeout_secs` when +/// non-zero, the policy default otherwise. +/// +/// That bound is what makes this entry point safe to fan out at +/// launch. The catch-up sweep starts one call per stuck lock; when +/// zero meant "wait forever", a device that was offline (or an SPV +/// session that never connected) turned each of those into a +/// permanently parked worker thread. Expiry now simply ends the pass, +/// leaving the row tracked and resumable, and the next sweep picks up +/// a proof that landed in between straight from the record. #[no_mangle] pub unsafe extern "C" fn asset_lock_manager_catch_up_blocking( handle: Handle, @@ -106,8 +156,11 @@ pub unsafe extern "C" fn asset_lock_manager_catch_up_blocking( check_ptr!(txid); let out_point = parse_outpoint(txid, vout); - // `timeout_secs == 0` requests an unbounded wait (a ChainLock is - // guaranteed finality; a broadcast lock is pending, never failed). + // `timeout_secs == 0` declines to specify a bound; it does NOT ask + // for an unbounded wait. `resume_asset_lock` reads the resulting + // `None` as "apply the recovery policy's default", which on every + // proof-waiting arm is the 180s `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT`. + // See this function's `# Timeouts` section. let timeout = (timeout_secs != 0).then(|| Duration::from_secs(timeout_secs)); tracing::info!( From 3ab29ab468affb4e7a68376661654b34e3441a14 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 20 Aug 2026 17:59:51 -0400 Subject: [PATCH 07/12] fix(example-apps): key shielded resume single-flighting on the operation, not just the recipient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shielded fund coordinators (Kotlin + Swift) deduplicate by (walletId, recipientRaw43), but resumable locks normally default to the same wallet-owned shielded recipient, so two different locks share one slot key. startFunding returned the FIRST lock's controller for an InFlight/Completed slot without invoking the new closure — tapping a second resumable lock while the first was running (or within its 30s completed-retention window) silently showed the first operation and never called shieldedResumeFundFromAssetLock for the second outpoint. A fresh shield to the same recipient could suppress a resume the same way. Reuse now additionally requires a matching operation identity (the resumed lock's outpoint, or the fresh-shield marker): - same operation: reuse, unchanged single-flight; - different operation, slot InFlight: BlockedByOtherWalletFunding — the wallet-wide shield serialization verdict, same as a different recipient; - different operation, slot Completed: a fresh start — the retained controller is replaced, and retention sweeps are identity-guarded so the old controller's timer cannot evict the replacement. Adds Kotlin coordinator coverage for two resumable locks sharing the default recipient (blocked while in flight, started during the completed-retention window, sweep does not evict the replacement). Addresses review finding 7ad61228ce24. Co-Authored-By: Claude Fable 5 --- .../ShieldedFundFromAssetLockController.kt | 22 +++- .../ShieldedFundFromAssetLockCoordinator.kt | 60 +++++++-- .../example/ui/shielded/ShieldedFundScreen.kt | 15 ++- ...hieldedFundFromAssetLockCoordinatorTest.kt | 122 ++++++++++++++++-- .../ShieldedFundFromAssetLockController.swift | 23 +++- ...ShieldedFundFromAssetLockCoordinator.swift | 69 +++++++--- .../Views/ShieldedFundFromAssetLockView.swift | 24 +++- 7 files changed, 276 insertions(+), 59 deletions(-) diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockController.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockController.kt index a71724c2a99..1139195c149 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockController.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockController.kt @@ -62,6 +62,18 @@ class ShieldedFundFromAssetLockController( var lastSubmittedAt: Long? = null private set + /** + * Identity of the operation currently occupying this slot — the + * fresh-shield marker or the resumed lock's outpoint. Set by every + * accepted [submit]. The coordinator compares it to tell a re-tap of + * the SAME operation (reuse the controller) from a DIFFERENT + * operation on the same `(walletId, recipient)` slot, whose body + * [submit] would otherwise silently drop (two resumable locks + * normally share the wallet's default shielded recipient). + */ + var operationId: String? = null + private set + private var task: Job? = null /** Composite id for stable list diffing — wallet hex + recipient hex. */ @@ -71,15 +83,17 @@ class ShieldedFundFromAssetLockController( /** * Submit the funding. Defensively rejects [Phase.InFlight] and * [Phase.Completed]; [Phase.Idle] / [Phase.Failed] are allowed - * restarts. [body] performs the FFI shield call (returns nothing) or - * throws; it runs on its own dispatcher, the terminal flip hops back to - * [scope]. ← Swift `submit`. + * restarts. [operationId] names the operation the body performs (see + * [operationId]); [body] performs the FFI shield call (returns + * nothing) or throws; it runs on its own dispatcher, the terminal + * flip hops back to [scope]. ← Swift `submit`. */ - fun submit(body: suspend () -> Unit) { + fun submit(operationId: String, body: suspend () -> Unit) { when (_phase.value) { is Phase.Idle, is Phase.Failed -> Unit is Phase.InFlight, is Phase.Completed -> return } + this.operationId = operationId _phase.value = Phase.InFlight lastSubmittedAt = now() task = scope.launch { diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinator.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinator.kt index 75755546e13..843c66d487d 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinator.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinator.kt @@ -22,6 +22,15 @@ import kotlinx.coroutines.launch * [StartFundingResult.BlockedByOtherWalletFunding] (mirroring the Rust-side * `shield_guard` mutex — two concurrent Orchard builds on one wallet would * race the note-commitment tree). + * + * Reuse of a controller additionally requires a matching **operation + * identity** (`operationId` — the resumed lock's outpoint, or the + * fresh-shield marker): resumable locks normally default to the same + * wallet-owned shielded recipient, so two different locks share one slot + * key, and reusing the first lock's controller would silently drop the + * second lock's resume body. A different operation is blocked while the + * slot is in flight and replaces the retained controller once it has + * completed. */ class ShieldedFundFromAssetLockCoordinator( private val scope: CoroutineScope, @@ -69,16 +78,29 @@ class ShieldedFundFromAssetLockCoordinator( _controllers.value.values.sortedByDescending { it.lastSubmittedAt ?: Long.MIN_VALUE } /** - * Start a funding for the slot. Reuses the existing controller if the - * SAME recipient is already in flight / just completed. Rejects a - * DIFFERENT recipient while another is in flight on the same wallet - * ([StartFundingResult.BlockedByOtherWalletFunding]). [Phase.Idle] / + * Start a funding for the slot. Reuses the existing controller only + * when the SAME operation ([operationId]) is already in flight / just + * completed on the slot — a re-tap. A DIFFERENT operation on an + * in-flight slot is reported as + * [StartFundingResult.BlockedByOtherWalletFunding] rather than + * silently reusing the controller (which would drop its body: two + * resumable locks normally share the wallet's default shielded + * recipient, so the slot key alone cannot tell them apart). A + * different operation on a *completed* slot is a fresh start — the + * retained controller is replaced. Rejects a DIFFERENT recipient + * while another is in flight on the same wallet. [Phase.Idle] / * [Phase.Failed] on the same slot are legitimate restarts. * ← Swift `startFunding`. + * + * [operationId] is the identity of the requested operation: the + * resumed lock's outpoint for a resume, a fixed marker for a fresh + * shield. Wallet-wide serialization is unchanged — at most one + * shield-class operation runs per wallet either way. */ fun startFunding( walletId: ByteArray, recipientRaw43: ByteArray, + operationId: String, body: suspend () -> Unit, ): StartFundingResult { val key = key(walletId, recipientRaw43) @@ -94,14 +116,30 @@ class ShieldedFundFromAssetLockCoordinator( } if (existing != null) { - when (existing.phase.value) { + when (val phase = existing.phase.value) { is ShieldedFundFromAssetLockController.Phase.InFlight, is ShieldedFundFromAssetLockController.Phase.Completed, - -> return StartFundingResult.Started(existing) + -> { + if (existing.operationId == operationId) { + // Re-tap of the same operation: bind to its state. + return StartFundingResult.Started(existing) + } + if (phase is ShieldedFundFromAssetLockController.Phase.InFlight) { + // A different operation while one is running on this + // wallet: same serialization verdict as a different + // recipient — the caller gets the blocker, not a + // controller that will never run its body. + return StartFundingResult.BlockedByOtherWalletFunding(existing) + } + // Completed slot + different operation: a fresh start, + // not a re-tap. Fall through and REPLACE the retained + // controller (its sweep is identity-guarded, so the old + // retention timer can't evict the replacement). + } is ShieldedFundFromAssetLockController.Phase.Idle, is ShieldedFundFromAssetLockController.Phase.Failed, -> { - existing.submit(body) + existing.submit(operationId, body) scheduleRetentionSweep(key, existing) return StartFundingResult.Started(existing) } @@ -109,7 +147,7 @@ class ShieldedFundFromAssetLockCoordinator( } val controller = ShieldedFundFromAssetLockController(walletId, recipientRaw43, scope, now) _controllers.update { it + (key to controller) } - controller.submit(body) + controller.submit(operationId, body) scheduleRetentionSweep(key, controller) return StartFundingResult.Started(controller) } @@ -131,6 +169,12 @@ class ShieldedFundFromAssetLockCoordinator( scope.launch { var completedAt: Long? = null while (true) { + // The slot may have been handed to a REPLACEMENT controller + // (a different operation started during this controller's + // completed-retention window). This sweep then owns nothing: + // exit without touching the slot, leaving the replacement's + // own sweep in charge. + if (_controllers.value[key] !== controller) return@launch when (controller.phase.value) { is ShieldedFundFromAssetLockController.Phase.Completed -> { val nowMs = now() diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt index 7eff42013de..6748bfd00fc 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt @@ -265,8 +265,12 @@ fun ShieldedFundScreen( // Resume mode dispatches to a different FFI than a fresh // shield, so resolve the whole body up front — including // the outpoint parse, which must not fail after the - // coordinator has already claimed the slot. - val submitBody: suspend () -> Unit = if (isResume) { + // coordinator has already claimed the slot. The operation + // id carries the resumed lock's outpoint: resumable locks + // default to the same wallet-owned shielded recipient, so + // the coordinator's slot key alone cannot tell two locks + // apart and would silently reuse the first controller. + val (operationId, submitBody) = if (isResume) { val lock = resumeLock ?: return@SubmitButton val parsed = parseOutPoint(lock.outPointHex) if (parsed == null) { @@ -274,7 +278,7 @@ fun ShieldedFundScreen( return@SubmitButton } val (txid, vout) = parsed - { + val body: suspend () -> Unit = { m.shieldedResumeFundFromAssetLock( walletId = walletId, outPointTxid = txid, @@ -282,15 +286,17 @@ fun ShieldedFundScreen( recipientRaw43 = recipientBytes, ) } + "resume:${lock.outPointHex}" to body } else { val amountDuffs = amount ?: return@SubmitButton - { + val body: suspend () -> Unit = { m.shieldedFundFromAssetLock( walletId = walletId, recipientRaw43 = recipientBytes, amountDuffs = amountDuffs, ) } + "shield" to body } isSubmitting = true @@ -303,6 +309,7 @@ fun ShieldedFundScreen( val result = container.shieldedFundCoordinator.startFunding( walletId = walletId, recipientRaw43 = recipientBytes, + operationId = operationId, body = submitBody, ) when (result) { diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinatorTest.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinatorTest.kt index b4e846513ba..71aea133846 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinatorTest.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinatorTest.kt @@ -11,6 +11,7 @@ import org.dashfoundation.example.services.shielded.ShieldedFundFromAssetLockCon import org.dashfoundation.example.services.shielded.ShieldedFundFromAssetLockCoordinator.StartFundingResult import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotSame import org.junit.Assert.assertNull import org.junit.Assert.assertSame import org.junit.Assert.assertTrue @@ -54,7 +55,7 @@ class ShieldedFundFromAssetLockCoordinatorTest { val coordinator = coordinator() val gate = CompletableDeferred() - val controller = coordinator.startFunding(walletA, recipientX) { gate.await() }.controller() + val controller = coordinator.startFunding(walletA, recipientX, "shield") { gate.await() }.controller() assertEquals(Phase.InFlight, controller.phase.value) assertTrue(controller.phase.value.isActive) @@ -70,7 +71,7 @@ class ShieldedFundFromAssetLockCoordinatorTest { val coordinator = coordinator() val gate = CompletableDeferred() - val controller = coordinator.startFunding(walletA, recipientX) { gate.await() }.controller() + val controller = coordinator.startFunding(walletA, recipientX, "shield") { gate.await() }.controller() gate.completeExceptionally(RuntimeException("proof build failed")) advanceUntilIdle() @@ -86,8 +87,8 @@ class ShieldedFundFromAssetLockCoordinatorTest { val gate = CompletableDeferred() var invocations = 0 - val first = coordinator.startFunding(walletA, recipientX) { invocations++; gate.await() }.controller() - val second = coordinator.startFunding(walletA, recipientX) { invocations++; gate.await() }.controller() + val first = coordinator.startFunding(walletA, recipientX, "shield") { invocations++; gate.await() }.controller() + val second = coordinator.startFunding(walletA, recipientX, "shield") { invocations++; gate.await() }.controller() assertSame(first, second) assertEquals(1, coordinator.controllers.value.size) @@ -103,10 +104,10 @@ class ShieldedFundFromAssetLockCoordinatorTest { val coordinator = coordinator() val gate = CompletableDeferred() - val started = coordinator.startFunding(walletA, recipientX) { gate.await() } + val started = coordinator.startFunding(walletA, recipientX, "shield") { gate.await() } assertTrue(started is StartFundingResult.Started) - val blocked = coordinator.startFunding(walletA, recipientY) { gate.await() } + val blocked = coordinator.startFunding(walletA, recipientY, "shield") { gate.await() } assertTrue(blocked is StartFundingResult.BlockedByOtherWalletFunding) assertSame( started.controller(), @@ -124,8 +125,8 @@ class ShieldedFundFromAssetLockCoordinatorTest { val coordinator = coordinator() val gate = CompletableDeferred() - val a = coordinator.startFunding(walletA, recipientX) { gate.await() } - val b = coordinator.startFunding(walletB, recipientY) { gate.await() } + val a = coordinator.startFunding(walletA, recipientX, "shield") { gate.await() } + val b = coordinator.startFunding(walletB, recipientY, "shield") { gate.await() } assertTrue(a is StartFundingResult.Started) assertTrue(b is StartFundingResult.Started) assertEquals(2, coordinator.controllers.value.size) @@ -140,11 +141,11 @@ class ShieldedFundFromAssetLockCoordinatorTest { val firstGate = CompletableDeferred() val secondGate = CompletableDeferred() - coordinator.startFunding(walletA, recipientX) { firstGate.await() } + coordinator.startFunding(walletA, recipientX, "shield") { firstGate.await() } firstGate.complete(Unit) runCurrent() // first → Completed (not active) - val second = coordinator.startFunding(walletA, recipientY) { secondGate.await() } + val second = coordinator.startFunding(walletA, recipientY, "shield") { secondGate.await() } assertTrue(second is StartFundingResult.Started) secondGate.complete(Unit) @@ -156,7 +157,7 @@ class ShieldedFundFromAssetLockCoordinatorTest { val coordinator = coordinator(retentionMillis = 30_000L, pollMillis = 1_000L) val gate = CompletableDeferred() - val controller = coordinator.startFunding(walletA, recipientX) { gate.await() }.controller() + val controller = coordinator.startFunding(walletA, recipientX, "shield") { gate.await() }.controller() gate.complete(Unit) runCurrent() assertTrue(controller.phase.value is Phase.Completed) @@ -175,7 +176,7 @@ class ShieldedFundFromAssetLockCoordinatorTest { val coordinator = coordinator() val gate = CompletableDeferred() - coordinator.startFunding(walletA, recipientX) { gate.await() } + coordinator.startFunding(walletA, recipientX, "shield") { gate.await() } gate.completeExceptionally(RuntimeException("nope")) advanceUntilIdle() assertEquals(1, coordinator.controllers.value.size) @@ -184,4 +185,101 @@ class ShieldedFundFromAssetLockCoordinatorTest { assertTrue(coordinator.controllers.value.isEmpty()) assertNull(coordinator.controller(walletA, recipientX)) } + + // ------------------------------------------------------------------ + // Operation identity: two resumable locks share the default recipient + // ------------------------------------------------------------------ + + @Test + fun `re-tapping the same resumable lock reuses the in-flight controller`() = runTest { + val coordinator = coordinator() + val gate = CompletableDeferred() + var invocations = 0 + + val first = coordinator + .startFunding(walletA, recipientX, "resume:aa:0") { invocations++; gate.await() } + .controller() + val second = coordinator.startFunding(walletA, recipientX, "resume:aa:0") { + invocations++ + gate.await() + } + + assertTrue(second is StartFundingResult.Started) + assertSame(first, second.controller()) + runCurrent() + assertEquals(1, invocations) + + gate.complete(Unit) + advanceUntilIdle() + } + + @Test + fun `a second resumable lock on the same recipient is blocked while the first is in flight`() = runTest { + val coordinator = coordinator() + val gate = CompletableDeferred() + var secondInvocations = 0 + + val first = coordinator.startFunding(walletA, recipientX, "resume:aa:0") { gate.await() } + assertTrue(first is StartFundingResult.Started) + + // Same wallet, same (default) recipient, DIFFERENT lock: reusing + // the first controller would silently drop this body. The caller + // must see the wallet-serialization verdict instead. + val second = coordinator.startFunding(walletA, recipientX, "resume:bb:0") { + secondInvocations++ + } + assertTrue(second is StartFundingResult.BlockedByOtherWalletFunding) + assertSame( + first.controller(), + (second as StartFundingResult.BlockedByOtherWalletFunding).blocker, + ) + assertEquals(1, coordinator.controllers.value.size) + runCurrent() + assertEquals(0, secondInvocations) + + gate.complete(Unit) + advanceUntilIdle() + } + + @Test + fun `a second resumable lock starts during the first's completed retention window`() = runTest { + val coordinator = coordinator(retentionMillis = 30_000L, pollMillis = 1_000L) + val firstGate = CompletableDeferred() + val secondGate = CompletableDeferred() + var secondInvocations = 0 + + val first = coordinator + .startFunding(walletA, recipientX, "resume:aa:0") { firstGate.await() } + .controller() + firstGate.complete(Unit) + runCurrent() + assertTrue(first.phase.value is Phase.Completed) + + // Within the 30s retention window, resume a DIFFERENT lock on the + // same recipient: a fresh controller must actually run its body — + // the retained completed controller may show first's terminal + // state to a re-tap of first only. + val second = coordinator.startFunding(walletA, recipientX, "resume:bb:0") { + secondInvocations++ + secondGate.await() + } + assertTrue(second is StartFundingResult.Started) + val secondController = second.controller() + assertNotSame(first, secondController) + runCurrent() + assertEquals(1, secondInvocations) + assertEquals(Phase.InFlight, secondController.phase.value) + + // The FIRST controller's retention sweep expires while the second + // operation is still in flight — it must not evict the + // replacement occupying its old slot. + advanceTimeBy(31_000L) + runCurrent() + assertSame(secondController, coordinator.controller(walletA, recipientX)) + assertEquals(Phase.InFlight, secondController.phase.value) + + secondGate.complete(Unit) + advanceUntilIdle() + assertEquals(Phase.Completed, secondController.phase.value) + } } diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockController.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockController.swift index d4e38dc3db5..e0558f0a28e 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockController.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockController.swift @@ -79,6 +79,15 @@ final class ShieldedFundFromAssetLockController: ObservableObject { /// purge ~30s after the success transition). private(set) var lastSubmittedAt: Date? + /// Identity of the operation currently occupying this slot — the + /// fresh-shield marker or the resumed lock's outpoint. Set by every + /// accepted `submit`. The coordinator compares it to tell a re-tap + /// of the SAME operation (reuse the controller) from a DIFFERENT + /// operation on the same `(walletId, recipient)` slot, whose body + /// `submit` would otherwise silently drop (two resumable locks + /// normally share the wallet's default shielded recipient). + private(set) var operationId: String? + /// Active funding task. Holds a reference so the coordinator's /// stash retains the work until completion; cancellation isn't /// wired today (the FFI call doesn't yet support clean abort). @@ -100,19 +109,21 @@ final class ShieldedFundFromAssetLockController: ObservableObject { /// the legitimate-restart flow through them (a user retries a /// failure via `failed → submit`). /// - /// `body` performs the actual FFI call. It runs detached on a - /// background priority. Unlike the address-funding sibling, the - /// FFI returns `Void` — the shielded note arrives via the next - /// sync, not from the broadcast call — so the controller flips - /// `phase` to `.completed` (no balance payload) / `.failed` + /// `operationId` names the operation the body performs (see + /// `operationId`). `body` performs the actual FFI call. It runs + /// detached on a background priority. Unlike the address-funding + /// sibling, the FFI returns `Void` — the shielded note arrives via + /// the next sync, not from the broadcast call — so the controller + /// flips `phase` to `.completed` (no balance payload) / `.failed` /// accordingly. - func submit(body: @escaping () async throws -> Void) { + func submit(operationId: String, body: @escaping () async throws -> Void) { switch phase { case .idle, .failed: break case .inFlight, .completed: return } + self.operationId = operationId phase = .inFlight lastSubmittedAt = Date() task = Task { [weak self] in diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift index a8ad016b24c..980111a0f1c 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift @@ -86,35 +86,59 @@ final class ShieldedFundFromAssetLockCoordinator: ObservableObject { } /// Start a funding for the slot, or reuse an existing controller - /// if one is already in flight on the same recipient. Returns a - /// `StartFundingResult` so the caller can distinguish a fresh - /// start from a wallet-level conflict (see the type doc for - /// rationale). + /// if the SAME operation is already in flight on the same + /// recipient. Returns a `StartFundingResult` so the caller can + /// distinguish a fresh start from a wallet-level conflict (see + /// the type doc for rationale). /// /// Single-flighting works on two levels: - /// 1. **Per-recipient** (slot key match): a second tap on the - /// same recipient during the FFI window re-presents the - /// existing controller, never races a duplicate FFI call. - /// 2. **Per-wallet** (new in this revision): if any controller - /// on the same wallet is `.inFlight` for a *different* - /// recipient, reject with + /// 1. **Per-operation** (slot key + `operationId` match): a + /// second tap on the same operation during the FFI window + /// re-presents the existing controller, never races a + /// duplicate FFI call. The operation id (the resumed lock's + /// outpoint, or the fresh-shield marker) is required because + /// resumable locks normally default to the same wallet-owned + /// shielded recipient — two different locks share one slot + /// key, and reusing the first lock's controller would + /// silently drop the second lock's resume body. A DIFFERENT + /// operation is `.blockedByOtherWalletFunding` while the slot + /// is in flight, and replaces the retained controller once it + /// has completed. + /// 2. **Per-wallet**: if any controller on the same wallet is + /// `.inFlight` for a *different* recipient, reject with /// `.blockedByOtherWalletFunding`. Mirrors the Rust-side /// `shield_guard` mutex on `PlatformWallet` that /// serializes shield-class operations. func startFunding( walletId: Data, recipientRaw43: Data, + operationId: String, body: @escaping () async throws -> Void ) -> StartFundingResult { let key = SlotKey(walletId: walletId, recipientRaw43: recipientRaw43) if let existing = controllers[key] { switch existing.phase { - case .inFlight, .completed: - // Active or just-completed — don't re-enter. - // Returning the existing controller lets the caller - // bind to its progress / terminal state without - // disrupting it. - return .started(existing) + case .inFlight: + if existing.operationId == operationId { + // Re-tap of the same operation — bind to its progress. + return .started(existing) + } + // A different operation while one is running on this + // wallet: same serialization verdict as a different + // recipient — the caller gets the blocker, not a + // controller that will never run its body. + return .blockedByOtherWalletFunding(existing) + case .completed: + if existing.operationId == operationId { + // Re-tap after success — re-present the terminal + // state without re-entering. + return .started(existing) + } + // Completed slot + different operation: a fresh start, + // not a re-tap. Fall through past the per-wallet check + // below and REPLACE the retained controller (its sweep + // is identity-guarded, so the old retention timer can't + // evict the replacement). case .idle, .failed: // Legitimate restart paths. We've already checked // the same-recipient slot here; before letting the @@ -128,7 +152,7 @@ final class ShieldedFundFromAssetLockCoordinator: ObservableObject { ) { return .blockedByOtherWalletFunding(blocker) } - existing.submit(body: body) + existing.submit(operationId: operationId, body: body) // Spawn a fresh retention sweep. The original sweep // exited the moment the first attempt hit `.failed` // (see `scheduleRetentionSweep`'s `.failed: return` @@ -157,7 +181,7 @@ final class ShieldedFundFromAssetLockCoordinator: ObservableObject { recipientRaw43: recipientRaw43 ) controllers[key] = controller - controller.submit(body: body) + controller.submit(operationId: operationId, body: body) scheduleRetentionSweep(key: key, controller: controller) return .started(controller) } @@ -203,6 +227,15 @@ final class ShieldedFundFromAssetLockCoordinator: ObservableObject { guard let controller = controller else { return } var completedAt: Date? while !Task.isCancelled { + // The slot may have been handed to a REPLACEMENT + // controller (a different operation started during this + // controller's completed-retention window). This sweep + // then owns nothing: exit without touching the slot, + // leaving the replacement's own sweep in charge. + let ownsSlot = await MainActor.run { + self?.controllers[key] === controller + } + guard ownsSlot == true else { return } let phase = await MainActor.run { controller.phase } switch phase { case .completed: diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift index b29c094ef01..bf8cbbeac99 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift @@ -658,6 +658,11 @@ struct ShieldedFundFromAssetLockView: View { let walletId = wallet.walletId let manager = walletManager + // The operation id carries the resumed lock's outpoint: resumable + // locks default to the same wallet-owned shielded recipient, so + // the coordinator's slot key alone cannot tell two locks apart + // and would silently reuse the first lock's controller. + let operationId: String let body: () async throws -> Void if let lock = resumeFromLock { guard let parsed = parseOutPoint(lock.outPointHex) else { @@ -666,6 +671,7 @@ struct ShieldedFundFromAssetLockView: View { ) return } + operationId = "resume:\(lock.outPointHex)" body = { try await manager.shieldedResumeFundFromAssetLock( walletId: walletId, @@ -681,6 +687,7 @@ struct ShieldedFundFromAssetLockView: View { let fundingAccountIndex = fundingCoreAccountIndex, let duffs = parsedDuffs else { return } + operationId = "shield" body = { try await manager.shieldedFundFromAssetLock( walletId: walletId, @@ -694,17 +701,20 @@ struct ShieldedFundFromAssetLockView: View { } // Single-flight gate via the coordinator. Two levels: - // - Same recipient + in-flight: returns the existing - // controller (the user sees the same progress view). - // - Different recipient but another shielded funding in - // flight on this wallet: surfaces a typed "wait" - // error pointing at the in-flight recipient. Mirrors - // the Rust-side `shield_guard` mutex that serializes - // all shield-class ops per wallet. + // - Same operation (same recipient + operation id) + + // in-flight: returns the existing controller (the user + // sees the same progress view). + // - Any OTHER shielded funding in flight on this wallet — + // different recipient, or a different lock/fresh shield + // on the same recipient: surfaces a typed "wait" error + // pointing at the in-flight blocker. Mirrors the + // Rust-side `shield_guard` mutex that serializes all + // shield-class ops per wallet. let coordinator = walletManager.shieldedFundFromAssetLockCoordinator switch coordinator.startFunding( walletId: walletId, recipientRaw43: recipient, + operationId: operationId, body: body ) { case .started(let controller): From 19849ebf43aa4cc791a5e510d615d28e56539821 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 20 Aug 2026 18:00:48 -0400 Subject: [PATCH 08/12] docs(wallet-ffi): carve the accepted Built re-broadcast out of the zero-timeout bound claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resume/catch-up ABI docs claimed timeout_secs == 0 substitutes the 180s UNCONFIRMED_BROADCAST_PROOF_TIMEOUT on every proof-waiting arm, so the block_on'd host thread is parked for a bounded time in all cases. resume_asset_lock retains one deliberate exception: a Built lock whose re-broadcast the broadcaster positively ACCEPTED (Ok, not MaybeSent) forwards the original None to wait_for_proof and keeps the unbounded positive-evidence wait the initial funding path performs after its own successful broadcast. Document zero as selecting a state-dependent policy: ambiguous Built re-broadcasts, Broadcast rows, and the proof-less RecoveredFromChain fallback get the 180s default; an accepted Built re-broadcast waits for the transaction's proof (its ChainLock, ~2.5min in normal operation) without a hard bound — callers that need one pass a non-zero timeout_secs. The launch fan-out safety argument is narrowed to match: an unconnected/undeliverable broadcast reports Rejected/MaybeSent, never Ok, so offline devices only take the bounded arms. Both duplicated inline comments and the catch-up Rustdoc updated consistently. Documentation only; no behavior change. Addresses review finding d67117741b2a. Co-Authored-By: Claude Fable 5 --- .../src/asset_lock/sync.rs | 83 ++++++++++++------- 1 file changed, 53 insertions(+), 30 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs b/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs index 2c8d1424adb..a1274f5e22f 100644 --- a/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs +++ b/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs @@ -52,16 +52,26 @@ fn parse_outpoint(txid: *const [u8; 32], vout: u32) -> dashcore::OutPoint { /// /// `timeout_secs == 0` does **not** request an unbounded wait — it /// declines to specify one, and `resume_asset_lock` then applies the -/// recovery policy's own state-dependent default. Today every -/// proof-waiting arm substitutes the same 180s -/// `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` bound (sized to comfortably -/// cover a ~2.5min ChainLock), because on a resume none of them can -/// establish that the transaction is live on the network: a resume -/// re-broadcast reports `MaybeSent` for an accepted and a rejected -/// transaction alike. Waiting without a bound on that signal is a -/// `Notify` loop with no terminating event, and under the -/// `runtime().block_on(...)` below it pins the calling host thread -/// permanently rather than merely delaying an answer. +/// recovery policy's own state-dependent default: +/// +/// - An ambiguous `Built` re-broadcast (the broadcaster reports +/// `MaybeSent` for an accepted and a rejected transaction alike), +/// a `Broadcast` row, and the defensive proof-less +/// `RecoveredFromChain` fallback all substitute the 180s +/// `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` bound (sized to +/// comfortably cover a ~2.5min ChainLock): none of them can +/// establish that the transaction is live on the network, and +/// waiting without a bound on that signal is a `Notify` loop with +/// no terminating event — under the `runtime().block_on(...)` +/// below it pins the calling host thread permanently rather than +/// merely delaying an answer. +/// - The one exception: a `Built` re-broadcast the broadcaster +/// positively ACCEPTED (`Ok`) keeps the unbounded wait — the same +/// positive-evidence wait the initial funding path performs after +/// its own successful broadcast. The proof arrives with the +/// transaction's ChainLock (~2.5min) in normal operation, but the +/// wait is not time-bounded: a caller that needs a hard upper +/// bound on this thread must pass a non-zero `timeout_secs`. /// /// Expiry is non-destructive: the tracked row keeps its status, so a /// proof arriving afterwards is returned by the very next resume @@ -88,11 +98,13 @@ pub unsafe extern "C" fn asset_lock_manager_resume( check_ptr!(out_derivation_path); let out_point = parse_outpoint(txid, vout); - // `timeout_secs == 0` declines to specify a bound; it does NOT ask - // for an unbounded wait. `resume_asset_lock` reads the resulting - // `None` as "apply the recovery policy's default", which on every - // proof-waiting arm is the 180s `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT`. - // See this function's `# Timeouts` section. + // `timeout_secs == 0` declines to specify a bound. `resume_asset_lock` + // reads the resulting `None` as "apply the recovery policy's + // state-dependent default": the 180s + // `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` on every proof-waiting arm + // except a `Built` re-broadcast the broadcaster positively accepted, + // which keeps the unbounded initial-funding wait. See this + // function's `# Timeouts` section. let timeout = (timeout_secs != 0).then(|| Duration::from_secs(timeout_secs)); let option = ASSET_LOCK_MANAGER_STORAGE.with_item(handle, |manager| { @@ -133,19 +145,28 @@ pub unsafe extern "C" fn asset_lock_manager_resume( /// /// Identical contract to [`asset_lock_manager_resume`], which this /// delegates to: `timeout_secs == 0` selects the recovery policy's -/// state-dependent default rather than an unbounded wait, and that +/// state-dependent default rather than an unbounded wait. That /// default is the 180s `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` on every -/// arm that actually waits for a proof. So the thread this parks is -/// parked for a bounded time in all cases — `timeout_secs` when -/// non-zero, the policy default otherwise. +/// arm that waits for a proof without positive evidence the +/// transaction is on the network — an ambiguous `Built` re-broadcast, +/// a `Broadcast` row, the defensive proof-less `RecoveredFromChain` +/// fallback. The one exception is a `Built` re-broadcast the +/// broadcaster positively accepted (`Ok`): that arm keeps the +/// unbounded initial-funding wait, so the thread is parked until the +/// accepted transaction's proof arrives (its ChainLock, ~2.5min in +/// normal operation) rather than for a fixed bound. Pass a non-zero +/// `timeout_secs` for a hard upper bound. /// -/// That bound is what makes this entry point safe to fan out at +/// That policy is what makes this entry point safe to fan out at /// launch. The catch-up sweep starts one call per stuck lock; when -/// zero meant "wait forever", a device that was offline (or an SPV -/// session that never connected) turned each of those into a -/// permanently parked worker thread. Expiry now simply ends the pass, -/// leaving the row tracked and resumable, and the next sweep picks up -/// a proof that landed in between straight from the record. +/// zero meant "wait forever" on EVERY waiting arm, a device that was +/// offline (or an SPV session that never connected) turned each of +/// those into a permanently parked worker thread. An unconnected or +/// undeliverable broadcast can only take the bounded arms now (a +/// broadcaster that never dispatched reports `Rejected` / +/// `MaybeSent`, not `Ok`), expiry simply ends the pass, leaving the +/// row tracked and resumable, and the next sweep picks up a proof +/// that landed in between straight from the record. #[no_mangle] pub unsafe extern "C" fn asset_lock_manager_catch_up_blocking( handle: Handle, @@ -156,11 +177,13 @@ pub unsafe extern "C" fn asset_lock_manager_catch_up_blocking( check_ptr!(txid); let out_point = parse_outpoint(txid, vout); - // `timeout_secs == 0` declines to specify a bound; it does NOT ask - // for an unbounded wait. `resume_asset_lock` reads the resulting - // `None` as "apply the recovery policy's default", which on every - // proof-waiting arm is the 180s `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT`. - // See this function's `# Timeouts` section. + // `timeout_secs == 0` declines to specify a bound. `resume_asset_lock` + // reads the resulting `None` as "apply the recovery policy's + // state-dependent default": the 180s + // `UNCONFIRMED_BROADCAST_PROOF_TIMEOUT` on every proof-waiting arm + // except a `Built` re-broadcast the broadcaster positively accepted, + // which keeps the unbounded initial-funding wait. See this + // function's `# Timeouts` section. let timeout = (timeout_secs != 0).then(|| Duration::from_secs(timeout_secs)); tracing::info!( From d46ae46285a6977cf2735f231c33ebfed715100b Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 20 Aug 2026 18:00:48 -0400 Subject: [PATCH 09/12] docs(wallet): record why reconcile degrades every proof-upgrade failure to code-24 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catch-all Err arm in reconcile_asset_lock_submit_result reads as if it accidentally swallows WalletNotFound / AssetLockProofWait alongside the expected FinalityTimeout. It is deliberate: the already-consumed classification comes from Platform's outpoint-matched consensus error, not from the local ChainLock-promotion lookup, so a failed lookup does not invalidate it — and the non-timeout failures occur precisely in the degraded-local-state scenarios (lock untracked after a restore, persister failure) where the host's code-24 branch is the only path that can still resolve the operation from Platform-side evidence. Recording-path failures still propagate. Documentation only; no behavior change. Addresses review finding 9237664c50df (declined — rationale on the thread). Co-Authored-By: Claude Fable 5 --- .../src/wallet/asset_lock/orchestration.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs index d981737e575..54125815179 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs @@ -482,6 +482,22 @@ impl AssetLockManager { // (an IS-locked lock consumed seconds after broadcast has // no ChainLock yet). Record nothing, keep the code-24 // signal, let the caller retry. + // + // Deliberately a catch-all, not `FinalityTimeout`-only. + // The code-24 classification above came from Platform's + // outpoint-matched consensus error, not from this local + // lookup, so a `WalletNotFound` / `AssetLockProofWait` + // (lock untracked after a restore, persister failure, + // record-map mismatch) does not invalidate it — the + // promotion is best-effort evidence-attachment either + // way. Propagating those errors instead would swap the + // host's actionable already-consumed branch for a + // generic local error in exactly the degraded-state + // scenarios where that branch is the only path that can + // still resolve the operation from Platform-side + // evidence. Failures on the RECORDING path below do + // propagate (`mark_asset_lock_consumption_unknown` keeps + // its `?`). Err(e) => { tracing::warn!( outpoint = %out_point, From 6ff9c1e28873b9afe9cd585ab4a8aa825a5d9c4f Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:33:17 -0400 Subject: [PATCH 10/12] test(wallet): pin both promotion-failure arms of the reconciliation downgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catch-all Err arm in reconcile_asset_lock_submit_result deliberately downgrades EVERY upgrade_to_chain_lock_proof failure to the code-24 AssetLockAlreadyConsumed signal, but only the outcome was pinned — and by exactly one scenario. Worse, that scenario was not the one its test documented: the built funding tx is never registered as a TransactionRecord and NoopTestPersister keeps the trait's Ok(None) lookup, so the "terminates without a ChainLock" test never reached wait_for_chain_lock at all. It fast-failed with AssetLockProofWait — accidentally covering the non-timeout arm while leaving the timeout arm it described unexercised. Restructure into a shared fixture plus one test per arm, each asserting the promotion's error variant DIRECTLY before checking the downgrade, so the scenarios cannot silently collapse onto the same path: - already_consumed_reconciliation_terminates_without_a_chainlock now registers the funding tx's (mempool, non-chain-locked) record so the promotion genuinely dispatches to wait_for_chain_lock and burns the bound: pinned as FinalityTimeout, then downgraded. - already_consumed_reconciliation_downgrades_non_timeout_promotion_failure (new) keeps the record unavailable — the post-restore degraded-state shape — and pins the AssetLockProofWait fast-fail, then the downgrade with the tracked row untouched. Verified against the feared refactor: narrowing the catch-all to FinalityTimeout-only fails the new test ("must DOWNGRADE to the code-24 signal ... got AssetLockProofWait") while the rest of the suite stays green. Addresses review finding 658b2cacd55e. Co-Authored-By: Claude Fable 5 --- .../src/wallet/asset_lock/orchestration.rs | 289 ++++++++++++++---- 1 file changed, 227 insertions(+), 62 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs index 54125815179..f4056957dea 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs @@ -962,39 +962,59 @@ mod tests { } } - /// Regression: the already-consumed reconciliation must TERMINATE when - /// the ChainLock it wants never arrives. - /// - /// Shape: the funding transaction is present and tracked but its record - /// is not in a chain-locked block, the effective proof is an - /// InstantSend proof (so the IS→CL promotion runs), no SPV chainlock is - /// ever delivered, and `chain_lock_timeout` is `None` — exactly what all - /// three production call sites pass (`identity/network/registration.rs` - /// x2, `platform_addresses/fund_from_asset_lock.rs`). - /// - /// Before the fix `None` meant "wait forever" and this future never - /// resolved. Under FFI that is a permanently pinned host thread, since - /// every one of those call sites is reached through `runtime() - /// .block_on(...)`. The realistic trigger is ordinary: a lock consumed - /// seconds after broadcast is IS-locked but not yet chain-locked (~2.5 - /// min away), and never chain-locked at all when the device is offline. + // ----------------------------------------------------------------- + // Reconciliation promotion-failure regressions + // + // Two tests pin the two failure shapes of the IS→CL promotion inside + // `reconcile_asset_lock_submit_result`, one per arm of the deliberate + // catch-all on its `Err` branch: + // - `already_consumed_reconciliation_terminates_without_a_chainlock` + // pins the BOUNDED-WAIT shape (`FinalityTimeout`); + // - `already_consumed_reconciliation_downgrades_non_timeout_promotion_failure` + // pins the DEGRADED-LOCAL-STATE shape (`AssetLockProofWait`). + // Each first asserts the promotion error variant DIRECTLY, so the two + // scenarios cannot silently collapse onto the same path, then asserts + // the shared downgrade outcome. + // ----------------------------------------------------------------- + + use crate::test_support::{ + funded_wallet_manager, AlwaysRejectedBroadcaster, NoopTestPersister, + }; + use crate::wallet::asset_lock::manager::AssetLockManager; + use crate::wallet::asset_lock::tracked::{AssetLockStatus, TrackedAssetLock}; + use crate::wallet::persister::WalletPersister; + use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; + use dpp::consensus::basic::identity::IdentityAssetLockTransactionOutPointAlreadyConsumedError; + use dpp::identity::state_transition::asset_lock_proof::InstantAssetLockProof; + use std::sync::Arc; + + /// Shared fixture: a funded wallet with ONE tracked, IS-locked asset + /// lock whose effective proof is an Instant proof — the shape that + /// routes `reconcile_asset_lock_submit_result` through the + /// `upgrade_to_chain_lock_proof` promotion (a Chain proof would + /// short-circuit past it). /// - /// `start_paused` lets the runtime auto-advance the bounded sleep, so - /// the assertion is that the call resolves at all — and resolves as the - /// typed code-24 `AssetLockAlreadyConsumed` the hosts branch on, not as - /// the `FinalityTimeout` of the failed promotion. - #[tokio::test(start_paused = true)] - async fn already_consumed_reconciliation_terminates_without_a_chainlock() { - use crate::test_support::{ - funded_wallet_manager, AlwaysRejectedBroadcaster, NoopTestPersister, - }; - use crate::wallet::asset_lock::manager::AssetLockManager; - use crate::wallet::asset_lock::tracked::{AssetLockStatus, TrackedAssetLock}; - use crate::wallet::persister::WalletPersister; + /// The built funding transaction is deliberately NOT registered as a + /// `TransactionRecord` anywhere: `build_asset_lock_transaction` only + /// reserves inputs, nothing is broadcast, and `NoopTestPersister` + /// keeps the persistence trait's `Ok(None)` record lookup. Out of the + /// box the promotion therefore fast-fails with `AssetLockProofWait` + /// ("transaction not found"); a test that wants the bounded-wait + /// `FinalityTimeout` shape instead must register a (non-chain-locked) + /// record for `transaction` first. + struct InstantReconciliationContext { + manager: AssetLockManager, + wallet_manager: + Arc>>, + wallet_id: WalletId, + transaction: dashcore::Transaction, + out_point: OutPoint, + instant_proof: AssetLockProof, + } + + async fn instant_reconciliation_context() -> InstantReconciliationContext { use dashcore::{InstantLock, Network}; - use dpp::identity::state_transition::asset_lock_proof::InstantAssetLockProof; use key_wallet::account::account_type::StandardAccountType; - use std::sync::Arc; use tokio::sync::Notify; let (wallet_manager, wallet_id, _generation, signer) = @@ -1041,7 +1061,7 @@ mod tests { out_point, TrackedAssetLock { out_point, - transaction, + transaction: transaction.clone(), account_index: 0, funding_type: AssetLockFundingType::IdentityRegistration, identity_index: 0, @@ -1052,23 +1072,42 @@ mod tests { ); } - // The unauthenticated code-24 consensus response that puts - // `reconcile_asset_lock_submit_result` on the reconciliation path. - use dpp::consensus::basic::identity::IdentityAssetLockTransactionOutPointAlreadyConsumedError; - let already_consumed = - dash_sdk::Error::Protocol(dpp::ProtocolError::ConsensusError(Box::new( - IdentityAssetLockTransactionOutPointAlreadyConsumedError::new( - out_point.txid, - out_point.vout as usize, - ) - .into(), - ))); - - let error = manager + InstantReconciliationContext { + manager, + wallet_manager, + wallet_id, + transaction, + out_point, + instant_proof, + } + } + + /// The unauthenticated code-24 consensus response that puts + /// `reconcile_asset_lock_submit_result` on the reconciliation path. + fn already_consumed_error(out_point: OutPoint) -> dash_sdk::Error { + dash_sdk::Error::Protocol(dpp::ProtocolError::ConsensusError(Box::new( + IdentityAssetLockTransactionOutPointAlreadyConsumedError::new( + out_point.txid, + out_point.vout as usize, + ) + .into(), + ))) + } + + /// The downgrade outcome both promotion-failure shapes must share: + /// `reconcile_asset_lock_submit_result` (called with `None`, exactly + /// what every production call site passes) still returns the typed + /// code-24 `AssetLockAlreadyConsumed`, and the tracked row keeps what + /// it had — no ChainLock proof was obtainable, so nothing may claim + /// consumption-unknown state, and a later retry can still pick the + /// proof up. + async fn assert_downgraded_to_already_consumed(ctx: &InstantReconciliationContext) { + let error = ctx + .manager .reconcile_asset_lock_submit_result::<()>( - Err(already_consumed), - &out_point, - &instant_proof, + Err(already_consumed_error(ctx.out_point)), + &ctx.out_point, + &ctx.instant_proof, None, ) .await @@ -1077,29 +1116,155 @@ mod tests { assert!( matches!( error, - PlatformWalletError::AssetLockAlreadyConsumed(actual) if actual == out_point + PlatformWalletError::AssetLockAlreadyConsumed(actual) if actual == ctx.out_point ), - "reconciliation must terminate carrying the code-24 signal even when the \ - ChainLock never arrives, got {error:?}" + "a failed IS→CL promotion must DOWNGRADE to the code-24 signal the hosts \ + branch on — not propagate the promotion's own error, got {error:?}" ); - // No ChainLock proof was obtainable, so nothing may claim - // consumption-unknown state: the row keeps what it had, and a later - // retry can still pick the proof up. - let status = wallet_manager - .read() - .await - .get_wallet_info(&wallet_id) + let wm = ctx.wallet_manager.read().await; + let lock = wm + .get_wallet_info(&ctx.wallet_id) .expect("wallet") .tracked_asset_locks - .get(&out_point) - .expect("lock stays tracked") - .status - .clone(); + .get(&ctx.out_point) + .expect("lock stays tracked"); assert_eq!( - status, + lock.status, AssetLockStatus::InstantSendLocked, "without a chain proof the lock must NOT be promoted to RecoveredFromChain" ); + assert_eq!( + lock.proof, + Some(ctx.instant_proof.clone()), + "the tracked proof must be untouched by a failed promotion" + ); + } + + /// Regression: the already-consumed reconciliation must TERMINATE when + /// the ChainLock it wants never arrives. + /// + /// Shape: the funding transaction is present and tracked and its record + /// is registered but not in a chain-locked block, the effective proof + /// is an InstantSend proof (so the IS→CL promotion runs and dispatches + /// to `wait_for_chain_lock`), and no SPV chainlock is ever delivered. + /// + /// Before the fix a `None` reconciliation timeout meant "wait forever" + /// and this future never resolved. Under FFI that is a permanently + /// pinned host thread, since every production call site is reached + /// through `runtime().block_on(...)`. The realistic trigger is + /// ordinary: a lock consumed seconds after broadcast is IS-locked but + /// not yet chain-locked (~2.5 min away), and never chain-locked at all + /// when the device is offline. + /// + /// `start_paused` lets the runtime auto-advance the bounded sleep, so + /// the assertion is that the call resolves at all — and resolves as the + /// typed code-24 `AssetLockAlreadyConsumed` the hosts branch on, not as + /// the `FinalityTimeout` of the failed promotion. + #[tokio::test(start_paused = true)] + async fn already_consumed_reconciliation_terminates_without_a_chainlock() { + use key_wallet::account::account_type::StandardAccountType; + use key_wallet::account::AccountType; + use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; + use key_wallet::managed_account::transaction_record::{ + TransactionDirection, TransactionRecord, + }; + use key_wallet::transaction_checking::{TransactionContext, TransactionType}; + + let ctx = instant_reconciliation_context().await; + + // Register the funding tx's record (mempool context, NOT chain- + // locked) under BIP44 account 0, so the promotion's record lookup + // succeeds and it genuinely dispatches to `wait_for_chain_lock`. + // Without this the lookup misses and the promotion fast-fails with + // `AssetLockProofWait` before any waiting — the OTHER regression's + // scenario, which must stay distinct from this one. + { + let record = TransactionRecord::new( + ctx.transaction.clone(), + AccountType::Standard { + index: 0, + standard_account_type: StandardAccountType::BIP44Account, + }, + TransactionContext::Mempool, + TransactionType::Standard, + TransactionDirection::Outgoing, + Vec::new(), + Vec::new(), + 0, + ); + let mut wm = ctx.wallet_manager.write().await; + wm.get_wallet_info_mut(&ctx.wallet_id) + .expect("wallet must remain registered") + .core_wallet + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("funded fixture has BIP44 account 0") + .transactions_mut() + .insert(ctx.out_point.txid, record); + } + + // Pin the SCENARIO, not just the outcome: the promotion itself must + // burn the bound and report `FinalityTimeout` (auto-advanced under + // `start_paused`), proving this test exercises the timeout arm of + // the reconciliation catch-all and not the fast-fail one. + let promotion_err = ctx + .manager + .upgrade_to_chain_lock_proof(&ctx.out_point, Some(RECONCILIATION_CHAIN_LOCK_TIMEOUT)) + .await + .expect_err("no ChainLock ever arrives: the promotion must fail"); + assert!( + matches!( + promotion_err, + PlatformWalletError::FinalityTimeout(actual) if actual == ctx.out_point + ), + "expected the promotion to time out waiting for a ChainLock, got {promotion_err:?}" + ); + + assert_downgraded_to_already_consumed(&ctx).await; + } + + /// Companion regression pinning the DELIBERATE breadth of the + /// promotion's `Err` catch-all in `reconcile_asset_lock_submit_result` + /// (see the comment on that arm): a NON-timeout promotion failure must + /// be downgraded to the code-24 signal exactly like a timeout, because + /// the already-consumed classification came from Platform's + /// outpoint-matched consensus error — a failed local lookup does not + /// invalidate it, and the non-timeout failures occur precisely in the + /// degraded-local-state scenarios where the host's code-24 branch is + /// the only path that can still resolve the operation. + /// + /// Shape: the lock is tracked, but its transaction record is + /// unavailable — never registered in any account's in-memory map (the + /// fixture never broadcasts) and unknown to the persister + /// (`NoopTestPersister` keeps the trait's `Ok(None)` default). That is + /// the post-restore / wallet-state-mismatch shape, and the promotion + /// fast-fails with `AssetLockProofWait` instead of waiting. + /// + /// "Fixing" the catch-all to propagate everything but + /// `FinalityTimeout` turns the reconcile result below into + /// `AssetLockProofWait` and fails this test — that narrowing was + /// proposed and declined in review (finding 9237664c50df); this test + /// keeps the decision from silently regressing. + #[tokio::test(start_paused = true)] + async fn already_consumed_reconciliation_downgrades_non_timeout_promotion_failure() { + let ctx = instant_reconciliation_context().await; + + // Pin the SCENARIO first: with the record unavailable, the + // promotion must fail with the NON-timeout `AssetLockProofWait` + // fast-fail. If a future fixture change made the record findable, + // this assertion — not a silently green downgrade check — fails. + let promotion_err = ctx + .manager + .upgrade_to_chain_lock_proof(&ctx.out_point, Some(RECONCILIATION_CHAIN_LOCK_TIMEOUT)) + .await + .expect_err("record unavailable: the promotion must fail"); + assert!( + matches!(promotion_err, PlatformWalletError::AssetLockProofWait(_)), + "expected the non-timeout AssetLockProofWait fast-fail, got {promotion_err:?}" + ); + + assert_downgraded_to_already_consumed(&ctx).await; } } From fc623361a115a1b629c76a198ad2df3b094333d4 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Sun, 23 Aug 2026 15:04:38 -0400 Subject: [PATCH 11/12] fix(wallet): consult the local proof before failing a rejected defensive re-broadcast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A row can sit at Broadcast while its transaction record already carries finality: LockNotifyHandler only wakes waiters, so an IS/CL event that arrives with no waiter active enriches the record without advancing the tracked status, and enrich_from_record upgrades only chain-locked records on scan paths (an InstantSend context is invisible to it). On the next launch catchUpStuckAssetLocks resumes the row before SPV connects, the defensive re-broadcast draws Rejected (unstarted client / zero connected peers), and the Broadcast arm failed the resume even though wait_for_proof would have returned the proof on its first iteration — straight from the local record, without any network. On Rejected, probe the record once via wait_for_proof with a zero bound (exactly one local record/persister check, expires before touching the network) and complete the resume from the proof when one exists; surface the broadcast error, row untouched, only when the probe finds nothing. Co-Authored-By: Claude Fable 5 --- .../src/wallet/asset_lock/sync/recovery.rs | 211 +++++++++++++++--- 1 file changed, 184 insertions(+), 27 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs index 194fa850c9e..736e0516b4d 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs @@ -367,7 +367,23 @@ impl AssetLockManager { // immediately from the SPV/persisted record. // // A DEFINITE `Rejected` ends the resume early — but it says - // NOTHING about the row, and must not be read as one. + // NOTHING about the row, and must not be read as one. In + // fact the row's RECORD may already hold the answer: a lock + // can sit at `Broadcast` while its transaction record + // carries an IS lock or a chain-locked context, because + // finality that arrives with no waiter active enriches the + // record without advancing the tracked status + // (`LockNotifyHandler` only wakes waiters, and + // `enrich_from_record` upgrades only chain-locked records + // on scan paths). So before surfacing the rejection, probe + // the record once, without waiting — `wait_for_proof` with + // a zero bound performs exactly one local record/persister + // check and expires before touching the network. On the + // canonical trigger (`catchUpStuckAssetLocks` resuming + // rows at launch before SPV connects) that probe is the + // difference between completing an already-final lock + // entirely offline and failing it every launch until + // connectivity returns. // // `Rejected` is scoped to the attempt that produced it. With // the production `SpvBroadcaster` it is reachable from @@ -395,27 +411,46 @@ impl AssetLockManager { // successful broadcast too (app killed between the send and // the status advance), which is precisely why the `Built` arm // above also only surfaces the error and leaves its row alone. + let mut local_proof = None; if let Err(e) = self.broadcaster.broadcast(&tx).await { if matches!(e, BroadcastError::Rejected { .. }) { - tracing::warn!( + match self.wait_for_proof(out_point, Some(Duration::ZERO)).await { + Ok(proof) => { + tracing::info!( + outpoint = %out_point, + error = %e, + "resume_asset_lock: defensive re-broadcast of a \ + Broadcast-status lock was rejected, but the \ + local record already holds finality — \ + completing the resume from the local proof" + ); + local_proof = Some(proof); + } + Err(probe_err) => { + tracing::warn!( + outpoint = %out_point, + error = %e, + probe = %probe_err, + "resume_asset_lock: defensive re-broadcast of a \ + Broadcast-status lock was definitively rejected \ + and no local proof exists — this attempt never \ + left the device, which proves nothing about the \ + original broadcast; leaving the row tracked at \ + Broadcast and failing the resume" + ); + return Err(e.into()); + } + } + } else { + tracing::debug!( outpoint = %out_point, error = %e, "resume_asset_lock: defensive re-broadcast of a \ - Broadcast-status lock was definitively rejected — \ - this attempt never left the device, which proves \ - nothing about the original broadcast; leaving the \ - row tracked at Broadcast and failing the resume" + Broadcast-status lock returned an unknown outcome (likely \ + already in a mempool or mined); proceeding to wait \ + for proof" ); - return Err(e.into()); } - tracing::debug!( - outpoint = %out_point, - error = %e, - "resume_asset_lock: defensive re-broadcast of a \ - Broadcast-status lock returned an unknown outcome (likely \ - already in a mempool or mined); proceeding to wait \ - for proof" - ); } // Bounded like the `Built` arm, and for the same reason. This // arm is only entered on a RESUME, i.e. for a transaction @@ -439,19 +474,25 @@ impl AssetLockManager { // having asked for an unbounded wait. The shielded seed pool // reads `FinalityTimeout` as a pacing signal, so re-typing it // for everyone would break a working flow to fix another. - let bounded = timeout.or(Some(UNCONFIRMED_BROADCAST_PROOF_TIMEOUT)); - let proof = match self.wait_for_proof(out_point, bounded).await { - Ok(proof) => proof, - Err(PlatformWalletError::FinalityTimeout(_)) if timeout.is_none() => { - let reason = format!( - "asset lock {} is tracked as broadcast but no \ - InstantSend/ChainLock proof arrived within {:?}; the \ - lock remains tracked and resumable", - out_point, UNCONFIRMED_BROADCAST_PROOF_TIMEOUT - ); - return Err(PlatformWalletError::TransactionBroadcastUnconfirmed(reason)); + let proof = if let Some(proof) = local_proof { + proof + } else { + let bounded = timeout.or(Some(UNCONFIRMED_BROADCAST_PROOF_TIMEOUT)); + match self.wait_for_proof(out_point, bounded).await { + Ok(proof) => proof, + Err(PlatformWalletError::FinalityTimeout(_)) if timeout.is_none() => { + let reason = format!( + "asset lock {} is tracked as broadcast but no \ + InstantSend/ChainLock proof arrived within {:?}; the \ + lock remains tracked and resumable", + out_point, UNCONFIRMED_BROADCAST_PROOF_TIMEOUT + ); + return Err(PlatformWalletError::TransactionBroadcastUnconfirmed( + reason, + )); + } + Err(e) => return Err(e), } - Err(e) => return Err(e), }; self.validate_or_upgrade_proof(proof, account_index, out_point) .await? @@ -1289,6 +1330,122 @@ mod tests { ); } + /// A definite rejection must consult the LOCAL record before failing + /// the resume. + /// + /// A row can sit at `Broadcast` while its transaction record already + /// carries finality: `LockNotifyHandler` only wakes waiters, so an + /// IS/CL event that arrives with no waiter active enriches the record + /// but never advances the tracked status, and `enrich_from_record` + /// upgrades only `InChainLockedBlock` records on scan paths — an + /// `InstantSend` context is invisible to it. On the next launch + /// `catchUpStuckAssetLocks` resumes the row before SPV connects, the + /// defensive re-broadcast draws `Rejected` (unstarted client / zero + /// peers), and the pre-fix arm failed the resume even though + /// `wait_for_proof` would have returned the proof on its first + /// iteration, straight from the record, without any network at all. + #[tokio::test] + async fn definite_rejection_on_a_broadcast_lock_yields_the_local_proof() { + use dashcore::ephemerealdata::instant_lock::InstantLock; + use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; + use key_wallet::managed_account::transaction_record::{ + TransactionDirection, TransactionRecord, + }; + use key_wallet::transaction_checking::{TransactionContext, TransactionType}; + + let (wallet_manager, wallet_id, _balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let sdk = Arc::new( + dash_sdk::SdkBuilder::new_mock() + .with_network(Network::Testnet) + .build() + .expect("mock sdk"), + ); + let manager = AssetLockManager::new( + sdk, + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(Notify::new()), + Arc::new(AlwaysRejectedBroadcaster), + WalletPersister::new(wallet_id, Arc::new(RecordingPersistence::default())), + ); + let (transaction, _path) = manager + .build_asset_lock_transaction( + 1_000_000, + 0, + AssetLockFundingType::AssetLockAddressTopUp, + 4, + &signer, + ) + .await + .expect("build asset lock"); + let out_point = OutPoint::new(transaction.txid(), 0); + { + let mut wm = wallet_manager.write().await; + let info = wm + .get_wallet_info_mut(&wallet_id) + .expect("wallet must remain registered"); + // The finality that arrived while nobody was waiting: an + // IS-locked record for the funding tx, filed under the BIP44 + // account the lock was built from. + let record = TransactionRecord::new( + transaction.clone(), + AccountType::Standard { + index: 0, + standard_account_type: StandardAccountType::BIP44Account, + }, + TransactionContext::InstantSend(InstantLock::default()), + TransactionType::Standard, + TransactionDirection::Outgoing, + Vec::new(), + Vec::new(), + 0, + ); + info.core_wallet + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("funded wallet has BIP44 account 0") + .transactions_mut() + .insert(record.txid, record); + info.tracked_asset_locks.insert( + out_point, + TrackedAssetLock { + out_point, + transaction, + account_index: 0, + funding_type: AssetLockFundingType::AssetLockAddressTopUp, + identity_index: 4, + amount: 1_000_000, + status: AssetLockStatus::Broadcast, + proof: None, + }, + ); + } + + let (proof, _path) = manager + .resume_asset_lock(&out_point, None) + .await + .expect("a locally-proven lock must survive a rejected re-broadcast"); + assert!( + matches!(proof, dpp::prelude::AssetLockProof::Instant(_)), + "the proof must come from the record's InstantSend context: {proof:?}" + ); + assert_eq!( + wallet_manager + .read() + .await + .get_wallet_info(&wallet_id) + .expect("wallet") + .tracked_asset_locks + .get(&out_point) + .expect("lock stays tracked") + .status, + AssetLockStatus::InstantSendLocked, + "the resume must advance the row exactly as a waited-for proof would" + ); + } + /// Regression: the `Broadcast` arm's proof wait must terminate on the /// UNBOUNDED resume path too. /// From d03dcf7fb41e30e865f250516e3decbfdc0bfdff Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Mon, 24 Aug 2026 07:42:43 -0400 Subject: [PATCH 12/12] fix(example-apps): mint a unique operation id per fresh shield MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-4 single-flight keyed controller reuse by (slot, operationId), but both hosts passed a fixed "shield" marker for every fresh shield. Within the coordinator's ~30s completed-retention window, a second fresh shield to the same recipient matched the retained controller's operation id and rebound to the old Completed state — the new FFI body never ran (Swift: dismiss the completed sheet and shield again; Kotlin: back out of the completed progress route and resubmit the form). Fresh shields now mint "shield:" at submission time in both call sites (ShieldedFundFromAssetLockView.swift, ShieldedFundScreen.kt), so every fresh user attempt is a distinct operation: blocked while another is in flight, a genuine replacement once the slot has completed. "resume:" stays stable so a re-tap of the same lock still rebinds to its controller. Coordinator docs updated in both hosts; two Kotlin coordinator regression tests pin the replacement (body runs, controller replaced, sweep hand-off) and the in-flight block. Co-Authored-By: Claude Fable 5 --- .../ShieldedFundFromAssetLockCoordinator.kt | 24 +++--- .../example/ui/shielded/ShieldedFundScreen.kt | 11 ++- ...hieldedFundFromAssetLockCoordinatorTest.kt | 73 +++++++++++++++++++ ...ShieldedFundFromAssetLockCoordinator.swift | 19 +++-- .../Views/ShieldedFundFromAssetLockView.swift | 9 ++- 5 files changed, 116 insertions(+), 20 deletions(-) diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinator.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinator.kt index 843c66d487d..edf4d52de6b 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinator.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinator.kt @@ -24,13 +24,15 @@ import kotlinx.coroutines.launch * race the note-commitment tree). * * Reuse of a controller additionally requires a matching **operation - * identity** (`operationId` — the resumed lock's outpoint, or the - * fresh-shield marker): resumable locks normally default to the same - * wallet-owned shielded recipient, so two different locks share one slot - * key, and reusing the first lock's controller would silently drop the - * second lock's resume body. A different operation is blocked while the - * slot is in flight and replaces the retained controller once it has - * completed. + * identity** (`operationId` — the resumed lock's outpoint, or a unique + * per-submission fresh-shield id): resumable locks normally default to + * the same wallet-owned shielded recipient, so two different locks share + * one slot key, and reusing the first lock's controller would silently + * drop the second lock's resume body. A different operation is blocked + * while the slot is in flight and replaces the retained controller once + * it has completed. Fresh shields mint a NEW id per submission for the + * same reason: a fixed marker would match the retained completed + * controller and suppress the next fresh shield's body. */ class ShieldedFundFromAssetLockCoordinator( private val scope: CoroutineScope, @@ -93,9 +95,11 @@ class ShieldedFundFromAssetLockCoordinator( * ← Swift `startFunding`. * * [operationId] is the identity of the requested operation: the - * resumed lock's outpoint for a resume, a fixed marker for a fresh - * shield. Wallet-wide serialization is unchanged — at most one - * shield-class operation runs per wallet either way. + * resumed lock's outpoint for a resume (stable, so a re-tap of the + * same lock rebinds), a unique per-submission id for a fresh shield + * (so a second fresh shield never rebinds to the previous one's + * retained result). Wallet-wide serialization is unchanged — at most + * one shield-class operation runs per wallet either way. */ fun startFunding( walletId: ByteArray, diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt index 6748bfd00fc..b12de248558 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/shielded/ShieldedFundScreen.kt @@ -47,6 +47,7 @@ import org.dashfoundation.example.ui.funding.shortOutPointDisplay import org.dashfoundation.example.ui.funding.statusLabel import org.dashfoundation.example.util.hexToBytes import org.dashfoundation.example.util.toHex +import java.util.UUID /** * Shield funds from an asset lock — port of `ShieldedFundFromAssetLockView.swift`. @@ -270,6 +271,14 @@ fun ShieldedFundScreen( // default to the same wallet-owned shielded recipient, so // the coordinator's slot key alone cannot tell two locks // apart and would silently reuse the first controller. + // A fresh shield mints a NEW id per submission: the + // coordinator retains a completed controller for ~30s, + // and a fixed marker would match it and rebind — showing + // the old result instead of running this submission's + // body. Minted here, at submission time, so returning to + // this form and shielding again is always a new + // operation ("resume:" stays stable so a + // re-tap of the SAME lock still rebinds). val (operationId, submitBody) = if (isResume) { val lock = resumeLock ?: return@SubmitButton val parsed = parseOutPoint(lock.outPointHex) @@ -296,7 +305,7 @@ fun ShieldedFundScreen( amountDuffs = amountDuffs, ) } - "shield" to body + "shield:${UUID.randomUUID()}" to body } isSubmitting = true diff --git a/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinatorTest.kt b/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinatorTest.kt index 71aea133846..1b075da485c 100644 --- a/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinatorTest.kt +++ b/packages/kotlin-sdk/KotlinExampleApp/app/src/test/java/org/dashfoundation/example/services/shielded/ShieldedFundFromAssetLockCoordinatorTest.kt @@ -282,4 +282,77 @@ class ShieldedFundFromAssetLockCoordinatorTest { advanceUntilIdle() assertEquals(Phase.Completed, secondController.phase.value) } + + // ------------------------------------------------------------------ + // Operation identity: fresh shields mint a unique id per submission + // ------------------------------------------------------------------ + + @Test + fun `a second fresh shield with a distinct id replaces the retained completed controller`() = runTest { + val coordinator = coordinator(retentionMillis = 30_000L, pollMillis = 1_000L) + val firstGate = CompletableDeferred() + val secondGate = CompletableDeferred() + var secondInvocations = 0 + + // The call sites mint "shield:" at submission time; a + // FIXED marker here would rebind to the first controller's + // retained Completed state and never run the second body. + val first = coordinator + .startFunding(walletA, recipientX, "shield:attempt-1") { firstGate.await() } + .controller() + firstGate.complete(Unit) + runCurrent() + assertTrue(first.phase.value is Phase.Completed) + + // Within the retention window, a SECOND fresh shield to the + // same recipient must replace the retained controller and + // execute its own body. + val second = coordinator.startFunding(walletA, recipientX, "shield:attempt-2") { + secondInvocations++ + secondGate.await() + } + assertTrue(second is StartFundingResult.Started) + val secondController = second.controller() + assertNotSame(first, secondController) + runCurrent() + assertEquals(1, secondInvocations) + assertEquals(Phase.InFlight, secondController.phase.value) + + secondGate.complete(Unit) + runCurrent() + assertEquals(Phase.Completed, secondController.phase.value) + // The replacement owns the slot until its own retention sweep + // retires it as usual. + assertSame(secondController, coordinator.controller(walletA, recipientX)) + advanceUntilIdle() + assertNull(coordinator.controller(walletA, recipientX)) + } + + @Test + fun `a second fresh shield is blocked while the first is still in flight`() = runTest { + val coordinator = coordinator() + val gate = CompletableDeferred() + var secondInvocations = 0 + + val first = coordinator.startFunding(walletA, recipientX, "shield:attempt-1") { gate.await() } + assertTrue(first is StartFundingResult.Started) + + // Distinct per-submission ids mean a second fresh shield during + // the first's flight is a DIFFERENT operation: it must surface + // the wallet-serialization verdict, not silently rebind. + val second = coordinator.startFunding(walletA, recipientX, "shield:attempt-2") { + secondInvocations++ + } + assertTrue(second is StartFundingResult.BlockedByOtherWalletFunding) + assertSame( + first.controller(), + (second as StartFundingResult.BlockedByOtherWalletFunding).blocker, + ) + assertEquals(1, coordinator.controllers.value.size) + runCurrent() + assertEquals(0, secondInvocations) + + gate.complete(Unit) + advanceUntilIdle() + } } diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift index 980111a0f1c..02fe09d2c7c 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/ShieldedFundFromAssetLockCoordinator.swift @@ -96,14 +96,17 @@ final class ShieldedFundFromAssetLockCoordinator: ObservableObject { /// second tap on the same operation during the FFI window /// re-presents the existing controller, never races a /// duplicate FFI call. The operation id (the resumed lock's - /// outpoint, or the fresh-shield marker) is required because - /// resumable locks normally default to the same wallet-owned - /// shielded recipient — two different locks share one slot - /// key, and reusing the first lock's controller would - /// silently drop the second lock's resume body. A DIFFERENT - /// operation is `.blockedByOtherWalletFunding` while the slot - /// is in flight, and replaces the retained controller once it - /// has completed. + /// outpoint, or a unique per-submission fresh-shield id) is + /// required because resumable locks normally default to the + /// same wallet-owned shielded recipient — two different locks + /// share one slot key, and reusing the first lock's controller + /// would silently drop the second lock's resume body. A + /// DIFFERENT operation is `.blockedByOtherWalletFunding` while + /// the slot is in flight, and replaces the retained controller + /// once it has completed. Fresh shields mint a NEW id per + /// submission for the same reason: a fixed marker would match + /// the retained completed controller and suppress the next + /// fresh shield's body. /// 2. **Per-wallet**: if any controller on the same wallet is /// `.inFlight` for a *different* recipient, reject with /// `.blockedByOtherWalletFunding`. Mirrors the Rust-side diff --git a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift index bf8cbbeac99..800e2bd2e6a 100644 --- a/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift +++ b/packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift @@ -662,6 +662,13 @@ struct ShieldedFundFromAssetLockView: View { // locks default to the same wallet-owned shielded recipient, so // the coordinator's slot key alone cannot tell two locks apart // and would silently reuse the first lock's controller. + // A fresh shield mints a NEW id per submission: the coordinator + // retains a completed controller for ~30s, and a fixed marker + // would match it and rebind — reopening the old result instead + // of running this submission's body. Minted here, at submission + // time, so dismissing a completed sheet and shielding again is + // always a new operation ("resume:" stays stable so a + // re-tap of the SAME lock still rebinds). let operationId: String let body: () async throws -> Void if let lock = resumeFromLock { @@ -687,7 +694,7 @@ struct ShieldedFundFromAssetLockView: View { let fundingAccountIndex = fundingCoreAccountIndex, let duffs = parsedDuffs else { return } - operationId = "shield" + operationId = "shield:\(UUID().uuidString)" body = { try await manager.shieldedFundFromAssetLock( walletId: walletId,