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 30, 2023
1 parent 9c75d80 commit b9405ca
Show file tree
Hide file tree
Showing 5 changed files with 87 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
61 changes: 41 additions & 20 deletions beacon_node/execution_layer/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use eth2::types::{builder_bid::SignedBuilderBid, ForkVersionedResponse};
use ethers_core::abi::ethereum_types::FromStrRadixErr;
use ethers_core::types::Transaction as EthersTransaction;
use fork_choice::ForkchoiceUpdateParameters;
use lazy_static::lazy_static;
use lru::LruCache;
use payload_status::process_payload_status;
pub use payload_status::PayloadStatus;
Expand Down Expand Up @@ -70,6 +71,11 @@ pub const DEFAULT_JWT_FILE: &str = "jwt.hex";
/// in an LRU cache to avoid redundant lookups. This is the size of that cache.
const EXECUTION_BLOCKS_LRU_CACHE_SIZE: usize = 128;

lazy_static! {
/// The threshold for taking the builder value even if the execution engine says to override it.
pub static ref BUILDER_PROFIT_THRESHOLD: Uint256 = Uint256::from(250_000_000_000_000_000u64); // 0.25 ETH
}

/// A fee recipient address for use during block production. Only used as a very last resort if
/// there is no address provided by the user.
///
Expand Down Expand Up @@ -775,7 +781,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 @@ -844,7 +850,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 @@ -864,7 +870,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 @@ -878,20 +884,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 @@ -900,12 +906,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 @@ -914,8 +921,22 @@ 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 {
// 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 < *BUILDER_PROFIT_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()));
}

info!(
self.log(),
"Relay block is more profitable than local block";
Expand All @@ -929,7 +950,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 @@ -949,7 +970,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 @@ -964,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()))
}
}
}
Expand Down Expand Up @@ -1069,17 +1090,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 @@ -1091,13 +1112,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 @@ -1108,14 +1129,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 @@ -1201,7 +1222,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
21 changes: 14 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 @@ -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)]
Expand Down Expand Up @@ -425,20 +425,27 @@ 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 json_payload = serde_json::to_string(&payload).map_err(convert_err)?;
let header: ExecutionPayloadHeader<E> = 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)?,
Expand Down

0 comments on commit b9405ca

Please sign in to comment.