From e99959ced0062159d629930f488374e29f63c42b Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 23 Jul 2026 11:28:25 -0400 Subject: [PATCH 1/7] feat(key-wallet): owner-tagged reservations + release_if_owner to close the broadcast-release TOCTOU (platform#4185) ReservationSet previously mapped OutPoint -> reserved-at-height with no per-reservation owner, and released unconditionally by outpoint. The platform broadcast path (dashpay/platform#4185) reserves an asset-lock/deferred send's inputs, awaits the broadcast, and on a Rejected result releases them by outpoint. If key-wallet's TTL sweep reclaims that reservation mid-await and a different concurrent build re-reserves the same outpoint (same wallet generation, so any Arc::ptr_eq generation guard still passes), the rejected build's cleanup frees the OTHER build's reservation -> a double-spend window. The platform layer cannot detect this because the sweep is invisible inside key-wallet; the "release only if still mine" check must be atomic under the ReservationSet mutex. Stamp every reservation with a ReservationToken (a Copy newtype over a monotonic per-set counter, unique even across same-height reserves). reserve() returns the token it stamped; release_if_owner(outpoints, token) removes only outpoints still owned by that token, atomically under the mutex, and is a no-op for any swept-and-re-reserved outpoint. The unconditional release() is kept for the processed-spend path, where the coin is genuinely spent and must leave the set regardless of owner. Wiring: - assemble_unsigned returns the stamped token; build_signed's sign-failure path now releases owner-guarded (it too awaits, so it shares the hazard). - Additive token-returning builds: build_unsigned_reserved / build_signed_reserved. build_unsigned / build_signed keep their signatures. - Public seam for platform: ManagedCoreFundsAccount::release_reservation_if_owner and AssetLockResult::reservation_token; ReservationToken re-exported. Co-Authored-By: Claude Fable 5 --- key-wallet/src/lib.rs | 1 + .../managed_core_funds_account.rs | 33 ++- key-wallet/src/managed_account/mod.rs | 1 + key-wallet/src/managed_account/reservation.rs | 234 ++++++++++++++++-- .../managed_wallet_info/asset_lock_builder.rs | 24 +- .../transaction_builder.rs | 83 ++++++- 6 files changed, 342 insertions(+), 34 deletions(-) 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 da4c2c13a..533ec1556 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,37 @@ 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, which reserves an + /// asset-lock/deferred send's inputs, awaits the broadcast, and on a + /// `Rejected` result releases them. During that await key-wallet's TTL sweep + /// can invisibly reclaim the reservation and a different concurrent build can + /// re-reserve the same outpoint (under a new token, same wallet generation). + /// The unconditional [`Self::release_reservation`] would then free the other + /// build's inputs, letting coin selection hand them to a second transaction — + /// a double-spend window. Passing the original token makes the release a + /// no-op for any input that has since changed owners, closing that window. + /// The check is atomic under the reservation set's mutex, so no sweep or + /// re-reserve can interleave between "is it still mine?" and the removal. + /// + /// The platform layer cannot make this safe on its own because the sweep + /// happens inside key-wallet where it has no visibility; the owner check must + /// live here. See `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..386427b10 100644 --- a/key-wallet/src/managed_account/reservation.rs +++ b/key-wallet/src/managed_account/reservation.rs @@ -12,6 +12,26 @@ //! 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 release path +//! that a rejected/abandoned broadcast uses ([`ReservationSet::release_if_owner`]) +//! removes an outpoint only if it is *still owned by the releasing build*. +//! +//! The concrete hazard (see `dashpay/platform#4185`): the platform broadcast +//! path reserves an asset-lock/deferred send's inputs, `.await`s the broadcast, +//! and on a `Rejected` result releases those inputs so they become spendable +//! again. During that await the TTL sweep below can reclaim the reservation, and +//! a *different* concurrent build can re-reserve the very same outpoint (same +//! wallet generation, so any `Arc::ptr_eq` generation guard still matches). An +//! unconditional release-by-outpoint would then free the *other* build's inputs, +//! letting coin selection hand them to a second transaction — a double-spend +//! window. The platform layer cannot detect this because the sweep happens +//! inside key-wallet invisibly; the "release only if still mine" check must be +//! atomic under this set's own mutex, which is exactly what an owner token buys. use std::collections::{HashMap, HashSet}; use std::sync::{Arc, Mutex, MutexGuard}; @@ -31,24 +51,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 +120,92 @@ 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 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 + /// its reservation may have been swept and re-taken by a concurrent build, + /// and an unconditional release-by-outpoint would free that other build's + /// inputs, opening the double-spend window described in the module docs + /// (`dashpay/platform#4185`). Because the 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) { + let mut reserved = self.lock(); + for outpoint in outpoints { + let owned_by_token = + reserved.entries.get(outpoint).is_some_and(|entry| entry.owner == token); + if owned_by_token { + reserved.entries.remove(outpoint); + } } } } @@ -175,4 +297,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 4542ace06..95567ec50 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 @@ -10,7 +10,7 @@ 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::fee::FeeRate; use crate::wallet::managed_wallet_info::transaction_builder::{BuilderError, TransactionBuilder}; @@ -72,6 +72,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` — so a reservation + /// the TTL sweep reclaimed and another build re-took mid-broadcast is not + /// freed out from under that other build. See `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. @@ -206,7 +218,7 @@ impl ManagedWalletInfo { // Build first, derive credit keys after — a build failure must not // consume any funding-key indices. - let (transaction, fee) = TransactionBuilder::new() + let (transaction, fee, reservation_token) = TransactionBuilder::new() .set_fee_rate(FeeRate::new(fee_per_kb)) .set_current_height(height) .set_special_payload(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new( @@ -214,7 +226,7 @@ impl ManagedWalletInfo { ))) .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. @@ -235,6 +247,7 @@ impl ManagedWalletInfo { transaction, fee, keys: AssetLockCreditKeys::Private(keys), + reservation_token, }) } @@ -277,7 +290,7 @@ impl ManagedWalletInfo { let credit_outputs: Vec = credit_output_fundings.iter().map(|f| f.output.clone()).collect(); - let (transaction, fee) = TransactionBuilder::new() + let (transaction, fee, reservation_token) = TransactionBuilder::new() .set_fee_rate(FeeRate::new(fee_per_kb)) .set_current_height(height) .set_special_payload(TransactionPayload::AssetLockPayloadType(AssetLockPayload::new( @@ -285,7 +298,7 @@ impl ManagedWalletInfo { ))) .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?; // Credit-output bookkeeping: for each funding, peek the next unused @@ -339,6 +352,7 @@ impl ManagedWalletInfo { 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 b2afbefe7..af04068b4 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}; use crate::wallet::managed_wallet_info::fee::FeeRate; @@ -279,7 +279,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); @@ -392,14 +403,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. @@ -427,12 +440,31 @@ impl TransactionBuilder { /// 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()?; + let (tx, fee, _reservation) = self.build_unsigned_reserved()?; + Ok((tx, fee)) + } + + /// Like [`Self::build_unsigned`], but also returns the [`ReservationToken`] + /// stamped onto the inputs this build reserved (`None` when no reservation + /// set is attached). + /// + /// Use this instead of [`Self::build_unsigned`] when the built 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. Hold the returned + /// token and release with + /// [`ManagedCoreFundsAccount::release_reservation_if_owner`] so a + /// reservation the TTL sweep reclaimed and another build re-took is not + /// freed out from under that other build (see `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 @@ -446,24 +478,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); } @@ -471,7 +528,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)) } } From 9d9af46520d112398b020e8cde27d320bc0e4a2a Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Thu, 23 Jul 2026 11:39:01 -0400 Subject: [PATCH 2/7] style: cargo fmt Co-Authored-By: Claude Fable 5 --- key-wallet/src/managed_account/managed_core_funds_account.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 533ec1556..dd00ba75a 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -156,8 +156,7 @@ impl ManagedCoreFundsAccount { /// [`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(); + let outpoints: Vec = tx.input.iter().map(|input| input.previous_output).collect(); self.reservations.release_if_owner(&outpoints, token); } From b3c216ac9bce8400f99a49d7ca568f042cf8ef0d Mon Sep 17 00:00:00 2001 From: bfoss765 Date: Fri, 24 Jul 2026 08:54:03 -0400 Subject: [PATCH 3/7] fix(key-wallet): release owner-tagged reservation on asset-lock downstream failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit Major (PR #916, asset_lock_builder.rs :219-252 / :293-357). Both asset-lock builders took a reservation via build_signed_reserved and captured its reservation_token, but a failure in the code that runs AFTER the successful signed build — credit-key derivation in build_asset_lock, and the Phase 1-3 signer/bookkeeping loop in build_asset_lock_with_signer — returned Err without releasing the reservation. The already-signed transaction's inputs then stranded until the 24-block TTL sweep, since the caller never received the token to release them itself. Clone the funding account's ReservationSet (a shared Arc view) before each loop re-borrows self.accounts, and on any failure release only THIS build's reservation with the owner-guarded release_if_owner(&reserved, token) — never the unconditional release, which could free a concurrent build's inputs that the TTL sweep + re-reserve handed over during this build (the very TOCTOU of dashpay/platform#4185 this PR closes). The success path is unchanged: a successful build consumes the reservation as designed, and the signer-failure release inside build_signed_reserved returns via `?` before this new code runs, so there is no double-release. Co-Authored-By: Claude Fable 5 --- .../managed_wallet_info/asset_lock_builder.rs | 154 ++++++++++++------ 1 file changed, 103 insertions(+), 51 deletions(-) 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 95567ec50..a7887897a 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,7 +5,7 @@ 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; @@ -229,19 +229,44 @@ impl ManagedWalletInfo { .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: an unconditional release could free a concurrent + // build's inputs that the TTL sweep + re-reserve handed over during + // this build (the TOCTOU of `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, @@ -301,6 +326,19 @@ impl ManagedWalletInfo { .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: an + // unconditional release could free a concurrent build's inputs that the + // TTL sweep + re-reserve handed over during this build (the TOCTOU of + // `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. @@ -308,45 +346,59 @@ 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, From 4d927c155b6740ba7e1f271144217f7e82b69990 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Fri, 31 Jul 2026 01:00:53 -0400 Subject: [PATCH 4/7] feat(sml): masternodes_by_voting_key lookup on MasternodeList MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a by-voting-key lookup (returns matching pro_reg_tx_hashes) so SDK consumers can resolve which masternodes an imported voting key controls without the legacy dashj masternode list — needed for contested-username voting after the dashj engine is held post-cutover. Data was already present on every entry (key_id_voting, pro_reg_tx_hash); this is the missing index/helper plus a unit test covering multi-match, single-match and no-match. Co-Authored-By: Claude Opus 4.8 --- .../sml/masternode_list/masternode_helpers.rs | 85 ++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/dash/src/sml/masternode_list/masternode_helpers.rs b/dash/src/sml/masternode_list/masternode_helpers.rs index 4b55a3291..161d564d5 100644 --- a/dash/src/sml/masternode_list/masternode_helpers.rs +++ b/dash/src/sml/masternode_list/masternode_helpers.rs @@ -1,9 +1,26 @@ use std::net::IpAddr; -use crate::ProTxHash; use crate::sml::masternode_list::MasternodeList; +use crate::{ProTxHash, PubkeyHash}; impl MasternodeList { + /// Every masternode in the list whose voting key hash matches + /// `voting_key_id`, returned as their registration proTxHashes. + /// + /// Mirrors dashj's `MasternodeList.getMasternodesByVotingKey(votingKeyId)` + /// — the lookup contested-username voting uses to resolve which + /// masternode(s) a given voting key is entitled to cast a vote for. + /// `key_id_voting` is the 20-byte hash160 of the voting public key; a + /// single voting key can back more than one masternode, so the result is + /// a `Vec` (empty when no entry matches). + pub fn masternodes_by_voting_key(&self, voting_key_id: &PubkeyHash) -> Vec { + self.masternodes + .values() + .filter(|node| node.masternode_list_entry.key_id_voting == *voting_key_id) + .map(|node| node.masternode_list_entry.pro_reg_tx_hash) + .collect() + } + pub fn has_valid_masternode(&self, pro_reg_tx_hash: &ProTxHash) -> bool { self.masternodes .get(pro_reg_tx_hash) @@ -46,3 +63,69 @@ pub fn reverse_cmp_sup(lhs: [u8; 32], rhs: [u8; 32]) -> bool { // equal false } + +#[cfg(test)] +mod tests { + use std::net::{Ipv4Addr, SocketAddr, SocketAddrV4}; + + use hashes::Hash; + + use crate::bls_sig_utils::BLSPublicKey; + use crate::sml::masternode_list::MasternodeList; + use crate::sml::masternode_list_entry::{ + EntryMasternodeType, MasternodeListEntry, MasternodeNetInfo, + }; + use crate::{BlockHash, ProTxHash, PubkeyHash}; + + /// Build a `MasternodeList` from `(proTxHash-seed, voting-key-id)` pairs so + /// each entry gets a distinct proTxHash and a caller-chosen voting key. + fn list_from(entries: Vec<(u8, [u8; 20])>) -> MasternodeList { + let masternodes = entries + .into_iter() + .map(|(seed, voting_key_id)| { + let mut hash_bytes = [0u8; 32]; + hash_bytes[0] = seed; + let pro_tx_hash = ProTxHash::from_byte_array(hash_bytes); + let entry = MasternodeListEntry { + version: 1, + pro_reg_tx_hash: pro_tx_hash, + confirmed_hash: None, + service_address: MasternodeNetInfo::Legacy(SocketAddr::V4(SocketAddrV4::new( + Ipv4Addr::new(10, 0, 0, seed), + 9999, + ))), + operator_public_key: BLSPublicKey::from([0u8; 48]), + key_id_voting: PubkeyHash::from_byte_array(voting_key_id), + is_valid: true, + mn_type: EntryMasternodeType::Regular, + }; + (pro_tx_hash, entry.into()) + }) + .collect(); + MasternodeList::build(masternodes, Default::default(), BlockHash::from_byte_array([0u8; 32]), 0) + .build() + } + + #[test] + fn masternodes_by_voting_key_filters_and_collects() { + let key_a = [0xAAu8; 20]; + let key_b = [0xBBu8; 20]; + // Two masternodes share voting key A, one uses key B. + let list = list_from(vec![(1, key_a), (2, key_b), (3, key_a)]); + + let mut matched = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array(key_a)); + // Order is BTreeMap (proTxHash) order; sort the seed byte for a stable assert. + matched.sort_by_key(|h| h.to_byte_array()[0]); + assert_eq!(matched.len(), 2); + assert_eq!(matched[0].to_byte_array()[0], 1); + assert_eq!(matched[1].to_byte_array()[0], 3); + + let single = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array(key_b)); + assert_eq!(single.len(), 1); + assert_eq!(single[0].to_byte_array()[0], 2); + + // A voting key no masternode uses yields an empty vec. + let none = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array([0xCCu8; 20])); + assert!(none.is_empty()); + } +} From 2a56bb087714fa377bcf42fe5830d9b27971c095 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Sat, 1 Aug 2026 15:19:59 -0400 Subject: [PATCH 5/7] docs(sml): guarantee ascending ProTxHash order for masternodes_by_voting_key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the one unresolved CodeRabbit review thread on PR #916 (masternode_helpers.rs:116-121): the function returned matches in `BTreeMap` order but the public docs never promised an order, and the test hid that by sorting the result before asserting on it. Document the ordering as a guaranteed part of the API rather than an implementation accident, and say why it holds: `masternodes` is a `BTreeMap` (std guarantees ascending-key iteration) and `ProTxHash` derives `Ord` over its 32 internal bytes. Also note that because `ProTxHash` is `#[hash_newtype(forward)]`, the internal byte order is the same order its hex `Display` reads in — so the sequence is sorted the way it prints, which is the non-obvious part for a codebase that reverses byte order on several other hash types. Test changes: - `masternodes_by_voting_key_filters_and_collects` no longer sorts the result. It asserts the whole `Vec` at once, so it now pins ordering as well as contents. - New `masternodes_by_voting_key_returns_ascending_pro_tx_hash_order` seeds the list in DESCENDING proTxHash order, so a result that merely echoed insertion order would come back reversed and fail. It also asserts strict ascent through the public `Ord` impl, which is the comparison callers would actually use. The ordering claim is verified by the new test, not assumed. Also fixes a `cargo fmt` violation in this file's `list_from` test helper that was already present on the branch (`cargo fmt --all -- --check` was failing before this commit and passes after). cargo fmt --all -- --check -> clean cargo test -p dashcore --lib -> 577 passed, 0 failed Co-Authored-By: Claude Opus 4.8 --- .../sml/masternode_list/masternode_helpers.rs | 67 ++++++++++++++++--- 1 file changed, 57 insertions(+), 10 deletions(-) diff --git a/dash/src/sml/masternode_list/masternode_helpers.rs b/dash/src/sml/masternode_list/masternode_helpers.rs index 161d564d5..cc4d5a89a 100644 --- a/dash/src/sml/masternode_list/masternode_helpers.rs +++ b/dash/src/sml/masternode_list/masternode_helpers.rs @@ -13,6 +13,20 @@ impl MasternodeList { /// `key_id_voting` is the 20-byte hash160 of the voting public key; a /// single voting key can back more than one masternode, so the result is /// a `Vec` (empty when no entry matches). + /// + /// # Ordering + /// + /// Results are returned in **ascending `ProTxHash` order**. This is a + /// guaranteed part of the API, not an accident of the current + /// implementation: the backing `masternodes` collection is a + /// `BTreeMap`, whose iteration order is defined by the + /// standard library to be ascending key order, and `ProTxHash` derives + /// `Ord` over its 32 internal bytes. Because `ProTxHash` is a + /// `#[hash_newtype(forward)]` hash, that internal byte order is also the + /// order its hex `Display` reads in, so the returned sequence is sorted + /// the same way it prints. Callers that need a deterministic vote order + /// (contested-username voting does) may rely on this directly without + /// re-sorting. pub fn masternodes_by_voting_key(&self, voting_key_id: &PubkeyHash) -> Vec { self.masternodes .values() @@ -102,8 +116,20 @@ mod tests { (pro_tx_hash, entry.into()) }) .collect(); - MasternodeList::build(masternodes, Default::default(), BlockHash::from_byte_array([0u8; 32]), 0) - .build() + MasternodeList::build( + masternodes, + Default::default(), + BlockHash::from_byte_array([0u8; 32]), + 0, + ) + .build() + } + + /// The `ProTxHash` that `list_from` derives for a given seed byte. + fn hash_for_seed(seed: u8) -> ProTxHash { + let mut hash_bytes = [0u8; 32]; + hash_bytes[0] = seed; + ProTxHash::from_byte_array(hash_bytes) } #[test] @@ -113,19 +139,40 @@ mod tests { // Two masternodes share voting key A, one uses key B. let list = list_from(vec![(1, key_a), (2, key_b), (3, key_a)]); - let mut matched = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array(key_a)); - // Order is BTreeMap (proTxHash) order; sort the seed byte for a stable assert. - matched.sort_by_key(|h| h.to_byte_array()[0]); - assert_eq!(matched.len(), 2); - assert_eq!(matched[0].to_byte_array()[0], 1); - assert_eq!(matched[1].to_byte_array()[0], 3); + // Asserted as a whole `Vec`, so this pins the documented ascending + // `ProTxHash` ordering as well as the contents. + let matched = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array(key_a)); + assert_eq!(matched, vec![hash_for_seed(1), hash_for_seed(3)]); let single = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array(key_b)); - assert_eq!(single.len(), 1); - assert_eq!(single[0].to_byte_array()[0], 2); + assert_eq!(single, vec![hash_for_seed(2)]); // A voting key no masternode uses yields an empty vec. let none = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array([0xCCu8; 20])); assert!(none.is_empty()); } + + #[test] + fn masternodes_by_voting_key_returns_ascending_pro_tx_hash_order() { + // Seed the list in DESCENDING proTxHash order so a result that merely + // echoed insertion order would come back reversed. The documented + // guarantee is ascending order regardless of insertion order, which + // only holds because `masternodes` is a `BTreeMap`. + let key = [0xAAu8; 20]; + let list = list_from(vec![(9, key), (5, key), (7, key), (1, key)]); + + let matched = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array(key)); + assert_eq!( + matched, + vec![hash_for_seed(1), hash_for_seed(5), hash_for_seed(7), hash_for_seed(9)], + "results must be in ascending ProTxHash order, not insertion order" + ); + + // Belt and braces: the sequence is sorted by the same comparison the + // public `Ord` impl exposes to callers. + assert!( + matched.windows(2).all(|w| w[0] < w[1]), + "returned hashes must be strictly ascending under ProTxHash: Ord" + ); + } } From c59911b812d5f0660809d4d03e1f15a5d2e5ad02 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:12:22 -0400 Subject: [PATCH 6/7] chore(sml): drop unrelated masternode_helpers changes from this PR The `masternodes_by_voting_key` lookup and its tests are independent of the owner-tagged reservation work and only made this PR harder to review. Revert the file to its `dev` state; the lookup can land in its own PR. Addresses review comment on masternode_helpers.rs (scope). Co-Authored-By: Claude Opus 4.8 --- .../sml/masternode_list/masternode_helpers.rs | 132 +----------------- 1 file changed, 1 insertion(+), 131 deletions(-) diff --git a/dash/src/sml/masternode_list/masternode_helpers.rs b/dash/src/sml/masternode_list/masternode_helpers.rs index cc4d5a89a..4b55a3291 100644 --- a/dash/src/sml/masternode_list/masternode_helpers.rs +++ b/dash/src/sml/masternode_list/masternode_helpers.rs @@ -1,40 +1,9 @@ use std::net::IpAddr; +use crate::ProTxHash; use crate::sml::masternode_list::MasternodeList; -use crate::{ProTxHash, PubkeyHash}; impl MasternodeList { - /// Every masternode in the list whose voting key hash matches - /// `voting_key_id`, returned as their registration proTxHashes. - /// - /// Mirrors dashj's `MasternodeList.getMasternodesByVotingKey(votingKeyId)` - /// — the lookup contested-username voting uses to resolve which - /// masternode(s) a given voting key is entitled to cast a vote for. - /// `key_id_voting` is the 20-byte hash160 of the voting public key; a - /// single voting key can back more than one masternode, so the result is - /// a `Vec` (empty when no entry matches). - /// - /// # Ordering - /// - /// Results are returned in **ascending `ProTxHash` order**. This is a - /// guaranteed part of the API, not an accident of the current - /// implementation: the backing `masternodes` collection is a - /// `BTreeMap`, whose iteration order is defined by the - /// standard library to be ascending key order, and `ProTxHash` derives - /// `Ord` over its 32 internal bytes. Because `ProTxHash` is a - /// `#[hash_newtype(forward)]` hash, that internal byte order is also the - /// order its hex `Display` reads in, so the returned sequence is sorted - /// the same way it prints. Callers that need a deterministic vote order - /// (contested-username voting does) may rely on this directly without - /// re-sorting. - pub fn masternodes_by_voting_key(&self, voting_key_id: &PubkeyHash) -> Vec { - self.masternodes - .values() - .filter(|node| node.masternode_list_entry.key_id_voting == *voting_key_id) - .map(|node| node.masternode_list_entry.pro_reg_tx_hash) - .collect() - } - pub fn has_valid_masternode(&self, pro_reg_tx_hash: &ProTxHash) -> bool { self.masternodes .get(pro_reg_tx_hash) @@ -77,102 +46,3 @@ pub fn reverse_cmp_sup(lhs: [u8; 32], rhs: [u8; 32]) -> bool { // equal false } - -#[cfg(test)] -mod tests { - use std::net::{Ipv4Addr, SocketAddr, SocketAddrV4}; - - use hashes::Hash; - - use crate::bls_sig_utils::BLSPublicKey; - use crate::sml::masternode_list::MasternodeList; - use crate::sml::masternode_list_entry::{ - EntryMasternodeType, MasternodeListEntry, MasternodeNetInfo, - }; - use crate::{BlockHash, ProTxHash, PubkeyHash}; - - /// Build a `MasternodeList` from `(proTxHash-seed, voting-key-id)` pairs so - /// each entry gets a distinct proTxHash and a caller-chosen voting key. - fn list_from(entries: Vec<(u8, [u8; 20])>) -> MasternodeList { - let masternodes = entries - .into_iter() - .map(|(seed, voting_key_id)| { - let mut hash_bytes = [0u8; 32]; - hash_bytes[0] = seed; - let pro_tx_hash = ProTxHash::from_byte_array(hash_bytes); - let entry = MasternodeListEntry { - version: 1, - pro_reg_tx_hash: pro_tx_hash, - confirmed_hash: None, - service_address: MasternodeNetInfo::Legacy(SocketAddr::V4(SocketAddrV4::new( - Ipv4Addr::new(10, 0, 0, seed), - 9999, - ))), - operator_public_key: BLSPublicKey::from([0u8; 48]), - key_id_voting: PubkeyHash::from_byte_array(voting_key_id), - is_valid: true, - mn_type: EntryMasternodeType::Regular, - }; - (pro_tx_hash, entry.into()) - }) - .collect(); - MasternodeList::build( - masternodes, - Default::default(), - BlockHash::from_byte_array([0u8; 32]), - 0, - ) - .build() - } - - /// The `ProTxHash` that `list_from` derives for a given seed byte. - fn hash_for_seed(seed: u8) -> ProTxHash { - let mut hash_bytes = [0u8; 32]; - hash_bytes[0] = seed; - ProTxHash::from_byte_array(hash_bytes) - } - - #[test] - fn masternodes_by_voting_key_filters_and_collects() { - let key_a = [0xAAu8; 20]; - let key_b = [0xBBu8; 20]; - // Two masternodes share voting key A, one uses key B. - let list = list_from(vec![(1, key_a), (2, key_b), (3, key_a)]); - - // Asserted as a whole `Vec`, so this pins the documented ascending - // `ProTxHash` ordering as well as the contents. - let matched = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array(key_a)); - assert_eq!(matched, vec![hash_for_seed(1), hash_for_seed(3)]); - - let single = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array(key_b)); - assert_eq!(single, vec![hash_for_seed(2)]); - - // A voting key no masternode uses yields an empty vec. - let none = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array([0xCCu8; 20])); - assert!(none.is_empty()); - } - - #[test] - fn masternodes_by_voting_key_returns_ascending_pro_tx_hash_order() { - // Seed the list in DESCENDING proTxHash order so a result that merely - // echoed insertion order would come back reversed. The documented - // guarantee is ascending order regardless of insertion order, which - // only holds because `masternodes` is a `BTreeMap`. - let key = [0xAAu8; 20]; - let list = list_from(vec![(9, key), (5, key), (7, key), (1, key)]); - - let matched = list.masternodes_by_voting_key(&PubkeyHash::from_byte_array(key)); - assert_eq!( - matched, - vec![hash_for_seed(1), hash_for_seed(5), hash_for_seed(7), hash_for_seed(9)], - "results must be in ascending ProTxHash order, not insertion order" - ); - - // Belt and braces: the sequence is sorted by the same comparison the - // public `Ord` impl exposes to callers. - assert!( - matched.windows(2).all(|w| w[0] < w[1]), - "returned hashes must be strictly ascending under ProTxHash: Ord" - ); - } -} From 5ade591aeb06b0bee5912a83ce4baffab111097d Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:12:34 -0400 Subject: [PATCH 7/7] refactor(key-wallet): address reservation review feedback - Remove the `build_unsigned` convenience wrapper. It reserved the selected inputs but discarded the ReservationToken, leaving callers no way to do an owner-guarded release and inviting the exact misuse the token exists to prevent. It had no production callers; the only users were tests, now moved to `build_unsigned_reserved`. - `ReservationSet::release_if_owner`: replace the two-lookup (get-then-remove) per outpoint with a single Entry-API lookup ("remove only if still mine"), preserving exact semantics. - Consolidate the platform#4185 ownership rationale. It was restated in full in several places that would drift apart; keep the single canonical explanation on `ReservationSet::release_if_owner` and have the module doc, the asset-lock builder, the AssetLockResult token field, and `release_reservation_if_owner` point to it. Co-Authored-By: Claude Opus 4.8 --- .../managed_core_funds_account.rs | 19 ++---- key-wallet/src/managed_account/reservation.rs | 51 +++++++-------- .../managed_wallet_info/asset_lock_builder.rs | 17 ++--- .../transaction_builder.rs | 65 +++++++++---------- .../transaction_building.rs | 18 ++--- 5 files changed, 75 insertions(+), 95 deletions(-) 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 79748ceea..2418152dc 100644 --- a/key-wallet/src/managed_account/managed_core_funds_account.rs +++ b/key-wallet/src/managed_account/managed_core_funds_account.rs @@ -137,21 +137,10 @@ impl ManagedCoreFundsAccount { /// /// 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, which reserves an - /// asset-lock/deferred send's inputs, awaits the broadcast, and on a - /// `Rejected` result releases them. During that await key-wallet's TTL sweep - /// can invisibly reclaim the reservation and a different concurrent build can - /// re-reserve the same outpoint (under a new token, same wallet generation). - /// The unconditional [`Self::release_reservation`] would then free the other - /// build's inputs, letting coin selection hand them to a second transaction — - /// a double-spend window. Passing the original token makes the release a - /// no-op for any input that has since changed owners, closing that window. - /// The check is atomic under the reservation set's mutex, so no sweep or - /// re-reserve can interleave between "is it still mine?" and the removal. - /// - /// The platform layer cannot make this safe on its own because the sweep - /// happens inside key-wallet where it has no visibility; the owner check must - /// live here. See `dashpay/platform#4185`. + /// 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 diff --git a/key-wallet/src/managed_account/reservation.rs b/key-wallet/src/managed_account/reservation.rs index 386427b10..1715c5fd4 100644 --- a/key-wallet/src/managed_account/reservation.rs +++ b/key-wallet/src/managed_account/reservation.rs @@ -17,21 +17,11 @@ //! //! 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 release path -//! that a rejected/abandoned broadcast uses ([`ReservationSet::release_if_owner`]) -//! removes an outpoint only if it is *still owned by the releasing build*. -//! -//! The concrete hazard (see `dashpay/platform#4185`): the platform broadcast -//! path reserves an asset-lock/deferred send's inputs, `.await`s the broadcast, -//! and on a `Rejected` result releases those inputs so they become spendable -//! again. During that await the TTL sweep below can reclaim the reservation, and -//! a *different* concurrent build can re-reserve the very same outpoint (same -//! wallet generation, so any `Arc::ptr_eq` generation guard still matches). An -//! unconditional release-by-outpoint would then free the *other* build's inputs, -//! letting coin selection hand them to a second transaction — a double-spend -//! window. The platform layer cannot detect this because the sweep happens -//! inside key-wallet invisibly; the "release only if still mine" check must be -//! atomic under this set's own mutex, which is exactly what an owner token buys. +//! [`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}; @@ -185,13 +175,18 @@ impl ReservationSet { /// meanwhile — is left untouched. /// /// This is the owner-guarded counterpart to [`Self::release`] and the whole - /// reason reservations carry a [`ReservationToken`]. 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 - /// its reservation may have been swept and re-taken by a concurrent build, - /// and an unconditional release-by-outpoint would free that other build's - /// inputs, opening the double-spend window described in the module docs - /// (`dashpay/platform#4185`). Because the check and the removal happen + /// 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. /// @@ -199,12 +194,16 @@ impl ReservationSet { /// 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 { - let owned_by_token = - reserved.entries.get(outpoint).is_some_and(|entry| entry.owner == token); - if owned_by_token { - reserved.entries.remove(outpoint); + // 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(); + } } } } 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 c4ca43368..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 @@ -116,9 +116,9 @@ pub struct AssetLockResult { /// 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` — so a reservation - /// the TTL sweep reclaimed and another build re-took mid-broadcast is not - /// freed out from under that other build. See `dashpay/platform#4185`. + /// 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, @@ -330,9 +330,8 @@ impl ManagedWalletInfo { // 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: an unconditional release could free a concurrent - // build's inputs that the TTL sweep + re-reserve handed over during - // this build (the TOCTOU of `dashpay/platform#4185`). + // 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(); @@ -457,10 +456,8 @@ impl ManagedWalletInfo { // 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: an - // unconditional release could free a concurrent build's inputs that the - // TTL sweep + re-reserve handed over during this build (the TOCTOU of - // `dashpay/platform#4185`). + // 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(); 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 c29aa7b27..076581ed6 100644 --- a/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs +++ b/key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs @@ -467,27 +467,22 @@ 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, fee, _reservation) = self.build_unsigned_reserved()?; - Ok((tx, fee)) - } - - /// Like [`Self::build_unsigned`], but also returns the [`ReservationToken`] - /// stamped onto the inputs this build reserved (`None` when no reservation - /// set is attached). + /// 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. /// - /// Use this instead of [`Self::build_unsigned`] when the built 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. Hold the returned - /// token and release with - /// [`ManagedCoreFundsAccount::release_reservation_if_owner`] so a - /// reservation the TTL sweep reclaimed and another build re-took is not - /// freed out from under that other build (see `dashpay/platform#4185`). + /// 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> { @@ -759,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(); @@ -777,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!( @@ -797,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)), @@ -901,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; @@ -930,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; @@ -950,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"); @@ -983,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"); @@ -1092,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; @@ -1164,7 +1159,7 @@ mod tests { .add_inputs([utxo2.clone()]) .add_inputs([utxo3.clone()]) .add_output(&destination, 500000) - .build_unsigned() + .build_unsigned_reserved() .unwrap() .0; @@ -1230,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; @@ -1302,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);