From 0936d6210e62d806c62d3c9b01fa9cab7f2068e7 Mon Sep 17 00:00:00 2001 From: Mohan <86064887+binarybaron@users.noreply.github.com> Date: Mon, 4 Aug 2025 11:46:57 +0200 Subject: [PATCH] fix(swap): Split approve-and-sign and publish-lock-tx into two states (#498) * fix(swap): Split approve-and-sign and publish-lock-tx into two states * fix: cannot get blockchain height * add RetrievingMoneroBlockheight, RetrievingMoneroBlockheight tauri events * propagate daemon blcok height fetch error, treat height 0 as error * check if tx_lock was previously published --- CHANGELOG.md | 2 + justfile | 6 +++ monero-sys/src/bridge.rs | 5 +- monero-sys/src/lib.rs | 40 ++++++++++------ src-gui/src/models/tauriModelExt.ts | 3 ++ .../alert/SwapStatusAlert/SwapStatusAlert.tsx | 4 ++ .../pages/swap/swap/SwapStatePage.tsx | 8 ++++ swap/src/cli/api/tauri_bindings.rs | 8 ++-- swap/src/cli/cancel_and_refund.rs | 13 +++++ swap/src/database/bob.rs | 24 ++++++++++ swap/src/protocol/bob/state.rs | 9 ++++ swap/src/protocol/bob/swap.rs | 48 +++++++++++++++---- swap/tests/harness/mod.rs | 9 ++-- 13 files changed, 147 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index abbdf12a..0d5c767f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- GUI + CLI: Fixed a potential race condition where if the user closed the app while the Bitcoin was in the process of being published, manual recovery would be required to get to a recoverable state. + ## [3.0.0-beta.4] - 2025-08-03 - GUI: The following rendezvous points have been added to the default list of rendezvous points. If you're running a maker, please add them to your config file (under `network.rendezvous_point`). They are operated by members of the community that have shown to be reliable. These rendezvous points act as bootstrapping nodes for peer discovery: `/dns4/eigen.center/tcp/8888/p2p/12D3KooWS5RaYJt4ANKMH4zczGVhNcw5W214e2DDYXnjs5Mx5zAT`, `/dns4/swapanarchy.cfd/tcp/8888/p2p/12D3KooWRtyVpmyvwzPYXuWyakFbRKhyXGrjhq6tP7RrBofpgQGp`, `/dns4/darkness.su/tcp/8888/p2p/12D3KooWFQAgVVS9t9UgL6v1sLprJVM7am5hFK7vy9iBCCoCBYmU` diff --git a/justfile b/justfile index 19c1c8dc..2d928dcb 100644 --- a/justfile +++ b/justfile @@ -53,6 +53,12 @@ gui_build: tests: cargo nextest run +docker_test_happy_path: + cargo test --package swap --test happy_path -- --nocapture + +docker_test_all: + cargo test --package swap --test all -- --nocapture + # Tests the Rust bindings for Monero test_monero_sys: cd monero-sys && cargo nextest run diff --git a/monero-sys/src/bridge.rs b/monero-sys/src/bridge.rs index a87c69cf..a00c88af 100644 --- a/monero-sys/src/bridge.rs +++ b/monero-sys/src/bridge.rs @@ -202,9 +202,12 @@ pub mod ffi { /// Set a listener to the wallet. unsafe fn setListener(self: Pin<&mut Wallet>, listener: *mut WalletListener) -> Result<()>; - /// Get the daemon's blockchain height. + /// Get the daemon's blockchain target height. fn daemonBlockChainTargetHeight(self: &Wallet) -> Result; + /// Get the daemon's blockchain height. + fn daemonBlockChainHeight(self: &Wallet) -> Result; + /// Check if wallet was ever synchronized. fn synchronized(self: &Wallet) -> Result; diff --git a/monero-sys/src/lib.rs b/monero-sys/src/lib.rs index c950a802..ef80f7ab 100644 --- a/monero-sys/src/lib.rs +++ b/monero-sys/src/lib.rs @@ -398,21 +398,28 @@ impl WalletHandle { /// Retries at most 5 times with a 500ms delay between attempts. pub async fn blockchain_height(&self) -> anyhow::Result { const MAX_RETRIES: u64 = 5; + const RETRY_DELAY: u64 = 500; + + let mut last_error = None; for _ in 0..MAX_RETRIES { - if let Some(height) = self + match self .call(move |wallet| wallet.daemon_blockchain_height()) .await { - return Ok(height); + Ok(0) => last_error = Some(anyhow!("Daemon blockchain height is 0")), + Err(e) => last_error = Some(e), + Ok(height) => return Ok(height), } - tokio::time::sleep(std::time::Duration::from_millis(500)).await; + tracing::warn!(error=%last_error.as_ref().unwrap_or(&anyhow!("Unknown error")), "Failed to get blockchain height, retrying in {}ms", RETRY_DELAY); + + tokio::time::sleep(std::time::Duration::from_millis(RETRY_DELAY)).await; } self.check_wallet().await?; - bail!("Failed to get blockchain height after 5 attempts"); + bail!("Failed to get blockchain height after 5 attempts: {last_error:?}"); } /// Transfer funds to an address without approval. @@ -1705,20 +1712,23 @@ impl FfiWallet { /// /// Returns the height of the blockchain, if connected. /// Returns None if not connected. - fn daemon_blockchain_height(&self) -> Option { + fn daemon_blockchain_height(&self) -> anyhow::Result { // Here we actually use the _target_ height -- incase the remote node is // currently catching up we want to work with the height it ends up at. - match self + let target_height = self.inner.daemonBlockChainTargetHeight().context( + "Failed to get daemon blockchain target height: FFI call failed with exception", + )?; + + let height = self .inner - .daemonBlockChainTargetHeight() - .context( - "Failed to get daemon blockchain target height: FFI call failed with exception", - ) - .expect("Shouldn't panic") - { - 0 => None, - height => Some(height), - } + .daemonBlockChainHeight() + .context("Failed to get daemon blockchain height: FFI call failed with exception")?; + + // wallet2 is such a crazy construction that sometimes the target_height is less than the current height + // we therefore take the max of the two + let max_height = std::cmp::max(height, target_height); + + Ok(max_height) } /// Get the total balance across all accounts. diff --git a/src-gui/src/models/tauriModelExt.ts b/src-gui/src/models/tauriModelExt.ts index 0d058442..f2a5f7d3 100644 --- a/src-gui/src/models/tauriModelExt.ts +++ b/src-gui/src/models/tauriModelExt.ts @@ -25,6 +25,7 @@ export type TauriSwapProgressEventExt = export enum BobStateName { Started = "quote has been requested", SwapSetupCompleted = "execution setup done", + BtcLockReadyToPublish = "btc lock ready to publish", BtcLocked = "btc is locked", XmrLockProofReceived = "XMR lock transaction transfer proof received", XmrLocked = "xmr is locked", @@ -47,6 +48,8 @@ export function bobStateNameToHumanReadable(stateName: BobStateName): string { return "Started"; case BobStateName.SwapSetupCompleted: return "Setup completed"; + case BobStateName.BtcLockReadyToPublish: + return "Bitcoin lock ready to publish"; case BobStateName.BtcLocked: return "Bitcoin locked"; case BobStateName.XmrLockProofReceived: diff --git a/src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx b/src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx index 1e4d3cdb..c2112c7b 100644 --- a/src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx +++ b/src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx @@ -216,6 +216,10 @@ export function StateAlert({ } return ; + // If the Bitcoin lock transaction has not been published yet + // there is no need to display an alert + case BobStateName.BtcLockReadyToPublish: + return null; default: exhaustiveGuard(swap.state_name); } diff --git a/src-gui/src/renderer/components/pages/swap/swap/SwapStatePage.tsx b/src-gui/src/renderer/components/pages/swap/swap/SwapStatePage.tsx index 52f5e53d..b64165b2 100644 --- a/src-gui/src/renderer/components/pages/swap/swap/SwapStatePage.tsx +++ b/src-gui/src/renderer/components/pages/swap/swap/SwapStatePage.tsx @@ -50,6 +50,14 @@ export default function SwapStatePage({ state }: { state: SwapState | null }) { return ; } break; + case "RetrievingMoneroBlockheight": + return ( + + ); + case "BtcLockPublishInflight": + return ( + + ); case "BtcLockTxInMempool": if (state.curr.type === "BtcLockTxInMempool") { return ; diff --git a/swap/src/cli/api/tauri_bindings.rs b/swap/src/cli/api/tauri_bindings.rs index e2380bca..6de1f3a7 100644 --- a/swap/src/cli/api/tauri_bindings.rs +++ b/swap/src/cli/api/tauri_bindings.rs @@ -603,7 +603,8 @@ impl TauriEmitter for Option { ) -> Result { match self { Some(tauri) => tauri.request_bitcoin_approval(details, timeout_secs).await, - None => bail!("No Tauri handle available"), + // If no TauriHandle is available, we just approve the request + None => Ok(true), } } @@ -886,10 +887,9 @@ pub enum TauriSwapProgressEvent { #[typeshare(serialized_as = "number")] #[serde(with = "::bitcoin::amount::serde::as_sat")] btc_lock_amount: bitcoin::Amount, - #[typeshare(serialized_as = "number")] - #[serde(with = "::bitcoin::amount::serde::as_sat")] - btc_tx_lock_fee: bitcoin::Amount, }, + RetrievingMoneroBlockheight, + BtcLockPublishInflight, BtcLockTxInMempool { #[typeshare(serialized_as = "string")] btc_lock_txid: bitcoin::Txid, diff --git a/swap/src/cli/cancel_and_refund.rs b/swap/src/cli/cancel_and_refund.rs index a43987ac..00ab9925 100644 --- a/swap/src/cli/cancel_and_refund.rs +++ b/swap/src/cli/cancel_and_refund.rs @@ -42,6 +42,14 @@ pub async fn cancel( // We do not know the block height, so we set it to 0 state3.cancel(BlockHeight { height: 0 }) } + // Refunding in this state is not possible but we still allow it + // because this function is only used for recovery purposes. + // We can try to do a refund here so we do. + BobState::BtcLockReadyToPublish { + state3, + monero_wallet_restore_blockheight, + .. + } => state3.cancel(monero_wallet_restore_blockheight), BobState::BtcLocked { state3, monero_wallet_restore_blockheight, @@ -144,6 +152,11 @@ pub async fn refund( state3, monero_wallet_restore_blockheight, } => state3.cancel(monero_wallet_restore_blockheight), + BobState::BtcLockReadyToPublish { + state3, + monero_wallet_restore_blockheight, + .. + } => state3.cancel(monero_wallet_restore_blockheight), BobState::XmrLockProofReceived { state, monero_wallet_restore_blockheight, diff --git a/swap/src/database/bob.rs b/swap/src/database/bob.rs index d459cabe..9affda68 100644 --- a/swap/src/database/bob.rs +++ b/swap/src/database/bob.rs @@ -17,6 +17,11 @@ pub enum Bob { ExecutionSetupDone { state2: bob::State2, }, + BtcLockReadyToPublish { + btc_lock_tx_signed: bitcoin::Transaction, + state3: bob::State3, + monero_wallet_restore_blockheight: BlockHeight, + }, BtcLocked { state3: bob::State3, monero_wallet_restore_blockheight: BlockHeight, @@ -65,6 +70,15 @@ impl From for Bob { tx_lock_fee, }, BobState::SwapSetupCompleted(state2) => Bob::ExecutionSetupDone { state2 }, + BobState::BtcLockReadyToPublish { + btc_lock_tx_signed, + state3, + monero_wallet_restore_blockheight, + } => Bob::BtcLockReadyToPublish { + btc_lock_tx_signed, + state3, + monero_wallet_restore_blockheight, + }, BobState::BtcLocked { state3, monero_wallet_restore_blockheight, @@ -114,6 +128,15 @@ impl From for BobState { tx_lock_fee, }, Bob::ExecutionSetupDone { state2 } => BobState::SwapSetupCompleted(state2), + Bob::BtcLockReadyToPublish { + btc_lock_tx_signed, + state3, + monero_wallet_restore_blockheight, + } => BobState::BtcLockReadyToPublish { + btc_lock_tx_signed, + state3, + monero_wallet_restore_blockheight, + }, Bob::BtcLocked { state3, monero_wallet_restore_blockheight, @@ -153,6 +176,7 @@ impl fmt::Display for Bob { match self { Bob::Started { .. } => write!(f, "Started"), Bob::ExecutionSetupDone { .. } => f.write_str("Execution setup done"), + Bob::BtcLockReadyToPublish { .. } => f.write_str("Bitcoin lock ready to publish"), Bob::BtcLocked { .. } => f.write_str("Bitcoin locked"), Bob::XmrLockProofReceived { .. } => { f.write_str("XMR lock transaction transfer proof received") diff --git a/swap/src/protocol/bob/state.rs b/swap/src/protocol/bob/state.rs index 8b59f9f2..55cc39af 100644 --- a/swap/src/protocol/bob/state.rs +++ b/swap/src/protocol/bob/state.rs @@ -32,6 +32,11 @@ pub enum BobState { change_address: bitcoin::Address, }, SwapSetupCompleted(State2), + BtcLockReadyToPublish { + btc_lock_tx_signed: Transaction, + state3: State3, + monero_wallet_restore_blockheight: BlockHeight, + }, BtcLocked { state3: State3, monero_wallet_restore_blockheight: BlockHeight, @@ -65,6 +70,9 @@ impl fmt::Display for BobState { match self { BobState::Started { .. } => write!(f, "quote has been requested"), BobState::SwapSetupCompleted(..) => write!(f, "execution setup done"), + BobState::BtcLockReadyToPublish { .. } => { + write!(f, "btc lock ready to publish") + } BobState::BtcLocked { .. } => write!(f, "btc is locked"), BobState::XmrLockProofReceived { .. } => { write!(f, "XMR lock transaction transfer proof received") @@ -94,6 +102,7 @@ impl BobState { ) -> Result> { Ok(match self.clone() { BobState::Started { .. } + | BobState::BtcLockReadyToPublish { .. } | BobState::SafelyAborted | BobState::SwapSetupCompleted(_) => None, BobState::BtcLocked { state3: state, .. } diff --git a/swap/src/protocol/bob/swap.rs b/swap/src/protocol/bob/swap.rs index e6c42c55..aa8d3ee8 100644 --- a/swap/src/protocol/bob/swap.rs +++ b/swap/src/protocol/bob/swap.rs @@ -127,8 +127,6 @@ async fn next_state( swap_id, TauriSwapProgressEvent::SwapSetupInflight { btc_lock_amount: btc_amount, - // TODO: Replace this with the actual fee - btc_tx_lock_fee: bitcoin::Amount::ZERO, }, ); @@ -182,7 +180,9 @@ async fn next_state( match approval_result { Ok(true) => { - tracing::debug!("User approved swap offer"); + tracing::debug!( + "User approved swap offer, fetching current Monero blockheight" + ); // Record the current monero wallet block height so we don't have to scan from // block 0 once we create the redeem wallet. @@ -193,15 +193,23 @@ async fn next_state( // If the Monero transaction gets confirmed before Bob comes online again then // Bob would record a wallet-height that is past the lock transaction height, // which can lead to the wallet not detect the transaction. + event_emitter.emit_swap_progress_event( + swap_id, + TauriSwapProgressEvent::RetrievingMoneroBlockheight, + ); + let monero_wallet_restore_blockheight = monero_wallet .blockchain_height() .await .context("Failed to fetch current Monero blockheight")?; - // Publish the signed Bitcoin lock transaction - let (..) = bitcoin_wallet.broadcast(signed_tx, "lock").await?; + tracing::debug!( + %monero_wallet_restore_blockheight, + "User approved swap offer, recording monero wallet restore blockheight", + ); - BobState::BtcLocked { + BobState::BtcLockReadyToPublish { + btc_lock_tx_signed: signed_tx, state3, monero_wallet_restore_blockheight, } @@ -218,8 +226,32 @@ async fn next_state( } } } - // Bob has locked Bitcoin - // Watch for Alice to lock Monero or for cancel timelock to elapse + // User has approved the swap + // Bitcoin lock transaction has been signed + // Monero restore height has been recorded + BobState::BtcLockReadyToPublish { + btc_lock_tx_signed, + state3, + monero_wallet_restore_blockheight, + } => { + event_emitter + .emit_swap_progress_event(swap_id, TauriSwapProgressEvent::BtcLockPublishInflight); + + // Check if the transaction has already been broadcasted + // It could be that the operation was aborted after the transaction reached the Electrum server + // but before we transitioned to the BtcLocked state + if let Ok(Some(_)) = bitcoin_wallet.get_raw_transaction(state3.tx_lock_id()).await { + tracing::info!(txid = %state3.tx_lock_id(), "Bitcoin lock transaction already published, skipping publish"); + }else { + // Publish the signed Bitcoin lock transaction + let (..) = bitcoin_wallet.broadcast(btc_lock_tx_signed, "lock").await?; + } + + BobState::BtcLocked { + state3, + monero_wallet_restore_blockheight, + } + } BobState::BtcLocked { state3, monero_wallet_restore_blockheight, diff --git a/swap/tests/harness/mod.rs b/swap/tests/harness/mod.rs index 94d87bba..60611b2a 100644 --- a/swap/tests/harness/mod.rs +++ b/swap/tests/harness/mod.rs @@ -315,6 +315,7 @@ async fn init_test_wallets( monero::Network::Mainnet, true, None, + None, ) .await .unwrap(); @@ -1125,7 +1126,7 @@ pub struct SlowCancelConfig; impl GetConfig for SlowCancelConfig { fn get_config() -> Config { Config { - bitcoin_cancel_timelock: CancelTimelock::new(180), + bitcoin_cancel_timelock: CancelTimelock::new(180).into(), ..env::Regtest::get_config() } } @@ -1136,7 +1137,7 @@ pub struct FastCancelConfig; impl GetConfig for FastCancelConfig { fn get_config() -> Config { Config { - bitcoin_cancel_timelock: CancelTimelock::new(10), + bitcoin_cancel_timelock: CancelTimelock::new(10).into(), ..env::Regtest::get_config() } } @@ -1147,8 +1148,8 @@ pub struct FastPunishConfig; impl GetConfig for FastPunishConfig { fn get_config() -> Config { Config { - bitcoin_cancel_timelock: CancelTimelock::new(10), - bitcoin_punish_timelock: PunishTimelock::new(10), + bitcoin_cancel_timelock: CancelTimelock::new(10).into(), + bitcoin_punish_timelock: PunishTimelock::new(10).into(), ..env::Regtest::get_config() } }