Skip to content

Use Local Payload if More Profitable than Builder #3934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 2, 2023
Merged

Use Local Payload if More Profitable than Builder #3934

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d89d640
Use Local Payload if More Profitable than Builder
ethDreamer Jan 31, 2023
4b112ae
Rename clone -> clone_from_ref
ethDreamer Jan 31, 2023
8831b74
Minimize Clones of GetPayloadResponse
ethDreamer Feb 1, 2023
2c44ad6
Cleanup & Fix Tests
ethDreamer Feb 1, 2023
fe843be
Added Tests for Payload Choice by Profit
ethDreamer Feb 1, 2023
1fe8399
Fix Outdated Comments
ethDreamer Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions beacon_node/execution_layer/src/engine_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ use std::convert::TryFrom;
use strum::IntoStaticStr;
use superstruct::superstruct;
pub use types::{
Address, EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadHeader, FixedVector,
ForkName, Hash256, Uint256, VariableList, Withdrawal,
Address, EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadHeader,
ExecutionPayloadRef, FixedVector, ForkName, Hash256, Uint256, VariableList, Withdrawal,
};
use types::{ExecutionPayloadCapella, ExecutionPayloadEip4844, ExecutionPayloadMerge};

Expand Down Expand Up @@ -337,16 +337,16 @@ pub struct GetPayloadResponse<T: EthSpec> {
}

impl<T: EthSpec> GetPayloadResponse<T> {
pub fn execution_payload(self) -> ExecutionPayload<T> {
pub fn execution_payload(&self) -> ExecutionPayloadRef<T> {
match self {
GetPayloadResponse::Merge(response) => {
ExecutionPayload::Merge(response.execution_payload)
ExecutionPayloadRef::<T>::Merge(&response.execution_payload)
}
GetPayloadResponse::Capella(response) => {
ExecutionPayload::Capella(response.execution_payload)
ExecutionPayloadRef::<T>::Capella(&response.execution_payload)
}
GetPayloadResponse::Eip4844(response) => {
ExecutionPayload::Eip4844(response.execution_payload)
ExecutionPayloadRef::<T>::Eip4844(&response.execution_payload)
}
}
}
Expand Down
18 changes: 8 additions & 10 deletions beacon_node/execution_layer/src/engine_api/http.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,7 +804,7 @@ impl HttpJsonRpc {
pub async fn get_payload_v1<T: EthSpec>(
&self,
payload_id: PayloadId,
) -> Result<ExecutionPayload<T>, Error> {
) -> Result<GetPayloadResponse<T>, Error> {
let params = json!([JsonPayloadIdRequest::from(payload_id)]);

let payload_v1: JsonExecutionPayloadV1<T> = self
Expand All @@ -815,7 +815,11 @@ impl HttpJsonRpc {
)
.await?;

Ok(JsonExecutionPayload::V1(payload_v1).into())
Ok(GetPayloadResponse::Merge(GetPayloadResponseMerge {
execution_payload: payload_v1.into(),
// Have to guess zero here as we don't know the value
block_value: Uint256::zero(),
}))
}

pub async fn get_payload_v2<T: EthSpec>(
Expand Down Expand Up @@ -1015,16 +1019,10 @@ impl HttpJsonRpc {
&self,
fork_name: ForkName,
payload_id: PayloadId,
) -> Result<ExecutionPayload<T>, Error> {
) -> Result<GetPayloadResponse<T>, Error> {
let engine_capabilities = self.get_engine_capabilities(None).await?;
if engine_capabilities.get_payload_v2 {
// TODO: modify this method to return GetPayloadResponse instead
// of throwing away the `block_value` and returning only the
// ExecutionPayload
Ok(self
.get_payload_v2(fork_name, payload_id)
.await?
.execution_payload())
self.get_payload_v2(fork_name, payload_id).await
} else if engine_capabilities.new_payload_v1 {
self.get_payload_v1(payload_id).await
} else {
Expand Down
95 changes: 79 additions & 16 deletions beacon_node/execution_layer/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,13 @@ impl From<ApiError> for Error {
}

pub enum BlockProposalContents<T: EthSpec, Payload: AbstractExecPayload<T>> {
Payload(Payload),
Payload {
payload: Payload,
block_value: Uint256,
},
PayloadAndBlobs {
payload: Payload,
block_value: Uint256,
kzg_commitments: Vec<KzgCommitment>,
blobs: Vec<Blob<T>>,
},
Expand All @@ -130,51 +134,85 @@ pub enum BlockProposalContents<T: EthSpec, Payload: AbstractExecPayload<T>> {
impl<T: EthSpec, Payload: AbstractExecPayload<T>> BlockProposalContents<T, Payload> {
pub fn payload(&self) -> &Payload {
match self {
Self::Payload(payload) => payload,
Self::Payload {
payload,
block_value: _,
} => payload,
Self::PayloadAndBlobs {
payload,
block_value: _,
kzg_commitments: _,
blobs: _,
} => payload,
}
}
pub fn to_payload(self) -> Payload {
match self {
Self::Payload(payload) => payload,
Self::Payload {
payload,
block_value: _,
} => payload,
Self::PayloadAndBlobs {
payload,
block_value: _,
kzg_commitments: _,
blobs: _,
} => payload,
}
}
pub fn kzg_commitments(&self) -> Option<&[KzgCommitment]> {
match self {
Self::Payload(_) => None,
Self::Payload {
payload: _,
block_value: _,
} => None,
Self::PayloadAndBlobs {
payload: _,
block_value: _,
kzg_commitments,
blobs: _,
} => Some(kzg_commitments),
}
}
pub fn blobs(&self) -> Option<&[Blob<T>]> {
match self {
Self::Payload(_) => None,
Self::Payload {
payload: _,
block_value: _,
} => None,
Self::PayloadAndBlobs {
payload: _,
block_value: _,
kzg_commitments: _,
blobs,
} => Some(blobs),
}
}
pub fn block_value(&self) -> &Uint256 {
match self {
Self::Payload {
payload: _,
block_value,
} => block_value,
Self::PayloadAndBlobs {
payload: _,
block_value,
kzg_commitments: _,
blobs: _,
} => block_value,
}
}
pub fn default_at_fork(fork_name: ForkName) -> Result<Self, BeaconStateError> {
Ok(match fork_name {
ForkName::Base | ForkName::Altair | ForkName::Merge | ForkName::Capella => {
BlockProposalContents::Payload(Payload::default_at_fork(fork_name)?)
BlockProposalContents::Payload {
payload: Payload::default_at_fork(fork_name)?,
block_value: Uint256::zero(),
}
}
ForkName::Eip4844 => BlockProposalContents::PayloadAndBlobs {
payload: Payload::default_at_fork(fork_name)?,
block_value: Uint256::zero(),
blobs: vec![],
kzg_commitments: vec![],
},
Expand Down Expand Up @@ -808,6 +846,18 @@ impl<T: EthSpec> ExecutionLayer<T> {
"parent_hash" => ?parent_hash,
);

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(local));
}

match verify_builder_bid(
&relay,
parent_hash,
Expand All @@ -818,7 +868,10 @@ impl<T: EthSpec> ExecutionLayer<T> {
spec,
) {
Ok(()) => Ok(ProvenancedPayload::Builder(
BlockProposalContents::Payload(relay.data.message.header),
BlockProposalContents::Payload {
payload: relay.data.message.header,
block_value: relay.data.message.value,
},
)),
Err(reason) if !reason.payload_invalid() => {
info!(
Expand Down Expand Up @@ -869,12 +922,18 @@ impl<T: EthSpec> ExecutionLayer<T> {
spec,
) {
Ok(()) => Ok(ProvenancedPayload::Builder(
BlockProposalContents::Payload(relay.data.message.header),
BlockProposalContents::Payload {
payload: relay.data.message.header,
block_value: relay.data.message.value,
},
)),
// 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::Builder(
BlockProposalContents::Payload(relay.data.message.header),
BlockProposalContents::Payload {
payload: relay.data.message.header,
block_value: relay.data.message.value,
},
)),
Err(reason) => {
metrics::inc_counter_vec(
Expand Down Expand Up @@ -1071,9 +1130,9 @@ impl<T: EthSpec> ExecutionLayer<T> {
);
engine.api.get_payload::<T>(current_fork, payload_id).await
};
let (blob, payload) = tokio::join!(blob_fut, payload_fut);
let payload = payload.map(|full_payload| {
if full_payload.fee_recipient() != payload_attributes.suggested_fee_recipient() {
let (blob, payload_response) = tokio::join!(blob_fut, payload_fut);
let (payload, block_value) = payload_response.map(|payload_response| {
if payload_response.execution_payload().fee_recipient() != payload_attributes.suggested_fee_recipient() {
error!(
self.log(),
"Inconsistent fee recipient";
Expand All @@ -1082,28 +1141,32 @@ impl<T: EthSpec> ExecutionLayer<T> {
indicate that fees are being diverted to another address. Please \
ensure that the value of suggested_fee_recipient is set correctly and \
that the Execution Engine is trusted.",
"fee_recipient" => ?full_payload.fee_recipient(),
"fee_recipient" => ?payload_response.execution_payload().fee_recipient(),
"suggested_fee_recipient" => ?payload_attributes.suggested_fee_recipient(),
);
}
if f(self, &full_payload).is_some() {
if f(self, &payload_response.execution_payload().clone_from_ref()).is_some() {
warn!(
self.log(),
"Duplicate payload cached, this might indicate redundant proposal \
attempts."
);
}
full_payload.into()
(payload_response.execution_payload().clone_from_ref().into(), *payload_response.block_value())
})?;
if let Some(blob) = blob.transpose()? {
// FIXME(sean) cache blobs
Ok(BlockProposalContents::PayloadAndBlobs {
payload,
block_value,
blobs: blob.blobs,
kzg_commitments: blob.kzgs,
})
} else {
Ok(BlockProposalContents::Payload(payload))
Ok(BlockProposalContents::Payload {
payload,
block_value,
})
}
})
.await
Expand Down
10 changes: 10 additions & 0 deletions consensus/types/src/execution_payload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,16 @@ pub struct ExecutionPayload<T: EthSpec> {
pub withdrawals: Withdrawals<T>,
}

impl<'a, T: EthSpec> ExecutionPayloadRef<'a, T> {
// this emulates clone on a normal reference type
pub fn clone_from_ref(&self) -> ExecutionPayload<T> {
map_execution_payload_ref!(&'a _, self, move |payload, cons| {
cons(payload);
payload.clone().into()
})
}
}

impl<T: EthSpec> ExecutionPayload<T> {
pub fn from_ssz_bytes(bytes: &[u8], fork_name: ForkName) -> Result<Self, ssz::DecodeError> {
match fork_name {
Expand Down