From 8d76607343bfbbab24dda4bf7d8cf17e6906f77c Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 19 Apr 2021 17:26:11 +1000 Subject: [PATCH] Refactor monero-harness containers 1. Split up image::Monero into Monerod and MoneroWalletRpc 2. Don't use `bash` to run the internal command. Instead we disable the entrypoint script as per https://github.com/XMRto/monero#raw-commands 3. Remove the start up delay by listening for the correct log message. To make this more resilient, we make the log level NOT configurable and instead always log verbosely. --- .github/workflows/ci.yml | 2 - Cargo.lock | 1 - monero-harness/Cargo.toml | 1 - monero-harness/src/image.rs | 253 ++++++++++++++------------------ monero-harness/src/lib.rs | 60 ++++---- monero-harness/tests/monerod.rs | 2 +- monero-harness/tests/wallet.rs | 3 +- swap/tests/happy_path.rs | 2 - swap/tests/harness/mod.rs | 26 ++-- 9 files changed, 153 insertions(+), 197 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9c9b1cc8..ac711da9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -131,5 +131,3 @@ jobs: - name: Run test ${{ matrix.test_name }} run: cargo test --package swap --all-features --test ${{ matrix.test_name }} "" - env: - MONERO_ADDITIONAL_SLEEP_PERIOD: 60000 diff --git a/Cargo.lock b/Cargo.lock index 261d3600..ac0a1f30 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2167,7 +2167,6 @@ dependencies = [ "anyhow", "futures", "monero-rpc", - "port_check", "rand 0.7.3", "spectral", "testcontainers 0.12.0", diff --git a/monero-harness/Cargo.toml b/monero-harness/Cargo.toml index fa57f1a9..0b090d0c 100644 --- a/monero-harness/Cargo.toml +++ b/monero-harness/Cargo.toml @@ -8,7 +8,6 @@ edition = "2018" anyhow = "1" futures = "0.3" monero-rpc = { path = "../monero-rpc" } -port_check = "0.1" rand = "0.7" spectral = "0.6" testcontainers = "0.12" diff --git a/monero-harness/src/image.rs b/monero-harness/src/image.rs index d44ce6d3..4e1ee8bc 100644 --- a/monero-harness/src/image.rs +++ b/monero-harness/src/image.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, env::var, thread::sleep, time::Duration}; +use std::collections::HashMap; use testcontainers::{ core::{Container, Docker, WaitForMessage}, Image, @@ -6,18 +6,22 @@ use testcontainers::{ pub const MONEROD_DAEMON_CONTAINER_NAME: &str = "monerod"; pub const MONEROD_DEFAULT_NETWORK: &str = "monero_network"; -pub const MONEROD_RPC_PORT: u16 = 48081; -pub const WALLET_RPC_PORT: u16 = 48083; + +/// The port we use for all RPC communication. +/// +/// This is the default when running monerod. +/// For `monero-wallet-rpc` we always need to specify a port. To make things +/// simpler, we just specify the same one. They are in different containers so +/// this doesn't matter. +pub const RPC_PORT: u16 = 18081; #[derive(Debug)] -pub struct Monero { - args: Args, - entrypoint: Option, - wait_for_message: String, +pub struct Monerod { + args: MonerodArgs, } -impl Image for Monero { - type Args = Args; +impl Image for Monerod { + type Args = MonerodArgs; type EnvVars = HashMap; type Volumes = HashMap; type EntryPoint = str; @@ -30,17 +34,8 @@ impl Image for Monero { container .logs() .stdout - .wait_for_message(&self.wait_for_message) + .wait_for_message("JOINING all threads") .unwrap(); - - let additional_sleep_period = - var("MONERO_ADDITIONAL_SLEEP_PERIOD").map(|value| value.parse()); - - if let Ok(Ok(sleep_period)) = additional_sleep_period { - let sleep_period = Duration::from_millis(sleep_period); - - sleep(sleep_period) - } } fn args(&self) -> ::Args { @@ -56,69 +51,80 @@ impl Image for Monero { } fn with_args(self, args: ::Args) -> Self { - Monero { args, ..self } - } - - fn with_entrypoint(self, entrypoint: &Self::EntryPoint) -> Self { - Self { - entrypoint: Some(entrypoint.to_string()), - ..self - } + Self { args } } fn entrypoint(&self) -> Option { - self.entrypoint.to_owned() + Some("".to_owned()) // an empty entrypoint disables the entrypoint + // script and gives us full control } } -impl Default for Monero { +impl Default for Monerod { fn default() -> Self { - Monero { - args: Args::default(), - entrypoint: Some("".into()), - wait_for_message: "core RPC server started ok".to_string(), + Self { + args: MonerodArgs::default(), } } } -impl Monero { - pub fn wallet(name: &str, daemon_address: String) -> Self { - let wallet = WalletArgs::new(name, daemon_address, WALLET_RPC_PORT); - let default = Monero::default(); - Self { - args: Args { - image_args: ImageArgs::WalletArgs(wallet), - }, - wait_for_message: "Run server thread name: RPC".to_string(), - ..default - } - } +#[derive(Debug)] +pub struct MoneroWalletRpc { + args: MoneroWalletRpcArgs, } -#[derive(Clone, Debug)] -pub struct Args { - image_args: ImageArgs, +impl Image for MoneroWalletRpc { + type Args = MoneroWalletRpcArgs; + type EnvVars = HashMap; + type Volumes = HashMap; + type EntryPoint = str; + + fn descriptor(&self) -> String { + "xmrto/monero:v0.17.2.0".to_owned() + } + + fn wait_until_ready(&self, container: &Container<'_, D, Self>) { + container + .logs() + .stdout + .wait_for_message("JOINING all threads") + .unwrap(); + } + + fn args(&self) -> ::Args { + self.args.clone() + } + + fn volumes(&self) -> Self::Volumes { + HashMap::new() + } + + fn env_vars(&self) -> Self::EnvVars { + HashMap::new() + } + + fn with_args(self, args: ::Args) -> Self { + Self { args } + } + + fn entrypoint(&self) -> Option { + Some("".to_owned()) // an empty entrypoint disables the entrypoint + // script and gives us full control + } } -impl Default for Args { +impl Default for MoneroWalletRpc { fn default() -> Self { Self { - image_args: ImageArgs::MonerodArgs(MonerodArgs::default()), + args: MoneroWalletRpcArgs::default(), } } } -#[derive(Clone, Debug)] -pub enum ImageArgs { - MonerodArgs(MonerodArgs), - WalletArgs(WalletArgs), -} - -impl ImageArgs { - fn args(&self) -> String { - match self { - ImageArgs::MonerodArgs(monerod_args) => monerod_args.args(), - ImageArgs::WalletArgs(wallet_args) => wallet_args.args(), +impl MoneroWalletRpc { + pub fn new(name: &str, daemon_address: String) -> Self { + Self { + args: MoneroWalletRpcArgs::new(name, daemon_address), } } } @@ -129,51 +135,39 @@ pub struct MonerodArgs { pub offline: bool, pub rpc_payment_allow_free_loopback: bool, pub confirm_external_bind: bool, - pub non_interactive: bool, pub no_igd: bool, pub hide_my_port: bool, pub rpc_bind_ip: String, - pub rpc_bind_port: u16, pub fixed_difficulty: u32, pub data_dir: String, - pub log_level: u32, } -#[derive(Debug, Clone)] -pub struct WalletArgs { - pub disable_rpc_login: bool, - pub confirm_external_bind: bool, - pub wallet_dir: String, - pub rpc_bind_ip: String, - pub rpc_bind_port: u16, - pub daemon_address: String, - pub log_level: u32, -} - -/// Sane defaults for a mainnet regtest instance. impl Default for MonerodArgs { fn default() -> Self { - MonerodArgs { + Self { regtest: true, offline: true, rpc_payment_allow_free_loopback: true, confirm_external_bind: true, - non_interactive: true, no_igd: true, hide_my_port: true, rpc_bind_ip: "0.0.0.0".to_string(), - rpc_bind_port: MONEROD_RPC_PORT, fixed_difficulty: 1, data_dir: "/monero".to_string(), - log_level: 2, } } } -impl MonerodArgs { - // Return monerod args as is single string so we can pass it to bash. - fn args(&self) -> String { - let mut args = vec!["monerod".to_string()]; +impl IntoIterator for MonerodArgs { + type Item = String; + type IntoIter = ::std::vec::IntoIter; + + fn into_iter(self) -> ::IntoIter { + let mut args = vec![ + "monerod".to_string(), + "--log-level=4".to_string(), + "--non-interactive".to_string(), + ]; if self.regtest { args.push("--regtest".to_string()) @@ -191,10 +185,6 @@ impl MonerodArgs { args.push("--confirm-external-bind".to_string()) } - if self.non_interactive { - args.push("--non-interactive".to_string()) - } - if self.no_igd { args.push("--no-igd".to_string()) } @@ -204,45 +194,60 @@ impl MonerodArgs { } if !self.rpc_bind_ip.is_empty() { - args.push(format!("--rpc-bind-ip {}", self.rpc_bind_ip)); - } - - if self.rpc_bind_port != 0 { - args.push(format!("--rpc-bind-port {}", self.rpc_bind_port)); + args.push(format!("--rpc-bind-ip={}", self.rpc_bind_ip)); } if !self.data_dir.is_empty() { - args.push(format!("--data-dir {}", self.data_dir)); + args.push(format!("--data-dir={}", self.data_dir)); } if self.fixed_difficulty != 0 { - args.push(format!("--fixed-difficulty {}", self.fixed_difficulty)); + args.push(format!("--fixed-difficulty={}", self.fixed_difficulty)); } - if self.log_level != 0 { - args.push(format!("--log-level {}", self.log_level)); - } + args.into_iter() + } +} - args.join(" ") +#[derive(Debug, Clone)] +pub struct MoneroWalletRpcArgs { + pub disable_rpc_login: bool, + pub confirm_external_bind: bool, + pub wallet_dir: String, + pub rpc_bind_ip: String, + pub daemon_address: String, +} + +impl Default for MoneroWalletRpcArgs { + fn default() -> Self { + unimplemented!("A default instance for `MoneroWalletRpc` doesn't make sense because we always need to connect to a node.") } } -impl WalletArgs { - pub fn new(wallet_name: &str, daemon_address: String, rpc_port: u16) -> Self { - WalletArgs { +impl MoneroWalletRpcArgs { + pub fn new(wallet_name: &str, daemon_address: String) -> Self { + Self { disable_rpc_login: true, confirm_external_bind: true, wallet_dir: wallet_name.into(), rpc_bind_ip: "0.0.0.0".into(), - rpc_bind_port: rpc_port, daemon_address, - log_level: 4, } } +} + +impl IntoIterator for MoneroWalletRpcArgs { + type Item = String; + type IntoIter = ::std::vec::IntoIter; - // Return monero-wallet-rpc args as is single string so we can pass it to bash. - fn args(&self) -> String { - let mut args = vec!["monero-wallet-rpc".to_string()]; + fn into_iter(self) -> ::IntoIter { + let mut args = vec![ + "monero-wallet-rpc".to_string(), + format!("--wallet-dir={}", self.wallet_dir), + format!("--daemon-address={}", self.daemon_address), + format!("--rpc-bind-port={}", RPC_PORT), + "--log-level=4".to_string(), + ]; if self.disable_rpc_login { args.push("--disable-rpc-login".to_string()) @@ -252,40 +257,10 @@ impl WalletArgs { args.push("--confirm-external-bind".to_string()) } - if !self.wallet_dir.is_empty() { - args.push(format!("--wallet-dir {}", self.wallet_dir)); - } - if !self.rpc_bind_ip.is_empty() { - args.push(format!("--rpc-bind-ip {}", self.rpc_bind_ip)); - } - - if self.rpc_bind_port != 0 { - args.push(format!("--rpc-bind-port {}", self.rpc_bind_port)); - } - - if !self.daemon_address.is_empty() { - args.push(format!("--daemon-address {}", self.daemon_address)); + args.push(format!("--rpc-bind-ip={}", self.rpc_bind_ip)); } - if self.log_level != 0 { - args.push(format!("--log-level {}", self.log_level)); - } - - args.join(" ") - } -} - -impl IntoIterator for Args { - type Item = String; - type IntoIter = ::std::vec::IntoIter; - - fn into_iter(self) -> ::IntoIter { - vec![ - "/bin/bash".to_string(), - "-c".to_string(), - format!("{} ", self.image_args.args()), - ] - .into_iter() + args.into_iter() } } diff --git a/monero-harness/src/lib.rs b/monero-harness/src/lib.rs index c667e19e..3c7214af 100644 --- a/monero-harness/src/lib.rs +++ b/monero-harness/src/lib.rs @@ -22,17 +22,15 @@ //! Also provides standalone JSON RPC clients for monerod and monero-wallet-rpc. pub mod image; -use crate::image::{ - MONEROD_DAEMON_CONTAINER_NAME, MONEROD_DEFAULT_NETWORK, MONEROD_RPC_PORT, WALLET_RPC_PORT, -}; -use anyhow::{anyhow, bail, Result}; +use crate::image::{MONEROD_DAEMON_CONTAINER_NAME, MONEROD_DEFAULT_NETWORK, RPC_PORT}; +use anyhow::{anyhow, bail, Context, Result}; use monero_rpc::{ monerod, monerod::MonerodRpc as _, wallet::{self, GetAddress, MoneroWalletRpc as _, Refreshed, Transfer}, }; use std::time::Duration; -use testcontainers::{clients::Cli, core::Port, Container, Docker, RunArgs}; +use testcontainers::{clients::Cli, Container, Docker, RunArgs}; use tokio::time; /// How often we mine a block. @@ -57,14 +55,18 @@ impl<'c> Monero { pub async fn new( cli: &'c Cli, additional_wallets: Vec<&'static str>, - ) -> Result<(Self, Vec>)> { + ) -> Result<( + Self, + Container<'c, Cli, image::Monerod>, + Vec>, + )> { let prefix = format!("{}_", random_prefix()); let monerod_name = format!("{}{}", prefix, MONEROD_DAEMON_CONTAINER_NAME); let network = format!("{}{}", prefix, MONEROD_DEFAULT_NETWORK); tracing::info!("Starting monerod: {}", monerod_name); let (monerod, monerod_container) = Monerod::new(cli, monerod_name, network)?; - let mut containers = vec![monerod_container]; + let mut containers = vec![]; let mut wallets = vec![]; let miner = "miner"; @@ -82,7 +84,7 @@ impl<'c> Monero { containers.push(container); } - Ok((Self { monerod, wallets }, containers)) + Ok((Self { monerod, wallets }, monerod_container, containers)) } pub fn monerod(&self) -> &Monerod { @@ -194,19 +196,15 @@ impl<'c> Monerod { cli: &'c Cli, name: String, network: String, - ) -> Result<(Self, Container<'c, Cli, image::Monero>)> { - let monerod_rpc_port: u16 = - port_check::free_local_port().ok_or_else(|| anyhow!("Could not retrieve free port"))?; - - let image = image::Monero::default(); + ) -> Result<(Self, Container<'c, Cli, image::Monerod>)> { + let image = image::Monerod::default(); let run_args = RunArgs::default() .with_name(name.clone()) - .with_network(network.clone()) - .with_mapped_port(Port { - local: monerod_rpc_port, - internal: MONEROD_RPC_PORT, - }); - let docker = cli.run_with_args(image, run_args); + .with_network(network.clone()); + let container = cli.run_with_args(image, run_args); + let monerod_rpc_port = container + .get_host_port(RPC_PORT) + .context("port not exposed")?; Ok(( Self { @@ -215,7 +213,7 @@ impl<'c> Monerod { network, client: monerod::Client::localhost(monerod_rpc_port)?, }, - docker, + container, )) } @@ -240,23 +238,19 @@ impl<'c> MoneroWalletRpc { name: &str, monerod: &Monerod, prefix: String, - ) -> Result<(Self, Container<'c, Cli, image::Monero>)> { - let wallet_rpc_port: u16 = - port_check::free_local_port().ok_or_else(|| anyhow!("Could not retrieve free port"))?; - - let daemon_address = format!("{}:{}", monerod.name, MONEROD_RPC_PORT); - let image = image::Monero::wallet(&name, daemon_address); + ) -> Result<(Self, Container<'c, Cli, image::MoneroWalletRpc>)> { + let daemon_address = format!("{}:{}", monerod.name, RPC_PORT); + let image = image::MoneroWalletRpc::new(&name, daemon_address); let network = monerod.network.clone(); let run_args = RunArgs::default() // prefix the container name so we can run multiple tests .with_name(format!("{}{}", prefix, name)) - .with_network(network.clone()) - .with_mapped_port(Port { - local: wallet_rpc_port, - internal: WALLET_RPC_PORT, - }); - let docker = cli.run_with_args(image, run_args); + .with_network(network.clone()); + let container = cli.run_with_args(image, run_args); + let wallet_rpc_port = container + .get_host_port(RPC_PORT) + .context("port not exposed")?; // create new wallet let client = wallet::Client::localhost(wallet_rpc_port)?; @@ -272,7 +266,7 @@ impl<'c> MoneroWalletRpc { network, client, }, - docker, + container, )) } diff --git a/monero-harness/tests/monerod.rs b/monero-harness/tests/monerod.rs index 03311072..d218167b 100644 --- a/monero-harness/tests/monerod.rs +++ b/monero-harness/tests/monerod.rs @@ -13,7 +13,7 @@ async fn init_miner_and_mine_to_miner_address() { .set_default(); let tc = Cli::default(); - let (monero, _monerod_container) = Monero::new(&tc, vec![]).await.unwrap(); + let (monero, _monerod_container, _wallet_containers) = Monero::new(&tc, vec![]).await.unwrap(); monero.init_and_start_miner().await.unwrap(); diff --git a/monero-harness/tests/wallet.rs b/monero-harness/tests/wallet.rs index df26d348..b25683ad 100644 --- a/monero-harness/tests/wallet.rs +++ b/monero-harness/tests/wallet.rs @@ -17,7 +17,8 @@ async fn fund_transfer_and_check_tx_key() { let send_to_bob = 5_000_000_000; let tc = Cli::default(); - let (monero, _containers) = Monero::new(&tc, vec!["alice", "bob"]).await.unwrap(); + let (monero, _monerod_container, _wallet_containers) = + Monero::new(&tc, vec!["alice", "bob"]).await.unwrap(); let alice_wallet = monero.wallet("alice").unwrap(); let bob_wallet = monero.wallet("bob").unwrap(); diff --git a/swap/tests/happy_path.rs b/swap/tests/happy_path.rs index 5f1ac5b3..2f50ff14 100644 --- a/swap/tests/happy_path.rs +++ b/swap/tests/happy_path.rs @@ -4,8 +4,6 @@ use harness::SlowCancelConfig; use swap::protocol::{alice, bob}; use tokio::join; -/// Run the following tests with RUST_MIN_STACK=10000000 - #[tokio::test] async fn happy_path() { harness::setup_test(SlowCancelConfig, |mut ctx| async move { diff --git a/swap/tests/harness/mod.rs b/swap/tests/harness/mod.rs index 7acec257..09ef421a 100644 --- a/swap/tests/harness/mod.rs +++ b/swap/tests/harness/mod.rs @@ -150,11 +150,16 @@ async fn init_containers(cli: &Cli) -> (Monero, Containers<'_>) { let electrs = init_electrs_container(&cli, prefix.clone(), bitcoind_name, prefix) .await .expect("could not init electrs"); - let (monero, monerods) = init_monero_container(&cli).await; + let (monero, monerod_container, monero_wallet_rpc_containers) = + Monero::new(&cli, vec![MONERO_WALLET_NAME_ALICE, MONERO_WALLET_NAME_BOB]) + .await + .unwrap(); + (monero, Containers { bitcoind_url, bitcoind, - monerods, + monerod_container, + monero_wallet_rpc_containers, electrs, }) } @@ -189,20 +194,6 @@ async fn init_bitcoind_container( Ok((docker, bitcoind_url.clone())) } -async fn init_monero_container( - cli: &Cli, -) -> ( - Monero, - Vec>, -) { - let (monero, monerods) = - Monero::new(&cli, vec![MONERO_WALLET_NAME_ALICE, MONERO_WALLET_NAME_BOB]) - .await - .unwrap(); - - (monero, monerods) -} - pub async fn init_electrs_container( cli: &Cli, volume: String, @@ -892,7 +883,8 @@ pub async fn mint(node_url: Url, address: bitcoin::Address, amount: bitcoin::Amo struct Containers<'a> { bitcoind_url: Url, bitcoind: Container<'a, Cli, bitcoind::Bitcoind>, - monerods: Vec>, + monerod_container: Container<'a, Cli, image::Monerod>, + monero_wallet_rpc_containers: Vec>, electrs: Container<'a, Cli, electrs::Electrs>, }