Browse Source

[net] Fix issue Dag prune issue

There was an error in calculating the timestamp for the next "rotation",
i.e. when the Dag should prune.
This commit fixes:
- An underflow when calculating the sleep time for the Dag pruning
  process
- An off-by-one error that caused next_rotation_timestamp to give a
  timestamp in the past

Unit tests have also been added to prevent the above problems from
occurring again. They should be general enough to catch rotation periods
that are more complex than a period of `1`, which is what we are
currently using
y 2 years ago
parent
commit
d8e957788b
2 changed files with 43 additions and 4 deletions
  1. 3 3
      src/event_graph/mod.rs
  2. 40 1
      src/event_graph/util.rs

+ 3 - 3
src/event_graph/mod.rs

@@ -20,7 +20,6 @@ use std::{
     cmp::Ordering,
     collections::{HashMap, HashSet, VecDeque},
     sync::Arc,
-    time::UNIX_EPOCH,
 };
 
 use async_recursion::async_recursion;
@@ -35,7 +34,7 @@ use smol::{
 use crate::{
     net::P2pPtr,
     system::{sleep, timeout::timeout, StoppableTask, StoppableTaskPtr, Subscriber, SubscriberPtr},
-    Error, Result,
+    Error, Result, event_graph::util::seconds_until_next_rotation,
 };
 
 /// An event graph event
@@ -460,7 +459,8 @@ impl EventGraph {
             };
 
             // Sleep until it's time to rotate.
-            let s = UNIX_EPOCH.elapsed().unwrap().as_secs() - next_rotation;
+            let s = seconds_until_next_rotation(next_rotation);
+            
             debug!(target: "event_graph::dag_prune_task()", "Sleeping {}s until next DAG prune", s);
             sleep(s).await;
             debug!(target: "event_graph::dag_prune_task()", "Rotation period reached");

+ 40 - 1
src/event_graph/util.rs

@@ -57,7 +57,8 @@ pub(super) fn next_rotation_timestamp(starting_timestamp: u64, rotation_period:
     let days_passed = days_since(starting_timestamp);
 
     // Find out how many rotation periods have occurred since
-    // the starting point
+    // the starting point.
+    // Note: when rotation_period = 1, rotations_since_start = days_passed
     let rotations_since_start = (days_passed + rotation_period - 1) / rotation_period;
 
     // Find out the number of days until the next rotation. Panic if result is beyond the range
@@ -66,11 +67,32 @@ pub(super) fn next_rotation_timestamp(starting_timestamp: u64, rotation_period:
         (rotations_since_start * rotation_period - days_passed).try_into().unwrap();
 
     // Get the timestamp for the next rotation
+    if days_until_next_rotation == 0 { 
+        // If there are 0 days until the next rotation, we want
+        // to rotate tomorrow, at midnight. This is a special case.
+        return midnight_timestamp(1);
+    }
     midnight_timestamp(days_until_next_rotation)
 }
 
+/// Calculate the time in seconds until the next_rotation, given
+/// as a timestamp.
+/// `next_rotation` here represents a timestamp in UNIX epoch format.
+pub(super) fn seconds_until_next_rotation(next_rotation: u64) -> u64 {
+    // Store `now` in a variable in order to avoid a TOCTOU error.
+    // There may be a drift of one second between this panic check and 
+    // the return value if we get unlucky.
+    let now = UNIX_EPOCH.elapsed().unwrap().as_secs();
+    if next_rotation < now {
+        panic!("Next rotation timestamp is in the past");
+    }
+    next_rotation - now
+}
+
 #[cfg(test)]
 mod tests {
+    use crate::event_graph::INITIAL_GENESIS;
+
     use super::*;
 
     #[test]
@@ -91,6 +113,13 @@ mod tests {
         // So the next rotation should be 4 days from now.
         let expected = midnight_timestamp(4);
         assert_eq!(next_rotation_timestamp(starting_point, rotation_period), expected);
+
+        // When starting from today with a rotation period of 1 (day), 
+        // we should get tomorrow's timestamp.
+        // This is a special case.
+        let midnight_today: u64 = midnight_timestamp(0);
+        let midnight_tomorrow = midnight_today + 86400u64; // add a day, in seconds
+        assert_eq!(midnight_tomorrow, next_rotation_timestamp(midnight_today, 1));
     }
 
     #[test]
@@ -104,4 +133,14 @@ mod tests {
     fn test_next_rotation_timestamp_panics_on_division_by_zero() {
         next_rotation_timestamp(0, 0);
     }
+
+    #[test]
+    fn test_seconds_until_next_rotation_is_within_rotation_interval() {
+        let days_rotation = 1u64;
+        // The amount of time in seconds between rotations.
+        let rotation_interval = days_rotation * 86400u64;
+        let next_rotation_timestamp = next_rotation_timestamp(INITIAL_GENESIS, days_rotation);
+        let s = seconds_until_next_rotation(next_rotation_timestamp); 
+        assert!(s < rotation_interval);
+    }
 }