-
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
[1/4] Route Blinding Receives: Groundwork #8752
[1/4] Route Blinding Receives: Groundwork #8752
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 Configration File (
|
485b765
to
451bda7
Compare
e92e850
to
d537c35
Compare
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 that you managed to do end-to-end payments to blinded routes using Bolt 11, very cool work and a big achievement 🎉! I have looked a bit ahead how everything fits together in the next PRs, but will study them more in depth.
d537c35
to
04d97a4
Compare
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.
Thanks for the review @bitromortac 🙏
Updated & responded
85a1b2a
to
aded983
Compare
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.
Really cool change to allow for Blinded Paths in Bolt11 invoices, this PR looks really good ⭐️, will approve as soon as I reviewed the second one to gain more context.
zpay32/blinded_path.go
Outdated
// EncodeBlindedHop writes the passed BlindedHopInfo to the given writer. | ||
// | ||
// 1) Blinded node pub key: 33 bytes | ||
// 2) Cipher text length: 2 bytes |
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.
isn't the length of the cypertext a bigsize type, at least that's also what is used below.
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.
at least that's also what is used below.
Not sure i see this?
The code below uses a fixed 2 byte length. We can change the encoding to bigsize if you think that would be better 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.
Correct, hmm so yeah I was thinking of bigsize type because the spec says so or ?
type: 10 (encrypted_recipient_data)
data:
[...*byte:encrypted_data]
but maybe I am misunderstanding ?
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.
cool yeah I can update it to that. Just went for the "let's force it to be small" approach since bigsize will have some overhead but I think you are right that it would be better. Will update
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.
cool - updated to use bigSize
zpay32/blinded_path.go
Outdated
} | ||
|
||
numHops := len(p.Hops) | ||
if numHops > math.MaxUint8 { |
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.
Q: Is the limit of the number of hops als the overall onion package size (1300 bytes) ?
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.
yes definitely - the question is: should we do that check here? also - if we do, it probably makes sense to force it to be a number way less than 1300 right cause that 1300 still needs to include other hop payloads. So im wondering how we can determine a good magic number for this... perhaps just checking < 1300 is fine for now?
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.
Good point, hmm do we validate the consistency of data in other Encode/Decode methods ? Probably overkill to add the specific check here, maybe keep it as is and and the reason in the comment. Open for both ways.
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.
Ok given the maximum number of hops determined calculated in the bLIP (7), i've updated the check here to be more strict.
aded983
to
55ed46c
Compare
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.
thanks for the review @ziggie1984! 🙏
zpay32/blinded_path.go
Outdated
// EncodeBlindedHop writes the passed BlindedHopInfo to the given writer. | ||
// | ||
// 1) Blinded node pub key: 33 bytes | ||
// 2) Cipher text length: 2 bytes |
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.
at least that's also what is used below.
Not sure i see this?
The code below uses a fixed 2 byte length. We can change the encoding to bigsize if you think that would be better though?
zpay32/blinded_path.go
Outdated
} | ||
|
||
numHops := len(p.Hops) | ||
if numHops > math.MaxUint8 { |
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.
yes definitely - the question is: should we do that check here? also - if we do, it probably makes sense to force it to be a number way less than 1300 right cause that 1300 still needs to include other hop payloads. So im wondering how we can determine a good magic number for this... perhaps just checking < 1300 is fine for now?
@bitromortac: review reminder |
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.
Had some final comments, I think we are almost good to go :)
// include this final delta in their aggregate delta. A | ||
// sender-set delta may be added to account for block arrival | ||
// during payment, but we do not set it in this test. | ||
// Create final hop parameters for payment amount = 110. |
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.
Q: So we will not allow an additional block arrival padding for the blinded path case ?
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.
ah good point! - I think I was sort of assuming that the recipient would add this buffer to the communicated blinded path cltv delta but after looking again at the spec and the proposal doc - it looks like you are right and the sender still sets this. Will update!
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.
ah, although... check this reply from t-bast: https://github.com/lightning/bolts/pull/798/files#r1059339014
In it he mentions that it is potentially no longer necessary for the sender to account for this buffer since the receiver can take care of it with dummy hops (obvs then we need to make sure to actually add these dummy hops).
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've opened an issue on the spec repo to get some general clarification around CLTV delta's and blinded paths:
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.
that seems to be an ok approach, to let the receiver handle this, since it is already tasked with building routes that are specially selected
zpay32/blinded_path.go
Outdated
} | ||
|
||
numHops := len(p.Hops) | ||
if numHops > math.MaxUint8 { |
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.
Good point, hmm do we validate the consistency of data in other Encode/Decode methods ? Probably overkill to add the specific check here, maybe keep it as is and and the reason in the comment. Open for both ways.
zpay32/blinded_path.go
Outdated
// EncodeBlindedHop writes the passed BlindedHopInfo to the given writer. | ||
// | ||
// 1) Blinded node pub key: 33 bytes | ||
// 2) Cipher text length: 2 bytes |
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.
Correct, hmm so yeah I was thinking of bigsize type because the spec says so or ?
type: 10 (encrypted_recipient_data)
data:
[...*byte:encrypted_data]
but maybe I am misunderstanding ?
}) | ||
} | ||
|
||
introNode := path.Hops[0].BlindedNodePub |
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.
Isn't the introduction point a unblinded pubkey in the lnrpc.BlindedPath
struct, I wonder why we add the blinded nodekey of the first hope ?
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.
yeah so this is cause the first hop's key will be the real node key. Did this for space saving in the invoice since we never need the intro node's blinded pub key as the sender.
but can update depending on how that discussion goes :)
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.
thanks for the review @ziggie1984 !
will start on updates asap
// include this final delta in their aggregate delta. A | ||
// sender-set delta may be added to account for block arrival | ||
// during payment, but we do not set it in this test. | ||
// Create final hop parameters for payment amount = 110. |
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.
ah good point! - I think I was sort of assuming that the recipient would add this buffer to the communicated blinded path cltv delta but after looking again at the spec and the proposal doc - it looks like you are right and the sender still sets this. Will update!
zpay32/blinded_path.go
Outdated
// EncodeBlindedHop writes the passed BlindedHopInfo to the given writer. | ||
// | ||
// 1) Blinded node pub key: 33 bytes | ||
// 2) Cipher text length: 2 bytes |
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.
cool yeah I can update it to that. Just went for the "let's force it to be small" approach since bigsize will have some overhead but I think you are right that it would be better. Will update
}) | ||
} | ||
|
||
introNode := path.Hops[0].BlindedNodePub |
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.
yeah so this is cause the first hop's key will be the real node key. Did this for space saving in the invoice since we never need the intro node's blinded pub key as the sender.
but can update depending on how that discussion goes :)
Adding a write-up here in the hopes that it will provide some clarity to reviewers of what my current understanding of things is: please let me know if i'm missing something 🙏 cc @bitromortac & @ziggie1984 CLTVs, CLTV-deltas and invoice expiry in a paymentNormal LN payment:
For the other hops:
Route Blinding:
Notes to self and to reviewers:
See progress on:NOTE: has been edited (01/07/2024) |
Thanks a lot for the writeup and thorough analysis @ellemouton! some small corrections for iv):
Concerning the points:
|
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, this looks ready 🎉 (there are still some discussions around the encoding, but I like the current state of it). I have a suggestion concerning the padding.
// include this final delta in their aggregate delta. A | ||
// sender-set delta may be added to account for block arrival | ||
// during payment, but we do not set it in this test. | ||
// Create final hop parameters for payment amount = 110. |
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.
that seems to be an ok approach, to let the receiver handle this, since it is already tasked with building routes that are specially selected
55ed46c
to
6307874
Compare
Thank you for the explanation, I think I now understood the whole ctlv expiry for blinded paths.
But all those changes will only affect the follow up PRs regarding the CLTV Delta (I think PR 2) |
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.
LGTM 🔥
6307874
to
843260f
Compare
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.
Thanks @ziggie1984 🙏
Ready for a final look @bitromortac 🙏
This commit is purely a refactor. In it, we let the `BlindedEdge` struct carry a pointer to the `BlindedPayment` that it was derived from. This is done now because later on in the PR series, we will need more information about the `BlindedPayment` that an edge was derived from. Since we now pass in the whole BlindedPayment, we swap out the `cipherText` member for a `hopIndex` member so that we dont carry around two sources of truth in the same struct.
Expand the AdditionalEdges interface with a BlindedPayment method. In upcoming commits, we will want to know if an AdditionalEdge was derived from a blinded payment or not and we will also need some information from the blinded payment it was derived from. So we expand the interface here to avoid needing to do type casts later on. The new method may return nil if the edge was not derived from a blinded payment.
Add a constructor for unified edge. In upcoming commits, we will add a new member to unifiedEdge and a constructor forces us to not forget to populate a required member.
Later on in this series, we will need to know during path finding if an edge we are traversing was derived from a blinded payment path. In preparation for that, we add a BlindedPayment member to the `unifiedEdge` struct. The reason we will need this later on is because: In the case where we receive multiple blinded paths from the receipient, we will first swap out the final hop node of each path with a single unified target node so that path finding can work as normal. Once we have selected a route though, we will want to know which path an edge belongs to so that we can swap the correct destination node back in.
For the final hop in a blinded route, the SCID and RelayInfo fields will _not_ be set. So these fields need to be converted to optional records. The existing BlindedRouteData constructor is also renamed to `NewNonFinalBlindedRouteData` in preparation for a `NewFinalBlindedRouteData` constructor which will be used to construct the blinded data for the final hop which will contain a much smaller set of data. The SCID and RelayInfo parameters of the constructor are left as non-pointers in order to force the caller to set them in the case that the constructor is called for non-final nodes. The other option would be to create a single constructor where all parameters are optional but I think this makes it easier for the caller to make a mistake.
Add the PathID (tlv type 6) field to BlindedRouteData. This will be used for the final hop of a blinded route. A new constructor is also added for BlindedRouteData which can specifically be used for the final hop.
When we start creating blinded paths to ourselves, we will want to be able to pad the data for each hop so that the `encrypted_recipient_data` for each hop is the same. We add a `PadBy` method that allows a caller to add a certain number of bytes to the padding field. Note that adding n bytes won't always mean that the encoded payload will increase by size n since there will be overhead for the type and lenght fields for the new TLV field. This will also be the case when the number of bytes added results in a BigSize bucket jump for TLV length field. The responsibility of ensuring that the final payloads are the same size is left to the caller who may need to call PadBy iteratively to achieve the goal. I decided to leave this to the caller since doing this at the actual TLV level will be quite intrusive & I think it is uneccessary to touch that code for this unique use case.
843260f
to
4bb85e5
Compare
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.
LGTM 🎉, I have a few questions left. Would we wait for all of the PRs to have ACKs to merge this?
Personally, I think we should just merge since this is not practically usable till the second PR is merged anyways. So we can always throw in small changes at the start of that PR if we realise we need to change something |
In this commit, the ability is added to encode blinded payment paths and add them to a Bolt 11 invoice.
This commit adds a blinded_paths field to the PayReq proto message. A new helper called `CreateRPCBlindedPayments` is then added to convert the zpay32 type to the existing `lnrpc.BlindedPaymentPath` type and add this to the `PayReq` in the `DecodePayReq` rpc method.
Only include the final hop's cltv delta in the total timelock calculation if the route does not include a blinded path. This is because in a blinded path, the final hops final cltv delta will be included in the blinded path's accumlated cltv delta value. With this commit, we remove the responsibility of remembering not to set the `finalHop.cltvDelta` from the caller of `newRoute`. The relevant test is updated accordingly.
4bb85e5
to
85ddffb
Compare
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.
LGTM 🍩
func NewBlindedEdge(policy *models.CachedEdgePolicy, payment *BlindedPayment, | ||
hopIndex int) (*BlindedEdge, error) { | ||
|
||
if payment == nil { |
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.
👍 re defensive programming
// NewFinalHopBlindedRouteData creates the data that's provided for the final | ||
// hop in a blinded route. | ||
func NewFinalHopBlindedRouteData(constraints *PaymentConstraints, | ||
pathID []byte) *BlindedRouteData { |
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.
Non-blocking here, but we could also consider making them to individual types further encode the distinction in the types themselves.
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.
yeah good idea 👍 that would make things much nicer and easier to reason about
// 6) Encoded BlindedHops. | ||
func (p *BlindedPaymentPath) Encode(w io.Writer) error { | ||
var relayInfo [26]byte | ||
binary.BigEndian.PutUint32(relayInfo[:4], p.FeeBaseMsat) |
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.
Non-blocking, you can also use binary.Write
to avoid having to do the slicing in the buffer to place things properly: https://pkg.go.dev/encoding/binary#Write
Can make things easier to review, and also you don't need to worry about the alignment arithmetic.
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.
cool yeah good idea - will add a commit in the follow up PR 👍
// | ||
// NOTE: This is optional and should not be set at the same time as | ||
// RouteHints. | ||
BlindedPaymentPaths []*BlindedPaymentPath |
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.
😎
@@ -355,6 +376,13 @@ func validateInvoice(invoice *Invoice) error { | |||
return fmt.Errorf("no payment hash found") | |||
} | |||
|
|||
if len(invoice.RouteHints) != 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.
👍
To make review a bit more manageable, I've split out some of the groundwork needed for
the main route blinding receives PR. This PR thus has no functional changes.
Tracking Issue: #6690
PR Overview:
BlindedPayment
associated with an edge in path finding later on.BlindedRouteData
that will be required for receives. Namely, PathID and Padding.EDIT: bLIP proposal here: lightning/blips#39