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.

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

make execution-endpoint required #5165

Merged
merged 12 commits into from
Nov 1, 2024
Merged

make execution-endpoint required #5165

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3dedab2
make `execution-endpoint` mandatory
zhiqiangxu Feb 1, 2024
baf755b
use parse_required instead
zhiqiangxu Feb 1, 2024
486686d
make test pass
zhiqiangxu Feb 22, 2024
d8dd82f
Merge branch 'unstable' into make_ee_required
zhiqiangxu Jun 28, 2024
bc30999
fix test
zhiqiangxu Jun 28, 2024
bf8be11
Merge branch 'unstable' into make_ee_required
dapplion Oct 2, 2024
93bc19b
Fix cli help text
macladson Oct 17, 2024
d745d09
Fix tests
macladson Oct 25, 2024
dd37102
Merge branch 'unstable' into make_ee_required
macladson Oct 25, 2024
dae79a5
Add comment
macladson Oct 27, 2024
1d7694f
Clarification
michaelsproul Nov 1, 2024
9f869a4
Merge remote-tracking branch 'origin/unstable' into make_ee_required
michaelsproul Nov 1, 2024
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
1 change: 1 addition & 0 deletions beacon_node/src/cli.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
39 changes: 26 additions & 13 deletions lighthouse/tests/beacon_node.rs
View file
Open in desktop
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:8545/";
michaelsproul marked this conversation as resolved.
Show resolved Hide resolved
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,17 @@ 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 }
}

fn new_with_no_execution_endpoint() -> CommandLineTest {
macladson marked this conversation as resolved.
Show resolved Hide resolved
let base_cmd = base_cmd();
CommandLineTest { cmd: base_cmd }
}
Expand Down Expand Up @@ -104,7 +117,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 +266,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 +472,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 +493,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 +514,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 +541,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 +563,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 +604,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 +726,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 +2443,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));
Copy link
Member

Choose a reason for hiding this comment

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

Note that sync_eth1_chain will now effectively default to true since it is set to true when --execution_endpoint is provided (which will now be all the time).

This may very well deprecate this config value entirely, although it can still technically be set back to false using the --disable-deposit-contract-sync flag.

Copy link
Member

Choose a reason for hiding this comment

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

I think this means we can hide and deprecate the --eth1 flag.

}

#[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 +2462,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
Loading