Allow Asynchronously responding to Invoice Request

[shaavan]

Introduce a new event and set of supporting functions to allow flexibility in responding asynchronously to a received invoice request.

This PR provides foundational work for further improvements in future. For example, this allows supporting Offers created in currency denomination, as a user can do appropriate pre-processing for a received InvoiceRequest, before sending an Invoice for it.

[codecov]

Codecov Report

Attention: Patch coverage is 74.01575% with 66 lines in your changes missing coverage. Please review.

Project coverage is 89.57%. Comparing base (66fb520) to head (9b4daa4).

Files with missing lines	Patch %	Lines
lightning/src/ln/channelmanager.rs	74.82%	34 Missing and 1 partial ⚠️
lightning/src/events/mod.rs	0.00%	19 Missing ⚠️
lightning/src/offers/invoice.rs	73.07%	4 Missing and 3 partials ⚠️
lightning/src/offers/invoice_request.rs	57.14%	3 Missing ⚠️
lightning/src/ln/offers_tests.rs	96.72%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3318      +/-   ##
==========================================
- Coverage   89.64%   89.57%   -0.07%     
==========================================
  Files         126      126              
  Lines      102676   102849     +173     
  Branches   102676   102849     +173     
==========================================
+ Hits        92045    92132      +87     
- Misses       7912     7987      +75     
- Partials     2719     2730      +11     

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

[jkczyz]

Is this redundant with the check in the builder?

[shaavan]

The check in the builder does trigger a Bolt12SemanticError when this case comes up. But this feels more like a user-side error than a problem with the InvoiceRequest itself.

To make sure we generate the right type of error and avoid sending an InvoiceError in this situation, I've added this extra check to handle it properly.

[jkczyz]

Isn't this also called in the builder?

[shaavan]

Yes!
But AFAIU, this is also called here so that the amount_msats value can be used with the create_inbound_payment and create_blinded_payment_paths functions.

[shaavan]

Updated from pr3318.01 to pr3318.02 (diff):

Changes:

1. Rebase on main to resolve merge conflicts and fix CI.

[shaavan]

Updated from pr3318.02 to pr3318.03 (diff):
Addressed @jkczyz comments

Changes:

1. Update Responder to be required field.
2. Update InvoiceRequestReceived Docs.
3. Update condition check when the Custom Amount should be provided.
4. Create the InvoiceRequestReceived event only when InvoiceRequest is verified.

[TheBlueMatt]

I'm kinda skeptical of the approach of pushing yet more message handling out through the event handling interface. Looking back I'm really not a fan of the approach we took with manually_handle_bolt12_invoices, too. Instead of hooking things in the middle of their logic in ChannelManager and pushing them up as events, should we instead refactor the BOLT 12 handling in ChannelManager to expose an OffersMessageHandler which does the context testing and message validation and then passes the messages off to a handler interface.

ChannelManager can handle use this internally to do the first layer of message processing. For invoice_requests this basically leaves ChannelManager just fetching a blinded path and calling respond. For invoices ChannelManager would basically just take the invoice and pass it to the outbound payment logic.

[TheBlueMatt]

I also regret not doing it this way initially, since I assume this would have reduced the code in LNDK rather nontrivially :/

[jkczyz]

I'm kinda skeptical of the approach of pushing yet more message handling out through the event handling interface. Looking back I'm really not a fan of the approach we took with manually_handle_bolt12_invoices, too. Instead of hooking things in the middle of their logic in ChannelManager and pushing them up as events, should we instead refactor the BOLT 12 handling in ChannelManager to expose an OffersMessageHandler which does the context testing and message validation and then passes the messages off to a handler interface.

ChannelManager can handle use this internally to do the first layer of message processing. For invoice_requests this basically leaves ChannelManager just fetching a blinded path and calling respond. For invoices ChannelManager would basically just take the invoice and pass it to the outbound payment logic.

I'll need a more concrete description of this to make sure that I understand what you are proposing.

Let's call the refactored out struct OffersMessageFlow to avoid overloading OffersMessageHandler, which I presume ChannelManger would still implement and thus would typically still be used as a parameter to OnionMessenger. Then OffersMessageFlow would itself be parameterized by some new trait for users to implement, IIUC, which would need to trickle up to ChannelManager since it would be holding an OffersMessageFlow.

So, in practice, users would simply implement a small trait and parameterize ChannelManager with it. And implementations of this trait would be used to examine the messages and returning whether they should be acted on and possibly with what values (e.g., an amount in case of fiat-denominations).

IIUC, the logic for constructing an invoice would still reside in ChannelManager, too, as constructing an invoice relies on many of its fields.

Does all of that sound right?

I also regret not doing it this way initially, since I assume this would have reduced the code in LNDK rather nontrivially :/

Hmm... does it make it that much simpler? The above can be accomplished without OffersMessageFlow by simply adding another parameter to ChannelManager. Unless I misunderstood your proposal, the only thing OffersMessageFlow would used for would be the following two chunks of code:

rust-lightning/lightning/src/ln/channelmanager.rs

Lines 11138 to 11155 in a952d2d

	let nonce = match context {
	None if invoice_request.metadata().is_some() => None,
	Some(OffersContext::InvoiceRequest { nonce }) => Some(nonce),
	_ => return None,
	};
	let invoice_request = match nonce {
	Some(nonce) => match invoice_request.verify_using_recipient_data(
	nonce, expanded_key, secp_ctx,
	) {
	Ok(invoice_request) => invoice_request,
	Err(()) => return None,
	},
	None => match invoice_request.verify_using_metadata(expanded_key, secp_ctx) {
	Ok(invoice_request) => invoice_request,
	Err(()) => return None,
	},
	};

rust-lightning/lightning/src/ln/channelmanager.rs

Lines 11240 to 11243 in a952d2d

	let payment_id = match self.verify_bolt12_invoice(&invoice, context.as_ref()) {
	Ok(payment_id) => payment_id,
	Err(()) => return None,
	};

Or do you also want to move the Bolt12Invoice construction into OffersMessageFlow? If so, ChannelManager would first call it to verify the request. Then depending on the result, call another method with a payment_hash and payment_paths.

Let me know if I misunderstood anything.

[TheBlueMatt]

I'll be honest I hadn't thought it all the way through, but, no I do think we should have the OffersMessageFlow create Bolt12Invoices. The only ChannelManager access that currently requires is create_inbound_payment (to fetch a payment_hash and payment_secret), create_blinded_payment_paths (both of which could be intermediated through a trait), highest_seen_timestamp (to fetch the time in no-std, which could also pass through the trait), and node_signer (which the OffersMessageFlow could simply hold a reference to).

We could have the OffersMessageFlow be the only implementer of OffersMessageHandler and make users configure the OnionMessenger with an OffersMessageFlow<ChannelManager>, but we could also expose the guts of OffersMessageFlow crate-internal so that we can create this intermediation trait on the fly and pass it in to individual message handlers.

[jkczyz]

We could have the OffersMessageFlow be the only implementer of OffersMessageHandler and make users configure the OnionMessenger with an OffersMessageFlow<ChannelManager>, but we could also expose the guts of OffersMessageFlow crate-internal so that we can create this intermediation trait on the fly and pass it in to individual message handlers.

We also have ChannelManager::pending_offers_messages for user-initiated messages (e.g., used by pay_for_offer to enqueue invoice requests), which OnionMessenger queries. So if OffersMessageFlow implements OffersMessageHandler, we'd likely need to move those methods there. FWIW, it seems like we are (at least partially) recreating the old InvoicePayer abstraction since OffersMessageFlow would need to be parameterized by some trait that ChannelManager would implement. The MessageRouter trait probably could move to OffersMessageFlow, though I'm not sure if other/future onion-message uses would still remain in ChannelManager.

I'm actually more in favor of this now, especially if allows us to reduce the amount of code in ChannelManager.

[jkczyz]

I'm actually more in favor of this now, especially if allows us to reduce the amount of code in ChannelManager.

InvoiceRequest retries will be difficult, though. 😕

[jkczyz]

@TheBlueMatt I was thinking about this a bit more. For the Fedimint case, the gateway will need to communicate with the client before responding or possibly (depending on the ultimate approach) wait for the federation to sign, IIUC. That would mean we'd either have to block waiting for the response or expose an asynchronous interface after all. This probably could be avoided for the currency use case if exchange rates are cached, though.

We could still refactor but seems we'd need to allow for asynchronous responses?

[TheBlueMatt]

InvoiceRequest retries will be difficult, though. 😕

Hmm, given our current retry logic we could easily move it to this new struct. If we substantially change it that might have to change for DoS risks, but probably we won't hugely change it.

@TheBlueMatt I was thinking about this a bit more. For the Fedimint case, the gateway will need to communicate with the client before responding or possibly (depending on the ultimate approach) wait for the federation to sign, IIUC. That would mean we'd either have to block waiting for the response or expose an asynchronous interface after all. This probably could be avoided for the currency use case if exchange rates are cached, though.

Maybe the trait from the OffersMessageFlow to the ChannelManager passes a Bolt12InvoiceBuilder? That way it can "do all the work" and then pass it off for the last step?