Use `MonotonicTime` as `Instant` shifted by 10 years forward #2322

andrei-21 · 2023-05-27T16:43:03Z

Such implementation allows MonotonicTime to go backward up to 10 years on all platforms. On some platforms (e.g. iOS) Instant is represented as u64 of nanoseconds since the boot of the system. Obviously such implementation does not allow to go backward before the time of the boot.
Fixes #2311.

codecov-commenter · 2023-05-27T17:51:07Z

Codecov Report

Patch coverage: 81.25% and no project coverage change.

Comparison is base (bd12067) 90.32% compared to head (189b070) 90.33%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2322   +/-   ##
=======================================
  Coverage   90.32%   90.33%           
=======================================
  Files         106      106           
  Lines       54951    54965   +14     
  Branches    54951    54965   +14     
=======================================
+ Hits        49633    49651   +18     
+ Misses       5318     5314    -4

Impacted Files	Coverage Δ
lightning/src/ln/outbound_payment.rs	`89.47% <0.00%> (ø)`
lightning/src/routing/scoring.rs	`94.09% <ø> (ø)`
lightning/src/util/time.rs	`89.85% <86.66%> (+4.40%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt · 2023-05-30T23:19:46Z

Awesome, thanks! ISTM we should simply drop the Time impl for Instant and always use MonotonicTime, no?

andrei-21 · 2023-06-02T16:08:06Z

Awesome, thanks! ISTM we should simply drop the Time impl for Instant and always use MonotonicTime, no?

Makes sense. Now Instant is only used in tests.

jkczyz · 2023-06-02T20:35:58Z

lightning/src/util/time.rs

+	}
+
+	fn elapsed(&self) -> Duration {
+		self.0.elapsed()


Won't this result in an underflow? Instant::elapsed uses Instant::now() - *self.

Also, note that this method is used in Writeable for ChannelLiquidity<T>.

Won't this result in an underflow? Instant::elapsed uses Instant::now() - *self.

Ah, actually that's not a problem since you are adding -- not subtracting -- 10 years. My mistake.

I think you were right. I changed the implementation and added tests (the tests would fail without the fix).

lol, yeah, I confused myself. The subtraction occurs in Instant::elapsed, but the test that I wrote wasn't catching the failure.

lightning/src/util/time.rs

lightning/src/routing/scoring.rs

jkczyz · 2023-06-27T21:13:45Z

Such implementation allows MonotonicTime to go backward up to 10 years on all platforms. On some platforms (e.g. iOS) Instant is represented as u64 of nanoseconds since the boot of the system. Obviously such implementation does not allow to go backward before the time of the boot. Fixes #2311.

I don't think this fix pertains to the system time not being monotonic. That should already be fixed in the Readable implementation for ChannelLiquidity. Instead, this fixes a problem where a platform can't represent an Instance sometime prior to the system boot. I'll push a commit adding docs to say as much.

jkczyz · 2023-06-27T21:23:38Z

Such implementation allows MonotonicTime to go backward up to 10 years on all platforms. On some platforms (e.g. iOS) Instant is represented as u64 of nanoseconds since the boot of the system. Obviously such implementation does not allow to go backward before the time of the boot. Fixes #2311.

I don't think this fix pertains to the system time not being monotonic. That should already be fixed in the Readable implementation for ChannelLiquidity. Instead, this fixes a problem where a platform can't represent an Instance sometime prior to the system boot. I'll push a commit adding docs to say as much.

Ah, you can disregard this argument. I see what you were saying, but I'll push the doc change regardless.

jkczyz

Please go ahead and squash the fixup and the forthcoming test rename. Otherwise, seems good to go.

jkczyz · 2023-06-28T19:33:33Z

lightning/src/util/time.rs

@@ -180,7 +180,10 @@ pub mod tests {
 	#[cfg(not(feature = "no-std"))]
 	fn monotonic_time_go_backward() {


Could you name this monotonic_time_subtracts?

jkczyz · 2023-06-28T19:36:40Z

lightning/src/util/time.rs

+	}
+
+	fn elapsed(&self) -> Duration {
+		self.0.elapsed()


lol, yeah, I confused myself. The subtraction occurs in Instant::elapsed, but the test that I wrote wasn't catching the failure.

Such implementation allows `MonotonicTime` to go backward up to 10 years on all platforms. On some platforms (e.g. iOS) `Instant` is represented as `u64` of nanoseconds since the boot of the system. Obviously such implementation does not allow to go backward before the time of the boot. Co-authored-by: Andrei <[email protected]> Co-authored-by: Jeffrey Czyz <[email protected]>

lightning/src/util/time.rs

andrei-21 force-pushed the fix/instant-overflow branch 2 times, most recently from a9f5498 to 3a54cff Compare May 28, 2023 20:13

TheBlueMatt added this to the 0.0.116 milestone May 30, 2023

andrei-21 force-pushed the fix/instant-overflow branch from 3a54cff to a4cb4db Compare June 2, 2023 14:23

andrei-21 marked this pull request as ready for review June 2, 2023 16:07

wpaulino previously approved these changes Jun 2, 2023

View reviewed changes

jkczyz reviewed Jun 2, 2023

View reviewed changes

jkczyz dismissed wpaulino’s stale review via 4507d6f June 27, 2023 21:27

jkczyz force-pushed the fix/instant-overflow branch from a4cb4db to 4507d6f Compare June 27, 2023 21:27

jkczyz reviewed Jun 28, 2023

View reviewed changes

andrei-21 force-pushed the fix/instant-overflow branch from b138ea2 to 47966b8 Compare June 29, 2023 09:19

jkczyz previously approved these changes Jun 29, 2023

View reviewed changes

TheBlueMatt reviewed Jun 29, 2023

View reviewed changes

lightning/src/util/time.rs Outdated Show resolved Hide resolved

lightning/src/util/time.rs Outdated Show resolved Hide resolved

andrei-21 dismissed jkczyz’s stale review via 189b070 June 29, 2023 15:20

andrei-21 force-pushed the fix/instant-overflow branch from 47966b8 to 189b070 Compare June 29, 2023 15:20

TheBlueMatt approved these changes Jun 29, 2023

View reviewed changes

jkczyz approved these changes Jun 29, 2023

View reviewed changes

TheBlueMatt merged commit ab80fcf into lightningdevkit:main Jun 29, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `MonotonicTime` as `Instant` shifted by 10 years forward #2322

Use `MonotonicTime` as `Instant` shifted by 10 years forward #2322

andrei-21 commented May 27, 2023

codecov-commenter commented May 27, 2023 •

edited

Loading

TheBlueMatt commented May 30, 2023

andrei-21 commented Jun 2, 2023

jkczyz Jun 2, 2023

jkczyz Jun 27, 2023

andrei-21 Jun 28, 2023

jkczyz Jun 28, 2023

jkczyz commented Jun 27, 2023

jkczyz commented Jun 27, 2023

jkczyz left a comment

jkczyz Jun 28, 2023

andrei-21 Jun 29, 2023

jkczyz Jun 28, 2023

		@@ -180,7 +180,10 @@ pub mod tests {
		#[cfg(not(feature = "no-std"))]
		fn monotonic_time_go_backward() {

Use MonotonicTime as Instant shifted by 10 years forward #2322

Use MonotonicTime as Instant shifted by 10 years forward #2322

Conversation

andrei-21 commented May 27, 2023

codecov-commenter commented May 27, 2023 • edited Loading

Codecov Report

TheBlueMatt commented May 30, 2023

andrei-21 commented Jun 2, 2023

jkczyz Jun 2, 2023

Choose a reason for hiding this comment

jkczyz Jun 27, 2023

Choose a reason for hiding this comment

andrei-21 Jun 28, 2023

Choose a reason for hiding this comment

jkczyz Jun 28, 2023

Choose a reason for hiding this comment

jkczyz commented Jun 27, 2023

jkczyz commented Jun 27, 2023

jkczyz left a comment

Choose a reason for hiding this comment

jkczyz Jun 28, 2023

Choose a reason for hiding this comment

andrei-21 Jun 29, 2023

Choose a reason for hiding this comment

jkczyz Jun 28, 2023

Choose a reason for hiding this comment

Use `MonotonicTime` as `Instant` shifted by 10 years forward #2322

Use `MonotonicTime` as `Instant` shifted by 10 years forward #2322

codecov-commenter commented May 27, 2023 •

edited

Loading