`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

👋 Thanks for assigning @TheBlueMatt 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 · 2025-12-12T17:54:26Z

Codecov Report

❌ Patch coverage is 70.71214% with 292 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.08%. Comparing base (d09f1d2) to head (104eb5f).

Files with missing lines	Patch %	Lines
lightning-liquidity/src/lsps1/service.rs	58.77%	178 Missing and 17 partials ⚠️
lightning-liquidity/src/lsps1/peer_state.rs	84.05%	34 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	82.05%	2 Missing and 5 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.91%   86.08%   +0.16%     
==========================================
  Files         156      158       +2     
  Lines      103958   104995    +1037     
  Branches   103958   104995    +1037     
==========================================
+ Hits        89317    90381    +1064     
+ Misses      12122    12016     -106     
- Partials     2519     2598      +79

Flag	Coverage Δ
tests	`86.08% <70.71%> (+0.16%)`	⬆️

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

TheBlueMatt · 2026-02-17T18:49:33Z

lightning-liquidity/src/lsps1/peer_state.rs

+		let pending_orders = self
+			.outbound_channels_by_order_id
+			.iter()
+			.filter(|(_, v)| v.is_pending_payment())


No, we cannot simply ignore some subset of things taking up memory because of their state. This is a DoS vector all the same.

Hmm? But as long as we don't expose a way to cleanup the historical state, not only accounting for entries pending payment will lead to the client eventually "filling up" their quota of channels? (also cf #4282 (comment))

This really only copies what we already do for LSPS2, FWIW.

lightning-liquidity/src/lsps1/service.rs

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

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

tnull · 2026-02-18T13:07:53Z

Addressed pending comments and rebased to resolve some minor conflicts. Let me know if I can squash fixups.

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 27 commits February 18, 2026 13:40

Add serialization logic for LSPS1 PeerState types

45a39ec

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

f Remove redundant FeeRate Writeable/Readable implementations

7de8384

Implement LSPS1ServiceHandler persistence and state pruning

b707bb0

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

ab47e72

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

5ac2675

Co-authored by Claude AI

Add some checks on provided payment details

a98d16b

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.

f Expose refund_onchain_address in RequestForPaymentDetails event

292adb4

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

5b2b169

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

Add invalid_token_provided API method

65fa261

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.

f Add invalid_token_provided to RequestForPaymentDetails event docs

c8b5259

f s/partof/part of/

c469777

Add test case for invalid_token_provided flow

0b12167

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

Drop lsps1_service cfg flag

d18d3dd

Fix clippy lints

386b734

f Mention refund_onchain_address

529f379

f Account for refund_onchain_address in tests

a7a5477

Drop unused LSPS1OnchainPayment type

88e9fbf

Drop unused LSPS1ServiceEvent::Refund event

9c7be15

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

Add onchain_payment_required method

f2ccc69

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.

f reword docs

f9bd7b8

f Avoid double-lookup

de78a03

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

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

104eb5f

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

tnull force-pushed the 2025-11-lsps1-refactor branch from ed6d0c9 to 104eb5f Compare February 18, 2026 13:07

tnull requested a review from TheBlueMatt February 18, 2026 13:07

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull commented Feb 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

tnull commented Dec 12, 2025 •

edited

Loading

ldk-reviews-bot commented Dec 12, 2025 •

edited

Loading

codecov bot commented Dec 12, 2025 •

edited

Loading