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

@@ -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

Cargo.lock

@@ -2167,7 +2167,6 @@ dependencies = [
  "anyhow",
  "futures",
  "monero-rpc",
- "port_check",
  "rand 0.7.3",
  "spectral",
  "testcontainers 0.12.0",

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"

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 {
+pub struct Monerod {
-    args: Args,
+    args: MonerodArgs,
-    entrypoint: Option<String>,
-    wait_for_message: String,
 }
 
-impl Image for Monero {
+impl Image for Monerod {
-    type Args = Args;
+    type Args = MonerodArgs;
     type EnvVars = HashMap<String, String>;
     type Volumes = HashMap<String, String>;
     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) -> <Self as Image>::Args {
@@ -56,69 +51,80 @@ impl Image for Monero {
     }
 
     fn with_args(self, args: <Self as Image>::Args) -> Self {
-        Monero { args, ..self }
+        Self { args }
-    }
-
-    fn with_entrypoint(self, entrypoint: &Self::EntryPoint) -> Self {
-        Self {
-            entrypoint: Some(entrypoint.to_string()),
-            ..self
-        }
     }
 
     fn entrypoint(&self) -> Option<String> {
-        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(),
-        }
-    }
-}
-
-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(Clone, Debug)]
-pub struct Args {
-    image_args: ImageArgs,
-}
-
-impl Default for Args {
     fn default() -> Self {
         Self {
-            image_args: ImageArgs::MonerodArgs(MonerodArgs::default()),
+            args: MonerodArgs::default(),
         }
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Debug)]
-pub enum ImageArgs {
+pub struct MoneroWalletRpc {
-    MonerodArgs(MonerodArgs),
+    args: MoneroWalletRpcArgs,
-    WalletArgs(WalletArgs),
 }
 
-impl ImageArgs {
+impl Image for MoneroWalletRpc {
-    fn args(&self) -> String {
+    type Args = MoneroWalletRpcArgs;
-        match self {
+    type EnvVars = HashMap<String, String>;
-            ImageArgs::MonerodArgs(monerod_args) => monerod_args.args(),
+    type Volumes = HashMap<String, String>;
-            ImageArgs::WalletArgs(wallet_args) => wallet_args.args(),
+    type EntryPoint = str;
+
+    fn descriptor(&self) -> String {
+        "xmrto/monero:v0.17.2.0".to_owned()
+    }
+
+    fn wait_until_ready<D: Docker>(&self, container: &Container<'_, D, Self>) {
+        container
+            .logs()
+            .stdout
+            .wait_for_message("JOINING all threads")
+            .unwrap();
+    }
+
+    fn args(&self) -> <Self as Image>::Args {
+        self.args.clone()
+    }
+
+    fn volumes(&self) -> Self::Volumes {
+        HashMap::new()
+    }
+
+    fn env_vars(&self) -> Self::EnvVars {
+        HashMap::new()
+    }
+
+    fn with_args(self, args: <Self as Image>::Args) -> Self {
+        Self { args }
+    }
+
+    fn entrypoint(&self) -> Option<String> {
+        Some("".to_owned()) // an empty entrypoint disables the entrypoint
+                            // script and gives us full control
+    }
+}
+
+impl Default for MoneroWalletRpc {
+    fn default() -> Self {
+        Self {
+            args: MoneroWalletRpcArgs::default(),
+        }
+    }
+}
+
+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 {
+impl IntoIterator for MonerodArgs {
-    // Return monerod args as is single string so we can pass it to bash.
+    type Item = String;
-    fn args(&self) -> String {
+    type IntoIter = ::std::vec::IntoIter<String>;
-        let mut args = vec!["monerod".to_string()];
+
+    fn into_iter(self) -> <Self as IntoIterator>::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));
+            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.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.into_iter()
-            args.push(format!("--log-level {}", self.log_level));
-        }
-
-        args.join(" ")
     }
 }
 
-impl WalletArgs {
+#[derive(Debug, Clone)]
-    pub fn new(wallet_name: &str, daemon_address: String, rpc_port: u16) -> Self {
+pub struct MoneroWalletRpcArgs {
-        WalletArgs {
+    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 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,
+        }
     }
 }
 
-    // Return monero-wallet-rpc args as is single string so we can pass it to bash.
+impl IntoIterator for MoneroWalletRpcArgs {
-    fn args(&self) -> String {
+    type Item = String;
-        let mut args = vec!["monero-wallet-rpc".to_string()];
+    type IntoIter = ::std::vec::IntoIter<String>;
+
+    fn into_iter(self) -> <Self as IntoIterator>::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));
+            args.push(format!("--rpc-bind-ip={}", self.rpc_bind_ip));
         }
 
-        if self.rpc_bind_port != 0 {
+        args.into_iter()
-            args.push(format!("--rpc-bind-port {}", self.rpc_bind_port));
-        }
-
-        if !self.daemon_address.is_empty() {
-            args.push(format!("--daemon-address {}", self.daemon_address));
-        }
-
-        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<String>;
-
-    fn into_iter(self) -> <Self as IntoIterator>::IntoIter {
-        vec![
-            "/bin/bash".to_string(),
-            "-c".to_string(),
-            format!("{} ", self.image_args.args()),
-        ]
-        .into_iter()
     }
 }

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::{
+use crate::image::{MONEROD_DAEMON_CONTAINER_NAME, MONEROD_DEFAULT_NETWORK, RPC_PORT};
-    MONEROD_DAEMON_CONTAINER_NAME, MONEROD_DEFAULT_NETWORK, MONEROD_RPC_PORT, WALLET_RPC_PORT,
+use anyhow::{anyhow, bail, Context, Result};
-};
-use anyhow::{anyhow, bail, 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<Container<'c, Cli, image::Monero>>)> {
+    ) -> Result<(
+        Self,
+        Container<'c, Cli, image::Monerod>,
+        Vec<Container<'c, Cli, image::MoneroWalletRpc>>,
+    )> {
         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>)> {
+    ) -> Result<(Self, Container<'c, Cli, image::Monerod>)> {
-        let monerod_rpc_port: u16 =
+        let image = image::Monerod::default();
-            port_check::free_local_port().ok_or_else(|| anyhow!("Could not retrieve free port"))?;
-
-        let image = image::Monero::default();
         let run_args = RunArgs::default()
             .with_name(name.clone())
-            .with_network(network.clone())
+            .with_network(network.clone());
-            .with_mapped_port(Port {
+        let container = cli.run_with_args(image, run_args);
-                local: monerod_rpc_port,
+        let monerod_rpc_port = container
-                internal: MONEROD_RPC_PORT,
+            .get_host_port(RPC_PORT)
-            });
+            .context("port not exposed")?;
-        let docker = cli.run_with_args(image, run_args);
 
         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>)> {
+    ) -> Result<(Self, Container<'c, Cli, image::MoneroWalletRpc>)> {
-        let wallet_rpc_port: u16 =
+        let daemon_address = format!("{}:{}", monerod.name, RPC_PORT);
-            port_check::free_local_port().ok_or_else(|| anyhow!("Could not retrieve free port"))?;
+        let image = image::MoneroWalletRpc::new(&name, daemon_address);
-
-        let daemon_address = format!("{}:{}", monerod.name, MONEROD_RPC_PORT);
-        let image = image::Monero::wallet(&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_network(network.clone());
-            .with_mapped_port(Port {
+        let container = cli.run_with_args(image, run_args);
-                local: wallet_rpc_port,
+        let wallet_rpc_port = container
-                internal: WALLET_RPC_PORT,
+            .get_host_port(RPC_PORT)
-            });
+            .context("port not exposed")?;
-        let docker = cli.run_with_args(image, run_args);
 
         // create new wallet
         let client = wallet::Client::localhost(wallet_rpc_port)?;
@@ -272,7 +266,7 @@ impl<'c> MoneroWalletRpc {
                 network,
                 client,
             },
-            docker,
+            container,
         ))
     }
 

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();
 

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();
 

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 {

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<Container<'_, Cli, monero_harness::image::Monero>>,
-) {
-    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<Container<'a, Cli, image::Monero>>,
+    monerod_container: Container<'a, Cli, image::Monerod>,
+    monero_wallet_rpc_containers: Vec<Container<'a, Cli, image::MoneroWalletRpc>>,
     electrs: Container<'a, Cli, electrs::Electrs>,
 }