Add a payment recipient abstraction

[t-bast]

We were previously directly creating onion payloads inside the various payment state machines and manipulating tlv fields. This was a layering violation that was somewhat ok because in most cases we only needed to create the onion payload for the recipient at the beginning of the payment flow and didn't need to modify it, except for a small change in the MPP case. When we introduced MPP and started modifying tlv payloads "on-the-fly" inside payment FSMs, I knew this wasn't very clean, but it probably wasn't worth a big refactoring yet.

The introduction of trampoline payments was another hint that something wasn't right with the model, because we had to handle trampoline onions directly in the payment initiator: again, we deferred a bigger refactoring because it was early and we weren't really sure what the right model would be to work with other future feature work.

The introduction of blinded payments is another feature where we run into this issue, because we can only build the onion payload for the recipient after we've chosen the routes (to insert the right encrypted recipient data) and how to split the amount, so our model of creating a FinalPayload at the beginning of the payment flow doesn't really fit.

We clean this up by introducing payment recipients that abstract away the creation of onion payloads. This makes it much easier to integrate blinded payments. It also allows us to clean up the way we do trampoline payments and potentially support splitting across multiple trampoline routes (not included in this PR as this change isn't immediately needed).

ℹ️ It's important to note that our support for sending trampoline payments has always just been an imperfect implementation mostly meant to test the trampoline relay scenarios. It is a bit weird in some places, and when trampoline payments come closer to being standardized, we should introduce a dedicated FSM on top of the MultiPartPaymentLifecycle FSM (that chooses how to split across multiple trampoline routes if needed and correctly handles trampolines fees/expiry retries). I don't think this should be done now and it seems to me that the current design will make it easy to implement when needed, but let me know if you feel otherwise. Another reason to defer this is that I'd like to first introduce support for blinded trampoline routes, which has a lot of benefits!

[t-bast]

@pm47 @thomash-acinq I tagged you for review, but I think we should wait to integrate this only after the release, since it's a non-trivial refactoring, so there's no rush. It may be useful to have a look at #2481 and #2482 that build on top of this if you want to see the full picture.

We can keep building on top of these branches until the release is out.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 93.45794% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.89%. Comparing base (2a7649d) to head (9627aca).
⚠️ Report is 532 commits behind head on master.

Files with missing lines	Patch %	Lines
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	40.00%	6 Missing ⚠️
...r/acinq/eclair/payment/send/PaymentInitiator.scala	90.38%	5 Missing ⚠️
...r/acinq/eclair/payment/send/PaymentLifecycle.scala	93.24%	5 Missing ⚠️
...scala/fr/acinq/eclair/payment/send/Recipient.scala	92.85%	3 Missing ⚠️
.../scala/fr/acinq/eclair/payment/PaymentPacket.scala	88.23%	2 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2480   +/-   ##
=======================================
  Coverage   84.88%   84.89%           
=======================================
  Files         201      202    +1     
  Lines       15850    15905   +55     
  Branches      671      686   +15     
=======================================
+ Hits        13454    13502   +48     
- Misses       2396     2403    +7     

Files with missing lines	Coverage Δ
...n/scala/fr/acinq/eclair/json/JsonSerializers.scala	95.12% <100.00%> (ø)
.../scala/fr/acinq/eclair/payment/Bolt11Invoice.scala	93.06% <100.00%> (+6.96%)	⬆️
.../scala/fr/acinq/eclair/payment/Bolt12Invoice.scala	98.83% <ø> (-0.04%)	⬇️
...c/main/scala/fr/acinq/eclair/payment/Invoice.scala	100.00% <100.00%> (ø)
.../scala/fr/acinq/eclair/payment/PaymentEvents.scala	97.61% <100.00%> (ø)
...cinq/eclair/payment/receive/MultiPartHandler.scala	93.65% <ø> (ø)
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala	95.89% <100.00%> (-2.05%)	⬇️
...clair/payment/send/MultiPartPaymentLifecycle.scala	94.07% <100.00%> (-0.40%)	⬇️
.../src/main/scala/fr/acinq/eclair/router/Graph.scala	97.47% <100.00%> (ø)
...cala/fr/acinq/eclair/router/RouteCalculation.scala	95.48% <100.00%> (+0.78%)	⬆️
... and 7 more

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thomash-acinq]

I had hoped the trampoline implementation details would be hidden inside Recipient but it creates confusing redundant fields: nodeId/targetNodeId and totalAmount/targetTotalAmount. You've explained why you did this but I don't think it's the right solution. And actually you haven't completely solved the problem of the base fee, it is still there in the targetMaxFee.
Since Multipart trampoline is not used anywhere today I think we should ignore it for now and add support for it in the router when the time is right.

[t-bast]

I had hoped the trampoline implementation details would be hidden inside Recipient but it creates confusing redundant fields: nodeId/targetNodeId and totalAmount/targetTotalAmount. You've explained why you did this but I don't think it's the right solution. And actually you haven't completely solved the problem of the base fee, it is still there in the targetMaxFee.

I agree, I was hoping this could get simpler, but now I don't think it can (at least not without deep changes to MultiPartPaymentLifecycle which I'd like to avoid), because we really have to target the trampoline node for path-finding, otherwise we cannot correctly deal with the fact that the fees for the trampoline hop should be paid only once even if we find multiple paths to the trampoline node.

But I'm completely open to exploring different designs, if you have ideas for different abstractions that can work without big changes to PaymentLifecycle/MultiPartPaymentLifecycle/RouteCalculation and better handle trampoline, don't hesitate to try them out and share them!

Since Multipart trampoline is not used anywhere today I think we should ignore it for now and add support for it in the router when the time is right.

I think you misunderstood what I meant by MPP aggregation issues, this is an issue that we have today (it isn't linked to splitting across multiple trampoline nodes). Let me detail the issue more, hopefully you can find ideas on how to address it better than what I currently did.

If we want to get rid of the targetNodeId / targetAmount fields, we need to be able to just find routes to the final node and let path-finding abstract away the fact that the last hop is using trampoline. This means some form of injecting an edge Ted -> Dave where Ted is the trampoline node we want to use and Dave is the final recipient. Let's assume that Alice wants to send 100 000 sat to Dave.

The issue arises when we split the payment to Ted, ie path-finding says we must split the payment across the following two routes:

• 40 000 sat through Alice -> Bob -> Ted -> Dave
• 60 000 sat through Alice -> Carol -> Ted -> Dave

What will really happen is that the payment will be split between Alice and Ted, but Ted will aggregate the two payments before relaying it, potentially with a single HTLC to Dave. If we get rid of the targetNodeId / targetAmount mechanism, those two payments will be sent independently and can't be aggregated by Ted, which means that:

• Alice is paying more fees that she should, because she's paying the base fee of the trampoline hop twice instead of once
• The success probability and fee efficiency are lower, because Ted has to handle the two payments separately

I think we really need to handle this MPP aggregation at the trampoline node right now, because it is quite fundamental in the design and one of the important benefits of trampoline (we leverage this a lot in Phoenix to ensure that the trampoline node knows the total amount to be able to decide whether a new channel needs to be opened to carry the payment or not).

add support for it in the router when the time is right.

I don't know if that would even be the right approach, because I don't see how to do that inside the router while taking into account the fact that a split payment may have some routes that fail and other that work and are pending waiting for the complete amount to be sent. My current understanding is that we must handle that at the MultiPartPaymentLifecycle layer, so the router should just be a generic path-finding algorithm, otherwise the router would need to be much more stateful and would end up duplicating state that already has to be inside the MultiPartPaymentLifecycle...

[thomash-acinq]

this is an issue that we have today (it isn't linked to splitting across multiple trampoline nodes).

OK, I had missed this. I guess this solution is good enough for now then.

[t-bast]

I've reworked the Recipient abstraction to get rid of the target(NodeId|Amount) hacks, I feel this is much better now. The trick was simply to carry the Recipient abstraction all the way to the Router using RouteRequest, which lets the trick of using a random nodeId be neatly confined in RouteCalculation.scala.

[thomash-acinq]

Great to finally converge on a solution we're both happy with.

[pm47]

This is a light review, I'm missing context that led to the design decisions you made, but it LGTM.