Add static invoice creation utils to `ChannelManager` #3408

valentinewallace · 2024-11-14T01:36:13Z

Add static invoice creation utilities as part of supporting the async payments BOLTs spec Support async payments in BOLT 12 lightning/bolts#1149.
Take this opportunity to more easily test some code added in Support paying static invoices #3140 that went untested at the time. This is the bulk of the diff.
Address a piece of feedback from Support paying static invoices #3140 regarding InvoiceRequests being unavailable when the time comes to send the async payment, cc Support paying static invoices #3140 (comment)

lightning/src/ln/channelmanager.rs

jkczyz · 2024-11-19T23:19:53Z

lightning/src/ln/outbound_payment.rs

 pub(crate) struct RetryableInvoiceRequest {
 	pub(crate) invoice_request: InvoiceRequest,
 	pub(crate) nonce: Nonce,
+	pub(super) needs_retry: bool,
 }


Would it be worth not holding on to these for offers that don't support async payments? Can't recall if we can tell from the offer or if it isn't known until the static invoice is received.

IIRC there is nothing in the offers, just at the invoice request level.

lightning/src/routing/router.rs

jkczyz · 2024-11-20T20:54:18Z

lightning/src/ln/channelmanager.rs

+		let amount_msat = offer.amount().and_then(|amount| {
+			match amount {
+				Amount::Bitcoin { amount_msats } => Some(amount_msats),
+				Amount::Currency { .. } => None
+			}
+		});


Is quantity_max allowed for offers paying often-offline nodes? If so, is the amount checked by them in some way? If not, should we prevent building a static invoice from such offers?

quantity_max is currently allowed. I'm not sure I follow your question though:

If so, is the amount checked by them in some way?

Who is "them"? It looks like the payer checks the amount/quantity when sending an invreq but not sure I'm following.

Does the recipient check that the amount is sufficient for the requested quantity before releasing the preimage?

Yeah seems they will have to, if that isn't done already. Currently we don't support receiving to the static invoices created by these utils (just creating them), so this is definitely something to consider in the follow-up.

jkczyz · 2024-11-20T20:58:35Z

lightning/src/ln/channelmanager.rs

+		for path in message_paths_to_always_online_node {
+			builder = builder.path(path);
+		}


If the recipient is online, does the InvoiceRequest get forwarded along? If so, how does the recipient authenticate it?

If the recipient is online, does the InvoiceRequest get forwarded along?

Yep!

how does the recipient authenticate it?

Since the recipient issued the offer themselves, they authenticate it the same way as always-online recipients, i.e. via verify_using_recipient_data, if I'm understanding you.

Right, so I guess that means the recipient must construct these paths since it needs to include the Nonce used to derive the signing keys.

Hmm... but the function is creating and returning the Nonce. I think we'll need to pass it in?

To be clear, we don't include metadata in the offer when we have blinded paths. Can't recall if we do something different when constructing an offer for use with async payments.

I brought this up during review club. Since payment path contains the offer nonce -- and the sender will include the invoice request with the payment -- we should be able to verify the invoice request is authentic with this nonce. But ISTM we still want the same nonce in the offer's message paths for the normal case when the recipient is online.

Ah I misunderstood at first, this is a good point.... I'm not sure yet what a good solution would be.

For context, message_paths_to_always_online_node terminate at the always-online node. Currently in LDK the recipient expects to receive the invreq either over a blinded path from the offer or to their node id but with offer_metadata set (IIUC), but neither of those are supported in this PR atm.

For more context, I think the always-online node is expected to be a direct peer of the async receiver, otherwise an onion message round-trip would be incurred to check whether the recipient is online when the always-online node receives the invreq.

So it seems like our two options here are to either set the offer metadata for async receive offers (which may be a privacy leak), or for the recipient to provide the always-online node with a one-hop blinded path to send the invreq over, separately from their static invoice. Either of these options should allow the recipient to verify the invreq if they happen to be online at the time.

Any thoughts on this?

I wonder how big that privacy leak is in practice? If we assume most BPs to async-recipients are to an LSP and probably 1-hop BPs for reliability, then its probably nothing (as such BPs are always to async-recipients). If we assume they're sometimes 2-hops, then its probably not huge but is at least something non-zero. It may also just be unintuitive that this leak exists and users might not notice it.

That said, how does the LSP currently pass the invreq to the recipient? Presumably there's some offer -> node registration process so the LSP has a DB with (offer, node, static_invoice) tuples? Replacing the node with a one-hop BP doesn't sound like all that much work, I think, depending on what the API from LDK looks like?

That said, how does the LSP currently pass the invreq to the recipient? Presumably there's some offer -> node registration process so the LSP has a DB with (offer, node, static_invoice) tuples? Replacing the node with a one-hop BP doesn't sound like all that much work, I think, depending on what the API from LDK looks like?

Yep. I think I agree the blinded path route is the way to go. It's more flexible in case async receivers prefer their offer's corresponding always-online node to be a hop or two away (IMO this would introduce too much payment latency in practice, but who knows), and avoids increasing the size of the offer via offer_metadata.

We may want a separate method on ChannelManager to create this blinded path, though it's actually already supported via MessageRouter since the recipient has the offer nonce. So I may punt on hashing out the details of this for now until LSP async payments support is more fleshed out.

TheBlueMatt

Nice, thanks!

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2024-11-26T18:13:10Z

lightning/src/blinded_path/payment.rs

+///
+/// [`Offer`]: crate::offers::offer::Offer
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct AsyncBolt12OfferContext {


#3427 applies here too, though not sure if it needs to be addressed in this PR.

Tracking this on #2298. I'm inclined to build on Jeff's solution to #3427 to make sure we align with the rest of the offers code unless there are objections.

Opened #3435.

lightning/src/ln/channelmanager.rs

valentinewallace · 2024-11-27T16:21:20Z

lightning/src/events/mod.rs

@@ -203,6 +203,15 @@ impl PaymentPurpose {
 					payment_context: context,
 				}
 			},
+			Some(PaymentContext::AsyncBolt12Offer(_context)) => {
+				debug_assert!(false, "Receiving async payments is not yet supported");


I think due in part to #3427 this debug_assert may be reachable. I have a patch in an upcoming PR to change this method to return a Result (to handle the case where the sender doesn't include an invoice request when async paying) so I may just add that diff to this commit.

Edit: added 3a18f5a.

lightning/src/ln/channelmanager.rs

lightning/src/blinded_path/payment.rs

jkczyz

Fix-ups LGTM. Just comments from reviewing the tests.

lightning/src/ln/channelmanager.rs

jkczyz · 2024-12-04T23:49:50Z

lightning/src/ln/blinded_payment_tests.rs

+	// Set the random bytes so we can predict the offer outbound payment context nonce.
+	*nodes[0].keys_manager.override_random_bytes.lock().unwrap() = Some([42; 32]);


Shouldn't these tests go in offers_tests.rs and use extract_offer_nonce?

We can't use extract_offer_nonce because the nonce we're trying to extract comes from the reply path to the invreq, not the invreq itself. But I switched away from manually constructing the hmac, see the latest fixup!

jkczyz

LGTM. Just some minor comments to address.

lightning/src/ln/channelmanager.rs

lightning/src/ln/blinded_payment_tests.rs

lightning/src/ln/channelmanager.rs

jkczyz

Feel free to squash after fixing the doc comment

TheBlueMatt

Aside from the comments here, LGTM.

lightning/src/blinded_path/payment.rs

TheBlueMatt · 2024-12-11T19:49:39Z

lightning/src/blinded_path/message.rs

+		/// one of our offers.
+		///
+		/// [`Offer`]: crate::offers::offer::Offer
+		offer_id: OfferId,


Remind me why we need this? The HeldHtlcAvailable message contains almost nothing, right? What offer_id are we authenticating against? We presumably don't even have an offer at the point where we receive this.

This is basically just used as an input into the HMAC field 10 lines below, so it helps authenticate this context. But maybe the nonce is sufficient as a unique input? I was following the pattern of hmac_for_payment_id used for regular bolt 12 invoices but using an offer_id instead.

For AsyncPaymentsContext::OutboundPayment when we receive the ReleaseHtlcAvailable message we need to look up the pending payment with its payment_id, then we use the nonce and hmac to authenticate the field. Here, I don't see a use for the offer_id?

Arguably it would be reasonable to reply to every inbound HeldHtlcAvailable message by just asking that the HTLC be released. It would imply someone can trivially ask if/when you're online which isn't ideal, so we should have something here (a nonce/hmac pair seems to suffice), but ultimately we may get a bunch of these for any given offer, so its not like we can track them and remove offers after they're paid. We might want a timeout here so that we can stop replying to old ones that have expired, I guess?

In any case, not sure what use we'd have for the OfferId.

Yeah, I didn't really have a sense of whether nonce + hmac was sufficient since we haven't used only those things previously, but since it is I went ahead and removed it. Tracked expiring these paths on the async payments issue.

Why change the format later? Should be trivial to store the timeout here, no?

To be clear, in this scenario the node trying to ask if we're online has our node id. Another way for nodes to trivially ask if we're online is just by attempting to connect to us as a peer -- but presumably in this case the attacker is prevented from doing it that way by not knowing our socket address?

Why change the format later? Should be trivial to store the timeout here, no?

Yeah, we may want to expose a method to calculate the absolute expiry from the inbound_payment module so we align with the expiry in the payment secret. Seemed reasonable to punt but will look into doing it here

To be clear, in this scenario the node trying to ask if we're online has our node id. Another way for nodes to trivially ask if we're online is just by attempting to connect to us as a peer -- but presumably in this case the attacker is prevented from doing it that way by not knowing our socket address?

No, I'm thinking of a mobile node that's only connect to an LSP and for which we only have a blinded path to.

Yeah, we may want to expose a method to calculate the absolute expiry from the inbound_payment module so we align with the expiry in the payment secret. Seemed reasonable to punt but will look into doing it here

Sure, that's fine, just seemed easy to shove in there if its not we can do a followup.

Ok I started doing this but tbh I'd prefer to save it for follow-up if it's all the same to you.

No, I'm thinking of a mobile node that's only connect to an LSP and for which we only have a blinded path to.

Ok so for context I think that means the attacker would be reusing the blinded_node_ids to construct their path. What I'm not following is that in order to blind the TLVs of their choosing, I thought they would need the shared secret corresponding to the node's unblinded pubkey and the blinding point?

valentinewallace · 2024-12-12T17:12:02Z

Removed the offer_id field from the payment context, squashed with this diff:

diff --git a/lightning/src/blinded_path/payment.rs b/lightning/src/blinded_path/payment.rs
index 70ac9823e..d72c13353 100644
--- a/lightning/src/blinded_path/payment.rs
+++ b/lightning/src/blinded_path/payment.rs
@@ -362,10 +362,6 @@ pub struct Bolt12OfferContext {
 /// [`Offer`]: crate::offers::offer::Offer
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct AsyncBolt12OfferContext {
-       /// The identifier of the [`Offer`].
-       ///
-       /// [`Offer`]: crate::offers::offer::Offer
-       pub offer_id: OfferId,
        /// The [`Nonce`] used to verify that an inbound [`InvoiceRequest`] corresponds to this static
        /// invoice's offer.
        ///
@@ -650,8 +646,7 @@ impl_writeable_tlv_based!(Bolt12OfferContext, {
 });

 impl_writeable_tlv_based!(AsyncBolt12OfferContext, {
-       (0, offer_id, required),
-       (2, offer_nonce, required),
+       (0, offer_nonce, required),
 });

 impl_writeable_tlv_based!(Bolt12RefundContext, {});
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index ba21b87ac..ed31e00c6 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -9602,7 +9602,7 @@ where
                let secp_ctx = &self.secp_ctx;

                let payment_context = PaymentContext::AsyncBolt12Offer(
-                       AsyncBolt12OfferContext { offer_id: offer.id(), offer_nonce }
+                       AsyncBolt12OfferContext { offer_nonce }
                );
                let amount_msat = offer.amount().and_then(|amount| {
                        match amount {

lightning/src/blinded_path/message.rs

TheBlueMatt · 2024-12-13T16:07:28Z

LGTM, but needs rebase now :/

lightning/src/offers/signer.rs

Prior to this patch, we would take() the invoice request stored for AwaitingInvoice outbound payments when retrying sending the invoice request onion message. This doesn't work for async payments because we need to keep the invoice request stored for inclusion in the payment onion. Therefore, clone it instead of take()ing it.

Prior to this fix, we would attempt to mark outbound async payments as abandoned but silently fail because they were in state AwaitingInvoice, which the mark_abandoned utility doesn't currently work for. These payments would eventually be removed by the remove_stale_payments method, but there would be a delay in generating the PaymentFailed event. Move to manually removing the outbound payment entry.

Useful for creating payment paths for static invoices which are typically amount-less.

Will be useful for static invoices' blinded paths, which may have long expiries. Rather than having a default max_cltv_expiry, we now base it on the invoice expiry.

This context is stored in the blinded payment paths we put in static invoices and is useful to authenticate payments over these paths to the recipient. We can't reuse Bolt12OfferContext for this because we don't have access to the invoice request fields at static invoice creation time.

This context is included in static invoice's blinded message paths, provided back to us in HeldHtlcAvailable onion messages for blinded path authentication. In future work, we will check if this context is valid and respond with a ReleaseHeldHtlc message to release the upstream payment if so. We also add creation methods for the hmac used for authenticating said blinded path.

We can't use our regular offer creation util for receiving async payments because the recipient can't be relied on to be online to service invoice_requests. Therefore, add a new offer creation util that is parameterized by blinded message paths to another node on the network that *is* always-online and can serve static invoices on behalf of the often-offline recipient. Also add a utility for creating static invoices corresponding to these offers. See new utils' docs and BOLTs PR 1149 for more info.

Since adding support for creating static invoices from ChannelManager, it's easier to test these failure cases that went untested when we added support for paying static invoices.

codecov · 2024-12-13T19:15:16Z

Codecov Report

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

Project coverage is 89.72%. Comparing base (1a8bf62) to head (1c2554a).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	66.66%	7 Missing ⚠️
lightning/src/events/mod.rs	61.53%	4 Missing and 1 partial ⚠️
lightning/src/ln/outbound_payment.rs	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3408      +/-   ##
==========================================
- Coverage   89.73%   89.72%   -0.02%     
==========================================
  Files         130      130              
  Lines      107793   107815      +22     
  Branches   107793   107815      +22     
==========================================
+ Hits        96727    96732       +5     
- Misses       8662     8677      +15     
- Partials     2404     2406       +2

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

jkczyz self-requested a review November 14, 2024 18:35

valentinewallace commented Nov 14, 2024

View reviewed changes

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

valentinewallace force-pushed the 2024-11-async-receive-offer-utils branch from 86cfb96 to 8feb3cb Compare November 14, 2024 20:46

valentinewallace marked this pull request as ready for review November 14, 2024 20:49

jkczyz reviewed Nov 20, 2024

View reviewed changes

TheBlueMatt reviewed Nov 26, 2024

View reviewed changes

valentinewallace commented Nov 26, 2024

View reviewed changes

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

valentinewallace mentioned this pull request Nov 26, 2024

Async Payments #2298

Open

22 tasks

valentinewallace force-pushed the 2024-11-async-receive-offer-utils branch from 8feb3cb to 73a28ef Compare November 26, 2024 23:37

valentinewallace commented Nov 27, 2024

View reviewed changes

valentinewallace force-pushed the 2024-11-async-receive-offer-utils branch from 73a28ef to 63601e4 Compare November 27, 2024 16:34

TheBlueMatt reviewed Dec 4, 2024

View reviewed changes

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

lightning/src/blinded_path/payment.rs Outdated Show resolved Hide resolved

valentinewallace mentioned this pull request Dec 4, 2024

Support receiving async payments #3440

Draft

valentinewallace force-pushed the 2024-11-async-receive-offer-utils branch from 63601e4 to 00fceb2 Compare December 4, 2024 22:28

jkczyz mentioned this pull request Dec 4, 2024

Authenticate blinded payment paths #3435

Merged

jkczyz reviewed Dec 4, 2024

View reviewed changes

jkczyz reviewed Dec 6, 2024

View reviewed changes

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

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

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

valentinewallace force-pushed the 2024-11-async-receive-offer-utils branch from 1320eb6 to ac69d09 Compare December 6, 2024 20:06

jkczyz reviewed Dec 6, 2024

View reviewed changes

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

jkczyz reviewed Dec 6, 2024

View reviewed changes

valentinewallace force-pushed the 2024-11-async-receive-offer-utils branch from ac69d09 to 588e19f Compare December 6, 2024 21:06

jkczyz previously approved these changes Dec 6, 2024

View reviewed changes

valentinewallace requested a review from TheBlueMatt December 6, 2024 21:21

TheBlueMatt reviewed Dec 11, 2024

View reviewed changes

valentinewallace dismissed jkczyz’s stale review via 4d9135e December 12, 2024 17:11

valentinewallace force-pushed the 2024-11-async-receive-offer-utils branch from 588e19f to 4d9135e Compare December 12, 2024 17:11

valentinewallace force-pushed the 2024-11-async-receive-offer-utils branch from 4d9135e to 2c50051 Compare December 12, 2024 21:16

valentinewallace commented Dec 12, 2024

View reviewed changes

lightning/src/blinded_path/message.rs Outdated Show resolved Hide resolved

jkczyz reviewed Dec 13, 2024

View reviewed changes

lightning/src/offers/signer.rs Outdated Show resolved Hide resolved

valentinewallace added 12 commits December 13, 2024 13:47

Fix outdated static invoice docs.

43029bf

Make create_blinded_payment_paths methods amount optional.

e810075

Useful for creating payment paths for static invoices which are typically amount-less.

Factor invoice expiry into blinded path max_cltv_expiry

542deeb

Will be useful for static invoices' blinded paths, which may have long expiries. Rather than having a default max_cltv_expiry, we now base it on the invoice expiry.

f remove offer_id from held htlc avail message context

9049c78

f doc update

cd37786

f no offer id in held htlc message context

1067709

Test failures on paying static invoices

1c2554a

Since adding support for creating static invoices from ChannelManager, it's easier to test these failure cases that went untested when we added support for paying static invoices.

valentinewallace force-pushed the 2024-11-async-receive-offer-utils branch from 2c50051 to 1c2554a Compare December 13, 2024 19:06

		// Set the random bytes so we can predict the offer outbound payment context nonce.
		*nodes[0].keys_manager.override_random_bytes.lock().unwrap() = Some([42; 32]);

Add static invoice creation utils to ChannelManager #3408

Are you sure you want to change the base?

Add static invoice creation utils to ChannelManager #3408

Conversation

valentinewallace commented Nov 14, 2024

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

jkczyz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt 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

valentinewallace Dec 12, 2024 • edited Loading

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

Choose a reason for hiding this comment

valentinewallace commented Dec 12, 2024

TheBlueMatt commented Dec 13, 2024

codecov bot commented Dec 13, 2024

Codecov Report

Add static invoice creation utils to `ChannelManager` #3408

Add static invoice creation utils to `ChannelManager` #3408

valentinewallace Nov 27, 2024 •

edited

Loading

jkczyz left a comment •

edited

Loading

valentinewallace Dec 12, 2024 •

edited

Loading