Skip to content

Keysend bLIP - #5

Merged
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend
Dec 22, 2021
Merged

Keysend bLIP#5
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend

Conversation

@valentinewallace

Copy link
Copy Markdown
Contributor

Migrated from lightning/bolts#892

@lightning-developerlightning-developer left a comment

Copy link
Copy Markdown

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.

Comment threadblip-0003.md Outdated
Comment threadblip-0003.md
Comment threadblip-0002.md Outdated
Comment threadblip-0003.md
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadblip-0002.md Outdated
Comment threadblip-0002.md Outdated
Comment threadblip-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@TheBlueMattTheBlueMattDec 16, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
ContributorAuthor

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

Comment threadblip-0003.md Outdated
@valentinewallace
valentinewallaceforce-pushed the 2021-12-keysend branch 4 times, most recently from b974bdf to db5ea27CompareDecember 16, 2021 22:36

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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:masterDec 22, 2021
@Roasbeef

Copy link
Copy Markdown
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.

@ryanthegentryryanthegentry mentioned this pull request Jan 25, 2022
Comment threadblip-0002.md

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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 freeto 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

@valentinewallace@Roasbeef@cdecker@TheBlueMatt@joostjager@t-bast@lightning-developer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Keysend bLIP by valentinewallace · Pull Request #5 · lightning/blips · GitHub
Skip to content

Keysend bLIP - #5

Merged
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend
Dec 22, 2021
Merged

Keysend bLIP#5
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend

Conversation

@valentinewallace

Copy link
Copy Markdown
Contributor

Migrated from lightning/bolts#892

@lightning-developerlightning-developer left a comment

Copy link
Copy Markdown

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.

Comment threadblip-0003.md Outdated
Comment threadblip-0003.md
Comment threadblip-0002.md Outdated
Comment threadblip-0003.md
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadblip-0002.md Outdated
Comment threadblip-0002.md Outdated
Comment threadblip-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@TheBlueMattTheBlueMattDec 16, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
ContributorAuthor

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

Comment threadblip-0003.md Outdated
@valentinewallace
valentinewallaceforce-pushed the 2021-12-keysend branch 4 times, most recently from b974bdf to db5ea27CompareDecember 16, 2021 22:36

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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:masterDec 22, 2021
@Roasbeef

Copy link
Copy Markdown
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.

@ryanthegentryryanthegentry mentioned this pull request Jan 25, 2022
Comment threadblip-0002.md

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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 freeto 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

@valentinewallace@Roasbeef@cdecker@TheBlueMatt@joostjager@t-bast@lightning-developer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Keysend bLIP by valentinewallace · Pull Request #5 · lightning/blips · GitHub
Skip to content

Keysend bLIP - #5

Merged
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend
Dec 22, 2021
Merged

Keysend bLIP#5
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend

Conversation

@valentinewallace

Copy link
Copy Markdown
Contributor

Migrated from lightning/bolts#892

@lightning-developerlightning-developer left a comment

Copy link
Copy Markdown

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.

Comment threadblip-0003.md Outdated
Comment threadblip-0003.md
Comment threadblip-0002.md Outdated
Comment threadblip-0003.md
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadblip-0002.md Outdated
Comment threadblip-0002.md Outdated
Comment threadblip-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@TheBlueMattTheBlueMattDec 16, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
ContributorAuthor

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

Comment threadblip-0003.md Outdated
@valentinewallace
valentinewallaceforce-pushed the 2021-12-keysend branch 4 times, most recently from b974bdf to db5ea27CompareDecember 16, 2021 22:36

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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:masterDec 22, 2021
@Roasbeef

Copy link
Copy Markdown
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.

@ryanthegentryryanthegentry mentioned this pull request Jan 25, 2022
Comment threadblip-0002.md

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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 freeto 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

@valentinewallace@Roasbeef@cdecker@TheBlueMatt@joostjager@t-bast@lightning-developer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Keysend bLIP by valentinewallace · Pull Request #5 · lightning/blips · GitHub
Skip to content

Keysend bLIP - #5

Merged
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend
Dec 22, 2021
Merged

Keysend bLIP#5
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend

Conversation

@valentinewallace

Copy link
Copy Markdown
Contributor

Migrated from lightning/bolts#892

@lightning-developerlightning-developer left a comment

Copy link
Copy Markdown

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.

Comment threadblip-0003.md Outdated
Comment threadblip-0003.md
Comment threadblip-0002.md Outdated
Comment threadblip-0003.md
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadblip-0002.md Outdated
Comment threadblip-0002.md Outdated
Comment threadblip-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@TheBlueMattTheBlueMattDec 16, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
ContributorAuthor

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

Comment threadblip-0003.md Outdated
@valentinewallace
valentinewallaceforce-pushed the 2021-12-keysend branch 4 times, most recently from b974bdf to db5ea27CompareDecember 16, 2021 22:36

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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:masterDec 22, 2021
@Roasbeef

Copy link
Copy Markdown
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.

@ryanthegentryryanthegentry mentioned this pull request Jan 25, 2022
Comment threadblip-0002.md

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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 freeto 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

@valentinewallace@Roasbeef@cdecker@TheBlueMatt@joostjager@t-bast@lightning-developer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Keysend bLIP by valentinewallace · Pull Request #5 · lightning/blips · GitHub
Skip to content

Keysend bLIP - #5

Merged
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend
Dec 22, 2021
Merged

Keysend bLIP#5
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend

Conversation

@valentinewallace

Copy link
Copy Markdown
Contributor

Migrated from lightning/bolts#892

@lightning-developerlightning-developer left a comment

Copy link
Copy Markdown

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.

Comment threadblip-0003.md Outdated
Comment threadblip-0003.md
Comment threadblip-0002.md Outdated
Comment threadblip-0003.md
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadblip-0002.md Outdated
Comment threadblip-0002.md Outdated
Comment threadblip-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@TheBlueMattTheBlueMattDec 16, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
ContributorAuthor

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

Comment threadblip-0003.md Outdated
@valentinewallace
valentinewallaceforce-pushed the 2021-12-keysend branch 4 times, most recently from b974bdf to db5ea27CompareDecember 16, 2021 22:36

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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:masterDec 22, 2021
@Roasbeef

Copy link
Copy Markdown
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.

@ryanthegentryryanthegentry mentioned this pull request Jan 25, 2022
Comment threadblip-0002.md

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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 freeto 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

@valentinewallace@Roasbeef@cdecker@TheBlueMatt@joostjager@t-bast@lightning-developer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Keysend bLIP by valentinewallace · Pull Request #5 · lightning/blips · GitHub
Skip to content

Keysend bLIP - #5

Merged
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend
Dec 22, 2021
Merged

Keysend bLIP#5
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend

Conversation

@valentinewallace

Copy link
Copy Markdown
Contributor

Migrated from lightning/bolts#892

@lightning-developerlightning-developer left a comment

Copy link
Copy Markdown

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.

Comment threadblip-0003.md Outdated
Comment threadblip-0003.md
Comment threadblip-0002.md Outdated
Comment threadblip-0003.md
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadblip-0002.md Outdated
Comment threadblip-0002.md Outdated
Comment threadblip-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@TheBlueMattTheBlueMattDec 16, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
ContributorAuthor

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

Comment threadblip-0003.md Outdated
@valentinewallace
valentinewallaceforce-pushed the 2021-12-keysend branch 4 times, most recently from b974bdf to db5ea27CompareDecember 16, 2021 22:36

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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:masterDec 22, 2021
@Roasbeef

Copy link
Copy Markdown
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.

@ryanthegentryryanthegentry mentioned this pull request Jan 25, 2022
Comment threadblip-0002.md

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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 freeto 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

@valentinewallace@Roasbeef@cdecker@TheBlueMatt@joostjager@t-bast@lightning-developer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Keysend bLIP by valentinewallace · Pull Request #5 · lightning/blips · GitHub
Skip to content

Keysend bLIP - #5

Merged
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend
Dec 22, 2021
Merged

Keysend bLIP#5
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend

Conversation

@valentinewallace

Copy link
Copy Markdown
Contributor

Migrated from lightning/bolts#892

@lightning-developerlightning-developer left a comment

Copy link
Copy Markdown

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.

Comment threadblip-0003.md Outdated
Comment threadblip-0003.md
Comment threadblip-0002.md Outdated
Comment threadblip-0003.md
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadblip-0002.md Outdated
Comment threadblip-0002.md Outdated
Comment threadblip-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@TheBlueMattTheBlueMattDec 16, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
ContributorAuthor

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

Comment threadblip-0003.md Outdated
@valentinewallace
valentinewallaceforce-pushed the 2021-12-keysend branch 4 times, most recently from b974bdf to db5ea27CompareDecember 16, 2021 22:36

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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:masterDec 22, 2021
@Roasbeef

Copy link
Copy Markdown
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.

@ryanthegentryryanthegentry mentioned this pull request Jan 25, 2022
Comment threadblip-0002.md

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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 freeto 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

@valentinewallace@Roasbeef@cdecker@TheBlueMatt@joostjager@t-bast@lightning-developer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Keysend bLIP by valentinewallace · Pull Request #5 · lightning/blips · GitHub
Skip to content

Keysend bLIP - #5

Merged
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend
Dec 22, 2021
Merged

Keysend bLIP#5
t-bast merged 1 commit into
lightning:masterfrom
valentinewallace:2021-12-keysend

Conversation

@valentinewallace

Copy link
Copy Markdown
Contributor

Migrated from lightning/bolts#892

@lightning-developerlightning-developer left a comment

Copy link
Copy Markdown

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.

Comment threadblip-0003.md Outdated
Comment threadblip-0003.md
Comment threadblip-0002.md Outdated
Comment threadblip-0003.md
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated
Comment threadblip-0003.md Outdated

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadblip-0002.md Outdated
Comment threadblip-0002.md Outdated
Comment threadblip-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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@TheBlueMattTheBlueMattDec 16, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
ContributorAuthor

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

Comment threadblip-0003.md Outdated
@valentinewallace
valentinewallaceforce-pushed the 2021-12-keysend branch 4 times, most recently from b974bdf to db5ea27CompareDecember 16, 2021 22:36

@t-bastt-bast left a comment

Copy link
Copy Markdown
Contributor

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:masterDec 22, 2021
@Roasbeef

Copy link
Copy Markdown
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.

@ryanthegentryryanthegentry mentioned this pull request Jan 25, 2022
Comment threadblip-0002.md

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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 freeto 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

@valentinewallace@Roasbeef@cdecker@TheBlueMatt@joostjager@t-bast@lightning-developer