diff --git a/bin/reth/Cargo.toml b/bin/reth/Cargo.toml index 788af5fa49cb..9d2f7f851902 100644 --- a/bin/reth/Cargo.toml +++ b/bin/reth/Cargo.toml @@ -121,6 +121,7 @@ min-debug-logs = ["tracing/release_max_level_debug"] min-trace-logs = ["tracing/release_max_level_trace"] optimism = [ "reth-primitives/optimism", + "reth-primitives/value-256", "reth-revm/optimism", "reth-interfaces/optimism", "reth-rpc/optimism", diff --git a/bin/reth/src/args/payload_builder_args.rs b/bin/reth/src/args/payload_builder_args.rs index 93f9ad2571d4..dbb6e679efac 100644 --- a/bin/reth/src/args/payload_builder_args.rs +++ b/bin/reth/src/args/payload_builder_args.rs @@ -98,7 +98,7 @@ impl TypedValueParser for ExtradataValueParser { format!( "Payload builder extradata size exceeds {MAXIMUM_EXTRA_DATA_SIZE}bytes limit" ), - )) + )); } Ok(val.to_string()) } diff --git a/bin/reth/src/debug_cmd/build_block.rs b/bin/reth/src/debug_cmd/build_block.rs index 9aa05b51f97d..39ebce44d370 100644 --- a/bin/reth/src/debug_cmd/build_block.rs +++ b/bin/reth/src/debug_cmd/build_block.rs @@ -246,8 +246,11 @@ impl Command { // TODO: add support for withdrawals withdrawals: None, #[cfg(feature = "optimism")] - optimism_payload_attributes: reth_rpc_types::engine::OptimismPayloadAttributes::default( - ), + optimism_payload_attributes: reth_rpc_types::engine::OptimismPayloadAttributes { + transactions: None, + no_tx_pool: None, + gas_limit: None, + }, }; let payload_config = PayloadConfig::new( Arc::clone(&best_block), diff --git a/bin/reth/src/lib.rs b/bin/reth/src/lib.rs index 6e01fc52ceb8..a092838effad 100644 --- a/bin/reth/src/lib.rs +++ b/bin/reth/src/lib.rs @@ -39,6 +39,7 @@ pub mod dirs; pub mod init; pub mod node; pub mod p2p; +pub mod precaution; pub mod prometheus_exporter; pub mod recover; pub mod runner; diff --git a/bin/reth/src/main.rs b/bin/reth/src/main.rs index 220b04d9d0e7..f0aef027c3a1 100644 --- a/bin/reth/src/main.rs +++ b/bin/reth/src/main.rs @@ -4,6 +4,10 @@ static ALLOC: jemallocator::Jemalloc = jemallocator::Jemalloc; fn main() { + // Ensure feature flags are set correctly for different chains + reth::precaution::ensure_ethereum!(); + reth::precaution::ensure_optimism!(); + if let Err(err) = reth::cli::run() { eprintln!("Error: {err:?}"); std::process::exit(1); diff --git a/bin/reth/src/node/mod.rs b/bin/reth/src/node/mod.rs index 4120379bd4a7..aeb4299530ac 100644 --- a/bin/reth/src/node/mod.rs +++ b/bin/reth/src/node/mod.rs @@ -87,6 +87,11 @@ use std::{ use tokio::sync::{mpsc::unbounded_channel, oneshot, watch}; use tracing::*; +#[cfg(feature = "optimism")] +use reth_rpc_api::EngineApiClient; +#[cfg(feature = "optimism")] +use reth_rpc_types::engine::ForkchoiceState; + pub mod cl_events; pub mod events; @@ -558,9 +563,9 @@ impl NodeCommand { #[cfg(feature = "optimism")] if self.chain.optimism && !self.rollup.enable_genesis_walkback { let client = _rpc_server_handles.auth.http_client(); - reth_rpc_api::EngineApiClient::fork_choice_updated_v2( + EngineApiClient::fork_choice_updated_v2( &client, - reth_rpc_types::engine::ForkchoiceState { + ForkchoiceState { head_block_hash: head.hash, safe_block_hash: head.hash, finalized_block_hash: head.hash, @@ -789,7 +794,7 @@ impl NodeCommand { // try to look up the header in the database if let Some(header) = header { info!(target: "reth::cli", ?tip, "Successfully looked up tip block in the database"); - return Ok(header.seal_slow()) + return Ok(header.seal_slow()); } info!(target: "reth::cli", ?tip, "Fetching tip block from the network."); @@ -797,7 +802,7 @@ impl NodeCommand { match get_single_header(&client, tip).await { Ok(tip_header) => { info!(target: "reth::cli", ?tip, "Successfully fetched tip"); - return Ok(tip_header) + return Ok(tip_header); } Err(error) => { error!(target: "reth::cli", %error, "Failed to fetch the tip. Retrying..."); diff --git a/bin/reth/src/precaution.rs b/bin/reth/src/precaution.rs new file mode 100644 index 000000000000..0750f65ec534 --- /dev/null +++ b/bin/reth/src/precaution.rs @@ -0,0 +1,24 @@ +//! Helpers to ensure certain features are enabled or disabled. +//! +//! The motivation for this is to prevent that a binary is accidentally built with a feature that is +//! not intended to be used. +//! +//! Currently conflicting features are: `value-u256` which is required by optimism. + +/// A macro to ensure that the crate's features are compatible with ethereum +#[macro_export] +macro_rules! ensure_ethereum { + () => { + #[cfg(feature = "value-256")] + compile_error!("The `value-256` feature is enabled but for `ethereum` it must be disabled: https://github.com/paradigmxyz/reth/issues/4891"); + }; +} + +/// A macro to ensure that the crate's features are compatible with optimism +#[macro_export] +macro_rules! ensure_optimism { + () => { + #[cfg(not(feature = "value-256"))] + compile_error!("The `value-256` feature is disabled but for `optimism` it must be enabled: https://github.com/paradigmxyz/reth/issues/4891"); + }; +} diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index 8d86dfb806c9..06b7cd58b05f 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -355,7 +355,7 @@ where inconsistent_stage_checkpoint = stage_checkpoint, "Pipeline sync progress is inconsistent" ); - return self.blockchain.block_hash(first_stage_checkpoint) + return self.blockchain.block_hash(first_stage_checkpoint); } } @@ -431,7 +431,7 @@ where Ok(None) => { // we don't have the block yet and the distance exceeds the allowed // threshold - return Some(state.finalized_block_hash) + return Some(state.finalized_block_hash); } Ok(Some(_)) => { // we're fully synced to the finalized block @@ -472,7 +472,7 @@ where ) -> Option { // check pre merge block error if insert_err.map(|err| err.is_block_pre_merge()).unwrap_or_default() { - return Some(B256::ZERO) + return Some(B256::ZERO); } // If this is sent from new payload then the parent hash could be in a side chain, and is @@ -487,7 +487,7 @@ where // we need to check if the parent block is the last POW block, if so then the payload is // the first POS. The engine API spec mandates a zero hash to be returned: if parent_header.difficulty != U256::ZERO { - return Some(B256::ZERO) + return Some(B256::ZERO); } // parent is canonical POS block @@ -571,11 +571,11 @@ where // FCU resulted in a fatal error from which we can't recover let err = err.clone(); let _ = tx.send(Err(error)); - return OnForkchoiceUpdateOutcome::Fatal(err) + return OnForkchoiceUpdateOutcome::Fatal(err); } } let _ = tx.send(Err(error)); - return OnForkchoiceUpdateOutcome::Processed + return OnForkchoiceUpdateOutcome::Processed; } }; @@ -600,7 +600,7 @@ where if self.sync.has_reached_max_block(tip_number) { // Terminate the sync early if it's reached the maximum user // configured block. - return OnForkchoiceUpdateOutcome::ReachedMaxBlock + return OnForkchoiceUpdateOutcome::ReachedMaxBlock; } } ForkchoiceStatus::Syncing => { @@ -629,21 +629,21 @@ where ) -> RethResult { trace!(target: "consensus::engine", ?state, "Received new forkchoice state update"); if state.head_block_hash.is_zero() { - return Ok(OnForkChoiceUpdated::invalid_state()) + return Ok(OnForkChoiceUpdated::invalid_state()); } // check if the new head hash is connected to any ancestor that we previously marked as // invalid let lowest_buffered_ancestor_fcu = self.lowest_buffered_ancestor_or(state.head_block_hash); if let Some(status) = self.check_invalid_ancestor(lowest_buffered_ancestor_fcu) { - return Ok(OnForkChoiceUpdated::with_invalid(status)) + return Ok(OnForkChoiceUpdated::with_invalid(status)); } if self.sync.is_pipeline_active() { // We can only process new forkchoice updates if the pipeline is idle, since it requires // exclusive access to the database trace!(target: "consensus::engine", "Pipeline is syncing, skipping forkchoice update"); - return Ok(OnForkChoiceUpdated::syncing()) + return Ok(OnForkChoiceUpdated::syncing()); } if self.hooks.is_hook_with_db_write_running() { @@ -654,7 +654,7 @@ where "Hook is in progress, skipping forkchoice update. \ This may affect the performance of your node as a validator." ); - return Ok(OnForkChoiceUpdated::syncing()) + return Ok(OnForkChoiceUpdated::syncing()); } let start = Instant::now(); @@ -719,7 +719,7 @@ where // attributes if let Some(invalid_fcu_response) = self.ensure_consistent_state(state)? { trace!(target: "consensus::engine", ?state, head=?state.head_block_hash, "Forkchoice state is inconsistent, returning invalid response"); - return Ok(invalid_fcu_response) + return Ok(invalid_fcu_response); } // the CL requested to build a new payload on top of this new VALID head @@ -730,7 +730,7 @@ where ); trace!(target: "consensus::engine", status = ?payload_response, ?state, "Returning forkchoice status"); - return Ok(payload_response) + return Ok(payload_response); } PayloadStatus::new(PayloadStatusEnum::Valid, Some(state.head_block_hash)) @@ -739,7 +739,7 @@ where if let RethError::Canonical(ref err) = error { if err.is_fatal() { tracing::error!(target: "consensus::engine", ?err, "Encountered fatal error"); - return Err(error) + return Err(error); } } @@ -751,7 +751,7 @@ where self.ensure_consistent_state_with_status(state, &status)? { trace!(target: "consensus::engine", ?status, ?state, "Forkchoice state is inconsistent, returning invalid response"); - return Ok(invalid_fcu_response) + return Ok(invalid_fcu_response); } trace!(target: "consensus::engine", ?status, ?state, "Returning forkchoice status"); @@ -807,7 +807,7 @@ where // we likely do not have the finalized or safe blocks, and would return an incorrect // INVALID status instead. if status.is_valid() { - return self.ensure_consistent_state(state) + return self.ensure_consistent_state(state); } Ok(None) @@ -833,7 +833,7 @@ where if !state.finalized_block_hash.is_zero() && !self.blockchain.is_canonical(state.finalized_block_hash)? { - return Ok(Some(OnForkChoiceUpdated::invalid_state())) + return Ok(Some(OnForkChoiceUpdated::invalid_state())); } // Finalized block is consistent, so update it in the canon chain tracker. @@ -847,7 +847,7 @@ where if !state.safe_block_hash.is_zero() && !self.blockchain.is_canonical(state.safe_block_hash)? { - return Ok(Some(OnForkChoiceUpdated::invalid_state())) + return Ok(Some(OnForkChoiceUpdated::invalid_state())); } // Safe block is consistent, so update it in the canon chain tracker. @@ -911,7 +911,7 @@ where if !safe_block_hash.is_zero() { if self.blockchain.safe_block_hash()? == Some(safe_block_hash) { // nothing to update - return Ok(()) + return Ok(()); } let safe = @@ -931,7 +931,7 @@ where if !finalized_block_hash.is_zero() { if self.blockchain.finalized_block_hash()? == Some(finalized_block_hash) { // nothing to update - return Ok(()) + return Ok(()); } let finalized = self @@ -965,7 +965,7 @@ where if let Some(invalid_ancestor) = self.check_invalid_ancestor(state.head_block_hash) { warn!(target: "consensus::engine", ?error, ?state, ?invalid_ancestor, head=?state.head_block_hash, "Failed to canonicalize the head hash, head is also considered invalid"); debug!(target: "consensus::engine", head=?state.head_block_hash, current_error=?error, "Head was previously marked as invalid"); - return invalid_ancestor + return invalid_ancestor; } #[allow(clippy::single_match)] @@ -977,7 +977,7 @@ where return PayloadStatus::from_status(PayloadStatusEnum::Invalid { validation_error: error.to_string(), }) - .with_latest_valid_hash(B256::ZERO) + .with_latest_valid_hash(B256::ZERO); } RethError::BlockchainTree(BlockchainTreeError::BlockHashNotFoundInChain { .. }) => { // This just means we couldn't find the block when attempting to make it canonical, @@ -1059,7 +1059,7 @@ where // begin a payload build process. In such an event, the forkchoiceState update MUST NOT // be rolled back. if attrs.timestamp.to::() <= head.timestamp { - return OnForkChoiceUpdated::invalid_payload_attributes() + return OnForkChoiceUpdated::invalid_payload_attributes(); } // 8. Client software MUST begin a payload build process building on top of @@ -1126,7 +1126,7 @@ where if let Some(status) = self.check_invalid_ancestor_with_head(lowest_buffered_ancestor, block.hash) { - return Ok(status) + return Ok(status); } let res = if self.sync.is_pipeline_idle() && !self.hooks.is_hook_with_db_write_running() { @@ -1214,7 +1214,7 @@ where } let status = PayloadStatusEnum::from(error); - return Err(PayloadStatus::new(status, latest_valid_hash)) + return Err(PayloadStatus::new(status, latest_valid_hash)); } }; @@ -1269,7 +1269,7 @@ where let latest_valid_hash = self.latest_valid_hash_for_invalid_payload(parent_hash, None); let status = PayloadStatusEnum::from(PayloadError::InvalidVersionedHashes); - return Err(PayloadStatus::new(status, latest_valid_hash)) + return Err(PayloadStatus::new(status, latest_valid_hash)); } // we can use `zip` safely here because we already compared their length @@ -1281,7 +1281,7 @@ where let latest_valid_hash = self.latest_valid_hash_for_invalid_payload(parent_hash, None); let status = PayloadStatusEnum::from(PayloadError::InvalidVersionedHashes); - return Err(PayloadStatus::new(status, latest_valid_hash)) + return Err(PayloadStatus::new(status, latest_valid_hash)); } } } else if !block_versioned_hashes.is_empty() { @@ -1289,7 +1289,7 @@ where // provided in the new payload call, so the payload is invalid let latest_valid_hash = self.latest_valid_hash_for_invalid_payload(parent_hash, None); let status = PayloadStatusEnum::from(PayloadError::InvalidVersionedHashes); - return Err(PayloadStatus::new(status, latest_valid_hash)) + return Err(PayloadStatus::new(status, latest_valid_hash)); } Ok(()) @@ -1345,7 +1345,7 @@ where if let Some(status) = self.check_invalid_ancestor_with_head(block.parent_hash, block.hash) { - return Ok(status) + return Ok(status); } // not known to be invalid, but we don't know anything else @@ -1444,7 +1444,7 @@ where // check if the block's parent is already marked as invalid if self.check_invalid_ancestor_with_head(block.parent_hash, block.hash).is_some() { // can skip this invalid block - return + return; } match self @@ -1510,7 +1510,7 @@ where // threshold self.sync.set_pipeline_sync_target(target); // we can exit early here because the pipeline will take care of syncing - return + return; } // continue downloading the missing parent @@ -1613,7 +1613,7 @@ where } EngineSyncEvent::PipelineTaskDropped => { error!(target: "consensus::engine", "Failed to receive spawned pipeline"); - return Some(Err(BeaconConsensusEngineError::PipelineChannelClosed)) + return Some(Err(BeaconConsensusEngineError::PipelineChannelClosed)); } EngineSyncEvent::PipelineFinished { result, reached_max_block } => { return self.on_pipeline_finished(result, reached_max_block) @@ -1651,7 +1651,7 @@ where // update the `invalid_headers` cache with the new invalid headers self.invalid_headers.insert(bad_block); - return None + return None; } // update the canon chain if continuous is enabled @@ -1669,7 +1669,7 @@ where }, Err(error) => { error!(target: "consensus::engine", ?error, "Error getting canonical header for continuous sync"); - return Some(Err(error.into())) + return Some(Err(error.into())); } }; self.blockchain.set_canonical_head(max_header); @@ -1681,7 +1681,7 @@ where // This is only possible if the node was run with `debug.tip` // argument and without CL. warn!(target: "consensus::engine", "No fork choice state available"); - return None + return None; } }; @@ -1751,7 +1751,7 @@ where } Err(error) => { error!(target: "consensus::engine", ?error, "Error restoring blockchain tree state"); - return Some(Err(error.into())) + return Some(Err(error.into())); } }; } @@ -1789,7 +1789,7 @@ where self.blockchain.connect_buffered_blocks_to_canonical_hashes() { error!(target: "consensus::engine", ?error, "Error connecting buffered blocks to canonical hashes on hook result"); - return Err(error.into()) + return Err(error.into()); } } } @@ -1854,11 +1854,11 @@ where OnForkchoiceUpdateOutcome::Processed => {} OnForkchoiceUpdateOutcome::ReachedMaxBlock => { // reached the max block, we can terminate the future - return Poll::Ready(Ok(())) + return Poll::Ready(Ok(())); } OnForkchoiceUpdateOutcome::Fatal(err) => { // fatal error, we can terminate the future - return Poll::Ready(Err(RethError::Execution(err).into())) + return Poll::Ready(Err(RethError::Execution(err).into())); } } } @@ -1886,7 +1886,7 @@ where match this.sync.poll(cx) { Poll::Ready(sync_event) => { if let Some(res) = this.on_sync_event(sync_event) { - return Poll::Ready(res) + return Poll::Ready(res); } // this could have taken a while, so we start the next cycle to handle any new // engine messages @@ -1921,7 +1921,7 @@ where // incoming engine messages and sync events are drained, so we can yield back // control - return Poll::Pending + return Poll::Pending; } } } diff --git a/crates/primitives/Cargo.toml b/crates/primitives/Cargo.toml index 51383642fcee..2deb9d823429 100644 --- a/crates/primitives/Cargo.toml +++ b/crates/primitives/Cargo.toml @@ -80,6 +80,7 @@ hash-db = "~0.15" # value-256 is needed for the main_codec proptests to pass reth-primitives = { path = ".", features = ["value-256"] } + # necessary so we don't hit a "undeclared 'std'": # https://github.com/paradigmxyz/reth/pull/177#discussion_r1021172198 secp256k1.workspace = true @@ -94,7 +95,7 @@ test-utils = ["dep:plain_hasher", "dep:hash-db", "dep:ethers-core"] # default of 128 bits. value-256 = ["reth-codecs/value-256"] clap = ["dep:clap"] -optimism = ["value-256"] +optimism = [] [[bench]] name = "recover_ecdsa_crit" diff --git a/crates/primitives/src/lib.rs b/crates/primitives/src/lib.rs index b5aa3edfb15d..7a08c6b07610 100644 --- a/crates/primitives/src/lib.rs +++ b/crates/primitives/src/lib.rs @@ -17,9 +17,6 @@ #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] #![allow(clippy::non_canonical_clone_impl)] -#[cfg(all(feature = "value-256", not(feature = "optimism")))] -compile_error!("The `optimism` feature flag is required if `value-256` is enabled."); - mod account; pub mod basefee; mod block; diff --git a/crates/rpc/rpc-types/src/eth/engine/payload.rs b/crates/rpc/rpc-types/src/eth/engine/payload.rs index a64de3a3b82e..dad2087a1195 100644 --- a/crates/rpc/rpc-types/src/eth/engine/payload.rs +++ b/crates/rpc/rpc-types/src/eth/engine/payload.rs @@ -385,7 +385,7 @@ pub struct PayloadAttributes { } /// Optimism Payload Attributes -#[derive(Clone, Default, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] #[cfg(feature = "optimism")] pub struct OptimismPayloadAttributes { diff --git a/crates/storage/codecs/derive/src/compact/mod.rs b/crates/storage/codecs/derive/src/compact/mod.rs index 701ec8d89068..9f6ee1062400 100644 --- a/crates/storage/codecs/derive/src/compact/mod.rs +++ b/crates/storage/codecs/derive/src/compact/mod.rs @@ -164,9 +164,9 @@ pub fn get_bit_size(ftype: &str) -> u8 { "u64" | "BlockNumber" | "TxNumber" | "ChainId" | "NumTransactions" => 4, "u128" => 5, "U256" => 6, - #[cfg(not(feature = "optimism"))] + #[cfg(not(feature = "value-256"))] "TxValue" => 5, // u128 for ethereum chains assuming high order bits are not used - #[cfg(feature = "optimism")] + #[cfg(feature = "value-256")] // for fuzz/prop testing and chains that may require full 256 bits "TxValue" => 6, _ => 0,