*Require* payment_secret for multi-part payments#712
*Require* payment_secret for multi-part payments#712cfromknecht merged 1 commit intolightning:masterfrom
Conversation
t-bast
left a comment
There was a problem hiding this comment.
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 <rusty@rustcorp.com.au>
It makes sense, and it's been proposed for addition to the spec to broad agreement: lightning/bolts#712 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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. |
(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.