-
Notifications
You must be signed in to change notification settings - Fork 417
Async send: sender-side #4046
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?
Async send: sender-side #4046
Conversation
👋 Hi! I see this is a draft PR. |
02eb665
to
0ca36b6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4046 +/- ##
==========================================
- Coverage 88.77% 88.59% -0.19%
==========================================
Files 176 175 -1
Lines 129375 129711 +336
Branches 129375 129711 +336
==========================================
+ Hits 114856 114920 +64
- Misses 11919 12262 +343
+ Partials 2600 2529 -71
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:
|
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.
Just a few initial comments.
Awesome to see how you bring everything that has been planned and discussed upfront together in this PR. Great work Val.
// If we expect the HTLCs for this payment to be held at our next-hop counterparty, don't | ||
// retry the payment. In future iterations of this feature, we will send this payment via | ||
// trampoline and the counterparty will retry on our behalf. | ||
if hold_htlcs_at_next_hop { |
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.
And the case where our trampoline-counterparty didn't want to retry, or has some other kind of problem. I think then we ourselves still want to retry with a different peer?
Although it is probably mostly theoretically. In a client<->LSP setup, there isn't any other peer to retry with.
self.abandon_payment_with_reason( | ||
// If the call to `Self::hold_htlc_channels` succeeded, then we are a private node and can | ||
// hold the HTLCs for this payment at our next-hop channel counterparty until the recipient | ||
// comes online. This allows us to go offline after locking in the HTLCs. |
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 prevents private nodes that are always online and also connected to peers that support hold from sending the held_htlc OM directly? Not a big concern I think.
/// Parameters for the reply path to a [`HeldHtlcAvailable`] onion message. | ||
pub enum HeldHtlcReplyPath { | ||
/// The reply path to the [`HeldHtlcAvailable`] message should terminate at our node. | ||
ToUs { |
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.
It's too late now, but I think it would have been okay to just not implement the always-online sender to mostly-offline receiver case yet and focus purely on LSP clients.
let static_invoice = match htlc.source.static_invoice() { | ||
Some(inv) => inv, | ||
None => { | ||
// This is reachable but it means the counterparty is buggy and included a release |
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.
Log?
@@ -10997,6 +11007,10 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ | |||
} | |||
}; | |||
self.fail_holding_cell_htlcs(htlcs_to_fail, msg.channel_id, counterparty_node_id); | |||
for (static_invoice, reply_path) in static_invoices { | |||
let res = self.flow.enqueue_held_htlc_available(&static_invoice, HeldHtlcReplyPath::ToCounterparty { path: reply_path }); | |||
debug_assert!(res.is_ok(), "enqueue_held_htlc_available can only fail for non-async senders"); |
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.
Log in release mode?
// If we are configured to be an announced node, we are expected to be always-online and can | ||
// advertise the htlc_hold feature. | ||
if config.enable_htlc_hold { | ||
features.set_htlc_hold_optional(); |
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 do wonder whether the feature and advertising it is necessary. Another more LSP-oriented approach would be to not advertise anything and configure clients to assume that their channel peer has the feature available. I understand that we want to be generic, but not sure how likely it is to see any setup other than LSP<->client using this.
In upcoming commits, we'll be creating blinded paths during the process of creating a revoke_and_ack message within the Channel struct. These paths will be included in said RAA to be used as reply paths for often-offline senders held_htlc_available messages. Because we hold the per-peer lock corresponding to the Channel while creating this RAA, we can't use our typical approach of calling ChannelManager::get_peers_for_blinded_path to create these blinded paths. The ::get_peers method takes each peer's lock in turn in order to check for usable channels/onion message feature support, and it's not permitted to hold multiple peer state locks at the same time due to the potential for deadlocks (see the debug_sync module). To avoid taking other peer state locks while holding a particular Channel's peer state lock, here we cache the set of peers in the OffersMessageFlow, which is the struct that ultimately creates the blinded paths for the RAA.
As part of supporting sending payments as an often-offline sender, the sender needs to be able to set a flag in their update_add_htlc message indicating that the HTLC should be held until receipt of a release_held_htlc onion message from the often-offline payment recipient. We don't yet ever set this flag, but lay the groundwork by including the field in the update_add struct. See-also <lightning/bolts#989>
As part of supporting sending payments as an often-offline sender, the often-offline sender's channel counterparty needs to advertise a feature bit indicating that they support holding onto the sender's HTLC until they receive a release_held_htlc onion message from the recipient indicating that they are online and ready to receive the payment. See-also <lightning/bolts#989> We don't yet advertise support of this feature, so here we also disconnect from peers that try to send us hold HTLCs.
As part of supporting sending payments as an often-offline sender, the often-offline sender's channel counterparty needs to advertise a feature bit indicating that they support holding onto the sender's HTLC until they receive a release_held_htlc onion message from the recipient indicating that they are online and ready to receive the payment. Here we add a config flag to turn on advertising this feature, and fail back hold_htlcs if this config flag is not set. See-also <lightning/bolts#989>
As part of supporting sending payments as an often-offline sender, the sender needs to send held_htlc_available onion messages where the reply path terminates at their always-online channel counterparty that is holding the HTLC until the recipient comes online. That way when the recipient sends release_held_htlc, the sender's counterparty will receive that message. To accomplish this, the sender's always-online counterparty includes said reply path in the revoke_and_ack message corresponding to the held HTLC. Here we add support for this field, though we don't set it yet. We also had to tweak the ser macros for this because impl_writeable_msg had never had to write a Vec in a message TLV field before.
Makes the next commit cleaner.
As part of supporting sending payments as an often-offline sender, the sender's always-online channel counterparty needs to hold onto the sender's HTLC until they receive a release_held_htlc onion message from the often-offline recipient. Here we implement storing these held HTLCs in the existing ChannelManager::pending_intercepted_htlcs map. We want to move in the direction of obviating the need to persistence the ChannelManager entirely, so it doesn't really make sense to add a whole new map for these HTLCs.
As part of supporting sending payments as an often-offline sender, the sender needs to send held_htlc_available onion messages such that the reply path to the message terminates at their always-online channel counterparty that is holding the HTLC. That way when the recipient responds with release_held_htlc, the sender's counterparty will receive that message. Here we add a method for creating said reply path, which will be used in the next commit.
As part of supporting sending payments as an often-offline sender, the sender needs to send held_htlc_available onion messages such that the reply path to the message terminates at their always-online channel counterparty that is holding the HTLC. That way when the recipient responds with release_held_htlc, the sender's counterparty will receive that message. Here the counterparty starts including said reply paths in the revoke_and_ack message destined for the sender, so the sender can use these paths in subsequent held_htlc_available messages. We put the paths in the RAA to ensure the sender receives the blinded paths, because failure to deliver the paths means the HTLC will timeout/fail.
As part of supporting sending payments as an often-offline sender, the sender's always-online channel counterparty needs to hold onto the sender's HTLC until they receive a release_held_htlc onion message from the often-offline recipient. Here we implement forwarding these held HTLCs upon receipt of the release message from the recipient.
Useful to filter for channel peers that support a specific feature, in this case the hold_htlc feature, in upcoming commits.
As part of supporting sending payments as an often-offline sender, the sender needs to be able to set a flag in their update_add_htlc message indicating that the HTLC should be held until receipt of a release_held_htlc onion message from the often-offline payment recipient. We don't yet ever set this flag, but lay the groundwork by including the field in the HTLCSource::OutboundRoute enum variant. See-also <lightning/bolts#989>
As part of supporting sending payments as an often-offline sender, the sender needs to be able to set a flag in their update_add_htlc message indicating that the HTLC should be held until receipt of a release_held_htlc onion message from the often-offline payment recipient. We don't yet ever set this flag, but lay the groundwork by including the parameter in the pay_route method. See-also <lightning/bolts#989>
As part of supporting sending payments as an often-offline sender, the sender needs to be able to set a flag in their update_add_htlc message indicating that the HTLC should be held until receipt of a release_held_htlc onion message from the often-offline payment recipient. We don't yet ever set this flag, but lay the groundwork by including the field in the outbound payment variant for static invoices. We also add a helper method to gather channels for nodes that advertise support for the hold_htlc feature, which will be used in the next commit. See-also <lightning/bolts#989>
As part of supporting sending payments as an often-offline sender, the sender needs to be able to set a flag in their update_add_htlc message indicating that the HTLC should be held until receipt of a release_held_htlc onion message from the often-offline payment recipient. The prior commits laid groundwork to finally set the flag here in this commit. See-also <lightning/bolts#989>
As part of supporting sending payments as an often-offline sender, the sender needs to send held_htlc_available onion messages such that the reply path to the message terminates at their always-online channel counterparty that is holding the HTLC. That way when the recipient responds with release_held_htlc, the sender's counterparty will receive that message. Here we lay some groundwork for using a counterparty-created reply path when sending held_htlc_available as an async sender in the next commit.
As part of supporting sending payments as an often-offline sender, the sender needs to send held_htlc_available onion messages such that the reply path to the message terminates at their always-online channel counterparty that is holding the HTLC. That way when the recipient responds with release_held_htlc, the sender's counterparty will receive that message. After laying groundwork over some past commits, here we as an async sender send held_htlc_available messages using reply paths created by our always-online channel counterparty.
Now that we support the feature of sending payments as an often-offline sender to an often-offline recipient, including the sender's LSP-side, we can start conditionally advertising the feature bit to other nodes on the network.
0ca36b6
to
e1fa5eb
Compare
|
||
let recipient_id = vec![42; 32]; | ||
let inv_server_paths = | ||
invoice_server.node.blinded_paths_for_async_recipient(recipient_id.clone(), None).unwrap(); |
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.
Pre-existing by now, but what is the point of the caller specifying an expiry here? What should they set it to if not None, and how does this help them?
// If we have an announced channel, we are a node that is expected to be always-online and | ||
// shouldn't be relying on channel counterparties to hold onto our HTLCs for us while | ||
// waiting for the payment recipient to come online. | ||
if channel.context().should_announce() { | ||
any_announced_channels.store(true, Ordering::Relaxed); | ||
} | ||
if any_announced_channels.load(Ordering::Relaxed) { | ||
return 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.
wondering if this check is necessary since above in should_send_sync
we are strictly checking that this a node that only set private channels.
lightning/src/ln/channelmanager.rs
Outdated
let reply_path = HeldHtlcReplyPath::ToUs { | ||
payment_id, | ||
self.get_peers_for_blinded_path(), | ||
); | ||
peers: self.get_peers_for_blinded_path(), | ||
}; | ||
let enqueue_held_htlc_available_res = | ||
self.flow.enqueue_held_htlc_available(invoice, reply_path); |
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.
Not sure if this a too rare case but if we are an often-offline sender with an LSP that doesn't support hltc_hold
it will end up in this path and sending a held_htlc_available
with a reply path back for the release_held_htlc
back to us. Feels like this a case where it will most likely fail given that it would require both async sender and receiver to be online at same time. Wondering if it should instead fail the payment?
@@ -1131,14 +1151,19 @@ where | |||
/// [`ReleaseHeldHtlc`]: crate::onion_message::async_payments::ReleaseHeldHtlc | |||
/// [`supports_onion_messages`]: crate::types::features::Features::supports_onion_messages | |||
pub fn enqueue_held_htlc_available( | |||
&self, invoice: &StaticInvoice, payment_id: PaymentId, peers: Vec<MessageForwardNode>, | |||
&self, invoice: &StaticInvoice, reply_path_params: HeldHtlcReplyPath, |
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: needs doc update - no longer takes peers param directly
Implements the sender-side of sending payments as an often-offline sender to an often-offline recipient.
Partially addresses #2298
Based on #4044, #4045