From ea10f4b4dff471c1611eef45ce035524a9a7b550 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Fri, 9 Dec 2022 12:42:23 -0500 Subject: [PATCH] fix(core): set `GethInstance` p2p_port in spawn (#1933) * fix(core): set GethInstance p2p_port in spawn * previously, spawning Geth with disabled discovery would not actually set the p2p port in the returned GethInstance. This is because it would only pass the port to the geth command. If the value started as None, it would remain to be None in the GethInstance even though a port was passed to the command. * adds tests for the existence of ports in GethInstance * remove rpc port test * rpc port test doesn't make sense because it will always be set, it is not an Option * use sigterm instead of sigkill * use sigint instead of sigterm * run tests in different directories * prevents flaky behavior because cargo runs tests in parallel, if the default directory were used, each GethInstance would fight for geth's file system lock. * remove nix as a dep * sigkill works for now --- Cargo.lock | 29 ++++++++++++++-- ethers-core/Cargo.toml | 1 + ethers-core/src/utils/geth.rs | 64 ++++++++++++++++++++++++++++++----- 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index db04bdca..2e3a4459 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -666,7 +666,7 @@ dependencies = [ "libc", "log", "matches", - "nix", + "nix 0.13.1", "serde", "thiserror", "wasm-bindgen", @@ -834,7 +834,7 @@ dependencies = [ "cfg-if 1.0.0", "crossbeam-utils", "lazy_static", - "memoffset", + "memoffset 0.6.5", "scopeguard", ] @@ -1414,6 +1414,7 @@ dependencies = [ "hex", "hex-literal", "k256", + "nix 0.26.1", "once_cell", "open-fastrlp", "proc-macro2", @@ -1424,6 +1425,7 @@ dependencies = [ "serde_json", "strum", "syn", + "tempfile", "thiserror", "tiny-keccak", "unicode-xid", @@ -2460,6 +2462,15 @@ dependencies = [ "autocfg", ] +[[package]] +name = "memoffset" +version = "0.7.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5de893c32cde5f383baa4c04c5d6dbdd735cfd4a794b0debdb2bb1b421da5ff4" +dependencies = [ + "autocfg", +] + [[package]] name = "memory_units" version = "0.4.0" @@ -2530,6 +2541,20 @@ dependencies = [ "void", ] +[[package]] +name = "nix" +version = "0.26.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "46a58d1d356c6597d08cde02c2f09d785b09e28711837b1ed667dc652c08a694" +dependencies = [ + "bitflags", + "cfg-if 1.0.0", + "libc", + "memoffset 0.7.1", + "pin-utils", + "static_assertions", +] + [[package]] name = "nu-ansi-term" version = "0.46.0" diff --git a/ethers-core/Cargo.toml b/ethers-core/Cargo.toml index 987c1654..9dd1c927 100644 --- a/ethers-core/Cargo.toml +++ b/ethers-core/Cargo.toml @@ -44,6 +44,7 @@ syn = { version = "1.0.105", optional = true } proc-macro2 = { version = "1.0.47", optional = true } [dev-dependencies] +tempfile = { version = "3.3.0", default-features = false } serde_json = { version = "1.0.64", default-features = false } bincode = { version = "1.3.3", default-features = false } once_cell = { version = "1.16.0" } diff --git a/ethers-core/src/utils/geth.rs b/ethers-core/src/utils/geth.rs index 04921c90..8657c888 100644 --- a/ethers-core/src/utils/geth.rs +++ b/ethers-core/src/utils/geth.rs @@ -331,15 +331,15 @@ impl Geth { } // Dev mode with custom block time - match self.mode { + let p2p_port = match self.mode { GethMode::Dev(DevOptions { block_time }) => { cmd.arg("--dev"); if let Some(block_time) = block_time { cmd.arg("--dev.period").arg(block_time.to_string()); } + None } GethMode::NonDev(PrivateNetOptions { p2p_port, discovery }) => { - // automatically enable and set the p2p port if we are in non-dev mode let port = if let Some(port) = p2p_port { port } else { unused_port() }; cmd.arg("--port").arg(port.to_string()); @@ -347,8 +347,9 @@ impl Geth { if !discovery { cmd.arg("--nodiscover"); } + Some(port) } - } + }; if let Some(chain_id) = self.chain_id { cmd.arg("--networkid").arg(chain_id.to_string()); @@ -400,11 +401,58 @@ impl Geth { child.stderr = Some(reader.into_inner()); - let p2p_port = match self.mode { - GethMode::Dev(_) => None, - GethMode::NonDev(PrivateNetOptions { p2p_port, .. }) => p2p_port, - }; - GethInstance { pid: child, port, ipc: self.ipc_path, data_dir: self.data_dir, p2p_port } } } + +// These tests should use a different datadir for each `Geth` spawned +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn p2p_port() { + let temp_dir = tempfile::tempdir().unwrap(); + let temp_dir_path = temp_dir.path().to_path_buf(); + + // disabling discovery should put the geth instance into non-dev mode, and it should have a + // p2p port. + let geth = Geth::new().disable_discovery().data_dir(temp_dir_path).spawn(); + let p2p_port = geth.p2p_port(); + + drop(geth); + temp_dir.close().unwrap(); + + assert!(p2p_port.is_some()); + } + + #[test] + fn explicit_p2p_port() { + let temp_dir = tempfile::tempdir().unwrap(); + let temp_dir_path = temp_dir.path().to_path_buf(); + + // if a p2p port is explicitly set, it should be used + let geth = Geth::new().p2p_port(1234).data_dir(temp_dir_path).spawn(); + let p2p_port = geth.p2p_port(); + + drop(geth); + temp_dir.close().unwrap(); + + assert_eq!(p2p_port, Some(1234)); + } + + #[test] + fn dev_mode() { + let temp_dir = tempfile::tempdir().unwrap(); + let temp_dir_path = temp_dir.path().to_path_buf(); + + // dev mode should not have a p2p port, and dev should be the default + let geth = Geth::new().data_dir(temp_dir_path).spawn(); + let p2p_port = geth.p2p_port(); + + drop(geth); + temp_dir.close().unwrap(); + + assert!(p2p_port.is_none()); + } +}