From c8cf631ea3a73f6d1f67c9ca37ad4817e2aaf0bd Mon Sep 17 00:00:00 2001 From: Louis Laugier <23471791+louislaugier@users.noreply.github.com> Date: Wed, 19 Aug 2026 21:10:25 +0200 Subject: [PATCH] fix(asb): serialize Monero locks and refund unfundable swaps Two overlapping swaps could select the same Monero output when building their lock transactions. wallet2 only marks an output spent once its lock tx is relayed, and monero-sys has no reserve API, so the second swap's lock tx becomes a permanent double-spend that monerod rejects forever -- the swap then hangs until the cancel timelock and the taker is refunded ("xmr is refundable"). A related case: when concurrent swaps together need more XMR than the maker holds, even a serialized swap can be forced to reselect a sibling's freshly spent outputs, build a double-spend, and then retry the rejected publish forever, since the publish has no deadline. Serialize the construct-through-first-publish window with a process-wide async mutex so at most one unpublished lock tx exists at a time; overlapping swaps that are each fundable pick different outputs and both succeed. A max-hold deadline releases the mutex if a swap wedges on a rejected publish. Under the guard, before constructing, check that the unlocked balance covers the lock and fail with a permanent error (an early Bitcoin refund; no Monero was locked, so it is safe) if a sibling already took the shared balance, instead of building a doomed lock. Two mainnet wedges were then followed end to end on a live maker running this change, and they split the failure into two distinct signatures: Swap 5eabdea8 (2026-08-26) -- insufficient balance, missed. Wallet 13.156 XMR against a 13.963 XMR lock; the pre-check still PASSED (it never logged its refusal), the publish was rejected with an empty-reason TransactionRejected, and the swap retried that same transaction until the cancel timelock, ~4.2h. The balance recovering mid-wedge (20.29 XMR unreserved for 33 minutes) changed nothing. Swap 0abd1dcd (2026-08-27) -- ample balance, unreachable by any balance check. Wallet 41.3 XMR unlocked against a 6.85 XMR lock; the pre-check was RIGHT to pass, but the constructed tx had selected outputs freshly spent by an earlier swap that wallet2 had not yet reflected. monerod rejected that one fixed transaction 799 times over 3.9h until the cancel timelock. Meanwhile the max-hold deadline released the mutex and sibling swap 4f885739 constructed with fresh outputs and completed end-to-end DURING the wedge -- the serialization half doing exactly its job. Neither wedge ever locked Monero; both takers were refunded. What this pins down: the construct-time gate must operate on OUTPUTS, not balance -- the daemon-side is_key_image_spent query, as the primary check. And the two end-states want different exits: insufficient funds routes to the early Bitcoin refund this patch adds, while sufficient-funds-stale-selection would have been SAVED by rebuilding the lock tx on rejection -- the direction #1142 explores, complementary to this patch rather than superseded by it. Verified with the concurrent_bobs_before_xmr_lock_proof_sent, concurrent_bobs_after_xmr_lock_proof_sent, a new concurrent_bobs_insufficient_xmr, and happy_path integration tests. Refs #120 Co-Authored-By: Claude Opus 5 (1M context) Per review, the serialization logic lives in MoneroLockPhase / LockPhaseSession (protocol::alice::lock_phase), a unit-tested struct, so the state machine only reports whether its current state is inside the phase and reacts to the deadline. --- CHANGELOG.md | 5 + swap/src/protocol/alice.rs | 1 + swap/src/protocol/alice/lock_phase.rs | 202 ++++++++++++++++++ swap/src/protocol/alice/swap.rs | 153 ++++++++++++- .../tests/concurrent_bobs_insufficient_xmr.rs | 87 ++++++++ swap/tests/harness/mod.rs | 21 +- 6 files changed, 458 insertions(+), 11 deletions(-) create mode 100644 swap/src/protocol/alice/lock_phase.rs create mode 100644 swap/tests/concurrent_bobs_insufficient_xmr.rs 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");