cerc-io/lighthouse
10
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Refactor & Fix Bugs in Payload Selection Logic (#4950)

Browse Source 
* Refactor & Fix Bugs in Payload Selection Logic

* Fix lint

* Update beacon_node/execution_layer/src/lib.rs

* Finish renaming function
This commit is contained in:
ethDreamer 2023-11-28 22:20:12 -06:00 committed by GitHub
parent 86163d94f2
commit 43d98153d6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
1 changed files with 298 additions and 290 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

442
beacon_node/execution_layer/src/lib.rs
View File

@ -320,6 +320,7 @@ pub struct BuilderParams {
pub chain_health: ChainHealth, pub chain_health: ChainHealth,
} }
#[derive(PartialEq)]
pub enum ChainHealth { pub enum ChainHealth {
Healthy, Healthy,
Unhealthy(FailedCondition), Unhealthy(FailedCondition),
@ -327,7 +328,7 @@ pub enum ChainHealth {
PreMerge, PreMerge,
} }
#[derive(Debug)] #[derive(Debug, PartialEq)]
pub enum FailedCondition { pub enum FailedCondition {
Skips, Skips,
SkipsPerEpoch, SkipsPerEpoch,
@ -928,20 +929,22 @@ impl<T: EthSpec> ExecutionLayer<T> {
} }
} }
async fn determine_and_fetch_payload( /// Fetches local and builder paylaods concurrently, Logs and returns results.
async fn fetch_builder_and_local_payloads(
&self, &self,
builder: &BuilderHttpClient,
parent_hash: ExecutionBlockHash, parent_hash: ExecutionBlockHash,
builder_params: &BuilderParams,
payload_attributes: &PayloadAttributes, payload_attributes: &PayloadAttributes,
forkchoice_update_params: ForkchoiceUpdateParameters, forkchoice_update_params: ForkchoiceUpdateParameters,
builder_params: BuilderParams,
current_fork: ForkName, current_fork: ForkName,
spec: &ChainSpec, ) -> (
) -> Result<ProvenancedPayload<BlockProposalContentsType<T>>, Error> { Result<Option<ForkVersionedResponse<SignedBuilderBid<T>>>, builder_client::Error>,
if let Some(builder) = self.builder() { Result<GetPayloadResponse<T>, Error>,
) {
let slot = builder_params.slot; let slot = builder_params.slot;
let pubkey = builder_params.pubkey; let pubkey = &builder_params.pubkey;
match builder_params.chain_health {
ChainHealth::Healthy => {
info!( info!(
self.log(), self.log(),
"Requesting blinded header from connected builder"; "Requesting blinded header from connected builder";
@ -951,10 +954,10 @@ impl<T: EthSpec> ExecutionLayer<T> {
); );
// Wait for the builder *and* local EL to produce a payload (or return an error). // Wait for the builder *and* local EL to produce a payload (or return an error).
let ((relay_result, relay_duration), (local_result_type, local_duration)) = tokio::join!( let ((relay_result, relay_duration), (local_result, local_duration)) = tokio::join!(
timed_future(metrics::GET_BLINDED_PAYLOAD_BUILDER, async { timed_future(metrics::GET_BLINDED_PAYLOAD_BUILDER, async {
builder builder
.get_builder_header::<T>(slot, parent_hash, &pubkey) .get_builder_header::<T>(slot, parent_hash, pubkey)
.await .await
}), }),
timed_future(metrics::GET_BLINDED_PAYLOAD_LOCAL, async { timed_future(metrics::GET_BLINDED_PAYLOAD_LOCAL, async {
@ -965,13 +968,12 @@ impl<T: EthSpec> ExecutionLayer<T> {
current_fork, current_fork,
) )
.await .await
}) .and_then(|local_result_type| match local_result_type {
);
let local_result = match local_result_type? {
GetPayloadResponseType::Full(payload) => Ok(payload), GetPayloadResponseType::Full(payload) => Ok(payload),
GetPayloadResponseType::Blinded(_) => Err(Error::PayloadTypeMismatch), GetPayloadResponseType::Blinded(_) => Err(Error::PayloadTypeMismatch),
}; })
})
);
info!( info!(
self.log(), self.log(),
@ -990,7 +992,80 @@ impl<T: EthSpec> ExecutionLayer<T> {
"parent_hash" => ?parent_hash, "parent_hash" => ?parent_hash,
); );
return match (relay_result, local_result) { (relay_result, local_result)
}
async fn determine_and_fetch_payload(
&self,
parent_hash: ExecutionBlockHash,
payload_attributes: &PayloadAttributes,
forkchoice_update_params: ForkchoiceUpdateParameters,
builder_params: BuilderParams,
current_fork: ForkName,
spec: &ChainSpec,
) -> Result<ProvenancedPayload<BlockProposalContentsType<T>>, Error> {
let Some(builder) = self.builder() else {
// no builder.. return local payload
return self
.get_full_payload_caching(
parent_hash,
payload_attributes,
forkchoice_update_params,
current_fork,
)
.await
.and_then(GetPayloadResponseType::try_into)
.map(ProvenancedPayload::Local);
};
// check chain health
if builder_params.chain_health != ChainHealth::Healthy {
// chain is unhealthy, gotta use local payload
match builder_params.chain_health {
ChainHealth::Unhealthy(condition) => info!(
self.log(),
"Chain is unhealthy, using local payload";
"info" => "this helps protect the network. the --builder-fallback flags \
can adjust the expected health conditions.",
"failed_condition" => ?condition
),
// Intentional no-op, so we never attempt builder API proposals pre-merge.
ChainHealth::PreMerge => (),
ChainHealth::Optimistic => info!(
self.log(),
"Chain is optimistic; can't build payload";
"info" => "the local execution engine is syncing and the builder network \
cannot safely be used - unable to propose block"
),
ChainHealth::Healthy => crit!(
self.log(),
"got healthy but also not healthy.. this shouldn't happen!"
),
}
return self
.get_full_payload_caching(
parent_hash,
payload_attributes,
forkchoice_update_params,
current_fork,
)
.await
.and_then(GetPayloadResponseType::try_into)
.map(ProvenancedPayload::Local);
}
let (relay_result, local_result) = self
.fetch_builder_and_local_payloads(
builder.as_ref(),
parent_hash,
&builder_params,
payload_attributes,
forkchoice_update_params,
current_fork,
)
.await;
match (relay_result, local_result) {
(Err(e), Ok(local)) => { (Err(e), Ok(local)) => {
warn!( warn!(
self.log(), self.log(),
@ -1016,145 +1091,6 @@ impl<T: EthSpec> ExecutionLayer<T> {
local.try_into()?, local.try_into()?,
))) )))
} }
(Ok(Some(relay)), Ok(local)) => {
let header = &relay.data.message.header();
info!(
self.log(),
"Received local and builder payloads";
"relay_block_hash" => ?header.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!(
self.log(),
"Local block is more profitable than relay block";
"local_block_value" => %local_value,
"relay_value" => %relay_value
);
return Ok(ProvenancedPayload::Local(
BlockProposalContentsType::Full(local.try_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(
BlockProposalContentsType::Full(local.try_into()?),
));
}
} else {
info!(
self.log(),
"Relay block is more profitable than local block";
"local_block_value" => %local_value,
"relay_value" => %relay_value
);
}
}
match verify_builder_bid(
&relay,
parent_hash,
payload_attributes,
Some(local.block_number()),
self.inner.builder_profit_threshold,
current_fork,
spec,
) {
Ok(()) => Ok(ProvenancedPayload::try_from(relay.data.message)?),
Err(reason) if !reason.payload_invalid() => {
info!(
self.log(),
"Builder payload ignored";
"info" => "using local payload",
"reason" => %reason,
"relay_block_hash" => ?header.block_hash(),
"parent_hash" => ?parent_hash,
);
Ok(ProvenancedPayload::Local(BlockProposalContentsType::Full(
local.try_into()?,
)))
}
Err(reason) => {
metrics::inc_counter_vec(
&metrics::EXECUTION_LAYER_GET_PAYLOAD_BUILDER_REJECTIONS,
&[reason.as_ref().as_ref()],
);
warn!(
self.log(),
"Builder returned invalid payload";
"info" => "using local payload",
"reason" => %reason,
"relay_block_hash" => ?header.block_hash(),
"parent_hash" => ?parent_hash,
);
Ok(ProvenancedPayload::Local(BlockProposalContentsType::Full(
local.try_into()?,
)))
}
}
}
(Ok(Some(relay)), Err(local_error)) => {
let header = &relay.data.message.header();
info!(
self.log(),
"Received builder payload with local error";
"relay_block_hash" => ?header.block_hash(),
"local_error" => ?local_error,
"parent_hash" => ?parent_hash,
);
match verify_builder_bid(
&relay,
parent_hash,
payload_attributes,
None,
self.inner.builder_profit_threshold,
current_fork,
spec,
) {
Ok(()) => Ok(ProvenancedPayload::try_from(relay.data.message)?),
// If the payload is valid then use it. The local EE failed
// to produce a payload so we have no alternative.
Err(e) if !e.payload_invalid() => {
Ok(ProvenancedPayload::try_from(relay.data.message)?)
}
Err(reason) => {
metrics::inc_counter_vec(
&metrics::EXECUTION_LAYER_GET_PAYLOAD_BUILDER_REJECTIONS,
&[reason.as_ref().as_ref()],
);
crit!(
self.log(),
"Builder returned invalid payload";
"info" => "no local payload either - unable to propose block",
"reason" => %reason,
"relay_block_hash" => ?header.block_hash(),
"parent_hash" => ?parent_hash,
);
Err(Error::CannotProduceHeader)
}
}
}
(Err(relay_error), Err(local_error)) => { (Err(relay_error), Err(local_error)) => {
crit!( crit!(
self.log(), self.log(),
@ -1179,34 +1115,142 @@ impl<T: EthSpec> ExecutionLayer<T> {
Err(Error::CannotProduceHeader) Err(Error::CannotProduceHeader)
} }
}; (Ok(Some(relay)), Ok(local)) => {
} let header = &relay.data.message.header();
ChainHealth::Unhealthy(condition) => info!(
info!(
self.log(), self.log(),
"Chain is unhealthy, using local payload"; "Received local and builder payloads";
"info" => "this helps protect the network. the --builder-fallback flags \ "relay_block_hash" => ?header.block_hash(),
can adjust the expected health conditions.", "local_block_hash" => ?local.block_hash(),
"failed_condition" => ?condition "parent_hash" => ?parent_hash,
), );
// Intentional no-op, so we never attempt builder API proposals pre-merge.
ChainHealth::PreMerge => (), // check relay payload validity
ChainHealth::Optimistic => info!( if let Err(reason) = verify_builder_bid(
self.log(), &relay,
"Chain is optimistic; can't build payload";
"info" => "the local execution engine is syncing and the builder network \
cannot safely be used - unable to propose block"
),
}
}
self.get_full_payload_caching(
parent_hash, parent_hash,
payload_attributes, payload_attributes,
forkchoice_update_params, Some(local.block_number()),
current_fork, current_fork,
) spec,
.await ) {
.and_then(GetPayloadResponseType::try_into) // relay payload invalid -> return local
.map(ProvenancedPayload::Local) metrics::inc_counter_vec(
&metrics::EXECUTION_LAYER_GET_PAYLOAD_BUILDER_REJECTIONS,
&[reason.as_ref().as_ref()],
);
warn!(
self.log(),
"Builder returned invalid payload";
"info" => "using local payload",
"reason" => %reason,
"relay_block_hash" => ?header.block_hash(),
"parent_hash" => ?parent_hash,
);
return Ok(ProvenancedPayload::Local(BlockProposalContentsType::Full(
local.try_into()?,
)));
}
if self.inner.always_prefer_builder_payload {
return ProvenancedPayload::try_from(relay.data.message);
}
let relay_value = *relay.data.message.value();
let local_value = *local.block_value();
if local_value >= relay_value {
info!(
self.log(),
"Local block is more profitable than relay block";
"local_block_value" => %local_value,
"relay_value" => %relay_value
);
return Ok(ProvenancedPayload::Local(BlockProposalContentsType::Full(
local.try_into()?,
)));
}
if relay_value < self.inner.builder_profit_threshold {
info!(
self.log(),
"Builder payload ignored";
"info" => "using local payload",
"reason" => format!("payload value of {} does not meet user-configured profit-threshold of {}", relay_value, self.inner.builder_profit_threshold),
"relay_block_hash" => ?header.block_hash(),
"parent_hash" => ?parent_hash,
);
return Ok(ProvenancedPayload::Local(BlockProposalContentsType::Full(
local.try_into()?,
)));
}
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(BlockProposalContentsType::Full(
local.try_into()?,
)));
}
}
info!(
self.log(),
"Relay block is more profitable than local block";
"local_block_value" => %local_value,
"relay_value" => %relay_value
);
Ok(ProvenancedPayload::try_from(relay.data.message)?)
}
(Ok(Some(relay)), Err(local_error)) => {
let header = &relay.data.message.header();
info!(
self.log(),
"Received builder payload with local error";
"relay_block_hash" => ?header.block_hash(),
"local_error" => ?local_error,
"parent_hash" => ?parent_hash,
);
match verify_builder_bid(
&relay,
parent_hash,
payload_attributes,
None,
current_fork,
spec,
) {
Ok(()) => Ok(ProvenancedPayload::try_from(relay.data.message)?),
Err(reason) => {
metrics::inc_counter_vec(
&metrics::EXECUTION_LAYER_GET_PAYLOAD_BUILDER_REJECTIONS,
&[reason.as_ref().as_ref()],
);
crit!(
self.log(),
"Builder returned invalid payload";
"info" => "no local payload either - unable to propose block",
"reason" => %reason,
"relay_block_hash" => ?header.block_hash(),
"parent_hash" => ?parent_hash,
);
Err(Error::CannotProduceHeader)
}
}
}
}
} }
/// Get a full payload and cache its result in the execution layer's payload cache. /// Get a full payload and cache its result in the execution layer's payload cache.
@ -2027,10 +2071,6 @@ impl<T: EthSpec> ExecutionLayer<T> {
#[derive(AsRefStr)] #[derive(AsRefStr)]
#[strum(serialize_all = "snake_case")] #[strum(serialize_all = "snake_case")]
enum InvalidBuilderPayload { enum InvalidBuilderPayload {
LowValue {
profit_threshold: Uint256,
payload_value: Uint256,
},
ParentHash { ParentHash {
payload: ExecutionBlockHash, payload: ExecutionBlockHash,
expected: ExecutionBlockHash, expected: ExecutionBlockHash,
@ -2061,34 +2101,9 @@ enum InvalidBuilderPayload {
}, },
} }
impl InvalidBuilderPayload {
/// Returns `true` if a payload is objectively invalid and should never be included on chain.
fn payload_invalid(&self) -> bool {
match self {
// A low-value payload isn't invalid, it should just be avoided if possible.
InvalidBuilderPayload::LowValue { .. } => false,
InvalidBuilderPayload::ParentHash { .. } => true,
InvalidBuilderPayload::PrevRandao { .. } => true,
InvalidBuilderPayload::Timestamp { .. } => true,
InvalidBuilderPayload::BlockNumber { .. } => true,
InvalidBuilderPayload::Fork { .. } => true,
InvalidBuilderPayload::Signature { .. } => true,
InvalidBuilderPayload::WithdrawalsRoot { .. } => true,
}
}
}
impl fmt::Display for InvalidBuilderPayload { impl fmt::Display for InvalidBuilderPayload {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self { match self {
InvalidBuilderPayload::LowValue {
profit_threshold,
payload_value,
} => write!(
f,
"payload value of {} does not meet user-configured profit-threshold of {}",
payload_value, profit_threshold
),
InvalidBuilderPayload::ParentHash { payload, expected } => { InvalidBuilderPayload::ParentHash { payload, expected } => {
write!(f, "payload block hash was {} not {}", payload, expected) write!(f, "payload block hash was {} not {}", payload, expected)
} }
@ -2132,13 +2147,11 @@ fn verify_builder_bid<T: EthSpec>(
parent_hash: ExecutionBlockHash, parent_hash: ExecutionBlockHash,
payload_attributes: &PayloadAttributes, payload_attributes: &PayloadAttributes,
block_number: Option<u64>, block_number: Option<u64>,
profit_threshold: Uint256,
current_fork: ForkName, current_fork: ForkName,
spec: &ChainSpec, spec: &ChainSpec,
) -> Result<(), Box<InvalidBuilderPayload>> { ) -> Result<(), Box<InvalidBuilderPayload>> {
let is_signature_valid = bid.data.verify_signature(spec); let is_signature_valid = bid.data.verify_signature(spec);
let header = &bid.data.message.header(); let header = &bid.data.message.header();
let payload_value = bid.data.message.value();
// Avoid logging values that we can't represent with our Prometheus library. // Avoid logging values that we can't represent with our Prometheus library.
let payload_value_gwei = bid.data.message.value() / 1_000_000_000; let payload_value_gwei = bid.data.message.value() / 1_000_000_000;
@ -2157,12 +2170,7 @@ fn verify_builder_bid<T: EthSpec>(
.map(|withdrawals| Withdrawals::<T>::from(withdrawals).tree_hash_root()); .map(|withdrawals| Withdrawals::<T>::from(withdrawals).tree_hash_root());
let payload_withdrawals_root = header.withdrawals_root().ok().copied(); let payload_withdrawals_root = header.withdrawals_root().ok().copied();
if *payload_value < profit_threshold { if header.parent_hash() != parent_hash {
Err(Box::new(InvalidBuilderPayload::LowValue {
profit_threshold,
payload_value: *payload_value,
}))
} else if header.parent_hash() != parent_hash {
Err(Box::new(InvalidBuilderPayload::ParentHash { Err(Box::new(InvalidBuilderPayload::ParentHash {
payload: header.parent_hash(), payload: header.parent_hash(),
expected: parent_hash, expected: parent_hash,