Pass EffectiveCapacity through to scorer instead of a u64#1383
Pass EffectiveCapacity through to scorer instead of a u64#1383TheBlueMatt wants to merge 1 commit intolightningdevkit:mainfrom
EffectiveCapacity through to scorer instead of a u64#1383Conversation
Codecov Report
@@ Coverage Diff @@
## main #1383 +/- ##
========================================
Coverage 90.81% 90.82%
========================================
Files 73 73
Lines 41209 41407 +198
Branches 41209 41407 +198
========================================
+ Hits 37424 37607 +183
- Misses 3785 3800 +15
Continue to review full report at Codecov.
|
There is little reason to take the `EffectiveCapacity`, which has a bunch of information about the type of channel, and distill it down to a `u64` when scoring channels. Instead, we here pass the full information we know, in the form of the original `EffectiveCapacity`. This does create more branching in the main router loop, which appears to have a very slight (1-2%) performance loss, but that may well be within noise. Much more importantly, this resolves a panic in our log approximation where we can accidentally call `log(0)` when the channel's effective capacity is `u64::max_value()`.
TheBlueMatt
force-pushed
the
2022-03-no-log-0
branch
from
24183e4 to
72bc65e
Compare
Wouldn't it be quite simpler just to use let numerator = (max_liquidity_msat - amount_msat).saturating_add(1);
let denominator = (max_liquidity_msat - min_liquidity_msat).saturating_add(1); |
| Unknown { | ||
| /// An amount which should be considered "already used". Used during routing to keep track | ||
| /// of how much we're already considering routing through this channel. | ||
| previously_used_msat: u64, |
There was a problem hiding this comment.
Hmmm... I was hoping this would be a separate parameter to the scorer since it is relevant for all variants.
There was a problem hiding this comment.
Hmm, good point, let me benchmark that.
There was a problem hiding this comment.
Sure, but its preferrable to tick something off the TODO list while we're at it rather than just fix the narrow case, no?
Not sure exactly how it would ultimately shake out, but I was thinking something along these lines: #1227 (comment). Had a rough start to this as part of some tabled work from #1227 (jkczyz@b88b37e).
Then each scorer could use the in-use amount as they wish. ProbabilisticScorer would just need to subtract it from max_liquidity_msat.
I'd prefer if we make a small fix for 0.0.106 to avoid adding more review to be honest. Also, it somewhat conflicts with some work I pulled out of #1227 but haven't PR'ed: jkczyz@a926913.
There was a problem hiding this comment.
it somewhat conflicts with some work I pulled out of #1227 but haven't PR'ed: jkczyz@a926913.
Okay, sounds great! I'll close this and circle back around on the panic. I sent you the patch I'd started to build for this on discord, it looked like a performance win so probably good to incorporate it in your work.
Sure, but its preferrable to tick something off the TODO list while we're at it rather than just fix the narrow case, no? |
3 participants
There is little reason to take the
EffectiveCapacity, which hasa bunch of information about the type of channel, and distill it
down to a
u64when scoring channels.Instead, we here pass the full information we know, in the form of
the original
EffectiveCapacity. This does create more branchingin the main router loop, which appears to have a very slight (1-2%)
performance loss, but that may well be within noise.
Much more importantly, this resolves a panic in our log
approximation where we can accidentally call
log(0)when thechannel's effective capacity is
u64::max_value().