Skip to content

Commit

Permalink
make execution-endpoint required (sigp#5165)
Browse files Browse the repository at this point in the history 
* make `execution-endpoint` mandatory

* use parse_required instead

* make test pass

* Merge branch 'unstable' into make_ee_required

* fix test

* Merge branch 'unstable' into make_ee_required

* Fix cli help text

* Fix tests

* Merge branch 'unstable' into make_ee_required

* Add comment

* Clarification

* Merge remote-tracking branch 'origin/unstable' into make_ee_required
  • Loading branch information
@zhiqiangxu
zhiqiangxu authored Nov 1, 2024
1 parent 1126058 commit 16693b0
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 98 deletions.
1 change: 1 addition & 0 deletions beacon_node/src/cli.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,7 @@ pub fn cli_app() -> Command {
.help("Server endpoint for an execution layer JWT-authenticated HTTP \
JSON-RPC connection. Uses the same endpoint to populate the \
deposit cache.")
.required(true)
.action(ArgAction::Set)
.display_order(0)
)
Expand Down
170 changes: 86 additions & 84 deletions beacon_node/src/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,92 +284,94 @@ pub fn get_config<E: EthSpec>(
client_config.eth1.cache_follow_distance = Some(follow_distance);
}

if let Some(endpoints) = cli_args.get_one::<String>("execution-endpoint") {
let mut el_config = execution_layer::Config::default();

// Always follow the deposit contract when there is an execution endpoint.
//
// This is wasteful for non-staking nodes as they have no need to process deposit contract
// logs and build an "eth1" cache. The alternative is to explicitly require the `--eth1` or
// `--staking` flags, however that poses a risk to stakers since they cannot produce blocks
// without "eth1".
//
// The waste for non-staking nodes is relatively small so we err on the side of safety for
// stakers. The merge is already complicated enough.
client_config.sync_eth1_chain = true;

// Parse a single execution endpoint, logging warnings if multiple endpoints are supplied.
let execution_endpoint =
parse_only_one_value(endpoints, SensitiveUrl::parse, "--execution-endpoint", log)?;

// JWTs are required if `--execution-endpoint` is supplied. They can be either passed via
// file_path or directly as string.

let secret_file: PathBuf;
// Parse a single JWT secret from a given file_path, logging warnings if multiple are supplied.
if let Some(secret_files) = cli_args.get_one::<String>("execution-jwt") {
secret_file =
parse_only_one_value(secret_files, PathBuf::from_str, "--execution-jwt", log)?;

// Check if the JWT secret key is passed directly via cli flag and persist it to the default
// file location.
} else if let Some(jwt_secret_key) = cli_args.get_one::<String>("execution-jwt-secret-key")
{
use std::fs::File;
use std::io::Write;
secret_file = client_config.data_dir().join(DEFAULT_JWT_FILE);
let mut jwt_secret_key_file = File::create(secret_file.clone())
.map_err(|e| format!("Error while creating jwt_secret_key file: {:?}", e))?;
jwt_secret_key_file
.write_all(jwt_secret_key.as_bytes())
.map_err(|e| {
format!(
"Error occurred while writing to jwt_secret_key file: {:?}",
e
)
})?;
} else {
return Err("Error! Please set either --execution-jwt file_path or --execution-jwt-secret-key directly via cli when using --execution-endpoint".to_string());
}

// Parse and set the payload builder, if any.
if let Some(endpoint) = cli_args.get_one::<String>("builder") {
let payload_builder =
parse_only_one_value(endpoint, SensitiveUrl::parse, "--builder", log)?;
el_config.builder_url = Some(payload_builder);

el_config.builder_user_agent =
clap_utils::parse_optional(cli_args, "builder-user-agent")?;

el_config.builder_header_timeout =
clap_utils::parse_optional(cli_args, "builder-header-timeout")?
.map(Duration::from_millis);
}
// `--execution-endpoint` is required now.
let endpoints: String = clap_utils::parse_required(cli_args, "execution-endpoint")?;
let mut el_config = execution_layer::Config::default();

// Set config values from parse values.
el_config.secret_file = Some(secret_file.clone());
el_config.execution_endpoint = Some(execution_endpoint.clone());
el_config.suggested_fee_recipient =
clap_utils::parse_optional(cli_args, "suggested-fee-recipient")?;
el_config.jwt_id = clap_utils::parse_optional(cli_args, "execution-jwt-id")?;
el_config.jwt_version = clap_utils::parse_optional(cli_args, "execution-jwt-version")?;
el_config
.default_datadir
.clone_from(client_config.data_dir());
let execution_timeout_multiplier =
clap_utils::parse_required(cli_args, "execution-timeout-multiplier")?;
el_config.execution_timeout_multiplier = Some(execution_timeout_multiplier);

client_config.eth1.endpoint = Eth1Endpoint::Auth {
endpoint: execution_endpoint,
jwt_path: secret_file,
jwt_id: el_config.jwt_id.clone(),
jwt_version: el_config.jwt_version.clone(),
};
// Always follow the deposit contract when there is an execution endpoint.
//
// This is wasteful for non-staking nodes as they have no need to process deposit contract
// logs and build an "eth1" cache. The alternative is to explicitly require the `--eth1` or
// `--staking` flags, however that poses a risk to stakers since they cannot produce blocks
// without "eth1".
//
// The waste for non-staking nodes is relatively small so we err on the side of safety for
// stakers. The merge is already complicated enough.
client_config.sync_eth1_chain = true;

// Parse a single execution endpoint, logging warnings if multiple endpoints are supplied.
let execution_endpoint = parse_only_one_value(
endpoints.as_str(),
SensitiveUrl::parse,
"--execution-endpoint",
log,
)?;

// JWTs are required if `--execution-endpoint` is supplied. They can be either passed via
// file_path or directly as string.

let secret_file: PathBuf;
// Parse a single JWT secret from a given file_path, logging warnings if multiple are supplied.
if let Some(secret_files) = cli_args.get_one::<String>("execution-jwt") {
secret_file =
parse_only_one_value(secret_files, PathBuf::from_str, "--execution-jwt", log)?;

// Check if the JWT secret key is passed directly via cli flag and persist it to the default
// file location.
} else if let Some(jwt_secret_key) = cli_args.get_one::<String>("execution-jwt-secret-key") {
use std::fs::File;
use std::io::Write;
secret_file = client_config.data_dir().join(DEFAULT_JWT_FILE);
let mut jwt_secret_key_file = File::create(secret_file.clone())
.map_err(|e| format!("Error while creating jwt_secret_key file: {:?}", e))?;
jwt_secret_key_file
.write_all(jwt_secret_key.as_bytes())
.map_err(|e| {
format!(
"Error occurred while writing to jwt_secret_key file: {:?}",
e
)
})?;
} else {
return Err("Error! Please set either --execution-jwt file_path or --execution-jwt-secret-key directly via cli when using --execution-endpoint".to_string());
}

// Parse and set the payload builder, if any.
if let Some(endpoint) = cli_args.get_one::<String>("builder") {
let payload_builder =
parse_only_one_value(endpoint, SensitiveUrl::parse, "--builder", log)?;
el_config.builder_url = Some(payload_builder);

el_config.builder_user_agent = clap_utils::parse_optional(cli_args, "builder-user-agent")?;

el_config.builder_header_timeout =
clap_utils::parse_optional(cli_args, "builder-header-timeout")?
.map(Duration::from_millis);
}

// Set config values from parse values.
el_config.secret_file = Some(secret_file.clone());
el_config.execution_endpoint = Some(execution_endpoint.clone());
el_config.suggested_fee_recipient =
clap_utils::parse_optional(cli_args, "suggested-fee-recipient")?;
el_config.jwt_id = clap_utils::parse_optional(cli_args, "execution-jwt-id")?;
el_config.jwt_version = clap_utils::parse_optional(cli_args, "execution-jwt-version")?;
el_config
.default_datadir
.clone_from(client_config.data_dir());
let execution_timeout_multiplier =
clap_utils::parse_required(cli_args, "execution-timeout-multiplier")?;
el_config.execution_timeout_multiplier = Some(execution_timeout_multiplier);

client_config.eth1.endpoint = Eth1Endpoint::Auth {
endpoint: execution_endpoint,
jwt_path: secret_file,
jwt_id: el_config.jwt_id.clone(),
jwt_version: el_config.jwt_version.clone(),
};

// Store the EL config in the client config.
client_config.execution_layer = Some(el_config);
}
// Store the EL config in the client config.
client_config.execution_layer = Some(el_config);

// 4844 params
if let Some(trusted_setup) = context
Expand Down
2 changes: 1 addition & 1 deletion book/src/help_bn.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ The primary component which connects to the Ethereum 2.0 P2P network and
downloads, verifies and stores blocks. Provides a HTTP API for querying the
beacon chain and publishing messages to the network.
Usage: lighthouse beacon_node [OPTIONS]
Usage: lighthouse beacon_node [OPTIONS] --execution-endpoint <EXECUTION-ENDPOINT>
Options:
--auto-compact-db <auto-compact-db>
Expand Down
40 changes: 27 additions & 13 deletions lighthouse/tests/beacon_node.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ use types::non_zero_usize::new_non_zero_usize;
use types::{Address, Checkpoint, Epoch, Hash256, MainnetEthSpec};
use unused_port::{unused_tcp4_port, unused_tcp6_port, unused_udp4_port, unused_udp6_port};

const DEFAULT_ETH1_ENDPOINT: &str = "http://localhost:8545/";
const DEFAULT_EXECUTION_ENDPOINT: &str = "http://localhost:8551/";
const DEFAULT_EXECUTION_JWT_SECRET_KEY: &str =
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";

// These dummy ports should ONLY be used for `enr-xxx-port` flags that do not bind.
const DUMMY_ENR_TCP_PORT: u16 = 7777;
Expand Down Expand Up @@ -52,6 +54,18 @@ struct CommandLineTest {
}
impl CommandLineTest {
fn new() -> CommandLineTest {
let mut base_cmd = base_cmd();

base_cmd
.arg("--execution-endpoint")
.arg(DEFAULT_EXECUTION_ENDPOINT)
.arg("--execution-jwt-secret-key")
.arg(DEFAULT_EXECUTION_JWT_SECRET_KEY);
CommandLineTest { cmd: base_cmd }
}

// Required for testing different JWT authentication methods.
fn new_with_no_execution_endpoint() -> CommandLineTest {
let base_cmd = base_cmd();
CommandLineTest { cmd: base_cmd }
}
Expand Down Expand Up @@ -104,7 +118,7 @@ fn staking_flag() {
assert!(config.sync_eth1_chain);
assert_eq!(
config.eth1.endpoint.get_endpoint().to_string(),
DEFAULT_ETH1_ENDPOINT
DEFAULT_EXECUTION_ENDPOINT
);
});
}
Expand Down Expand Up @@ -253,7 +267,7 @@ fn always_prepare_payload_default() {
#[test]
fn always_prepare_payload_override() {
let dir = TempDir::new().expect("Unable to create temporary directory");
CommandLineTest::new()
CommandLineTest::new_with_no_execution_endpoint()
.flag("always-prepare-payload", None)
.flag(
"suggested-fee-recipient",
Expand Down Expand Up @@ -459,7 +473,7 @@ fn run_bellatrix_execution_endpoints_flag_test(flag: &str) {

// this is way better but intersperse is still a nightly feature :/
// let endpoint_arg: String = urls.into_iter().intersperse(",").collect();
CommandLineTest::new()
CommandLineTest::new_with_no_execution_endpoint()
.flag(flag, Some(&endpoint_arg))
.flag("execution-jwt", Some(&jwts_arg))
.run_with_zero_port()
Expand All @@ -480,7 +494,7 @@ fn run_bellatrix_execution_endpoints_flag_test(flag: &str) {
#[test]
fn run_execution_jwt_secret_key_is_persisted() {
let jwt_secret_key = "0x3cbc11b0d8fa16f3344eacfd6ff6430b9d30734450e8adcf5400f88d327dcb33";
CommandLineTest::new()
CommandLineTest::new_with_no_execution_endpoint()
.flag("execution-endpoint", Some("http://localhost:8551/"))
.flag("execution-jwt-secret-key", Some(jwt_secret_key))
.run_with_zero_port()
Expand All @@ -501,7 +515,7 @@ fn run_execution_jwt_secret_key_is_persisted() {
#[test]
fn execution_timeout_multiplier_flag() {
let dir = TempDir::new().expect("Unable to create temporary directory");
CommandLineTest::new()
CommandLineTest::new_with_no_execution_endpoint()
.flag("execution-endpoint", Some("http://meow.cats"))
.flag(
"execution-jwt",
Expand All @@ -528,7 +542,7 @@ fn bellatrix_jwt_secrets_flag() {
let mut file = File::create(dir.path().join("jwtsecrets")).expect("Unable to create file");
file.write_all(b"0x3cbc11b0d8fa16f3344eacfd6ff6430b9d30734450e8adcf5400f88d327dcb33")
.expect("Unable to write to file");
CommandLineTest::new()
CommandLineTest::new_with_no_execution_endpoint()
.flag("execution-endpoints", Some("http://localhost:8551/"))
.flag(
"jwt-secrets",
Expand All @@ -550,7 +564,7 @@ fn bellatrix_jwt_secrets_flag() {
#[test]
fn bellatrix_fee_recipient_flag() {
let dir = TempDir::new().expect("Unable to create temporary directory");
CommandLineTest::new()
CommandLineTest::new_with_no_execution_endpoint()
.flag("execution-endpoint", Some("http://meow.cats"))
.flag(
"execution-jwt",
Expand Down Expand Up @@ -591,7 +605,7 @@ fn run_payload_builder_flag_test_with_config<F: Fn(&Config)>(
f: F,
) {
let dir = TempDir::new().expect("Unable to create temporary directory");
let mut test = CommandLineTest::new();
let mut test = CommandLineTest::new_with_no_execution_endpoint();
test.flag("execution-endpoint", Some("http://meow.cats"))
.flag(
"execution-jwt",
Expand Down Expand Up @@ -713,7 +727,7 @@ fn run_jwt_optional_flags_test(jwt_flag: &str, jwt_id_flag: &str, jwt_version_fl
let jwt_file = "jwt-file";
let id = "bn-1";
let version = "Lighthouse-v2.1.3";
CommandLineTest::new()
CommandLineTest::new_with_no_execution_endpoint()
.flag("execution-endpoint", Some(execution_endpoint))
.flag(jwt_flag, dir.path().join(jwt_file).as_os_str().to_str())
.flag(jwt_id_flag, Some(id))
Expand Down Expand Up @@ -2430,13 +2444,13 @@ fn logfile_format_flag() {
fn sync_eth1_chain_default() {
CommandLineTest::new()
.run_with_zero_port()
.with_config(|config| assert_eq!(config.sync_eth1_chain, false));
.with_config(|config| assert_eq!(config.sync_eth1_chain, true));
}

#[test]
fn sync_eth1_chain_execution_endpoints_flag() {
let dir = TempDir::new().expect("Unable to create temporary directory");
CommandLineTest::new()
CommandLineTest::new_with_no_execution_endpoint()
.flag("execution-endpoints", Some("http://localhost:8551/"))
.flag(
"execution-jwt",
Expand All @@ -2449,7 +2463,7 @@ fn sync_eth1_chain_execution_endpoints_flag() {
#[test]
fn sync_eth1_chain_disable_deposit_contract_sync_flag() {
let dir = TempDir::new().expect("Unable to create temporary directory");
CommandLineTest::new()
CommandLineTest::new_with_no_execution_endpoint()
.flag("disable-deposit-contract-sync", None)
.flag("execution-endpoints", Some("http://localhost:8551/"))
.flag(
Expand Down

0 comments on commit 16693b0

Please sign in to comment.