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

[martinsaposnic]

Closes https://github.com/lightningdevkit/rust-lightning/issues/3471

As discussed here https://github.com/lightningdevkit/rust-lightning/pull/3662#discussion_r2018697826

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.

[ldk-reviews-bot]

👋 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.

[codecov[bot]]

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). Report is 69 commits behind head on main.

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

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

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

[ldk-reviews-bot]

👋 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.

[TheBlueMatt]

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]

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.

[TheBlueMatt]

Note that the TimeProvider trait is already introduced by the LSPS5 work in https://github.com/lightningdevkit/rust-lightning/pull/3662 (where timestamps are part of the message signing) and will here just be reused to fix the missing check in the LSPS2 service.

Wat? Why do all messages need a timestamp? Is there a reason we can't just leave them as 1970-01-01 on the client side for no-std builds with the same rationale as above?

[martinsaposnic]

Wat? Why do all messages need a timestamp? Is there a reason we can't just leave them as 1970-01-01 on the client side for no-std builds with the same rationale as above?

On LSPS5 specifically, we would be leaving no-std builds vulnerable to replay attacks, which I'm not sure about the severity of it, but could be quite annoying

The notification delivery service, once it has received the timestamp, signature, and webhook notification object:

• MUST check that the timestamp is within 10 minutes (before or after) of its local time. ...

Rationale The timestamp and signature checks are intended to avoid replay attacks, where some third party is able to copy the JSON object sent by the LSP and replays it in order to frame the LSP for spamming the client.

[TheBlueMatt]

I don't see why that's a concern. If someone registers a non-TLS webhook or one with weak authentication, that sounds like a them issue, but more generally, the handling of the webhook isn't the concern, it's the act of calling it - waking up the app to do things is expensive, sure doing them might double or triple the cost, but the cost is there either way. More generally, none of the messages look like messages that we'd suffer from if we handled them spuriously/duplicatively (as our action for all of them is likely to be "Connect to the LSP and operate normally the way we generally would").

[tnull]

I don't see why that's a concern. If someone registers a non-TLS webhook or one with weak authentication, that sounds like a them issue, but more generally, the handling of the webhook isn't the concern, it's the act of calling it - waking up the app to do things is expensive, sure doing them might double or triple the cost, but the cost is there either way. More generally, none of the messages look like messages that we'd suffer from if we handled them spuriously/duplicatively (as our action for all of them is likely to be "Connect to the LSP and operate normally the way we generally would").

Hmm, in any case this discussion seems more or less unrelated to this PR: I suggest to move it either to #3662, or, as it really is a spec-level discussion, to https://github.com/lightning/blips/pull/55

[martinsaposnic]

Closing this. Will reopen once #3662 is merged

[TheBlueMatt]

Fair. I understood that the spec was finished (ie it's too late to bother the spec and just let it stand but don't bother on our end). If that's not true I'm happy to engage there but if it's just gonna make it take forever to land I imagine it's not worth it.

[tnull]

Fair. I understood that the spec was finished

Yeah, it was finalized for now, but we're currently the only ones implementing bLIP-55/LSPS5, so we can make additional adjustments at this point.

it's just gonna make it take forever to land I imagine it's not worth it.

Yes, true.