Prevent port re-use in HTTP API tests (#4745)

## Issue Addressed CI is plagued by `AddrAlreadyInUse` failures, which are caused by race conditions in allocating free ports. This PR removes all usages of the `unused_port` crate for Lighthouse's HTTP API, in favour of passing `:0` as the listen address. As a result, the listen address isn't known ahead of time and must be read from the listening socket after it binds. This requires tying some self-referential knots, which is a little disruptive, but hopefully doesn't clash too much with Deneb 🤞 There are still a few usages of `unused_tcp4_port` left in cases where we start external processes, like the `watch` Postgres DB, Anvil, Geth, Nethermind, etc. Removing these usages is non-trivial because it's hard to read the port back from an external process after starting it with `--port 0`. We might be able to do something on Linux where we read from `/proc/`, but I'll leave that for future work.
sigp · Sep 20, 2023 · 4b6cb3d · 4b6cb3d
1 parent d386a07
commit 4b6cb3d
Show file tree

Hide file tree

Showing 12 changed files with 169 additions and 181 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/beacon_node/beacon_chain/Cargo.toml b/beacon_node/beacon_chain/Cargo.toml
@@ -65,7 +65,6 @@ sensitive_url = { path = "../../common/sensitive_url" }
 superstruct = "0.5.0"
 hex = "0.4.2"
 exit-future = "0.2.0"
-unused_port = {path = "../../common/unused_port"}
 oneshot_broadcast = { path = "../../common/oneshot_broadcast" }
 
 [[test]]

diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs
@@ -17,8 +17,8 @@ use bls::get_withdrawal_credentials;
 use execution_layer::{
     auth::JwtKey,
     test_utils::{
-        ExecutionBlockGenerator, MockExecutionLayer, TestingBuilder, DEFAULT_JWT_SECRET,
-        DEFAULT_TERMINAL_BLOCK,
+        ExecutionBlockGenerator, MockBuilder, MockBuilderServer, MockExecutionLayer,
+        DEFAULT_JWT_SECRET, DEFAULT_TERMINAL_BLOCK,
     },
     ExecutionLayer,
 };
@@ -167,7 +167,6 @@ pub struct Builder<T: BeaconChainTypes> {
     store_mutator: Option<BoxedMutator<T::EthSpec, T::HotStore, T::ColdStore>>,
     execution_layer: Option<ExecutionLayer<T::EthSpec>>,
     mock_execution_layer: Option<MockExecutionLayer<T::EthSpec>>,
-    mock_builder: Option<TestingBuilder<T::EthSpec>>,
     testing_slot_clock: Option<TestingSlotClock>,
     runtime: TestRuntime,
     log: Logger,
@@ -301,7 +300,6 @@ where
             store_mutator: None,
             execution_layer: None,
             mock_execution_layer: None,
-            mock_builder: None,
             testing_slot_clock: None,
             runtime,
             log,
@@ -433,7 +431,11 @@ where
         self
     }
 
-    pub fn mock_execution_layer(mut self) -> Self {
+    pub fn mock_execution_layer(self) -> Self {
+        self.mock_execution_layer_with_config(None)
+    }
+
+    pub fn mock_execution_layer_with_config(mut self, builder_threshold: Option<u128>) -> Self {
         let spec = self.spec.clone().expect("cannot build without spec");
         let shanghai_time = spec.capella_fork_epoch.map(|epoch| {
             HARNESS_GENESIS_TIME + spec.seconds_per_slot * E::slots_per_epoch() * epoch.as_u64()
@@ -442,55 +444,15 @@ where
             self.runtime.task_executor.clone(),
             DEFAULT_TERMINAL_BLOCK,
             shanghai_time,
-            None,
+            builder_threshold,
             Some(JwtKey::from_slice(&DEFAULT_JWT_SECRET).unwrap()),
             spec,
-            None,
         );
         self.execution_layer = Some(mock.el.clone());
         self.mock_execution_layer = Some(mock);
         self
     }
 
-    pub fn mock_execution_layer_with_builder(
-        mut self,
-        beacon_url: SensitiveUrl,
-        builder_threshold: Option<u128>,
-    ) -> Self {
-        // Get a random unused port
-        let port = unused_port::unused_tcp4_port().unwrap();
-        let builder_url = SensitiveUrl::parse(format!("http://127.0.0.1:{port}").as_str()).unwrap();
-
-        let spec = self.spec.clone().expect("cannot build without spec");
-        let shanghai_time = spec.capella_fork_epoch.map(|epoch| {
-            HARNESS_GENESIS_TIME + spec.seconds_per_slot * E::slots_per_epoch() * epoch.as_u64()
-        });
-        let mock_el = MockExecutionLayer::new(
-            self.runtime.task_executor.clone(),
-            DEFAULT_TERMINAL_BLOCK,
-            shanghai_time,
-            builder_threshold,
-            Some(JwtKey::from_slice(&DEFAULT_JWT_SECRET).unwrap()),
-            spec.clone(),
-            Some(builder_url.clone()),
-        )
-        .move_to_terminal_block();
-
-        let mock_el_url = SensitiveUrl::parse(mock_el.server.url().as_str()).unwrap();
-
-        self.mock_builder = Some(TestingBuilder::new(
-            mock_el_url,
-            builder_url,
-            beacon_url,
-            spec,
-            self.runtime.task_executor.clone(),
-        ));
-        self.execution_layer = Some(mock_el.el.clone());
-        self.mock_execution_layer = Some(mock_el);
-
-        self
-    }
-
     /// Instruct the mock execution engine to always return a "valid" response to any payload it is
     /// asked to execute.
     pub fn mock_execution_layer_all_payloads_valid(self) -> Self {
@@ -572,7 +534,7 @@ where
             shutdown_receiver: Arc::new(Mutex::new(shutdown_receiver)),
             runtime: self.runtime,
             mock_execution_layer: self.mock_execution_layer,
-            mock_builder: self.mock_builder.map(Arc::new),
+            mock_builder: None,
             rng: make_rng(),
         }
     }
@@ -597,7 +559,7 @@ pub struct BeaconChainHarness<T: BeaconChainTypes> {
     pub runtime: TestRuntime,
 
     pub mock_execution_layer: Option<MockExecutionLayer<T::EthSpec>>,
-    pub mock_builder: Option<Arc<TestingBuilder<T::EthSpec>>>,
+    pub mock_builder: Option<Arc<MockBuilder<T::EthSpec>>>,
 
     pub rng: Mutex<StdRng>,
 }
@@ -633,6 +595,49 @@ where
             .execution_block_generator()
     }
 
+    pub fn set_mock_builder(&mut self, beacon_url: SensitiveUrl) -> MockBuilderServer {
+        let mock_el = self
+            .mock_execution_layer
+            .as_ref()
+            .expect("harness was not built with mock execution layer");
+
+        let mock_el_url = SensitiveUrl::parse(mock_el.server.url().as_str()).unwrap();
+
+        // Create the builder, listening on a free port.
+        let (mock_builder, mock_builder_server) = MockBuilder::new_for_testing(
+            mock_el_url,
+            beacon_url,
+            self.spec.clone(),
+            self.runtime.task_executor.clone(),
+        );
+
+        // Set the builder URL in the execution layer now that its port is known.
+        let builder_listen_addr = mock_builder_server.local_addr();
+        let port = builder_listen_addr.port();
+        mock_el
+            .el
+            .set_builder_url(
+                SensitiveUrl::parse(format!("http://127.0.0.1:{port}").as_str()).unwrap(),
+                None,
+            )
+            .unwrap();
+
+        self.mock_builder = Some(Arc::new(mock_builder));
+
+        // Sanity check.
+        let el_builder = self
+            .chain
+            .execution_layer
+            .as_ref()
+            .unwrap()
+            .builder()
+            .unwrap();
+        let mock_el_builder = mock_el.el.builder().unwrap();
+        assert!(Arc::ptr_eq(&el_builder, &mock_el_builder));
+
+        mock_builder_server
+    }
+
     pub fn get_all_validators(&self) -> Vec<usize> {
         (0..self.validator_keypairs.len()).collect()
     }

diff --git a/beacon_node/execution_layer/Cargo.toml b/beacon_node/execution_layer/Cargo.toml
@@ -42,6 +42,8 @@ ethers-core = "1.0.2"
 builder_client = { path = "../builder_client" }
 fork_choice = { path = "../../consensus/fork_choice" }
 mev-rs = { git = "https://github.com/ralexstokes/mev-rs", rev = "216657016d5c0889b505857c89ae42c7aa2764af" }
+axum = "0.6"
+hyper = "0.14"
 ethereum-consensus = { git = "https://github.com/ralexstokes/ethereum-consensus", rev = "e380108" }
 ssz_rs = "0.9.0"
 tokio-stream = { version = "0.1.9", features = [ "sync" ] }
@@ -51,3 +53,4 @@ hash256-std-hasher = "0.15.2"
 triehash = "0.8.4"
 hash-db = "0.15.2"
 pretty_reqwest_error = { path = "../../common/pretty_reqwest_error" }
+arc-swap = "1.6.0"
diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs
@@ -5,6 +5,7 @@
 //! deposit-contract functionality that the `beacon_node/eth1` crate already provides.
 
 use crate::payload_cache::PayloadCache;
+use arc_swap::ArcSwapOption;
 use auth::{strip_prefix, Auth, JwtKey};
 use builder_client::BuilderHttpClient;
 pub use engine_api::EngineCapabilities;
@@ -209,7 +210,7 @@ pub enum FailedCondition {
 
 struct Inner<E: EthSpec> {
     engine: Arc<Engine>,
-    builder: Option<BuilderHttpClient>,
+    builder: ArcSwapOption<BuilderHttpClient>,
     execution_engine_forkchoice_lock: Mutex<()>,
     suggested_fee_recipient: Option<Address>,
     proposer_preparation_data: Mutex<HashMap<u64, ProposerPreparationDataEntry>>,
@@ -324,25 +325,9 @@ impl<T: EthSpec> ExecutionLayer<T> {
             Engine::new(api, executor.clone(), &log)
         };
 
-        let builder = builder_url
-            .map(|url| {
-                let builder_client = BuilderHttpClient::new(url.clone(), builder_user_agent)
-                    .map_err(Error::Builder)?;
-
-                info!(
-                    log,
-                    "Using external block builder";
-                    "builder_url" => ?url,
-                    "builder_profit_threshold" => builder_profit_threshold,
-                    "local_user_agent" => builder_client.get_user_agent(),
-                );
-                Ok::<_, Error>(builder_client)
-            })
-            .transpose()?;
-
         let inner = Inner {
             engine: Arc::new(engine),
-            builder,
+            builder: ArcSwapOption::empty(),
             execution_engine_forkchoice_lock: <_>::default(),
             suggested_fee_recipient,
             proposer_preparation_data: Mutex::new(HashMap::new()),
@@ -356,19 +341,45 @@ impl<T: EthSpec> ExecutionLayer<T> {
             last_new_payload_errored: RwLock::new(false),
         };
 
-        Ok(Self {
+        let el = Self {
             inner: Arc::new(inner),
-        })
+        };
+
+        if let Some(builder_url) = builder_url {
+            el.set_builder_url(builder_url, builder_user_agent)?;
+        }
+
+        Ok(el)
     }
-}
 
-impl<T: EthSpec> ExecutionLayer<T> {
     fn engine(&self) -> &Arc<Engine> {
         &self.inner.engine
     }
 
-    pub fn builder(&self) -> &Option<BuilderHttpClient> {
-        &self.inner.builder
+    pub fn builder(&self) -> Option<Arc<BuilderHttpClient>> {
+        self.inner.builder.load_full()
+    }
+
+    /// Set the builder URL after initialization.
+    ///
+    /// This is useful for breaking circular dependencies between mock ELs and mock builders in
+    /// tests.
+    pub fn set_builder_url(
+        &self,
+        builder_url: SensitiveUrl,
+        builder_user_agent: Option<String>,
+    ) -> Result<(), Error> {
+        let builder_client = BuilderHttpClient::new(builder_url.clone(), builder_user_agent)
+            .map_err(Error::Builder)?;
+        info!(
+            self.log(),
+            "Using external block builder";
+            "builder_url" => ?builder_url,
+            "builder_profit_threshold" => self.inner.builder_profit_threshold.as_u128(),
+            "local_user_agent" => builder_client.get_user_agent(),
+        );
+        self.inner.builder.swap(Some(Arc::new(builder_client)));
+        Ok(())
     }
 
     /// Cache a full payload, keyed on the `tree_hash_root` of the payload