From 7bfe02be1cde9c08bb24b3b8ed5b0c5689d96327 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 29 Aug 2019 12:46:18 +1000 Subject: [PATCH] Refactor slot clock. --- eth2/utils/slot_clock/src/lib.rs | 19 +- eth2/utils/slot_clock/src/metrics.rs | 7 +- .../slot_clock/src/system_time_slot_clock.rs | 191 +++++++----------- .../slot_clock/src/testing_slot_clock.rs | 37 ++-- 4 files changed, 98 insertions(+), 156 deletions(-) diff --git a/eth2/utils/slot_clock/src/lib.rs b/eth2/utils/slot_clock/src/lib.rs index 871743c9e..988f3d322 100644 --- a/eth2/utils/slot_clock/src/lib.rs +++ b/eth2/utils/slot_clock/src/lib.rs @@ -5,24 +5,19 @@ mod metrics; mod system_time_slot_clock; mod testing_slot_clock; -use std::time::Duration; +use std::time::{Duration, Instant}; -pub use crate::system_time_slot_clock::{Error as SystemTimeSlotClockError, SystemTimeSlotClock}; -pub use crate::testing_slot_clock::{Error as TestingSlotClockError, TestingSlotClock}; +pub use crate::system_time_slot_clock::SystemTimeSlotClock; +pub use crate::testing_slot_clock::TestingSlotClock; pub use metrics::scrape_for_metrics; pub use types::Slot; pub trait SlotClock: Send + Sync + Sized { - type Error; + fn new(genesis_slot: Slot, genesis: Instant, slot_duration: Duration) -> Self; - /// Create a new `SlotClock`. - /// - /// Returns an Error if `slot_duration_seconds == 0`. - fn new(genesis_slot: Slot, genesis_seconds: u64, slot_duration_seconds: u64) -> Self; + fn present_slot(&self) -> Option; - fn present_slot(&self) -> Result, Self::Error>; + fn duration_to_next_slot(&self) -> Option; - fn duration_to_next_slot(&self) -> Result, Self::Error>; - - fn slot_duration_millis(&self) -> u64; + fn slot_duration(&self) -> Duration; } diff --git a/eth2/utils/slot_clock/src/metrics.rs b/eth2/utils/slot_clock/src/metrics.rs index e0d3923e0..1abd93c48 100644 --- a/eth2/utils/slot_clock/src/metrics.rs +++ b/eth2/utils/slot_clock/src/metrics.rs @@ -18,7 +18,7 @@ lazy_static! { /// Update the global metrics `DEFAULT_REGISTRY` with info from the slot clock. pub fn scrape_for_metrics(clock: &U) { let present_slot = match clock.present_slot() { - Ok(Some(slot)) => slot, + Some(slot) => slot, _ => Slot::new(0), }; @@ -28,5 +28,8 @@ pub fn scrape_for_metrics(clock: &U) { present_slot.epoch(T::slots_per_epoch()).as_u64() as i64, ); set_gauge(&SLOTS_PER_EPOCH, T::slots_per_epoch() as i64); - set_gauge(&MILLISECONDS_PER_SLOT, clock.slot_duration_millis() as i64); + set_gauge( + &MILLISECONDS_PER_SLOT, + clock.slot_duration().as_millis() as i64, + ); } diff --git a/eth2/utils/slot_clock/src/system_time_slot_clock.rs b/eth2/utils/slot_clock/src/system_time_slot_clock.rs index c493a8be8..88c9c0e63 100644 --- a/eth2/utils/slot_clock/src/system_time_slot_clock.rs +++ b/eth2/utils/slot_clock/src/system_time_slot_clock.rs @@ -1,99 +1,68 @@ use super::SlotClock; -use std::time::{Duration, SystemTime}; +use std::time::{Duration, Instant}; use types::Slot; pub use std::time::SystemTimeError; -#[derive(Debug, PartialEq)] -pub enum Error { - SlotDurationIsZero, - SystemTimeError(String), -} - /// Determines the present slot based upon the present system time. #[derive(Clone)] pub struct SystemTimeSlotClock { genesis_slot: Slot, - genesis_seconds: u64, - slot_duration_seconds: u64, + genesis: Instant, + slot_duration: Duration, } impl SlotClock for SystemTimeSlotClock { - type Error = Error; + fn new(genesis_slot: Slot, genesis: Instant, slot_duration: Duration) -> Self { + if slot_duration.as_millis() == 0 { + panic!("SystemTimeSlotClock cannot have a < 1ms slot duration."); + } - /// Create a new `SystemTimeSlotClock`. - /// - /// Returns an Error if `slot_duration_seconds == 0`. - fn new(genesis_slot: Slot, genesis_seconds: u64, slot_duration_seconds: u64) -> Self { Self { genesis_slot, - genesis_seconds, - slot_duration_seconds, + genesis, + slot_duration, } } - fn present_slot(&self) -> Result, Error> { - if self.slot_duration_seconds == 0 { - return Err(Error::SlotDurationIsZero); - } + fn present_slot(&self) -> Option { + let now = Instant::now(); - let syslot_time = SystemTime::now(); - let duration_since_epoch = syslot_time.duration_since(SystemTime::UNIX_EPOCH)?; - let duration_since_genesis = - duration_since_epoch.checked_sub(Duration::from_secs(self.genesis_seconds)); - - match duration_since_genesis { - None => Ok(None), - Some(d) => Ok(slot_from_duration(self.slot_duration_seconds, d) - .and_then(|s| Some(s + self.genesis_slot))), + if now < self.genesis { + None + } else { + let slot = Slot::from( + (now.duration_since(self.genesis).as_millis() / self.slot_duration.as_millis()) + as u64, + ); + Some(slot + self.genesis_slot) } } - fn duration_to_next_slot(&self) -> Result, Error> { - duration_to_next_slot(self.genesis_seconds, self.slot_duration_seconds) + fn duration_to_next_slot(&self) -> Option { + let now = Instant::now(); + if now < self.genesis { + None + } else { + let duration_since_genesis = now - self.genesis; + let millis_since_genesis = duration_since_genesis.as_millis(); + let millis_per_slot = self.slot_duration.as_millis(); + + let current_slot = millis_since_genesis / millis_per_slot; + let next_slot = current_slot + 1; + + let next_slot = + self.genesis + Duration::from_millis((next_slot * millis_per_slot) as u64); + + Some(next_slot.duration_since(now)) + } } - fn slot_duration_millis(&self) -> u64 { - self.slot_duration_seconds * 1000 + fn slot_duration(&self) -> Duration { + self.slot_duration } } -impl From for Error { - fn from(e: SystemTimeError) -> Error { - Error::SystemTimeError(format!("{:?}", e)) - } -} - -fn slot_from_duration(slot_duration_seconds: u64, duration: Duration) -> Option { - Some(Slot::new( - duration.as_secs().checked_div(slot_duration_seconds)?, - )) -} -// calculate the duration to the next slot -fn duration_to_next_slot( - genesis_time: u64, - seconds_per_slot: u64, -) -> Result, Error> { - let now = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH)?; - let genesis_time = Duration::from_secs(genesis_time); - - if now < genesis_time { - return Ok(None); - } - - let since_genesis = now - genesis_time; - - let elapsed_slots = since_genesis.as_secs() / seconds_per_slot; - - let next_slot_start_seconds = (elapsed_slots + 1) - .checked_mul(seconds_per_slot) - .expect("Next slot time should not overflow u64"); - - let time_to_next_slot = Duration::from_secs(next_slot_start_seconds) - since_genesis; - - Ok(Some(time_to_next_slot)) -} - #[cfg(test)] mod tests { use super::*; @@ -104,71 +73,51 @@ mod tests { */ #[test] fn test_slot_now() { - let slot_time = 100; let genesis_slot = Slot::new(0); - let now = SystemTime::now(); - let since_epoch = now.duration_since(SystemTime::UNIX_EPOCH).unwrap(); + let prior_genesis = + |seconds_prior: u64| Instant::now() - Duration::from_secs(seconds_prior); - let genesis = since_epoch.as_secs() - slot_time * 89; + let clock = + SystemTimeSlotClock::new(genesis_slot, prior_genesis(0), Duration::from_secs(1)); + assert_eq!(clock.present_slot(), Some(Slot::new(0))); - let clock = SystemTimeSlotClock { + let clock = + SystemTimeSlotClock::new(genesis_slot, prior_genesis(5), Duration::from_secs(1)); + assert_eq!(clock.present_slot(), Some(Slot::new(5))); + + let clock = SystemTimeSlotClock::new( genesis_slot, - genesis_seconds: genesis, - slot_duration_seconds: slot_time, - }; - assert_eq!(clock.present_slot().unwrap(), Some(Slot::new(89))); + Instant::now() - Duration::from_millis(500), + Duration::from_secs(1), + ); + assert_eq!(clock.present_slot(), Some(Slot::new(0))); + assert!(clock.duration_to_next_slot().unwrap() < Duration::from_millis(500)); - let clock = SystemTimeSlotClock { + let clock = SystemTimeSlotClock::new( genesis_slot, - genesis_seconds: since_epoch.as_secs(), - slot_duration_seconds: slot_time, - }; - assert_eq!(clock.present_slot().unwrap(), Some(Slot::new(0))); - - let clock = SystemTimeSlotClock { - genesis_slot, - genesis_seconds: since_epoch.as_secs() - slot_time * 42 - 5, - slot_duration_seconds: slot_time, - }; - assert_eq!(clock.present_slot().unwrap(), Some(Slot::new(42))); + Instant::now() - Duration::from_millis(1_500), + Duration::from_secs(1), + ); + assert_eq!(clock.present_slot(), Some(Slot::new(1))); + assert!(clock.duration_to_next_slot().unwrap() < Duration::from_millis(500)); } #[test] - fn test_slot_from_duration() { - let slot_time = 100; - - assert_eq!( - slot_from_duration(slot_time, Duration::from_secs(0)), - Some(Slot::new(0)) - ); - assert_eq!( - slot_from_duration(slot_time, Duration::from_secs(10)), - Some(Slot::new(0)) - ); - assert_eq!( - slot_from_duration(slot_time, Duration::from_secs(100)), - Some(Slot::new(1)) - ); - assert_eq!( - slot_from_duration(slot_time, Duration::from_secs(101)), - Some(Slot::new(1)) - ); - assert_eq!( - slot_from_duration(slot_time, Duration::from_secs(1000)), - Some(Slot::new(10)) - ); + #[should_panic] + fn zero_seconds() { + SystemTimeSlotClock::new(Slot::new(0), Instant::now(), Duration::from_secs(0)); } #[test] - fn test_slot_from_duration_slot_time_zero() { - let slot_time = 0; + #[should_panic] + fn zero_millis() { + SystemTimeSlotClock::new(Slot::new(0), Instant::now(), Duration::from_millis(0)); + } - assert_eq!(slot_from_duration(slot_time, Duration::from_secs(0)), None); - assert_eq!(slot_from_duration(slot_time, Duration::from_secs(10)), None); - assert_eq!( - slot_from_duration(slot_time, Duration::from_secs(1000)), - None - ); + #[test] + #[should_panic] + fn less_than_one_millis() { + SystemTimeSlotClock::new(Slot::new(0), Instant::now(), Duration::from_nanos(999)); } } diff --git a/eth2/utils/slot_clock/src/testing_slot_clock.rs b/eth2/utils/slot_clock/src/testing_slot_clock.rs index f741d3b87..0b65b1569 100644 --- a/eth2/utils/slot_clock/src/testing_slot_clock.rs +++ b/eth2/utils/slot_clock/src/testing_slot_clock.rs @@ -1,12 +1,11 @@ use super::SlotClock; use std::sync::RwLock; -use std::time::Duration; +use std::time::{Duration, Instant}; use types::Slot; -#[derive(Debug, PartialEq)] -pub enum Error {} - -/// Determines the present slot based upon the present system time. +/// A slot clock where the slot is manually set instead of being determined by the system time. +/// +/// Useful for testing scenarios. pub struct TestingSlotClock { slot: RwLock, } @@ -17,32 +16,30 @@ impl TestingSlotClock { } pub fn advance_slot(&self) { - self.set_slot(self.present_slot().unwrap().unwrap().as_u64() + 1) + self.set_slot(self.present_slot().unwrap().as_u64() + 1) } } impl SlotClock for TestingSlotClock { - type Error = Error; - - /// Create a new `TestingSlotClock` at `genesis_slot`. - fn new(genesis_slot: Slot, _genesis_seconds: u64, _slot_duration_seconds: u64) -> Self { + fn new(genesis_slot: Slot, _genesis: Instant, _slot_duration: Duration) -> Self { TestingSlotClock { slot: RwLock::new(genesis_slot), } } - fn present_slot(&self) -> Result, Error> { + fn present_slot(&self) -> Option { let slot = *self.slot.read().expect("TestingSlotClock poisoned."); - Ok(Some(slot)) + Some(slot) } /// Always returns a duration of 1 second. - fn duration_to_next_slot(&self) -> Result, Error> { - Ok(Some(Duration::from_secs(1))) + fn duration_to_next_slot(&self) -> Option { + Some(Duration::from_secs(1)) } - fn slot_duration_millis(&self) -> u64 { - 0 + /// Always returns a slot duration of 0 seconds. + fn slot_duration(&self) -> Duration { + Duration::from_secs(0) } } @@ -52,11 +49,9 @@ mod tests { #[test] fn test_slot_now() { - let null = 0; - - let clock = TestingSlotClock::new(Slot::new(10), null, null); - assert_eq!(clock.present_slot(), Ok(Some(Slot::new(10)))); + let clock = TestingSlotClock::new(Slot::new(10), Instant::now(), Duration::from_secs(0)); + assert_eq!(clock.present_slot(), Some(Slot::new(10))); clock.set_slot(123); - assert_eq!(clock.present_slot(), Ok(Some(Slot::new(123)))); + assert_eq!(clock.present_slot(), Some(Slot::new(123))); } }