`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.11%. Comparing base (14e522f) to head (e31b5c8).

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.94%   86.11%   +0.17%     
==========================================
  Files         159      161       +2     
  Lines      104644   105681    +1037     
  Branches   104644   105681    +1037     
==========================================
+ Hits        89934    91009    +1075     
+ Misses      12204    12088     -116     
- Partials     2506     2584      +78

Flag	Coverage Δ
tests	`86.11% <70.71%> (+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

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.

Maybe we need a way to clean up historical state then? We can't just leave a trivial DoD vulnerability lying around, that's not acceptable?

Hmm, but how is it a DoS vulnerability if they have to pay for a channel for each entry? I assume the LSP would be happy if they buy so many channels that they run OOM? (for that to be entirely true we should probably also filter the FailedAndRefunded case, though)

That said, I agree we should allow for a way to cleanup the state, though as with the messages, I'd prefer to do that in a follow-up PR to add corresponding APIs across the different spec implementations, not just for LSPS1. Would that be fine with you?

Right, it seems reasonable to hold onto channel info that was paid-for (though we should definitely have a way to clear that out eventually...maybe a configurable timeout?) but as you note we currently don't prune failed, which someone can trivially pollute us with. That was my main complaint here.

Alright, now included a fixup (9457478) that now matches on "not paid or completed", i.e., now includes failed in the accounting for rate-limiting. And then we'll need to address pruning / forgetting state for both LSPS1/LSPS2 in a follow-up, now tracking here: #4444

lightning-liquidity/src/lsps1/service.rs

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

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.

tnull · 2026-02-25T11:03:18Z

Still needs the DoS vuln fixed, but mostly looks good.

Rebased and addressed that in a fixup, let me know if I can squash.

TheBlueMatt · 2026-02-25T18:27:33Z

Yes please feel free to squash.

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-25T18:45:03Z

Yes please feel free to squash.

Squashed without further changes.

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 5 commits February 25, 2026 11:37

Drop OutboundCRChannel

582783e

The `OutboundChannel` construct simply wrapped `ChannelOrder` which we can now simply use directly.

Actually remember the order state in ChannelOrder

05bdad7

We here remember and update the order state and channel details in `ChannelOrder`

LSPS1ServiceHandler: Use TimeProvider when creating new orders

8de6ad1

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

tnull force-pushed the 2025-11-lsps1-refactor branch from 104eb5f to 97ee151 Compare February 25, 2026 10:57

tnull requested a review from TheBlueMatt February 25, 2026 11:04

tnull added 19 commits February 25, 2026 19:44

Add serialization logic for LSPS1 PeerState types

b372d4c

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

Implement LSPS1ServiceHandler persistence and state pruning

81767ab

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

430c3b7

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

eb53151

Co-authored by Claude AI

Add some checks on provided payment details

b5169ed

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

abb5b1c

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

Add invalid_token_provided API method

fab875c

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

e54c35d

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

Drop lsps1_service cfg flag

b3c45c5

Fix clippy lints

95d192f

Drop unused LSPS1OnchainPayment type

41ed59b

Drop unused LSPS1ServiceEvent::Refund event

e82ec52

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

Add onchain_payment_required method

13858e0

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.

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

e31b5c8

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

tnull force-pushed the 2025-11-lsps1-refactor branch from 97ee151 to e31b5c8 Compare February 25, 2026 18:44

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!

Choose a reason for hiding this comment

Uh oh!

tnull Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tnull commented Feb 25, 2026

Uh oh!

TheBlueMatt commented Feb 25, 2026

Uh oh!

tnull commented Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

tnull commented Dec 12, 2025 •

edited

Loading

ldk-reviews-bot commented Dec 12, 2025 •

edited

Loading

codecov bot commented Dec 12, 2025 •

edited

Loading

tnull Feb 19, 2026 •

edited

Loading