routing: add RouteOrigin interface for multi-source pathfinding#10764
routing: add RouteOrigin interface for multi-source pathfinding#10764calvinrzachman wants to merge 3 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a flexible mechanism for multi-source pathfinding in lnd. By replacing the concrete source vertex with a RouteOrigin interface, the pathfinder can now efficiently determine the optimal route from a set of potential gateway nodes. This change is particularly beneficial for external payment controllers that manage multiple dispatch backends, allowing them to route payments through the most cost-effective gateway without requiring complex graph modifications. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
🟠 PR Severity: HIGH
🟠 High (4 files)
AnalysisAll changed files are in the Test files ( To override, add a |
There was a problem hiding this comment.
Code Review
This pull request refactors pathfinding to support multiple origins via a new RouteOrigin interface, allowing Dijkstra's algorithm to terminate at any node in a set. This change enables multi-backend payment services to find the cheapest path from several gateway nodes. A review comment correctly identifies a potential nil pointer dereference in findPath when accessing the distance map for the selected source, suggesting a more robust check for origin presence in the map.
|
@calvinrzachman, remember to re-request review from reviewers when ready |
There was a problem hiding this comment.
Should we return here only in case we have a single source? Otherwise we'd reject pathfinding attempts that would contain self among others. (I know that's not a mode you currently operate in).
There was a problem hiding this comment.
Yeah, don't think I'd this with how it's used currently but still a good catch. Gated both early returns on single-source mode per your fixup — multi-origin now skips the balance pre-check so the search can discover paths through other gateways even when self lacks balance.
There was a problem hiding this comment.
same here, maybe needs a single source check
There was a problem hiding this comment.
updated. same fix applied to both the errInsufficientBalance and errNoPathFound returns.
| // on first visit rather than terminating immediately. For | ||
| // multi-origin, the target may happen to be in the origin set | ||
| // but we still want a direct route from another origin. | ||
| _, isSingle := origin.(*singleOrigin) |
There was a problem hiding this comment.
I think this is kind of an antipattern, I'd prefer a function (closure) instead of an interface of type type RouteOrigin func(v route.Vertex) bool, that way we can use simple closures and would probably be more performant. To do that we'd need to add a routeToSelf bool.
There was a problem hiding this comment.
I had considered keeping the interface with something like an AllowCircular() method as an alternative way to avoid the type assertion, but the func type is probably simpler.
With routeToSelf extracted to an explicit bool there's no remaining reason for the interface. Switched to the func type as advised. Thanks for the example commits.
4c5132e to
673ee6a
Compare
6b942a7 to
cd7723d
Compare
942436e to
f77dc93
Compare
bitromortac
left a comment
There was a problem hiding this comment.
Thank you for the changes, I think the PR still needs some rebase work and commit messages need updates.
| // Implementations should be O(1). findPath calls RouteOrigin once | ||
| // per heap pop and once per edge relaxation, so any per-call cost | ||
| // directly contributes to path-finding latency. | ||
| type RouteOrigin func(v route.Vertex) bool |
There was a problem hiding this comment.
nit: rename to IsRouteOrigin to reflect the bool type.
There was a problem hiding this comment.
renamed to IsRouteOrigin ✅
| cfg *PathFindingConfig, self, source, target route.Vertex, | ||
| amt lnwire.MilliSatoshi, timePref float64, finalHtlcExpiry int32) ( | ||
| []*unifiedEdge, float64, error) | ||
| cfg *PathFindingConfig, self route.Vertex, origin RouteOrigin, |
| ), | ||
| } | ||
|
|
||
| ctx := newPathFindingTestContext(t, useCache, testChannels, "gw1") |
There was a problem hiding this comment.
maybe specify a realistic "self"?
There was a problem hiding this comment.
moved to use a separate node as self rather than a gateway.
| // When the target is also an origin (circular payment scenario), the | ||
| // pathfinder should still find a valid route. | ||
| targetIsOrigin := multiOrigin(map[route.Vertex]struct{}{ | ||
| gw1: {}, |
There was a problem hiding this comment.
I think this test is not really computing a circular route as we don't have another channel originating from target? and maybe isolate by removing gw1
There was a problem hiding this comment.
You're right, this isn't a circular route in the single-node sense. It's a cross-gateway rebalance (logical circular rebalance for a multi-node entityt): the proxy controls both gw1 and dest, and the pathfinder routes from gw1 rather than trying dest→dest. Updated the comment.
| }, | ||
| r, cfg, sourceNode.PubKeyBytes, source, target, amt, | ||
| r, cfg, sourceNode.PubKeyBytes, | ||
| func(v route.Vertex) bool { return v == sourceAlias }, target, amt, |
There was a problem hiding this comment.
Please do linter fixes already as you introduce changes, otherwise this adds review friction.
There was a problem hiding this comment.
apologies. reduced to fewer commits and this should be fixed now 🙏
| var routeEdges []*unifiedEdge | ||
| err = graph.GraphSession(ctx, func(graph graphdb.NodeTraverser) error { | ||
| route, _, err = findPath( | ||
| sourceAlias := source |
There was a problem hiding this comment.
the copy seems unnecessary?
There was a problem hiding this comment.
Removed. Was a workaround for a variable shadow that no longer exists.
| // balance available. | ||
| // balance available. This check is skipped when self is not in the | ||
| // origin set (e.g. multi-origin), since local balance information is | ||
| // not available for remote origin nodes. |
There was a problem hiding this comment.
Please put the doc change to the commit where behavior is changed, same below with routeToSelf
There was a problem hiding this comment.
should be fixed now with commit consolidation.
| gw1: {}, | ||
| target: {}, | ||
| }) | ||
| source, path, err = findPathWithOrigin( |
There was a problem hiding this comment.
Maybe it would be best to restructure the PR such that you do the prep work earlier, such as returning the source. Then you could introduce all the tests at once when you allow for multi-origins.
There was a problem hiding this comment.
Restructured things a bit. The first commit is a pure refactor returning the source vertex from findPath. Then IsRouteOrigin and multi-origin tests come in the second commit, followed up by the docs in the final commit.
findPath now returns the source vertex it settled on as its first return value, alongside the path and probability. For standard callers this is always the source they passed in. This prepares for a follow-up that generalizes the source parameter so the pathfinder can terminate at any vertex in a caller-provided set.
The findPath function previously accepted a concrete source vertex that the path-finding loop terminated at. This introduces an IsRouteOrigin func type and replaces the source parameter on findPath with it. Standard callers pass a simple vertex-equality closure and get identical behavior. IsRouteOrigin is the source-end counterpart to AdditionalEdge, which extends the graph at the destination end via route hints. After the path-finding loop, we verify the settled node is a valid origin before unraveling the forward path. When the heap empties without reaching an origin, we return errNoPathFound. A routeToSelf bool parameter restricts circular self-payments to single-origin callers. The local balance pre-check is gated on routeToSelf so multi-origin searches continue through other gateways when self lacks sufficient balance. SessionSource gains an optional Origin field threaded to the payment session via functional options. RouteRequest accepts an optional Origin field so FindRoute can use multi-origin, same as the payment session.
f77dc93 to
93d9fd5
Compare
|
One readability concern: In the early local balance checks, it is used more like "strict local/default origin mode": if total < amt {
if routeToSelf {
return errInsufficientBalance
}
}At that point it does not necessarily mean the target is self yet; it mostly means we are in the legacy/default single-origin path where local self liquidity is decisive. Later it is narrowed into the actual route-to-self meaning: routeToSelf = routeToSelf && isOrigin(target)and then used to allow/disallow the target cycle behavior. Would it be clearer to split these concepts or rename the incoming bool? For example, an input like |
ziggie1984
left a comment
There was a problem hiding this comment.
Looking good, only some minor questions
|
|
||
| // RouteOrigin determines where routes can originate from. The backward | ||
| // Dijkstra terminates when it reaches any origin vertex. This is the | ||
| // source-end counterpart to AdditionalEdge, which extends the graph at the |
There was a problem hiding this comment.
Does this allow also chaining of the source vertex or is only one entry allowed ? Having Gateyway1<=GatewayPrevious ...
There was a problem hiding this comment.
Don't think we have chaining here. IsRouteOrigin is a flat set of termination points, not a chain of edges. I think the analogy made in the PR description to route hints is helpful but not total. Route hints on the receive side add actual edges the pathfinder traverses (each hop becomes an onion layer). IsRouteOrigin just widens the termination condition of path finding - no edges added or extra hops.
There was a problem hiding this comment.
Each origin must be a node you operate and can dispatch HTLCs from (via SendOnion). The use case is an external controller that pathfinds centrally across multiple lnd backends (like PS): the predicate returns true for each gateway backend, and the pathfinder terminates at whichever one provides the cheapest path.
You could technically put any node in the origin set and the pathfinder would produce a valid graph path. But the route is also a payment instruction: the source node must dispatch an HTLC to the first hop. If you don't operate that node, you can't execute payment via the route.
| "balance: %v", amt, total) | ||
|
|
||
| return route.Vertex{}, nil, 0, errInsufficientBalance | ||
| if routeToSelf { |
There was a problem hiding this comment.
so the supplied optional origins could also include the source node and that's why we only abort if it is the legacy case only ?
Change Description
Path finding currently terminates at a single concrete source vertex, which assumes the node doing the routing is the node dispatching HTLCs. An external router controller that pathfinds centrally and dispatches from multiple lnd "gateway" backends needs routes that can originate from any of those backends.
This PR introduces a
RouteOrigininterface that generalizes where routes can originate from.RouteOriginreplaces the concrete source parameter with an interface method (IsOrigin(v) bool) so the path-finding algorithm (a modified version of Dijkstra running from target to source) can terminate at any vertex in a caller-provided set, naturally selecting whichever provides the cheapest path.This is the source-end counterpart to what
AdditionalEdgedoes at the destination end. Route hints inject extra edges near the target so the pathfinder can reach nodes the graph doesn't know about.RouteOriginworks similarly at the source: it's equivalent to adding a virtual super-sources*with zero-weight edges to each origin and running single-source Dijkstra.RouteOriginachieves the same result withouts*or extra edges in the graph by widening the termination check. Subpaths of shortest paths are shortest paths, so stripping the virtual edge leaves the optimal path from the selected origin. And the min-heap ensures the first origin popped has the smallest distance among all origins in the set (no cheaper one can remain in the queue).The default
singleOriginwraps a single vertex and produces identical behavior for all existing callers so should present no functional change for standard lnd.RouteOrigininterface andsingleOriginimplementation. Replace the concretesourceparameter onfindPathwithorigin RouteOrigin.findPathreturns the origin vertex it selected as its first return value.RequestRoutepasses this tonewRoutesoroute.SourcePubKeyis set correctly regardless of origin type.SessionSourcegains an optionalOriginfield, threaded to the payment session via functional options.RouteRequestaccepts an optionalOriginfield soFindRoute(used byQueryRoutes) can use multi-origin, same as the payment session and theSendPaymentV2flow.routeToSelfto single-origin callers. When a multi-origin set includes the target vertex, circular routing is not the intent. Rather we want a direct route from another origin.