Skip to content

Commit

Permalink
Addressed sigp#4487
Browse files Browse the repository at this point in the history
  • Loading branch information
@ethDreamer
ethDreamer committed Jul 10, 2023
1 parent c3ef84b commit 7927624
Show file tree
Hide file tree
Showing 6 changed files with 133 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))]
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
55 changes: 35 additions & 20 deletions beacon_node/execution_layer/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,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 @@ -845,7 +845,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 @@ -865,7 +865,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 @@ -879,20 +879,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 @@ -901,12 +901,27 @@ 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();

// TODO: determine an actual heuristic for when we take the
// EL suggestion that we ignore the builder payload.
if *local.should_override_builder().unwrap_or(&false)
&& (relay_value / local_value < Uint256::from(2u64))
{
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()));
}

if !self.inner.always_prefer_builder_payload {
if local_value >= relay_value {
info!(
Expand All @@ -915,7 +930,7 @@ 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 {
info!(
self.log(),
Expand All @@ -930,7 +945,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 @@ -950,7 +965,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 @@ -965,7 +980,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 @@ -1070,17 +1085,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 @@ -1092,13 +1107,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 @@ -1109,14 +1124,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 @@ -1202,7 +1217,7 @@ impl<T: EthSpec> ExecutionLayer<T> {
);
}

Ok(payload_response.into())
Ok(payload_response)
})
.await
.map_err(Box::new)
Expand Down
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
14 changes: 7 additions & 7 deletions beacon_node/execution_layer/src/test_utils/mock_builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,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)]
Expand Down Expand Up @@ -427,20 +427,20 @@ impl<E: EthSpec> mev_rs::BlindedBlockProvider for MockBuilder<E> {
finalized_hash: Some(finalized_execution_hash),
};

let payload = self
let payload: ExecutionPayload<E> = self
.el
.get_full_payload_caching::<BlindedPayload<E>>(
.get_full_payload_caching(
head_execution_hash,
&payload_attributes,
forkchoice_update_params,
fork,
)
.await
.map_err(convert_err)?
.to_payload()
.to_execution_payload_header();
.into();
let header: ExecutionPayloadHeader<E> = payload.into();

let json_payload = serde_json::to_string(&payload).map_err(convert_err)?;
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)?,
Expand Down
59 changes: 59 additions & 0 deletions consensus/types/src/execution_payload_header.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,3 +312,62 @@ impl<T: EthSpec> ForkVersionDeserialize for ExecutionPayloadHeader<T> {
})
}
}

impl<E: EthSpec> From<ExecutionPayload<E>> for ExecutionPayloadHeader<E> {
fn from(payload: ExecutionPayload<E>) -> Self {
match payload {
ExecutionPayload::Merge(payload) => Self::Merge(ExecutionPayloadHeaderMerge {
parent_hash: payload.parent_hash,
fee_recipient: payload.fee_recipient,
state_root: payload.state_root,
receipts_root: payload.receipts_root,
logs_bloom: payload.logs_bloom.clone(),
prev_randao: payload.prev_randao,
block_number: payload.block_number,
gas_limit: payload.gas_limit,
gas_used: payload.gas_used,
timestamp: payload.timestamp,
extra_data: payload.extra_data.clone(),
base_fee_per_gas: payload.base_fee_per_gas,
block_hash: payload.block_hash,
transactions_root: payload.transactions.tree_hash_root(),
}),
ExecutionPayload::Capella(payload) => Self::Capella(ExecutionPayloadHeaderCapella {
parent_hash: payload.parent_hash,
fee_recipient: payload.fee_recipient,
state_root: payload.state_root,
receipts_root: payload.receipts_root,
logs_bloom: payload.logs_bloom.clone(),
prev_randao: payload.prev_randao,
block_number: payload.block_number,
gas_limit: payload.gas_limit,
gas_used: payload.gas_used,
timestamp: payload.timestamp,
extra_data: payload.extra_data.clone(),
base_fee_per_gas: payload.base_fee_per_gas,
block_hash: payload.block_hash,
transactions_root: payload.transactions.tree_hash_root(),
withdrawals_root: payload.withdrawals.tree_hash_root(),
}),
ExecutionPayload::Deneb(payload) => Self::Deneb(ExecutionPayloadHeaderDeneb {
parent_hash: payload.parent_hash,
fee_recipient: payload.fee_recipient,
state_root: payload.state_root,
receipts_root: payload.receipts_root,
logs_bloom: payload.logs_bloom.clone(),
prev_randao: payload.prev_randao,
block_number: payload.block_number,
gas_limit: payload.gas_limit,
gas_used: payload.gas_used,
timestamp: payload.timestamp,
extra_data: payload.extra_data.clone(),
base_fee_per_gas: payload.base_fee_per_gas,
block_hash: payload.block_hash,
transactions_root: payload.transactions.tree_hash_root(),
withdrawals_root: payload.withdrawals.tree_hash_root(),
data_gas_used: payload.data_gas_used,
excess_data_gas: payload.excess_data_gas,
}),
}
}
}

0 comments on commit 7927624

Please sign in to comment.