From 6bac5ce12b94c397ee0a7d5423a0eeab814c3682 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 12 Apr 2024 07:21:00 +0300 Subject: [PATCH] Deprecate `http-spec-fork` and `http-allow-sync-stalled` (#5500) * deprecate flags * fmt * remove backslash * remove hidden flags from the book * Merge branch 'unstable' of https://github.com/sigp/lighthouse into deprecate-http-spec-fork-and-http-allow-sync-stalled * add warn, re-add tests * Apply suggestions from code review * Merge remote-tracking branch 'origin/unstable' into deprecate-http-spec-fork-and-http-allow-sync-stalled * Fix imports --- beacon_node/http_api/src/lib.rs | 8 +------- beacon_node/src/cli.rs | 9 ++++----- beacon_node/src/config.rs | 14 +++++++++++--- book/src/help_bn.md | 6 ------ lighthouse/tests/beacon_node.rs | 10 +++++----- scripts/local_testnet/beacon_node.sh | 1 - testing/simulator/src/sync_sim.rs | 2 -- 7 files changed, 21 insertions(+), 29 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 9e6022dc954..cc117c3fb92 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -144,7 +144,6 @@ pub struct Config { pub listen_port: u16, pub allow_origin: Option, pub tls_config: Option, - pub allow_sync_stalled: bool, pub spec_fork_name: Option, pub data_dir: PathBuf, pub sse_capacity_multiplier: usize, @@ -162,7 +161,6 @@ impl Default for Config { listen_port: 5052, allow_origin: None, tls_config: None, - allow_sync_stalled: false, spec_fork_name: None, data_dir: PathBuf::from(DEFAULT_ROOT_DIR), sse_capacity_multiplier: 1, @@ -321,7 +319,6 @@ pub fn serve( shutdown: impl Future + Send + Sync + 'static, ) -> Result { let config = ctx.config.clone(); - let allow_sync_stalled = config.allow_sync_stalled; let log = ctx.log.clone(); // Configure CORS. @@ -482,10 +479,7 @@ pub fn serve( | SyncState::SyncTransition | SyncState::BackFillSyncing { .. } => Ok(()), SyncState::Synced => Ok(()), - SyncState::Stalled if allow_sync_stalled => Ok(()), - SyncState::Stalled => Err(warp_utils::reject::not_synced( - "sync is stalled".to_string(), - )), + SyncState::Stalled => Ok(()), } }, ); diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index d46eb0f403e..818cdbd460f 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -392,9 +392,9 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .long("http-spec-fork") .requires("enable_http") .value_name("FORK") - .help("Serve the spec for a specific hard fork on /eth/v1/config/spec. It should \ - not be necessary to set this flag.") + .help("This flag is deprecated and has no effect.") .takes_value(true) + .hidden(true) ) .arg( Arg::with_name("http-enable-tls") @@ -425,9 +425,8 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { Arg::with_name("http-allow-sync-stalled") .long("http-allow-sync-stalled") .requires("enable_http") - .help("Forces the HTTP to indicate that the node is synced when sync is actually \ - stalled. This is useful for very small testnets. TESTING ONLY. DO NOT USE ON \ - MAINNET.") + .help("This flag is deprecated and has no effect.") + .hidden(true) ) .arg( Arg::with_name("http-sse-capacity-multiplier") diff --git a/beacon_node/src/config.rs b/beacon_node/src/config.rs index 9b0032e3068..5a27b148c99 100644 --- a/beacon_node/src/config.rs +++ b/beacon_node/src/config.rs @@ -128,8 +128,12 @@ pub fn get_config( client_config.http_api.allow_origin = Some(allow_origin.to_string()); } - if let Some(fork_name) = clap_utils::parse_optional(cli_args, "http-spec-fork")? { - client_config.http_api.spec_fork_name = Some(fork_name); + if cli_args.is_present("http-spec-fork") { + warn!( + log, + "Ignoring --http-spec-fork"; + "info" => "this flag is deprecated and will be removed" + ); } if cli_args.is_present("http-enable-tls") { @@ -148,7 +152,11 @@ pub fn get_config( } if cli_args.is_present("http-allow-sync-stalled") { - client_config.http_api.allow_sync_stalled = true; + warn!( + log, + "Ignoring --http-allow-sync-stalled"; + "info" => "this flag is deprecated and will be removed" + ); } client_config.http_api.sse_capacity_multiplier = diff --git a/book/src/help_bn.md b/book/src/help_bn.md index 996642f0fc7..e437925a0e8 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -70,9 +70,6 @@ FLAGS: enables --http and --validator-monitor-auto and enables SSE logging. -h, --help Prints help information --http Enable the RESTful HTTP API server. Disabled by default. - --http-allow-sync-stalled Forces the HTTP to indicate that the node is synced when sync is actually - stalled. This is useful for very small testnets. TESTING ONLY. DO NOT USE - ON MAINNET. --http-enable-tls Serves the RESTful HTTP API server over TLS. This feature is currently experimental. --import-all-attestations Import and aggregate all attestations, regardless of validator @@ -280,9 +277,6 @@ OPTIONS: --http-port Set the listen TCP port for the RESTful HTTP API server. - --http-spec-fork - Serve the spec for a specific hard fork on /eth/v1/config/spec. It should not be necessary to set this flag. - --http-sse-capacity-multiplier Multiplier to apply to the length of HTTP server-sent-event (SSE) channels. Increasing this value can prevent messages from being dropped. diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 68d8e46eb02..4a50126945b 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -19,7 +19,7 @@ use std::string::ToString; use std::time::Duration; use tempfile::TempDir; use types::non_zero_usize::new_non_zero_usize; -use types::{Address, Checkpoint, Epoch, ExecutionBlockHash, ForkName, Hash256, MainnetEthSpec}; +use types::{Address, Checkpoint, Epoch, ExecutionBlockHash, 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/"; @@ -1583,14 +1583,15 @@ fn http_allow_origin_all_flag() { .run_with_zero_port() .with_config(|config| assert_eq!(config.http_api.allow_origin, Some("*".to_string()))); } + #[test] fn http_allow_sync_stalled_flag() { CommandLineTest::new() .flag("http", None) .flag("http-allow-sync-stalled", None) - .run_with_zero_port() - .with_config(|config| assert_eq!(config.http_api.allow_sync_stalled, true)); + .run_with_zero_port(); } + #[test] fn http_enable_beacon_processor() { CommandLineTest::new() @@ -1642,8 +1643,7 @@ fn http_spec_fork_override() { CommandLineTest::new() .flag("http", None) .flag("http-spec-fork", Some("altair")) - .run_with_zero_port() - .with_config(|config| assert_eq!(config.http_api.spec_fork_name, Some(ForkName::Altair))); + .run_with_zero_port(); } // Tests for Metrics flags. diff --git a/scripts/local_testnet/beacon_node.sh b/scripts/local_testnet/beacon_node.sh index 2660dfa3c00..940fe2b8581 100755 --- a/scripts/local_testnet/beacon_node.sh +++ b/scripts/local_testnet/beacon_node.sh @@ -67,5 +67,4 @@ exec $lighthouse_binary \ --target-peers $((BN_COUNT - 1)) \ --execution-endpoint $execution_endpoint \ --execution-jwt $execution_jwt \ - --http-allow-sync-stalled \ $BN_ARGS diff --git a/testing/simulator/src/sync_sim.rs b/testing/simulator/src/sync_sim.rs index 78f7e1ee9fb..ba4ea4530af 100644 --- a/testing/simulator/src/sync_sim.rs +++ b/testing/simulator/src/sync_sim.rs @@ -94,8 +94,6 @@ fn syncing_sim( beacon_config.dummy_eth1_backend = true; beacon_config.sync_eth1_chain = true; - beacon_config.http_api.allow_sync_stalled = true; - beacon_config.network.enr_address = (Some(Ipv4Addr::LOCALHOST), None); // Generate the directories and keystores required for the validator clients.