From 2221373e43eec2f6c94f8492db4b7862940f0c2d Mon Sep 17 00:00:00 2001 From: rob-maron <132852777+rob-maron@users.noreply.github.com> Date: Thu, 14 Dec 2023 15:23:33 -0500 Subject: [PATCH] [Stability] Only use open ports for test runs (#2230) * fix combined network test determinism * fix test prerequisites * fix lints --- constants/src/lib.rs | 4 ---- hotshot/Cargo.toml | 1 + .../src/traits/networking/libp2p_network.rs | 10 +++++--- .../traits/networking/web_server_network.rs | 6 +++-- libp2p-networking/src/network/mod.rs | 9 +++++--- libp2p-networking/src/network/node.rs | 8 +++---- libp2p-networking/src/network/node/handle.rs | 2 +- testing/tests/combined_network.rs | 23 ------------------- 8 files changed, 23 insertions(+), 40 deletions(-) diff --git a/constants/src/lib.rs b/constants/src/lib.rs index 279ad62890..6ca29a2de0 100644 --- a/constants/src/lib.rs +++ b/constants/src/lib.rs @@ -17,7 +17,3 @@ pub const COMBINED_NETWORK_MIN_PRIMARY_FAILURES: u64 = 5; /// the number of messages to send over the secondary network before re-attempting the (presumed down) primary network pub const COMBINED_NETWORK_PRIMARY_CHECK_INTERVAL: u64 = 5; - -/// the amount of time to wait for async_std tests to spin down the Libp2p listeners -/// and allow future tests to run -pub const ASYNC_STD_LIBP2P_LISTENER_SPINDOWN_TIME: u64 = 4; diff --git a/hotshot/Cargo.toml b/hotshot/Cargo.toml index 74bd1eab9d..39105e6b64 100644 --- a/hotshot/Cargo.toml +++ b/hotshot/Cargo.toml @@ -114,6 +114,7 @@ surf-disco = { workspace = true } time = { workspace = true } local-ip-address = "0.5.6" dyn-clone = { git = "https://github.com/dtolnay/dyn-clone", tag = "1.0.16" } +portpicker = "0.1.1" tracing = { workspace = true } typenum = { workspace = true } diff --git a/hotshot/src/traits/networking/libp2p_network.rs b/hotshot/src/traits/networking/libp2p_network.rs index a30ccb577a..caf2d72d3e 100644 --- a/hotshot/src/traits/networking/libp2p_network.rs +++ b/hotshot/src/traits/networking/libp2p_network.rs @@ -142,7 +142,7 @@ where fn generator( expected_node_count: usize, num_bootstrap: usize, - network_id: usize, + _network_id: usize, da_committee_size: usize, _is_da: bool, ) -> Box Self + 'static> { @@ -173,9 +173,13 @@ where node_id, node_id < num_bootstrap as u64 ); + + // pick a free, unused UDP port for testing + let port = portpicker::pick_unused_port().expect("Could not find an open port"); + let addr = - // Multiaddr::from_str(&format!("/ip4/127.0.0.1/udp/0/quic-v1")).unwrap(); - Multiaddr::from_str(&format!("/ip4/127.0.0.1/udp/{}{}/quic-v1", 5000 + node_id, network_id)).unwrap(); + Multiaddr::from_str(&format!("/ip4/127.0.0.1/udp/{port}/quic-v1")).unwrap(); + // We assign node's public key and stake value rather than read from config file since it's a test let privkey = TYPES::SignatureKey::generated_from_seed_indexed([0u8; 32], node_id).1; diff --git a/hotshot/src/traits/networking/web_server_network.rs b/hotshot/src/traits/networking/web_server_network.rs index c7270ac0ae..21f9dcdec0 100644 --- a/hotshot/src/traits/networking/web_server_network.rs +++ b/hotshot/src/traits/networking/web_server_network.rs @@ -25,7 +25,6 @@ use hotshot_types::{ }, }; use hotshot_web_server::{self, config}; -use rand::random; use serde::{Deserialize, Serialize}; use surf_disco::Url; @@ -1130,7 +1129,10 @@ impl TestableNetworkingImplementation for WebServerNetwo ) -> Box Self + 'static> { let (server_shutdown_sender, server_shutdown) = oneshot(); let sender = Arc::new(server_shutdown_sender); - let port = random::(); + + // pick random, unused port + let port = portpicker::pick_unused_port().expect("Could not find an open port"); + let url = Url::parse(format!("http://localhost:{port}").as_str()).unwrap(); info!("Launching web server on port {port}"); // Start web server diff --git a/libp2p-networking/src/network/mod.rs b/libp2p-networking/src/network/mod.rs index 2d35fdca5e..05134a34bf 100644 --- a/libp2p-networking/src/network/mod.rs +++ b/libp2p-networking/src/network/mod.rs @@ -195,13 +195,16 @@ pub fn gen_multiaddr(port: u16) -> Multiaddr { build_multiaddr!(Ip4([0, 0, 0, 0]), Udp(port), QuicV1) } +/// `BoxedTransport` is a type alias for a boxed tuple containing a `PeerId` and a `StreamMuxerBox`. +/// +/// This type is used to represent a transport in the libp2p network framework. The `PeerId` is a unique identifier for each peer in the network, and the `StreamMuxerBox` is a type of multiplexer that can handle multiple substreams over a single connection. +type BoxedTransport = Boxed<(PeerId, StreamMuxerBox)>; + /// Generate authenticated transport /// # Errors /// could not sign the quic key with `identity` #[instrument(skip(identity))] -pub async fn gen_transport( - identity: Keypair, -) -> Result, NetworkError> { +pub async fn gen_transport(identity: Keypair) -> Result { let quic_transport = { let mut config = quic::Config::new(&identity); config.handshake_timeout = std::time::Duration::from_secs(20); diff --git a/libp2p-networking/src/network/node.rs b/libp2p-networking/src/network/node.rs index 1853781025..e4c44ca401 100644 --- a/libp2p-networking/src/network/node.rs +++ b/libp2p-networking/src/network/node.rs @@ -13,7 +13,8 @@ pub use self::{ use super::{ behaviours::gossip::GossipBehaviour, error::{GossipsubBuildSnafu, GossipsubConfigSnafu, NetworkError, TransportSnafu}, - gen_transport, ClientRequest, NetworkDef, NetworkEvent, NetworkEventInternal, NetworkNodeType, + gen_transport, BoxedTransport, ClientRequest, NetworkDef, NetworkEvent, NetworkEventInternal, + NetworkNodeType, }; use crate::network::{ @@ -35,7 +36,6 @@ use futures::{select, FutureExt, StreamExt}; use hotshot_constants::KAD_DEFAULT_REPUB_INTERVAL_SEC; use libp2p::core::transport::ListenerId; use libp2p::{ - core::{muxing::StreamMuxerBox, transport::Boxed}, gossipsub::{ Behaviour as Gossipsub, ConfigBuilder as GossipsubConfigBuilder, Message as GossipsubMessage, MessageAuthenticity, MessageId, Topic, ValidationMode, @@ -150,7 +150,7 @@ impl NetworkNode { /// Currently: /// * Generates a random key pair and associated [`PeerId`] /// * Launches a hopefully production ready transport: - /// QUIC v1 (RFC 9000) + DNS + Websocket + XX auth + /// QUIC v1 (RFC 9000) + DNS /// * Generates a connection to the "broadcast" topic /// * Creates a swarm to manage peers and events #[instrument] @@ -163,7 +163,7 @@ impl NetworkNode { }; let peer_id = PeerId::from(identity.public()); debug!(?peer_id); - let transport: Boxed<(PeerId, StreamMuxerBox)> = gen_transport(identity.clone()).await?; + let transport: BoxedTransport = gen_transport(identity.clone()).await?; trace!("Launched network transport"); // Generate the swarm let mut swarm: Swarm = { diff --git a/libp2p-networking/src/network/node/handle.rs b/libp2p-networking/src/network/node/handle.rs index 5c50d61cb7..ae5a77a0c5 100644 --- a/libp2p-networking/src/network/node/handle.rs +++ b/libp2p-networking/src/network/node/handle.rs @@ -88,7 +88,7 @@ impl NetworkNodeHandle { /// constructs a new node listening on `known_addr` #[instrument] pub async fn new(config: NetworkNodeConfig, id: usize) -> Result { - //`randomly assigned port + // randomly assigned port let listen_addr = config .bound_addr .clone() diff --git a/testing/tests/combined_network.rs b/testing/tests/combined_network.rs index b78f48a39e..663d6a32b8 100644 --- a/testing/tests/combined_network.rs +++ b/testing/tests/combined_network.rs @@ -1,8 +1,5 @@ use std::time::Duration; -#[cfg(async_executor_impl = "async-std")] -use hotshot_constants::ASYNC_STD_LIBP2P_LISTENER_SPINDOWN_TIME; - use hotshot_testing::{ completion_task::{CompletionTaskDescription, TimeBasedCompletionTaskDescription}, node_types::{CombinedImpl, TestTypes}, @@ -48,10 +45,6 @@ async fn test_combined_network() { .launch() .run_test() .await; - - // async_std needs time to spin down the handler - #[cfg(async_executor_impl = "async-std")] - async_std::task::sleep(Duration::from_secs(ASYNC_STD_LIBP2P_LISTENER_SPINDOWN_TIME)).await; } // A run where the webserver crashes part-way through @@ -102,10 +95,6 @@ async fn test_combined_network_webserver_crash() { .launch() .run_test() .await; - - // async_std needs time to spin down the handler - #[cfg(async_executor_impl = "async-std")] - async_std::task::sleep(Duration::from_secs(ASYNC_STD_LIBP2P_LISTENER_SPINDOWN_TIME)).await; } // A run where the webserver crashes partway through @@ -162,10 +151,6 @@ async fn test_combined_network_reup() { .launch() .run_test() .await; - - // async_std needs time to spin down the handler - #[cfg(async_executor_impl = "async-std")] - async_std::task::sleep(Duration::from_secs(ASYNC_STD_LIBP2P_LISTENER_SPINDOWN_TIME)).await; } // A run where half of the nodes disconnect from the webserver @@ -216,10 +201,6 @@ async fn test_combined_network_half_dc() { .launch() .run_test() .await; - - // async_std needs time to spin down the handler - #[cfg(async_executor_impl = "async-std")] - async_std::task::sleep(Duration::from_secs(ASYNC_STD_LIBP2P_LISTENER_SPINDOWN_TIME)).await; } fn generate_random_node_changes( @@ -293,8 +274,4 @@ async fn test_stress_combined_network_fuzzy() { .launch() .run_test() .await; - - // async_std needs time to spin down the handler - #[cfg(async_executor_impl = "async-std")] - async_std::task::sleep(Duration::from_secs(ASYNC_STD_LIBP2P_LISTENER_SPINDOWN_TIME)).await; }