Refactor slot clock to remove underflow

Previously I had used `Instant` to refer to the genesis time.

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -158,12 +158,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         ));
 
         // Slot clock
-        let slot_clock = T::SlotClock::from_eth2_genesis(
+        let slot_clock = T::SlotClock::new(
             spec.genesis_slot,
-            genesis_state.genesis_time,
+            Duration::from_secs(genesis_state.genesis_time),
             Duration::from_millis(spec.milliseconds_per_slot),
-        )
+        );
-        .map_err(|_| Error::SlotClockDidNotStart)?;
 
         info!(log, "Beacon chain initialized from genesis";
               "validator_count" => genesis_state.validators.len(),
@@ -202,12 +201,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
         let state = &p.canonical_head.beacon_state;
 
-        let slot_clock = T::SlotClock::from_eth2_genesis(
+        let slot_clock = T::SlotClock::new(
             spec.genesis_slot,
-            state.genesis_time,
+            Duration::from_secs(state.genesis_time),
             Duration::from_millis(spec.milliseconds_per_slot),
-        )
+        );
-        .map_err(|_| Error::SlotClockDidNotStart)?;
 
         let last_finalized_root = p.canonical_head.beacon_state.finalized_checkpoint.root;
         let last_finalized_block = &p.canonical_head.beacon_block;

eth2/utils/slot_clock/src/lib.rs

@@ -5,40 +5,27 @@ mod metrics;
 mod system_time_slot_clock;
 mod testing_slot_clock;
 
-use std::time::{Duration, Instant, SystemTime, SystemTimeError, UNIX_EPOCH};
+use std::time::Duration;
 
 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;
 
+/// A clock that reports the current slot.
+///
+/// The clock is not required to be monotonically increasing and may go backwards.
 pub trait SlotClock: Send + Sync + Sized {
-    fn from_eth2_genesis(
+    /// Creates a new slot clock where the first slot is `genesis_slot`, genesis occured
-        genesis_slot: Slot,
+    /// `genesis_duration` after the `UNIX_EPOCH` and each slot is `slot_duration` apart.
-        genesis_seconds: u64,
+    fn new(genesis_slot: Slot, genesis_duration: Duration, slot_duration: Duration) -> Self;
-        slot_duration: Duration,
-    ) -> Result<Self, SystemTimeError> {
-        let duration_between_now_and_unix_epoch = SystemTime::now().duration_since(UNIX_EPOCH)?;
-        let duration_between_unix_epoch_and_genesis = Duration::from_secs(genesis_seconds);
-
-        let genesis_instant = if duration_between_now_and_unix_epoch
-            < duration_between_unix_epoch_and_genesis
-        {
-            Instant::now()
-                + (duration_between_unix_epoch_and_genesis - duration_between_now_and_unix_epoch)
-        } else {
-            Instant::now()
-                - (duration_between_now_and_unix_epoch - duration_between_unix_epoch_and_genesis)
-        };
-
-        Ok(Self::new(genesis_slot, genesis_instant, slot_duration))
-    }
-
-    fn new(genesis_slot: Slot, genesis: Instant, slot_duration: Duration) -> Self;
 
+    /// Returns the slot at this present time.
     fn now(&self) -> Option<Slot>;
 
-    fn duration_to_next_slot(&self) -> Option<Duration>;
+    /// Returns the duration between slots
-
     fn slot_duration(&self) -> Duration;
+
+    /// Returns the duration until the next slot.
+    fn duration_to_next_slot(&self) -> Option<Duration>;
 }

eth2/utils/slot_clock/src/system_time_slot_clock.rs

@@ -1,5 +1,5 @@
 use super::SlotClock;
-use std::time::{Duration, Instant};
+use std::time::{Duration, SystemTime, UNIX_EPOCH};
 use types::Slot;
 
 pub use std::time::SystemTimeError;
@@ -8,53 +8,60 @@ pub use std::time::SystemTimeError;
 #[derive(Clone)]
 pub struct SystemTimeSlotClock {
     genesis_slot: Slot,
-    genesis: Instant,
+    genesis_duration: Duration,
     slot_duration: Duration,
 }
 
 impl SlotClock for SystemTimeSlotClock {
-    fn new(genesis_slot: Slot, genesis: Instant, slot_duration: Duration) -> Self {
+    fn new(genesis_slot: Slot, genesis_duration: Duration, slot_duration: Duration) -> Self {
         if slot_duration.as_millis() == 0 {
             panic!("SystemTimeSlotClock cannot have a < 1ms slot duration.");
         }
 
         Self {
             genesis_slot,
-            genesis,
+            genesis_duration,
             slot_duration,
         }
     }
 
     fn now(&self) -> Option<Slot> {
-        let now = Instant::now();
+        let now = SystemTime::now().duration_since(UNIX_EPOCH).ok()?;
+        let genesis = self.genesis_duration;
 
-        if now < self.genesis {
+        if now > genesis {
-            None
+            let since_genesis = now
-        } else {
+                .checked_sub(genesis)
-            let slot = Slot::from(
+                .expect("Control flow ensures now is greater than genesis");
-                (now.duration_since(self.genesis).as_millis() / self.slot_duration.as_millis())
+            let slot =
-                    as u64,
+                Slot::from((since_genesis.as_millis() / self.slot_duration.as_millis()) as u64);
-            );
             Some(slot + self.genesis_slot)
+        } else {
+            None
         }
     }
 
     fn duration_to_next_slot(&self) -> Option<Duration> {
-        let now = Instant::now();
+        let now = SystemTime::now().duration_since(UNIX_EPOCH).ok()?;
-        if now < self.genesis {
+        let genesis = self.genesis_duration;
-            Some(self.genesis - now)
+
+        let slot_start = |slot: Slot| -> Duration {
+            let slot = slot.as_u64() as u32;
+            genesis + slot * self.slot_duration
+        };
+
+        if now > genesis {
+            Some(
+                slot_start(self.now()? + 1)
+                    .checked_sub(now)
+                    .expect("The next slot cannot start before now"),
+            )
         } else {
-            let duration_since_genesis = now - self.genesis;
+            Some(
-            let millis_since_genesis = duration_since_genesis.as_millis();
+                genesis
-            let millis_per_slot = self.slot_duration.as_millis();
+                    .checked_sub(now)
-
+                    .expect("Control flow ensures genesis is greater than or equal to now"),
-            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))
         }
     }
 
@@ -75,30 +82,28 @@ mod tests {
     fn test_slot_now() {
         let genesis_slot = Slot::new(0);
 
-        let prior_genesis =
+        let prior_genesis = |milliseconds_prior: u64| {
-            |seconds_prior: u64| Instant::now() - Duration::from_secs(seconds_prior);
+            SystemTime::now()
+                .duration_since(UNIX_EPOCH)
+                .expect("should get system time")
+                - Duration::from_millis(milliseconds_prior)
+        };
 
         let clock =
             SystemTimeSlotClock::new(genesis_slot, prior_genesis(0), Duration::from_secs(1));
         assert_eq!(clock.now(), Some(Slot::new(0)));
 
         let clock =
-            SystemTimeSlotClock::new(genesis_slot, prior_genesis(5), Duration::from_secs(1));
+            SystemTimeSlotClock::new(genesis_slot, prior_genesis(5_000), Duration::from_secs(1));
         assert_eq!(clock.now(), Some(Slot::new(5)));
 
-        let clock = SystemTimeSlotClock::new(
+        let clock =
-            genesis_slot,
+            SystemTimeSlotClock::new(genesis_slot, prior_genesis(500), Duration::from_secs(1));
-            Instant::now() - Duration::from_millis(500),
-            Duration::from_secs(1),
-        );
         assert_eq!(clock.now(), Some(Slot::new(0)));
         assert!(clock.duration_to_next_slot().unwrap() < Duration::from_millis(500));
 
-        let clock = SystemTimeSlotClock::new(
+        let clock =
-            genesis_slot,
+            SystemTimeSlotClock::new(genesis_slot, prior_genesis(1_500), Duration::from_secs(1));
-            Instant::now() - Duration::from_millis(1_500),
-            Duration::from_secs(1),
-        );
         assert_eq!(clock.now(), Some(Slot::new(1)));
         assert!(clock.duration_to_next_slot().unwrap() < Duration::from_millis(500));
     }
@@ -106,18 +111,26 @@ mod tests {
     #[test]
     #[should_panic]
     fn zero_seconds() {
-        SystemTimeSlotClock::new(Slot::new(0), Instant::now(), Duration::from_secs(0));
+        SystemTimeSlotClock::new(Slot::new(0), Duration::from_secs(0), Duration::from_secs(0));
     }
 
     #[test]
     #[should_panic]
     fn zero_millis() {
-        SystemTimeSlotClock::new(Slot::new(0), Instant::now(), Duration::from_millis(0));
+        SystemTimeSlotClock::new(
+            Slot::new(0),
+            Duration::from_secs(0),
+            Duration::from_millis(0),
+        );
     }
 
     #[test]
     #[should_panic]
     fn less_than_one_millis() {
-        SystemTimeSlotClock::new(Slot::new(0), Instant::now(), Duration::from_nanos(999));
+        SystemTimeSlotClock::new(
+            Slot::new(0),
+            Duration::from_secs(0),
+            Duration::from_nanos(999),
+        );
     }
 }

eth2/utils/slot_clock/src/testing_slot_clock.rs

@@ -1,6 +1,6 @@
 use super::SlotClock;
 use std::sync::RwLock;
-use std::time::{Duration, Instant};
+use std::time::Duration;
 use types::Slot;
 
 /// A slot clock where the slot is manually set instead of being determined by the system time.
@@ -21,7 +21,7 @@ impl TestingSlotClock {
 }
 
 impl SlotClock for TestingSlotClock {
-    fn new(genesis_slot: Slot, _genesis: Instant, _slot_duration: Duration) -> Self {
+    fn new(genesis_slot: Slot, _genesis_duration: Duration, _slot_duration: Duration) -> Self {
         TestingSlotClock {
             slot: RwLock::new(genesis_slot),
         }
@@ -49,7 +49,9 @@ mod tests {
 
     #[test]
     fn test_slot_now() {
-        let clock = TestingSlotClock::new(Slot::new(10), Instant::now(), Duration::from_secs(0));
+        let null = Duration::from_secs(0);
+
+        let clock = TestingSlotClock::new(Slot::new(10), null, null);
         assert_eq!(clock.now(), Some(Slot::new(10)));
         clock.set_slot(123);
         assert_eq!(clock.now(), Some(Slot::new(123)));

validator_client/src/service.rs

@@ -159,14 +159,11 @@ impl<B: BeaconNodeDuties + 'static, S: Signer + 'static, E: EthSpec> Service<B,
         };
 
         // build the validator slot clock
-        let slot_clock = SystemTimeSlotClock::from_eth2_genesis(
+        let slot_clock = SystemTimeSlotClock::new(
             genesis_slot,
-            genesis_time,
+            Duration::from_secs(genesis_time),
             Duration::from_millis(eth2_config.spec.milliseconds_per_slot),
-        )
+        );
-        .map_err::<error_chain::Error, _>(|e| {
-            format!("Unable to start slot clock: {}.", e).into()
-        })?;
 
         /* Generate the duties manager */