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

routing: track amt ranges in mission control #3493

Merged
merged 2 commits into from Nov 22, 2019

Conversation

joostjager
Copy link
Collaborator

@joostjager joostjager commented Sep 12, 2019

Previously we tracked the last result for a node connection only. This could lead to the situation where a meaningful observation would be overwritten by a far less meaningful one. For example: a failure for 100.000 sats is overwritten by a success for 100 sats.

Besides better route selection in general, this also helps with building the bos node score list.

Probing with minimal amounts in particular will likely wipe out useful history for more representative amounts.

This PR changes mission control to track both that the success and the last failure. This creates three amount ranges for which probabilities are calculated differently:

  • <=successAmt: return a high probability (default 0.95)
  • successAmt+1 to failAmt-1: in this range, we don't have much information. Return the a priori (default 0.6)
  • >=failAmt: penalize the connection and decay over time back to the a priori

@joostjager joostjager added incomplete blocked labels Sep 12, 2019
@joostjager joostjager force-pushed the small-successes branch 6 times, most recently from 883f699 to 6846bc7 Compare Sep 26, 2019
@joostjager joostjager changed the title routing: track amt ranges in mission control [don't review] routing: track amt ranges in mission control Sep 26, 2019
@joostjager joostjager force-pushed the small-successes branch 2 times, most recently from fca612e to a39b10d Compare Sep 27, 2019
@joostjager joostjager requested a review from halseth Sep 27, 2019
@joostjager joostjager force-pushed the small-successes branch 2 times, most recently from d138b83 to 33c1eaf Compare Sep 30, 2019
lnrpc/routerrpc/router.proto Outdated Show resolved Hide resolved
lnrpc/routerrpc/router.proto Show resolved Hide resolved
lnrpc/routerrpc/router_server.go Show resolved Hide resolved
case result.Success:
// Weigh success with a constant high weight of 1. There
// is no decay.
if result.SuccessAmt != 0 && amt <= result.SuccessAmt {
Copy link
Collaborator

@halseth halseth Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't SuccessTIme be used here,, similar to how FailTIme.IsZero is checked below?

Copy link
Collaborator Author

@joostjager joostjager Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition can actually be removed. If there is no success recorded (or pushed down by an amt independent failure), successAmt will be zero and amt is always greater than zero.

routing/missioncontrol.go Show resolved Hide resolved
routing/missioncontrol.go Show resolved Hide resolved
@joostjager joostjager force-pushed the small-successes branch 3 times, most recently from 3532f2c to 370c5fc Compare Sep 30, 2019
@joostjager joostjager requested a review from halseth Sep 30, 2019
@joostjager joostjager removed the incomplete label Sep 30, 2019
@joostjager joostjager added this to the 0.9 milestone Oct 8, 2019
@Roasbeef Roasbeef requested a review from carlaKC Oct 23, 2019
@Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Oct 23, 2019

Can be rebased now that the dependent PR is merged.

@joostjager joostjager added this to TODO in v0.9.0-beta Oct 23, 2019
@joostjager joostjager moved this from TODO to WIP in v0.9.0-beta Oct 23, 2019
@joostjager joostjager self-assigned this Oct 23, 2019
// applied based on this result. Only applies to fail results.
MinPenalizeAmt lnwire.MilliSatoshi
// Amt is the amount that was (attempted to be) forwarded in the last
// payment attempt.
Copy link
Collaborator

@carlaKC carlaKC Oct 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the amount that most recently failed, as in attempted to be forwarded= failed? Not clear from the comment what this represents, but I'm lacking a bit of context on this one.

Copy link
Collaborator Author

@joostjager joostjager Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean with 'attempted to be forwarded= failed'. But this is (usually) the amount of the last failure.

Comments fixed to explain this better.

Copy link
Collaborator Author

@joostjager joostjager Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now seeing that this refers to the first commit. Removed the "(attempted to be)" part. It may not be that helpful, because there is also a success boolean in this struct.

result.timeReply,
pairResult,
),
pair.From, pair.To, result.timeReply, &pairResult,
Copy link
Collaborator

@carlaKC carlaKC Oct 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: does this need a newline?

Copy link
Collaborator Author

@joostjager joostjager Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which newline exactly do you mean?

Copy link
Collaborator

@carlaKC carlaKC Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the params can fit on one line within 80 char limit
m.setLastPairResult(pair.From, pair.To, result.timeReply, &pairResult)

Copy link
Collaborator Author

@joostjager joostjager Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also with our standard tab size 8?

// prevents the success range from shrinking when there is no
// reason to do so. For example: small amount probes shouldn't
// affect a previous success for a much larger amount.
if successAmt > current.SuccessAmt {
Copy link
Collaborator

@carlaKC carlaKC Oct 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an argument that scaling down the success amount makes sense, since larger success amounts may deplete the outgoing capacity on a channel? As is, a single large payment "vouches" for all amounts below it succeeding, even though we may not necessarily be seeing payments of this magnitude.

Copy link
Collaborator Author

@joostjager joostjager Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo we should stay away from that kind of inference. There may be private channels that shadow a single public channel, which interferes with assumptions of that kind.

@joostjager joostjager added mission control payments routing and removed blocked labels Oct 31, 2019
@joostjager joostjager force-pushed the small-successes branch 2 times, most recently from b63db30 to f897cd5 Compare Nov 19, 2019
@joostjager joostjager requested a review from carlaKC Nov 19, 2019
@joostjager joostjager force-pushed the small-successes branch 2 times, most recently from 617d59b to 6f76da1 Compare Nov 19, 2019
@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Nov 19, 2019
@joostjager joostjager requested a review from carlaKC Nov 20, 2019
Copy link
Collaborator

@carlaKC carlaKC left a comment

Happy with the logic as is, but I think it's worth adding a comment on TimedPairResult's SuccessTime to explicitly indicate that it is not necessarily matched with the SuccessAmount.

Copy link
Collaborator

@halseth halseth left a comment

Mostly some questions and nits, nothing really blocking. Agree that adding some comments to TimedPairResult would be useful. Good PR :)

lnrpc/routerrpc/router.proto Outdated Show resolved Hide resolved
routing/result_interpretation.go Outdated Show resolved Hide resolved
routing/missioncontrol.go Outdated Show resolved Hide resolved
routing/missioncontrol.go Outdated Show resolved Hide resolved
routing/missioncontrol.go Show resolved Hide resolved
lnrpc/routerrpc/router.proto Show resolved Hide resolved
routing/probability_estimator.go Outdated Show resolved Hide resolved
routing/probability_estimator.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the small-successes branch 2 times, most recently from 9212050 to e4b5b16 Compare Nov 21, 2019
@joostjager joostjager requested a review from halseth Nov 21, 2019
Copy link
Collaborator

@halseth halseth left a comment

LGTM 💯

v0.9.0-beta automation moved this from Needs Review to Approved Nov 21, 2019
@joostjager joostjager merged commit 43d2e75 into lightningnetwork:master Nov 22, 2019
1 of 2 checks passed
v0.9.0-beta automation moved this from Approved to Done Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants