Merge pull request #148 from comit-network/properly-wait-for-tx-finality-for-assertion-on-bitcoin-balance

Wait for Bob's refund finality

swap/src/main.rs

@@ -135,11 +135,9 @@ async fn main() -> Result<()> {
                 Arc::new(monero_wallet),
                 alice_addr,
                 alice_peer_id,
+                config,
             );
-            let (swap, event_loop) = bob_factory
-                .with_init_params(swap_amounts, config)
-                .build()
-                .await?;
+            let (swap, event_loop) = bob_factory.with_init_params(swap_amounts).build().await?;
 
             tokio::spawn(async move { event_loop.run().await });
             bob::run(swap).await?;
@@ -212,6 +210,7 @@ async fn main() -> Result<()> {
                 Arc::new(monero_wallet),
                 alice_addr,
                 alice_peer_id,
+                config,
             );
             let (swap, event_loop) = bob_factory.build().await?;
 

swap/src/protocol/bob.rs

@@ -44,6 +44,7 @@ pub struct Swap {
     pub db: Database,
     pub bitcoin_wallet: Arc<bitcoin::Wallet>,
     pub monero_wallet: Arc<monero::Wallet>,
+    pub config: Config,
     pub swap_id: Uuid,
 }
 
@@ -60,17 +61,16 @@ pub struct Builder {
     monero_wallet: Arc<monero::Wallet>,
 
     init_params: InitParams,
+    config: Config,
 }
 
 enum InitParams {
     None,
-    New {
-        swap_amounts: SwapAmounts,
-        config: Config,
-    },
+    New { swap_amounts: SwapAmounts },
 }
 
 impl Builder {
+    #[allow(clippy::too_many_arguments)]
     pub fn new(
         seed: Seed,
         db_path: PathBuf,
@@ -79,6 +79,7 @@ impl Builder {
         monero_wallet: Arc<monero::Wallet>,
         alice_address: Multiaddr,
         alice_peer_id: PeerId,
+        config: Config,
     ) -> Self {
         let identity = network::Seed::new(seed).derive_libp2p_identity();
         let peer_id = identity.public().into_peer_id();
@@ -93,27 +94,22 @@ impl Builder {
             bitcoin_wallet,
             monero_wallet,
             init_params: InitParams::None,
+            config,
         }
     }
 
-    pub fn with_init_params(self, swap_amounts: SwapAmounts, config: Config) -> Self {
+    pub fn with_init_params(self, swap_amounts: SwapAmounts) -> Self {
         Self {
-            init_params: InitParams::New {
-                swap_amounts,
-                config,
-            },
+            init_params: InitParams::New { swap_amounts },
             ..self
         }
     }
 
     pub async fn build(self) -> Result<(bob::Swap, bob::EventLoop)> {
         match self.init_params {
-            InitParams::New {
-                swap_amounts,
-                config,
-            } => {
+            InitParams::New { swap_amounts } => {
                 let initial_state = self
-                    .make_initial_state(swap_amounts.btc, swap_amounts.xmr, config)
+                    .make_initial_state(swap_amounts.btc, swap_amounts.xmr, self.config)
                     .await?;
 
                 let (event_loop, event_loop_handle) = self.init_event_loop()?;
@@ -128,10 +124,12 @@ impl Builder {
                         bitcoin_wallet: self.bitcoin_wallet.clone(),
                         monero_wallet: self.monero_wallet.clone(),
                         swap_id: self.swap_id,
+                        config: self.config,
                     },
                     event_loop,
                 ))
             }
+
             InitParams::None => {
                 // reopen the existing database
                 let db = Database::open(self.db_path.as_path())?;
@@ -155,6 +153,7 @@ impl Builder {
                         bitcoin_wallet: self.bitcoin_wallet.clone(),
                         monero_wallet: self.monero_wallet.clone(),
                         swap_id: self.swap_id,
+                        config: self.config,
                     },
                     event_loop,
                 ))

swap/src/protocol/bob/state.rs

@@ -15,6 +15,7 @@ use crate::{
         BroadcastSignedTransaction, BuildTxLockPsbt, GetBlockHeight, GetNetwork, GetRawTransaction,
         Transaction, TransactionBlockHeight, TxCancel, Txid, WatchForRawTransaction,
     },
+    config::Config,
     monero,
     monero::monero_private_key,
     protocol::{alice, bob},
@@ -550,46 +551,34 @@ impl State4 {
         .await
     }
 
-    pub async fn refund_btc<W: bitcoin::BroadcastSignedTransaction>(
-        &self,
-        bitcoin_wallet: &W,
-    ) -> Result<()> {
+    pub async fn refund_btc<W>(&self, bitcoin_wallet: &W, config: Config) -> Result<()>
+    where
+        W: bitcoin::BroadcastSignedTransaction + bitcoin::WaitForTransactionFinality,
+    {
         let tx_cancel =
             bitcoin::TxCancel::new(&self.tx_lock, self.cancel_timelock, self.A, self.b.public());
         let tx_refund = bitcoin::TxRefund::new(&tx_cancel, &self.refund_address);
 
-        {
-            let sig_b = self.b.sign(tx_cancel.digest());
-            let sig_a = self.tx_cancel_sig_a.clone();
+        let adaptor = Adaptor::<Sha256, Deterministic<Sha256>>::default();
 
-            let signed_tx_cancel = tx_cancel.clone().add_signatures(
-                &self.tx_lock,
-                (self.A, sig_a),
-                (self.b.public(), sig_b),
-            )?;
+        let sig_b = self.b.sign(tx_refund.digest());
+        let sig_a =
+            adaptor.decrypt_signature(&self.s_b.into_secp256k1(), self.tx_refund_encsig.clone());
 
-            let _ = bitcoin_wallet
-                .broadcast_signed_transaction(signed_tx_cancel)
-                .await?;
-        }
+        let signed_tx_refund = tx_refund.add_signatures(
+            &tx_cancel.clone(),
+            (self.A, sig_a),
+            (self.b.public(), sig_b),
+        )?;
 
-        {
-            let adaptor = Adaptor::<Sha256, Deterministic<Sha256>>::default();
+        let txid = bitcoin_wallet
+            .broadcast_signed_transaction(signed_tx_refund)
+            .await?;
 
-            let sig_b = self.b.sign(tx_refund.digest());
-            let sig_a = adaptor
-                .decrypt_signature(&self.s_b.into_secp256k1(), self.tx_refund_encsig.clone());
+        bitcoin_wallet
+            .wait_for_transaction_finality(txid, config)
+            .await?;
 
-            let signed_tx_refund = tx_refund.add_signatures(
-                &tx_cancel.clone(),
-                (self.A, sig_a),
-                (self.b.public(), sig_b),
-            )?;
-
-            let _ = bitcoin_wallet
-                .broadcast_signed_transaction(signed_tx_refund)
-                .await?;
-        }
         Ok(())
     }
 

swap/src/protocol/bob/swap.rs

@@ -8,6 +8,7 @@ use uuid::Uuid;
 
 use crate::{
     bitcoin,
+    config::Config,
     database::{Database, Swap},
     monero,
     protocol::bob::{self, event_loop::EventLoopHandle, state::*},
@@ -54,6 +55,7 @@ pub async fn run_until(
         swap.monero_wallet,
         OsRng,
         swap.swap_id,
+        swap.config,
     )
     .await
 }
@@ -70,6 +72,7 @@ async fn run_until_internal<R>(
     monero_wallet: Arc<monero::Wallet>,
     mut rng: R,
     swap_id: Uuid,
+    config: Config,
 ) -> Result<BobState>
 where
     R: RngCore + CryptoRng + Send,
@@ -103,6 +106,7 @@ where
                     monero_wallet,
                     rng,
                     swap_id,
+                    config,
                 )
                 .await
             }
@@ -124,6 +128,7 @@ where
                     monero_wallet,
                     rng,
                     swap_id,
+                    config,
                 )
                 .await
             }
@@ -189,6 +194,7 @@ where
                     monero_wallet,
                     rng,
                     swap_id,
+                    config,
                 )
                 .await
             }
@@ -230,6 +236,7 @@ where
                     monero_wallet,
                     rng,
                     swap_id,
+                    config,
                 )
                 .await
             }
@@ -265,6 +272,7 @@ where
                     monero_wallet,
                     rng,
                     swap_id,
+                    config,
                 )
                 .await
             }
@@ -286,6 +294,7 @@ where
                     monero_wallet,
                     rng,
                     swap_id,
+                    config,
                 )
                 .await
             }
@@ -311,6 +320,7 @@ where
                     monero_wallet,
                     rng,
                     swap_id,
+                    config,
                 )
                 .await
             }
@@ -321,7 +331,7 @@ where
                         bail!("Internal error: canceled state reached before cancel timelock was expired");
                     }
                     ExpiredTimelocks::Cancel => {
-                        state.refund_btc(bitcoin_wallet.as_ref()).await?;
+                        state.refund_btc(bitcoin_wallet.as_ref(), config).await?;
                         BobState::BtcRefunded(state)
                     }
                     ExpiredTimelocks::Punish => BobState::BtcPunished {
@@ -340,6 +350,7 @@ where
                     monero_wallet,
                     rng,
                     swap_id,
+                    config,
                 )
                 .await
             }

swap/tests/refund_restart_alice.rs

@@ -5,7 +5,7 @@ pub mod testutils;
 /// Bob locks btc and Alice locks xmr. Alice fails to act so Bob refunds. Alice
 /// then also refunds.
 #[tokio::test]
-async fn given_alice_restarts_after_xmr_is_locked_abort_swap() {
+async fn given_alice_restarts_after_xmr_is_locked_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;
@@ -20,6 +20,7 @@ async fn given_alice_restarts_after_xmr_is_locked_abort_swap() {
 
         // Alice does not act, Bob refunds
         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;
@@ -27,11 +28,6 @@ async fn given_alice_restarts_after_xmr_is_locked_abort_swap() {
 
         let alice_state = alice::run(alice_swap).await.unwrap();
 
-        // TODO: The test passes like this, but the assertion should be done after Bob
-        // refunded, not at the end because this can cause side-effects!
-        //  We have to properly wait for the refund tx's finality inside the assertion,
-        // which requires storing the refund_tx_id in the the state!
-        ctx.assert_bob_refunded(bob_state.unwrap()).await;
         ctx.assert_alice_refunded(alice_state).await;
     })
     .await;

swap/tests/testutils/mod.rs

@@ -62,6 +62,7 @@ struct BobParams {
     monero_wallet: Arc<monero::Wallet>,
     alice_address: Multiaddr,
     alice_peer_id: PeerId,
+    config: Config,
 }
 
 impl BobParams {
@@ -74,6 +75,7 @@ impl BobParams {
             self.monero_wallet.clone(),
             self.alice_address.clone(),
             self.alice_peer_id.clone(),
+            self.config,
         )
     }
 }
@@ -112,7 +114,7 @@ impl TestContext {
         let (swap, event_loop) = self
             .bob_params
             .builder()
-            .with_init_params(self.swap_amounts, Config::regtest())
+            .with_init_params(self.swap_amounts)
             .build()
             .await
             .unwrap();
@@ -357,6 +359,7 @@ where
         monero_wallet: bob_monero_wallet.clone(),
         alice_address: alice_params.listen_address.clone(),
         alice_peer_id: alice_params.peer_id().await,
+        config,
     };
 
     let test = TestContext {