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

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

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" }

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