Async receive prefactor #3517

valentinewallace · 2025-01-09T19:33:02Z

Some test util refactors, additional test coverage, and one fix in preparation for finalizing support for async receive.

~~Based on #3408~~. Prepares for #3440

valentinewallace · 2025-01-09T20:20:14Z

lightning/src/ln/outbound_payment.rs

@@ -1045,6 +1045,11 @@ impl OutboundPayments {
 						abandon_with_entry!(entry, PaymentFailureReason::UnknownRequiredFeatures);
 						return Err(Bolt12PaymentError::UnknownRequiredFeatures)
 					}
+					if duration_since_epoch > invoice.created_at().saturating_add(invoice.relative_expiry()) {


Note for reviewers: for inbound payments, we buffer expiries by 2 hours to account for inaccurate block times in no-std. So we could do that here too, but it seemed maybe unnecessary since static invoices are expected to have quite long expiries... Let me know if we'd prefer to do that. (Our other outbound expiry checks are std-only, but I wanted no-std here for ease of testing).

Don't think it matters much, but given we already do it in a few places seems like we could DRY it up into a utility function and do the offset. 🤷‍♂️

I looked for a bit and couldn't find a good place to put the util (invoice_utils might make sense but it's being deprecated IIUC?). So I might just leave this as-is until/unless we refactor outbound_payments to detect expired invoices in no-std more generally.

valentinewallace · 2025-01-17T18:35:23Z

Rebased on main after #3408 landed.

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2025-01-21T19:54:16Z

lightning/src/ln/outbound_payment.rs

@@ -1045,6 +1045,11 @@ impl OutboundPayments {
 						abandon_with_entry!(entry, PaymentFailureReason::UnknownRequiredFeatures);
 						return Err(Bolt12PaymentError::UnknownRequiredFeatures)
 					}
+					if duration_since_epoch > invoice.created_at().saturating_add(invoice.relative_expiry()) {


Don't think it matters much, but given we already do it in a few places seems like we could DRY it up into a utility function and do the offset. 🤷‍♂️

TheBlueMatt · 2025-01-21T20:06:02Z

lightning/src/ln/channelmanager.rs

+		/// blinded path was not used.
+		///
+		/// Used in part to determine the [`events::PaymentPurpose`].
+		payment_context: Option<PaymentContext>,


Hmm, do we actually have to store the new field here? ISTM we could just fail earlier in create_recv_pending_htlc_info, but do we need the new field for the full async receive work?

Good point, but yeah we do need the new field in #3440. Amended the commit message.

lightning/src/ln/offers_tests.rs

valentinewallace · 2025-01-22T19:53:26Z

Squash-pushed with two small changes:

diff --git a/lightning/src/ln/offers_tests.rs b/lightning/src/ln/offers_tests.rs
index 3f8f5a3fb..803441f09 100644
--- a/lightning/src/ln/offers_tests.rs
+++ b/lightning/src/ln/offers_tests.rs
@@ -2316,9 +2316,6 @@ fn rejects_keysend_to_non_static_invoice_path() {
        nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
        do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
        expect_payment_failed_conditions(&nodes[0], payment_hash, true, PaymentFailedConditions::new());
-       nodes[1].logger.assert_log_contains(
-               "lightning::ln::channelmanager", "received a keysend payment to a non-async payments context", 1
-       );
 }

 #[test]
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index 743fff45e..38f841eac 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -458,7 +458,8 @@ impl_writeable_tlv_based_enum_legacy!(StaleExpiration,
 /// [`Event::PaymentFailed`]: crate::events::Event::PaymentFailed
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum RetryableSendFailure {
-       /// The provided [`PaymentParameters::expiry_time`] indicated that the payment has expired.
+       /// The provided invoice or [`PaymentParameters::expiry_time`] indicated that the payment has
+       /// expired.
        #[cfg_attr(feature = "std", doc = "")]
        #[cfg_attr(feature = "std", doc = "Note that this error is *not* caused by [`Retry::Timeout`].")]
        ///

codecov · 2025-01-22T20:03:20Z

Codecov Report

Attention: Patch coverage is 91.75258% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.55%. Comparing base (aa2c6fe) to head (f81964f).
Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	72.72%	2 Missing and 1 partial ⚠️
lightning/src/ln/peer_handler.rs	0.00%	2 Missing ⚠️
lightning/src/onion_message/functional_tests.rs	0.00%	2 Missing ⚠️
lightning/src/ln/offers_tests.rs	98.07%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3517      +/-   ##
==========================================
+ Coverage   88.44%   88.55%   +0.11%     
==========================================
  Files         149      149              
  Lines      113330   114969    +1639     
  Branches   113330   114969    +1639     
==========================================
+ Hits       100238   101815    +1577     
- Misses      10621    10622       +1     
- Partials     2471     2532      +61

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

jkczyz · 2025-01-22T21:30:10Z

lightning/src/ln/channelmanager.rs

+										if payment_context.is_some() {
+											if !matches!(payment_context, Some(PaymentContext::AsyncBolt12Offer(_))) {
+												log_trace!(self.logger, "Failing new HTLC with payment_hash {}: received a keysend payment to a non-async payments context {:#?}", payment_hash, payment_context);
+											}
+											fail_htlc!(claimable_htlc, payment_hash);
+										}


Just noting that these cases will be separated in the follow-up PR, AFAICT.

lightning/src/ln/outbound_payment.rs

valentinewallace · 2025-01-24T16:09:19Z

Pushed another docs tweak at @TheBlueMatt's request:

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index 8bf9d5696..9433c84a5 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -458,7 +458,8 @@ impl_writeable_tlv_based_enum_legacy!(StaleExpiration,
 /// [`Event::PaymentFailed`]: crate::events::Event::PaymentFailed
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum RetryableSendFailure {
-       /// The invoice or [`PaymentParameters::expiry_time`] indicated that the payment has expired.
+       /// The BOLT 12 invoice was expired by the time we received it or the
+       /// [`PaymentParameters::expiry_time`] were expired on a retry attempt.
        #[cfg_attr(feature = "std", doc = "")]
        #[cfg_attr(feature = "std", doc = "Note that this error is *not* caused by [`Retry::Timeout`].")]
        ///

Blinded HTLCs are always failed back with the same error, so DRY the test code that fails them backwards. This util will also be used for async payments testing in upcoming commits.

Needed to authenticate that the held_htlc_available message is being sent over a reply path that we originally created and that isn't expired before we reply with release_held_htlc. This context will be used in upcoming commits when we add support for async receive.

valentinewallace · 2025-01-24T17:41:38Z

Rebased due to conflicts

Prior to this patch, if we received an expired static invoice we would delay surfacing the payment failure until after the recipient had come online and sent the release_held_htlc OM, which could be a long time later. Now, we'll detect that the invoice is expired as soon as it's received.

Here we bubble up the payment context into PendingHTLCRouting::ReceiveKeysend and check it when receiving a spontaneous payment prior to generating a claimable event. Prior to this patch, we would have accepted out-of-context keysends sent over blinded paths taken from our BOLT 12 invoices. As a side effect of this, our blinded keysend success test cases now fail, so those tests are now removed. Their coverage is re-added in future commits when we add support for async receive, meaning we're able to receive blinded keysends in the correct payment context. While we could avoid storing the payment context for the purposes of this bugfix, we go ahead and store it now because it will be needed when support for receiving async payments is added.

valentinewallace · 2025-01-24T19:37:02Z

Another docs tweak:

diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index 1a212c7c4..07a97759f 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -561,8 +561,8 @@ pub enum PaymentFailureReason {
        #[cfg_attr(feature = "std", doc = "")]
        #[cfg_attr(feature = "std", doc = "[`Retry::Timeout`]: crate::ln::channelmanager::Retry::Timeout")]
        RetriesExhausted,
-       /// The payment expired while retrying, based on the provided
-       /// [`PaymentParameters::expiry_time`].
+       /// Either the BOLT 12 invoice was expired by the time we received it or the payment expired while
+       /// retrying based on the provided [`PaymentParameters::expiry_time`].
        ///
        /// Also used for [`InvoiceRequestExpired`] when downgrading to version prior to 0.0.124.
        ///
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index 5127cf47c..138f01007 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -458,8 +458,9 @@ impl_writeable_tlv_based_enum_legacy!(StaleExpiration,
 /// [`Event::PaymentFailed`]: crate::events::Event::PaymentFailed
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum RetryableSendFailure {
-       /// The BOLT 12 invoice was expired by the time we received it or the
-       /// [`PaymentParameters::expiry_time`] were expired on a retry attempt.
+       /// The provided [`PaymentParameters::expiry_time`] indicated that the payment has expired or
+       /// the BOLT 12 invoice paid to via [`ChannelManager::send_payment_for_bolt12_invoice`] was
+       /// expired.
        #[cfg_attr(feature = "std", doc = "")]
        #[cfg_attr(feature = "std", doc = "Note that this error is *not* caused by [`Retry::Timeout`].")]
        ///

This error variant is also used when manually sending to BOLT 12 invoices is enabled, so document that.

TheBlueMatt

LGTM. Changes since @jkczyz's ACK are all doc things so just gonna land this.

valentinewallace force-pushed the 2025-01-async-receive-prefactor branch from e984b03 to 50fb8fe Compare January 9, 2025 19:35

valentinewallace mentioned this pull request Jan 9, 2025

Support receiving async payments #3440

Merged

valentinewallace commented Jan 9, 2025

View reviewed changes

valentinewallace mentioned this pull request Jan 9, 2025

Async Payments #2298

Open

26 tasks

valentinewallace force-pushed the 2025-01-async-receive-prefactor branch from 50fb8fe to ddaf22f Compare January 17, 2025 18:34

TheBlueMatt added the weekly goal Someone wants to land this this week label Jan 18, 2025

TheBlueMatt reviewed Jan 21, 2025

View reviewed changes

jkczyz reviewed Jan 22, 2025

View reviewed changes

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

valentinewallace force-pushed the 2025-01-async-receive-prefactor branch from ddaf22f to ff84599 Compare January 22, 2025 19:52

valentinewallace force-pushed the 2025-01-async-receive-prefactor branch from ff84599 to f81964f Compare January 22, 2025 19:54

jkczyz previously approved these changes Jan 22, 2025

View reviewed changes

TheBlueMatt reviewed Jan 23, 2025

View reviewed changes

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

valentinewallace dismissed jkczyz’s stale review via 47ef7d1 January 23, 2025 17:45

valentinewallace force-pushed the 2025-01-async-receive-prefactor branch 2 times, most recently from 47ef7d1 to 694a8b9 Compare January 24, 2025 16:08

valentinewallace added 3 commits January 24, 2025 12:39

Tests: DRY failing a blinded HTLC backwards

708e8b5

Blinded HTLCs are always failed back with the same error, so DRY the test code that fails them backwards. This util will also be used for async payments testing in upcoming commits.

Tests: DRY static invoice creation

da8cb9a

valentinewallace force-pushed the 2025-01-async-receive-prefactor branch from 694a8b9 to e047950 Compare January 24, 2025 17:41

valentinewallace added 2 commits January 24, 2025 14:34

valentinewallace force-pushed the 2025-01-async-receive-prefactor branch from e047950 to db87b29 Compare January 24, 2025 19:35

Tweak RetryableSendFailure::PaymentExpired docs

f799e6b

This error variant is also used when manually sending to BOLT 12 invoices is enabled, so document that.

valentinewallace force-pushed the 2025-01-async-receive-prefactor branch from db87b29 to f799e6b Compare January 24, 2025 19:40

TheBlueMatt approved these changes Jan 24, 2025

View reviewed changes

TheBlueMatt merged commit 8588943 into lightningdevkit:main Jan 24, 2025
24 of 25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async receive prefactor #3517

Async receive prefactor #3517

valentinewallace commented Jan 9, 2025 •

edited

Loading

valentinewallace Jan 9, 2025

TheBlueMatt Jan 21, 2025

valentinewallace Jan 22, 2025

valentinewallace commented Jan 17, 2025

TheBlueMatt Jan 21, 2025

TheBlueMatt Jan 21, 2025

valentinewallace Jan 22, 2025

valentinewallace commented Jan 22, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading

jkczyz Jan 22, 2025

valentinewallace commented Jan 24, 2025

valentinewallace commented Jan 24, 2025

valentinewallace commented Jan 24, 2025

TheBlueMatt left a comment

Async receive prefactor #3517

Async receive prefactor #3517

Conversation

valentinewallace commented Jan 9, 2025 • edited Loading

valentinewallace Jan 9, 2025

Choose a reason for hiding this comment

TheBlueMatt Jan 21, 2025

Choose a reason for hiding this comment

valentinewallace Jan 22, 2025

Choose a reason for hiding this comment

valentinewallace commented Jan 17, 2025

TheBlueMatt Jan 21, 2025

Choose a reason for hiding this comment

TheBlueMatt Jan 21, 2025

Choose a reason for hiding this comment

valentinewallace Jan 22, 2025

Choose a reason for hiding this comment

valentinewallace commented Jan 22, 2025

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

jkczyz Jan 22, 2025

Choose a reason for hiding this comment

valentinewallace commented Jan 24, 2025

valentinewallace commented Jan 24, 2025

valentinewallace commented Jan 24, 2025

TheBlueMatt left a comment

Choose a reason for hiding this comment

valentinewallace commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading