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

@@ -144,7 +144,6 @@ pub struct Config {
     pub listen_port: u16,
     pub allow_origin: Option<String>,
     pub tls_config: Option<TlsConfig>,
-    pub allow_sync_stalled: bool,
     pub spec_fork_name: Option<ForkName>,
     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<T: BeaconChainTypes>(
     shutdown: impl Future<Output = ()> + Send + Sync + 'static,
 ) -> Result<HttpServer, Error> {
     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<T: BeaconChainTypes>(
                         | SyncState::SyncTransition
                         | SyncState::BackFillSyncing { .. } => Ok(()),
                         SyncState::Synced => Ok(()),
-                        SyncState::Stalled if allow_sync_stalled => Ok(()),
+                        SyncState::Stalled => Ok(()),
-                        SyncState::Stalled => Err(warp_utils::reject::not_synced(
-                            "sync is stalled".to_string(),
-                        )),
                     }
                 },
             );

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 \
+                .help("This flag is deprecated and has no effect.")
-                       not be necessary to set this flag.")
                 .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 \
+                .help("This flag is deprecated and has no effect.")
-                    stalled. This is useful for very small testnets. TESTING ONLY. DO NOT USE ON \
+                .hidden(true)
-                    MAINNET.")
         )
         .arg(
             Arg::with_name("http-sse-capacity-multiplier")

beacon_node/src/config.rs

@@ -128,8 +128,12 @@ pub fn get_config<E: EthSpec>(
             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")? {
+        if cli_args.is_present("http-spec-fork") {
-            client_config.http_api.spec_fork_name = Some(fork_name);
+            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<E: EthSpec>(
         }
 
         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 =

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 <PORT>
             Set the listen TCP port for the RESTful HTTP API server.
 
-        --http-spec-fork <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 <N>
             Multiplier to apply to the length of HTTP server-sent-event (SSE) channels. Increasing this value can
             prevent messages from being dropped.

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()
+        .run_with_zero_port();
-        .with_config(|config| assert_eq!(config.http_api.allow_sync_stalled, true));
 }
+
 #[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()
+        .run_with_zero_port();
-        .with_config(|config| assert_eq!(config.http_api.spec_fork_name, Some(ForkName::Altair)));
 }
 
 // Tests for Metrics flags.

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

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.