multi: make payment address mandatory #9702
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
erickcestari
force-pushed
the
require-payment-secret
branch
5 times, most recently
from
28c722d to
18545d8
Compare
zpay32/invoice.go
Outdated
| @@ -381,6 +381,12 @@ func validateInvoice(invoice *Invoice) error { | |||
| return fmt.Errorf("no payment hash found") | |||
| } | |||
|
|
|||
| // The invoice must contain a payment address (payment secret) | |||
| // if it does not contain blinded paths. | |||
| if len(invoice.BlindedPaymentPaths) == 0 && invoice.PaymentAddr.IsNone() { | |||
There was a problem hiding this comment.
Could you please wrap this line to stay within the 80-character limit?
There was a problem hiding this comment.
@erickcestari what do you think about the two phase approach I proposed in #9718?
If we reject the invoices in the decoding phase, then we won't be able to decode the invoices at all.
IMO, the pertinent change in user behavior is that we'll no longer pay to invoices that don't have the addr/secret.
There was a problem hiding this comment.
This would also serve to decouple the feature bit layer from invoice parsing. We should always be able to decode a well structured invoice. At a later step, we then apply our feature bit requirements, which may lead to rejected payment attempts.
There was a problem hiding this comment.
Could you please wrap this line to stay within the 80-character limit?
Done
|
@saubyk the workflow is awaiting approval from a maintainer. |
zpay32/invoice.go
Outdated
| @@ -381,6 +381,12 @@ func validateInvoice(invoice *Invoice) error { | |||
| return fmt.Errorf("no payment hash found") | |||
| } | |||
|
|
|||
| // The invoice must contain a payment address (payment secret) | |||
| // if it does not contain blinded paths. | |||
| if len(invoice.BlindedPaymentPaths) == 0 && invoice.PaymentAddr.IsNone() { | |||
There was a problem hiding this comment.
@erickcestari what do you think about the two phase approach I proposed in #9718?
If we reject the invoices in the decoding phase, then we won't be able to decode the invoices at all.
IMO, the pertinent change in user behavior is that we'll no longer pay to invoices that don't have the addr/secret.
Why would we decode an invoice that we can't pay? The idea behind making the payment address (payment secret) a required field is to fail early, during invoice validation, if it's not present. That way, we avoid decoding something that's already invalid. Wouldn't an invoice missing a required field be considered structurally invalid? I was thinking of following the approach used by LDK, where they enforce the presence of the payment secret during the validation phase: But let me know if I'm missing something. |
erickcestari
force-pushed
the
require-payment-secret
branch
2 times, most recently
from
122a84a to
7848e6a
Compare
Make the payment secret field ('s') mandatory for BOLT11 payment requests,
implementing the requirement specified in BOLT11 spec PR lightning/bolts#1242.
This enhances privacy by preventing intermediate nodes from probing the
destination using payment onions.
This commit implements the following changes:
- Adds validation in `zpay32` to fail decoding if the 's' field is
missing when no blinded path is provided.
- Adds a test vector for an invoice missing the 's' field.
- Updates existing tests to accommodate the mandatory payment address requirement.
erickcestari
force-pushed
the
require-payment-secret
branch
from
7848e6a to
811aac3
Compare
Sometimes you just want to decode an invoice. Many node management UIs support just decoding an invoice. Invoice validity (BOLT 11) is distinct from the feature bits and fields we deem mandatory. The goal here is to make a distinction between a valid invoice (parses w/o errors), from one that may not meet the stricter requirements based on our current feature bits. |
|
If you look at the spec PR itself, it adds this section:
This matches my suggestion: allow the invoice to parse, but fail when we go to pay it. |
|
IMO, the two-phase approach proposed by @Roasbeef brings flexibility, leaving room for improved node management UI development. |
|
Thanks for the clarification! I'll go ahead and implement the first phase for now. I think I was a bit too focused on just the decoding check, but this approach makes sense and should be fine. |
|
For now, I’ll mark this PR as a draft since it’s still far from completing the first phase. I’ll open a separate PR to handle that. |
Make the payment secret field ('s') mandatory for BOLT11 payment requests, implementing the requirement specified in BOLT11 spec PR lightning/bolts#1242.
Change Description
zpay32to fail decoding if the 's' field is missing when no blinded path is provided.Closes #9700