-
Notifications
You must be signed in to change notification settings - Fork 379
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
ProbabilisticScorer optimizations #1375
ProbabilisticScorer optimizations #1375
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1375 +/- ##
==========================================
- Coverage 90.77% 90.75% -0.02%
==========================================
Files 73 73
Lines 41062 41091 +29
Branches 41062 41091 +29
==========================================
+ Hits 37272 37291 +19
- Misses 3790 3800 +10
Continue to review full report at Codecov.
|
e5874fe
to
e895c25
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.
LGTM. A few comments about docs and some minor nits that don't matter much.
e895c25
to
ab7f851
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.
LGTM post-squash.
ab7f851
to
5d3e90b
Compare
lightning/src/routing/scoring.rs
Outdated
/// | ||
/// The liquidity penalty is effectively limited to `2 * liquidity_penalty_multiplier_msat`. | ||
/// | ||
/// Default value: 10,000 msat |
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.
Note this default is probably much too high for the linear case - I don't think we should change it as long as the cost_function
default is log, but maybe we want to call out in the docs that it should be changed if the cost function is?
lightning/src/routing/scoring.rs
Outdated
/// | ||
/// Default value: 10,000 msat | ||
/// Default value: [`ProbabilisticScoringCostFunction::NegativeLogSuccessProbability`] |
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.
Do we want to consider changing this? I think log only makes sense if you're doing large payment volume or probing, which I don't think most of our users are?
5d3e90b
to
b4324c0
Compare
Okay, so thinking about the defaults here - lets assume that most channels are 100k-5m in capacity, and that our defaults should target users with 0 learning (or at least all info having decayed) cause they don't have much payment volume. Rene's work concluded that, by far, the failure probability of a hop is, roughly, Lets consider five possible settings -
Lets consider each setting at 3 hops, 5 hops, and 10 hops for values of 1k, 10k, 50k sats across 100k, and 1m channels. 1k-over-1m (0.1% failure rate)
10k-over-1m (1% failure rate, or 10k-over-1m)
50k-over-1m (5% failure probability)
10k-over-100k (10% failure probability)
50k-over-100k (50% failure probability)
It think its clear here that log is just completely absurdly low - it reserves half the range for the 90%-99% failure cases, which avoids large penalties through nearly all the data above. Even if we were to double the the penalties applied in log it'd still assign very low penalties, see eg the 10% per-hop failure table, where even doubling log would get us ~10 sats in penalty for a path that has a 65-80% failure probability. Log 6.25 has a similar issue, although to a smaller extent, with initial per-hop penalties much higher even at low failure rates. Linear also seems pretty broken, especially in the 10-hops case for lower probability, 0.2 sats for a 10-hop path is unlikely to work out well. Log 12.5 and linear 0.5-per-hop are both reasonable contenders, but I slightly prefer linear+0.5 here - the high penalties assigned in the 0.1% failure rate table for log 12.5 seem much too conservative even though we end up in almost the same place in the 10% and 50% tables. I propose we default to linear with a 0.5sat per-hop penalty |
I tried log w/ 0.5 sat per hop and a 50 sat multiplier, and it's pretty close to linear w/ 0.5 per-hop, especially at 1-10% failure probability. 1k-over-1m (0.1% failure rate)
10k-over-1m (1% failure rate, or 10k-over-1m)
50k-over-1m (5% failure probability)
10k-over-100k (10% failure probability)
50k-over-100k (50% failure probability)
|
First of all, I apologize, I had the wrong limit on the logs in the original script, so the 0.1% failure rate table is wrong. Its corrected below (and in the pasted script on the issue). Hmm, I realize the parameter we're missing in all of this is the payment amount - is 150 sats penalty too much? If you're sending a million sats, probably not, if you're sending 50k sats, probably. I don't think we need to address that there, but in whatever followup we do to pass the scorer more information we should also pass the payment amount and give the As for the specific direction to go in here, I added five more options below (swapped the multipliers in the logs with additional probability of failure and added a 30x/40x variant of your proposal). The 40x variant of your proposal is basically the same as the linear+0.5 proposal for all the numbers that matter (<=10%), and I have to admit I prefer it over 30/50x in the 5% and 10% tables, though of course its all marginal enough to basically be a wash. I hadn't originally thought about doing the linear addition in the log versions because its somewhat nonsensical from a theoretical perspective - why linearize the probability but then add a per-hop penalty - but if it means we can drop the linear option entirely and not branch in the scorer, sure, lets do it. 1k-over-1m (0.1% failure rate)
10k-over-1m (1% failure rate, or 10k-over-1m)
50k-over-1m (5% failure probability)
10k-over-100k (10% failure probability)
50k-over-100k (50% failure probability)
(75% failure probability)
|
Ok, so drop the cost function variant and decide on a default multiplier. You're thinking 40x? I think I tried that and 50x on a couple numbers at first and just used 50x in the script because it was a clean 0-100 sat range. But either is fine by me. Could even do a power of two to make it a shift, but then it's either 32 or 64, which is probably either too low or too high. |
Serializing scorer parameters makes it difficult to experiment with different settings.
ProbabilisticScorer tends to prefer longer routes to shorter ones. Make the default scoring behavior include a customizable base penalty to avoid longer routes.
When a routing hint is given in an invoice, the effective capacity of the channel is assumed to be infinite (i.e., u64::max_value) if the hop is private. Adding 1 to this in the success probability calculation will cause an overflow and ultimately an `index out of bounds panic` in log10_times_1024. This was not an issue with using log10 because the use of f64 would give infinite which casts to 0 for u64.
Ah, never mind on this as we are actually using msats. |
This reduce a branch in the 0 and u6::max_value cases.
b4324c0
to
4af9d44
Compare
Using a larger multiplier gives more reasonable penalties for larger success probabilities.
4af9d44
to
9596cc7
Compare
Yea, I think that's fine. Of course we basically picked a log probability set which closely hews to linear for most reasonable values, so dunno if it was worth it vs dropping log and using linear to begin with, but, its the code we have, and it is kinda nice that it gets really huge on the upper end of the probability range 🤷♂️ .
Well, then we're looking at 64K or 32K, so same thing, basically :p. Yea, sadly I don't think that's granular enough. |
Forced push to kick CI.
I think the benefit of the log is what you mentioned in #1375 (comment). |
Right, unless you use the per-hop penalty, which doesn't fit into that model. 🤷♂️ |
/// A multiplier used in conjunction with the negative `log10` of the channel's success | ||
/// probability for a payment to determine the liquidity penalty. | ||
/// | ||
/// The penalty is based in part by the knowledge learned from prior successful and unsuccessful |
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.
nit: s/in part by/in part on
Without recent knowledge of channel liquidity balances,ProbabilisticScorer
tends to give low penalties unless the payment amount is a large portion of a channel's capacity. Provide an option that gives a linear penalty increase as the success probability decreases, which may be useful in such scenarios.Additionally, give a base penalty such that shorter paths are preferred over longer ones.Make a few optimizations to
ProbabilisticScorer
including a per hopbase_penalty_msat
. Also, fix a integer overflow that will cause the log approximation to panic.Closes #1372