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)

valentinewallace · 2024-11-14T20:20:43Z

lightning/src/ln/channelmanager.rs

+		const SECONDS_PER_BLOCK: u32 = 10 * 60;
+		let relative_expiry_blocks = relative_expiry_seconds / SECONDS_PER_BLOCK;
+		let max_cltv_expiry = core::cmp::max(relative_expiry_blocks, CLTV_FAR_FAR_AWAY)


Note for reviewers: I'm not sure this is robust enough with block times being somewhat unreliable. Maybe it should be buffered?

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.

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.

lightning/src/ln/channelmanager.rs

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.

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.

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 the blinded path using the static invoice's corresponding offer id.

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.

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

valentinewallace commented Nov 14, 2024

View reviewed changes

valentinewallace added 3 commits November 14, 2024 15:31

Fix outdated static invoice docs.

847b43b

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 added 5 commits November 26, 2024 18:33

f assert instead of debug_assert

5a66f12

Make create_blinded_payment_paths methods amount optional.

698527b

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

f don't use default amount

b1791f4

Factor invoice expiry into blinded path max_cltv_expiry

8b74ed4

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.

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 added 5 commits November 27, 2024 11:33

f remove reachable debug_assert

3a18f5a

f fix docs

c38be3d

Test failures on paying static invoices

63601e4

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 73a28ef to 63601e4 Compare November 27, 2024 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add static invoice creation utils to `ChannelManager` #3408

Add static invoice creation utils to `ChannelManager` #3408

valentinewallace commented Nov 14, 2024

valentinewallace Nov 14, 2024

jkczyz Nov 19, 2024

vincenzopalazzo Nov 21, 2024

jkczyz Nov 20, 2024

valentinewallace Nov 21, 2024

jkczyz Nov 22, 2024

valentinewallace Nov 25, 2024

jkczyz Nov 20, 2024

valentinewallace Nov 21, 2024

jkczyz Nov 22, 2024

jkczyz Nov 22, 2024

jkczyz Nov 22, 2024

jkczyz Nov 22, 2024

valentinewallace Nov 25, 2024

TheBlueMatt Nov 26, 2024

valentinewallace Nov 26, 2024

TheBlueMatt left a comment

TheBlueMatt Nov 26, 2024

valentinewallace Nov 26, 2024

valentinewallace Nov 27, 2024 •

edited

Loading

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

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

valentinewallace Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Add static invoice creation utils to `ChannelManager` #3408

Add static invoice creation utils to `ChannelManager` #3408

valentinewallace Nov 27, 2024 •

edited

Loading