]> git.bitcoin.ninja Git - rust-lightning/commitdiff
Simplify `Instant` mocking in outbound payments
authorMatt Corallo <git@bluematt.me>
Fri, 16 Aug 2024 22:36:30 +0000 (22:36 +0000)
committerMatt Corallo <git@bluematt.me>
Wed, 21 Aug 2024 14:11:01 +0000 (14:11 +0000)
To handle `std` and `no-std` concepts of time in scoring, we'd
originally written a generic `Time` trait which we could use to
fetch the current time, observe real (not wall-clock) elapsed time,
and serialize the time.

Eventually, scoring stopped using this `Time` trait but outbound
payment retry expiry started using it instead to mock time in
tests.

Since scoring no longer uses the full features which required the
`Time` trait, we can substantially simplify by just having the
mocking option.

lightning/src/ln/outbound_payment.rs
lightning/src/ln/payment_tests.rs
lightning/src/util/mod.rs
lightning/src/util/time.rs

index 1fda943373d7f2ae49dd03e5c2e6938dae2e5176..69e97aa35730e84cd3efac831a3bd22fad48c3d3 100644 (file)
@@ -26,9 +26,8 @@ use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, PaymentParameters
 use crate::sign::{EntropySource, NodeSigner, Recipient};
 use crate::util::errors::APIError;
 use crate::util::logger::Logger;
-use crate::util::time::Time;
-#[cfg(all(feature = "std", test))]
-use crate::util::time::tests::SinceEpoch;
+#[cfg(feature = "std")]
+use crate::util::time::Instant;
 use crate::util::ser::ReadableArgs;
 
 use core::fmt::{self, Display, Formatter};
@@ -319,12 +318,9 @@ impl Retry {
                        (Retry::Attempts(max_retry_count), PaymentAttempts { count, .. }) => {
                                max_retry_count > count
                        },
-                       #[cfg(all(feature = "std", not(test)))]
-                       (Retry::Timeout(max_duration), PaymentAttempts { first_attempted_at, .. }) =>
-                               *max_duration >= crate::util::time::MonotonicTime::now().duration_since(*first_attempted_at),
-                       #[cfg(all(feature = "std", test))]
+                       #[cfg(feature = "std")]
                        (Retry::Timeout(max_duration), PaymentAttempts { first_attempted_at, .. }) =>
-                               *max_duration >= SinceEpoch::now().duration_since(*first_attempted_at),
+                               *max_duration >= Instant::now().duration_since(*first_attempted_at),
                }
        }
 }
@@ -339,42 +335,28 @@ pub(super) fn has_expired(route_params: &RouteParameters) -> bool {
        false
 }
 
-pub(crate) type PaymentAttempts = PaymentAttemptsUsingTime<ConfiguredTime>;
-
 /// Storing minimal payment attempts information required for determining if a outbound payment can
 /// be retried.
-pub(crate) struct PaymentAttemptsUsingTime<T: Time> {
+pub(crate) struct PaymentAttempts {
        /// This count will be incremented only after the result of the attempt is known. When it's 0,
        /// it means the result of the first attempt is not known yet.
        pub(crate) count: u32,
        /// This field is only used when retry is `Retry::Timeout` which is only build with feature std
        #[cfg(feature = "std")]
-       first_attempted_at: T,
-       #[cfg(not(feature = "std"))]
-       phantom: core::marker::PhantomData<T>,
-
+       first_attempted_at: Instant,
 }
 
-#[cfg(not(feature = "std"))]
-type ConfiguredTime = crate::util::time::Eternity;
-#[cfg(all(feature = "std", not(test)))]
-type ConfiguredTime = crate::util::time::MonotonicTime;
-#[cfg(all(feature = "std", test))]
-type ConfiguredTime = SinceEpoch;
-
-impl<T: Time> PaymentAttemptsUsingTime<T> {
+impl PaymentAttempts {
        pub(crate) fn new() -> Self {
-               PaymentAttemptsUsingTime {
+               PaymentAttempts {
                        count: 0,
                        #[cfg(feature = "std")]
-                       first_attempted_at: T::now(),
-                       #[cfg(not(feature = "std"))]
-                       phantom: core::marker::PhantomData,
+                       first_attempted_at: Instant::now(),
                }
        }
 }
 
-impl<T: Time> Display for PaymentAttemptsUsingTime<T> {
+impl Display for PaymentAttempts {
        fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> {
                #[cfg(not(feature = "std"))]
                return write!(f, "attempts: {}", self.count);
@@ -383,7 +365,7 @@ impl<T: Time> Display for PaymentAttemptsUsingTime<T> {
                        f,
                        "attempts: {}, duration: {}s",
                        self.count,
-                       T::now().duration_since(self.first_attempted_at).as_secs()
+                       Instant::now().duration_since(self.first_attempted_at).as_secs()
                );
        }
 }
index 291befa2abf618d3f4f321e5067fcbead0d60fbf..60dd82b75b0751f3dc3ec551405b8ae0d3a5f957 100644 (file)
@@ -46,7 +46,7 @@ use crate::routing::gossip::NodeId;
 
 #[cfg(feature = "std")]
 use {
-       crate::util::time::tests::SinceEpoch,
+       crate::util::time::Instant as TestTime,
        std::time::{SystemTime, Instant, Duration},
 };
 
@@ -2340,7 +2340,7 @@ fn do_automatic_retries(test: AutoRetry) {
                        pass_failed_attempt_with_retry_along_path!(channel_id_2, true);
 
                        // Advance the time so the second attempt fails due to timeout.
-                       SinceEpoch::advance(Duration::from_secs(61));
+                       TestTime::advance(Duration::from_secs(61));
 
                        // Make sure we don't retry again.
                        nodes[0].node.process_pending_htlc_forwards();
index f19a9b8d8aa2d3b85b636ec1821abef04a4ad2f1..bb138dd69d1c1d2400c7c37ed61204ca350fe5c8 100644 (file)
@@ -31,9 +31,11 @@ pub(crate) mod atomic_counter;
 pub(crate) mod async_poll;
 pub(crate) mod byte_utils;
 pub(crate) mod transaction_utils;
-pub(crate) mod time;
 pub mod hash_tables;
 
+#[cfg(feature = "std")]
+pub(crate) mod time;
+
 pub mod indexed_map;
 
 /// Logging macro utilities.
index bedeab1d4890ac78c0dbda3f2506e8a28ee9f07d..106a4ce4e172bf896d47387ebf168d5836cfe3d5 100644 (file)
@@ -4,93 +4,25 @@
 // You may not use this file except in accordance with one or both of these
 // licenses.
 
-//! [`Time`] trait and different implementations. Currently, it's mainly used in tests so we can
-//! manually advance time.
-//! Other crates may symlink this file to use it while [`Time`] trait is sealed here.
-
-use core::ops::Sub;
-use core::time::Duration;
-
-/// A measurement of time.
-pub trait Time: Copy + Sub<Duration, Output = Self> where Self: Sized {
-       /// Returns an instance corresponding to the current moment.
-       fn now() -> Self;
-
-       /// Returns the amount of time passed between `earlier` and `self`.
-       fn duration_since(&self, earlier: Self) -> Duration;
-}
-
-/// A state in which time has no meaning.
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
-pub struct Eternity;
-
-impl Time for Eternity {
-       fn now() -> Self {
-               Self
-       }
-
-       fn duration_since(&self, _earlier: Self) -> Duration {
-               Duration::from_secs(0)
-       }
-}
-
-impl Sub<Duration> for Eternity {
-       type Output = Self;
-
-       fn sub(self, _other: Duration) -> Self {
-               self
-       }
-}
-
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
-#[cfg(feature = "std")]
-pub struct MonotonicTime(std::time::Instant);
-
-/// The amount of time to shift `Instant` forward to prevent overflow when subtracting a `Duration`
-/// from `Instant::now` on some operating systems (e.g., iOS representing `Instance` as `u64`).
-#[cfg(feature = "std")]
-const SHIFT: Duration = Duration::from_secs(10 * 365 * 24 * 60 * 60); // 10 years.
-
-#[cfg(feature = "std")]
-impl Time for MonotonicTime {
-       fn now() -> Self {
-               let instant = std::time::Instant::now().checked_add(SHIFT).expect("Overflow on MonotonicTime instantiation");
-               Self(instant)
-       }
-
-       fn duration_since(&self, earlier: Self) -> Duration {
-               // On rust prior to 1.60 `Instant::duration_since` will panic if time goes backwards.
-               // However, we support rust versions prior to 1.60 and some users appear to have "monotonic
-               // clocks" that go backwards in practice (likely relatively ancient kernels/etc). Thus, we
-               // manually check for time going backwards here and return a duration of zero in that case.
-               let now = Self::now();
-               if now.0 > earlier.0 { now.0 - earlier.0 } else { Duration::from_secs(0) }
-       }
-}
-
-#[cfg(feature = "std")]
-impl Sub<Duration> for MonotonicTime {
-       type Output = Self;
-
-       fn sub(self, other: Duration) -> Self {
-               let instant = self.0.checked_sub(other).expect("MonotonicTime is not supposed to go backward futher than 10 years");
-               Self(instant)
-       }
-}
+//! A simple module which either re-exports [`std::time::Instant`] or a mocked version of it for
+//! tests.
 
 #[cfg(test)]
-pub mod tests {
-       use super::{Time, Eternity};
+pub use test::Instant;
+#[cfg(not(test))]
+pub use std::time::Instant;
 
+#[cfg(test)]
+mod test {
        use core::time::Duration;
        use core::ops::Sub;
        use core::cell::Cell;
 
        /// Time that can be advanced manually in tests.
        #[derive(Clone, Copy, Debug, PartialEq, Eq)]
-       pub struct SinceEpoch(Duration);
+       pub struct Instant(Duration);
 
-       impl SinceEpoch {
+       impl Instant {
                thread_local! {
                        static ELAPSED: Cell<Duration> = core::cell::Cell::new(Duration::from_secs(0));
                }
@@ -98,19 +30,17 @@ pub mod tests {
                pub fn advance(duration: Duration) {
                        Self::ELAPSED.with(|elapsed| elapsed.set(elapsed.get() + duration))
                }
-       }
 
-       impl Time for SinceEpoch {
-               fn now() -> Self {
+               pub fn now() -> Self {
                        Self(Self::ELAPSED.with(|elapsed| elapsed.get()))
                }
 
-               fn duration_since(&self, earlier: Self) -> Duration {
+               pub fn duration_since(&self, earlier: Self) -> Duration {
                        self.0 - earlier.0
                }
        }
 
-       impl Sub<Duration> for SinceEpoch {
+       impl Sub<Duration> for Instant {
                type Output = Self;
 
                fn sub(self, other: Duration) -> Self {
@@ -120,21 +50,13 @@ pub mod tests {
 
        #[test]
        fn time_passes_when_advanced() {
-               let now = SinceEpoch::now();
+               let now = Instant::now();
 
-               SinceEpoch::advance(Duration::from_secs(1));
-               SinceEpoch::advance(Duration::from_secs(1));
+               Instant::advance(Duration::from_secs(1));
+               Instant::advance(Duration::from_secs(1));
 
-               let later = SinceEpoch::now();
+               let later = Instant::now();
 
                assert_eq!(now.0 + Duration::from_secs(2), later.0);
        }
-
-       #[test]
-       fn time_never_passes_in_an_eternity() {
-               let now = Eternity::now();
-               let later = Eternity::now();
-
-               assert_eq!(later, now);
-       }
 }