Include invoice requests in async payment onions #3207

valentinewallace · 2024-07-26T21:16:10Z

Title. Implements the sending-side of the final commit of lightning/bolts#1149.

Address followup Support paying static invoices #3140 (comment)
Address followup Support paying static invoices #3140 (comment)

Based on #3140.

codecov · 2024-07-26T21:25:49Z

Codecov Report

Attention: Patch coverage is 87.50000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (cdd1298) to head (b5ccb98).

Files with missing lines	Patch %	Lines
lightning/src/ln/outbound_payment.rs	94.54%	3 Missing ⚠️
lightning/src/ln/channelmanager.rs	60.00%	2 Missing ⚠️
lightning/src/ln/peer_handler.rs	0.00%	2 Missing ⚠️
lightning/src/onion_message/functional_tests.rs	0.00%	2 Missing ⚠️
lightning/src/routing/router.rs	0.00%	2 Missing ⚠️
lightning/src/events/mod.rs	0.00%	1 Missing ⚠️
lightning/src/ln/msgs.rs	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3207      +/-   ##
==========================================
- Coverage   89.64%   89.62%   -0.02%     
==========================================
  Files         126      126              
  Lines      102676   102705      +29     
  Branches   102676   102705      +29     
==========================================
+ Hits        92043    92053      +10     
- Misses       7909     7926      +17     
- Partials     2724     2726       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2024-09-17T19:39:03Z

Needs rebase.

lightning/src/ln/outbound_payment.rs

lightning/src/events/mod.rs

lightning/src/ln/msgs.rs

TheBlueMatt · 2024-10-02T16:34:51Z

lightning/src/ln/outbound_payment.rs

+		match outbounds.entry(payment_id) {
+			hash_map::Entry::Occupied(entry) => match entry.remove() {
+				PendingOutboundPayment::InvoiceReceived { .. } => {
+					outbounds.insert(payment_id, retryable_payment);


Please use the entry API to avoid rehashing here.

Hmm, I did this to avoid cloning the invoice request. It seemed necessary to fully move the payment out of the map to do so. Or do you mean avoid removeing in the ::InvoiceReceived case, and keep it in the ::StaticInvoiceReceived case?

You could split it, yea, but you could also take the invoice_request before overwriting the field?

Check out the latest version! I ended up splitting it, because it looks like you can't just take since it's also used later in pay_route_internal.

lightning/src/events/mod.rs

jkczyz · 2024-10-23T18:20:07Z

lightning/src/ln/outbound_payment.rs

+						invoice_request:
+							retryable_invoice_request
+							.take()
+							.ok_or(Bolt12PaymentError::UnexpectedInvoice)?
+							.invoice_request,


This may be None if the InvoiceRequest was already retried, so we'll need to have some other means of tracking whether or not to retry. I think this was mentioned in an earlier PR, so perhaps it will be handled in a follow-up?

Ah yeah, probably will do that in a follow-up. Looks like it's tracked here #2298. Thanks for the reminder!

lightning/src/ln/outbound_payment.rs

valentinewallace · 2024-10-24T17:49:06Z

Rebased due to conflicts, then pushed an additional typo fix:

diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index bc7bbb641..823c5e2c2 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -566,7 +566,7 @@ pub enum PaymentFailureReason {
        /// routes - we tried the payment over a few routes but were not able to find any further
        /// candidate routes beyond those.
        ///
-       /// Also used for [`BlindedPathCreationFailed`] down downgrading to versions prior to 0.0.124.
+       /// Also used for [`BlindedPathCreationFailed`] when downgrading to versions prior to 0.0.124.
        RouteNotFound,
        /// This error should generally never happen. This likely means that there is a problem with
        /// your router.

lightning/src/ln/outbound_payment.rs

lightning/src/events/mod.rs

jkczyz

LGTM other than some minor comments.

jkczyz · 2024-10-29T20:00:53Z

lightning/src/routing/router.rs

@@ -613,8 +613,9 @@ impl RouteParameters {
 		&mut self, recipient_onion: &RecipientOnionFields, is_keysend: bool, best_block_height: u32
 	) -> Result<(), ()> {
 		let keysend_preimage_opt = is_keysend.then(|| PaymentPreimage([42; 32]));
+		// TODO: no way to account for the invoice request here yet


Can't we pass it to RouteParameters::set_max_path_length as an Option parameter? Doesn't look like it is ever called internally, though. Is this mainly for use outside of ChannelManager?

We can though we'll need some way to clarify in the API that it should only be passed in for payments to a static invoice, which isn't part of LDK's public API yet. Not sure if it's worth addressing now.

Doesn't look like it is ever called internally, though. Is this mainly for use outside of ChannelManager?

Yep, just seems like a useful util on its own IIRC.

jkczyz · 2024-10-29T20:16:24Z

lightning/src/ln/msgs.rs

 			} => {
 				// We need to update [`ln::outbound_payment::RecipientOnionFields::with_custom_tlvs`]
 				// to reject any reserved types in the experimental range if new ones are ever
 				// standardized.
+				let invoice_request_tlv = invoice_request.map(|invreq| (77_777, invreq.encode())); // TODO: update TLV type once the async payments spec is merged


Will this type eventually be less than 2^16? If so, should we just pick something there and move this to _encode_varint_length_prefixed_tlv instead of using custom_tlvs?

My point of reference is mainly that eclair's approach has been to use experimental TLVs until the corresponding BOLT is close(r) to merge, e.g. for their experimental trampoline features. It does seem a little bit safer and might allow us to end up with a lower type since there wouldn't be risk of overlap with another feature?

TheBlueMatt

LGTM. Pretty annoyed about copying the invoice requests around when sending a payment, but don't see an obvious way to fix it.

lightning/src/ln/outbound_payment.rs

jkczyz

LGTM. Good to squash whenever.

RouteNotFound did not fit here because that error is reserved for failing to find a route for a payment, whereas here we are failing to create a blinded path back to ourselves..

Since we started using this error in send_payment_for_bolt12_invoice, this error type is no longer only used on retry but also on initial send.

"Release" is overloaded in the trait's release_pending_messages method, since the latter releases pending async payments onion messages to the peer manager, vs the release_held_htlc method handles the release_held_htlc onion message by attempting to send an HTLC to the recipient.

The method doesn't actually use its &self parameter, and this makes it more obvious that we aren't going to deadlock by calling the method if the outbound_payments lock is already acquired.

Per BOLTs PR 1149, when paying a static invoice we need to include our original invoice request in the HTLC onion since the recipient wouldn't have received it previously. We use an experimental TLV type for this new onion payload field, since the spec is still not merged in the BOLTs.

Add a new invoice request parameter to onion_utils::build_onion_payloads. As of this commit it will always be passed in as None, to be updated in future commits. Per BOLTs PR 1149, when paying a static invoice we need to include our original invoice request in the HTLC onion since the recipient wouldn't have received it previously.

Add a new invoice request parameter to onion_utils::create_payment_onion. As of this commit it will always be passed in as None, to be updated in future commits. Per BOLTs PR 1149, when paying a static invoice we need to include our original invoice request in the HTLC onion since the recipient wouldn't have received it previously.

Add a new invoice request parameter to outbound_payments and channelmanager send-to-route internal utils. As of this commit the invreq will always be passed in as None, to be updated in future commits. Per BOLTs PR 1149, when paying a static invoice we need to include our original invoice request in the HTLC onion since the recipient wouldn't have received it previously.

When transitioning outbound payments from AwaitingInvoice to StaticInvoiceReceived, include the invreq in the new state's outbound payment storage for future inclusion in an async payment onion. Per BOLTs PR 1149, when paying a static invoice we need to include our original invoice request in the HTLC onion since the recipient wouldn't have received it previously.

Past commits have set us up to include invoice requests in outbound async payment onions. Here we actually pull the invoice request from where it's stored in outbound_payments and pass it into the correct utility for inclusion in the onion on initial send. Per BOLTs PR 1149, when paying a static invoice we need to include our original invoice request in the HTLC onion since the recipient wouldn't have received it previously.

While in the last commit we began including invoice requests in async payment onions on initial send, further work is needed to include them on retry. Here we begin storing invreqs in our retry data, and pass them along for inclusion in the onion on payment retry. Per BOLTs PR 1149, when paying a static invoice we need to include our original invoice request in the HTLC onion since the recipient wouldn't have received it previously.

Async payments include the original invoice request in the payment onion. Since invreqs may include blinded paths, it's important to factor them into our max path length calculations since they may take up a significant portion of the 1300-byte onion.

valentinewallace · 2024-10-30T20:20:16Z

I went ahead and squashed, the diff prior was:

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index c68209e8d..c3fde629a 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -954,7 +954,7 @@ impl OutboundPayments {
 		let onion_session_privs = match outbounds.entry(payment_id) {
 			hash_map::Entry::Occupied(entry) => match entry.get() {
 				PendingOutboundPayment::InvoiceReceived { .. } => {
-					let (retryable_payment, onion_session_privs) = self.create_pending_payment(
+					let (retryable_payment, onion_session_privs) = Self::create_pending_payment(
 						payment_hash, recipient_onion.clone(), keysend_preimage, None, &route,
 						Some(retry_strategy), payment_params, entropy_source, best_block_height
 					);
@@ -965,7 +965,7 @@ impl OutboundPayments {
 					let invreq = if let PendingOutboundPayment::StaticInvoiceReceived { invoice_request, .. } = entry.remove() {
 						invoice_request
 					} else { unreachable!() };
-					let (retryable_payment, onion_session_privs) = self.create_pending_payment(
+					let (retryable_payment, onion_session_privs) = Self::create_pending_payment(
 						payment_hash, recipient_onion.clone(), keysend_preimage, Some(invreq), &route,
 						Some(retry_strategy), payment_params, entropy_source, best_block_height
 					);
@@ -1572,7 +1572,7 @@ impl OutboundPayments {
 		match pending_outbounds.entry(payment_id) {
 			hash_map::Entry::Occupied(_) => Err(PaymentSendFailure::DuplicatePayment),
 			hash_map::Entry::Vacant(entry) => {
-				let (payment, onion_session_privs) = self.create_pending_payment(
+				let (payment, onion_session_privs) = Self::create_pending_payment(
 					payment_hash, recipient_onion, keysend_preimage, None, route, retry_strategy,
 					payment_params, entropy_source, best_block_height
 				);
@@ -1583,7 +1583,7 @@ impl OutboundPayments {
 	}
 
 	fn create_pending_payment<ES: Deref>(
-		&self, payment_hash: PaymentHash, recipient_onion: RecipientOnionFields,
+		payment_hash: PaymentHash, recipient_onion: RecipientOnionFields,
 		keysend_preimage: Option<PaymentPreimage>, invoice_request: Option<InvoiceRequest>,
 		route: &Route, retry_strategy: Option<Retry>, payment_params: Option<PaymentParameters>,
 		entropy_source: &ES, best_block_height: u32
@@ -2276,8 +2276,8 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
 		(2, retry_strategy, required),
 		(4, max_total_routing_fee_msat, option),
 	},
-	// Added in 0.0.125. Prior versions will drop these outbounds on downgrade, which is safe because
-	// no HTLCs are in-flight.
+	// Added in 0.1. Prior versions will drop these outbounds on downgrade, which is safe because no
+	// HTLCs are in-flight.
 	(9, StaticInvoiceReceived) => {
 		(0, payment_hash, required),
 		(2, keysend_preimage, required),

I also removed the links to lightning/bolts#1149 in the commit messages because I realized they were majorly cluttering up that PR.

TheBlueMatt

Thanks!

valentinewallace added the blocked on dependent pr label Jul 26, 2024

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from dca3ac9 to f02d149 Compare July 31, 2024 17:53

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from f02d149 to 6c56443 Compare August 20, 2024 20:29

valentinewallace force-pushed the 2024-07-invreq-in-onion branch 2 times, most recently from c202e04 to e3ae4c1 Compare September 3, 2024 21:28

valentinewallace mentioned this pull request Sep 6, 2024

Support paying static invoices #3140

Merged

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from e3ae4c1 to 4d0da54 Compare September 12, 2024 16:25

jkczyz self-requested a review September 13, 2024 20:59

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from 4d0da54 to 1350979 Compare September 18, 2024 14:38

valentinewallace removed the blocked on dependent pr label Sep 18, 2024

valentinewallace marked this pull request as ready for review September 18, 2024 14:38

valentinewallace commented Sep 18, 2024

View reviewed changes

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from 1350979 to b5ccb98 Compare September 18, 2024 15:33

TheBlueMatt reviewed Oct 2, 2024

View reviewed changes

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from b5ccb98 to 568decf Compare October 21, 2024 22:15

vincenzopalazzo mentioned this pull request Oct 22, 2024

docs: add review club 3207 lightningdevkit/ldk-review-club#41

Merged

jkczyz reviewed Oct 23, 2024

View reviewed changes

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from 568decf to 9e316c7 Compare October 24, 2024 01:30

Fix unused warning in fuzz.

cdc0c3b

valentinewallace force-pushed the 2024-07-invreq-in-onion branch 2 times, most recently from 486beb9 to ed8cc0c Compare October 24, 2024 17:47

jkczyz reviewed Oct 24, 2024

View reviewed changes

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

lightning/src/events/mod.rs Show resolved Hide resolved

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from ed8cc0c to c56d8ef Compare October 24, 2024 22:23

jkczyz reviewed Oct 29, 2024

View reviewed changes

TheBlueMatt reviewed Oct 29, 2024

View reviewed changes

lightning/src/ln/outbound_payment.rs Outdated Show resolved Hide resolved

lightning/src/ln/outbound_payment.rs Show resolved Hide resolved

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from c56d8ef to 47d82a8 Compare October 30, 2024 17:10

jkczyz reviewed Oct 30, 2024

View reviewed changes

valentinewallace added 13 commits October 30, 2024 16:13

Add new PaymentFailureReason::BlindedPathCreationFailed

3945bf8

RouteNotFound did not fit here because that error is reserved for failing to find a route for a payment, whereas here we are failing to create a blinded path back to ourselves..

Update docs for PaymentFailureReason::RouteNotFound

88d689a

Since we started using this error in send_payment_for_bolt12_invoice, this error type is no longer only used on retry but also on initial send.

Stop taking &self in outbound_payments' create_inbound_payment

639446a

The method doesn't actually use its &self parameter, and this makes it more obvious that we aren't going to deadlock by calling the method if the outbound_payments lock is already acquired.

Fix outdated comment on PendingOutboundPayment::StaticInvoiceReceived

240dd0a

valentinewallace force-pushed the 2024-07-invreq-in-onion branch from 47d82a8 to 240dd0a Compare October 30, 2024 20:18

jkczyz approved these changes Oct 30, 2024

View reviewed changes

TheBlueMatt approved these changes Nov 1, 2024

View reviewed changes

TheBlueMatt merged commit a130bd6 into lightningdevkit:main Nov 1, 2024
16 of 18 checks passed

sbddesign mentioned this pull request Nov 8, 2024

Bitcoin Socratic Seminar #34 Ideas atlantabitdevs/atlbit.dev#62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include invoice requests in async payment onions #3207

Include invoice requests in async payment onions #3207

valentinewallace commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

TheBlueMatt commented Sep 17, 2024

TheBlueMatt Oct 2, 2024

valentinewallace Oct 21, 2024

TheBlueMatt Oct 23, 2024

valentinewallace Oct 24, 2024 •

edited

Loading

jkczyz Oct 23, 2024

valentinewallace Oct 24, 2024

valentinewallace commented Oct 24, 2024

jkczyz left a comment

jkczyz Oct 29, 2024

valentinewallace Oct 30, 2024

jkczyz Oct 29, 2024

valentinewallace Oct 30, 2024

TheBlueMatt left a comment

jkczyz left a comment

valentinewallace commented Oct 30, 2024

TheBlueMatt left a comment

Include invoice requests in async payment onions #3207

Include invoice requests in async payment onions #3207

Conversation

valentinewallace commented Jul 26, 2024 • edited Loading

codecov bot commented Jul 26, 2024 • edited Loading

Codecov Report

TheBlueMatt commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Oct 24, 2024

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

valentinewallace commented Oct 30, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

valentinewallace commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

valentinewallace Oct 24, 2024 •

edited

Loading