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: add queryprob rpc #3556

Merged
merged 5 commits into from Oct 31, 2019
Merged

Conversation

@joostjager
Copy link
Collaborator

joostjager commented Sep 30, 2019

In #3462, we removed probability reporting for querymc. This PR brings it back as a separate rpc for which a specific amount needs to be given.

In addition to the probability, it also returns the raw data for the requested pair. An example use case for this is to get a sense for the freshness of a pair and use this to decide whether to do a prepay probe for a route.

@joostjager joostjager requested review from cfromknecht and Roasbeef as code owners Sep 30, 2019
@joostjager joostjager removed request for Roasbeef and cfromknecht Sep 30, 2019
@joostjager joostjager force-pushed the joostjager:query-mc-prob branch 2 times, most recently from b1e9859 to 9ec1007 Sep 30, 2019
@joostjager joostjager requested a review from halseth Sep 30, 2019
@joostjager joostjager force-pushed the joostjager:query-mc-prob branch 4 times, most recently from 2ea9411 to 8644ceb Oct 23, 2019
@joostjager joostjager requested a review from carlaKC Oct 24, 2019
@joostjager joostjager self-assigned this Oct 24, 2019
@joostjager joostjager added the v0.9.0 label Oct 24, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Oct 24, 2019
@joostjager joostjager added this to the 0.9.0 milestone Oct 24, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Oct 24, 2019
Copy link
Collaborator

carlaKC left a comment

LGTM, a few small nits about naming but overall a pretty straightforward.

cmd/lncli/cmd_query_mc_probability.go Outdated Show resolved Hide resolved
cmd/lncli/cmd_query_mc_probability.go Outdated Show resolved Hide resolved
@@ -125,7 +125,24 @@ type timedPairResult struct {
// timestamp is the time when this result was obtained.
timestamp time.Time

pairResult

This comment has been minimized.

Copy link
@halseth

halseth Oct 29, 2019

Collaborator

now that these field are replicated both here and in pairResult, can this struct type be removed now?

}

if !data.Timestamp.IsZero() {
rpcData.Timestamp = data.Timestamp.Unix()

This comment has been minimized.

Copy link
@halseth

halseth Oct 29, 2019

Collaborator

why don't do Unix() also when zero?

This comment has been minimized.

Copy link
@joostjager

joostjager Oct 29, 2019

Author Collaborator

It is a very negative number. The beginning of time.Time is far before 1970.

This comment has been minimized.

Copy link
@halseth

halseth Oct 29, 2019

Collaborator

ouch, TIL

cmd/lncli/cmd_query_mc_probability.go Outdated Show resolved Hide resolved
double probability = 1 [json_name = "probability"];

/// The historical data for the requested pair.
PairData history = 2 [json_name = "history"];

This comment has been minimized.

Copy link
@halseth

halseth Oct 29, 2019

Collaborator

is the reason for PairData to reuse it here?

This comment has been minimized.

Copy link
@joostjager

joostjager Oct 29, 2019

Author Collaborator

yes

@joostjager joostjager force-pushed the joostjager:query-mc-prob branch from 8644ceb to 73eba92 Oct 29, 2019
joostjager added 5 commits Sep 26, 2019
This prepares for decoupling the result interpretation of a single
payment attempt from the information stored in mission control memory
on the history of a node pair. A planned follow-up where we store both
the last success and last failure requires this decoupling.
Prepares for this data structure being accessed in mission control rpc
implementations.
With a separate proto message, it becomes possible to also return the
pair data for a single pair. This prepares for the new mc probability
querying rpc.
Probabilities are no longer returned for querymc calls. To still provide
some insight into the mission control internals, this commit adds a new
rpc that calculates a success probability estimate for a specific node
pair and amount.
@joostjager joostjager requested a review from halseth Oct 29, 2019
@joostjager joostjager force-pushed the joostjager:query-mc-prob branch from 73eba92 to fb57255 Oct 29, 2019
Copy link
Collaborator

halseth left a comment

Pretty straightforward diff, only exposing state that already exists within MC. LGTM 🎅

@joostjager joostjager moved this from Needs Review to Approved in v0.9.0-beta Oct 29, 2019
@joostjager joostjager moved this from Approved to Needs Review in v0.9.0-beta Oct 29, 2019
@joostjager joostjager moved this from Needs Review to Approved in v0.9.0-beta Oct 29, 2019
@joostjager joostjager changed the title routerrpc: add querymcprob rpc routerrpc: add queryprob rpc Oct 29, 2019
Copy link
Collaborator

carlaKC left a comment

Looks good, adding approval post new changes being pushed to see whether my grey tick changes, but I don't think my review will count towards approvals: "At least 2 approving reviews are required by reviewers with write access".

@joostjager joostjager merged commit fcf81ed into lightningnetwork:master Oct 31, 2019
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.03%) to 62.975%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
v0.9.0-beta automation moved this from Approved to Done Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
4 participants
You can’t perform that action at this time.