From c5cd0d9b3f49f3fef76dc3be75a2d773b75bf9b2 Mon Sep 17 00:00:00 2001 From: GeemoCandama Date: Tue, 18 Oct 2022 04:02:07 +0000 Subject: [PATCH] add execution-timeout-multiplier flag to optionally increase timeouts (#3631) ## Issue Addressed Add flag to lengthen execution layer timeouts Which issue # does this PR address? #3607 ## Proposed Changes Added execution-timeout-multiplier flag and a cli test to ensure the execution layer config has the multiplier set correctly. Please list or describe the changes introduced by this PR. Add execution_timeout_multiplier to the execution layer config as Option and pass the u32 to HttpJsonRpc. ## Additional Info Not certain that this is the best way to implement it so I'd appreciate any feedback. Please provide any additional information. For example, future considerations or information useful for reviewers. --- beacon_node/eth1/src/service.rs | 10 ++- beacon_node/eth1/tests/test.rs | 9 ++- .../execution_layer/src/engine_api/http.rs | 63 ++++++++++++++----- beacon_node/execution_layer/src/lib.rs | 5 +- beacon_node/src/cli.rs | 9 ++- beacon_node/src/config.rs | 3 + lighthouse/tests/beacon_node.rs | 16 +++++ 7 files changed, 91 insertions(+), 24 deletions(-) diff --git a/beacon_node/eth1/src/service.rs b/beacon_node/eth1/src/service.rs index fae6eef9c2..c6b87e88e3 100644 --- a/beacon_node/eth1/src/service.rs +++ b/beacon_node/eth1/src/service.rs @@ -290,6 +290,7 @@ pub struct Config { pub max_blocks_per_update: Option, /// If set to true, the eth1 caches are wiped clean when the eth1 service starts. pub purge_cache: bool, + pub execution_timeout_multiplier: u32, } impl Config { @@ -347,6 +348,7 @@ impl Default for Config { max_log_requests_per_update: Some(5_000), max_blocks_per_update: Some(8_192), purge_cache: false, + execution_timeout_multiplier: 1, } } } @@ -361,11 +363,13 @@ pub fn endpoint_from_config(config: &Config) -> Result { } => { let auth = Auth::new_with_path(jwt_path, jwt_id, jwt_version) .map_err(|e| format!("Failed to initialize jwt auth: {:?}", e))?; - HttpJsonRpc::new_with_auth(endpoint, auth) + HttpJsonRpc::new_with_auth(endpoint, auth, Some(config.execution_timeout_multiplier)) + .map_err(|e| format!("Failed to create eth1 json rpc client: {:?}", e)) + } + Eth1Endpoint::NoAuth(endpoint) => { + HttpJsonRpc::new(endpoint, Some(config.execution_timeout_multiplier)) .map_err(|e| format!("Failed to create eth1 json rpc client: {:?}", e)) } - Eth1Endpoint::NoAuth(endpoint) => HttpJsonRpc::new(endpoint) - .map_err(|e| format!("Failed to create eth1 json rpc client: {:?}", e)), } } diff --git a/beacon_node/eth1/tests/test.rs b/beacon_node/eth1/tests/test.rs index 9f81f91e19..7e58f07e24 100644 --- a/beacon_node/eth1/tests/test.rs +++ b/beacon_node/eth1/tests/test.rs @@ -493,7 +493,8 @@ mod deposit_tree { let mut deposit_roots = vec![]; let mut deposit_counts = vec![]; - let client = HttpJsonRpc::new(SensitiveUrl::parse(ð1.endpoint()).unwrap()).unwrap(); + let client = + HttpJsonRpc::new(SensitiveUrl::parse(ð1.endpoint()).unwrap(), None).unwrap(); // Perform deposits to the smart contract, recording it's state along the way. for deposit in &deposits { @@ -597,7 +598,8 @@ mod http { .expect("should start eth1 environment"); let deposit_contract = ð1.deposit_contract; let web3 = eth1.web3(); - let client = HttpJsonRpc::new(SensitiveUrl::parse(ð1.endpoint()).unwrap()).unwrap(); + let client = + HttpJsonRpc::new(SensitiveUrl::parse(ð1.endpoint()).unwrap(), None).unwrap(); let block_number = get_block_number(&web3).await; let logs = blocking_deposit_logs(&client, ð1, 0..block_number).await; @@ -711,7 +713,8 @@ mod fast { MainnetEthSpec::default_spec(), ) .unwrap(); - let client = HttpJsonRpc::new(SensitiveUrl::parse(ð1.endpoint()).unwrap()).unwrap(); + let client = + HttpJsonRpc::new(SensitiveUrl::parse(ð1.endpoint()).unwrap(), None).unwrap(); let n = 10; let deposits: Vec<_> = (0..n).map(|_| random_deposit_data()).collect(); for deposit in &deposits { diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 0f848a7716..be68c37b06 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -518,22 +518,32 @@ pub mod deposit_methods { pub struct HttpJsonRpc { pub client: Client, pub url: SensitiveUrl, + pub execution_timeout_multiplier: u32, auth: Option, } impl HttpJsonRpc { - pub fn new(url: SensitiveUrl) -> Result { + pub fn new( + url: SensitiveUrl, + execution_timeout_multiplier: Option, + ) -> Result { Ok(Self { client: Client::builder().build()?, url, + execution_timeout_multiplier: execution_timeout_multiplier.unwrap_or(1), auth: None, }) } - pub fn new_with_auth(url: SensitiveUrl, auth: Auth) -> Result { + pub fn new_with_auth( + url: SensitiveUrl, + auth: Auth, + execution_timeout_multiplier: Option, + ) -> Result { Ok(Self { client: Client::builder().build()?, url, + execution_timeout_multiplier: execution_timeout_multiplier.unwrap_or(1), auth: Some(auth), }) } @@ -590,7 +600,11 @@ impl std::fmt::Display for HttpJsonRpc { impl HttpJsonRpc { pub async fn upcheck(&self) -> Result<(), Error> { let result: serde_json::Value = self - .rpc_request(ETH_SYNCING, json!([]), ETH_SYNCING_TIMEOUT) + .rpc_request( + ETH_SYNCING, + json!([]), + ETH_SYNCING_TIMEOUT * self.execution_timeout_multiplier, + ) .await?; /* @@ -614,7 +628,7 @@ impl HttpJsonRpc { self.rpc_request( ETH_GET_BLOCK_BY_NUMBER, params, - ETH_GET_BLOCK_BY_NUMBER_TIMEOUT, + ETH_GET_BLOCK_BY_NUMBER_TIMEOUT * self.execution_timeout_multiplier, ) .await } @@ -625,8 +639,12 @@ impl HttpJsonRpc { ) -> Result, Error> { let params = json!([block_hash, RETURN_FULL_TRANSACTION_OBJECTS]); - self.rpc_request(ETH_GET_BLOCK_BY_HASH, params, ETH_GET_BLOCK_BY_HASH_TIMEOUT) - .await + self.rpc_request( + ETH_GET_BLOCK_BY_HASH, + params, + ETH_GET_BLOCK_BY_HASH_TIMEOUT * self.execution_timeout_multiplier, + ) + .await } pub async fn get_block_by_hash_with_txns( @@ -634,8 +652,12 @@ impl HttpJsonRpc { block_hash: ExecutionBlockHash, ) -> Result>, Error> { let params = json!([block_hash, true]); - self.rpc_request(ETH_GET_BLOCK_BY_HASH, params, ETH_GET_BLOCK_BY_HASH_TIMEOUT) - .await + self.rpc_request( + ETH_GET_BLOCK_BY_HASH, + params, + ETH_GET_BLOCK_BY_HASH_TIMEOUT * self.execution_timeout_multiplier, + ) + .await } pub async fn new_payload_v1( @@ -645,7 +667,11 @@ impl HttpJsonRpc { let params = json!([JsonExecutionPayloadV1::from(execution_payload)]); let response: JsonPayloadStatusV1 = self - .rpc_request(ENGINE_NEW_PAYLOAD_V1, params, ENGINE_NEW_PAYLOAD_TIMEOUT) + .rpc_request( + ENGINE_NEW_PAYLOAD_V1, + params, + ENGINE_NEW_PAYLOAD_TIMEOUT * self.execution_timeout_multiplier, + ) .await?; Ok(response.into()) @@ -658,7 +684,11 @@ impl HttpJsonRpc { let params = json!([JsonPayloadIdRequest::from(payload_id)]); let response: JsonExecutionPayloadV1 = self - .rpc_request(ENGINE_GET_PAYLOAD_V1, params, ENGINE_GET_PAYLOAD_TIMEOUT) + .rpc_request( + ENGINE_GET_PAYLOAD_V1, + params, + ENGINE_GET_PAYLOAD_TIMEOUT * self.execution_timeout_multiplier, + ) .await?; Ok(response.into()) @@ -678,7 +708,7 @@ impl HttpJsonRpc { .rpc_request( ENGINE_FORKCHOICE_UPDATED_V1, params, - ENGINE_FORKCHOICE_UPDATED_TIMEOUT, + ENGINE_FORKCHOICE_UPDATED_TIMEOUT * self.execution_timeout_multiplier, ) .await?; @@ -695,7 +725,8 @@ impl HttpJsonRpc { .rpc_request( ENGINE_EXCHANGE_TRANSITION_CONFIGURATION_V1, params, - ENGINE_EXCHANGE_TRANSITION_CONFIGURATION_V1_TIMEOUT, + ENGINE_EXCHANGE_TRANSITION_CONFIGURATION_V1_TIMEOUT + * self.execution_timeout_multiplier, ) .await?; @@ -732,13 +763,13 @@ mod test { let echo_auth = Auth::new(JwtKey::from_slice(&DEFAULT_JWT_SECRET).unwrap(), None, None); ( - Arc::new(HttpJsonRpc::new_with_auth(rpc_url, rpc_auth).unwrap()), - Arc::new(HttpJsonRpc::new_with_auth(echo_url, echo_auth).unwrap()), + Arc::new(HttpJsonRpc::new_with_auth(rpc_url, rpc_auth, None).unwrap()), + Arc::new(HttpJsonRpc::new_with_auth(echo_url, echo_auth, None).unwrap()), ) } else { ( - Arc::new(HttpJsonRpc::new(rpc_url).unwrap()), - Arc::new(HttpJsonRpc::new(echo_url).unwrap()), + Arc::new(HttpJsonRpc::new(rpc_url, None).unwrap()), + Arc::new(HttpJsonRpc::new(echo_url, None).unwrap()), ) }; diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 68071ee9b1..f222f28c33 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -159,6 +159,7 @@ pub struct Config { pub default_datadir: PathBuf, /// The minimum value of an external payload for it to be considered in a proposal. pub builder_profit_threshold: u128, + pub execution_timeout_multiplier: Option, } /// Provides access to one execution engine and provides a neat interface for consumption by the @@ -180,6 +181,7 @@ impl ExecutionLayer { jwt_version, default_datadir, builder_profit_threshold, + execution_timeout_multiplier, } = config; if urls.len() > 1 { @@ -224,7 +226,8 @@ impl ExecutionLayer { let engine: Engine = { let auth = Auth::new(jwt_key, jwt_id, jwt_version); debug!(log, "Loaded execution endpoint"; "endpoint" => %execution_url, "jwt_path" => ?secret_file.as_path()); - let api = HttpJsonRpc::new_with_auth(execution_url, auth).map_err(Error::ApiError)?; + let api = HttpJsonRpc::new_with_auth(execution_url, auth, execution_timeout_multiplier) + .map_err(Error::ApiError)?; Engine::new(api, executor.clone(), &log) }; diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 1e51849876..0b7518b957 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -503,7 +503,14 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .requires("execution-endpoint") .takes_value(true) ) - + .arg( + Arg::with_name("execution-timeout-multiplier") + .long("execution-timeout-multiplier") + .value_name("NUM") + .help("Unsigned integer to multiply the default execution timeouts by.") + .default_value("1") + .takes_value(true) + ) /* * Database purging and compaction. */ diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index ecd4d736a6..7666134b41 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -335,6 +335,9 @@ pub fn get_config( el_config.default_datadir = client_config.data_dir.clone(); el_config.builder_profit_threshold = clap_utils::parse_required(cli_args, "builder-profit-threshold")?; + let execution_timeout_multiplier = + clap_utils::parse_required(cli_args, "execution-timeout-multiplier")?; + el_config.execution_timeout_multiplier = Some(execution_timeout_multiplier); // If `--execution-endpoint` is provided, we should ignore any `--eth1-endpoints` values and // use `--execution-endpoint` instead. Also, log a deprecation warning. diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index a00fd7a822..34041a82c8 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -407,6 +407,22 @@ fn run_execution_jwt_secret_key_is_persisted() { }); } #[test] +fn execution_timeout_multiplier_flag() { + let dir = TempDir::new().expect("Unable to create temporary directory"); + CommandLineTest::new() + .flag("execution-endpoint", Some("http://meow.cats")) + .flag( + "execution-jwt", + dir.path().join("jwt-file").as_os_str().to_str(), + ) + .flag("execution-timeout-multiplier", Some("3")) + .run_with_zero_port() + .with_config(|config| { + let config = config.execution_layer.as_ref().unwrap(); + assert_eq!(config.execution_timeout_multiplier, Some(3)); + }); +} +#[test] fn merge_execution_endpoints_flag() { run_merge_execution_endpoints_flag_test("execution-endpoints") }