Skip to content

Commit

Permalink
refactor: replace extra fields with ExecutionPayloadSidecar in engine
Browse files Browse the repository at this point in the history
  • Loading branch information
onbjerg committed Oct 23, 2024
1 parent 5e0ba41 commit cae526e
Show file tree
Hide file tree
Showing 17 changed files with 179 additions and 215 deletions.
2 changes: 0 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 2 additions & 4 deletions bin/reth/src/commands/debug_cmd/replay_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,8 @@ impl<C: ChainSpecParser<ChainSpec = ChainSpec>> Command<C> {
beacon_engine_handle.fork_choice_updated(state, payload_attrs).await?;
debug!(target: "reth::cli", ?response, "Received for forkchoice updated");
}
StoredEngineApiMessage::NewPayload { payload, cancun_fields } => {
// todo: prague (last arg)
let response =
beacon_engine_handle.new_payload(payload, cancun_fields, None).await?;
StoredEngineApiMessage::NewPayload { payload, sidecar } => {
let response = beacon_engine_handle.new_payload(payload, sidecar).await?;
debug!(target: "reth::cli", ?response, "Received for new payload");
}
};
Expand Down
13 changes: 6 additions & 7 deletions crates/consensus/beacon/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ reth-node-types.workspace = true
reth-chainspec = { workspace = true, optional = true }

# ethereum
alloy-eips.workspace = true
alloy-primitives.workspace = true
alloy-rpc-types-engine.workspace = true

Expand Down Expand Up @@ -78,10 +77,10 @@ assert_matches.workspace = true

[features]
optimism = [
"reth-chainspec",
"reth-primitives/optimism",
"reth-provider/optimism",
"reth-blockchain-tree/optimism",
"reth-db/optimism",
"reth-db-api/optimism"
"reth-chainspec",
"reth-primitives/optimism",
"reth-provider/optimism",
"reth-blockchain-tree/optimism",
"reth-db/optimism",
"reth-db-api/optimism",
]
15 changes: 3 additions & 12 deletions crates/consensus/beacon/src/engine/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@ use crate::{
engine::message::OnForkChoiceUpdated, BeaconConsensusEngineEvent, BeaconEngineMessage,
BeaconForkChoiceUpdateError, BeaconOnNewPayloadError,
};
use alloy_eips::eip7685::Requests;
use alloy_rpc_types_engine::{
CancunPayloadFields, ExecutionPayload, ForkchoiceState, ForkchoiceUpdated, PayloadStatus,
ExecutionPayload, ExecutionPayloadSidecar, ForkchoiceState, ForkchoiceUpdated, PayloadStatus,
};
use futures::TryFutureExt;
use reth_engine_primitives::EngineTypes;
Expand Down Expand Up @@ -47,18 +46,10 @@ where
pub async fn new_payload(
&self,
payload: ExecutionPayload,
cancun_fields: Option<CancunPayloadFields>,
execution_requests: Option<Requests>,
sidecar: ExecutionPayloadSidecar,
) -> Result<PayloadStatus, BeaconOnNewPayloadError> {
let (tx, rx) = oneshot::channel();
// HACK(onbjerg): We should have a pectra payload fields struct, this is just a temporary
// workaround.
let _ = self.to_engine.send(BeaconEngineMessage::NewPayload {
payload,
cancun_fields,
execution_requests,
tx,
});
let _ = self.to_engine.send(BeaconEngineMessage::NewPayload { payload, sidecar, tx });
rx.await.map_err(|_| BeaconOnNewPayloadError::EngineUnavailable)?
}

Expand Down
12 changes: 4 additions & 8 deletions crates/consensus/beacon/src/engine/message.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use crate::engine::{error::BeaconOnNewPayloadError, forkchoice::ForkchoiceStatus};
use alloy_eips::eip7685::Requests;
use alloy_rpc_types_engine::{
CancunPayloadFields, ExecutionPayload, ForkChoiceUpdateResult, ForkchoiceState,
ExecutionPayload, ExecutionPayloadSidecar, ForkChoiceUpdateResult, ForkchoiceState,
ForkchoiceUpdateError, ForkchoiceUpdated, PayloadId, PayloadStatus, PayloadStatusEnum,
};
use futures::{future::Either, FutureExt};
Expand Down Expand Up @@ -145,12 +144,9 @@ pub enum BeaconEngineMessage<Engine: EngineTypes> {
NewPayload {
/// The execution payload received by Engine API.
payload: ExecutionPayload,
/// The cancun-related newPayload fields, if any.
cancun_fields: Option<CancunPayloadFields>,
// HACK(onbjerg): We should have a pectra payload fields struct, this is just a temporary
// workaround.
/// The pectra EIP-7685 execution requests.
execution_requests: Option<Requests>,
/// The execution payload sidecar with additional version-specific fields received by
/// engine API.
sidecar: ExecutionPayloadSidecar,
/// The sender for returning payload status result.
tx: oneshot::Sender<Result<PayloadStatus, BeaconOnNewPayloadError>>,
},
Expand Down
50 changes: 25 additions & 25 deletions crates/consensus/beacon/src/engine/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use alloy_eips::eip7685::Requests;
use alloy_primitives::{BlockNumber, B256};
use alloy_rpc_types_engine::{
CancunPayloadFields, ExecutionPayload, ForkchoiceState, PayloadStatus, PayloadStatusEnum,
ExecutionPayload, ExecutionPayloadSidecar, ForkchoiceState, PayloadStatus, PayloadStatusEnum,
PayloadValidationError,
};
use futures::{stream::BoxStream, Future, StreamExt};
Expand Down Expand Up @@ -1081,14 +1080,11 @@ where
///
/// This returns a [`PayloadStatus`] that represents the outcome of a processed new payload and
/// returns an error if an internal error occurred.
#[instrument(level = "trace", skip(self, payload, cancun_fields), fields(block_hash = ?payload.block_hash(), block_number = %payload.block_number(), is_pipeline_idle = %self.sync.is_pipeline_idle()), target = "consensus::engine")]
#[instrument(level = "trace", skip(self, payload, sidecar), fields(block_hash = ?payload.block_hash(), block_number = %payload.block_number(), is_pipeline_idle = %self.sync.is_pipeline_idle()), target = "consensus::engine")]
fn on_new_payload(
&mut self,
payload: ExecutionPayload,
cancun_fields: Option<CancunPayloadFields>,
// HACK(onbjerg): We should have a pectra payload fields struct, this is just a temporary
// workaround.
execution_requests: Option<Requests>,
sidecar: ExecutionPayloadSidecar,
) -> Result<Either<PayloadStatus, SealedBlock>, BeaconOnNewPayloadError> {
self.metrics.new_payload_messages.increment(1);

Expand Down Expand Up @@ -1118,11 +1114,7 @@ where
//
// This validation **MUST** be instantly run in all cases even during active sync process.
let parent_hash = payload.parent_hash();
let block = match self.payload_validator.ensure_well_formed_payload(
payload,
cancun_fields.into(),
execution_requests,
) {
let block = match self.payload_validator.ensure_well_formed_payload(payload, sidecar) {
Ok(block) => block,
Err(error) => {
error!(target: "consensus::engine", %error, "Invalid payload");
Expand Down Expand Up @@ -1867,13 +1859,8 @@ where
BeaconEngineMessage::ForkchoiceUpdated { state, payload_attrs, tx } => {
this.on_forkchoice_updated(state, payload_attrs, tx);
}
BeaconEngineMessage::NewPayload {
payload,
cancun_fields,
execution_requests,
tx,
} => {
match this.on_new_payload(payload, cancun_fields, execution_requests) {
BeaconEngineMessage::NewPayload { payload, sidecar, tx } => {
match this.on_new_payload(payload, sidecar) {
Ok(Either::Right(block)) => {
this.set_blockchain_tree_action(
BlockchainTreeAction::InsertNewPayload { block, tx },
Expand Down Expand Up @@ -2061,7 +2048,12 @@ mod tests {
assert_matches!(rx.try_recv(), Err(TryRecvError::Empty));

// consensus engine is still idle because no FCUs were received
let _ = env.send_new_payload(block_to_payload_v1(SealedBlock::default()), None).await;
let _ = env
.send_new_payload(
block_to_payload_v1(SealedBlock::default()),
ExecutionPayloadSidecar::none(),
)
.await;

assert_matches!(rx.try_recv(), Err(TryRecvError::Empty));

Expand Down Expand Up @@ -2626,7 +2618,7 @@ mod tests {
0,
BlockParams { ommers_count: Some(0), ..Default::default() },
)),
None,
ExecutionPayloadSidecar::none(),
)
.await;

Expand All @@ -2641,7 +2633,7 @@ mod tests {
1,
BlockParams { ommers_count: Some(0), ..Default::default() },
)),
None,
ExecutionPayloadSidecar::none(),
)
.await;

Expand Down Expand Up @@ -2719,7 +2711,10 @@ mod tests {

// Send new payload
let result = env
.send_new_payload_retry_on_syncing(block_to_payload_v1(block2.clone()), None)
.send_new_payload_retry_on_syncing(
block_to_payload_v1(block2.clone()),
ExecutionPayloadSidecar::none(),
)
.await
.unwrap();

Expand Down Expand Up @@ -2854,7 +2849,9 @@ mod tests {
2,
BlockParams { parent: Some(parent), ommers_count: Some(0), ..Default::default() },
);
let res = env.send_new_payload(block_to_payload_v1(block), None).await;
let res = env
.send_new_payload(block_to_payload_v1(block), ExecutionPayloadSidecar::none())
.await;
let expected_result = PayloadStatus::from_status(PayloadStatusEnum::Syncing);
assert_matches!(res, Ok(result) => assert_eq!(result, expected_result));

Expand Down Expand Up @@ -2924,7 +2921,10 @@ mod tests {

// Send new payload
let result = env
.send_new_payload_retry_on_syncing(block_to_payload_v1(block2.clone()), None)
.send_new_payload_retry_on_syncing(
block_to_payload_v1(block2.clone()),
ExecutionPayloadSidecar::none(),
)
.await
.unwrap();

Expand Down
10 changes: 5 additions & 5 deletions crates/consensus/beacon/src/engine/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
};
use alloy_primitives::{BlockNumber, Sealable, B256};
use alloy_rpc_types_engine::{
CancunPayloadFields, ExecutionPayload, ForkchoiceState, ForkchoiceUpdated, PayloadStatus,
ExecutionPayload, ExecutionPayloadSidecar, ForkchoiceState, ForkchoiceUpdated, PayloadStatus,
};
use reth_blockchain_tree::{
config::BlockchainTreeConfig, externals::TreeExternals, BlockchainTree, ShareableBlockchainTree,
Expand Down Expand Up @@ -68,21 +68,21 @@ impl<DB> TestEnv<DB> {
pub async fn send_new_payload<T: Into<ExecutionPayload>>(
&self,
payload: T,
cancun_fields: Option<CancunPayloadFields>,
sidecar: ExecutionPayloadSidecar,
) -> Result<PayloadStatus, BeaconOnNewPayloadError> {
self.engine_handle.new_payload(payload.into(), cancun_fields, None).await
self.engine_handle.new_payload(payload.into(), sidecar).await
}

/// Sends the `ExecutionPayload` message to the consensus engine and retries if the engine
/// is syncing.
pub async fn send_new_payload_retry_on_syncing<T: Into<ExecutionPayload>>(
&self,
payload: T,
cancun_fields: Option<CancunPayloadFields>,
sidecar: ExecutionPayloadSidecar,
) -> Result<PayloadStatus, BeaconOnNewPayloadError> {
let payload: ExecutionPayload = payload.into();
loop {
let result = self.send_new_payload(payload.clone(), cancun_fields.clone()).await?;
let result = self.send_new_payload(payload.clone(), sidecar.clone()).await?;
if !result.is_syncing() {
return Ok(result)
}
Expand Down
9 changes: 5 additions & 4 deletions crates/engine/local/src/miner.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Contains the implementation of the mining mode for the local engine.
use alloy_primitives::{TxHash, B256};
use alloy_rpc_types_engine::{CancunPayloadFields, ForkchoiceState};
use alloy_rpc_types_engine::{CancunPayloadFields, ExecutionPayloadSidecar, ForkchoiceState};
use eyre::OptionExt;
use futures_util::{stream::Fuse, StreamExt};
use reth_beacon_consensus::BeaconEngineMessage;
Expand Down Expand Up @@ -221,9 +221,10 @@ where
let (tx, rx) = oneshot::channel();
self.to_engine.send(BeaconEngineMessage::NewPayload {
payload: block_to_payload(payload.block().clone()),
cancun_fields,
// todo: prague
execution_requests: None,
// todo: prague support
sidecar: cancun_fields
.map(ExecutionPayloadSidecar::v3)
.unwrap_or_else(ExecutionPayloadSidecar::none),
tx,
})?;

Expand Down
34 changes: 12 additions & 22 deletions crates/engine/tree/src/tree/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use alloy_primitives::{
BlockNumber, B256, U256,
};
use alloy_rpc_types_engine::{
CancunPayloadFields, ExecutionPayload, ForkchoiceState, PayloadStatus, PayloadStatusEnum,
ExecutionPayload, ExecutionPayloadSidecar, ForkchoiceState, PayloadStatus, PayloadStatusEnum,
PayloadValidationError,
};
use reth_beacon_consensus::{
Expand Down Expand Up @@ -70,7 +70,6 @@ use crate::{
engine::{EngineApiKind, EngineApiRequest},
tree::metrics::EngineApiMetrics,
};
use alloy_eips::eip7685::Requests;
pub use config::TreeConfig;
pub use invalid_block_hook::{InvalidBlockHooks, NoopInvalidBlockHook};
pub use persistence_state::PersistenceState;
Expand Down Expand Up @@ -722,8 +721,7 @@ where
fn on_new_payload(
&mut self,
payload: ExecutionPayload,
cancun_fields: Option<CancunPayloadFields>,
execution_requests: Option<Requests>,
sidecar: ExecutionPayloadSidecar,
) -> Result<TreeOutcome<PayloadStatus>, InsertBlockFatalError> {
trace!(target: "engine::tree", "invoked new payload");
self.metrics.engine.new_payload_messages.increment(1);
Expand Down Expand Up @@ -754,11 +752,7 @@ where
//
// This validation **MUST** be instantly run in all cases even during active sync process.
let parent_hash = payload.parent_hash();
let block = match self.payload_validator.ensure_well_formed_payload(
payload,
cancun_fields.into(),
execution_requests,
) {
let block = match self.payload_validator.ensure_well_formed_payload(payload, sidecar) {
Ok(block) => block,
Err(error) => {
error!(target: "engine::tree", %error, "Invalid payload");
Expand Down Expand Up @@ -1241,14 +1235,8 @@ where
error!(target: "engine::tree", "Failed to send event: {err:?}");
}
}
BeaconEngineMessage::NewPayload {
payload,
cancun_fields,
execution_requests,
tx,
} => {
let output =
self.on_new_payload(payload, cancun_fields, execution_requests);
BeaconEngineMessage::NewPayload { payload, sidecar, tx } => {
let output = self.on_new_payload(payload, sidecar);
if let Err(err) = tx.send(output.map(|o| o.outcome).map_err(|e| {
reth_beacon_consensus::BeaconOnNewPayloadError::Internal(
Box::new(e),
Expand Down Expand Up @@ -2585,6 +2573,7 @@ mod tests {
use crate::persistence::PersistenceAction;
use alloy_primitives::{Bytes, Sealable};
use alloy_rlp::Decodable;
use alloy_rpc_types_engine::{CancunPayloadFields, ExecutionPayloadSidecar};
use assert_matches::assert_matches;
use reth_beacon_consensus::{EthBeaconConsensus, ForkchoiceStatus};
use reth_chain_state::{test_utils::TestBlockBuilder, BlockState};
Expand Down Expand Up @@ -2862,11 +2851,10 @@ mod tests {
self.tree
.on_new_payload(
payload.into(),
Some(CancunPayloadFields {
ExecutionPayloadSidecar::v3(CancunPayloadFields {
parent_beacon_block_root: block.parent_beacon_block_root.unwrap(),
versioned_hashes: vec![],
}),
None,
)
.unwrap();
}
Expand Down Expand Up @@ -3129,7 +3117,10 @@ mod tests {

let mut test_harness = TestHarness::new(HOLESKY.clone());

let outcome = test_harness.tree.on_new_payload(payload.into(), None, None).unwrap();
let outcome = test_harness
.tree
.on_new_payload(payload.into(), ExecutionPayloadSidecar::none())
.unwrap();
assert!(outcome.outcome.is_syncing());

// ensure block is buffered
Expand Down Expand Up @@ -3173,8 +3164,7 @@ mod tests {
.on_engine_message(FromEngine::Request(
BeaconEngineMessage::NewPayload {
payload: payload.clone().into(),
cancun_fields: None,
execution_requests: None,
sidecar: ExecutionPayloadSidecar::none(),
tx,
}
.into(),
Expand Down
Loading

0 comments on commit cae526e

Please sign in to comment.