Combine sync distance tolerance flags into one

lighthouse/tests/validator_client.rs

@@ -1,4 +1,4 @@
-use validator_client::{ApiTopic, Config};
+use validator_client::{ApiTopic, BeaconNodeSyncDistanceTiers, Config};
 
 use crate::exec::CommandLineTestExec;
 use bls::{Keypair, PublicKeyBytes};
@@ -10,7 +10,7 @@ use std::process::Command;
 use std::str::FromStr;
 use std::string::ToString;
 use tempfile::TempDir;
-use types::Address;
+use types::{Address, Slot};
 
 /// Returns the `lighthouse validator_client` command.
 fn base_cmd() -> Command {
@@ -576,37 +576,29 @@ fn broadcast_flag() {
         });
 }
 
-/// Tests for validator fallback parameter flags.
+/// Tests for validator fallback flags.
 #[test]
-fn beacon_node_sync_tolerance_flag() {
+fn beacon_nodes_sync_tolerances_flag_default() {
-    CommandLineTest::new()
+    CommandLineTest::new().run().with_config(|config| {
-        .flag("beacon-node-sync-tolerance", Some("4"))
+        assert_eq!(
-        .run()
+            config.beacon_node_fallback.sync_tolerances,
-        .with_config(|config| {
+            BeaconNodeSyncDistanceTiers::default()
-            assert_eq!(config.beacon_node_fallback.sync_tolerance, Some(4));
+        )
     });
 }
 #[test]
-fn beacon_node_small_sync_distance_modifier_flag() {
+fn beacon_nodes_sync_tolerances_flag() {
     CommandLineTest::new()
-        .flag("beacon-node-small-sync-distance-modifier", Some("16"))
+        .flag("beacon-nodes-sync-tolerances", Some("4,4,4"))
         .run()
         .with_config(|config| {
             assert_eq!(
-                config.beacon_node_fallback.small_sync_distance_modifier,
+                config.beacon_node_fallback.sync_tolerances,
-                Some(16)
+                BeaconNodeSyncDistanceTiers {
-            );
+                    synced: Slot::new(4),
-        });
+                    small: Slot::new(8),
-}
+                    medium: Slot::new(12),
-#[test]
+                }
-fn beacon_node_medium_sync_distance_modifier_flag() {
-    CommandLineTest::new()
-        .flag("beacon-node-medium-sync-distance-modifier", Some("32"))
-        .run()
-        .with_config(|config| {
-            assert_eq!(
-                config.beacon_node_fallback.medium_sync_distance_modifier,
-                Some(32)
             );
         });
 }

validator_client/src/beacon_node_fallback.rs

@@ -47,15 +47,16 @@ const FUTURE_SLOT_TOLERANCE: Slot = Slot::new(1);
 pub struct Config {
     /// Disables publishing http api requests to all beacon nodes for select api calls.
     pub disable_run_on_all: bool,
-    /// Sets the number of slots behind the head a beacon node is allowed to be to still be
+    pub sync_tolerances: BeaconNodeSyncDistanceTiers,
-    /// considered `synced`.
+    // Sets the number of slots behind the head a beacon node is allowed to be to still be
-    pub sync_tolerance: Option<u64>,
+    // considered `synced`.
-    /// Sets the size of the range of the `small` sync distance tier. This range starts immediately
+    //pub sync_tolerance: Option<u64>,
-    /// after `sync_tolerance`.
+    // Sets the size of the range of the `small` sync distance tier. This range starts immediately
-    pub small_sync_distance_modifier: Option<u64>,
+    // after `sync_tolerance`.
-    /// Sets the size of the range of the `medium` sync distance tier. This range starts immediately
+    //pub small_sync_distance_modifier: Option<u64>,
-    /// after the `small` range.
+    // Sets the size of the range of the `medium` sync distance tier. This range starts immediately
-    pub medium_sync_distance_modifier: Option<u64>,
+    // after the `small` range.
+    //pub medium_sync_distance_modifier: Option<u64>,
 }
 
 /// Indicates a measurement of latency between the VC and a BN.
@@ -358,7 +359,7 @@ impl<T: SlotClock, E: EthSpec> BeaconNodeFallback<T, E> {
         spec: ChainSpec,
         log: Logger,
     ) -> Self {
-        let distance_tiers = BeaconNodeSyncDistanceTiers::from_config(&config);
+        let distance_tiers = config.sync_tolerances;
         Self {
             candidates: Arc::new(RwLock::new(candidates)),
             distance_tiers,

validator_client/src/beacon_node_health.rs

@@ -1,7 +1,8 @@
-use crate::beacon_node_fallback::Config;
+use itertools::Itertools;
 use serde::{Deserialize, Serialize};
 use std::cmp::Ordering;
 use std::fmt::{Debug, Display, Formatter};
+use std::str::FromStr;
 use types::Slot;
 
 /// Sync distances between 0 and DEFAULT_SYNC_TOLERANCE are considered `synced`.
@@ -30,35 +31,49 @@ pub enum SyncDistanceTier {
 
 /// Contains the different sync distance tiers which are determined at runtime by the
 /// `sync_tolerance` CLI flag and the `sync_distance_modifier` flags.
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
 pub struct BeaconNodeSyncDistanceTiers {
-    synced: SyncDistance,
+    pub synced: SyncDistance,
-    small: SyncDistance,
+    pub small: SyncDistance,
-    medium: SyncDistance,
+    pub medium: SyncDistance,
+}
+
+impl Default for BeaconNodeSyncDistanceTiers {
+    fn default() -> Self {
+        Self {
+            synced: DEFAULT_SYNC_TOLERANCE,
+            small: DEFAULT_SYNC_TOLERANCE + DEFAULT_SMALL_SYNC_DISTANCE_MODIFIER,
+            medium: DEFAULT_SYNC_TOLERANCE
+                + DEFAULT_SMALL_SYNC_DISTANCE_MODIFIER
+                + DEFAULT_MEDIUM_SYNC_DISTANCE_MODIFIER,
+        }
+    }
+}
+
+impl FromStr for BeaconNodeSyncDistanceTiers {
+    type Err = String;
+
+    fn from_str(s: &str) -> Result<Self, String> {
+        let values: (u64, u64, u64) = s
+            .split(',')
+            .map(|s| {
+                s.parse()
+                    .map_err(|e| format!("Invalid sync distance modifier: {e:?}"))
+            })
+            .collect::<Result<Vec<_>, _>>()?
+            .into_iter()
+            .collect_tuple()
+            .ok_or("Invalid number of sync distance modifiers".to_string())?;
+
+        Ok(BeaconNodeSyncDistanceTiers {
+            synced: Slot::new(values.0),
+            small: Slot::new(values.0 + values.1),
+            medium: Slot::new(values.0 + values.1 + values.2),
+        })
+    }
 }
 
 impl BeaconNodeSyncDistanceTiers {
-    pub fn from_config(config: &Config) -> Self {
-        let synced = config
-            .sync_tolerance
-            .map(Slot::new)
-            .unwrap_or(DEFAULT_SYNC_TOLERANCE);
-        let small_mod = config
-            .small_sync_distance_modifier
-            .map(Slot::new)
-            .unwrap_or(DEFAULT_SMALL_SYNC_DISTANCE_MODIFIER);
-        let medium_mod = config
-            .medium_sync_distance_modifier
-            .map(Slot::new)
-            .unwrap_or(DEFAULT_MEDIUM_SYNC_DISTANCE_MODIFIER);
-
-        Self {
-            synced,
-            small: synced + small_mod,
-            medium: synced + small_mod + medium_mod,
-        }
-    }
-
     /// Takes a given sync distance and determines its tier based on the `sync_tolerance` defined by
     /// the CLI.
     pub fn compute_distance_tier(&self, distance: SyncDistance) -> SyncDistanceTier {
@@ -74,18 +89,6 @@ impl BeaconNodeSyncDistanceTiers {
     }
 }
 
-impl Default for BeaconNodeSyncDistanceTiers {
-    fn default() -> Self {
-        Self {
-            synced: DEFAULT_SYNC_TOLERANCE,
-            small: DEFAULT_SYNC_TOLERANCE + DEFAULT_SMALL_SYNC_DISTANCE_MODIFIER,
-            medium: DEFAULT_SYNC_TOLERANCE
-                + DEFAULT_SMALL_SYNC_DISTANCE_MODIFIER
-                + DEFAULT_MEDIUM_SYNC_DISTANCE_MODIFIER,
-        }
-    }
-}
-
 /// Execution Node health metrics.
 ///
 /// Currently only considers `el_offline`.
@@ -290,12 +293,13 @@ mod tests {
         SyncDistanceTier,
     };
     use crate::beacon_node_fallback::Config;
+    use std::str::FromStr;
     use types::Slot;
 
     #[test]
     fn all_possible_health_tiers() {
         let config = Config::default();
-        let beacon_node_sync_distance_tiers = BeaconNodeSyncDistanceTiers::from_config(&config);
+        let beacon_node_sync_distance_tiers = config.sync_tolerances;
 
         let mut health_vec = vec![];
 
@@ -375,29 +379,24 @@ mod tests {
         let medium_high = new_distance_tier(64, &distance_tiers);
         let large = new_distance_tier(65, &distance_tiers);
 
-        assert!(synced_low.tier == 1);
+        assert_eq!(synced_low.tier, 1);
-        assert!(synced_high.tier == 1);
+        assert_eq!(synced_high.tier, 1);
-        assert!(small_low.tier == 2);
+        assert_eq!(small_low.tier, 2);
-        assert!(small_high.tier == 2);
+        assert_eq!(small_high.tier, 2);
-        assert!(medium_low.tier == 4);
+        assert_eq!(medium_low.tier, 4);
         assert_eq!(medium_high.tier, 4);
-        assert!(large.tier == 10);
+        assert_eq!(large.tier, 10);
     }
 
     #[test]
-    fn sync_tolerance_from_config() {
+    fn sync_tolerance_from_str() {
-        // Config should set the tiers as:
+        // String should set the tiers as:
         // synced: 0..=4
         // small: 5..=8
         // medium 9..=12
         // large: 13..
-        let config = Config {
+
-            disable_run_on_all: false,
+        let distance_tiers = BeaconNodeSyncDistanceTiers::from_str("4,4,4").unwrap();
-            sync_tolerance: Some(4),
-            small_sync_distance_modifier: Some(4),
-            medium_sync_distance_modifier: Some(4),
-        };
-        let distance_tiers = BeaconNodeSyncDistanceTiers::from_config(&config);
 
         let synced_low = new_distance_tier(0, &distance_tiers);
         let synced_high = new_distance_tier(4, &distance_tiers);
@@ -410,12 +409,12 @@ mod tests {
 
         let large = new_distance_tier(13, &distance_tiers);
 
-        assert!(synced_low.tier == 1);
+        assert_eq!(synced_low.tier, 1);
-        assert!(synced_high.tier == 1);
+        assert_eq!(synced_high.tier, 1);
-        assert!(small_low.tier == 2);
+        assert_eq!(small_low.tier, 2);
-        assert!(small_high.tier == 2);
+        assert_eq!(small_high.tier, 2);
-        assert!(medium_low.tier == 4);
+        assert_eq!(medium_low.tier, 4);
         assert_eq!(medium_high.tier, 4);
-        assert!(large.tier == 10);
+        assert_eq!(large.tier, 10);
     }
 }

validator_client/src/cli.rs

@@ -368,29 +368,28 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> {
                 .takes_value(false),
         )
         .arg(
-            Arg::with_name("beacon-node-sync-tolerance")
+            Arg::with_name("beacon-nodes-sync-tolerances")
-                .long("beacon-node-sync-tolerance")
+                .long("beacon-nodes-sync-tolerances")
-                .help("Sets the number of slots behind the head that each connected Beacon Node can be \
+                .value_name("SYNC_TOLERANCES")
-                    to still be considered synced. Effectively this gives more priority to the first \
+                .help("A comma-separated list of 3 values which sets the size of each sync distance range when \
-                    connected Beacon Node.")
+                    determining the health of each connected beacon node. \
-                .takes_value(true),
+                    The first value determines the `Synced` range. \
-        )
+                    If a connected beacon node is synced to within this number of slots it is considered 'Synced'. \
-        .arg(
+                    The second value determines the `Small` sync distance range. \
-            Arg::with_name("beacon-node-small-sync-distance-modifier")
+                    This range starts immediately after the `Synced` range. \
-                .long("beacon-node-small-sync-distance-modifier")
+                    The third value determines the `Medium` sync distance range. \
-                .help("Only use this if you know what you are doing. Incorrectly setting this value \
+                    This range starts immediately after the `Small` range. \
-                    can result in suboptimal fallback behaviour. Sets the size (in slots) of the \
+                    Any sync distance value beyond that is considered `Large`. \
-                    `small` sync distance range when calculating the health tiers of connected \
+                    For example, a value of `8,8,48` would have ranges like the following: \
-                    Beacon Nodes. The range falls immediately after the end of the `synced` range.")
+                    `Synced`: 0..=8 \
-                .takes_value(true),
+                    `Small`: 9..=16 \
-        )
+                    `Medium`: 17..=64 \
-        .arg(
+                    `Large`: 65.. \
-            Arg::with_name("beacon-node-medium-sync-distance-modifier")
+                    These values are used to determine what ordering beacon node fallbacks are used in. \
-                .long("beacon-node-medium-sync-distance-modifier")
+                    Generally, `Synced` nodes are preferred over `Small` and so on. \
-                .help("Only use this if you know what you are doing. Incorrectly setting this value \
+                    Nodes in the `Synced` range will tie-break based on their ordering in `--beacon-nodes`. \
-                    can result in suboptimal fallback behaviour. Sets the size (in slots) of the \
+                    This ensures the primary beacon node is prioritised. \
-                    `medium` sync distance range when calculating the health tiers of connected \
+                    [default: 8,8,48]")
-                    Beacon Nodes. The range falls immediately after the end of the `small` range.")
+                .takes_value(true)
-                .takes_value(true),
         )
 }

validator_client/src/config.rs

@@ -1,6 +1,8 @@
 use crate::beacon_node_fallback::ApiTopic;
 use crate::graffiti_file::GraffitiFile;
-use crate::{beacon_node_fallback, http_api, http_metrics};
+use crate::{
+    beacon_node_fallback, beacon_node_health::BeaconNodeSyncDistanceTiers, http_api, http_metrics,
+};
 use clap::ArgMatches;
 use clap_utils::{flags::DISABLE_MALLOC_TUNING_FLAG, parse_optional, parse_required};
 use directory::{
@@ -14,6 +16,7 @@ use slog::{info, warn, Logger};
 use std::fs;
 use std::net::IpAddr;
 use std::path::PathBuf;
+use std::str::FromStr;
 use types::{Address, GRAFFITI_BYTES_LEN};
 
 pub const DEFAULT_BEACON_NODE: &str = "http://localhost:5052/";
@@ -254,30 +257,11 @@ impl Config {
 
         config.beacon_node_fallback.disable_run_on_all = cli_args.is_present("disable-run-on-all");
 
-        if let Some(sync_tolerance) = cli_args.value_of("beacon-node-sync-tolerance") {
+        if let Some(sync_tolerance) = cli_args.value_of("beacon-nodes-sync-tolerances") {
-            config.beacon_node_fallback.sync_tolerance = Some(
+            config.beacon_node_fallback.sync_tolerances =
-                sync_tolerance
+                BeaconNodeSyncDistanceTiers::from_str(sync_tolerance)?;
-                    .parse::<u64>()
+        } else {
-                    .map_err(|_| "beacon-node-sync-tolerance is not a valid u64.")?,
+            config.beacon_node_fallback.sync_tolerances = BeaconNodeSyncDistanceTiers::default();
-            );
-        }
-
-        if let Some(small_modifier) = cli_args.value_of("beacon-node-small-sync-distance-modifier")
-        {
-            config.beacon_node_fallback.small_sync_distance_modifier = Some(
-                small_modifier
-                    .parse::<u64>()
-                    .map_err(|_| "beacon-node-small-sync-distance-modifier is not a valid u64.")?,
-            );
-        }
-
-        if let Some(medium_modifier) =
-            cli_args.value_of("beacon-node-medium-sync-distance-modifier")
-        {
-            config.beacon_node_fallback.medium_sync_distance_modifier =
-                Some(medium_modifier.parse::<u64>().map_err(|_| {
-                    "beacon-node-medium-sync-distance-modifier is not a valid u64."
-                })?);
         }
 
         /*

validator_client/src/lib.rs

@@ -21,6 +21,7 @@ pub mod initialized_validators;
 pub mod validator_store;
 
 pub use beacon_node_fallback::ApiTopic;
+pub use beacon_node_health::BeaconNodeSyncDistanceTiers;
 pub use cli::cli_app;
 pub use config::Config;
 use initialized_validators::InitializedValidators;