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.
This commit is contained in:
Michael Sproul
2023-09-20 01:19:03 +00:00
parent d386a07b0c
commit 4b6cb3db2c
12 changed files with 172 additions and 184 deletions

View File

@@ -10,15 +10,14 @@ use eth2::{
types::{BlockId as CoreBlockId, ForkChoiceNode, StateId as CoreStateId, *},
BeaconNodeHttpClient, Error, StatusCode, Timeouts,
};
use execution_layer::test_utils::TestingBuilder;
use execution_layer::test_utils::DEFAULT_BUILDER_THRESHOLD_WEI;
use execution_layer::test_utils::{
Operation, DEFAULT_BUILDER_PAYLOAD_VALUE_WEI, DEFAULT_MOCK_EL_PAYLOAD_VALUE_WEI,
MockBuilder, Operation, DEFAULT_BUILDER_PAYLOAD_VALUE_WEI, DEFAULT_BUILDER_THRESHOLD_WEI,
DEFAULT_MOCK_EL_PAYLOAD_VALUE_WEI,
};
use futures::stream::{Stream, StreamExt};
use futures::FutureExt;
use http_api::{
test_utils::{create_api_server, create_api_server_on_port, ApiServer},
test_utils::{create_api_server, ApiServer},
BlockId, StateId,
};
use lighthouse_network::{Enr, EnrExt, PeerId};
@@ -73,7 +72,7 @@ struct ApiTester {
network_rx: NetworkReceivers<E>,
local_enr: Enr,
external_peer_id: PeerId,
mock_builder: Option<Arc<TestingBuilder<E>>>,
mock_builder: Option<Arc<MockBuilder<E>>>,
}
struct ApiTesterConfig {
@@ -120,24 +119,28 @@ impl ApiTester {
}
pub async fn new_from_config(config: ApiTesterConfig) -> Self {
// Get a random unused port
let spec = config.spec;
let port = unused_port::unused_tcp4_port().unwrap();
let beacon_url = SensitiveUrl::parse(format!("http://127.0.0.1:{port}").as_str()).unwrap();
let harness = Arc::new(
BeaconChainHarness::builder(MainnetEthSpec)
.spec(spec.clone())
.chain_config(ChainConfig {
reconstruct_historic_states: config.retain_historic_states,
..ChainConfig::default()
})
.logger(logging::test_logger())
.deterministic_keypairs(VALIDATOR_COUNT)
.fresh_ephemeral_store()
.mock_execution_layer_with_builder(beacon_url.clone(), config.builder_threshold)
.build(),
);
let mut harness = BeaconChainHarness::builder(MainnetEthSpec)
.spec(spec.clone())
.chain_config(ChainConfig {
reconstruct_historic_states: config.retain_historic_states,
..ChainConfig::default()
})
.logger(logging::test_logger())
.deterministic_keypairs(VALIDATOR_COUNT)
.fresh_ephemeral_store()
.mock_execution_layer_with_config(config.builder_threshold)
.build();
harness
.mock_execution_layer
.as_ref()
.unwrap()
.server
.execution_block_generator()
.move_to_terminal_block()
.unwrap();
harness.advance_slot();
@@ -245,29 +248,40 @@ impl ApiTester {
let ApiServer {
server,
listening_socket: _,
listening_socket,
network_rx,
local_enr,
external_peer_id,
} = create_api_server_on_port(chain.clone(), &harness.runtime, log, port).await;
} = create_api_server(chain.clone(), &harness.runtime, log).await;
harness.runtime.task_executor.spawn(server, "api_server");
// Late-initalize the mock builder now that the mock execution node and beacon API ports
// have been allocated.
let beacon_api_port = listening_socket.port();
let beacon_url =
SensitiveUrl::parse(format!("http://127.0.0.1:{beacon_api_port}").as_str()).unwrap();
let mock_builder_server = harness.set_mock_builder(beacon_url.clone());
// Start the mock builder service prior to building the chain out.
harness.runtime.task_executor.spawn(
async move {
if let Err(e) = mock_builder_server.await {
panic!("error in mock builder server: {e:?}");
}
},
"mock_builder_server",
);
let mock_builder = harness.mock_builder.clone();
let client = BeaconNodeHttpClient::new(
beacon_url,
Timeouts::set_all(Duration::from_secs(SECONDS_PER_SLOT)),
);
let builder_ref = harness.mock_builder.as_ref().unwrap().clone();
harness.runtime.task_executor.spawn(
async move { builder_ref.run().await },
"mock_builder_server",
);
let mock_builder = harness.mock_builder.clone();
Self {
harness,
harness: Arc::new(harness),
chain,
client,
next_block,
@@ -379,7 +393,6 @@ impl ApiTester {
.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::Value(Uint256::from(
DEFAULT_BUILDER_THRESHOLD_WEI,
)));
@@ -402,7 +415,6 @@ impl ApiTester {
.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::Value(Uint256::from(
DEFAULT_BUILDER_PAYLOAD_VALUE_WEI,
)));
@@ -3275,6 +3287,7 @@ impl ApiTester {
.unwrap()
.get_payload_by_root(&payload.tree_hash_root())
.is_none());
self
}
@@ -3283,7 +3296,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::GasLimit(30_000_000));
let slot = self.chain.slot().unwrap();
@@ -3326,7 +3338,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::FeeRecipient(test_fee_recipient));
let slot = self.chain.slot().unwrap();
@@ -3368,7 +3379,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::ParentHash(invalid_parent_hash));
let slot = self.chain.slot().unwrap();
@@ -3417,7 +3427,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::PrevRandao(invalid_prev_randao));
let slot = self.chain.slot().unwrap();
@@ -3462,7 +3471,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::BlockNumber(invalid_block_number));
let slot = self.chain.slot().unwrap();
@@ -3509,7 +3517,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::Timestamp(invalid_timestamp));
let slot = self.chain.slot().unwrap();
@@ -3549,11 +3556,7 @@ impl ApiTester {
}
pub async fn test_payload_rejects_invalid_signature(self) -> Self {
self.mock_builder
.as_ref()
.unwrap()
.builder
.invalid_signatures();
self.mock_builder.as_ref().unwrap().invalid_signatures();
let slot = self.chain.slot().unwrap();
let epoch = self.chain.epoch().unwrap();
@@ -3831,7 +3834,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::Value(Uint256::from(
DEFAULT_BUILDER_THRESHOLD_WEI - 1,
)));
@@ -3868,7 +3870,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::Value(Uint256::from(
DEFAULT_MOCK_EL_PAYLOAD_VALUE_WEI + 1,
)));
@@ -3905,7 +3906,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::Value(Uint256::from(
DEFAULT_MOCK_EL_PAYLOAD_VALUE_WEI,
)));
@@ -3942,7 +3942,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::Value(Uint256::from(
DEFAULT_MOCK_EL_PAYLOAD_VALUE_WEI - 1,
)));
@@ -3979,7 +3978,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::Value(Uint256::from(
DEFAULT_MOCK_EL_PAYLOAD_VALUE_WEI + 1,
)));
@@ -3996,7 +3994,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::WithdrawalsRoot(withdrawals_root));
let epoch = self.chain.epoch().unwrap();
@@ -4029,7 +4026,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::Value(Uint256::from(
DEFAULT_MOCK_EL_PAYLOAD_VALUE_WEI + 1,
)));
@@ -4037,7 +4033,6 @@ impl ApiTester {
self.mock_builder
.as_ref()
.unwrap()
.builder
.add_operation(Operation::WithdrawalsRoot(Hash256::repeat_byte(0x42)));
let slot = self.chain.slot().unwrap();