From 61f871e38065e2296442c6208f04814f8fe8974d Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 6 Aug 2026 19:31:36 +0700 Subject: [PATCH 1/8] fix(platform-wallet): age-guard the finalized-transaction handle broadcast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased down to the age-guard onto current v4.2-dev: the #4185/#4308 stack it was riding merged, and #4323/#4325 renamed the finalized- transaction surface (the v2 suffix is gone), so the guard now lands on core_wallet_broadcast_signed_transaction and the slice-based finalize_transaction signature. Mirrors the deferred registry-token age policy on the finalized-handle path: RESERVATION_MAX_AGE_BLOCKS (20; key-wallet TTL 24) and reservation_expired() live in wallet::reservations, shared by both surfaces. broadcast_finalized_transaction refuses with StaleReservation (FFI ErrorStaleReservationToken, 34) before touching the broadcaster once the reservation's stamp height has aged past the bound — and the refusal reconciles the reservation on the way out, exactly like the registry's stale-token branch: the FFI wrapper has already consumed the opaque handle, so no follow-up abandon is possible, and the owner- guarded release (safe at any age; a no-op once ownership transferred) frees the still-owned inputs for the instructed immediate rebuild. Abandon/free likewise release owner-guarded at any age, with the by-outpoint skip retained only for token-less builds. Boundary tests cover both account types on the platform and FFI layers, including the terminal FFI stale-broadcast path. Co-Authored-By: Claude Fable 5 --- .../dashsdk/errors/DashSdkError.kt | 24 +- .../dashsdk/wallet/ManagedCoreWallet.kt | 28 +- .../src/core_wallet/broadcast.rs | 112 ++++++++ packages/rs-platform-wallet-ffi/src/error.rs | 53 ++++ packages/rs-platform-wallet/src/error.rs | 27 ++ .../rs-platform-wallet/src/test_support.rs | 32 +++ .../src/wallet/core/broadcast.rs | 269 +++++++++++++++++- .../src/wallet/core/transaction.rs | 39 +++ .../src/wallet/reservations.rs | 72 +++++ .../src/wallet/signed_payment_registry.rs | 69 ++--- 10 files changed, 663 insertions(+), 62 deletions(-) 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 b6c555a3add..59863deb13b 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 @@ -266,13 +266,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 dbed938ea3e..cb7fe5eb35d 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 @@ -27,14 +27,38 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable { check(it != 0L) { "ManagedCoreWallet has been closed" } } - /** Consume and broadcast a finalized transaction. */ + /** + * 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), so a follow-up [abandonTransaction] is an + * invalid-handle error, 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 = 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. + */ 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..55de0604cf5 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs @@ -288,6 +288,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 +347,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 cb560d46f31..a36356671aa 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -283,6 +283,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 @@ -579,6 +596,14 @@ 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 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 @@ -1195,6 +1220,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/error.rs b/packages/rs-platform-wallet/src/error.rs index 8dab7054699..79b857cbb54 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -118,6 +118,33 @@ pub enum PlatformWalletError { )] 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`](crate::wallet::reservations::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), diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index 31c7abdf446..559f8d0c9ea 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -532,6 +532,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/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 6c53c5dba57..c0270630312 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -4,7 +4,7 @@ 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}; impl CoreWallet { @@ -18,10 +18,47 @@ 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 { + if reservation_expired( + transaction.reservation_height(), + self.last_processed_height().await, + ) { + self.abandon_transaction(transaction).await; + return Err(PlatformWalletError::StaleReservation); + } match self.broadcaster.broadcast(transaction.transaction()).await { Ok(txid) => Ok(txid), Err(error) => { @@ -158,14 +195,17 @@ mod tests { 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::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 +287,229 @@ 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; + } + } + + /// 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 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/transaction.rs b/packages/rs-platform-wallet/src/wallet/core/transaction.rs index 2987d01518b..17e3db6862b 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. @@ -523,7 +524,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/reservations.rs b/packages/rs-platform-wallet/src/wallet/reservations.rs index 365d6514229..f213be8e362 100644 --- a/packages/rs-platform-wallet/src/wallet/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/reservations.rs @@ -22,6 +22,78 @@ 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; + +/// 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. /// 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..d482d74e695 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 @@ -667,13 +636,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 From 0f2cb03b3d8d34b6cb989f0b6d8cd907961328ee Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Mon, 10 Aug 2026 14:53:04 -0400 Subject: [PATCH 2/8] docs(platform-wallet): de-link the pub(crate) reservation bound from a public doc Co-Authored-By: Claude Fable 5 --- packages/rs-platform-wallet/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 79b857cbb54..8f3780a58b5 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -123,7 +123,7 @@ pub enum PlatformWalletError { /// 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`](crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS) + /// `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 From ffc05fc44609fe5af2c5945fc11f51cfe4d6fa77 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Mon, 10 Aug 2026 15:41:59 -0400 Subject: [PATCH 3/8] fix(kotlin-sdk): map the native stale-broadcast error on the public broadcastTransaction The method documents DashSdkError.PlatformWallet.StaleReservationToken but called the JNI native directly, so direct callers received the internal DashSDKException instead of the documented public type. Co-Authored-By: Claude Fable 5 --- .../org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 cb7fe5eb35d..3e67b47c847 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 @@ -42,11 +42,12 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable { * release. Recover by rebuilding the transaction, which can reselect the * freed inputs immediately. */ - fun broadcastTransaction(tx: FinalizedCoreTransaction): String = + fun broadcastTransaction(tx: FinalizedCoreTransaction): String = mapNativeErrors { WalletManagerNative.coreWalletBroadcastSignedTransaction( handle, tx.takeForBroadcast(), ) + } /** * Consume a finalized transaction without sending. With the build's owner From e4e678448633c9b5b3b617086dca9b43b9c87f04 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Mon, 10 Aug 2026 15:54:10 -0400 Subject: [PATCH 4/8] fix(platform-wallet): validate reservation age atomically with dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pre-checked age is not an ordering invariant: between the check and the broadcaster await, sync catch-up can advance last_processed_height past the bound and a concurrent finalization can trigger key-wallet's TTL sweep, re-reserving the same inputs under a new token — the old signed transaction then hits the wire against reassigned UTXOs. New shared primitive dispatch_unexpired performs the age check and reaches the broadcaster under ONE wallet-manager READ guard. Both writers this orders against — the ReservationSet TTL sweep (inside coin selection) and height advancement — mutate under the manager WRITE lock, so 'the reservation is unexpired' and 'dispatch has begun' become a single atomic observation. Ownership needs no separate probe: the key-wallet TTL exceeds RESERVATION_MAX_AGE_BLOCKS on the same clock, so an unexpired reservation cannot already have been swept. Both check-then-dispatch sites now route through it: the finalized- handle broadcast and the registry-token broadcast (whose composite gains the reservation height and returns the stale verdict for the registry's existing owner-guarded reconciliation). Reconciliation runs OUTSIDE the guard — those paths retake manager locks. Deliberate cost: writers queue behind the network await, bounded by the broadcaster's own timeout — the price of the invariant without a key-wallet-side in-broadcast pin. Co-Authored-By: Claude Fable 5 --- .../src/wallet/core/broadcast.rs | 145 ++++++++++++++++-- .../src/wallet/signed_payment_registry.rs | 51 +++--- 2 files changed, 160 insertions(+), 36 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index c0270630312..e9b5053020c 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -1,5 +1,6 @@ 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; @@ -7,7 +8,57 @@ use crate::broadcaster::{BroadcastError, TransactionBroadcaster}; 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 dispatch as ONE observation under the wallet-manager + /// READ lock. + /// + /// The two writers this orders against are 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). Held across the + /// check AND the broadcaster await, the read guard makes "the + /// reservation is unexpired" and "dispatch has begun" a single atomic + /// observation: neither a sweep nor a height advance can interleave + /// between them. Ownership needs no separate probe: key-wallet's TTL + /// exceeds `RESERVATION_MAX_AGE_BLOCKS` on the same clock, so a + /// reservation that passes the age check under this guard cannot + /// already have been swept, and self-releases only run on this + /// transaction's own rejection/abandon paths, which are sequenced + /// after this call returns. + /// + /// Deliberate cost: writers (height catch-up, finalization) queue + /// behind the network await, bounded by the broadcaster's own timeout. + /// That is the price of the ordering invariant without a + /// key-wallet-side in-broadcast pin. + /// + /// Callers do their stale/rejection reconciliation AFTER this returns: + /// those paths retake manager locks and must not run under this guard + /// (tokio's write-preferring `RwLock` would deadlock a re-entrant read + /// behind a queued writer). + pub(crate) async fn dispatch_unexpired( + &self, + reservation_height: u32, + transaction: &Transaction, + ) -> GuardedDispatch { + let wm = self.wallet_manager.read().await; + let height = wm + .get_wallet_and_info(&self.wallet_id) + .map(|(_, info)| info.core_wallet.last_processed_height()); + if reservation_expired(reservation_height, height) { + return GuardedDispatch::Stale; + } + GuardedDispatch::Sent(self.broadcaster.broadcast(transaction).await) + } + /// Broadcast an atomically finalized transaction. A definitive rejection /// releases its reservation; an ambiguous `MaybeSent` outcome retains it. /// @@ -52,16 +103,23 @@ impl CoreWallet { &self, transaction: &SignedCoreTransaction, ) -> Result { - if reservation_expired( - transaction.reservation_height(), - self.last_processed_height().await, - ) { - self.abandon_transaction(transaction).await; - return Err(PlatformWalletError::StaleReservation); - } - match self.broadcaster.broadcast(transaction.transaction()).await { - Ok(txid) => Ok(txid), - Err(error) => { + // Check and dispatch are one guarded observation — see + // [`Self::dispatch_unexpired`]. A plain check-then-send here let + // sync catch-up age the reservation and a concurrent finalization + // sweep + re-reserve the same inputs between the two steps, so the + // old signed transaction could hit the wire against reassigned + // UTXOs. Reconciliation stays OUTSIDE the guard (it retakes + // manager locks). + 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(), @@ -166,15 +224,28 @@ 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. 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; @@ -189,6 +260,7 @@ 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; @@ -414,6 +486,53 @@ mod tests { } } + /// The age bound is validated ATOMICALLY with dispatch, not merely + /// before it: [`CoreWallet::dispatch_unexpired`] samples the height and + /// reaches the broadcaster under ONE wallet-manager read guard, so a + /// height advance (a manager WRITE) can never interleave between a + /// passed check and the send. The single-threaded proof of that + /// ordering: the same handle's inputs dispatch while fresh, and the + /// identical call refuses — broadcaster untouched — once catch-up + /// advances the clock past the bound. The verdict is derived from the + /// state observed INSIDE the guard at each dispatch, never from a + /// caller-side pre-check that could go stale in the gap. + #[tokio::test] + async fn guarded_dispatch_rechecks_age_inside_the_guard() { + 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 under the guard" + ); + + // 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 inside the guard, 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 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 d482d74e695..8b9bc4b8fe4 100644 --- a/packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs +++ b/packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs @@ -467,39 +467,44 @@ 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 + // atomically with dispatch, under the wallet-manager read guard + // inside `broadcast_payment_releasing_reservation` — 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 From 9b033cb07dabd78be9a244cc0e054fb75924cbc7 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Tue, 11 Aug 2026 08:52:38 -0400 Subject: [PATCH 5/8] fix(platform-wallet): drop the manager guard before the broadcast await MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Held across the broadcaster await, the read guard starved the very pipeline the await depends on: the production SpvBroadcaster waits on dash-spv's mempool manager, whose local-transaction handler takes wallet.write() on this same manager lock before it can process the echo/IS-lock/confirmation events that complete the wait. Every dispatch therefore rode the full 30s acceptance timeout to an ambiguous MaybeSent — reservation kept while the transaction was actually on-chain, rebuild selection left with no spendable UTXOs — and tokio's write-preferring queue stalled the whole manager for the window. The mock broadcasters in the test suite never touch the wallet lock, which is why no test caught it. The age check stays at dispatch time under the read guard; the guard now drops before the await (the same lock-free shape as broadcast_releasing_on_rejection). The residual check-to-wire gap is covered by key-wallet's TTL margin — the same margin that already covers the propagation phase, which the guard never spanned — and releasing early is strictly stronger afterwards: the mempool pipeline marks the inputs spent in the wallet's own view within milliseconds instead of after the timeout. All atomicity claims in docs, comments, and the test narrative are rewritten to the actual contract. Co-Authored-By: Claude Fable 5 --- .../src/wallet/core/broadcast.rs | 107 ++++++++++-------- .../src/wallet/signed_payment_registry.rs | 13 ++- 2 files changed, 66 insertions(+), 54 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index e9b5053020c..c29df5cf267 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -18,43 +18,51 @@ pub(crate) enum GuardedDispatch { } impl CoreWallet { - /// Age-check and dispatch as ONE observation under the wallet-manager - /// READ lock. + /// Age-check under the wallet-manager READ lock, then dispatch + /// immediately after releasing it. /// - /// The two writers this orders against are 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). Held across the - /// check AND the broadcaster await, the read guard makes "the - /// reservation is unexpired" and "dispatch has begun" a single atomic - /// observation: neither a sweep nor a height advance can interleave - /// between them. Ownership needs no separate probe: key-wallet's TTL - /// exceeds `RESERVATION_MAX_AGE_BLOCKS` on the same clock, so a - /// reservation that passes the age check under this guard cannot - /// already have been swept, and self-releases only run on this - /// transaction's own rejection/abandon paths, which are sequenced - /// after this call 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. /// - /// Deliberate cost: writers (height catch-up, finalization) queue - /// behind the network await, bounded by the broadcaster's own timeout. - /// That is the price of the ordering invariant without a - /// key-wallet-side in-broadcast pin. + /// 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. Check-to-wire + /// is therefore a small non-atomic gap rather than an invariant; it is + /// covered by the same TTL margin as the propagation phase that follows + /// (the guard never spanned rebroadcasts either), and dropping early is + /// strictly stronger afterwards: the mempool pipeline marks the inputs + /// spent in the wallet's own view within milliseconds instead of after + /// the timeout. (Same lock-free shape as + /// `broadcast_releasing_on_rejection`.) /// /// Callers do their stale/rejection reconciliation AFTER this returns: - /// those paths retake manager locks and must not run under this guard - /// (tokio's write-preferring `RwLock` would deadlock a re-entrant read - /// behind a queued writer). + /// those paths retake manager locks. pub(crate) async fn dispatch_unexpired( &self, reservation_height: u32, transaction: &Transaction, ) -> GuardedDispatch { - let wm = self.wallet_manager.read().await; - let height = wm - .get_wallet_and_info(&self.wallet_id) - .map(|(_, info)| info.core_wallet.last_processed_height()); - if reservation_expired(reservation_height, height) { - return GuardedDispatch::Stale; + { + let wm = self.wallet_manager.read().await; + let height = wm + .get_wallet_and_info(&self.wallet_id) + .map(|(_, info)| info.core_wallet.last_processed_height()); + if reservation_expired(reservation_height, height) { + return GuardedDispatch::Stale; + } + // Guard dropped here — see above: holding it across the await + // starves the SPV pipeline that must complete the wait. } GuardedDispatch::Sent(self.broadcaster.broadcast(transaction).await) } @@ -103,13 +111,15 @@ impl CoreWallet { &self, transaction: &SignedCoreTransaction, ) -> Result { - // Check and dispatch are one guarded observation — see - // [`Self::dispatch_unexpired`]. A plain check-then-send here let - // sync catch-up age the reservation and a concurrent finalization - // sweep + re-reserve the same inputs between the two steps, so the - // old signed transaction could hit the wire against reassigned - // UTXOs. Reconciliation stays OUTSIDE the guard (it retakes - // manager locks). + // 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 residual check-to-wire gap and + // why the manager guard must not span the broadcaster await are + // documented on `dispatch_unexpired`. Reconciliation retakes + // manager locks after it returns. match self .dispatch_unexpired(transaction.reservation_height(), transaction.transaction()) .await @@ -486,18 +496,19 @@ mod tests { } } - /// The age bound is validated ATOMICALLY with dispatch, not merely - /// before it: [`CoreWallet::dispatch_unexpired`] samples the height and - /// reaches the broadcaster under ONE wallet-manager read guard, so a - /// height advance (a manager WRITE) can never interleave between a - /// passed check and the send. The single-threaded proof of that - /// ordering: the same handle's inputs dispatch while fresh, and the - /// identical call refuses — broadcaster untouched — once catch-up - /// advances the clock past the bound. The verdict is derived from the - /// state observed INSIDE the guard at each dispatch, never from a - /// caller-side pre-check that could go stale in the gap. + /// 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 residual check-to-wire gap is covered by key-wallet's TTL + /// margin, the same margin that covers the propagation phase. 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_inside_the_guard() { + async fn guarded_dispatch_rechecks_age_at_dispatch() { let (core, signer, outputs) = funded_core_wallet( account_type_standard(AccountTypePreference::BIP44), Arc::new(AlwaysOkBroadcaster), @@ -515,7 +526,7 @@ mod tests { .await; assert!( matches!(fresh, GuardedDispatch::Sent(Ok(_))), - "a fresh reservation must dispatch under the guard" + "a fresh reservation must dispatch" ); // Catch-up advances the clock past the bound; the identical call now @@ -527,7 +538,7 @@ mod tests { .await; assert!( matches!(stale, GuardedDispatch::Stale), - "an aged reservation must refuse inside the guard, not dispatch" + "an aged reservation must refuse at the check, not dispatch" ); core.abandon_transaction(&finalized).await; 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 8b9bc4b8fe4..be0ca53f8e5 100644 --- a/packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs +++ b/packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs @@ -472,12 +472,13 @@ impl SignedPaymentRegistry { // immediate rebuild, an ambiguous outcome keeps it, and the release is // bound to the token's own wallet generation. // - // The age bound is NOT pre-checked here: it is re-validated - // atomically with dispatch, under the wallet-manager read guard - // inside `broadcast_payment_releasing_reservation` — 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 + // 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 From 80c54b44205fe3be332e1e179d755c133ad99d0c Mon Sep 17 00:00:00 2001 From: bfoss765 Date: Wed, 12 Aug 2026 08:52:54 -0400 Subject: [PATCH 6/8] fix(platform-wallet): pin in-broadcast inputs across the dispatch await MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guarded dispatch proves reservation freshness under the manager read guard but must drop that guard before the broadcaster await (holding it starves the SPV mempool pipeline). Both production broadcasters can suspend before submission, and in that unbounded gap sync catch-up can advance last_processed_height past key-wallet's reservation TTL, letting a concurrent build's selection sweep the dispatched build's reservation and re-reserve the same inputs — the already-signed transaction would then hit the wire against inputs reassigned to another payment. Close the window with a non-expiring in-broadcast pin on WalletGeneration, installed atomically with the freshness check while the read guard is still held (freshness below the TTL on the same clock IS the ownership proof — sweeps and height advances run under the write lock) and released by RAII only after the broadcaster returns, cancelled dispatches included. Pins are counted per outpoint so a duplicate dispatch of the same transaction keeps the fence until its last send returns. Every coin-selection choke point — finalize_transaction, the contact-payment build, the asset-lock build — now refuses a build whose selection picked a pinned input, releasing its fresh reservation exactly under the still-held write guard. The registry-token broadcast shares dispatch_unexpired and therefore the same primitive. Also document the Kotlin-side consume semantics: after broadcastTransaction consumes the handle, a follow-up abandonTransaction fails locally with IllegalStateException before any native code runs — not with a native invalid-handle error. Co-Authored-By: Claude Opus 4.8 --- .../dashsdk/wallet/ManagedCoreWallet.kt | 11 +- .../src/wallet/asset_lock/build.rs | 35 +++ .../src/wallet/core/broadcast.rs | 183 +++++++++++-- .../src/wallet/core/generation.rs | 258 +++++++++++++++++- .../src/wallet/core/transaction.rs | 20 ++ .../src/wallet/identity/network/payments.rs | 28 ++ 6 files changed, 505 insertions(+), 30 deletions(-) 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 3e67b47c847..82fd8e145fb 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 @@ -37,8 +37,11 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable { * 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), so a follow-up [abandonTransaction] is an - * invalid-handle error, not a recovery path — there is nothing left to + * 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. */ @@ -59,6 +62,10 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable { * 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( 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 1fe80310c2a..9d2e6a70f1c 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -195,6 +195,41 @@ impl AssetLockManager { )) })?; + // 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(pinned) = info.generation.in_broadcast_conflict(&result.transaction) { + let funds_account = match funding_account { + AssetLockFundingAccount::Bip44 { account_index } => info + .core_wallet + .bip44_managed_account_at_index(account_index), + AssetLockFundingAccount::CoinJoin { account_index } => info + .core_wallet + .accounts + .coinjoin_accounts + .get(&account_index), + }; + if let Some(account) = funds_account { + match result.reservation_token { + Some(token) => account.release_reservation_if_owner(&result.transaction, token), + None => account.release_reservation(&result.transaction), + } + } + return Err(PlatformWalletError::AssetLockTransaction(format!( + "selected input {pinned} is mid-broadcast by an in-flight dispatch; \ + retry after it completes" + ))); + } + // 4. Pull the (pubkey, path) for our single credit output. // // `build_asset_lock_with_signer` always returns the `Public` diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index c29df5cf267..8404b4e9821 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -18,8 +18,9 @@ pub(crate) enum GuardedDispatch { } impl CoreWallet { - /// Age-check under the wallet-manager READ lock, then dispatch - /// immediately after releasing it. + /// 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 @@ -28,7 +29,9 @@ impl CoreWallet { /// 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. + /// 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, @@ -37,14 +40,30 @@ impl CoreWallet { /// 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. Check-to-wire - /// is therefore a small non-atomic gap rather than an invariant; it is - /// covered by the same TTL margin as the propagation phase that follows - /// (the guard never spanned rebroadcasts either), and dropping early is - /// strictly stronger afterwards: the mempool pipeline marks the inputs - /// spent in the wallet's own view within milliseconds instead of after - /// the timeout. (Same lock-free shape as - /// `broadcast_releasing_on_rejection`.) + /// 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, every coin-selection choke point refuses a build whose + /// selection picked a pinned input (under the same write lock the sweep + /// runs under), and it is released — via RAII, so a cancelled dispatch + /// releases it too — only after the broadcaster returns, which is + /// strictly after initial network dispatch. The propagation phase that + /// follows is unchanged: once dispatch returns, the mempool pipeline + /// marks the inputs spent in the wallet's own view within milliseconds. + /// + /// 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. @@ -53,17 +72,22 @@ impl CoreWallet { reservation_height: u32, transaction: &Transaction, ) -> GuardedDispatch { - { + let _in_broadcast_pin = { let wm = self.wallet_manager.read().await; - let height = wm - .get_wallet_and_info(&self.wallet_id) - .map(|(_, info)| info.core_wallet.last_processed_height()); + 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; } - // Guard dropped here — see above: holding it across the await - // starves the SPV pipeline that must complete the wait. - } + // 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 is dropped only after the broadcaster returns below. + 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. + }; GuardedDispatch::Sent(self.broadcaster.broadcast(transaction).await) } @@ -116,10 +140,12 @@ impl CoreWallet { // 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 residual check-to-wire gap and - // why the manager guard must not span the broadcaster await are - // documented on `dispatch_unexpired`. Reconciliation retakes - // manager locks after it returns. + // 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 @@ -239,7 +265,10 @@ impl CoreWallet { /// 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. On the stale outcome + /// 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( @@ -502,8 +531,9 @@ mod tests { /// 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 residual check-to-wire gap is covered by key-wallet's TTL - /// margin, the same margin that covers the propagation phase. The + /// 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. @@ -640,6 +670,109 @@ mod tests { } } + /// 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 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, and + /// only after the dispatch returns (pin dropped, RAII) may a new build + /// take the input again. + #[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; + + // Age the handle to ONE BELOW the guard bound: the freshness check + // must pass, which is exactly what makes the pre-submission window + // dangerous without the pin. + 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 } + }); + // The dispatcher is now suspended INSIDE the broadcaster: freshness + // checked, manager guard dropped, pin held. + entered.wait().await; + + // Catch-up races far past key-wallet's TTL measured from the original + // reservation stamp, so the NEXT selection's sweep reclaims the + // dispatched build's reservation and its input returns to the + // selectable pool. + advance_processed_height(&core, stamped + RESERVATION_MAX_AGE_BLOCKS + 48).await; + + // The competing finalize re-selects the fixture's only UTXO — the + // pinned input — and must be refused by the pin backstop, not + // completed into a conflicting signed transaction. + let competing = + try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + match competing { + Err(PlatformWalletError::TransactionBuild(message)) => assert!( + message.contains("mid-broadcast"), + "the refusal must name the in-flight broadcast, got: {message}" + ), + other => panic!("a build re-selecting a pinned input must be refused, got {other:?}"), + } + + // Let the dispatch complete: the send succeeds (the age check passed + // before the suspension) and the pin is dropped with it. + release.wait().await; + let sent = dispatcher.await.expect("dispatcher task"); + assert!( + sent.is_ok(), + "the pinned dispatch itself must complete, got {sent:?}" + ); + + // Pin lifted: a new build may take the input again. (The mock manager + // runs no mempool pipeline; in production the dispatched transaction's + // inputs would be marked spent by processing moments later — the + // assertion here is only that the pin's lifetime ended with the + // dispatch.) + let after = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let after = after.unwrap_or_else(|error| { + panic!("the pin must lift once the dispatch returns, got {error:?}") + }); + core.abandon_transaction(&after).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..e9afe8d200b 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,40 @@ 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 **pinned by an in-flight broadcast dispatch** + /// ([`pin_in_broadcast`](Self::pin_in_broadcast)), counted per outpoint. + /// + /// 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 non-expiring pin 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 pinned input, closing the sweep + re-reserve window + /// for as long as the dispatch is in flight. + /// + /// A *count* per outpoint 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`] unpin from a plain + /// (non-async) `Drop` — which is also what makes the pin + /// cancellation-safe when the dispatching future is dropped mid-await. + /// Never persisted: after a restart nothing is mid-dispatch. + in_broadcast: Mutex>, } impl Default for WalletGeneration { @@ -68,11 +106,12 @@ impl Default for WalletGeneration { } impl WalletGeneration { - /// A fresh generation: zeroed balance, uncontended gate. + /// A fresh generation: zeroed balance, uncontended gate, nothing pinned. pub fn new() -> Self { Self { balance: WalletBalance::new(), lifecycle: Arc::new(RwLock::new(())), + in_broadcast: Mutex::new(HashMap::new()), } } @@ -127,6 +166,112 @@ 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() + .unwrap_or_else(PoisonError::into_inner) + } + + /// 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 pin has **no TTL** — a suspended dispatch keeps its inputs fenced no + /// matter how far catch-up advances the clock — and is released only by + /// dropping the returned guard object, which happens even when the + /// dispatching future is cancelled mid-await (`Drop` runs on unwind and on + /// future drop alike). + /// + /// 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_insert(0) += 1; + } + } + InBroadcastPin { + generation: Arc::clone(self), + outpoints, + } + } + + /// The first of `transaction`'s inputs that is currently pinned by an + /// in-flight 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 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 pinned input is still + /// *reserved* and never reaches selection at all; this check is the + /// backstop for exactly the post-sweep window. + pub(crate) fn in_broadcast_conflict(&self, transaction: &Transaction) -> Option { + let pinned = self.in_broadcast_lock(); + transaction + .input + .iter() + .map(|input| input.previous_output) + .find(|outpoint| pinned.contains_key(outpoint)) + } + + /// Drop one pin count for each of `outpoints` — the [`InBroadcastPin`] + /// release half of [`pin_in_broadcast`](Self::pin_in_broadcast). + fn unpin_in_broadcast(&self, outpoints: &[OutPoint]) { + let mut pinned = self.in_broadcast_lock(); + for outpoint in outpoints { + match pinned.get_mut(outpoint) { + Some(count) if *count > 1 => *count -= 1, + Some(_) => { + pinned.remove(outpoint); + } + // Unreachable by construction — every pin increments before its + // guard can decrement — but a miscount must not panic a Drop. + None => debug_assert!(false, "unpin of an outpoint that was never pinned"), + } + } + } +} + +/// RAII guard for one dispatch's in-broadcast input pins — see +/// [`WalletGeneration::pin_in_broadcast`]. Dropping it (normal return, +/// unwind, or the dispatching future being cancelled mid-await) releases +/// exactly the pins that call took, count-wise, never another dispatch's. +pub(crate) struct InBroadcastPin { + generation: Arc, + outpoints: Vec, +} + +impl Drop for InBroadcastPin { + fn drop(&mut self) { + self.generation.unpin_in_broadcast(&self.outpoints); + } } impl Deref for WalletGeneration { @@ -136,3 +281,110 @@ impl Deref for WalletGeneration { &self.balance } } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use dashcore::{OutPoint, Transaction, TxIn, Txid}; + + use super::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) + } + + /// A held pin flags every input of the pinned transaction — and only + /// those — and dropping the pin clears the conflict. This is the RAII + /// contract the dispatch relies on for cancellation-safety: a dispatching + /// future dropped mid-await unpins exactly the same way. + #[test] + fn pin_flags_inputs_until_dropped() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(0x01, 0); + let b = outpoint(0x02, 1); + let unrelated = outpoint(0x03, 0); + + let pin = generation.pin_in_broadcast(&spending(&[a, b])); + + // Both pinned inputs conflict; an unrelated selection does not. + 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), + "a mixed selection must surface its pinned input" + ); + assert_eq!( + generation.in_broadcast_conflict(&spending(&[unrelated])), + None + ); + + drop(pin); + assert_eq!( + generation.in_broadcast_conflict(&spending(&[a, b])), + None, + "dropping the pin must clear the conflict" + ); + } + + /// Pins COUNT per outpoint: two concurrent dispatches of the same + /// transaction (legal through `&SignedCoreTransaction`, idempotent on the + /// wire) each take a pin, and the fence must hold until the LAST one + /// returns — the first completion must not unpin the other's in-flight + /// send. + #[test] + fn pins_are_counted_per_outpoint() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(0x10, 0); + let tx = spending(&[a]); + + let first = generation.pin_in_broadcast(&tx); + let second = generation.pin_in_broadcast(&tx); + + drop(first); + assert_eq!( + generation.in_broadcast_conflict(&tx), + Some(a), + "one dispatch still in flight must keep the outpoint fenced" + ); + + drop(second); + assert_eq!(generation.in_broadcast_conflict(&tx), None); + } + + /// Pins are per generation: a re-created wallet's fresh generation starts + /// with nothing pinned, and the old generation's pins die with its last + /// handle — nothing leaks across the recreation boundary. + #[test] + fn pins_do_not_cross_generations() { + let old_generation = Arc::new(WalletGeneration::new()); + let a = outpoint(0x20, 0); + let _pin = old_generation.pin_in_broadcast(&spending(&[a])); + + let new_generation = Arc::new(WalletGeneration::new()); + assert_eq!( + new_generation.in_broadcast_conflict(&spending(&[a])), + None, + "a fresh generation must not inherit the old generation's pins" + ); + } +} diff --git a/packages/rs-platform-wallet/src/wallet/core/transaction.rs b/packages/rs-platform-wallet/src/wallet/core/transaction.rs index 17e3db6862b..3ca67a7c4ce 100644 --- a/packages/rs-platform-wallet/src/wallet/core/transaction.rs +++ b/packages/rs-platform-wallet/src/wallet/core/transaction.rs @@ -402,6 +402,26 @@ 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. + if let Some(pinned) = info.generation.in_broadcast_conflict(&unsigned) { + release_all!(offered_accounts, info.core_wallet.accounts, &unsigned); + return Err(PlatformWalletError::TransactionBuild(format!( + "selected input {pinned} is mid-broadcast by an in-flight dispatch; \ + retry after it completes" + ))); + } + // 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 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 d447f282222..0ca9f3fedbf 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -1269,6 +1269,34 @@ 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. Roll back the consumed payment + // address exactly like the build-failure arm above — nothing was + // persisted or broadcast. + if let Some(pinned) = info.generation.in_broadcast_conflict(&tx) { + managed_account.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); + } + return Err(PlatformWalletError::TransactionBuild(format!( + "selected input {pinned} is mid-broadcast by an in-flight dispatch; \ + retry after it completes" + ))); + } + (payment_address, used_flip_changeset, tx, fee) }; From 2b911bc40e34ca1ff143cb896dd1bd591ba03b60 Mon Sep 17 00:00:00 2001 From: bfoss765 Date: Thu, 13 Aug 2026 12:22:24 -0400 Subject: [PATCH 7/8] fix(platform-wallet): fence dispatched inputs past the broadcaster return MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dispatch_unexpired` dropped its in-broadcast pin the moment `TransactionBroadcaster::broadcast` returned. That is safe only for `SpvBroadcaster`, which injects the transaction 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 both an accepted response and an ambiguous `MaybeSent` returned with the input still selectable while the transaction was in flight — and if catch-up had advanced `last_processed_height` past key-wallet's 24-block reservation TTL during the await, the reservation was already swept too. The input was then neither reserved nor fenced: exactly the sweep + re-select race the pin was added to close (dashpay/platform#4309). The pin becomes a two-phase fence on `WalletGeneration`: * dispatching — the existing counted, non-expiring pin, from check-and-pin until the broadcaster returns. * pending-spend — installed when the broadcaster returns anything but a definitive pre-send rejection, lasting `IN_BROADCAST_FENCE_BLOCKS` (24, key-wallet's own `RESERVATION_TTL_BLOCKS`) past the height the dispatch was authorized at. The second phase is key-wallet's reservation renewal implemented one layer up: `ReservationSet` exposes no renew primitive at the pinned revision, so instead of re-stamping the reservation we re-anchor an equivalent TTL at dispatch — which is the moment the transaction actually reached the network, and the point the TTL should always have been measured from. Only `BroadcastError::Rejected` frees the inputs at dispatch return; that outcome proves nothing is on the wire, and the caller releases the reservation in the same breath. The fence lapses rather than persisting: an outpoint the wallet has already observed as spent never reaches selection at all, so in the normal case the bound is never consulted. It exists only so a never-observed transaction cannot strand its inputs forever, and it leaves the residual exposure identical to the one key-wallet's reservation TTL already accepts. Nothing here touches the wallet-manager lock, so the SPV-starvation fix from 9b033cb07d is preserved verbatim: the manager read guard is still dropped before the broadcaster await. Lapsed entries are reaped by the conflict check itself — the only place the fence is read — so the map stays bounded with no background task. Tests: the barrier-gated race test's tail assertion is corrected (its 48-block catch-up already outruns the new bound, so it still proves the *dispatching* phase and says so); a new `dispatched_input_stays_fenced_after_the_broadcaster_returns` dispatches at the oldest height the age guard admits, then probes a height where the reservation is provably swept and only the fence stands. Reverting the `retain_pending_spend` call makes that test's competing build succeed, i.e. it reproduces the reported race. Five generation-level tests cover the non-expiring dispatching phase, the bound, on-read reaping, the longer-fence-wins merge, and the rejection path installing no fence. Co-Authored-By: Claude Opus 4.8 --- .../src/wallet/asset_lock/build.rs | 6 +- .../src/wallet/core/broadcast.rs | 194 ++++++++- .../src/wallet/core/generation.rs | 384 +++++++++++++++--- .../src/wallet/core/transaction.rs | 5 +- .../src/wallet/identity/network/payments.rs | 5 +- .../src/wallet/reservations.rs | 46 +++ 6 files changed, 551 insertions(+), 89 deletions(-) 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 ddba5a3df45..664981aee3b 100644 --- a/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs +++ b/packages/rs-platform-wallet/src/wallet/asset_lock/build.rs @@ -17,6 +17,7 @@ use key_wallet::wallet::managed_wallet_info::asset_lock_builder::{ }; use key_wallet::wallet::managed_wallet_info::managed_account_operations::ManagedAccountOperations; use key_wallet::wallet::managed_wallet_info::transaction_building::AccountTypePreference; +use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; use key_wallet::wallet::Wallet; @@ -233,7 +234,10 @@ impl AssetLockManager { // 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(pinned) = info.generation.in_broadcast_conflict(&result.transaction) { + if let Some(pinned) = info.generation.in_broadcast_conflict( + &result.transaction, + info.core_wallet.last_processed_height(), + ) { // 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 diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 8404b4e9821..51288900b1a 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -52,13 +52,39 @@ impl CoreWallet { /// 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, every coin-selection choke point refuses a build whose - /// selection picked a pinned input (under the same write lock the sweep - /// runs under), and it is released — via RAII, so a cancelled dispatch - /// releases it too — only after the broadcaster returns, which is - /// strictly after initial network dispatch. The propagation phase that - /// follows is unchanged: once dispatch returns, the mempool pipeline - /// marks the inputs spent in the wallet's own view within milliseconds. + /// 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 + /// (`InBroadcastPin::retain_pending_spend`) + /// lasting + /// [`IN_BROADCAST_FENCE_BLOCKS`](crate::wallet::reservations::IN_BROADCAST_FENCE_BLOCKS) + /// past the height this dispatch was authorized at. Once the wallet does + /// observe the spend the outpoint stops reaching selection at all, so the + /// fence goes inert without waiting for that bound; the bound is only the + /// backstop for a transaction that is never observed, and matches the TTL + /// the reservation itself would have had, re-anchored at dispatch. + /// + /// Neither phase touches the wallet-manager lock, so nothing here can + /// starve the SPV mempool pipeline: the guard is still dropped before the + /// broadcaster await, exactly as it was. /// /// A wallet no longer in the manager skips the pin (there is no /// registered generation to fence builds on — they cannot fund from a @@ -72,7 +98,7 @@ impl CoreWallet { reservation_height: u32, transaction: &Transaction, ) -> GuardedDispatch { - let _in_broadcast_pin = { + let mut 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()); @@ -81,14 +107,33 @@ impl CoreWallet { } // 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 is dropped only after the broadcaster returns below. - info.map(|info| info.generation.pin_in_broadcast(transaction)) + // 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 anchored at the + // height sampled right here. + info.map(|info| { + info.generation + .pin_in_broadcast(transaction, info.core_wallet.last_processed_height()) + }) // 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. }; - GuardedDispatch::Sent(self.broadcaster.broadcast(transaction).await) + let outcome = self.broadcaster.broadcast(transaction).await; + // Retain the fence for everything except a definitive pre-send + // rejection: only `Rejected` proves the transaction is not on the + // network, so only `Rejected` may free the inputs at dispatch return. + // An ambiguous `MaybeSent` is precisely the case that must stay fenced. + if !matches!( + outcome, + Err(crate::broadcaster::BroadcastError::Rejected { .. }) + ) { + if let Some(pin) = in_broadcast_pin.as_mut() { + pin.retain_pending_spend(); + } + } + drop(in_broadcast_pin); + GuardedDispatch::Sent(outcome) } /// Broadcast an atomically finalized transaction. A definitive rejection @@ -315,7 +360,7 @@ mod tests { RejectFirstBroadcaster, WalletSigner, }; use crate::wallet::core::CoreWallet; - use crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS; + use crate::wallet::reservations::{IN_BROADCAST_FENCE_BLOCKS, RESERVATION_MAX_AGE_BLOCKS}; use crate::{PlatformWalletError, SignedCoreTransaction}; /// Builds a testnet `CoreWallet` over the shared funded fixture and a @@ -753,7 +798,7 @@ mod tests { } // Let the dispatch complete: the send succeeds (the age check passed - // before the suspension) and the pin is dropped with it. + // before the suspension). release.wait().await; let sent = dispatcher.await.expect("dispatcher task"); assert!( @@ -761,18 +806,127 @@ mod tests { "the pinned dispatch itself must complete, got {sent:?}" ); - // Pin lifted: a new build may take the input again. (The mock manager - // runs no mempool pipeline; in production the dispatched transaction's - // inputs would be marked spent by processing moments later — the - // assertion here is only that the pin's lifetime ended with the - // dispatch.) + // A new build may take the input again — but note WHY, because it is + // no longer "the pin lifted with the dispatch". The dispatch converted + // its pin into a pending-spend fence bounded at + // `dispatch_height + IN_BROADCAST_FENCE_BLOCKS`, and the catch-up above + // raced 48 blocks past the reservation stamp — well beyond that bound — + // so the fence is already lapsed here. The retained fence itself, and + // the bound it lapses at, are covered by + // `dispatched_input_stays_fenced_after_the_broadcaster_returns`. + assert!( + stamped + RESERVATION_MAX_AGE_BLOCKS + 48 + >= (stamped + RESERVATION_MAX_AGE_BLOCKS - 1) + IN_BROADCAST_FENCE_BLOCKS, + "this test's catch-up must outrun the pending-spend bound for the \ + assertion below to be about the pin, not the fence" + ); let after = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; let after = after.unwrap_or_else(|error| { - panic!("the pin must lift once the dispatch returns, got {error:?}") + panic!("the dispatching pin must lift once the dispatch returns, got {error:?}") }); core.abandon_transaction(&after).await; } + /// `dashpay/platform#4309`: THE RACE THE DISPATCHING PIN ALONE LEFT OPEN. + /// The broadcaster returning is not the spend being observed. The mock + /// manager here runs no mempool pipeline, which is precisely the + /// `DapiBroadcaster` shape — `broadcast` awaits `sdk.execute` and injects + /// nothing into this wallet's state — so at dispatch return the input is + /// still in the selectable set while the transaction is in flight. With the + /// pin dropped at that point, a competing build re-selected it immediately + /// (the previous revision of the test above asserted exactly that). The + /// pending-spend fence keeps it out until + /// `IN_BROADCAST_FENCE_BLOCKS` past the dispatch height, and no longer: + /// a never-observed transaction must not strand its inputs forever. + /// + /// Heights are chosen so the reservation is provably swept while the fence + /// still stands — the state pre-fix was "unreserved AND unfenced". + #[tokio::test] + async fn dispatched_input_stays_fenced_after_the_broadcaster_returns() { + 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; + + // Dispatch at the OLDEST height the age guard still admits — one below + // `RESERVATION_MAX_AGE_BLOCKS`. That is what separates the two clocks: + // the reservation's TTL runs from `stamped`, the fence's bound from + // here, so there is a window in which the reservation is swept and only + // the fence protects the input. (A handle sitting between finalize and + // broadcast is exactly how that gap arises in production.) + let dispatch_height = stamped + RESERVATION_MAX_AGE_BLOCKS - 1; + advance_processed_height(&core, dispatch_height).await; + assert!(core + .broadcast_finalized_transaction(&finalized) + .await + .is_ok()); + + // Catch-up past key-wallet's 24-block reservation TTL (measured from + // the reservation stamp), so the funding reservation is swept and the + // input returns to the selectable pool — but still short of the fence's + // dispatch-anchored bound. The fence is now the ONLY thing holding it; + // pre-fix this window was unreserved AND unfenced. + let swept_but_fenced = stamped + IN_BROADCAST_FENCE_BLOCKS + 4; + assert!( + swept_but_fenced >= stamped + 24 + && swept_but_fenced < dispatch_height + IN_BROADCAST_FENCE_BLOCKS, + "the probe height must be past key-wallet's reservation TTL and below the fence bound" + ); + advance_processed_height(&core, swept_but_fenced).await; + let racing = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + match racing { + Err(PlatformWalletError::TransactionBuild(message)) => assert!( + message.contains("mid-broadcast"), + "the post-dispatch refusal must name the in-flight broadcast, got: {message}" + ), + other => panic!( + "an input handed to the network must stay fenced after the \ + broadcaster returns, got {other:?}" + ), + } + + // At the bound the fence lapses and the input is selectable again. + advance_processed_height(&core, dispatch_height + IN_BROADCAST_FENCE_BLOCKS).await; + let after = try_finalize_tx(&core, AccountTypePreference::BIP44, &outputs, &signer).await; + let after = after + .unwrap_or_else(|error| panic!("the fence must lapse at its bound, 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 e9afe8d200b..6f91d8fa487 100644 --- a/packages/rs-platform-wallet/src/wallet/core/generation.rs +++ b/packages/rs-platform-wallet/src/wallet/core/generation.rs @@ -63,8 +63,8 @@ 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 **pinned by an in-flight broadcast dispatch** - /// ([`pin_in_broadcast`](Self::pin_in_broadcast)), counted per outpoint. + /// 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 @@ -75,28 +75,72 @@ pub struct WalletGeneration { /// `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 non-expiring pin 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 pinned input, closing the sweep + re-reserve window - /// for as long as the dispatch is in flight. + /// 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. /// - /// A *count* per outpoint 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. + /// # Two phases, because dispatch return is not "the spend is safe" + /// + /// [`InBroadcastFence`] holds both phases per outpoint: + /// + /// * **dispatching** — a counted, non-expiring pin, live from check-and-pin + /// until the broadcaster returns. + /// * **pending-spend** — a height-bounded fence installed *when the + /// broadcaster returns anything other than a definitive pre-send + /// rejection*, i.e. when the transaction may be on the network. + /// + /// 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`). + /// + /// 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`] unpin from a plain - /// (non-async) `Drop` — which is also what makes the pin + /// 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. - /// Never persisted: after a restart nothing is mid-dispatch. - in_broadcast: Mutex>, + /// Never persisted: after a restart nothing is mid-dispatch, and a + /// transaction that actually landed is reconciled by sync. + in_broadcast: Mutex>, +} + +/// One outpoint's broadcast fence — see `WalletGeneration::in_broadcast`. +#[derive(Debug, Default)] +struct InBroadcastFence { + /// Dispatches currently *inside* the broadcaster await for this outpoint. + /// Non-expiring while non-zero: a suspended dispatch keeps its inputs + /// fenced no matter how far catch-up advances the clock. + dispatching: u32, + /// `last_processed_height` at which the pending-spend phase lapses, set when + /// a dispatch returns without a definitive pre-send rejection. `None` means + /// no dispatch has handed this outpoint to the network. + pending_until: Option, +} + +impl InBroadcastFence { + /// Whether this fence still blocks re-selection at `current_height`. + fn blocks(&self, current_height: u32) -> bool { + self.dispatching > 0 + || self + .pending_until + .is_some_and(|until| current_height < until) + } } impl Default for WalletGeneration { @@ -171,7 +215,7 @@ impl WalletGeneration { /// 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> { + fn in_broadcast_lock(&self) -> MutexGuard<'_, HashMap> { self.in_broadcast .lock() .unwrap_or_else(PoisonError::into_inner) @@ -192,17 +236,27 @@ impl WalletGeneration { /// broadcaster await the guard must not span (see the /// [`in_broadcast`](Self::in_broadcast) field docs for the full race). /// - /// The pin has **no TTL** — a suspended dispatch keeps its inputs fenced no - /// matter how far catch-up advances the clock — and is released only by - /// dropping the returned guard object, which happens even when the + /// The dispatching phase has **no TTL** — a suspended dispatch keeps its + /// inputs fenced no matter how far catch-up advances the clock — 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). /// + /// `dispatch_height` is the `last_processed_height` sampled in the *same* + /// guarded section as the freshness check. It anchors the pending-spend + /// phase that [`InBroadcastPin::retain_pending_spend`] installs, so that + /// phase is measured from the moment the transaction was authorized to go + /// to the network rather than from the much older reservation stamp. + /// /// 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 { + pub(crate) fn pin_in_broadcast( + self: &Arc, + transaction: &Transaction, + dispatch_height: u32, + ) -> InBroadcastPin { let outpoints: Vec = transaction .input .iter() @@ -211,29 +265,45 @@ impl WalletGeneration { { let mut pinned = self.in_broadcast_lock(); for outpoint in &outpoints { - *pinned.entry(*outpoint).or_insert(0) += 1; + pinned.entry(*outpoint).or_default().dispatching += 1; } } InBroadcastPin { generation: Arc::clone(self), outpoints, + dispatch_height, + retain_pending_spend: false, } } - /// The first of `transaction`'s inputs that is currently pinned by an - /// in-flight broadcast dispatch, or `None` when the selection is clear. + /// 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 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 pinned input is still - /// *reserved* and never reaches selection at all; this check is the - /// backstop for exactly the post-sweep window. - pub(crate) fn in_broadcast_conflict(&self, transaction: &Transaction) -> Option { - let pinned = self.in_broadcast_lock(); + /// 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. + /// + /// `current_height` is the caller's `last_processed_height`, read under the + /// same write guard — the identical clock the pending-spend bound was + /// stamped against and the one key-wallet's TTL sweep runs on. + /// + /// Lapsed 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 bounded by + /// the outpoints dispatched since the last build without any background + /// task. + pub(crate) fn in_broadcast_conflict( + &self, + transaction: &Transaction, + current_height: u32, + ) -> Option { + let mut pinned = self.in_broadcast_lock(); + pinned.retain(|_, fence| fence.blocks(current_height)); transaction .input .iter() @@ -241,36 +311,80 @@ impl WalletGeneration { .find(|outpoint| pinned.contains_key(outpoint)) } - /// Drop one pin count for each of `outpoints` — the [`InBroadcastPin`] - /// release half of [`pin_in_broadcast`](Self::pin_in_broadcast). - fn unpin_in_broadcast(&self, outpoints: &[OutPoint]) { + /// End one dispatch's hold on `outpoints` — the [`InBroadcastPin`] release + /// half of [`pin_in_broadcast`](Self::pin_in_broadcast). + /// + /// `pending_until` is `Some(height)` when that dispatch reached the network + /// (anything but a definitive pre-send rejection): the dispatching count + /// drops but the outpoint stays fenced until `height`. It is `None` for a + /// 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. + fn unpin_in_broadcast(&self, outpoints: &[OutPoint], pending_until: Option) { let mut pinned = self.in_broadcast_lock(); for outpoint in outpoints { - match pinned.get_mut(outpoint) { - Some(count) if *count > 1 => *count -= 1, - Some(_) => { - pinned.remove(outpoint); - } - // Unreachable by construction — every pin increments before its - // guard can decrement — but a miscount must not panic a Drop. - None => debug_assert!(false, "unpin of an outpoint that was never pinned"), + 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); + if let Some(until) = pending_until { + // Never shorten a fence another dispatch already extended: two + // concurrent dispatches of the same transaction must both be + // covered, so the later bound wins. + fence.pending_until = Some(fence.pending_until.map_or(until, |cur| cur.max(until))); + } + if fence.dispatching == 0 && fence.pending_until.is_none() { + pinned.remove(outpoint); } } } } -/// RAII guard for one dispatch's in-broadcast input pins — see +/// 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) releases -/// exactly the pins that call took, count-wise, never another dispatch's. +/// unwind, or the dispatching future being cancelled mid-await) ends exactly +/// the dispatching hold that call took, count-wise, never another dispatch's. +/// +/// By default the drop frees the outpoints outright: a guard dropped without +/// [`retain_pending_spend`](Self::retain_pending_spend) means the transaction +/// never reached the network (a definitive pre-send rejection, or a cancelled +/// dispatch), so there is nothing on the wire to fence against. pub(crate) struct InBroadcastPin { generation: Arc, outpoints: Vec, + /// `last_processed_height` sampled in the guarded section that took this + /// pin — the anchor for the pending-spend bound. + dispatch_height: u32, + /// Set by [`retain_pending_spend`](Self::retain_pending_spend). + retain_pending_spend: bool, +} + +impl InBroadcastPin { + /// Convert this pin, on drop, into a pending-spend fence lasting + /// [`IN_BROADCAST_FENCE_BLOCKS`](crate::wallet::reservations::IN_BROADCAST_FENCE_BLOCKS) + /// blocks past the height the dispatch was authorized at. + /// + /// Called once the broadcaster has returned anything other than a + /// definitive pre-send rejection — i.e. once the transaction may be on the + /// network but this wallet has not necessarily observed the spend yet. See + /// the `WalletGeneration::in_broadcast` field docs for why dispatch return + /// is not, by itself, safe. + pub(crate) fn retain_pending_spend(&mut self) { + self.retain_pending_spend = true; + } } impl Drop for InBroadcastPin { fn drop(&mut self) { - self.generation.unpin_in_broadcast(&self.outpoints); + let pending_until = self.retain_pending_spend.then(|| { + self.dispatch_height + .saturating_add(crate::wallet::reservations::IN_BROADCAST_FENCE_BLOCKS) + }); + self.generation + .unpin_in_broadcast(&self.outpoints, pending_until); } } @@ -289,6 +403,11 @@ mod tests { use dashcore::{OutPoint, Transaction, TxIn, Txid}; use super::WalletGeneration; + use crate::wallet::reservations::IN_BROADCAST_FENCE_BLOCKS; + + /// The height every test pins at, so a fence installed by + /// `retain_pending_spend` lapses at `DISPATCH_HEIGHT + IN_BROADCAST_FENCE_BLOCKS`. + const DISPATCH_HEIGHT: u32 = 1_000; /// A minimal transaction spending exactly the given outpoints — the only /// part of a transaction the pin machinery reads. @@ -313,9 +432,11 @@ mod tests { } /// A held pin flags every input of the pinned transaction — and only - /// those — and dropping the pin clears the conflict. This is the RAII - /// contract the dispatch relies on for cancellation-safety: a dispatching - /// future dropped mid-await unpins exactly the same way. + /// those — and dropping a pin that was NOT retained clears the conflict. + /// This is the RAII contract the dispatch relies on for + /// cancellation-safety: a dispatching future dropped mid-await never + /// reaches `retain_pending_spend`, so it unpins outright, exactly as a + /// definitive pre-send rejection does. #[test] fn pin_flags_inputs_until_dropped() { let generation = Arc::new(WalletGeneration::new()); @@ -323,27 +444,131 @@ mod tests { let b = outpoint(0x02, 1); let unrelated = outpoint(0x03, 0); - let pin = generation.pin_in_broadcast(&spending(&[a, b])); + let pin = generation.pin_in_broadcast(&spending(&[a, b]), DISPATCH_HEIGHT); // Both pinned inputs conflict; an unrelated selection does not. - 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])), + generation.in_broadcast_conflict(&spending(&[a]), DISPATCH_HEIGHT), + Some(a) + ); + assert_eq!( + generation.in_broadcast_conflict(&spending(&[b]), DISPATCH_HEIGHT), + Some(b) + ); + assert_eq!( + generation.in_broadcast_conflict(&spending(&[unrelated, a]), DISPATCH_HEIGHT), Some(a), "a mixed selection must surface its pinned input" ); assert_eq!( - generation.in_broadcast_conflict(&spending(&[unrelated])), + generation.in_broadcast_conflict(&spending(&[unrelated]), DISPATCH_HEIGHT), None ); drop(pin); assert_eq!( - generation.in_broadcast_conflict(&spending(&[a, b])), + generation.in_broadcast_conflict(&spending(&[a, b]), DISPATCH_HEIGHT), + None, + "dropping an unretained pin must clear the conflict" + ); + } + + /// The dispatching pin has no TTL: however far catch-up advances the + /// clock while the broadcaster is suspended, the inputs stay fenced. + #[test] + fn dispatching_pin_never_expires() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(0x05, 0); + let tx = spending(&[a]); + + let _pin = generation.pin_in_broadcast(&tx, DISPATCH_HEIGHT); + + assert_eq!( + generation.in_broadcast_conflict(&tx, DISPATCH_HEIGHT + 10_000), + Some(a), + "a suspended dispatch must keep its inputs fenced at any height" + ); + } + + /// `dashpay/platform#4309`: a dispatch that reached the network keeps its + /// inputs fenced AFTER the broadcaster returns — the DAPI path performs no + /// local mempool injection, so the wallet has not observed the spend yet + /// and the outpoint would otherwise be immediately re-selectable. The + /// fence lapses only once the clock has advanced a full + /// `IN_BROADCAST_FENCE_BLOCKS` past the dispatch height. + #[test] + fn retained_pin_fences_past_dispatch_until_the_bound() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(0x30, 0); + let tx = spending(&[a]); + + let mut pin = generation.pin_in_broadcast(&tx, DISPATCH_HEIGHT); + pin.retain_pending_spend(); + drop(pin); + + assert_eq!( + generation.in_broadcast_conflict(&tx, DISPATCH_HEIGHT), + Some(a), + "the fence must survive the dispatch return" + ); + assert_eq!( + generation.in_broadcast_conflict(&tx, DISPATCH_HEIGHT + IN_BROADCAST_FENCE_BLOCKS - 1), + Some(a), + "one block below the bound must still fence" + ); + assert_eq!( + generation.in_broadcast_conflict(&tx, DISPATCH_HEIGHT + IN_BROADCAST_FENCE_BLOCKS), + None, + "exactly at the bound the fence lapses" + ); + } + + /// A lapsed fence is reaped, not merely ignored: the read that observes + /// the lapse is what prunes the entry, so the map cannot grow without + /// bound across dispatches. + #[test] + fn lapsed_fences_are_reaped_on_read() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(0x31, 0); + let unrelated = outpoint(0x32, 0); + + let mut pin = generation.pin_in_broadcast(&spending(&[a]), DISPATCH_HEIGHT); + pin.retain_pending_spend(); + drop(pin); + assert_eq!(generation.in_broadcast_lock().len(), 1); + + // A read past the bound — about an unrelated selection — still reaps. + assert_eq!( + generation.in_broadcast_conflict( + &spending(&[unrelated]), + DISPATCH_HEIGHT + IN_BROADCAST_FENCE_BLOCKS + ), + None + ); + assert!( + generation.in_broadcast_lock().is_empty(), + "the lapsed entry must be pruned by the read that observed the lapse" + ); + } + + /// The rejection path is the ONLY one that frees inputs at dispatch + /// return, and it frees them completely — no residual pending-spend fence + /// keeps an immediate rebuild out. + #[test] + fn rejected_dispatch_frees_the_input_immediately() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(0x33, 0); + let tx = spending(&[a]); + + // No `retain_pending_spend` — this models `BroadcastError::Rejected`. + drop(generation.pin_in_broadcast(&tx, DISPATCH_HEIGHT)); + + assert_eq!( + generation.in_broadcast_conflict(&tx, DISPATCH_HEIGHT), None, - "dropping the pin must clear the conflict" + "a definitively rejected send must not fence its inputs" ); + assert!(generation.in_broadcast_lock().is_empty()); } /// Pins COUNT per outpoint: two concurrent dispatches of the same @@ -357,18 +582,45 @@ mod tests { let a = outpoint(0x10, 0); let tx = spending(&[a]); - let first = generation.pin_in_broadcast(&tx); - let second = generation.pin_in_broadcast(&tx); + let first = generation.pin_in_broadcast(&tx, DISPATCH_HEIGHT); + let second = generation.pin_in_broadcast(&tx, DISPATCH_HEIGHT); drop(first); assert_eq!( - generation.in_broadcast_conflict(&tx), + generation.in_broadcast_conflict(&tx, DISPATCH_HEIGHT), Some(a), "one dispatch still in flight must keep the outpoint fenced" ); drop(second); - assert_eq!(generation.in_broadcast_conflict(&tx), None); + assert_eq!(generation.in_broadcast_conflict(&tx, DISPATCH_HEIGHT), None); + } + + /// Two concurrent dispatches of the same transaction that BOTH reach the + /// network must leave the longer fence standing — a first completion at a + /// lower dispatch height must not shorten the second's protection. + #[test] + fn the_longer_pending_fence_wins() { + let generation = Arc::new(WalletGeneration::new()); + let a = outpoint(0x11, 0); + let tx = spending(&[a]); + + let mut early = generation.pin_in_broadcast(&tx, DISPATCH_HEIGHT); + let mut late = generation.pin_in_broadcast(&tx, DISPATCH_HEIGHT + 5); + late.retain_pending_spend(); + drop(late); + early.retain_pending_spend(); + drop(early); + + assert_eq!( + generation.in_broadcast_conflict(&tx, DISPATCH_HEIGHT + IN_BROADCAST_FENCE_BLOCKS), + Some(a), + "the later dispatch's bound must win over the earlier one's" + ); + assert_eq!( + generation.in_broadcast_conflict(&tx, DISPATCH_HEIGHT + 5 + IN_BROADCAST_FENCE_BLOCKS), + None + ); } /// Pins are per generation: a re-created wallet's fresh generation starts @@ -378,11 +630,11 @@ mod tests { fn pins_do_not_cross_generations() { let old_generation = Arc::new(WalletGeneration::new()); let a = outpoint(0x20, 0); - let _pin = old_generation.pin_in_broadcast(&spending(&[a])); + let _pin = old_generation.pin_in_broadcast(&spending(&[a]), DISPATCH_HEIGHT); let new_generation = Arc::new(WalletGeneration::new()); assert_eq!( - new_generation.in_broadcast_conflict(&spending(&[a])), + new_generation.in_broadcast_conflict(&spending(&[a]), DISPATCH_HEIGHT), None, "a fresh generation must not inherit the old generation's pins" ); diff --git a/packages/rs-platform-wallet/src/wallet/core/transaction.rs b/packages/rs-platform-wallet/src/wallet/core/transaction.rs index 9aeaba9e99b..76f21957c93 100644 --- a/packages/rs-platform-wallet/src/wallet/core/transaction.rs +++ b/packages/rs-platform-wallet/src/wallet/core/transaction.rs @@ -431,7 +431,10 @@ impl CoreWallet { // (`WalletGeneration::pin_in_broadcast`). Still under the write // guard, so the check is atomic with our reservation and the // release is exact. - if let Some(pinned) = info.generation.in_broadcast_conflict(&unsigned) { + if let Some(pinned) = info + .generation + .in_broadcast_conflict(&unsigned, info.core_wallet.last_processed_height()) + { release_all!(offered_accounts, info.core_wallet.accounts, &unsigned); return Err(PlatformWalletError::TransactionBuild(format!( "selected input {pinned} is mid-broadcast by an in-flight dispatch; \ 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 b86761004a2..5651d77f65e 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -1322,7 +1322,10 @@ impl DashPayView<'_, B> { // nothing no-op. Roll back the consumed payment address exactly // like the build-failure arm above — nothing was persisted or // broadcast. - if let Some(pinned) = info.generation.in_broadcast_conflict(&tx) { + if let Some(pinned) = info + .generation + .in_broadcast_conflict(&tx, info.core_wallet.last_processed_height()) + { for at in &offered_accounts { if let Some(managed) = info.core_wallet.accounts.funds_account_mut(at) { managed.release_reservation(&tx); diff --git a/packages/rs-platform-wallet/src/wallet/reservations.rs b/packages/rs-platform-wallet/src/wallet/reservations.rs index 3b924a76aea..c133aed7faf 100644 --- a/packages/rs-platform-wallet/src/wallet/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/reservations.rs @@ -50,6 +50,52 @@ use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; /// for `last_processed_height` to lag a few blocks behind the true tip. pub(crate) const RESERVATION_MAX_AGE_BLOCKS: u32 = 20; +/// How long, in `last_processed_height` blocks past **dispatch**, a transaction +/// that reached the network keeps its inputs fenced against re-selection by +/// [`WalletGeneration::pin_in_broadcast`](crate::wallet::core::WalletGeneration::pin_in_broadcast)'s +/// pending-spend phase. +/// +/// # Why a fence past dispatch is needed at all +/// +/// `SpvBroadcaster` injects the dispatched transaction into dash-spv's local +/// mempool pipeline, so on that path the wallet marks the inputs spent within +/// milliseconds of dispatch returning and they leave the selectable set on +/// their own. `DapiBroadcaster::broadcast` does no such injection — it awaits +/// `sdk.execute` and returns — so on the DAPI path an accepted response *and* +/// an ambiguous `MaybeSent` both return with the inputs still selectable here +/// while the transaction is in flight. Ending the fence at dispatch return +/// therefore reopens the sweep + re-select race on that path +/// (`dashpay/platform#4309`): key-wallet's `ReservationSet` TTL is stamped at +/// *build* time, so a handle that sat between `finalize` and broadcast can be +/// swept the instant the next selection runs. +/// +/// # Why exactly key-wallet's TTL, re-anchored at dispatch +/// +/// The correct fix would be to renew the underlying reservation at dispatch so +/// its TTL runs from the moment the transaction actually went to the network; +/// key-wallet exposes no such primitive at the pinned revision (`ReservationSet` +/// and its `RESERVATION_TTL_BLOCKS` are private). This constant is that renewal +/// implemented one layer up: **24, key-wallet's own `RESERVATION_TTL_BLOCKS`** +/// (~1 h at the mainnet block target), measured from dispatch instead of from +/// the build. The inputs are then continuously protected — by the reservation +/// until its build-anchored TTL, then by this fence — for a full TTL past the +/// moment they were actually committed to the network, which is the point the +/// TTL was always meant to be measured from. Coupled by convention, exactly as +/// [`RESERVATION_MAX_AGE_BLOCKS`] above is: if key-wallet's TTL changes, change +/// this in lockstep. +/// +/// # Why it must lapse +/// +/// A fenced outpoint that the wallet has already observed as spent never +/// reaches a selection in the first place, so in the common case this bound is +/// never consulted — the fence goes inert on its own. The bound exists for the +/// transaction that is *never* observed (dropped from mempool for fee or +/// conflict): its reservation is already gone at TTL, and a non-expiring fence +/// would strand those funds permanently with nothing able to clear it. Lapsing +/// at the same TTL leaves the residual exposure identical to the one +/// key-wallet's reservation TTL already accepts, and no larger. +pub(crate) const IN_BROADCAST_FENCE_BLOCKS: u32 = 24; + /// 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 From 6cb9cf793382a166861e32acc3ed8e541bf784d0 Mon Sep 17 00:00:00 2001 From: bfoss765 Date: Thu, 13 Aug 2026 12:53:50 -0400 Subject: [PATCH 8/8] refactor(platform-wallet): anchor the broadcast fence on the checked height MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dispatch_unexpired` sampled `last_processed_height` twice under the same manager read guard — once for the freshness check, once for the pin's anchor. Both reads are identical in practice, but the pin is now taken via `info.zip(height)` so the fence is anchored on the very value the check consumed and the two cannot drift apart. Co-Authored-By: Claude Opus 4.8 --- .../src/wallet/core/broadcast.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 51288900b1a..1d239f96c93 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -109,12 +109,14 @@ impl CoreWallet { // 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 anchored at the - // height sampled right here. - info.map(|info| { - info.generation - .pin_in_broadcast(transaction, info.core_wallet.last_processed_height()) - }) + // broadcaster return too, as a pending-spend fence. + // + // That fence is anchored on the SAME `height` the freshness check + // just consumed, not a fresh sample: the two must not be able to + // disagree, or the fence could be stamped against a clock the + // check never saw. + info.zip(height) + .map(|(info, height)| info.generation.pin_in_broadcast(transaction, height)) // 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.