sim-lib: only use start delay if explicitly specified

As is, we'll run with a 0 second start delay for:
- Random activities
- Defined activities with no `start_secs` specified

This very likely isn't the intent for defined activities (people should
set `start_secs=0` if they want their payments to fire immediately),
and doesn't work for random activity because we just fire all payments
on start.

Instead, we switch over to using our payment wait as the start delay in
the absence of this field being set for defined activities and always
for random activities.

Author: carlaKC

sim-lib/src/defined_activity.rs

@@ -8,7 +8,7 @@ use tokio::time::Duration;
 #[derive(Clone)]
 pub struct DefinedPaymentActivity {
     destination: NodeInfo,
-    start: Duration,
+    start: Option<Duration>,
     count: Option<u64>,
     wait: ValueOrRange<u16>,
     amount: ValueOrRange<u64>,
@@ -17,7 +17,7 @@ pub struct DefinedPaymentActivity {
 impl DefinedPaymentActivity {
     pub fn new(
         destination: NodeInfo,
-        start: Duration,
+        start: Option<Duration>,
         count: Option<u64>,
         wait: ValueOrRange<u16>,
         amount: ValueOrRange<u64>,
@@ -52,7 +52,7 @@ impl DestinationGenerator for DefinedPaymentActivity {
 }
 
 impl PaymentGenerator for DefinedPaymentActivity {
-    fn payment_start(&self) -> Duration {
+    fn payment_start(&self) -> Option<Duration> {
         self.start
     }
 
@@ -81,7 +81,6 @@ impl PaymentGenerator for DefinedPaymentActivity {
 #[cfg(test)]
 mod tests {
     use super::DefinedPaymentActivity;
-    use super::*;
     use crate::test_utils::{create_nodes, get_random_keypair};
     use crate::{DestinationGenerator, PaymentGenerationError, PaymentGenerator};
 
@@ -95,7 +94,7 @@ mod tests {
 
         let generator = DefinedPaymentActivity::new(
             node.clone(),
-            Duration::from_secs(0),
+            None,
             None,
             crate::ValueOrRange::Value(60),
             crate::ValueOrRange::Value(payment_amt),

sim-lib/src/lib.rs

@@ -185,8 +185,7 @@ pub struct ActivityParser {
     #[serde(with = "serializers::serde_node_id")]
     pub destination: NodeId,
     /// The time in the simulation to start the payment.
-    #[serde(default)]
-    pub start_secs: u16,
+    pub start_secs: Option<u16>,
     /// The number of payments to send over the course of the simulation.
     #[serde(default)]
     pub count: Option<u64>,
@@ -207,7 +206,7 @@ pub struct ActivityDefinition {
     /// The destination of the payment.
     pub destination: NodeInfo,
     /// The time in the simulation to start the payment.
-    pub start_secs: u16,
+    pub start_secs: Option<u16>,
     /// The number of payments to send over the course of the simulation.
     pub count: Option<u64>,
     /// The interval of the event, as in every how many seconds the payment is performed.
@@ -328,7 +327,7 @@ pub struct PaymentGenerationError(String);
 
 pub trait PaymentGenerator: Display + Send {
     /// Returns the time that the payments should start
-    fn payment_start(&self) -> Duration;
+    fn payment_start(&self) -> Option<Duration>;
 
     /// Returns the number of payments that should be made
     fn payment_count(&self) -> Option<u64>;
@@ -819,7 +818,9 @@ impl Simulation {
             for description in self.activity.iter() {
                 let activity_generator = DefinedPaymentActivity::new(
                     description.destination.clone(),
-                    Duration::from_secs(description.start_secs.into()),
+                    description
+                        .start_secs
+                        .map(|start| Duration::from_secs(start.into())),
                     description.count,
                     description.interval_secs,
                     description.amount_msat,
@@ -1087,22 +1088,7 @@ async fn produce_events<N: DestinationGenerator + ?Sized, A: PaymentGenerator +
             }
         }
 
-        let wait: Duration = if current_count == 0 {
-            let start = node_generator.payment_start();
-            if start != Duration::from_secs(0) {
-                log::debug!(
-                    "Using a start delay. The first payment for {source} will be at {:?}.",
-                    start
-                );
-            }
-            start
-        } else {
-            let wait = node_generator
-                .next_payment_wait()
-                .map_err(SimulationError::PaymentGenerationError)?;
-            log::debug!("Next payment for {source} in {:?}.", wait);
-            wait
-        };
+        let wait = get_payment_delay(current_count, &source, node_generator.as_ref())?;
 
         select! {
             biased;
@@ -1144,6 +1130,38 @@ async fn produce_events<N: DestinationGenerator + ?Sized, A: PaymentGenerator +
     }
 }
 
+/// Gets the wait time for the next payment. If this is the first payment being generated, and a specific start delay
+/// was set we return a once-off delay. Otherwise, the interval between payments is used.
+fn get_payment_delay<A: PaymentGenerator + ?Sized>(
+    call_count: u64,
+    source: &NodeInfo,
+    node_generator: &A,
+) -> Result<Duration, SimulationError> {
+    // Note: we can't check if let Some() && call_count (syntax not allowed) so we add an additional branch in here.
+    // The alternative is to call payment_start twice (which is _technically_ fine because it always returns the same
+    // value), but this approach only costs us a few extra lines so we go for the more verbose approach so that we
+    // don't have to make any assumptions about the underlying operation of payment_start.
+    if call_count != 0 {
+        let wait = node_generator
+            .next_payment_wait()
+            .map_err(SimulationError::PaymentGenerationError)?;
+        log::debug!("Next payment for {source} in {:?}.", wait);
+        Ok(wait)
+    } else if let Some(start) = node_generator.payment_start() {
+        log::debug!(
+            "First payment for {source} will be after a start delay of {:?}.",
+            start
+        );
+        Ok(start)
+    } else {
+        let wait = node_generator
+            .next_payment_wait()
+            .map_err(SimulationError::PaymentGenerationError)?;
+        log::debug!("First payment for {source} in {:?}.", wait);
+        Ok(wait)
+    }
+}
+
 async fn consume_simulation_results(
     logger: Arc<Mutex<PaymentResultLogger>>,
     mut receiver: Receiver<(Payment, PaymentResult)>,
@@ -1380,7 +1398,10 @@ async fn track_payment_result(
 
 #[cfg(test)]
 mod tests {
-    use crate::MutRng;
+    use crate::{get_payment_delay, test_utils, MutRng, PaymentGenerationError, PaymentGenerator};
+    use mockall::mock;
+    use std::fmt;
+    use std::time::Duration;
 
     #[test]
     fn create_seeded_mut_rng() {
@@ -1407,4 +1428,74 @@ mod tests {
 
         assert_ne!(rng_1.next_u64(), rng_2.next_u64())
     }
+
+    mock! {
+        pub Generator {}
+
+        impl fmt::Display for Generator {
+            fn fmt<'a>(&self, f: &mut fmt::Formatter<'a>) -> fmt::Result;
+        }
+
+        impl PaymentGenerator for Generator {
+            fn payment_start(&self) -> Option<Duration>;
+            fn payment_count(&self) -> Option<u64>;
+            fn next_payment_wait(&self) -> Result<Duration, PaymentGenerationError>;
+            fn payment_amount(&self, destination_capacity: Option<u64>) -> Result<u64, PaymentGenerationError>;
+        }
+    }
+
+    #[test]
+    fn test_no_payment_delay() {
+        let node = test_utils::create_nodes(1, 100_000)
+            .first()
+            .unwrap()
+            .0
+            .clone();
+
+        // Setup mocked generator to have no start time and send payments every 5 seconds.
+        let mut mock_generator = MockGenerator::new();
+        mock_generator.expect_payment_start().return_once(|| None);
+        let payment_interval = Duration::from_secs(5);
+        mock_generator
+            .expect_next_payment_wait()
+            .returning(move || Ok(payment_interval));
+
+        assert_eq!(
+            get_payment_delay(0, &node, &mock_generator).unwrap(),
+            payment_interval
+        );
+        assert_eq!(
+            get_payment_delay(1, &node, &mock_generator).unwrap(),
+            payment_interval
+        );
+    }
+
+    #[test]
+    fn test_payment_delay() {
+        let node = test_utils::create_nodes(1, 100_000)
+            .first()
+            .unwrap()
+            .0
+            .clone();
+
+        // Setup mocked generator to have a start delay and payment interval with different values.
+        let mut mock_generator = MockGenerator::new();
+        let start_delay = Duration::from_secs(10);
+        mock_generator
+            .expect_payment_start()
+            .return_once(move || Some(start_delay));
+        let payment_interval = Duration::from_secs(5);
+        mock_generator
+            .expect_next_payment_wait()
+            .returning(move || Ok(payment_interval));
+
+        assert_eq!(
+            get_payment_delay(0, &node, &mock_generator).unwrap(),
+            start_delay
+        );
+        assert_eq!(
+            get_payment_delay(1, &node, &mock_generator).unwrap(),
+            payment_interval
+        );
+    }
 }

sim-lib/src/random_activity.rs

@@ -214,8 +214,8 @@ fn events_per_month(source_capacity_msat: u64, multiplier: f64, expected_payment
 
 impl PaymentGenerator for RandomPaymentActivity {
     /// Returns the time that the payments should start. This will always be 0 for the RandomPaymentActivity type.
-    fn payment_start(&self) -> Duration {
-        Duration::from_secs(0)
+    fn payment_start(&self) -> Option<Duration> {
+        None
     }
 
     /// Returns the number of payments that should be made. This will always be None for the RandomPaymentActivity type.