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 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
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
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
from
c6eb6b3 to
a2aa7c3
Compare
|
Rebased to resolve conflicts. |
|
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
4 times, most recently
from
3ae7578 to
4489325
Compare
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
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
3988fe5 to
c05c96d
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
Signed-off-by: Elias Rohrer <dev@tnull.de>
Add missing cross-validation of `LSPS1OrderParams` against `LSPS1Options` as required by bLIP-51: - Check `required_channel_confirmations` >= `min_required_channel_confirmations` - Check `funding_confirms_within_blocks` >= `min_funding_confirms_within_blocks` - Check total channel balance (`lsp_balance_sat` + `client_balance_sat`) is within [`min_channel_balance_sat`, `max_channel_balance_sat`], using `checked_add` to guard against overflow Co-Authored-By: HAL 9000
Previously, if any `.await?` in the persist loop returned an error, the `?` would propagate out of `persist()` before reaching the `fetch_sub` at the end of the loop. This left the counter permanently > 0, causing all subsequent `persist()` calls to early-return and effectively disabling persistence for the lifetime of the handler. Fix this by extracting the loop into `do_persist()` and unconditionally resetting the counter via `store(0, Release)` in the outer `persist()` after `do_persist()` returns, regardless of success or failure. Co-Authored-By: HAL 9000
tnull
force-pushed
the
2025-11-lsps1-refactor
branch
from
c05c96d to
47e5c04
Compare
|
@TheBlueMatt Alright, I think we entered hallucination / loopy territory with Claude here. Also fixed the |
|
We might want to backport 47e5c04 to 0.2 |
|
Backported in #4706 |
v0.2.3 - Jun 18, 2026 - "Through the Loupe" API Updates =========== * `DefaultMessageRouter` will now always generate blinded message paths that provide no privacy (where our node is the introduction node) for nodes with public channels. This works around an issue which will appear for any nodes with LND peers that enable onion messaging - such peers will refuse to forward BOLT 12 messages from unknown third parties, which most BOLT 12 payers rely on today (lightningdevkit#4647). * Explicit `amount_msats` of 0 is rejected in BOLT 12 `Offer`s; `OfferBuilder` now maps 0-amounts to an amount of `None` (lightningdevkit#4324). Bug Fixes ========= * `Features::supports_zero_conf` no longer clears the `ZeroConf` features and `Features::requires_zero_conf` now correctly reports required, rather than supported, status (lightningdevkit#4517). * If an MPP payment is claimed but `ChannelMonitorUpdate`s for some parts are still being completed asynchronously, further channel updates (e.g. forwarding another payment) are pending and the node restarts, the channel could have become stuck (lightningdevkit#4520). * The presence of unconfirmed transactions actually no longer causes `ElectrumSyncClient` to spuriously fail to sync (lightningdevkit#4590). * LSPS1, LSPS2, and LSPS5 persistence will no longer get stuck and refuse to persist again after a single failure from the KVStore (lightningdevkit#4597, lightningdevkit#4282). * Dropping the future returned by `OutputSweeper::regenerate_and_broadcast_spend_if_necessary` no longer results in future calls to the same method being spuriously ignored (lightningdevkit#4598). * Used async-receive offers are no longer refreshed on every timer tick once their refresh time is reached (lightningdevkit#4672). * `FilesystemStore::list_all_keys` will no longer fail if there are stale intermediate files lying around from a previous unclean shutdown (lightningdevkit#4618). * When forwarding an HTLC while in a blinded path with proportional fees over 200%, LDK will no longer spuriously allow a forward that pays us 1 msat too little in fees (lightningdevkit#4697). * Fixed a rare case where a channel could get stuck on reconnect when using both async `ChannelMonitorUpdate` persistence and async signing (lightningdevkit#4684). * If we had exactly zero balance in a zero-fee-commitment channel, the counterparty was able to splice all of their balance out, violating the reserve requirements they'd otherwise be forced to keep (lightningdevkit#4580). * Providing an `Event::HTLCIntercepted` to the `LSPS2ServiceHandler` twice no longer results in spuriously opening a channel early (lightningdevkit#4656). * `Event::PaymentSent::fee_paid_msat` is no longer `None` in cases where `ChannelManager::abandon_payment` was called before the payment ultimately completes anyway (lightningdevkit#4651). * `AnchorDescriptor::previous_utxo` now provides the correct `script_pubkey` for non-zero-commitment-fee anchor channels (lightningdevkit#4669). * Syncing a `ChainMonitor` using the `Confirm` trait will no longer write some full `ChannelMonitor`s to disk several times per block (lightningdevkit#4544). * `OMDomainResolver` now correctly accounts for failed queries when rate limiting, ensuring we continue to respond to queries after failures (lightningdevkit#4591). * Calling `ChannelManager::send_payment_with_route` without a `route_params` and with an invalid `Route` will no longer panic (lightningdevkit#4707). * `LSPS2ServiceHandler::channel_open_failed` now correctly fails intercepted HTLCs rather than allowing them to fail just before expiry (lightningdevkit#4677). * `StaticInvoice::is_offer_expired` was corrected to check offer, rather than static invoice, expiry (lightningdevkit#4594). * `lightning-custom-message`'s handling of `peer_connected` events now ensures that sub-handlers will see a `peer_disconnected` event if a different sub-handler refused the connection by `Err`ing `peer_connected` (lightningdevkit#4595). * Replay protection for LSPS5 signatures now detects replays which are only different in the encoded signature's case (lightningdevkit#4701). * When `lightning-liquidity` is configured in the background processor, there is no longer a stream of `Persisting LiquidityManager...` log spam (lightningdevkit#4246). * Incomplete MPP keysend payments will no longer see their HTLCs held until expiry (lightningdevkit#4558). * `InvoiceRequestBuilder` will no longer accept a `quantity` of `0` for a BOLT 12 `Offer`, allowing any quantity up to a bound (lightningdevkit#4667). * `lightning-custom-message` handlers that return `Ok(None)` when asked to deserialize a message in their defined range no longer cause panics (lightningdevkit#4709). * Several spurious debug assertions were fixed (lightningdevkit#4537, lightningdevkit#4618, lightningdevkit#4026) Security ======== 0.2.3 fixes several underestimates of the anchor reserves required to ensure we can reliably close channels, several denial-of-service vulnerabilities and a sanitization issue. * `Bolt11Invoice::recover_payee_pub_key` no longer panics if called on an invoice which set an explicit public key, rather than relying on public key recovery. Note that this method is called from `PaymentParameters::from_bolt11_invoice` (lightningdevkit#4717). * Maliciously-crafted unpayable invoices which have overflowing feerates will no longer cause an `unwrap` failure panic (lightningdevkit#4716). * Parsing an `LSPSDateTime` which is before 1970 no longer panics. This is reachable when parsing messages from counterparties (lightningdevkit#4715). * `possiblyrandom` did not properly generate random data except when it was explicitly configured to. By default this means LDK is vulnerable to various HashDoS attacks (lightningdevkit#4719). * `OMNameResolver` will no longer panic when looking up payment instructions which include unicode characters at the start of a TXT record (lightningdevkit#4718). * When using the `anchor_channel_reserves` module to calculate reserves required to pay for fees when closing anchor channels, zero-fee-commitment channels were not considered. This could allow a counterparty to open many channels, leaving us unable to properly force-close (lightningdevkit#4592). * The `anchor_channel_reserves` module overestimated the value of `Utxo`s in the wallet by ignoring the `TxIn` cost to spend them (lightningdevkit#4670). * `PrintableString` did not properly sanitize unicode format characters, allowing an attacker to corrupt the rendering of logs or UI (lightningdevkit#4593, lightningdevkit#4605). * RGS data is now limited in how large of a graph it is able to cause a client to store in memory. Note that RGS data is still considered a DoS vector in general and you should only use semi-trusted RGS data (lightningdevkit#4713). * Counterparty-provided strings in failure messages are no longer logged in full, reducing the ability of such a counterparty to spam our logs (lightningdevkit#4714). * Reading a corrupted `ChannelManager` or `ProbabilisticScorer` can no longer cause us to allocate large amounts of memory (lightningdevkit#4712). Thanks to Project Loupe for reporting most of the issues fixed in this release. Conflicts resolved in: * lightning/src/chain/channelmonitor.rs * lightning/src/events/mod.rs * lightning/src/ln/channelmanager.rs * lightning/src/ln/mod.rs * lightning/src/ln/offers_tests.rs * lightning/src/ln/onion_utils.rs
5 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.