Asynchronous `Bolt12Invoice` payment #3078

jkczyz · 2024-05-22T16:31:50Z

Users such as Fedimint would like to examine a Bolt12Invoice before paying it. Add a UserConfig option for deferring payment of those invoices when received. When set, a new Event::InvoiceReceived is generated instead and payment can be made using ChannelManager::send_payment_for_bolt12_invoice.

codecov-commenter · 2024-05-22T16:42:49Z

Codecov Report

Attention: Patch coverage is 76.27119% with 28 lines in your changes missing coverage. Please review.

Project coverage is 90.02%. Comparing base (a666401) to head (97c1d65).
Report is 24 commits behind head on main.

Files	Patch %	Lines
lightning/src/events/mod.rs	0.00%	18 Missing ⚠️
lightning/src/ln/channelmanager.rs	80.00%	5 Missing ⚠️
lightning/src/offers/invoice.rs	0.00%	4 Missing ⚠️
lightning/src/ln/offers_tests.rs	98.57%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3078      +/-   ##
==========================================
+ Coverage   89.82%   90.02%   +0.20%     
==========================================
  Files         119      121       +2     
  Lines       97987   100303    +2316     
  Branches    97987   100303    +2316     
==========================================
+ Hits        88014    90297    +2283     
- Misses       7397     7427      +30     
- Partials     2576     2579       +3

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

lightning/src/offers/invoice.rs

TheBlueMatt · 2024-05-28T21:32:09Z

LGTM (modulo failing fuzz test), but remind me again why they want to examine invoices for outbound payments?

jkczyz · 2024-05-30T16:09:16Z

LGTM (modulo failing fuzz test), but remind me again why they want to examine invoices for outbound payments?

Sorry for the delay. Wanted to double check with them on the specifics. The gateway's lightning node requests the invoice on the users behalf. The user doesn't trust the gateway so needs to confirm that the amount is expected and that it is paying the right entity (i.e., the containing offer is expected and thus the invoice has the right signature).

TheBlueMatt · 2024-05-31T14:53:11Z

Right, thanks, I guess the user needs to actually transfer the ecash to the gateway (conditional on the payment preimage) but they want to validate the payment_hash/amount pair in the invoice first.

tnull · 2024-06-11T15:56:26Z

@jkczyz I think this needs a (assumingly minor) rebase.

A future InvoiceReceived event will include a Bolt12Invoice. Since Event implements Readable, so must Bolt12Invoice.

A future InvoiceReceived event will include a Responder. Since Event implements Readable, so must Responder.

jkczyz · 2024-06-11T16:13:45Z

Rebased

TheBlueMatt · 2024-06-11T17:22:44Z

lightning/src/events/mod.rs

+	/// This event will only be generated if [`UserConfig::manually_handle_bolt12_invoices`] is set.
+	/// Use [`ChannelManager::send_payment_for_bolt12_invoice`] to pay the invoice or
+	/// [`ChannelManager::abandon_payment`] to abandon the associated payment.
+	///


Should we mention how long the user has to pay the invoice before it'll time out and they'll have to start again (and is that gonna be an issue for fedi)?

Wait, don't we expire the payment based on StaleExpiration too? As if we'd never received the invoice.

Oh, right. I forgot that that's checked at time of payment.

Ended up reverting those changes and re-writing the send_payment_for_bolt12_invoice accordingly.

Did you forget to push this?

Seems to be there: f2721af

Oh I interpreted "re-writing the send_payment_for_bolt12_invoice accordingly" comment as you changed it so we don't time out, rather than rewriting the docs.

Hmmm...send_payment_for_bolt12_invoice docs were changed to clarify when a Bolt12PaymentError::UnexpectedInvoice can occur, including the timeout scenarios. StaleExpiration is not explicitly mentioned because it is pub(crate). I did remove the invoice-level expiration part since that's really a detail surfaced by Event::PaymentFailed, which is linked in the send_payment_for_bolt12_invoice docs.

Right, I just read the comment without looking at the code, I had expected a behavior change based on the comment, rather than doc changes. But doc changes are fine too :)

tnull

LGTM, just a few questions and nits.

tnull · 2024-06-12T10:22:32Z

lightning/src/onion_message/messenger.rs

@@ -325,12 +325,18 @@ impl OnionMessageRecipient {

 /// The `Responder` struct creates an appropriate [`ResponseInstruction`]
 /// for responding to a message.
+#[derive(Clone, Debug, Eq, PartialEq)]
 pub struct Responder {
 	/// The path along which a response can be sent.
 	reply_path: BlindedPath,
 	path_id: Option<[u8; 32]>


I'm probably missing something, but isn't path_id a TLV field in the encrypted_data_tlv stream? Are we positive it will always fit in 32 bytes?

This is the path_id from the BlindedPath that we created for receiving the message. So, currently yes though we never set it. It's only in the Responder for logging purposes, which given the previous sentence means it's not very useful at the moment. 😛

Though that is a good point considering we are likely going to change it to a Vec and start using it as part of #3085. I'm wondering how useful logging it really is unless we also log it when creating the blinded path, too.

Right, I was wondering if it indeed is variable-length whether we should switch the type now before we actually start to serialize objects? I guess the field is TLV anyways and from a brief look it even might work out, but not entirely sure if WithoutLength is byte identical with the [u8; 32] serialization?

@TheBlueMatt Any thoughts on where we should go with this?

If we don't set it maybe we drop it from our logging (even if we intend to set it later)? Its not really clear to me what users have useful to do with a path_id?

(If we want to do that we can do that in a followup PR as long as it comes before the next release, I imagine)

It's useful for correlating an onion message to the blinded path it was sent over -- but only if the path_id of the latter is actually logged, too -- and with any response to that onion message. But seeing that data encoded in the path_id will be given to the handler in #3085, then any logging responsibility could be moved to the handler instead, if desired, and use handler-specific concepts rather than opaque bytes.

I'm not sure where that leaves Responder. It would just a wrapper around BlindedPath with methods for converting to a ResponseInstruction. Those methods could be moved to BlindedPath, but that would add an onion_message module dependency to blinded_path, which doesn't feel right. Maybe the wrapper is fine to keep?

then any logging responsibility could be moved to the handler instead, if desired, and use handler-specific concepts rather than opaque bytes.

Right, as a user I really want this info to come to me in the form of an offer_id or a payment_id. path_id is yet another token I have to correlate with something I know.

I'm not sure where that leaves Responder. It would just a wrapper around BlindedPath with methods for converting to a ResponseInstruction. Those methods could be moved to BlindedPath, but that would add an onion_message module dependency to blinded_path, which doesn't feel right. Maybe the wrapper is fine to keep?

I'd say its fine to keep the wrapper, its a separate concept. What it has internally doesn't matter too much.

lightning/src/events/mod.rs

tnull · 2024-06-12T10:29:44Z

lightning/src/util/config.rs

@@ -847,6 +847,16 @@ pub struct UserConfig {
 	///
 	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
 	pub accept_mpp_keysend: bool,
+	/// If this is set to true, the user needs to manually pay [`Bolt12Invoice`]s when received.


nit: Could tick true/false here.

Done in a separate commit throughout this file along with other literal values. Also noticed that some of the doc formatting wasn't carrying over to the rustdocs without adding some newlines.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2024-06-12T13:56:51Z

Looks like CI is very sad :(

TheBlueMatt · 2024-06-12T16:49:33Z

lightning/src/ln/channelmanager.rs

+	/// fails or if the encoded `payment_id` is not recognized. The latter may happen once the
+	/// payment is no longer tracked because the payment was attempted after:
+	/// - an invoice for the `payment_id` was already paid,
+	/// - one timer tick has elapsed since initially requesting the invoice when paying an offer, or


This is relatively fast, won't this be an issue for fedi where we need to do another network round-trip (maybe to a mobile device) to provide the invoice?

Timer ticks for ChannelManager should be one minute. And this may actually be up to two timer ticks as implemented. So really at least one full timer tick.

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

Lines 1596 to 1608 in b8cdde8

let is_stale = match expiration {

StaleExpiration::AbsoluteTimeout(absolute_expiry) => {

*absolute_expiry <= duration_since_epoch

},

StaleExpiration::TimerTicks(timer_ticks_remaining) => {

if *timer_ticks_remaining > 0 {

*timer_ticks_remaining -= 1;

false

} else {

true

}

},

};

Yea, I guess that seemed kinda fast to me, but maybe its fine.

TheBlueMatt

LGTM

lightning/src/util/config.rs

Some users may want to handle a Bolt12Invoice asynchronously, either in a different process or by first performing additional verification before paying the invoice. Add an InvoiceReceived event to facilitate this.

jkczyz · 2024-06-12T20:29:33Z

Went ahead and squashed the fixups.

TheBlueMatt

We can deal with the path_id question in a followup, as long as its in the same release.

BOLT12 invoices are automatically paid once they have been verified. Users may want to manually pay them by first performing additional checks. Add a manually_handle_bolt12_invoices configuration option that when set generates an Event::InvoiceReceived instead of paying the invoice.

UserConfig::manually_handle_bolt12_invoices allows deferring payment of BOLT12 invoices by generating Event::InvoiceReceived. Expose ChannelManager::send_payment_for_bolt12_invoice to allow users to pay the Bolt12Invoice included in the event. While the event contains the PaymentId for reference, that parameter is now removed from the method in favor of extracting the PaymentId from the invoice's payer_metadata.

TheBlueMatt · 2024-06-13T01:08:48Z

lightning/src/util/config.rs

@@ -879,6 +890,7 @@ impl Readable for UserConfig {
 			manually_accept_inbound_channels: Readable::read(reader)?,
 			accept_intercept_htlcs: Readable::read(reader)?,
 			accept_mpp_keysend: Readable::read(reader)?,
+			manually_handle_bolt12_invoices: Readable::read(reader)?,


We should add a TLV stream here to read from for new values...eventually. I havent spent a lot of CPU time on the current fuzzer so not any reason to do it now, but we should do it for future changes.

tnull reviewed May 23, 2024

View reviewed changes

lightning/src/offers/invoice.rs Show resolved Hide resolved

jkczyz mentioned this pull request May 28, 2024

Add BOLT12 support lightningdevkit/ldk-node#256

Merged

jkczyz mentioned this pull request May 29, 2024

BOLT 12 static invoice encoding and building #3082

Merged

2 tasks

jkczyz added 5 commits June 11, 2024 11:05

Use implicit returns from match expressions

bccdf0d

Clean up invoice handling result logic

1173e06

Re-order imports in events module

3edbe76

Implement Readable for Bolt12Invoice

bfdfb9d

A future InvoiceReceived event will include a Bolt12Invoice. Since Event implements Readable, so must Bolt12Invoice.

Implement Readable for Responder

4c297c9

A future InvoiceReceived event will include a Responder. Since Event implements Readable, so must Responder.

jkczyz force-pushed the 2024-05-invoice-event branch from 48aedd0 to a4fdae2 Compare June 11, 2024 16:13

TheBlueMatt reviewed Jun 11, 2024

View reviewed changes

jkczyz force-pushed the 2024-05-invoice-event branch 2 times, most recently from cb6eb2a to e56ac73 Compare June 11, 2024 20:34

tnull reviewed Jun 12, 2024

View reviewed changes

TheBlueMatt reviewed Jun 12, 2024

View reviewed changes

jkczyz force-pushed the 2024-05-invoice-event branch from e56ac73 to 5721be9 Compare June 12, 2024 17:14

TheBlueMatt reviewed Jun 12, 2024

View reviewed changes

lightning/src/util/config.rs Outdated Show resolved Hide resolved

Add an InvoiceReceived event

4666c33

Some users may want to handle a Bolt12Invoice asynchronously, either in a different process or by first performing additional verification before paying the invoice. Add an InvoiceReceived event to facilitate this.

jkczyz force-pushed the 2024-05-invoice-event branch from 5721be9 to a3def00 Compare June 12, 2024 20:28

jkczyz force-pushed the 2024-05-invoice-event branch from a3def00 to b5ceb27 Compare June 12, 2024 20:50

TheBlueMatt previously approved these changes Jun 12, 2024

View reviewed changes

jkczyz added 2 commits June 12, 2024 19:38

Reformat and tick config docs

232959c

jkczyz added 2 commits June 12, 2024 19:38

Add pays_bolt12_invoice_asynchronously test

97c1d65

jkczyz dismissed TheBlueMatt’s stale review via 97c1d65 June 13, 2024 00:40

jkczyz force-pushed the 2024-05-invoice-event branch from b5ceb27 to 97c1d65 Compare June 13, 2024 00:40

TheBlueMatt approved these changes Jun 13, 2024

View reviewed changes

tnull approved these changes Jun 13, 2024

View reviewed changes

tnull merged commit 3ec0dcd into lightningdevkit:main Jun 13, 2024
14 of 16 checks passed

TheBlueMatt mentioned this pull request Jun 13, 2024

Consider removing path_id from Responder #3124

Closed

jkczyz mentioned this pull request Jun 17, 2024

Introduce MessageContext and use it to allow abandon failed payments #3085

Merged

jkczyz mentioned this pull request Jun 25, 2024

Implement Readable for Bolt12Invoice #3143

Closed

tnull mentioned this pull request Jul 4, 2024

Propagate routing errors up to send_payment_for_bolt12_invoice #3159

Closed

slanesuke mentioned this pull request Jul 8, 2024

Resolve return behavior for BOLT12 Offers lightningdevkit/ldk-node#321

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asynchronous `Bolt12Invoice` payment #3078

Asynchronous `Bolt12Invoice` payment #3078

jkczyz commented May 22, 2024

codecov-commenter commented May 22, 2024 •

edited

Loading

TheBlueMatt commented May 28, 2024

jkczyz commented May 30, 2024

TheBlueMatt commented May 31, 2024

tnull commented Jun 11, 2024

jkczyz commented Jun 11, 2024

TheBlueMatt Jun 11, 2024

TheBlueMatt Jun 11, 2024

jkczyz Jun 11, 2024

jkczyz Jun 11, 2024

TheBlueMatt Jun 12, 2024

jkczyz Jun 12, 2024

TheBlueMatt Jun 12, 2024

jkczyz Jun 12, 2024

TheBlueMatt Jun 12, 2024

tnull left a comment

tnull Jun 12, 2024

jkczyz Jun 12, 2024

tnull Jun 12, 2024 •

edited

Loading

jkczyz Jun 12, 2024

TheBlueMatt Jun 12, 2024

TheBlueMatt Jun 12, 2024

jkczyz Jun 12, 2024 •

edited

Loading

TheBlueMatt Jun 12, 2024

tnull Jun 12, 2024

jkczyz Jun 12, 2024

TheBlueMatt commented Jun 12, 2024

TheBlueMatt Jun 12, 2024

jkczyz Jun 12, 2024

TheBlueMatt Jun 12, 2024

TheBlueMatt left a comment

jkczyz commented Jun 12, 2024

TheBlueMatt left a comment

TheBlueMatt Jun 13, 2024 •

edited

Loading

	let is_stale = match expiration {
	StaleExpiration::AbsoluteTimeout(absolute_expiry) => {
	*absolute_expiry <= duration_since_epoch
	},
	StaleExpiration::TimerTicks(timer_ticks_remaining) => {
	if *timer_ticks_remaining > 0 {
	*timer_ticks_remaining -= 1;
	false
	} else {
	true
	}
	},
	};

Asynchronous Bolt12Invoice payment #3078

Asynchronous Bolt12Invoice payment #3078

Conversation

jkczyz commented May 22, 2024

codecov-commenter commented May 22, 2024 • edited Loading

Codecov Report

TheBlueMatt commented May 28, 2024

jkczyz commented May 30, 2024

TheBlueMatt commented May 31, 2024

tnull commented Jun 11, 2024

jkczyz commented Jun 11, 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

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Jun 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

jkczyz Jun 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

TheBlueMatt commented Jun 12, 2024

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

jkczyz commented Jun 12, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Asynchronous `Bolt12Invoice` payment #3078

Asynchronous `Bolt12Invoice` payment #3078

codecov-commenter commented May 22, 2024 •

edited

Loading

tnull Jun 12, 2024 •

edited

Loading

jkczyz Jun 12, 2024 •

edited

Loading

TheBlueMatt Jun 13, 2024 •

edited

Loading