Skip to content
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

routerrpc: report route success probability #3449

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@joostjager
Copy link
Collaborator

commented Sep 3, 2019

This PR adds a new success_prob field to the response of QueryRoutes. The intention of adding this field is to make probability based path finding more transparent and help understand why a route is returned.

@joostjager joostjager force-pushed the joostjager:route-success-prob branch from 122e8e2 to 956943d Sep 3, 2019
@joostjager joostjager requested review from halseth and wpaulino Sep 3, 2019
@halseth
halseth approved these changes Sep 3, 2019
Copy link
Collaborator

left a comment

LGTM 👍

lnrpc/routerrpc/router_backend.go Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:route-success-prob branch from 956943d to bc3a217 Sep 3, 2019

/**
The success probability of the returned route based on the current mission
control state. [EXPERIMENTAL]

This comment has been minimized.

Copy link
@wpaulino

wpaulino Sep 4, 2019

Collaborator

Are we fine with adding an experimental field to the main RPC server?

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 4, 2019

Author Collaborator

I think we need to introduce something like this. At some point, the main rpc server will be gone and how to indicate experimental fields then? I also don't think we want to make copies of rpc calls in sub servers just to be able to add a single experimental field. There are more experimental fields coming up. For invoice htlcs in #3390 and channel metrics in #3332.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Sep 4, 2019

Collaborator

What's the rationale for having this field be experimental then? We should also come up with a standard for marking these as experimental since #3332 has a different format.

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 4, 2019

Author Collaborator

It is not very critical data, mainly meant for debugging. If we mark it as experimental, we can change the field without deprecating it in a major release first. It is primarily signaling what users can expect of this field in terms of api stability.

But this is all open for discussion, just as the format is.

I also looked to see if we could add something more structured like you can with deprecated, but it doesn't seem to be easy in proto.

lnrpc/rpc.proto Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Show resolved Hide resolved
@wpaulino wpaulino added this to the 0.8.0 milestone Sep 4, 2019
@joostjager joostjager force-pushed the joostjager:route-success-prob branch from bc3a217 to ebfb05c Sep 4, 2019
@joostjager joostjager requested a review from wpaulino Sep 4, 2019
@wpaulino wpaulino removed their request for review Sep 4, 2019
Copy link
Collaborator

left a comment

LGTM 💾

The intention of adding this field is to make probability based path
finding more transparent and help understand why a route is returned.
@joostjager joostjager force-pushed the joostjager:route-success-prob branch from ebfb05c to 99a920f Sep 5, 2019
@joostjager joostjager merged commit fb565bc into lightningnetwork:master Sep 5, 2019
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.02%) to 61.155%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.