From 6a75c840b542e7053f0dd82f8c1e6451b215d0c0 Mon Sep 17 00:00:00 2001 From: Daniel Karzel Date: Tue, 19 Jan 2021 14:44:54 +1100 Subject: [PATCH] Fix Alice redeem scenario - Properly check the timelocks before trying to redeem - Distinguish different failure scenarios and reactions to it. - if we fail to construct the redeem transaction: wait for cancel. - if we fail to publish the redeem transaction: wait for cancel but let the user know that restarting the application will result in retrying to publish the tx. - if we succeed to publish the tx but then fail when waiting for finality, print error to the user (secreat already leaked, the user has to check manually if the tx was included) --- swap/src/protocol/alice/steps.rs | 9 +- swap/src/protocol/alice/swap.rs | 96 +++++++++++--------- swap/tests/refund_restart_alice_cancelled.rs | 35 +++++++ 3 files changed, 89 insertions(+), 51 deletions(-) create mode 100644 swap/tests/refund_restart_alice_cancelled.rs diff --git a/swap/src/protocol/alice/steps.rs b/swap/src/protocol/alice/steps.rs index 09cf3cec..3a469424 100644 --- a/swap/src/protocol/alice/steps.rs +++ b/swap/src/protocol/alice/steps.rs @@ -191,19 +191,16 @@ pub fn build_bitcoin_redeem_transaction( pub async fn publish_bitcoin_redeem_transaction( redeem_tx: bitcoin::Transaction, bitcoin_wallet: Arc, - config: Config, -) -> Result<()> +) -> Result<::bitcoin::Txid> where W: BroadcastSignedTransaction + WaitForTransactionFinality, { info!("Attempting to publish bitcoin redeem txn"); - let tx_id = bitcoin_wallet + let txid = bitcoin_wallet .broadcast_signed_transaction(redeem_tx) .await?; - bitcoin_wallet - .wait_for_transaction_finality(tx_id, config) - .await + Ok(txid) } pub async fn publish_cancel_transaction( diff --git a/swap/src/protocol/alice/swap.rs b/swap/src/protocol/alice/swap.rs index 2ed2eb2f..c08cfcbd 100644 --- a/swap/src/protocol/alice/swap.rs +++ b/swap/src/protocol/alice/swap.rs @@ -1,6 +1,6 @@ //! Run an XMR/BTC swap in the role of Alice. //! Alice holds XMR and wishes receive BTC. -use anyhow::Result; +use anyhow::{bail, Result}; use async_recursion::async_recursion; use futures::{ future::{select, Either}, @@ -8,12 +8,12 @@ use futures::{ }; use rand::{CryptoRng, RngCore}; use std::sync::Arc; -use tracing::info; +use tracing::{error, info}; use uuid::Uuid; use crate::{ bitcoin, - bitcoin::{TransactionBlockHeight, WatchForRawTransaction}, + bitcoin::{TransactionBlockHeight, WaitForTransactionFinality, WatchForRawTransaction}, config::Config, database, database::Database, @@ -254,53 +254,59 @@ async fn run_until_internal( state3, encrypted_signature, } => { - // TODO: Evaluate if it is correct for Alice to Redeem no matter what. - // If cancel timelock expired she should potentially not try redeem. (The - // implementation gives her an advantage.) + let state = match state3.expired_timelocks(bitcoin_wallet.as_ref()).await? { + ExpiredTimelocks::None => { + match build_bitcoin_redeem_transaction( + encrypted_signature, + &state3.tx_lock, + state3.a.clone(), + state3.s_a, + state3.B, + &state3.redeem_address, + ) { + Ok(tx) => { + match publish_bitcoin_redeem_transaction(tx, bitcoin_wallet.clone()) + .await + { + Ok(txid) => { + let publishded_redeem_tx = bitcoin_wallet + .wait_for_transaction_finality(txid, config) + .await; - let signed_tx_redeem = match build_bitcoin_redeem_transaction( - encrypted_signature, - &state3.tx_lock, - state3.a.clone(), - state3.s_a, - state3.B, - &state3.redeem_address, - ) { - Ok(tx) => tx, - Err(_) => { - state3 - .wait_for_cancel_timelock_to_expire(bitcoin_wallet.as_ref()) - .await?; + match publishded_redeem_tx { + Ok(_) => { + AliceState::BtcRedeemed + } + Err(e) => { + bail!("Waiting for Bitcoin transaction finality failed with {}! The redeem transaction was published, but it is not ensured that the transaction was included! You're screwed.", e) + } + } + } + Err(e) => { + error!("Publishing the redeem transaction failed with {}, attempting to wait for cancellation now. If you restart the application before the timelock is expired publishing the redeem transaction will be retried.", e); + state3 + .wait_for_cancel_timelock_to_expire( + bitcoin_wallet.as_ref(), + ) + .await?; - let state = AliceState::CancelTimelockExpired { state3 }; - let db_state = (&state).into(); - db.insert_latest_state(swap_id, database::Swap::Alice(db_state)) - .await?; - return run_until_internal( - state, - is_target_state, - event_loop_handle, - bitcoin_wallet, - monero_wallet, - config, - swap_id, - db, - ) - .await; + AliceState::CancelTimelockExpired { state3 } + } + } + } + Err(e) => { + error!("Constructing the redeem transaction failed with {}, attempting to wait for cancellation now.", e); + state3 + .wait_for_cancel_timelock_to_expire(bitcoin_wallet.as_ref()) + .await?; + + AliceState::CancelTimelockExpired { state3 } + } + } } + _ => AliceState::CancelTimelockExpired { state3 }, }; - // TODO(Franck): Error handling is delicate here. - // If Bob sees this transaction he can redeem Monero - // e.g. If the Bitcoin node is down then the user needs to take action. - publish_bitcoin_redeem_transaction( - signed_tx_redeem, - bitcoin_wallet.clone(), - config, - ) - .await?; - - let state = AliceState::BtcRedeemed; let db_state = (&state).into(); db.insert_latest_state(swap_id, database::Swap::Alice(db_state)) .await?; diff --git a/swap/tests/refund_restart_alice_cancelled.rs b/swap/tests/refund_restart_alice_cancelled.rs new file mode 100644 index 00000000..5165aaed --- /dev/null +++ b/swap/tests/refund_restart_alice_cancelled.rs @@ -0,0 +1,35 @@ +use swap::protocol::{alice, alice::AliceState, bob}; + +pub mod testutils; + +/// Bob locks btc and Alice locks xmr. Alice fails to act so Bob refunds. Alice +/// is forced to refund even though she learned the secret and would be able to +/// redeem had the timelock not expired. +#[tokio::test] +async fn given_alice_restarts_after_enc_sig_learned_and_bob_already_cancelled_refund_swap() { + testutils::setup_test(|mut ctx| async move { + let alice_swap = ctx.new_swap_as_alice().await; + let bob_swap = ctx.new_swap_as_bob().await; + + let bob = bob::run(bob_swap); + let bob_handle = tokio::spawn(bob); + + let alice_state = alice::run_until(alice_swap, alice::swap::is_encsig_learned) + .await + .unwrap(); + assert!(matches!(alice_state, AliceState::EncSigLearned {..})); + + // Wait for Bob to refund, because Alice does not act + let bob_state = bob_handle.await.unwrap(); + ctx.assert_bob_refunded(bob_state.unwrap()).await; + + // Once bob has finished Alice is restarted and refunds as well + let alice_swap = ctx.recover_alice_from_db().await; + assert!(matches!(alice_swap.state, AliceState::EncSigLearned {..})); + + let alice_state = alice::run(alice_swap).await.unwrap(); + + ctx.assert_alice_refunded(alice_state).await; + }) + .await; +}