Client trusts lsp #572

martinsaposnic · 2025-06-09T19:35:10Z

Please just review the second commit 53a8174. The first commit is just compatibility changes so it works with rust-lightning

This PR uses the functionality introduced in this other PR lightningdevkit/rust-lightning#3838, and creates 2 tests that demonstrates the feature

Changes:

call store_funding_transaction when funding transaction is created
call channel_needs_manual_broadcast to check if it needs to call funding_transaction_generated_manual_broadcast or funding_transaction_generated
call lsps2_funding_tx_broadcast_safe when FundingTxBroadcastSafe is emitted
a refactor on PaymentClaimable so it does not autoclaim (this will be changed in a future commit)
a new fn receive_via_jit_channel_manual_claim so the client can actually do a manual claim with claim_for_hash, instead of auto claiming
two beautiful tests that check the functionality
- lsps2_client_trusts_lsp checks that the funding_tx is not broadcasted until the htlc is claimed
- lsps2_lsp_trusts_client_but_client_does_not_claim checks that the funding_tx is broadcasted even though the client didn't claim the htlc

ldk-reviews-bot · 2025-06-09T19:35:13Z

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

ldk-reviews-bot · 2025-06-12T08:39:17Z

🔔 1st Reminder

Hey @tnull! 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.

tnull · 2025-06-11T09:22:50Z

src/event.rs

-									"Failed to process funding transaction: {:?}",
-									err
+						self.liquidity_source.as_ref().map(|ls| {
+							ls.lsps2_store_funding_transaction(


Would we always need to store the funding transaction, even if we're not doing the full flow?

I moved it to inside the if statement, so it will store it only if needs_manual_broadcast is true 👍

tnull · 2025-06-11T09:24:35Z

src/event.rs

-								return Err(ReplayEvent());
-							},
-						};
+						self.payment_store.update(&update).map_err(|e| {


There are a lot of unrelated changes happening in this commit. a) not sure if I agree that they are worth doing at all but b) always try to keep such changes in separate commits please.

Sorry about that, I made a rebase and a cleanup. now this commit 53a8174 will have the minimal changes to make the client_trusts_lsp flow work

There are a lot of unrelated changes happening in this commit.

This was a refactor on the PaymentClaimable event to make the manual claim work on my tests.

That part is removed after the rebase, so we should be good now

ldk-reviews-bot · 2025-06-18T19:50:38Z

🔔 1st Reminder

Hey @tnull! 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 · 2025-06-21T00:00:26Z

🔔 2nd Reminder

Hey @tnull! 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 · 2025-06-23T00:00:59Z

🔔 3rd Reminder

Hey @tnull! 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 · 2025-06-25T00:01:59Z

🔔 4th Reminder

Hey @tnull! 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.

tnull · 2025-06-26T12:02:30Z

cc @johncantrell97 as he previously looked into this and might be interested in reviewing/and or might have additional thoughts.

enigbe

I have partially reviewed 53a81743 (pending tests) and found it helpful in clarifying my understanding of LSPs. I have some initial comments/questions regarding the following:

client_trusts_lsp configuration

client_trusts_lsp is configured statically but I have assumed that based on blip52, this could (should?) be dynamically configured without restarting the node when it detects an attack.
Given the current implementation, what happens to in-flight outbound JIT channels (whose clients are awaiting funding transactions to be broadcast) during a trust mode transition, i.e. lsp_trusts_client to client_trusts_lsp?

error silencing

I don't fully understand why possible APIMisuseError relating to channel_needs_manual_broadcast is silenced/ignored with unwrap_or(false). Should we proceed with funding if we can't, for example, associate a channel with the user_channel_id?

enigbe · 2025-07-09T19:43:54Z

src/liquidity.rs

@@ -134,6 +133,8 @@ pub struct LSPS2ServiceConfig {
 	pub min_payment_size_msat: u64,
 	/// The maximum payment size that we will accept when opening a channel.
 	pub max_payment_size_msat: u64,
+	/// Use the client trusts lsp model
+	pub client_trusts_lsp: bool,


I'm trying to understand the client_trusts_lsp configuration here. According to bLIP-52, I expect the LSP to dynamically switch from lsp_trusts_client to client_trusts_lsp mode upon detecting an attack, without requiring a restart.
However, this configuration appears to be static and set at node startup. If that's the case, what happens when:

A node initially running in lsp_trusts_client mode detects an attack

The node restarts with client_trusts_lsp: true to switch modes

There's an existing outbound JIT channel where the client expects the LSP to broadcast the funding transaction before sending the preimage to claim.

What are the potential consequences for that in-flight JIT channel during the mode transition?

just pushed a fixup that makes the flag dynamic

There's an existing outbound JIT channel where the client expects the LSP to broadcast the funding transaction before sending the preimage to claim.

I just posted a question about this https://discord.com/channels/915026692102316113/994015949176963183/1397280758196080660

my current interpretation about this is that once lsps2.buy succeeds, that flag is part of the contract for this flow. the LSP can always abort and make you start over, but it cannot change the trust model mid negotiation

we will see what they respond on discord

my current interpretation about this is that once lsps2.buy succeeds, that flag is part of the contract for this flow. the LSP can always abort and make you start over, but it cannot change the trust model mid negotiation

I created a test that shows how this works (test lsps2_in_flight_under_attack_switch)

my current interpretation about this is that once lsps2.buy succeeds, that flag is part of the contract for this flow. the LSP can always abort and make you start over, but it cannot change the trust model mid negotiation

Yes, as also noted on Discord, I agree with that interpretation and even think that for now we can leave the flag to be statically determined at startup.

enigbe · 2025-07-09T21:17:53Z

src/liquidity.rs

+			if let Some(handler) = lsps2_service_handler {
+				handler
+					.channel_needs_manual_broadcast(user_channel_id, &counterparty_node_id)
+					.unwrap_or(false)


Unsure about unwrap_or here. I think we should log the error and be concerned about its cause if channel_needs_manual_broadcast returns an APIError::APIMisuseError? Say we couldn't find a channel mapped to the provided user_channel_id, should we still go on to call channel_manager.funding_transaction_generated for a non-existent channel when handling FundingGenerationReady? I am confused about this.

this is also fixed in commit 2cc5267

thank you for the review!

…nual_broadcast logs error if it fails

martinsaposnic mentioned this pull request Jun 9, 2025

Support client_trusts_lsp on LSPS2 lightningdevkit/rust-lightning#3838

Draft

tnull self-requested a review June 10, 2025 08:38

tnull reviewed Jun 12, 2025

View reviewed changes

martinsaposnic added 2 commits June 16, 2025 16:47

Compatibility changes. DO NOT REVIEW

c9c1b38

Support client_trusts_lsp

53a8174

martinsaposnic force-pushed the client-trusts-lsp branch from 480a3ce to 53a8174 Compare June 16, 2025 19:47

martinsaposnic requested a review from tnull June 16, 2025 19:49

martinsaposnic marked this pull request as ready for review June 17, 2025 14:29

tnull removed their request for review June 26, 2025 12:13

enigbe reviewed Jul 9, 2025

View reviewed changes

fixup: client_trusts_lsp is now a dynamic flag. also channel_needs_ma…

2cc5267

…nual_broadcast logs error if it fails

Client trusts lsp #572

Are you sure you want to change the base?

Client trusts lsp #572

Uh oh!

Conversation

martinsaposnic commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinsaposnic Jun 16, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jun 18, 2025

Uh oh!

ldk-reviews-bot commented Jun 21, 2025

Uh oh!

ldk-reviews-bot commented Jun 23, 2025

Uh oh!

ldk-reviews-bot commented Jun 25, 2025

Uh oh!

tnull commented Jun 26, 2025

Uh oh!

enigbe left a comment

Choose a reason for hiding this comment

client_trusts_lsp configuration

error silencing

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

martinsaposnic commented Jun 9, 2025 •

edited

Loading

ldk-reviews-bot commented Jun 9, 2025 •

edited

Loading

martinsaposnic Jun 16, 2025 •

edited

Loading