Skip to content

Commit

Permalink
Addressed sigp#4487
Browse files Browse the repository at this point in the history 
Add override threshold flag
Added tests for Override Threshold Flag
Override default shown in decimal
  • Loading branch information
@ethDreamer
ethDreamer committed Aug 9, 2023
1 parent 02c7a2e commit 0cfceba
Show file tree
Hide file tree
Showing 8 changed files with 212 additions and 27 deletions.
28 changes: 28 additions & 0 deletions beacon_node/execution_layer/src/engine_api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,34 @@ pub struct GetPayloadResponse<T: EthSpec> {
pub block_value: Uint256,
#[superstruct(only(Deneb))]
pub blobs_bundle: BlobsBundleV1<T>,
#[superstruct(only(Deneb), partial_getter(copy))]
pub should_override_builder: bool,
}

impl<E: EthSpec> GetPayloadResponse<E> {
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<GetPayloadResponseRef<'a, T>> for ExecutionPayloadRef<'a, T> {
Expand Down
3 changes: 3 additions & 0 deletions beacon_node/execution_layer/src/engine_api/json_structures.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,8 @@ pub struct JsonGetPayloadResponse<T: EthSpec> {
pub block_value: Uint256,
#[superstruct(only(V3))]
pub blobs_bundle: JsonBlobsBundleV1<T>,
#[superstruct(only(V3))]
pub should_override_builder: bool,
}

impl<T: EthSpec> From<JsonGetPayloadResponse<T>> for GetPayloadResponse<T> {
Expand All @@ -320,6 +322,7 @@ impl<T: EthSpec> From<JsonGetPayloadResponse<T>> for GetPayloadResponse<T> {
execution_payload: response.execution_payload.into(),
block_value: response.block_value,
blobs_bundle: response.blobs_bundle.into(),
should_override_builder: response.should_override_builder,
})
}
}
Expand Down
134 changes: 114 additions & 20 deletions beacon_node/execution_layer/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ struct Inner<E: EthSpec> {
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
Expand Down Expand Up @@ -330,6 +331,7 @@ pub struct Config {
pub builder_profit_threshold: u128,
pub execution_timeout_multiplier: Option<u32>,
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
Expand All @@ -339,6 +341,40 @@ pub struct ExecutionLayer<T: EthSpec> {
inner: Arc<Inner<T>>,
}

/// 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<f32> {
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<T: EthSpec> ExecutionLayer<T> {
/// 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<Self, Error> {
Expand All @@ -354,6 +390,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
builder_profit_threshold,
execution_timeout_multiplier,
always_prefer_builder_payload,
ignore_builder_override_suggestion_threshold,
} = config;

if urls.len() > 1 {
Expand Down Expand Up @@ -433,6 +470,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
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),
};

Expand Down Expand Up @@ -755,7 +793,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
current_fork,
)
.await
.map(ProvenancedPayload::Local)
.map(|get_payload_response| ProvenancedPayload::Local(get_payload_response.into()))
}
};

Expand Down Expand Up @@ -824,7 +862,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
.await
}),
timed_future(metrics::GET_BLINDED_PAYLOAD_LOCAL, async {
self.get_full_payload_caching::<Payload>(
self.get_full_payload_caching(
parent_hash,
payload_attributes,
forkchoice_update_params,
Expand All @@ -844,7 +882,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
},
"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(),
Expand All @@ -858,20 +896,20 @@ impl<T: EthSpec> ExecutionLayer<T> {
"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;
Expand All @@ -880,12 +918,13 @@ impl<T: EthSpec> ExecutionLayer<T> {
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!(
Expand All @@ -894,7 +933,24 @@ impl<T: EthSpec> ExecutionLayer<T> {
"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(),
Expand All @@ -909,7 +965,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
&relay,
parent_hash,
payload_attributes,
Some(local.payload().block_number()),
Some(local.block_number()),
self.inner.builder_profit_threshold,
current_fork,
spec,
Expand All @@ -929,7 +985,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
"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(
Expand All @@ -944,7 +1000,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
"relay_block_hash" => ?header.block_hash(),
"parent_hash" => ?parent_hash,
);
Ok(ProvenancedPayload::Local(local))
Ok(ProvenancedPayload::Local(local.into()))
}
}
}
Expand Down Expand Up @@ -1049,17 +1105,17 @@ impl<T: EthSpec> ExecutionLayer<T> {
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<Payload: AbstractExecPayload<T>>(
async fn get_full_payload(
&self,
parent_hash: ExecutionBlockHash,
payload_attributes: &PayloadAttributes,
forkchoice_update_params: ForkchoiceUpdateParameters,
current_fork: ForkName,
) -> Result<BlockProposalContents<T, Payload>, Error> {
) -> Result<GetPayloadResponse<T>, Error> {
self.get_full_payload_with(
parent_hash,
payload_attributes,
Expand All @@ -1071,13 +1127,13 @@ impl<T: EthSpec> ExecutionLayer<T> {
}

/// Get a full payload and cache its result in the execution layer's payload cache.
async fn get_full_payload_caching<Payload: AbstractExecPayload<T>>(
async fn get_full_payload_caching(
&self,
parent_hash: ExecutionBlockHash,
payload_attributes: &PayloadAttributes,
forkchoice_update_params: ForkchoiceUpdateParameters,
current_fork: ForkName,
) -> Result<BlockProposalContents<T, Payload>, Error> {
) -> Result<GetPayloadResponse<T>, Error> {
self.get_full_payload_with(
parent_hash,
payload_attributes,
Expand All @@ -1088,14 +1144,14 @@ impl<T: EthSpec> ExecutionLayer<T> {
.await
}

async fn get_full_payload_with<Payload: AbstractExecPayload<T>>(
async fn get_full_payload_with(
&self,
parent_hash: ExecutionBlockHash,
payload_attributes: &PayloadAttributes,
forkchoice_update_params: ForkchoiceUpdateParameters,
current_fork: ForkName,
f: fn(&ExecutionLayer<T>, ExecutionPayloadRef<T>) -> Option<ExecutionPayload<T>>,
) -> Result<BlockProposalContents<T, Payload>, Error> {
) -> Result<GetPayloadResponse<T>, Error> {
self.engine()
.request(move |engine| async move {
let payload_id = if let Some(id) = engine
Expand Down Expand Up @@ -1181,7 +1237,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
);
}

Ok(payload_response.into())
Ok(payload_response)
})
.await
.map_err(Box::new)
Expand Down Expand Up @@ -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);
}
}
1 change: 1 addition & 0 deletions beacon_node/execution_layer/src/test_utils/handle_rpc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ pub async fn handle_rpc<T: EthSpec>(
GENERIC_ERROR_CODE,
))?
.into(),
should_override_builder: false,
})
.unwrap()
}
Expand Down
Loading

0 comments on commit 0cfceba

Please sign in to comment.