Skip to content

Send payments to blinded routes#2482

Merged
t-bast merged 2 commits into
masterfrom
send-to-blinded-route
Dec 16, 2022
Merged

Send payments to blinded routes#2482
t-bast merged 2 commits into
masterfrom
send-to-blinded-route

Conversation

@t-bast
Copy link
Copy Markdown
Member

@t-bast t-bast commented Nov 10, 2022

Since blinded routes have to be used from start to end and are somewhat similar to Bolt 11 routing hints, we model them as an abstract single hop during path-finding. This makes it trivial to reuse existing algorithms without any modifications.

We then add support for paying blinded routes. We introduce a new type of recipient for those payments, that uses blinded hops and creates onion payloads accordingly. There is a subtlety in the case where we're the introduction of the blinded route: when that happens we need to decrypt the first payload to figure out where to send the payment.

When we receive a failure from a blinded route, we simply ignore it in retries: we don't know what caused the issue so we assume it's permanent, which makes sense in most cases since we cannot change the relaying parameters (fees and expiry delta are chosen by the recipient).

By building on top of #2480, the change is quite easy to review and doesn't impact the payment state machines much. I've added a lot of e2e tests #2490, where the error handling has (finally) been correctly implemented.

This is an alternative to #2467: what I didn't like in that PR were:

  • the new Partial payment onion class, which is a clear layering violation and requires reviewers to look at the whole flow of code to verify that this object contains the right data (it's quite foot-gunny because a tlv stream isn't strongly typed, it may records you don't expect...)
  • the complexity of PaymentPacket.scala, which became very hard to read
  • the use of a ShortChannelId in blinded hops, which was hiding subtle issues

I tried to build on top of #2467 at first, but when I realized that it was important to add the new abstraction proposed in #2480, it was impossible to do. I did pick a lot of the code from that PR though, most of the actual business logic is the same, it's just the abstractions that are different.

Copy link
Copy Markdown
Contributor

@thomash-acinq thomash-acinq left a comment

Choose a reason for hiding this comment

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

Can you add some tests to check that it actually works?

Comment thread eclair-core/src/main/scala/fr/acinq/eclair/payment/send/Recipient.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentPacket.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/payment/send/Recipient.scala Outdated
@t-bast t-bast force-pushed the send-to-blinded-route branch from 560cb25 to 55f6960 Compare November 15, 2022 12:50
@t-bast
Copy link
Copy Markdown
Member Author

t-bast commented Nov 15, 2022

Can you add some tests to check that it actually works?

I added more tests in 55f6960, let me know if there are specific test cases that would be useful that I missed. I didn't add yet an e2e test because it would require the code from #2471

I also had to add utility test functions to generate blinded routes / bolt12 invoices, those should be cleaned up when we have #2471

Comment thread eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/OfferCodecs.scala Outdated
@t-bast t-bast force-pushed the send-to-blinded-route branch 2 times, most recently from 0564362 to f81512e Compare November 18, 2022 14:56
@t-bast t-bast force-pushed the send-to-blinded-route branch from f81512e to 04019f9 Compare November 25, 2022 12:55
Copy link
Copy Markdown
Contributor

@thomash-acinq thomash-acinq left a comment

Choose a reason for hiding this comment

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

You should add the tests too now that #2499 has been merged, especially the zero-hop test which would not pass currently.

Comment thread eclair-core/src/main/scala/fr/acinq/eclair/payment/send/Recipient.scala Outdated
@t-bast t-bast force-pushed the send-to-blinded-route branch from 04019f9 to 2f65ed0 Compare November 25, 2022 17:43
@t-bast t-bast marked this pull request as ready for review November 25, 2022 17:46
@t-bast t-bast force-pushed the send-to-blinded-route branch 2 times, most recently from c8b9a38 to 426c4d6 Compare December 7, 2022 14:53
@t-bast t-bast requested a review from thomash-acinq December 7, 2022 14:59
@t-bast t-bast force-pushed the send-to-blinded-route branch from 426c4d6 to 39a4e5d Compare December 12, 2022 12:36
Since blinded routes have to be used from start to end and are somewhat
similar to Bolt 11 routing hints, we model them as an abstract single hop
during path-finding. This makes it trivial to reuse existing algorithms
without any modifications.

We then add support for paying blinded routes. We introduce a new type
of recipient for those payments, that uses blinded hops and creates
onion payloads accordingly. There is a subtlety in the case where we're
the introduction of the blinded route: when that happens we need to
decrypt the first payload to figure out where to send the payment.

When we receive a failure from a blinded route, we simply ignore it in
retries: we don't know what caused the issue so we assume it's permanent,
which makes sense in most cases since we cannot change the relaying
parameters (fees and expiry delta are chosen by the recipient).
@t-bast t-bast force-pushed the send-to-blinded-route branch from db15525 to 002a741 Compare December 16, 2022 09:56
thomash-acinq
thomash-acinq previously approved these changes Dec 16, 2022
Copy link
Copy Markdown
Contributor

@thomash-acinq thomash-acinq left a comment

Choose a reason for hiding this comment

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

Looks all good.

Comment thread eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala
@t-bast t-bast merged commit 74719b8 into master Dec 16, 2022
@t-bast t-bast deleted the send-to-blinded-route branch December 16, 2022 15:52
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.

2 participants