Skip to content
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

Merged
merged 1 commit into from
Dec 22, 2021
Merged

Conversation

valentinewallace
Copy link
Contributor

Migrated from lightning/bolts#892

Copy link

@lightning-developer lightning-developer left a 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.

blip-0003.md Outdated Show resolved Hide resolved
blip-0003.md Show resolved Hide resolved
blip-0002.md Outdated Show resolved Hide resolved
blip-0003.md Show resolved Hide resolved
blip-0003.md Outdated Show resolved Hide resolved
blip-0003.md Outdated Show resolved Hide resolved
blip-0003.md Outdated Show resolved Hide resolved
blip-0003.md Outdated Show resolved Hide resolved
blip-0003.md Outdated Show resolved Hide resolved
blip-0003.md Outdated Show resolved Hide resolved
Copy link
Contributor

@t-bast t-bast left a 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-0002.md Outdated Show resolved Hide resolved
blip-0002.md Outdated Show resolved Hide resolved
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
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

@TheBlueMatt TheBlueMatt Dec 16, 2021

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.

Copy link
Contributor

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?).

Copy link

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.

Copy link
Contributor Author

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

blip-0003.md Outdated Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the 2021-12-keysend branch 4 times, most recently from b974bdf to db5ea27 Compare December 16, 2021 22:36
Copy link
Contributor

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @valentinewallace!

@t-bast t-bast merged commit 44434a9 into lightning:master Dec 22, 2021
@Roasbeef
Copy link
Contributor

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).

A bit late on this, but it wasn't part of keysend as it was created before we started to specify MPP as we know it today, namely the semantics around the payment_addr/secret field. Also given the pre-image is included directly in the payload, the receiver can pull at anytime, which means you can run into the same issues re partial fulfilment. AMP improves on this, as the payment can only be pulled once all the shards arrive.

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.

@ryanthegentry ryanthegentry mentioned this pull request Jan 25, 2022

| Type | Name | Link |
|------------|-----------------------------|--------------------------------|
| 5482373484 | `keysend_preimage` | [bLIP 3](./blip-0003.md) |

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.

Copy link
Contributor

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!

Copy link
Contributor Author

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

guggero pushed a commit to guggero/blips that referenced this pull request Oct 23, 2024
blip-29: use variable-length bytes for fixed-point coefficient encoding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants