diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt index 6b6623fdbcc..33de8206989 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt @@ -326,13 +326,23 @@ sealed class DashSdkError( PlatformWallet(message, cause) /** - * `ErrorStaleReservationToken` (native code 34). A deferred - * (BIP70/BIP270) [broadcastSigned][org.dashfoundation.dashsdk.wallet.ManagedPlatformWallet.broadcastSigned] - * token has outlived its funding reservation's lifetime: key-wallet's - * TTL may already have swept and re-selected the inputs, so acting on it - * could touch a newer, unrelated reservation. The call did NOT touch the - * network. NOT retryable in place — rebuild the payment with - * [buildSignedPayment][org.dashfoundation.dashsdk.wallet.ManagedPlatformWallet.buildSignedPayment]. + * `ErrorStaleReservationToken` (native code 34). A payment's funding + * reservation has outlived its lifetime: key-wallet's TTL may already + * have swept and re-selected the inputs, so sending it could spend + * against a newer, unrelated reservation. The call did NOT touch the + * network, and it released the still-owned reservation on the way out + * (owner-guarded — a no-op if ownership had already transferred). NOT + * retryable in place — rebuild the payment, which can reselect the + * freed inputs immediately. + * + * The code is shared by BOTH deferred-payment surfaces (the messages + * distinguish them): a deferred (BIP70/BIP270) + * [broadcastSigned][org.dashfoundation.dashsdk.wallet.ManagedPlatformWallet.broadcastSigned] + * token, rebuilt with + * [buildSignedPayment][org.dashfoundation.dashsdk.wallet.ManagedPlatformWallet.buildSignedPayment]; + * and a finalized handle whose + * [broadcastTransaction][org.dashfoundation.dashsdk.wallet.ManagedCoreWallet.broadcastTransaction] + * aged past the same reservation bound (abandon still works at any age). * * Sibling of the other two deferred-token failures this code used to * conflate: [ReservationTokenConsumed] (unknown / already broadcast / diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt index 37aca33c542..259e7b864f3 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt @@ -48,14 +48,46 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable { ) } - /** Consume and broadcast a finalized transaction. */ - fun broadcastTransaction(tx: FinalizedCoreTransaction): String = + /** + * Consume and broadcast a finalized transaction. A handle held past the + * reservation age bound throws the typed + * [StaleReservationToken][org.dashfoundation.dashsdk.errors.DashSdkError.PlatformWallet.StaleReservationToken] + * (native code 34, shared with the deferred-token surface) instead of + * broadcasting against inputs key-wallet's TTL may have re-selected. + * + * On that refusal the handle has **already been consumed** by this call and + * its funding reservation released owner-guarded (freed only while this + * build still owned it; a no-op once a TTL sweep or re-reservation + * transferred ownership). This call consumes the Kotlin-side handle up + * front (on EVERY outcome, success included), so a follow-up + * [abandonTransaction] fails locally with [IllegalStateException] because + * [FinalizedCoreTransaction] has already been consumed; it never re-enters + * native code and is not a recovery path — there is nothing left to + * release. Recover by rebuilding the transaction, which can reselect the + * freed inputs immediately. + */ + fun broadcastTransaction(tx: FinalizedCoreTransaction): String = mapNativeErrors { WalletManagerNative.coreWalletBroadcastSignedTransaction( handle, tx.takeForBroadcast(), ) + } - /** Consume without sending and release the selected inputs immediately. */ + /** + * Consume a finalized transaction without sending. With the build's owner + * token present (the normal funded-finalize case) the release is + * owner-guarded and safe at any age: it frees the selected inputs while + * this build still owns them — so a rebuild can reselect them immediately — + * and no-ops once key-wallet's TTL sweep or a re-reservation transferred + * ownership. Only a token-less handle honours the reservation age bound and + * skips its unguarded by-outpoint release past it (releasing by outpoint + * could free a newer build's reservation), leaving the aged reservation for + * the TTL to reclaim. The handle is torn down either way. + * + * Consumes the Kotlin-side handle: calling this (or [broadcastTransaction]) + * on an already-consumed [FinalizedCoreTransaction] fails locally with + * [IllegalStateException] before any native code runs. + */ fun abandonTransaction(tx: FinalizedCoreTransaction) { WalletManagerNative.coreWalletAbandonSignedTransaction( handle, diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs index 8eacdf4f355..d1f1edc79cf 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs @@ -31,6 +31,28 @@ fn classify_broadcast_result( /// (removed, or re-created under the same id) is refused with `NotFound` (98) /// **before** the network is touched; the handle is consumed and its reservation /// reconciled. This mirrors the deferred-token path's `WalletRemoved` → 98. +/// +/// # `ErrorStaleReservationToken` (34) is TERMINAL +/// +/// A handle held past `RESERVATION_MAX_AGE_BLOCKS` — the wallet's +/// `last_processed_height` advanced that far beyond the funding reservation's +/// stamp — is refused with `ErrorStaleReservationToken` (34) and no txid, again +/// **before** the network is touched. Nothing was sent. +/// +/// There is no retry and no abandon from that outcome: the handle was already +/// consumed at the top of this call, so a second +/// `core_wallet_broadcast_signed_transaction` with it returns `NotFound` (98) +/// rather than resending, and `core_wallet_abandon_signed_transaction` likewise +/// finds nothing to free. The refusal path performs the reconciliation itself — +/// it releases the funding reservation owner-guarded, so the inputs are free +/// while this build still owned them and untouched once a TTL sweep or +/// re-reservation transferred ownership. +/// +/// **The caller must REBUILD the transaction.** That is the whole recovery: the +/// released inputs are immediately reselectable by a fresh +/// `core_wallet_tx_builder_*` → `finalize` sequence, and no cleanup call is +/// needed (or possible) in between. See +/// `aged_broadcast_refuses_and_releases_for_rebuild`. #[no_mangle] pub unsafe extern "C" fn core_wallet_broadcast_signed_transaction( handle: Handle, @@ -288,6 +310,26 @@ mod tests { runtime().block_on(core.abandon_transaction(&retry)); } + /// Prove the funding reservation was released owner-guarded: a fresh + /// finalize of the same size reselects the single fixture UTXO. An aged + /// abandon/free with the build's owner token present releases via + /// `release_reservation_if_owner` (safe at any age — no-op once ownership + /// transferred), so the input must be immediately reselectable. + fn assert_released_for_rebuild(core: &TestCore, signer: &WalletSigner, tag: u8) { + let rebuild = runtime().block_on(core.finalize_transaction( + TransactionBuilder::new().add_output( + &Address::dummy(Network::Testnet, usize::from(tag)), + 1_000_000, + ), + &[AccountTypePreference::BIP44], + 0, + signer, + )); + let rebuilt = rebuild + .expect("aged abandon/free must release the still-owned reservation for a rebuild"); + runtime().block_on(core.abandon_transaction(&rebuilt)); + } + #[test] fn double_free_is_safe_and_releases_reservation() { let (core, signer) = @@ -327,6 +369,98 @@ mod tests { CORE_WALLET_STORAGE.remove(other_handle); } + /// The deinit/GC backstop (`core_wallet_signed_transaction_free`) is the + /// exact path shumkov flagged: a `FinalizedCoreTransaction` never broadcast + /// or abandoned, freed by the host GC long after finalize. The funded + /// finalize stamped an owner token, so the aged free still releases — + /// owner-guarded via `release_reservation_if_owner`, which is safe at any + /// age (it no-ops once key-wallet's TTL swept and an unrelated build + /// re-reserved the outpoint) — freeing the still-owned input for a rebuild. + /// The handle is torn down (the storage entry is removed) so a re-free is a + /// safe no-op. + #[test] + fn aged_free_releases_owner_guarded() { + let (core, signer) = + runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); + let transaction_handle = insert(&core, finalize(&core, &signer, 48)); + + // Age the pinned handle past the guard bound (still below the TTL, so the + // reservation is provably still held — only the software guard trips). + runtime().block_on(platform_wallet::test_support::age_core_past_reservation_guard(&core)); + + core_wallet_signed_transaction_free(transaction_handle); + + // The aged free released owner-guarded: the input is reselectable. + assert_released_for_rebuild(&core, &signer, 49); + // Handle is gone regardless — a re-free is a harmless no-op. + core_wallet_signed_transaction_free(transaction_handle); + } + + /// The FFI broadcast/abandon *failure* paths (invalid or wrong-generation + /// wallet handle) route their cleanup through `abandon_transaction`, so they + /// inherit the same policy: an aged handle with the build's owner token + /// still releases owner-guarded (safe at any age), so the failure-path + /// cleanup frees the still-owned input instead of stranding it. + #[test] + fn aged_failure_path_abandon_releases_owner_guarded() { + let (origin, signer) = + runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); + let transaction_handle = insert(&origin, finalize(&origin, &signer, 50)); + + runtime().block_on(platform_wallet::test_support::age_core_past_reservation_guard(&origin)); + + // Invalid wallet handle → routes through abandon_transaction, then returns + // ErrorInvalidHandle. The embedded aged reservation is released + // owner-guarded on the way out. + let invalid = + unsafe { core_wallet_abandon_signed_transaction(u64::MAX, transaction_handle) }; + assert_eq!( + invalid.code, + PlatformWalletFFIResultCode::ErrorInvalidHandle + ); + assert_released_for_rebuild(&origin, &signer, 51); + } + + /// The terminal FFI stale-broadcast behavior: by the time the age guard + /// runs, `core_wallet_broadcast_signed_transaction` has already consumed + /// the opaque handle (and the host bindings cleared theirs before entering + /// the ABI), so no follow-up abandon is possible. The refusal must + /// therefore reconcile the reservation itself — owner-guarded, freeing the + /// still-owned input so the instructed immediate rebuild can reselect it — + /// and surface the shared `ErrorStaleReservationToken` (34) code with no + /// txid. A retry of the consumed handle is `NotFound`, not a resend. + #[test] + fn aged_broadcast_refuses_and_releases_for_rebuild() { + let (core, signer) = + runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); + let core_handle = CORE_WALLET_STORAGE.insert(core.clone()); + let transaction_handle = insert(&core, finalize(&core, &signer, 52)); + + runtime().block_on(platform_wallet::test_support::age_core_past_reservation_guard(&core)); + + let mut txid = ptr::null_mut(); + let stale = unsafe { + core_wallet_broadcast_signed_transaction(core_handle, transaction_handle, &mut txid) + }; + assert_eq!( + stale.code, + PlatformWalletFFIResultCode::ErrorStaleReservationToken + ); + assert!(txid.is_null()); + + // The refusal released owner-guarded: the input is reselectable with no + // further cleanup call. + assert_released_for_rebuild(&core, &signer, 53); + + // The handle was consumed by the refused broadcast — a retry cannot + // reconsume it. + let retry = unsafe { + core_wallet_broadcast_signed_transaction(core_handle, transaction_handle, &mut txid) + }; + assert_eq!(retry.code, PlatformWalletFFIResultCode::NotFound); + CORE_WALLET_STORAGE.remove(core_handle); + } + #[test] fn abandon_then_free_or_broadcast_cannot_reconsume_handle() { let (core, signer) = diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index e146feb3406..a45552e3166 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -305,6 +305,23 @@ pub enum PlatformWalletFFIResultCode { /// [`Self::ErrorReservationWalletMismatch`] (36, minted against a different /// wallet generation). All three are non-retryable-in-place and none touched /// the network; they are distinct codes so a host can message each precisely. + /// + /// Also maps `PlatformWalletError::StaleReservation` from the atomic + /// finalized-transaction handle path + /// (`core_wallet_broadcast_signed_transaction`): a pinned handle whose + /// funding reservation aged past the SAME `RESERVATION_MAX_AGE_BLOCKS` bound + /// carries the identical "may already have been swept — rebuild" meaning, so + /// the two surfaces intentionally share this one code. The handle carries + /// no numeric reservation token, hence a distinct (token-less) wallet-error + /// variant behind the same FFI code. The refusal reconciles the reservation + /// on the way out: a funded finalize always stamps an owner token, so the + /// release is owner-guarded (safe at any age — a no-op once ownership + /// transferred) and the still-owned inputs are freed for the instructed + /// rebuild. Abandon/free of a handle never surfaces this — abandon returns + /// no result code and likewise releases owner-guarded at any age; only a + /// token-less build skips its unguarded by-outpoint release past the bound + /// (leaving the aged outpoint to key-wallet's TTL, since releasing it + /// unguarded could free an unrelated newer build's reservation). ErrorStaleReservationToken = 34, /// Maps `SignedPaymentError::StaleToken`. The deferred reservation token is @@ -643,6 +660,33 @@ impl From for PlatformWalletFFIResult { PlatformWalletError::TransactionBroadcast(..) => { PlatformWalletFFIResultCode::ErrorTransactionBroadcastRejected } + // The finalized-transaction handle path's age guard. Shares the + // `ErrorStaleReservationToken` code with the deferred registry-token + // sibling (`SignedPaymentError::StaleReservationToken`): both mean + // "the funding reservation may already have been swept — rebuild", + // and neither touched the network. See the code's doc note. + PlatformWalletError::StaleReservation => { + PlatformWalletFFIResultCode::ErrorStaleReservationToken + } + // A coin selection that picked an input still held by an in-flight + // broadcast dispatch. Typed on the Rust side (it carries the + // conflicting `OutPoint`, and is the one build refusal that is + // safely retryable unchanged), but DELIBERATELY mapped to the same + // numeric code it produced before that variant existed: all three + // choke points previously returned it as + // `TransactionBuild` / `AssetLockTransaction`, neither of which is + // matched here, so both fell to `ErrorUnknown`. + // + // Minting a dedicated code is a separate, coordinated change — the + // numeric space is a cross-PR registry (see the claim table on + // `ErrorStaleReservationToken` above) and every new value has to be + // mirrored into the Swift and Kotlin result enums. This arm exists + // so the mapping is an explicit, reviewable decision in one place + // rather than an accident of the catch-all, and so it is a one-line + // change when a code is claimed (`dashpay/platform#4309`). + PlatformWalletError::InputMidBroadcast { .. } => { + PlatformWalletFFIResultCode::ErrorUnknown + } // A definitively-failed address-nonce race (reaches the blanket impl // via identity `top_up_from_addresses` → `?`/`.into()`). Exposing // provided/expected nonce as structured out-fields is INTENTIONALLY @@ -1353,6 +1397,34 @@ mod tests { assert_eq!(msg, rendered, "Display payload must survive verbatim"); } + /// The finalized-transaction handle age guard + /// (`core_wallet_broadcast_signed_transaction` → `broadcast_finalized_transaction`) + /// surfaces `PlatformWalletError::StaleReservation` through the blanket + /// `From` impl, which must reuse the deferred registry-token path's + /// `ErrorStaleReservationToken` (34) code rather than flattening to + /// `ErrorUnknown` — the two surfaces share the "reservation may have been + /// swept; rebuild" meaning and this one code. The typed Display rendering + /// survives across the boundary as the message. + #[test] + fn stale_reservation_maps_to_shared_stale_reservation_code() { + let err = PlatformWalletError::StaleReservation; + let rendered = err.to_string(); + let result: PlatformWalletFFIResult = err.into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorStaleReservationToken, + "StaleReservation must reuse the registry-token stale code (rendered: {rendered})" + ); + assert!(!result.message.is_null()); + let msg = unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_string_lossy() + .into_owned(); + assert_eq!( + msg, rendered, + "Display payload must survive the FFI boundary verbatim" + ); + } + /// `AddressNonceMismatch` maps to the dedicated `ErrorAddressNonceMismatch` /// FFI code through the blanket `From` impl (the path identity /// `top_up_from_addresses` takes via `?`/`.into()`) rather than flattening diff --git a/packages/rs-platform-wallet/src/changeset/core_bridge.rs b/packages/rs-platform-wallet/src/changeset/core_bridge.rs index 2ce63111cc5..305d5bf5fc7 100644 --- a/packages/rs-platform-wallet/src/changeset/core_bridge.rs +++ b/packages/rs-platform-wallet/src/changeset/core_bridge.rs @@ -1275,9 +1275,9 @@ fn derive_spent_utxos(record: &TransactionRecord) -> Vec { .input_details .iter() .filter_map(|detail| { - let input = record.transaction.input.get(detail.index as usize)?; + let outpoint = spent_outpoint(record, detail)?; Some(Utxo { - outpoint: input.previous_output, + outpoint, txout: TxOut { value: detail.value, script_pubkey: detail.address.script_pubkey(), @@ -1294,6 +1294,41 @@ fn derive_spent_utxos(record: &TransactionRecord) -> Vec { .collect() } +/// The outpoint one [`InputDetail`] says this record spent, or `None` when the +/// detail's index does not address a real input. +/// +/// The single definition of "this record spent one of ours", shared by +/// [`derive_spent_utxos`] above — which turns it into the persister's +/// [`CoreChangeSet::spent_utxos`] removals — and by +/// [`spent_outpoints`], which drives the in-broadcast fence's release. The two +/// consumers must not be able to disagree about which inputs count: the fence +/// releases an outpoint precisely when the wallet treats it as spent, so a +/// divergence would either strand a fence forever or drop one early +/// (`dashpay/platform#4309`). +/// +/// [`InputDetail`]: key_wallet::managed_account::transaction_record::InputDetail +fn spent_outpoint( + record: &TransactionRecord, + detail: &key_wallet::managed_account::transaction_record::InputDetail, +) -> Option { + record + .transaction + .input + .get(detail.index as usize) + .map(|input| input.previous_output) +} + +/// Every outpoint of ours that `record` spends. +/// +/// The fence-side view of [`derive_spent_utxos`], built on the same +/// [`spent_outpoint`] walk — see that function for why they share it. +pub(crate) fn spent_outpoints(record: &TransactionRecord) -> impl Iterator + '_ { + record + .input_details + .iter() + .filter_map(|detail| spent_outpoint(record, detail)) +} + impl CoreChangeSet { /// Cheap "should we bother round-tripping the persister" check used /// by the adapter to drop empty events without locking. Skips the diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index d2b50861297..c9865ee6ed2 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -105,22 +105,99 @@ pub enum PlatformWalletError { /// A core transaction broadcast failed with an **ambiguous** outcome — the /// transaction may already have reached the network (transport timeout /// after delivery, partial peer send, or an internal multi-node retry - /// whose earlier attempt may have succeeded). The spent inputs' - /// reservation is intentionally kept, so an immediate retry fails at - /// input selection instead of double-spending; the reservation-TTL - /// backstop (or a sync observing the transaction) reconciles the outcome. + /// whose earlier attempt may have succeeded). The spent inputs are + /// intentionally kept out of the selectable set, so an immediate retry + /// fails at input selection instead of double-spending. + /// + /// # What actually reconciles this, and what does not + /// + /// The inputs are held by TWO independent things, and only one of them + /// expires. Key-wallet's `ReservationSet` entry is swept once the wallet's + /// `last_processed_height` advances `RESERVATION_MAX_AGE_BLOCKS` past the + /// height it was stamped at. The generation's pending-spend fence + /// ([`WalletGeneration`](crate::wallet::core::WalletGeneration)) is NOT + /// swept with it and has no bound of its own: it is released by the wallet + /// OBSERVING the outpoint spent, and by nothing else + /// (`dashpay/platform#4309`). + /// + /// So an earlier promise made here — that the reservation TTL reconciles an + /// ambiguous outcome — no longer holds and was never sound: elapsed time is + /// not evidence about the transaction, which stays valid and relayable no + /// matter how long the wait. The build refusal that follows a `MaybeSent` + /// is [`Self::InputMidBroadcast`], and it stands until a spend is observed + /// (this wallet's own transaction landing, or a conflicting one taking the + /// outpoint) or the generation is torn down. /// /// The shielded sibling is [`Self::ShieldedSpendUnconfirmed`]. #[error( "Transaction broadcast outcome unknown — it may already be on the \ - network; its inputs stay reserved until a sync or the reservation \ - TTL reconciles the outcome: {0}" + network; its inputs stay unspendable until this wallet observes them \ + spent: {0}" )] TransactionBroadcastUnconfirmed(String), + /// A finalized transaction handle + /// (`core_wallet_tx_builder_finalize` → `broadcast_finalized_transaction`) + /// was held long enough that its funding reservation may already have been + /// swept and re-selected by key-wallet's TTL: the wallet's + /// `last_processed_height` advanced at least + /// `RESERVATION_MAX_AGE_BLOCKS` + /// blocks past the height the reservation was stamped at + /// ([`SignedCoreTransaction::reservation_height`](crate::SignedCoreTransaction::reservation_height)). + /// Broadcasting it could spend against a newer, unrelated reservation, so it + /// is refused **before** touching the network — NOT retryable in place, the + /// caller must rebuild the payment. The refusal reconciles the reservation + /// on the way out: a funded finalize always stamps an owner token, so the + /// release is owner-guarded (`release_reservation_if_owner`, safe at any + /// age — it no-ops once ownership transferred) and the still-owned inputs + /// are freed for the instructed rebuild. Abandoning/freeing the handle + /// likewise releases owner-guarded at any age; only a token-less build + /// skips its unguarded by-outpoint release past the bound and leaves the + /// aged outpoint for key-wallet's TTL to reclaim. + /// + /// This is the handle-path sibling of the deferred registry-token + /// [`SignedPaymentError::StaleReservationToken`](crate::SignedPaymentError::StaleReservationToken); + /// both share the same age bound and the FFI `ErrorStaleReservationToken` + /// code. Carries no token — the handle path is keyed by an opaque handle, + /// not a numeric reservation token. + #[error("finalized transaction reservation has outlived its lifetime; rebuild the payment")] + StaleReservation, + #[error("Transaction building failed: {0}")] TransactionBuild(String), + /// Coin selection picked an outpoint that a broadcast dispatch is still + /// holding — the transaction spending it is in flight, or has reached the + /// network and has not yet been observed spent by this wallet + /// ([`WalletGeneration::in_broadcast_conflict`](crate::wallet::core::WalletGeneration::in_broadcast_conflict)). + /// Completing the build would race that transaction on the wire, so it is + /// refused and its own fresh reservation released. NOTHING was built, + /// signed or broadcast. + /// + /// A TRANSIENT, EXPECTED condition, and the reason it is a variant of its + /// own rather than a [`Self::TransactionBuild`] / + /// [`Self::AssetLockTransaction`] string: it is the one build failure a + /// caller may safely retry UNCHANGED once the in-flight dispatch settles, + /// and telling it apart from a genuine build failure previously meant + /// substring-matching prose (`message.contains("mid-broadcast")`, which the + /// tests did too). All three selection choke points — the + /// finalized-transaction build, the contact-payment build and the + /// asset-lock build — now return this one variant. + /// + /// `outpoint` is the first conflicting input, carried structurally so + /// callers and diagnostics need not parse it back out of a message. + /// + /// Reaching a caller at all is the uncommon path: a fenced input is + /// normally still reserved and never offered to selection. This fires only + /// in the window after key-wallet's reservation TTL swept that dispatch's + /// reservation, which is exactly what the fence exists to cover + /// (`dashpay/platform#4309`). + #[error( + "selected input {outpoint} is mid-broadcast by an in-flight dispatch; \ + retry after it completes" + )] + InputMidBroadcast { outpoint: dashcore::OutPoint }, + /// The address handed to [`CoreWallet::sign_message`] cannot be a signing /// target at all: unparseable, encoded for a different network than the /// wallet's, or not P2PKH. A caller-input error — the classic Dash diff --git a/packages/rs-platform-wallet/src/manager/load.rs b/packages/rs-platform-wallet/src/manager/load.rs index ce44a55d0e7..674ddfc118c 100644 --- a/packages/rs-platform-wallet/src/manager/load.rs +++ b/packages/rs-platform-wallet/src/manager/load.rs @@ -82,7 +82,24 @@ impl PlatformWalletManager

{ tracked_asset_locks.extend(account_locks); } - let generation = Arc::new(WalletGeneration::new()); + // Canonical id recomputed from the wallet's own key material. + // Computed up front — before `insert_wallet` consumes `wallet` — + // so we can both validate it against the persisted map key + // (below) and key this generation's in-broadcast fence map by it. + let wallet_id = wallet.compute_wallet_id(); + + // The fence map is per WALLET, not per generation + // (`dashpay/platform#4309`, review round 8). On a first load the + // registry is empty and this is a fresh map; a re-load — or a load + // that follows a removal — inherits whatever pending spends the + // previous generation under this id left standing, rather than + // handing the restored UTXOs back unprotected. + // + // A fresh PROCESS still starts empty: this registry is not durable. + // See `InBroadcastFences` for what closing that half requires. + let generation = Arc::new(WalletGeneration::with_fences( + self.in_broadcast_fences_for(&wallet_id), + )); // Mirror the inner `ManagedWalletInfo.balance` (already // recomputed from the freshly-loaded UTXO set on the FFI // side via `update_balance`) into the lock-free `Arc` the @@ -107,12 +124,6 @@ impl PlatformWalletManager

{ dpns_name_states: std::collections::BTreeMap::new(), }; - // Canonical id recomputed from the wallet's own key material. - // Computed up front — before `insert_wallet` consumes `wallet` — - // so we can both validate it against the persisted map key and - // detect a wallet that an earlier load already registered. - let wallet_id = wallet.compute_wallet_id(); - if wallet_id != expected_wallet_id { load_error = Some(PlatformWalletError::WalletCreation(format!( "Persisted wallet id {} does not match recomputed id {}", diff --git a/packages/rs-platform-wallet/src/manager/mod.rs b/packages/rs-platform-wallet/src/manager/mod.rs index d697bd15a54..9192dfae148 100644 --- a/packages/rs-platform-wallet/src/manager/mod.rs +++ b/packages/rs-platform-wallet/src/manager/mod.rs @@ -31,7 +31,7 @@ use crate::manager::platform_address_sync::PlatformAddressSyncManager; use crate::manager::shielded_sync::ShieldedSyncManager; use crate::spv::SpvRuntime; use crate::wallet::asset_lock::LockNotifyHandler; -use crate::wallet::core::BalanceUpdateHandler; +use crate::wallet::core::{BalanceUpdateHandler, SpendObservationHandler}; use crate::wallet::identity::network::DashPayPaymentHandler; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use crate::wallet::PlatformWallet; @@ -386,9 +386,11 @@ pub struct PlatformWalletManager { /// onto the freshly-created `NetworkShieldedCoordinator` that /// forwards into `on_shielded_sync_progress`. Sub-managers /// (`SpvRuntime`, `PlatformAddressSyncManager`, etc.) hold their - /// own clones already, so `configure_shielded` is the only reader of - /// this retained handle — hence it is `shielded`-gated. - #[cfg(feature = "shielded")] + /// own clones already, so `configure_shielded` is the only + /// production reader of this retained handle — hence it is gated to + /// `shielded`, plus `test` so the handler-wiring test can dispatch + /// an event through the manager's own fan-out. + #[cfg(any(test, feature = "shielded"))] pub(super) event_manager: Arc, pub(super) persister: Arc

, /// Tracked (wallet-independent) masternodes for this manager's @@ -418,6 +420,31 @@ pub struct PlatformWalletManager { /// failed / rescan pending" state rather than re-freezing silently on /// the next launch. pub(super) sync_fault: Arc, + /// Per-WALLET in-broadcast fence maps, handed to every + /// [`WalletGeneration`](crate::wallet::core::WalletGeneration) registered + /// under each id (`dashpay/platform#4309`, review round 8). + /// + /// A fence describes a signed transaction that may be live on the network. + /// That fact outlives the wallet *instance* that dispatched it: removing a + /// wallet and re-creating it under the same id used to mint a generation + /// with an empty map, so the re-created wallet restored the persisted UTXO + /// with nothing holding it — not the fence, not key-wallet's memory-only + /// reservation — and could sign a conflicting spend of an outpoint the + /// original transaction still spends. Keying the map here instead makes the + /// replacement inherit it. + /// + /// **Deliberately never pruned.** A removed wallet's entry stays, because a + /// removal is exactly when the protection must survive; dropping it on + /// removal would restore the bug for the recreate-after-remove path this + /// exists to close. Growth is bounded by the number of distinct wallet ids + /// this process has registered, and each entry reaps its own cleared rows + /// on read. + /// + /// A `std::sync::Mutex`: touched only at wallet registration and load, for + /// one map lookup, and never held across an await. + pub(super) in_broadcast_fences: std::sync::Mutex< + std::collections::BTreeMap>, + >, } impl PlatformWalletManager

{ @@ -474,6 +501,14 @@ impl PlatformWalletManager

{ // with SPV's write lock. let lock_handler = Arc::new(LockNotifyHandler::new(Arc::clone(&lock_notify))); let balance_handler = Arc::new(BalanceUpdateHandler::new(Arc::clone(&wallets))); + // SpendObservationHandler releases in-broadcast input fences when the + // wallet observes the fenced outpoints spent — the evidence that ends + // the fence a dispatch installs (`dashpay/platform#4309`). It takes the + // same `wallets` map, and for the same lock reason as the balance + // handler: the event fires inside SPV's block-processing write section, + // so the generation cannot be resolved through the wallet-manager lock. + let spend_observation_handler = + Arc::new(SpendObservationHandler::new(Arc::clone(&wallets))); // DashPayPaymentHandler records incoming DashPay payments and // confirms sent ones off the wallet-event fan-out, keeping that // domain logic out of the generic core-changeset bridge. It holds @@ -487,6 +522,7 @@ impl PlatformWalletManager

{ app_handler, lock_handler, balance_handler, + spend_observation_handler, Arc::clone(&dashpay_payment_handler) as Arc, ])); @@ -542,7 +578,7 @@ impl PlatformWalletManager

{ shielded_sync_manager: shielded_sync, #[cfg(feature = "shielded")] shielded_coordinator, - #[cfg(feature = "shielded")] + #[cfg(any(test, feature = "shielded"))] event_manager, persister, tracked_masternodes: std::sync::Arc::new(Default::default()), @@ -550,9 +586,30 @@ impl PlatformWalletManager

{ event_adapter_join: tokio::sync::Mutex::new(Some(event_adapter_join)), registry, sync_fault, + in_broadcast_fences: std::sync::Mutex::new(std::collections::BTreeMap::new()), } } + /// The in-broadcast fence map for `wallet_id`, creating it on first use. + /// + /// Every [`WalletGeneration`](crate::wallet::core::WalletGeneration) this + /// manager mints for a wallet is built from this, so a generation that + /// replaces another under the same id inherits its pending-spend fences — + /// see the [`in_broadcast_fences`](Self#structfield.in_broadcast_fences) + /// field docs (`dashpay/platform#4309`). + pub(super) fn in_broadcast_fences_for( + &self, + wallet_id: &WalletId, + ) -> Arc { + Arc::clone( + self.in_broadcast_fences + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .entry(*wallet_id) + .or_default(), + ) + } + /// Whether the wallet-event adapter has frozen a durable sync /// watermark this manager's lifetime (dashpay/platform#4069). /// @@ -1022,6 +1079,77 @@ mod tests { )) } + /// The constructor must register [`SpendObservationHandler`] on the event + /// fan-out, over the LIVE wallets map (`dashpay/platform#4309`, review + /// round 6): a spend-bearing wallet event dispatched through the manager's + /// own `event_manager` must release a registered wallet's in-broadcast + /// fence. Dropping the handler from the constructor's handler list — the + /// accidental-omission regression this pins — fails the final assertion, + /// because nothing else on the fan-out calls `observe_spent`. + #[tokio::test] + async fn constructor_wires_spend_observation_into_the_event_fanout() { + use dashcore::hashes::Hash as _; + + let mgr = make_manager(); + + // A funded wallet registered in the manager's live wallets map — the + // same map the constructor handed to its handlers. + let (wallet_manager, wallet_id, generation, _signer) = + crate::test_support::funded_wallet_manager( + key_wallet::account::account_type::StandardAccountType::BIP44Account, + ) + .await; + let spv = Arc::new(SpvRuntime::new( + Arc::clone(&wallet_manager), + Arc::new(PlatformEventManager::new(Vec::new())), + )); + let wallet = Arc::new(PlatformWallet::new( + Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")), + wallet_id, + wallet_manager, + Arc::clone(&generation), + Arc::new(Notify::new()), + Arc::new(NoopPersister) as Arc, + Arc::new(crate::broadcaster::SpvBroadcaster::new(spv)), + )); + mgr.wallets.write().await.insert(wallet_id, wallet); + + // Fence an outpoint the way a dispatch does: pin, then settle into the + // pending-spend phase that only an observed spend may end. + let tx = dashcore::Transaction { + version: 2, + lock_time: 0, + input: vec![dashcore::TxIn { + previous_output: dashcore::OutPoint { + txid: dashcore::Txid::from_slice(&[9u8; 32]).expect("txid"), + vout: 0, + }, + script_sig: dashcore::ScriptBuf::new(), + sequence: 0xffff_ffff, + witness: dashcore::Witness::new(), + }], + output: Vec::new(), + special_transaction_payload: None, + }; + generation.pin_in_broadcast(&tx).settle_pending_spend(); + assert!( + generation.in_broadcast_conflict(&tx).is_some(), + "the settled pin must leave the pending-spend fence up" + ); + + // The spend event, dispatched through the manager's OWN fan-out — not + // a hand-built handler — so the assertion covers registration itself. + mgr.event_manager + .on_wallet_event(&crate::test_support::observed_spend_event(wallet_id, &tx)); + + assert!( + generation.in_broadcast_conflict(&tx).is_none(), + "a spend event through the manager's event fan-out must release \ + the registered wallet's fence — is SpendObservationHandler still \ + in the constructor's handler list?" + ); + } + /// `shutdown()` joins every started coordinator through the shared /// [`ThreadRegistry`], reports each as cleanly joined, and is /// idempotent — a second call finds nothing running and still reports diff --git a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs index c9eafee286b..063e10939bb 100644 --- a/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs +++ b/packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs @@ -210,7 +210,21 @@ impl PlatformWalletManager

{ // place below, BEFORE the address-pool snapshot is taken. let mut wallet_info = ManagedWalletInfo::from_wallet(&wallet, birth_height); - let generation = Arc::new(WalletGeneration::new()); + // The id this wallet is about to occupy. Computed HERE — before + // `downgrade_to_external_signable` below flips the wallet type, and + // before `wallet` is moved into `insert_wallet` — because the + // generation must be born already holding this wallet's fence map. + // + // A registration under an id a previous generation held is exactly the + // remove-and-recreate case: that generation's pending-spend fences + // protect signed transactions that are still valid and still relayable, + // so the replacement inherits them rather than starting clean + // (`dashpay/platform#4309`, review round 8). A first registration finds + // no entry and gets an empty map, as before. + let registration_wallet_id = wallet.compute_wallet_id(); + let generation = Arc::new(WalletGeneration::with_fences( + self.in_broadcast_fences_for(®istration_wallet_id), + )); // Snapshot per-account xpubs and address-pool entries BEFORE // the wallet / managed-info are moved into insert_wallet. The @@ -388,6 +402,31 @@ impl PlatformWalletManager

{ })? }; + // `insert_wallet` recomputes the id from the (now external-signable) + // wallet. The two agree by construction — the downgrade flips only the + // wallet TYPE, and the type's own branch returns the same stamped + // network-scoped digest — but the fence map is safety state, so a + // divergence must not silently key it under an id nothing will look up. + // Alias the same `Arc` under the authoritative id instead: one map, + // reachable either way. + debug_assert_eq!( + wallet_id, registration_wallet_id, + "the registered wallet id must match the id the fence map was keyed by" + ); + if wallet_id != registration_wallet_id { + tracing::error!( + registered = %hex::encode(wallet_id), + fenced_as = %hex::encode(registration_wallet_id), + "wallet id changed across registration; aliasing the in-broadcast \ + fence map under both" + ); + let fences = self.in_broadcast_fences_for(®istration_wallet_id); + self.in_broadcast_fences + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(wallet_id, fences); + } + // Emit metadata + per-account xpubs + per-pool address // snapshots to the persister so the watch-only restore path // has everything it needs on next launch. The whole @@ -1075,6 +1114,164 @@ mod register_wallet_duplicate_tests { "duplicate create must map to WalletAlreadyExists, got: {err:?}" ); } + + /// `dashpay/platform#4309`, REVIEW ROUND 8 — PENDING-SPEND PROTECTION MUST + /// SURVIVE WALLET RECREATION. + /// + /// The in-broadcast fence used to live in the `WalletGeneration` itself, so + /// it was not merely process-local but *generation*-local. Removing a wallet + /// and re-creating it under the same id mints a fresh generation, and the + /// fence map went with the old one — while the signed transaction it was + /// protecting stays perfectly valid and can still be relayed by a DAPI + /// endpoint or a peer that retained it. The re-created wallet restored the + /// persisted UTXO with neither the fence nor key-wallet's memory-only + /// reservation holding it, and could sign a conflicting spend of the very + /// same outpoint. + /// + /// Fences are therefore keyed by WALLET, not by generation: a generation + /// that replaces another under the same id inherits its predecessor's + /// pending-spend fences, and they are retired by the same evidence as ever — + /// an observed spend — not by the replacement. + /// + /// Red before the fix: the re-created wallet reported no conflict at all. + #[tokio::test] + async fn a_recreated_wallet_inherits_the_pending_fences_of_the_generation_it_replaces() { + use dashcore::hashes::Hash; + use dashcore::{OutPoint, ScriptBuf, Transaction, TxIn, Txid, Witness}; + + let manager = make_manager(); + let network = Network::Testnet; + let seed_bytes = Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid test mnemonic") + .to_seed(""); + + let wallet = manager + .create_wallet_from_seed_bytes( + network, + &seed_bytes, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("first create should succeed"); + let wallet_id = wallet.wallet_id(); + + // One dispatch that reached the network and has NOT been observed + // spent — the state that must outlive the generation holding it. + let funded = OutPoint { + txid: Txid::from_slice(&[0x7E; 32]).expect("valid txid"), + vout: 0, + }; + let tx = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: funded, + script_sig: ScriptBuf::new(), + sequence: 0xffff_ffff, + witness: Witness::new(), + }], + output: Vec::new(), + special_transaction_payload: None, + }; + wallet + .generation() + .pin_in_broadcast(&tx) + .settle_pending_spend(); + assert_eq!( + wallet.generation().in_broadcast_conflict(&tx), + Some(funded), + "the dispatching wallet must fence its own pending spend" + ); + + // Remove and re-create under the same id — the host-visible + // "delete and restore this wallet" round trip. + manager.remove_wallet(&wallet_id).await.expect("remove"); + let recreated = manager + .create_wallet_from_seed_bytes( + network, + &seed_bytes, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("re-create should succeed"); + assert_eq!( + recreated.wallet_id(), + wallet_id, + "the re-created wallet must occupy the same id" + ); + + assert_eq!( + recreated.generation().in_broadcast_conflict(&tx), + Some(funded), + "a wallet re-created under the same id must inherit the pending-spend \ + fence of the generation it replaced — the signed transaction that \ + fence protects is still valid and still relayable" + ); + } + + /// The inheritance above is per WALLET, not global: one wallet's dispatch + /// must never fence another wallet's inputs. + #[tokio::test] + async fn fences_do_not_leak_between_different_wallets() { + use dashcore::hashes::Hash; + use dashcore::{OutPoint, ScriptBuf, Transaction, TxIn, Txid, Witness}; + + let manager = make_manager(); + let seed_bytes = Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid test mnemonic") + .to_seed(""); + + // Same seed, DIFFERENT networks — key-wallet stamps a network-scoped + // id, so these are two distinct wallets in one manager. + let first = manager + .create_wallet_from_seed_bytes( + Network::Testnet, + &seed_bytes, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("testnet create"); + let second = manager + .create_wallet_from_seed_bytes( + Network::Devnet, + &seed_bytes, + WalletAccountCreationOptions::Default, + Some(0), + ) + .await + .expect("devnet create"); + assert_ne!(first.wallet_id(), second.wallet_id()); + + let funded = OutPoint { + txid: Txid::from_slice(&[0x7F; 32]).expect("valid txid"), + vout: 0, + }; + let tx = Transaction { + version: 2, + lock_time: 0, + input: vec![TxIn { + previous_output: funded, + script_sig: ScriptBuf::new(), + sequence: 0xffff_ffff, + witness: Witness::new(), + }], + output: Vec::new(), + special_transaction_payload: None, + }; + first + .generation() + .pin_in_broadcast(&tx) + .settle_pending_spend(); + + assert_eq!( + second.generation().in_broadcast_conflict(&tx), + None, + "a different wallet's fence must not block this one's builds" + ); + } } /// Removal versus a same-id re-registration that lands *during* the removal diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index 31c7abdf446..cf63544023d 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -263,6 +263,64 @@ pub(crate) async fn funded_wallet_manager_with_outputs( (Arc::new(RwLock::new(wm)), wallet_id, generation, signer) } +/// The `WalletEvent` the wallet emits when it first observes `tx` spending +/// its outpoints — the real shape the spend-observation seam +/// ([`SpendObservationHandler`](crate::wallet::core::SpendObservationHandler)) +/// consumes off the event fan-out. +/// +/// `input_details` claims EVERY input as ours, which is what upstream +/// populates for inputs that spent this wallet's outpoints — and the only +/// part of the record either the in-broadcast fence or +/// `CoreChangeSet::spent_utxos` reads. +/// +/// Shared between the broadcast-fence release tests +/// (`wallet::core::broadcast`) and the manager-level fan-out wiring test +/// (`manager::tests`), so the two cannot drift onto different event shapes. +#[cfg(test)] +pub(crate) fn observed_spend_event( + wallet_id: WalletId, + tx: &Transaction, +) -> key_wallet_manager::WalletEvent { + use dashcore::Address as DashAddress; + use key_wallet::managed_account::transaction_record::{ + InputDetail, TransactionDirection, TransactionRecord, + }; + use key_wallet::transaction_checking::transaction_router::TransactionType; + + let record = TransactionRecord::new( + tx.clone(), + key_wallet::account::AccountType::Standard { + index: 0, + standard_account_type: StandardAccountType::BIP44Account, + }, + TransactionContext::InBlock(BlockInfo::new( + 1_000, + dashcore::BlockHash::from([7u8; 32]), + 1_234_567_890, + )), + TransactionType::Standard, + TransactionDirection::Outgoing, + tx.input + .iter() + .enumerate() + .map(|(index, _)| InputDetail { + index: index as u32, + value: 0, + address: DashAddress::dummy(Network::Testnet, 1), + }) + .collect(), + Vec::new(), + 0, + ); + key_wallet_manager::WalletEvent::TransactionDetected { + wallet_id, + record: Box::new(record), + balance: key_wallet::WalletCoreBalance::default(), + account_balances: std::collections::BTreeMap::new(), + addresses_derived: Vec::new(), + } +} + /// Funds BOTH standard families — BIP44 account 0 and BIP32 account 0 — each /// with its own chain-locked UTXO set, for the pooled-send tests: a spend /// larger than either family's balance must draw from both. @@ -532,6 +590,38 @@ pub async fn funded_spv_core_wallet( ) } +/// Advance `core`'s `last_processed_height` to just past the reservation age +/// guard bound ([`RESERVATION_MAX_AGE_BLOCKS`](crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS)) +/// but below key-wallet's `ReservationSet` TTL, so a handle finalized at the +/// current height ages enough to trip the software guard while its underlying +/// reservation is provably still held (no key-wallet sweep yet). Returns the new +/// height. +/// +/// FFI lifecycle tests use this to exercise aged owner-guarded cleanup — the +/// deinit/GC backstop and the broadcast/abandon failure paths that route their +/// cleanup through `abandon_transaction`, which releases owner-guarded at any +/// age (only a token-less build skips its by-outpoint release). +pub async fn age_core_past_reservation_guard(core: &crate::CoreWallet) -> u32 +where + B: crate::broadcaster::TransactionBroadcaster + ?Sized, +{ + use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; + + let stamped = core + .last_processed_height() + .await + .expect("wallet present in manager"); + let target = stamped + crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS + 2; + { + let mut wm = core.wallet_manager.write().await; + let (_, info) = wm + .get_wallet_and_info_mut(&core.wallet_id()) + .expect("wallet present in manager"); + info.core_wallet.update_last_processed_height(target); + } + target +} + /// No-op persister satisfying [`PlatformWalletManager`] construction for tests /// that need a full [`PlatformWallet`] but no real persistence pipeline. pub struct NoopTestPersister; diff --git a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs index a8084c1fa21..8e0890d19af 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -128,6 +128,18 @@ impl AssetLockManager { /// reservation token, and the accounts that contributed inputs — the /// caller's release path needs every one of them, since a pooled build /// reserves in each contributing account's own set under the one token. + /// + /// # This form hands the transaction back UNSENT + /// + /// It is the build-only entry point: the caller broadcasts through some + /// other surface, or not at all. So the in-broadcast fence + /// [`build_asset_lock_transaction_fenced`](Self::build_asset_lock_transaction_fenced) + /// installs is released again before returning — there is no dispatch here + /// to keep it alive, and a fence with no settler behind it would hold these + /// inputs against every later build for the life of the process. The + /// reservation is left held exactly as before. The internal funded pipeline + /// ([`Self::broadcast_funded_asset_lock_with_funding`]) takes the fenced + /// form instead and carries the pin through to its own broadcast. #[allow(clippy::type_complexity)] pub async fn build_asset_lock_transaction_with_funding( &self, @@ -145,6 +157,64 @@ impl AssetLockManager { Vec, ), PlatformWalletError, + > { + let (transaction, path, token, accounts, pin) = self + .build_asset_lock_transaction_fenced( + amount, + funding_sources, + source_index, + funding_type, + identity_index, + signer, + ) + .await?; + pin.settle_released(); + Ok((transaction, path, token, accounts)) + } + + /// [`build_asset_lock_transaction_with_funding`](Self::build_asset_lock_transaction_with_funding) + /// that additionally returns the selection's IN-BROADCAST PIN, installed + /// atomically with the reservation while the wallet-manager write guard was + /// still held. + /// + /// The conflict check this build runs (below) stops it from consuming an + /// input another dispatch has fenced. On its own that is only half of the + /// contract: the transaction it just built carries no fence of its own, so + /// everything the caller does afterwards — the pool durability gate, the + /// tracking write, and the broadcast await itself — runs unfenced. The + /// broadcaster can suspend before submission, catch-up can advance + /// `last_processed_height` past key-wallet's 24-block reservation TTL in + /// that gap, and a competing build can then sweep and re-reserve this very + /// input, find no fence, pass its own copy of the check, and complete — + /// after which this build's already-signed asset lock still goes to the wire + /// against an input reassigned to another payment (`dashpay/platform#4309`, + /// review round 7). + /// + /// The returned pin closes that. The CALLER OWNS ITS SETTLEMENT and must + /// account for every exit: [`InBroadcastPin::settle_released`] on a + /// definitive pre-send failure (an abort before the broadcaster is reached, + /// or a definitive rejection), and + /// [`InBroadcastPin::settle_pending_spend`] — or simply dropping it — on + /// every other outcome, which leaves the pending-spend fence standing until + /// the wallet observes the spend. + #[allow(clippy::type_complexity)] + pub(crate) async fn build_asset_lock_transaction_fenced( + &self, + amount: AssetLockBuildAmount, + funding_sources: &[AccountTypePreference], + source_index: u32, + funding_type: AssetLockFundingType, + identity_index: u32, + signer: &S, + ) -> Result< + ( + Transaction, + DerivationPath, + Option, + Vec, + crate::wallet::core::InBroadcastPin, + ), + PlatformWalletError, > { let (amount_duffs, drain) = match amount { AssetLockBuildAmount::Exact(v) => (v, false), @@ -232,6 +302,41 @@ impl AssetLockManager { map_builder_error(e, required) })?; + // Refuse a selection that picked an input pinned by an IN-FLIGHT + // BROADCAST dispatch (`WalletGeneration::pin_in_broadcast`): this + // build's own selection swept that dispatch's aged reservation + // (catch-up advanced past key-wallet's TTL while it was suspended + // pre-submission) and re-reserved the input, so broadcasting this + // asset lock would race the pinned, already-signed transaction on + // the wire. Same backstop as `finalize_transaction` and the + // contact-payment build. The release runs under the write guard + // held since selection, so it is exact; the token form is + // owner-guarded like the drain-floor abandon below. The consumed + // funding key index is the same residue any discarded build leaves, + // reclaimed by the gap-limit scan. + if let Some(outpoint) = info.generation.in_broadcast_conflict(&result.transaction) { + // The pooled build reserves in EVERY contributing account's own + // set under the one owner token, so the release must sweep + // `result.funding_accounts` — the same per-account idiom as + // `release_reservation_after_rejected_broadcast`; accounts that + // supplied nothing no-op. + for funding_account in &result.funding_accounts { + if let Some(account) = info.core_wallet.accounts.funds_account(funding_account) { + match result.reservation_token { + Some(token) => { + account.release_reservation_if_owner(&result.transaction, token) + } + None => account.release_reservation(&result.transaction), + } + } + } + // Typed and shared with the other two choke points rather than an + // `AssetLockTransaction` string — the condition and the correct + // caller response are identical on all three + // (`PlatformWalletError::InputMidBroadcast`). + return Err(PlatformWalletError::InputMidBroadcast { outpoint }); + } + // 4. Pull the (pubkey, path) for our single credit output. // // `build_asset_lock_with_signer` always returns the `Public` @@ -255,11 +360,28 @@ impl AssetLockManager { } }; + // FENCE THIS SELECTION IN TURN, before the write guard drops — the + // other half of the conflict check above. Installed here rather than + // beside that check so the two credit-key error paths in between cannot + // return past a live pin: with the pending-spend phase carrying no + // deadline, a pin dropped on an abort would fence these inputs against + // every later build with no transaction to protect and nothing able to + // clear it. + // + // Nothing between the check and this line touches reservations or the + // fence map, and the wallet-manager WRITE guard has been held across + // both, so check-and-pin is still one atomic step against the TTL sweep + // and against `last_processed_height` advancement — the two mutations + // that could otherwise interleave. See the method docs for the race + // this closes. + let in_broadcast_pin = info.generation.pin_in_broadcast(&result.transaction); + Ok(( result.transaction, path, result.reservation_token, result.funding_accounts, + in_broadcast_pin, )) } @@ -825,8 +947,15 @@ impl AssetLockManager { // accounts that actually contributed inputs — a pooled build // reserves in each of their own sets under the one token, so every // release below has to reach all of them. - let (tx, path, reservation_token, funding_accounts) = self - .build_asset_lock_transaction_with_funding( + // `in_broadcast_pin` fences those inputs from the moment they were + // reserved — installed under the build's own write guard, so no + // competing build can sweep and re-reserve them across the durability + // gate and the broadcast await below (`dashpay/platform#4309`). Every + // exit from here on settles it: released on the aborts that never + // reach the broadcaster and on a definitive rejection, left pending + // otherwise. + let (tx, path, reservation_token, funding_accounts, in_broadcast_pin) = self + .build_asset_lock_transaction_fenced( amount, funding_sources, source_index, @@ -864,6 +993,17 @@ impl AssetLockManager { { if locked_amount_duffs < minimum { drop(build_persist_guard); + // Nothing reached the broadcaster, so the fence has no + // transaction to protect: release it alongside the reservation + // — but AFTER the cleanup, never before it. The cleanup awaits + // the manager read lock, and an input that is unfenced while + // still reserved-or-reusable is exactly the window review round + // 8 closed on the contact-send path + // (`dashpay/platform#4309`). This site's release is + // owner-guarded by `reservation_token`, so a newer build's + // reservation cannot be clobbered here even so; the ordering is + // uniform across every settle-with-cleanup site rather than + // resting on that one argument. crate::wallet::reservations::release_reservation_after_rejected_broadcast( &self.wallet_manager, &self.wallet_id, @@ -872,6 +1012,7 @@ impl AssetLockManager { reservation_token, ) .await; + in_broadcast_pin.settle_released(); return Err(PlatformWalletError::AssetLockTransaction(format!( "drained asset lock of {locked_amount_duffs} duffs is below the required \ minimum of {minimum} duffs (the balance cannot clear the shield pool fee); \ @@ -908,7 +1049,13 @@ impl AssetLockManager { // above: drop the serialization guard, owner-release across // every contributor, THEN surface the durability error — // otherwise an immediate retry cannot reselect the BIP44 / - // BIP32 / DashPay inputs until the TTL sweep frees them. + // BIP32 / DashPay inputs until the TTL sweep frees them. The + // fence goes with the reservation for the same reason: the + // broadcaster was never reached, so it protects nothing, and + // leaving it would block the retry the release exists to enable. + // It comes down AFTER the cleanup, not before — see the + // drain-floor branch above for why every settle-with-cleanup + // site keeps that order (`dashpay/platform#4309`, round 8). drop(build_persist_guard); crate::wallet::reservations::release_reservation_after_rejected_broadcast( &self.wallet_manager, @@ -918,6 +1065,7 @@ impl AssetLockManager { reservation_token, ) .await; + in_broadcast_pin.settle_released(); return Err(PlatformWalletError::AssetLockTransaction(format!( "aborted before broadcast: could not durably record the invitation \ funding index (broadcasting anyway would risk voucher-key reuse on \ @@ -960,7 +1108,16 @@ impl AssetLockManager { // transaction — so at no point is the row resumable while its // inputs are re-spendable. A `MaybeSent` failure keeps both the // reservation and the resumable row. - if let Err(e) = self.broadcaster.broadcast(&tx).await { + // + // The in-broadcast fence is held ACROSS this await — that is what it + // is for — and settled on the way out. It follows the reservation + // exactly: freed only where the reservation is freed (a definitive + // rejection whose `Built` row was actually removed), and otherwise + // left as a pending-spend fence until the wallet observes the spend. + // A cancellation or unwind inside `broadcast` reaches neither arm and + // settles as pending through `InBroadcastPin::drop`. + let broadcast_outcome = self.broadcaster.broadcast(&tx).await; + if let Err(e) = broadcast_outcome { if matches!(e, crate::broadcaster::BroadcastError::Rejected { .. }) { let cs_untrack = self.untrack_asset_lock(&out_point).await; // Release only when the Built row was actually removed. If @@ -973,6 +1130,12 @@ impl AssetLockManager { let removed_built_row = cs_untrack.removed.contains(&out_point); self.queue_asset_lock_changeset(cs_untrack); if removed_built_row { + // Provably nothing on the wire and the row is gone: free the + // fence with the reservation so the rebuild can reselect — + // the fence coming down LAST, after the cleanup await, so + // the input is never unfenced while still reusable + // (`dashpay/platform#4309`, round 8; see the drain-floor + // branch for the full window). crate::wallet::reservations::release_reservation_after_rejected_broadcast( &self.wallet_manager, &self.wallet_id, @@ -981,10 +1144,23 @@ impl AssetLockManager { reservation_token, ) .await; + in_broadcast_pin.settle_released(); + } else { + // The untrack guard fired: a concurrent `resume_asset_lock` + // advanced the row past `Built`, which is positive evidence + // the transaction reached the network after all. The + // reservation stays held, and so must the fence. + in_broadcast_pin.settle_pending_spend(); } + } else { + // Ambiguous `MaybeSent`: the transaction may be on the network. + in_broadcast_pin.settle_pending_spend(); } return Err(e.into()); } + // Accepted. On the DAPI broadcaster nothing was injected locally, so the + // inputs are still selectable here until the spend is observed. + in_broadcast_pin.settle_pending_spend(); // 4. Transition to Broadcast and queue the changeset. let cs_broadcast = self @@ -1255,6 +1431,161 @@ mod tests { } } + /// Broadcaster that PARKS inside `broadcast` — the production suspension + /// the in-broadcast fence exists to cover. Signals `entered` once it has + /// the transaction (manager guard already dropped, nothing submitted) and + /// waits on `release` before returning. + struct GatedBroadcaster { + entered: Arc, + release: Arc, + } + + #[async_trait] + impl TransactionBroadcaster for GatedBroadcaster { + async fn broadcast(&self, transaction: &Transaction) -> Result { + self.entered.wait().await; + self.release.wait().await; + Ok(transaction.txid()) + } + } + + /// Run ordinary historical catch-up on the fixture wallet: advance both + /// height clocks well past key-wallet's 24-block reservation TTL, so a + /// reservation stamped before the call is swept by the next selection. + async fn catch_up_past_the_reservation_ttl( + wallet_manager: &Arc>>, + wallet_id: WalletId, + height: u32, + ) { + use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; + let mut wm = wallet_manager.write().await; + let info = wm.get_wallet_info_mut(&wallet_id).expect("wallet info"); + info.core_wallet.update_last_processed_height(height); + info.core_wallet.update_synced_height(height); + } + + /// The fixture's single spendable BIP-44 outpoint. + async fn the_only_funded_outpoint( + wallet_manager: &Arc>>, + wallet_id: WalletId, + ) -> OutPoint { + let wm = wallet_manager.read().await; + let (_, info) = wm.get_wallet_and_info(&wallet_id).expect("wallet present"); + let utxos = &info + .core_wallet + .accounts + .standard_bip44_accounts + .get(&0) + .expect("BIP-44 managed account 0") + .utxos; + assert_eq!( + utxos.len(), + 1, + "the race needs exactly one selectable UTXO, so both builds must \ + contend for the same input" + ); + *utxos.keys().next().expect("one utxo") + } + + /// `dashpay/platform#4309`, REVIEW ROUND 7 — THE ASSET-LOCK BUILD'S OWN + /// FENCE. + /// + /// The build's conflict check stopped it from CONSUMING an input another + /// dispatch had fenced. It did not fence the selection it had just made, so + /// everything between the check and the direct `broadcaster.broadcast(&tx)` + /// — the pool durability gate, the `Built` tracking write, and the await + /// itself — ran with no pin on those inputs. + /// + /// 1. A funded asset lock builds, signs, releases the manager guard, and + /// SUSPENDS inside the broadcaster before submission. + /// 2. Catch-up advances the wallet far past key-wallet's 24-block + /// reservation TTL, so the parked build's reservation is swept. + /// 3. A competing asset-lock build runs. There is exactly one spendable + /// UTXO, so it selects the same input the parked lock already spends. + /// + /// Before the fix step 3 SUCCEEDED and returned a second signed asset lock + /// against that input. It must now be refused with `InputMidBroadcast`. + /// + /// The two builds run through two `AssetLockManager`s over ONE shared + /// wallet manager. That is not a workaround for the per-manager + /// build→persist serialization guard: production drops that guard before + /// the broadcast (it orders pool snapshots, nothing else), so a single + /// manager leaves exactly the same window open. Two managers just make the + /// second build's broadcaster independent of the parked one. The fence + /// lives on the shared wallet generation, which is what both see. + #[tokio::test] + async fn a_suspended_asset_lock_fences_its_inputs_against_a_competing_build() { + let (wallet_manager, wallet_id, _generation, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let funded = the_only_funded_outpoint(&wallet_manager, wallet_id).await; + + let entered = Arc::new(tokio::sync::Barrier::new(2)); + let release = Arc::new(tokio::sync::Barrier::new(2)); + let (parked_manager, _p1) = asset_lock_manager_over( + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(GatedBroadcaster { + entered: Arc::clone(&entered), + release: Arc::clone(&release), + }), + ); + let (competing_manager, _p2) = asset_lock_manager_over( + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(CountingOkBroadcaster::default()), + ); + + let parked = async { + parked_manager + .broadcast_funded_asset_lock( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await + }; + + let competitor = async { + // Parked inside `broadcast`: signed, guard dropped, nothing + // submitted — the window the fence has to cover. + entered.wait().await; + catch_up_past_the_reservation_ttl(&wallet_manager, wallet_id, 17_000).await; + + let racing = competing_manager + .broadcast_funded_asset_lock( + 1_000_000, + 0, + AssetLockFundingType::IdentityRegistration, + 0, + &signer, + ) + .await; + release.wait().await; + racing + }; + + let (sent, racing) = tokio::join!(parked, competitor); + + match racing { + Err(PlatformWalletError::InputMidBroadcast { outpoint }) => assert_eq!( + outpoint, funded, + "the refusal must name the input the parked lock spends" + ), + other => panic!( + "a competing asset-lock build must be refused while the original is \ + mid-broadcast — unfenced, it returned a second signed lock spending \ + the same input, got {other:?}" + ), + } + + assert!( + sent.is_ok(), + "the parked asset lock itself must complete normally, got {sent:?}" + ); + } + /// Builds an `AssetLockManager` over the CoinJoin-funded fixture /// (CoinJoin account 0 holds a single 10_000_000-duff spendable UTXO). async fn coinjoin_funded_asset_lock_manager( diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 6c53c5dba57..cd33e38b379 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -1,13 +1,191 @@ use dashcore::Transaction; use key_wallet::account::account_type::StandardAccountType; +use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use key_wallet::ReservationToken; use super::SignedCoreTransaction; use crate::broadcaster::{BroadcastError, TransactionBroadcaster}; -use crate::wallet::reservations::broadcast_releasing_on_rejection; +use crate::wallet::reservations::{broadcast_releasing_on_rejection, reservation_expired}; use crate::{CoreWallet, PlatformWalletError}; +/// Outcome of [`CoreWallet::dispatch_unexpired`] — the guarded +/// age-check-and-send. `Stale` means the broadcaster was never touched. +pub(crate) enum GuardedDispatch { + /// The reservation aged past the bound; nothing was sent. + Stale, + /// The broadcaster was reached; its verbatim outcome. + Sent(Result), +} + impl CoreWallet { + /// Age-check AND pin under the wallet-manager READ lock, then dispatch + /// immediately after releasing it, keeping the pin until the broadcaster + /// returns. + /// + /// The age check orders against key-wallet's `ReservationSet` TTL + /// sweep — it runs inside coin selection, which mutates wallet state + /// under the manager WRITE lock — and `last_processed_height` + /// advancement (same lock): a reservation that passes the check under + /// this guard cannot already have been swept, because key-wallet's TTL + /// exceeds `RESERVATION_MAX_AGE_BLOCKS` on the same height clock, and + /// self-releases only run on this transaction's own rejection/abandon + /// paths, which are sequenced after this call returns. That proof of + /// still-held ownership is what authorizes the pin taken in the same + /// guarded section (the pin's owner check). + /// + /// The guard is deliberately DROPPED before the broadcaster await. The + /// production `SpvBroadcaster` waits on dash-spv's mempool pipeline, + /// and that pipeline's local-transaction handler takes `wallet.write()` + /// on this same manager lock before it can process the very + /// echo/IS-lock/confirmation events the wait needs — held across the + /// await, the guard starves the pipeline and every dispatch rides the + /// full acceptance timeout to an ambiguous verdict while the whole + /// manager stalls behind tokio's write-preferring queue. (Same + /// lock-free shape as `broadcast_releasing_on_rejection`.) + /// + /// What spans the await instead is the **in-broadcast pin** + /// ([`WalletGeneration::pin_in_broadcast`](super::WalletGeneration::pin_in_broadcast)), + /// installed on the manager-registered generation while the guard was + /// still held. Both production broadcasters can suspend *before* + /// submission (the SPV path awaits configuration, event subscription and + /// the network lock ahead of its local dispatch), catch-up can advance + /// the clock by many blocks in that gap, and async scheduling puts no + /// bound on it — so a freshness check alone is not an ordering + /// invariant against the TTL sweep + re-reserve race. The pin is: it + /// has no TTL while the dispatch is in flight, and every coin-selection + /// choke point refuses a build whose selection picked a pinned input + /// (under the same write lock the sweep runs under). + /// + /// # Where the fence is released, and why that point is safe + /// + /// The pin is *not* simply dropped when the broadcaster returns. That + /// return means "the transaction may now be on the network", not "this + /// wallet has observed the spend", and the two differ per broadcaster: + /// `SpvBroadcaster` injects into dash-spv's local mempool pipeline, so the + /// inputs leave this wallet's selectable set within milliseconds; + /// `DapiBroadcaster::broadcast` only awaits `sdk.execute` and injects + /// nothing, so on that path the inputs are still selectable while the + /// transaction is in flight (`dashpay/platform#4309`). So: + /// + /// * **Definitive pre-send rejection** (`BroadcastError::Rejected`) — the + /// transaction provably did not reach the network. The fence is dropped + /// immediately here, and the caller releases the reservation in the same + /// breath, so an instant rebuild can reselect the inputs. + /// * **Anything else** (accepted, or an ambiguous `MaybeSent`) — the pin is + /// converted to a **pending-spend fence** that lasts until this wallet + /// OBSERVES the outpoints spent + /// ([`WalletGeneration::observe_spent`](super::WalletGeneration::observe_spent)), + /// by the dispatch's own transaction or by a competing one. + /// + /// # Why the fence waits for an observation instead of a height bound + /// + /// Three earlier revisions bounded the pending-spend phase at + /// `last_processed_height + N` and disagreed only about where to sample the + /// height — before the await, after it, after it under a still-held guard. + /// Every one of them can be consumed by a routine historical catch-up: the + /// wallet advances that height by thousands of blocks in seconds, and those + /// blocks were mined BEFORE this transaction was submitted, so they are not + /// evidence that it has been seen or dropped. On the `DapiBroadcaster` path + /// — which returns from `sdk.execute` without injecting anything into local + /// wallet state — the input then becomes reselectable while the transaction + /// is in flight (`dashpay/platform#4309`, review round 5). + /// + /// The release condition is therefore evidence, not elapsed chain: the + /// outpoint is freed when the wallet sees it spent. That is a fact about + /// this transaction rather than about the chain's past, and it arrives on + /// both broadcaster paths — SPV within milliseconds via its local mempool + /// pipeline, DAPI when the transaction is relayed back or lands in a block. + /// + /// There is NO backstop timeout behind that, and deliberately so. A + /// one-hour monotonic deadline used to sit here as a liveness valve; a + /// clock catch-up cannot fast-forward is still not evidence about this + /// transaction, and once it lapsed the next build could sign a conflicting + /// spend of inputs the original might still take (`dashpay/platform#4309`, + /// review round 7). A transaction the wallet never observes at all — evicted + /// for fee, conflicted away unseen — therefore holds its inputs for the rest + /// of the process. That is the correct trade: those are exactly the inputs a + /// possibly-live signed transaction spends. See the + /// [`in_broadcast`](super::WalletGeneration) field docs for the invariant + /// and for the two liveness shapes that may shorten the wait without + /// weakening it. + /// + /// # Why there is no post-await manager guard any more + /// + /// Round 4 of this review added one: the fence's height had to be sampled + /// and installed inside a single manager read guard, or a writer queued + /// behind it could advance the clock in between and the fence would land + /// already lapsed. With no clock to sample at all there is nothing for a + /// height writer to interleave with — the settle sets a flag inside the + /// `in_broadcast` critical section. So it needs no manager lock, and this + /// method now touches the wallet-manager lock exactly once, before the + /// send, which also removes a lock acquisition from every dispatch. + /// + /// A wallet no longer in the manager skips the pin (there is no + /// registered generation to fence builds on — they cannot fund from a + /// removed wallet); liveness is the FFI layer's generation check, + /// established before this runs. + /// + /// Callers do their stale/rejection reconciliation AFTER this returns: + /// those paths retake manager locks. + pub(crate) async fn dispatch_unexpired( + &self, + reservation_height: u32, + transaction: &Transaction, + ) -> GuardedDispatch { + let in_broadcast_pin = { + let wm = self.wallet_manager.read().await; + let info = wm.get_wallet_info(&self.wallet_id); + let height = info.map(|info| info.core_wallet.last_processed_height()); + if reservation_expired(reservation_height, height) { + return GuardedDispatch::Stale; + } + // Pin BEFORE the guard drops: check-and-pin is one atomic step, + // and freshness under this guard proves the reservation is still + // ours to pin (see the method docs). The pin outlives the guard, + // and — unless the send is definitively rejected — outlives the + // broadcaster return too, as a pending-spend fence. + // + // `height` is NOT handed to the pin, and no height is sampled for + // it later either. It authorizes the freshness check and nothing + // else; the fence answers to observed spends and to NO clock of any + // kind — not chain height, not wall time — because nothing that + // merely elapses is evidence about this transaction (see the method + // docs). + info.map(|info| info.generation.pin_in_broadcast(transaction)) + // Guard dropped here — holding it across the await starves the + // SPV pipeline that must complete the wait; the pin, not the + // guard, covers check-to-wire. + }; + let outcome = self.broadcaster.broadcast(transaction).await; + // The pin already fences by default, so the inputs stay held on EVERY + // exit from the await above — including one this code never observes: + // the dispatching future being cancelled, or an unwind, mid-`broadcast`. + // Neither says anything about whether the transaction reached the + // network, and freeing the inputs there lets an immediate reselection + // double-spend a transaction already on the wire (`dashpay/platform#4309`). + // + // Only a definitive pre-send rejection proves nothing was sent, so it is + // the one outcome that releases. An ambiguous `MaybeSent` stays fenced. + if let Some(pin) = in_broadcast_pin { + if matches!( + outcome, + Err(crate::broadcaster::BroadcastError::Rejected { .. }) + ) { + // Provably nothing on the wire: free the outpoints outright, so + // an immediate rebuild can reselect them. + pin.settle_released(); + } else { + // EVERY non-rejection outcome — accepted or ambiguous + // `MaybeSent` — opens the pending-spend phase, which holds the + // outpoints until the wallet observes them spent. No manager + // guard is taken: there is no height to sample, so there is + // nothing for a concurrent height writer to interleave with. + pin.settle_pending_spend(); + } + } + GuardedDispatch::Sent(outcome) + } + /// Broadcast an atomically finalized transaction. A definitive rejection /// releases its reservation; an ambiguous `MaybeSent` outcome retains it. /// @@ -18,13 +196,61 @@ impl CoreWallet { /// same inputs under a new token. Releasing by outpoint alone would then /// free that other build's inputs (the `dashpay/platform#4185` double-spend /// window); presenting the token frees only inputs this build still owns. + /// + /// # Reservation age guard + /// + /// A finalized-transaction handle can be pinned by the host for an + /// arbitrary time between `finalize` and this broadcast. If the wallet's + /// `last_processed_height` advances at least + /// [`RESERVATION_MAX_AGE_BLOCKS`](crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS) + /// blocks past the height the funding reservation was stamped at + /// ([`SignedCoreTransaction::reservation_height`]), key-wallet's own + /// `ReservationSet` TTL could already have swept those inputs and let an + /// unrelated build re-select them. Broadcasting then would spend against a + /// newer, unrelated reservation, so the send is refused with + /// [`PlatformWalletError::StaleReservation`] **before** the broadcaster is + /// touched — mirroring the deferred registry token's + /// [`broadcast`](crate::SignedPaymentRegistry::broadcast) guard, off the + /// same bound and the same `last_processed_height` clock, and running after + /// the FFI layer's generation-identity check just as the registry does. + /// + /// The refusal also reconciles the reservation, exactly like the registry's + /// stale-token branch: the FFI wrapper has already consumed the opaque + /// handle by the time this runs (and the host bindings clear their local + /// handles before entering the ABI), so a follow-up + /// [`abandon_transaction`](Self::abandon_transaction) is unreachable from + /// the caller's side. Abandoning here releases owner-guarded + /// (`release_reservation_if_owner`), which is safe at ANY age — between the + /// guard bound and key-wallet's TTL the reservation is typically STILL this + /// build's, so the release is what lets the instructed immediate rebuild + /// reselect the inputs instead of stranding them until the TTL backstop. + /// Only a token-less build (never reached on the funded finalize path) + /// skips, leaving the aged reservation for the TTL to reclaim. pub async fn broadcast_finalized_transaction( &self, transaction: &SignedCoreTransaction, ) -> Result { - match self.broadcaster.broadcast(transaction.transaction()).await { - Ok(txid) => Ok(txid), - Err(error) => { + // The age check happens at dispatch time, inside + // [`Self::dispatch_unexpired`] — not out here, where it would go + // stale before the send (sync catch-up can age the reservation and + // a concurrent finalization can sweep + re-reserve the same inputs + // in the gap, letting the old signed transaction hit the wire + // against reassigned UTXOs). The check also installs the + // in-broadcast pin that fences the inputs against exactly that + // sweep + re-reserve until the broadcaster returns; why the manager + // guard itself must not span the broadcaster await is documented on + // `dispatch_unexpired`. Reconciliation retakes manager locks after + // it returns. + match self + .dispatch_unexpired(transaction.reservation_height(), transaction.transaction()) + .await + { + GuardedDispatch::Stale => { + self.abandon_transaction(transaction).await; + Err(PlatformWalletError::StaleReservation) + } + GuardedDispatch::Sent(Ok(txid)) => Ok(txid), + GuardedDispatch::Sent(Err(error)) => { if matches!(error, crate::broadcaster::BroadcastError::Rejected { .. }) { self.release_transaction_reservation( transaction.funding_accounts(), @@ -129,15 +355,31 @@ impl CoreWallet { /// build stamped across all of them /// (`SignedCoreTransaction::reservation_token`), `None` only when the build /// reserved nothing. + /// `reservation_height` is the height the funding reservation was + /// stamped at; the age bound is re-checked ATOMICALLY with dispatch + /// under the manager read guard ([`Self::dispatch_unexpired`]) — a + /// pre-checked age is not an invariant, because catch-up can advance + /// the clock and a concurrent finalization can sweep + re-reserve the + /// inputs between a caller's check and the send. The same guarded + /// section installs the in-broadcast pin that fences the inputs against + /// that sweep + re-reserve for the whole broadcaster await — the same + /// primitive as the finalized-handle path. On the stale outcome + /// nothing was sent and NOTHING is released here: the caller owns the + /// reconciliation policy (the registry reconciles owner-guarded). pub(crate) async fn broadcast_payment_releasing_reservation( &self, accounts: &[key_wallet::account::AccountType], transaction: &Transaction, token: Option, + reservation_height: u32, ) -> Result { - match self.broadcaster.broadcast(transaction).await { - Ok(txid) => Ok(txid), - Err(error) => { + match self + .dispatch_unexpired(reservation_height, transaction) + .await + { + GuardedDispatch::Stale => Err(PlatformWalletError::StaleReservation), + GuardedDispatch::Sent(Ok(txid)) => Ok(txid), + GuardedDispatch::Sent(Err(error)) => { if matches!(error, BroadcastError::Rejected { .. }) { self.release_transaction_reservation(accounts, transaction, token) .await; @@ -152,20 +394,25 @@ impl CoreWallet { mod tests { use std::sync::Arc; + use super::GuardedDispatch; use dashcore::{Address as DashAddress, Network, Transaction}; use key_wallet::account::account_type::StandardAccountType; use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; use key_wallet::signer::Signer; use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy; use key_wallet::wallet::managed_wallet_info::transaction_builder::TransactionBuilder; + use key_wallet::wallet::managed_wallet_info::transaction_building::AccountTypePreference; use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use crate::broadcaster::TransactionBroadcaster; use crate::test_support::{ - funded_wallet_manager, AlwaysMaybeSentBroadcaster, RejectFirstBroadcaster, WalletSigner, + funded_wallet_manager, AlwaysMaybeSentBroadcaster, AlwaysOkBroadcaster, + RejectFirstBroadcaster, WalletSigner, }; - use crate::wallet::core::CoreWallet; - use crate::PlatformWalletError; + use crate::wallet::core::{CoreWallet, SpendObservationHandler}; + use crate::wallet::platform_wallet::WalletId; + use crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS; + use crate::{PlatformWalletError, SignedCoreTransaction}; /// Builds a testnet `CoreWallet` over the shared funded fixture and a /// 1_000_000-duff payment to a dummy recipient. @@ -247,6 +494,922 @@ mod tests { Ok(tx) } + /// Atomically fund + reserve + sign a `SignedCoreTransaction` the way the + /// finalized-handle path (`core_wallet_tx_builder_finalize`) does, capturing + /// the reservation's stamp height on the returned handle. + async fn finalize_tx( + core: &CoreWallet, + account_type: AccountTypePreference, + outputs: &[(DashAddress, u64)], + signer: &WalletSigner, + ) -> SignedCoreTransaction { + try_finalize_tx(core, account_type, outputs, signer) + .await + .expect("finalize should succeed") + } + + /// Like [`finalize_tx`] but surfaces the build error instead of panicking — + /// used to prove a *rebuild* fails when a still-held reservation keeps its + /// inputs out of the selectable pool. + async fn try_finalize_tx( + core: &CoreWallet, + account_type: AccountTypePreference, + outputs: &[(DashAddress, u64)], + signer: &WalletSigner, + ) -> Result { + let mut builder = TransactionBuilder::new(); + for (addr, amount) in outputs { + builder = builder.add_output(addr, *amount); + } + core.finalize_transaction(builder, &[account_type], 0, signer) + .await + } + + /// Force the wallet's `last_processed_height` forward, simulating chain + /// progress between `finalize` and a later broadcast of the pinned + /// handle — the window in which key-wallet's `ReservationSet` TTL can sweep + /// the funding reservation. Same clock the age guard reads. + async fn advance_processed_height( + core: &CoreWallet, + height: u32, + ) { + let mut wm = core.wallet_manager.write().await; + let (_, info) = wm + .get_wallet_and_info_mut(&core.wallet_id()) + .expect("wallet present in manager"); + info.core_wallet.update_last_processed_height(height); + } + + /// A freshly finalized handle — no chain progress since `finalize` — + /// broadcasts normally: the age guard does not trip. + #[tokio::test] + async fn fresh_finalized_handle_broadcasts() { + for account_type in [AccountTypePreference::BIP44, AccountTypePreference::BIP32] { + let (core, signer, outputs) = funded_core_wallet( + account_type_standard(account_type), + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let finalized = finalize_tx(&core, account_type, &outputs, &signer).await; + let sent = core.broadcast_finalized_transaction(&finalized).await; + assert!( + sent.is_ok(), + "a fresh handle must broadcast for {account_type:?}, got {sent:?}" + ); + } + } + + /// A handle pinned while the wallet syncs past `RESERVATION_MAX_AGE_BLOCKS` + /// beyond its reservation stamp must be refused with `StaleReservation` + /// (never a send — the broadcaster is `AlwaysOk`, so a leaked send would + /// surface as `Ok`). The refusal itself reconciles the reservation, + /// OWNER-GUARDED — this is terminal at the FFI boundary, where the opaque + /// handle was consumed before the guard ran, so no follow-up abandon is + /// possible. Below key-wallet's TTL the reservation is still this build's, + /// `release_reservation_if_owner` frees it, and the instructed immediate + /// rebuild reselects the inputs with NO further cleanup call. A late + /// abandon of the stale original is then an owner-guarded no-op — ownership + /// has transferred to the rebuild, whose reservation must survive it. + #[tokio::test] + async fn aged_finalized_handle_refusal_releases_for_rebuild() { + for account_type in [AccountTypePreference::BIP44, AccountTypePreference::BIP32] { + let (core, signer, outputs) = funded_core_wallet( + account_type_standard(account_type), + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(&core, account_type, &outputs, &signer).await; + + // Advance past the guard bound (stay below key-wallet's 24-block TTL, + // so the reservation is provably still held — only our guard tripped). + advance_processed_height(&core, stamped + RESERVATION_MAX_AGE_BLOCKS + 2).await; + + let sent = core.broadcast_finalized_transaction(&finalized).await; + assert!( + matches!(sent, Err(PlatformWalletError::StaleReservation)), + "an aged handle must refuse with StaleReservation for \ + {account_type:?}, got {sent:?}" + ); + + // The refusal released the still-owned reservation: an immediate + // rebuild reselects the single fixture UTXO without any abandon. + let rebuilt = try_finalize_tx(&core, account_type, &outputs, &signer).await; + let rebuilt = rebuilt.unwrap_or_else(|error| { + panic!( + "the stale refusal must release the still-owned reservation \ + so a rebuild succeeds for {account_type:?}, got {error:?}" + ) + }); + + // A late abandon of the stale original must be an owner-guarded + // no-op: ownership transferred to the rebuild, so the rebuild's + // reservation still holds the fixture's only UTXO and a competing + // finalize must fail. + core.abandon_transaction(&finalized).await; + let competing = try_finalize_tx(&core, account_type, &outputs, &signer).await; + assert!( + competing.is_err(), + "abandoning the consumed stale handle must not free the \ + rebuild's reservation for {account_type:?}, got a successful \ + competing finalize" + ); + core.abandon_transaction(&rebuilt).await; + } + } + + /// The age bound is validated by [`CoreWallet::dispatch_unexpired`] + /// itself, immediately before the send — never by a caller-side + /// pre-check that could go stale in the gap. The height sample and the + /// expiry verdict happen under a wallet-manager read guard that is + /// dropped before the broadcaster await (holding it across the await + /// starves the SPV mempool pipeline — see `dispatch_unexpired`'s doc); + /// the check-to-wire gap is covered by the in-broadcast pin installed + /// in the same guarded section (see + /// `in_broadcast_pin_blocks_reselection_until_dispatch_returns`). The + /// single-threaded proof here: the same handle's inputs dispatch while + /// fresh, and the identical call refuses — broadcaster untouched — + /// once catch-up advances the clock past the bound. + #[tokio::test] + async fn guarded_dispatch_rechecks_age_at_dispatch() { + let (core, signer, outputs) = funded_core_wallet( + account_type_standard(AccountTypePreference::BIP44), + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + + // Fresh: the guarded dispatch reaches the broadcaster. + let fresh = core + .dispatch_unexpired(finalized.reservation_height(), finalized.transaction()) + .await; + assert!( + matches!(fresh, GuardedDispatch::Sent(Ok(_))), + "a fresh reservation must dispatch" + ); + + // Catch-up advances the clock past the bound; the identical call now + // refuses inside the guard with the broadcaster never touched + // (`AlwaysOk` would have surfaced a leaked send as `Sent(Ok)`). + advance_processed_height(&core, stamped + RESERVATION_MAX_AGE_BLOCKS + 2).await; + let stale = core + .dispatch_unexpired(finalized.reservation_height(), finalized.transaction()) + .await; + assert!( + matches!(stale, GuardedDispatch::Stale), + "an aged reservation must refuse at the check, not dispatch" + ); + + core.abandon_transaction(&finalized).await; + } + + /// Below the guard bound the reservation is provably still ours (no sweep + /// possible yet), so abandon/free release it — owner-guarded, via the token + /// the funded finalize stamped — returning the inputs so an immediate + /// rebuild reselects them. + #[tokio::test] + async fn below_bound_finalized_handle_abandon_releases() { + for account_type in [AccountTypePreference::BIP44, AccountTypePreference::BIP32] { + let (core, signer, outputs) = funded_core_wallet( + account_type_standard(account_type), + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(&core, account_type, &outputs, &signer).await; + + // Aged, but one shy of the guard bound: still below both the guard and + // the TTL, so the reservation is unambiguously ours to release. + advance_processed_height(&core, stamped + RESERVATION_MAX_AGE_BLOCKS - 1).await; + + core.abandon_transaction(&finalized).await; + + // The release freed the input: an immediate rebuild reselects it. + let rebuilt = try_finalize_tx(&core, account_type, &outputs, &signer).await; + assert!( + rebuilt.is_ok(), + "below-bound abandon must release the input so a rebuild reselects \ + it for {account_type:?}, got {rebuilt:?}" + ); + core.abandon_transaction(&rebuilt.expect("rebuild")).await; + } + } + + /// The guard boundary is exact: `current - stamped >= RESERVATION_MAX_AGE_BLOCKS` + /// refuses, one block below still broadcasts — for both standard account + /// types, like the fresh/aged tests. + #[tokio::test] + async fn finalized_handle_age_guard_boundary_is_exact() { + for account_type in [AccountTypePreference::BIP44, AccountTypePreference::BIP32] { + // One below the bound: still fresh enough to broadcast. + let (below_core, below_signer, below_outputs) = funded_core_wallet( + account_type_standard(account_type), + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let below_stamped = below_core + .last_processed_height() + .await + .expect("last processed height"); + let below = finalize_tx(&below_core, account_type, &below_outputs, &below_signer).await; + advance_processed_height(&below_core, below_stamped + RESERVATION_MAX_AGE_BLOCKS - 1) + .await; + assert!( + below_core + .broadcast_finalized_transaction(&below) + .await + .is_ok(), + "one block below the bound must still broadcast ({account_type:?})" + ); + + // Exactly at the bound: refused. + let (at_core, at_signer, at_outputs) = funded_core_wallet( + account_type_standard(account_type), + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let at_stamped = at_core + .last_processed_height() + .await + .expect("last processed height"); + let at = finalize_tx(&at_core, account_type, &at_outputs, &at_signer).await; + advance_processed_height(&at_core, at_stamped + RESERVATION_MAX_AGE_BLOCKS).await; + assert!( + matches!( + at_core.broadcast_finalized_transaction(&at).await, + Err(PlatformWalletError::StaleReservation) + ), + "exactly at the bound must refuse with StaleReservation ({account_type:?})" + ); + } + } + + /// Map a builder `AccountTypePreference` (BIP44/BIP32 only in these tests) + /// to the `StandardAccountType` the funded fixture is keyed by. + fn account_type_standard(account_type: AccountTypePreference) -> StandardAccountType { + match account_type { + AccountTypePreference::BIP44 => StandardAccountType::BIP44Account, + AccountTypePreference::BIP32 => StandardAccountType::BIP32Account, + other => { + unreachable!("only standard-account funding is exercised by these tests: {other:?}") + } + } + } + + /// A broadcaster that models the pre-submission suspension window of the + /// production broadcasters: `broadcast` parks between two barriers, so the + /// test can interleave catch-up and a competing build while the dispatch + /// is provably mid-await (freshness already checked, guard already + /// dropped, pin held). + struct GatedBroadcaster { + entered: Arc, + release: Arc, + } + + #[async_trait::async_trait] + impl TransactionBroadcaster for GatedBroadcaster { + async fn broadcast( + &self, + transaction: &Transaction, + ) -> Result { + self.entered.wait().await; + self.release.wait().await; + Ok(transaction.txid()) + } + } + + /// The `WalletEvent` the wallet emits when it observes `tx` — the real + /// shape the spend-observation seam consumes. Shared fixture, so this + /// module and the manager-level wiring test cannot drift onto different + /// event shapes. + fn spend_event( + core: &CoreWallet, + tx: &Transaction, + ) -> key_wallet_manager::WalletEvent { + crate::test_support::observed_spend_event(core.wallet_id(), tx) + } + + /// An `Arc` sharing `core`'s manager, wallet id, and + /// generation — the entry the production wallets map holds for this + /// wallet, so the spend-observation tests can resolve the REAL registered + /// generation through a real map. Its own `SpvBroadcaster` is inert: the + /// spend-observation seam only ever reads `generation()` through it. + fn platform_wallet_sharing( + core: &CoreWallet, + ) -> Arc { + let spv = Arc::new(crate::spv::SpvRuntime::new( + Arc::clone(&core.wallet_manager), + Arc::new(crate::events::PlatformEventManager::new(Vec::new())), + )); + Arc::new(crate::wallet::PlatformWallet::new( + Arc::clone(&core.sdk), + core.wallet_id(), + Arc::clone(&core.wallet_manager), + Arc::clone(core.generation()), + Arc::new(tokio::sync::Notify::new()), + Arc::new(crate::test_support::NoopTestPersister) + as Arc, + Arc::new(crate::broadcaster::SpvBroadcaster::new(spv)), + )) + } + + /// A wallets map — the production `BTreeMap>` + /// behind its own `RwLock` — holding one entry per fixture wallet. + fn wallets_map( + cores: &[&CoreWallet], + ) -> Arc< + tokio::sync::RwLock< + std::collections::BTreeMap>, + >, + > { + Arc::new(tokio::sync::RwLock::new( + cores + .iter() + .map(|core| (core.wallet_id(), platform_wallet_sharing(core))) + .collect(), + )) + } + + /// Retire fences from `event` by driving the PRODUCTION spend-observation + /// seam end to end: a real [`SpendObservationHandler`] over a real wallets + /// map whose entry shares `core`'s registered generation. `on_wallet_event` + /// therefore exercises the whole handler path — the variant gate + /// (`observing_wallet`), the projection (`observed_spends`), the + /// wallets-map `try_read`, the wallet-id lookup, and the selected + /// generation's release — not a shortcut to `observe_spent` + /// (`dashpay/platform#4309`, review round 6). + fn observe_via_event_handler( + core: &CoreWallet, + event: key_wallet_manager::WalletEvent, + ) { + assert!( + !crate::wallet::core::spend_observer::observed_spends(&event).is_empty(), + "the fixture event must report at least one spend, or the test \ + would pass without observing anything" + ); + let handler = SpendObservationHandler::new(wallets_map(&[core])); + dash_spv::EventHandler::on_wallet_event(&handler, &event); + } + + /// Assert that `result` is the typed in-broadcast conflict, and return the + /// outpoint it names. + /// + /// The tests used to spell this `message.contains("mid-broadcast")`, which + /// is exactly the substring-matching the typed + /// `PlatformWalletError::InputMidBroadcast` variant removes + /// (`dashpay/platform#4309`, review round 5 suggestion). + fn expect_mid_broadcast( + result: Result, + context: &str, + ) -> dashcore::OutPoint { + match result { + Err(PlatformWalletError::InputMidBroadcast { outpoint }) => outpoint, + other => panic!("{context}, got {other:?}"), + } + } + + /// THE CHECK-TO-WIRE RACE the in-broadcast pin closes: the freshness + /// check passes under the manager read guard, the guard drops, and the + /// dispatch suspends inside the broadcaster BEFORE submission. Catch-up + /// then advances the clock past key-wallet's reservation TTL, so a + /// competing finalize's own selection sweeps the dispatched build's + /// reservation and re-selects its input — pre-pin, that build completed + /// and raced the already-signed transaction on the wire. With the pin + /// held across the await, the competing finalize must be REFUSED. + /// + /// The tail covers the HANDOFF from the dispatching pin to the + /// pending-spend fence: the dispatch returns, the pin lifts, and the + /// outpoint stays fenced anyway — now with no dependence on how far the + /// 48-block catch-up moved the chain clock. + #[tokio::test] + async fn in_broadcast_pin_blocks_reselection_until_dispatch_returns() { + let entered = Arc::new(tokio::sync::Barrier::new(2)); + let release = Arc::new(tokio::sync::Barrier::new(2)); + let (core, signer, outputs) = funded_core_wallet( + StandardAccountType::BIP44Account, + Arc::new(GatedBroadcaster { + entered: Arc::clone(&entered), + release: Arc::clone(&release), + }), + ) + .await; + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let fenced = finalized.transaction().input[0].previous_output; + + // Dispatch at the oldest height the age guard admits, so the pin is + // taken and the broadcaster then parks pre-submission. + advance_processed_height(&core, stamped + RESERVATION_MAX_AGE_BLOCKS - 1).await; + let dispatcher = tokio::spawn({ + let core = core.clone(); + async move { core.broadcast_finalized_transaction(&finalized).await } + }); + entered.wait().await; + + // Catch-up well past key-wallet's reservation TTL while the dispatch is + // suspended: the reservation is swept, so only the pin holds the input. + advance_processed_height(&core, stamped + RESERVATION_MAX_AGE_BLOCKS + 48).await; + let racing = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + assert_eq!( + expect_mid_broadcast( + racing, + "a build that swept a mid-dispatch reservation must be refused" + ), + fenced, + "the refusal must name the conflicting outpoint" + ); + + release.wait().await; + let sent = dispatcher.await.expect("dispatcher task"); + assert!( + sent.is_ok(), + "the pinned dispatch itself must complete, got {sent:?}" + ); + + // The dispatching pin has now lifted — and the input is STILL not + // selectable. This assertion has been through three revisions of + // `dashpay/platform#4309`: it originally asserted the input was free + // again (the bug), then that it was fenced until a height-anchored + // bound. It now holds regardless of the chain clock, because the fence + // is waiting for an observed spend that this mock manager — which runs + // no mempool pipeline, exactly like the `DapiBroadcaster` path — never + // produces. + let still_fenced = + try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + expect_mid_broadcast( + still_fenced, + "the broadcaster returning is not the spend being observed, so the \ + input must stay fenced", + ); + } + + /// `dashpay/platform#4309`, THE ROUND-5 BLOCKER, VERBATIM. + /// + /// > after [the guard is released], a synchronization writer queued during + /// > the short critical section — or ordinary catch-up completing before + /// > the next build — can immediately advance `last_processed_height` by + /// > the whole interval. […] Those elapsed heights may be historical blocks + /// > mined BEFORE the transaction was submitted, so they provide no + /// > evidence that the submitted transaction has been observed or dropped. + /// + /// This is the test that FAILS on every prior revision of this PR. Each of + /// them installed `pending_until = + IN_BROADCAST_FENCE_BLOCKS` + /// and reaped the fence once `last_processed_height` reached it; the + /// catch-up below clears that bound by a wide margin no matter which height + /// was sampled — pre-await, post-await, or post-await under a held guard — + /// so all three leave the input reselectable here while the transaction is + /// on the network. + /// + /// The broadcaster is `AlwaysOk`: the transaction is ACCEPTED, so it is + /// certainly on the wire. The manager runs no mempool pipeline, which is + /// the `DapiBroadcaster` shape — `sdk.execute` returns without injecting + /// anything locally — so nothing has observed the spend. + #[tokio::test] + async fn fence_survives_a_full_historical_catch_up_advance() { + let (core, signer, outputs) = funded_core_wallet( + StandardAccountType::BIP44Account, + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let fenced = finalized.transaction().input[0].previous_output; + + assert!(core + .broadcast_finalized_transaction(&finalized) + .await + .is_ok()); + + // Historical catch-up. Not a few blocks past some bound — a whole + // month of blocks, all of them mined long before this transaction was + // submitted, applied in the instant between the dispatch returning and + // the next build. This is the ordinary mobile resync, and it is what + // consumed every height-anchored bound this PR previously shipped. + let caught_up = stamped + 17_000; + advance_processed_height(&core, caught_up).await; + + let racing = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + assert_eq!( + expect_mid_broadcast( + racing, + "historical catch-up must not retire a fence: those blocks predate \ + the transaction and are not evidence it was seen or dropped" + ), + fenced, + ); + + // And it is not merely slow to expire — no amount of further chain + // progress retires it either. + advance_processed_height(&core, caught_up + 500_000).await; + expect_mid_broadcast( + try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await, + "no quantity of elapsed height may retire the fence", + ); + + // The ONLY thing that can: an observed spend. + core.generation().observe_spent([fenced]); + let after = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let after = after.unwrap_or_else(|error| { + panic!("an observed spend must release the fence, got {error:?}") + }); + core.abandon_transaction(&after).await; + } + + /// The fence's designed release: the wallet OBSERVES the dispatched + /// transaction's own spend, off the wallet-event fan-out. + /// + /// Drives the real seam — [`SpendObservationHandler`] fed a + /// `TransactionDetected` event carrying the dispatched transaction — rather + /// than calling `observe_spent` directly, so this covers the projection + /// from a `WalletEvent` to the outpoints it retires. That projection is + /// shared with `CoreChangeSet::spent_utxos`, so the fence and the + /// persister's spent set cannot disagree. + #[tokio::test] + async fn observing_the_dispatched_transaction_releases_the_fence() { + let (core, signer, outputs) = funded_core_wallet( + StandardAccountType::BIP44Account, + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let sent_tx = finalized.transaction().clone(); + let fenced = sent_tx.input[0].previous_output; + + assert!(core + .broadcast_finalized_transaction(&finalized) + .await + .is_ok()); + + // Catch-up past key-wallet's 24-block reservation TTL, so the funding + // reservation is swept and the input is back in the selectable pool. + // That is the window the fence exists for — without it the reservation, + // not the fence, is what refuses the competing build, and this test + // would pass without exercising the fence at all. + advance_processed_height(&core, stamped + 17_000).await; + assert_eq!( + expect_mid_broadcast( + try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await, + "the dispatched input must be fenced before the spend is observed", + ), + fenced, + ); + + // The wallet sees its own transaction — mempool relay on the DAPI path, + // or the local pipeline on the SPV one. Either way this event is what + // arrives. + observe_via_event_handler(&core, spend_event(&core, &sent_tx)); + + let after = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let after = after.unwrap_or_else(|error| { + panic!("observing the dispatch's own spend must release the fence, got {error:?}") + }); + core.abandon_transaction(&after).await; + } + + /// A COMPETING spend releases the fence too. The outpoint has left this + /// wallet's selectable set whoever spent it, so there is no re-selection + /// left that could race anything on the wire — continuing to fence would + /// hold the input for good and protect nothing. + #[tokio::test] + async fn observing_a_competing_spend_releases_the_fence() { + let (core, signer, outputs) = funded_core_wallet( + StandardAccountType::BIP44Account, + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let fenced = finalized.transaction().input[0].previous_output; + + assert!(core + .broadcast_finalized_transaction(&finalized) + .await + .is_ok()); + + // Sweep the funding reservation (see the sibling test above), so the + // fence is the only thing holding the input. + advance_processed_height(&core, stamped + 17_000).await; + expect_mid_broadcast( + try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await, + "the dispatched input must be fenced before any spend is observed", + ); + + // A DIFFERENT transaction spending the same outpoint — a competing + // spend the wallet observes. Its txid differs from the dispatched one. + let mut competing = finalized.transaction().clone(); + competing.lock_time = finalized.transaction().lock_time + 1; + assert_ne!( + competing.txid(), + finalized.transaction().txid(), + "the fixture must model a genuinely different transaction" + ); + + observe_via_event_handler(&core, spend_event(&core, &competing)); + + let after = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let after = after.unwrap_or_else(|error| { + panic!("a competing spend must also release the fence, got {error:?}") + }); + assert_eq!( + after.transaction().input[0].previous_output, + fenced, + "the fixture has one UTXO, so the rebuild reselects the same outpoint" + ); + core.abandon_transaction(&after).await; + } + + /// The handler releases ONLY the generation registered under the event's + /// wallet id (`dashpay/platform#4309`, review round 6). Two fenced wallets + /// share ONE wallets map — the production shape — and: an event naming a + /// wallet id registered NOWHERE releases neither fence, and wallet A's own + /// spend event releases A's fence while B's stands. A handler that routed + /// by anything but the event's wallet id, or that failed its map lookup + /// open, fails one of the two halves. + #[tokio::test] + async fn spend_observation_releases_only_the_matching_registered_generation() { + let (core_a, signer_a, outputs_a) = funded_core_wallet( + StandardAccountType::BIP44Account, + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let (core_b, signer_b, outputs_b) = funded_core_wallet( + StandardAccountType::BIP44Account, + Arc::new(AlwaysOkBroadcaster), + ) + .await; + assert_ne!( + core_a.wallet_id(), + core_b.wallet_id(), + "the fixture must model two distinct wallets" + ); + + // Dispatch both wallets' single UTXO and sweep both funding + // reservations, so each fence is the only thing holding its input + // (see the sibling release tests). + let mut sent = Vec::new(); + for (core, signer, outputs) in [ + (&core_a, &signer_a, &outputs_a), + (&core_b, &signer_b, &outputs_b), + ] { + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(core, AccountTypePreference::BIP44, outputs, signer).await; + sent.push(finalized.transaction().clone()); + assert!(core + .broadcast_finalized_transaction(&finalized) + .await + .is_ok()); + advance_processed_height(core, stamped + 17_000).await; + expect_mid_broadcast( + try_finalize_tx(core, AccountTypePreference::BIP44, outputs, signer).await, + "the dispatched input must be fenced before any spend is observed", + ); + } + + let handler = SpendObservationHandler::new(wallets_map(&[&core_a, &core_b])); + + // An event naming a wallet id registered NOWHERE: the lookup misses, + // nothing panics, and neither fence moves — the fail-safe direction. + let mut foreign = spend_event(&core_a, &sent[0]); + match &mut foreign { + key_wallet_manager::WalletEvent::TransactionDetected { wallet_id, .. } => { + *wallet_id = [0xEE; 32]; + } + other => unreachable!("the fixture builds TransactionDetected, got {other:?}"), + } + dash_spv::EventHandler::on_wallet_event(&handler, &foreign); + for (core, signer, outputs) in [ + (&core_a, &signer_a, &outputs_a), + (&core_b, &signer_b, &outputs_b), + ] { + expect_mid_broadcast( + try_finalize_tx(core, AccountTypePreference::BIP44, outputs, signer).await, + "an event for an unregistered wallet must release no fence", + ); + } + + // Wallet A's own spend event: A's registered generation releases, + // B's — same map, same handler, different wallet id — stands. + dash_spv::EventHandler::on_wallet_event(&handler, &spend_event(&core_a, &sent[0])); + let rebuilt = try_finalize_tx(&core_a, AccountTypePreference::BIP44, &outputs_a, &signer_a) + .await + .unwrap_or_else(|error| { + panic!("the matching wallet's fence must release, got {error:?}") + }); + core_a.abandon_transaction(&rebuilt).await; + expect_mid_broadcast( + try_finalize_tx(&core_b, AccountTypePreference::BIP44, &outputs_b, &signer_b).await, + "the other registered wallet's fence must stand", + ); + } + + /// `dashpay/platform#4309`, REVIEW ROUND 7 — THE END-TO-END REGRESSION. + /// + /// The pending-spend phase used to expire one hour after the dispatch + /// settled, on a monotonic clock. The clock was the right kind — catch-up + /// cannot move it — but a deadline of ANY kind is the wrong instrument: the + /// signed transaction stays valid, and an hour passing proves nothing about + /// whether a peer retained it. A DAPI endpoint that accepts the transaction + /// while withholding it from the network, or an app backgrounded past the + /// deadline, was enough. With key-wallet's reservation also swept by + /// catch-up, the next build then re-selected the input and SIGNED A + /// CONFLICTING TRANSACTION over a spend that might still land. + /// + /// This drives that exact sequence through the real send path: accept the + /// transaction (`AlwaysOk`, and this manager runs no mempool pipeline — the + /// `DapiBroadcaster` shape, so nothing observes the spend), run catch-up far + /// past key-wallet's reservation TTL, bring due every timeout the fence might + /// carry, and build again. On the deadline-bearing revision that second build + /// SUCCEEDED and returned a second signed transaction spending the same + /// input. It must now be refused, and released only by the observed spend. + #[tokio::test] + async fn an_elapsed_deadline_cannot_retire_the_fence_a_spend_still_needs() { + let (core, signer, outputs) = funded_core_wallet( + StandardAccountType::BIP44Account, + Arc::new(AlwaysOkBroadcaster), + ) + .await; + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let fenced = finalized.transaction().input[0].previous_output; + + assert!(core + .broadcast_finalized_transaction(&finalized) + .await + .is_ok()); + + // Catch-up runs far past key-wallet's reservation TTL, so the funding + // reservation is swept and the input is selectable again as far as + // key-wallet is concerned. The fence is the only thing still holding it. + advance_processed_height(&core, stamped + 17_000).await; + expect_mid_broadcast( + try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await, + "chain progress must not retire the fence", + ); + + // Now let every elapsed-time release the fence might carry come due — + // the hour of wall clock the old backstop waited out. + assert!( + core.generation().test_elapse_time_based_release(&fenced), + "the accepted dispatch must be in the pending-spend phase" + ); + + assert_eq!( + expect_mid_broadcast( + try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await, + "an elapsed deadline must not hand back an input whose transaction \ + may still be on the wire — the old backstop let this build sign a \ + conflicting spend of it", + ), + fenced, + ); + + // The one release that carries evidence. + core.generation().observe_spent([fenced]); + let after = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let after = after.unwrap_or_else(|error| { + panic!("an observed spend must release the fence, got {error:?}") + }); + core.abandon_transaction(&after).await; + } + + /// `dashpay/platform#4309`, the CANCELLATION path. + /// + /// A caller wrapping the send in `timeout`/`select!` drops the dispatching + /// future mid-`broadcast`. That path reaches neither the release nor any + /// return value, and cancellation proves nothing: DAPI may have delivered + /// the request while awaiting its response, SPV may have dispatched to + /// peers while awaiting an echo or IS-lock. So the fence must survive it — + /// and, unlike in earlier revisions, it needs no special case to do so: + /// `Drop` sets the same flag the normal path does, so a cancelled dispatch + /// settles exactly like a returning one. + /// + /// Catch-up runs far past any bound a previous revision would have + /// installed before the abort. + #[tokio::test] + async fn cancelled_dispatch_keeps_its_fence_across_catch_up() { + let entered = Arc::new(tokio::sync::Barrier::new(2)); + let release = Arc::new(tokio::sync::Barrier::new(2)); + let (core, signer, outputs) = funded_core_wallet( + StandardAccountType::BIP44Account, + Arc::new(GatedBroadcaster { + entered: Arc::clone(&entered), + release: Arc::clone(&release), + }), + ) + .await; + let stamped = core + .last_processed_height() + .await + .expect("last processed height"); + let finalized = finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let fenced = finalized.transaction().input[0].previous_output; + + let dispatcher = tokio::spawn({ + let core = core.clone(); + async move { core.broadcast_finalized_transaction(&finalized).await } + }); + // Parked inside `broadcast`: pin held, guard dropped, nothing decided. + entered.wait().await; + + advance_processed_height(&core, stamped + 17_000).await; + + // Cancel mid-await, exactly as `timeout`/`select!` would. Awaiting the + // handle guarantees the future — and with it `InBroadcastPin::drop` — + // has actually run before the assertions below. + dispatcher.abort(); + let cancelled = dispatcher.await; + assert!( + cancelled.is_err_and(|error| error.is_cancelled()), + "the dispatching future must have been cancelled mid-broadcast" + ); + + assert_eq!( + expect_mid_broadcast( + try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await, + "a cancelled dispatch may have reached the network, so its fence \ + must survive — including across catch-up" + ), + fenced, + ); + + // A cancelled dispatch's fence is released the same way every other one + // is — by evidence, and by nothing that merely elapses. Letting any + // timeout it might carry come due changes nothing. + assert!( + core.generation().test_elapse_time_based_release(&fenced), + "the cancelled dispatch must have settled into the pending phase" + ); + expect_mid_broadcast( + try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await, + "cancellation says nothing about what reached the network, so no \ + elapsed deadline may hand the input back", + ); + + core.generation().observe_spent([fenced]); + let after = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let after = after.unwrap_or_else(|error| { + panic!("an observed spend must release a cancelled dispatch's fence, got {error:?}") + }); + core.abandon_transaction(&after).await; + } + + /// The rejection path is the one outcome that frees the inputs at dispatch + /// return: Core definitively did not accept the transaction, so there is + /// nothing on the wire to fence against and an immediate rebuild must + /// reselect. No pending-spend fence may be installed. + #[tokio::test] + async fn definitively_rejected_dispatch_installs_no_fence() { + let (core, signer, outputs) = funded_core_wallet( + StandardAccountType::BIP44Account, + Arc::new(RejectFirstBroadcaster::new()), + ) + .await; + let finalized = finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + + let sent = core.broadcast_finalized_transaction(&finalized).await; + assert!( + matches!(sent, Err(PlatformWalletError::TransactionBroadcast(_))), + "the fixture must reject the first send, got {sent:?}" + ); + + // Rejection released the reservation AND installed no fence, so the + // rebuild succeeds at the very next height with no waiting. + let rebuilt = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let rebuilt = rebuilt.unwrap_or_else(|error| { + panic!("a definitively rejected send must leave its inputs free, got {error:?}") + }); + core.abandon_transaction(&rebuilt).await; + } + /// A pre-send broadcast rejection must release the UTXO reservation taken /// while building the transaction, so an immediate retry can reselect those /// inputs instead of failing with spurious insufficient funds until the TTL diff --git a/packages/rs-platform-wallet/src/wallet/core/generation.rs b/packages/rs-platform-wallet/src/wallet/core/generation.rs index 5d70488443a..fa451ea918a 100644 --- a/packages/rs-platform-wallet/src/wallet/core/generation.rs +++ b/packages/rs-platform-wallet/src/wallet/core/generation.rs @@ -1,9 +1,13 @@ //! Per-wallet-*generation* shared state: the identity marker every handle to -//! one generation shares, and that generation's lifecycle gate. +//! one generation shares, that generation's lifecycle gate, and the +//! in-broadcast outpoint pins that fence a mid-dispatch transaction's inputs +//! against concurrent re-selection. +use std::collections::HashMap; use std::ops::Deref; -use std::sync::Arc; +use std::sync::{Arc, Mutex, MutexGuard, PoisonError}; +use dashcore::{OutPoint, Transaction}; use tokio::sync::{OwnedRwLockWriteGuard, RwLock, RwLockReadGuard}; use super::balance::WalletBalance; @@ -59,6 +63,307 @@ pub struct WalletGeneration { /// a retry loop, and the guard must outlive the loop iteration that produced /// the `Arc` it came from. lifecycle: Arc>, + /// Outpoints currently fenced against re-selection because a broadcast + /// dispatch owns them ([`pin_in_broadcast`](Self::pin_in_broadcast)). + /// + /// The guarded dispatch (`CoreWallet::dispatch_unexpired`) proves under the + /// wallet-manager read guard that a finalized transaction's funding + /// reservation is still its own, then must release that guard before the + /// broadcaster await (holding it starves the SPV mempool pipeline). The + /// broadcaster can suspend *before* submission, and in that gap sync + /// catch-up can advance `last_processed_height` far enough that key-wallet's + /// `ReservationSet` TTL sweeps the reservation and a concurrent build + /// re-reserves the very same inputs — the dispatch would then put an + /// already-signed transaction on the wire against inputs reassigned to + /// another payment. This map is the fence that outlives the dropped guard: + /// every coin-selection choke point (`CoreWallet::finalize_transaction`, the + /// contact-payment build, the asset-lock build) checks its freshly reserved + /// selection against it — still under the manager write lock, the same + /// synchronization height advancement and the TTL sweep run under — and + /// refuses a build whose selection picked a fenced input. + /// + /// # Two phases, because dispatch return is not "the spend is safe" + /// + /// [`InBroadcastFence`] holds both phases per outpoint: + /// + /// * **dispatching** — a counted, never-expiring pin, live from + /// check-and-pin until the broadcaster returns. + /// * **pending-spend** — installed when the broadcaster returns anything + /// other than a definitive pre-send rejection, i.e. when the transaction + /// may be on the network. It is released when the wallet OBSERVES the + /// outpoint spent ([`observe_spent`](Self::observe_spent)), and by + /// nothing else. It does not expire. + /// + /// The second phase exists because dispatch returning does not mean the + /// wallet has observed the spend. `SpvBroadcaster` injects the transaction + /// into dash-spv's local mempool pipeline, so its inputs leave this wallet's + /// selectable set within milliseconds — but `DapiBroadcaster::broadcast` only + /// awaits `sdk.execute` and performs no local injection at all, so both an + /// accepted response and an ambiguous `MaybeSent` return with the input still + /// selectable here while the transaction is in flight. Dropping the fence at + /// dispatch return would therefore reopen, on the DAPI path, exactly the + /// sweep + re-select race the pin was added to close + /// (`dashpay/platform#4309`). + /// + /// # The pending-spend phase ends on EVIDENCE, and on nothing else + /// + /// **The invariant: no quantity that merely ELAPSES may retire this + /// phase.** Not chain height, and not wall-clock time either. Four earlier + /// revisions violated it — three bounded the phase at `height + N` blocks + /// and argued only about *which* height to anchor on (the pre-send check's, + /// a post-await sample, a post-await sample installed under one manager + /// guard); the fourth replaced that with a one-hour monotonic deadline. The + /// height forms were unsound because `last_processed_height` is not a clock + /// during catch-up: the wallet can advance it by thousands of blocks in + /// seconds, and every one of those blocks was mined BEFORE the transaction + /// was submitted, so an ordinary historical sync consumed the whole + /// interval (`dashpay/platform#4309`, review round 5). + /// + /// The monotonic deadline fixed the wrong half of that. Making the clock + /// unfast-forwardable does not make elapsed time evidence, and the fence + /// needs evidence: a signed transaction does not become invalid by getting + /// older, and no amount of waiting proves no peer retained it. A malicious + /// or isolated DAPI endpoint can accept the transaction while withholding + /// it from this wallet and from the network, and a mobile wallet can sit + /// backgrounded far longer than any deadline worth setting. Once the + /// deadline lapses and catch-up has also swept key-wallet's reservation, + /// the next build prunes the fence and signs a CONFLICTING transaction — + /// and the retained original can still be broadcast afterwards, so either + /// user intent can win the double-spend race (`dashpay/platform#4309`, + /// review round 7). + /// + /// So there is no deadline at all. The pending-spend phase is released by + /// exactly one thing — the wallet observing the outpoint spent, which is + /// positive evidence that the race the fence exists to prevent can no + /// longer happen: + /// + /// * the dispatch's own transaction is seen in the mempool or in a block — + /// the spend the fence was protecting has landed; or + /// * a competing transaction spends the outpoint — the outpoint is gone + /// from this wallet's selectable set regardless, so there is nothing left + /// to fence. + /// + /// [`observe_spent`](Self::observe_spent) is driven from the same + /// spend-processing path that feeds + /// [`CoreChangeSet::spent_utxos`](crate::changeset::CoreChangeSet), so the + /// fence and the persisted spent set agree on what "spent" means by + /// construction. + /// + /// # What the missing deadline costs, and why that is the right trade + /// + /// A fence whose transaction is never observed at all — evicted for fee, or + /// conflicted away without this wallet seeing the conflict — holds its + /// inputs for the rest of the process. That is deliberate. Those inputs are + /// exactly the ones a signed, possibly-live transaction spends, and the + /// alternative to holding them is signing a second transaction that spends + /// them too. + /// + /// The cost is bounded and cheap: the map is per generation, never + /// persisted (after a restart nothing is mid-dispatch, and a transaction + /// that actually landed is reconciled by sync), and grows only with the + /// outpoints this process has actually dispatched. A stuck fence is also + /// self-limiting in practice — the transaction it protects is either + /// eventually relayed back, mined, or conflicted, and all three arrive here + /// as an observed spend. + /// + /// The two additive shapes that could shorten the wait are LIVENESS paths, + /// not timeouts: persist the pending transaction and query or rebroadcast + /// it, or take an explicit caller-driven abandon/replacement declaration. + /// Both end the phase on a statement about *this transaction*. Neither is + /// implemented here, and neither may be replaced by a bound that simply + /// runs out. + /// + /// A *count* for the dispatching phase rather than a set: + /// `broadcast_finalized_transaction` takes `&SignedCoreTransaction`, so a + /// direct Rust caller can dispatch the same transaction twice concurrently + /// (idempotent on the wire — same txid). Counting keeps the pin held until + /// the LAST dispatch returns instead of letting the first completion unpin + /// the other's in-flight send. + /// + /// A `std::sync::Mutex` like key-wallet's own `ReservationSet`: critical + /// sections are a few hash operations, never held across an await, and the + /// sync lock is what lets [`InBroadcastPin::drop`] settle the fence from a + /// plain (non-async) `Drop` — which is also what makes the pin + /// cancellation-safe when the dispatching future is dropped mid-await. + /// + /// # Scoped to the WALLET, not to this generation + /// + /// Held behind a shared [`InBroadcastFences`] `Arc` that + /// [`PlatformWalletManager`](crate::PlatformWalletManager) keys by + /// `wallet_id` and hands to every generation registered under that id, so + /// removing a wallet and re-creating it under the same id inherits the + /// pending spends rather than starting clean + /// (`dashpay/platform#4309`, review round 8). + /// + /// The balance and the lifecycle gate above genuinely describe *this* + /// instance, and must not cross a recreation. A fence does not: it + /// describes a signed transaction that may be live on the network, and a + /// transaction does not become invalid because the wallet object holding + /// its record was replaced. A DAPI endpoint or peer that retained it can + /// still relay it afterwards, so a generation-local fence let the + /// re-created wallet restore the persisted UTXO — with neither the fence + /// nor key-wallet's memory-only reservation on it — and sign a conflicting + /// spend. Inheritance is strictly the conservative direction: fences are + /// still retired only by [`observe_spent`](Self::observe_spent), and an + /// observation on the new generation clears what the old one installed + /// because both name the same map. + /// + /// This closes the in-process half. The map is still not PERSISTED, so a + /// process restart loses it; see the module-level note on + /// `InBroadcastFences` for what closing that half requires. + in_broadcast: Arc, + /// Test-only one-shot hook fired at the dispatching→pending midpoint — + /// see [`WalletGeneration::on_next_settle_boundary`]. + #[cfg(test)] + settle_boundary_hook: SettleBoundaryHook, +} + +/// Holder for the test-only settle-boundary hook. +/// +/// A newtype purely so [`WalletGeneration`] can keep its derived [`Debug`]: +/// `Box` has none. +#[cfg(test)] +#[derive(Default)] +struct SettleBoundaryHook(Mutex>>); + +#[cfg(test)] +impl std::fmt::Debug for SettleBoundaryHook { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("SettleBoundaryHook(..)") + } +} + +/// One wallet's in-broadcast fence map, shared by every +/// [`WalletGeneration`] ever registered under that wallet's id. +/// +/// Owning the map here rather than inside `WalletGeneration` is what lets a +/// pending spend outlive the instance that dispatched it: a remove-and-recreate +/// under the same id mints a fresh generation but hands it this same `Arc`, so +/// the still-valid signed transaction's inputs stay fenced +/// (`dashpay/platform#4309`, review round 8). See the +/// `WalletGeneration::in_broadcast` field docs for the full argument. +/// +/// # Not yet durable +/// +/// The manager's registry is process-lifetime, so this closes recreation but +/// NOT a process restart: a fresh process loads the persisted UTXO with no +/// fence on it. Closing that half needs the pending transaction itself to be +/// durable — either recorded locally at dispatch, the way the SPV path already +/// is via dash-spv's mempool injection (which would remove the input from the +/// persisted UTXO set through the existing `CoreChangeSet::records` / +/// `spent_utxos` fields, so no new persistence surface is needed), or written +/// to a dedicated pending-spend table and rehydrated here before spending is +/// enabled. Both change host-visible state and belong in their own change; the +/// invariant this map must keep in the meantime is unchanged — nothing that +/// merely ELAPSES may retire a fence. +#[derive(Debug, Default)] +pub(crate) struct InBroadcastFences { + fences: Mutex>, +} + +impl InBroadcastFences { + /// Recovers from a poisoned mutex rather than panicking — see + /// [`WalletGeneration::in_broadcast_lock`]. + fn lock(&self) -> MutexGuard<'_, HashMap> { + self.fences.lock().unwrap_or_else(PoisonError::into_inner) + } +} + +/// One outpoint's broadcast fence — see `WalletGeneration::in_broadcast`. +#[derive(Debug, Default)] +struct InBroadcastFence { + /// Dispatches currently *inside* the broadcaster await for this outpoint. + /// Never expires while non-zero: a suspended dispatch keeps its inputs + /// fenced no matter what else happens. + dispatching: u32, + /// A dispatch has handed this outpoint to the network and the wallet has + /// not yet observed it spent — the PENDING-SPEND phase. + /// + /// A plain flag, deliberately: not a deadline, not a height, not anything + /// that can come due. Only [`WalletGeneration::observe_spent`] clears it. + /// See the `WalletGeneration::in_broadcast` field docs for why every bound + /// tried here — three chain-derived, one monotonic — was unsound. + pending: bool, + /// The wallet has OBSERVED this outpoint spent + /// ([`WalletGeneration::observe_spent`]). Retires the pending-spend phase + /// and suppresses re-installation by a dispatch of the same transaction + /// that is still inside its broadcaster await — the SPV path routinely + /// observes the spend before `broadcast` returns, and re-fencing an + /// already-spent outpoint would leave a dead entry in the map that nothing + /// could ever clear. + observed_spent: bool, +} + +impl InBroadcastFence { + /// Whether this fence still blocks re-selection. + /// + /// Takes NO clock of any kind — no height, and (since review round 7) no + /// [`Instant`](std::time::Instant) either. A fence is held while a dispatch + /// is in flight or its transaction may be on the network, and is released + /// only by evidence ([`WalletGeneration::observe_spent`]). Nothing elapses + /// (`dashpay/platform#4309`). + fn blocks(&self) -> bool { + self.dispatching > 0 || self.pending + } + + /// Open the pending-spend phase. + /// + /// A no-op once the spend has been observed: the evidence that retires the + /// phase must not be undone by a slower concurrent dispatch of the same + /// transaction settling afterwards. + /// + /// Idempotent, and there is nothing left to order between two concurrent + /// dispatches of the same transaction. This used to install a deadline and + /// take care never to SHORTEN an existing one so both dispatches stayed + /// covered; with no deadline, one flag covers both by construction. + fn open_pending(&mut self) { + if self.observed_spent { + return; + } + self.pending = true; + } + + /// Record that the wallet observed this outpoint spent and retire the + /// pending-spend phase. + /// + /// The dispatching count is untouched: it tracks live `InBroadcastPin`s, + /// not chain state, and a pin must end at its own drop or the count leaks. + fn observe_spent(&mut self) { + self.observed_spent = true; + self.pending = false; + } + + /// Whether nothing holds this outpoint any more, so the entry can be + /// dropped from the map. + fn is_clear(&self) -> bool { + self.dispatching == 0 && !self.pending + } +} + +/// How one dispatch's pending-spend phase settles when its [`InBroadcastPin`] +/// is dropped — see [`WalletGeneration::pin_in_broadcast`]. +/// +/// The INITIAL value is the least-informed one: a pin that learns nothing +/// before it drops must fence (`dashpay/platform#4309`). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +enum PendingSpendSettle { + /// The transaction may be on the network — the broadcaster returned + /// something other than a definitive pre-send rejection, or the dispatch + /// stopped without returning at all (cancelled or unwound mid-`broadcast`). + /// Both open the pending-spend phase, which then waits for an observed + /// spend. + /// + /// The two cases need no distinction any more. When the phase carried a + /// height-derived bound they did: a cancelled dispatch had no post-await + /// sample to anchor on, so it had to fence unanchored and borrow a later + /// selection's clock. With no bound to anchor there is nothing to sample, + /// and a `Drop` that can reach neither a lock nor an await settles + /// identically to a normal return. + #[default] + Pending, + /// A definitive pre-send rejection — the one outcome that proves the + /// transaction never reached the network. No pending-spend phase at all. + Released, } impl Default for WalletGeneration { @@ -68,11 +373,33 @@ impl Default for WalletGeneration { } impl WalletGeneration { - /// A fresh generation: zeroed balance, uncontended gate. + /// A fresh generation with fences of its own: zeroed balance, uncontended + /// gate, nothing pinned. + /// + /// Production registration and load go through + /// [`with_fences`](Self::with_fences) instead, so a generation replacing + /// another under the same `wallet_id` inherits its pending spends. This + /// form is for a wallet with no predecessor — and for tests that want an + /// isolated map. pub fn new() -> Self { + Self::with_fences(Arc::new(InBroadcastFences::default())) + } + + /// A fresh generation sharing `fences` with every other generation of the + /// same wallet. + /// + /// The balance and the lifecycle gate are per generation — they describe + /// *this* instance. The fence map is not: it describes signed transactions + /// that may be live on the network, and those outlive the instance that + /// dispatched them (`dashpay/platform#4309`, review round 8). See the + /// [`in_broadcast`](Self#structfield.in_broadcast) field docs. + pub(crate) fn with_fences(fences: Arc) -> Self { Self { balance: WalletBalance::new(), lifecycle: Arc::new(RwLock::new(())), + in_broadcast: fences, + #[cfg(test)] + settle_boundary_hook: SettleBoundaryHook::default(), } } @@ -127,6 +454,453 @@ impl WalletGeneration { pub async fn teardown_guard(&self) -> OwnedRwLockWriteGuard<()> { Arc::clone(&self.lifecycle).write_owned().await } + + /// Recovers from a poisoned mutex rather than panicking: the guarded data + /// is a plain count map with no invariant a partial write could break, and + /// panicking here would strand every later build and dispatch on this + /// generation. (Same policy as key-wallet's `ReservationSet`.) + fn in_broadcast_lock(&self) -> MutexGuard<'_, HashMap> { + self.in_broadcast.lock() + } + + /// Pin `transaction`'s inputs as **in-broadcast** until the returned + /// [`InBroadcastPin`] is dropped. + /// + /// Taken by the guarded dispatch (`CoreWallet::dispatch_unexpired`) while + /// it still holds the wallet-manager READ guard that proved the funding + /// reservation fresh — the freshness bound sits strictly below key-wallet's + /// reservation TTL on the same `last_processed_height` clock, and both the + /// TTL sweep and height advancement mutate under the manager WRITE lock, so + /// under that guard the reservation is provably still this build's: that + /// proof is the pin's owner check, and installing the pin before the guard + /// drops makes check-and-pin one atomic step. The pin then *outlives* the + /// guard, deliberately: it is what keeps the check meaningful across the + /// broadcaster await the guard must not span (see the + /// [`in_broadcast`](Self::in_broadcast) field docs for the full race). + /// + /// The dispatching phase has **no TTL** — a suspended dispatch keeps its + /// inputs fenced no matter how long it takes — and ends only when the + /// returned guard is dropped, which happens even when the dispatching + /// future is cancelled mid-await (`Drop` runs on unwind and on future drop + /// alike). + /// + /// # No height is taken here, and none is taken later either + /// + /// This call takes NO `last_processed_height`, and neither does the settle + /// that follows it. Chain height cannot bound this fence at all: catch-up + /// advances it over blocks mined before the transaction was ever submitted, + /// so any `height + N` bound can be consumed by an ordinary historical sync + /// without a single piece of evidence about the dispatch + /// (`dashpay/platform#4309`). The pending-spend phase ends when the wallet + /// OBSERVES the outpoint spent ([`observe_spent`](Self::observe_spent)), + /// and there is no fallback bound of any other kind either — a wall clock + /// the chain cannot move is still not evidence about this transaction + /// (review round 7). Accepting no clock at either end makes the + /// mis-anchoring unrepresentable rather than merely corrected. + /// + /// Callers pin on the generation currently REGISTERED in the manager + /// (`PlatformWalletInfo::generation`), the same object the build-side + /// conflict checks read, so the fence works even for a dispatch through a + /// stale-generation handle. + pub(crate) fn pin_in_broadcast(self: &Arc, transaction: &Transaction) -> InBroadcastPin { + let outpoints: Vec = transaction + .input + .iter() + .map(|input| input.previous_output) + .collect(); + { + let mut pinned = self.in_broadcast_lock(); + for outpoint in &outpoints { + pinned.entry(*outpoint).or_default().dispatching += 1; + } + } + InBroadcastPin { + generation: Arc::clone(self), + outpoints, + // Fenced by default: a pin that learns nothing before it drops must + // still hold the inputs. Only a definitive pre-send rejection + // narrows this. See the `InBroadcastPin` type docs + // (`dashpay/platform#4309`). + settle: PendingSpendSettle::Pending, + } + } + + /// The first of `transaction`'s inputs that is currently fenced by a + /// broadcast dispatch, or `None` when the selection is clear. + /// + /// Called by every coin-selection choke point immediately after it built + /// and reserved a selection, while it still holds the wallet-manager WRITE + /// guard: a hit means this build's own selection swept an aged reservation + /// whose transaction is mid-dispatch (or already handed to the network) and + /// re-reserved its input — completing the build would race that transaction + /// on the wire, so the caller must release its fresh reservation (exact + /// under the still-held write guard) and refuse the build. In the normal + /// case a fenced input is still *reserved* and never reaches selection at + /// all; this check is the backstop for exactly the post-sweep window. + /// + /// # No height parameter, deliberately + /// + /// This used to take the caller's `last_processed_height` and reap every + /// fence the chain had advanced past. That is the defect: during catch-up + /// the wallet advances that height over blocks mined BEFORE the dispatch, + /// so an ordinary historical sync completing between a dispatch and this + /// call could retire a fence protecting a transaction that had just gone to + /// the network (`dashpay/platform#4309`, review round 5). The fence now + /// answers to observed spends ALONE — no chain clock, and no wall clock + /// either (review round 7) — so this call retires nothing by consulting it. + /// + /// Cleared entries are reaped here rather than by a timer: this is the only + /// place the fence is consulted, so pruning on read keeps the map free of + /// entries nothing holds without any background task. It is only a tidy-up + /// — [`observe_spent`](Self::observe_spent) already removes what it clears, + /// and a fence that still blocks is never pruned here for any reason. + pub(crate) fn in_broadcast_conflict(&self, transaction: &Transaction) -> Option { + let mut pinned = self.in_broadcast_lock(); + pinned.retain(|_, fence| fence.blocks()); + transaction + .input + .iter() + .map(|input| input.previous_output) + .find(|outpoint| pinned.contains_key(outpoint)) + } + + /// Release the pending-spend fence on every outpoint in `outpoints` that + /// this wallet has just OBSERVED spent. + /// + /// This is the fence's real release path — the one that carries evidence. + /// It is driven off the wallet-event fan-out by + /// [`SpendObservationHandler`](super::SpendObservationHandler), from the + /// same per-record input walk that feeds + /// [`CoreChangeSet::spent_utxos`](crate::changeset::CoreChangeSet), so + /// "the fence considers this spent" and "the persister removes this UTXO" + /// are the same fact by construction. + /// + /// Both spend shapes are a release, and for the same reason — after either + /// one there is no longer a selectable outpoint whose re-selection could + /// race a transaction on the wire: + /// + /// * **the dispatch's own transaction**, seen in the mempool or in a block. + /// This is the overwhelmingly common case and the one the fence was + /// waiting for. + /// * **a competing transaction** spending the same outpoint. The outpoint + /// leaves this wallet's UTXO set either way, so continuing to fence it + /// would protect nothing and hold the input for good. + /// + /// Idempotent, and safe for outpoints this generation never fenced — + /// block processing hands over every spend it sees, the vast majority of + /// which have nothing to do with any dispatch. + /// + /// Takes only the `in_broadcast` `std::sync::Mutex` for a few hash + /// operations and never awaits, so it is safe to call from a synchronous + /// event handler running inside SPV's block-processing write section. + pub(crate) fn observe_spent(&self, outpoints: impl IntoIterator) { + let mut pinned = self.in_broadcast_lock(); + for outpoint in outpoints { + let Some(fence) = pinned.get_mut(&outpoint) else { + continue; + }; + fence.observe_spent(); + if fence.is_clear() { + pinned.remove(&outpoint); + } + } + } + + /// End one dispatch's hold on `outpoints` — the [`InBroadcastPin`] release + /// half of [`pin_in_broadcast`](Self::pin_in_broadcast). + /// + /// `settle` says what that dispatch proved: + /// + /// * [`PendingSpendSettle::Pending`] — the transaction may be on the + /// network (any non-rejection outcome, or a cancelled/unwound dispatch + /// that returned nothing at all). The dispatching count drops and the + /// pending-spend phase opens, to be released by an observed spend and by + /// nothing else. + /// * [`PendingSpendSettle::Released`] — a definitive pre-send rejection, + /// which frees the outpoint immediately: the transaction is provably not + /// on the wire, and the caller releases its reservation in the same breath + /// so an immediate rebuild can reselect. + /// + /// # One critical section, so the handoff is never observable half-done + /// + /// Lifting the dispatching hold and opening the pending-spend phase happen + /// under a single `in_broadcast` lock acquisition. There is no clock to + /// read and no guard to release in between, so no observer can catch this + /// outpoint in the torn state — `dispatching` already lifted, pending-spend + /// not yet open — that would make it briefly selectable. Earlier revisions + /// sampled a `last_processed_height` from the wallet-manager lock and had + /// to hold that guard across the install to get the same property + /// (`dashpay/platform#4309`, review round 4); setting a flag needs no guard + /// at all. + /// + /// [`Self::settle_boundary_hook`] fires at exactly that midpoint under + /// `cfg(test)` — after the first outpoint's dispatching hold is lifted and + /// before its pending phase opens, i.e. inside the torn state itself, not + /// merely after the lock is acquired. A hook that fired on lock + /// acquisition would be satisfied by the first half of a split + /// implementation too; fired here, only a critical section that spans + /// both halves keeps the boundary unobservable + /// (`dashpay/platform#4309`, review round 6). + fn unpin_in_broadcast(&self, outpoints: &[OutPoint], settle: PendingSpendSettle) { + let mut pinned = self.in_broadcast_lock(); + for outpoint in outpoints { + let Some(fence) = pinned.get_mut(outpoint) else { + // Unreachable by construction — every pin inserts before its + // guard can remove — but a miscount must not panic a Drop. + debug_assert!(false, "unpin of an outpoint that was never pinned"); + continue; + }; + fence.dispatching = fence.dispatching.saturating_sub(1); + // The dispatching→pending midpoint: this outpoint's dispatching + // hold is lifted, its pending phase is not yet open. One-shot, so + // in effect it fires at the first outpoint's midpoint. + #[cfg(test)] + self.fire_settle_boundary_hook(); + match settle { + PendingSpendSettle::Pending => fence.open_pending(), + PendingSpendSettle::Released => {} + } + if fence.is_clear() { + pinned.remove(outpoint); + } + } + } + + /// `outpoint`'s raw fence state — `(dispatching, pending, observed_spent)` + /// — or `None` when nothing holds it. + /// + /// A test-only WINDOW ON THE TRANSITION, deliberately not + /// [`in_broadcast_conflict`](Self::in_broadcast_conflict): that call reaps + /// as a side effect, so it cannot report whether a dispatch's + /// dispatching→pending handoff had completed at the moment it was observed. + #[cfg(test)] + pub(crate) fn in_broadcast_fence_state( + &self, + outpoint: &OutPoint, + ) -> Option<(u32, bool, bool)> { + self.in_broadcast_lock() + .get(outpoint) + .map(|fence| (fence.dispatching, fence.pending, fence.observed_spent)) + } + + /// Bring due whatever elapsed-time release `outpoint`'s fence still + /// carries, and report whether it is in the pending-spend phase. + /// + /// **It carries none**, which is the point. Under the current design the + /// pending-spend phase has no deadline to bring due, so this call mutates + /// nothing at all and only answers "is this outpoint pending?". + /// + /// Kept — and kept callable — because it is the harness the round-7 + /// regressions are written against, and it means the same thing in both + /// designs: *let every timeout this fence might have expire, then look*. + /// Against the deadline-bearing implementation the same call retired the + /// fence and the next [`in_broadcast_conflict`](Self::in_broadcast_conflict) + /// handed the input back for re-selection; against this one the fence + /// stands until an observed spend. Those two outcomes are exactly what + /// `the_pending_fence_outlives_any_elapsed_deadline` and + /// `an_elapsed_deadline_cannot_retire_the_fence_a_spend_still_needs` + /// discriminate (`dashpay/platform#4309`). + #[cfg(test)] + pub(crate) fn test_elapse_time_based_release(&self, outpoint: &OutPoint) -> bool { + self.in_broadcast_lock() + .get(outpoint) + .is_some_and(|fence| fence.pending) + } + + /// Run `hook` at the dispatching→pending midpoint of the very next + /// [`unpin_in_broadcast`](Self::unpin_in_broadcast) on this generation: + /// after an outpoint's dispatching hold is lifted, before its pending + /// phase is opened — the torn state itself. + /// + /// The test-only synchronization hook that makes the handoff regression + /// DETERMINISTIC (`dashpay/platform#4309`, review round 5 suggestion). The + /// previous regression parked a writer and hoped the scheduler granted it + /// the lock inside a window a handful of instructions wide, so it stayed + /// green against the pre-fix code. With this hook the observer is run at + /// the midpoint by construction, and what it can see there is the whole + /// assertion. + /// + /// The firing point matters (round 6): fired on lock ACQUISITION, the + /// observation would complete before any fence was touched, so an + /// implementation that split the decrement and the pending install into + /// separate critical sections — the regression under test — would satisfy + /// it with its first section alone. Fired between the two operations, the + /// observation is protected only if one critical section spans both. + /// + /// One-shot: consumed by the settle that fires it, so an unrelated later + /// settle cannot re-enter the test's handshake. + #[cfg(test)] + pub(crate) fn on_next_settle_boundary(&self, hook: Box) { + *self + .settle_boundary_hook + .0 + .lock() + .unwrap_or_else(PoisonError::into_inner) = Some(hook); + } + + /// A NON-BLOCKING look at `outpoint`'s fence, for an observer that must + /// distinguish "the transition is in progress" from "the outpoint is free". + /// + /// A blocking read cannot make that distinction: correct code holds the + /// `in_broadcast` lock across the whole dispatching→pending handoff, so an + /// observer that simply waits for the lock always sees the finished state + /// and can never tell whether it was granted mid-transition or after it. + /// Probing with `try_lock` turns "held" into an observable outcome, which is + /// exactly the invariant the deterministic handoff regression asserts + /// (`dashpay/platform#4309`, review round 5). + #[cfg(test)] + pub(crate) fn try_probe_in_broadcast(&self, outpoint: &OutPoint) -> InBroadcastProbe { + match self.in_broadcast.fences.try_lock() { + Err(std::sync::TryLockError::WouldBlock) => InBroadcastProbe::TransitionInProgress, + Err(std::sync::TryLockError::Poisoned(poisoned)) => { + Self::probe_entry(poisoned.into_inner().get(outpoint)) + } + Ok(pinned) => Self::probe_entry(pinned.get(outpoint)), + } + } + + #[cfg(test)] + fn probe_entry(fence: Option<&InBroadcastFence>) -> InBroadcastProbe { + match fence { + Some(fence) if fence.blocks() => InBroadcastProbe::Fenced, + _ => InBroadcastProbe::Free, + } + } + + /// Take and run a hook armed by [`Self::on_next_settle_boundary`], if any. + /// + /// Called with the `in_broadcast` lock HELD, which is the point: an + /// observer that tries to read the fence from another thread while this + /// runs must find the lock held rather than a half-applied transition. + #[cfg(test)] + fn fire_settle_boundary_hook(&self) { + let hook = self + .settle_boundary_hook + .0 + .lock() + .unwrap_or_else(PoisonError::into_inner) + .take(); + if let Some(hook) = hook { + hook(); + } + } +} + +/// What [`WalletGeneration::try_probe_in_broadcast`] saw. +#[cfg(test)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum InBroadcastProbe { + /// The `in_broadcast` lock was held — a settle (or a conflict check) is + /// mid-flight, so the outpoint cannot be selected by anyone right now. + TransitionInProgress, + /// The outpoint carries a live fence. + Fenced, + /// Nothing holds the outpoint: a build could select it. + Free, +} + +/// RAII guard for one dispatch's in-broadcast input fence — see +/// [`WalletGeneration::pin_in_broadcast`]. Dropping it (normal return, +/// unwind, or the dispatching future being cancelled mid-await) ends exactly +/// the dispatching hold that call took, count-wise, never another dispatch's. +/// +/// The drop fences the outpoints by DEFAULT, as a pending spend. Only +/// [`settle_released`](Self::settle_released) — called on a definitive pre-send +/// rejection, the one outcome that PROVES the transaction is not on the wire — +/// frees them outright. +/// +/// The default is deliberately the conservative one (`dashpay/platform#4309`). +/// This guard's drop runs on paths that carry no information about whether the +/// transaction was sent: the dispatching future cancelled mid-await, an unwind, +/// or a suspension inside the broadcaster before submission. Treating those +/// like a rejection — the previous behaviour — frees inputs that may already be +/// spent on the network, so an immediate reselection double-spends them. Absence +/// of evidence that a send happened is not evidence that it did not, so the +/// fence must survive every exit except the one that proves otherwise. +/// +/// # The pending-spend phase waits for evidence, not for a bound to run out +/// +/// Fencing by default is only half of it. Three earlier revisions paired that +/// default with a `last_processed_height + N` bound and argued about where to +/// sample the height; all three could be consumed by an ordinary historical +/// catch-up, because those elapsed blocks were mined before the transaction was +/// submitted and say nothing about it (`dashpay/platform#4309`, review round 5). +/// A fourth swapped the height for a one-hour monotonic deadline, which fails +/// the same way for the same reason: a signed transaction does not expire, so +/// an hour of a clock no one can fast-forward is still not evidence that its +/// inputs are safe to spend again (review round 7). +/// +/// The pending-spend phase ends when the wallet OBSERVES the outpoint spent +/// ([`WalletGeneration::observe_spent`]), and nothing else ends it. That is +/// readable without a lock, a guard or an await, so EVERY exit — normal return, +/// cancellation, unwind — settles the same way. The cancellation path needs no +/// special case at all any more. +pub(crate) struct InBroadcastPin { + generation: Arc, + outpoints: Vec, + /// How the pending-spend phase settles on drop. Starts + /// [`PendingSpendSettle::Pending`] — the conservative state — and is + /// narrowed only by an explicit [`settle_released`](Self::settle_released). + settle: PendingSpendSettle, +} + +impl InBroadcastPin { + /// End the dispatching phase and open the pending-spend fence, which then + /// waits for [`WalletGeneration::observe_spent`] and for nothing else — it + /// carries no deadline. + /// + /// # Why this takes no height, and no guard + /// + /// It used to take a `last_processed_height` sampled after the broadcaster + /// returned, from a wallet-manager guard the caller had to keep held across + /// the call so no writer could advance the clock between the sample and the + /// install. A later revision swapped that height for a monotonic + /// `Instant::now` deadline read inside the `in_broadcast` critical section. + /// + /// Both are gone, and so is the bound they computed. What this installs is + /// a plain flag: there is no clock to sample, so no guard to hold and no + /// window to protect. The phase it opens ends on an observed spend + /// (`dashpay/platform#4309`). + /// + /// # This is equivalent to just dropping the pin + /// + /// Kept as an explicit consuming call because it states the dispatch's + /// verdict at the call site, symmetrically with + /// [`settle_released`](Self::settle_released) — which is the one that + /// actually differs from the default. Not calling either is always SAFE: + /// [`Drop`] settles exactly this way, which is what makes the cancellation + /// and unwind paths correct without a special case. + pub(crate) fn settle_pending_spend(self) { + drop(self); + } + + /// Free the outpoints outright, consuming the pin, instead of leaving the + /// pending-spend fence it installs by default. + /// + /// Call ONLY on a definitive pre-send failure — an outcome that proves the + /// transaction never reached the network, so there is nothing on the wire + /// to fence against and an immediate retry may reselect the inputs. Two + /// shapes qualify: a definitive `BroadcastError::Rejected`, and an abort + /// taken on a build's own pre-broadcast path (a failed durability gate, a + /// drain-floor refusal) where the broadcaster was never reached at all. + /// An ambiguous outcome, a cancellation, or an unwind must NOT call this: + /// see the type docs and the `WalletGeneration::in_broadcast` field docs for + /// why dispatch return is not, by itself, safe. + /// + /// This is the ONLY narrowing of the pin's default. Everything else — an + /// accepted send, an ambiguous `MaybeSent`, a cancellation, an unwind — + /// leaves the pending-spend fence in place to await an observed spend. + pub(crate) fn settle_released(mut self) { + self.settle = PendingSpendSettle::Released; + drop(self); + } +} + +impl Drop for InBroadcastPin { + fn drop(&mut self) { + self.generation + .unpin_in_broadcast(&self.outpoints, self.settle); + } } impl Deref for WalletGeneration { @@ -136,3 +910,531 @@ impl Deref for WalletGeneration { &self.balance } } + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::{mpsc, Arc}; + + use dashcore::{OutPoint, Transaction, TxIn, Txid}; + + use super::{InBroadcastFences, InBroadcastProbe, WalletGeneration}; + + /// A minimal transaction spending exactly the given outpoints — the only + /// part of a transaction the pin machinery reads. + fn spending(outpoints: &[OutPoint]) -> Transaction { + Transaction { + version: 2, + lock_time: 0, + input: outpoints + .iter() + .map(|outpoint| TxIn { + previous_output: *outpoint, + ..Default::default() + }) + .collect(), + output: Vec::new(), + special_transaction_payload: None, + } + } + + fn outpoint(byte: u8, vout: u32) -> OutPoint { + OutPoint::new(Txid::from([byte; 32]), vout) + } + + /// Settle a pin the way a dispatch that may have reached the network does: + /// the broadcaster returned something other than a definitive rejection. + fn settle_dispatched(generation: &Arc, tx: &Transaction) { + generation.pin_in_broadcast(tx).settle_pending_spend(); + } + + /// A held pin flags every input of the pinned transaction — and only + /// those — and dropping a pin whose fence was explicitly RELEASED clears + /// the conflict. Release models the one outcome that proves nothing was + /// sent: a definitive pre-send rejection. + #[test] + fn pin_flags_inputs_until_dropped() { + let generation = Arc::new(WalletGeneration::new()); + let (a, b, unrelated) = (outpoint(1, 0), outpoint(1, 1), outpoint(2, 0)); + let pinned_tx = spending(&[a, b]); + + let pin = generation.pin_in_broadcast(&pinned_tx); + + assert_eq!(generation.in_broadcast_conflict(&spending(&[a])), Some(a)); + assert_eq!(generation.in_broadcast_conflict(&spending(&[b])), Some(b)); + assert_eq!( + generation.in_broadcast_conflict(&spending(&[unrelated, a])), + Some(a), + "the conflict is reported for whichever input is fenced" + ); + assert_eq!( + generation.in_broadcast_conflict(&spending(&[unrelated])), + None, + "an unrelated input is untouched by the pin" + ); + + pin.settle_released(); + assert_eq!( + generation.in_broadcast_conflict(&spending(&[a, b])), + None, + "a released pin frees its outpoints outright" + ); + } + + /// `dashpay/platform#4309`: dropping a pin WITHOUT a definitive rejection + /// keeps the fence. This is the cancellation / unwind / suspension path, + /// none of which proves the transaction failed to reach the network. + #[test] + fn dropping_an_unreleased_pin_keeps_the_fence() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(3, 0); + let tx = spending(&[a]); + + drop(generation.pin_in_broadcast(&tx)); + + assert_eq!( + generation.in_broadcast_conflict(&tx), + Some(a), + "an un-released pin must leave the outpoint fenced on drop" + ); + } + + /// THE HEADLINE PROPERTY (`dashpay/platform#4309`, review round 5). + /// + /// A pending-spend fence is not consulted against chain height at all, so + /// no amount of catch-up can retire it. Previous revisions bounded the + /// fence at `height + IN_BROADCAST_FENCE_BLOCKS` and every one of them lost + /// the fence to a historical sync that advanced the clock past the bound + /// over blocks mined BEFORE the dispatch. + /// + /// `in_broadcast_conflict` no longer takes a height, so this test states + /// the property the only way it can still be stated: the fence survives + /// unboundedly many consultations and any amount of elapsed chain, and only + /// an observation clears it. + #[test] + fn a_pending_fence_is_immune_to_chain_progress() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(4, 0); + let tx = spending(&[a]); + + settle_dispatched(&generation, &tx); + + // Stand in for an arbitrarily long catch-up: every build during it + // consults the fence, and each consultation also reaps. None of them + // may retire this entry. + for _ in 0..10_000 { + assert_eq!( + generation.in_broadcast_conflict(&tx), + Some(a), + "no number of selections — i.e. no amount of chain progress — \ + may retire a fence that has seen no observed spend" + ); + } + } + + /// The fence's real release: the wallet observes the outpoint spent by the + /// dispatch's OWN transaction. + #[test] + fn an_observed_spend_clears_the_fence() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(5, 0); + let tx = spending(&[a]); + + settle_dispatched(&generation, &tx); + assert_eq!(generation.in_broadcast_conflict(&tx), Some(a)); + + generation.observe_spent([a]); + + assert_eq!( + generation.in_broadcast_conflict(&tx), + None, + "observing the spend is what ends the fence" + ); + } + + /// A COMPETING spend clears the fence too, and for the same reason: after + /// it the outpoint is out of this wallet's selectable set, so there is no + /// re-selection left that could race anything on the wire. + #[test] + fn a_competing_spend_also_clears_the_fence() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(6, 0); + let ours = spending(&[a]); + + settle_dispatched(&generation, &ours); + + // A different transaction spending the same outpoint — the wallet sees + // it and hands the outpoint over as spent. + let competing = spending(&[a, outpoint(7, 0)]); + generation.observe_spent(competing.input.iter().map(|i| i.previous_output)); + + assert_eq!(generation.in_broadcast_conflict(&ours), None); + } + + /// Observing spends the fence never knew about is a harmless no-op — block + /// processing hands over every spend it sees, and almost none of them + /// belong to a dispatch. + #[test] + fn observing_unfenced_outpoints_is_a_no_op() { + let generation = Arc::new(WalletGeneration::new()); + let fenced = outpoint(8, 0); + let tx = spending(&[fenced]); + settle_dispatched(&generation, &tx); + + generation.observe_spent([outpoint(9, 0), outpoint(9, 1)]); + generation.observe_spent([]); + + assert_eq!( + generation.in_broadcast_conflict(&tx), + Some(fenced), + "unrelated observations must not disturb a live fence" + ); + } + + /// The DISPATCHING phase never expires and is never released by an + /// observation: it tracks a live `InBroadcastPin`, so it ends at that pin's + /// drop and nowhere else. An observation arriving mid-dispatch (the SPV + /// path routinely beats the broadcaster's return) still suppresses the + /// pending phase the settle would otherwise open. + #[test] + fn a_mid_dispatch_observation_suppresses_the_pending_phase() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(10, 0); + let tx = spending(&[a]); + + let pin = generation.pin_in_broadcast(&tx); + generation.observe_spent([a]); + + assert_eq!( + generation.in_broadcast_conflict(&tx), + Some(a), + "the dispatching hold outlives an observation — the pin is still live" + ); + + pin.settle_pending_spend(); + + assert_eq!( + generation.in_broadcast_conflict(&tx), + None, + "an already-observed spend must not be re-fenced by the settle, or \ + the map would carry a dead entry nothing could ever clear" + ); + assert_eq!( + generation.in_broadcast_fence_state(&a), + None, + "and the entry is reaped rather than left behind" + ); + } + + /// `dashpay/platform#4309`, REVIEW ROUND 7 — THE UNIT-LEVEL REGRESSION. + /// + /// The pending-spend fence used to carry a one-hour monotonic deadline, and + /// `in_broadcast_conflict` retired the fence on that deadline ALONE. Elapsed + /// time is not evidence: the signed transaction is still valid, and nothing + /// about an hour passing proves no peer retained it. A withholding DAPI + /// endpoint or an hour-backgrounded app was therefore enough to hand the + /// input back to the next build, which would sign a conflicting transaction + /// over inputs the original might still spend. + /// + /// `test_elapse_time_based_release` means "let every timeout this fence + /// might carry come due, then tell me whether it is pending". Against the + /// deadline-bearing implementation it retired the fence and the assertion + /// below failed; against this one there is no deadline to bring due, so the + /// fence stands and only the observed spend at the end releases it. + #[test] + fn the_pending_fence_outlives_any_elapsed_deadline() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(11, 0); + let tx = spending(&[a]); + + settle_dispatched(&generation, &tx); + assert_eq!(generation.in_broadcast_conflict(&tx), Some(a)); + + assert!( + generation.test_elapse_time_based_release(&a), + "the settled fence must be in the pending-spend phase" + ); + + assert_eq!( + generation.in_broadcast_conflict(&tx), + Some(a), + "elapsed time must NOT retire a pending fence: the signed transaction \ + is still valid and may still be on the wire, so re-selecting its \ + input would sign a double spend" + ); + + // Evidence — and only evidence — releases it. + generation.observe_spent([a]); + assert_eq!( + generation.in_broadcast_conflict(&tx), + None, + "an observed spend is what ends the pending phase" + ); + } + + /// A dispatching pin is not in the pending phase at all: it is held by a + /// live in-flight dispatch, which no release condition of any kind touches + /// until the pin drops. + #[test] + fn a_dispatching_pin_is_not_in_the_pending_phase() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(12, 0); + let tx = spending(&[a]); + + let pin = generation.pin_in_broadcast(&tx); + assert!( + !generation.test_elapse_time_based_release(&a), + "a dispatching pin has not opened the pending phase" + ); + assert_eq!(generation.in_broadcast_conflict(&tx), Some(a)); + + drop(pin); + } + + /// Two concurrent dispatches of the same transaction: the pin is COUNTED, + /// so the first completion must not unpin the second's in-flight send. + #[test] + fn pins_are_counted_per_outpoint() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(13, 0); + let tx = spending(&[a]); + + let first = generation.pin_in_broadcast(&tx); + let second = generation.pin_in_broadcast(&tx); + + first.settle_released(); + assert_eq!( + generation.in_broadcast_conflict(&tx), + Some(a), + "one dispatch's rejection must not free another's in-flight inputs" + ); + + second.settle_released(); + assert_eq!(generation.in_broadcast_conflict(&tx), None); + } + + /// Two dispatches of the same transaction settling in sequence: the second + /// settle must never UNDO the fence the first installed. This used to be a + /// statement about deadlines never being shortened; with no deadline the + /// property is simply that the phase stays open. + #[test] + fn a_second_settle_does_not_undo_the_first_fence() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(14, 0); + let tx = spending(&[a]); + + settle_dispatched(&generation, &tx); + assert_eq!( + generation.in_broadcast_fence_state(&a), + Some((0, true, false)), + "a settled fence is pending, with no dispatch still in flight" + ); + + // A second, later dispatch of the same transaction. + settle_dispatched(&generation, &tx); + assert_eq!( + generation.in_broadcast_fence_state(&a), + Some((0, true, false)), + "the second settle leaves the fence exactly as held — both \ + dispatches stay covered" + ); + assert_eq!(generation.in_broadcast_conflict(&tx), Some(a)); + } + + /// Fences are per WALLET, and a generation that replaces another under the + /// same id INHERITS them (`dashpay/platform#4309`, review round 8). + /// + /// This test used to assert the opposite — that a re-created wallet got a + /// fresh map — and that was the bug: the map went with the old generation + /// while the transaction it protected stayed valid and relayable, so the + /// replacement could sign a conflicting spend of the same outpoint. The + /// end-to-end round trip through the manager is + /// `a_recreated_wallet_inherits_the_pending_fences_of_the_generation_it_replaces`; + /// this pins the mechanism. + #[test] + fn pins_cross_generations_of_the_same_wallet() { + let fences = Arc::new(InBroadcastFences::default()); + let first = Arc::new(WalletGeneration::with_fences(Arc::clone(&fences))); + let a = outpoint(15, 0); + let tx = spending(&[a]); + + // A dispatch that reached the network: settled into the pending-spend + // phase, awaiting an observed spend that has not arrived. + settle_dispatched(&first, &tx); + assert_eq!(first.in_broadcast_conflict(&tx), Some(a)); + + // The wallet is removed and re-created under the same id. + let second = Arc::new(WalletGeneration::with_fences(fences)); + assert_eq!( + second.in_broadcast_conflict(&tx), + Some(a), + "the replacement generation must inherit the pending-spend fence — \ + the signed transaction it protects is still live" + ); + + // …and the inherited fence still answers only to EVIDENCE, observed + // through whichever generation is current. + second.observe_spent([a]); + assert_eq!( + second.in_broadcast_conflict(&tx), + None, + "an observed spend on the new generation clears what the old one installed" + ); + } + + /// Inheritance is scoped to one wallet: two wallets' fence maps are + /// separate objects, so neither can block the other's builds. + #[test] + fn pins_do_not_cross_between_wallets() { + let first = Arc::new(WalletGeneration::new()); + let second = Arc::new(WalletGeneration::new()); + let a = outpoint(15, 0); + let tx = spending(&[a]); + + let _pin = first.pin_in_broadcast(&tx); + + assert_eq!(first.in_broadcast_conflict(&tx), Some(a)); + assert_eq!( + second.in_broadcast_conflict(&tx), + None, + "another wallet's fence must not block this one's builds" + ); + } + + /// Cleared entries are reaped on read, so the map carries no rows nothing + /// holds. What clears a row is an OBSERVED SPEND — never elapsed anything — + /// and the reap must not touch a row that still blocks, even one whose + /// transaction was dispatched arbitrarily long ago. + #[test] + fn cleared_fences_are_reaped_on_read_and_held_ones_are_not() { + let generation = Arc::new(WalletGeneration::new()); + let (a, held) = (outpoint(16, 0), outpoint(18, 0)); + let tx = spending(&[a]); + let held_tx = spending(&[held]); + + settle_dispatched(&generation, &tx); + settle_dispatched(&generation, &held_tx); + + // `a`'s spend is observed while a dispatching pin still holds it, so + // the row survives the observation and is reaped by the settle. + let pin = generation.pin_in_broadcast(&tx); + generation.observe_spent([a]); + assert_eq!( + generation.in_broadcast_fence_state(&a), + Some((1, false, true)), + "the observation retired the pending phase but not the live dispatch" + ); + pin.settle_pending_spend(); + + // Reading about an UNRELATED transaction still reaps. + assert_eq!( + generation.in_broadcast_conflict(&spending(&[outpoint(17, 0)])), + None + ); + assert_eq!( + generation.in_broadcast_fence_state(&a), + None, + "the cleared entry must be gone from the map, not merely inert" + ); + assert_eq!( + generation.in_broadcast_conflict(&held_tx), + Some(held), + "and the reap must not take a fence that is still held" + ); + } + + /// `dashpay/platform#4309`, REVIEW ROUND 5 SUGGESTION: THE + /// DISPATCHING→PENDING HANDOFF REGRESSION, MADE DETERMINISTIC. + /// + /// The transition lifts the dispatching hold and opens the pending-spend + /// phase. If those two ever land in separate critical sections, an observer + /// in between sees the outpoint held by NOTHING and a build can select an + /// input whose transaction may be on the wire. + /// + /// The previous regression parked a manager writer and hoped the scheduler + /// granted it the lock inside a window a handful of instructions wide. It + /// did not reliably do so — the reviewer showed it stays green against the + /// pre-fix code — so it proved nothing. + /// + /// This one is deterministic. [`WalletGeneration::on_next_settle_boundary`] + /// runs the observer AT the midpoint by construction — after the + /// dispatching hold is lifted, before the pending phase opens, so the + /// probe lands inside the torn state itself rather than before any fence + /// was touched (round 6) — and the settling thread BLOCKS until the + /// observer has published what it saw, so there is no race to lose. The + /// observer probes with `try_lock` + /// ([`WalletGeneration::try_probe_in_broadcast`]) rather than blocking, + /// because a blocking read cannot distinguish "held across the whole + /// transition" — the property under test — from "granted after it". + /// + /// Legal: `TransitionInProgress`. Illegal: `Free`, which is exactly what a + /// split transition would expose. + #[test] + fn the_settle_handoff_is_never_observable_half_done() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(18, 0); + let tx = spending(&[a]); + + let pin = generation.pin_in_broadcast(&tx); + + // Hand the observer its own generation handle; it runs on another + // thread, woken exactly at the midpoint. + let (at_midpoint_tx, at_midpoint_rx) = mpsc::channel::<()>(); + let (observed_tx, observed_rx) = mpsc::channel::(); + let observer = std::thread::spawn({ + let generation = Arc::clone(&generation); + move || { + at_midpoint_rx.recv().expect("midpoint signal"); + let probe = generation.try_probe_in_broadcast(&a); + observed_tx.send(probe).expect("publish observation"); + } + }); + + // At the midpoint: wake the observer and do not proceed until it has + // published. That is what removes the scheduling race — the settle is + // provably still in progress while the observation is taken. + generation.on_next_settle_boundary(Box::new(move || { + at_midpoint_tx.send(()).expect("wake observer"); + let probe = observed_rx.recv().expect("observation"); + assert_ne!( + probe, + InBroadcastProbe::Free, + "the dispatching→pending handoff was observable half-done: the \ + outpoint was held by nothing mid-transition, so a build could \ + have selected an input whose transaction may be on the wire" + ); + })); + + pin.settle_pending_spend(); + observer.join().expect("observer thread"); + + // And the end state is a live fence, not merely an unobservable + // transition into nothing. + assert_eq!( + generation.in_broadcast_conflict(&tx), + Some(a), + "the settled fence must be live once the transition completes" + ); + } + + /// The settle-boundary hook is ONE-SHOT, so a test's handshake cannot be + /// re-entered by an unrelated later settle on the same generation. + #[test] + fn the_settle_boundary_hook_fires_once() { + let generation = Arc::new(WalletGeneration::new()); + let fired = Arc::new(AtomicUsize::new(0)); + let tx = spending(&[outpoint(19, 0)]); + + generation.on_next_settle_boundary(Box::new({ + let fired = Arc::clone(&fired); + move || { + fired.fetch_add(1, Ordering::SeqCst); + } + })); + + settle_dispatched(&generation, &tx); + settle_dispatched(&generation, &tx); + + assert_eq!(fired.load(Ordering::SeqCst), 1); + } +} diff --git a/packages/rs-platform-wallet/src/wallet/core/mod.rs b/packages/rs-platform-wallet/src/wallet/core/mod.rs index ec4cbd9b8e4..e03363948f2 100644 --- a/packages/rs-platform-wallet/src/wallet/core/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/core/mod.rs @@ -4,6 +4,7 @@ mod broadcast; pub mod generation; // Inherent `CoreWallet::sign_message` only — no types to re-export. mod sign_message; +pub mod spend_observer; pub(crate) use sign_message::is_signable_funding_account; mod transaction; pub mod wallet; @@ -11,6 +12,8 @@ pub mod wallet; pub use balance::WalletBalance; pub use balance_handler::BalanceUpdateHandler; pub use generation::WalletGeneration; +pub(crate) use generation::{InBroadcastFences, InBroadcastPin}; +pub use spend_observer::SpendObservationHandler; pub(crate) use transaction::resolve_source_accounts; pub use transaction::{SignedCoreTransaction, ASSET_LOCK_FUNDING_SOURCES, SEND_FUNDING_SOURCES}; pub use wallet::CoreWallet; diff --git a/packages/rs-platform-wallet/src/wallet/core/spend_observer.rs b/packages/rs-platform-wallet/src/wallet/core/spend_observer.rs new file mode 100644 index 00000000000..92fd3bd73a6 --- /dev/null +++ b/packages/rs-platform-wallet/src/wallet/core/spend_observer.rs @@ -0,0 +1,345 @@ +//! Event handler that releases in-broadcast input fences when the wallet +//! OBSERVES the fenced outpoints spent. +//! +//! This is the evidence half of the broadcast fence +//! ([`WalletGeneration::pin_in_broadcast`](super::WalletGeneration::pin_in_broadcast)). +//! The dispatch side installs a fence when a transaction may have reached the +//! network; this side takes it down when the wallet can actually see that the +//! outpoints are spent. + +use std::collections::BTreeMap; +use std::sync::Arc; + +use dash_spv::EventHandler; +use tokio::sync::RwLock; + +use crate::changeset::core_bridge::spent_outpoints; +use crate::events::{PlatformEventHandler, WalletEvent}; +use crate::wallet::platform_wallet::WalletId; +use crate::wallet::PlatformWallet; + +/// Releases a wallet generation's in-broadcast fences as the wallet observes +/// the fenced outpoints spent. +/// +/// # Why the fence needs this at all +/// +/// A dispatch that returns anything but a definitive pre-send rejection leaves +/// its inputs fenced, because the broadcaster's return says "this may be on the +/// network", not "this wallet has seen the spend" — and on the +/// `DapiBroadcaster` path the two are far apart, since `sdk.execute` injects +/// nothing into local wallet state. Something has to end that fence, and three +/// earlier revisions tried to end it on elapsed `last_processed_height`. That +/// cannot work: catch-up advances the chain clock over blocks mined *before* +/// the transaction was submitted, so an ordinary historical sync retires a +/// fence without a shred of evidence about the transaction it was protecting +/// (`dashpay/platform#4309`). +/// +/// So the fence ends on the observation instead, and this handler is where the +/// observation arrives. +/// +/// # Which events, and why these are the right ones +/// +/// The two variants that carry spend-bearing transaction records, which are +/// exactly the two [`build_core_changeset`](crate::changeset::core_bridge) +/// derives [`CoreChangeSet::spent_utxos`](crate::changeset::CoreChangeSet) +/// from: +/// +/// * [`WalletEvent::TransactionDetected`] — first sighting, typically the +/// mempool relay of the transaction this very wallet just dispatched. On the +/// DAPI path this is the moment the wallet learns its own send exists. +/// * [`WalletEvent::BlockProcessed`] — the `inserted` records, i.e. spends +/// arriving in a block (including the dispatch's own transaction confirming +/// without ever having been seen in the mempool). +/// +/// `TransactionInstantLocked` and `ChainLockProcessed` are deliberately not +/// handled: they promote the finality of a record the wallet already has, and +/// the spend was already observed when that record first arrived. Handling them +/// would re-derive the same outpoints for no change. +/// +/// Both spend shapes release, and the fence does not care which it saw — the +/// dispatch's own transaction, or a competing transaction spending the same +/// outpoint. After either one the outpoint is out of this wallet's selectable +/// set, so there is no re-selection left to race. See +/// [`WalletGeneration::observe_spent`](super::WalletGeneration::observe_spent). +/// +/// # Lock discipline +/// +/// Mirrors [`BalanceUpdateHandler`](super::BalanceUpdateHandler), for the same +/// reason: `on_wallet_event` is synchronous and runs inside SPV's block +/// processing, which holds the wallet-manager WRITE lock for the whole batch. +/// Resolving the generation through *that* lock would deadlock or silently drop +/// every event during initial sync, so this handler holds an `Arc` clone of the +/// manager's `wallets` map instead — a separate lock, written only by manager +/// lifecycle methods, so `try_read` essentially never contends. Releasing the +/// fence then takes only the generation's `in_broadcast` `std::sync::Mutex` for +/// a few hash operations and never awaits. +/// +/// A dropped observation (contended map, or a wallet not in the map) is +/// FAIL-SAFE in the direction that matters: the fence simply stays up until +/// another spend event for the same outpoint arrives. There is no backstop +/// behind it — no deadline retires a pending-spend fence +/// (`dashpay/platform#4309`) — so a dropped observation costs a wait, not +/// safety. It can delay a release; it can never cause one. +pub struct SpendObservationHandler { + wallets: Arc>>>, +} + +impl SpendObservationHandler { + pub fn new(wallets: Arc>>>) -> Self { + Self { wallets } + } + + /// Hand `outpoints` to `wallet_id`'s generation as observed spends. + fn release_fences(&self, wallet_id: &WalletId, outpoints: Vec) { + if outpoints.is_empty() { + return; + } + // try_read on the wallets map, NOT the SPV-contended wallet_manager + // lock — see the type docs. + let Ok(wallets) = self.wallets.try_read() else { + tracing::debug!( + wallet = %hex::encode(wallet_id), + spent = outpoints.len(), + "in-broadcast fence release deferred: wallets-map lock contended" + ); + return; + }; + if let Some(wallet) = wallets.get(wallet_id) { + wallet.generation().observe_spent(outpoints); + } + } +} + +impl EventHandler for SpendObservationHandler { + fn on_wallet_event(&self, event: &WalletEvent) { + if let Some(wallet_id) = observing_wallet(event) { + self.release_fences(wallet_id, observed_spends(event)); + } + } +} + +impl PlatformEventHandler for SpendObservationHandler {} + +/// The wallet whose fences `event` can retire, or `None` for a variant that +/// carries no spend. +fn observing_wallet(event: &WalletEvent) -> Option<&WalletId> { + match event { + WalletEvent::TransactionDetected { wallet_id, .. } + | WalletEvent::BlockProcessed { wallet_id, .. } => Some(wallet_id), + WalletEvent::TransactionInstantLocked { .. } + | WalletEvent::ChainLockProcessed { .. } + | WalletEvent::SyncHeightAdvanced { .. } => None, + } +} + +/// Project a [`WalletEvent`] into the outpoints of ours it reports spent. +/// +/// Split out of the handler so the projection — which decides *what counts as +/// observing a spend*, the fence's entire release condition — is unit-testable +/// without standing up a `PlatformWallet` and a manager, and so the dispatch +/// tests can drive the real projection rather than a hand-rolled stand-in. +/// +/// Built on [`spent_outpoints`], the same per-record input walk that produces +/// [`CoreChangeSet::spent_utxos`](crate::changeset::CoreChangeSet), so the +/// fence and the persisted spent set cannot diverge. +pub(crate) fn observed_spends(event: &WalletEvent) -> Vec { + match event { + // First sighting — typically the mempool relay of the transaction this + // wallet just dispatched. On the DAPI path this is the moment the + // wallet learns its own send exists. + WalletEvent::TransactionDetected { record, .. } => spent_outpoints(record).collect(), + // Spends arriving in a block, including a dispatch's own transaction + // confirming without ever having been seen in the mempool. + // + // `inserted` only: `updated` and `matured` re-emit records the wallet + // already holds, whose spends were observed when they first arrived. + WalletEvent::BlockProcessed { inserted, .. } => { + inserted.iter().flat_map(spent_outpoints).collect() + } + // Finality promotions of records the wallet already holds, and a bare + // watermark advance. No new spend in any of them — and note that the + // watermark is precisely the "chain moved" signal that must NOT touch + // a fence (`dashpay/platform#4309`). + WalletEvent::TransactionInstantLocked { .. } + | WalletEvent::ChainLockProcessed { .. } + | WalletEvent::SyncHeightAdvanced { .. } => Vec::new(), + } +} + +#[cfg(test)] +mod tests { + //! Cover the projection — which events count as observing a spend, and + //! which outpoints they yield. That decision IS the fence's release + //! condition (`dashpay/platform#4309`), so it is pinned here rather than + //! only exercised end to end. + + use dashcore::hashes::Hash; + use dashcore::{ + Address as DashAddress, BlockHash, Network, OutPoint, ScriptBuf, Transaction, TxIn, Txid, + Witness, + }; + use key_wallet::account::{AccountType, StandardAccountType}; + use key_wallet::managed_account::transaction_record::{ + InputDetail, TransactionDirection, TransactionRecord, + }; + use key_wallet::transaction_checking::transaction_router::TransactionType; + use key_wallet::transaction_checking::{BlockInfo, TransactionContext}; + use key_wallet::WalletCoreBalance; + + use super::*; + + const WALLET_ID: WalletId = [3u8; 32]; + + fn outpoint(byte: u8) -> OutPoint { + OutPoint { + txid: Txid::from_slice(&[byte; 32]).expect("valid txid"), + vout: 0, + } + } + + fn spending(outpoints: &[OutPoint]) -> Transaction { + Transaction { + version: 2, + lock_time: 0, + input: outpoints + .iter() + .map(|previous_output| TxIn { + previous_output: *previous_output, + script_sig: ScriptBuf::new(), + sequence: 0xffff_ffff, + witness: Witness::new(), + }) + .collect(), + output: Vec::new(), + special_transaction_payload: None, + } + } + + /// A record whose `input_details` claim the given input indexes as ours — + /// the shape upstream builds for inputs that spent our outpoints. + fn record_claiming(tx: &Transaction, ours: &[u32]) -> TransactionRecord { + TransactionRecord::new( + tx.clone(), + AccountType::Standard { + index: 0, + standard_account_type: StandardAccountType::BIP44Account, + }, + TransactionContext::InBlock(BlockInfo::new( + 1_000, + BlockHash::from_slice(&[4u8; 32]).expect("valid block hash"), + 1_234_567_890, + )), + TransactionType::Standard, + TransactionDirection::Outgoing, + ours.iter() + .map(|index| InputDetail { + index: *index, + value: 1_000, + address: DashAddress::dummy(Network::Testnet, 1), + }) + .collect(), + Vec::new(), + 0, + ) + } + + fn detected(record: TransactionRecord) -> WalletEvent { + WalletEvent::TransactionDetected { + wallet_id: WALLET_ID, + record: Box::new(record), + balance: WalletCoreBalance::default(), + account_balances: std::collections::BTreeMap::new(), + addresses_derived: Vec::new(), + } + } + + fn block_processed(inserted: Vec) -> WalletEvent { + WalletEvent::BlockProcessed { + wallet_id: WALLET_ID, + height: 1_000, + chain_lock: None, + inserted, + updated: Vec::new(), + matured: Vec::new(), + balance: WalletCoreBalance::default(), + account_balances: std::collections::BTreeMap::new(), + addresses_derived: Vec::new(), + } + } + + /// A first sighting — the mempool relay of our own DAPI-broadcast send — + /// reports its spends. This is the event that ends the fence in the case + /// the whole redesign exists for. + #[test] + fn a_detected_transaction_reports_its_spends() { + let (a, b) = (outpoint(1), outpoint(2)); + let tx = spending(&[a, b]); + + assert_eq!( + observed_spends(&detected(record_claiming(&tx, &[0, 1]))), + [a, b] + ); + } + + /// Only inputs the record claims as OURS count. A transaction that also + /// spends someone else's coins must not retire a fence on an outpoint this + /// wallet does not own — same rule `CoreChangeSet::spent_utxos` follows, + /// because both walk `input_details`. + #[test] + fn only_our_inputs_are_reported() { + let (ours, theirs) = (outpoint(3), outpoint(4)); + let tx = spending(&[ours, theirs]); + + assert_eq!( + observed_spends(&detected(record_claiming(&tx, &[0]))), + [ours], + "an input the record does not claim is not a spend of ours" + ); + } + + /// An `input_details` index that does not address a real input is skipped + /// rather than panicking. + #[test] + fn an_out_of_range_input_index_is_skipped() { + let tx = spending(&[outpoint(5)]); + + assert!(observed_spends(&detected(record_claiming(&tx, &[7]))).is_empty()); + } + + /// A block's `inserted` records report their spends — the dispatch's own + /// transaction confirming without ever being seen in the mempool. + #[test] + fn block_processed_reports_inserted_record_spends() { + let (a, b) = (outpoint(6), outpoint(7)); + let first = spending(&[a]); + let second = spending(&[b]); + + let spends = observed_spends(&block_processed(vec![ + record_claiming(&first, &[0]), + record_claiming(&second, &[0]), + ])); + + assert_eq!(spends, [a, b]); + } + + /// THE VARIANT THAT MUST NEVER TOUCH A FENCE. + /// + /// `SyncHeightAdvanced` is the bare "the chain moved" watermark, and it is + /// precisely the signal three earlier revisions of this fix let retire a + /// fence — via a `last_processed_height + N` bound rather than directly, + /// but with the same effect. It reports no spend and must stay that way + /// (`dashpay/platform#4309`). + #[test] + fn chain_progress_alone_reports_no_spend() { + let event = WalletEvent::SyncHeightAdvanced { + wallet_id: WALLET_ID, + height: 900_000, + }; + + assert!(observed_spends(&event).is_empty()); + assert!( + observing_wallet(&event).is_none(), + "a bare watermark advance must not even resolve a wallet to act on" + ); + } +} diff --git a/packages/rs-platform-wallet/src/wallet/core/transaction.rs b/packages/rs-platform-wallet/src/wallet/core/transaction.rs index d7f883cf1e0..1bf6dd15a56 100644 --- a/packages/rs-platform-wallet/src/wallet/core/transaction.rs +++ b/packages/rs-platform-wallet/src/wallet/core/transaction.rs @@ -21,6 +21,7 @@ use key_wallet::{DerivationPath, ReservationToken, Utxo}; use super::{CoreWallet, WalletGeneration}; use crate::broadcaster::TransactionBroadcaster; +use crate::wallet::reservations::reservation_expired; use crate::PlatformWalletError; /// What funded (or failed to fund) a build, for attributing a shortfall. @@ -418,6 +419,29 @@ impl CoreWallet { }; } + // Refuse a selection that picked an input pinned by an IN-FLIGHT + // BROADCAST. A pinned input is normally still reserved and never + // reaches selection; getting here means this build's own + // selection swept that dispatch's aged reservation (catch-up + // advanced the clock past key-wallet's TTL while the dispatch + // was suspended pre-submission) and re-reserved the input under + // our token. Completing this build would race the pinned, + // already-signed transaction on the wire — the double-spend the + // dispatch-side age guard exists to prevent + // (`WalletGeneration::pin_in_broadcast`). Still under the write + // guard, so the check is atomic with our reservation and the + // release is exact. + // + // The refusal is TYPED (`InputMidBroadcast`, carrying the + // conflicting outpoint) rather than a build-failure string: this is + // the one build error that is safely retryable unchanged once the + // dispatch settles, and callers should not have to substring-match + // prose to tell it apart (`dashpay/platform#4309`). + if let Some(outpoint) = info.generation.in_broadcast_conflict(&unsigned) { + release_all!(offered_accounts, info.core_wallet.accounts, &unsigned); + return Err(PlatformWalletError::InputMidBroadcast { outpoint }); + } + // Map every selected input back to the account that owns it. That // mapping — not the offered list — is what the transaction carries: // selection routinely takes nothing from most offered sources, and @@ -540,7 +564,45 @@ impl CoreWallet { } /// Release a finalized transaction that the caller has chosen not to send. + /// + /// # Reservation age guard + /// + /// This is the abandon/free arm of the finalized-transaction handle — + /// including the FFI broadcast/abandon *failure* paths (invalid or + /// wrong-generation wallet handle) that route their cleanup here, and the + /// host-language deinit/GC backstop + /// (`core_wallet_signed_transaction_free`). A pinned handle can reach it + /// long after `finalize`, so it honors the **same** age bound as + /// [`broadcast_finalized_transaction`](Self::broadcast_finalized_transaction), + /// off the same shared [`reservation_expired`] predicate and the same + /// `last_processed_height` clock. + /// + /// With the build's owner token present the release is owner-guarded + /// (`release_reservation_if_owner`), which is safe at ANY age: it frees the + /// inputs only while this build still owns them and no-ops once key-wallet's + /// TTL sweep or a re-reservation transferred ownership. Between + /// [`RESERVATION_MAX_AGE_BLOCKS`](crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS) + /// and the TTL the reservation is typically STILL this build's, so an aged + /// abandon must still release — skipping would strand the inputs for + /// several more blocks while the host has already discarded the payment. + /// Only a token-less build (never reached on the funded finalize path) + /// honours the age bound and skips: its only release primitive is the + /// unguarded by-outpoint form, which after a sweep could free a newer + /// build's reservation. This mirrors the deferred registry's + /// `reconcile_removed_entry` policy exactly. pub async fn abandon_transaction(&self, transaction: &SignedCoreTransaction) { + if transaction.reservation_token.is_none() + && reservation_expired( + transaction.reservation_height, + self.last_processed_height().await, + ) + { + // Aged, and no owner token to guard the release: the outpoint may + // have been swept and re-reserved by an unrelated build. Leave it + // for key-wallet's TTL; releasing by outpoint could free that newer + // reservation. + return; + } self.release_transaction_reservation( &transaction.funding_accounts, &transaction.transaction, diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index c084ea667a1..acfb2811390 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -1098,7 +1098,7 @@ impl DashPayView<'_, B> { // re-acquires that (non-reentrant) lock internally. self.drain_pending_contact_crypto(provider).await; - let (payment_address, used_flip_changeset, tx, fee, funding_accounts) = { + let (payment_address, used_flip_changeset, tx, fee, funding_accounts, in_broadcast_pin) = { let mut wm = self.wallet_manager.write().await; // Resolve the external account's xpub so we can derive addresses. @@ -1307,12 +1307,74 @@ impl DashPayView<'_, B> { } }; + // Refuse a selection that picked an input pinned by an IN-FLIGHT + // BROADCAST dispatch (`WalletGeneration::pin_in_broadcast`): our + // own selection swept that dispatch's aged reservation (catch-up + // advanced past key-wallet's TTL while it was suspended + // pre-submission) and re-reserved the input, so completing this + // payment would race the pinned, already-signed transaction on + // the wire. Same backstop as `finalize_transaction` and the + // asset-lock build. The `build_signed` reservation is token-less; + // the by-outpoint release is exact because the write guard has + // been held since selection — and it must sweep EVERY account + // that offered funding (pooled selection), the same superset the + // rejected-broadcast release below uses; accounts that supplied + // nothing no-op. Roll back the consumed payment address exactly + // like the build-failure arm above — nothing was persisted or + // broadcast. + if let Some(outpoint) = info.generation.in_broadcast_conflict(&tx) { + for at in &offered_accounts { + if let Some(managed) = info.core_wallet.accounts.funds_account_mut(at) { + managed.release_reservation(&tx); + } + } + if let Some(external_account) = info + .core_wallet + .accounts + .dashpay_external_accounts + .get_mut(&key) + { + return_contact_payment_address_to_pool(external_account, &payment_address); + } + // Typed, and the SAME variant the other two choke points + // return — see `PlatformWalletError::InputMidBroadcast`. + return Err(PlatformWalletError::InputMidBroadcast { outpoint }); + } + + // …and FENCE THIS SELECTION IN TURN, before the write guard drops. + // + // The check above is only half of the contract. It stops this build + // from consuming an input another dispatch has fenced; without the + // pin, the transaction this build just signed carries no fence of + // its own, and everything below — the durability store, and the + // `broadcaster.broadcast(&tx)` await — runs unfenced. The + // broadcaster can suspend before submission; catch-up can advance + // `last_processed_height` past key-wallet's 24-block reservation + // TTL in that gap; a competing build then sweeps and re-reserves + // this very input, finds no fence on it, passes its own copy of the + // check above, and completes — after which THIS future resumes and + // puts its already-signed transaction on the wire against an input + // reassigned to another payment (`dashpay/platform#4309`, review + // round 7). + // + // So the pin is installed under the guard that just proved the + // reservation is ours, making check-and-pin one atomic step, and it + // outlives the guard exactly the way the finalized-handle dispatch's + // does (`CoreWallet::dispatch_unexpired`). It is settled on every + // exit below: released on a definitive pre-send failure (the + // durability abort, a rejected broadcast), and left as a + // pending-spend fence on every other outcome — accepted, ambiguous, + // or this future being cancelled/unwound mid-broadcast, which + // `InBroadcastPin::drop` covers without any code here. + let in_broadcast_pin = info.generation.pin_in_broadcast(&tx); + ( payment_address, used_flip_changeset, tx, fee, offered_accounts, + in_broadcast_pin, ) }; @@ -1327,11 +1389,19 @@ impl DashPayView<'_, B> { // leaves a one-address gap that the pool's gap window absorbs on // retry — bounded, because a signed transaction exists here, unlike // the unbounded build-failure case rolled back above. - self.persister.store(used_flip_changeset).map_err(|e| { - PlatformWalletError::Persistence(format!( + if let Err(e) = self.persister.store(used_flip_changeset) { + // A definitive PRE-SEND failure: the broadcaster below was never + // reached, so the transaction provably is not on the wire and its + // inputs are safe to reselect immediately. Release the fence in the + // same breath as the abort — carrying it past this point would hold + // the inputs against every later build with no transaction to + // protect, and (with no deadline behind it) nothing would ever + // clear it. + in_broadcast_pin.settle_released(); + return Err(PlatformWalletError::Persistence(format!( "failed to persist payment-address used flip: {e}" - )) - })?; + ))); + } // --- 3. Broadcast the transaction, releasing the build's UTXO // reservation if the broadcast is definitively rejected pre-send. --- @@ -1340,8 +1410,40 @@ impl DashPayView<'_, B> { // came from a BIP32 or contact-receiving account would otherwise leave // those reserved until the TTL backstop, and an immediate retry would // fail with a spurious insufficient-funds. + // The pin installed under the build guard is held across this await — + // that is the whole point of it — and settled on the way out. Only a + // definitive rejection releases the inputs; every other outcome leaves + // the pending-spend fence standing until the wallet observes the spend + // (`dashpay/platform#4309`). A cancellation or unwind inside `broadcast` + // reaches neither arm and settles as pending through + // `InBroadcastPin::drop`, which is the conservative direction. let broadcast_result = match self.broadcaster.broadcast(&tx).await { Err(e) if matches!(e, crate::broadcaster::BroadcastError::Rejected { .. }) => { + // Provably nothing on the wire: free the fence alongside the + // reservation, so the instructed immediate retry can reselect. + // + // ORDER MATTERS — the cleanup runs FIRST, under the still-live + // fence, and only then does the pin come down + // (`dashpay/platform#4309`, review round 8). The cleanup is an + // `.await`: it must re-acquire the wallet-manager read lock, and + // on this path it carries NO reservation token, so it performs an + // unconditional `release_reservation`. Releasing the fence first + // opened a window in which this input was neither fenced nor — + // once catch-up had swept the build's reservation — reserved. A + // build already queued on the manager write lock could take it in + // that window, pass the now-absent conflict check, and drop the + // lock with its external signer still pending (finalized builds + // install no pin until broadcast); the unconditional cleanup then + // deleted THAT build's newer reservation, and a second + // finalization could reserve and sign the same input — two live + // conflicting handles. + // + // With the fence held across the cleanup there is no such window: + // a queued build that runs first meets the fence and rolls back + // its own selection, so there is never a newer reservation for + // the unconditional release to clobber. `release_reservation_ + // after_rejected_broadcast` documents this ordering requirement + // for exactly this reason. crate::wallet::reservations::release_reservation_after_rejected_broadcast( &self.wallet_manager, &self.wallet_id, @@ -1352,9 +1454,18 @@ impl DashPayView<'_, B> { None, ) .await; + in_broadcast_pin.settle_released(); Err(e) } - other => other, + other => { + // Accepted, or an ambiguous `MaybeSent`. The transaction may be + // on the network, and this manager may be running the DAPI + // broadcaster, which injects nothing locally — so the inputs are + // still selectable here and must stay fenced until an observed + // spend says otherwise. + in_broadcast_pin.settle_pending_spend(); + other + } }; let txid = match broadcast_result { Ok(txid) => txid, @@ -6624,6 +6735,285 @@ mod tests { } } + /// `dashpay/platform#4309`, REVIEW ROUND 7 — THE CONTACT-PAYMENT BUILD'S + /// OWN FENCE. + /// + /// The build's conflict check stopped it from CONSUMING an input another + /// dispatch had fenced. It did not fence the selection it had just made, so + /// the stretch after the manager write guard drops — the durability store + /// and `broadcaster.broadcast(&tx)` — ran with no pin at all. This test + /// drives the resulting race end to end: + /// + /// 1. A contact payment builds, signs, releases the guard, and SUSPENDS + /// inside the broadcaster before submission. + /// 2. Catch-up advances the wallet's height far past key-wallet's 24-block + /// reservation TTL, so the parked build's reservation is swept and its + /// input is selectable again. + /// 3. A competing contact payment builds. There is exactly one spendable + /// UTXO, so it selects the same input the parked transaction already + /// spends. + /// + /// Before the fix step 3 SUCCEEDED — it found no fence (the parked build + /// never installed one), passed its own conflict check, and returned a + /// second signed transaction against the same input, which the resuming + /// original then raced on the wire. It must now be refused. + #[tokio::test] + async fn a_suspended_contact_payment_fences_its_inputs_against_a_competing_build() { + use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; + + let (manager, _persister, wallet_id, owner_id, contact_id) = + register_sender_and_external_account().await; + let wallet_arc = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet_arc.identity(); + + // ONE spendable UTXO, so the competing build can only select the very + // outpoint the parked transaction already spends. + fund_bip44_account_0(&manager, wallet_id, 0xD1, 1_000_000).await; + let funded = dashcore::OutPoint { + txid: ::from_slice(&[0xD1; 32]) + .expect("txid"), + vout: 0, + }; + + let seed = Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid mnemonic") + .to_seed(""); + let provider = SeedCryptoProvider::from_seed(seed, Network::Testnet); + let signer = SeedSigner::new(seed, Network::Testnet); + + let stamped = synced_height(&manager, wallet_id).await; + let entered = Arc::new(tokio::sync::Barrier::new(2)); + let release = Arc::new(tokio::sync::Barrier::new(2)); + let gated = with_gated_broadcaster(iw, Arc::clone(&entered), Arc::clone(&release)); + let accepting = with_accepting_broadcaster(iw); + + let parked = async { + gated + .dashpay() + .send_payment(&owner_id, &contact_id, 100_000, None, &signer, &provider) + .await + }; + + let competitor = async { + // Parked inside `broadcast`: signed, guard dropped, nothing + // submitted — the exact window the fence has to cover. + entered.wait().await; + + // Ordinary historical catch-up, well past key-wallet's reservation + // TTL. The parked build's reservation is swept; nothing but the + // fence is holding its input now. + set_synced_height(&manager, wallet_id, stamped + 17_000).await; + + let racing = accepting + .dashpay() + .send_payment(&owner_id, &contact_id, 100_000, None, &signer, &provider) + .await; + release.wait().await; + racing + }; + + let (sent, racing) = tokio::join!(parked, competitor); + + match racing { + Err(PlatformWalletError::InputMidBroadcast { outpoint }) => assert_eq!( + outpoint, funded, + "the refusal must name the input the parked transaction spends" + ), + other => panic!( + "a competing build must be refused while the original is mid-broadcast — \ + unfenced, it returned a second signed transaction spending the same \ + input, got {other:?}" + ), + } + + assert!( + sent.is_ok(), + "the parked payment itself must complete normally, got {sent:?}" + ); + } + + /// `dashpay/platform#4309`, REVIEW ROUND 8 — THE FENCE MUST OUTLIVE THE + /// REJECTED-BROADCAST RESERVATION CLEANUP. + /// + /// The definitive-rejection arm used to drop the fence FIRST and only then + /// await `release_reservation_after_rejected_broadcast`. That cleanup is + /// token-less on this path, so it performs an UNCONDITIONAL + /// `release_reservation`, and it can only run after re-acquiring the + /// wallet-manager read lock — an await. In that window the input was + /// neither fenced nor (once catch-up had swept it) reserved, so a build + /// already queued on the manager write lock could reserve it, pass the + /// now-absent conflict check, and drop the lock with an external signer + /// still pending. The unconditional cleanup then deleted THAT build's + /// newer reservation, leaving the outpoint free for a second finalization + /// to reserve and sign — two fresh conflicting handles over one input. + /// + /// The invariant that closes it: the fence stays up THROUGH the cleanup and + /// comes down only after it. A queued build that runs first then meets a + /// live fence and rolls back its own selection instead. + /// + /// Driven here by holding the wallet-manager WRITE lock across the + /// broadcaster's rejection. The cleanup needs the READ lock, so it cannot + /// complete while the test holds the write side — which makes the assertion + /// an invariant rather than a race: with the fix the fence CANNOT be gone at + /// this observation point, because the only code that releases it runs after + /// a cleanup that is provably still blocked. Before the fix the release ran + /// synchronously the instant `broadcast` returned, so the fence was gone. + #[tokio::test] + async fn the_contact_send_fence_outlives_its_rejected_broadcast_reservation_cleanup() { + use crate::wallet::identity::network::contact_requests::SeedCryptoProvider; + + let (manager, _persister, wallet_id, owner_id, contact_id) = + register_sender_and_external_account().await; + let wallet_arc = manager.get_wallet(&wallet_id).await.expect("wallet"); + let iw = wallet_arc.identity(); + + fund_bip44_account_0(&manager, wallet_id, 0xD2, 1_000_000).await; + let funded = dashcore::OutPoint { + txid: ::from_slice(&[0xD2; 32]) + .expect("txid"), + vout: 0, + }; + + let seed = Mnemonic::from_phrase(TEST_MNEMONIC, Language::English) + .expect("valid mnemonic") + .to_seed(""); + let provider = SeedCryptoProvider::from_seed(seed, Network::Testnet); + let signer = SeedSigner::new(seed, Network::Testnet); + + let entered = Arc::new(tokio::sync::Barrier::new(2)); + let release = Arc::new(tokio::sync::Barrier::new(2)); + let gated = + with_gated_rejecting_broadcaster(iw, Arc::clone(&entered), Arc::clone(&release)); + + let parked = async { + gated + .dashpay() + .send_payment(&owner_id, &contact_id, 100_000, None, &signer, &provider) + .await + }; + + let observer = async { + // Signed, build guard dropped, parked inside `broadcast`. + entered.wait().await; + + // Take the manager WRITE lock and keep it: the rejection cleanup + // below wants the READ lock, so it is pinned outside this hold. + let held = iw.wallet_manager.write().await; + + // Let the broadcaster return `Rejected`. The send now runs its + // rejection arm; the cleanup blocks on the read lock. + release.wait().await; + for _ in 0..256 { + tokio::task::yield_now().await; + } + + let observed = wallet_arc.generation().in_broadcast_fence_state(&funded); + drop(held); + observed + }; + + let (sent, observed) = tokio::join!(parked, observer); + + assert!( + observed.is_some(), + "the contact-send fence must still stand while the rejected \ + broadcast's reservation cleanup is pending — it was already \ + released (observed: {observed:?})" + ); + + assert!( + sent.is_err(), + "a definitively rejected send must surface an error, got {sent:?}" + ); + + // …and once the cleanup HAS run, the fence comes down: a definitive + // rejection is provable evidence nothing reached the wire, so the + // input must be immediately reselectable. + assert_eq!( + wallet_arc.generation().in_broadcast_fence_state(&funded), + None, + "after the cleanup completes the rejected send must free its fence" + ); + } + + /// [`GatedBroadcaster`], but the transport definitively REJECTS after the + /// park — the shape the rejection-arm ordering test needs. + struct GatedRejectingBroadcaster { + entered: Arc, + release: Arc, + } + + #[async_trait::async_trait] + impl crate::broadcaster::TransactionBroadcaster for GatedRejectingBroadcaster { + async fn broadcast( + &self, + _transaction: &dashcore::Transaction, + ) -> Result { + self.entered.wait().await; + self.release.wait().await; + Err(crate::broadcaster::BroadcastError::Rejected { + reason: "test rejection".to_string(), + }) + } + } + + fn with_gated_rejecting_broadcaster( + real: &crate::wallet::identity::IdentityWallet, + entered: Arc, + release: Arc, + ) -> crate::wallet::identity::IdentityWallet { + crate::wallet::identity::IdentityWallet { + sdk: Arc::clone(&real.sdk), + wallet_manager: Arc::clone(&real.wallet_manager), + wallet_id: real.wallet_id, + asset_locks: Arc::clone(&real.asset_locks), + persister: real.persister.clone(), + broadcaster: Arc::new(GatedRejectingBroadcaster { entered, release }), + sdk_writer: Arc::clone(&real.sdk_writer), + dpns_operation_gate: Arc::clone(&real.dpns_operation_gate), + dpns_sync_progress: Arc::clone(&real.dpns_sync_progress), + } + } + + /// Broadcaster stub that PARKS inside `broadcast` — the production + /// suspension the in-broadcast fence exists to cover. It signals `entered` + /// once it has the transaction (guard already dropped, nothing submitted) + /// and waits on `release` before returning. + struct GatedBroadcaster { + entered: Arc, + release: Arc, + } + + #[async_trait::async_trait] + impl crate::broadcaster::TransactionBroadcaster for GatedBroadcaster { + async fn broadcast( + &self, + transaction: &dashcore::Transaction, + ) -> Result { + self.entered.wait().await; + self.release.wait().await; + Ok(transaction.txid()) + } + } + + fn with_gated_broadcaster( + real: &crate::wallet::identity::IdentityWallet, + entered: Arc, + release: Arc, + ) -> crate::wallet::identity::IdentityWallet { + crate::wallet::identity::IdentityWallet { + sdk: Arc::clone(&real.sdk), + wallet_manager: Arc::clone(&real.wallet_manager), + wallet_id: real.wallet_id, + asset_locks: Arc::clone(&real.asset_locks), + persister: real.persister.clone(), + broadcaster: Arc::new(GatedBroadcaster { entered, release }), + sdk_writer: Arc::clone(&real.sdk_writer), + dpns_operation_gate: Arc::clone(&real.dpns_operation_gate), + dpns_sync_progress: Arc::clone(&real.dpns_sync_progress), + } + } + /// Re-specialize a live `IdentityWallet` onto the /// accepting broadcaster, sharing every other Arc (wallet manager, SDK, /// asset locks, persister, sdk_writer) so the two handles operate on the diff --git a/packages/rs-platform-wallet/src/wallet/reservations.rs b/packages/rs-platform-wallet/src/wallet/reservations.rs index dc95dce4844..f260b7c1282 100644 --- a/packages/rs-platform-wallet/src/wallet/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/reservations.rs @@ -4,10 +4,20 @@ //! contributing funding account's `ReservationSet` and leaves the reservation //! held on success, expecting the transaction to be broadcast. When the //! broadcast *fails* the reservation must be reconciled here: released for an -//! immediate retry when Core definitively rejected the transaction, kept (for -//! the reservation-TTL backstop or a later sync) when acceptance is unknown. -//! A pooled build reserves across several accounts under one owner token, so -//! the reconciliation takes the whole contributor list, not one account. +//! immediate retry when Core definitively rejected the transaction, kept when +//! acceptance is unknown. A pooled build reserves across several accounts under +//! one owner token, so the reconciliation takes the whole contributor list, not +//! one account. +//! +//! "Kept" means kept until key-wallet's own `ReservationSet` TTL sweeps it, and +//! that sweep is NOT what makes the ambiguous case safe. The inputs of a +//! transaction that may be on the network are held by the generation's +//! pending-spend fence, which the TTL does not touch and which no elapsed +//! quantity retires — only an observed spend does +//! (`dashpay/platform#4309`). Reservation cleanup here and fence settlement in +//! the caller are two separate obligations; see +//! [`release_reservation_after_rejected_broadcast`] for the order they must run +//! in. //! //! Every build-then-broadcast path must go through //! [`broadcast_releasing_on_rejection`] so the cleanup exists once instead of @@ -25,6 +35,104 @@ use tokio::sync::RwLock; use crate::broadcaster::{BroadcastError, TransactionBroadcaster}; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; +/// Maximum age, in `last_processed_height` blocks, of a held funding +/// reservation before an operation that would *consume* it (broadcast) is +/// refused. Shared by the two deferred/split core-send surfaces so they bound a +/// reservation's lifetime against the same TTL with one number: +/// +/// * the deferred build → broadcast/release registry +/// ([`SignedPaymentRegistry`](crate::SignedPaymentRegistry)), and +/// * the atomic finalized-transaction handle path +/// (`core_wallet_tx_builder_finalize` → +/// `broadcast_finalized_transaction`). +/// +/// Kept strictly below key-wallet's `RESERVATION_TTL_BLOCKS` (24, ~1h at the +/// mainnet block target): a `build_signed` / `finalize_transaction` reservation +/// is stamped at the wallet's `last_processed_height` (via `set_current_height`) +/// and swept by a later `reserve`/`reserved` call — itself stamped with the same +/// `last_processed_height` clock — once it is `RESERVATION_TTL_BLOCKS` old, +/// silently returning the outpoint to the selectable pool where an unrelated +/// build can re-select and re-reserve it. `ReservationSet::release` removes an +/// outpoint unconditionally, with no ownership/generation check, so acting on a +/// reservation that was already swept could free (or broadcast against) a newer, +/// unrelated one. Refusing at this lower bound guarantees the guard always trips +/// **before** the underlying reservation could have been swept, leaving a margin +/// for `last_processed_height` to lag a few blocks behind the true tip. +pub(crate) const RESERVATION_MAX_AGE_BLOCKS: u32 = 20; + +// THERE IS DELIBERATELY NO TIMEOUT CONSTANT FOR THE BROADCAST INPUT FENCE. +// +// An `IN_BROADCAST_FENCE_ORPHAN_TIMEOUT` used to live here: one hour on a +// monotonic `Instant`, after which the pending-spend phase of +// `WalletGeneration::pin_in_broadcast` released an outpoint the wallet had +// never observed spent. It was the fifth bound this fence was given and the +// fifth to be unsound (`dashpay/platform#4309` — three height-anchored forms in +// rounds 2-4, the monotonic one in rounds 5-6, all removed in round 7). +// +// The monotonic clock did fix what the height-anchored bounds got wrong — +// catch-up cannot fast-forward it. It did not fix the actual defect, which is +// that ELAPSED TIME IS NOT EVIDENCE. A signed transaction does not become +// invalid by getting older, and waiting does not prove no peer retained it: a +// withholding DAPI endpoint can accept the transaction while keeping it off the +// network, and a backgrounded mobile wallet can outlast any deadline worth +// setting. When the deadline lapsed and catch-up had also swept key-wallet's +// reservation, the next build pruned the fence and signed a CONFLICTING +// transaction over inputs the original might still spend. +// +// So the fence now ends on evidence only — `WalletGeneration::observe_spent`, +// or a definitive pre-send failure. Anything added here later must be a +// LIVENESS path that says something about the pending transaction itself +// (persist it and query or rebroadcast it; take an explicit caller-driven +// abandon), never a duration that runs out. See the `in_broadcast` field docs +// on `WalletGeneration` for the full contract. + +/// Whether a reservation stamped at `registered_height` is too old to act on at +/// `current_height` (see [`RESERVATION_MAX_AGE_BLOCKS`]). The registration +/// height is mandatory on both surfaces — it is derived from the finalized +/// [`SignedCoreTransaction::reservation_height`](crate::SignedCoreTransaction) +/// (captured inside the funding critical section, before the potentially-slow +/// external signer ran), never sampled independently. +/// +/// *Consuming* (broadcasting) a stale reservation is refused: once the outpoint +/// may already have been swept by key-wallet's TTL and re-reserved by an +/// unrelated build, broadcasting would spend against that newer reservation. +/// The guarded broadcasts +/// ([`broadcast_finalized_transaction`](crate::CoreWallet::broadcast_finalized_transaction) +/// and the registry's [`broadcast`](crate::SignedPaymentRegistry::broadcast)) +/// refuse with their stale-reservation errors, reconciling the reservation on +/// the way out. Cleanup (abandon/free, and that refusal-path reconciliation) +/// distinguishes two cases by the build's owner token: +/// +/// * **Owner token present** (every funded finalize): the release is +/// owner-guarded (`release_reservation_if_owner`) and therefore safe at ANY +/// age — it frees the inputs only while this build still owns them and no-ops +/// once a TTL sweep or re-reservation transferred ownership — so aged cleanup +/// still releases, letting an immediate rebuild reselect the inputs. +/// * **Token-less** (a build that reserved nothing): the only release primitive +/// is `ReservationSet::release`, which removes an outpoint unconditionally +/// with no ownership check, so past the bound the by-outpoint release is +/// skipped and the aged reservation is left for key-wallet's TTL to reclaim. +/// +/// An unknown *current* height means the wallet is gone from the manager, which +/// disables the guard (`None` → not expired). That is safe only because every +/// caller establishes liveness first and so never reaches here with a removed +/// wallet: the registry's +/// [`broadcast`](crate::SignedPaymentRegistry::broadcast) refuses with +/// `SignedPaymentError::WalletRemoved` before sampling the height, its +/// `reconcile_removed_entry` release is itself generation-bound and no-ops on a +/// missing wallet, and the finalized-transaction handle path runs after the +/// FFI layer's generation-identity check. The earlier claim that "the +/// wallet-mismatch / account-lookup paths already reject those cases" was wrong +/// for the registry broadcast path — `is_same_generation` compares handles (a +/// removed generation matches itself) and that path performs no account lookup +/// at all (`dashpay/platform#4185`). +pub(crate) fn reservation_expired(registered_height: u32, current_height: Option) -> bool { + match current_height { + Some(current) => current.saturating_sub(registered_height) >= RESERVATION_MAX_AGE_BLOCKS, + None => false, + } +} + /// Broadcast `tx` and reconcile the funding account's UTXO reservation on /// failure. /// @@ -84,11 +192,29 @@ pub(crate) async fn broadcast_releasing_on_rejection>, wallet_id: &WalletId, diff --git a/packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs b/packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs index a7a29620782..be0ca53f8e5 100644 --- a/packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs +++ b/packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs @@ -41,7 +41,8 @@ //! recreation needs the manager write lock, so it cannot slip between that //! check and the release; a stale token can therefore never free a re-created //! generation's reservation. -//! * A token has a bounded lifetime ([`RESERVATION_MAX_AGE_BLOCKS`]). Once the +//! * A token has a bounded lifetime +//! ([`RESERVATION_MAX_AGE_BLOCKS`](crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS)). Once the //! wallet's `last_processed_height` has advanced far enough past the height at //! which `build_signed` / `finalize_transaction` stamped the reservation that //! key-wallet's own `ReservationSet` TTL could have swept and re-selected the @@ -79,6 +80,10 @@ use key_wallet::ReservationToken as FundingReservationToken; use crate::broadcaster::TransactionBroadcaster; use crate::wallet::core::{CoreWallet, SignedCoreTransaction}; +// The age bound and its predicate are shared with the atomic finalized- +// transaction handle path (`broadcast_finalized_transaction`), so both surfaces +// measure a reservation's lifetime against key-wallet's TTL with one number. +use crate::wallet::reservations::reservation_expired; use crate::PlatformWalletError; /// Opaque handle to a registered, signed-but-unsent payment. Minted by @@ -121,48 +126,6 @@ impl std::fmt::Display for ReservationToken { } } -/// Maximum age, in `last_processed_height` blocks, of a registered token before -/// its broadcast or release is refused. -/// -/// Kept strictly below key-wallet's `RESERVATION_TTL_BLOCKS` (24, ~1h at the -/// mainnet block target): a `build_signed` / `finalize_transaction` reservation -/// is stamped at the wallet's `last_processed_height` (via `set_current_height`) -/// and swept by a later `reserve`/`reserved` call — itself stamped with the same -/// `last_processed_height` clock — once it is `RESERVATION_TTL_BLOCKS` old, -/// silently returning the outpoint to the selectable pool where an unrelated -/// build can re-select and re-reserve it. -/// `ReservationSet::release` removes an outpoint unconditionally, with no -/// ownership/generation check, so acting on a token whose reservation was -/// already swept could free (or broadcast against) a newer, unrelated -/// reservation. Refusing at this lower bound guarantees the guard always trips -/// **before** the underlying reservation could have been swept, leaving a margin -/// for `last_processed_height` to lag a few blocks behind the true tip. -const RESERVATION_MAX_AGE_BLOCKS: u32 = 20; - -/// Whether a token stamped at `registered_height` is too old to act on at -/// `current_height` (see [`RESERVATION_MAX_AGE_BLOCKS`]). The registration -/// height is mandatory — it is derived from the finalized -/// [`SignedCoreTransaction::reservation_height`](crate::SignedCoreTransaction) -/// the registry consumed. -/// -/// An unknown *current* height means the wallet is gone from the manager, which -/// disables the guard (`None` → not expired). That is safe only because every -/// caller establishes liveness first and so never reaches here with a removed -/// wallet: [`broadcast`](SignedPaymentRegistry::broadcast) refuses with -/// [`SignedPaymentError::WalletRemoved`] before sampling the height, and -/// [`reconcile_removed_entry`](SignedPaymentRegistry::reconcile_removed_entry)'s -/// release is itself generation-bound and no-ops on a missing wallet. The -/// earlier claim that "the wallet-mismatch / account-lookup paths already reject -/// those cases" was wrong for the broadcast path — `is_same_generation` compares -/// handles (a removed generation matches itself) and the broadcast path performs -/// no account lookup at all (`dashpay/platform#4185`). -fn reservation_expired(registered_height: u32, current_height: Option) -> bool { - match current_height { - Some(current) => current.saturating_sub(registered_height) >= RESERVATION_MAX_AGE_BLOCKS, - None => false, - } -} - /// Failure of a deferred broadcast/release token operation. #[derive(Debug, thiserror::Error)] pub enum SignedPaymentError { @@ -196,11 +159,15 @@ pub enum SignedPaymentError { #[error("reservation token {0} belongs to a wallet that is no longer in the manager")] WalletRemoved(ReservationToken), - /// The token has outlived [`RESERVATION_MAX_AGE_BLOCKS`], so its underlying + /// The token has outlived + /// [`RESERVATION_MAX_AGE_BLOCKS`](crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS), so its underlying /// UTXO reservation may already have been swept by key-wallet's TTL and - /// re-selected by an unrelated build. Acting on it (broadcast or release) - /// could touch a newer reservation, so it is refused and the caller must - /// rebuild the payment. + /// re-selected by an unrelated build. The *broadcast* is refused and the + /// caller must rebuild the payment — but the reservation itself is + /// reconciled on the way out: with the build's owner token present the + /// release is owner-guarded and safe at any age (it no-ops once ownership + /// transferred), freeing still-owned inputs for the rebuild. Only a + /// token-less entry is dropped without releasing. #[error("reservation token {0} has outlived its reservation lifetime; rebuild the payment")] StaleReservationToken(ReservationToken), @@ -260,8 +227,10 @@ struct RegisteredPayment { /// reservation with (`SignedCoreTransaction::reservation_height`). Compared /// against the wallet's current `last_processed_height` to refuse a /// broadcast/release once the reservation could plausibly have been swept by - /// key-wallet's TTL (see [`RESERVATION_MAX_AGE_BLOCKS`]). Mandatory: it is - /// derived from the consumed ownership object, never sampled independently. + /// key-wallet's TTL (see + /// [`RESERVATION_MAX_AGE_BLOCKS`](crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS)). + /// Mandatory: it is derived from the consumed ownership object, never + /// sampled independently. registered_height: u32, /// The key-wallet [`FundingReservationToken`] stamped onto the funding /// inputs when `finalize_transaction` reserved them @@ -498,39 +467,45 @@ impl SignedPaymentRegistry { return Err(SignedPaymentError::WalletRemoved(token)); } - // Refuse to SEND a token whose reservation could already have been - // swept and re-selected by an unrelated build — but reconcile its - // reservation first. With the build's owner token present the release - // is safe at ANY age: `release_reservation_if_owner` frees the inputs - // only while this build still owns them and no-ops after a TTL sweep - // or re-reservation transferred ownership. Between the guard bound - // (RESERVATION_MAX_AGE_BLOCKS) and key-wallet's TTL the reservation is - // typically STILL HELD, so dropping without releasing would strand the - // inputs for several more blocks while telling the caller to rebuild — - // and the rebuild would fail selection. Only a token-less entry falls - // back to the drop-without-release policy (an unguarded by-outpoint - // release could free a newer build's reservation). - if reservation_expired( - entry.registered_height, - current.last_processed_height().await, - ) { - Self::reconcile_removed_entry(entry).await; - return Err(SignedPaymentError::StaleReservationToken(token)); - } - // One releasing-broadcast path for every funding variant, CoinJoin // included: a definitive rejection releases the reservation for an // immediate rebuild, an ambiguous outcome keeps it, and the release is // bound to the token's own wallet generation. - let txid = entry + // + // The age bound is NOT pre-checked here: it is re-validated at + // dispatch time inside `broadcast_payment_releasing_reservation` + // (height sampled under the manager read guard, which drops before + // the broadcaster await) — a check made out here is stale by the + // time the send begins (catch-up can advance the clock, and a + // concurrent finalization can sweep + re-reserve the inputs in + // the gap). On the stale outcome the + // broadcaster was never touched and the entry is reconciled below + // exactly as the old pre-check did: with the build's owner token + // present the release is safe at ANY age + // (`release_reservation_if_owner` no-ops once ownership was + // transferred); between the guard bound and key-wallet's TTL the + // reservation is typically STILL HELD, so releasing is what lets + // the instructed immediate rebuild reselect the inputs. Only a + // token-less entry falls back to drop-without-release (an + // unguarded by-outpoint release could free a newer build's + // reservation). + match entry .core .broadcast_payment_releasing_reservation( &entry.funding_accounts, &entry.tx, entry.funding_reservation_token, + entry.registered_height, ) - .await?; - Ok(txid) + .await + { + Ok(txid) => Ok(txid), + Err(PlatformWalletError::StaleReservation) => { + Self::reconcile_removed_entry(entry).await; + Err(SignedPaymentError::StaleReservationToken(token)) + } + Err(error) => Err(error.into()), + } } /// Reconcile one already-removed entry's reservation, bound to the token's @@ -667,13 +642,13 @@ mod tests { use super::{ RegisterWrongGeneration, ReservationToken, SignedPaymentError, SignedPaymentRegistry, - RESERVATION_MAX_AGE_BLOCKS, }; use crate::broadcaster::{BroadcastError, TransactionBroadcaster}; use crate::test_support::{ funded_wallet_manager, AlwaysMaybeSentBroadcaster, AlwaysRejectedBroadcaster, WalletSigner, }; use crate::wallet::core::{CoreWallet, SignedCoreTransaction}; + use crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS; use crate::PlatformWalletError; /// The [`AccountTypePreference`] a `build_signed_tx` funding account maps to diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift index 69af4d0e297..8ce56359353 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift @@ -242,6 +242,23 @@ public class ManagedCoreWallet { /// Consume and broadcast an atomically finalized transaction, returning /// the authoritative accepted/rejected/unknown network outcome. + /// + /// - Important: `.errorStaleReservationToken` (native code 34) is a + /// **terminal** outcome, not a retryable one. It means the handle was held + /// until the wallet's `last_processed_height` advanced past the funding + /// reservation's age bound, so the transaction was refused *before* the + /// network was touched — nothing was sent. + /// + /// Neither a retry nor an abandon is possible: `takeForBroadcast()` above + /// has already consumed this handle, so calling + /// `broadcastTransactionWithOutcome(_:)` again throws locally, and + /// `abandonTransaction(_:)` has nothing left to release. The refusal path + /// in Rust reconciles the reservation itself, releasing the inputs + /// owner-guarded. + /// + /// **Recover by rebuilding the transaction.** The released inputs are + /// immediately reselectable by a fresh builder → `finalize` sequence, with + /// no cleanup call in between. public func broadcastTransactionWithOutcome( _ tx: FinalizedCoreTransaction ) throws -> CoreTransactionBroadcastOutcome {