diff --git a/key-wallet/src/lib.rs b/key-wallet/src/lib.rs index 89b64b949..b777b8b9d 100644 --- a/key-wallet/src/lib.rs +++ b/key-wallet/src/lib.rs @@ -60,6 +60,7 @@ pub use managed_account::address_pool::{AddressInfo, AddressPool, KeySource, Poo pub use managed_account::managed_account_type::ManagedAccountType; pub use managed_account::managed_platform_account::ManagedPlatformAccount; pub use managed_account::platform_address::PlatformP2PKHAddress; +pub use managed_account::ReservationToken; pub use mnemonic::{Language, Mnemonic}; pub use seed::Seed; pub use signer::{ExtendedPubKeySigner, Signer, SignerMethod, TransactionCategory}; diff --git a/key-wallet/src/managed_account/managed_core_funds_account.rs b/key-wallet/src/managed_account/managed_core_funds_account.rs index 1f7001f67..2418152dc 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -19,7 +19,7 @@ use crate::managed_account::address_pool; use crate::managed_account::managed_account_trait::ManagedAccountTrait; use crate::managed_account::managed_account_type::ManagedAccountType; use crate::managed_account::managed_core_keys_account::ManagedCoreKeysAccount; -use crate::managed_account::reservation::ReservationSet; +use crate::managed_account::reservation::{ReservationSet, ReservationToken}; use crate::managed_account::transaction_record::{ InputDetail, OutputDetail, OutputRole, TransactionDirection, }; @@ -130,6 +130,25 @@ impl ManagedCoreFundsAccount { self.reservations.release(tx.input.iter().map(|input| &input.previous_output)); } + /// Owner-guarded release of `tx`'s input reservations: releases an input + /// only if it is *still owned by* `token`, the [`ReservationToken`] returned + /// when this build reserved its inputs (from + /// [`build_unsigned_reserved`]/[`build_signed_reserved`]). + /// + /// This is the release a caller must use when it abandons a transaction + /// *after having `.await`ed something* between reserving and releasing — + /// above all the platform broadcast path — never the unconditional + /// [`Self::release_reservation`]. See `ReservationSet::release_if_owner` for + /// the release/re-reserve race this closes and why the owner check must live + /// inside key-wallet (`dashpay/platform#4185`). + /// + /// [`build_unsigned_reserved`]: crate::wallet::managed_wallet_info::transaction_builder::TransactionBuilder::build_unsigned_reserved + /// [`build_signed_reserved`]: crate::wallet::managed_wallet_info::transaction_builder::TransactionBuilder::build_signed_reserved + pub fn release_reservation_if_owner(&self, tx: &Transaction, token: ReservationToken) { + let outpoints: Vec = tx.input.iter().map(|input| input.previous_output).collect(); + self.reservations.release_if_owner(&outpoints, token); + } + /// Get a reference to the inner keys-account state. pub fn keys(&self) -> &ManagedCoreKeysAccount { &self.keys diff --git a/key-wallet/src/managed_account/mod.rs b/key-wallet/src/managed_account/mod.rs index 5d1766c5f..7a5b5daff 100644 --- a/key-wallet/src/managed_account/mod.rs +++ b/key-wallet/src/managed_account/mod.rs @@ -27,3 +27,4 @@ pub mod transaction_record; pub use managed_account_ref::{ManagedAccountRef, ManagedAccountRefMut, OwnedManagedCoreAccount}; pub use managed_core_funds_account::ManagedCoreFundsAccount; pub use managed_core_keys_account::ManagedCoreKeysAccount; +pub use reservation::ReservationToken; diff --git a/key-wallet/src/managed_account/reservation.rs b/key-wallet/src/managed_account/reservation.rs index db6fd9adf..1715c5fd4 100644 --- a/key-wallet/src/managed_account/reservation.rs +++ b/key-wallet/src/managed_account/reservation.rs @@ -12,6 +12,16 @@ //! held survives the lock being released and is observed by the next build. It //! is never persisted: after a restart it is empty, where chain and mempool //! sync are the source of truth for which coins are already spent. +//! +//! # Why reservations carry an owner token +//! +//! Releasing a reservation by outpoint alone is unsafe once *releasing* and +//! *re-reserving* can interleave. Every reservation is therefore stamped with a +//! [`ReservationToken`] identifying the build that made it, and the +//! abandon/rejected-broadcast release path ([`ReservationSet::release_if_owner`]) +//! removes an outpoint only if it is *still owned by the releasing build*. See +//! [`ReservationSet::release_if_owner`] for the concrete hazard this closes and +//! why the check must be atomic under this set's mutex (`dashpay/platform#4185`). use std::collections::{HashMap, HashSet}; use std::sync::{Arc, Mutex, MutexGuard}; @@ -31,24 +41,68 @@ use dashcore::blockdata::transaction::OutPoint; /// the very double-spend this guards against. const RESERVATION_TTL_BLOCKS: u32 = 24; +/// Opaque, per-`reserve`-call identity stamped onto every outpoint that call +/// reserves. +/// +/// A token is the *proof of ownership* a build presents to the reservation +/// set's `release_if_owner` so a release only removes the build's own +/// reservation. Each `reserve` call mints a fresh token from a monotonic +/// counter, so two reservations never share one — not even two reservations +/// taken at the same block height, which is why the height (which collides +/// freely) cannot serve as the identity. +/// +/// The inner counter is private and there is no public constructor: a token can +/// only originate from a real `reserve` call. That is deliberate — it prevents a +/// caller from forging a token that happens to match another build's ownership +/// and releasing inputs out from under it. +/// +/// Copy semantics let a build hold its token cheaply across an `.await` (e.g. +/// the platform broadcast path in `dashpay/platform#4185`) and present it again +/// when releasing. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct ReservationToken(u64); + +/// A single reserved outpoint: when it was reserved (for the TTL backstop) and +/// which build owns it (for owner-guarded release). +#[derive(Debug, Clone, Copy)] +struct Reservation { + /// Block height at which the reservation was taken; drives the TTL sweep. + reserved_at_height: u32, + /// The build that reserved this outpoint. Only this owner may release it via + /// [`ReservationSet::release_if_owner`]. + owner: ReservationToken, +} + +/// Mutex-guarded interior: the reservations keyed by outpoint plus the counter +/// that mints the next [`ReservationToken`]. +#[derive(Debug, Default)] +struct Reserved { + entries: HashMap, + /// Monotonically increasing source of unique tokens. Never persisted and + /// only ever incremented, so within a process every issued token is unique. + /// (Wraparound would need ~2^64 reserves in one process lifetime, which is + /// unreachable in practice.) + next_token: u64, +} + /// Ephemeral, in-memory set of reserved outpoints. Cloning shares the /// underlying state, which is what lets a build's reservation outlive the /// wallet lock and be seen by the next build. #[derive(Debug, Clone, Default)] pub(crate) struct ReservationSet { - inner: Arc>>, + inner: Arc>, } impl ReservationSet { /// Recovers from a poisoned mutex rather than panicking: the guarded data is - /// a plain `HashMap` with no invariant a partial write could break, and - /// panicking here would strand all later coin selection in a long-running - /// node. - fn lock(&self) -> MutexGuard<'_, HashMap> { + /// a plain map plus a counter with no invariant a partial write could break, + /// and panicking here would strand all later coin selection in a + /// long-running node. + fn lock(&self) -> MutexGuard<'_, Reserved> { self.inner.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) } - fn sweep(reserved: &mut HashMap, current_height: u32) { + fn sweep(reserved: &mut Reserved, current_height: u32) { // Height 0 means the wallet has no processed height yet, so the elapsed // span is unknown and no entry can be reliably judged stale. An entry // stamped at height 0 therefore relies on a later non-zero height being @@ -56,34 +110,101 @@ impl ReservationSet { if current_height == 0 { return; } - reserved.retain(|_, reserved_at| { - current_height.saturating_sub(*reserved_at) < RESERVATION_TTL_BLOCKS + reserved.entries.retain(|_, reservation| { + current_height.saturating_sub(reservation.reserved_at_height) < RESERVATION_TTL_BLOCKS }); } /// Reserve `outpoints` as of `current_height`, dropping expired entries - /// first. Re-reserving an outpoint refreshes its height. - pub(crate) fn reserve(&self, outpoints: &[OutPoint], current_height: u32) { + /// first, and return the [`ReservationToken`] stamped onto all of them. + /// + /// Every outpoint in a single call shares the one returned token: the caller + /// keeps it and later presents it to [`Self::release_if_owner`] to release + /// only what this call reserved. Re-reserving an outpoint (a later `reserve` + /// naming it again) refreshes its height *and* transfers ownership to the new + /// token — the previous owner's [`Self::release_if_owner`] then becomes a + /// no-op for it, which is precisely the behavior that closes the + /// release/re-reserve race described in the module docs (`platform#4185`). + pub(crate) fn reserve(&self, outpoints: &[OutPoint], current_height: u32) -> ReservationToken { let mut reserved = self.lock(); Self::sweep(&mut reserved, current_height); + + let owner = ReservationToken(reserved.next_token); + // Increment even on an empty `outpoints` slice so a token is never + // reissued; wrapping is documented-unreachable but avoids a debug panic. + reserved.next_token = reserved.next_token.wrapping_add(1); + for outpoint in outpoints { - reserved.insert(*outpoint, current_height); + reserved.entries.insert( + *outpoint, + Reservation { + reserved_at_height: current_height, + owner, + }, + ); } + owner } /// Return the currently reserved outpoints, dropping expired entries first. pub(crate) fn reserved(&self, current_height: u32) -> HashSet { let mut reserved = self.lock(); Self::sweep(&mut reserved, current_height); - reserved.keys().copied().collect() + reserved.entries.keys().copied().collect() } - /// Release the given outpoints. Idempotent: releasing an outpoint that is - /// not reserved does nothing. + /// Unconditionally release the given outpoints, regardless of owner. + /// Idempotent: releasing an outpoint that is not reserved does nothing. + /// + /// This is correct only where the coins are *known spent* and so must leave + /// the set no matter which build reserved them — e.g. when a processed spend + /// hands its inputs from this ephemeral set to the durable spent set. A + /// caller that is merely abandoning an in-flight build (rejected broadcast, + /// cancelled send) must use [`Self::release_if_owner`] instead, so it cannot + /// free a reservation another build has since taken over. pub(crate) fn release<'a>(&self, outpoints: impl IntoIterator) { let mut reserved = self.lock(); for outpoint in outpoints { - reserved.remove(outpoint); + reserved.entries.remove(outpoint); + } + } + + /// Release only the `outpoints` still owned by `token`, atomically under this + /// set's mutex. An outpoint reserved by a different owner — because the TTL + /// sweep reclaimed this build's reservation and another build re-reserved it + /// meanwhile — is left untouched. + /// + /// This is the owner-guarded counterpart to [`Self::release`] and the whole + /// reason reservations carry a [`ReservationToken`]; it is the canonical + /// explanation the rest of the reservation machinery points back to. It is + /// the release a build must use when it abandons an in-flight transaction + /// after having `.await`ed something (a broadcast, an external signer): + /// during that await the TTL sweep can reclaim this build's reservation and a + /// *different* concurrent build can re-reserve the very same outpoint under a + /// new token (same wallet generation, so any `Arc::ptr_eq` generation guard + /// still matches). An unconditional release-by-outpoint would then free that + /// other build's inputs, letting coin selection hand them to a second + /// transaction — a double-spend window (`dashpay/platform#4185`). The + /// platform layer cannot detect this because the sweep happens inside + /// key-wallet invisibly; because the ownership check and the removal happen + /// together while the mutex is held, no sweep or re-reserve can interleave + /// between them. + /// + /// Idempotent and a no-op for any outpoint that is unreserved or owned by a + /// different token — including one this build's own reservation already lost + /// to a sweep, so a late release after reclamation is harmless. + pub(crate) fn release_if_owner(&self, outpoints: &[OutPoint], token: ReservationToken) { + use std::collections::hash_map::Entry; + let mut reserved = self.lock(); + for outpoint in outpoints { + // Single hash lookup per outpoint: the `Entry` locates the slot once, + // and `remove` reuses it — "remove only if still mine", no second + // `get` before the `remove`. + if let Entry::Occupied(entry) = reserved.entries.entry(*outpoint) { + if entry.get().owner == token { + entry.remove(); + } + } } } } @@ -175,4 +296,86 @@ mod tests { set.reserve(&[a], 1); assert!(clone.reserved(1).contains(&a)); } + + #[test] + fn each_reserve_call_mints_a_distinct_token() { + let set = ReservationSet::default(); + // Two reserves at the SAME height must still get different tokens — the + // whole point of not keying ownership on height, which collides. + let token_a = set.reserve(&[outpoint(0x10, 0)], 100); + let token_b = set.reserve(&[outpoint(0x11, 0)], 100); + assert_ne!(token_a, token_b); + } + + #[test] + fn release_if_owner_releases_only_the_owning_builds_outpoints() { + let set = ReservationSet::default(); + let mine = outpoint(0x20, 0); + let theirs = outpoint(0x21, 0); + + let my_token = set.reserve(&[mine], 100); + let _their_token = set.reserve(&[theirs], 100); + + // Releasing with my token frees only my outpoint; theirs is untouched. + set.release_if_owner(&[mine, theirs], my_token); + assert!(!set.reserved(100).contains(&mine)); + assert!(set.reserved(100).contains(&theirs)); + } + + #[test] + fn release_if_owner_is_a_noop_for_unreserved_or_wrong_token() { + let set = ReservationSet::default(); + let a = outpoint(0x22, 0); + let stale_token = set.reserve(&[a], 100); + + // Simulate the outpoint being released and re-reserved by someone else. + set.release([&a]); + let _new_token = set.reserve(&[a], 100); + + // The stale token no longer owns `a`, so its release must not remove it. + set.release_if_owner(&[a], stale_token); + assert!(set.reserved(100).contains(&a)); + + // A token for an outpoint that was never reserved is simply ignored. + let never = outpoint(0x23, 0); + set.release_if_owner(&[never], stale_token); + assert!(!set.reserved(100).contains(&never)); + } + + /// The core TOCTOU regression: reserve X under token A, then simulate the + /// TTL sweep reclaiming X and a *different* concurrent build re-reserving X + /// under token B. Token A's late `release_if_owner` (the rejected-broadcast + /// cleanup) must NOT remove X, because X now belongs to build B — releasing + /// it would hand B's input to coin selection and open a double-spend window. + /// See `dashpay/platform#4185`. + #[test] + fn release_if_owner_does_not_free_a_reservation_taken_over_after_a_sweep() { + let set = ReservationSet::default(); + let x = outpoint(0x30, 0); + + // Build A reserves X. + let token_a = set.reserve(&[x], 100); + assert!(set.reserved(100).contains(&x)); + + // TTL sweep reclaims A's reservation mid-await (modeled by advancing the + // height past the TTL so the next reserve's sweep drops A's entry)... + let swept_height = 100 + RESERVATION_TTL_BLOCKS; + // ...and build B re-reserves the very same outpoint under a new token. + let token_b = set.reserve(&[x], swept_height); + assert_ne!(token_a, token_b); + assert!(set.reserved(swept_height).contains(&x)); + + // Build A's rejected-broadcast cleanup releases by (outpoint, token A). + set.release_if_owner(&[x], token_a); + + // X must still be reserved — it is now owned by build B. + assert!( + set.reserved(swept_height).contains(&x), + "release_if_owner with the stale owner token must not free B's reservation" + ); + + // And B can still release its own reservation normally. + set.release_if_owner(&[x], token_b); + assert!(!set.reserved(swept_height).contains(&x)); + } } diff --git a/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs b/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs index 8c00a9a9e..26595961d 100644 --- a/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs @@ -5,12 +5,12 @@ use dashcore::blockdata::transaction::special_transaction::asset_lock::AssetLockPayload; use dashcore::blockdata::transaction::special_transaction::TransactionPayload; -use dashcore::{Transaction, TxOut}; +use dashcore::{OutPoint, Transaction, TxOut}; use secp256k1::PublicKey; use std::fmt; use crate::managed_account::managed_account_trait::ManagedAccountTrait; -use crate::managed_account::ManagedCoreKeysAccount; +use crate::managed_account::{ManagedCoreKeysAccount, ReservationToken}; use crate::signer::Signer; use crate::wallet::managed_wallet_info::coin_selection::SelectionStrategy; use crate::wallet::managed_wallet_info::fee::FeeRate; @@ -110,6 +110,18 @@ pub struct AssetLockResult { /// Per-credit-output key material. See [`AssetLockCreditKeys`] for /// ordering and variant semantics. pub keys: AssetLockCreditKeys, + /// Owner token for the reservation this build took on the funding inputs, + /// or `None` if the funding account carried no reservation set. + /// + /// The caller broadcasts `transaction` and, on a rejected broadcast, must + /// release the reserved inputs with + /// [`ManagedCoreFundsAccount::release_reservation_if_owner`] passing this + /// token — never the unconditional `release_reservation`. See + /// `ReservationSet::release_if_owner` for why owner-guarded release is + /// required here (`dashpay/platform#4185`). + /// + /// [`ManagedCoreFundsAccount::release_reservation_if_owner`]: crate::managed_account::ManagedCoreFundsAccount::release_reservation_if_owner + pub reservation_token: Option, } /// Errors specific to asset lock transaction building. @@ -306,30 +318,55 @@ impl ManagedWalletInfo { if drain { builder = builder.set_selection_strategy(SelectionStrategy::All); } - let (transaction, fee) = builder + let (transaction, fee, reservation_token) = builder .set_funding(funds_acc, acc) .require_final_inputs() - .build_signed(wallet, |addr| funds_acc.address_derivation_path(&addr)) + .build_signed_reserved(wallet, |addr| funds_acc.address_derivation_path(&addr)) .await?; - // Derive one private key per credit output. - let mut keys = Vec::with_capacity(credit_output_fundings.len()); - for funding in &credit_output_fundings { - let funding_key_account = resolve_funding_account( - &mut self.accounts, - funding.funding_type, - funding.identity_index, - )?; - let key = funding_key_account - .next_private_key(&root_xpriv, network) - .map_err(|e| AssetLockError::KeyDerivation(e.to_string()))?; - keys.push(key); - } + // The build above reserved the funding inputs. Clone the reservation + // handle (a shared `Arc` view of the same set) now, before the loop + // below re-borrows `self.accounts` — a mid-loop failure can no longer + // reach `funds_acc` to release, and the caller never received the token + // to release it either, so a leaked reservation would strand the + // already-signed inputs until the 24-block TTL sweep. Owner-guarded + // release only (see `ReservationSet::release_if_owner`, + // `dashpay/platform#4185`). + let reservations = funds_acc.reservations().clone(); + let reserved: Vec = + transaction.input.iter().map(|input| input.previous_output).collect(); + + // Derive one private key per credit output. On any failure, release + // this build's own reservation before returning. + let keys = match (|| -> Result, AssetLockError> { + let mut keys = Vec::with_capacity(credit_output_fundings.len()); + for funding in &credit_output_fundings { + let funding_key_account = resolve_funding_account( + &mut self.accounts, + funding.funding_type, + funding.identity_index, + )?; + let key = funding_key_account + .next_private_key(&root_xpriv, network) + .map_err(|e| AssetLockError::KeyDerivation(e.to_string()))?; + keys.push(key); + } + Ok(keys) + })() { + Ok(keys) => keys, + Err(e) => { + if let Some(token) = reservation_token { + reservations.release_if_owner(&reserved, token); + } + return Err(e); + } + }; Ok(AssetLockResult { transaction, fee, keys: AssetLockCreditKeys::Private(keys), + reservation_token, }) } @@ -408,12 +445,23 @@ impl ManagedWalletInfo { if drain { builder = builder.set_selection_strategy(SelectionStrategy::All); } - let (transaction, fee) = builder + let (transaction, fee, reservation_token) = builder .set_funding(funds_acc, &acc) .require_final_inputs() - .build_signed(signer, |addr| funds_acc.address_derivation_path(&addr)) + .build_signed_reserved(signer, |addr| funds_acc.address_derivation_path(&addr)) .await?; + // The build above reserved the funding inputs. Clone the reservation + // handle (a shared `Arc` view of the same set) before the bookkeeping + // loop below re-borrows `self.accounts`, so a failure during Phase 1–3 + // — which runs after the transaction is already signed — can still + // release THIS build's reservation instead of stranding the signed + // inputs until the 24-block TTL sweep. Owner-guarded release only (see + // `ReservationSet::release_if_owner`, `dashpay/platform#4185`). + let reservations = funds_acc.reservations().clone(); + let reserved: Vec = + transaction.input.iter().map(|input| input.previous_output).collect(); + // Credit-output bookkeeping: for each funding, peek the next unused // path on its account, ask the signer for the matching pubkey, and // only mark the index used once the signer has succeeded. @@ -421,50 +469,65 @@ impl ManagedWalletInfo { // This protects against a signer failure mid-loop leaving earlier // fundings' pool indices irreversibly consumed: if `public_key` // errors, the current funding's index is still free, and no - // subsequent fundings have touched their pools yet. - let mut credit_output_keys = Vec::with_capacity(credit_output_fundings.len()); - for funding in &credit_output_fundings { - // Phase 1 (sync): peek without marking used. Borrow is scoped - // to the block so we can re-resolve the account after the - // signer await. - let (path, index) = { - let funding_key_account = resolve_funding_account( - &mut self.accounts, - funding.funding_type, - funding.identity_index, - )?; - funding_key_account - .peek_next_path() - .map_err(|e| AssetLockError::KeyDerivation(e.to_string()))? - }; - - // Phase 2 (async): signer round-trip. If this errors, we return - // without ever calling mark_first_pool_index_used — index stays - // free for a retry. - let pubkey = signer - .public_key(&path) - .await - .map_err(|e| AssetLockError::Signer(e.to_string()))?; - - // Phase 3 (sync): signer succeeded, commit the index. - { - let funding_key_account = resolve_funding_account( - &mut self.accounts, - funding.funding_type, - funding.identity_index, - )?; - funding_key_account - .mark_first_pool_index_used(index) - .map_err(|e| AssetLockError::KeyDerivation(e.to_string()))?; + // subsequent fundings have touched their pools yet. On any failure we + // also release this build's own reservation before returning. + let credit_output_keys = match async { + let mut credit_output_keys = Vec::with_capacity(credit_output_fundings.len()); + for funding in &credit_output_fundings { + // Phase 1 (sync): peek without marking used. Borrow is scoped + // to the block so we can re-resolve the account after the + // signer await. + let (path, index) = { + let funding_key_account = resolve_funding_account( + &mut self.accounts, + funding.funding_type, + funding.identity_index, + )?; + funding_key_account + .peek_next_path() + .map_err(|e| AssetLockError::KeyDerivation(e.to_string()))? + }; + + // Phase 2 (async): signer round-trip. If this errors, we return + // without ever calling mark_first_pool_index_used — index stays + // free for a retry. + let pubkey = signer + .public_key(&path) + .await + .map_err(|e| AssetLockError::Signer(e.to_string()))?; + + // Phase 3 (sync): signer succeeded, commit the index. + { + let funding_key_account = resolve_funding_account( + &mut self.accounts, + funding.funding_type, + funding.identity_index, + )?; + funding_key_account + .mark_first_pool_index_used(index) + .map_err(|e| AssetLockError::KeyDerivation(e.to_string()))?; + } + + credit_output_keys.push((pubkey, path)); } - - credit_output_keys.push((pubkey, path)); + Ok::<_, AssetLockError>(credit_output_keys) } + .await + { + Ok(keys) => keys, + Err(e) => { + if let Some(token) = reservation_token { + reservations.release_if_owner(&reserved, token); + } + return Err(e); + } + }; Ok(AssetLockResult { transaction, fee, keys: AssetLockCreditKeys::Public(credit_output_keys), + reservation_token, }) } } diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs index 747f23e8a..076581ed6 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -3,7 +3,7 @@ //! This module provides high-level transaction building functionality //! using types from the dashcore crate. -use crate::managed_account::reservation::ReservationSet; +use crate::managed_account::reservation::{ReservationSet, ReservationToken}; use crate::managed_account::ManagedCoreFundsAccount; use crate::wallet::managed_wallet_info::coin_selection::{ CoinSelector, SelectionStrategy, CHANGE_OUTPUT_SIZE, TX_OUTPUT_SIZE, @@ -282,7 +282,18 @@ impl TransactionBuilder { size } - fn assemble_unsigned(mut self) -> Result<(Transaction, Vec), BuilderError> { + /// Select inputs, build the unsigned transaction, and reserve the chosen + /// inputs. The optional [`ReservationToken`] is `Some` exactly when a + /// reservation set was attached (via [`set_funding`]) and identifies the + /// reservation this build just took, so a caller that later abandons the + /// build can release *only* its own inputs via + /// [`ReservationSet::release_if_owner`]. It is `None` for builds with no + /// reservation set (nothing was reserved, nothing to release). + /// + /// [`set_funding`]: Self::set_funding + fn assemble_unsigned( + mut self, + ) -> Result<(Transaction, Vec, Option), BuilderError> { if let Some(TransactionPayload::AssetLockPayloadType(p)) = &self.special_payload { if p.credit_outputs.is_empty() { return Err(BuilderError::NoOutputs); @@ -424,14 +435,16 @@ impl TransactionBuilder { // Reserve the chosen inputs so a concurrent build skips them until the // broadcast transaction is processed back into the wallet (which - // releases the reservation) or the TTL backstop reclaims it. - if let Some(reservations) = &self.reservations { + // releases the reservation) or the TTL backstop reclaims it. Keep the + // stamped owner token so the caller can release only this reservation + // if the build is later abandoned (see `release_if_owner`). + let reservation_token = self.reservations.as_ref().map(|reservations| { let outpoints: Vec = selected_inputs.iter().map(|utxo| utxo.outpoint).collect(); - reservations.reserve(&outpoints, self.current_height); - } + reservations.reserve(&outpoints, self.current_height) + }); - return Ok((transaction, selected_inputs)); + return Ok((transaction, selected_inputs, reservation_token)); // BIP-69: Sort outputs by amount first, then by scriptPubKey // lexicographically. @@ -454,17 +467,31 @@ impl TransactionBuilder { } } - /// Build the unsigned transaction. The returned fee is the fee the - /// transaction actually pays: Σ(selected input values) − Σ(output values). - /// This can exceed the size-based fee target when a dust change remainder - /// (≤ 546 duffs) is dropped and left to miners. - pub fn build_unsigned(self) -> Result<(Transaction, u64), BuilderError> { - let (tx, inputs) = self.assemble_unsigned()?; + /// Build the unsigned transaction, returning it alongside the fee it pays + /// and the [`ReservationToken`] stamped onto the inputs this build reserved + /// (`None` when no reservation set is attached). + /// + /// The returned fee is the fee the transaction actually pays: + /// Σ(selected input values) − Σ(output values). This can exceed the + /// size-based fee target when a dust change remainder (≤ 546 duffs) is + /// dropped and left to miners. + /// + /// Hold the returned token when the transaction may be abandoned after an + /// `.await` that releases the wallet lock — most importantly the platform + /// broadcast path, which reserves inputs, awaits the broadcast, and on + /// rejection must release them — and release with + /// [`ManagedCoreFundsAccount::release_reservation_if_owner`]. See + /// `ReservationSet::release_if_owner` for why owner-guarded release is + /// required (`dashpay/platform#4185`). + pub fn build_unsigned_reserved( + self, + ) -> Result<(Transaction, u64, Option), BuilderError> { + let (tx, inputs, reservation) = self.assemble_unsigned()?; let total_input: u64 = inputs.iter().map(|utxo| utxo.value()).sum(); let total_output: u64 = tx.output.iter().map(|out| out.value).sum(); - Ok((tx, total_input.saturating_sub(total_output))) + Ok((tx, total_input.saturating_sub(total_output), reservation)) } /// Build and sign the transaction. The `path_resolver` maps each input @@ -478,24 +505,49 @@ impl TransactionBuilder { signer: &S, path_resolver: P, ) -> Result<(Transaction, u64), BuilderError> + where + S: TransactionSigner + ?Sized + Sync, + P: Fn(Address) -> Option + Send, + { + let (tx, fee, _reservation) = self.build_signed_reserved(signer, path_resolver).await?; + Ok((tx, fee)) + } + + /// Like [`Self::build_signed`], but also returns the [`ReservationToken`] + /// stamped onto the inputs this build reserved (`None` when no reservation + /// set is attached), for callers that may later abandon the transaction + /// after awaiting a broadcast. See [`Self::build_unsigned_reserved`] for why + /// the token is needed and how to release with it. + pub async fn build_signed_reserved( + self, + signer: &S, + path_resolver: P, + ) -> Result<(Transaction, u64, Option), BuilderError> where S: TransactionSigner + ?Sized + Sync, P: Fn(Address) -> Option + Send, { let reservations = self.reservations.clone(); - let (tx, inputs) = self.assemble_unsigned()?; + let (tx, inputs, reservation) = self.assemble_unsigned()?; let total_input: u64 = inputs.iter().map(|utxo| utxo.value()).sum(); // Signing never reaches the network for a local key, but an external // signer can fail. A failed sign means the reserved inputs are still // spendable, so release them now instead of stranding the funds until // the TTL backstop reclaims them. + // + // Release owner-guarded: `sign_tx` is an `.await`, and while it runs the + // TTL sweep could reclaim this build's reservation and a concurrent + // build could re-reserve the same outpoint under a new token. An + // unconditional release-by-outpoint would then free that other build's + // inputs (the double-spend window of `dashpay/platform#4185`), so we + // release only outpoints still owned by the token this build stamped. let reserved: Vec = inputs.iter().map(|utxo| utxo.outpoint).collect(); let tx = match signer.sign_tx(tx, inputs, path_resolver).await { Ok(tx) => tx, Err(err) => { - if let Some(reservations) = &reservations { - reservations.release(reserved.iter()); + if let (Some(reservations), Some(token)) = (&reservations, reservation) { + reservations.release_if_owner(&reserved, token); } return Err(err); } @@ -503,7 +555,7 @@ impl TransactionBuilder { let total_output: u64 = tx.output.iter().map(|out| out.value).sum(); - Ok((tx, total_input.saturating_sub(total_output))) + Ok((tx, total_input.saturating_sub(total_output), reservation)) } } @@ -702,8 +754,8 @@ mod tests { .add_inputs([utxo]) .add_output(&destination, 50000) .set_change_address(change) - .build_unsigned() - .map(|(tx, _)| tx); + .build_unsigned_reserved() + .map(|(tx, _, _)| tx); assert!(tx.is_ok()); let transaction = tx.unwrap(); @@ -720,7 +772,7 @@ mod tests { .set_current_height(200) .add_inputs([utxo]) .add_output(&destination, 50000) - .build_unsigned(); + .build_unsigned_reserved(); // Insufficient funds now surface via the coin selector wrapper too. assert!(matches!( @@ -740,7 +792,7 @@ mod tests { .set_current_height(200) .add_inputs([utxo]) // no set_change_address .add_output(&destination, 100_000) - .build_unsigned(); + .build_unsigned_reserved(); assert!( matches!(result, Err(BuilderError::NoChangeAddress)), @@ -844,7 +896,7 @@ mod tests { .set_change_address(change_address.clone()) .add_inputs(utxos) .add_output(&recipient_address, 500000) - .build_unsigned() + .build_unsigned_reserved() .unwrap() .0; @@ -873,7 +925,7 @@ mod tests { .set_change_address(change_address.clone()) .add_inputs(utxos) .add_output(&recipient_address, 150000) - .build_unsigned() + .build_unsigned_reserved() .unwrap() .0; @@ -893,14 +945,14 @@ mod tests { let recipient_address = Address::dummy(Network::Testnet, 0); let change_address = Address::dummy(Network::Testnet, 0); - let (tx, fee) = TransactionBuilder::new() + let (tx, fee, _) = TransactionBuilder::new() .set_current_height(200) .set_selection_strategy(SelectionStrategy::SmallestFirst) .set_fee_rate(FeeRate::normal()) .set_change_address(change_address) .add_inputs(utxos) .add_output(&recipient_address, 150000) - .build_unsigned() + .build_unsigned_reserved() .unwrap(); assert_eq!(tx.output.len(), 1, "dust change must be dropped"); @@ -926,13 +978,13 @@ mod tests { }], }; - let (tx, fee) = TransactionBuilder::new() + let (tx, fee, _) = TransactionBuilder::new() .set_current_height(200) .set_fee_rate(FeeRate::normal()) .set_change_address(change_address) .set_special_payload(TransactionPayload::AssetLockPayloadType(asset_lock_payload)) .add_inputs(utxos) - .build_unsigned() + .build_unsigned_reserved() .unwrap(); assert_eq!(tx.output.len(), 2, "OP_RETURN burn output + change"); @@ -1035,7 +1087,7 @@ mod tests { .add_output(&address1, 300000) // Higher amount .add_output(&address2, 100000) // Lower amount .add_output(&address1, 200000) // Middle amount - .build_unsigned() + .build_unsigned_reserved() .unwrap() .0; @@ -1107,7 +1159,7 @@ mod tests { .add_inputs([utxo2.clone()]) .add_inputs([utxo3.clone()]) .add_output(&destination, 500000) - .build_unsigned() + .build_unsigned_reserved() .unwrap() .0; @@ -1173,7 +1225,7 @@ mod tests { .set_special_payload(TransactionPayload::AssetLockPayloadType(asset_lock_payload)) .add_output(&recipient_address, 50000) .add_inputs(utxos) - .build_unsigned() + .build_unsigned_reserved() .unwrap() .0; @@ -1245,13 +1297,13 @@ mod tests { funds.utxos.insert(utxo.outpoint, utxo.clone()); let destination = Address::dummy(Network::Testnet, 0); - let (tx, _) = TransactionBuilder::new() + let (tx, _, _) = TransactionBuilder::new() .set_current_height(200) .set_fee_rate(FeeRate::normal()) .set_funding(&mut funds, &account) .set_change_address(Address::dummy(Network::Testnet, 1)) .add_output(&destination, 500_000) - .build_unsigned() + .build_unsigned_reserved() .expect("build unsigned"); // Every input the build selected is reserved, so a later build observes diff --git a/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs b/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs index 029ab4782..425d36ac7 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_building.rs @@ -158,7 +158,7 @@ mod tests { .set_change_address(change_address.clone()) .add_output(&recipient_address, 150000) .add_inputs(utxos) - .build_unsigned() + .build_unsigned_reserved() .unwrap() .0; @@ -190,13 +190,13 @@ mod tests { .require_network(Network::Testnet) .unwrap(); - let (tx, _fee) = TransactionBuilder::new() + let (tx, _fee, _) = TransactionBuilder::new() .set_fee_rate(FeeRate::normal()) .set_current_height(200) .set_change_address(change_address) .add_output(&recipient_address, 150000) .add_inputs(utxos) - .build_unsigned() + .build_unsigned_reserved() .expect("ordinary spend of an unconfirmed UTXO must succeed"); assert!(!tx.input.is_empty()); } @@ -217,13 +217,13 @@ mod tests { let total = 600_000u64; let fee = FeeRate::normal().calculate_fee(8 + 1 + 1 + 34 + 3 * 148); let deliverable = total - fee; - let (tx, _fee) = TransactionBuilder::new() + let (tx, _fee, _) = TransactionBuilder::new() .set_fee_rate(FeeRate::normal()) .set_current_height(200) .set_selection_strategy(SelectionStrategy::All) .add_inputs(utxos) .add_output(&dest, deliverable) - .build_unsigned() + .build_unsigned_reserved() .unwrap(); assert_eq!(tx.input.len(), 3, "sweep spends every input"); @@ -312,7 +312,7 @@ mod tests { .add_output(&recipient_address, 150000) .add_inputs(utxos); - let tx = builder.build_unsigned().unwrap().0; + let tx = builder.build_unsigned_reserved().unwrap().0; let serialized = dashcore::consensus::encode::serialize(&tx); // Size should be close to our estimation @@ -345,7 +345,7 @@ mod tests { .add_output(&recipient_address, 500000) .add_inputs(utxos); - let tx = builder.build_unsigned().unwrap().0; + let tx = builder.build_unsigned_reserved().unwrap().0; // Total input: 1000000 // Output to recipient: 500000 @@ -376,7 +376,7 @@ mod tests { .set_change_address(change_address.clone()) .add_output(&recipient_address, 1000000) // More than available .add_inputs(utxos) - .build_unsigned(); + .build_unsigned_reserved(); assert!(result.is_err()); } @@ -402,7 +402,7 @@ mod tests { .add_output(&recipient_address, 150000) .add_inputs(utxos); - let tx = builder.build_unsigned().unwrap().0; + let tx = builder.build_unsigned_reserved().unwrap().0; // Should only have 1 output (no change) assert_eq!(tx.output.len(), 1);