ProbabilisticScorer improvements #1399
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1399 +/- ##
==========================================
+ Coverage 90.76% 90.83% +0.06%
==========================================
Files 73 73
Lines 41195 41476 +281
Branches 41195 41476 +281
==========================================
+ Hits 37392 37676 +284
+ Misses 3803 3800 -3
Continue to review full report at Codecov.
|
lightning/src/routing/scoring.rs
Outdated
| /// provide higher penalties for large payments. | ||
| /// | ||
| /// Default value: false | ||
| pub include_amount_penalty: bool, |
There was a problem hiding this comment.
I'm not sure if we want to make this a switch instead of a second penalty - I think a second penalty more closely lines up with what fees are (a combo of % and absolute fees), which is the property we're going for here - ability to match penalties up to expected/reasonable fees.
There was a problem hiding this comment.
Updated as discussed offline. Please take a look at the amount_penalty_multiplier_msat docs. I tried to make them a bit more human understandable. Let me know if I hit the mark or if it can be fine tuned in any way.
jkczyz
force-pushed
the
2022-03-scoring-tweaks
branch
from
7817e53 to
beecbb3
Compare
jkczyz
force-pushed
the
2022-03-scoring-tweaks
branch
from
beecbb3 to
53f8760
Compare
|
LGTM. Feel free to squash. |
jkczyz
force-pushed
the
2022-03-scoring-tweaks
branch
from
5b80256 to
51e1de9
Compare
jkczyz
changed the title
| } else if max_liquidity_msat != self.capacity_msat { | ||
| // Avoid using the failed channel on retry. | ||
| u64::max_value() | ||
| } else { |
There was a problem hiding this comment.
how likely is this case to ever be hit, that amount_msat is precisely equal to max_liquidity_msat?
There was a problem hiding this comment.
If we just tried the payment, its possible, the final else here pretty unlikely I'd guess. Maybe for "very round" numbers - 1m sats, 100k sats, etc it could happen.
There was a problem hiding this comment.
Yeah, also if the HTLC maximum is some round amount, it may get hit. We should really use the capacity here if known, but currently we do not. I have a commit to do this that was dropped from the original ProbabilisticScorer PR.
jkczyz
force-pushed
the
2022-03-scoring-tweaks
branch
from
51e1de9 to
c8626ca
Compare
In ProbabilisticScorer, the channel liquidity balance is reduced whenever a payment fails at the corresponding channel. The payment may still be retried through the channel, however, because the liquidity penalty is capped. Use u64::max_value instead in this situation to avoid retrying over the same path. This effectively makes u64::max_value the penalty for amounts exceeding the upper bound, as well. As an edge case, avoid using u64::max_value on attempts where the amount is equal to the effective capacity, which may be the HTLC maximum when the channel capacity is unknown.
The cost of large payments tends to be dominated by the channel fees. To avoid this, add an amount penalty to ProbabilisticScorer with a user configurable multiplier. The multiplier is applied for every 2^20th of the amount weighted by the negative log10 of the channel's success probability for the payment.
jkczyz
force-pushed
the
2022-03-scoring-tweaks
branch
from
c8626ca to
5425b2b
Compare
In
ProbabilisticScorer, the channel liquidity balance is reduced whenever a payment fails at the corresponding channel. The payment may still be retried through the channel, however, because the liquidity penalty is capped. Useu64::max_valueinstead in this situation to avoid retrying over the same path. This effectively makesu64::max_valuethe penalty for amounts exceeding the upper bound, as well.Also, penalties tends to be dominated by fees for large payments. To avoid this, add an amount penalty to
ProbabilisticScorerwith a user configurable multiplier. The multiplier is applied for every2^20th of the amount weighted by the negativelog10of the channel's success probability for the payment.