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.33% 88.45% +0.11%
==========================================
Files 177 177
Lines 131896 132695 +799
Branches 131896 132695 +799
==========================================
+ Hits 116512 117372 +860
+ Misses 12728 12660 -68
- Partials 2656 2663 +7
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?
| @@ -11,6 +11,7 @@ | |||
|
|
|||
| 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?
| let mut payment_queue = core::mem::take(payment_queue); | ||
| payment_queue.add_htlc(htlc); | ||
| *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
lightning/src/routing/router.rs
Outdated
| @@ -1075,7 +1075,7 @@ impl PaymentParameters { | |||
| } | |||
|
|
|||
| /// A struct for configuring parameters for routing the payment. | |||
| #[derive(Clone, Copy)] | |||
| #[derive(Clone, Copy, Debug)] | |||
| @@ -35,7 +35,7 @@ lightning = { version = "0.2.0", path = "../lightning", default-features = false | |||
| lightning-macros = { version = "0.2", path = "../lightning-macros", default-features = false } | |||
| 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! |
|
just pushed a few fixup commits that address the latest comments, and also introduced the whole logic to "track the manual-broadcast flag and pass it to the ChannelMonitor to not broadcast commitment txs" let me know what you think, thanks!! |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
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
force-pushed
the
client-trusts-lsp
branch
from
0850ab9 to
dd46295
Compare
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).
|
also FYI, ldk-node's counterpart of this PR is ready lightningdevkit/ldk-node#572 |
| @@ -6511,6 +6555,8 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP | |||
| prev_holder_commitment_tx, | |||
| }, | |||
| pending_funding: pending_funding.unwrap_or(vec![]), | |||
| is_manual_broadcast: is_manual_broadcast.unwrap_or(false), | |||
| funding_seen_onchain: funding_seen_onchain.unwrap_or(false), | |||
There was a problem hiding this comment.
Shouldn't we set this to true on upgrade, assuming we saw it?
| @@ -2246,6 +2255,12 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> { | |||
| /// close channel with their commitment transaction after a substantial amount of time. Best | |||
| /// may be to contact the other node operator out-of-band to coordinate other options available | |||
| /// to you. | |||
| /// | |||
| /// Note: For channels where the funding transaction is being manually managed (see | |||
There was a problem hiding this comment.
Shouldn't we allow a manual call to override the auto-broadcast logic?
| self.pending_funding.iter().any(|f| f.funding_txid() == *txid) | ||
| { | ||
| log_trace!(logger, "transaction_unconfirmed removed observed funding. resetting funding_seen_onchain"); | ||
| self.funding_seen_onchain = false; |
There was a problem hiding this comment.
This isn't so simple re: splicing, but also I think we shouldn't ever unset this - the important thing is that we know that a funding tx was broadcasted, not that it is currently confirmed.
| @@ -5426,10 +5456,14 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> { | |||
| let should_broadcast = self.should_broadcast_holder_commitment_txn(logger); | |||
| if let Some(payment_hash) = should_broadcast { | |||
| let reason = ClosureReason::HTLCsTimedOut { payment_hash: Some(payment_hash) }; | |||
| let (mut new_outpoints, mut new_outputs) = | |||
| if self.is_manual_broadcast && !self.funding_seen_onchain { | |||
| let _ = self.generate_claimable_outpoints_and_watch_outputs(Some(reason)); | |||
There was a problem hiding this comment.
Not sure why we're making this call? Also, lots of indentation via spaces in this file.
There was a problem hiding this comment.
I need it because of the side effect. generate_claimable_outpoints_and_watch_outputs(Some(reason)) enqueues MonitorEvent::HolderForceClosedWithInfo, which is what actually closes the channel.
If I skip it when is_manual_broadcast == true and the funding_tx has not been seen, the LSP->client channel does not close, even though the client has not claimed the HTLC (this is tested by client_trusts_lsp_partial_fee_does_not_trigger_broadcast and htlc_timeout_before_client_claim_results_in_handling_failed, and they both fail if I skip the call to generate_claimable_outpoints_and_watch_outputs)
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
2 times, most recently
from
dbe7434 to
344b859
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
2 times, most recently
from
8683442 to
5bde2e6
Compare
|
ok, just squashed all the fixups, pulled the channelmonitor changes into a new commit, and then pushed a new fixup addressing the latest comments. let me know what you think thanks! |
| /// | ||
| /// Note that `next_channel_id` is required to be provided. Therefore, the corresponding | ||
| /// Note that `next_channel_id` and `skimmed_fee_msat` are required to be provided. Therefore, the corresponding |
There was a problem hiding this comment.
Here and a few other places, please keep comments/doc strings line-wrapped to 100 chars.
| /// [`crate::ln::channelmanager::ChannelManager::funding_transaction_generated_manual_broadcast`]), | ||
| /// automatic broadcasts are suppressed until the funding transaction has been observed on-chain. | ||
| /// Calling this method overrides that suppression and queues the latest holder commitment | ||
| /// transaction for broadcast even if the funding has not yet been seen on-chain. This is unsafe |
There was a problem hiding this comment.
I wouldn't say "this is unsafe", its not, it will just result in unconfirmable transactions being broadcast or bump events for commitments we can't confirm. We should clarify that and explicitly reference the event imo.
| @@ -5151,6 +5180,14 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> { | |||
| F::Target: FeeEstimator, | |||
| L::Target: Logger, | |||
| { | |||
| for &(_, tx) in txdata.iter() { | |||
| let txid = tx.compute_txid(); | |||
There was a problem hiding this comment.
This is relatively expensive for clients connecting full blocks. Can you move this logic into the existing loop that already computes the txids?
| @@ -1131,6 +1131,9 @@ pub(crate) struct ChannelMonitorImpl<Signer: EcdsaChannelSigner> { | |||
| funding: FundingScope, | |||
| pending_funding: Vec<FundingScope>, | |||
|
|
|||
| is_manual_broadcast: bool, | |||
There was a problem hiding this comment.
These need doc comments explaining how they're set on upgrade, ie under what conditions we can't rely on them being correct.
| @@ -6394,6 +6440,8 @@ impl<'a, 'b, ES: EntropySource, SP: SignerProvider> ReadableArgs<(&'a ES, &'b SP | |||
| (31, channel_parameters, (option: ReadableArgs, None)), | |||
| (32, pending_funding, optional_vec), | |||
| (34, alternative_funding_confirmed, option), | |||
| (36, is_manual_broadcast, option), | |||
There was a problem hiding this comment.
I don't think we want these to be even, right? If we downgrade we just end up with the old behavior where we'll broadcast but shouldn't, which seems correct, at least in the sense that its the old behavior.
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
5bde2e6 to
398b9bc
Compare
…d funding_tx is not broadcasted yet
keep doc/comments to 100 chars max per line adjust channel monitor comments move funding search to filter_block make is_manual_broadcast and funding_seen_onchain odd. its ok to be odd
martinsaposnic
force-pushed
the
client-trusts-lsp
branch
from
398b9bc to
a091350
Compare
|
I just pushed a new small fixup commit addressing the latest comments a single self-hosted CI build is failing for unrelated reasons, I will investigate thanks! |
|
🔔 3rd Reminder Hey @tnull @TheBlueMatt! This PR has been waiting for your review. |
| self.generate_claimable_outpoints_and_watch_outputs(None); | ||
| claimable_outpoints.append(&mut claimables); | ||
| watch_outputs.append(&mut outputs); | ||
| // Avoid broadcasting in manual-broadcast mode until funding is seen on-chain. |
| // Avoid broadcasting in manual-broadcast mode until funding is seen on-chain. | ||
| if !self.is_manual_broadcast || self.funding_seen_onchain { | ||
| let (mut claimables, mut outputs) = | ||
| self.generate_claimable_outpoints_and_watch_outputs(None); |
There was a problem hiding this comment.
Shouldn't this be called no matter the state of is_manual_broadcast/funding_seen_onchain, just ignoring the results?
| if !self.funding_seen_onchain && (txid == self.funding.funding_txid() || | ||
| self.pending_funding.iter().any(|f| f.funding_txid() == txid)) | ||
| { | ||
| self.funding_seen_onchain = true; |
There was a problem hiding this comment.
Should we not replay the spends if the monitor was already closed in this case? Basically if we hit a channel-closing-needed case (where we call generate_claimable_outpoints_and_watch_outputs and then ignore the result) while the funding tx hasn't been seen, and then later the funding tx is seen, we should presumably immediately broadcast the commitment transaction and any spends of it.
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.