From 6079450381d686e831e3a9551a2399aaa0b4bdfd Mon Sep 17 00:00:00 2001 From: Itay Tsabary Date: Tue, 14 Jan 2025 16:49:44 +0200 Subject: [PATCH 1/3] chore(starknet_infra_utils): remove get_available_socket commit-id:0e96c675 --- Cargo.lock | 1 + crates/starknet_infra_utils/Cargo.toml | 2 +- crates/starknet_infra_utils/src/test_utils.rs | 17 ++------------ crates/starknet_sequencer_infra/Cargo.toml | 1 + .../remote_component_client_server_test.rs | 23 ++++++++++++------- 5 files changed, 20 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6598bbc812..6dddb823c9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11138,6 +11138,7 @@ dependencies = [ "assert_matches", "async-trait", "hyper 0.14.32", + "once_cell", "papyrus_config", "pretty_assertions", "rstest", diff --git a/crates/starknet_infra_utils/Cargo.toml b/crates/starknet_infra_utils/Cargo.toml index 704b98b876..de9fa0b1f1 100644 --- a/crates/starknet_infra_utils/Cargo.toml +++ b/crates/starknet_infra_utils/Cargo.toml @@ -7,7 +7,7 @@ license-file.workspace = true description = "Infrastructure utility." [features] -testing = ["tokio/net"] +testing = [] [lints] workspace = true diff --git a/crates/starknet_infra_utils/src/test_utils.rs b/crates/starknet_infra_utils/src/test_utils.rs index 00337b2ec7..f1a8e19152 100644 --- a/crates/starknet_infra_utils/src/test_utils.rs +++ b/crates/starknet_infra_utils/src/test_utils.rs @@ -1,6 +1,5 @@ use std::net::{IpAddr, Ipv4Addr, SocketAddr}; -use tokio::net::TcpListener; const PORTS_PER_INSTANCE: u16 = 60; pub const MAX_NUMBER_OF_INSTANCES_PER_TEST: u16 = 10; @@ -22,6 +21,7 @@ pub enum TestIdentifier { EndToEndFlowTest, MempoolSendsTxToOtherPeerTest, MempoolReceivesTxFromOtherPeerTest, + InfraUnitTests, } impl From for u16 { @@ -31,6 +31,7 @@ impl From for u16 { TestIdentifier::EndToEndFlowTest => 1, TestIdentifier::MempoolSendsTxToOtherPeerTest => 2, TestIdentifier::MempoolReceivesTxFromOtherPeerTest => 3, + TestIdentifier::InfraUnitTests => 4, } } } @@ -81,17 +82,3 @@ impl AvailablePorts { SocketAddr::new(IpAddr::from(Ipv4Addr::LOCALHOST), self.get_next_port()) } } - -/// Returns a unique IP address and port for testing purposes. -/// Tests run in parallel, so servers (like RPC or web) running on separate tests must have -/// different ports, otherwise the server will fail with "address already in use". -pub async fn get_available_socket() -> SocketAddr { - // Dynamically select port. - // First, set the port to 0 (dynamic port). - TcpListener::bind("127.0.0.1:0") - .await - .expect("Failed to bind to address") - // Then, resolve to the actual selected port. - .local_addr() - .expect("Failed to get local address") -} diff --git a/crates/starknet_sequencer_infra/Cargo.toml b/crates/starknet_sequencer_infra/Cargo.toml index d45bd7d25a..687e66511d 100644 --- a/crates/starknet_sequencer_infra/Cargo.toml +++ b/crates/starknet_sequencer_infra/Cargo.toml @@ -29,6 +29,7 @@ validator.workspace = true [dev-dependencies] assert_matches.workspace = true +once_cell.workspace = true pretty_assertions.workspace = true starknet-types-core.workspace = true starknet_infra_utils = { workspace = true, features = ["testing"] } diff --git a/crates/starknet_sequencer_infra/src/tests/remote_component_client_server_test.rs b/crates/starknet_sequencer_infra/src/tests/remote_component_client_server_test.rs index 6b3e31b312..c69efd10b4 100644 --- a/crates/starknet_sequencer_infra/src/tests/remote_component_client_server_test.rs +++ b/crates/starknet_sequencer_infra/src/tests/remote_component_client_server_test.rs @@ -7,10 +7,11 @@ use hyper::body::to_bytes; use hyper::header::CONTENT_TYPE; use hyper::service::{make_service_fn, service_fn}; use hyper::{Body, Client, Request, Response, Server, StatusCode, Uri}; +use once_cell::sync::Lazy; use rstest::rstest; use serde::de::DeserializeOwned; use serde::Serialize; -use starknet_infra_utils::test_utils::get_available_socket; +use starknet_infra_utils::test_utils::{AvailablePorts, TestIdentifier}; use starknet_types_core::felt::Felt; use tokio::sync::mpsc::channel; use tokio::sync::Mutex; @@ -64,6 +65,12 @@ const DESERIALIZE_REQ_ERROR_MESSAGE: &str = "Could not deserialize client reques const DESERIALIZE_RES_ERROR_MESSAGE: &str = "Could not deserialize server response"; const VALID_VALUE_A: ValueA = Felt::ONE; +// Define the shared fixture +static AVAILABLE_PORTS: Lazy>> = Lazy::new(|| { + let available_ports = AvailablePorts::new(TestIdentifier::InfraUnitTests.into(), 0); + Arc::new(Mutex::new(available_ports)) +}); + #[async_trait] impl ComponentAClientTrait for RemoteComponentClient { async fn a_get_value(&self) -> ResultA { @@ -116,7 +123,7 @@ async fn create_client_and_faulty_server(body: T) -> ComponentAClient where T: Serialize + DeserializeOwned + Debug + Send + Sync + 'static + Clone, { - let socket = get_available_socket().await; + let socket = AVAILABLE_PORTS.lock().await.get_next_local_host_socket(); task::spawn(async move { // TODO (lev, itay): Exam/change the bounds of the T for the hadler function. async fn handler( @@ -193,8 +200,8 @@ async fn setup_for_tests(setup_value: ValueB, a_socket: SocketAddr, b_socket: So #[tokio::test] async fn test_proper_setup() { let setup_value: ValueB = Felt::from(90); - let a_socket = get_available_socket().await; - let b_socket = get_available_socket().await; + let a_socket = AVAILABLE_PORTS.lock().await.get_next_local_host_socket(); + let b_socket = AVAILABLE_PORTS.lock().await.get_next_local_host_socket(); setup_for_tests(setup_value, a_socket, b_socket).await; let a_client_config = RemoteClientConfig::default(); @@ -207,8 +214,8 @@ async fn test_proper_setup() { #[tokio::test] async fn test_faulty_client_setup() { - let a_socket = get_available_socket().await; - let b_socket = get_available_socket().await; + let a_socket = AVAILABLE_PORTS.lock().await.get_next_local_host_socket(); + let b_socket = AVAILABLE_PORTS.lock().await.get_next_local_host_socket(); // Todo(uriel): Find a better way to pass expected value to the setup // 123 is some arbitrary value, we don't check it anyway. setup_for_tests(Felt::from(123), a_socket, b_socket).await; @@ -242,7 +249,7 @@ async fn test_faulty_client_setup() { #[tokio::test] async fn test_unconnected_server() { - let socket = get_available_socket().await; + let socket = AVAILABLE_PORTS.lock().await.get_next_local_host_socket(); let client_config = RemoteClientConfig::default(); let client = ComponentAClient::new(client_config, socket); let expected_error_contained_keywords = ["Connection refused"]; @@ -271,7 +278,7 @@ async fn test_faulty_server( #[tokio::test] async fn test_retry_request() { - let socket = get_available_socket().await; + let socket = AVAILABLE_PORTS.lock().await.get_next_local_host_socket(); // Spawn a server that responses with OK every other request. task::spawn(async move { let should_send_ok = Arc::new(Mutex::new(false)); From 5fa2e4d2a172fd0b8e6fc1ed25d365e62ef5909a Mon Sep 17 00:00:00 2001 From: Itay Tsabary Date: Wed, 15 Jan 2025 15:27:40 +0200 Subject: [PATCH 2/3] chore(papyrus_common): reallocate and remove redundant functions commit-id:53a51006 --- crates/papyrus_common/src/lib.rs | 1 - crates/papyrus_common/src/tcp.rs | 24 ------------------- .../src/bin/run_simulation.rs | 12 +++++++++- 3 files changed, 11 insertions(+), 26 deletions(-) delete mode 100644 crates/papyrus_common/src/tcp.rs diff --git a/crates/papyrus_common/src/lib.rs b/crates/papyrus_common/src/lib.rs index 3fc0c90bea..12e6eeeab7 100644 --- a/crates/papyrus_common/src/lib.rs +++ b/crates/papyrus_common/src/lib.rs @@ -8,7 +8,6 @@ pub mod pending_classes; pub mod python_json; pub mod state; pub mod storage_query; -pub mod tcp; pub(crate) fn usize_into_felt(u: usize) -> Felt { u128::try_from(u).expect("Expect at most 128 bits").into() diff --git a/crates/papyrus_common/src/tcp.rs b/crates/papyrus_common/src/tcp.rs deleted file mode 100644 index 072f07cf1c..0000000000 --- a/crates/papyrus_common/src/tcp.rs +++ /dev/null @@ -1,24 +0,0 @@ -use std::net::TcpListener; - -pub fn find_free_port() -> u16 { - // The socket is automatically closed when the function exits. - // The port may still be available when accessed, but this is not guaranteed. - // TODO(Asmaa): find a reliable way to ensure the port stays free. - let listener = TcpListener::bind("0.0.0.0:0").expect("Failed to bind"); - listener.local_addr().expect("Failed to get local address").port() -} - -pub fn find_n_free_ports(n: usize) -> Vec { - // The socket is automatically closed when the function exits. - // The port may still be available when accessed, but this is not guaranteed. - // TODO(Asmaa): find a reliable way to ensure the port stays free. - let listeners = - Vec::from_iter((0..n).map(|_| TcpListener::bind("0.0.0.0:0").expect("Failed to bind"))); - - let mut ports = Vec::with_capacity(n); - for listener in listeners { - let port = listener.local_addr().expect("Failed to get local address").port(); - ports.push(port); - } - ports -} diff --git a/crates/sequencing/papyrus_consensus/src/bin/run_simulation.rs b/crates/sequencing/papyrus_consensus/src/bin/run_simulation.rs index 65f32c7780..ded914a552 100644 --- a/crates/sequencing/papyrus_consensus/src/bin/run_simulation.rs +++ b/crates/sequencing/papyrus_consensus/src/bin/run_simulation.rs @@ -4,6 +4,7 @@ //! uses the `run_consensus` binary which is able to simulate network issues for consensus messages. use std::collections::HashSet; use std::fs::{self, File}; +use std::net::TcpListener; use std::os::unix::process::CommandExt; use std::process::Command; use std::str::FromStr; @@ -13,7 +14,6 @@ use clap::Parser; use fs2::FileExt; use lazy_static::lazy_static; use nix::unistd::Pid; -use papyrus_common::tcp::find_free_port; use papyrus_protobuf::consensus::DEFAULT_VALIDATOR_ID; use tokio::process::Command as TokioCommand; @@ -185,6 +185,16 @@ impl Drop for LockDir { } } +// WARNING: This is not a reliable way to obtain a free port, most notably failing when multiple +// concurrent instances rely on this mechanism. +fn find_free_port() -> u16 { + // The socket is automatically closed when the function exits. + // The port may still be available when accessed, but this is not guaranteed. + // TODO(Asmaa): find a reliable way to ensure the port stays free. + let listener = TcpListener::bind("0.0.0.0:0").expect("Failed to bind"); + listener.local_addr().expect("Failed to get local address").port() +} + fn parse_duration(s: &str) -> Result { let secs = u64::from_str(s)?; Ok(Duration::from_secs(secs)) From e5436d339c38b151880c3314d416e73f8fc8ccca Mon Sep 17 00:00:00 2001 From: Itay Tsabary Date: Wed, 15 Jan 2025 15:27:58 +0200 Subject: [PATCH 3/3] wip commit-id:70204a8b --- crates/sequencing/papyrus_consensus/Cargo.toml | 4 ++++ crates/starknet_infra_utils/src/test_utils.rs | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/sequencing/papyrus_consensus/Cargo.toml b/crates/sequencing/papyrus_consensus/Cargo.toml index d31172b915..1b35ca8b8e 100644 --- a/crates/sequencing/papyrus_consensus/Cargo.toml +++ b/crates/sequencing/papyrus_consensus/Cargo.toml @@ -40,3 +40,7 @@ test-case.workspace = true [lints] workspace = true + +[[bin]] +name = "run_simulation" +path = "src/bin/run_simulation.rs" diff --git a/crates/starknet_infra_utils/src/test_utils.rs b/crates/starknet_infra_utils/src/test_utils.rs index f1a8e19152..21b71c868a 100644 --- a/crates/starknet_infra_utils/src/test_utils.rs +++ b/crates/starknet_infra_utils/src/test_utils.rs @@ -1,6 +1,5 @@ use std::net::{IpAddr, Ipv4Addr, SocketAddr}; - const PORTS_PER_INSTANCE: u16 = 60; pub const MAX_NUMBER_OF_INSTANCES_PER_TEST: u16 = 10; const MAX_NUMBER_OF_TESTS: u16 = 10;