Support client_trusts_lsp on LSPS2 #3838
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3838 +/- ##
==========================================
+ Coverage 88.57% 88.61% +0.04%
==========================================
Files 179 179
Lines 134374 134666 +292
Branches 134374 134666 +292
==========================================
+ Hits 119016 119333 +317
+ Misses 12604 12558 -46
- Partials 2754 2775 +21
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:
|
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
2 times, most recently
from
5d8508d to
a038ab6
Compare
|
A few extra concerns: HTLCs routed over 0-conf channels might hit CLTV timeouts if the channel should be confirmed but isn’t yet, so if the user takes forever to claim the payment, then there could be some trouble there. Also, I'm not sure if it would be better to have new possible states to explicitly model the idea of having sent the channel_ready but the funding_tx is not broadcasted yet. Also sharing the trust_model state between 4 of the 5 possible states is not something I'm convinced. I have a few different options for this, but I'm open to comments and suggestions |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
There was a problem hiding this comment.
Thanks for looking into this.
This generally makes sense to me, although we should really work out how we'd deal with operational channels for which we withhold the funding transaction broadcast.
HTLCs routed over 0-conf channels might hit CLTV timeouts if the channel should be > confirmed but isn’t yet, so if the user takes forever to claim the payment, then there
could be some trouble there.
Yes. IMO this means that we need to introduce proper (read: clean API, and tested) support for 'hosted channels', i.e., channels that are operational even though the funding transaction hasn't been confirmed yet. Not sure if @TheBlueMatt has an opinion here?
Also, I'm not sure if it would be better to have new possible states to explicitly model > the idea of having sent the channel_ready but the funding_tx is not broadcasted yet. > Also sharing the trust_model state between 4 of the 5 possible states is not > something I'm convinced. I have a few different options for this, but I'm open to > comments and suggestions
Yeah, as mentioned in the comments, it would def. be preferable if we could avoid the many clones. Also it's overall a lot of boilerplate for just three fields handed through, maybe there is a simpler approach?
| use alloc::string::{String, ToString}; | ||
| use alloc::vec::Vec; | ||
| use bitcoin::Transaction; |
There was a problem hiding this comment.
nit: Let's move this down to the other bitcoin types.
|
|
||
| fn new(client_trusts_lsp: bool) -> Self { | ||
| if client_trusts_lsp { | ||
| return TrustModel::ClientTrustsLsp { |
There was a problem hiding this comment.
nit: Please avoid these explicit returns here.
| if client_trusts_lsp { | ||
| return TrustModel::ClientTrustsLsp { | ||
| funding_tx_broadcast_safe: false, | ||
| htlc_claimed: false, |
There was a problem hiding this comment.
nit: Maybe payment_claimed to align with the event type?
| *self = OutboundJITChannelState::PendingChannelOpen { | ||
| payment_queue, | ||
| opening_fee_msat: *opening_fee_msat, | ||
| trust_model: trust_model.clone(), |
There was a problem hiding this comment.
Would be good if we could find a way to avoid these clones. Given that these are just two bools and the transaction option, I also wonder if it's indeed worth all the boilerplate, or if it might suffice to have these fields live on the state/channel objects directly.
| Ok(()) | ||
| } | ||
|
|
||
| /// Called by ldk-node when the funding transaction is safe to broadcast. |
There was a problem hiding this comment.
Note that in this context we don't know where this will be used, so we shouldn't assume LDK Node is the only consumer of this API.
lightning/src/ln/channelmanager.rs
Outdated
| /// broadcast it manually. | ||
| /// | ||
| /// Used in LSPS2 on a client_trusts_lsp model | ||
| CheckedManualBroadcast(Transaction), |
There was a problem hiding this comment.
nit: Let's move to the second positition here and elsewhere.
lightning/src/ln/channelmanager.rs
Outdated
| /// # Warning | ||
| /// Improper use of this method could lead to channel state inconsistencies. | ||
| /// Ensure the transaction being broadcast is valid and expected by LDK. | ||
| pub fn unsafe_broadcast_transaction(&self, tx: &Transaction) { |
There was a problem hiding this comment.
Hmm, I'm not sure if this would qualify for the unsafe_ prefix and the Warning. It's really just the normal flow, just that we leave broadcasting to the user instead of using the BroadcasterInterface.
There was a problem hiding this comment.
we leave broadcasting to the user instead of using the BroadcasterInterface
actually, I think it would be good to emit an event ClientPaidSoPleaseBroadcastTransactionNow instead of doing it automatically, or even better, reuse the LdkEvent::FundingTxBroadcastSafe, which is literally the event used to let the user know they should manually broadcast a transaction. I will investigate if that's possible
|
|
||
| /// A struct for configuring parameters for routing the payment. | ||
| #[derive(Clone, Copy)] | ||
| #[derive(Clone, Copy, Debug)] |
| bitcoin = { version = "0.32.2", default-features = false } | ||
| futures = { version = "0.3", optional = true } | ||
| esplora-client = { version = "0.12", default-features = false, optional = true } | ||
| esplora-client = { version = "0.11", default-features = false, optional = true } |
There was a problem hiding this comment.
Please don't include unrelated changes here.
|
👋 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. |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
a038ab6 to
eb5f42f
Compare
|
This needs a rebase now that #3662 landed. |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
3 times, most recently
from
8d7da60 to
82a4bc1
Compare
I'm close, I have a half baked functional test. I want to finish that before putting this as ready for review |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
3 times, most recently
from
fb259f4 to
6412945
Compare
|
ok, this should be ready now @tnull all comments are addressed in fixup commits. also I wrote a full end to end test that covers the client_trusts_lsp flow (directly in this repo, not in ldk-node, which was not possible before! 😄 ) . also added some documentation that explains how the client_trusts_lsp flow works. thanks! |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
a091350 to
dd002d6
Compare
just pushed a fixup addressing this comment ^^ linting is failing due to unrelated-to-this-pr reasons let me know what you think, thanks!! |
|
🔔 4th Reminder Hey @tnull @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @tnull @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull @TheBlueMatt! This PR has been waiting for your review. |
| /// `true` when absent during upgrade so holder broadcasts aren't gated unexpectedly. | ||
| funding_seen_onchain: bool, | ||
| /// Tracks whether manual-broadcasting was requested before the funding transaction appeared on-chain. | ||
| manual_broadcast_pending: bool, |
There was a problem hiding this comment.
Hmm, I feel like we can just use holder_tx_signed rather than adding a new bool.
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
dd002d6 to
f70df04
Compare
|
I rebased with main and added this so CI passes diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index 77bdb4599..ee6922369 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -426,6 +426,8 @@ pub const NO_LIQUIDITY_MANAGER: Option<
C = &dyn chain::Filter,
TimeProvider = dyn lightning_liquidity::utils::time::TimeProvider,
TP = &dyn lightning_liquidity::utils::time::TimeProvider,
+ BroadcasterInterface = dyn BroadcasterInterface,
+ T = &dyn BroadcasterInterface,
> + Send
+ Sync,
>, |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
f70df04 to
f9bfa8c
Compare
| // collected. Before that happens, LDK may force-close the not‑yet‑funded channel | ||
| // (for example when a forwarded HTLC nears expiry). Broadcasting funding after a | ||
| // close could then confirm the commitment and trigger unintended on‑chain handling. | ||
| // To avoid this, we check ChannelManager’s view (`is_usable`) before broadcasting. |
There was a problem hiding this comment.
This comment is outdated now, we check is_channel_ready. Can we also change the is_usable variable to avoid confusion now that we check the other flag?
| /// to broadcast the funding transaction yourself in this flow. | ||
| /// | ||
| /// [`set_funding_tx_broadcast_safe`]: Self::set_funding_tx_broadcast_safe | ||
| pub fn store_funding_transaction( |
There was a problem hiding this comment.
I wonder if we also need a way to abort the flow if we get a DiscardFunding event?
There was a problem hiding this comment.
we have a channel_open_abandoned which I'm tempted on using, but that particular method only works before the OpenChannel event is issued.
not sure what to expect or what to communicate if a DiscardFunding event is received after a OpenChannel event is emitted but before the funding_tx is broadcasted. would we need to force close the jit channel too? send a new event so the user force closes? @tnull
There was a problem hiding this comment.
not sure what to expect or what to communicate if a
DiscardFundingevent is received after aOpenChannelevent is emitted but before thefunding_txis broadcasted. would we need to force close the jit channel too? send a new event so the user force closes? @tnull
Hmm, yeah, it's probably fine for now as we'd get DiscardFunding after the channel has been closed. It probably means that we need to retry the channel open, i.e., have to reset via channel_open_failed.
|
It seems this needs rebase now that we landed the LSPS persistence logic, sorry about that. |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
5 times, most recently
from
b3875f2 to
a1eef19
Compare
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
a1eef19 to
8ce4147
Compare
not related to this PR, will confirm |
There was a problem hiding this comment.
I did another pass and I think the changes here look good.
I have yet to take a look at #4109 to fully understand what's the plan there, but IMO this PR should be ready.
Want to note that the generated test changes are very verbose. While looking reasonable, I hope I didn't miss anything there.
|
|
||
| // this is ONLY used on LSPS2 so it says it's not used but it is | ||
| #[allow(dead_code)] | ||
| pub(crate) fn create_service_client_and_payer_nodes<'a, 'b, 'c>( |
There was a problem hiding this comment.
Seems we now have a lot of different variants of these create_service_and_client_nodes helpers. I wonder if we could DRY them up eventually, but doesn't need to happen here.
| use lightning::get_event_msg; | ||
| use lightning::ln::channelmanager::PaymentId; | ||
| use lightning::ln::channelmanager::Retry; | ||
| use lightning::ln::functional_test_utils::create_funding_transaction; |
There was a problem hiding this comment.
nit: All of these imports should be grouped at the module-level.
| setup_test_lsps2_nodes_with_kv_stores(nodes, service_kv_store, client_kv_store) | ||
| } | ||
|
|
||
| fn setup_test_lsps2_nodes_with_payer<'a, 'b, 'c>( |
There was a problem hiding this comment.
Same here, would be good to eventually reduce/DRY up the boilerplate a bit at some point.
|
|
||
| fn get_funding_tx(&self) -> Option<Transaction> { | ||
| match self { | ||
| TrustModel::ClientTrustsLsp { funding_tx, .. } => funding_tx.clone(), |
There was a problem hiding this comment.
There's no need to clone this now, you can just return a reference to the tx.
The feature
lightningdevkit/ldk-node#479
Currently, our LSPS2 service implementation only supports the lsp_trusts_client model, which means that "If the client does not claim the payment, and the funding transaction confirms, then the LSP will have its funds locked in a channel that was not paid for."
On a client_trusts_lsp model, the LSP will NOT broadcast the funding transaction until the client claims the payment.
The plan:
(This plan was validated and acknowledged by @tnull in private). There are differences between the plan and the implementation, but it roughly describes the approach.
LSPS2 Process & Events: These are handled the same way as before. No changes here.
When the OpenChannel event is emitted, ldk-node calls create_channel as usual. The key difference is: If client_trusts_lsp = true, after emitting the OpenChannel event, we start tracking the HTLC that our client will eventually claim.
Funding Transaction Broadcast Logic: The batch_funding_transaction_generated_intern function decides whether the funding transaction should be broadcast automatically. There are two existing funding types:
I will introduce a third type:
With this:
lsps2_service on ldk-node will now interact with lsps2_service on rust-lightning in two new key moments:
Changes:
funding_transaction_generated_manual_broadcaston channel_manager. Uses FundingType::CheckedManualBroadcast, which validates but does not automatically broadcastchannel_needs_manual_broadcast. This is used by ldk-node to know if funding_transaction_generated or funding_transaction_generated_manual_broadcast should be called when FundingGenerationReady event is triggeredstore_funding_transaction. This is used by ldk-node when the funding transaction is created. We need to store it because the broadcast of the funding transaction may be deferred.funding_tx_broadcast_safe. This is used by ldk-node when the FundingTxBroadcastSafe event is triggeredbroadcast_transaction_if_appliesfrom the lsps2/serviceLDK Node integration
In this PR lightningdevkit/ldk-node#572 on ldk-node, you can see that 2 tests are created that demonstrates the funcionality described above.
client_trusts_lsp=true
In the test, receive_via_jit_channel_manual_claim is called, the mempool is checked to assert that the funding transaction was not broadcasted yet (it should not because client_trusts_lsp=true, and the client has not claimed the htlc yet).
Then the client calls
claim_for_hash, and the mempool is checked again, and now the funding transaction should be thereclient_trusts_lsp=false
In the test, receive_via_jit_channel_manual_claim is called, the mempool is checked to assert that the funding transaction was broadcasted (because the LSP trusts the client), even though the client has not claimed the htlc yet. In this case, the LSP was tricked, and it will have its funds locked in a channel that was not paid for.
Side note: for the tests to work I had to create a new
receive_via_jit_channel_manual_claimso the client can manually claim the htlc using theclaim_for_hash.