Refactor slot clock.

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<Slot>;
 
-    fn present_slot(&self) -> Result<Option<Slot>, Self::Error>;
+    fn duration_to_next_slot(&self) -> Option<Duration>;
 
-    fn duration_to_next_slot(&self) -> Result<Option<Duration>, Self::Error>;
-
-    fn slot_duration_millis(&self) -> u64;
+    fn slot_duration(&self) -> Duration;
 }

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<T: EthSpec, U: SlotClock>(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<T: EthSpec, U: SlotClock>(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,
+    );
 }

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<Option<Slot>, Error> {
-        if self.slot_duration_seconds == 0 {
-            return Err(Error::SlotDurationIsZero);
-        }
+    fn present_slot(&self) -> Option<Slot> {
+        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<Option<Duration>, Error> {
-        duration_to_next_slot(self.genesis_seconds, self.slot_duration_seconds)
+    fn duration_to_next_slot(&self) -> Option<Duration> {
+        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<SystemTimeError> for Error {
-    fn from(e: SystemTimeError) -> Error {
-        Error::SystemTimeError(format!("{:?}", e))
-    }
-}
-
-fn slot_from_duration(slot_duration_seconds: u64, duration: Duration) -> Option<Slot> {
-    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<Option<Duration>, 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));
     }
 }

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<Slot>,
 }
@@ -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<Option<Slot>, Error> {
+    fn present_slot(&self) -> Option<Slot> {
         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<Option<Duration>, Error> {
-        Ok(Some(Duration::from_secs(1)))
+    fn duration_to_next_slot(&self) -> Option<Duration> {
+        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)));
     }
 }