Expose the historical success probability calculation itself #2466

TheBlueMatt · 2023-08-01T20:57:31Z

In 3f32f60 we exposed the
historical success probability buckets directly, with a long method
doc explaining how to use it. While this is great for logging
exactly what the internal model thinks, its also helpful to let
users know what the internal model thinks the success probability
is directly, allowing them to compare route success probabilities.

Here we do so but only for the historical tracking buckets.

This also has a few cleanups in the scoring logic. This is the first few commits from #2176, without the major change to actually swap to different buckets.

codecov-commenter · 2023-08-01T22:33:25Z

Codecov Report

Patch coverage: 98.61% and project coverage change: +1.38% 🎉

Comparison is base (bada713) 90.80% compared to head (9db6652) 92.18%.
Report is 457 commits behind head on main.

❗ Current head 9db6652 differs from pull request most recent head 2bd2637. Consider uploading reports for the commit 2bd2637 to get more accurate results

❗ 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    #2466      +/-   ##
==========================================
+ Coverage   90.80%   92.18%   +1.38%     
==========================================
  Files         104      109       +5     
  Lines       53004    78528   +25524     
  Branches    53004    78528   +25524     
==========================================
+ Hits        48131    72394   +24263     
- Misses       4873     6134    +1261

Files Changed	Coverage Δ
lightning/src/routing/scoring.rs	`95.90% <98.61%> (+1.77%)`	⬆️

... and 89 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alecchendev

Still new to the scoring logic, but LGTM for the most part

lightning/src/routing/scoring.rs

alecchendev · 2023-08-02T19:07:51Z

lightning/src/routing/scoring.rs

+		let max_liquidity_msat = available_capacity.saturating_sub(
+			self.decayed_offset_msat(*self.max_liquidity_offset_msat));


nit: this is the same equation as self.max_liquidity_msat() right?

Yes, but we reuse the available_capacity calculation further down. Not that it should matter much as long as the calculation functions are inlined, but the decay'ing part may not be.

Hmmm.. why make this change? Seems like the max_liquidity_msat calculation should be kept to one place?

Because we reuse the available_capacity calculation further down and this way we do it once.

Hmm... but this isn't a problem if it is inlined. Seems it would be better to keep the max_liquidity_msat calculation in one place than trying to optimize this.

Sure, I'll #[inline(always)] In general I don't trust compilers, while they do lots of fancy things, they often miss really obvious crap because it didn't exactly match some pattern they use to detect potential optimizations (which they tend to lean heavily on because otherwise compiling would take days). Luckily identical code in following lines should be pretty hard to miss...not like I haven't seen LLVM miss equally obvious shit in the past.

lightning/src/routing/scoring.rs

tnull

LGTM, feel free to squash.

TheBlueMatt · 2023-08-13T23:26:26Z

Squashed without further changes.

jkczyz · 2023-08-22T17:01:20Z

lightning/src/routing/scoring.rs

+		let max_liquidity_msat = available_capacity.saturating_sub(
+			self.decayed_offset_msat(*self.max_liquidity_offset_msat));


Hmmm.. why make this change? Seems like the max_liquidity_msat calculation should be kept to one place?

lightning/src/routing/scoring.rs

jkczyz · 2023-08-22T17:34:22Z

lightning/src/routing/scoring.rs

-			};
-			#[cfg(not(fuzzing))]
-			debug_assert!(payment_amt_64th_bucket <= 64);
-			if payment_amt_64th_bucket > 64 { return res; }


Commit message says this just moves code, but doesn't this early return get eliminated? Do we have any tests for this?

Its still there - https://github.com/lightningdevkit/rust-lightning/pull/2466/files#diff-b00a115e9303b98772e06346a86a0312aa9790d78a73904eb6a20f938cd6da0fR1664

I don't quite follow. The early return that was removed was in penalty_msat, not calculate_success_probability_times_billion. Even if the latter returns None, the code in the else clause below is executed where previously it would not have been.

Oops, I was looking at the Move the bucketed history tracking logic into a scoring submodule commit, which is a straightline code move. Indeed, this changes behavior theoretically, but the code is unreachable - in order to reach it the early return above needs to get hit, which requires amount_msat < available_capacity, and this code requires amount_msat > self.capacity_msat, which can't both be true. If we were to remove the above hunk, this would ultimately be more correct - if we find that we cannot calculate a penalty because we can't possibly send, we shouldn't early-return with no penalty (which is equivalent to thinking we have 100% chance of success), we should calculate a penalty to incorporate the historical penalty params somehow.

lightning/src/routing/scoring.rs

jkczyz · 2023-08-23T18:38:25Z

lightning/src/routing/scoring.rs

-						let (min_buckets, max_buckets, _) = buckets.get_decayed_buckets(now,
-							*dir_liq.last_updated, self.decay_params.historical_no_updates_half_life);
+						let (min_buckets, max_buckets, _) = dir_liq.liquidity_offset_history
+							.get_decayed_buckets(now, *dir_liq.last_updated,


Maybe instead of passing now and last_updated to get_decayed_buckets and calculate_success_probability_times_billion, let's store copies in HistoricalMinMaxBuckets at construction time.

Or if we want to avoid those inadvertently diverging, get rid of HistoricalMinMaxBuckets and just put its two fields and methods on DirectedChannelLiquidity?

I honestly find it pretty weird to have a now time stored in DirectedChannelLiquidity - the struct is about the liquidity as we're aware of it, why does it care what time it is?

It's an implementation detail to compute the decay.

I guess you can pass it to all pub functions instead.

Grr, I thought I responded to this sorry. Not sure what you meant by the last comment there, but we still need to hash out how we're gonna handle time in the scorer generally, so let's revisit this when we do so? I'm not really a fan of the now/computing decay during scoring, maybe we should do so when creating the DirectedChannelLiquidity, but we can leave it here since we're not really touching that code and skip it in the MinMaxBuckets?

lightning/src/routing/scoring.rs

jkczyz · 2023-08-23T19:02:21Z

lightning/src/routing/scoring.rs

 	}
 }

+mod bucketed_history {


Is having these in a submodule that useful? Note in another comment that I left, it may actually be better to get rid of HistoricalMinMaxBuckets in order to share now and last_updated.

The module provides nice code separation from the rest of scoring, and as @tnull pointed out, the scoring file is starting to get pretty big, so probably good to move this into a seprate .rs file, just don't want to do it quite yet since there's several PRs already built on this one I don't want to rebase.

Hmm... I think it would make more sense separate out all the liquidity code and keep the historical tracking with it.

Aside from the core concept of "how do I convert liquidity bounds into a probability" (which I plan to move into a util method in a coming PR), they don't really share anything - they're doing very different calculation. Personally, I find scorer.rs pretty hard to follow, there are too many structs flying around tracking liquidity bounds, current liquidity bounds, etc. If we can split it into separate, isolated (which requires a module in rust, module members can always read struct fields otherwise) bits it'd read much cleaner. I don't feel super strongly about a new file, but a module at least makes sense, and probably putting the log table in a new file cause its really distracting.

Aside from the core concept of "how do I convert liquidity bounds into a probability" (which I plan to move into a util method in a coming PR), they don't really share anything - they're doing very different calculation.

That's the penalty computation. But tracking and updating the current liquidity seems to have a relationship with historical liquidity in so far they are both code related to liquidity, even if the code itself doesn't overlap. I mean, structurally they belong together at very least given one is a sub-struct of the other. Seems odd to have those in separate modules.

Personally, I find scorer.rs pretty hard to follow, there are too many structs flying around tracking liquidity bounds, current liquidity bounds, etc. If we can split it into separate, isolated (which requires a module in rust, module members can always read struct fields otherwise) bits it'd read much cleaner. I don't feel super strongly about a new file, but a module at least makes sense, and probably putting the log table in a new file cause its really distracting.

FWIW, when I said "liquidity code and keep the historical tracking with it", I meant move ChannelLiquidity, DirectedChannelLiquidity, and all the historical tracking structs into a separate module.

That's the penalty computation. But tracking and updating the current liquidity seems to have a relationship with historical liquidity in so far they are both code related to liquidity, even if the code itself doesn't overlap. I mean, structurally they belong together at very least given one is a sub-struct of the other. Seems odd to have those in separate modules.

Mmm, fair, don't feel super strongly about that/separate file, but the in-file module makes fields "private", which is really nice.

FWIW, when I said "liquidity code and keep the historical tracking with it", I meant move ChannelLiquidity, DirectedChannelLiquidity, and all the historical tracking structs into a separate module.

That's a large part of the file, though? Aside from the traits themselves and the log tables and tests of the same.

That's a large part of the file, though? Aside from the traits themselves and the log tables and tests of the same.

Yeah, implementation code is about 600 lines excluding log approximation (about 800 with). Only a few of the tests examine the liquidity structs directly. The rest look at channel_penalty_msat.

I'm fine putting that in one file / module and having scoring.rs use whatever interface that it exposes.

Yea, we can debate the file structure separately after we do all the rewriting here and in the next two PRs. In either case, I'd like to keep the module as a way to enforce private-ness of helper utilities and struct fields in the historical state tracking/penalty calculating.

jkczyz · 2023-08-23T19:10:15Z

lightning/src/routing/scoring.rs

-			};
-			#[cfg(not(fuzzing))]
-			debug_assert!(payment_amt_64th_bucket <= 64);
-			if payment_amt_64th_bucket > 64 { return res; }


I don't quite follow. The early return that was removed was in penalty_msat, not calculate_success_probability_times_billion. Even if the latter returns None, the code in the else clause below is executed where previously it would not have been.

jkczyz · 2023-08-23T19:13:23Z

lightning/src/routing/scoring.rs

+		let max_liquidity_msat = available_capacity.saturating_sub(
+			self.decayed_offset_msat(*self.max_liquidity_offset_msat));


Hmm... but this isn't a problem if it is inlined. Seems it would be better to keep the max_liquidity_msat calculation in one place than trying to optimize this.

jkczyz

LGTM aside from #2466 (comment), but that can always wait on a follow-up. Feel free to squash.

Currently we let an `htlc_amount >= channel_capacity` pass through from `penalty_msat` to `calculate_success_probability_times_billion`, but only if its only marginally bigger (less than 65/64ths). This is fine as `calculate_success_probability_times_billion` handles bogus values just fine (it will always return a zero probability in such cases). However, this is risky, and in fact breaks in the coming commits, so instead check it before ever calling through to the historical bucket probability calculations.

When we attempt to score a channel which has a success probability very low, we may have a log well above our cut-off of two. For the liquidity penalties this works great, we bound it by `NEGATIVE_LOG10_UPPER_BOUND` and `min` the two scores. For the amount liquidity penalty we didn't do any `min`ing at all. This fix is to min the log itself first and then reuse the min'd log in both calculations.

This simply moves code which will simplify the next commit somewhat.

In 3f32f60 we exposed the historical success probability buckets directly, with a long method doc explaining how to use it. While this is great for logging exactly what the internal model thinks, its also helpful to let users know what the internal model thinks the success probability is directly, allowing them to compare route success probabilities. Here we do so but only for the historical tracking buckets.

This removes the need to reconstruct the struct in a number of places by simply creating it up front.

TheBlueMatt · 2023-08-23T21:15:26Z

Oops, sorry, I thought I replied to that, did now. Squashed without any further changes.

TheBlueMatt added this to the 0.0.117 milestone Aug 1, 2023

TheBlueMatt mentioned this pull request Aug 1, 2023

Move the historical bucket tracker to 32 unequal sized buckets #2176

Merged

tnull self-requested a review August 2, 2023 06:46

alecchendev reviewed Aug 2, 2023

View reviewed changes

tnull reviewed Aug 9, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-07-expose-success-prob branch from b9484ed to bc23ac8 Compare August 11, 2023 00:42

tnull reviewed Aug 11, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-07-expose-success-prob branch from bc23ac8 to 31448fd Compare August 13, 2023 23:26

tnull previously approved these changes Aug 14, 2023

View reviewed changes

jkczyz self-requested a review August 14, 2023 15:01

jkczyz reviewed Aug 22, 2023

View reviewed changes

TheBlueMatt dismissed tnull’s stale review via b87ccbc August 23, 2023 00:47

TheBlueMatt force-pushed the 2023-07-expose-success-prob branch from 31448fd to b87ccbc Compare August 23, 2023 00:47

jkczyz reviewed Aug 23, 2023

View reviewed changes

TheBlueMatt force-pushed the 2023-07-expose-success-prob branch from b87ccbc to 9db6652 Compare August 23, 2023 19:53

jkczyz reviewed Aug 23, 2023

View reviewed changes

TheBlueMatt added 6 commits August 23, 2023 21:15

Find payment bucket in calculate_success_probability_times_billion

5687310

This simply moves code which will simplify the next commit somewhat.

Move the bucketed history tracking logic into a scoring submodule

2a1dff4

Store a HistoricalMinMaxBuckets in DirectedChannelLiquidity

2bd2637

This removes the need to reconstruct the struct in a number of places by simply creating it up front.

TheBlueMatt force-pushed the 2023-07-expose-success-prob branch from 9db6652 to 2bd2637 Compare August 23, 2023 21:15

TheBlueMatt mentioned this pull request Aug 23, 2023

Fetch time at start of routing #2483

Closed

jkczyz approved these changes Aug 24, 2023

View reviewed changes

tnull approved these changes Aug 25, 2023

View reviewed changes

tnull merged commit af3a369 into lightningdevkit:main Aug 25, 2023

		let max_liquidity_msat = available_capacity.saturating_sub(
		self.decayed_offset_msat(*self.max_liquidity_offset_msat));

Expose the historical success probability calculation itself #2466

Expose the historical success probability calculation itself #2466

Uh oh!

Conversation

TheBlueMatt commented Aug 1, 2023

Uh oh!

codecov-commenter commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alecchendev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Aug 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 1, 2023 •

edited

Loading