-
Notifications
You must be signed in to change notification settings - Fork 494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*Require* payment_secret for multi-part payments #712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like someone is actively implementing MPP 😃
I think we can give users some leeway here. During a transition period, we can't expect wallets to all implement payment secret and MPP. As such my current strategy is to set mpp_optional
and payment_secret_optional
in the invoice. Later we can start setting payment_secret_mandatory
in the invoice, once telemetry shows that all the payments we receive do send a payment secret.
But when the sender actually uses multiple HTLCs, then the receiver does indeed require receiving a payment secret in the onion (the exact condition in our code is htlcAmount < totalAmount
).
I don't know if we should make something like that explicit in the spec or not (as it's more of an implementation choice/detail during a transition period).
Can you explain this a bit? How is it worse than the status quo if the intermediary doesn't know the payment secret? |
Actually, maybe you're right: With non-MPP, you need to probe with an amount >= the invoice amount, otherwise you get the generic error. With MPP, you can probe with 1msat amount, but you still need to guess the total_amount exactly which is a slightly more difficult task already. But I still think it makes sense to say "if you're using mpp, you need to supply the secret". |
Yes, this change only says: if invoice offers mpp, it must offer payment_secret.
I think I specified this correctly, but it's not clear from the patch context, you need to read further up to see where I added the new conditions? |
Totally agree, SGTM 👍 |
LGTM |
It makes sense, and it's been proposed for addition to the spec to broad agreement: lightning/bolts#712 Signed-off-by: Rusty Russell <[email protected]>
It makes sense, and it's been proposed for addition to the spec to broad agreement: lightning/bolts#712 Signed-off-by: Rusty Russell <[email protected]>
This means the BOLT11 invoice must offer it (we already say it must set the field if it offers it), and that the receiving node must require it (again, we already say it must check it if it requires it). Without the payment_secret, MPP payments are especially vulnerable to probing attacks: unlike normal payments (with amounts) they can be detected with 1msat payment probes. Signed-off-by: Rusty Russell <[email protected]>
ad2579d
to
6ad8ee4
Compare
Rebased and fix folded, now #643 is merged. |
ACK |
ACK 6ad8ee4 |
@cfromknecht WDYT? It feels to me that we should merge this now that MPP has been merged, this has to come with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, ACK 6ad8ee4
(Based on #643 so ignore first three commits)This means the BOLT11 invoice must offer it (we already say it must set the field if it offers it), and that the receiving node must require it (again, we already say it must check it if it requires it).
Without the payment_secret, MPP payments are especially vulnerable to probing attacks: unlike normal payments (with amounts) they can be detected with 1msat payment probes.