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

Support keysend to blinded paths #2935

Merged

Conversation

valentinewallace
Copy link
Contributor

@valentinewallace valentinewallace commented Mar 13, 2024

Support sending and receiving keysend payments to blinded paths that we generate.

Will be useful for async payments support in the case that the often-offline payee's LSP (or some other node) provides a keysend invoice to payers on the payee's behalf.

#[test]
fn blinded_keysend() {
let mut mpp_keysend_config = test_default_channel_config();
mpp_keysend_config.accept_mpp_keysend = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jkczyz @TheBlueMatt FYI --

I ended up keeping the payment secret in keysend blinded paths. I'm not sure why I thought it wasn't supposed to be reused at first, but I think it's fine to reuse it and provides value for keysend payments.

As a result, it was easy to adapt the existing PendingHTLCRouting::ReceiveKeysend variant for blinded keysends.
However, reusing this variant means that users that want to receive blinded keysends have to set accept_mpp_keysend in their config or else LDK will reject the payment.

I think that's okay, but wanted to call it out in case there are differing opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems ok to me. But why does it need to be set true in this test if blinded_mpp_keysend tests MPP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accept_mpp_keysend means that users opt into potentially breaking compat with < 0.0.116, which can happen if PendingHTLCRouting::ReceiveKeysend::payment_data is set. Because the payment secret will always be present in keysends to blinded paths, ::payment_data will always be Some, so users need to opt into the potential compat breakage. Does that make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess? ISTM we could instead actually just use the keysend_preimage - only the sender has that, so no need to worry about the secret anywhere. In general I kinda anticipated no one would support reading a payment secret in a blinded-path-receive HTLC, but certainly we could require it 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess? ISTM we could instead actually just use the keysend_preimage - only the sender has that, so no need to worry about the secret anywhere. In general I kinda anticipated no one would support reading a payment secret in a blinded-path-receive HTLC, but certainly we could require it 🤷

I'm confused, we currently require payment secret to be present in all blinded path receives (so we can do stateless inbound payment verification). So the user isn't providing the payment secret, we're providing it back to ourselves in the blinded path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, okay, nevermind, I was confused where that field was coming from.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 98.94737% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 89.22%. Comparing base (f5ee8c2) to head (664abf2).

Files Patch % Lines
lightning/src/ln/channelmanager.rs 80.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2935      +/-   ##
==========================================
+ Coverage   89.20%   89.22%   +0.01%     
==========================================
  Files         117      117              
  Lines       95513    95599      +86     
  Branches    95513    95599      +86     
==========================================
+ Hits        85203    85296      +93     
+ Misses       7822     7818       -4     
+ Partials     2488     2485       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@valentinewallace valentinewallace added this to the 0.0.122 milestone Mar 15, 2024
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

We'll need to document somewhere that nodes are supposed to use the payment_secret for MPP keysend receive even for blinded paths. We might reconsider it at some point and instead just copy the payment_preimage into the secret field, which honestly we maybe should have done to begin with.

@valentinewallace valentinewallace mentioned this pull request Mar 19, 2024
15 tasks
@TheBlueMatt TheBlueMatt merged commit 5e41425 into lightningdevkit:main Mar 20, 2024
15 of 16 checks passed
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.

None yet

4 participants