Hide InvoiceFeatures behind InvoiceBuilder API

[jkczyz]

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]

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?

[jkczyz]

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]

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.

[jkczyz]

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?

[TheBlueMatt]

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.

[jkczyz]

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.

[TheBlueMatt]

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.

[jkczyz]

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

[codecov]

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]

ACK modulo being able to set unknown feature bits

[TheBlueMatt]

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.

[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

[jkczyz]

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]

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 { ... } ?

[jkczyz]

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.

[jkczyz]

Squashed fixup commits.

[ariard]

Code Review ACK 2226ae2

[jkczyz]

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]

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?

[jkczyz]

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]

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.

[jkczyz]

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?

[TheBlueMatt]

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.

[jkczyz]

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.

[TheBlueMatt]

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.

[jkczyz]

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

[codecov]

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]

ACK modulo being able to set unknown feature bits

[TheBlueMatt]

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.

[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

[jkczyz]

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]

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 { ... } ?

[jkczyz]

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.

[jkczyz]

Squashed fixup commits.

[ariard]

Code Review ACK 2226ae2

[jkczyz]

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]

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?

[jkczyz]

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]

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.

[jkczyz]

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?

[TheBlueMatt]

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.

[jkczyz]

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.

[TheBlueMatt]

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.

[jkczyz]

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

[codecov]

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]

ACK modulo being able to set unknown feature bits

[TheBlueMatt]

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.

[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

[jkczyz]

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]

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 { ... } ?

[jkczyz]

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.

[jkczyz]

Squashed fixup commits.

[ariard]

Code Review ACK 2226ae2

[jkczyz]

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]

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?

[jkczyz]

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]

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.

[jkczyz]

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?

[TheBlueMatt]

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.

[jkczyz]

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.

[TheBlueMatt]

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.

[jkczyz]

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

[codecov]

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]

ACK modulo being able to set unknown feature bits

[TheBlueMatt]

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.

[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

[jkczyz]

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]

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 { ... } ?

[jkczyz]

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.

[jkczyz]

Squashed fixup commits.

[ariard]

Code Review ACK 2226ae2

[jkczyz]

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]

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?

[jkczyz]

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]

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.

[jkczyz]

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?

[TheBlueMatt]

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.

[jkczyz]

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.

[TheBlueMatt]

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.

[jkczyz]

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

[codecov]

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]

ACK modulo being able to set unknown feature bits

[TheBlueMatt]

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.

[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

[jkczyz]

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]

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 { ... } ?

[jkczyz]

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.

[jkczyz]

Squashed fixup commits.

[ariard]

Code Review ACK 2226ae2

[jkczyz]

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]

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?

[jkczyz]

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]

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.

[jkczyz]

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?

[TheBlueMatt]

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.

[jkczyz]

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.

[TheBlueMatt]

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.

[jkczyz]

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

[codecov]

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]

ACK modulo being able to set unknown feature bits

[TheBlueMatt]

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.

[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

[jkczyz]

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]

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 { ... } ?

[jkczyz]

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.

[jkczyz]

Squashed fixup commits.

[ariard]

Code Review ACK 2226ae2

[jkczyz]

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]

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?

[jkczyz]

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]

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.

[jkczyz]

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?

[TheBlueMatt]

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.

[jkczyz]

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.

[TheBlueMatt]

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.

[jkczyz]

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

[codecov]

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]

ACK modulo being able to set unknown feature bits

[TheBlueMatt]

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.

[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

[jkczyz]

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]

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 { ... } ?

[jkczyz]

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.

[jkczyz]

Squashed fixup commits.

[ariard]

Code Review ACK 2226ae2

[jkczyz]

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]

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?

[jkczyz]

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]

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.

[jkczyz]

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?

[TheBlueMatt]

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.

[jkczyz]

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.

[TheBlueMatt]

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.

[jkczyz]

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

[codecov]

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]

ACK modulo being able to set unknown feature bits

[TheBlueMatt]

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.

[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

[jkczyz]

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]

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 { ... } ?

[jkczyz]

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.

[jkczyz]

Squashed fixup commits.

[ariard]

Code Review ACK 2226ae2