Merge probabilistic scores from external source #3562

joostjager · 2025-01-27T08:41:35Z

Usage in LDK node: lightningdevkit/ldk-node#449

Only "fix historical liquidity bucket decay" should be backported

codecov · 2025-01-27T08:53:19Z

Codecov Report

Attention: Patch coverage is 90.03559% with 28 lines in your changes missing coverage. Please review.

Project coverage is 88.53%. Comparing base (c5fd164) to head (2c1bdfd).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/routing/scoring.rs	90.03%	25 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3562      +/-   ##
==========================================
+ Coverage   88.52%   88.53%   +0.01%     
==========================================
  Files         149      149              
  Lines      115030   115279     +249     
  Branches   115030   115279     +249     
==========================================
+ Hits       101833   102067     +234     
- Misses      10706    10720      +14     
- Partials     2491     2492       +1

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

joostjager · 2025-01-30T11:30:48Z

lightning/src/routing/scoring.rs

+
+	fn time_passed(&mut self, duration_since_epoch: Duration, decay_params: ProbabilisticScoringDecayParameters) {
+		self.0.retain(|_scid, liquidity| {
+			liquidity.min_liquidity_offset_msat =


Maybe move this into the ChannelLiquidity (singular) struct

If we do so, would we gain much by introducing ChannelLiquidities at all? Maybe we just use HashMap<u64, ChannelLiquidity> in the API?

I think being able to offer state-level functionality does make things a bit cleaner. Maybe I should also add a merge method on this level.

The original reason for this struct though is to be able to use ser/deser logic without a scorer.

joostjager · 2025-01-30T11:31:09Z

lightning/src/routing/scoring.rs

-	channel_liquidities: HashMap<u64, ChannelLiquidity>,
+	channel_liquidities: ChannelLiquidities,
+}
+/// ChannelLiquidities contains live and historical liquidity bounds for each channel.


Objections to moving this into its own file?

Yea, splitting scoring.rs into two modules would be nice. We generally don't put individual structs in their own module just for the sake of it but when files get too big, splitting them down the middle (if there's a clean way to do it) is always nice...there's a few files that are in desperate need of it.

For my workflow, sticking to one struct per file would work well. I do find myself navigating quite a bit in these large files, using editor features (find, find symbol, find ref) to make it easier but not perfect. I'd rather use the folder/file hierarchy and pinning of files as tabs for example. But it is personal of course.

Some type of split would be welcome either way. For this PR I could start with a liquidity_information (open to naming suggestions) module and place the new ChannelLiquidities in it. Then in a separate PR move more liquidity code (ChannelLiquidity, HistoricalLiquidityTracker, HistoricalBucketRangeTracker and tests) in there. Thoughts?

Thoughts?

I like the idea of breaking up our humongous modules and splitting more types out, be it in this PR or a follow up.
IMO, we could consider a folder structure such as:

src/routing/scoring/mod.rs (moved from src/routing/scoring.rs for backwards compat of the path) src/routing/scoring/liquidity_tracking.rs (or just liquidity.rs ?)

FWIW, another easy step towards cleaning up/smaller files would be to move the entire bucketed_history sub-module out of scoring.rs and into a dedicated bucketed_history.rs file. Although, if we do this, we could consider movng the *Liquidity* types there, too.

Skipping the moves for now as this PR is getting close to ready.

lightning/src/routing/scoring.rs

tnull · 2025-01-30T11:57:13Z

lightning/src/routing/scoring.rs


 impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> Writeable for ProbabilisticScorer<G, L> where L::Target: Logger {
 	#[inline]
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		write_tlv_fields!(w, {


I think we want to maintain reading/writing the TLV fields here, rather than moving them into ChannelLiquidities, given that we're more likely to add additional fields requiring persisting on the more general ProbabilisticScorer.

Is it likely that there will be more persistent state unrelated to channels? If so, wouldn't that state also be placed in ChannelLiquidities, extending it with additional fields beyond the hash map? At that point, the struct might need a more general name, but I imagine those new fields would still be part of what you'd want to export/import.

For now, the purpose of creating the struct is to allow deserialization of state from disk or network without having to construct a full probabilistic scorer, which includes non-persistent and irrelevant fields.

lightning/src/routing/scoring.rs

tnull · 2025-01-30T12:09:42Z

lightning/src/routing/scoring.rs

+
+	fn time_passed(&mut self, duration_since_epoch: Duration, decay_params: ProbabilisticScoringDecayParameters) {
+		self.0.retain(|_scid, liquidity| {
+			liquidity.min_liquidity_offset_msat =


If we do so, would we gain much by introducing ChannelLiquidities at all? Maybe we just use HashMap<u64, ChannelLiquidity> in the API?

TheBlueMatt

Sorry for the delay here.

TheBlueMatt · 2025-02-01T14:15:08Z

lightning/src/routing/scoring.rs

 					liquidity.liquidity_history.decay_buckets(elapsed_time.as_secs_f64() / half_life);
-					liquidity.offset_history_last_updated = duration_since_epoch;
+					liquidity.offset_history_last_updated += decay_params.historical_no_updates_half_life;


Not sure I get why we're calling decay_buckets in a loop. It already does buckets *= (1/2)^half_lives so we shouldn't need to call it repeatedly.

Well...actually, looking at it it is wrong, its doing buckets *= 1024 / 2048^half_lives instead of buckets *= (1024 / 2048)^half_lives, but we should fix the math instead of calling it in a loop :)

I am not sure either 😅 At some point I concluded that this was a discrete operation, probably set on the wrong foot by that if elapsed_time > decay_params.historical_no_updates_half_life statement.

I do wonder though why the buckets aren't decayed always like the live bounds and have this 1 half life waiting time? In the end, half life is just a way to express a rate, and it seems a bit strange to also use that in the way it is used in the if expression.

Good catch on the formula. Your suggestion is correct, but 1024/2048 is just 0.5 and doesn't work with integer math. Added a commit to fix it, and a unit test.

I do wonder though why the buckets aren't decayed always like the live bounds and have this 1 half life waiting time? In the end, half life is just a way to express a rate, and it seems a bit strange to also use that in the way it is used in the if expression.

This goes back a bit to the conclusion we took that half-lives for scoring data are Always Wrong (because they're both too fast and too slow). Thus, we really want the historical buckets to only decay as we get new data in them (which they always do when we get new data in them). However, it seems like it'd be obviously bad if we're scoring one channel with data that's a month old while treating it the same as another channel with data that's a minute old, so we added a decay, but made it pretty aggressively slow and only run it if we haven't gotten data in a while (since we'd prefer to only decay based on new data).

Not so easy to feel confident that that is indeed the right thing to do, but that's the gut-part of all of this I suppose.

I've added this explanation as a comment to the code.

TheBlueMatt · 2025-02-01T14:31:02Z

lightning/src/routing/scoring.rs

-	channel_liquidities: HashMap<u64, ChannelLiquidity>,
+	channel_liquidities: ChannelLiquidities,
+}
+/// ChannelLiquidities contains live and historical liquidity bounds for each channel.


Yea, splitting scoring.rs into two modules would be nice. We generally don't put individual structs in their own module just for the sake of it but when files get too big, splitting them down the middle (if there's a clean way to do it) is always nice...there's a few files that are in desperate need of it.

lightning/src/routing/scoring.rs

TheBlueMatt

Basically LGTM, a few nits and one actual comment.

lightning/src/routing/scoring.rs

joostjager · 2025-02-06T14:50:11Z

Added set method.

lightning/src/routing/scoring.rs

TheBlueMatt · 2025-02-07T13:41:27Z

lightning/src/routing/scoring.rs

+}
+/// Container for live and historical liquidity bounds for each channel.
+#[derive(Clone)]
+pub struct ChannelLiquidities(HashMap<u64, ChannelLiquidity>);


I realize we don't actually expose this anywhere so its not possible with the current API for an LSP to expose one of these.

How this works in ldk-node - or is intended to work - is that the serialized version of this data is fetched from the database and exposed.

This struct comes into play when merging the scores again. Bytes are retrieved via a url, and then deserialized into this ChannelLiquidities struct.

Right, but we have to also expose the ability to fetch this struct from a ProbabilisticScorer on the LSP end so that it can write the data to a server to be fetched from the URL :)

Why is that? I am assuming the LSP does something like https://github.com/lightningdevkit/ldk-node/pull/458/files

Added get_scores on ProbabilisticScorer.

Renamed the getter to scores() to follow convention

Wrap the liquidities hash map into a struct so that decay and serialization functionality can be attached. This allows external data to be serialized into this struct and decayed to make it comparable and mergeable.

Add a new scorer that is able to combine local score with scores coming in from an external source. This allows light nodes with a limited view on the network to improve payment success rates.

The formula for applying half lives was incorrect. Test coverage added.

tnull · 2025-02-10T12:02:49Z

lightning/src/routing/scoring.rs

@@ -1148,6 +1148,11 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> ProbabilisticScorer<G, L> whe
 	pub fn set(&mut self, external_scores: ChannelLiquidities) {
 		_ = mem::replace(&mut self.channel_liquidities, external_scores);
 	}
+
+	// Returns the current scores.
+	pub fn get_scores(&self) -> ChannelLiquidities {


nit: Please follow the Rust API Guidelines where possible, i.e., in the general case the get_ prefix is avoided and the method name should often reflect the field name. Suggestion: channel_liquidities

We'd then mix scores (in parameter names) and channel_liquidities. Probably better to align them - but which one is the better name?

I'd be voting for channel_liquidities but doesn't matter too much, so feel free to leave it as scores if you prefer, IMO.

Leaving it to scores, as we refer to this data as scores elsewhere too now. It is a good point, it isn't fully consistent. Naming is hard.

tnull

LGTM, one nit.

lightning/src/routing/scoring.rs

This commit expands on the previously introduced merge method by offering a way to simply replace the local scores by the liquidity information that is obtained from an external source.

lightning/src/routing/scoring.rs

Allows access to the scorer state. An example use case is an LSP exposing the global network view in its scorer over http to light clients.

TheBlueMatt

Landing as the diff since @tnull's ACK is just

$ git diff-tree -U2 6f40f73ac766ae35f559a375bba2d59b2b139753 e9921ddb016dba1c6ce0b371e4ced7faf4956b62
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index ae6c754a9..bbdd75284 100644
--- a/lightning/src/routing/scoring.rs
+++ b/lightning/src/routing/scoring.rs
@@ -1151,6 +1151,6 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> ProbabilisticScorer<G, L> whe

 	/// Returns the current scores.
-	pub fn scores(&self) -> ChannelLiquidities {
-		self.channel_liquidities.clone()
+	pub fn scores(&self) -> &ChannelLiquidities {
+		&self.channel_liquidities
 	}
 }

TheBlueMatt · 2025-02-21T22:49:36Z

Backported the decay fix in #3613

joostjager force-pushed the merge-scores branch from 71a9fc0 to f726a8e Compare January 27, 2025 08:45

joostjager force-pushed the merge-scores branch 8 times, most recently from c24cc83 to 85f3fee Compare January 30, 2025 11:30

joostjager commented Jan 30, 2025

View reviewed changes

joostjager force-pushed the merge-scores branch from 85f3fee to 05bca3b Compare January 30, 2025 11:46

joostjager commented Jan 30, 2025

View reviewed changes

lightning/src/routing/scoring.rs Show resolved Hide resolved

joostjager mentioned this pull request Jan 30, 2025

Periodical external pathfinding scores merge lightningdevkit/ldk-node#449

Open

tnull reviewed Jan 30, 2025

View reviewed changes

joostjager force-pushed the merge-scores branch from 05bca3b to d6caa86 Compare January 30, 2025 12:39

joostjager requested a review from tnull January 30, 2025 12:55

joostjager force-pushed the merge-scores branch 2 times, most recently from fdad047 to ced0adc Compare January 30, 2025 16:31

joostjager marked this pull request as ready for review January 31, 2025 08:53

joostjager requested a review from TheBlueMatt January 31, 2025 11:34

TheBlueMatt reviewed Feb 1, 2025

View reviewed changes

joostjager force-pushed the merge-scores branch 3 times, most recently from b459831 to 055177c Compare February 3, 2025 09:29

joostjager requested a review from TheBlueMatt February 3, 2025 10:02

joostjager force-pushed the merge-scores branch from 055177c to 6acce79 Compare February 3, 2025 10:55

joostjager added the weekly goal Someone wants to land this this week label Feb 4, 2025

TheBlueMatt reviewed Feb 4, 2025

View reviewed changes

lightning/src/routing/scoring.rs Show resolved Hide resolved

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

joostjager requested a review from tnull February 6, 2025 14:50

tnull reviewed Feb 6, 2025

View reviewed changes

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

joostjager force-pushed the merge-scores branch from 3808c85 to 2c1bdfd Compare February 6, 2025 15:24

joostjager requested a review from tnull February 6, 2025 15:25

TheBlueMatt added the backport 0.1 label Feb 7, 2025

TheBlueMatt reviewed Feb 7, 2025

View reviewed changes

joostjager added 3 commits February 7, 2025 16:39

refactor hashmap to channelliquidities struct

252b2d3

Wrap the liquidities hash map into a struct so that decay and serialization functionality can be attached. This allows external data to be serialized into this struct and decayed to make it comparable and mergeable.

add combined scorer

311a083

Add a new scorer that is able to combine local score with scores coming in from an external source. This allows light nodes with a limited view on the network to improve payment success rates.

fix historical liquidity bucket decay

bb468dd

The formula for applying half lives was incorrect. Test coverage added.

joostjager force-pushed the merge-scores branch from 2c1bdfd to 6bd71c2 Compare February 7, 2025 15:40

tnull reviewed Feb 10, 2025

View reviewed changes

joostjager force-pushed the merge-scores branch 2 times, most recently from c42f96c to 0514071 Compare February 10, 2025 13:25

joostjager requested a review from tnull February 10, 2025 13:40

tnull previously approved these changes Feb 10, 2025

View reviewed changes

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

add set_scores method on CombinedScorer to overwrite local data

630246b

This commit expands on the previously introduced merge method by offering a way to simply replace the local scores by the liquidity information that is obtained from an external source.

joostjager dismissed tnull’s stale review via 6f40f73 February 10, 2025 13:48

joostjager force-pushed the merge-scores branch from 0514071 to 6f40f73 Compare February 10, 2025 13:48

tnull previously approved these changes Feb 10, 2025

View reviewed changes

joostjager requested a review from TheBlueMatt February 10, 2025 13:49

TheBlueMatt reviewed Feb 10, 2025

View reviewed changes

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

add scores getter on ProbabilisticScorer

e9921dd

Allows access to the scorer state. An example use case is an LSP exposing the global network view in its scorer over http to light clients.

joostjager dismissed tnull’s stale review via e9921dd February 10, 2025 15:29

joostjager force-pushed the merge-scores branch from 6f40f73 to e9921dd Compare February 10, 2025 15:29

TheBlueMatt approved these changes Feb 10, 2025

View reviewed changes

tnull approved these changes Feb 10, 2025

View reviewed changes

TheBlueMatt merged commit f866e2c into lightningdevkit:main Feb 10, 2025
24 of 26 checks passed

TheBlueMatt removed the backport 0.1 label Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge probabilistic scores from external source #3562

Merge probabilistic scores from external source #3562

joostjager commented Jan 27, 2025 •

edited by TheBlueMatt

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading

joostjager Jan 30, 2025

tnull Jan 30, 2025

joostjager Jan 30, 2025

joostjager Jan 30, 2025

TheBlueMatt Feb 1, 2025

joostjager Feb 3, 2025

tnull Feb 3, 2025

tnull Feb 3, 2025 •

edited

Loading

joostjager Feb 5, 2025

tnull Jan 30, 2025

joostjager Jan 30, 2025 •

edited

Loading

tnull Jan 30, 2025

TheBlueMatt left a comment

TheBlueMatt Feb 1, 2025

joostjager Feb 3, 2025

TheBlueMatt Feb 4, 2025

joostjager Feb 5, 2025

TheBlueMatt Feb 1, 2025

TheBlueMatt left a comment

joostjager commented Feb 6, 2025

TheBlueMatt Feb 7, 2025

joostjager Feb 7, 2025

TheBlueMatt Feb 7, 2025

joostjager Feb 7, 2025

joostjager Feb 10, 2025

joostjager Feb 10, 2025

tnull Feb 10, 2025

joostjager Feb 10, 2025 •

edited

Loading

tnull Feb 10, 2025

joostjager Feb 10, 2025

tnull left a comment

TheBlueMatt left a comment

TheBlueMatt commented Feb 21, 2025

Merge probabilistic scores from external source #3562

Merge probabilistic scores from external source #3562

Conversation

joostjager commented Jan 27, 2025 • edited by TheBlueMatt Loading

codecov bot commented Jan 27, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

joostjager commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 21, 2025

joostjager commented Jan 27, 2025 •

edited by TheBlueMatt

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading

tnull Feb 3, 2025 •

edited

Loading

joostjager Jan 30, 2025 •

edited

Loading

joostjager Feb 10, 2025 •

edited

Loading