Support client_trusts_lsp=true on ldk-node #687

martinsaposnic · 2025-10-29T19:05:51Z

implement changes introduced on lightningdevkit/rust-lightning#3838 for rust-lightning.

as discussed, client_trusts_lsp is a flag set at startup.

ldk-reviews-bot · 2025-10-29T19:05:53Z

I've assigned @tnull 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.

toneloc · 2025-10-29T21:00:11Z

With LDK Node including this PR, I got a compilation error and had to add these imports

use crate::lightning::util::errors::APIError;
use crate::bitcoin::Transaction;

It compiled after that, then I tried to do LSPS2 JIT onboarding, but it failed ("route not found" from the client side).

Note sure if that's an issue with this PR or me.

martinsaposnic · 2025-10-29T21:05:37Z

I’m sorry. This should have been a draft. I just cherry picked the commits from the old PR. I will make CI pass in the next few days

tnull · 2025-10-30T12:02:04Z

Alright, @martinsaposnic please ping me for review as soon as this is ready!

toneloc · 2025-11-11T23:08:28Z

Any progress here? Thanks @martinsaposnic

implement changes introduced on lightningdevkit/rust-lightning#3838 as discussed, client_trusts_lsp is a flag set at startup. a new function receive_via_jit_channel_manual_claim is introduced to bolt11 so we allow the client to manually claim a payment (used on tests).

martinsaposnic · 2025-11-13T14:22:49Z

@tnull this should be ready now. let me know if you have comments

tnull

Actual code changes seem more or less trivial, but this PR needs some more cleanup.

tnull · 2025-11-14T09:49:37Z

src/event.rs

-							counterparty_node_id,
-							final_tx,
-						) {
+						let needs_manual_broadcast =


nit: Would be cleaner if we did:

diff --git a/src/event.rs b/src/event.rs index 7f467977..9a5516d8 100644 --- a/src/event.rs +++ b/src/event.rs @@ -506,20 +506,17 @@ where locktime, ) { Ok(final_tx) => { - let needs_manual_broadcast = - match self.liquidity_source.as_ref().map(|ls| { - ls.as_ref().lsps2_channel_needs_manual_broadcast( + let needs_manual_broadcast = self + .liquidity_source + .as_ref() + .and_then(|ls| { + ls.lsps2_channel_needs_manual_broadcast( counterparty_node_id, user_channel_id, ) - }) { - Some(Ok(v)) => v, - Some(Err(e)) => { - log_error!(self.logger, "Failed to determine if channel needs manual broadcast: {:?}", e); - false - }, - None => false, - }; + .ok() + }) + .unwrap_or(false); let result = if needs_manual_broadcast { self.liquidity_source.as_ref().map(|ls| {

tnull · 2025-11-14T09:51:16Z

src/liquidity.rs

 	kv_store: Arc<DynStore>,
 	config: Arc<Config>,
 	logger: L,
+	broadcaster: Arc<Broadcaster>,


No need to add this field if we already have tx_broadcaster: Arc<Broadcaster> above.

tnull · 2025-11-14T09:56:05Z

src/liquidity.rs

+	) -> Result<bool, APIError> {
+		// if we are not in a client_trusts_lsp model, we don't check and just return false
+		if !self.is_client_trusts_lsp() {
+			log_debug!(self.logger, "Skipping funding transaction broadcast as client trusts LSP.");


Huh, this is very confusing/wrong. We're actually auto-broadcasting, not manually/witholding the broadcast because we're in LSP-trusts-client, right?

Same below

tnull · 2025-11-14T09:56:53Z

src/liquidity.rs

+	pub(crate) fn lsps2_channel_needs_manual_broadcast(
+		&self, counterparty_node_id: PublicKey, user_channel_id: u128,
+	) -> Result<bool, APIError> {
+		// if we are not in a client_trusts_lsp model, we don't check and just return false


Please drop this redundant comment, it doesn't explain anything beyond the code below, just potentially adds to the confusion.

tnull · 2025-11-14T09:59:20Z

src/liquidity.rs


+	pub(crate) fn lsps2_channel_needs_manual_broadcast(
+		&self, counterparty_node_id: PublicKey, user_channel_id: u128,
+	) -> Result<bool, APIError> {


The Result<.., APIError> on lightning-liquidity pattern isn't great to begin with, but we should def. not have any methods in LDK Node using APIError. Please define an error variant if you really need one, but I suspect we don't actually need to.

Please clean this up, for example something like this

diff --git a/src/liquidity.rs b/src/liquidity.rs index a293311d..ddd517b1 100644 --- a/src/liquidity.rs +++ b/src/liquidity.rs @@ -312,22 +312,18 @@ where pub(crate) fn lsps2_channel_needs_manual_broadcast( &self, counterparty_node_id: PublicKey, user_channel_id: u128, - ) -> Result<bool, APIError> { - // if we are not in a client_trusts_lsp model, we don't check and just return false - if !self.is_client_trusts_lsp() { - log_debug!(self.logger, "Skipping funding transaction broadcast as client trusts LSP."); - return Ok(false); - } - - // if we are in a client_trusts_lsp model, then we check if the LSP has an LSPS2 operation in progress - self.lsps2_service.as_ref().map_or(Ok(false), |_| { - let lsps2_service_handler = self.liquidity_manager.lsps2_service_handler(); - if let Some(handler) = lsps2_service_handler { - handler.channel_needs_manual_broadcast(user_channel_id, &counterparty_node_id) - } else { - log_error!(self.logger, "LSPS2 service handler is not available."); - Ok(false) - } + ) -> bool { + self.lsps2_service.as_ref().map_or(false, |lsps2_service| { + lsps2_service.service_config.client_trusts_lsp + || self + .liquidity_manager() + .lsps2_service_handler() + .and_then(|handler| { + handler + .channel_needs_manual_broadcast(user_channel_id, &counterparty_node_id) + .ok() + }) + .unwrap_or(false) }) }

.. would be much more concise and readable, IMO. Also no need for 3-line util methods like is_client_trusts_lsp that just complicate things in this case.

(likewise below)

tnull · 2025-11-14T10:19:43Z

src/liquidity.rs

+		})
+	}
+
+	pub(crate) fn lsps2_store_funding_transaction(


Given that we use them both at the same callsite and for the same purpose, should we just merge lsps2_channel_needs_manual_broadcast / lsps2_store_funding_transaction to broadcast_or_store_funding_transaction and call that in the event handler? Then most of this redundant boilerplate goes away, we just check once what we should do and call the respective method on channelmanager here?

tnull · 2025-11-14T10:22:59Z

tests/integration_tests_rust.rs

 }
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+async fn lsps2_client_trusts_lsp() {


Can we simply DRY this up with lsps2_client_server_integration by renaming it do_lsps2_client_server_integration taking a client_trusts_lsp: bool that is set. Then we can simply add lsps2_client_server_integration_client_trusts_lsp that calls do_lsps2_client_server_integration(true).

tnull · 2025-11-14T10:25:47Z

tests/integration_tests_rust.rs

+	println!("Waiting for funding transaction to be broadcast... It will be there because LSP trusts the client, even though the client has not claimed it yet.");
+	let mut funding_tx_found = false;
+	for _ in 0..500 {
+		std::thread::sleep(std::time::Duration::from_millis(100));


Ugh, let's avoid too many sleeps in tests please, as over time they will considerably slow down our CI for no reason. Please use wait_for_tx for stuff like this, which at least does some polling with back-off.

tnull · 2025-11-14T10:27:58Z

tests/integration_tests_rust.rs

+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+async fn lsps2_lsp_trusts_client_but_client_does_not_claim() {


Hmm, as mentioned above, I'd prefer if we could add the mempool check based on the flag in do_lsps2_client_server_integration(false). I don't think this needs yet another full test case, as lsps2_client_service_integration provides the same (actually more) coverage here already.

martinsaposnic force-pushed the client-trusts-lsp-2 branch 2 times, most recently from 4927b73 to 27dc7b4 Compare October 29, 2025 19:09

ldk-reviews-bot requested a review from tnull October 29, 2025 19:16

martinsaposnic marked this pull request as draft October 29, 2025 21:05

tnull removed their request for review October 30, 2025 12:01

tnull added this to the 0.7 milestone Nov 13, 2025

tnull mentioned this pull request Nov 13, 2025

LDK Node v0.7 Tracking issue #443

Open

10 tasks

martinsaposnic force-pushed the client-trusts-lsp-2 branch 4 times, most recently from bfeb9c0 to 7835f72 Compare November 13, 2025 14:16

martinsaposnic force-pushed the client-trusts-lsp-2 branch from 7835f72 to 97d9e53 Compare November 13, 2025 14:20

martinsaposnic marked this pull request as ready for review November 13, 2025 14:22

ldk-reviews-bot requested a review from tnull November 13, 2025 14:22

tnull reviewed Nov 14, 2025

View reviewed changes

Support client_trusts_lsp=true on ldk-node #687

Are you sure you want to change the base?

Support client_trusts_lsp=true on ldk-node #687

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

toneloc commented Oct 29, 2025

Uh oh!

martinsaposnic commented Oct 29, 2025

Uh oh!

tnull commented Oct 30, 2025

Uh oh!

toneloc commented Nov 11, 2025

Uh oh!

martinsaposnic commented Nov 13, 2025

Uh oh!

tnull left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

martinsaposnic commented Oct 29, 2025 •

edited

Loading

ldk-reviews-bot commented Oct 29, 2025 •

edited

Loading