Skip to content

Commit d89a487

Browse files
committed
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.
1 parent 1c83e61 commit d89a487

File tree

4 files changed

+32
-126
lines changed

4 files changed

+32
-126
lines changed

lightning/src/ln/outbound_payment.rs

Lines changed: 11 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,8 @@ use crate::routing::router::{BlindedTail, InFlightHtlcs, Path, PaymentParameters
2626
use crate::sign::{EntropySource, NodeSigner, Recipient};
2727
use crate::util::errors::APIError;
2828
use crate::util::logger::Logger;
29-
use crate::util::time::Time;
30-
#[cfg(all(feature = "std", test))]
31-
use crate::util::time::tests::SinceEpoch;
29+
#[cfg(feature = "std")]
30+
use crate::util::time::Instant;
3231
use crate::util::ser::ReadableArgs;
3332

3433
use core::fmt::{self, Display, Formatter};
@@ -319,12 +318,9 @@ impl Retry {
319318
(Retry::Attempts(max_retry_count), PaymentAttempts { count, .. }) => {
320319
max_retry_count > count
321320
},
322-
#[cfg(all(feature = "std", not(test)))]
323-
(Retry::Timeout(max_duration), PaymentAttempts { first_attempted_at, .. }) =>
324-
*max_duration >= crate::util::time::MonotonicTime::now().duration_since(*first_attempted_at),
325-
#[cfg(all(feature = "std", test))]
321+
#[cfg(feature = "std")]
326322
(Retry::Timeout(max_duration), PaymentAttempts { first_attempted_at, .. }) =>
327-
*max_duration >= SinceEpoch::now().duration_since(*first_attempted_at),
323+
*max_duration >= Instant::now().duration_since(*first_attempted_at),
328324
}
329325
}
330326
}
@@ -339,42 +335,28 @@ pub(super) fn has_expired(route_params: &RouteParameters) -> bool {
339335
false
340336
}
341337

342-
pub(crate) type PaymentAttempts = PaymentAttemptsUsingTime<ConfiguredTime>;
343-
344338
/// Storing minimal payment attempts information required for determining if a outbound payment can
345339
/// be retried.
346-
pub(crate) struct PaymentAttemptsUsingTime<T: Time> {
340+
pub(crate) struct PaymentAttempts {
347341
/// This count will be incremented only after the result of the attempt is known. When it's 0,
348342
/// it means the result of the first attempt is not known yet.
349343
pub(crate) count: u32,
350344
/// This field is only used when retry is `Retry::Timeout` which is only build with feature std
351345
#[cfg(feature = "std")]
352-
first_attempted_at: T,
353-
#[cfg(not(feature = "std"))]
354-
phantom: core::marker::PhantomData<T>,
355-
346+
first_attempted_at: Instant,
356347
}
357348

358-
#[cfg(not(feature = "std"))]
359-
type ConfiguredTime = crate::util::time::Eternity;
360-
#[cfg(all(feature = "std", not(test)))]
361-
type ConfiguredTime = crate::util::time::MonotonicTime;
362-
#[cfg(all(feature = "std", test))]
363-
type ConfiguredTime = SinceEpoch;
364-
365-
impl<T: Time> PaymentAttemptsUsingTime<T> {
349+
impl PaymentAttempts {
366350
pub(crate) fn new() -> Self {
367-
PaymentAttemptsUsingTime {
351+
PaymentAttempts {
368352
count: 0,
369353
#[cfg(feature = "std")]
370-
first_attempted_at: T::now(),
371-
#[cfg(not(feature = "std"))]
372-
phantom: core::marker::PhantomData,
354+
first_attempted_at: Instant::now(),
373355
}
374356
}
375357
}
376358

377-
impl<T: Time> Display for PaymentAttemptsUsingTime<T> {
359+
impl Display for PaymentAttempts {
378360
fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> {
379361
#[cfg(not(feature = "std"))]
380362
return write!(f, "attempts: {}", self.count);
@@ -383,7 +365,7 @@ impl<T: Time> Display for PaymentAttemptsUsingTime<T> {
383365
f,
384366
"attempts: {}, duration: {}s",
385367
self.count,
386-
T::now().duration_since(self.first_attempted_at).as_secs()
368+
Instant::now().duration_since(self.first_attempted_at).as_secs()
387369
);
388370
}
389371
}

lightning/src/ln/payment_tests.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ use crate::routing::gossip::NodeId;
4646

4747
#[cfg(feature = "std")]
4848
use {
49-
crate::util::time::tests::SinceEpoch,
49+
crate::util::time::Instant as TestTime,
5050
std::time::{SystemTime, Instant, Duration},
5151
};
5252

@@ -2340,7 +2340,7 @@ fn do_automatic_retries(test: AutoRetry) {
23402340
pass_failed_attempt_with_retry_along_path!(channel_id_2, true);
23412341

23422342
// Advance the time so the second attempt fails due to timeout.
2343-
SinceEpoch::advance(Duration::from_secs(61));
2343+
TestTime::advance(Duration::from_secs(61));
23442344

23452345
// Make sure we don't retry again.
23462346
nodes[0].node.process_pending_htlc_forwards();

lightning/src/util/mod.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,11 @@ pub(crate) mod atomic_counter;
3131
pub(crate) mod async_poll;
3232
pub(crate) mod byte_utils;
3333
pub(crate) mod transaction_utils;
34-
pub(crate) mod time;
3534
pub mod hash_tables;
3635

36+
#[cfg(feature = "std")]
37+
pub(crate) mod time;
38+
3739
pub mod indexed_map;
3840

3941
/// Logging macro utilities.

lightning/src/util/time.rs

Lines changed: 16 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -4,113 +4,43 @@
44
// You may not use this file except in accordance with one or both of these
55
// licenses.
66

7-
//! [`Time`] trait and different implementations. Currently, it's mainly used in tests so we can
8-
//! manually advance time.
9-
//! Other crates may symlink this file to use it while [`Time`] trait is sealed here.
10-
11-
use core::ops::Sub;
12-
use core::time::Duration;
13-
14-
/// A measurement of time.
15-
pub trait Time: Copy + Sub<Duration, Output = Self> where Self: Sized {
16-
/// Returns an instance corresponding to the current moment.
17-
fn now() -> Self;
18-
19-
/// Returns the amount of time passed between `earlier` and `self`.
20-
fn duration_since(&self, earlier: Self) -> Duration;
21-
}
22-
23-
/// A state in which time has no meaning.
24-
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
25-
pub struct Eternity;
26-
27-
impl Time for Eternity {
28-
fn now() -> Self {
29-
Self
30-
}
31-
32-
fn duration_since(&self, _earlier: Self) -> Duration {
33-
Duration::from_secs(0)
34-
}
35-
}
36-
37-
impl Sub<Duration> for Eternity {
38-
type Output = Self;
39-
40-
fn sub(self, _other: Duration) -> Self {
41-
self
42-
}
43-
}
44-
45-
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
46-
#[cfg(feature = "std")]
47-
pub struct MonotonicTime(std::time::Instant);
48-
49-
/// The amount of time to shift `Instant` forward to prevent overflow when subtracting a `Duration`
50-
/// from `Instant::now` on some operating systems (e.g., iOS representing `Instance` as `u64`).
51-
#[cfg(feature = "std")]
52-
const SHIFT: Duration = Duration::from_secs(10 * 365 * 24 * 60 * 60); // 10 years.
53-
54-
#[cfg(feature = "std")]
55-
impl Time for MonotonicTime {
56-
fn now() -> Self {
57-
let instant = std::time::Instant::now().checked_add(SHIFT).expect("Overflow on MonotonicTime instantiation");
58-
Self(instant)
59-
}
60-
61-
fn duration_since(&self, earlier: Self) -> Duration {
62-
// On rust prior to 1.60 `Instant::duration_since` will panic if time goes backwards.
63-
// However, we support rust versions prior to 1.60 and some users appear to have "monotonic
64-
// clocks" that go backwards in practice (likely relatively ancient kernels/etc). Thus, we
65-
// manually check for time going backwards here and return a duration of zero in that case.
66-
let now = Self::now();
67-
if now.0 > earlier.0 { now.0 - earlier.0 } else { Duration::from_secs(0) }
68-
}
69-
}
70-
71-
#[cfg(feature = "std")]
72-
impl Sub<Duration> for MonotonicTime {
73-
type Output = Self;
74-
75-
fn sub(self, other: Duration) -> Self {
76-
let instant = self.0.checked_sub(other).expect("MonotonicTime is not supposed to go backward futher than 10 years");
77-
Self(instant)
78-
}
79-
}
7+
//! A simple module which either re-exports [`std::time::Instant`] or a mocked version of it for
8+
//! tests.
809
8110
#[cfg(test)]
82-
pub mod tests {
83-
use super::{Time, Eternity};
11+
pub use test::Instant;
12+
#[cfg(not(test))]
13+
pub use std::time::Instant;
8414

15+
#[cfg(test)]
16+
mod test {
8517
use core::time::Duration;
8618
use core::ops::Sub;
8719
use core::cell::Cell;
8820

8921
/// Time that can be advanced manually in tests.
9022
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
91-
pub struct SinceEpoch(Duration);
23+
pub struct Instant(Duration);
9224

93-
impl SinceEpoch {
25+
impl Instant {
9426
thread_local! {
9527
static ELAPSED: Cell<Duration> = core::cell::Cell::new(Duration::from_secs(0));
9628
}
9729

9830
pub fn advance(duration: Duration) {
9931
Self::ELAPSED.with(|elapsed| elapsed.set(elapsed.get() + duration))
10032
}
101-
}
10233

103-
impl Time for SinceEpoch {
104-
fn now() -> Self {
34+
pub fn now() -> Self {
10535
Self(Self::ELAPSED.with(|elapsed| elapsed.get()))
10636
}
10737

108-
fn duration_since(&self, earlier: Self) -> Duration {
38+
pub fn duration_since(&self, earlier: Self) -> Duration {
10939
self.0 - earlier.0
11040
}
11141
}
11242

113-
impl Sub<Duration> for SinceEpoch {
43+
impl Sub<Duration> for Instant {
11444
type Output = Self;
11545

11646
fn sub(self, other: Duration) -> Self {
@@ -120,21 +50,13 @@ pub mod tests {
12050

12151
#[test]
12252
fn time_passes_when_advanced() {
123-
let now = SinceEpoch::now();
53+
let now = Instant::now();
12454

125-
SinceEpoch::advance(Duration::from_secs(1));
126-
SinceEpoch::advance(Duration::from_secs(1));
55+
Instant::advance(Duration::from_secs(1));
56+
Instant::advance(Duration::from_secs(1));
12757

128-
let later = SinceEpoch::now();
58+
let later = Instant::now();
12959

13060
assert_eq!(now.0 + Duration::from_secs(2), later.0);
13161
}
132-
133-
#[test]
134-
fn time_never_passes_in_an_eternity() {
135-
let now = Eternity::now();
136-
let later = Eternity::now();
137-
138-
assert_eq!(later, now);
139-
}
14062
}

0 commit comments

Comments
 (0)