Experimental offer TLVs #3237

jkczyz · 2024-08-12T14:19:38Z

The BOLT12 spec defines experimental TLV ranges that are allowed in messages. Allow this range when parsing those messages and include those bytes in subsequent messages (i.e., experimental offer TLVs are included in an invreq after the signature). Account for these bytes when computing metadata, verifying messages, and constructing OfferIds.

~~Based on #3218.~~
Fixes #3168.

codecov · 2024-08-12T15:16:21Z

Codecov Report

Attention: Patch coverage is 90.74273% with 86 lines in your changes missing coverage. Please review.

Project coverage is 89.69%. Comparing base (605952c) to head (fda4b8d).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/ser.rs	18.18%	44 Missing and 10 partials ⚠️
lightning/src/offers/invoice.rs	96.25%	4 Missing and 7 partials ⚠️
lightning/src/offers/invoice_request.rs	96.77%	5 Missing and 4 partials ⚠️
lightning/src/offers/offer.rs	96.19%	5 Missing and 2 partials ⚠️
lightning/src/offers/refund.rs	94.68%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3237      +/-   ##
==========================================
+ Coverage   89.68%   89.69%   +0.01%     
==========================================
  Files         126      127       +1     
  Lines      103306   107769    +4463     
  Branches   103306   107769    +4463     
==========================================
+ Hits        92648    96664    +4016     
- Misses       7945     8381     +436     
- Partials     2713     2724      +11

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

jkczyz · 2024-10-02T16:51:43Z

Rebased

TheBlueMatt

I find this pretty hard to review because several commits rewrite code that was rewritten in previous commits, making it hard to figure out what's wrong or what's just wrong until the last commit.

lightning/src/offers/static_invoice.rs

lightning/src/offers/invoice.rs

TheBlueMatt · 2024-10-02T20:24:27Z

lightning/src/offers/invoice_request.rs

-		let tagged_hash = TaggedHash::from_valid_tlv_stream_bytes(SIGNATURE_TAG, &bytes);
+		let experimental_bytes = Vec::new();
+
+		let tlv_stream = TlvStream::new(&bytes).chain(TlvStream::new(&experimental_bytes));


I find this really hard to read as of this commit - we're writing all the contents into bytes but then using an empty vec for the experimental set? Why is that not gonna result in TLVs in the wrong buffer? Later on there's a new set returned by as_tlv_stream but by doing that in a separate commit this is really hard to review.

At this point, we will fail parsing any Offer with TLVs in the experimental range. So offer_bytes will not contain any experimental TLVs.

It seemed easier to reason about by adding experimental_bytes in a dedicated commit. Otherwise, the change gets lost in the parsing commits (e.g., 927c477), which are large changesets.

Also note that we never use parts from as_tlv_stream that don't originated from us. We need to use the bytes instead because they may contain unknown odd TLVs.

It seemed easier to reason about by adding experimental_bytes in a dedicated commit. Otherwise, the change gets lost in the parsing commits (e.g., 927c477), which are large changesets.

Currently with things repeatedly changing in later commits I find myself reviewing the entire PR as one big diff because I can't keep track of what is going to stay the same and what's gonna change by the end, so I'm definitely okay with things being a part of larger commits as long as things aren't getting rewritten twice in the same PR :/

Hrmm... I guess I can squash this commit into 927c477. The only other time this changes is trivially in d9a9e9d to add parsing support for the invoice request experimental range. Alternatively, I could move the logic added in 927c477 into this commit, which I've now done.

Added another fixup for UnsignedStaticInvoice.

Feel free to squash all the fixups and commits and I'll review again from the top anyway.

TheBlueMatt · 2024-10-02T20:29:32Z

lightning/src/offers/merkle.rs

@@ -254,6 +254,7 @@ pub(super) struct TlvRecord<'a> {
 	type_bytes: &'a [u8],
 	// The entire TLV record.
 	pub(super) record_bytes: &'a [u8],
+	pub(super) end: usize,


This looks unused in this commit.

Used in try_from for UnsignedBolt12Invoice and UnsignedInvoiceRequest.

TheBlueMatt · 2024-10-02T20:32:05Z

lightning/src/offers/invoice.rs

@@ -492,17 +492,26 @@ where

 impl UnsignedBolt12Invoice {
 	fn new(invreq_bytes: &[u8], contents: InvoiceContents) -> Self {
+		const NON_EXPERIMENTAL_TYPES: core::ops::Range<u64> = 0..INVOICE_REQUEST_TYPES.end;
+		const EXPERIMENTAL_TYPES: core::ops::Range<u64> = EXPERIMENTAL_OFFER_TYPES;


Shouldn't this be EXPERIMENTAL_OFFER_TYPES.start - EXPERIMENTAL_INVOICE_REQUEST_TYPES.end?

Nope... at this point we've only added support for parsing experimental offer TLVs.

Right, which makes reviewing this patchset pretty confusing - we add things that don't really make sense as of the commit they're added in, and changed later to be correct. I'm not sure how to review that except to review the whole PR in one big diff-tree.

I don't believe that is the case. Before this commit we didn't support parsing experimental types and after it we support experimental types in the offer range as indicated by this line and the commit message. In a later commit the range is expanded to support parsing experimental types in the invoice request range. Correctness is maintained the entire way since we will fail to parse anything that includes data in a yet-to-be supported experimental range.

This now is set to the ultimate range upfront.

TheBlueMatt · 2024-10-02T20:33:39Z

lightning/src/offers/invoice.rs

 	) -> Result<PaymentId, ()> {
+		const EXPERIMENTAL_TYPES: core::ops::Range<u64> = EXPERIMENTAL_OFFER_TYPES;


Lets not rename constants to make them less specific.

This is later expanded to cover other experimental ranges as support is added.

Right, but at each turn the commits are confusing because they have a wrong value for where we're going. I find it really confusing to have to read some code, see one value, think hmm, that's not right, then go read the final state of the PR, realize no, indeed, it was not right, but its changed later, so I have no idea what the final code is actually going to look like.

This now is set to the ultimate range upfront.

TheBlueMatt · 2024-10-02T20:47:24Z

lightning/src/offers/invoice.rs

@@ -1684,7 +1684,9 @@ mod tests {
 					message_paths: None,
 				},
 				SignatureTlvStreamRef { signature: Some(&invoice.signature()) },
-				ExperimentalOfferTlvStreamRef {},
+				ExperimentalOfferTlvStreamRef {
+					experimental_foo: None,


Rather than testing by overriding specific fields, can we just add support for passing TLV streams into the builders? We'll need/want to support passing custom types soon anyway (in #2829 or a followup, at least).

Hmm... right, though it seems there's a larger question as what sort of interface we should have when reading such TLVs. I think my assumption was we'd want to directly add TLVs that we support. If it's odd and we don't know it, we can ignore it. If it's even and we don't know it, we'd failed to parse. Unless instead we want ways to configure support kinda like custom messages. But that seems like a much larger change.

Yea, I think we definitely should support passing custom TLVs, but I'm not sure it needs to be complicated. The logic in RecipientOnionFields to handle custom TLVs is pretty simple, and I'd imagined we'd duplicate basically that here.

I don't think we should do that in this PR. Sure, changing the builder isn't too difficult, but It's a significant change to the tlv_stream! macro to be able to support that. I'm not even sure if it can be done, TBH, without a substantial re-write since custom experimental TLVs may have types less than LDK-supported experimental TLVs.

Alright, sounds good.

jkczyz

I find this pretty hard to review because several commits rewrite code that was rewritten in previous commits, making it hard to figure out what's wrong or what's just wrong until the last commit.

I don't believe that the case anywhere. The commits were written to maintain correctness from start to finish. Do you have a specific example in mind?

jkczyz · 2024-10-04T15:06:13Z

lightning/src/offers/invoice.rs

@@ -492,17 +492,26 @@ where

 impl UnsignedBolt12Invoice {
 	fn new(invreq_bytes: &[u8], contents: InvoiceContents) -> Self {
+		const NON_EXPERIMENTAL_TYPES: core::ops::Range<u64> = 0..INVOICE_REQUEST_TYPES.end;
+		const EXPERIMENTAL_TYPES: core::ops::Range<u64> = EXPERIMENTAL_OFFER_TYPES;


I don't believe that is the case. Before this commit we didn't support parsing experimental types and after it we support experimental types in the offer range as indicated by this line and the commit message. In a later commit the range is expanded to support parsing experimental types in the invoice request range. Correctness is maintained the entire way since we will fail to parse anything that includes data in a yet-to-be supported experimental range.

jkczyz · 2024-10-04T15:31:30Z

lightning/src/offers/invoice_request.rs

-		let tagged_hash = TaggedHash::from_valid_tlv_stream_bytes(SIGNATURE_TAG, &bytes);
+		let experimental_bytes = Vec::new();
+
+		let tlv_stream = TlvStream::new(&bytes).chain(TlvStream::new(&experimental_bytes));


Hrmm... I guess I can squash this commit into 927c477. The only other time this changes is trivially in d9a9e9d to add parsing support for the invoice request experimental range. Alternatively, I could move the logic added in 927c477 into this commit, which I've now done.

jkczyz · 2024-10-04T20:12:08Z

lightning/src/offers/invoice.rs

@@ -1684,7 +1684,9 @@ mod tests {
 					message_paths: None,
 				},
 				SignatureTlvStreamRef { signature: Some(&invoice.signature()) },
-				ExperimentalOfferTlvStreamRef {},
+				ExperimentalOfferTlvStreamRef {
+					experimental_foo: None,


I don't think we should do that in this PR. Sure, changing the builder isn't too difficult, but It's a significant change to the tlv_stream! macro to be able to support that. I'm not even sure if it can be done, TBH, without a substantial re-write since custom experimental TLVs may have types less than LDK-supported experimental TLVs.

TheBlueMatt

This LGTM, I think. I started trying to review it per-commit again and got very very confused, so just gave up and reviewed the whole patchset, which is much easier, if also pretty annoying.

I do wonder if we're gonna end up wanting to change things a decent chunk when we go to actually support setting experimental entries in the structs. I assume we will, but probably not enough to merit changing this PR, though.

TheBlueMatt · 2024-10-16T18:51:52Z

lightning/src/offers/invoice.rs

@@ -491,19 +492,32 @@ where

 impl UnsignedBolt12Invoice {
 	fn new(invreq_bytes: &[u8], contents: InvoiceContents) -> Self {
+		const NON_EXPERIMENTAL_TYPES: core::ops::Range<u64> = 0..INVOICE_REQUEST_TYPES.end;
+		const EXPERIMENTAL_TYPES: core::ops::Range<u64> = 0..0;


Please don't add things and then change them in later commits, if at all possible.

Alright, I moved the constants ultimately used here up to this commit. Shouldn't affect the behavior since we don't support parsing messages with experimental TLV records at this point in the commit history.

TheBlueMatt · 2024-10-16T19:07:35Z

lightning/src/offers/invoice.rs

+		const NON_EXPERIMENTAL_TYPES: core::ops::Range<u64> = 0..INVOICE_REQUEST_TYPES.end;
+		const EXPERIMENTAL_TYPES: core::ops::Range<u64> = 0..0;
+
+		let mut bytes = Vec::new();


Unrelated to this PR (aside from it adding another vec), but rather than rebuilding the bytes here can we just store the invoice request (bytes) in the UnsignedBolt12Invoice and then do the work here in sign, building a single, new bytes with the invreq and invoice bytes in one go? Alternatively, we could iterate the bytes in sign to build the bytes without storing an extra vec here, but not sure its really worth it.

Possibly... I'd have to take a more thorough look to see if there's anything preventing it. Right now, this is also used to calculate the TaggedHash, so it may result in some duplication or need some more refactoring to avoid that.

It would also add a lifetime on the structs, which we may want to avoid? Forgetting if that will be problematic for bindings.

TheBlueMatt · 2024-10-16T19:10:43Z

lightning/src/offers/invoice_request.rs

+
+		invoice_request_tlv_stream.write(&mut bytes).unwrap();
+
+		const EXPERIMENTAL_OFFER_TYPES: core::ops::Range<u64> = 0..0;


Please don't add things and then change them in later commits, if at all possible.

Likewise here.

TheBlueMatt · 2024-10-16T19:16:09Z

lightning/src/offers/offer.rs

@@ -687,6 +687,12 @@ impl Offer {
 		self.contents.expects_quantity()
 	}

+	pub(super) fn tlv_stream_iter<'a>(


The commit message ends with "Using a common function ensures th", which looks cut off.

TheBlueMatt · 2024-10-16T19:23:40Z

lightning/src/offers/invoice.rs

@@ -845,7 +845,7 @@ impl Bolt12Invoice {
 				(&refund.payer.0, REFUND_IV_BYTES_WITH_METADATA)
 			},
 		};
-		self.contents.verify(TlvStream::new(&self.bytes), metadata, key, iv_bytes, secp_ctx)
+		self.contents.verify(&self.bytes, metadata, key, iv_bytes, secp_ctx)


The commit message for this change says "Passing bytes directly to InvoiceContents::verify improves readability.", but this is also used to deal with the filtering in verify, which is useful in the coming commits, no? Would be nice if the commit message described this a bit more.

Good call. Added more context.

TheBlueMatt · 2024-10-16T19:26:41Z

lightning/src/offers/invoice.rs

 	) -> Result<PaymentId, ()> {
+		const EXPERIMENTAL_TYPES: core::ops::Range<u64> = EXPERIMENTAL_OFFER_TYPES;


Right, but at each turn the commits are confusing because they have a wrong value for where we're going. I find it really confusing to have to read some code, see one value, think hmm, that's not right, then go read the final state of the PR, realize no, indeed, it was not right, but its changed later, so I have no idea what the final code is actually going to look like.

TheBlueMatt · 2024-10-16T19:46:39Z

lightning/src/offers/invoice.rs

+		const NON_EXPERIMENTAL_TYPES: core::ops::Range<u64> = 0..INVOICE_REQUEST_TYPES.end;
+		const EXPERIMENTAL_TYPES: core::ops::Range<u64> =


These constants are a bit confusingly named - they are actually only filters for invreq TLVs, but we're defining them in invoice.rs in a struct about invoices.

Added a clarifying comment here and in static_invoice.rs. They are local constants, so I don't think it's worth using overly elaborate names.

Using the tlv_stream macro without a type needing a reference results in a compilation error because of an unused lifetime parameter. To avoid this, add an optional lifetime parameter to the macro. This allows for experimental TLVs, which will be empty initially, and TLVs of entirely primitive types.

TlvRecord has a few fields, but comparing only the record_bytes is sufficient for equality since the other fields are initialized from it. Remove the Eq and PartialEq derives as they compare these other fields.

When constructing UnsignedInvoiceRequest or UnsignedBolt12Invoice, use a separate field for experimental TLV bytes. This allows for properly inserting the signature TLVs before the experimental TLVs when signing.

Add a utility function for iterating over Offer TLV records contained in any valid TLV stream bytes. Using a common function ensures that experimental TLV records are included once they are supported.

Passing bytes directly to InvoiceContents::verify improves readability as then a TlvStream for each TLV record range can be created from the bytes instead of needing to clone the TlvStream upfront. In an upcoming commit, the experimental TLV record range will utilize this.

Upcoming commits will allow parsing BOLT12 messages that include TLV records in the experimental range. Include these ranges when verifying messages since they will be included in the message bytes.

The BOLT12 spec defines an experimental TLV range that are allowed in offer messages. Allow this range when parsing an offer and include those bytes in any invoice requests. Also include those bytes when computing an OfferId and verifying that an InvoiceRequest is for a valid Offer.

Offer metadata is generated from the offer TLVs and should included those in the experimental range. When verifying invoice request and invoice messages, these TLVs must be included. Similarly, OfferId construction should included these TLVs as well. Modify the BOLT12 verification tests to cover these TLVs.

The BOLT12 spec defines an experimental TLV range that are allowed in invoice_request messages. Allow this range when parsing an invoice request and include those bytes in any invoice. Also include those bytes when verifying that a Bolt12Invoice is for a valid InvoiceRequest.

Payer metadata is generated from the invreq TLVs and should included those in the experimental range. When verifying invoice messages, these TLVs must be included. Modify the BOLT12 verification tests to cover them.

The BOLT12 spec defines an experimental TLV range that is allowed in offer and invoice_request messages. The remaining TLV-space is for experimental use in invoice messages. Allow this range when parsing an invoice and include it when signing one.

jkczyz · 2024-10-18T01:51:50Z

This LGTM, I think. I started trying to review it per-commit again and got very very confused, so just gave up and reviewed the whole patchset, which is much easier, if also pretty annoying.

2f9d44a and 906a6fb should address these concerns.

I do wonder if we're gonna end up wanting to change things a decent chunk when we go to actually support setting experimental entries in the structs. I assume we will, but probably not enough to merit changing this PR, though.

Arbitrary ones set by users, yes. Any we support should be the same as any TLV.

jkczyz force-pushed the 2024-08-offers-experimental-tlvs branch 3 times, most recently from 1d15a6f to 55c6c56 Compare August 15, 2024 00:19

jkczyz added the blocked on dependent pr label Aug 15, 2024

jkczyz marked this pull request as ready for review August 15, 2024 17:00

jkczyz mentioned this pull request Aug 21, 2024

Disallow user-provided payer_signing_pubkey #3264

Open

jkczyz mentioned this pull request Aug 30, 2024

Support paying static invoices #3140

Merged

jkczyz force-pushed the 2024-08-offers-experimental-tlvs branch 4 times, most recently from 3f5521c to 694533b Compare September 11, 2024 20:24

jkczyz force-pushed the 2024-08-offers-experimental-tlvs branch from 694533b to 1fc4d51 Compare September 16, 2024 21:49

Add parsing tests for unknown BOLT12 TLVs

e26c095

jkczyz force-pushed the 2024-08-offers-experimental-tlvs branch from 1fc4d51 to b84d36b Compare October 2, 2024 16:50

TheBlueMatt reviewed Oct 2, 2024

View reviewed changes

jkczyz force-pushed the 2024-08-offers-experimental-tlvs branch from b84d36b to 5f2991d Compare October 4, 2024 20:29

jkczyz commented Oct 4, 2024

View reviewed changes

jkczyz force-pushed the 2024-08-offers-experimental-tlvs branch from 5f2991d to be40cc7 Compare October 4, 2024 21:14

TheBlueMatt added Seeking Code Review and removed blocked on dependent pr labels Oct 8, 2024

jkczyz force-pushed the 2024-08-offers-experimental-tlvs branch from be40cc7 to fda4b8d Compare October 8, 2024 15:40

TheBlueMatt reviewed Oct 16, 2024

View reviewed changes

jkczyz added 6 commits October 17, 2024 15:09

f - fix docs

ee4be4f

Remove unneeded Eq and PartialEq derives

2ddbc18

TlvRecord has a few fields, but comparing only the record_bytes is sufficient for equality since the other fields are initialized from it. Remove the Eq and PartialEq derives as they compare these other fields.

Separate bytes for experimental TLVs

94efd75

When constructing UnsignedInvoiceRequest or UnsignedBolt12Invoice, use a separate field for experimental TLV bytes. This allows for properly inserting the signature TLVs before the experimental TLVs when signing.

f - avoid updating EXPERIMENTAL_TYPES in later commits

2f9d44a

f - add clarifying comments to local constants

b017b1c

jkczyz added 12 commits October 17, 2024 20:46

Function for iterating over Offer TLV records

7b5e94a

Add a utility function for iterating over Offer TLV records contained in any valid TLV stream bytes. Using a common function ensures that experimental TLV records are included once they are supported.

Include experimental TLV records when verifying

906a6fb

Upcoming commits will allow parsing BOLT12 messages that include TLV records in the experimental range. Include these ranges when verifying messages since they will be included in the message bytes.

Add parsing tests for experimental offer TLVs

8688c90

Add offer test vectors for experimental TLVs

653728f

Test verification with experimental invreq TLVs

d99cca6

Payer metadata is generated from the invreq TLVs and should included those in the experimental range. When verifying invoice messages, these TLVs must be included. Modify the BOLT12 verification tests to cover them.

Add parsing tests for experimental invreq TLVs

98ef5cd

Parse experimental invoice TLV records

1d5f757

The BOLT12 spec defines an experimental TLV range that is allowed in offer and invoice_request messages. The remaining TLV-space is for experimental use in invoice messages. Allow this range when parsing an invoice and include it when signing one.

Add parsing tests for experimental invoice TLVs

0a116c2

jkczyz force-pushed the 2024-08-offers-experimental-tlvs branch from fda4b8d to 0a116c2 Compare October 18, 2024 01:48

		) -> Result<PaymentId, ()> {
		const EXPERIMENTAL_TYPES: core::ops::Range<u64> = EXPERIMENTAL_OFFER_TYPES;


		invoice_request_tlv_stream.write(&mut bytes).unwrap();

		const EXPERIMENTAL_OFFER_TYPES: core::ops::Range<u64> = 0..0;

		const NON_EXPERIMENTAL_TYPES: core::ops::Range<u64> = 0..INVOICE_REQUEST_TYPES.end;
		const EXPERIMENTAL_TYPES: core::ops::Range<u64> =

Experimental offer TLVs #3237

Are you sure you want to change the base?

Experimental offer TLVs #3237

Conversation

jkczyz commented Aug 12, 2024 • edited Loading

codecov bot commented Aug 12, 2024 • edited Loading

Codecov Report

jkczyz commented Oct 2, 2024

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

jkczyz 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

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

jkczyz commented Oct 18, 2024

jkczyz commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading