-
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
lnrpc+routing: remove k shortest path finding #3054
lnrpc+routing: remove k shortest path finding #3054
Conversation
5cb7d4b
to
b6c2364
Compare
b6c2364
to
7a5bd29
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.
Impossible not to like :)
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 - only minor thing is that the RPC is still named QueryRoutes
.
I didn't think about that, but changing that would break all clients, even the ones that only request a single route already now. Not renaming it requires users that rely on multiple routes to notice this change, because their application may silently behave differently. |
Could you add some information for those users who need more than a single route? |
@C-Otto you should be able to implement this client side by using the ignore parameters of queryroutes. |
@joostjager Yes, I learned about the Router RPC calls that give the information I need. Attempt a payment, figure out why it fails, ignore the culprit node/edge, try again. A bit cumbersome, but it might work. See the discussion/progress in C-Otto/rebalance-lnd#51 and #3206 in case you're interested. |
This PR is the follow up of #2497. After deprecating the
num_routes
parameter inlnd
version 0.6, the parameter and all k-shortest related code can now be removed.