From: Matt Corallo Date: Fri, 16 Aug 2024 22:36:30 +0000 (+0000) Subject: Simplify `Instant` mocking in outbound payments X-Git-Tag: v0.0.124-rc1~18^2~3 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=d89a487d5b87f8f8f7308be50d14e29d85f66e6d;p=rust-lightning Simplify `Instant` mocking in outbound payments 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. --- diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 1fda94337..69e97aa35 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -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; - /// Storing minimal payment attempts information required for determining if a outbound payment can /// be retried. -pub(crate) struct PaymentAttemptsUsingTime { +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, - + 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 PaymentAttemptsUsingTime { +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 Display for PaymentAttemptsUsingTime { +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 Display for PaymentAttemptsUsingTime { 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() ); } } diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 291befa2a..60dd82b75 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -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(); diff --git a/lightning/src/util/mod.rs b/lightning/src/util/mod.rs index f19a9b8d8..bb138dd69 100644 --- a/lightning/src/util/mod.rs +++ b/lightning/src/util/mod.rs @@ -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. diff --git a/lightning/src/util/time.rs b/lightning/src/util/time.rs index bedeab1d4..106a4ce4e 100644 --- a/lightning/src/util/time.rs +++ b/lightning/src/util/time.rs @@ -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 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 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 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 = 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 for SinceEpoch { + impl Sub 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); - } }