Merge #136

136: Testnet resume fixes r=da-kami a=da-kami

Add a few log statements on Bob's side to make the user experience better.
Update / remove ToDos.

I set the log level to `Info` in main again, `Debug` heavily clutters the output. In order to make `Debug` more usable we might want to review printing all those `rpc` messages. But this goes beyond the scope of this PR.


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

monero-harness/src/rpc/wallet.rs

@@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize};
 use tracing::debug;
 
 /// JSON RPC client for monero-wallet-rpc.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct Client {
     pub inner: reqwest::Client,
     pub url: Url,

swap/src/main.rs

@@ -41,7 +41,7 @@ extern crate prettytable;
 
 #[tokio::main]
 async fn main() -> Result<()> {
-    init_tracing(LevelFilter::Trace).expect("initialize tracing");
+    init_tracing(LevelFilter::Info).expect("initialize tracing");
 
     let opt = Options::from_args();
     let config = Config::testnet();

swap/src/monero/wallet.rs

@@ -2,8 +2,14 @@ use ::monero::{Address, Network, PrivateKey, PublicKey};
 use anyhow::Result;
 use async_trait::async_trait;
 use backoff::{backoff::Constant as ConstantBackoff, future::FutureOperation as _};
+use bitcoin::hashes::core::sync::atomic::AtomicU32;
 use monero_harness::rpc::wallet;
-use std::{str::FromStr, time::Duration};
+use std::{
+    str::FromStr,
+    sync::{atomic::Ordering, Arc},
+    time::Duration,
+};
+use tracing::info;
 use url::Url;
 
 use crate::monero::{
@@ -109,33 +115,47 @@ impl WatchForTransfer for Wallet {
         }
 
         let address = Address::standard(self.network, public_spend_key, public_view_key.into());
+        let wallet = self.inner.clone();
 
-        let res = (|| async {
-            // NOTE: Currently, this is conflating IO errors with the transaction not being
-            // in the blockchain yet, or not having enough confirmations on it. All these
-            // errors warrant a retry, but the strategy should probably differ per case
-            let proof = self
-                .inner
-                .check_tx_key(
-                    &String::from(transfer_proof.tx_hash()),
-                    &transfer_proof.tx_key().to_string(),
-                    &address.to_string(),
-                )
-                .await
-                .map_err(|_| backoff::Error::Transient(Error::TxNotFound))?;
+        let confirmations = Arc::new(AtomicU32::new(0u32));
+        let res = (move || {
+            let confirmations = confirmations.clone();
+            let transfer_proof = transfer_proof.clone();
+            let wallet = wallet.clone();
+            async move {
+                // NOTE: Currently, this is conflicting IO errors with the transaction not being
+                // in the blockchain yet, or not having enough confirmations on it. All these
+                // errors warrant a retry, but the strategy should probably differ per case
+                let proof = wallet
+                    .check_tx_key(
+                        &String::from(transfer_proof.tx_hash()),
+                        &transfer_proof.tx_key().to_string(),
+                        &address.to_string(),
+                    )
+                    .await
+                    .map_err(|_| backoff::Error::Transient(Error::TxNotFound))?;
 
-            if proof.received != expected_amount.as_piconero() {
-                return Err(backoff::Error::Permanent(Error::InsufficientFunds {
-                    expected: expected_amount,
-                    actual: Amount::from_piconero(proof.received),
-                }));
+                if proof.received != expected_amount.as_piconero() {
+                    return Err(backoff::Error::Permanent(Error::InsufficientFunds {
+                        expected: expected_amount,
+                        actual: Amount::from_piconero(proof.received),
+                    }));
+                }
+
+                if proof.confirmations > confirmations.load(Ordering::SeqCst) {
+                    confirmations.store(proof.confirmations, Ordering::SeqCst);
+                    info!(
+                        "Monero lock tx received {} out of {} confirmations",
+                        proof.confirmations, expected_confirmations
+                    );
+                }
+
+                if proof.confirmations < expected_confirmations {
+                    return Err(backoff::Error::Transient(Error::InsufficientConfirmations));
+                }
+
+                Ok(proof)
             }
-
-            if proof.confirmations < expected_confirmations {
-                return Err(backoff::Error::Transient(Error::InsufficientConfirmations));
-            }
-
-            Ok(proof)
         })
         .retry(ConstantBackoff::new(Duration::from_secs(1)))
         .await;

swap/src/protocol/alice/steps.rs

@@ -135,6 +135,9 @@ where
         .await?;
 
     // TODO(Franck): Wait for Monero to be confirmed once
+    //  Waiting for XMR confirmations should not be done in here, but 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_message2(channel, alice::Message2 {

swap/src/protocol/alice/swap.rs

@@ -210,8 +210,6 @@ pub async fn run_until(
                 .await
             }
             AliceState::XmrLocked { state3 } => {
-                // todo: match statement and wait for cancel timelock to expire can probably be
-                // expressed more cleanly
                 let state = match state3.expired_timelocks(bitcoin_wallet.as_ref()).await? {
                     ExpiredTimelocks::None => {
                         let wait_for_enc_sig =

swap/src/protocol/bob/swap.rs

@@ -3,7 +3,7 @@ use async_recursion::async_recursion;
 use rand::{CryptoRng, RngCore};
 use std::sync::Arc;
 use tokio::select;
-use tracing::info;
+use tracing::{debug, info};
 use uuid::Uuid;
 
 use crate::{
@@ -153,8 +153,12 @@ where
                     select! {
                         msg2 = msg2_watcher => {
 
+                            let msg2 = msg2?;
+                            info!("Received XMR lock transaction transfer proof from Alice, watching for transfer confirmations");
+                            debug!("Transfer proof: {:?}", msg2.tx_lock_proof);
+
                             let xmr_lock_watcher = state3.clone()
-                                .watch_for_lock_xmr(monero_wallet.as_ref(), msg2?, monero_wallet_restore_blockheight.height);
+                                .watch_for_lock_xmr(monero_wallet.as_ref(), msg2, monero_wallet_restore_blockheight.height);
                             let cancel_timelock_expires = state3.wait_for_cancel_timelock_to_expire(bitcoin_wallet.as_ref());
 
                             select! {
@@ -201,7 +205,7 @@ where
                     let tx_redeem_encsig = state.tx_redeem_encsig();
 
                     let state4_clone = state.clone();
-                    // TODO(Franck): Refund if message cannot be sent.
+
                     let enc_sig_sent_watcher = event_loop_handle.send_message3(tx_redeem_encsig);
                     let bitcoin_wallet = bitcoin_wallet.clone();
                     let cancel_timelock_expires =