Add bLIP-52 / LSPS2 service-side support #420

tnull · 2024-12-10T13:32:48Z

This adds initial bLIP-52 / LSPS2 service-side to LDK Node. When configured via Builder::set_liquidity_provider_lsps2, the node will serve JIT channels to clients.

In this initial version we have the user configure the detail fields via an LSPS2ServiceConfig, and do not track and retry honoring pending buy requests. This means that we will simply (silently due to lack of a feedback mechanism in the bLIP-52 / LSPS2 protocol) fail the flow if, e.g., we don't have sufficient liquidity or the user isn't connected to us at the time of handling the payment.

toneloc · 2025-02-05T18:57:35Z

Thank you for ushering this along. I am excited for this to be included.

Previously, we named internal fields/APIs `lspsX_service` as us being the client was implied. Since we're about to also add service-side functionalities, such naming would start to get confusing. We hence rename them to follow a `lspsX_client` scheme, and will add the service-side APIs using the `service` terminology.

tnull · 2025-02-06T19:51:34Z

Rebased after #418 landed.

tnull · 2025-02-06T19:54:32Z

@G8XSU This has now been unblocked. As mentioned offline, while the LSP-facing API is still tbd, I'd be inclined to land this soon with a very simple initial version of the API, perhaps behind a cfg-gate to begin with.

G8XSU · 2025-02-14T01:28:07Z

src/builder.rs

+	/// **Caution**: LSP service support is in **alpha** and is considered an experimental feature.
+	///
+	/// [LSPS2]: https://github.com/BitcoinAndLightningLayerSpecs/lsp/blob/main/LSPS2/README.md
+	pub fn set_liquidity_provider_lsps2(


I know we have probably used setX in names because it is builder but it is getting slightly confusing with lsps client and service.
Personally, I was slightly confused due to the fact "we need to set liquidity provider to act as lsps-client".

no strong opinion, but imo, these methods could be renamed to enable_lsps2_service, enable_lsps2_client and so on.
what do you think?

alternatively, we can add another layer here similar to .bolt11() and .bolt12().

Hmm, thanks for bringing this up. I see you point in that theset_liquidity_provider case that could be a bit confusing, but still think I'd like to keep the set_ pattern everywhere for now. For one, it follows the Rust API guidelines for naming setters. Also note that once we break the pattern, we might need to revisit all of the builder methods, which a) would be a larger refactor out-of-scope of this PR and b) would make the API less predicatable, IMO. But noted.

G8XSU · 2025-02-14T01:37:22Z

src/liquidity.rs

@@ -165,6 +190,7 @@ where
 {
 	lsps1_client: Option<LSPS1Client>,
 	lsps2_client: Option<LSPS2Client>,
+	lsps2_service: Option<LSPS2Service>,


doubt: what is LiquiditySource here? it contains both liquidity-provider and liquidity-consumer

Yes, LiquiditySource is simply the internal object we use to handle everything liquidity-related. Let me know if you'd have a suggestion for a much better name, but for now I decided to not rename it also here.

src/liquidity.rs

G8XSU · 2025-02-14T02:20:52Z

Reviewed everything before the WIP commit (ef2ece2)

.. and while we're at it we move the VSS child key indexes to constants.

.. we might eventually want to drop them anyways, but for now we rename them to make them easily discernable from their counterparts in `builder.rs`.

.. for consistency, as we're about to add `LSPS2ServiceConfig` there, too.

We add the capability to configure LSPS2 service mode in `Builder` and `LiquiditySourceBuilder`.

…for clarity

tnull · 2025-02-14T14:55:58Z

@G8XSU Now finished this initial support with a rudimentary API that let's the user configure via a LSPS2ServiceConfig object. Also added a end-to-end client<>service integration test, which confirms it's working as intended in the happy case. (note we currently do not track pending requests and hence do not retry channel opening if it would fail for one reason or another at this stage, now tracking this here: #473).

Given the given limitations I'm still on the fence whether we should already expose this fully as an 'experimental' feature, or if we should hide it behind a cfg flag for now.

G8XSU

Reviewed integration test 2e9a3b2

Mostly pending review of 4a00e8c

tests/integration_tests_rust.rs

.. and forward it to our `LiquditySource`.

.. to align with other event handling variants: First log, then act, then emit event if everything went okay.

.. so far we just silently fail if something goes wrong, eventually we'll need to implement retrying channel opens to honor buy requests that didn't succeed on the first attempt.

tnull · 2025-02-20T08:05:28Z

Added another fixup asserting the channel size in tests, and another one slightly clarifying the docs on the overprovisioning factor field.

tnull · 2025-02-21T09:20:12Z

@G8XSU Let me know when I can squash the fixups.

G8XSU · 2025-02-26T06:06:02Z

src/liquidity.rs

+				counterparty_node_id,
+				token,
+			}) => {
+				if let Some(lsps2_service_handler) =


IMO, LSPS might need to handle its events dynamically at runtime, rather than relying on static configuration. In other words, it could evaluate each event, apply the appropriate business logic, and then act accordingly.

While I understand that we may not want to expose actionable events from the LDK Node right now, what is our long-term thinking for this approach?

IMO, LSPS might need to handle its events dynamically at runtime, rather than relying on static configuration. In other words, it could evaluate each event, apply the appropriate business logic, and then act accordingly.

Well, that's exactly the kind of question I want to figure out with users/based on user feedback. It could be that prospective users would appreciate some ready-to-go automation in some areas, but still want to maintain control over others.

While I understand that we may not want to expose actionable events from the LDK Node right now, what is our long-term thinking for this approach?

I'm not sure we'd want to mirror LDK's "general" actionable events for stuff like this. If we find users demand control over certain parts of the flow, we should create specific interfaces for this in LDK Node. How the corresponding API would look like in Server is tbd, but generally I suspect we can accommodate most of users' requirements without custom logic to begin with.

I'm not sure we'd want to mirror LDK's "general" actionable events for stuff like this.

FWIW, it is not at all specific to LDK, it is just a generic events interface which is a common pattern.

How the corresponding API would look like in Server is tbd, but generally I suspect we can accommodate most of users' requirements without custom logic to begin with.

For a server/enterprise deployment an event interface might make more sense(I am also on the fence regarding this since it is slightly more complicated), but we will know more with better user requirements. One thing to note is that server can't really support an event interface without ldk-node supporting the same.

G8XSU · 2025-02-26T06:11:54Z

src/liquidity.rs

+				) {
+					Ok(_) => {},
+					Err(e) => {
+						// TODO: We just silently fail here. Eventually we will need to remember


in cases where we silently fail(pending TODO's), how does the client handle it? iiuc they won't get any failure response, and will wait forever for the response.

Yes this is why we should retry channel opening upon reconnection of the client (see #473) before finally giving up. But note that clients can also timeout requests and don't lose anything as they'd only pay upon claiming the forwarded payment post channel opening.

But note that clients can also timeout requests and don't lose anything

Just for my understanding, Do they currently timeout requests ?
I understand that they don't lose sats, but a pending unresolved request might consume unnecessary resources on client side.

G8XSU · 2025-02-26T06:17:39Z

src/liquidity.rs

+				// We set the forwarding fee to 0 for now as we're getting paid by the channel fee.
+				//
+				// TODO: revisit this decision eventually.
+				config.channel_config.forwarding_fee_base_msat = 0;


isn't this something that can be part of service_config?

I'd rather not put it there as this point, as it doesn't make sense to have the LSP doubly-paid for the same service. It would also complicate all the calculations even more. If we eventually want to allow the LSP to charge forwarding fees, they probably should only be updated after the initial payment flow went through.

G8XSU

Lgtm, feel free to squash fixups.

G8XSU · 2025-02-27T17:29:53Z

src/liquidity.rs

+pub struct LSPS2ServiceConfig {
+	/// A token we may require to be sent by the clients.
+	///
+	/// If set, only requests matching this token will be accepted.


do we need to specify where in request the token must be set?

G8XSU · 2025-02-27T17:36:33Z

src/liquidity.rs

+				counterparty_node_id,
+				token,
+			}) => {
+				if let Some(lsps2_service_handler) =


I'm not sure we'd want to mirror LDK's "general" actionable events for stuff like this.

FWIW, it is not at all specific to LDK, it is just a generic events interface which is a common pattern.

How the corresponding API would look like in Server is tbd, but generally I suspect we can accommodate most of users' requirements without custom logic to begin with.

For a server/enterprise deployment an event interface might make more sense(I am also on the fence regarding this since it is slightly more complicated), but we will know more with better user requirements. One thing to note is that server can't really support an event interface without ldk-node supporting the same.

G8XSU · 2025-02-27T17:39:38Z

src/liquidity.rs

+
+					let opening_fee_params = RawOpeningFeeParams {
+						min_fee_msat: service_config.min_channel_opening_fee_msat,
+						proportional: service_config.channel_opening_fee_ppm,


imo, we might also need a max_fee_msat in getInfo, since a proportional fee for bigger channels might not make sense. (non-actionable for now)

G8XSU · 2025-02-27T17:42:00Z

src/liquidity.rs

+				) {
+					Ok(_) => {},
+					Err(e) => {
+						// TODO: We just silently fail here. Eventually we will need to remember


But note that clients can also timeout requests and don't lose anything

Just for my understanding, Do they currently timeout requests ?
I understand that they don't lose sats, but a pending unresolved request might consume unnecessary resources on client side.

G8XSU · 2025-02-27T17:45:45Z

src/liquidity.rs

+				};
+
+				// Fail if we have insufficient onchain funds available.
+				let over_provisioning_msat = (amt_to_forward_msat


we might need a max_channel_size as well to avoid draining all the liquidity for a single channel.

G8XSU · 2025-02-27T18:03:11Z

src/liquidity.rs

+				) {
+					Ok(_) => {},
+					Err(e) => {
+						// TODO: We just silently fail here. Eventually we will need to remember


Eventually we will need to remember the pending requests.

It is a bummer to remember all pending requests, can we timeout after few retries?

tnull marked this pull request as draft December 10, 2024 13:33

tnull force-pushed the 2024-12-add-lsps2-service-support branch 2 times, most recently from 1cca737 to cd2795d Compare December 10, 2024 15:17

tnull added the blocked on dependent PR label Jan 15, 2025

tnull force-pushed the 2024-12-add-lsps2-service-support branch from cd2795d to dfdef1d Compare January 16, 2025 09:38

tnull force-pushed the 2024-12-add-lsps2-service-support branch from dfdef1d to 7538c83 Compare January 30, 2025 13:59

tnull force-pushed the 2024-12-add-lsps2-service-support branch 2 times, most recently from c1f2b6c to ef2ece2 Compare February 6, 2025 19:48

tnull requested a review from G8XSU February 6, 2025 19:51

tnull removed the blocked on dependent PR label Feb 6, 2025

tnull added this to the 0.5 milestone Feb 13, 2025

G8XSU reviewed Feb 14, 2025

View reviewed changes

tnull added 6 commits February 14, 2025 11:15

Refactor derive_xprv to make it reusable

5b276d7

.. and while we're at it we move the VSS child key indexes to constants.

Prefix the lightning-liqudity config types with Ldk for clarity

15f2fbe

.. we might eventually want to drop them anyways, but for now we rename them to make them easily discernable from their counterparts in `builder.rs`.

Move LSPSXClientConfig to liquidity

09343ce

.. for consistency, as we're about to add `LSPS2ServiceConfig` there, too.

Allow configuring LSPS2 service via builders

2a77b14

We add the capability to configure LSPS2 service mode in `Builder` and `LiquiditySourceBuilder`.

f Prefix the lightning-liqudity LSPS2ServiceConfig type with Ldk …

7d8e26d

…for clarity

f Add more fields to LSPS2ServiceConfig

29440f1

tnull force-pushed the 2024-12-add-lsps2-service-support branch from ef2ece2 to b09be67 Compare February 14, 2025 14:49

tnull marked this pull request as ready for review February 14, 2025 14:49

tnull changed the title ~~Add LSPS2 service-side support~~ Add bLIP-52 / LSPS2 service-side support Feb 14, 2025

tnull mentioned this pull request Feb 14, 2025

bLIP-52/LSPS2 service: Retry failed channel openings #473

Open

tnull force-pushed the 2024-12-add-lsps2-service-support branch from b09be67 to 2e9a3b2 Compare February 14, 2025 14:59

G8XSU reviewed Feb 20, 2025

View reviewed changes

tests/integration_tests_rust.rs Show resolved Hide resolved

tnull added 8 commits February 20, 2025 09:04

f Clarify overprovisioning docs

07f3833

Add LDK event handling

cf8c802

.. and forward it to our `LiquditySource`.

f Make logging more consistent

0019584

f Set accept_intercept_htlcs in Builder if we're an LSP

64897a5

Move PaymentForwarded event emission down

435c661

.. to align with other event handling variants: First log, then act, then emit event if everything went okay.

Add LSPS2ServiceEvent handling

6f50ad3

.. so far we just silently fail if something goes wrong, eventually we'll need to implement retrying channel opens to honor buy requests that didn't succeed on the first attempt.

Add LSPS2 client<>service integration test

85ffede

f Move send below comment

4e5b05e

tnull force-pushed the 2024-12-add-lsps2-service-support branch from dfee2f6 to 503f3b7 Compare February 20, 2025 08:04

f Assert channel value correctness

bad639a

tnull force-pushed the 2024-12-add-lsps2-service-support branch from 503f3b7 to bad639a Compare February 21, 2025 09:19

tnull requested a review from G8XSU February 21, 2025 09:19

f Uniffi tests

744ad40

G8XSU reviewed Feb 26, 2025

View reviewed changes

G8XSU reviewed Feb 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bLIP-52 / LSPS2 service-side support #420

Add bLIP-52 / LSPS2 service-side support #420

tnull commented Dec 10, 2024 •

edited

Loading

toneloc commented Feb 5, 2025

tnull commented Feb 6, 2025

tnull commented Feb 6, 2025

G8XSU Feb 14, 2025

tnull Feb 14, 2025 •

edited

Loading

G8XSU Feb 14, 2025

tnull Feb 14, 2025

G8XSU commented Feb 14, 2025

tnull commented Feb 14, 2025 •

edited

Loading

G8XSU left a comment •

edited

Loading

tnull commented Feb 20, 2025

tnull commented Feb 21, 2025

G8XSU Feb 26, 2025

tnull Feb 26, 2025

G8XSU Feb 27, 2025

G8XSU Feb 26, 2025

tnull Feb 26, 2025

G8XSU Feb 27, 2025

G8XSU Feb 26, 2025

tnull Feb 26, 2025

G8XSU left a comment

G8XSU Feb 27, 2025

G8XSU Feb 27, 2025

G8XSU Feb 27, 2025

G8XSU Feb 27, 2025

G8XSU Feb 27, 2025

G8XSU Feb 27, 2025

Add bLIP-52 / LSPS2 service-side support #420

Are you sure you want to change the base?

Add bLIP-52 / LSPS2 service-side support #420

Conversation

tnull commented Dec 10, 2024 • edited Loading

toneloc commented Feb 5, 2025

tnull commented Feb 6, 2025

tnull commented Feb 6, 2025

Choose a reason for hiding this comment

tnull Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G8XSU commented Feb 14, 2025

tnull commented Feb 14, 2025 • edited Loading

G8XSU left a comment • edited Loading

Choose a reason for hiding this comment

tnull commented Feb 20, 2025

tnull commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G8XSU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Dec 10, 2024 •

edited

Loading

tnull Feb 14, 2025 •

edited

Loading

tnull commented Feb 14, 2025 •

edited

Loading

G8XSU left a comment •

edited

Loading