From 9f1deb9fdc1755b699c4d77da2f4ab492abebd36 Mon Sep 17 00:00:00 2001 From: Daniel Karzel Date: Wed, 24 Feb 2021 16:34:04 +1100 Subject: [PATCH] Wrap the Monero wallet client in a Mutex In order to ensure that we can atomically generate_from_keys and then reload a wallet, we have to wrap the client of the monero wallet RPC inside a mutex. When introducing the Mutex I noticed that several inner RPC calls were leaking to the swap crate monero wallet. As this is a violation of boundaries I introduced the traits `GetAddress`, `WalletBlockHeight` and `Refresh`. Note that the monero wallet could potentially know its own public view key and public spend key. If we refactor the wallet to include this information upon wallet creation we can also generate addresses using `monero::Address::standard`. --- swap/src/bin/asb.rs | 4 +-- swap/src/bin/swap_cli.rs | 4 +-- swap/src/monero.rs | 17 +++++++++++ swap/src/monero/wallet.rs | 53 ++++++++++++++++++++++++++++------- swap/src/protocol/bob/swap.rs | 5 ++-- swap/tests/testutils/mod.rs | 23 ++++++--------- 6 files changed, 75 insertions(+), 31 deletions(-) diff --git a/swap/src/bin/asb.rs b/swap/src/bin/asb.rs index 1f06003a..38cc88da 100644 --- a/swap/src/bin/asb.rs +++ b/swap/src/bin/asb.rs @@ -31,7 +31,7 @@ use swap::{ execution_params::GetExecutionParams, fs::default_config_path, monero, - monero::{Amount, CreateWallet, OpenWallet}, + monero::{Amount, CreateWallet, GetAddress, OpenWallet}, protocol::alice::EventLoop, seed::Seed, trace::init_tracing, @@ -177,7 +177,7 @@ async fn init_wallets( let balance = monero_wallet.get_balance().await?; if balance == Amount::ZERO { - let deposit_address = monero_wallet.inner.get_address(0).await?.address; + let deposit_address = monero_wallet.get_main_address().await?; warn!( "The Monero balance is 0, make sure to deposit funds at: {}", deposit_address diff --git a/swap/src/bin/swap_cli.rs b/swap/src/bin/swap_cli.rs index affe443d..f17d224e 100644 --- a/swap/src/bin/swap_cli.rs +++ b/swap/src/bin/swap_cli.rs @@ -30,7 +30,7 @@ use swap::{ execution_params::GetExecutionParams, fs::default_config_path, monero, - monero::{CreateWallet, OpenWallet}, + monero::{CreateWallet, OpenWallet, WalletBlockHeight}, protocol::{ bob, bob::{cancel::CancelError, Builder}, @@ -316,7 +316,7 @@ async fn init_wallets( ); } - let _test_wallet_connection = monero_wallet.inner.block_height().await?; + let _test_wallet_connection = monero_wallet.block_height().await?; info!("The Monero wallet RPC is set up correctly!"); Ok((bitcoin_wallet, monero_wallet)) diff --git a/swap/src/monero.rs b/swap/src/monero.rs index a01c68c3..469de5d4 100644 --- a/swap/src/monero.rs +++ b/swap/src/monero.rs @@ -8,6 +8,8 @@ use crate::bitcoin; use ::bitcoin::hashes::core::fmt::Formatter; use anyhow::Result; use async_trait::async_trait; +use monero::Address; +use monero_rpc::wallet::{BlockHeight, Refreshed}; use rand::{CryptoRng, RngCore}; use rust_decimal::{ prelude::{FromPrimitive, ToPrimitive}, @@ -228,6 +230,21 @@ pub trait CreateWallet { async fn create_wallet(&self, file_name: &str) -> Result<()>; } +#[async_trait] +pub trait WalletBlockHeight { + async fn block_height(&self) -> Result; +} + +#[async_trait] +pub trait GetAddress { + async fn get_main_address(&self) -> Result
; +} + +#[async_trait] +pub trait Refresh { + async fn refresh(&self) -> Result; +} + #[derive(thiserror::Error, Debug, Clone, PartialEq)] #[error("Overflow, cannot convert {0} to u64")] pub struct OverflowError(pub String); diff --git a/swap/src/monero/wallet.rs b/swap/src/monero/wallet.rs index ce1d1a97..f3a061c9 100644 --- a/swap/src/monero/wallet.rs +++ b/swap/src/monero/wallet.rs @@ -1,38 +1,43 @@ use crate::monero::{ - Amount, CreateWallet, CreateWalletForOutput, InsufficientFunds, OpenWallet, PrivateViewKey, - PublicViewKey, Transfer, TransferProof, TxHash, WatchForTransfer, + Amount, CreateWallet, CreateWalletForOutput, GetAddress, InsufficientFunds, OpenWallet, + PrivateViewKey, PublicViewKey, Refresh, Transfer, TransferProof, TxHash, WalletBlockHeight, + WatchForTransfer, }; use ::monero::{Address, Network, PrivateKey, PublicKey}; use anyhow::Result; use async_trait::async_trait; use backoff::{backoff::Constant as ConstantBackoff, future::retry}; use bitcoin::hashes::core::sync::atomic::AtomicU32; -use monero_rpc::wallet; +use monero_rpc::{ + wallet, + wallet::{BlockHeight, Refreshed}, +}; use std::{ str::FromStr, sync::{atomic::Ordering, Arc}, time::Duration, }; +use tokio::sync::Mutex; use tracing::info; use url::Url; #[derive(Debug)] pub struct Wallet { - pub inner: wallet::Client, + pub inner: Mutex, pub network: Network, } impl Wallet { pub fn new(url: Url, network: Network) -> Self { Self { - inner: wallet::Client::new(url), + inner: Mutex::new(wallet::Client::new(url)), network, } } /// Get the balance of the primary account. pub async fn get_balance(&self) -> Result { - let amount = self.inner.get_balance(0).await?; + let amount = self.inner.lock().await.get_balance(0).await?; Ok(Amount::from_piconero(amount)) } @@ -51,6 +56,8 @@ impl Transfer for Wallet { let res = self .inner + .lock() + .await .transfer(0, amount.as_piconero(), &destination_address.to_string()) .await?; @@ -82,6 +89,8 @@ impl CreateWalletForOutput for Wallet { let _ = self .inner + .lock() + .await .generate_from_keys( &address.to_string(), &private_spend_key.to_string(), @@ -97,7 +106,7 @@ impl CreateWalletForOutput for Wallet { #[async_trait] impl OpenWallet for Wallet { async fn open_wallet(&self, file_name: &str) -> Result<()> { - self.inner.open_wallet(file_name).await?; + self.inner.lock().await.open_wallet(file_name).await?; Ok(()) } } @@ -105,7 +114,7 @@ impl OpenWallet for Wallet { #[async_trait] impl CreateWallet for Wallet { async fn create_wallet(&self, file_name: &str) -> Result<()> { - self.inner.create_wallet(file_name).await?; + self.inner.lock().await.create_wallet(file_name).await?; Ok(()) } } @@ -129,7 +138,6 @@ impl WatchForTransfer for Wallet { } let address = Address::standard(self.network, public_spend_key, public_view_key.into()); - let wallet = self.inner.clone(); let confirmations = Arc::new(AtomicU32::new(0u32)); @@ -137,7 +145,10 @@ impl WatchForTransfer for Wallet { // 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 + let proof = self + .inner + .lock() + .await .check_tx_key( &String::from(transfer_proof.tx_hash()), &transfer_proof.tx_key().to_string(), @@ -176,3 +187,25 @@ impl WatchForTransfer for Wallet { Ok(()) } } + +#[async_trait] +impl WalletBlockHeight for Wallet { + async fn block_height(&self) -> Result { + self.inner.lock().await.block_height().await + } +} + +#[async_trait] +impl GetAddress for Wallet { + async fn get_main_address(&self) -> Result
{ + let address = self.inner.lock().await.get_address(0).await?; + Ok(Address::from_str(address.address.as_str())?) + } +} + +#[async_trait] +impl Refresh for Wallet { + async fn refresh(&self) -> Result { + self.inner.lock().await.refresh().await + } +} diff --git a/swap/src/protocol/bob/swap.rs b/swap/src/protocol/bob/swap.rs index 72686335..a6b75316 100644 --- a/swap/src/protocol/bob/swap.rs +++ b/swap/src/protocol/bob/swap.rs @@ -4,7 +4,7 @@ use crate::{ database::{Database, Swap}, execution_params::ExecutionParams, monero, - monero::InsufficientFunds, + monero::{InsufficientFunds, WalletBlockHeight}, protocol::bob::{self, event_loop::EventLoopHandle, state::*, QuoteRequest}, }; use anyhow::{bail, Result}; @@ -132,8 +132,7 @@ async fn run_until_internal( // TODO: This can be optimized further by extracting the block height when // tx-lock was included. However, scanning a few more blocks won't do any harm // and is simpler. - let monero_wallet_restore_blockheight = - monero_wallet.inner.block_height().await?; + let monero_wallet_restore_blockheight = monero_wallet.block_height().await?; select! { transfer_proof = transfer_proof_watcher => { diff --git a/swap/tests/testutils/mod.rs b/swap/tests/testutils/mod.rs index 392d1ba8..0d4fc750 100644 --- a/swap/tests/testutils/mod.rs +++ b/swap/tests/testutils/mod.rs @@ -21,12 +21,17 @@ use swap::{ execution_params, execution_params::{ExecutionParams, GetExecutionParams}, monero, + monero::Refresh, protocol::{alice, alice::AliceState, bob, bob::BobState}, seed::Seed, }; use tempfile::tempdir; use testcontainers::{clients::Cli, Container, Docker, RunArgs}; -use tokio::{sync::mpsc, task::JoinHandle, time::interval}; +use tokio::{ + sync::{mpsc, Mutex}, + task::JoinHandle, + time::interval, +}; use tracing::dispatcher::DefaultGuard; use tracing_log::LogTracer; use url::Url; @@ -168,12 +173,7 @@ impl TestContext { assert_eq!(btc_balance_after_swap, self.alice_starting_balances.btc); // Ensure that Alice's balance is refreshed as we use a newly created wallet - self.alice_monero_wallet - .as_ref() - .inner - .refresh() - .await - .unwrap(); + self.alice_monero_wallet.as_ref().refresh().await.unwrap(); let xmr_balance_after_swap = self .alice_monero_wallet .as_ref() @@ -232,12 +232,7 @@ impl TestContext { ); // Ensure that Bob's balance is refreshed as we use a newly created wallet - self.bob_monero_wallet - .as_ref() - .inner - .refresh() - .await - .unwrap(); + self.bob_monero_wallet.as_ref().refresh().await.unwrap(); let xmr_balance_after_swap = self.bob_monero_wallet.as_ref().get_balance().await.unwrap(); assert_eq!( xmr_balance_after_swap, @@ -595,7 +590,7 @@ async fn init_test_wallets( .unwrap(); let xmr_wallet = swap::monero::Wallet { - inner: monero.wallet(name).unwrap().client(), + inner: Mutex::new(monero.wallet(name).unwrap().client()), network: monero::Network::default(), };