Fix port acquisition in BackgroundAnvil (#708)

* Fix BackgroundDevnet: remove clippy suppression; fix unresponsiveness msg

* Reuse port acquisition logic from background_devnet

* Expand use of anyhow::anyhow! macro

* Fix old typos: hardat -> hardhat

Author: FabijanC

crates/starknet-devnet-core/src/messaging/ethereum.rs

@@ -269,7 +269,7 @@ impl EthereumMessaging {
         &self,
         cancellation_delay_seconds: U256,
     ) -> DevnetResult<Address> {
-        // Default value from anvil and hardat multiplied by 20.
+        // Default value from anvil and hardhat multiplied by 20.
         let gas_price: U256 = 20000000000_u128.into();
 
         let contract = abigen::MockStarknetMessaging::deploy(

tests/integration/common/background_anvil.rs

@@ -6,9 +6,9 @@ use ethers::prelude::*;
 use ethers::providers::{Http, Provider};
 use ethers::types::Address;
 use k256::ecdsa::SigningKey;
-use rand::Rng;
 use reqwest::StatusCode;
 
+use super::background_server::get_acquired_port;
 use super::constants::{DEFAULT_ETH_ACCOUNT_PRIVATE_KEY, HOST};
 use super::errors::TestError;
 use super::safe_child::SafeChild;
@@ -30,31 +30,29 @@ mod abigen {
 }
 
 impl BackgroundAnvil {
-    /// To avoid TOCTOU or binding issues, we try random ports and try to start
-    /// Anvil on this port (as Anvil will actually open the socket right after binding).
-    #[allow(dead_code)] // dead_code needed to pass clippy
     pub(crate) async fn spawn() -> Result<Self, TestError> {
         BackgroundAnvil::spawn_with_additional_args(&[]).await
     }
 
+    /// Spawns an instance at random port. Assumes CLI args in `args` don't contain `--port`.
     pub(crate) async fn spawn_with_additional_args(args: &[&str]) -> Result<Self, TestError> {
-        // Relies on `background_devnet::BackgroundDevnet` starting its check from smaller values
-        // (1025). Relies on the probability of M simultaneously spawned Anvils occupying
-        // different ports being fairly big (N*(N-1)*...*(N-M+1) / N**M; N=65_000-20_000+1)
-        let port = rand::thread_rng().gen_range(20_000..=65_000);
-
         let process = Command::new("anvil")
             .arg("--port")
-            .arg(port.to_string())
+            .arg("0")
             .arg("--silent")
             .args(args)
             .spawn()
             .expect("Could not start background Anvil");
-        let safe_process = SafeChild { process };
+        let mut safe_process = SafeChild { process };
+
+        let sleep_time = time::Duration::from_millis(500);
+        let max_retries = 30;
+        let port = get_acquired_port(&mut safe_process, sleep_time, max_retries)
+            .await
+            .map_err(|e| TestError::AnvilNotStartable(format!("Cannot determine port: {e:?}")))?;
 
         let url = format!("http://{HOST}:{port}");
         let client = reqwest::Client::new();
-        let max_retries = 30;
         for _ in 0..max_retries {
             if let Ok(anvil_block_rsp) = send_dummy_request(&client, &url).await {
                 assert_eq!(anvil_block_rsp.status(), StatusCode::OK);
@@ -65,34 +63,30 @@ impl BackgroundAnvil {
                 return Ok(Self { process: safe_process, url, provider, provider_signer });
             }
 
-            tokio::time::sleep(time::Duration::from_millis(500)).await;
+            tokio::time::sleep(sleep_time).await;
         }
 
-        Err(TestError::AnvilNotStartable)
+        Err(TestError::AnvilNotStartable("Not responsive for too long".into()))
     }
 
     pub async fn deploy_l1l2_contract(
         &self,
         messaging_address: Address,
     ) -> Result<Address, TestError> {
-        let contract = abigen::L1L2Example::deploy(
-            self.provider_signer.clone(),
-            messaging_address,
-        )
+        // Required by the new version of anvil, as default is no longer accepted.
+        // We use here the default value from anvil and hardhat multiplied by 2.
+        let gas_price = 2_000_000_000;
+        let contract = abigen::L1L2Example::deploy(self.provider_signer.clone(), messaging_address)
             .map_err(|e| {
                 TestError::EthersError(format!(
                     "Error formatting messaging contract deploy request: {e}"
                 ))
             })?
-        // Required by the new version of anvil, as default is no longer accepted.
-        // We use here the default value from anvil and hardat multiplied by 2.
-            .gas_price(2000000000)
+            .gas_price(gas_price)
             .send()
             .await
             .map_err(|e| {
-                TestError::EthersError(format!(
-                    "Error deploying messaging contract: {e}"
-                ))
+                TestError::EthersError(format!("Error deploying messaging contract: {e}"))
             })?;
 
         Ok(contract.address())

tests/integration/common/background_devnet.rs

@@ -3,6 +3,7 @@ use std::fmt::LowerHex;
 use std::process::{Command, Stdio};
 use std::time;
 
+use anyhow::anyhow;
 use lazy_static::lazy_static;
 use reqwest::{Client, StatusCode};
 use serde_json::json;
@@ -23,6 +24,7 @@ use super::constants::{
 use super::errors::{RpcError, TestError};
 use super::reqwest_client::{PostReqwestSender, ReqwestClient};
 use super::utils::{to_hex_felt, FeeUnit, ImpersonationAction};
+use crate::common::background_server::get_acquired_port;
 use crate::common::constants::{
     DEVNET_EXECUTABLE_BINARY_PATH, DEVNET_MANIFEST_PATH, ETH_ERC20_CONTRACT_ADDRESS,
 };
@@ -63,31 +65,6 @@ fn get_devnet_command() -> Command {
     }
 }
 
-async fn get_acquired_port(
-    process: &mut SafeChild,
-    sleep_time: time::Duration,
-    max_retries: usize,
-) -> Result<u16, anyhow::Error> {
-    let pid = process.id();
-    for _ in 0..max_retries {
-        if let Ok(ports) = listeners::get_ports_by_pid(pid) {
-            if ports.len() == 1 {
-                return Ok(ports.into_iter().next().unwrap());
-            }
-        }
-
-        if let Ok(Some(status)) = process.process.try_wait() {
-            return Err(anyhow::Error::msg(format!(
-                "Background Devnet process exited with status {status}"
-            )));
-        }
-
-        tokio::time::sleep(sleep_time).await;
-    }
-
-    Err(anyhow::Error::msg(format!("Could not identify a unique port used by PID {pid}")))
-}
-
 async fn wait_for_successful_response(
     client: &Client,
     healthcheck_url: &str,
@@ -98,7 +75,7 @@ async fn wait_for_successful_response(
         if let Ok(alive_resp) = client.get(healthcheck_url).send().await {
             let status = alive_resp.status();
             if status != StatusCode::OK {
-                return Err(anyhow::Error::msg(format!("Server responded with: {status}")));
+                return Err(anyhow!("Server responded with: {status}"));
             }
 
             return Ok(());
@@ -107,18 +84,15 @@ async fn wait_for_successful_response(
         tokio::time::sleep(sleep_time).await;
     }
 
-    Err(anyhow::Error::msg("Server not reachable at {}"))
+    Err(anyhow!("Not responsive: {healthcheck_url}"))
 }
 
 impl BackgroundDevnet {
-    /// Ensures the background instance spawns at a free port, checks at most `MAX_RETRIES`
-    /// times
-    #[allow(dead_code)] // dead_code needed to pass clippy
     pub(crate) async fn spawn() -> Result<Self, TestError> {
         BackgroundDevnet::spawn_with_additional_args(&[]).await
     }
 
-    pub async fn spawn_forkable_devnet() -> Result<BackgroundDevnet, anyhow::Error> {
+    pub(crate) async fn spawn_forkable_devnet() -> Result<BackgroundDevnet, anyhow::Error> {
         let args = ["--state-archive-capacity", "full"];
         let devnet = BackgroundDevnet::spawn_with_additional_args(&args).await?;
         Ok(devnet)
@@ -180,7 +154,7 @@ impl BackgroundDevnet {
         println!("Spawned background devnet at {devnet_url}");
 
         let devnet_rpc_url = Url::parse(format!("{devnet_url}{RPC_PATH}").as_str())?;
-        Ok(BackgroundDevnet {
+        Ok(Self {
             reqwest_client: ReqwestClient::new(devnet_url.clone(), client),
             json_rpc_client: JsonRpcClient::new(HttpTransport::new(devnet_rpc_url.clone())),
             port,

tests/integration/common/background_server.rs

@@ -0,0 +1,26 @@
+use std::time;
+
+use super::safe_child::SafeChild;
+
+pub(crate) async fn get_acquired_port(
+    process: &mut SafeChild,
+    sleep_time: time::Duration,
+    max_retries: usize,
+) -> Result<u16, anyhow::Error> {
+    let pid = process.id();
+    for _ in 0..max_retries {
+        if let Ok(ports) = listeners::get_ports_by_pid(pid) {
+            if ports.len() == 1 {
+                return Ok(ports.into_iter().next().unwrap());
+            }
+        }
+
+        if let Ok(Some(status)) = process.process.try_wait() {
+            return Err(anyhow::anyhow!("Background server process exited with status {status}"));
+        }
+
+        tokio::time::sleep(sleep_time).await;
+    }
+
+    Err(anyhow::anyhow!("Could not identify a unique port used by PID {pid}"))
+}

tests/integration/common/errors.rs

@@ -14,8 +14,8 @@ pub enum TestError {
     #[error("Could not start Devnet. Make sure you built it with `cargo build --release`: {0}")]
     DevnetNotStartable(String),
 
-    #[error("Could not start Anvil")]
-    AnvilNotStartable,
+    #[error("Could not start Anvil: {0}")]
+    AnvilNotStartable(String),
 
     #[error("Ethers error: {0}")]
     EthersError(String),

tests/integration/common/mod.rs

@@ -1,6 +1,7 @@
 #![cfg(test)]
 pub mod background_anvil;
 pub mod background_devnet;
+mod background_server;
 pub mod constants;
 pub mod errors;
 pub mod fees;

tests/integration/common/safe_child.rs

@@ -11,8 +11,7 @@ impl SafeChild {
     }
 }
 
-/// By implementing Drop, we ensure there are no zombie background Devnet processes
-/// in case of an early test failure
+/// By implementing Drop, we ensure there are no zombie processes in case of early test failure
 impl Drop for SafeChild {
     fn drop(&mut self) {
         self.process.kill().expect("Cannot kill process");

tests/integration/common/utils.rs

@@ -300,8 +300,7 @@ pub async fn deploy_argent_account(
 
     let account_address = deployment.address();
     devnet.mint(account_address, 1e18 as u128).await;
-    let deployment_result =
-        deployment.send().await.map_err(|e| anyhow::Error::msg(format!("{e:?}")))?;
+    let deployment_result = deployment.send().await.map_err(|e| anyhow::anyhow!("{e:?}"))?;
 
     Ok((deployment_result, signer))
 }