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

TheBlueMatt

LGTM. Can you rewrite the commit messages to ad a bit more context and not have any lines in the commit message run longer than ~70 chars before a line break?

tnull

Basically LGTM, one question

Also, nit: please avoid rebasing if not necessary for conflict resolution, as it's making it harder for reviewers to follow what changed during a push.

tnull · 2025-02-06T12:13:12Z

lightning/src/routing/scoring.rs

+
+impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> Writeable for CombinedScorer<G, L> where L::Target: Logger {
+	fn write<W: crate::util::ser::Writer>(&self, writer: &mut W) -> Result<(), crate::io::Error> {
+		self.local_only_scorer.write(writer)


I think we discussed also storing a cache of the merged scorer to avoid regressing, e.g., after restart. Should we still add that?

I am not sure if that is really necessary. Restarts may be infrequent, and also in ldk-node, logic could be added to always sync during the startup sequence before any payments can be made?

Well, if we're thinking 'self-custodial mobile node' restarts might be in fact very frequent, and we'd want to make sure that the scoring data is available as soon as the node is ready to pay. We definitely also don't want to block startup of the node on the scoring update. We could of course consider having LDK Node store the last received update and merge that on startup, but might as well persist the cache here, if we're already doing all the other handling of the two scorers?

Yes that's reasonable. I can add it to this PR. How would that work - add another level of tlv-nesting here with two types for the two scorers?

Probably the simplest approach, yea.

But does this work? Previously a single scorer was persisted underneath a key in the store. Now to that same key, another tlv level is added, breaking backward compatibility. Interested to hear how this is typically handled in LDK.

tnull

Ah, we also talked about a simple setter that would allow us to override (not merge) the stored scores. Could we still add that as a separate commit in this PR?

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

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.

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 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 force-pushed the merge-scores branch from 6acce79 to 0609cc1 Compare February 5, 2025 09:13

joostjager requested a review from TheBlueMatt February 5, 2025 14:20

TheBlueMatt previously approved these changes Feb 5, 2025

View reviewed changes

joostjager force-pushed the merge-scores branch from 0609cc1 to a4993a4 Compare February 6, 2025 08:30

tnull reviewed Feb 6, 2025

View reviewed changes

joostjager dismissed TheBlueMatt’s stale review via 3808c85 February 6, 2025 14:49

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 4 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.

add set method on CombinedScorer to overwrite local data

6bd71c2

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 force-pushed the merge-scores branch from 2c1bdfd to 6bd71c2 Compare February 7, 2025 15:40

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

TheBlueMatt left a comment

tnull left a comment •

edited

Loading

tnull Feb 6, 2025

joostjager Feb 6, 2025 •

edited

Loading

tnull Feb 6, 2025

joostjager Feb 6, 2025

TheBlueMatt Feb 6, 2025

joostjager Feb 7, 2025

tnull left a comment

joostjager commented Feb 6, 2025

TheBlueMatt Feb 7, 2025

joostjager Feb 7, 2025

TheBlueMatt Feb 7, 2025

joostjager Feb 7, 2025

Merge probabilistic scores from external source #3562

Are you sure you want to change the base?

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

TheBlueMatt left a comment

Choose a reason for hiding this comment

tnull left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Feb 6, 2025 • edited Loading

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 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

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

tnull left a comment •

edited

Loading

joostjager Feb 6, 2025 •

edited

Loading