Support receiving async payments #3440

valentinewallace · 2024-12-04T19:23:19Z

Finishes support for receiving async payments.

The last piece after this for the async-receive side is support for being the async receiver's LSP and serving invoices on their behalf.

Partially addresses #2298

Based on ~~#3408~~, ~~#3517~~
~~Also based on #2933, it enables the use of one of the test utils due to more uniform handling of HTLC receive failures.~~

valentinewallace · 2024-12-04T19:25:42Z

This still needs some work and tests but I opened it in case it's useful for reviewing #3408.

valentinewallace · 2025-01-27T19:40:54Z

lightning/src/ln/async_payments_tests.rs

 use bitcoin::secp256k1;
 use bitcoin::secp256k1::Secp256k1;

 use core::convert::Infallible;
 use core::time::Duration;

+// Goes through the async receive onion message flow, returning the final release_held_htlc OM.


For reviewers: the last commit here is rather large here but it's all tests and test util changes. Let me know if it would be helpful to break it up!

My text editor removes trailing whitespace automatically so let's make the removal official.

Store an absolute expiry in blinded message contexts for inbound async payments. Without this, anyone with the path corresponding to this context is able to trivially ask if we're online forever.

If we receive a message that an HTLC is being held upstream for us, send a reply onion message back releasing it since we are online to receive the corresponding payment.

As part of receiving an async payment, we need to verify the sender's original invoice request. Therefore, add support for parsing the invreq contained in the onion and storing it in PendingHTLCForwards to prepare for when we add this verification in an upcoming commit. The invreq also needs to be bubbled up for inclusion in the PaymentClaimable event's PaymentPurpose.

jkczyz

Looks good. Need to review the tests still.

lightning/src/events/mod.rs

TheBlueMatt

I only skimmed the tests but they LGTM

lightning/src/events/mod.rs

valentinewallace · 2025-02-06T17:55:18Z

Added a comment:

diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index 46304b1ae..0b7673158 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -1867,6 +1867,9 @@ impl MaybeReadable for Event {
                                                (11, payment_context, option),
                                                (13, payment_id, option),
                                        });
+                                       // We can pass in `None` for the invoice request parameter here because if this is an
+                                       // offer payment we will only ever have written a `Bolt12OfferContext` (which already
+                                       // contains the invreq), never an `AsyncBolt12OfferContext`.
                                        let purpose = match payment_secret {
                                                Some(secret) => PaymentPurpose::from_parts(payment_preimage, secret, payment_context, None)
                                                        .map_err(|()| msgs::DecodeError::InvalidValue)?,

jkczyz · 2025-02-06T21:54:16Z

lightning/src/ln/functional_test_utils.rs

-				assert_eq!(events_2.len(), 2);
+				// If we successfully decode the HTLC onion but then fail later in
+				// process_pending_htlc_forwards, then we'll queue the failure and generate a new
+				// `ProcessPendingHTLCForwards` event. If we fail during the process of decoding the HTLC,
+				// we'll fail it immediately with no intermediate forwarding event.
+				assert!(events_2.len() == 1 || events_2.len() == 2);


Wouldn't this pass when two events is expected but a regression results in only one? Is there another way to assert that would avoid this? Maybe through what's in expected_failure or in how it set?

IIRC I couldn't find a way to tell with the current code. This would be fixed if #1302 (comment) (or possibly #3561) were addressed, though. There really shouldn't be 1 event ever except for the bug documented in the former link.

After a lot of setup in prior commits, here we finally finish support for receiving HTLCs paid to static BOLT 12 invoices. It amounts to verifying the invoice request contained within the onion and generating the right PaymentPurpose for the claimable event.

Previously, this test util did not account for config overrides supplied at node creation time. Uncovered because it caused test nodes to be unable to forward over private channels created with this util because that is not the default.

valentinewallace · 2025-02-07T20:40:23Z

Refactored from_parts a bit:

diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index 0b7673158..bbaaca4e8 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -28,7 +28,6 @@ use crate::ln::msgs;
 use crate::ln::types::ChannelId;
 use crate::types::payment::{PaymentPreimage, PaymentHash, PaymentSecret};
 use crate::offers::invoice::Bolt12Invoice;
-use crate::offers::invoice_request::VerifiedInvoiceRequest;
 use crate::onion_message::messenger::Responder;
 use crate::routing::gossip::NetworkUpdate;
 use crate::routing::router::{BlindedTail, Path, RouteHop, RouteParameters};
@@ -179,9 +178,10 @@ impl PaymentPurpose {
 		}
 	}
 
+	/// Errors when provided an `AsyncBolt12OfferContext`, see below.
 	pub(crate) fn from_parts(
 		payment_preimage: Option<PaymentPreimage>, payment_secret: PaymentSecret,
-		payment_context: Option<PaymentContext>, invreq: Option<VerifiedInvoiceRequest>,
+		payment_context: Option<PaymentContext>
 	) -> Result<Self, ()> {
 		match payment_context {
 			None => {
@@ -205,16 +205,10 @@ impl PaymentPurpose {
 				})
 			},
 			Some(PaymentContext::AsyncBolt12Offer(_)) => {
-				let invoice_request = invreq.ok_or(())?;
-				if payment_preimage.is_none() { return Err(()) }
-				Ok(PaymentPurpose::Bolt12OfferPayment {
-					payment_preimage,
-					payment_secret,
-					payment_context: Bolt12OfferContext {
-						offer_id: invoice_request.offer_id,
-						invoice_request: invoice_request.fields(),
-					},
-				})
+				// Callers are expected to convert from `AsyncBolt12OfferContext` to `Bolt12OfferContext`
+				// using the invoice request provided in the payment onion prior to calling this method.
+				debug_assert!(false);
+				Err(())
 			}
 		}
 	}
@@ -1867,11 +1861,8 @@ impl MaybeReadable for Event {
 						(11, payment_context, option),
 						(13, payment_id, option),
 					});
-					// We can pass in `None` for the invoice request parameter here because if this is an
-					// offer payment we will only ever have written a `Bolt12OfferContext` (which already
-					// contains the invreq), never an `AsyncBolt12OfferContext`.
 					let purpose = match payment_secret {
-						Some(secret) => PaymentPurpose::from_parts(payment_preimage, secret, payment_context, None)
+						Some(secret) => PaymentPurpose::from_parts(payment_preimage, secret, payment_context)
 							.map_err(|()| msgs::DecodeError::InvalidValue)?,
 						None if payment_preimage.is_some() => PaymentPurpose::SpontaneousPayment(payment_preimage.unwrap()),
 						None => return Err(msgs::DecodeError::InvalidValue),
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index cc7f4ad4f..b4fb5312f 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -6265,7 +6265,6 @@ where
 											payment_preimage,
 											payment_data.payment_secret,
 											payment_context,
-											None,
 										) {
 											Ok(purpose) => purpose,
 											Err(()) => {
@@ -6303,9 +6302,13 @@ where
 													fail_htlc!(claimable_htlc, payment_hash);
 												}
 											};
+											let payment_purpose_context = PaymentContext::Bolt12Offer(Bolt12OfferContext {
+												offer_id: verified_invreq.offer_id,
+												invoice_request: verified_invreq.fields(),
+											});
 											match events::PaymentPurpose::from_parts(
-												Some(keysend_preimage), payment_data.payment_secret, payment_context,
-												Some(verified_invreq),
+												Some(keysend_preimage), payment_data.payment_secret,
+												Some(payment_purpose_context),
 											) {
 												Ok(purpose) => purpose,
 												Err(()) => {

jkczyz

LGTM

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2025-02-07T23:28:53Z

Feel free to squash.

In the previous commit we completed support for async receive from an always-online sender to an often-offline receiver, minus support for acting as the async receiver's always-online channel counterparty.

valentinewallace · 2025-02-07T23:38:30Z

Squashed and had to push one more time to fix CI:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 9c4367a5b..a9d934056 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4693,7 +4693,7 @@ where
                self.pending_outbound_payments.test_add_new_pending_payment(payment_hash, recipient_onion, payment_id, route, None, &self.entropy_source, best_block_height)
        }

-       #[cfg(test)]
+       #[cfg(all(test, async_payments))]
        pub(crate) fn test_modify_pending_payment<Fn>(
                &self, payment_id: &PaymentId, mut callback: Fn
        ) where Fn: FnMut(&mut PendingOutboundPayment) {

TheBlueMatt · 2025-02-08T21:37:03Z

lightning/src/ln/async_payments_tests.rs

-	let htlc_updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
-	assert_eq!(htlc_updates.update_add_htlcs.len(), 1);
+	let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(events.len(), 1);


For this kind of test it would be really nice to check that we didn't release the HTLC prior to handling the onion message above.

valentinewallace mentioned this pull request Dec 4, 2024

Add static invoice creation utils to ChannelManager #3408

Merged

valentinewallace mentioned this pull request Dec 4, 2024

Async Payments #2298

Open

26 tasks

valentinewallace mentioned this pull request Jan 9, 2025

Async receive prefactor #3517

Merged

valentinewallace force-pushed the 2024-12-async-payment-receive branch 2 times, most recently from 904adf6 to 6f7fcba Compare January 9, 2025 20:50

valentinewallace mentioned this pull request Jan 15, 2025

Fix outbound payments memory leak on buggy router #3531

Merged

valentinewallace force-pushed the 2024-12-async-payment-receive branch from 6f7fcba to f2ac259 Compare January 17, 2025 20:12

valentinewallace force-pushed the 2024-12-async-payment-receive branch 2 times, most recently from 9262e08 to f6370df Compare January 27, 2025 19:19

valentinewallace marked this pull request as ready for review January 27, 2025 19:39

valentinewallace commented Jan 27, 2025

View reviewed changes

valentinewallace added 4 commits January 30, 2025 13:42

Remove trailing whitespace in events/mod.rs

065e300

My text editor removes trailing whitespace automatically so let's make the removal official.

Support expiring async payments blinded message paths

ffef1c3

Store an absolute expiry in blinded message contexts for inbound async payments. Without this, anyone with the path corresponding to this context is able to trivially ask if we're online forever.

Release async payment HTLCs held upstream via OM

6448a0d

If we receive a message that an HTLC is being held upstream for us, send a reply onion message back releasing it since we are online to receive the corresponding payment.

valentinewallace force-pushed the 2024-12-async-payment-receive branch from f6370df to c21fea2 Compare January 30, 2025 18:49

valentinewallace added the weekly goal Someone wants to land this this week label Feb 3, 2025

jkczyz reviewed Feb 5, 2025

View reviewed changes

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

TheBlueMatt reviewed Feb 5, 2025

View reviewed changes

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

valentinewallace force-pushed the 2024-12-async-payment-receive branch from c21fea2 to 4b929c3 Compare February 6, 2025 17:54

jkczyz reviewed Feb 6, 2025

View reviewed changes

valentinewallace added 2 commits February 7, 2025 12:38

valentinewallace force-pushed the 2024-12-async-payment-receive branch from 4b929c3 to c5bd26f Compare February 7, 2025 20:39

jkczyz previously approved these changes Feb 7, 2025

View reviewed changes

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

valentinewallace dismissed jkczyz’s stale review via 97a4337 February 7, 2025 23:20

valentinewallace force-pushed the 2024-12-async-payment-receive branch from 97a4337 to 5144036 Compare February 7, 2025 23:25

valentinewallace force-pushed the 2024-12-async-payment-receive branch from 5144036 to 95e453a Compare February 7, 2025 23:29

jkczyz previously approved these changes Feb 7, 2025

View reviewed changes

Test async receive

1f53a31

In the previous commit we completed support for async receive from an always-online sender to an often-offline receiver, minus support for acting as the async receiver's always-online channel counterparty.

valentinewallace dismissed jkczyz’s stale review via 1f53a31 February 7, 2025 23:38

valentinewallace force-pushed the 2024-12-async-payment-receive branch from 95e453a to 1f53a31 Compare February 7, 2025 23:38

jkczyz approved these changes Feb 8, 2025

View reviewed changes

TheBlueMatt approved these changes Feb 8, 2025

View reviewed changes

TheBlueMatt merged commit f5c0433 into lightningdevkit:main Feb 8, 2025
23 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support receiving async payments #3440

Support receiving async payments #3440

valentinewallace commented Dec 4, 2024 •

edited

Loading

valentinewallace commented Dec 4, 2024

valentinewallace Jan 27, 2025

jkczyz left a comment

TheBlueMatt left a comment •

edited

Loading

valentinewallace commented Feb 6, 2025

jkczyz Feb 6, 2025

valentinewallace Feb 6, 2025

valentinewallace commented Feb 7, 2025

jkczyz left a comment

TheBlueMatt commented Feb 7, 2025

valentinewallace commented Feb 7, 2025

TheBlueMatt Feb 8, 2025

Support receiving async payments #3440

Support receiving async payments #3440

Conversation

valentinewallace commented Dec 4, 2024 • edited Loading

valentinewallace commented Dec 4, 2024

valentinewallace Jan 27, 2025

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment • edited Loading

Choose a reason for hiding this comment

valentinewallace commented Feb 6, 2025

jkczyz Feb 6, 2025

Choose a reason for hiding this comment

valentinewallace Feb 6, 2025

Choose a reason for hiding this comment

valentinewallace commented Feb 7, 2025

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 7, 2025

valentinewallace commented Feb 7, 2025

TheBlueMatt Feb 8, 2025

Choose a reason for hiding this comment

valentinewallace commented Dec 4, 2024 •

edited

Loading

TheBlueMatt left a comment •

edited

Loading