LSPS2: Add TimeProvider trait for expiry checks in no-std and std builds #3746

martinsaposnic · 2025-04-20T16:56:38Z

This PR introduces a TimeProvider trait to enable expiry checks for OpeningFeeParams on LSPS2 in both std and no-std environments. Previously, expiry validation was skipped in no-std builds due to the lack of a system clock.

With this change:

A TimeProvider trait is defined, allowing users to supply their own time source.
A DefaultTimeProvider implementation is provided for std environments.
All expiry checks (is_past, is_expired_opening_fee_params, etc.) now require a TimeProvider instance.
The LSPS2 service, utility functions, and tests are updated to use the new trait.
The API is extended to allow injecting a custom time provider, ensuring expiry checks are always performed, even in no-std builds.

This abstraction will be even more useful for LSPS5, which relies on time-based logic much more extensively than LSPS2.

- Add 'time' feature flag to allow disabling time-dependent functionality. - Include 'time' in default features - It will allow users to disable SystemTime::now without disabling all std features

- Provides a single method duration_since_epoch() -> Duration - Also a DefaultTimeProvider implementation is provided that uses std::time

ldk-reviews-bot · 2025-04-20T16:56:41Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

- is_past() on LSPSDateTime is changed so it receives a time_provider to get current time. - is_expired_opening_fee_params() now receives a time_provider to pass to is_past() - is_expired_opening_fee_params now does not depend on std being enabled. - LSPS2 service now passes around the time_provider to be able to use wherever necessary. - LSPS2 service now provides two new() methods: 1. a default new() that uses DefaultTimeProvider 2. a new_with_custom_time_provider() that expects a custom time_provider Devices with no access to std will need to use the latter

codecov · 2025-04-20T17:09:07Z

Codecov Report

Attention: Patch coverage is 77.77778% with 16 lines in your changes missing coverage. Please review.

Project coverage is 89.08%. Comparing base (c6921fa) to head (0f17d59).

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps2/service.rs	70.96%	9 Missing ⚠️
lightning-liquidity/src/manager.rs	53.33%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3746      +/-   ##
==========================================
- Coverage   89.15%   89.08%   -0.08%     
==========================================
  Files         156      156              
  Lines      123837   123875      +38     
  Branches   123837   123875      +38     
==========================================
- Hits       110408   110348      -60     
- Misses      10754    10835      +81     
- Partials     2675     2692      +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

I probably still miss some context, but kept thinking why not just inject the dependency always and let the user instantiate either the std time provider or their own.

joostjager · 2025-04-21T06:52:47Z

lightning-liquidity/src/lsps0/ser.rs

+/// This trait is used to provide the current time service operations and to convert between timestamps and durations.
+pub trait TimeProvider {
+	/// Get the current time as a duration since the Unix epoch.
+	fn duration_since_epoch(&self) -> Duration;


I know this name is sort of standard in LDK, but to me something like unix_time would have been better. Duration is already the type, and epoch doesn't specify which epoch.

I disagree, exactly because you'd expect unix_time to be a SystemTime or u64, but not a Duration. Plus, homogeneity is an important API feature, so keeping this in-line with the rest of internal APIs is a big plus, IMO.

No need to change it now. As you say, keeping it consistent is a good API feature. For me as a developer though, this isn't the most descriptive name.

joostjager · 2025-04-21T06:57:40Z

lightning-liquidity/src/manager.rs

+					#[cfg(feature = "time")]
+					{
+						// Use default new if time feature is enabled and no custom provider
+						LSPS2ServiceHandler::new(


Can't TimeProvider be simply required here (no option), and then instantiate or panic higher up in the call stack? Also no need for two new variants then.

joostjager · 2025-04-21T07:00:07Z

lightning-liquidity/src/lsps0/ser.rs

+
+/// Default time provider using the system clock.
+#[derive(Clone, Debug)]
+#[cfg(feature = "time")]


I re-read the PR description, but was still thinking why isn't this just std gated?

Sorry for not mentioning this in the PR description (I’ll update it shortly). This change comes from a discussion in this PR, specifically this comment from tnull:

Actually, now that we went this way, let's not feature gate this behind std, but rather a new time feature. Context is the following: on WASM, you generally have access to std, but not entirely. For example, time-dependent behaviors like SystemTime::now would just panic at runtime. However, when previously feature-gating on std, we good feedback from some WASM users that would like to keep using the non-time std features.
TLDR: would be nice to allow users to disable the SystemTime::now default without disabling the entirety of std. So let's introduce a time feature, as we did in other crates (cf. lightning-transaction-sync).

@tnull tagging Elias for review and input.

But if those users run with std and just don't inject the panic'ing default std time provider and instead supply their own, aren't they good with the time flag?

But if those users run with std and just don't inject the panic'ing default std time provider and instead supply their own, aren't they good with the time flag?

Yes, but we still shouldn't expose a 'default' API that would panic if used? That said, given that it requires std, we should probably feature-gate this on all(feature = "std", feature = "time"), or have time enable std.

Yes, but we still shouldn't expose a 'default' API that would panic if used?

If some platforms have an incomplete implementation of the std feature, is it the responsibility of this library to protect against hitting the missing part via a feature flag?

I also wouldn't make it a 'default' API, but just require a time provider to be supplied. Overall it seems to me that a time flag may be unnecessary.

joostjager · 2025-04-21T07:01:53Z

lightning-liquidity/Cargo.toml

@@ -14,8 +14,9 @@ categories = ["cryptography::cryptocurrencies"]
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

 [features]
-default = ["std"]
+default = ["std", "time"]


Disabling SystemTIme::now without disabling all of std: couldn't this be realized by just passing in another time provider than the std one, without requiring this flag?

ldk-reviews-bot · 2025-04-21T07:07:04Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

tnull

Thanks for looking into this, though I'm not convinced moving forward with this (and reviewing it in parallel) makes sense until #3662 lands. At least if you don't want to rebase this PR on the entirety of #3662, which could get very unwieldy given the number of commit over there.

tnull · 2025-04-22T17:42:05Z

lightning-liquidity/src/lsps0/ser.rs

+/// This trait is used to provide the current time service operations and to convert between timestamps and durations.
+pub trait TimeProvider {
+	/// Get the current time as a duration since the Unix epoch.
+	fn duration_since_epoch(&self) -> Duration;


I disagree, exactly because you'd expect unix_time to be a SystemTime or u64, but not a Duration. Plus, homogeneity is an important API feature, so keeping this in-line with the rest of internal APIs is a big plus, IMO.

tnull · 2025-04-22T17:45:20Z

lightning-liquidity/src/lsps0/ser.rs

+
+/// Default time provider using the system clock.
+#[derive(Clone, Debug)]
+#[cfg(feature = "time")]


But if those users run with std and just don't inject the panic'ing default std time provider and instead supply their own, aren't they good with the time flag?

Yes, but we still shouldn't expose a 'default' API that would panic if used? That said, given that it requires std, we should probably feature-gate this on all(feature = "std", feature = "time"), or have time enable std.

tnull · 2025-04-22T17:47:08Z

lightning-liquidity/src/lsps0/ser.rs

+/// Trait defining a time provider
+///
+/// This trait is used to provide the current time service operations and to convert between timestamps and durations.
+pub trait TimeProvider {


I'm a bit confused why this is re-added here, as we're already adding it (in a different place) in #3662. Maybe it would be good to put this PR in draft until #3662 has been merged, or rebase it on top of #3662, as changes there will need to be consequentially accounted for here?

This PR is extracting some of the code from LSPS5 PR. My idea was to get this PR merged and then rebase LSPS5 on top of it, that way we would lose some commits from the LSPS5 PR and make it a bit smaller (same idea with the solution to this issue #3459 (PR coming soon), where I'm extracting some of the code written in the LSPS5 PR and putting it in a new PR, that way making the LSPS5 PR smaller)

TheBlueMatt · 2025-04-29T15:20:21Z

I do kinda wonder if this is worth it? What is the practical user impact of not checking for expiry? Presumably the LSP will simply fail the attempt to use the past offers and the client will simply request new offers, assuming it has past offers, but currently we don't persist offers (and probably shouldn't?) so in practice we'll basically always be requesting fresh offers anyway.

tnull · 2025-04-29T15:53:20Z

I do kinda wonder if this is worth it? What is the practical user impact of not checking for expiry? Presumably the LSP will simply fail the attempt to use the past offers and the client will simply request new offers, assuming it has past offers, but currently we don't persist offers (and probably shouldn't?) so in practice we'll basically always be requesting fresh offers anyway.

Note that the TimeProvider trait is already introduced by the LSPS5 work in #3662 (where timestamps are part of the message signing) and will here just be reused to fix the missing check in the LSPS2 service.

martinsaposnic added 2 commits April 20, 2025 13:35

Separate time functionality into dedicated feature flag.

8c51013

- Add 'time' feature flag to allow disabling time-dependent functionality. - Include 'time' in default features - It will allow users to disable SystemTime::now without disabling all std features

Create a new trait TimeProvider

cca6fa0

- Provides a single method duration_since_epoch() -> Duration - Also a DefaultTimeProvider implementation is provided that uses std::time

martinsaposnic added 2 commits April 20, 2025 13:59

Lightning liquidity manager now may receive an Option<TimeProvider>

0f17d59

martinsaposnic force-pushed the no-std-time-lsps2 branch from 49c8b02 to 0f17d59 Compare April 20, 2025 16:59

ldk-reviews-bot requested a review from joostjager April 20, 2025 17:07

joostjager reviewed Apr 21, 2025

View reviewed changes

tnull self-requested a review April 21, 2025 16:59

tnull reviewed Apr 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LSPS2: Add TimeProvider trait for expiry checks in no-std and std builds #3746

LSPS2: Add TimeProvider trait for expiry checks in no-std and std builds #3746

martinsaposnic commented Apr 20, 2025

ldk-reviews-bot commented Apr 20, 2025 •

edited

Loading

codecov bot commented Apr 20, 2025

joostjager left a comment

joostjager Apr 21, 2025

tnull Apr 22, 2025

joostjager Apr 23, 2025

joostjager Apr 21, 2025

joostjager Apr 21, 2025

martinsaposnic Apr 21, 2025

joostjager Apr 21, 2025 •

edited

Loading

tnull Apr 22, 2025

joostjager Apr 23, 2025

joostjager Apr 21, 2025

ldk-reviews-bot commented Apr 21, 2025

tnull left a comment

tnull Apr 22, 2025

tnull Apr 22, 2025

tnull Apr 22, 2025

martinsaposnic Apr 22, 2025

TheBlueMatt commented Apr 29, 2025

tnull commented Apr 29, 2025

LSPS2: Add TimeProvider trait for expiry checks in no-std and std builds #3746

Are you sure you want to change the base?

LSPS2: Add TimeProvider trait for expiry checks in no-std and std builds #3746

Conversation

martinsaposnic commented Apr 20, 2025

ldk-reviews-bot commented Apr 20, 2025 • edited Loading

codecov bot commented Apr 20, 2025

Codecov Report

joostjager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Apr 21, 2025

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 29, 2025

tnull commented Apr 29, 2025

ldk-reviews-bot commented Apr 20, 2025 •

edited

Loading

joostjager Apr 21, 2025 •

edited

Loading