Allow combining of Prob Scorers #2469
Conversation
|
Mh, I think it would be nice to enable this, but it currently leaves out the most important part: combining the historical liquidity data, which likely will not be as easy as just merging by replacing with the most recent data items. |
lightning/src/routing/scoring.rs
Outdated
| if current.last_updated.duration_since(item.last_updated) == Duration::from_secs(0) { | ||
| channel_liquidities.insert(id, item); | ||
| } |
There was a problem hiding this comment.
The most recent isn't necessarily better. Either the min or or the max liquidity would have been affected, but not both. So you may be discarding a better estimate of one for a worse estimate of the other -- or just getting a worse estimate for both if the updated one was based on a smaller value. You probably want the one that has the smallest delta between min and max, which indicates a tighter bound. Taking account the decay should account for older data.
There was a problem hiding this comment.
Changed to this, probably best solution is being able to combine the HistoricalBucketRangeTracker but that is outside my paygrade haha
benthecarman
force-pushed
the
combine-scorer
branch
from
b5f44c5 to
9d64597
Compare
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2469 +/- ##
==========================================
- Coverage 90.34% 90.31% -0.04%
==========================================
Files 106 106
Lines 55784 55779 -5
Branches 55784 55779 -5
==========================================
- Hits 50398 50376 -22
- Misses 5386 5403 +17
☔ View full report in Codecov by Sentry. |
lightning/src/routing/scoring.rs
Outdated
| impl<T: Time> ChannelLiquidity<T> { | ||
| /// The difference between the upper and lower liquidity bounds. | ||
| fn liquidity_offset_delta(&self) -> u64 { | ||
| self.max_liquidity_offset_msat - self.min_liquidity_offset_msat |
There was a problem hiding this comment.
These are relative offsets (i.e., from the channel capacity / max htlc and zero, respectively). You want the absolute, decayed values instead. Otherwise, no knowledge (i.e., offsets of zero) would be preferred. See min_liquidity_msat and max_liquidity_msat instead, which also account for decays.
Please also add tests. :)
There was a problem hiding this comment.
Those are available for the directed version, I can put fake info to get those, otherwise not sure the best way to handle this
There was a problem hiding this comment.
I think just adding a private function using the canonical order is fine. It doesn't matter which way they are directed so long as they are the same direction when compared.
|
Combining the buckets (which I think we need to do) shouldn't be too hard, I think. Basically just add each bucket up and divide by two, I think. Set the last-update time to the newest one and call it a day. |
benthecarman
force-pushed
the
combine-scorer
branch
from
9d64597 to
9606fa1
Compare
|
I think I am now correctly combining the channel liquidities |
| self.min_liquidity_offset_msat = self.min_liquidity_offset_msat.max(other.min_liquidity_offset_msat); | ||
| self.max_liquidity_offset_msat = self.max_liquidity_offset_msat.min(other.max_liquidity_offset_msat); |
There was a problem hiding this comment.
Please add a test. You need to use the methods on DirectedChannelLiquidity otherwise these aren't decayed and thus aren't directly comparable. Each ChannelLiquidity will likely have a different last_updated, meaning one may need to be decayed more than the other.
| self.max_liquidity_offset_history.combine(other.max_liquidity_offset_history); | ||
|
|
||
| if self.last_updated.duration_since(other.last_updated) < Duration::from_secs(0) { | ||
| self.last_updated = other.last_updated; |
There was a problem hiding this comment.
Changing last_updated without decaying the liquidity offsets to reflect the new time won't work. The data is stored as undecayed offsets. When the penalty is calculated, the offsets are decayed based on elapsed time since the last_updated. And any time last_updated is modified, any unaffected offset from the update is set to its decayed value. You'll need to do something similar here, otherwise offsets may not be properly decayed.
See DirectedChannelLiquidity::set_min_liquidity_msat and DirectedChannelLiquidity::set_max_liquidity_msat. I think it would be easiest to normalize self and other by setting setting each offset to their decayed values and set last_updated to T::now(). Then they can be directly compared.
|
Is this still on your radar @benthecarman? |
|
This would be cool functionality but I'm not really sure what I'm doing here |
|
Shame, I don't think this PR was that far away. |
Could be useful if getting scorers from multiple different sources