From 173d077751bce0522d5542f05c5ac04709f716e5 Mon Sep 17 00:00:00 2001 From: patrini32 Date: Mon, 1 Jul 2024 21:14:44 +0000 Subject: [PATCH] feat (Cli): Display reason for failed cancel-refund operation to the user (#1668) We now display the reason for a failed cancel-refund operation to the user. Fixes #683 --- .github/workflows/ci.yml | 1 + CHANGELOG.md | 1 + swap/src/api/request.rs | 1 + swap/src/cli/cancel_and_refund.rs | 130 ++++++++++++++---- swap/src/protocol/bob/state.rs | 37 ++--- ..._refund_using_cancel_and_refund_command.rs | 2 +- ...and_refund_command_timelock_not_expired.rs | 15 +- ...punishes_after_bob_dead_and_bob_cancels.rs | 98 +++++++++++++ 8 files changed, 224 insertions(+), 61 deletions(-) create mode 100644 swap/tests/alice_manually_punishes_after_bob_dead_and_bob_cancels.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2b0a0ce6..b3f742fc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -157,6 +157,7 @@ jobs: alice_and_bob_refund_using_cancel_and_refund_command, alice_and_bob_refund_using_cancel_then_refund_command, alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired, + alice_manually_punishes_after_bob_dead_and_bob_cancels, punish, alice_punishes_after_restart_bob_dead, alice_manually_punishes_after_bob_dead, diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c48cb79..098db809 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - CLI: Buffer received transfer proofs for later processing if we're currently running a different swap +- CLI: We now display the reason for a failed cancel-refund operation to the user (#683) ## [0.13.1] - 2024-06-10 diff --git a/swap/src/api/request.rs b/swap/src/api/request.rs index 800e1c06..50274ab1 100644 --- a/swap/src/api/request.rs +++ b/swap/src/api/request.rs @@ -821,6 +821,7 @@ impl Request { .await .map_err(|err| { method_span.in_scope(|| { + // The {:?} formatter is used to print the entire error chain tracing::debug!(err = format!("{:?}", err), "API call resulted in an error"); }); err diff --git a/swap/src/cli/cancel_and_refund.rs b/swap/src/cli/cancel_and_refund.rs index d542b7ed..e92283ea 100644 --- a/swap/src/cli/cancel_and_refund.rs +++ b/swap/src/cli/cancel_and_refund.rs @@ -1,5 +1,4 @@ -use crate::bitcoin::wallet::Subscription; -use crate::bitcoin::{parse_rpc_error_code, RpcErrorCode, Wallet}; +use crate::bitcoin::{ExpiredTimelocks, Wallet}; use crate::protocol::bob::BobState; use crate::protocol::Database; use anyhow::{bail, Result}; @@ -13,7 +12,7 @@ pub async fn cancel_and_refund( db: Arc, ) -> Result { if let Err(err) = cancel(swap_id, bitcoin_wallet.clone(), db.clone()).await { - tracing::info!(%err, "Could not submit cancel transaction"); + tracing::warn!(%err, "Could not cancel swap. Attempting to refund anyway"); }; let state = match refund(swap_id, bitcoin_wallet, db).await { @@ -21,7 +20,6 @@ pub async fn cancel_and_refund( Err(e) => bail!(e), }; - tracing::info!("Refund transaction submitted"); Ok(state) } @@ -29,7 +27,7 @@ pub async fn cancel( swap_id: Uuid, bitcoin_wallet: Arc, db: Arc, -) -> Result<(Txid, Subscription, BobState)> { +) -> Result<(Txid, BobState)> { let state = db.get_state(swap_id).await?.try_into()?; let state6 = match state { @@ -47,34 +45,69 @@ pub async fn cancel( | BobState::XmrRedeemed { .. } | BobState::BtcPunished { .. } | BobState::SafelyAborted => bail!( - "Cannot cancel swap {} because it is in state {} which is not refundable.", + "Cannot cancel swap {} because it is in state {} which is not cancellable.", swap_id, state ), }; - tracing::info!(%swap_id, "Manually cancelling swap"); + tracing::info!(%swap_id, "Attempting to manually cancel swap"); - let (txid, subscription) = match state6.submit_tx_cancel(bitcoin_wallet.as_ref()).await { - Ok(txid) => txid, + // Attempt to just publish the cancel transaction + match state6.submit_tx_cancel(bitcoin_wallet.as_ref()).await { + Ok((txid, _)) => { + let state = BobState::BtcCancelled(state6); + db.insert_latest_state(swap_id, state.clone().into()) + .await?; + Ok((txid, state)) + } + + // If we fail to submit the cancel transaction it can have one of two reasons: + // 1. The cancel timelock hasn't expired yet + // 2. The cancel transaction has already been published by Alice Err(err) => { - if let Ok(error_code) = parse_rpc_error_code(&err) { - tracing::debug!(%error_code, "parse rpc error"); - if error_code == i64::from(RpcErrorCode::RpcVerifyAlreadyInChain) { - tracing::info!("Cancel transaction has already been confirmed on chain"); - } else if error_code == i64::from(RpcErrorCode::RpcVerifyError) { - tracing::info!("General error trying to submit cancel transaction"); + // Check if Alice has already published the cancel transaction while we were absent + if let Ok(tx) = state6.check_for_tx_cancel(bitcoin_wallet.as_ref()).await { + let state = BobState::BtcCancelled(state6); + db.insert_latest_state(swap_id, state.clone().into()) + .await?; + tracing::info!("Alice has already cancelled the swap"); + return Ok((tx.txid(), state)); + } + + // The cancel transaction has not been published yet and we failed to publish it ourselves + // Here we try to figure out why + match state6.expired_timelock(bitcoin_wallet.as_ref()).await { + // We cannot cancel because Alice has already cancelled and punished afterwards + Ok(ExpiredTimelocks::Punish { .. }) => { + let state = BobState::BtcPunished { + tx_lock_id: state6.tx_lock_id(), + }; + db.insert_latest_state(swap_id, state.clone().into()) + .await?; + tracing::info!("You have been punished for not refunding in time"); + bail!(err.context("Cannot cancel swap because we have already been punished")); + } + // We cannot cancel because the cancel timelock has not expired yet + Ok(ExpiredTimelocks::None { blocks_left }) => { + bail!(err.context( + format!( + "Cannot cancel swap because the cancel timelock has not expired yet. Blocks left: {}", + blocks_left + ) + )); + } + Ok(ExpiredTimelocks::Cancel { .. }) => { + bail!(err.context("Failed to cancel swap even though cancel timelock has expired. This is unexpected.")); + } + Err(timelock_err) => { + bail!(err + .context(timelock_err) + .context("Failed to cancel swap and could not check timelock status")); } } - bail!(err); } - }; - - let state = BobState::BtcCancelled(state6); - db.insert_latest_state(swap_id, state.clone().into()) - .await?; - - Ok((txid, subscription, state)) + } } pub async fn refund( @@ -104,12 +137,51 @@ pub async fn refund( ), }; - tracing::info!(%swap_id, "Manually refunding swap"); - state6.publish_refund_btc(bitcoin_wallet.as_ref()).await?; + tracing::info!(%swap_id, "Attempting to manually refund swap"); - let state = BobState::BtcRefunded(state6); - db.insert_latest_state(swap_id, state.clone().into()) - .await?; + // Attempt to just publish the refund transaction + match state6.publish_refund_btc(bitcoin_wallet.as_ref()).await { + Ok(_) => { + let state = BobState::BtcRefunded(state6); + db.insert_latest_state(swap_id, state.clone().into()) + .await?; - Ok(state) + Ok(state) + } + + // If we fail to submit the refund transaction it can have one of two reasons: + // 1. The cancel transaction has not been published yet + // 2. The refund timelock has already expired and we have been punished + Err(bitcoin_publication_err) => { + match state6.expired_timelock(bitcoin_wallet.as_ref()).await { + // We have been punished + Ok(ExpiredTimelocks::Punish { .. }) => { + let state = BobState::BtcPunished { + tx_lock_id: state6.tx_lock_id(), + }; + db.insert_latest_state(swap_id, state.clone().into()) + .await?; + tracing::info!("You have been punished for not refunding in time"); + bail!(bitcoin_publication_err + .context("Cannot refund swap because we have already been punished")); + } + Ok(ExpiredTimelocks::None { blocks_left }) => { + bail!( + bitcoin_publication_err.context(format!( + "Cannot refund swap because the cancel timelock has not expired yet. Blocks left: {}", + blocks_left + )) + ); + } + Ok(ExpiredTimelocks::Cancel { .. }) => { + bail!(bitcoin_publication_err.context("Failed to refund swap even though cancel timelock has expired. This should is unexpected.")); + } + Err(e) => { + bail!(bitcoin_publication_err + .context(e) + .context("Failed to refund swap and could not check timelock status")); + } + } + } + } } diff --git a/swap/src/protocol/bob/state.rs b/swap/src/protocol/bob/state.rs index 58c1d723..03f56d28 100644 --- a/swap/src/protocol/bob/state.rs +++ b/swap/src/protocol/bob/state.rs @@ -646,18 +646,20 @@ impl State6 { tx_cancel_status, )) } - - pub async fn check_for_tx_cancel( - &self, - bitcoin_wallet: &bitcoin::Wallet, - ) -> Result { - let tx_cancel = bitcoin::TxCancel::new( + pub fn construct_tx_cancel(&self) -> Result { + bitcoin::TxCancel::new( &self.tx_lock, self.cancel_timelock, self.A, self.b.public(), self.tx_cancel_fee, - )?; + ) + } + pub async fn check_for_tx_cancel( + &self, + bitcoin_wallet: &bitcoin::Wallet, + ) -> Result { + let tx_cancel = self.construct_tx_cancel()?; let tx = bitcoin_wallet.get_raw_transaction(tx_cancel.txid()).await?; @@ -668,15 +670,10 @@ impl State6 { &self, bitcoin_wallet: &bitcoin::Wallet, ) -> Result<(Txid, Subscription)> { - let transaction = bitcoin::TxCancel::new( - &self.tx_lock, - self.cancel_timelock, - self.A, - self.b.public(), - self.tx_cancel_fee, - )? - .complete_as_bob(self.A, self.b.clone(), self.tx_cancel_sig_a.clone()) - .context("Failed to complete Bitcoin cancel transaction")?; + let transaction = self + .construct_tx_cancel()? + .complete_as_bob(self.A, self.b.clone(), self.tx_cancel_sig_a.clone()) + .context("Failed to complete Bitcoin cancel transaction")?; let (tx_id, subscription) = bitcoin_wallet.broadcast(transaction, "cancel").await?; @@ -691,13 +688,7 @@ impl State6 { } pub fn signed_refund_transaction(&self) -> Result { - let tx_cancel = bitcoin::TxCancel::new( - &self.tx_lock, - self.cancel_timelock, - self.A, - self.b.public(), - self.tx_cancel_fee, - )?; + let tx_cancel = self.construct_tx_cancel()?; let tx_refund = bitcoin::TxRefund::new(&tx_cancel, &self.refund_address, self.tx_refund_fee); diff --git a/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command.rs b/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command.rs index 1870bed4..04cabec4 100644 --- a/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command.rs +++ b/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command.rs @@ -50,7 +50,7 @@ async fn given_alice_and_bob_manually_refund_after_funds_locked_both_refund() { // Bob manually cancels bob_join_handle.abort(); - let (_, _, state) = cli::cancel(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db).await?; + let (_, state) = cli::cancel(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db).await?; assert!(matches!(state, BobState::BtcCancelled { .. })); let (bob_swap, bob_join_handle) = ctx diff --git a/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired.rs b/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired.rs index 16953866..6584a3d2 100644 --- a/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired.rs +++ b/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired.rs @@ -42,10 +42,10 @@ async fn given_alice_and_bob_manually_cancel_when_timelock_not_expired_errors() let error = cli::cancel(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db) .await .unwrap_err(); - assert_eq!( - parse_rpc_error_code(&error).unwrap(), - i64::from(RpcErrorCode::RpcVerifyRejected) - ); + + assert!(error + .to_string() + .contains("Cannot cancel swap because the cancel timelock has not expired yet")); ctx.restart_alice().await; let alice_swap = ctx.alice_next_swap().await; @@ -72,10 +72,9 @@ async fn given_alice_and_bob_manually_cancel_when_timelock_not_expired_errors() let error = cli::refund(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db) .await .unwrap_err(); - assert_eq!( - parse_rpc_error_code(&error).unwrap(), - i64::from(RpcErrorCode::RpcVerifyError) - ); + assert!(error + .to_string() + .contains("Cannot refund swap because the cancel timelock has not expired yet")); let (bob_swap, _) = ctx .stop_and_resume_bob_from_db(bob_join_handle, swap_id) diff --git a/swap/tests/alice_manually_punishes_after_bob_dead_and_bob_cancels.rs b/swap/tests/alice_manually_punishes_after_bob_dead_and_bob_cancels.rs new file mode 100644 index 00000000..374d6160 --- /dev/null +++ b/swap/tests/alice_manually_punishes_after_bob_dead_and_bob_cancels.rs @@ -0,0 +1,98 @@ +pub mod harness; + +use harness::alice_run_until::is_xmr_lock_transaction_sent; +use harness::bob_run_until::is_btc_locked; +use harness::FastPunishConfig; +use swap::asb; +use swap::asb::FixedRate; +use swap::cli; +use swap::protocol::alice::AliceState; +use swap::protocol::bob::BobState; +use swap::protocol::{alice, bob}; +/// Bob locks Btc and Alice locks Xmr. Bob does not act; he fails to send Alice +/// the encsig and fail to refund or redeem. Alice punishes using the cancel and +/// punish command. Then Bob tries to refund. +#[tokio::test] +async fn alice_manually_punishes_after_bob_dead_and_bob_cancels() { + harness::setup_test(FastPunishConfig, |mut ctx| async move { + let (bob_swap, bob_join_handle) = ctx.bob_swap().await; + let bob_swap_id = bob_swap.id; + let bob_swap = tokio::spawn(bob::run_until(bob_swap, is_btc_locked)); + + let alice_swap = ctx.alice_next_swap().await; + let alice_bitcoin_wallet = alice_swap.bitcoin_wallet.clone(); + + let alice_swap = tokio::spawn(alice::run_until( + alice_swap, + is_xmr_lock_transaction_sent, + FixedRate::default(), + )); + + let bob_state = bob_swap.await??; + assert!(matches!(bob_state, BobState::BtcLocked { .. })); + + let alice_state = alice_swap.await??; + + // Ensure cancel timelock is expired + if let AliceState::XmrLockTransactionSent { state3, .. } = alice_state { + alice_bitcoin_wallet + .subscribe_to(state3.tx_lock) + .await + .wait_until_confirmed_with(state3.cancel_timelock) + .await?; + } else { + panic!("Alice in unexpected state {}", alice_state); + } + + // manual cancel (required to be able to punish) + + ctx.restart_alice().await; + let alice_swap = ctx.alice_next_swap().await; + let (_, alice_state) = + asb::cancel(alice_swap.swap_id, alice_swap.bitcoin_wallet, alice_swap.db).await?; + + // Ensure punish timelock is expired + if let AliceState::BtcCancelled { state3, .. } = alice_state { + alice_bitcoin_wallet + .subscribe_to(state3.tx_cancel()) + .await + .wait_until_confirmed_with(state3.punish_timelock) + .await?; + } else { + panic!("Alice in unexpected state {}", alice_state); + } + + // manual punish + + ctx.restart_alice().await; + let alice_swap = ctx.alice_next_swap().await; + let (_, alice_state) = + asb::punish(alice_swap.swap_id, alice_swap.bitcoin_wallet, alice_swap.db).await?; + ctx.assert_alice_punished(alice_state).await; + // Bob is in wrong state. + let (bob_swap, bob_join_handle) = ctx + .stop_and_resume_bob_from_db(bob_join_handle, bob_swap_id) + .await; + assert!(matches!(bob_swap.state, BobState::BtcLocked { .. })); + bob_join_handle.abort(); + + let (_, state) = cli::cancel(bob_swap_id, bob_swap.bitcoin_wallet, bob_swap.db).await?; + // Bob should be in BtcCancelled state now. + assert!(matches!(state, BobState::BtcCancelled { .. })); + + let (bob_swap, _) = ctx + .stop_and_resume_bob_from_db(bob_join_handle, bob_swap_id) + .await; + assert!(matches!(bob_swap.state, BobState::BtcCancelled { .. })); + // Alice punished Bob, so he should be in the BtcPunished state. + let error = cli::refund(bob_swap_id, bob_swap.bitcoin_wallet, bob_swap.db) + .await + .unwrap_err(); + assert_eq!( + error.to_string(), + "Cannot refund swap because we have already been punished" + ); + Ok(()) + }) + .await; +}