LSPS5 implementation #3662
LSPS5 implementation #3662
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
This is a huge PR, but it wasn’t obvious to me how to split it in a way that would still make sense (I did split it into small commits to make it easier to review.). I’m open to suggestions if you have ideas on how this could be structured differently. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3662 +/- ##
==========================================
- Coverage 89.77% 89.71% -0.07%
==========================================
Files 164 168 +4
Lines 134446 135940 +1494
Branches 134446 135940 +1494
==========================================
+ Hits 120705 121955 +1250
- Misses 11068 11252 +184
- Partials 2673 2733 +60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Wow, thank you for looking into this! I did a first pass, and it looks pretty amazing already!
Before going too much into further details, here are a few general comments upfront:
-
I'm generally no fan of introducing additional dependencies here, an in particular not
reqwestandtokio. I think following the pattern so farBroadcastNotificationscould be a request that the user handles with any HTTP client they want and then could call back intoLSPS5ServiceHandler. Alternatively, we could also use a trait similar to the currentHTTPClient, but I don't think we want to keep the default implementation. Note that the blockingreqwestvariant wraps atokioruntime internally, and therefore should never (1, 2, ...) be used together. I guess technically we could consider a defaultasyncversion of the trait that usesasyncreqwest, but I would prefer to simply have well-documented trait on our end that the user can implement however they choose to. Also note that stackingtokioruntimes is heavily discouraged in general, so assuming our users would themselves use atokioruntime, we shouldn't wrap one inLSPS5ServiceHandler. -
Note that
lightning-liquidityis optionally no-stdcompliant, so please don't rely onstdwherever possible, often it's just a matter of usingcoreinstead and importing the respective types fromcrate::prelude. If you really find yourselves needing to usestd, make sure it's feature gated behindfeature = "std"and we provide an alternative for users that don't support it. -
Minor: Regarding formatting we're using tabs, not spaces. Feel free to run
./contrib/run-rustfmt.shafter each commit to run our formatting scripts. -
This PR in its current scope is great, just want to note that eventually we need to add persistence for the state. As we haven't fully fleshed out the persistence strategy for
lightning-liquidityin general yet, it's actually preferred to defer this to a follow-up, but just wanted to mention it. Also note that some changes toMessageQueue/EventQueuewill happen inlightning-liquidity: IntroduceEventQueuenotifier and wake BP for message processing #3509, but will ping you to rebase once that has been merged. (just sidenotes here).
I hope these initial points make sense, let me know if you have any questions, or once you made corresponding changes and think this is ready for the next round of review!
|
👋 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. |
Thanks for asking! I'm totally fine to keep this (with its current scope) in a single PR, as long as we keep the commit history pretty clean to allow continuing review to happen commit-by-commit. To this end, please make sure add any fixup commits clearly marked (e.g. via a |
|
Btw, I'm not sure if you're familiar with the previous attempt of implementing LSPS5: #3499 Given this is a clean slate, not sure how much there is to learn, but still might be worth a look. Also not sure if @johncantrell97 would be interested in reviewing this PR, too, as he's familiar with the codebase and LSPS5. |
martinsaposnic
force-pushed
the
lsps5
branch
2 times, most recently
from
1d4b47c to
edf5346
Compare
|
@tnull, ready for the next review round!
CI is failing because of the usage of the |
martinsaposnic
force-pushed
the
lsps5
branch
7 times, most recently
from
9809682 to
af5929e
Compare
|
🔔 1st Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @tnull @valentinewallace! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
1 similar comment
|
🔔 10th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
There was a problem hiding this comment.
Still seems very much to me like we should consider landing the client (which afaiu we want to keep stateless?) and message framing while we discuss the service format more #3662 (comment)
martinsaposnic
force-pushed
the
lsps5
branch
2 times, most recently
from
044989e to
fd8ce8f
Compare
|
Lint is happy now. As discussed, now the service does not emit events when it responds to requests. I'm also working in parallel on DOS protections. Should I push it to this PR or open a new PR? Also, is coverage not working properly? e.g. it's saying that lsps5/service is only 6% covered, but it's in fact close to 95%, not sure what's going on there. Will investigate |
There was a problem hiding this comment.
Fixups look good to me, would be ready for squashing I think.
As discussed, now the service does not emit events when it responds to requests. I'm also working in parallel on DOS protections. Should I push it to this PR or open a new PR?
Thanks! I think my preference would be to land this and do further modifications in quick follow-ups. Not sure if @TheBlueMatt would be fine with that, too.
Also, is coverage not working properly? e.g. it's saying that lsps5/service is only 6% covered, but it's in fact close to 95%, not sure what's going on there. Will investigate
Hmm, not sure, let us know what you find.
|
|
||
| #[cfg(feature = "_test_utils")] | ||
| /// Advance the time by a specified number of seconds. | ||
| fn advance_time(&self, _seconds: u64); |
There was a problem hiding this comment.
Hmm, not sure this would work, as any TimeProvider would now need to offer these methods, even non-test providers. Can't these just be a method on the actual mock object, not part of the trait?
There was a problem hiding this comment.
Agreed, I dropped the commit
- Add 'time' feature flag to allow disabling time-dependent functionality - Include 'time' in default features - Allow users to disable SystemTime::now without disabling all std features - Follows pattern established in other crates (e.g., lightning-transaction-sync) - Improves compatibility with WASM environments
- do new_from_duration_since_epoch (instead of From<Duration>) - Avoid doing ambiguous timestamp types - Add abs_diff function to use on client / service
Adds a new url_utils.rs module that provides: - A lightweight URL parser specialized for LSPS5 webhook validation - An implementation focusing on scheme and host extraction - RFC-compliant scheme validation - Tests for various URL scenarios This implementation allows validating webhook URLs without depending on the external url crate
- Define LSPS5Request and LSPS5Response enums for webhook registration, listing, and removal. - Implement WebhookNotification and associated helper constructors for different notification types. - Implement serialization/deserialization support with comprehensive tests. - Improve LSPS5 message types, validation, and testing - Replace generic String types with strongly-typed Lsps5AppName and Lsps5WebhookUrl with built-in length and format validation - Restructure imports to follow one-per-line convention - Add constants for notification method strings - Make WebhookNotificationMethod enum more consistent with LSPS5 prefix - Use explicit serde_json::json and serde_json::Value instead of imports - Improve code documentation with proper ticks and references - Add comprehensive test vectors from the BLIP-0055 specification
- Introduce LSPS5ServiceEvent for LSPS-side webhook events including registration, listing, removal, and notification. - Define LSPS5ClientEvent for handling webhook outcomes on the client (Lightning node) side. - Outline WebhookNotificationParams enum to support notification-specific parameters. - Improve LSPS5 event documentation and field naming - Rename client/lsp fields to counterparty_node_id for consistent terminology - Replace generic String types with more specific Lsps5AppName and Lsps5WebhookUrl - Add comprehensive documentation for all events and fields - Include format specifications (UTF-8, ISO8601) and size constraints - Add request_id field to all relevant events for consistent request tracking - Provide detailed descriptions of error codes and their meanings - Use complete sentences in documentation comments
Implements the LSPS5 webhook registration service that allows LSPs to notify clients of important events via webhooks. This service handles webhook registration, listing, removal, and notification delivery according to the LSPS5 specification. Some details: - A generic HttpClient trait is defined so users can provide their own HTTP implementation - A generic TimeProvider trait is defined with a DefaultTimeProvider that uses std functionality - Uses URL utils to validate webhook URLs according to LSPS5 requirements - Uses secure message signing logic from the lightning::util::message_signing module - Works with the events and messages defined in earlier commits - Tests will be provided in a future commit
Implements the client-side functionality for LSPS5 webhook registration, allowing Lightning clients to register, list, and remove webhooks with LSPs. This client handler processes responses and verifies webhook notification signatures. Key features: - Full client API for webhook registration operations - Per-peer state tracking for pending requests - Automatic request timeout and cleanup - Security validation for webhook URLs - Notification signature verification - Add store_signature and check_signature to prevent replay attacks - Some tests are provided but more will come in a future commit This implementation pairs with the service-side LSPS5 webhook handler to complete the webhook registration protocol according to the LSPS5 specification.
Fully integrates the LSPS5 webhook components into the lightning-liquidity framework, enabling usage through the LiquidityManager. It includes - Registering LSPS5 events in the event system - Adding LSPS5 module to the main library exports - Updating LSPS0 serialization to handle LSPS5 messages - Adding LSPS5 configuration options to client and service config structures - Implementing message handling for LSPS5 requests and responses - Adding accessor methods for LSPS5 client and service handlers - LiquidityManager::new_with_custom_time_provider is created so it can be passed to the service and client handlers ::new With this change, LSPS5 webhook functionality can now be accessed through the standard LiquidityManager interface, following the same pattern as other LSPS protocols.
I think it was just a one-time slip up. I ran it three more times and it came back fine. |
|
Fixups squashed. After all the rebasing rounds, some stuff ended up in the wrong commits, so I did a cleanup rebase and put things back where they belong. |
There was a problem hiding this comment.
Most of the way through but figured I'd post comments now.
Instead of having AI write your docs and commit messages (boy it loves lists, especially when there's only like one relevant thing and it can shove in two irrelevant notes in the list), please think about what is actually important for a reader vs not. We have a lot of methods and a lot of docs that people have to read. Having more details than is required is not a great outcome, as much as is missing important details.
| .filter(|s| !s.is_empty()) | ||
| .ok_or_else(|| (LSPS5ProtocolError::UrlParse))?; | ||
|
|
||
| if host_without_auth.is_empty() || host_without_auth.contains(' ') { |
There was a problem hiding this comment.
Not quite sure what we're trying to accomplish here, but if we want to be strict about the hostname, there's a lot more to reject than just spaces, we should reject anything that isn't a number, letter, . or - (plus : for the port).
| /// This struct provides parsing and access to these core parts of a URL string. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct LSPSUrl { | ||
| host: UntrustedString, |
There was a problem hiding this comment.
It doesn't look like the host is use anywhere, is there a reason we shouldn't just not store it?
|
|
||
| Ok(LSPSUrl { | ||
| host: UntrustedString(host_str.to_string()), | ||
| url: UntrustedString(url_str.to_string()), |
There was a problem hiding this comment.
Its already a string :)
| pub struct LSPSUrl { | ||
| host: UntrustedString, | ||
| /// The full URL string. | ||
| url: UntrustedString, |
There was a problem hiding this comment.
In general we usually don't bother using UntrustedStrings in internal code - we use it when we're exposing strings that could contain control chars or shell escapes or the like in public APIs, but the indirection internally doesn't seem worth it?
Also, here specifically, if we're just more strict about the charset (alphanumeric, -, ., /, :, and %) I think it'd be fine anyway.
|
|
||
| /// App name for LSPS5 webhooks. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct LSPS5AppName(UntrustedString); |
There was a problem hiding this comment.
nit: rather than storing a heap String, should we just store a ([u8; 64], u8) pair for chars + length?
| /// | ||
| /// This identifies which webhook registration should be notified. | ||
| /// | ||
| /// **Note**: The [`app_name`] must have been previously registered via [`lsps5.set_webhook`]. |
There was a problem hiding this comment.
maybe "will have been"? This is an event we're giving to the user, we shouldn't be asking them to verify this, but rather telling them that it was verified, but maybe just drop this note entirely? Similarly for the other fields.
| /// When this event occurs, the client should: | ||
| /// 1. Present an appropriate error message to the user | ||
| /// 2. Consider retry strategies based on the specific error | ||
| /// 3. If the error is due to reaching webhook limits, prompt the user to remove |
There was a problem hiding this comment.
Ha, I cannot imagine a wallet app that allows users to manage webhooks by hand :p. In general, the "the client should" listings all seem a little verbose (though useful!), maybe just condense them assuming a normal bitcoin wallet that keeps one or two webhooks registered at all times for only themselves?
| /// Constructs a `LSPS5ServiceHandler`. | ||
| pub(crate) fn new( | ||
| event_queue: Arc<EventQueue>, pending_messages: Arc<MessageQueue>, channel_manager: CM, | ||
| config: LSPS5ServiceConfig, time_provider: TP, |
There was a problem hiding this comment.
nit: lets rename this new_with_time_provider and have a separate impl block (gated on "time") that has a new method that just uses DefaultTimeProvider (directly, by doing a impl Deref for DefaultTimeProvider { type Target = Self; ... })
| let now = | ||
| LSPSDateTime::new_from_duration_since_epoch(self.time_provider.duration_since_epoch()); | ||
|
|
||
| let no_change = client_webhooks |
There was a problem hiding this comment.
We do a lookup of params.app_name in client_webhooks something like three times, we should entry at the top :)
| "Service handler received LSPS5 response message. This should never happen." | ||
| ); | ||
| let err = format!( | ||
| "Service handler received LSPS5 response message from node {:?}. |
A complete implementation for LSPS5 (spec defined here lightning/blips#55)
Reviewing commit by commit is recommended (~40% of the added lines are tests)
Notes:
- Will rebase fromDONElightning-liquidity: IntroduceEventQueuenotifier and wake BP for message processing #3509 once that PR is merged