[3/4] Route Blinding: send MPP over multiple blinded paths #8764
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
ellemouton
force-pushed
the
rb-send-via-multi-path
branch
from
4e7bec8
to
374f31e
Compare
ellemouton
changed the title
ellemouton
force-pushed
the
rb-send-via-multi-path
branch
from
374f31e
to
850a20b
Compare
ellemouton
force-pushed
the
elle-rb-receives-2
branch
from
801c676
to
d1f0f5d
Compare
ellemouton
force-pushed
the
rb-send-via-multi-path
branch
2 times, most recently
from
0c3de82
to
50b4295
Compare
There was a problem hiding this comment.
Completed an initial pass, need to fill the following gaps in my mental model:
- What makes this case different than the single path MPP case?
- Why do the pubkey need to be swapped out?
- What are the new semantics and constraints re the final CLTV as relates to the path?
ellemouton
force-pushed
the
elle-rb-receives-2
branch
from
d1f0f5d
to
4bcf5f1
Compare
ellemouton
force-pushed
the
rb-send-via-multi-path
branch
from
50b4295
to
53dab50
Compare
There was a problem hiding this comment.
Thanks for taking a look @Roasbeef 🦖
Responding to your Qs:
What makes this case different than the single path MPP case?
In the single MPP case, a single blinded path can be used multiple times in the construction of the various paths used for the parts. So importantly: the end of the path will always be the same (same blinded path). Meaning that if something fails within that part of the route, nothing can be done by the sender & retries are not helpful.
With this PR, the sender finally can take advantage of all the different blinded paths provided by the recipient in the invoice. So now, the MPP parts can have different endings. If an error occurs in one blinded path, the sender can try another. Or 2 MPP parts can be sent along two different blinded paths.
Why do the pubkeys need to be swapped out?
In our pathfinding logic today, things work cause we have a single target pubkey. Even if we only have hop hints to rely on, all paths lead to Rome (Rome being this single pub key). With blinded paths, the last hop of each hop will have a different pub key. In order to take advantage of the existing path finding logic as it stands today, we swap out these various different destination pub keys for a single one so that the blinded path edges act no differently to hop hints & once again we can rely on all paths leading to Rome.
It's only at onion construction time (once a path has been chosen) where we actually care about the true value of the destination pub key.
What are the new semantics and constraints re the final CLTV as relates to the path?
The important thing to know with route blinding is that the min_final_cltv_expiry_delta is no longer communicated explicitly to the sender via the invoice. Instead, it is now included in the over-all CLTV delta of the path. What this then looks like in the code is that for the additional edges we overlay onto the graph, we make is such that the CLTV delta for the intro node is this accumulated CLTV delta and then each hop after that has a CLTV delta of 0. So we just say that the final CLTV delta is also 0.
However there is one edge case here: it is for the case where the path has only an introduction node (and no hops). In this case: the intro node is also the intro node and ie, the CLTV for the path is the same as the final CLTV. BUT we cannot rely on this being correctly overlayed via additional edges here cause there are no additional edges if the intro node is the only node. So we have to make sure that the finalCLTV in this unique case is set to the path's CLTV and not left as zero.
Finally, there is one more assumption we make here: if a recipient provides us with a set of paths of various lengths but one of those paths is an intro-node-only path, then we just go ahead and discard the rest of the paths since it doesnt make sense to try those since we know that the intro node is the dest node.
I wonder if this would be considered a wrong behaviour of the person providing us the invoice, so maybe we should consider this as an invalid invoice to spot wrong implementations ? |
|
@ellemouton, remember to re-request review from reviewers when ready |
|
I went to create an invoice with a blinded path on testnet, and ran into this panic: Perhaps it's the case of a missing edge policy: https://github.com/ellemouton/lnd/blob/53dab50d393f3acf80d846e5bdcbdad8ee4b6cb5/routing/router.go#L725-L728 |
|
Can be rebased to point to master now! |
|
Hmm seems like we need to check that the inpolicy is not nil as we already do in other parts in the routing codebase ? |
|
@ziggie1984 yep was thinking the same thing, I'll add that then try again. FWIW, this is a very very very old node, which makes it great for testing! |
This commit introduces a new type, `BlindedPaymentPathSet`. For now, it holds only a single `BlindedPayment` but eventually it will hold and manage a set of blinded payments provided for a specific payment. To make the PR easier to follow though, we start off just letting it hold a single one and do some basic replacements.
Building on from the previous commit, here we pass the PathSet around everywhere where we previously passed around the single BlindedPayment.
If multiple blinded paths are provided, they will each have a different pub key for the destination node. This makes using our existing pathfinding logic tricky since it depends on having a single destination node (characterised by a single pub key). We want to re-use this logic. So what we do is swap out the pub keys of the destinaion hop with a pseudo target pub key. This will then be used during pathfinding. Later on once a path is found, we will swap the real destination keys back in so that onion creation can be done.
Instead of needing to remember how to handle the FinalCLTV value of a blinded payment path at various points in the code base, we hide the logic behind a unified FinalCLTVDelta method on the blinded path.
Continue adding some complexity behind the BlindedPaymentPathSet. What we do here is add a new IntroNodeOnlyPath method. The assumption we make here is: If multiple blinded paths are provided to us in an invoice but one of those paths only includes an intro node, then there is no point in looking at any other path since we know that the intro node is the destination node. So in such a case, we would have discarded any other path in the `NewBlindedPaymentPathSet` constructor. So then we would only have a single blinded path made up of an introduction node only. In this specific case, in the `newRoute` function, no edge passed to the function would have a blindedPayment associated with it (since there are no blinded hops in this case). So we will have a case where `blindedPathSet` passed to `newRoute` is not nil but `blindedPayment` is nil since nonce was extacted from any edge. If this happens then we can assume that this is the Intro-Node-Only situation described above. And so we grabe the associated payment from the path set.
ellemouton
force-pushed
the
rb-send-via-multi-path
branch
from
53dab50
to
d2bc6ea
Compare
We only need the ChannelID, so no need to use the InPolicy which might be nil.
ellemouton
force-pushed
the
rb-send-via-multi-path
branch
from
d2bc6ea
to
b271922
Compare
There was a problem hiding this comment.
Thanks for the review @ziggie1984 & thanks for catching that panic @Roasbeef - turns out, we really dont need the InPolicy here. We just need the ChannelID, so i've removed the InPolicy dep (see the last commit)
| } | ||
|
|
||
| // For now, we assert that all the paths have the same set of features. | ||
| features := paths[0].Features |
There was a problem hiding this comment.
invoice features & path features are different.
not sure im completely following the question?
|
oops - I accidentally clicked re-request @ziggie1984 🙈 sorry about that |
To prevent an attacker from causing us to assign a huge in-memory buffer, we place a cap on the maximum cipher text size of a blinded path hop.
|
Tried out the latest branch, and I was able to make an initial blinded paths payment! Ran into some other issues along the way:
|
|
I think the instances of Some other featrure requests after trying things out:
|
There was a problem hiding this comment.
Reviewed 2 of 9 files at r2, 5 of 10 files at r6, 5 of 5 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @bitromortac, @ellemouton, and @ziggie1984)
In this PR, we remove the restriction of only being able to send to a single blinded payment path.
Nodes will now be able to take advantage of the multiple blinded paths provided for a given
payment to perform MPP if needed.
Depends on: #8735
Tracking Issue: #6690
This change is