(routing+lnrpc): arbitrary route source + route transformation#9153
(routing+lnrpc): arbitrary route source + route transformation#9153calvinrzachman wants to merge 3 commits intolightningnetwork:elle-base-branch-payment-servicefrom
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
calvinrzachman
force-pushed
the
route-manipulation
branch
2 times, most recently
from
ad747b3 to
0dd29b2
Compare
High level thoughtsthanks for the detailed outline. I guess I had initially thought we could have a new very abstract "virtual channel" type that the remote channel router could open with each of the LND nodes. Each LND node can be given an allow-list of who to accept such a channel from. If that abstraction could be possible, then the underlying method of sending should be able work as is today right? Ie channel router just builds the routes and sends directly to switch which uses UpdateAddHTLC to communicate the send. Then the LND node will see the incoming UpdateAddHTLC from an upstream virtual channel. Anyways - that is just what I thought the plan was. But perhaps getting channels to be that abstract is a bit of a tall order. Re the PR description
I think I lost you a bit at this point. are you saying that the remove router will call SendPayment on the LND nodes? I thought a new SendOnion endpoint was the idea? Or are you saying that the SendPayment is called on the channelrouter? If the former, then im not sure why the real LND node needs to transform the route? Yeah I think I lost you after this point. It would help to have a step by step flow (ie, what is calling what when and what is doing what transformation when etc etc). |
|
|
||
| // A transformation which will be applied to every route created for | ||
| // this payment session. | ||
| routeTransform RouteTransformFunc |
There was a problem hiding this comment.
can use fn.Option instead so that we dont have to remember to do a nil check
There was a problem hiding this comment.
Updated routeTransform to fn.Option[RouteTransformFunc] on both sessionOptions and SessionSource ✅
| // option is a functional option for configuring the payment session. | ||
| type option func(*paymentSession) | ||
|
|
||
| // withRouteTransform sets a route transformation function. | ||
| func withRouteTransform(routeTransform RouteTransformFunc) option { | ||
| return func(ps *paymentSession) { | ||
| ps.routeTransform = routeTransform | ||
| } |
There was a problem hiding this comment.
best practice is to have a separate paySessOptions struct with the specific things we all a caller to "edit".
There was a problem hiding this comment.
so I'd do:
// paySessOptions holds ...
type paySessOptions struct {
// routeTransform is ...
routeTransform fn.Option[RouteTransformFunc]
}
// newDefaultPaySessOptions constructs...
func newDefaultPaySessOptions() *paySessOptions {
return &paySessOptions{}
}
// paySessOption is a functional option for configuring the payment session.
type paySessOption func(*paySessOptions)
// withRouteTransform sets a route transformation function.
func withRouteTransform(routeTransform RouteTransformFunc) paySessOption {
return func(ps *paySessOptions) {
ps.routeTransform = fn.Some(routeTransform)
}
}
...
type paymentSession struct {
opts *paySessOptions
...
...
options := newDefaultPaySessOptions()
for _, opt := range opts {
opt(options)
}
return &paymentSession{
opts: options,
...
There was a problem hiding this comment.
added sessionOptions struct with defaultSessionOptions() constructor and withRouteTransform as a functional option.
| } | ||
| } | ||
|
|
||
| func TestRouteTransformFunc(t *testing.T) { |
There was a problem hiding this comment.
added a doc in here.
| // log is a payment session-specific logger. | ||
| log btclog.Logger | ||
|
|
||
| // A transformation which will be applied to every route created for |
There was a problem hiding this comment.
comment should start with name of variable
| if m.RouteTransform != nil { | ||
| options = append(options, withRouteTransform(m.RouteTransform)) | ||
| } |
There was a problem hiding this comment.
var options []paySessOption
m.RouteTransform.WhenSome(func(fn RouteTransformFunc) {
options = append(options, withRouteTransform(fn))
})
| } | ||
|
|
||
| // RouteTransformFunc defines a function type for transforming a route. | ||
| type RouteTransformFunc func(route *route.Route) *route.Route |
There was a problem hiding this comment.
do we definitely want the flexibility to transform the entire route? It's a bit of a red flag to me cause we are depending on this module to construct a route but then we also as a caller are able to just replace that entire route... so can we not restrict the options a bit? Like WithPeeledFirstNode and things like that?
| The source node from whose perspective the route was built. If empty, | ||
| self is assumed. | ||
| */ | ||
| string source_pub_key = 9; |
| } | ||
| } | ||
|
|
||
| prevNodePubKey := r.SelfNode |
There was a problem hiding this comment.
prevNodePubKey is not necessarily selfnode anymore
There was a problem hiding this comment.
I think we should add tests for this change to make sure that things like this would be caught.
There was a problem hiding this comment.
gotta make sure that everywhere where r.SelfNode is used is still correct given this commit
|
|
||
| // GetSelfNode queries the current node identity public key used by the | ||
| // ChannelRouter. | ||
| func (r *ChannelRouter) GetSelfNode() route.Vertex { |
There was a problem hiding this comment.
can just call it SelfNode. Getter is implied (unless it clashes with a member in ChannelRouter (which I dont think it does))
|
|
||
| ## RPC Updates | ||
|
|
||
| * [Allow arbitrary source public key for BuildRoute RPC](https://github.com/lightningnetwork/lnd/pull/9153) so that lnd's _*ChannelRouter*_ and _*routerrpc.Server*_ can build routes from the perspective of different nodes. |
There was a problem hiding this comment.
wrap at 80 🙏
(while we are here, we can fix the wrapping of the entry above (for 9017) as well)
|
@calvinrzachman, remember to re-request review from reviewers when ready |
|
!lightninglabs-deploy mute |
calvinrzachman
changed the base branch from
master
to
elle-base-branch-payment-service
calvinrzachman
force-pushed
the
route-manipulation
branch
2 times, most recently
from
23ff296 to
f8cc659
Compare
| return route, err | ||
| // Apply the route transformation, if provided. | ||
| var transformErr error | ||
| p.opts.routeTransform.WhenSome( |
There was a problem hiding this comment.
I think the point brought up below (why can't the caller transform it themselves?) may still be relevant. Still getting through the diff.
There was a problem hiding this comment.
On second thought, the payment session has awareness of this as it's the one that's constructing the route in the first place. The caller typically makes the session, then exits (eg: one time payment send), so it would need some lower level hook somewhere in the process.
There was a problem hiding this comment.
Yeah, for the QueryRoutes/BuildRoute --> SendToRouteV2 flow where an external RPC client obtains the route from lnd there is ample opportunity for the client to manipulate the route however they desire. The routerrpc.SendPaymentV2 handles the path-finding, onion construction, dispatch, etc. all internally and so and external RPC client has no opportunity to manipulate the route. The lower level hook helps out with this. Though notably, I think this design (#10764) obviates the need for any route transform hooks. If it's deemed suitable - it also simplifies the the client side implementation no longer requiring the addition of additional edges on the source side to the underlying channel graph used by the router.
The RouterBackend assumes that the source public key for a route is always the node’s public key when calling UnmarshallRoute. Here we update to allow overriding of this field to any arbitrary public key.
The routerrpc.Server and ChannelRouter assume that the source public key for a route is always the node’s public key when calling BuildRoute. We update to allow overriding of this field to any arbitrary public key. The list of hop public keys must be connected to the source via channels in our graph view.
Update release notes for public facing RPC change.
calvinrzachman
force-pushed
the
route-manipulation
branch
from
f8cc659 to
30c3bb9
Compare
github-actions
Bot
added
the
severity-high
|
🟠 PR Severity: HIGH
🟠 High (8 files)
🟢 Low (1 file)
⚪ Excluded (4 files)
Analysis: This PR modifies lnrpc/* RPC/API definitions and routing/router.go, both HIGH severity. Changes include proto file additions (API surface changes), swagger updates, and backend/server implementation changes, indicating a new RPC feature. Only 9 non-excluded files and ~90 lines, so no severity bump. A knowledgeable engineer should review proto/API changes for backwards compatibility and routing logic for correctness. To override, add a |
6 participants
Change Description
BuildRouteandUnmarshallRouteassume the source public key for a route is always the node's own key. This makes it impossible to construct or deserialize routes from the perspective of a different node.This PR adds a
source_pub_keyfield to theRouteproto message and updatesBuildRouteto accept an optional source override. When set, the route is constructed from the perspective of the provided source node — thehop sequence must be connected to that source via channels in the graph. When unset, the node's own public key is used as before.
UnmarshallRouterespects the field so that routes round-trip correctly.This is useful independently for any tooling that needs to reason about routes from an arbitrary vantage point. It also pairs with the
RouteOrigingeneralization in #10764 which handles multi-source pathfinding for theSendPaymentV2flow.source_pub_keyfield toRouteproto message in bothlightning.protoandrouter.proto.UnmarshallRouteto use the provided source key when set, defaulting toSelfNodewhen empty.BuildRouteto accept a source node parameter instead of always usingSelfNode.UnmarshallRoutewith arbitrary source keys.