-
Notifications
You must be signed in to change notification settings - Fork 46
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
Keysend bLIP #5
Keysend bLIP #5
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.
Thank you for the write up and the nice bLIP. I guess it is not very controversial as it is already being implemented almost everywhere.
Despite the comments in the text I was mainly wondering if we should specify the payment amount and what happens if the amount in the last onionion payload does not match what was being forwarded. Theoretically an intermediate node could try to send a lower amount and hope it is a keysend payment and see if the recipient settles. so I guess the recipient MUST also check that the value of the incoming HTLC matches the field in the onion and SHOULD otherwise fail the payment even though they MAY accept 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.
Thanks for putting this together!
I had never realized that MPP was disallowed with keysend, and really can't understand why, if you can enlighten me that would be great (eclair does allow MPP + Keysend).
blip-0003.md
Outdated
* MUST include a TLV record keyed by type `5482373484` with a TLV value of a | ||
randomly generated and cryptographically-secure 32-byte value that serves as | ||
the HTLC payment preimage | ||
* MUST NOT set a `payment_data` field in the onion routing packet's TLV payload |
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.
Can you explain why? Eclair doesn't disallow setting this, why would implementations do that? Having this field is actually what lets people use MPP with keysend.
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.
Ah I see, I don't mind removing this part and officially supporting MPP keysend.
This was before the Libera switch so I could be misremembering, but I talked to @cdecker on IRC about MPP support in keysend and his thinking was that that a payer-supplied payment secret doesn't make sense (since it no longer serves to "authenticate" the sender iiuc). Would appreciate if cdecker could weigh in here?
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.
I don't think it matters at all. It's true that the payment_secret
is redundant, as the preimage can act as the secret here, but as a recipient you really want to avoid revealing that preimage before you've received everything that the sender wanted you to receive, so you need to have a total_amount
field in the onion (which the payment_data
field provides).
As long as the receiver didn't reveal the preimage too early, intermediate nodes cannot cheat, as they don't know the preimage, and the receiver knows this is a keysend so it shouldn't accept HTLCs that don't contain the preimage in the onion payload.
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.
Its totally possible to support it, but IIUC lnd never did, and thus c-lightning, to match lnd's behavior, did not either. I could see an argument that without proof-of-payment for some given value a receiver may prefer to partial-claim an HTLC instead of failing it due to MPP timeout, but I'm not really sure it matters for the intended tipping use-case.
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.
I indeed don't think it matters for the tipping use-case...
Eclair has always allowed MPP with keysend, so I don't know if we should have these requirements about payment_data
, maybe it's best to just not say anything about it?
That shows that it would have been great to specify this, now we have non-compatible implementations on the network (even though we never got users complaining about failures here, are we sure lnd and c-lightning actively reject keysend payments that contain a payment_data
field?).
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.
Hm, I see how the payment_sata includes the total amount to be sent (why didn't we separate them...). I think that alone should be sufficient to keep it for MPP keysend support.
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.
Sounds good, removed the parts of the bLIP that specify MPP isn't supported
b974bdf
to
db5ea27
Compare
db5ea27
to
7662d30
Compare
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, thanks @valentinewallace!
A bit late on this, but it wasn't part of Also FWIW, most usage of keysend today on the network like the podcast annotation stuff usually uses pretty small amounts, so payment splitting isn't usually necessary in practice. |
|
||
| Type | Name | Link | | ||
|------------|-----------------------------|--------------------------------| | ||
| 5482373484 | `keysend_preimage` | [bLIP 3](./blip-0003.md) | |
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.
Is this the right location for this table row? I think it should be part of the payment_onion_payload
section.
the
update_add_htlc
message's onion payload
What is this exactly? The update_add_htlc
message can be extended with custom tlv fields, but that isn't the onion payload.
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.
Good catch, that does need to be fixed!
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.
Thanks @joostjager, corrected in #15
blip-29: use variable-length bytes for fixed-point coefficient encoding
Migrated from lightning/bolts#892