BOLT 12 `invoice_request` encoding and building #1738

jkczyz · 2022-09-23T23:08:11Z

Define an interface for BOLT 12 invoice_request messages. The underlying format consists of the original bytes and the parsed contents.

The bytes are later needed when constructing an invoice message. This is because it must mirror all the offer and invoice_request TLV records, including unknown ones, which aren't represented in the contents.

The contents will be used in invoice messages to avoid duplication. Some fields while required in a typical user-pays-merchant flow may not be necessary in the merchant-pays-user flow (e.g., refund, ATM).

Also, defines a builder for constructing an invoice request from an offer given a required payer id.

See #1597 for ongoing BOLT 12 work depending on this PR.

codecov-commenter · 2022-09-23T23:18:22Z

Codecov Report

Base: 90.59% // Head: 92.16% // Increases project coverage by +1.56% 🎉

Coverage data is based on head (39f65d6) compared to base (087c0bd).
Patch coverage: 94.86% of modified lines in pull request are covered.

❗ Current head 39f65d6 differs from pull request most recent head a2147d8. Consider uploading reports for the commit a2147d8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1738      +/-   ##
==========================================
+ Coverage   90.59%   92.16%   +1.56%     
==========================================
  Files          91       94       +3     
  Lines       47965    61379   +13414     
  Branches    47965    61379   +13414     
==========================================
+ Hits        43453    56567   +13114     
- Misses       4512     4812     +300

Impacted Files	Coverage Δ
lightning/src/ln/features.rs	`99.67% <ø> (ø)`
lightning/src/util/ser_macros.rs	`87.19% <0.00%> (ø)`
lightning/src/offers/payer.rs	`33.33% <33.33%> (ø)`
lightning/src/util/ser.rs	`91.84% <90.00%> (-0.09%)`	⬇️
lightning/src/offers/offer.rs	`93.45% <90.52%> (+1.67%)`	⬆️
lightning/src/offers/invoice_request.rs	`95.00% <95.00%> (ø)`
lightning/src/offers/merkle.rs	`100.00% <100.00%> (ø)`
lightning/src/offers/parse.rs	`94.00% <100.00%> (+0.52%)`	⬆️
lightning/src/util/persist.rs	`94.44% <0.00%> (-0.80%)`	⬇️
lightning/src/chain/keysinterface.rs	`90.06% <0.00%> (-0.69%)`	⬇️
... and 34 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lightning/src/offers/invoice_request.rs

TheBlueMatt

Doesn't have to happen in this PR (and probably shouldn't), but we need fuzz tests for all the new deserialization things before we flip the offers module public.

lightning/src/util/ser.rs

TheBlueMatt · 2022-11-22T19:24:28Z

lightning/src/offers/merkle.rs

+	let nonce_tag = tagged_hash_engine(sha256::Hash::from_engine({
+		let mut engine = sha256::Hash::engine();
+		engine.input("LnNonce".as_bytes());
+		engine.input(tlv_stream.peek().unwrap().type_bytes);


Two questions -
a) is this unwrap definitely going to always remain safe? Should we instead just make it an if statement to make it trivial?
b) wait, what? So now we add a nonce to each leaf that is only the first TLV record in the offer/invoice/etc? That doesn't seem sufficient, no? The nonce needs to be unpredictable such that an attacker can't reasonably guess it, AFAIU, is the first TLV record always unpredictable?

Two questions -
a) is this unwrap definitely going to always remain safe? Should we instead just make it an if statement to make it trivial?

Hmm... it should always be safe because invreq_metadata is type 0 and MUST be set. But it appears I'm not checking that. 😞

b) wait, what? So now we add a nonce to each leaf that is only the first TLV record in the offer/invoice/etc? That doesn't seem sufficient, no? The nonce needs to be unpredictable such that an attacker can't reasonably guess it, AFAIU, is the first TLV record always unpredictable?

It should always be invreq_metadata, so the BOLT says: "It's also the first entry (if present), ensuring an unpredictable nonce for hashing." Not sure why "if present" is mentioned, though.

Right, so if we don't include a metadata then the merkle tree proof construction thing is unsafe? That would imply that the metadata should be filled in by LDK, I think.

Pushed fixups to make it required.

Cool, thanks. I assume we'll eventually have utilities to construct offers that will include some crypto to validate them and then we'll handle this entirely for users?

Yeah, at least two utilities:

one for creating a stateless offer which would populate the offer_metadata such that it can be checked when handling an invoice_request message.

another for constructing an invoice_request which would populate invreq_metadata such that it can be checked when handling an invoice message.

lightning/src/offers/merkle.rs

TheBlueMatt · 2022-11-22T19:33:46Z

lightning/src/offers/merkle.rs

+			break;
+		}
+
+		for (i, j) in (0..num_leaves).step_by(step).zip((offset..num_leaves).step_by(step)) {


nit: I find this way harder to read than just writing out the for loop and multiplying the values by step.

Refactored into two local variables for left and right leaves to improve readability. The zip is nice because it seamlessly handles the case where there is an odd number of nodes on the level.

lightning/src/offers/offer.rs

TheBlueMatt · 2022-11-22T19:43:42Z

lightning/src/offers/invoice_request.rs

+	}
+
+	/// Sets the metadata for the invoice request. Useful for containing information about the
+	/// derivation of [`InvoiceRequest::payer_id`]. This should not leak any information such as


This doesn't really entirely tell me what this is for - why do need to store the derivation info? Where will I get this info back in the future? Why should it not leak any info?

Took a stab at rewriting this. The part about leaking was taken from the BOLT, but I'm not exactly sure the scenario that was being considered. Assumed it had something to do with payment correlation, but I think leaking that alone wouldn't be sufficient.

TheBlueMatt · 2022-11-22T19:45:02Z

lightning/src/offers/invoice_request.rs

+	}
+
+	/// Sets a quantity of items for the invoice request. If not set, `1` is assumed. Must conform
+	/// to [`Offer::is_valid_quantity`].


Shouldn't that mean this returns a Result and refuses to set the quantity if its going to be invalid?

Initially, I did so but it made it difficult to construct invalid requests in tests. So I moved it to build which also makes the interface a bit more consistent and less difficult for bindings. I can update the docs if you'd like.

I find it kinda strange that you can do something invalid and you only find out later, honestly. Can we just have a test-only method that skips the check?

Fair point. Updated to handle it upfront and use test-only method in the parsing tests setup. I'd like to limit these sort of checks to those that can performed against an already constructed object -- in this case an Offer -- rather than partially constructed like the rest of the InvoiceRequestContents.

lightning/src/offers/invoice_request.rs

TheBlueMatt · 2022-11-22T19:47:03Z

lightning/src/offers/invoice_request.rs

+	/// using a simple BIP-32 derivation path.
+	///
+	/// Successive calls to this method will override the previous setting.
+	pub fn metadata(mut self, metadata: Vec<u8>) -> Self {


We're gonna have to think about how we do bindings here - most languages don't support move semantics so we'll have to build a parallel builder for bindings, I guess?

Hmm... I was afraid you would say that. 😛 I'm not a fan of returning &mut Self since it requires creating a builder as mut before chaining the calls in a separate statement. For bindings, I suppose we could export a different struct that wraps the builder, having identical methods returning &mut Self, which internally would delegate as appropriate.

If we're gonna do bindings-only versions we shouldn't even bother returning &mut Self, just do set-ers.

I don't really buy that supporting builders is so important that we need to duplicate all our methods just for them, honestly. They're nice, but they're not that nice.

Sure, setters would be simpler. Will punt on this until after all builder PRs are complete to limit their size.

valentinewallace

Still going through a first pass, nothing major

lightning/src/offers/invoice_request.rs

lightning/src/util/ser_macros.rs

lightning/src/offers/invoice_request.rs

lightning/src/offers/payer.rs

lightning/src/offers/parse.rs

valentinewallace · 2022-12-02T16:59:19Z

lightning/src/offers/invoice_request.rs

+	///
+	/// Successive calls to this method will override the previous setting.
+	pub fn quantity(mut self, quantity: u64) -> Result<Self, SemanticError> {
+		self.invoice_request.offer.check_quantity(Some(quantity))?;


Doesn't the same reasoning apply to amount_msats and chain, so we shouldn't allow setting an invalid value those places too?

Hmm... yeah, that's a good question (cc @TheBlueMatt ). There's a few different scenarios:

an InvoiceRequest field's validity is determined by an Offer field (e.g., quantity and quantity_max, chain and chains)

an InvoiceRequest field's validity is also determined by another InvoiceRequest field (e.g., amount_msats and quantity).

an InvoiceRequest can be constructed only based on an Offer field (e.g., absolute_expiry)

For the first scenario, we can check at time of setting the field, but also need to check during build, in case the Offer expects the field to be set but it never was (e.g., fail if the Offer expects a quantity).

For the second scenario, checking (e.g., amount_msats) against the Offer is only sufficient if the second field (e.g., quantity) won't be set. If it will be, the check would need to be conditionally checked when the second field is set and also checked in build. Conditionally, because we can't assume the setting order. Other possibilities are to (a) not check when setting the second field but do check in build or (b) design the API so both fields must be set in the same call when both are expected. In practice, the user would need to examine the Offer before determining if setting the field (e.g., quantity) is needed.

For the third scenario, the check could be in either new or build, though doing, for example, the expiry check in build means you can construct a builder that will always fail. Could avoid this by having InvoiceRequestBuilder::new, and thus also Offer::request_invoice, return a Result. FWIW, it's marginally better in build for the expiry check example if the time between construction and building is far apart. Though I suppose the check could be performed in both places.

Sorry, that's a lot to consider. 😬 For now, I'll go ahead an check whenever possible with the exception of expiry since it's not really static. Let me know if you prefer something else.

Ok, checking amount_msats when setting is problematic because we don't check the Offer currency until calling build. We can duplicated the check, of course. But thinking about this more, it seems strange to emit UnsupportedCurrency and InsufficientAmount when conditionally performing the same checks when setting quantity.

For consistency, I'm starting to prefer my original approach of only checking in build. Alternatively, we could be somewhat inconsistent and only check in setters when:

all information is available

but only for errors related to the field being set

(e.g., quantity, chain, amount_msats when quantity is already set or not expected)

But otherwise not.

(e.g., amount_msats when quantity is expected but hasn't been set, amount_msats when setting quantity)

What do ya'll think?

Wait, why can't we check the offer currency on new? The offer isn't going to change between when we start the builder and when we call build, so we could fail then? I don't feel super strongly here. ISTM we should fail when we get the failing parameter, but I don't think any of these are likely to actually result in materially different user code. maybe there's a world where someone has an invoice request building UI that has a builder in the background and calls the various set methods when the user types into the relevant field, but I'm not sure that's actually likely.

Checking it in a new would work, too. Though it's nice to have the currency checked in the same place given the other checks involve calling the code with the unreachable path, which could be eliminated if combined. Pushed an untested version for consideration. May need to add more test-only unchecked setters.

Pushed an untested version for consideration

I'm confused, I dont see any tests in InvoiceRequestBuilder::new?

Ah, sorry. Meant that I pushed a version that does the checks in the amount_msats and build such that they are reused in both places and in the parsing code. This removes the unreachable code from Offer.

This also now fully tested.

lightning/src/offers/offer.rs

lightning/src/offers/merkle.rs

lightning/src/offers/offer.rs

lightning/src/offers/invoice_request.rs

valentinewallace · 2022-12-02T19:39:40Z

lightning/src/offers/invoice_request.rs

+
+impl<'a> UnsignedInvoiceRequest<'a> {
+	/// Signs the invoice request using the given function.
+	pub fn sign<F>(self, sign: F) -> Result<InvoiceRequest, secp256k1::Error>


Will we have a utility that takes a KeysManager in the future?

Yeah, that's the plan.

valentinewallace · 2022-12-02T19:41:09Z

lightning/src/offers/merkle.rs

+	sign: F, tag: &str, bytes: &[u8], pubkey: PublicKey,
+) -> Result<Signature, secp256k1::Error>
+where
+	F: FnOnce(&Message) -> Signature


FWIW, lightning-invoice allows its sign closure to error

Yeah, I suppose we should allow errors for something like remote signing? Guess we need to allow for custom errors then? Or do you prefer a different approach?

I'm fine with the lightning-invoice approach of custom error, or just () as a stopgap I suppose

Added SignError.

Seen when removing `#[allow(unused)]` from `offers` module.

lightning/src/offers/merkle.rs

valentinewallace · 2022-12-07T18:36:06Z

lightning/src/offers/invoice_request.rs

+	}
+
+	/// Sets the [`InvoiceRequest::amount_msats`] for paying an invoice. Must be at least the base
+	/// invoice amount (i.e., [`Offer::amount`] times [`quantity`]).


nit:

Suggested change

/// invoice amount (i.e., [`Offer::amount`] times [`quantity`]).

/// invoice amount (i.e., [`Offer::amount`]) times [`quantity`].

Parenthesization is correct but the terminology was outdated: s/base/expected

lightning/src/offers/invoice_request.rs

valentinewallace · 2022-12-07T20:00:12Z

lightning/src/offers/invoice_request.rs

+		let chain = self.invoice_request.chain();
+		if !self.offer.supports_chain(chain) {
+			return Err(SemanticError::UnsupportedChain);
+		}
+
+		if chain == self.offer.implied_chain() {
+			self.invoice_request.chain = None;
+		}
+
+		if self.offer.amount().is_none() && self.invoice_request.amount_msats.is_none() {
+			return Err(SemanticError::MissingAmount);
+		}
+
+		self.invoice_request.offer.check_quantity(self.invoice_request.quantity)?;


I don't feel strongly, but (at least some of) these checks and maybe below seem redundant to me now(?)

I think most are still required in cases where a setter wasn't used or ordered in a certain way. But some of the checks in check_amount_msats may not be needed if quantity was set before amount_msats. Would prefer to leave the redundancy to keep the code DRY and because ordering of setters will make it difficult to determine which would be required.

lightning/src/offers/invoice_request.rs

valentinewallace · 2022-12-07T20:41:25Z

Feel free to squash IMO.

valentinewallace

LGTM after @TheBlueMatt takes another look

valentinewallace · 2022-12-08T22:21:51Z

lightning/src/offers/invoice_request.rs

+	#[cfg(test)]
+	pub fn features(mut self, features: InvoiceRequestFeatures) -> Self {
+		self.invoice_request.features = features;
+		self
+	}


nit: I think this can go in the new cfg(test) impl InvoiceRequest { .. }

Kept this out on purpose as it may eventually be exposed as part of the interface whereas the others will always be test-only.

Oh, I thought our plan was to have methods for individual feature bits like lightning-invoice's basic_mpp(), but don't mind too much where it goes

Ah, true... 😅 Yeah, renamed and moved for consistency.

TheBlueMatt · 2022-12-08T23:13:40Z

Can you squash?

Define an interface for BOLT 12 `invoice_request` messages. The underlying format consists of the original bytes and the parsed contents. The bytes are later needed when constructing an `invoice` message. This is because it must mirror all the `offer` and `invoice_request` TLV records, including unknown ones, which aren't represented in the contents. The contents will be used in `invoice` messages to avoid duplication. Some fields while required in a typical user-pays-merchant flow may not be necessary in the merchant-pays-user flow (e.g., refund, ATM).

BOLT 12 uses Schnorr signatures for signing offers messages, which need to be serialized.

Offers uses a merkle root hash construction for signature calculation and verification. Add a submodule implementing this so that it can be used when parsing and signing invoice_request and invoice messages.

When reading an offer, an `invoice_request` message is sent over the wire. Implement Writeable for encoding the message and TryFrom for decoding it by defining in terms of TLV streams. These streams represent content for the payer metadata (0), reflected `offer` (1-79), `invoice_request` (80-159), and signature (240).

Add a builder for creating invoice requests for an offer given a payer_id. Other settings may be optional depending on the offer and duplicative settings will override previous settings. Building produces a semantically valid `invoice_request` message for the offer, which then can be signed for the payer_id.

jkczyz · 2022-12-09T15:18:04Z

Ok, CI should pass now. Was just failing on doc links.

Tests for checking invoice_request message semantics when building as defined by BOLT 12.

Tests for checking invoice_request message semantics when parsing bytes as defined by BOLT 12.

A BOLT 12 test vector uses an `invoice_request` message that has a currency, which aren't supported, so using OfferBuilder::build_unchecked is required to avoid a panic.

jkczyz · 2022-12-09T19:40:29Z

$ git diff-tree -U2 a2147d82 b25c8df6
diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs
index 0d9c777f..90f6c183 100644
--- a/lightning/src/offers/invoice_request.rs
+++ b/lightning/src/offers/invoice_request.rs
@@ -560,5 +560,5 @@ mod tests {
 
 		if let Err(e) = InvoiceRequest::try_from(buffer) {
-			panic!("error parsing offer: {:?}", e);
+			panic!("error parsing invoice request: {:?}", e);
 		}
 	}
@@ -897,5 +897,5 @@ mod tests {
 			.request_invoice(vec![1; 32], payer_pubkey()).unwrap()
 			.build().unwrap()
-			.sign(|digest| Err(()))
+			.sign(|_| Err(()))
 		{
 			Ok(_) => panic!("expected error"),

TheBlueMatt

🎉

jkczyz force-pushed the 2022-09-invoice-request branch from 6c08907 to 9c60c5a Compare September 29, 2022 15:40

jkczyz commented Sep 30, 2022

View reviewed changes

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

jkczyz force-pushed the 2022-09-invoice-request branch 2 times, most recently from 91c2906 to 5fcd9f3 Compare October 7, 2022 04:49

jkczyz force-pushed the 2022-09-invoice-request branch from 5fcd9f3 to d0da097 Compare October 7, 2022 21:08

jkczyz mentioned this pull request Oct 7, 2022

BOLT 12 offer parsing #1726

Merged

jkczyz force-pushed the 2022-09-invoice-request branch from d0da097 to a16cc51 Compare October 20, 2022 14:51

jkczyz force-pushed the 2022-09-invoice-request branch 3 times, most recently from 29da048 to bff159f Compare November 1, 2022 19:10

jkczyz force-pushed the 2022-09-invoice-request branch 4 times, most recently from 49871a1 to 3685b47 Compare November 10, 2022 15:02

jkczyz force-pushed the 2022-09-invoice-request branch 5 times, most recently from bcf17f2 to f6bb912 Compare November 18, 2022 21:42

jkczyz marked this pull request as ready for review November 18, 2022 21:42

TheBlueMatt reviewed Nov 22, 2022

View reviewed changes

jkczyz force-pushed the 2022-09-invoice-request branch from f6bb912 to e2cf8fe Compare November 29, 2022 04:17

TheBlueMatt added the Seeking Code Review label Nov 29, 2022

jkczyz force-pushed the 2022-09-invoice-request branch 2 times, most recently from a2632bd to fbe0275 Compare December 1, 2022 05:42

valentinewallace reviewed Dec 1, 2022

View reviewed changes

valentinewallace reviewed Dec 2, 2022

View reviewed changes

jkczyz added 2 commits December 2, 2022 15:26

Reduce visibility for offer auxiliary types

4b8b17d

Remove unused mut from OfferBuilder::amount_msats

3e17e7f

Seen when removing `#[allow(unused)]` from `offers` module.

jkczyz force-pushed the 2022-09-invoice-request branch from fbe0275 to 8f3026a Compare December 5, 2022 22:47

TheBlueMatt removed the Seeking Code Review label Dec 6, 2022

jkczyz force-pushed the 2022-09-invoice-request branch 2 times, most recently from f2d3f65 to a08e75b Compare December 7, 2022 16:31

valentinewallace reviewed Dec 7, 2022

View reviewed changes

jkczyz force-pushed the 2022-09-invoice-request branch 3 times, most recently from 7062a69 to 06bf0f9 Compare December 8, 2022 21:25

valentinewallace reviewed Dec 8, 2022

View reviewed changes

jkczyz force-pushed the 2022-09-invoice-request branch 2 times, most recently from 39f65d6 to 9964692 Compare December 9, 2022 04:11

jkczyz added 5 commits December 9, 2022 08:53

Schnorr Signature serialization

05bf27f

BOLT 12 uses Schnorr signatures for signing offers messages, which need to be serialized.

Merkle root hash computation

a7adc76

Offers uses a merkle root hash construction for signature calculation and verification. Add a submodule implementing this so that it can be used when parsing and signing invoice_request and invoice messages.

jkczyz force-pushed the 2022-09-invoice-request branch from 9964692 to a2147d8 Compare December 9, 2022 14:54

jkczyz added 3 commits December 9, 2022 13:28

Invoice request building tests

d666eb6

Tests for checking invoice_request message semantics when building as defined by BOLT 12.

Invoice request parsing tests

984c906

Tests for checking invoice_request message semantics when parsing bytes as defined by BOLT 12.

Add BOLT 12 merkle root test for invoice_request

b25c8df

A BOLT 12 test vector uses an `invoice_request` message that has a currency, which aren't supported, so using OfferBuilder::build_unchecked is required to avoid a panic.

jkczyz force-pushed the 2022-09-invoice-request branch from a2147d8 to b25c8df Compare December 9, 2022 19:30

TheBlueMatt approved these changes Dec 12, 2022

View reviewed changes

valentinewallace approved these changes Dec 12, 2022

View reviewed changes

valentinewallace merged commit 4a0010d into lightningdevkit:main Dec 12, 2022

jkczyz mentioned this pull request May 10, 2023

BOLT12 Offers Tracking Issue #1970

Open

60 tasks

jkczyz mentioned this pull request Aug 25, 2023

BOLT 12 outbound PaymentId #2468

Merged

	/// invoice amount (i.e., [`Offer::amount`] times [`quantity`]).
	/// invoice amount (i.e., [`Offer::amount`]) times [`quantity`].

BOLT 12 invoice_request encoding and building #1738

BOLT 12 invoice_request encoding and building #1738

Conversation

jkczyz commented Sep 23, 2022

codecov-commenter commented Sep 23, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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 7, 2022

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

TheBlueMatt commented Dec 8, 2022

jkczyz commented Dec 9, 2022

jkczyz commented Dec 9, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

BOLT 12 `invoice_request` encoding and building #1738

BOLT 12 `invoice_request` encoding and building #1738

codecov-commenter commented Sep 23, 2022 •

edited

Loading