-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[2/4] Route Blinding Receives: Receive and send to a single blinded path in an invoice. #8735
base: elle-rb-receives-1
Are you sure you want to change the base?
[2/4] Route Blinding Receives: Receive and send to a single blinded path in an invoice. #8735
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. 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 Configration File (
|
a7c5c6d
to
09d0075
Compare
expectedFinalSize: 10, | ||
}, | ||
{ | ||
name: "existing padding and diff of 1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: the fact that it takes 2 iterations less by just having an existing Padding field might warrant that we just add a zero length padding field from the get go?
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wanna hear other people's thoughts before I update anything though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
This commit adds a helper function called `padHopInfo` along with a test for it. This function will be used later on when building a blinded path. It is used to ensure that all encrypted blobs of a blinded path that we construct are padded to the same size.
This commit adds a function that can be used to compute the accumulated path policy for a blinded path as defined in the spec: https://github.com/lightning/bolts/blob/db278ab9b2baa0b30cfe79fb3de39280595938d3/04-onion-routing.md?plain=1#L255
This commit adds a helper function that will be used to adjust a hops policy values by certain given increase and decrease multipliers. This will be used in blinded paths to give policy values some buffer to avoid easy probing of blinded paths.
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. 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.
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.
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.
e92e850
to
d537c35
Compare
cc: @calvinrzachman for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work 🔥! Really cool to see this working end-to-end 🙏. I need to improve my understanding of the hop expiries a bit more and also check spec compliance, but the PR looks great on first pass!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one could save the encodings in paymentPath
already in the previous iterations?
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
totalFeeBase = calcNextTotalBaseFee( | ||
totalFeeBase, info.BaseFee, info.FeeRate, | ||
) | ||
|
||
totalFeeProp = calcNextTotalFeeRate(totalFeeProp, info.FeeRate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice implementation :)
func addPolicyBuffer(policy *models.ChannelEdgePolicy, | ||
incMultiplier, decMultiplier float64) (*bufferedChanPolicy, error) { | ||
|
||
if incMultiplier <= 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be larger or equal 1
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if i.BlindedPaths.PolicyIncreaseMultiplier <= 0 { | ||
return fmt.Errorf("the blinded route policy increase " + | ||
"multiplier must be greater than 1") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be 0
-> 1
as in the error message
; node as the introduction node. | ||
; invoices.blinding.min-num-hops=2 | ||
|
||
; The maximum number of blinded paths to select. This doesn't include our node, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mentions paths instead of hops
; The amount by which to increase certain policy values of hops on a blinded | ||
; path in order to add a probing buffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could perhaps need a more detailed description explaining the tradeoffs
|
||
// PathID returns the path ID encoded in the payload of a blinded | ||
// payment. | ||
PathID() *chainhash.Hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One could re-use MultiPath
for PathID
and total sat, but it's probably cleaner this way, right?
// createBlindedRoute creates a blinded route to the recipient node provided. | ||
// The set of hops is expected to start at the introduction node and end at | ||
// the recipient. | ||
func (b *blindedForwardTest) createBlindedRoute(hops []*forwardingEdge, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great to see those deletions, nice compact and clean tests 🎉!
Tracking Issue: #6690
Depends on #8752
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.
Lower level overview: