Merge #149

149: Fix Alice redeem scenario r=da-kami a=da-kami

Follow up of #144, partial fix of https://github.com/comit-network/xmr-btc-swap/issues/137

 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)



Co-authored-by: Daniel Karzel <daniel@comit.network>

swap/src/protocol/alice/steps.rs

@@ -186,19 +186,16 @@ pub fn build_bitcoin_redeem_transaction(
 pub async fn publish_bitcoin_redeem_transaction<W>(
     redeem_tx: bitcoin::Transaction,
     bitcoin_wallet: Arc<W>,
-    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<W>(

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 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?;
-
-                        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;
+                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;
+
+                                        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?;
+
+                                        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?;

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;
+}