`lightning-liquidity`: Refactor LSPS1 service-side by tnull · Pull Request #4282 · lightningdevkit/rust-lightning

tnull · 2025-12-12T15:28:18Z

Closes #3480.

We 'refactor' (rewrite) the LSPS1ServiceHandler, move state handling to a dedicated PeerState, add an STM pattern, add persistence for the service state, add some more critical API paths, add test coverage, and finally remove the cfg(lsps1_service) flag.

ldk-reviews-bot · 2025-12-12T15:28:21Z

👋 I see @TheBlueMatt was un-assigned.
If you'd like another reviewer assignment, please click here.

codecov · 2025-12-12T17:54:26Z

Codecov Report

❌ Patch coverage is 71.76113% with 279 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.11%. Comparing base (14e522f) to head (ac4c5f4).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps1/service.rs	59.73%	168 Missing and 16 partials ⚠️
lightning-liquidity/src/lsps1/peer_state.rs	84.92%	31 Missing and 32 partials ⚠️
lightning/src/util/ser.rs	38.09%	10 Missing and 3 partials ⚠️
lightning-liquidity/src/persist.rs	65.51%	6 Missing and 4 partials ⚠️
lightning-liquidity/src/manager.rs	80.95%	2 Missing and 6 partials ⚠️
lightning-liquidity/src/lsps1/msgs.rs	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4282      +/-   ##
==========================================
+ Coverage   85.94%   86.11%   +0.17%     
==========================================
  Files         159      161       +2     
  Lines      104644   105717    +1073     
  Branches   104644   105717    +1073     
==========================================
+ Hits        89934    91043    +1109     
+ Misses      12204    12091     -113     
- Partials     2506     2583      +77

Flag	Coverage Δ
tests	`86.11% <71.76%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

tnull · 2026-02-04T14:53:31Z

Rebased to resolve conflicts.

tnull · 2026-02-05T12:59:05Z

Should be good for review.

ldk-reviews-bot · 2026-02-07T12:59:33Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-02-09T13:00:09Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning-liquidity/src/lsps1/peer_state.rs

lightning-liquidity/src/lsps1/service.rs

TheBlueMatt · 2026-02-10T19:19:56Z

lightning-liquidity/src/lsps1/service.rs

+							&& 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();


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?

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?

TheBlueMatt · 2026-02-10T19:23:54Z

lightning-liquidity/src/lsps1/peer_state.rs

+	) -> 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());


might want some kind of size limit here to limit dos, tho we could also just do it at the start of the flow.

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.

lightning-liquidity/src/lsps1/service.rs

TheBlueMatt · 2026-02-10T19:26:49Z

lightning-liquidity/src/lsps1/service.rs

+					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);


Here and likely elsewhere, we really can't log at Error just because someone sends us a bogus message.

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

Hmm, okay. I'm not a fan of making things worse than it is only to fix it later, but...

lightning-liquidity/src/lsps1/service.rs

TheBlueMatt · 2026-02-10T19:46:49Z

lightning-liquidity/src/lsps1/peer_state.rs

+		#[cfg(not(feature = "time"))]
+		{
+			// TODO: We need to find a way to check expiry times in no-std builds.
+			all_payment_details_expired = false;


Probably can just insta-remove things that are failed, at least.

I don't think so, as we still want to show the Failed state back to the user querying their order status.

TheBlueMatt · 2026-02-10T19:49:39Z

lightning-liquidity/src/lsps1/peer_state.rs

+			self.state,
+			ChannelOrderState::ExpectingPayment { .. }
+				| ChannelOrderState::FailedAndRefunded { .. }
+		);


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?

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.

lightning-liquidity/src/lsps1/event.rs

ldk-reviews-bot · 2026-02-10T19:52:43Z

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

ldk-reviews-bot · 2026-02-14T00:01:02Z

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2026-02-16T00:02:15Z

🔔 2nd Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning-liquidity/src/lsps1/event.rs

lightning-liquidity/src/lsps1/peer_state.rs

lightning-liquidity/src/lsps1/service.rs

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 <dev@tnull.de>

.. we read the persisted state in `LiquidityManager::new` Signed-off-by: Elias Rohrer <dev@tnull.de>

Co-authored by Claude AI

As per spec, we check that the user provides at least one payment detail *and* that they don't provide onchain payment details if `refund_onchain_address` is unset.

.. 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 <dev@tnull.de>

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.

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 · 2026-03-02T12:22:59Z

Addressed/responded to pending comments.

TheBlueMatt · 2026-03-06T22:01:49Z

I believe I responded to the two open threads, so just waiting on a response there and then we can move forward.

Co-Authored-By: HAL 9000

Signed-off-by: Elias Rohrer <dev@tnull.de>

tnull marked this pull request as draft December 12, 2025 15:28

tnull force-pushed the 2025-11-lsps1-refactor branch 4 times, most recently from 773316a to fb519ab Compare December 12, 2025 16:08

tnull force-pushed the 2025-11-lsps1-refactor branch 2 times, most recently from b96685b to c6eb6b3 Compare December 15, 2025 12:27

tnull self-assigned this Dec 18, 2025

tnull added this to Weekly Goals Dec 18, 2025

tnull moved this to Goal: Merge in Weekly Goals Dec 18, 2025

tnull added this to the 0.3 milestone Jan 29, 2026

tnull force-pushed the 2025-11-lsps1-refactor branch from c6eb6b3 to a2aa7c3 Compare February 4, 2026 14:53

tnull marked this pull request as ready for review February 5, 2026 12:58

tnull requested a review from TheBlueMatt February 5, 2026 12:58

TheBlueMatt reviewed Feb 10, 2026

View reviewed changes

tnull force-pushed the 2025-11-lsps1-refactor branch 4 times, most recently from 3ae7578 to 4489325 Compare February 11, 2026 12:27

tnull requested a review from TheBlueMatt February 11, 2026 12:50

TheBlueMatt reviewed Feb 17, 2026

View reviewed changes

tnull added 19 commits March 2, 2026 13:01

Add serialization logic for LSPS1 PeerState types

4344547

We add the serializations for all types that will be persisted as part of the `PeerState`.

Implement LSPS1ServiceHandler persistence and state pruning

183bea3

We follow the model already employed in LSPS2/LSPS5 and implement state pruning and persistence for `LSPS1ServiceHandler` state. Signed-off-by: Elias Rohrer <dev@tnull.de>

Read persisted LSPS1ServiceHandler state on startup

43c4eed

.. we read the persisted state in `LiquidityManager::new` Signed-off-by: Elias Rohrer <dev@tnull.de>

Add test case asserting LSPS1ServiceState is persisted across restarts

cd4e828

Co-authored by Claude AI

Add some checks on provided payment details

bc7dc7c

As per spec, we check that the user provides at least one payment detail *and* that they don't provide onchain payment details if `refund_onchain_address` is unset.

Don't hold write lock in LSPS{1,2}ServiceHandler::peer_disconnected

91bfbd5

.. as there's no need to do so.

Add invalid_token_provided API method

2a0a901

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.

Add test case for invalid_token_provided flow

a94bd45

We test the just-added API. Co-authored by Claude AI

Drop lsps1_service cfg flag

c56d584

Fix clippy lints

d42a0f4

Drop unused LSPS1OnchainPayment type

c8b7740

Drop unused LSPS1ServiceEvent::Refund event

0ce5709

Turns out this was another variant we didn't actually use anywhere. So we're dropping it.

Add onchain_payment_required method

4deb09c

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.

tnull force-pushed the 2025-11-lsps1-refactor branch from e31b5c8 to ac4c5f4 Compare March 2, 2026 12:21

tnull requested a review from TheBlueMatt March 2, 2026 12:23

TheBlueMatt removed their request for review March 3, 2026 15:52

tnull requested a review from TheBlueMatt March 4, 2026 14:01

TheBlueMatt removed their request for review March 6, 2026 22:01

tnull added 3 commits March 12, 2026 23:39

f Remove total_pending_requests tracking from LSPS1 service

c78efdc

Co-Authored-By: HAL 9000

f Move num-pending-reqs. checking to PeerState::register_request

1f00f53

Reject clients if request registration failed (e.g., duplicative Id)

ec49c6c

Signed-off-by: Elias Rohrer <dev@tnull.de>

tnull force-pushed the 2025-11-lsps1-refactor branch from ac4c5f4 to ec49c6c Compare March 12, 2026 22:43

Conversation

tnull commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull commented Feb 4, 2026

Uh oh!

tnull commented Feb 5, 2026

Uh oh!

ldk-reviews-bot commented Feb 7, 2026

Uh oh!

ldk-reviews-bot commented Feb 9, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldk-reviews-bot commented Feb 10, 2026

Uh oh!

ldk-reviews-bot commented Feb 14, 2026

Uh oh!

ldk-reviews-bot commented Feb 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull commented Mar 2, 2026

Uh oh!

TheBlueMatt commented Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tnull commented Dec 12, 2025 •

edited

Loading

ldk-reviews-bot commented Dec 12, 2025 •

edited

Loading

codecov bot commented Dec 12, 2025 •

edited

Loading