Merge #319

319: Alice sweeps refunded funds into default wallet r=da-kami a=da-kami

Alice's refund scenario starts with generating the temporary wallet
from keys to claim the XMR which results in Alice' unloading the wallet.
Alice then loads her original wallet to be able to handle more swaps.
Since Alice is in the role of the long running daemon handling concurrent
swaps, the operation to close, claim and re-open her default wallet must
be atomic.
This PR adds an additional step, that sweeps all the refunded XMR back into
the default wallet. In order to ensure that this is possible, Alice has to
ensure that the locked XMR got enough confirmations.
These changes allow us to assert Alice's balance after refunding.

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

swap/src/bin/asb.rs

@@ -164,18 +164,16 @@ async fn init_wallets(
         bitcoin_balance
     );
 
-    let monero_wallet = monero::Wallet::new(
+    let monero_wallet = monero::Wallet::open_or_create(
         config.monero.wallet_rpc_url.clone(),
         DEFAULT_WALLET_NAME.to_string(),
         env_config,
-    );
-
-    // Setup the Monero wallet
-    monero_wallet.open_or_create().await?;
+    )
+    .await?;
 
     let balance = monero_wallet.get_balance().await?;
     if balance == Amount::ZERO {
-        let deposit_address = monero_wallet.get_main_address().await?;
+        let deposit_address = monero_wallet.get_main_address();
         warn!(
             "The Monero balance is 0, make sure to deposit funds at: {}",
             deposit_address

swap/src/bin/swap.rs

@@ -295,18 +295,12 @@ async fn init_monero_wallet(
         .run(network, monero_daemon_host.as_str())
         .await?;
 
-    let monero_wallet = monero::Wallet::new(
+    let monero_wallet = monero::Wallet::open_or_create(
         monero_wallet_rpc_process.endpoint(),
         MONERO_BLOCKCHAIN_MONITORING_WALLET_NAME.to_string(),
         env_config,
-    );
-
-    monero_wallet.open_or_create().await?;
-
-    let _test_wallet_connection = monero_wallet
-        .block_height()
-        .await
-        .context("Failed to validate connection to monero-wallet-rpc")?;
+    )
+    .await?;
 
     Ok((monero_wallet, monero_wallet_rpc_process))
 }

swap/src/monero/wallet.rs

@@ -5,7 +5,7 @@ use crate::monero::{
 use ::monero::{Address, Network, PrivateKey, PublicKey};
 use anyhow::{Context, Result};
 use monero_rpc::wallet;
-use monero_rpc::wallet::{BlockHeight, CheckTxKey, Client, Refreshed};
+use monero_rpc::wallet::{BlockHeight, CheckTxKey, Refreshed};
 use std::future::Future;
 use std::str::FromStr;
 use std::time::Duration;
@@ -19,24 +19,44 @@ pub struct Wallet {
     inner: Mutex<wallet::Client>,
     network: Network,
     name: String,
+    main_address: monero::Address,
     sync_interval: Duration,
 }
 
 impl Wallet {
-    pub fn new(url: Url, name: String, env_config: Config) -> Self {
-        Self::new_with_client(Client::new(url), name, env_config)
+    /// Connect to a wallet RPC and load the given wallet by name.
+    pub async fn open_or_create(url: Url, name: String, env_config: Config) -> Result<Self> {
+        let client = wallet::Client::new(url);
+
+        let open_wallet_response = client.open_wallet(name.as_str()).await;
+        if open_wallet_response.is_err() {
+            client.create_wallet(name.as_str()).await.context(
+                "Unable to create Monero wallet, please ensure that the monero-wallet-rpc is available",
+            )?;
+
+            debug!("Created Monero wallet {}", name);
+        } else {
+            debug!("Opened Monero wallet {}", name);
+        }
+
+        Self::connect(client, name, env_config).await
     }
 
-    pub fn new_with_client(client: wallet::Client, name: String, env_config: Config) -> Self {
-        Self {
+    /// Connects to a wallet RPC where a wallet is already loaded.
+    pub async fn connect(client: wallet::Client, name: String, env_config: Config) -> Result<Self> {
+        let main_address =
+            monero::Address::from_str(client.get_address(0).await?.address.as_str())?;
+        Ok(Self {
             inner: Mutex::new(client),
             network: env_config.monero_network,
             name,
+            main_address,
             sync_interval: env_config.monero_sync_interval(),
-        }
+        })
     }
 
-    pub async fn open(&self) -> Result<()> {
+    /// Re-open the wallet using the internally stored name.
+    pub async fn re_open(&self) -> Result<()> {
         self.inner
             .lock()
             .await
@@ -45,21 +65,8 @@ impl Wallet {
         Ok(())
     }
 
-    pub async fn open_or_create(&self) -> Result<()> {
-        let open_wallet_response = self.open().await;
-        if open_wallet_response.is_err() {
-            self.inner.lock().await.create_wallet(self.name.as_str()).await.context(
-                "Unable to create Monero wallet, please ensure that the monero-wallet-rpc is available",
-            )?;
-
-            debug!("Created Monero wallet {}", self.name);
-        } else {
-            debug!("Opened Monero wallet {}", self.name);
-        }
-
-        Ok(())
-    }
-
+    /// Close the wallet and open (load) another wallet by generating it from
+    /// keys. The generated wallet will remain loaded.
     pub async fn create_from_and_load(
         &self,
         private_spend_key: PrivateKey,
@@ -89,6 +96,10 @@ impl Wallet {
         Ok(())
     }
 
+    /// Close the wallet and open (load) another wallet by generating it from
+    /// keys. The generated wallet will be opened, all funds sweeped to the
+    /// main_address and then the wallet will be re-loaded using the internally
+    /// stored name.
     pub async fn create_from(
         &self,
         private_spend_key: PrivateKey,
@@ -98,23 +109,48 @@ impl Wallet {
         let public_spend_key = PublicKey::from_private_key(&private_spend_key);
         let public_view_key = PublicKey::from_private_key(&private_view_key.into());
 
-        let address = Address::standard(self.network, public_spend_key, public_view_key);
+        let temp_wallet_address =
+            Address::standard(self.network, public_spend_key, public_view_key);
 
         let wallet = self.inner.lock().await;
 
-        // Properly close the wallet before generating the other wallet to ensure that
+        // Close the default wallet before generating the other wallet to ensure that
         // it saves its state correctly
         let _ = wallet.close_wallet().await?;
 
         let _ = wallet
             .generate_from_keys(
-                &address.to_string(),
+                &temp_wallet_address.to_string(),
                 &private_spend_key.to_string(),
                 &PrivateKey::from(private_view_key).to_string(),
                 restore_height.height,
             )
             .await?;
 
+        // Try to send all the funds from the generated wallet to the default wallet
+        match wallet.refresh().await {
+            Ok(_) => match wallet
+                .sweep_all(self.main_address.to_string().as_str())
+                .await
+            {
+                Ok(sweep_all) => {
+                    for tx in sweep_all.tx_hash_list {
+                        tracing::info!(%tx, "Monero transferred back to default wallet {}", self.main_address);
+                    }
+                }
+                Err(e) => {
+                    tracing::warn!(
+                        "Transferring Monero back to default wallet {} failed with {:#}",
+                        self.main_address,
+                        e
+                    );
+                }
+            },
+            Err(e) => {
+                tracing::warn!("Refreshing the generated wallet failed with {:#}", e);
+            }
+        }
+
         let _ = wallet.open_wallet(self.name.as_str()).await?;
 
         Ok(())
@@ -209,9 +245,8 @@ impl Wallet {
         self.inner.lock().await.block_height().await
     }
 
-    pub async fn get_main_address(&self) -> Result<Address> {
-        let address = self.inner.lock().await.get_address(0).await?;
-        Ok(Address::from_str(address.address.as_str())?)
+    pub fn get_main_address(&self) -> Address {
+        self.main_address
     }
 
     pub async fn refresh(&self) -> Result<Refreshed> {

swap/src/protocol/alice/state.rs

@@ -2,7 +2,8 @@ use crate::bitcoin::{
     current_epoch, CancelTimelock, ExpiredTimelocks, PunishTimelock, TxCancel, TxPunish, TxRefund,
 };
 use crate::env::Config;
-use crate::monero::wallet::TransferRequest;
+use crate::monero::wallet::{TransferRequest, WatchRequest};
+use crate::monero::TransferProof;
 use crate::protocol::alice::{Message1, Message3};
 use crate::protocol::bob::{Message0, Message2, Message4};
 use crate::protocol::CROSS_CURVE_PROOF_SYSTEM;
@@ -357,6 +358,24 @@ impl State3 {
         }
     }
 
+    pub fn lock_xmr_watch_request(
+        &self,
+        transfer_proof: TransferProof,
+        conf_target: u32,
+    ) -> WatchRequest {
+        let S_a = monero::PublicKey::from_private_key(&monero::PrivateKey { scalar: self.s_a });
+
+        let public_spend_key = S_a + self.S_b_monero;
+        let public_view_key = self.v.public();
+        WatchRequest {
+            public_spend_key,
+            public_view_key,
+            transfer_proof,
+            conf_target,
+            expected: self.xmr,
+        }
+    }
+
     pub fn tx_cancel(&self) -> TxCancel {
         TxCancel::new(&self.tx_lock, self.cancel_timelock, self.a.public(), self.B)
     }

swap/src/protocol/alice/swap.rs

@@ -97,13 +97,20 @@ async fn run_until_internal(
                 .transfer(state3.lock_xmr_transfer_request())
                 .await?;
 
-            // TODO(Franck): Wait for Monero to be confirmed once
-            //  Waiting for XMR confirmations should not be done in here, but in a separate
+            monero_wallet
+                .watch_for_transfer(state3.lock_xmr_watch_request(transfer_proof.clone(), 1))
+                .await?;
+
+            // TODO: Waiting for XMR confirmations should be done in a separate
             //  state! We have to record that Alice has already sent the transaction.
             //  Otherwise Alice might publish the lock tx twice!
 
             event_loop_handle
-                .send_transfer_proof(transfer_proof)
+                .send_transfer_proof(transfer_proof.clone())
+                .await?;
+
+            monero_wallet
+                .watch_for_transfer(state3.lock_xmr_watch_request(transfer_proof, 10))
                 .await?;
 
             AliceState::XmrLocked {
@@ -283,22 +290,30 @@ async fn run_until_internal(
                 state3.B,
             )?;
 
-            let punish_tx_finalised = async {
+            let punish = async {
                 let (txid, finality) = bitcoin_wallet.broadcast(signed_tx_punish, "punish").await?;
-
                 finality.await?;
 
                 Result::<_, anyhow::Error>::Ok(txid)
-            };
+            }
+            .await;
 
-            let tx_refund = state3.tx_refund();
-            let refund_tx_seen =
-                bitcoin_wallet.watch_until_status(&tx_refund, |status| status.has_been_seen());
+            match punish {
+                Ok(_) => AliceState::BtcPunished,
+                Err(e) => {
+                    tracing::warn!(
+                        "Falling back to refund because punish transaction failed with {:#}",
+                        e
+                    );
 
-            select! {
-                result = refund_tx_seen => {
-                    result.context("Failed to monitor refund transaction")?;
+                    // Upon punish failure we assume that the refund tx was included but we
+                    // missed seeing it. In case we fail to fetch the refund tx we fail
+                    // with no state update because it is unclear what state we should transition
+                    // to. It does not help to race punish and refund inclusion,
+                    // because a punish tx failure is not recoverable (besides re-trying) if the
+                    // refund tx was not included.
 
+                    let tx_refund = state3.tx_refund();
                     let published_refund_tx =
                         bitcoin_wallet.get_raw_transaction(tx_refund.txid()).await?;
 
@@ -308,15 +323,13 @@ async fn run_until_internal(
                         state3.a.clone(),
                         state3.S_b_bitcoin,
                     )?;
+
                     AliceState::BtcRefunded {
                         spend_key,
                         state3,
                         monero_wallet_restore_blockheight,
                     }
                 }
-                _ = punish_tx_finalised => {
-                    AliceState::BtcPunished
-                }
             }
         }
         AliceState::XmrRefunded => AliceState::XmrRefunded,

swap/tests/bob_refunds_using_cancel_and_refund_command.rs

@@ -12,7 +12,7 @@ async fn given_bob_manually_refunds_after_btc_locked_bob_refunds() {
         let bob_swap = tokio::spawn(bob::run_until(bob_swap, is_btc_locked));
 
         let alice_swap = ctx.alice_next_swap().await;
-        let _ = tokio::spawn(alice::run(alice_swap));
+        let alice_swap = tokio::spawn(alice::run(alice_swap));
 
         let bob_state = bob_swap.await??;
         assert!(matches!(bob_state, BobState::BtcLocked { .. }));
@@ -56,6 +56,9 @@ async fn given_bob_manually_refunds_after_btc_locked_bob_refunds() {
 
         ctx.assert_bob_refunded(bob_state).await;
 
+        let alice_state = alice_swap.await??;
+        ctx.assert_alice_refunded(alice_state).await;
+
         Ok(())
     })
     .await

swap/tests/testutils/mod.rs

@@ -57,7 +57,7 @@ struct BobParams {
 
 impl BobParams {
     pub async fn builder(&self, event_loop_handle: bob::EventLoopHandle) -> Result<bob::Builder> {
-        let receive_address = self.monero_wallet.get_main_address().await?;
+        let receive_address = self.monero_wallet.get_main_address();
 
         Ok(bob::Builder::new(
             Database::open(&self.db_path.clone().as_path()).unwrap(),
@@ -191,7 +191,15 @@ impl TestContext {
             .get_balance()
             .await
             .unwrap();
-        assert_eq!(xmr_balance_after_swap, self.xmr_amount);
+
+        // Alice pays fees - comparison does not take exact lock fee into account
+        assert!(
+            xmr_balance_after_swap > self.alice_starting_balances.xmr - self.xmr_amount,
+            "{} > {} - {}",
+            xmr_balance_after_swap,
+            self.alice_starting_balances.xmr,
+            self.xmr_amount
+        );
     }
 
     pub async fn assert_alice_punished(&self, state: AliceState) {
@@ -237,7 +245,7 @@ impl TestContext {
         );
 
         // unload the generated wallet by opening the original wallet
-        self.bob_monero_wallet.open().await.unwrap();
+        self.bob_monero_wallet.re_open().await.unwrap();
         // refresh the original wallet to make sure the balance is caught up
         self.bob_monero_wallet.refresh().await.unwrap();
 
@@ -587,11 +595,13 @@ async fn init_test_wallets(
         .await
         .unwrap();
 
-    let xmr_wallet = swap::monero::Wallet::new_with_client(
+    let xmr_wallet = swap::monero::Wallet::connect(
         monero.wallet(name).unwrap().client(),
         name.to_string(),
         env_config,
-    );
+    )
+    .await
+    .unwrap();
 
     let electrum_rpc_url = {
         let input = format!("tcp://@localhost:{}", electrum_rpc_port);