-
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
routing: add support for pegged hops in findPaths #2444
Conversation
@@ -781,14 +853,257 @@ func findPath(g *graphParams, r *restrictParams, | |||
// hops, then it's invalid. | |||
numEdges := len(pathEdges) | |||
if numEdges > HopLimit { | |||
return nil, newErr(ErrMaxHopsExceeded, "potential path has "+ | |||
"too many hops") | |||
if r.stopAtMaxHopsExceeded { |
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 this point is too late to check max hops, because the search has already finished. It would be better to already check HopLimit
when extending the bestNode
in the main loop above. So don't extend any paths that are already HopLimit
hops. That way, the algorithm will continue the search with more costly paths that may stay below the limit. Similar to how the fee limit is checked in processEdge
.
This is a pre-existing problem, but now that you want to solve it, I think it is better to do it without findKPaths
.
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.
Hi @joostjager. I think it is not possible to check the hop limit during dijkstra, unless you extend it a non trivial way. The problem is that you may already have dismissed the 'more costly fee' alternate path before reaching the node that is identified as going beyond the HopLimit. Please take a look for example at the added test scenario that checks the bug fix. I may be wrong and there is a clever way to do that. I opened a specific PR to address the HopLimit problem PR#2380. Since this PR also needs the same breakup of code I merged code of PR#2380 in here too. I suggest we discuss how to best address the HopLimit problem there so we can fix that bug as soon as possible. Thanks!
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, I know that Dijkstra is not returning the optimal solution for this problem. It finds the lowest cost path to every node without taking into account the hop limit. But cutting off too long paths and continuing the search at least increases the chance to find a solution, as opposed to just returning an error.
I can see that trying to fix the limitation by running k-shortest paths is another way to address it, but it isn't optimal either. So the 'bug' still isn't fixed. I think it is computationally more intensive too.
Also, running into the hop limit is not something that commonly happens.
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.
But cutting off too long paths and continuing the search at least increases the chance to find a solution, as opposed to just returning an error.
I'm not sure I follow. Are you saying that PR#2380 does not fix the bug? It does fix the simple scenario I added in TestNewRoutePathTooLong to check that the bug was fixed. I am not aware of a graph in which the fix will not find a valid path, if such exists, but I haven't proved mathematically that such a graph doesn't exists either. Let me know if you have such a graph in mind.
I can see that trying to fix the limitation by running k-shortest paths is another way to address it, but it isn't optimal either. So the 'bug' still isn't fixed. I think it is computationally more intensive too.
What do you mean by it isn't optimal? What do you mean the bug is not fixed?
Also, running into the hop limit is not something that commonly happens.
Exactly. That is why the additional findKpaths is run only if the shortest path found has too much hops. No additional computations were added to findPath and it keeps the Dijkstra computational complexity and code intact. The hop limits checks were added to the findKPaths stage, and not to the Dijkstra findPath stage.
I understand the fix may look strange. I do not have a better fix than that. Cutting too long paths in Dijkstra does not find valid paths in all graphs. If there is a better algorithm, I'll be more than happy to implement it, if my help would be useful.
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.
Moved the discussion to #2380
What is the real-life use case for pegging nodes? I can imagine that it is useful to control the outgoing channel from self in the context of "channel balancing while paying", but does it matter which intermediate nodes are used? |
9e50a32
to
3bdb18f
Compare
3bdb18f
to
8a72df0
Compare
0320dd8
to
b0a6704
Compare
Pegging of both nodes and channels is working. The error in the integration is in the data loss protection, and I believe is unrelated to this code. |
Add the ability to query for routes that pass through one or more pre-specified (pegged) hops.The resulting route is composed of segments connecting between the pegged nodes. Each segment does not have loops, but the entire path may. The route selection assures that no edge will be transversed twice at the same direction. The alternate routes are stitched from N-1 shortest segment paths and one alternate segment path.
b0a6704
to
b2a7a96
Compare
The list of pegged hops force route to pass through specified nodes and channels.
b2a7a96
to
20a519f
Compare
Since |
Closing this for now as we've gone with an alternative approach (edge white/black listing). |
The PR addresses #2282
Edited (08-02-2019):
From the CLI queryroutes description:
Below is the documentation from the unit test added for this feature.
Feedback appreciated.