Bug: On iOS LDK panics on reading scorer

[andrei-21]

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:

1. Build and run a node with LDK on iOS
2. Send coins (to have something to store for the scorer)
3. Store the scorer
4. Reboot the device
5. 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 };

1. 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
2. scorer reader reads the moment of the event as a duration since the Unix epoch to duration_since_epoch
3. wall_clock_now is a SystemTime
4. wall_clock_now - duration_since_epoch is the duration since the event (till now)
5. now is Instant
6. if there was a reboot between the moment of the event and now then now will have number of nanosecods since the boot what is shorter than wall_clock_now - duration_since_epoch
7. 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).

[TheBlueMatt]

Grrrr, that leaves no simple "correct" option for us...SystemTime isn't a great fit either cause that can jump around quite a bit. We could store a Duration offset next to our Instants 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 also SystemTime and just deal with discontinuities, but that sucks too. Presumably Instant is also faster on some platforms (eg rtdsc vs a syscall to fetch the real time)?

[andrei-21]

What about using InstantCanGoBackward as Instant + 50 years then it will be safe to subtract durations up to 50 years?

[TheBlueMatt]

Mmm, indeed, that's simpler than the Instant + Duration crap, a constant offset sounds like a good idea.