[Stability] Only use open ports for test runs (#2230)

* fix combined network test determinism

* fix test prerequisites

* fix lints

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;

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 }

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<dyn Fn(u64) -> 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;

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<TYPES: NodeType> TestableNetworkingImplementation<TYPES> for WebServerNetwo
     ) -> Box<dyn Fn(u64) -> Self + 'static> {
         let (server_shutdown_sender, server_shutdown) = oneshot();
         let sender = Arc::new(server_shutdown_sender);
-        let port = random::<u16>();
+
+        // 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

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<Boxed<(PeerId, StreamMuxerBox)>, NetworkError> {
+pub async fn gen_transport(identity: Keypair) -> Result<BoxedTransport, NetworkError> {
     let quic_transport = {
         let mut config = quic::Config::new(&identity);
         config.handshake_timeout = std::time::Duration::from_secs(20);

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<NetworkDef> = {

libp2p-networking/src/network/node/handle.rs

@@ -88,7 +88,7 @@ impl<S: Default + Debug> NetworkNodeHandle<S> {
     /// constructs a new node listening on `known_addr`
     #[instrument]
     pub async fn new(config: NetworkNodeConfig, id: usize) -> Result<Self, NetworkNodeHandleError> {
-        //`randomly assigned port
+        // randomly assigned port
         let listen_addr = config
             .bound_addr
             .clone()

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