Merge #881

881: Record the monero-wallet-restore blockcheight before locking BTC r=da-kami a=da-kami

This solves issues where the CLI went offline after sending the BTC transaction, and the monero wallet restore blockheight being recorded after Alice locked the Monero, resulting in the generated XMR redeem wallet not detecting the transaction and reporting `No unlocked balance in the specified account`.

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

CHANGELOG.md

@@ -17,6 +17,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   The swap has to be in a `BtcRedeemed` state.
   Use `--help` for more details.
 
+### Changed
+
+- Record monero wallet restore blockheight in state `SwapSetupCompleted` already.
+  This solves issues where the CLI went offline after sending the BTC transaction, and the monero wallet restore blockheight being recorded after Alice locked the Monero, resulting in the generated XMR redeem wallet not detecting the transaction and reporting `No unlocked balance in the specified account`.
+  This is a breaking database change!
+  Swaps that were saved prior to this change may fail to load if they are in state `SwapSetupCompleted` of `BtcLocked`.
+  Make sure to finish your swaps before upgrading.
+
 ## [0.10.0] - 2021-10-15
 
 ### Removed

swap/src/bin/swap.rs

@@ -446,7 +446,7 @@ async fn main() -> Result<()> {
             match swap_state {
                 BobState::Started { .. }
                 | BobState::SwapSetupCompleted(_)
-                | BobState::BtcLocked(_)
+                | BobState::BtcLocked { .. }
                 | BobState::XmrLockProofReceived { .. }
                 | BobState::XmrLocked(_)
                 | BobState::EncSigSent(_)

swap/src/cli/cancel.rs

@@ -14,7 +14,7 @@ pub async fn cancel(
     let state = db.get_state(swap_id).await?.try_into()?;
 
     let state6 = match state {
-        BobState::BtcLocked(state3) => state3.cancel(),
+        BobState::BtcLocked { state3, .. } => state3.cancel(),
         BobState::XmrLockProofReceived { state, .. } => state.cancel(),
         BobState::XmrLocked(state4) => state4.cancel(),
         BobState::EncSigSent(state4) => state4.cancel(),

swap/src/cli/refund.rs

@@ -14,7 +14,7 @@ pub async fn refund(
     let state = db.get_state(swap_id).await?.try_into()?;
 
     let state6 = match state {
-        BobState::BtcLocked(state3) => state3.cancel(),
+        BobState::BtcLocked { state3, .. } => state3.cancel(),
         BobState::XmrLockProofReceived { state, .. } => state.cancel(),
         BobState::XmrLocked(state4) => state4.cancel(),
         BobState::EncSigSent(state4) => state4.cancel(),

swap/src/database/bob.rs

@@ -20,6 +20,7 @@ pub enum Bob {
     },
     BtcLocked {
         state3: bob::State3,
+        monero_wallet_restore_blockheight: BlockHeight,
     },
     XmrLockProofReceived {
         state: bob::State3,
@@ -57,7 +58,13 @@ impl From<BobState> for Bob {
                 change_address,
             },
             BobState::SwapSetupCompleted(state2) => Bob::ExecutionSetupDone { state2 },
-            BobState::BtcLocked(state3) => Bob::BtcLocked { state3 },
+            BobState::BtcLocked {
+                state3,
+                monero_wallet_restore_blockheight,
+            } => Bob::BtcLocked {
+                state3,
+                monero_wallet_restore_blockheight,
+            },
             BobState::XmrLockProofReceived {
                 state,
                 lock_transfer_proof,
@@ -95,7 +102,13 @@ impl From<Bob> for BobState {
                 change_address,
             },
             Bob::ExecutionSetupDone { state2 } => BobState::SwapSetupCompleted(state2),
-            Bob::BtcLocked { state3 } => BobState::BtcLocked(state3),
+            Bob::BtcLocked {
+                state3,
+                monero_wallet_restore_blockheight,
+            } => BobState::BtcLocked {
+                state3,
+                monero_wallet_restore_blockheight,
+            },
             Bob::XmrLockProofReceived {
                 state,
                 lock_transfer_proof,

swap/src/protocol/bob/state.rs

@@ -28,7 +28,10 @@ pub enum BobState {
         change_address: bitcoin::Address,
     },
     SwapSetupCompleted(State2),
-    BtcLocked(State3),
+    BtcLocked {
+        state3: State3,
+        monero_wallet_restore_blockheight: BlockHeight,
+    },
     XmrLockProofReceived {
         state: State3,
         lock_transfer_proof: TransferProof,
@@ -54,7 +57,7 @@ impl fmt::Display for BobState {
         match self {
             BobState::Started { .. } => write!(f, "quote has been requested"),
             BobState::SwapSetupCompleted(..) => write!(f, "execution setup done"),
-            BobState::BtcLocked(..) => write!(f, "btc is locked"),
+            BobState::BtcLocked { .. } => write!(f, "btc is locked"),
             BobState::XmrLockProofReceived { .. } => {
                 write!(f, "XMR lock transaction transfer proof received")
             }

swap/src/protocol/bob/swap.rs

@@ -85,6 +85,17 @@ async fn next_state(
             BobState::SwapSetupCompleted(state2)
         }
         BobState::SwapSetupCompleted(state2) => {
+            // Record the current monero wallet block height so we don't have to scan from
+            // block 0 once we create the redeem wallet.
+            // This has to be done **before** the Bitcoin is locked in order to ensure that
+            // if Bob goes offline the recorded wallet-height is correct.
+            // If we only record this later, it can happen that Bob publishes the Bitcoin
+            // transaction, goes offline, while offline Alice publishes Monero.
+            // If the Monero transaction gets confirmed before Bob comes online again then
+            // Bob would record a wallet-height that is past the lock transaction height,
+            // which can lead to the wallet not detect the transaction.
+            let monero_wallet_restore_blockheight = monero_wallet.block_height().await?;
+
             // Alice and Bob have exchanged info
             let (state3, tx_lock) = state2.lock_btc().await?;
             let signed_tx = bitcoin_wallet
@@ -93,11 +104,17 @@ async fn next_state(
                 .context("Failed to sign Bitcoin lock transaction")?;
             let (..) = bitcoin_wallet.broadcast(signed_tx, "lock").await?;
 
-            BobState::BtcLocked(state3)
+            BobState::BtcLocked {
+                state3,
+                monero_wallet_restore_blockheight,
+            }
         }
         // Bob has locked Btc
         // Watch for Alice to Lock Xmr or for cancel timelock to elapse
-        BobState::BtcLocked(state3) => {
+        BobState::BtcLocked {
+            state3,
+            monero_wallet_restore_blockheight,
+        } => {
             let tx_lock_status = bitcoin_wallet.subscribe_to(state3.tx_lock.clone()).await;
 
             if let ExpiredTimelocks::None = state3.current_epoch(bitcoin_wallet).await? {
@@ -105,10 +122,6 @@ async fn next_state(
                 let cancel_timelock_expires =
                     tx_lock_status.wait_until_confirmed_with(state3.cancel_timelock);
 
-                // Record the current monero wallet block height so we don't have to scan from
-                // block 0 once we create the redeem wallet.
-                let monero_wallet_restore_blockheight = monero_wallet.block_height().await?;
-
                 tracing::info!("Waiting for Alice to lock Monero");
 
                 select! {

swap/tests/alice_and_bob_refund_using_cancel_and_refund_command.rs

@@ -37,7 +37,7 @@ async fn given_alice_and_bob_manually_refund_after_funds_locked_both_refund() {
             .await;
 
         // Ensure cancel timelock is expired
-        if let BobState::BtcLocked(state3) = bob_swap.state.clone() {
+        if let BobState::BtcLocked { state3, .. } = bob_swap.state.clone() {
             bob_swap
                 .bitcoin_wallet
                 .subscribe_to(state3.tx_lock)

swap/tests/concurrent_bobs_before_xmr_lock_proof_sent.rs

@@ -20,7 +20,7 @@ async fn concurrent_bobs_before_xmr_lock_proof_sent() {
         let alice_swap_1 = tokio::spawn(alice::run(alice_swap_1, FixedRate::default()));
 
         let bob_state_1 = bob_swap_1.await??;
-        assert!(matches!(bob_state_1, BobState::BtcLocked(_)));
+        assert!(matches!(bob_state_1, BobState::BtcLocked { .. }));
 
         // make sure bob_swap_1's event loop is gone
         bob_join_handle_1.abort();
@@ -44,7 +44,7 @@ async fn concurrent_bobs_before_xmr_lock_proof_sent() {
         let (bob_swap_1, _) = ctx
             .stop_and_resume_bob_from_db(bob_join_handle_2, swap_id)
             .await;
-        assert!(matches!(bob_state_1, BobState::BtcLocked(_)));
+        assert!(matches!(bob_state_1, BobState::BtcLocked { .. }));
 
         // The 1st (paused) swap is expected to refund, because the transfer
         // proof is delivered to the wrong swap, and we currently don't store it in the

swap/tests/harness/mod.rs

@@ -974,7 +974,7 @@ pub mod bob_run_until {
     use swap::protocol::bob::BobState;
 
     pub fn is_btc_locked(state: &BobState) -> bool {
-        matches!(state, BobState::BtcLocked(..))
+        matches!(state, BobState::BtcLocked { .. })
     }
 
     pub fn is_lock_proof_received(state: &BobState) -> bool {