Skip to content
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.

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

feat: integrate EngineArgs into NodeCommand #13748

Merged
merged 6 commits into from
Jan 11, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
113 changes: 7 additions & 106 deletions bin/reth/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,60 +3,12 @@
#[global_allocator]
static ALLOC: reth_cli_util::allocator::Allocator = reth_cli_util::allocator::new_allocator();

use clap::{Args, Parser};
use clap::Parser;
use reth::cli::Cli;
use reth_ethereum_cli::chainspec::EthereumChainSpecParser;
use reth_node_builder::{
engine_tree_config::{
TreeConfig, DEFAULT_MEMORY_BLOCK_BUFFER_TARGET, DEFAULT_PERSISTENCE_THRESHOLD,
},
EngineNodeLauncher,
};
use reth_node_ethereum::{node::EthereumAddOns, EthereumNode};
use reth_provider::providers::BlockchainProvider;
use reth_tracing::tracing::warn;
use reth_node_ethereum::EthereumNode;
use tracing::info;

/// Parameters for configuring the engine
#[derive(Debug, Clone, Args, PartialEq, Eq)]
#[command(next_help_heading = "Engine")]
pub struct EngineArgs {
/// Enable the experimental engine features on reth binary
///
/// DEPRECATED: experimental engine is default now, use --engine.legacy to enable the legacy
/// functionality
#[arg(long = "engine.experimental", default_value = "false")]
pub experimental: bool,

/// Enable the legacy engine on reth binary
#[arg(long = "engine.legacy", default_value = "false")]
pub legacy: bool,

/// Configure persistence threshold for engine experimental.
#[arg(long = "engine.persistence-threshold", conflicts_with = "legacy", default_value_t = DEFAULT_PERSISTENCE_THRESHOLD)]
pub persistence_threshold: u64,

/// Configure the target number of blocks to keep in memory.
#[arg(long = "engine.memory-block-buffer-target", conflicts_with = "legacy", default_value_t = DEFAULT_MEMORY_BLOCK_BUFFER_TARGET)]
pub memory_block_buffer_target: u64,

/// Enable state root task
#[arg(long = "engine.state-root-task", conflicts_with = "legacy")]
pub state_root_task_enabled: bool,
}

impl Default for EngineArgs {
fn default() -> Self {
Self {
experimental: false,
legacy: false,
persistence_threshold: DEFAULT_PERSISTENCE_THRESHOLD,
memory_block_buffer_target: DEFAULT_MEMORY_BLOCK_BUFFER_TARGET,
state_root_task_enabled: false,
}
}
}

fn main() {
reth_cli_util::sigsegv_handler::install();

Expand All @@ -65,63 +17,12 @@ fn main() {
std::env::set_var("RUST_BACKTRACE", "1");
}

if let Err(err) =
Cli::<EthereumChainSpecParser, EngineArgs>::parse().run(|builder, engine_args| async move {
if engine_args.experimental {
warn!(target: "reth::cli", "Experimental engine is default now, and the --engine.experimental flag is deprecated. To enable the legacy functionality, use --engine.legacy.");
}

let use_legacy_engine = engine_args.legacy;
match use_legacy_engine {
false => {
let engine_tree_config = TreeConfig::default()
.with_persistence_threshold(engine_args.persistence_threshold)
.with_memory_block_buffer_target(engine_args.memory_block_buffer_target)
.with_state_root_task(engine_args.state_root_task_enabled);
let handle = builder
.with_types_and_provider::<EthereumNode, BlockchainProvider<_>>()
.with_components(EthereumNode::components())
.with_add_ons(EthereumAddOns::default())
.launch_with_fn(|builder| {
let launcher = EngineNodeLauncher::new(
builder.task_executor().clone(),
builder.config().datadir(),
engine_tree_config,
);
builder.launch_with(launcher)
})
.await?;
handle.node_exit_future.await
}
true => {
info!(target: "reth::cli", "Running with legacy engine");
let handle = builder.launch_node(EthereumNode::default()).await?;
handle.node_exit_future.await
}
}
})
{
if let Err(err) = Cli::<EthereumChainSpecParser>::parse().run(|builder, _| async move {
info!(target: "reth::cli", "Launching node");
let handle = builder.launch_node(EthereumNode::default()).await?;
handle.node_exit_future.await
}) {
eprintln!("Error: {err:?}");
std::process::exit(1);
}
}

#[cfg(test)]
mod tests {
use super::*;
use clap::Parser;

/// A helper type to parse Args more easily
#[derive(Parser)]
struct CommandParser<T: Args> {
#[command(flatten)]
args: T,
}

#[test]
fn test_parse_engine_args() {
let default_args = EngineArgs::default();
let args = CommandParser::<EngineArgs>::parse_from(["reth"]).args;
assert_eq!(args, default_args);
}
}
8 changes: 7 additions & 1 deletion crates/cli/commands/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use reth_ethereum_cli::chainspec::EthereumChainSpecParser;
use reth_node_builder::{NodeBuilder, WithLaunchContext};
use reth_node_core::{
args::{
DatabaseArgs, DatadirArgs, DebugArgs, DevArgs, NetworkArgs, PayloadBuilderArgs,
DatabaseArgs, DatadirArgs, DebugArgs, DevArgs, EngineArgs, NetworkArgs, PayloadBuilderArgs,
PruningArgs, RpcServerArgs, TxPoolArgs,
},
node_config::NodeConfig,
Expand Down Expand Up @@ -107,6 +107,10 @@ pub struct NodeCommand<
#[command(flatten)]
pub pruning: PruningArgs,

/// Engine cli arguments
#[command(flatten, next_help_heading = "Engine")]
pub engine: EngineArgs,

/// Additional cli arguments
#[command(flatten, next_help_heading = "Extension")]
pub ext: Ext,
Expand Down Expand Up @@ -160,6 +164,7 @@ impl<
dev,
pruning,
ext,
engine,
} = self;

// set up node config
Expand All @@ -177,6 +182,7 @@ impl<
db,
dev,
pruning,
engine,
};

let data_dir = node_config.datadir();
Expand Down
8 changes: 7 additions & 1 deletion crates/node/builder/src/builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use reth_db_api::{
database::Database,
database_metrics::{DatabaseMetadata, DatabaseMetrics},
};
use reth_engine_tree::tree::TreeConfig;
use reth_exex::ExExContext;
use reth_network::{
transactions::TransactionsManagerConfig, NetworkBuilder, NetworkConfig, NetworkConfigBuilder,
Expand Down Expand Up @@ -563,8 +564,13 @@ where
> {
let Self { builder, task_executor } = self;

let engine_tree_config = TreeConfig::default()
.with_persistence_threshold(builder.config.engine.persistence_threshold)
.with_memory_block_buffer_target(builder.config.engine.memory_block_buffer_target)
.with_state_root_task(builder.config.engine.state_root_task_enabled);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably make this a function of builder or config?

Copy link
Member

@gakonst gakonst Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, specifically it'd be nice if in the Optimism config we could instantiate TreeConfig from RollupArgs somehow, so that we don't need this: https://github.com/paradigmxyz/reth/blob/main/crates/optimism/bin/src/main.rs#L26-L41

Ideally this https://github.com/paradigmxyz/reth/blob/main/crates/optimism/bin/src/main.rs#L26C12-L39 becomes builder.node(OpNode::new(rollup_args))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for review


let launcher =
EngineNodeLauncher::new(task_executor, builder.config.datadir(), Default::default());
EngineNodeLauncher::new(task_executor, builder.config.datadir(), engine_tree_config);
builder.launch_with(launcher).await
}
}
Expand Down
52 changes: 52 additions & 0 deletions crates/node/core/src/args/engine.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
//! clap [Args](clap::Args) for engine purposes

use clap::Args;

use crate::node_config::{DEFAULT_MEMORY_BLOCK_BUFFER_TARGET, DEFAULT_PERSISTENCE_THRESHOLD};

/// Parameters for configuring the engine
#[derive(Debug, Clone, Args, PartialEq, Eq)]
#[command(next_help_heading = "Engine")]
pub struct EngineArgs {
/// Configure persistence threshold for engine experimental.
#[arg(long = "engine.persistence-threshold", conflicts_with = "legacy", default_value_t = DEFAULT_PERSISTENCE_THRESHOLD)]
pub persistence_threshold: u64,

/// Configure the target number of blocks to keep in memory.
#[arg(long = "engine.memory-block-buffer-target", conflicts_with = "legacy", default_value_t = DEFAULT_MEMORY_BLOCK_BUFFER_TARGET)]
pub memory_block_buffer_target: u64,

/// Enable state root task
#[arg(long = "engine.state-root-task", conflicts_with = "legacy")]
pub state_root_task_enabled: bool,
}

impl Default for EngineArgs {
fn default() -> Self {
Self {
persistence_threshold: DEFAULT_PERSISTENCE_THRESHOLD,
memory_block_buffer_target: DEFAULT_MEMORY_BLOCK_BUFFER_TARGET,
state_root_task_enabled: false,
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use clap::Parser;

/// A helper type to parse Args more easily
#[derive(Parser)]
struct CommandParser<T: Args> {
#[command(flatten)]
args: T,
}

#[test]
fn test_parse_engine_args() {
let default_args = EngineArgs::default();
let args = CommandParser::<EngineArgs>::parse_from(["reth"]).args;
assert_eq!(args, default_args);
}
}
4 changes: 4 additions & 0 deletions crates/node/core/src/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,5 +56,9 @@ pub use datadir_args::DatadirArgs;
mod benchmark_args;
pub use benchmark_args::BenchmarkArgs;

/// EngineArgs for configuring the engine
mod engine;
pub use engine::EngineArgs;

mod error;
pub mod types;
14 changes: 13 additions & 1 deletion crates/node/core/src/node_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use crate::{
args::{
DatabaseArgs, DatadirArgs, DebugArgs, DevArgs, NetworkArgs, PayloadBuilderArgs,
DatabaseArgs, DatadirArgs, DebugArgs, DevArgs, EngineArgs, NetworkArgs, PayloadBuilderArgs,
PruningArgs, RpcServerArgs, TxPoolArgs,
},
dirs::{ChainPath, DataDirPath},
Expand Down Expand Up @@ -31,6 +31,12 @@ use std::{
};
use tracing::*;

/// Triggers persistence when the number of canonical blocks in memory exceeds this threshold.
pub const DEFAULT_PERSISTENCE_THRESHOLD: u64 = 2;

/// How close to the canonical head we persist blocks.
pub const DEFAULT_MEMORY_BLOCK_BUFFER_TARGET: u64 = 2;

/// This includes all necessary configuration to launch the node.
/// The individual configuration options can be overwritten before launching the node.
///
Expand Down Expand Up @@ -133,6 +139,9 @@ pub struct NodeConfig<ChainSpec> {

/// All pruning related arguments
pub pruning: PruningArgs,

/// All engine related arguments
pub engine: EngineArgs,
}

impl NodeConfig<ChainSpec> {
Expand Down Expand Up @@ -161,6 +170,7 @@ impl<ChainSpec> NodeConfig<ChainSpec> {
dev: DevArgs::default(),
pruning: PruningArgs::default(),
datadir: DatadirArgs::default(),
engine: EngineArgs::default(),
}
}

Expand Down Expand Up @@ -449,6 +459,7 @@ impl<ChainSpec> NodeConfig<ChainSpec> {
db: self.db,
dev: self.dev,
pruning: self.pruning,
engine: self.engine,
}
}
}
Expand All @@ -475,6 +486,7 @@ impl<ChainSpec> Clone for NodeConfig<ChainSpec> {
dev: self.dev,
pruning: self.pruning.clone(),
datadir: self.datadir.clone(),
engine: self.engine.clone(),
}
}
}
Loading