diff --git a/lighthouse/tests/validator_client.rs b/lighthouse/tests/validator_client.rs index 98c34b3a9b..5087acf450 100644 --- a/lighthouse/tests/validator_client.rs +++ b/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() { - CommandLineTest::new() - .flag("beacon-node-sync-tolerance", Some("4")) - .run() - .with_config(|config| { - assert_eq!(config.beacon_node_fallback.sync_tolerance, Some(4)); - }); +fn beacon_nodes_sync_tolerances_flag_default() { + CommandLineTest::new().run().with_config(|config| { + assert_eq!( + config.beacon_node_fallback.sync_tolerances, + BeaconNodeSyncDistanceTiers::default() + ) + }); } #[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, - Some(16) - ); - }); -} -#[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) + config.beacon_node_fallback.sync_tolerances, + BeaconNodeSyncDistanceTiers { + synced: Slot::new(4), + small: Slot::new(8), + medium: Slot::new(12), + } ); }); } diff --git a/validator_client/src/beacon_node_fallback.rs b/validator_client/src/beacon_node_fallback.rs index f6ee1681bd..8e22c80c98 100644 --- a/validator_client/src/beacon_node_fallback.rs +++ b/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 - /// considered `synced`. - pub sync_tolerance: Option, - /// Sets the size of the range of the `small` sync distance tier. This range starts immediately - /// after `sync_tolerance`. - pub small_sync_distance_modifier: Option, - /// Sets the size of the range of the `medium` sync distance tier. This range starts immediately - /// after the `small` range. - pub medium_sync_distance_modifier: Option, + pub sync_tolerances: BeaconNodeSyncDistanceTiers, + // Sets the number of slots behind the head a beacon node is allowed to be to still be + // considered `synced`. + //pub sync_tolerance: Option, + // Sets the size of the range of the `small` sync distance tier. This range starts immediately + // after `sync_tolerance`. + //pub small_sync_distance_modifier: Option, + // Sets the size of the range of the `medium` sync distance tier. This range starts immediately + // after the `small` range. + //pub medium_sync_distance_modifier: Option, } /// Indicates a measurement of latency between the VC and a BN. @@ -358,7 +359,7 @@ impl BeaconNodeFallback { 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, diff --git a/validator_client/src/beacon_node_health.rs b/validator_client/src/beacon_node_health.rs index 0c060f9e11..d2e515de16 100644 --- a/validator_client/src/beacon_node_health.rs +++ b/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, - small: SyncDistance, - medium: SyncDistance, + pub synced: SyncDistance, + pub small: 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 { + let values: (u64, u64, u64) = s + .split(',') + .map(|s| { + s.parse() + .map_err(|e| format!("Invalid sync distance modifier: {e:?}")) + }) + .collect::, _>>()? + .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!(synced_high.tier == 1); - assert!(small_low.tier == 2); - assert!(small_high.tier == 2); - assert!(medium_low.tier == 4); + assert_eq!(synced_low.tier, 1); + assert_eq!(synced_high.tier, 1); + assert_eq!(small_low.tier, 2); + assert_eq!(small_high.tier, 2); + 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() { - // Config should set the tiers as: + fn sync_tolerance_from_str() { + // 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, - sync_tolerance: Some(4), - small_sync_distance_modifier: Some(4), - medium_sync_distance_modifier: Some(4), - }; - let distance_tiers = BeaconNodeSyncDistanceTiers::from_config(&config); + + let distance_tiers = BeaconNodeSyncDistanceTiers::from_str("4,4,4").unwrap(); 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!(synced_high.tier == 1); - assert!(small_low.tier == 2); - assert!(small_high.tier == 2); - assert!(medium_low.tier == 4); + assert_eq!(synced_low.tier, 1); + assert_eq!(synced_high.tier, 1); + assert_eq!(small_low.tier, 2); + assert_eq!(small_high.tier, 2); + assert_eq!(medium_low.tier, 4); assert_eq!(medium_high.tier, 4); - assert!(large.tier == 10); + assert_eq!(large.tier, 10); } } diff --git a/validator_client/src/cli.rs b/validator_client/src/cli.rs index 28dce1c6df..ee939de31f 100644 --- a/validator_client/src/cli.rs +++ b/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") - .long("beacon-node-sync-tolerance") - .help("Sets the number of slots behind the head that each connected Beacon Node can be \ - to still be considered synced. Effectively this gives more priority to the first \ - connected Beacon Node.") - .takes_value(true), - ) - .arg( - Arg::with_name("beacon-node-small-sync-distance-modifier") - .long("beacon-node-small-sync-distance-modifier") - .help("Only use this if you know what you are doing. Incorrectly setting this value \ - can result in suboptimal fallback behaviour. Sets the size (in slots) of the \ - `small` sync distance range when calculating the health tiers of connected \ - Beacon Nodes. The range falls immediately after the end of the `synced` range.") - .takes_value(true), - ) - .arg( - Arg::with_name("beacon-node-medium-sync-distance-modifier") - .long("beacon-node-medium-sync-distance-modifier") - .help("Only use this if you know what you are doing. Incorrectly setting this value \ - can result in suboptimal fallback behaviour. Sets the size (in slots) of the \ - `medium` sync distance range when calculating the health tiers of connected \ - Beacon Nodes. The range falls immediately after the end of the `small` range.") - .takes_value(true), + Arg::with_name("beacon-nodes-sync-tolerances") + .long("beacon-nodes-sync-tolerances") + .value_name("SYNC_TOLERANCES") + .help("A comma-separated list of 3 values which sets the size of each sync distance range when \ + determining the health of each connected beacon node. \ + The first value determines the `Synced` range. \ + If a connected beacon node is synced to within this number of slots it is considered 'Synced'. \ + The second value determines the `Small` sync distance range. \ + This range starts immediately after the `Synced` range. \ + The third value determines the `Medium` sync distance range. \ + This range starts immediately after the `Small` range. \ + Any sync distance value beyond that is considered `Large`. \ + For example, a value of `8,8,48` would have ranges like the following: \ + `Synced`: 0..=8 \ + `Small`: 9..=16 \ + `Medium`: 17..=64 \ + `Large`: 65.. \ + These values are used to determine what ordering beacon node fallbacks are used in. \ + Generally, `Synced` nodes are preferred over `Small` and so on. \ + Nodes in the `Synced` range will tie-break based on their ordering in `--beacon-nodes`. \ + This ensures the primary beacon node is prioritised. \ + [default: 8,8,48]") + .takes_value(true) ) } diff --git a/validator_client/src/config.rs b/validator_client/src/config.rs index a89c035c44..b79837f9c9 100644 --- a/validator_client/src/config.rs +++ b/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") { - config.beacon_node_fallback.sync_tolerance = Some( - sync_tolerance - .parse::() - .map_err(|_| "beacon-node-sync-tolerance is not a valid u64.")?, - ); - } - - 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::() - .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::().map_err(|_| { - "beacon-node-medium-sync-distance-modifier is not a valid u64." - })?); + if let Some(sync_tolerance) = cli_args.value_of("beacon-nodes-sync-tolerances") { + config.beacon_node_fallback.sync_tolerances = + BeaconNodeSyncDistanceTiers::from_str(sync_tolerance)?; + } else { + config.beacon_node_fallback.sync_tolerances = BeaconNodeSyncDistanceTiers::default(); } /* diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index fce470f22d..31046c10c6 100644 --- a/validator_client/src/lib.rs +++ b/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;