diff --git a/CHANGELOG.md b/CHANGELOG.md index c79b04eb3..206b3f04e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - GUI: Support outbound connections to makers through libp2p circuit relays. +- ASB: Fix concurrent swaps selecting the same Monero output when building their lock transactions, + which turned the losing swap's lock transaction into a permanent double-spend + and left the taker's swap stuck until it refunded. + A concurrent swap the maker can no longer fund is now refunded early, + instead of building a doomed lock transaction that wedges the swap. ## [4.14.0] - 2026-08-22 diff --git a/swap/src/protocol/alice.rs b/swap/src/protocol/alice.rs index 1c18cdb0e..c86d34bdc 100644 --- a/swap/src/protocol/alice.rs +++ b/swap/src/protocol/alice.rs @@ -10,6 +10,7 @@ use swap_env::env::Config; pub use swap_machine::alice::*; use uuid::Uuid; +pub mod lock_phase; pub mod swap; pub struct Swap { diff --git a/swap/src/protocol/alice/lock_phase.rs b/swap/src/protocol/alice/lock_phase.rs new file mode 100644 index 000000000..d8daffed1 --- /dev/null +++ b/swap/src/protocol/alice/lock_phase.rs @@ -0,0 +1,202 @@ +//! Serialization of the Monero lock phase across concurrent swaps. +//! +//! wallet2 only marks an output spent once its lock transaction is relayed and +//! monero-sys has no reserve API, so two overlapping swaps can pick the same +//! output and the loser's lock transaction becomes a permanent double-spend +//! that monerod rejects forever. [`MoneroLockPhase`] hands out one process-wide +//! permit for the phase; each swap tracks its participation through a +//! [`LockPhaseSession`] so the state machine only has to say whether its +//! current state is inside the phase. + +use std::time::{Duration, Instant}; + +use tokio::sync::{Mutex, MutexGuard}; + +/// Process-wide serialization of the Monero lock phase. +/// +/// The phase spans output selection (`BtcLocked`) through the first relay of +/// the lock transaction (`XmrLockTransactionConstructed`). +pub struct MoneroLockPhase { + mutex: Mutex<()>, + max_hold: Duration, +} + +impl MoneroLockPhase { + /// A lock phase whose sessions may hold the permit for at most `max_hold`. + pub fn new(max_hold: Duration) -> Self { + Self { + mutex: Mutex::new(()), + max_hold, + } + } + + /// Start tracking one swap's participation in the phase. + pub fn session(&self) -> LockPhaseSession<'_> { + LockPhaseSession { + phase: self, + held: None, + abandoned: false, + } + } +} + +/// One swap's view of the serialized phase, held on `run_until`'s stack because +/// construct and publish are separate states with a persist in between. +pub struct LockPhaseSession<'a> { + phase: &'a MoneroLockPhase, + held: Option<(MutexGuard<'a, ()>, Instant)>, + /// Set once this swap exhausts `max_hold` and continues unserialized. + abandoned: bool, +} + +impl LockPhaseSession<'_> { + /// Bring the session in line with whether the swap's current state is + /// inside the phase: acquire the process-wide permit on entry (unless this + /// swap already overstayed and continues unserialized), release it on exit. + pub async fn sync_to(&mut self, in_phase: bool) { + if !in_phase { + self.release(); + } else if self.held.is_none() && !self.abandoned { + let guard = self.phase.mutex.lock().await; + tracing::debug!("Entered the serialized Monero lock phase"); + self.held = Some((guard, Instant::now())); + } + } + + /// Remaining time this swap may keep the permit, or `None` when it is not + /// holding it (never entered, already released, or abandoned). + pub fn deadline(&self) -> Option { + self.held + .as_ref() + .map(|(_, since)| self.phase.max_hold.saturating_sub(since.elapsed())) + } + + /// Whether this session currently holds the process-wide permit. + pub fn holds_permit(&self) -> bool { + self.held.is_some() + } + + /// Leave the phase normally: release the permit and re-arm the session for + /// a future phase. + pub fn release(&mut self) { + if self.held.take().is_some() { + tracing::debug!("Left the serialized Monero lock phase"); + } + self.abandoned = false; + } + + /// Give up the permit after overstaying the deadline. The swap continues + /// unserialized until it leaves the phase, which re-arms it via + /// [`LockPhaseSession::release`]. + pub fn abandon(&mut self) { + self.held = None; + self.abandoned = true; + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const WAIT: Duration = Duration::from_millis(50); + + #[tokio::test] + async fn phase_is_exclusive_until_released() { + let phase = MoneroLockPhase::new(Duration::from_secs(60)); + + let mut first = phase.session(); + first.sync_to(true).await; + assert!(first.holds_permit()); + + let mut second = phase.session(); + assert!( + tokio::time::timeout(WAIT, second.sync_to(true)) + .await + .is_err(), + "second session must wait while the first holds the permit" + ); + assert!(!second.holds_permit()); + + first.release(); + assert!(!first.holds_permit()); + + tokio::time::timeout(WAIT, second.sync_to(true)) + .await + .expect("second session acquires once the first released"); + assert!(second.holds_permit()); + } + + #[tokio::test] + async fn deadline_is_bounded_by_max_hold_and_none_when_not_held() { + let phase = MoneroLockPhase::new(Duration::from_secs(60)); + let mut session = phase.session(); + + assert_eq!(session.deadline(), None); + + session.sync_to(true).await; + let deadline = session.deadline().expect("held sessions have a deadline"); + assert!(deadline <= Duration::from_secs(60)); + + session.sync_to(false).await; + assert_eq!(session.deadline(), None); + } + + #[tokio::test] + async fn deadline_expires_to_zero() { + let phase = MoneroLockPhase::new(Duration::from_millis(5)); + let mut session = phase.session(); + + session.sync_to(true).await; + tokio::time::sleep(Duration::from_millis(10)).await; + assert_eq!(session.deadline(), Some(Duration::ZERO)); + } + + #[tokio::test] + async fn abandoned_session_continues_unserialized_until_it_leaves_the_phase() { + let phase = MoneroLockPhase::new(Duration::from_secs(60)); + + let mut wedged = phase.session(); + wedged.sync_to(true).await; + wedged.abandon(); + assert!(!wedged.holds_permit()); + assert_eq!(wedged.deadline(), None); + + // Still inside the phase: the session must not re-acquire... + tokio::time::timeout(WAIT, wedged.sync_to(true)) + .await + .expect("an abandoned session never blocks"); + assert!(!wedged.holds_permit()); + + // ...so another swap is free to take the permit meanwhile. + let mut other = phase.session(); + tokio::time::timeout(WAIT, other.sync_to(true)) + .await + .expect("the permit is free after an abandon"); + assert!(other.holds_permit()); + other.release(); + + // Leaving the phase re-arms the abandoned session for the next one. + wedged.sync_to(false).await; + tokio::time::timeout(WAIT, wedged.sync_to(true)) + .await + .expect("a re-armed session acquires again"); + assert!(wedged.holds_permit()); + } + + #[tokio::test] + async fn dropping_a_session_frees_the_permit() { + let phase = MoneroLockPhase::new(Duration::from_secs(60)); + + { + let mut held = phase.session(); + held.sync_to(true).await; + assert!(held.holds_permit()); + } + + let mut next = phase.session(); + tokio::time::timeout(WAIT, next.sync_to(true)) + .await + .expect("dropping a holding session frees the permit"); + assert!(next.holds_permit()); + } +} diff --git a/swap/src/protocol/alice/swap.rs b/swap/src/protocol/alice/swap.rs index d29e7b7d3..38051c890 100644 --- a/swap/src/protocol/alice/swap.rs +++ b/swap/src/protocol/alice/swap.rs @@ -1,12 +1,13 @@ //! Run an XMR/BTC swap in the role of Alice. //! Alice holds XMR and wishes receive BTC. -use std::sync::Arc; +use std::sync::{Arc, LazyLock}; use std::time::Duration; use crate::asb::{EventLoopHandle, LatestRate}; use crate::common::retry; use crate::monero; use crate::monero::TransferProof; +use crate::protocol::alice::lock_phase::MoneroLockPhase; use crate::protocol::alice::{AliceState, HermesFundingPolicy, Swap, TipConfig}; use ::bitcoin::consensus::encode::serialize_hex; use anyhow::{Context, Result, bail}; @@ -22,6 +23,23 @@ use tokio::select; use tokio::time::timeout; use uuid::Uuid; +/// Serializes the Monero lock phase (output selection in `BtcLocked` through the first +/// relay in `XmrLockTransactionConstructed`) across all swaps in the process. wallet2 only +/// marks an output spent once its lock transaction is relayed and monero-sys has no reserve +/// API, so without this two overlapping swaps pick the same output and the loser's lock +/// transaction is a permanent double-spend that monerod rejects forever. Each swap tracks +/// its participation through a session held on `run_until`'s stack because construct and +/// publish are separate states. +static MONERO_LOCK_PHASE: LazyLock = + LazyLock::new(|| MoneroLockPhase::new(MONERO_LOCK_PHASE_MAX_HOLD)); + +/// Maximum time a swap may hold [`MONERO_LOCK_PHASE`]. Past it a swap wedged on a rejected +/// publish releases the guard and continues unserialized instead of starving the others. +const MONERO_LOCK_PHASE_MAX_HOLD: Duration = Duration::from_secs(20 * 60); + +/// `.expect` message for the persist retry loop, which never stops retrying. +const PERSIST_EXPECT: &str = "we never stop retrying to persist the latest Alice state"; + pub async fn run(swap: Swap, rate_service: LR) -> Result where LR: LatestRate + Clone, @@ -40,10 +58,23 @@ where { let mut current_state = swap.state; + // Tracks this swap's participation in the serialized Monero lock phase, across the + // separate `next_state` calls and the persist between them. See [`MONERO_LOCK_PHASE`]. + let mut lock_phase = MONERO_LOCK_PHASE.session(); + while !swap_machine::alice::is_complete(¤t_state) && !exit_early(¤t_state) { - current_state = next_state( + lock_phase + .sync_to(in_monero_lock_phase(¤t_state)) + .await; + + // While holding the permit, bound each step: the publish arm retries without a limit, + // and a wedged swap must not keep others from locking Monero. Cancelling is safe: + // no state is persisted and both arms are re-entrant. + let step_deadline = lock_phase.deadline(); + + let step = next_state( swap.swap_id, - current_state, + current_state.clone(), &mut swap.event_loop_handle, swap.bitcoin_wallet.clone(), swap.monero_wallet.clone(), @@ -51,10 +82,27 @@ where swap.developer_tip.clone(), swap.hermes_funding_policy, rate_service.clone(), - ) - .await?; + ); + + current_state = match step_deadline { + Some(deadline) => match timeout(deadline, step).await { + Ok(next) => next?, + Err(_) => { + abandon_lock_phase(¤t_state, &swap.monero_wallet).await; + lock_phase.abandon(); + continue; + } + }, + None => step.await?, + }; - retry( + // Release before the persist: the persist retries without a limit and must not pin + // the process-wide permit. + if !in_monero_lock_phase(¤t_state) { + lock_phase.release(); + } + + let persist = retry( "Persisting latest Alice state", || { let db = swap.db.clone(); @@ -68,9 +116,24 @@ where }, None, None, - ) - .await - .expect("we never stop retrying to persist the latest Alice state"); + ); + + // A persist of an in-phase state counts against the same deadline; past it we release + // the guard and finish the persist unserialized (the persist itself is never dropped). + match lock_phase.deadline() { + Some(remaining) => { + tokio::pin!(persist); + match timeout(remaining, &mut persist).await { + Ok(persisted) => persisted.expect(PERSIST_EXPECT), + Err(_) => { + abandon_lock_phase(¤t_state, &swap.monero_wallet).await; + lock_phase.abandon(); + persist.await.expect(PERSIST_EXPECT); + } + } + } + None => persist.await.expect(PERSIST_EXPECT), + } } Ok(current_state) @@ -196,6 +259,35 @@ where developer_tip.clone(), )?; + // Under MONERO_LOCK_PHASE, refuse to construct a lock the wallet cannot + // fund. A concurrent swap that already locked its Monero has reduced the + // spendable balance; without this guard wallet2 can still reselect the + // sibling's freshly spent outputs, build a lock that double-spends, and then + // wedge on a permanently rejected publish. A permanent error here routes to an + // early Bitcoin refund (no Monero was locked, so it is safe) instead. This is a + // best-effort guard bounded by how promptly the wallet reflects the sibling + // spend; a fully reliable check would query the daemon for spent key images. + let needed_pico = destinations + .iter() + .map(|(_, amount)| amount.as_pico()) + .sum::() + .saturating_add(swap_core::monero::CONSERVATIVE_MONERO_FEE.as_pico()); + let unlocked_pico = monero_wallet + .main_wallet() + .await + .unlocked_balance() + .await + .context("Failed to read unlocked Monero balance before constructing the lock transaction") + .map_err(backoff::Error::transient)? + .as_pico(); + if unlocked_pico < needed_pico { + return Err(backoff::Error::permanent(anyhow::anyhow!( + "Insufficient unlocked Monero to fund the lock transaction \ + ({unlocked_pico} < {needed_pico} piconero); a concurrent swap consumed \ + the shared balance, refunding this swap early" + ))); + } + let constructed = monero_wallet .construct_multi_destination_tx(&destinations) .await @@ -1274,6 +1366,49 @@ async fn cancel_timelock_not_expired( )) } +/// Whether `state` is inside the serialized Monero lock phase (see [`MONERO_LOCK_PHASE`]). +fn in_monero_lock_phase(state: &AliceState) -> bool { + matches!( + state, + AliceState::BtcLocked { .. } | AliceState::XmrLockTransactionConstructed { .. } + ) +} + +/// Releases [`MONERO_LOCK_PHASE`] after a swap overstays its deadline while still holding a +/// constructed lock transaction. If that transaction already reached the chain, scan it so +/// wallet2 marks its outputs spent before the next swap constructs; otherwise the +/// unserialized race returns for this one wedged swap. Bounded so an unresponsive daemon +/// cannot extend the hold. +async fn abandon_lock_phase(state: &AliceState, monero_wallet: &monero::Wallets) { + let AliceState::XmrLockTransactionConstructed { xmr_lock_tx, .. } = state else { + tracing::warn!("Monero lock phase exceeded its deadline; releasing the lock"); + return; + }; + + let tx_hash = monero::TxHash::from_tx(xmr_lock_tx); + tracing::warn!(%tx_hash, "Monero lock phase exceeded its deadline; releasing the lock"); + + let scanned = timeout(Duration::from_secs(60), async { + if monero_wallet.is_transaction_present(&tx_hash).await? { + monero_wallet + .main_wallet() + .await + .scan_transaction(tx_hash.0.clone()) + .await?; + } + anyhow::Ok(()) + }) + .await; + + match scanned { + Ok(Ok(())) => {} + Ok(Err(error)) => { + tracing::warn!(%tx_hash, %error, "Failed to scan the lock transaction on release") + } + Err(_) => tracing::warn!(%tx_hash, "Timed out scanning the lock transaction on release"), + } +} + #[cfg(test)] mod tests { use super::build_transfer_destinations; diff --git a/swap/tests/concurrent_bobs_insufficient_xmr.rs b/swap/tests/concurrent_bobs_insufficient_xmr.rs new file mode 100644 index 000000000..fdb8e54cb --- /dev/null +++ b/swap/tests/concurrent_bobs_insufficient_xmr.rs @@ -0,0 +1,87 @@ +pub mod harness; + +use harness::SlowCancelConfig; +use swap::asb::FixedRate; +use swap::protocol::alice::AliceState; +use swap::protocol::bob::BobState; +use swap::protocol::{alice, bob}; + +/// Two swaps run at once but the maker only holds enough Monero to fund ONE of +/// them (two 1-XMR outputs, each swap needs ~1.02 XMR). The serialized lock +/// phase must let the winner lock and make the loser fail CLEANLY at construct +/// time (an early Bitcoin refund) instead of building a lock that double-spends +/// the winner's inputs and then wedging on a permanently rejected publish. +#[tokio::test] +async fn concurrent_bobs_insufficient_xmr() { + harness::setup_test_funded(SlowCancelConfig, None, None, 2, |mut ctx| async move { + let (bob_swap_1, bob_join_handle_1) = ctx.bob_swap().await; + let bob_swap_1 = tokio::spawn(bob::run(bob_swap_1)); + let alice_swap_1 = ctx.alice_next_swap().await; + let alice_swap_1 = tokio::spawn(alice::run(alice_swap_1, FixedRate::default())); + + let (bob_swap_2, bob_join_handle_2) = ctx.bob_swap().await; + let bob_swap_2 = tokio::spawn(bob::run(bob_swap_2)); + let alice_swap_2 = ctx.alice_next_swap().await; + let alice_swap_2 = tokio::spawn(alice::run(alice_swap_2, FixedRate::default())); + + let bob_state_1 = bob_swap_1.await??; + let bob_state_2 = bob_swap_2.await??; + let alice_state_1 = alice_swap_1.await??; + let alice_state_2 = alice_swap_2.await??; + + bob_join_handle_1.abort(); + bob_join_handle_2.abort(); + + let alices = [&alice_state_1, &alice_state_2]; + let redeemed = alices + .iter() + .filter(|s| matches!(s, AliceState::BtcRedeemed)) + .count(); + // The un-fundable swap must refund cleanly. An early refund (construct + // failed, no Monero was locked) is the intended outcome; a plain refund + // is also acceptable, but the swap must reach a terminal refund state + // rather than wedge on a rejected double-spend publish. + let refunded = alices + .iter() + .filter(|s| { + matches!( + s, + AliceState::BtcEarlyRefunded(_) + | AliceState::SafelyAborted + | AliceState::XmrRefunded { .. } + ) + }) + .count(); + assert_eq!(redeemed, 1, "exactly one maker swap should lock and redeem"); + assert_eq!( + refunded, 1, + "the un-fundable maker swap must refund cleanly, not wedge; got {alice_state_1} / {alice_state_2}" + ); + + let bobs = [&bob_state_1, &bob_state_2]; + assert_eq!( + bobs.iter() + .filter(|s| matches!(s, BobState::XmrRedeemed { .. })) + .count(), + 1, + "one taker should redeem XMR" + ); + assert_eq!( + bobs.iter() + .filter(|s| { + matches!( + s, + BobState::BtcEarlyRefunded { .. } + | BobState::BtcEarlyRefundPublished { .. } + | BobState::BtcRefunded { .. } + ) + }) + .count(), + 1, + "the other taker should get its Bitcoin refunded (early refund, since the maker never locked Monero)" + ); + + Ok(()) + }) + .await; +} diff --git a/swap/tests/harness/mod.rs b/swap/tests/harness/mod.rs index 0e2dca4f4..27be88013 100644 --- a/swap/tests/harness/mod.rs +++ b/swap/tests/harness/mod.rs @@ -47,10 +47,11 @@ use uuid::Uuid; /// /// If use_subaddress is true, we will use a subaddress for the developer tip. We do this /// because using a subaddress changes things about the tx keys involved -pub async fn setup_test( +pub async fn setup_test_funded( _config: C, developer_tip_ratio: Option<(Decimal, bool)>, refund_policy: Option, + alice_xmr_outputs: u64, testfn: T, ) where T: Fn(TestContext) -> F, @@ -126,7 +127,7 @@ pub async fn setup_test( }; let alice_starting_balances = - StartingBalances::new(bitcoin::Amount::ZERO, xmr_amount, Some(10)); + StartingBalances::new(bitcoin::Amount::ZERO, xmr_amount, Some(alice_xmr_outputs)); let alice_seed = Seed::random().unwrap(); let alice_db_path = NamedTempFile::new().unwrap().path().to_path_buf(); let alice_config_path = alice_db_path.with_extension("config.toml"); @@ -297,6 +298,22 @@ ask_spread = "0" testfn(test).await.unwrap() } +/// Thin wrapper over [`setup_test_funded`] with the default maker Monero balance +/// (ten 1-XMR outputs), used by every test that does not need to constrain how +/// much unlocked XMR Alice holds. +pub async fn setup_test( + config: C, + developer_tip_ratio: Option<(Decimal, bool)>, + refund_policy: Option, + testfn: T, +) where + T: Fn(TestContext) -> F, + F: Future>, + C: GetConfig, +{ + setup_test_funded(config, developer_tip_ratio, refund_policy, 10, testfn).await +} + async fn init_containers(cli: &Cli) -> (Monero, Containers<'_>) { let prefix = random_prefix(); let bitcoind_name = format!("{}_{}", prefix, "bitcoind");