From f511ff093cd422acb9bb22b3a04b070122f871bb Mon Sep 17 00:00:00 2001 From: rishflab Date: Thu, 2 Sep 2021 09:16:25 +1000 Subject: [PATCH] Make --force the default behaviour for manual recovery Remove the force flag. There is a resume command that tries to gracefully restarts the protocol and tries to execute the happy path. Remove e2e tests which test the --force flag. --- .github/workflows/ci.yml | 1 - CHANGELOG.md | 6 + bors.toml | 1 - swap/src/asb/command.rs | 33 ++---- swap/src/asb/recovery/cancel.rs | 45 +++---- swap/src/asb/recovery/punish.rs | 94 ++++----------- swap/src/asb/recovery/redeem.rs | 13 +-- swap/src/asb/recovery/refund.rs | 92 +++++---------- swap/src/bin/asb.rs | 17 +-- swap/src/bin/swap.rs | 16 +-- swap/src/cli/cancel.rs | 38 +++--- swap/src/cli/command.rs | 22 +--- swap/src/cli/refund.rs | 58 ++++----- ..._refund_using_cancel_and_refund_command.rs | 27 +---- ...and_refund_command_timelock_not_expired.rs | 42 +++---- ...fund_command_timelock_not_expired_force.rs | 110 ------------------ .../alice_manually_punishes_after_bob_dead.rs | 18 +-- ..._manually_redeems_after_enc_sig_learned.rs | 1 - 18 files changed, 150 insertions(+), 484 deletions(-) delete mode 100644 swap/tests/alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired_force.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0ea8b7c5..0c1cea2b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -99,7 +99,6 @@ jobs: happy_path_restart_alice_after_xmr_locked, alice_and_bob_refund_using_cancel_and_refund_command, alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired, - alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired_force, punish, alice_punishes_after_restart_bob_dead, alice_manually_punishes_after_bob_dead, diff --git a/CHANGELOG.md b/CHANGELOG.md index e243cf5b..5f205f00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- The `cancel`, `refund` and `punish` subcommands in ASB and CLI are run with the `--force` by default and the `--force` option has been removed. + The force flag was used to ignore blockheight and protocol state checks. + Users can still restart a swap with these checks using the `resume` subcommand. + ## [0.8.3] - 2021-09-03 ### Fixed diff --git a/bors.toml b/bors.toml index 70016fc1..245041d2 100644 --- a/bors.toml +++ b/bors.toml @@ -12,7 +12,6 @@ status = [ "docker_tests (happy_path_restart_bob_before_xmr_locked)", "docker_tests (alice_and_bob_refund_using_cancel_and_refund_command)", "docker_tests (alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired)", - "docker_tests (alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired_force)", "docker_tests (punish)", "docker_tests (alice_punishes_after_restart_bob_dead)", "docker_tests (alice_manually_punishes_after_bob_dead)", diff --git a/swap/src/asb/command.rs b/swap/src/asb/command.rs index b09fc664..81019535 100644 --- a/swap/src/asb/command.rs +++ b/swap/src/asb/command.rs @@ -56,7 +56,7 @@ where cmd: Command::Balance, }, RawCommand::ManualRecovery(ManualRecovery::Redeem { - redeem_params: RecoverCommandParams { swap_id, force }, + redeem_params: RecoverCommandParams { swap_id }, do_not_await_finality, }) => Arguments { testnet: is_testnet, @@ -65,36 +65,36 @@ where env_config: env_config(is_testnet), cmd: Command::Redeem { swap_id, - force, + do_not_await_finality, }, }, RawCommand::ManualRecovery(ManualRecovery::Cancel { - cancel_params: RecoverCommandParams { swap_id, force }, + cancel_params: RecoverCommandParams { swap_id }, }) => Arguments { testnet: is_testnet, json: is_json, config_path: config_path(config, is_testnet)?, env_config: env_config(is_testnet), - cmd: Command::Cancel { swap_id, force }, + cmd: Command::Cancel { swap_id }, }, RawCommand::ManualRecovery(ManualRecovery::Refund { - refund_params: RecoverCommandParams { swap_id, force }, + refund_params: RecoverCommandParams { swap_id }, }) => Arguments { testnet: is_testnet, json: is_json, config_path: config_path(config, is_testnet)?, env_config: env_config(is_testnet), - cmd: Command::Refund { swap_id, force }, + cmd: Command::Refund { swap_id }, }, RawCommand::ManualRecovery(ManualRecovery::Punish { - punish_params: RecoverCommandParams { swap_id, force }, + punish_params: RecoverCommandParams { swap_id }, }) => Arguments { testnet: is_testnet, json: is_json, config_path: config_path(config, is_testnet)?, env_config: env_config(is_testnet), - cmd: Command::Punish { swap_id, force }, + cmd: Command::Punish { swap_id }, }, RawCommand::ManualRecovery(ManualRecovery::SafelyAbort { swap_id }) => Arguments { testnet: is_testnet, @@ -176,20 +176,16 @@ pub enum Command { Balance, Redeem { swap_id: Uuid, - force: bool, do_not_await_finality: bool, }, Cancel { swap_id: Uuid, - force: bool, }, Refund { swap_id: Uuid, - force: bool, }, Punish { swap_id: Uuid, - force: bool, }, SafelyAbort { swap_id: Uuid, @@ -309,13 +305,6 @@ pub struct RecoverCommandParams { help = "The swap id can be retrieved using the history subcommand" )] pub swap_id: Uuid, - - #[structopt( - short, - long, - help = "Circumvents certain checks when recovering. It is recommended to run a recovery command without --force first to see what is returned." - )] - pub force: bool, } #[cfg(test)] @@ -399,7 +388,6 @@ mod tests { env_config: mainnet_env_config, cmd: Command::Cancel { swap_id: Uuid::parse_str(SWAP_ID).unwrap(), - force: false, }, }; let args = parse_args(raw_ars).unwrap(); @@ -419,7 +407,6 @@ mod tests { env_config: mainnet_env_config, cmd: Command::Refund { swap_id: Uuid::parse_str(SWAP_ID).unwrap(), - force: false, }, }; let args = parse_args(raw_ars).unwrap(); @@ -439,7 +426,6 @@ mod tests { env_config: mainnet_env_config, cmd: Command::Punish { swap_id: Uuid::parse_str(SWAP_ID).unwrap(), - force: false, }, }; let args = parse_args(raw_ars).unwrap(); @@ -538,7 +524,6 @@ mod tests { env_config: testnet_env_config, cmd: Command::Cancel { swap_id: Uuid::parse_str(SWAP_ID).unwrap(), - force: false, }, }; let args = parse_args(raw_ars).unwrap(); @@ -559,7 +544,6 @@ mod tests { env_config: testnet_env_config, cmd: Command::Refund { swap_id: Uuid::parse_str(SWAP_ID).unwrap(), - force: false, }, }; let args = parse_args(raw_ars).unwrap(); @@ -580,7 +564,6 @@ mod tests { env_config: testnet_env_config, cmd: Command::Punish { swap_id: Uuid::parse_str(SWAP_ID).unwrap(), - force: false, }, }; let args = parse_args(raw_ars).unwrap(); diff --git a/swap/src/asb/recovery/cancel.rs b/swap/src/asb/recovery/cancel.rs index c3168026..b02dd22b 100644 --- a/swap/src/asb/recovery/cancel.rs +++ b/swap/src/asb/recovery/cancel.rs @@ -1,22 +1,15 @@ -use crate::bitcoin::{ExpiredTimelocks, Txid, Wallet}; +use crate::bitcoin::{Txid, Wallet}; use crate::database::{Database, Swap}; use crate::protocol::alice::AliceState; use anyhow::{bail, Result}; use std::sync::Arc; use uuid::Uuid; -#[derive(Debug, thiserror::Error, Clone, Copy)] -pub enum Error { - #[error("The cancel transaction cannot be published because the cancel timelock has not expired yet. Please try again later")] - CancelTimelockNotExpiredYet, -} - pub async fn cancel( swap_id: Uuid, bitcoin_wallet: Arc, db: Arc, - force: bool, -) -> Result> { +) -> Result<(Txid, AliceState)> { let state = db.get_state(swap_id)?.try_into_alice()?.into(); let (monero_wallet_restore_blockheight, transfer_proof, state3) = match state { @@ -31,18 +24,16 @@ pub async fn cancel( | AliceState::XmrLockTransferProofSent { monero_wallet_restore_blockheight, transfer_proof, state3 } // in cancel mode we do not care about the fact that we could redeem, but always wait for cancellation (leading either refund or punish) | AliceState::EncSigLearned { monero_wallet_restore_blockheight, transfer_proof, state3, .. } - | AliceState::CancelTimelockExpired { monero_wallet_restore_blockheight, transfer_proof, state3} => { + | AliceState::CancelTimelockExpired { monero_wallet_restore_blockheight, transfer_proof, state3} + | AliceState::BtcCancelled { monero_wallet_restore_blockheight, transfer_proof, state3 } + | AliceState::BtcRefunded { monero_wallet_restore_blockheight, transfer_proof, state3 ,.. } + | AliceState::BtcPunishable { monero_wallet_restore_blockheight, transfer_proof, state3 } => { (monero_wallet_restore_blockheight, transfer_proof, state3) } // The redeem transaction was already published, it is not safe to cancel anymore AliceState::BtcRedeemTransactionPublished { .. } => bail!(" The redeem transaction was already published, it is not safe to cancel anymore"), - // The cancel tx was already published, but Alice not yet in final state - AliceState::BtcCancelled { .. } - | AliceState::BtcRefunded { .. } - | AliceState::BtcPunishable { .. } - // Alice already in final state | AliceState::BtcRedeemed | AliceState::XmrRefunded @@ -50,22 +41,14 @@ pub async fn cancel( | AliceState::SafelyAborted => bail!("Swap is is in state {} which is not cancelable", state), }; - tracing::info!(%swap_id, "Trying to manually cancel swap"); - - if !force { - tracing::debug!(%swap_id, "Checking if cancel timelock is expired"); - - if let ExpiredTimelocks::None = state3.expired_timelocks(bitcoin_wallet.as_ref()).await? { - return Ok(Err(Error::CancelTimelockNotExpiredYet)); + let txid = match state3.submit_tx_cancel(bitcoin_wallet.as_ref()).await { + Ok(txid) => txid, + Err(err) => { + if let Some(bdk::Error::TransactionConfirmed) = err.downcast_ref::() { + tracing::info!("Cancel transaction has already been published and confirmed") + }; + bail!(err); } - } - - let txid = if let Ok(tx) = state3.check_for_tx_cancel(bitcoin_wallet.as_ref()).await { - let txid = tx.txid(); - tracing::debug!(%swap_id, "Cancel transaction has already been published: {}", txid); - txid - } else { - state3.submit_tx_cancel(bitcoin_wallet.as_ref()).await? }; let state = AliceState::BtcCancelled { @@ -77,5 +60,5 @@ pub async fn cancel( db.insert_latest_state(swap_id, Swap::Alice(db_state)) .await?; - Ok(Ok((txid, state))) + Ok((txid, state)) } diff --git a/swap/src/asb/recovery/punish.rs b/swap/src/asb/recovery/punish.rs index 965fdba3..ddd27c8f 100644 --- a/swap/src/asb/recovery/punish.rs +++ b/swap/src/asb/recovery/punish.rs @@ -1,4 +1,4 @@ -use crate::bitcoin::{self, ExpiredTimelocks, Txid}; +use crate::bitcoin::{self, Txid}; use crate::database::{Database, Swap}; use crate::protocol::alice::AliceState; use anyhow::{bail, Result}; @@ -7,16 +7,6 @@ use uuid::Uuid; #[derive(Debug, thiserror::Error)] pub enum Error { - // Errors indicating the swap can *currently* not be punished but might be later - #[error("Swap is not in a cancelled state Make sure to cancel the swap before trying to punish or use --force.")] - SwapNotCancelled, - #[error("The punish transaction cannot be published because the punish timelock has not expired yet. Please try again later")] - PunishTimelockNotExpiredYet, - - // Errors indicating that the swap cannot be refunded because it is in a abort/final state - // state - #[error("Cannot punish swap because it is in state {0} where no BTC was locked. Try aborting instead.")] - NoBtcLocked(AliceState), #[error("Cannot punish swap because it is in state {0} which is not punishable")] SwapNotPunishable(AliceState), } @@ -25,70 +15,34 @@ pub async fn punish( swap_id: Uuid, bitcoin_wallet: Arc, db: Arc, - force: bool, -) -> Result> { +) -> Result<(Txid, AliceState)> { let state = db.get_state(swap_id)?.try_into_alice()?.into(); - let state3 = if force { - match state { - - // In case no XMR has been locked, move to Safely Aborted - AliceState::Started { .. } => bail!(Error::NoBtcLocked(state)), - - // Punish potentially possible (no knowledge of cancel transaction) - AliceState::BtcLockTransactionSeen { state3 } - | AliceState::BtcLocked { state3, .. } - | AliceState::XmrLockTransactionSent {state3, ..} - | AliceState::XmrLocked {state3, ..} - | AliceState::XmrLockTransferProofSent {state3, ..} - | AliceState::EncSigLearned {state3, ..} - | AliceState::CancelTimelockExpired {state3, ..} - - // Punish possible due to cancel transaction already being published - | AliceState::BtcCancelled {state3, ..} - | AliceState::BtcPunishable {state3, ..} => { - state3 - } - - // If the swap was refunded it cannot be punished - AliceState::BtcRedeemTransactionPublished { .. } - | AliceState::BtcRefunded {..} - // Alice already in final state - | AliceState::BtcRedeemed - | AliceState::XmrRefunded - | AliceState::BtcPunished - | AliceState::SafelyAborted => bail!(Error::SwapNotPunishable(state)), - } - } else { - match state { - AliceState::Started { .. } => { - bail!(Error::NoBtcLocked(state)) - } - - AliceState::BtcCancelled { state3, .. } | AliceState::BtcPunishable { state3, .. } => { - state3 - } - - AliceState::BtcRefunded { .. } - | AliceState::BtcRedeemed - | AliceState::XmrRefunded - | AliceState::BtcPunished - | AliceState::SafelyAborted => bail!(Error::SwapNotPunishable(state)), - - _ => return Ok(Err(Error::SwapNotCancelled)), - } + let state3 = match state { + // Punish potentially possible (no knowledge of cancel transaction) + AliceState::BtcLockTransactionSeen { state3 } + | AliceState::BtcLocked { state3, .. } + | AliceState::XmrLockTransactionSent {state3, ..} + | AliceState::XmrLocked {state3, ..} + | AliceState::XmrLockTransferProofSent {state3, ..} + | AliceState::EncSigLearned {state3, ..} + | AliceState::CancelTimelockExpired {state3, ..} + // Punish possible due to cancel transaction already being published + | AliceState::BtcCancelled {state3, ..} + | AliceState::BtcPunishable {state3, ..} => { state3 } + // The state machine is in a state where punish is theoretically impossible but we try and punish anyway as this is what the user wants + AliceState::BtcRedeemTransactionPublished { state3 } + | AliceState::BtcRefunded { state3,.. } + | AliceState::Started { state3 } => { state3 } + // Alice already in final state + | AliceState::BtcRedeemed + | AliceState::XmrRefunded + | AliceState::BtcPunished + | AliceState::SafelyAborted => bail!(Error::SwapNotPunishable(state)), }; tracing::info!(%swap_id, "Trying to manually punish swap"); - if !force { - tracing::debug!(%swap_id, "Checking if punish timelock is expired"); - - if let ExpiredTimelocks::Cancel = state3.expired_timelocks(bitcoin_wallet.as_ref()).await? { - return Ok(Err(Error::PunishTimelockNotExpiredYet)); - } - } - let txid = state3.punish_btc(&bitcoin_wallet).await?; let state = AliceState::BtcPunished; @@ -96,5 +50,5 @@ pub async fn punish( db.insert_latest_state(swap_id, Swap::Alice(db_state)) .await?; - Ok(Ok((txid, state))) + Ok((txid, state)) } diff --git a/swap/src/asb/recovery/redeem.rs b/swap/src/asb/recovery/redeem.rs index 8344db7b..dd8daa54 100644 --- a/swap/src/asb/recovery/redeem.rs +++ b/swap/src/asb/recovery/redeem.rs @@ -1,4 +1,4 @@ -use crate::bitcoin::{ExpiredTimelocks, Txid, Wallet}; +use crate::bitcoin::{Txid, Wallet}; use crate::database::{Database, Swap}; use crate::protocol::alice::AliceState; use anyhow::{bail, Result}; @@ -24,7 +24,6 @@ pub async fn redeem( swap_id: Uuid, bitcoin_wallet: Arc, db: Arc, - force: bool, finality: Finality, ) -> Result<(Txid, AliceState)> { let state = db.get_state(swap_id)?.try_into_alice()?.into(); @@ -37,16 +36,6 @@ pub async fn redeem( } => { tracing::info!(%swap_id, "Trying to redeem swap"); - if !force { - tracing::debug!(%swap_id, "Checking if timelocks have expired"); - - let expired_timelocks = state3.expired_timelocks(bitcoin_wallet.as_ref()).await?; - match expired_timelocks { - ExpiredTimelocks::None => (), - _ => bail!("{:?} timelock already expired, consider using refund or punish. You can use --force to publish the redeem transaction, but be aware that it is not safe to do so anymore!", expired_timelocks) - } - } - let redeem_tx = state3.signed_redeem_transaction(*encrypted_signature)?; let (txid, subscription) = bitcoin_wallet.broadcast(redeem_tx, "redeem").await?; diff --git a/swap/src/asb/recovery/refund.rs b/swap/src/asb/recovery/refund.rs index 3297a454..1e91b49a 100644 --- a/swap/src/asb/recovery/refund.rs +++ b/swap/src/asb/recovery/refund.rs @@ -9,9 +9,6 @@ use uuid::Uuid; #[derive(Debug, thiserror::Error)] pub enum Error { - // Errors indicating the swap can *currently* not be refunded but might be later - #[error("Swap is not in a cancelled state. Make sure to cancel the swap before trying to refund or use --force.")] - SwapNotCancelled, #[error( "Counterparty {0} did not refund the BTC yet. You can try again later or try to punish." )] @@ -30,70 +27,35 @@ pub async fn refund( bitcoin_wallet: Arc, monero_wallet: Arc, db: Arc, - force: bool, -) -> Result> { +) -> Result { let state = db.get_state(swap_id)?.try_into_alice()?.into(); - let (monero_wallet_restore_blockheight, transfer_proof, state3) = if force { - match state { - - // In case no XMR has been locked, move to Safely Aborted - AliceState::Started { .. } - | AliceState::BtcLockTransactionSeen { .. } - | AliceState::BtcLocked { .. } => bail!(Error::NoXmrLocked(state)), - - // Refund potentially possible (no knowledge of cancel transaction) - AliceState::XmrLockTransactionSent { monero_wallet_restore_blockheight, transfer_proof, state3, } - | AliceState::XmrLocked { monero_wallet_restore_blockheight, transfer_proof, state3 } - | AliceState::XmrLockTransferProofSent { monero_wallet_restore_blockheight, transfer_proof, state3 } - | AliceState::EncSigLearned { monero_wallet_restore_blockheight, transfer_proof, state3, .. } - | AliceState::CancelTimelockExpired { monero_wallet_restore_blockheight, transfer_proof, state3 } - - // Refund possible due to cancel transaction already being published - | AliceState::BtcCancelled { monero_wallet_restore_blockheight, transfer_proof, state3 } - | AliceState::BtcRefunded { monero_wallet_restore_blockheight, transfer_proof, state3, .. } - | AliceState::BtcPunishable { monero_wallet_restore_blockheight, transfer_proof, state3, .. } => { - (monero_wallet_restore_blockheight, transfer_proof, state3) - } - - // Alice already in final state - AliceState::BtcRedeemTransactionPublished { .. } - | AliceState::BtcRedeemed - | AliceState::XmrRefunded - | AliceState::BtcPunished - | AliceState::SafelyAborted => bail!(Error::SwapNotRefundable(state)), + let (monero_wallet_restore_blockheight, transfer_proof, state3) = match state { + // In case no XMR has been locked, move to Safely Aborted + AliceState::Started { .. } + | AliceState::BtcLockTransactionSeen { .. } + | AliceState::BtcLocked { .. } => bail!(Error::NoXmrLocked(state)), + + // Refund potentially possible (no knowledge of cancel transaction) + AliceState::XmrLockTransactionSent { monero_wallet_restore_blockheight, transfer_proof, state3, } + | AliceState::XmrLocked { monero_wallet_restore_blockheight, transfer_proof, state3 } + | AliceState::XmrLockTransferProofSent { monero_wallet_restore_blockheight, transfer_proof, state3 } + | AliceState::EncSigLearned { monero_wallet_restore_blockheight, transfer_proof, state3, .. } + | AliceState::CancelTimelockExpired { monero_wallet_restore_blockheight, transfer_proof, state3 } + + // Refund possible due to cancel transaction already being published + | AliceState::BtcCancelled { monero_wallet_restore_blockheight, transfer_proof, state3 } + | AliceState::BtcRefunded { monero_wallet_restore_blockheight, transfer_proof, state3, .. } + | AliceState::BtcPunishable { monero_wallet_restore_blockheight, transfer_proof, state3, .. } => { + (monero_wallet_restore_blockheight, transfer_proof, state3) } - } else { - match state { - AliceState::Started { .. } | AliceState::BtcLocked { .. } => { - bail!(Error::NoXmrLocked(state)) - } - - AliceState::BtcCancelled { - monero_wallet_restore_blockheight, - transfer_proof, - state3, - } - | AliceState::BtcRefunded { - monero_wallet_restore_blockheight, - transfer_proof, - state3, - .. - } - | AliceState::BtcPunishable { - monero_wallet_restore_blockheight, - transfer_proof, - state3, - .. - } => (monero_wallet_restore_blockheight, transfer_proof, state3), - AliceState::BtcRedeemed - | AliceState::XmrRefunded - | AliceState::BtcPunished - | AliceState::SafelyAborted => bail!(Error::SwapNotRefundable(state)), - - _ => return Ok(Err(Error::SwapNotCancelled)), - } + // Alice already in final state + AliceState::BtcRedeemTransactionPublished { .. } + | AliceState::BtcRedeemed + | AliceState::XmrRefunded + | AliceState::BtcPunished + | AliceState::SafelyAborted => bail!(Error::SwapNotRefundable(state)), }; tracing::info!(%swap_id, "Trying to manually refund swap"); @@ -105,7 +67,7 @@ pub async fn refund( state3.extract_monero_private_key(published_refund_tx)? } else { let bob_peer_id = db.get_peer_id(swap_id)?; - return Ok(Err(Error::RefundTransactionNotPublishedYet(bob_peer_id))); + bail!(Error::RefundTransactionNotPublishedYet(bob_peer_id),); }; state3 @@ -123,5 +85,5 @@ pub async fn refund( db.insert_latest_state(swap_id, Swap::Alice(db_state)) .await?; - Ok(Ok(state)) + Ok(state) } diff --git a/swap/src/bin/asb.rs b/swap/src/bin/asb.rs index 08c81310..d8583aeb 100644 --- a/swap/src/bin/asb.rs +++ b/swap/src/bin/asb.rs @@ -244,15 +244,14 @@ async fn main() -> Result<()> { %monero_balance, "Current balance"); } - Command::Cancel { swap_id, force } => { + Command::Cancel { swap_id } => { let bitcoin_wallet = init_bitcoin_wallet(&config, &seed, env_config).await?; - let (txid, _) = - cancel(swap_id, Arc::new(bitcoin_wallet), Arc::new(db), force).await??; + let (txid, _) = cancel(swap_id, Arc::new(bitcoin_wallet), Arc::new(db)).await?; tracing::info!("Cancel transaction successfully published with id {}", txid); } - Command::Refund { swap_id, force } => { + Command::Refund { swap_id } => { let bitcoin_wallet = init_bitcoin_wallet(&config, &seed, env_config).await?; let monero_wallet = init_monero_wallet(&config, env_config).await?; @@ -261,17 +260,15 @@ async fn main() -> Result<()> { Arc::new(bitcoin_wallet), Arc::new(monero_wallet), Arc::new(db), - force, ) - .await??; + .await?; tracing::info!("Monero successfully refunded"); } - Command::Punish { swap_id, force } => { + Command::Punish { swap_id } => { let bitcoin_wallet = init_bitcoin_wallet(&config, &seed, env_config).await?; - let (txid, _) = - punish(swap_id, Arc::new(bitcoin_wallet), Arc::new(db), force).await??; + let (txid, _) = punish(swap_id, Arc::new(bitcoin_wallet), Arc::new(db)).await?; tracing::info!("Punish transaction successfully published with id {}", txid); } @@ -282,7 +279,6 @@ async fn main() -> Result<()> { } Command::Redeem { swap_id, - force, do_not_await_finality, } => { let bitcoin_wallet = init_bitcoin_wallet(&config, &seed, env_config).await?; @@ -291,7 +287,6 @@ async fn main() -> Result<()> { swap_id, Arc::new(bitcoin_wallet), Arc::new(db), - force, Finality::from_bool(do_not_await_finality), ) .await?; diff --git a/swap/src/bin/swap.rs b/swap/src/bin/swap.rs index b3f6fb05..5d2a1921 100644 --- a/swap/src/bin/swap.rs +++ b/swap/src/bin/swap.rs @@ -273,7 +273,6 @@ async fn main() -> Result<()> { } Command::Cancel { swap_id, - force, bitcoin_electrum_rpc_url, bitcoin_target_block, } => { @@ -292,20 +291,11 @@ async fn main() -> Result<()> { ) .await?; - let cancel = cli::cancel(swap_id, Arc::new(bitcoin_wallet), db, force).await?; - - match cancel { - Ok((txid, _)) => { - tracing::debug!("Cancel transaction successfully published with id {}", txid) - } - Err(cli::cancel::Error::CancelTimelockNotExpiredYet) => tracing::error!( - "The cancel transaction cannot be published yet, because the timelock has not expired. Please try again later" - ), - } + let (txid, _) = cli::cancel(swap_id, Arc::new(bitcoin_wallet), db).await?; + tracing::debug!("Cancel transaction successfully published with id {}", txid); } Command::Refund { swap_id, - force, bitcoin_electrum_rpc_url, bitcoin_target_block, } => { @@ -324,7 +314,7 @@ async fn main() -> Result<()> { ) .await?; - cli::refund(swap_id, Arc::new(bitcoin_wallet), db, force).await??; + cli::refund(swap_id, Arc::new(bitcoin_wallet), db).await?; } Command::ListSellers { rendezvous_point, diff --git a/swap/src/cli/cancel.rs b/swap/src/cli/cancel.rs index 7723067c..3693917f 100644 --- a/swap/src/cli/cancel.rs +++ b/swap/src/cli/cancel.rs @@ -1,22 +1,15 @@ -use crate::bitcoin::{ExpiredTimelocks, Txid, Wallet}; +use crate::bitcoin::{Txid, Wallet}; use crate::database::{Database, Swap}; use crate::protocol::bob::BobState; use anyhow::{bail, Result}; use std::sync::Arc; use uuid::Uuid; -#[derive(Debug, thiserror::Error, Clone, Copy)] -pub enum Error { - #[error("The cancel timelock has not expired yet.")] - CancelTimelockNotExpiredYet, -} - pub async fn cancel( swap_id: Uuid, bitcoin_wallet: Arc, db: Database, - force: bool, -) -> Result> { +) -> Result<(Txid, BobState)> { let state = db.get_state(swap_id)?.try_into_bob()?.into(); let state6 = match state { @@ -25,11 +18,12 @@ pub async fn cancel( BobState::XmrLocked(state4) => state4.cancel(), BobState::EncSigSent(state4) => state4.cancel(), BobState::CancelTimelockExpired(state6) => state6, + BobState::BtcRefunded(state6) => state6, + BobState::BtcCancelled(state6) => state6, + BobState::Started { .. } | BobState::SwapSetupCompleted(_) | BobState::BtcRedeemed(_) - | BobState::BtcCancelled(_) - | BobState::BtcRefunded(_) | BobState::XmrRedeemed { .. } | BobState::BtcPunished { .. } | BobState::SafelyAborted => bail!( @@ -41,25 +35,19 @@ pub async fn cancel( tracing::info!(%swap_id, "Manually cancelling swap"); - if !force { - tracing::debug!(%swap_id, "Checking if cancel timelock is expired"); - - if let ExpiredTimelocks::None = state6.expired_timelock(bitcoin_wallet.as_ref()).await? { - return Ok(Err(Error::CancelTimelockNotExpiredYet)); + let txid = match state6.submit_tx_cancel(bitcoin_wallet.as_ref()).await { + Ok(txid) => txid, + Err(err) => { + if let Some(bdk::Error::TransactionConfirmed) = err.downcast_ref::() { + tracing::info!("Cancel transaction has already been published and confirmed") + }; + bail!(err); } - } - - let txid = if let Ok(tx) = state6.check_for_tx_cancel(bitcoin_wallet.as_ref()).await { - tracing::debug!(%swap_id, "Cancel transaction has already been published"); - - tx.txid() - } else { - state6.submit_tx_cancel(bitcoin_wallet.as_ref()).await? }; let state = BobState::BtcCancelled(state6); let db_state = state.clone().into(); db.insert_latest_state(swap_id, Swap::Bob(db_state)).await?; - Ok(Ok((txid, state))) + Ok((txid, state)) } diff --git a/swap/src/cli/command.rs b/swap/src/cli/command.rs index 72918b88..43aff95c 100644 --- a/swap/src/cli/command.rs +++ b/swap/src/cli/command.rs @@ -171,7 +171,6 @@ where } RawCommand::Cancel { swap_id: SwapId { swap_id }, - force, bitcoin, } => { let (bitcoin_electrum_rpc_url, bitcoin_target_block) = @@ -184,7 +183,6 @@ where data_dir: data::data_dir_from(data, is_testnet)?, cmd: Command::Cancel { swap_id, - force, bitcoin_electrum_rpc_url, bitcoin_target_block, }, @@ -192,7 +190,6 @@ where } RawCommand::Refund { swap_id: SwapId { swap_id }, - force, bitcoin, } => { let (bitcoin_electrum_rpc_url, bitcoin_target_block) = @@ -205,7 +202,6 @@ where data_dir: data::data_dir_from(data, is_testnet)?, cmd: Command::Refund { swap_id, - force, bitcoin_electrum_rpc_url, bitcoin_target_block, }, @@ -261,13 +257,11 @@ pub enum Command { }, Cancel { swap_id: Uuid, - force: bool, bitcoin_electrum_rpc_url: Url, bitcoin_target_block: usize, }, Refund { swap_id: Uuid, - force: bool, bitcoin_electrum_rpc_url: Url, bitcoin_target_block: usize, }, @@ -376,25 +370,21 @@ enum RawCommand { #[structopt(flatten)] tor: Tor, }, - /// Try to cancel an ongoing swap (expert users only) + /// Force submission of the cancel transaction overriding the protocol state + /// machine and blockheight checks (expert users only) Cancel { #[structopt(flatten)] swap_id: SwapId, - #[structopt(short, long)] - force: bool, - #[structopt(flatten)] bitcoin: Bitcoin, }, - /// Try to cancel a swap and refund the BTC (expert users only) + /// Force submission of the refund transaction overriding the protocol state + /// machine and blockheight checks (expert users only) Refund { #[structopt(flatten)] swap_id: SwapId, - #[structopt(short, long)] - force: bool, - #[structopt(flatten)] bitcoin: Bitcoin, }, @@ -1190,7 +1180,6 @@ mod tests { data_dir: data_dir_path_cli().join(TESTNET), cmd: Command::Cancel { swap_id: Uuid::from_str(SWAP_ID).unwrap(), - force: false, bitcoin_electrum_rpc_url: Url::from_str(DEFAULT_ELECTRUM_RPC_URL_TESTNET) .unwrap(), bitcoin_target_block: DEFAULT_BITCOIN_CONFIRMATION_TARGET_TESTNET, @@ -1206,7 +1195,6 @@ mod tests { data_dir: data_dir_path_cli().join(MAINNET), cmd: Command::Cancel { swap_id: Uuid::from_str(SWAP_ID).unwrap(), - force: false, bitcoin_electrum_rpc_url: Url::from_str(DEFAULT_ELECTRUM_RPC_URL).unwrap(), bitcoin_target_block: DEFAULT_BITCOIN_CONFIRMATION_TARGET, }, @@ -1221,7 +1209,6 @@ mod tests { data_dir: data_dir_path_cli().join(TESTNET), cmd: Command::Refund { swap_id: Uuid::from_str(SWAP_ID).unwrap(), - force: false, bitcoin_electrum_rpc_url: Url::from_str(DEFAULT_ELECTRUM_RPC_URL_TESTNET) .unwrap(), bitcoin_target_block: DEFAULT_BITCOIN_CONFIRMATION_TARGET_TESTNET, @@ -1237,7 +1224,6 @@ mod tests { data_dir: data_dir_path_cli().join(MAINNET), cmd: Command::Refund { swap_id: Uuid::from_str(SWAP_ID).unwrap(), - force: false, bitcoin_electrum_rpc_url: Url::from_str(DEFAULT_ELECTRUM_RPC_URL).unwrap(), bitcoin_target_block: DEFAULT_BITCOIN_CONFIRMATION_TARGET, }, diff --git a/swap/src/cli/refund.rs b/swap/src/cli/refund.rs index a769eda9..f94b9430 100644 --- a/swap/src/cli/refund.rs +++ b/swap/src/cli/refund.rs @@ -5,45 +5,27 @@ use anyhow::{bail, Result}; use std::sync::Arc; use uuid::Uuid; -#[derive(thiserror::Error, Debug, Clone, Copy)] -#[error("Cannot refund because swap {0} was not cancelled yet. Make sure to cancel the swap before trying to refund.")] -pub struct SwapNotCancelledYet(pub Uuid); - -pub async fn refund( - swap_id: Uuid, - bitcoin_wallet: Arc, - db: Database, - force: bool, -) -> Result> { +pub async fn refund(swap_id: Uuid, bitcoin_wallet: Arc, db: Database) -> Result { let state = db.get_state(swap_id)?.try_into_bob()?.into(); - let state6 = if force { - match state { - BobState::BtcLocked(state3) => state3.cancel(), - BobState::XmrLockProofReceived { state, .. } => state.cancel(), - BobState::XmrLocked(state4) => state4.cancel(), - BobState::EncSigSent(state4) => state4.cancel(), - BobState::CancelTimelockExpired(state6) => state6, - BobState::BtcCancelled(state6) => state6, - BobState::Started { .. } - | BobState::SwapSetupCompleted(_) - | BobState::BtcRedeemed(_) - | BobState::BtcRefunded(_) - | BobState::XmrRedeemed { .. } - | BobState::BtcPunished { .. } - | BobState::SafelyAborted => bail!( - "Cannot refund swap {} because it is in state {} which is not refundable.", - swap_id, - state - ), - } - } else { - match state { - BobState::BtcCancelled(state6) => state6, - _ => { - return Ok(Err(SwapNotCancelledYet(swap_id))); - } - } + let state6 = match state { + BobState::BtcLocked(state3) => state3.cancel(), + BobState::XmrLockProofReceived { state, .. } => state.cancel(), + BobState::XmrLocked(state4) => state4.cancel(), + BobState::EncSigSent(state4) => state4.cancel(), + BobState::CancelTimelockExpired(state6) => state6, + BobState::BtcCancelled(state6) => state6, + BobState::Started { .. } + | BobState::SwapSetupCompleted(_) + | BobState::BtcRedeemed(_) + | BobState::BtcRefunded(_) + | BobState::XmrRedeemed { .. } + | BobState::BtcPunished { .. } + | BobState::SafelyAborted => bail!( + "Cannot refund swap {} because it is in state {} which is not refundable.", + swap_id, + state + ), }; state6.publish_refund_btc(bitcoin_wallet.as_ref()).await?; @@ -53,5 +35,5 @@ pub async fn refund( db.insert_latest_state(swap_id, Swap::Bob(db_state)).await?; - Ok(Ok(state)) + Ok(state) } 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 94e12506..524ca771 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,8 +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, false).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 @@ -61,40 +60,20 @@ async fn given_alice_and_bob_manually_refund_after_funds_locked_both_refund() { // Bob manually refunds bob_join_handle.abort(); - let bob_state = - cli::refund(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db, false).await??; + let bob_state = cli::refund(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db).await?; ctx.assert_bob_refunded(bob_state).await; - // manually cancel ALice's swap (effectively just notice that Bob already - // cancelled and record that) - ctx.restart_alice().await; - let alice_swap = ctx.alice_next_swap().await; - assert!(matches!( - alice_swap.state, - AliceState::XmrLockTransactionSent { .. } - )); - - asb::cancel( - alice_swap.swap_id, - alice_swap.bitcoin_wallet, - alice_swap.db, - false, - ) - .await??; - // manually refund ALice's swap ctx.restart_alice().await; let alice_swap = ctx.alice_next_swap().await; - assert!(matches!(alice_swap.state, AliceState::BtcCancelled { .. })); let alice_state = asb::refund( alice_swap.swap_id, alice_swap.bitcoin_wallet, alice_swap.monero_wallet, alice_swap.db, - false, ) - .await??; + .await?; ctx.assert_alice_refunded(alice_state).await; 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 13dd1160..73eda852 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 @@ -38,13 +38,13 @@ async fn given_alice_and_bob_manually_cancel_when_timelock_not_expired_errors() )); // Bob tries but fails to manually cancel - let result = cli::cancel(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db, false) - .await? + let error = cli::cancel(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db) + .await .unwrap_err(); - assert!(matches!( - result, - cli::cancel::Error::CancelTimelockNotExpiredYet - )); + match error.downcast::().unwrap() { + bdk::Error::Electrum(bdk::electrum_client::Error::Protocol(..)) => (), + unexpected => panic!("Failed to cancel due to unexpected error: {}", unexpected), + } ctx.restart_alice().await; let alice_swap = ctx.alice_next_swap().await; @@ -54,18 +54,9 @@ async fn given_alice_and_bob_manually_cancel_when_timelock_not_expired_errors() )); // Alice tries but fails manual cancel - let result = asb::cancel( - alice_swap.swap_id, - alice_swap.bitcoin_wallet, - alice_swap.db, - false, - ) - .await? - .unwrap_err(); - assert!(matches!( - result, - asb::cancel::Error::CancelTimelockNotExpiredYet - )); + let result = + asb::cancel(alice_swap.swap_id, alice_swap.bitcoin_wallet, alice_swap.db).await; + assert!(result.is_err()); let (bob_swap, bob_join_handle) = ctx .stop_and_resume_bob_from_db(bob_join_handle, swap_id) @@ -73,10 +64,13 @@ async fn given_alice_and_bob_manually_cancel_when_timelock_not_expired_errors() assert!(matches!(bob_swap.state, BobState::BtcLocked { .. })); // Bob tries but fails to manually refund - let result = cli::refund(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db, false) - .await? + let error = cli::refund(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db) + .await .unwrap_err(); - assert!(matches!(result, cli::refund::SwapNotCancelledYet(_))); + match error.downcast::().unwrap() { + bdk::Error::Electrum(bdk::electrum_client::Error::Protocol(..)) => (), + unexpected => panic!("Failed to refund due to unexpected error: {}", unexpected), + } let (bob_swap, _) = ctx .stop_and_resume_bob_from_db(bob_join_handle, swap_id) @@ -96,11 +90,9 @@ async fn given_alice_and_bob_manually_cancel_when_timelock_not_expired_errors() alice_swap.bitcoin_wallet, alice_swap.monero_wallet, alice_swap.db, - false, ) - .await? - .unwrap_err(); - assert!(matches!(result, asb::refund::Error::SwapNotCancelled)); + .await; + assert!(result.is_err()); ctx.restart_alice().await; let alice_swap = ctx.alice_next_swap().await; diff --git a/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired_force.rs b/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired_force.rs deleted file mode 100644 index de16ec96..00000000 --- a/swap/tests/alice_and_bob_refund_using_cancel_and_refund_command_timelock_not_expired_force.rs +++ /dev/null @@ -1,110 +0,0 @@ -pub mod harness; - -use harness::alice_run_until::is_xmr_lock_transaction_sent; -use harness::bob_run_until::is_btc_locked; -use harness::SlowCancelConfig; -use swap::asb::FixedRate; -use swap::protocol::alice::AliceState; -use swap::protocol::bob::BobState; -use swap::protocol::{alice, bob}; -use swap::{asb, cli}; - -#[tokio::test] -async fn given_alice_and_bob_manually_force_cancel_when_timelock_not_expired_errors() { - harness::setup_test(SlowCancelConfig, |mut ctx| async move { - let (bob_swap, bob_join_handle) = ctx.bob_swap().await; - let 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_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 (bob_swap, bob_join_handle) = ctx - .stop_and_resume_bob_from_db(bob_join_handle, swap_id) - .await; - assert!(matches!(bob_swap.state, BobState::BtcLocked { .. })); - - let alice_state = alice_swap.await??; - assert!(matches!( - alice_state, - AliceState::XmrLockTransactionSent { .. } - )); - - // Bob tries but fails to manually cancel - let result = cli::cancel(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db, true).await; - assert!(matches!(result, Err(_))); - - ctx.restart_alice().await; - let alice_swap = ctx.alice_next_swap().await; - assert!(matches!( - alice_swap.state, - AliceState::XmrLockTransactionSent { .. } - )); - - // Alice tries but fails manual cancel - let is_outer_err = asb::cancel( - alice_swap.swap_id, - alice_swap.bitcoin_wallet, - alice_swap.db, - true, - ) - .await - .is_err(); - assert!(is_outer_err); - - let (bob_swap, bob_join_handle) = ctx - .stop_and_resume_bob_from_db(bob_join_handle, swap_id) - .await; - assert!(matches!(bob_swap.state, BobState::BtcLocked { .. })); - - // Bob tries but fails to manually refund - let is_outer_err = cli::refund(bob_swap.id, bob_swap.bitcoin_wallet, bob_swap.db, true) - .await - .is_err(); - assert!(is_outer_err); - - let (bob_swap, _) = ctx - .stop_and_resume_bob_from_db(bob_join_handle, swap_id) - .await; - assert!(matches!(bob_swap.state, BobState::BtcLocked { .. })); - - ctx.restart_alice().await; - let alice_swap = ctx.alice_next_swap().await; - assert!(matches!( - alice_swap.state, - AliceState::XmrLockTransactionSent { .. } - )); - - // Alice tries but fails manual cancel - let refund_tx_not_published_yet = asb::refund( - alice_swap.swap_id, - alice_swap.bitcoin_wallet, - alice_swap.monero_wallet, - alice_swap.db, - true, - ) - .await? - .unwrap_err(); - assert!(matches!( - refund_tx_not_published_yet, - asb::refund::Error::RefundTransactionNotPublishedYet(..) - )); - - ctx.restart_alice().await; - let alice_swap = ctx.alice_next_swap().await; - assert!(matches!( - alice_swap.state, - AliceState::XmrLockTransactionSent { .. } - )); - - Ok(()) - }) - .await; -} diff --git a/swap/tests/alice_manually_punishes_after_bob_dead.rs b/swap/tests/alice_manually_punishes_after_bob_dead.rs index 2cfa11ae..b27904ac 100644 --- a/swap/tests/alice_manually_punishes_after_bob_dead.rs +++ b/swap/tests/alice_manually_punishes_after_bob_dead.rs @@ -48,13 +48,8 @@ async fn alice_manually_punishes_after_bob_dead() { 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, - false, - ) - .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 { @@ -71,13 +66,8 @@ async fn alice_manually_punishes_after_bob_dead() { 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, - false, - ) - .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; // Restart Bob after Alice punished to ensure Bob transitions to diff --git a/swap/tests/alice_manually_redeems_after_enc_sig_learned.rs b/swap/tests/alice_manually_redeems_after_enc_sig_learned.rs index 0ece1b85..dd79fc2f 100644 --- a/swap/tests/alice_manually_redeems_after_enc_sig_learned.rs +++ b/swap/tests/alice_manually_redeems_after_enc_sig_learned.rs @@ -32,7 +32,6 @@ async fn alice_manually_redeems_after_enc_sig_learned() { alice_swap.swap_id, alice_swap.bitcoin_wallet, alice_swap.db, - false, Finality::Await, ) .await?;