Support MPP Keysend

[alecchendev]

Closes #1222. This implements everything needed to support sending and receiving MPP keysend.

Some implementations reject keysend payments with payment secrets (we do before this PR) so we try and communicate to the user in RecipientOnionFields that they should be aware of this. There's also no great way to signal/tell when a node supports this so we leave it up to the user to decide when they want to route/send MPP keysends.

Since MPP keysend requires a payment secret which previously was not included in PendingHTLCRouting::ReceiveKeysend, downgrading can break deserialization, and so we also include a new flag UserConfig::accept_mpp_keysend to allow the user to opt-in to receive support.

[codecov-commenter]

Codecov Report

Patch coverage: 95.63% and project coverage change: +0.96 🎉

Comparison is base (5c89d01) 90.87% compared to head (9db962c) 91.83%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2156      +/-   ##
==========================================
+ Coverage   90.87%   91.83%   +0.96%     
==========================================
  Files         104      104              
  Lines       52789    68099   +15310     
  Branches    52789    68099   +15310     
==========================================
+ Hits        47972    62540   +14568     
- Misses       4817     5559     +742     

Impacted Files	Coverage Δ
lightning/src/ln/outbound_payment.rs	89.47% <ø> (-0.15%)	⬇️
lightning/src/util/config.rs	60.18% <25.00%> (+17.56%)	⬆️
lightning/src/ln/channelmanager.rs	89.96% <95.23%> (+2.85%)	⬆️
lightning/src/ln/payment_tests.rs	98.94% <98.18%> (+1.38%)	⬆️
lightning/src/ln/features.rs	96.20% <100.00%> (+0.12%)	⬆️
lightning/src/ln/functional_test_utils.rs	85.77% <100.00%> (-6.89%)	⬇️
lightning/src/ln/functional_tests.rs	98.77% <100.00%> (+0.53%)	⬆️
lightning/src/routing/router.rs	94.77% <100.00%> (+0.34%)	⬆️

... and 55 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheBlueMatt]

Ah, this also doesn't check that all parts have the same payment secret, I think. Its also going to conflict with passing the payment_metadata out through the receive pipeline. If its alright I'm gonna do some of the piping in there, we can merge that first, and that should simplify this sone.

[alecchendev]

Sure sounds good!

[alecchendev]

And yea I initially thought we didn't need to validate the payment secret since the preimage is serving the same role here, but I realize even then people probably should still be sending all parts with at least the same secret + it'll be good to keep things consistent with normal MPPs.

[TheBlueMatt]

Okay, let me know what you think on #2127, that should simplify things here a bit.

[valentinewallace]

2127 landed, looks like this is good for rebase.

[alecchendev]

Thanks for the heads up! Planning to get to this today or tomorrow

[valentinewallace]

Still going through an initial pass, but looks good :)

Not exactly related to the PR, but I think we can consolidate a good chunk of the logic in process_pending_htlc_forwards by always calling check_total_value for keysends now: https://pastebin.com/mTCLQxaS

[valentinewallace]

This DRY of process_pending_forwards might be a nice fit for this PR 🥹 #2156 (review)

[valentinewallace]

@TheBlueMatt I have some questions on this.

1. Currently we override the sender-provided RecipientOnionFields::payment_secret. That's fine, but I think send_spontaneous_payment should only take a payment_metadata then.
2. From looking through the lnd codebase, it seems like they will reject payment secret keysends. So it seems like ideally we should only set it if we're sending a multi-part keysend, and leave it blank otherwise.

Do these requested changes make sense, before I send alec on a wild goose chase?

[TheBlueMatt]

Currently we override the sender-provided RecipientOnionFields::payment_secret. That's fine, but I think send_spontaneous_payment should only take a payment_metadata then.

I disagree, while its true that we should generally not be sending a payment secret (or, in fact, payment metadata) for a keysend where we haven't communicated with the recipient or are doing MPP, we may have communicated with the recipient and still want to use keysend, in which case we'll want to be able to fill in the other fields. More generally, part of the point of RecipientOnionFields is we want to support custom TLVs eventually, which are definitely used in keysends.

From looking through the lnd codebase, it seems like they will reject payment secret keysends. So it seems like ideally we should only set it if we're sending a multi-part keysend, and leave it blank otherwise.

Right, I'm not sure what the exact right API is here, its kinda awkward to have the keysend methods also take the secret and then only actually work if you set the secret correctly corresponding to if you did multipath or not. Then again, lnd also will reject multipath keysends anyway, so its not like we're not always running that risk with such an API. Worse still, we separately take a PaymentParameters which controls the MPP bit for sending a spontaneous payment, but that's pretty well-hidden.

Maybe we have a separate send_lnd_compatible_keysend method that doesn't take a RecipientOnionFields or PaymentParameters at all? Or we just document it clearly on the keysend method and call out lnd as the exception to being able to do mpp both on the sending method, in the PaymentParameters constructor, and in the RecipientOnionFields constructor? This is all kinda a mess :(

[TheBlueMatt]

Or maybe we just have an lnd_compatible_spontaneous constructor on PaymentParameters and RecipientOnionFields separate from a spontaneous constructor on both and call it a day?

[valentinewallace]

Maybe lnd would be open to upstreaming this change? It looks like a few line change.

Maybe a stopgap could be to support receiving MPP keysends but not sending?

[valentinewallace]

We'll also want to update InvoiceFeatures::for_keysend to set mpp as optional

[TheBlueMatt]

I think we should support sending, just make very clear what's going on in the for_keysend constructors.

[alecchendev]

For how to handle the RecipientOnionFields::payment_secret that gets fed into a spontaneous payment method: I added a fixup commit to actually override whatever is inputted here (before it’d only do this if payment secret wasn’t present), such that for single-part keysend we never send a payment secret and for multi-part keysend we always send a secret. With this + making it clear in RecipientOnionFields docs that setting its payment_secret field does nothing on keysend (still getting around to this), even if it’s kinda weird to have a useless field in this one scenario, the user at least can’t accidentally do something wrong. And this accomodates lnd rejecting single-part keysends with payment secrets without any extra lnd-specific constructors iiuc. Is this a good way to go about it?

Otherwise, is the problem here lnd-specific or that generally not all implementations support receiving MPP keysend + there’s not a good way to signal support for it (since keysend means no invoice to communicate features + other contexts' feature bits supporting MPP and keysend individually don’t necessarily signal support for MPP keysend, and MPP keysend doesn’t have its own feature bit)? If anyone knows off the top of their head about eclair and cln that’d be appreciated otherwise I can take a look soon.

We'll also want to update InvoiceFeatures::for_keysend to set mpp as optional

I haven't looked at the routing code much, but if our payee doesn't support MPP keysend, will there be a chance that we end up using a multi-part route and getting the payment failed when we should've just used a single path route? Or if there's a single path route would that generally be preferred, in which case setting this feature by default would only really help us out if our payee does support it?

Also added a to do list in the description to keep track of some of the things I still gotta get around to :)

[valentinewallace]

I added a fixup commit to actually override whatever is inputted here (before it’d only do this if payment secret wasn’t present), such that for single-part keysend we never send a payment secret and for multi-part keysend we always send a secret.

I'm tentatively with this idea, plus documentation that lnd doesn't accept mpp keysends.

I'm not sure about an lnd-specific method, because I don't think wallets generally have logic to detect which lightning implementation they're sending to, so how usable would this bespoke api be?

Also, worth noting that I don't think said api would decrease overall failures rates that much, we'd just fail at pathfinding instead of at the destination (which is preferable, ofc).

[TheBlueMatt]

For how to handle the RecipientOnionFields::payment_secret that gets fed into a spontaneous payment method: I added a fixup commit to actually override whatever is inputted here (before it’d only do this if payment secret wasn’t present), such that for single-part keysend we never send a payment secret and for multi-part keysend we always send a secret.

I'd kinda prefer we not override what the user says. As long as the method names (and documentation) are super clear on RecipientOnionField we should do what the user told us to. In general I imagine this API will be used by various lightning clients that want to do something semi-custom (eg the podcasting donate-while-streaming stuff), where they may wish to send some extra data in the onion and get it on the other end. If the user gave us some stuff to put in the onion, IMO we should put it in the onion exactly as we were told to cause they expect it on the other end.

That said, its not exactly super critical, ideally they'd use a custom TLV for that or the new metadata field, rather than the secret, which has a more specific purpose.

Otherwise, is the problem here lnd-specific or that generally not all implementations support receiving MPP keysend + there’s not a good way to signal support for it (since keysend means no invoice to communicate features + other contexts' feature bits supporting MPP and keysend individually don’t necessarily signal support for MPP keysend, and MPP keysend doesn’t have its own feature bit)? If anyone knows off the top of their head about eclair and cln that’d be appreciated otherwise I can take a look soon.

Indeed, its not really lnd specific because LDK also doesn't support it! As y'all both noted there's no way to detect it cause there's no invoice.

Instead of blaming lnd in our API, we could instead go for keysend and mpp_keysend to differentiate them without focusing on an implementation, noting in the docs mpp_keysend isnt widely supported (just lnd now).

I haven't looked at the routing code much, but if our payee doesn't support MPP keysend, will there be a chance that we end up using a multi-part route and getting the payment failed when we should've just used a single path route? Or if there's a single path route would that generally be preferred, in which case setting this feature by default would only really help us out if our payee does support it?

If we set mpp_supported in the payee info, it'll maybe get used, if we don't it'll only use a single-path route (or fail if it cant find one).

[TheBlueMatt]

Heh, sorry this went off the rails, do we have (rough) consensus on the way forward here? I'm vaguely happy with really any of the above options, even if I prefer one or another, its all a bit of a mess.

[alecchendev]

Yea sorry for the delay, I'll be getting back to this soon, I should be good to push some changes within the next couple of days based on everything we've discussed

[alecchendev]

Made a bunch of changes, annotating main things here:

• Previously would override user's input and add the preimage as the secret on MPP keysend - got rid of that, sticks to using whatever user provides in RecipientOnionFields
• Previously added allowing no payment secret for MPP keysend from when I thought we didn't need it - got rid of that (will delete these first three commits when I squash)
• Added a parameter in PaymentParameters::for_keysend and InvoiceFeatures::for_keysend to set mpp optional or not
• Added docs to RecipientOnionFields
• DRYed up with Support MPP Keysend #2156 (review)
• Rebased/fixed conflict

[alecchendev]

CI is gonna fail until I squash since I made some changes to the tests (eventually fixed in 0b35ab8)

Btw thanks a ton for all the review as always @TheBlueMatt @valentinewallace!

[TheBlueMatt]

This looks good to me. Can you squash the fixup commits down into a clean git history?

[alecchendev]

Squashed! Also rewrote some of the commit messages/descriptions to make them a bit more intuitive, but the overall commits remained the same

[TheBlueMatt]

LGTM but the test looks borked.

[TheBlueMatt]

LGTM, feel free to squash and we can get another review pass from @valentinewallace

[valentinewallace]

Just some docs nits, LGTM! Sorry for the delay

[alecchendev]

Thanks for all the review!