lightning-liquidity: Refactor LSPS1 service-side
#4282
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
4 times, most recently
from
773316a to
fb519ab
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4282 +/- ##
==========================================
+ Coverage 86.01% 86.21% +0.19%
==========================================
Files 156 158 +2
Lines 102857 104430 +1573
Branches 102857 104430 +1573
==========================================
+ Hits 88474 90034 +1560
+ Misses 11876 11807 -69
- Partials 2507 2589 +82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
2 times, most recently
from
b96685b to
c6eb6b3
Compare
We add the first LSPS1 integration test. This is based on the unfinished work in lightningdevkit#3864, but rebased to account for the new ways we now do integration test setup.
.. for which we got warnings
We previously considered tracking payment confirmations as part of the handler. However, we can considerably simplify our logic if we stick with the current approach of having the LSPs track the payment status and update us when prompted through events.
Now that we don't do on-chain tracking in LSPS1, we can drop quite a few `LiquidityManager` parameters and generics, which were only added in anticipation of tracking on-chain state. Signed-off-by: Elias Rohrer <[email protected]>
We move the `PeerState` related types to a new module. In the following commits we'll bit-by-bit drop the `pub(super)`s introduced here, asserting better separation of state and logic going forward.
.. we will re-add a proper state machine in a later commit, but for now we can just drop all of this half-baked logic that doesn't actually do anything.
.. requiring less access to internals
Previously, we'd directly access the internal `outbound_` map of `PeerState`. Here we refactor the code to avoid this. Note this also highlighted a bug in that we currently don't actually update/persist the order state in `update_order_state`. We don't fix this here, but just improve isolation for now, as all state update behavior will be reworked later.
We introduce two new methods on `PeerState` to avoid direct access to the internal `pending_requests` map.
The `OutboundChannel` construct simply wrapped `ChannelOrder` which we can now simply use directly.
We here remember and update the order state and channel details in `ChannelOrder`
Since we by now have the `TimeProvider` trait, we might as well use it in `LSPS1ServiceHandler` instead of requiring the user to provide a `created_at` manually. Signed-off-by: Elias Rohrer <[email protected]>
In the future we might want to inline the fields in `LSPS1ServiceConfig` (especially once some are added that we'd want to always/never set for the user), but for now we just make the `supported_options` field in `LSPS1ServiceConfig` required, avoiding some dangerous `unwrap`s.
Previously, we'd use an event to have the user check the order status and then call back in. As we already track the order status, we here change that to a model where we respond immediately based on our state and have the user/LSP update that state whenever it detects a change (e.g., a received payment, reorg, etc.). In the next commmit we will add/modify the corresponding API methods to do so.
We add the serializations for all types that will be persisted as part of the `PeerState`.
We follow the model already employed in LSPS2/LSPS5 and implement state pruning and persistence for `LSPS1ServiceHandler` state. Signed-off-by: Elias Rohrer <[email protected]>
.. we read the persisted state in `LiquidityManager::new` Signed-off-by: Elias Rohrer <[email protected]>
|
Should be good for review. |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
| && payment_details.onchain.is_some() | ||
| { | ||
| // bLIP-51: 'LSP MUST disable on-chain payments if the client omits this field.' | ||
| let err = "Onchain payments must be disabled if no refund_onchain_address is set.".to_string(); |
There was a problem hiding this comment.
This requirement doesn't appear to be documented in LSPS1ServiceEvent::RequestForPaymentDetails or LSPS1PaymentInfo, but I'm not sure we should bother erroring here vs just removing the unsupported option before sending the order to the peer?
There was a problem hiding this comment.
Ah, good point. Turns out we didn't expose refund_onchain_address anywhere. I now added a fixup documenting the requirement on both the method and the event, added a refund_onchain_address field to the event and a onchain_payment_required method allowing the LSP to reject a request for this reason.
I think auto-stripping the onchain payment variant isn't great as we might end up without any payment variant, and would need to auto-reject the request then. Might be preferable to leave that up to the LSP in general?
| ) -> ChannelOrder { | ||
| let state = ChannelOrderState::new(payment_details); | ||
| let channel_order = ChannelOrder { order_params, state, created_at }; | ||
| self.outbound_channels_by_order_id.insert(order_id, channel_order.clone()); |
There was a problem hiding this comment.
might want some kind of size limit here to limit dos, tho we could also just do it at the start of the flow.
There was a problem hiding this comment.
Yeah, I now added a commit following the approach we took for LSPS2, i.e., adding MAX_TOTAL_PEERS, MAX_REQUESTS_PER_PEER and MAX_TOTAL_PENDING_REQUESTS limits.
| let msg = LSPS1Message::Response(request_id.clone(), response).into(); | ||
| message_queue_notifier.enqueue(counterparty_node_id, msg); | ||
| let err = format!("Failed to handle request due to: {}", e); | ||
| let action = ErrorAction::IgnoreAndLog(Level::Error); |
There was a problem hiding this comment.
Here and likely elsewhere, we really can't log at Error just because someone sends us a bogus message.
There was a problem hiding this comment.
Hmm, note that we already silently fail if we can't parse the message at all. This basically follows what we do elsewhere in the codebase. I think we should do #3492 as a follow-up to make sure we follow the same approach everywhere. I'll tag that 0.3
There was a problem hiding this comment.
Hmm, okay. I'm not a fan of making things worse than it is only to fix it later, but...
| #[cfg(not(feature = "time"))] | ||
| { | ||
| // TODO: We need to find a way to check expiry times in no-std builds. | ||
| all_payment_details_expired = false; |
There was a problem hiding this comment.
Probably can just insta-remove things that are failed, at least.
There was a problem hiding this comment.
I don't think so, as we still want to show the Failed state back to the user querying their order status.
| self.state, | ||
| ChannelOrderState::ExpectingPayment { .. } | ||
| | ChannelOrderState::FailedAndRefunded { .. } | ||
| ); |
There was a problem hiding this comment.
Do we ever want to prune things that were completed? A two year old channel lease that expired 18 months ago probably isn't interesting?
There was a problem hiding this comment.
Yes, probably, and we have the same issue with LSPS2. I think it would be good to add a consistent API that works for both in a follow up. Could be to leave it to the user to manually call a prune method, or possibly set an auto-prune config flag in the respective service configs? Anyway, I'd prefer to leave that as a follow-up.
|
👋 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
force-pushed
the
2025-11-lsps1-refactor
branch
from
922b0fa to
8a0d33d
Compare
.. as there's no need to do so.
We add a method that allows the LSP to signal to the client the token they used was invalid. We use the `102` error code as proposed in lightning/blips#68.
We test the just-added API. Co-authored by Claude AI
This refactors `ChannelOrder` to use an internal state machine enum `ChannelOrderState` that: - Encapsulates state-specific data in variants (e.g., `channel_info` only available in `CompletedAndChannelOpened`) - Provides type-safe state transitions - Replaces the generic `update_order_status` API with specific transition methods: `order_payment_received`, `order_channel_opened`, and `order_failed_and_refunded` The state machine has four states: - `ExpectingPayment`: Initial state, awaiting payment - `OrderPaid`: Payment received, awaiting channel open - `CompletedAndChannelOpened`: Terminal state with channel info - `FailedAndRefunded`: Terminal state for failed/refunded orders Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <[email protected]>
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
from
8a0d33d to
0b4d9dd
Compare
Add two new integration tests to cover the new public API methods: - `lsps1_order_state_transitions`: Tests the full flow of `order_payment_received` followed by `order_channel_opened`, verifying that payment states are updated correctly and channel info is returned after the channel is opened. - `lsps1_order_failed_and_refunded`: Tests the `order_failed_and_refunded` method, verifying that payment states are set to Refunded. Co-Authored-By: HAL 9000
Add `lsps1_expired_orders_are_pruned_and_not_persisted` test that verifies: - Orders with expired payment details (expires_at in the past) are accessible before persist() is called - After persist() is called, expired orders in ExpectingPayment state are pruned and no longer accessible - Pruned orders are not recovered after restart, confirming that the pruning also removes the persisted state Co-Authored-By: HAL 9000
The bLIP-51 specification defines a `HOLD` intermediate payment state: - `EXPECT_PAYMENT` -> `HOLD` -> `PAID` (success path) - `EXPECT_PAYMENT` -> `REFUNDED` (failure before payment) - `HOLD` -> `REFUNDED` (failure after payment received) This commit adds the `Hold` variant to `LSPS1PaymentState` and updates the state machine transitions: - `payment_received()` now sets payment state to `Hold` (not `Paid`) - `channel_opened()` transitions payment state from `Hold` to `Paid` - Tests updated to verify the correct state at each transition This allows LSPs to properly communicate when a payment has been received but the channel has not yet been opened (e.g., Lightning HTLC held, or on-chain tx detected but channel funding not published). Co-Authored-By: HAL 9000
Turns out this was another variant we didn't actually use anywhere. So we're dropping it.
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
from
0b4d9dd to
3ae7578
Compare
We previously had no way to reject requests in case the LSP requires onchain payment while the client not providing `refund_onchain_address`. Here we add a method allowing to do so.
Add per-peer and global rate limiting to `LSPS1ServiceHandler` to prevent resource exhaustion, mirroring the existing LSPS2 pattern. Introduce `MAX_PENDING_REQUESTS_PER_PEER` (10), `MAX_TOTAL_PENDING_REQUESTS` (1000), and `MAX_TOTAL_PEERS` (100000) constants and enforce them in `handle_create_order_request`. Rejected requests receive a `CreateOrderError` with `LSPS0_CLIENT_REJECTED_ERROR_CODE`. A `total_pending_requests` atomic counter tracks the global count, and a `verify_pending_request_counter` debug assertion ensures it stays in sync. Co-Authored-By: HAL 9000
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
from
3ae7578 to
4489325
Compare
Signed-off-by: Elias Rohrer <[email protected]>
4 participants
Closes #3480.
We 'refactor' (rewrite) the
LSPS1ServiceHandler, move state handling to a dedicatedPeerState, add an STM pattern, add persistence for the service state, add some more critical API paths, add test coverage, and finally remove thecfg(lsps1_service)flag.