From 0cfceba1bf33307008a288b7e8d2efcf60519e70 Mon Sep 17 00:00:00 2001 From: Mark Mackey Date: Mon, 10 Jul 2023 16:11:01 -0500 Subject: [PATCH] Addressed #4487 Add override threshold flag Added tests for Override Threshold Flag Override default shown in decimal --- beacon_node/execution_layer/src/engine_api.rs | 28 ++++ .../src/engine_api/json_structures.rs | 3 + beacon_node/execution_layer/src/lib.rs | 134 +++++++++++++++--- .../src/test_utils/handle_rpc.rs | 1 + .../src/test_utils/mock_builder.rs | 21 ++- beacon_node/src/cli.rs | 18 +++ beacon_node/src/config.rs | 2 + lighthouse/tests/beacon_node.rs | 32 +++++ 8 files changed, 212 insertions(+), 27 deletions(-) diff --git a/beacon_node/execution_layer/src/engine_api.rs b/beacon_node/execution_layer/src/engine_api.rs index 9dce3c0477c..7b463de6e26 100644 --- a/beacon_node/execution_layer/src/engine_api.rs +++ b/beacon_node/execution_layer/src/engine_api.rs @@ -386,6 +386,34 @@ pub struct GetPayloadResponse { pub block_value: Uint256, #[superstruct(only(Deneb))] pub blobs_bundle: BlobsBundleV1, + #[superstruct(only(Deneb), partial_getter(copy))] + pub should_override_builder: bool, +} + +impl GetPayloadResponse { + pub fn fee_recipient(&self) -> Address { + match self { + GetPayloadResponse::Merge(inner) => inner.execution_payload.fee_recipient, + GetPayloadResponse::Capella(inner) => inner.execution_payload.fee_recipient, + GetPayloadResponse::Deneb(inner) => inner.execution_payload.fee_recipient, + } + } + + pub fn block_hash(&self) -> ExecutionBlockHash { + match self { + GetPayloadResponse::Merge(inner) => inner.execution_payload.block_hash, + GetPayloadResponse::Capella(inner) => inner.execution_payload.block_hash, + GetPayloadResponse::Deneb(inner) => inner.execution_payload.block_hash, + } + } + + pub fn block_number(&self) -> u64 { + match self { + GetPayloadResponse::Merge(inner) => inner.execution_payload.block_number, + GetPayloadResponse::Capella(inner) => inner.execution_payload.block_number, + GetPayloadResponse::Deneb(inner) => inner.execution_payload.block_number, + } + } } impl<'a, T: EthSpec> From> for ExecutionPayloadRef<'a, T> { diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index d94e7078764..e46e2133d0b 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -298,6 +298,8 @@ pub struct JsonGetPayloadResponse { pub block_value: Uint256, #[superstruct(only(V3))] pub blobs_bundle: JsonBlobsBundleV1, + #[superstruct(only(V3))] + pub should_override_builder: bool, } impl From> for GetPayloadResponse { @@ -320,6 +322,7 @@ impl From> for GetPayloadResponse { execution_payload: response.execution_payload.into(), block_value: response.block_value, blobs_bundle: response.blobs_bundle.into(), + should_override_builder: response.should_override_builder, }) } } diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index c513606dcbf..7e39d29d15a 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -300,6 +300,7 @@ struct Inner { builder_profit_threshold: Uint256, log: Logger, always_prefer_builder_payload: bool, + ignore_builder_override_suggestion_threshold: f32, /// Track whether the last `newPayload` call errored. /// /// This is used *only* in the informational sync status endpoint, so that a VC using this @@ -330,6 +331,7 @@ pub struct Config { pub builder_profit_threshold: u128, pub execution_timeout_multiplier: Option, pub always_prefer_builder_payload: bool, + pub ignore_builder_override_suggestion_threshold: f32, } /// Provides access to one execution engine and provides a neat interface for consumption by the @@ -339,6 +341,40 @@ pub struct ExecutionLayer { inner: Arc>, } +/// This function will return the percentage difference between 2 U256 values, using `base_value` +/// as the denominator. It is accurate to 7 decimal places which is about the precision of +/// an f32. +/// +/// If some error is encountered in the calculation, None will be returned. +fn percentage_difference_u256(base_value: Uint256, comparison_value: Uint256) -> Option { + if base_value == Uint256::zero() { + return None; + } + // this is the total supply of ETH in WEI + let max_value = Uint256::from(12u8) * Uint256::exp10(25); + if base_value > max_value || comparison_value > max_value { + return None; + } + + // Now we should be able to calculate the difference without division by zero or overflow + const PRECISION: usize = 7; + let precision_factor = Uint256::exp10(PRECISION); + let scaled_difference = if base_value <= comparison_value { + (comparison_value - base_value) * precision_factor + } else { + (base_value - comparison_value) * precision_factor + }; + let scaled_proportion = scaled_difference / base_value; + // max value of scaled difference is 1.2 * 10^33, well below the max value of a u128 / f64 / f32 + let percentage = + 100.0f64 * scaled_proportion.low_u128() as f64 / precision_factor.low_u128() as f64; + if base_value <= comparison_value { + Some(percentage as f32) + } else { + Some(-percentage as f32) + } +} + impl ExecutionLayer { /// Instantiate `Self` with an Execution engine specified in `Config`, using JSON-RPC via HTTP. pub fn from_config(config: Config, executor: TaskExecutor, log: Logger) -> Result { @@ -354,6 +390,7 @@ impl ExecutionLayer { builder_profit_threshold, execution_timeout_multiplier, always_prefer_builder_payload, + ignore_builder_override_suggestion_threshold, } = config; if urls.len() > 1 { @@ -433,6 +470,7 @@ impl ExecutionLayer { builder_profit_threshold: Uint256::from(builder_profit_threshold), log, always_prefer_builder_payload, + ignore_builder_override_suggestion_threshold, last_new_payload_errored: RwLock::new(false), }; @@ -755,7 +793,7 @@ impl ExecutionLayer { current_fork, ) .await - .map(ProvenancedPayload::Local) + .map(|get_payload_response| ProvenancedPayload::Local(get_payload_response.into())) } }; @@ -824,7 +862,7 @@ impl ExecutionLayer { .await }), timed_future(metrics::GET_BLINDED_PAYLOAD_LOCAL, async { - self.get_full_payload_caching::( + self.get_full_payload_caching( parent_hash, payload_attributes, forkchoice_update_params, @@ -844,7 +882,7 @@ impl ExecutionLayer { }, "relay_response_ms" => relay_duration.as_millis(), "local_fee_recipient" => match &local_result { - Ok(proposal_contents) => format!("{:?}", proposal_contents.payload().fee_recipient()), + Ok(get_payload_response) => format!("{:?}", get_payload_response.fee_recipient()), Err(_) => "request failed".to_string() }, "local_response_ms" => local_duration.as_millis(), @@ -858,20 +896,20 @@ impl ExecutionLayer { "Builder error when requesting payload"; "info" => "falling back to local execution client", "relay_error" => ?e, - "local_block_hash" => ?local.payload().block_hash(), + "local_block_hash" => ?local.block_hash(), "parent_hash" => ?parent_hash, ); - Ok(ProvenancedPayload::Local(local)) + Ok(ProvenancedPayload::Local(local.into())) } (Ok(None), Ok(local)) => { info!( self.log(), "Builder did not return a payload"; "info" => "falling back to local execution client", - "local_block_hash" => ?local.payload().block_hash(), + "local_block_hash" => ?local.block_hash(), "parent_hash" => ?parent_hash, ); - Ok(ProvenancedPayload::Local(local)) + Ok(ProvenancedPayload::Local(local.into())) } (Ok(Some(relay)), Ok(local)) => { let header = &relay.data.message.header; @@ -880,12 +918,13 @@ impl ExecutionLayer { self.log(), "Received local and builder payloads"; "relay_block_hash" => ?header.block_hash(), - "local_block_hash" => ?local.payload().block_hash(), + "local_block_hash" => ?local.block_hash(), "parent_hash" => ?parent_hash, ); let relay_value = relay.data.message.value; let local_value = *local.block_value(); + if !self.inner.always_prefer_builder_payload { if local_value >= relay_value { info!( @@ -894,7 +933,24 @@ impl ExecutionLayer { "local_block_value" => %local_value, "relay_value" => %relay_value ); - return Ok(ProvenancedPayload::Local(local)); + return Ok(ProvenancedPayload::Local(local.into())); + } else if local.should_override_builder().unwrap_or(false) { + let percentage_difference = + percentage_difference_u256(local_value, relay_value); + if percentage_difference.map_or(false, |percentage| { + percentage + < self + .inner + .ignore_builder_override_suggestion_threshold + }) { + info!( + self.log(), + "Using local payload because execution engine suggested we ignore builder payload"; + "local_block_value" => %local_value, + "relay_value" => %relay_value + ); + return Ok(ProvenancedPayload::Local(local.into())); + } } else { info!( self.log(), @@ -909,7 +965,7 @@ impl ExecutionLayer { &relay, parent_hash, payload_attributes, - Some(local.payload().block_number()), + Some(local.block_number()), self.inner.builder_profit_threshold, current_fork, spec, @@ -929,7 +985,7 @@ impl ExecutionLayer { "relay_block_hash" => ?header.block_hash(), "parent_hash" => ?parent_hash, ); - Ok(ProvenancedPayload::Local(local)) + Ok(ProvenancedPayload::Local(local.into())) } Err(reason) => { metrics::inc_counter_vec( @@ -944,7 +1000,7 @@ impl ExecutionLayer { "relay_block_hash" => ?header.block_hash(), "parent_hash" => ?parent_hash, ); - Ok(ProvenancedPayload::Local(local)) + Ok(ProvenancedPayload::Local(local.into())) } } } @@ -1049,17 +1105,17 @@ impl ExecutionLayer { current_fork, ) .await - .map(ProvenancedPayload::Local) + .map(|get_payload_response| ProvenancedPayload::Local(get_payload_response.into())) } /// Get a full payload without caching its result in the execution layer's payload cache. - async fn get_full_payload>( + async fn get_full_payload( &self, parent_hash: ExecutionBlockHash, payload_attributes: &PayloadAttributes, forkchoice_update_params: ForkchoiceUpdateParameters, current_fork: ForkName, - ) -> Result, Error> { + ) -> Result, Error> { self.get_full_payload_with( parent_hash, payload_attributes, @@ -1071,13 +1127,13 @@ impl ExecutionLayer { } /// Get a full payload and cache its result in the execution layer's payload cache. - async fn get_full_payload_caching>( + async fn get_full_payload_caching( &self, parent_hash: ExecutionBlockHash, payload_attributes: &PayloadAttributes, forkchoice_update_params: ForkchoiceUpdateParameters, current_fork: ForkName, - ) -> Result, Error> { + ) -> Result, Error> { self.get_full_payload_with( parent_hash, payload_attributes, @@ -1088,14 +1144,14 @@ impl ExecutionLayer { .await } - async fn get_full_payload_with>( + async fn get_full_payload_with( &self, parent_hash: ExecutionBlockHash, payload_attributes: &PayloadAttributes, forkchoice_update_params: ForkchoiceUpdateParameters, current_fork: ForkName, f: fn(&ExecutionLayer, ExecutionPayloadRef) -> Option>, - ) -> Result, Error> { + ) -> Result, Error> { self.engine() .request(move |engine| async move { let payload_id = if let Some(id) = engine @@ -1181,7 +1237,7 @@ impl ExecutionLayer { ); } - Ok(payload_response.into()) + Ok(payload_response) }) .await .map_err(Box::new) @@ -2297,4 +2353,42 @@ mod test { }) .await; } + + #[tokio::test] + async fn percentage_difference_u256_tests() { + // ensure function returns `None` when base value is zero + assert_eq!(percentage_difference_u256(0.into(), 1.into()), None); + // ensure function returns `None` when either value is greater than 120 Million ETH + let max_value = Uint256::from(12u8) * Uint256::exp10(25); + assert_eq!( + percentage_difference_u256(1u8.into(), max_value + Uint256::from(1u8)), + None + ); + assert_eq!( + percentage_difference_u256(max_value + Uint256::from(1u8), 1u8.into()), + None + ); + // it should work up to max value + assert_eq!( + percentage_difference_u256(max_value, max_value / Uint256::from(2u8)), + Some(-50f32) + ); + // should work when base value is greater than comparison value + assert_eq!( + percentage_difference_u256(4u8.into(), 3u8.into()), + Some(-25f32) + ); + // should work when comparison value is greater than base value + assert_eq!( + percentage_difference_u256(4u8.into(), 5u8.into()), + Some(25f32) + ); + // should be accurate to 7 decimal places + let result = + percentage_difference_u256(Uint256::from(31415926u64), Uint256::from(13371337u64)) + .expect("should get percentage"); + // result = -57.4377116 + assert!(result > -57.43772); + assert!(result <= -57.43771); + } } diff --git a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs index 7ed954a30a6..13dcd7d2b60 100644 --- a/beacon_node/execution_layer/src/test_utils/handle_rpc.rs +++ b/beacon_node/execution_layer/src/test_utils/handle_rpc.rs @@ -306,6 +306,7 @@ pub async fn handle_rpc( GENERIC_ERROR_CODE, ))? .into(), + should_override_builder: false, }) .unwrap() } diff --git a/beacon_node/execution_layer/src/test_utils/mock_builder.rs b/beacon_node/execution_layer/src/test_utils/mock_builder.rs index 7a28e694297..93eb06de851 100644 --- a/beacon_node/execution_layer/src/test_utils/mock_builder.rs +++ b/beacon_node/execution_layer/src/test_utils/mock_builder.rs @@ -36,8 +36,8 @@ use task_executor::TaskExecutor; use tempfile::NamedTempFile; use tree_hash::TreeHash; use types::{ - Address, BeaconState, BlindedPayload, ChainSpec, EthSpec, ExecPayload, ForkName, Hash256, Slot, - Uint256, + Address, BeaconState, ChainSpec, EthSpec, ExecPayload, ExecutionPayload, + ExecutionPayloadHeader, ForkName, Hash256, Slot, Uint256, }; #[derive(Clone)] @@ -425,9 +425,9 @@ impl mev_rs::BlindedBlockProvider for MockBuilder { finalized_hash: Some(finalized_execution_hash), }; - let payload = self + let payload: ExecutionPayload = self .el - .get_full_payload_caching::>( + .get_full_payload_caching( head_execution_hash, &payload_attributes, forkchoice_update_params, @@ -435,10 +435,17 @@ impl mev_rs::BlindedBlockProvider for MockBuilder { ) .await .map_err(convert_err)? - .to_payload() - .to_execution_payload_header(); + .into(); - let json_payload = serde_json::to_string(&payload).map_err(convert_err)?; + let header: ExecutionPayloadHeader = match payload { + ExecutionPayload::Merge(payload) => ExecutionPayloadHeader::Merge((&payload).into()), + ExecutionPayload::Capella(payload) => { + ExecutionPayloadHeader::Capella((&payload).into()) + } + ExecutionPayload::Deneb(payload) => ExecutionPayloadHeader::Deneb((&payload).into()), + }; + + let json_payload = serde_json::to_string(&header).map_err(convert_err)?; let mut message = match fork { ForkName::Capella => BuilderBid::Capella(BuilderBidCapella { header: serde_json::from_str(json_payload.as_str()).map_err(convert_err)?, diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index 9b4b4630743..44580e4a518 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -1089,6 +1089,23 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .default_value("0") .takes_value(true) ) + .arg( + Arg::with_name("ignore-builder-override-suggestion-threshold") + .long("ignore-builder-override-suggestion-threshold") + .value_name("PERCENTAGE") + .help("When the EE advises Lighthouse to ignore the builder payload, this flag \ + specifies a percentage threshold for the difference between the reward from \ + the builder payload and the local EE's payload. This threshold must be met \ + for Lighthouse to consider ignoring the EE's suggestion. If the reward from \ + the builder's payload doesn't exceed the local payload by at least this \ + percentage, the local payload will be used. The conditions under which the \ + EE may make this suggestion depend on the EE's implementation, with the \ + primary intent being to safeguard against potential censorship attacks \ + from builders. Setting this flag to 0 will cause Lighthouse to always \ + ignore the EE's suggestion. Default: 10.0 (equivalent to 10%).") + .default_value("10.0") + .takes_value(true) + ) .arg( Arg::with_name("builder-user-agent") .long("builder-user-agent") @@ -1160,6 +1177,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { // to local payloads, therefore it fundamentally conflicts with // always using the builder. .conflicts_with("builder-profit-threshold") + .conflicts_with("ignore-builder-override-suggestion-threshold") ) .arg( Arg::with_name("invalid-gossip-verified-blocks-path") diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index fe0b163603e..3c5f6298414 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -345,6 +345,8 @@ 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")?; + el_config.ignore_builder_override_suggestion_threshold = + clap_utils::parse_required(cli_args, "ignore-builder-override-suggestion-threshold")?; let execution_timeout_multiplier = clap_utils::parse_required(cli_args, "execution-timeout-multiplier")?; el_config.execution_timeout_multiplier = Some(execution_timeout_multiplier); diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 56812552599..65e5cc7be54 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -734,6 +734,38 @@ fn builder_fallback_flags() { ); }, ); + run_payload_builder_flag_test_with_config( + "builder", + "http://meow.cats", + Some("ignore-builder-override-suggestion-threshold"), + Some("53.4"), + |config| { + assert_eq!( + config + .execution_layer + .as_ref() + .unwrap() + .ignore_builder_override_suggestion_threshold, + 53.4f32 + ); + }, + ); + run_payload_builder_flag_test_with_config( + "builder", + "http://meow.cats", + None, + None, + |config| { + assert_eq!( + config + .execution_layer + .as_ref() + .unwrap() + .ignore_builder_override_suggestion_threshold, + 10.0f32 + ); + }, + ); } #[test]