-
Notifications
You must be signed in to change notification settings - Fork 418
Support client_trusts_lsp on LSPS2 #3838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Support client_trusts_lsp on LSPS2 #3838
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3838 +/- ##
==========================================
- Coverage 88.84% 88.63% -0.21%
==========================================
Files 175 174 -1
Lines 127760 127827 +67
Branches 127760 127827 +67
==========================================
- Hits 113510 113304 -206
- Misses 11679 12035 +356
+ Partials 2571 2488 -83
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:
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 clone
s. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Let's move this down to the other bitcoin
types.
TrustModel::ClientTrustsLsp { funding_tx_broadcast_safe, htlc_claimed, funding_tx } => { | ||
*funding_tx_broadcast_safe && *htlc_claimed && funding_tx.is_some() | ||
}, | ||
TrustModel::LspTrustsClient => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this always be true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually the method name is confusing. this should be false because in lsp-trusts-client, the broadcast is automatic, so we should return false
to avoid doing a manual broadcast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixup commit changes this but will revert in a future commit, that will also include an e2e test
|
||
fn new(client_trusts_lsp: bool) -> Self { | ||
if client_trusts_lsp { | ||
return TrustModel::ClientTrustsLsp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please avoid these explicit return
s here.
if client_trusts_lsp { | ||
return TrustModel::ClientTrustsLsp { | ||
funding_tx_broadcast_safe: false, | ||
htlc_claimed: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good if we could find a way to avoid these clone
s. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unrelated?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
a038ab6
to
eb5f42f
Compare
This needs a rebase now that #3662 landed. |
8d7da60
to
82a4bc1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if/when you deem this ready for review!
I'm close, I have a half baked functional test. I want to finish that before putting this as ready for review |
44a4813
to
fb259f4
Compare
- improved documentation to explain client_trusts_lsp where appropiate - added a full end to end test that tests the client_trusts_lsp - added a new event BroadcastFundingTransaction so the user can broadcast when necessary - keep refactoring lsps2/service to simplify the code
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't review the integration tests.
lightning/src/ln/channelmanager.rs
Outdated
@@ -11546,6 +11568,15 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ | |||
} | |||
} | |||
|
|||
/// Manually broadcast a transaction using the internal transaction broadcaster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see the point of a method that just calls back into a user-provided interface? The caller in lightning-liquidity should probably just have a reference to the broadcaster.
lightning/src/ln/channelmanager.rs
Outdated
@@ -5767,6 +5775,18 @@ where | |||
self.batch_funding_transaction_generated_intern(temporary_chans, funding_type) | |||
} | |||
|
|||
/// Same as batch_funding_transaction_generated but it does not automatically broadcast the funding transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the same as funding_transaction_generated
? Also, this should be a link and at least link to Event::FundingTxBroadcastSafe
ClientTrustsLsp { | ||
funding_tx_broadcast_safe: bool, | ||
payment_claimed: bool, | ||
funding_tx: Option<Arc<Transaction>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see a reason this should be in an Arc.
@@ -414,6 +500,9 @@ impl OutboundJITChannel { | |||
|
|||
fn payment_forwarded(&mut self) -> Result<Option<ForwardHTLCsAction>, LightningError> { | |||
let action = self.state.payment_forwarded()?; | |||
if action.is_some() { | |||
self.trust_model.set_payment_claimed(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we currently test whether the HTLC that caused us to open the channel was forwarded or just any channel. I imagine someone could get enough in pending HTLCs to get a channel, then route a single sat to themselves, claim that, and get the funding broadcasted (this should probably be explicitly tested).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what I will do here is track an outstanding_opening_fee_msat
(this is the amount of fees that the client promised to pay), and as the PaymentForward
s come, I will substract the skimmed fees from it, until it's <= 0
once it's <= 0, it means that it's now safe for the LSP to proceed and broadcast the funding tx.
I did it this way because the PaymentForward does not tell what htlc was forwarded (I guess that's intentional?), so I cannot tell what HTLC exactly the client is claiming. it just tells me the channel and the skimmed fees.
I have the code ready, I'm improving the tests to cover some edge cases.
@TheBlueMatt please let me know if this makes sense conceptually or if I'd need to do something else
} | ||
|
||
/// Called when the funding transaction is safe to broadcast. | ||
/// This marks the funding_tx_broadcast_safe flag as true for the given user_channel_id. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
funding_tx_broadcast_safe
this isnt a public thing so shouldnt appear in docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will improve the name and docs of this function, but I'm still confused with what you mean by "funding_tx_broadcast_safe is not a public thing"
I'm making it necessary to call funding_tx_broadcast_safe()
BEFORE being able to broadcast the funding tx. is this incorrect?
Ok(()) | ||
} | ||
|
||
/// Called when the funding transaction is safe to broadcast. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a discussion of what "safe to broadcast" means, cause its not actually "safe to broadcast" in the context of LSPS, only in the context of lightning. Should at least link to the lightning
Event
.
7fd8c8c
to
644cec2
Compare
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_broadcast
on 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_applies
from 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_claim
so the client can manually claim the htlc using theclaim_for_hash
.