Skip to content

Route to blind#2429

Closed
thomash-acinq wants to merge 8 commits into
masterfrom
route-to-blind
Closed

Route to blind#2429
thomash-acinq wants to merge 8 commits into
masterfrom
route-to-blind

Conversation

@thomash-acinq
Copy link
Copy Markdown
Contributor

@thomash-acinq thomash-acinq commented Sep 22, 2022

Enable routing to blinded paths, needed for BOLT 12.

Before blinded paths, payments would be sent a single nodeId, they can now also be sent to blinded paths.
We introduce a new Recipient that can be either a ClearRecipient corresponding to the classical nodeId or a BlindRecipient corresponding to a blinded path.
The Recipient trait encapsulate everything that is needed to compute the final payloads to put in the onion (for the classical case, this covers the payment secret, payment metadata and also additional TLVs used for keysend or trampoline).

BOLT 12 allows to provide multiple blinded paths, this means that we don't know which recipient to use until after the router finds a route. That's why we no longer build the final payload early and build all of the payloads together.

@thomash-acinq thomash-acinq force-pushed the route-to-blind branch 3 times, most recently from 3c06cee to e392ce0 Compare September 22, 2022 15:42
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 28, 2022

Codecov Report

Merging #2429 (4a3d736) into master (6e381d4) will increase coverage by 0.21%.
The diff coverage is 93.88%.

@@            Coverage Diff             @@
##           master    #2429      +/-   ##
==========================================
+ Coverage   84.84%   85.06%   +0.21%     
==========================================
  Files         198      199       +1     
  Lines       15796    15865      +69     
  Branches      680      684       +4     
==========================================
+ Hits        13402    13495      +93     
+ Misses       2394     2370      -24     
Impacted Files Coverage Δ
...n/scala/fr/acinq/eclair/json/JsonSerializers.scala 95.12% <ø> (ø)
...c/main/scala/fr/acinq/eclair/payment/Invoice.scala 100.00% <ø> (ø)
...la/fr/acinq/eclair/payment/send/PaymentError.scala 25.00% <ø> (+5.00%) ⬆️
.../fr/acinq/eclair/wire/protocol/RouteBlinding.scala 95.65% <ø> (ø)
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 50.26% <40.00%> (-0.28%) ⬇️
.../scala/fr/acinq/eclair/payment/Bolt12Invoice.scala 96.73% <77.77%> (-2.13%) ⬇️
...r/acinq/eclair/payment/send/PaymentLifecycle.scala 85.95% <90.24%> (-0.98%) ⬇️
...r/acinq/eclair/payment/send/PaymentInitiator.scala 89.31% <91.83%> (-0.54%) ⬇️
...main/scala/fr/acinq/eclair/payment/Recipient.scala 94.73% <94.73%> (ø)
.../scala/fr/acinq/eclair/payment/Bolt11Invoice.scala 86.24% <100.00%> (+0.22%) ⬆️
... and 20 more

- Add an optional blinded route at the end of routes returned by the router
- Because the destination is not known befoure routing (the router may chose between several blinded routes), we defer creating the final payload to after the routing
@thomash-acinq thomash-acinq marked this pull request as ready for review September 29, 2022 14:09
Make it work when we are the introduction node
Copy link
Copy Markdown
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

I spent quite a while playing around with the implementation to understand where the pain points are, and it's now more clear to me what abstractions should be used here.

I think introducing a Recipient abstraction makes a lot of sense, but I think it should be slightly different from what you did.
You introduced multiple recipients to mimick the fact that blinded routes don't end at the same nodeId.
However, that is a leaky abstraction: there is a single recipient, not multiple ones.
I think that it should be the goal of the Recipient abstraction to hide the implementation details of inserting a dummy nodeId at the end of each blinded path.

Also, your Recipient trait leaks implementation details of blinded path by explicitly introducing an introduction point and a set of nodes.
I think we can do much better by representing each blinded path as a single virtual edge (Invoice.ExtraEdge) since a blinded path must always be used in its entirety.
That's what I did in 5169307 and it fits quite well in the overall payments architecture:

  • it requires no changes whatsoever to path-finding components: a blinded edge works exactly the same as a normal edge from the Router's point of view
  • one of the subtle benefits of that is that we avoid overflowing the default route length boundaries when using a multi-hops blinded route (which will likely be quite common with dummy hops)
  • it makes it explicit that blinded hops don't have a shortChannelId, which shows parts of the codebase that must be updated to correctly take this into account
  • the changes to PaymentLifecycle are also minimal, the only part that needs to be updated is the error handling, and with the explicit BlindedHop it should be quite trivial

I implemented this far enough to convince myself that it would work end-to-end: 5169307
I hope I didn't miss anything important, but I think building on top of this design will result in a smaller PR that fits more naturally in the existing architecture (and will help us detect potential bugs more easily).

@t-bast t-bast deleted the route-to-blind branch November 15, 2022 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants