Repository navigation
Bug: On iOS LDK panics on reading scorer #2311
Description
Current Behavior:
On iOS LDK panics on reading scorer with:
thread '<unnamed>' panicked at 'overflow when subtracting duration from instant', library/std/src/time.rs:424:33
Expected Behavior:
LDK does not panic, but reads the scorer.
Steps To Reproduce:
- Build and run a node with LDK on iOS
- Send coins (to have something to store for the scorer)
- Store the scorer
- Reboot the device
- Read the scorer file
Environment
Device: iPhone8 and github MacOS builder.
What Happens
Usually std::time::Instant uses duration since the device boot time.
Seems that iOS uses u64 to store nanosecods, what means it cannot represent moments before the boot.
It breaks code at https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/routing/scoring.rs#L1727
let mut duration_since_epoch = Duration::from_secs(0);
// ...
(4, duration_since_epoch, required),
// ...
let wall_clock_now = T::duration_since_epoch();
let now = T::now();
let last_updated = if wall_clock_now > duration_since_epoch {
now - (wall_clock_now - duration_since_epoch)
// ^----- panics here
} else { now };- scorer writer writes the moment of the event as a duration since the Unix epoch
let duration_since_epoch = T::duration_since_epoch() - self.last_updated.elapsed - scorer reader reads the moment of the event as a duration since the Unix epoch to
duration_since_epoch wall_clock_nowis aSystemTimewall_clock_now - duration_since_epochis the duration since the event (till now)nowisInstant- if there was a reboot between the moment of the event and now then
nowwill have number of nanosecods since the boot what is shorter thanwall_clock_now - duration_since_epoch now - (wall_clock_now - duration_since_epoch)panics
Simpler Test
#[test]
fn check_instant() {
use std::time::{Duration, Instant};
let now = Instant::now();
let duration = Duration::from_secs(1);
std::thread::sleep(duration);
let after = Instant::now();
// Learn how Instant is represented.
eprintln!("moment {now:?} after {duration:?} is {after:?}");
// One of the statement will likely fail.
now.checked_sub(Duration::from_millis(1)).unwrap();
now.checked_sub(Duration::from_millis(10)).unwrap();
now.checked_sub(Duration::from_millis(100)).unwrap();
now.checked_sub(Duration::from_secs(1)).unwrap();
now.checked_sub(Duration::from_secs(10)).unwrap();
now.checked_sub(Duration::from_secs(60)).unwrap();
now.checked_sub(Duration::from_secs(10 * 60)).unwrap();
now.checked_sub(Duration::from_secs(30 * 60)).unwrap();
now.checked_sub(Duration::from_secs(60 * 60)).unwrap();
now.checked_sub(Duration::from_secs(2 * 60 * 60)).unwrap();
now.checked_sub(Duration::from_secs(4 * 60 * 60)).unwrap();
now.checked_sub(Duration::from_secs(8 * 60 * 60)).unwrap();
now.checked_sub(Duration::from_secs(12 * 60 * 60)).unwrap();
now.checked_sub(Duration::from_secs(24 * 60 * 60)).unwrap();
now.checked_sub(Duration::from_secs(10 * 24 * 60 * 60)).unwrap();
now.checked_sub(Duration::from_secs(100 * 24 * 60 * 60)).unwrap();
}Conclusion
Instant can be compared (or subtracted) only with other instances of Instant (such that they stay within the duration of the program runtime). It is not supposed to be stored or interpreted in any other way.
From https://doc.rust-lang.org/std/time/struct.Instant.html:
Instants are opaque types that can only be compared to one another. There is no method to get “the number of seconds” from an instant. Instead, it only allows measuring the duration between two instants (or comparing two instants).
Activity
TheBlueMatt commentedon May 21, 2023
Grrrr, that leaves no simple "correct" option for us...
SystemTimeisn't a great fit either cause that can jump around quite a bit. We could store aDurationoffset next to ourInstants and always add that, but that's really annoying to deal with, not to mention increases the scorer's size and the complexity required per score fetch. We could alsoSystemTimeand just deal with discontinuities, but that sucks too. PresumablyInstantis also faster on some platforms (eg rtdsc vs a syscall to fetch the real time)?andrei-21 commentedon May 21, 2023
What about using
InstantCanGoBackwardasInstant + 50 yearsthen it will be safe to subtract durations up to 50 years?TheBlueMatt commentedon May 21, 2023
Mmm, indeed, that's simpler than the
Instant + Durationcrap, a constant offset sounds like a good idea.