BOLT 12 outbound `PaymentId` #2468

jkczyz · 2023-08-02T00:48:06Z

When receiving a BOLT 12 invoice originating from either an invoice request or a refund, the invoice should only be paid once. To accomplish this, require that the invoice includes an encrypted payment id in the payer metadata. This allows ChannelManager to track a payment when requesting but prior to receiving the invoice. Thus, it can determine if
the invoice has already been paid.

Includes a PaymentId in payer metadata (for InvoiceRequest and Refund) to track an expected payment
~~Adds an AwaitingInvoice variant to PendingOutboundPayment to track whether an Invoice is expected~~
~~Adds a InvoiceRequestFailed event to notify users of a request that didn't get a response or was unexpected~~

Based on #2432.

valentinewallace

Still parsing the payment_id commit

valentinewallace · 2023-08-08T17:51:36Z

lightning/src/events/mod.rs

@@ -1437,6 +1448,17 @@ impl MaybeReadable for Event {
 				};
 				f()
 			},
+			33u8 => {


Hmm, should this be even? Seems weird to let an outbound payment silently fail

I guess that would mean a user couldn't downgrade if they have this event serialized. Do I have that right? I don't know the right answer here. If the user needs to downgrade then this could prevent them from doing so.

Indeed, that's what that would mean. I don't have a strong opinion, as long as its in a pending-releasenotes file :)

lightning/src/offers/refund.rs

valentinewallace · 2023-08-08T23:21:27Z

lightning/src/offers/signer.rs

+	) -> Self {
+		// Encrypt payment_id
+		let encrypted_payment_id = payment_id.map(|payment_id| {
+			PaymentId(expanded_key.crypt_for_offer(payment_id.0, &nonce.0))


I think it's not the best practice to reuse nonces, not sure if it matters here though.

@TheBlueMatt Would like your expertise here.

The cardinal rule is "anything you put into a hash function you must always be able to deserialize into a unique input if you were to see the unhashed bytes". An IV (which in most cases is really just an extra input to your hash function) is mostly a way to force yourself to abide by that rule by fixing a few bytes of the input to a specific to a unique input in your code. In this case we really have a second IV (WITH[OUT]_ENCRYPTED_PAYMENT_ID_HMAC_INPUT) so its all fine.

So there's actually two different uses here.

For crypt_for_offer, the nonce is used as the IV for ChaCha20::get_single_block to encrypt the payment id.

For the hmac_for_offer, the nonce is used as one input along with an IV specific to the message type.

For the latter, WITH[OUT]_ENCRYPTED_PAYMENT_ID_HMAC_INPUT are also provided though it seems to me those might not be strictly necessary given the IV for the message type dictates whether a payment id is included in the input, in practice.

Does that sound right?

Ohoh, the question was about the nonce rather than reusing the IV in two separate contexts (which is what I responded to above). Yea, given the nonce is actually exposed to the sender, using it as the IV for the encryption of the payment id seems like a bad idea :). This may result in the sender being able to detect duplicate payment ids or repeated bits in the payment id.

What's the alternative? And are you defining the sender as the payer? The sender is the one constructing this (payer) metadata for inclusion in an InvoiceRequest. Do you mean the nonce is exposed to the recipient?

Okay, sorry, I got really confused cause we're talking about multiple nonces + IVs, but the nonce is used as an IV and....

Anyway, so what I was thinking was - what if a user re-uses a payment id (or has some otherwise-structured data in their payment id where a user learning the xor between two unencrypted payment ids is a Bad Thing). In a classic block cipher the IV is XOR'd with the first block to randomize, but its just straight XOR, not hidden at all. In that case, we could start to see an issue here, but that's not how Chacha works, where the IV is really just a second half of the key itself.

In general its bad practice to reuse material, but in this case I'm totally okay with it - its kinda nice to avoid shoving a second 16 byte thing in our metadata and the two uses here are both inputs to something that "looks like" a hash function - sha256 and chacha (which is a sponge), plus given its "public data"...who cares?

lightning/src/offers/signer.rs

valentinewallace · 2023-08-08T23:42:41Z

lightning/src/offers/signer.rs

+	let mut encrypted_payment_id = [0u8; PaymentId::LENGTH];
+	encrypted_payment_id.copy_from_slice(&metadata[..PaymentId::LENGTH]);
+
+	let metadata = &metadata[PaymentId::LENGTH..];


This variable shadowing confused me at first, could we name this something else?

Hmm... shadowing actually prevents you from passing the wrong metadata later. Could add a comment stating that the encrypted PaymentId is sliced off. Made the slicing explicit at each use for now, instead of shadowing. Not sure which you'd prefer.

lightning/src/offers/signer.rs

TheBlueMatt · 2023-08-14T02:04:12Z

lightning/src/offers/signer.rs

+	) -> Self {
+		// Encrypt payment_id
+		let encrypted_payment_id = payment_id.map(|payment_id| {
+			PaymentId(expanded_key.crypt_for_offer(payment_id.0, &nonce.0))


The cardinal rule is "anything you put into a hash function you must always be able to deserialize into a unique input if you were to see the unhashed bytes". An IV (which in most cases is really just an extra input to your hash function) is mostly a way to force yourself to abide by that rule by fixing a few bytes of the input to a specific to a unique input in your code. In this case we really have a second IV (WITH[OUT]_ENCRYPTED_PAYMENT_ID_HMAC_INPUT) so its all fine.

lightning/src/offers/signer.rs

TheBlueMatt · 2023-08-14T02:55:01Z

lightning/src/events/mod.rs

@@ -1437,6 +1448,17 @@ impl MaybeReadable for Event {
 				};
 				f()
 			},
+			33u8 => {


Indeed, that's what that would mean. I don't have a strong opinion, as long as its in a pending-releasenotes file :)

codecov-commenter · 2023-08-17T21:56:45Z

Codecov Report

Patch coverage: 84.58% and project coverage change: +0.27% 🎉

Comparison is base (32d6e91) 90.54% compared to head (3443e5e) 90.82%.
Report is 28 commits behind head on main.

❗ Current head 3443e5e differs from pull request most recent head c805fc8. Consider uploading reports for the commit c805fc8 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2468      +/-   ##
==========================================
+ Coverage   90.54%   90.82%   +0.27%     
==========================================
  Files         109      109              
  Lines       57180    59554    +2374     
  Branches    57180    59554    +2374     
==========================================
+ Hits        51776    54089    +2313     
- Misses       5404     5465      +61

Files Changed	Coverage Δ
lightning/src/events/mod.rs	`47.39% <0.00%> (+0.63%)`	⬆️
lightning/src/ln/channelmanager.rs	`89.84% <60.00%> (+2.64%)`	⬆️
lightning/src/offers/invoice_request.rs	`92.15% <70.00%> (-1.02%)`	⬇️
lightning/src/ln/outbound_payment.rs	`90.72% <85.96%> (+0.97%)`	⬆️
lightning/src/offers/refund.rs	`93.73% <86.66%> (-0.30%)`	⬇️
lightning/src/offers/signer.rs	`86.71% <95.55%> (+3.57%)`	⬆️
lightning/src/ln/inbound_payment.rs	`92.00% <100.00%> (+0.27%)`	⬆️
lightning/src/offers/invoice.rs	`88.84% <100.00%> (+0.31%)`	⬆️
lightning/src/offers/offer.rs	`92.70% <100.00%> (-0.02%)`	⬇️
lightning/src/util/chacha20.rs	`85.40% <100.00%> (+2.82%)`	⬆️
... and 1 more

... and 29 files with indirect coverage changes

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

TheBlueMatt · 2023-08-23T21:53:07Z

Needs rebase.

TheBlueMatt

Mostly nits, some questions, and only really two or three things.

lightning/src/offers/offer.rs

TheBlueMatt · 2023-08-23T22:52:08Z

lightning/src/offers/invoice_request.rs

+	/// Keys used for signing a [`Bolt12Invoice`] if they can be derived.
+	///
+	/// If `Some`, then must respond with methods that use derived keys. Otherwise, should respond
+	/// with an invoice signed explicitly.


"signed explicitly" is pretty confusing - I think we mean "signed by the node's node_id secret key" (or whatever we call it)?

Re-wrote in terms of which method should be called in each situation.

lightning/src/ln/inbound_payment.rs

TheBlueMatt · 2023-08-23T23:09:36Z

lightning/src/ln/inbound_payment.rs

+	pub(crate) fn crypt_for_offer(&self, mut bytes: [u8; 32], iv_bytes: &[u8; IV_LEN]) -> [u8; 32] {
+		let chacha_block = ChaCha20::get_single_block(&self.offers_base_key, iv_bytes);
+		for i in 0..bytes.len() {
+			bytes[i] = chacha_block[i] ^ bytes[i];


nit: this file does the same get_single_block-then-xor thing a bunch of times, should/can we just create a ChaCha20::encrypt_single_block? (also it skeeves me out seeing this, yes ChaCha is a stream cipher and xor is how we encrypt, but its weird that the callsite knows that).

Done though needed an in-place version for this new code.

lightning/src/ln/inbound_payment.rs

TheBlueMatt · 2023-08-23T23:30:54Z

lightning/src/offers/offer.rs

@@ -169,7 +170,7 @@ impl<'a, T: secp256k1::Signing> OfferBuilder<'a, DerivedMetadata, T> {
 		secp_ctx: &'a Secp256k1<T>
 	) -> Self where ES::Target: EntropySource {
 		let nonce = Nonce::from_entropy_source(entropy_source);
-		let derivation_material = MetadataMaterial::new(nonce, expanded_key, IV_BYTES);
+		let derivation_material = MetadataMaterial::new(nonce, expanded_key, IV_BYTES, None);


Unrelated to this PR, but is there no way in BOLT12 to use payment_metadata - ie encode something which will come back to the ultimate recipient in the onion? Our users can currently use payment_metadata as a sort of payment_id for inbound payments, but it seems like in BOLT12 they can't?

IIUC, it can go into the BlindedPaths inside the Bolt12Invoice. That's how we do stateless inbound payments for offers, at least. We could include additional data there.

Oh, duh, right. Yea, we'll want to make sure our pipeline there lets us include a PaymentMetadata directly, I think (cc @valentinewallace)

Def, added tracking for this to #1970.

TheBlueMatt · 2023-08-23T23:52:34Z

lightning/src/offers/signer.rs

@@ -227,5 +325,5 @@ fn hmac_for_message<'a>(
 		hmac.input(DERIVED_METADATA_HMAC_INPUT);


Unrelated to this PR and I cant comment there, but we have a type confusion issue above - we only actually include each TLV's value in the hmac, not the type, we absolutely need to include both.

TlvRecord::record_bytes contains the type and is included in the hmac above:

for record in tlv_stream { hmac.input(record.record_bytes); }

Ah, okay, didnt check the tlv stream, easy to confuse record_bytes for not including the type :)

lol... #1738 (comment)

Ha, well I agree record_bytes is better than data, but I still find it confusing :)

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2023-08-24T00:11:22Z

lightning/src/ln/outbound_payment.rs

+		let entry = pending_outbounds.entry(payment_id);
+		if let hash_map::Entry::Occupied(entry) = &entry {
+			if let PendingOutboundPayment::AwaitingInvoice { .. } = entry.get() { } else {
+				return Err(PaymentSendFailure::DuplicatePayment)


Lets use a separate method for upgrading from AwaitingInvoice to Retryable, I dont want someone to call send_payment with a BOLT11 after they send a BOLT12 and double-pay somehow.

Could you describe the scenario in more detail? Note that there is a send_payment_for_bolt12_invoice method added in the next PR.

If we call add_new_awaiting_invoice (via some pub ChannelManager::send_bolt12_payment), we get a PendingOutboundPayment::AwaitingInvoice-state payment. Now, if the user calls send_payment with a BOLT11 payment with the same payment id, add_new_pending_payment will overwrite our previous payment, thinking we've received the BOLT12 invoice. While I don't know why someone would do this, its definitely super unexpected behavior given the payment ids are explicitly an "idempotency token", and here we're not being idempotent.

Ah, I see... Unfortunately, while trying to reuse send_payment_internal, add_new_pending_payment would be called in both cases, so this will need some refactoring unless we are ok duplicating code.

It would be nice to perform the check upfront such that the code can be reused still without needing to do something like pipe a is_bol12 parameter through the helpers. But even so, it seems we don't hold the pending_outbound_payments lock throughout these calls. So there's probably edge cases where the check is valid initially but may not be valid later if the locks isn't held throughout.

Would it make sense to hold on to the locks and pass a hash map entry (either occupied or vacant) to the helper functions? Or is there a reason why we drop and re-acquire the lock throughout these helpers? (cc: @valentinewallace )

Unfortunately, while trying to reuse send_payment_internal, add_new_pending_payment would be called in both cases, so this will need some refactoring unless we are ok duplicating code.

I'm not convinced we should be calling through send_payment_internal - that method is set up to return early with a RetryableSendFailure for immediate return to the user, but at this point we're handling a bolt12 invoice and don't want to return immediately. Rather, retry_payment_internal is set to abandon_payment, creating a PaymentFailed event on failure and not returning something we'd have to figure out how to handle.

Ah, that makes sense. Ok, reverted but will be in the next PR as per #2468 (comment).

lightning/src/offers/invoice_request.rs

lightning/src/ln/outbound_payment.rs

TheBlueMatt · 2023-08-28T18:53:55Z

I think I'm happy with this, feel free to squash IMO.

lightning/src/util/chacha20.rs

valentinewallace · 2023-08-29T14:33:01Z

lightning/src/offers/invoice_request.rs

+	/// If `Some`, must call [`respond_using_derived_keys`] when responding. Otherwise, call
+	/// [`respond_with`].


Could we do something similar to SigningPubkeyStrategy to only expose the right method for the given verified invreq?

Not possible as this involves a dynamic check on the metadata. Ultimately, it comes from bytes that need to be parsed opposed to using statically constructed objects.

lightning/src/offers/invoice_request.rs

valentinewallace · 2023-08-29T15:02:18Z

lightning/src/offers/invoice_request.rs

@@ -556,6 +579,63 @@ impl InvoiceRequest {
 		InvoiceBuilder::for_offer(self, payment_paths, created_at, payment_hash)
 	}

+	/// Verifies that the request was for an offer created using the given key. Returns the verified


Can't comment on the right line, but it seems like if the offer was created using derived keys then users should not directly call InvReq::respond_with and should instead call verify first. If that's the case, I think InvReq::respond_with could use a docs note.

They could try to call it, but they would have a hard time signing the invoice. 😛 Added some more docs.

InvoiceRequest::verify_and_respond_using_derived_keys takes a payment hash. To avoid generating one for invoice requests that ultimately cannot be verified, split the method into one for verifying and another for responding.

Metadata such as the PaymentId should be encrypted when included in an InvoiceRequest or a Refund, as it is user data and is exposed to the payment recipient. Add an encryption key to ExpandedKey for this purpose instead of reusing offers_base_key.

This hides an encryption implementation detail from callers.

Similar to ChaCha20::encrypt_single_block only encrypts in-place.

When receiving a BOLT 12 invoice originating from either an invoice request or a refund, the invoice should only be paid once. To accomplish this, require that the invoice includes an encrypted payment id in the payer metadata. This allows ChannelManager to track a payment when requesting but prior to receiving the invoice. Thus, it can determine if the invoice has already been paid.

jkczyz mentioned this pull request Aug 2, 2023

BOLT 12 Invoice payments #2371

Merged

jkczyz requested review from valentinewallace and TheBlueMatt August 2, 2023 00:48

jkczyz added this to the 0.0.117 milestone Aug 7, 2023

valentinewallace reviewed Aug 8, 2023

View reviewed changes

TheBlueMatt reviewed Aug 14, 2023

View reviewed changes

jkczyz force-pushed the 2023-08-offer-payment-id branch from 8958209 to e0bdae9 Compare August 17, 2023 21:56

jkczyz force-pushed the 2023-08-offer-payment-id branch from e0bdae9 to fadf972 Compare August 23, 2023 21:14

jkczyz force-pushed the 2023-08-offer-payment-id branch from fadf972 to 4ebb061 Compare August 23, 2023 22:24

TheBlueMatt reviewed Aug 24, 2023

View reviewed changes

jkczyz force-pushed the 2023-08-offer-payment-id branch from 4ebb061 to 3443e5e Compare August 24, 2023 22:22

valentinewallace reviewed Aug 28, 2023

View reviewed changes

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

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

jkczyz force-pushed the 2023-08-offer-payment-id branch from 3443e5e to 840729d Compare August 28, 2023 18:51

jkczyz force-pushed the 2023-08-offer-payment-id branch from 840729d to c805fc8 Compare August 28, 2023 19:23

TheBlueMatt previously approved these changes Aug 29, 2023

View reviewed changes

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

jkczyz dismissed TheBlueMatt’s stale review via 098ad57 August 29, 2023 03:34

jkczyz force-pushed the 2023-08-offer-payment-id branch from c805fc8 to 098ad57 Compare August 29, 2023 03:34

valentinewallace reviewed Aug 29, 2023

View reviewed changes

jkczyz added 6 commits August 29, 2023 11:08

Split InvoiceRequest::verify_and_respond_using_derived_keys

21fa551

InvoiceRequest::verify_and_respond_using_derived_keys takes a payment hash. To avoid generating one for invoice requests that ultimately cannot be verified, split the method into one for verifying and another for responding.

Remove unnecessary #[allow(unused)]

971cb20

Add a ChaCha20 utility for encrypting a block

4732484

This hides an encryption implementation detail from callers.

Add a ChaCha20 utility for encrypting in place

861e0ee

Similar to ChaCha20::encrypt_single_block only encrypts in-place.

jkczyz force-pushed the 2023-08-offer-payment-id branch from 098ad57 to 7a3e06b Compare August 29, 2023 16:08

valentinewallace approved these changes Aug 29, 2023

View reviewed changes

TheBlueMatt approved these changes Aug 29, 2023

View reviewed changes

TheBlueMatt merged commit 073f078 into lightningdevkit:main Aug 29, 2023
12 of 14 checks passed

jkczyz changed the title ~~Offer outbound payments~~ Offer outbound PaymentId Sep 6, 2023

jkczyz changed the title ~~Offer outbound PaymentId~~ BOLT 12 outbound PaymentId Sep 6, 2023

vnprc mentioned this pull request Sep 7, 2023

TABConf Lightning Socratic Seminar TriangleBitDevs/TriangleBitDevs.github.io#12

Closed

btsea mentioned this pull request Sep 8, 2023

Socratic Session: Lightning Strikes! A Discussion of Recent Developments in the Lightning Ecosystem TABConf/2023.tabconf.com#190

Closed

jkczyz mentioned this pull request Sep 18, 2023

BOLT12 Offers Tracking Issue #1970

Open

60 tasks

@@ @@ -1437,6 +1448,17 @@ impl MaybeReadable for Event { @@
               				};
               				f()
               			},
+u8 => {

		@@ -227,5 +325,5 @@ fn hmac_for_message<'a>(
		hmac.input(DERIVED_METADATA_HMAC_INPUT);

		/// If `Some`, must call [`respond_using_derived_keys`] when responding. Otherwise, call
		/// [`respond_with`].

BOLT 12 outbound PaymentId #2468

BOLT 12 outbound PaymentId #2468

Conversation

jkczyz commented Aug 2, 2023 • edited Loading

valentinewallace 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

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

codecov-commenter commented Aug 17, 2023 • edited Loading

Codecov Report

TheBlueMatt commented Aug 23, 2023

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

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 commented Aug 28, 2023

Choose a reason for hiding this comment

jkczyz Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BOLT 12 outbound `PaymentId` #2468

BOLT 12 outbound `PaymentId` #2468

jkczyz commented Aug 2, 2023 •

edited

Loading

codecov-commenter commented Aug 17, 2023 •

edited

Loading

jkczyz Aug 29, 2023 •

edited

Loading