[2/4] Route Blinding Receives: Receive and send to a single blinded path in an invoice. #8735
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
changed the title
ellemouton
force-pushed
the
rb-receives
branch
2 times, most recently
from
a7c5c6d to
09d0075
Compare
lnrpc/invoicesrpc/addinvoice.go
Outdated
| // incMultiplier while the maximum HTLC msat value is adjusted via the | ||
| // decMultiplier. If adjustments of the HTLC values no longer make sense | ||
| // then the original HTLC value is used. | ||
| func addPolicyBuffer(policy *models.ChannelEdgePolicy, |
There was a problem hiding this comment.
this is a naive first implementation. Some thoughts:
- We could have different multipliers for each field
- Perhaps not good enough to just multiply by a constant cause this might let the sender do some fingerprinting especially in the case where the route is super short. So perhaps we should also round to the nearest 10 or something?
There was a problem hiding this comment.
Suggestions 1. and 2. would mean that a sender could maybe find out the receiver's implementation (which is maybe not a very strong requirement). With fingerprinting, do you mean the case where a fee adjustment takes place at a suspected hop which is then out of bounds, that the sender can find out that a hop was on the route? This seems to be difficult to do in the current implementation because there is randomization due to the other hop's policies when aggregating the full amount. This is different from the spec example where the same buffer values are used for all hops, which seemsto be worse as that assumption could be used to derive the buffers for each hop.
There was a problem hiding this comment.
Q: Just to verify my understanding is correct, so setting this buffer too high in the first place means the sender of the payment overpays the route ? So we need to find a middle ground between privacy but also increased cost, or is there a way for the receiver to reimburse the sender in favour of his increased privacy demand ?
There was a problem hiding this comment.
would mean that a sender could maybe find out the receiver's implementation
This is what I mean by "fingerprinting"
There was a problem hiding this comment.
Q: Just to verify my understanding is correct, so setting this buffer too high in the first place means the sender of the payment overpays the route ?
Yeah - we dont want these paths to be wayyy too expensive. No there isnt a re-imburse flow. But at the end of the day, we will give the receiver control over these values - so they always have the option of just setting a really wide buffer
There was a problem hiding this comment.
This seems to be difficult to do in the current implementation because there is randomization due to the other hop's policies when aggregating the full amount. This is different from the spec example where the same buffer values are used for all hops, which seemsto be worse as that assumption could be used to derive the buffers for each hop.
meaning you think this is ok as is?
|
|
||
| // FindBlindedPaths finds a selection of paths to the destination node that can | ||
| // be used in blinded payment paths. | ||
| func (r *ChannelRouter) FindBlindedPaths(destination route.Vertex, |
There was a problem hiding this comment.
some food for thought here:
- If we let the user set a non-equal MinHops and MaxHops, then this gonna pretty much select the shortest paths as is.. since those will always have the largest success probability. So perhaps we should also have a MinNumPaths and then always only select paths of the same length as long there are enough with that length to satisfy the MinNumPaths?
There was a problem hiding this comment.
- we also in general probably dont want to include paths of diff lengths. Currently the user has the power to enforce this through just setting MaxNumHops = MinNumHops but I dont think they should have to do that
There was a problem hiding this comment.
wanna hear other people's thoughts before I update anything though
There was a problem hiding this comment.
I tend to agree to enforce an equal length of routes, as the min hops number effectively determines the distance via sorting when we only know constant a priori probabilities (as you say). Also, if one looks at the routes created, if one allows min/max to be different, then we'll create shorter routes that are subsets of the larger ones. I wonder if that enables more probing vectors or is beneficial by creating a seemingly larger anonymity set?
There was a problem hiding this comment.
hmm what about making all of the fixed length by adding ddmmy hops ?
There was a problem hiding this comment.
Also, if one looks at the routes created, if one allows min/max to be different, then we'll create shorter routes that are subsets of the larger ones.
I think we should set minNumHops = maxNumHops as otherwise we'll give hints about which nodes are included in the other, longer paths (see the tests from the previous commit). We would degrade the longer path's privacy to the shorter ones if one can assume that all intro nodes may be present in the other paths as relay nodes. A downside of that is that it restricts the number of possible paths, so it may not be possible to find some path. This also circumvents the problem with the apriori probability (shortest paths are selected first).
There was a problem hiding this comment.
dont think this is applicable anymore with the dummy hops. All paths are equal length always now.
FindBlindedPaths can be left to just return paths of multiple lengths - it is fine cause we pad with dummy hops later
There was a problem hiding this comment.
Just wanted to be clearer here, we could get the following paths:
// We expect the following paths:
// - B, D
// - F, B, D
// - E, B, D
// - C, D
// - E, C, D
I could assume that B, F, E, and C may be part of the other blinded routes, because as they were selected as introduction nodes, they may be included in other routes as well, because for example B, D is a sub-route of F, B, D. Not sure if this is too constructed and it behaves differently for larger networks? The shorter ones would be selected first in the current setup, so this argument is weakened a bit.
There was a problem hiding this comment.
sorry im still not sure i follow - we would only see intro nodes & we would not know that the routes are different lengths... so how do we know that one hop is part of the other?
There was a problem hiding this comment.
We would degrade the longer path's privacy to the shorter ones if one can assume that all intro nodes may be present in the other paths as relay nodes. A downside of that is that it restricts the number of possible paths, so it may not be possible to find some path. This also circumvents the problem with the apriori probability (shortest paths are selected first).
Ahh interesting point 👀, I would for the first release maybe focus on successful paths and introduce more privacy focused settings down the road ?
ellemouton
force-pushed
the
elle-rb-receives-1
branch
from
e92e850 to
d537c35
Compare
ellemouton
changed the title
|
cc: @calvinrzachman for review |
lnrpc/invoicesrpc/addinvoice.go
Outdated
| plainText, err := record.EncodeBlindedRouteData( | ||
| hop.data, | ||
| ) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
|
|
||
| paymentPath[i] = &sphinx.HopInfo{ | ||
| NodePub: hop.nodeID, | ||
| PlainText: plainText, | ||
| } |
There was a problem hiding this comment.
one could save the encodings in paymentPath already in the previous iterations?
lnrpc/invoicesrpc/addinvoice.go
Outdated
| // iteration is required for padding values on the BigSize encoding bucket | ||
| // edges. The number of iterations that this function takes is also returned for | ||
| // testing purposes. | ||
| func padHopInfo(hopInfo []*hopData) ([]*sphinx.HopInfo, int, error) { |
There was a problem hiding this comment.
I see the problem clearer now compared to the first review. Trying to think about other alternatives. Maybe one could replace PadBy with an erring PadTo method that would fail in case we can't pad to the requested size (at the varInt transitions) and we'd then we'd bump the requested size each time we get an error, which should eventually lead to a valid padding. This would hide more complexity in PadTo
There was a problem hiding this comment.
This would hide more complexity in PadTo
Im not sure we want to add more complexity there. To me, PadTo shouldnt need to introspect how the TLV package is encoding things. To me it is nicer to have an outer function like this call the overall record.EncodeBlindedRouteData method so that we can really be sure that the lengths are converging. Else we are only zooming in on one field.
open to more thoughts here.
There was a problem hiding this comment.
to me the record package should not necessarily be aware of how we are going about using the padding. Although you could argue that it already kind of is aware cause of us only exposing PadBy(n) vs something like AddPadding([]byte).
But yeah I sort of prefer this iterative style here rather than relying on errors being returned & looping based off errors.
lnrpc/invoicesrpc/addinvoice.go
Outdated
| // incMultiplier while the maximum HTLC msat value is adjusted via the | ||
| // decMultiplier. If adjustments of the HTLC values no longer make sense | ||
| // then the original HTLC value is used. | ||
| func addPolicyBuffer(policy *models.ChannelEdgePolicy, |
There was a problem hiding this comment.
Suggestions 1. and 2. would mean that a sender could maybe find out the receiver's implementation (which is maybe not a very strong requirement). With fingerprinting, do you mean the case where a fee adjustment takes place at a suspected hop which is then out of bounds, that the sender can find out that a hop was on the route? This seems to be difficult to do in the current implementation because there is randomization due to the other hop's policies when aggregating the full amount. This is different from the spec example where the same buffer values are used for all hops, which seemsto be worse as that assumption could be used to derive the buffers for each hop.
There was a problem hiding this comment.
As always great work on this 👏
Wanted to play around with it via the lncli tool but I think its still not updated yet ?
Apart from that I had mostly general questions in regards of the blinded path topic, especially dummy hops which we could include to make every blinded path of the same size ? Will continue with PR 3
|
note to self: need to add new feature bit to the invoice here EDIT: done |
ellemouton
force-pushed
the
elle-rb-receives-1
branch
from
d537c35 to
6307874
Compare
lnrpc/invoicesrpc/addinvoice.go
Outdated
| // iteration is required for padding values on the BigSize encoding bucket | ||
| // edges. The number of iterations that this function takes is also returned for | ||
| // testing purposes. | ||
| func padHopInfo(hopInfo []*hopData) ([]*sphinx.HopInfo, int, error) { |
There was a problem hiding this comment.
This would hide more complexity in PadTo
Im not sure we want to add more complexity there. To me, PadTo shouldnt need to introspect how the TLV package is encoding things. To me it is nicer to have an outer function like this call the overall record.EncodeBlindedRouteData method so that we can really be sure that the lengths are converging. Else we are only zooming in on one field.
open to more thoughts here.
lnrpc/invoicesrpc/addinvoice.go
Outdated
| // incMultiplier while the maximum HTLC msat value is adjusted via the | ||
| // decMultiplier. If adjustments of the HTLC values no longer make sense | ||
| // then the original HTLC value is used. | ||
| func addPolicyBuffer(policy *models.ChannelEdgePolicy, |
There was a problem hiding this comment.
would mean that a sender could maybe find out the receiver's implementation
This is what I mean by "fingerprinting"
ellemouton
force-pushed
the
rb-receives
branch
3 times, most recently
from
08a136e to
ed7af80
Compare
This commit adds all the logic for building a blinded path (from a given route) and packaging it up in a zpay32.BlindedPaymentPath struct so that it is ready for adding to an invoice. It also includes logic for padding a path with dummy hops. Note that in this commit, the logic for choosing an actual path to us that can then be used in a blinded path is abstracted away. This logic will be fleshed out in a future commit.
Here we add a new `Blind` option to the `AddInvoiceData` which will signal that the new invoice should encode a blinded route. Certain other changes are also made in the case that this invoice contains a blinded route: 1) the payment address/secret no longer needs to be in the invoice itself since it will be put in the `PathID` recored of the encrypted recipient record for our hop. 2) When we sign the invoice, we now use an ephemeral key since we dont want the sender to be able to derive our real node pub key from the invoice signature. 3) The invoice's FinalCLTV field should be zero for blinded invoices since the CLTV delta info will be communicated in the accumulated route policy values.
This commit adds a new function, `findBlindedPaths`, that does a depth first search from the target node to find a set of blinded paths to the target node given the set of restrictions. This function will select and return any candidate path. A candidate path is a path to the target node with a size determined by the given hop number constraints where all the nodes on the path signal the route blinding feature _and_ the introduction node for the path has more than one public channel. Any filtering of paths based on payment value or success probabilities is left to the caller.
Add a `FindBlindedPaths` method to the `ChannelRouter` which will use the new `findBlindedPaths` function to get a set of candidate blinded path routes. It then uses mission control to select the best of these paths. Note that as of this commit, the MC data we get from these queries won't mean much since we wont have data about a channel in the direction towards us. But we do this now in preparation for a future PR which will start writing mission control success pairs for successful receives from blinded route paths.
Expose the ability to add blinded paths to an invoice. Also expose various configuration values. We also let the lncfg.Invoices struct satisfy the Validator interface so that we can verify all its config values in one place.
In this commit, we expose the option to create an invoice containing blinded paths to lncli.
In preparation for calling the TLV payload parsing logic recursively for when we need to peel dummy hops from an onion, this commit creates a new extractTLVPayload function. This is a pure refactor.
In this refactor commit, we extract all the steps from extractTLVPayload that have to do with parsing the payload from the sender and verifying the presence of various fields from the sender.
We further break up the extracTLVPayload into more modular pieces. The pieces are structured in such a way as to prepare for extracTLVPayload being called in a recursive manner from within `deriveBlindedRouteForwardingInfo` when we add the logic for handling dummy hops in a later commit. With this refactor, we completey remove the BlindingKit's DecryptAndValidateFwdInfo method.
This will be required to construct a new hop iterator for when peeling of dummy hops is done for route blinding.
We've covered all the logic for building a blinded path to ourselves and putting that into an invoice - so now we start preparing to actually be able to recognise the incoming payment as one from a blinded path we created. The incoming update_add_htlc will have an `encrypted_recipient_data` blob for us that we would have put in the original invoice. From this we extract the PathID which we wrote. We consider this the payment address and we use this to derive the associated invoice location. Blinded path payments will not include MPP records, so the payment address and total payment amount must be gleaned from the pathID and new totalAmtMsat onion field respectively. This commit only covers the final hop payload of a hop in a blinded path. Dummy hops will be handled in the following commit.
If a blinded path payload contains a signal that the following hop on the path is a dummy hop, then we iteratively peel the dummy hops until the final payload is reached.
The route blinding itests are now updated so that recipient logic is tested. The creation of a blinded route is also now done through the AddInvoice API instead of manually.
Update the SendPayment flow so that it is able to send to an invoice containing a blinded path.
Update one of the route blinding itests to do a full end-to-end test where the recipient generates and invoice with a blinded path and the sender just provides that invoice to SendPayment. The tests also covers the edge case where the recipient is the introduction node.
Make various sender side adjustments so that a sender is able to send an MP payment to a single blinded path without actually including an MPP record in the payment.
Add an itest that tests the addition of dummy hops to a blinded path. By testing that invoices containing such a path can be paid, it also tests the peeling of dummy hops by the receiver.
There was a problem hiding this comment.
Thanks yall.
I've addressed most comments i think. Many i've addressed in new commits at the end so that the OG commits dont need to be changed too much.
Want to emphasise that this PR is mainly about getting initial plumbing into place & i think we could theoretically go around in circles indefinitely if we try get max privacy from the very start. So I really think we should just focus on bare essentials here and we can focus on smaller tweaks later on.
So let me know if there are un-addressed comments you guys feel very strongly about & maybe consider providing a code snippet if possible. I fear the PR is getting way too big.
| // NextNodeID is the node ID of the next node on the path. In the | ||
| // context of blinded path payments, this is used to indicate the | ||
| // presence of dummy hops that need to be peeled from the onion. | ||
| NextNodeID tlv.OptionalRecordT[tlv.TlvType4, *btcec.PublicKey] |
There was a problem hiding this comment.
ok so I wrote a test to see what the actual sizes are. It can actually be that the non-final-hop payloads can be more than the average dummy if things fields like the next-blinding-override and/or feature fields are set.
But yeah peeps can probably assume that next-blinding-override and features are not set usually & then can make assumptions from there.
Looks like the change required is quite minimal. So gonna add 👍
lnrpc/invoicesrpc/addinvoice.go
Outdated
| // requires for the final hop of the payment. | ||
| minFinalCLTVExpiryDelta uint32 | ||
|
|
||
| // blocksUntilExpiry is the number of blocks that this blinded path |
There was a problem hiding this comment.
i dont think i completely follow (these expiries and cltv's always confuse me 🙃 )
By default:
- invoice expiry: 24 hours (ie, 144 blocks)
- blocks till expiry: 288 blocks
- minFinalCLTVDelta = 83.
why would we want to change minFinalCtlvdelta to 144?
moreover I think we still need to account for the overall max_cltv_delta of a route in general currently in LND (2016 blocks)?
can you elaborate on this part pls?
lnrpc/invoicesrpc/addinvoice.go
Outdated
|
|
||
| // dummyHopPolicy holds the policy values that should be used for dummy | ||
| // hops. Note that these will _not_ be buffered via addPolicyBuffer. | ||
| dummyHopPolicy *blindedHopPolicy |
There was a problem hiding this comment.
ok i've added a commit at the end that does this in a more sophisticated way. Let me know what you think. We just are not gonna get this perfect in this first PR I think
lnrpc/invoicesrpc/addinvoice.go
Outdated
| }, | ||
| minNumHops: cfg.MinNumHops, | ||
| // TODO: make configurable | ||
| dummyHopPolicy: &blindedHopPolicy{ |
There was a problem hiding this comment.
see new commit added at the end of the PR
|
|
||
| // FindBlindedPaths finds a selection of paths to the destination node that can | ||
| // be used in blinded payment paths. | ||
| func (r *ChannelRouter) FindBlindedPaths(destination route.Vertex, |
There was a problem hiding this comment.
sorry im still not sure i follow - we would only see intro nodes & we would not know that the routes are different lengths... so how do we know that one hop is part of the other?
|
|
||
| // Don't bother adding a route if its success probability less | ||
| // minimum that can be assigned to any single pair. | ||
| if totalRouteProbability <= DefaultMinRouteProbability { |
There was a problem hiding this comment.
could you clarify pls? maybe provide a code snippet
This commit introduces more sophisticated code for selecting dummy hop policy values for dummy hops in blinded paths. For the case where the path does contain real hops, the dummy hop policy values are derived by taking the average of those hop polices. For the case where there are no real hops (in other words, we are the introduction node), we use the default policy values used for normal ChannelUpdates but then for the MaxHTLC value, we take the average of all our open channel capacities.
There was a problem hiding this comment.
Thank you for this monster-size PR 🥇 high quality material shipping soon :)
Nit Comment:
Maybe we can be more detailed when returning the error requiring hints and blinded paths, meaning that we already consider private channels in the blinded paths therefore there is no need to require them there - just realized this fact :)
if ctx.IsSet("private") && ctx.IsSet("blind") {
return fmt.Errorf("cannot include both route hints and " +
"blinded paths in the same invoice")
}
| BaseFee: r.server.cfg.Bitcoin.BaseFee, | ||
| MinHTLCMsat: r.server.cfg.Bitcoin.MinHTLCIn, | ||
|
|
||
| // MaxHTLCMsat will be calculated on the fly by using |
There was a problem hiding this comment.
hmm, I think we can be more explicit in the comment and mention also that this only works when we are the introduction point of the route ?
| @@ -884,7 +884,9 @@ func testErrorHandlingOnChainFailure(ht *lntest.HarnessTest) { | |||
| // The following graph is created where Dave is the destination node, and he | |||
| // will choose Carol as the introduction node. The channel capacities are set in | |||
| // such a way that Alice will have to split the payment to dave over both the | |||
| // A->B->C-D and A->E->C->D routes. | |||
| // A->B->C-D and A->E->C->D routes. The Carol-Dave channel will also be made | |||
Tracking Issue: #6690
Depends on lightningnetwork/lightning-onion#63
High level overview:
This PR is the first in a set of 3 PRs that will implement route blinding receive logic in LND.
As LND does not yet support blot 12 invoices, this PR adds them to the Bolt11 invoice serialisation.
With this PR, a user can request that a bolt 11 invoice include blinded paths and will also
be able to pay to a bolt 11 invoice that includes blinded paths. However, with this PR, only one of the
paths in the invoice will be used. A follow up PR will add the ability to make use of multiple paths.
UPDATE: This has been updated to include dummy hop padding :)
Lower level overview: