Hide InvoiceFeatures behind InvoiceBuilder API #901

jkczyz · 2021-04-28T17:28:58Z

Instead of relying on users to set an invoice's features correctly, enforce the semantics inside InvoiceBuilder. For instance, if the user sets a PaymentSecret then InvoiceBuilder should ensure the appropriate feature bits are set. Thus, for this example, the TaggedField abstraction can be retained while still ensuring BOLT 11 semantics at the builder abstraction.

~~Based on #898.~~

TheBlueMatt · 2021-04-28T17:43:30Z

lightning-invoice/src/lib.rs

@@ -970,6 +1036,7 @@ impl Invoice {
 			signed_invoice: signed_invoice,
 		};
 		invoice.check_field_counts()?;
+		invoice.check_feature_bits()?;


Are there cases where you may want to use this on an invoice not generated by us? Where we may want to accept a non-basic-mpp invoice but not generate them?

I believe this is supported as written. check_feature_bits simply checks that BOLT 11 requirements are met. Note that the changes in this PR don't require that any feature bits are set. I know we discussed doing that offline yesterday, but I ended up finding a simpler way to enforce the semantics.

TheBlueMatt · 2021-04-28T17:45:13Z

lightning-invoice/src/lib.rs

+}
+
+impl<D: tb::Bool, H: tb::Bool, T: tb::Bool, C: tb::Bool> InvoiceBuilder<D, H, T, C, tb::False> {
+	/// Sets the payment secret and relevant features.


Hmm, we still need to be able to set more features here, no? You may want to set some future features which is probably in known but not in the set that is required to understand the invoice here.

Do you mean setting a feature as optional? If so, that is an open question that I had. For instance, would we want to have both basic_mpp_required and basic_mpp_optional methods? It wasn't entirely clear from the BOLT if either were possible:

if the basic_mpp feature is offered in the invoice:

MAY pay using Basic multi-part payments.

otherwise:

MUST NOT use Basic multi-part payments.

Does "offered" mean the odd bit (optional) was set? Is something similar possible for payment_secret? The BOLT seems to imply that if a secret is set then it must be used:

if there is a valid s field:

MUST use that as payment_secret

And what would optional mean here if basic_mpp is set as required as there is a dependency requirement?

Do you mean setting a feature as optional?

I meant a feature other than the two below. eg what if we have option_htlcs_colored_blue? Some users may really like blue HTLCs some users may not, and LDK may reasonably support receiving both. Thus, we need some way to provide a Features object, I think.

Does "offered" mean the odd bit (optional) was set?

I believe it means either. I think the handling of var_len_onion here is fine.

I meant a feature other than the two below. eg what if we have option_htlcs_colored_blue? Some users may really like blue HTLCs some users may not, and LDK may reasonably support receiving both. Thus, we need some way to provide a Features object, I think.

Ah, so the additional type parameter S is for specifying that a secret can be set at most once. Additionally, it is used to condition whether basic_mpp can bet set because that requires the payment_secret feature and thus also a secret field set in the invoice.

Other features may have similar type parameters associated with them if they require some other fields set in the invoice. For example, if the hypothetical feature were instead option_htlcs_colored with an additional field specifying the color blue, then we would add an option_htlcs_colored method taking a color to set in the invoice. It would also set the feature bits as necessary.

Long story short is the purpose of InvoiceBuilder is to enforce BOLT 11 semantics at compile time. Thus, it can never result in a SemanticsError only a CreationError. Allowing features to be set arbitrarily breaks that contract.

Whether the features are set as required or optional is an orthogonal concern. Since these features are optional in InvoiceFeatures::known(), then we'd likely want to set them optional here. I think I had decided on required because the dependency between them and wasn't sure what it would mean for some to be optional and others to be required.

Hmmmmm, right, for "known" bits that makes sense. I do wonder how we can support "experimental" feature bits (eg DLC invoices or so), but we really need the ability to set them in InvoiceFeatures as well which would imply its a separate thing.

For experimental features, I'd imagine there could be a separate method to set these which could also check against any known features.

codecov · 2021-04-28T17:49:16Z

Codecov Report

Merging #901 (718753d) into main (e26c3df) will increase coverage by 0.10%.
The diff coverage is 98.22%.

❗ Current head 718753d differs from pull request most recent head 2226ae2. Consider uploading reports for the commit 2226ae2 to get more accurate results

@@            Coverage Diff             @@
##             main     #901      +/-   ##
==========================================
+ Coverage   90.50%   90.60%   +0.10%     
==========================================
  Files          59       59              
  Lines       29624    30466     +842     
==========================================
+ Hits        26810    27604     +794     
- Misses       2814     2862      +48

Impacted Files	Coverage Δ
lightning-invoice/src/lib.rs	`90.32% <98.10%> (+2.73%)`	⬆️
lightning-invoice/src/utils.rs	`83.69% <100.00%> (ø)`
lightning/src/ln/features.rs	`98.83% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.03% <0.00%> (+0.22%)`	⬆️
lightning-invoice/src/ser.rs	`93.36% <0.00%> (+1.23%)`	⬆️
lightning-invoice/src/de.rs	`83.33% <0.00%> (+2.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e26c3df...2226ae2. Read the comment docs.

devrandom · 2021-04-30T15:10:51Z

ACK modulo being able to set unknown feature bits

TheBlueMatt · 2021-04-30T15:47:58Z

modulo being able to set unknown feature bits

I think we can do that in a followup? Currently InvoiceFeatures doesn't support custom bits at all, so there isn't a way to do it in invoice today anyway.

Feature payment_secret is required and depends on var_onion_optin, so the latter must also be required.

valentinewallace

This is looking pretty reasonable to me! I'd prefer if test changes were included in the commit they're testing so it's a bit clearer what's being tested, but not a huge deal

lightning-invoice/src/lib.rs

jkczyz · 2021-05-03T21:23:43Z

This is looking pretty reasonable to me! I'd prefer if test changes were included in the commit they're testing so it's a bit clearer what's being tested, but not a huge deal

I tend to agree though sometimes having a few atomic commits that culminate in the the tests can alleviate the review burden.

Here, the tests make use of InvoiceFeatures::known() which has the basic_mpp feature bit set. But the refactor involved removing that bit and then re-adding it in a later commit. I could have explicitly construct the features in the test, but I wanted the test to break if InvoiceFeatures::known() ever gets updated. That way InvoiceBuilder should always support our known features.

ariard

Code Review ACK 718753d

I don't think my point around basic_mpp matters given current spec.

ariard · 2021-05-03T22:38:21Z

lightning-invoice/src/lib.rs

+		self.tagged_fields = self.tagged_fields
+			.drain(..)
+			.map(|field| match field {
+				TaggedField::Features(f) => TaggedField::Features(f.set_basic_mpp_optional()),


Note, what if in the future we introduce a new tagged field requiring to set the 9 field without being dependent on payment secret. We might set mpp here without actually having payment_secret set in our feature bits ?

Maybe we should add a if features.supports_payment_secret { ... } ?

This method is conditionally implemented when payment_secret is set (i.e., when the S type parameter is tb::True). So supporting such a feature should not be problem -- calling basic_mpp() would be a compilation error still.

Instead of relying on users to set an invoice's features correctly, enforce the semantics inside InvoiceBuilder. For instance, if the user sets a PaymentSecret then InvoiceBuilder should ensure the appropriate feature bits are set. Thus, for this example, the TaggedField abstraction can be retained while still ensuring BOLT 11 semantics at the builder abstraction.

Since InvoiceFeatures are an implementation detail of InvoiceBuilder, an explicit call is needed to support the basic_mpp feature. Since it is dependent on the payment_secret feature, conditionally define the builder's method only when payment_secret has been set.

jkczyz · 2021-05-03T23:24:55Z

Squashed fixup commits.

ariard · 2021-05-03T23:26:39Z

Code Review ACK 2226ae2

TheBlueMatt reviewed Apr 28, 2021

View reviewed changes

jkczyz added 2 commits April 30, 2021 12:04

Require feature var_onion_optin

1ec0232

Feature payment_secret is required and depends on var_onion_optin, so the latter must also be required.

Sanity test InvoiceFeatures

7310e26

jkczyz force-pushed the 2021-04-invoice-feature-semantics branch 2 times, most recently from 2ddb995 to 61c79de Compare April 30, 2021 22:05

jkczyz marked this pull request as ready for review April 30, 2021 22:06

TheBlueMatt added this to the 0.0.14 milestone May 1, 2021

TheBlueMatt mentioned this pull request May 1, 2021

Bump versions to 0.0.14, lightning-invoice 0.5 #909

Merged

valentinewallace reviewed May 3, 2021

View reviewed changes

lightning-invoice/src/lib.rs Show resolved Hide resolved

TheBlueMatt approved these changes May 3, 2021

View reviewed changes

ariard reviewed May 3, 2021

View reviewed changes

jkczyz added 4 commits May 3, 2021 16:23

Test feature bits in InvoiceBuilder

0592c52

Test feature bit semantics in Invoice::from_signed

2226ae2

jkczyz force-pushed the 2021-04-invoice-feature-semantics branch from 718753d to 2226ae2 Compare May 3, 2021 23:24

TheBlueMatt merged commit d782df0 into lightningdevkit:main May 4, 2021

Hide InvoiceFeatures behind InvoiceBuilder API #901

Hide InvoiceFeatures behind InvoiceBuilder API #901

Uh oh!

Conversation

jkczyz commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devrandom commented Apr 30, 2021

Uh oh!

TheBlueMatt commented Apr 30, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz commented May 3, 2021

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz commented May 3, 2021

Uh oh!

ariard commented May 3, 2021

Uh oh!

Uh oh!

jkczyz commented Apr 28, 2021 •

edited

Loading

jkczyz Apr 28, 2021 •

edited

Loading

codecov bot commented Apr 28, 2021 •

edited

Loading