From 90494ba4a51c982e7bd898f02fd155b0974d721a Mon Sep 17 00:00:00 2001 From: Byron Hambly Date: Mon, 10 Jun 2024 18:53:52 +0200 Subject: [PATCH] fix: monero wallet refresh (#1596) This PR changes the following behaviour in the refresh functionality of the monero wallet - Allows for multiple retries because in some cases users have experienced an issue where the wallet rpc returns `no connection to daemon` even though the daemon is available. I'm not 100% sure why this happens but retrying often fixes the issue - Print the current sync height after each failed attempt at syncing to see how far we've come - The `monero-wallet-rpc` is started with the `--no-initial-sync` flag which ensures that as soon as it's started, it's ready to respond to requests - The `monero-wallet-rpc` was upgraded to `v0.18.3.1` because this PR https://github.com/monero-project/monero/pull/8941 has improved some of the issues mentioned above This PR is part of a larger effort to fix this issue https://github.com/comit-network/xmr-btc-swap/issues/1432 --- CHANGELOG.md | 2 ++ monero-rpc/src/wallet.rs | 6 ++++ swap/src/monero/wallet.rs | 67 ++++++++++++++++++++++++++++++----- swap/src/monero/wallet_rpc.rs | 1 + swap/src/protocol/bob/swap.rs | 2 +- swap/tests/harness/mod.rs | 2 +- 6 files changed, 69 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fa3bdeb..4b0ab63e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- Add retry logic to monero-wallet-rpc wallet refresh + ## [0.13.0] - 2024-05-29 - Minimum Supported Rust Version (MSRV) bumped to 1.74 diff --git a/monero-rpc/src/wallet.rs b/monero-rpc/src/wallet.rs index bc78e7a6..3e21ad1b 100644 --- a/monero-rpc/src/wallet.rs +++ b/monero-rpc/src/wallet.rs @@ -162,6 +162,12 @@ pub struct BlockHeight { pub height: u32, } +impl fmt::Display for BlockHeight { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.height) + } +} + #[derive(Clone, Copy, Debug, Deserialize)] #[serde(from = "CheckTxKeyResponse")] pub struct CheckTxKey { diff --git a/swap/src/monero/wallet.rs b/swap/src/monero/wallet.rs index 56fd8e60..99fa206a 100644 --- a/swap/src/monero/wallet.rs +++ b/swap/src/monero/wallet.rs @@ -45,6 +45,7 @@ impl Wallet { pub async fn connect(client: wallet::Client, name: String, env_config: Config) -> Result { 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, @@ -125,13 +126,14 @@ impl Wallet { let temp_wallet_address = Address::standard(self.network, public_spend_key, public_view_key); - let wallet = self.inner.lock().await; - // Close the default wallet before generating the other wallet to ensure that // it saves its state correctly - let _ = wallet.close_wallet().await?; + let _ = self.inner.lock().await.close_wallet().await?; - let _ = wallet + let _ = self + .inner + .lock() + .await .generate_from_keys( file_name, temp_wallet_address.to_string(), @@ -144,8 +146,14 @@ impl Wallet { .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()).await { + match self.refresh(3).await { + Ok(_) => match self + .inner + .lock() + .await + .sweep_all(self.main_address.to_string()) + .await + { Ok(sweep_all) => { for tx in sweep_all.tx_hash_list { tracing::info!( @@ -166,7 +174,12 @@ impl Wallet { } } - let _ = wallet.open_wallet(self.name.clone()).await?; + let _ = self + .inner + .lock() + .await + .open_wallet(self.name.clone()) + .await?; Ok(()) } @@ -261,8 +274,44 @@ impl Wallet { self.main_address } - pub async fn refresh(&self) -> Result { - Ok(self.inner.lock().await.refresh().await?) + pub async fn refresh(&self, max_attempts: usize) -> Result { + const RETRY_INTERVAL: Duration = Duration::from_secs(1); + + for i in 1..=max_attempts { + tracing::info!(name = %self.name, attempt=i, "Syncing Monero wallet"); + + let result = self.inner.lock().await.refresh().await; + + match result { + Ok(refreshed) => { + tracing::info!(name = %self.name, "Monero wallet synced"); + return Ok(refreshed); + } + Err(error) => { + let attempts_left = max_attempts - i; + + // We would not want to fail here if the height is not available + // as it is not critical for the operation of the wallet. + // We can just log a warning and continue. + let height = match self.inner.lock().await.get_height().await { + Ok(height) => height.to_string(), + Err(_) => { + tracing::warn!(name = %self.name, "Failed to fetch Monero wallet height during sync"); + "unknown".to_string() + } + }; + + tracing::warn!(attempt=i, %height, %attempts_left, name = %self.name, %error, "Failed to sync Monero wallet"); + + if attempts_left == 0 { + return Err(error.into()); + } + } + } + + tokio::time::sleep(RETRY_INTERVAL).await; + } + unreachable!("Loop should have returned by now"); } } diff --git a/swap/src/monero/wallet_rpc.rs b/swap/src/monero/wallet_rpc.rs index f99994df..c7009993 100644 --- a/swap/src/monero/wallet_rpc.rs +++ b/swap/src/monero/wallet_rpc.rs @@ -352,6 +352,7 @@ impl WalletRpc { .arg("--disable-rpc-login") .arg("--wallet-dir") .arg(self.working_dir.join("monero-data")) + .arg("--no-initial-sync") .spawn()?; let stdout = child diff --git a/swap/src/protocol/bob/swap.rs b/swap/src/protocol/bob/swap.rs index a1dec289..3900a17b 100644 --- a/swap/src/protocol/bob/swap.rs +++ b/swap/src/protocol/bob/swap.rs @@ -264,7 +264,7 @@ async fn next_state( } // Ensure that the generated wallet is synced so we have a proper balance - monero_wallet.refresh().await?; + monero_wallet.refresh(20).await?; // Sweep (transfer all funds) to the given address let tx_hashes = monero_wallet.sweep_all(monero_receive_address).await?; diff --git a/swap/tests/harness/mod.rs b/swap/tests/harness/mod.rs index 025b7e42..24083a71 100644 --- a/swap/tests/harness/mod.rs +++ b/swap/tests/harness/mod.rs @@ -891,7 +891,7 @@ impl Wallet for monero::Wallet { type Amount = monero::Amount; async fn refresh(&self) -> Result<()> { - self.refresh().await?; + self.refresh(1).await?; Ok(()) }