From b25a1352c1eecd50eba47fb4b3cd390a828554b0 Mon Sep 17 00:00:00 2001 From: Mac L Date: Tue, 1 Aug 2023 17:55:43 +1000 Subject: [PATCH] Fix todos and tests --- validator_client/src/beacon_node_fallback.rs | 17 +++++++-- validator_client/src/beacon_node_health.rs | 39 ++++++-------------- validator_client/src/http_api/tests.rs | 1 + validator_client/src/notifier.rs | 1 - 4 files changed, 25 insertions(+), 33 deletions(-) diff --git a/validator_client/src/beacon_node_fallback.rs b/validator_client/src/beacon_node_fallback.rs index 4e18246dad..f012f55c60 100644 --- a/validator_client/src/beacon_node_fallback.rs +++ b/validator_client/src/beacon_node_fallback.rs @@ -23,7 +23,7 @@ use std::marker::PhantomData; use std::sync::Arc; use std::time::{Duration, Instant}; use tokio::{sync::RwLock, time::sleep}; -use types::{ChainSpec, Config as ConfigSpec, EthSpec}; +use types::{ChainSpec, Config as ConfigSpec, EthSpec, Slot}; /// Message emitted when the VC detects the BN is using a different spec. const UPDATE_REQUIRED_LOG_HINT: &str = "this VC or the remote BN may need updating"; @@ -37,6 +37,10 @@ const UPDATE_REQUIRED_LOG_HINT: &str = "this VC or the remote BN may need updati /// having the correct nodes up and running prior to the start of the slot. const SLOT_LOOKAHEAD: Duration = Duration::from_secs(2); +/// If the beacon node slot_clock is within 1 slot, this is deemed acceptable. Otherwise the node +/// will be marked as CandidateError::TimeDiscrepancy. +const FUTURE_SLOT_TOLERANCE: Slot = Slot::new(1); + // Configuration for the Beacon Node fallback. #[derive(Copy, Clone, Debug, Default, Serialize, Deserialize)] pub struct Config { @@ -213,6 +217,13 @@ impl CandidateBeaconNode { if let Some(slot_clock) = slot_clock { match check_node_health(&self.beacon_node, log).await { Ok((head, is_optimistic, el_offline)) => { + let slot_clock_head = slot_clock.now().ok_or(CandidateError::Uninitialized)?; + + if head > slot_clock_head + FUTURE_SLOT_TOLERANCE { + return Err(CandidateError::TimeDiscrepancy); + } + let sync_distance = slot_clock_head.saturating_sub(head); + // Currently ExecutionEngineHealth is solely determined by online status. let execution_status = if el_offline { ExecutionEngineHealth::Unhealthy @@ -228,15 +239,13 @@ impl CandidateBeaconNode { let new_health = BeaconNodeHealth::from_status( self.id, + sync_distance, head, optimistic_status, execution_status, distance_tiers, - slot_clock, ); - // TODO(mac): Set metric here. - *self.health.write() = Ok(new_health); Ok(()) } diff --git a/validator_client/src/beacon_node_health.rs b/validator_client/src/beacon_node_health.rs index ed91d3a2c5..81df0cd992 100644 --- a/validator_client/src/beacon_node_health.rs +++ b/validator_client/src/beacon_node_health.rs @@ -1,18 +1,17 @@ use crate::beacon_node_fallback::Config; use serde_derive::{Deserialize, Serialize}; -use slot_clock::SlotClock; use std::cmp::Ordering; use std::fmt::{Debug, Display, Formatter}; use types::Slot; -// Sync distances between 0 and DEFAULT_SYNC_TOLERANCE are considered `synced`. -// Sync distance tiers are determined by the different modifiers. -// -// The default range is the following: -// Synced: 0..=8 -// Small: 9..=16 -// Medium: 17..=64 -// Large: 65.. +/// Sync distances between 0 and DEFAULT_SYNC_TOLERANCE are considered `synced`. +/// Sync distance tiers are determined by the different modifiers. +/// +/// The default range is the following: +/// Synced: 0..=8 +/// Small: 9..=16 +/// Medium: 17..=64 +/// Large: 65.. const DEFAULT_SYNC_TOLERANCE: Slot = Slot::new(8); const DEFAULT_SMALL_SYNC_DISTANCE_MODIFIER: Slot = Slot::new(8); const DEFAULT_MEDIUM_SYNC_DISTANCE_MODIFIER: Slot = Slot::new(48); @@ -195,15 +194,14 @@ impl PartialOrd for BeaconNodeHealth { } impl BeaconNodeHealth { - pub fn from_status( + pub fn from_status( id: usize, + sync_distance: Slot, head: Slot, optimistic_status: IsOptimistic, execution_status: ExecutionEngineHealth, distance_tiers: &BeaconNodeSyncDistanceTiers, - slot_clock: &T, ) -> Self { - let sync_distance = BeaconNodeHealth::compute_sync_distance(head, slot_clock); let health_tier = BeaconNodeHealth::compute_health_tier( sync_distance, optimistic_status, @@ -228,14 +226,6 @@ impl BeaconNodeHealth { self.health_tier } - fn compute_sync_distance(head: Slot, slot_clock: &T) -> SyncDistance { - // TODO(mac) May be worth distinguishing between nodes that are ahead of the `slot_clock`. - slot_clock - .now() - .map(|head_slot| head_slot.saturating_sub(head)) - .unwrap_or(Slot::max_value()) - } - fn compute_health_tier( sync_distance: SyncDistance, optimistic_status: IsOptimistic, @@ -306,20 +296,13 @@ mod tests { SyncDistanceTier, }; use crate::beacon_node_fallback::Config; - use slot_clock::{SlotClock, TestingSlotClock}; - use std::time::Duration; use types::Slot; #[test] fn all_possible_health_tiers() { - let current_head = Slot::new(64); - let config = Config::default(); let beacon_node_sync_distance_tiers = BeaconNodeSyncDistanceTiers::from_config(&config); - let slot_clock = - TestingSlotClock::new(current_head, Duration::from_secs(0), Duration::from_secs(1)); - let mut health_vec = vec![]; for head_slot in 0..=64 { @@ -327,11 +310,11 @@ mod tests { for ee_health in &[Healthy, Unhealthy] { let health = BeaconNodeHealth::from_status( 0, + Slot::new(0), Slot::new(head_slot), *optimistic_status, *ee_health, &beacon_node_sync_distance_tiers, - &slot_clock, ); health_vec.push(health); } diff --git a/validator_client/src/http_api/tests.rs b/validator_client/src/http_api/tests.rs index 3bff444703..35dcfffc55 100644 --- a/validator_client/src/http_api/tests.rs +++ b/validator_client/src/http_api/tests.rs @@ -110,6 +110,7 @@ impl ApiTester { let context = Arc::new(Context { task_executor: test_runtime.task_executor.clone(), api_secret, + block_service: None, validator_dir: Some(validator_dir.path().into()), secrets_dir: Some(secrets_dir.path().into()), validator_store: Some(validator_store.clone()), diff --git a/validator_client/src/notifier.rs b/validator_client/src/notifier.rs index 1e7940de57..521ff49a14 100644 --- a/validator_client/src/notifier.rs +++ b/validator_client/src/notifier.rs @@ -84,7 +84,6 @@ async fn notify( set_gauge(&http_metrics::metrics::ETH2_FALLBACK_CONNECTED, 0); } - // TODO(mac) Store all connected node info into metrics. for info in candidate_info { if let Some(health) = info.health { debug!(