Add a per-amount base penalty in the ProbabilisticScorer #1617

TheBlueMatt · 2022-07-14T01:14:45Z

Based on #1610 so I don't have to rebase aggressively, tagging 110 as its a user feature request and this is trivial.

There's not much reason to not have a per-hop-per-amount penalty in
the ProbabilisticScorer to go along with the per-hop penalty to
let it scale up to larger amounts, so we add one here.

Notably, we use a divisor of 2^30 instead of 2^20 (like the
equivalent liquidity penalty) as it allows for more flexibility,
and there's not really any reason to worry about us not being able
to create high enough penalties.

Closes #1616

jkczyz · 2022-07-14T01:27:30Z

Notably, we use a divisor of 2^30 instead of 2^20 (like the equivalent liquidity penalty) as it allows for more flexibility, and there's not really any reason to worry about us not being able to create high enough penalties.

2^20 is used for an amount penalty, not a liquidity penalty as stated here. Why have two different amount penalties?

TheBlueMatt · 2022-07-14T01:31:21Z

Sorry, that's unclear, we now have too many penalties - I meant the liquidity-amount penalty. Indeed, we could keep it consistent, I'm okay with that, but if you want the number to kick in only very gradually for very large payments 2^20 is just shy of a large enough divisor, I think. Honestly I regret not making the liquidity-amount penalty 2^30 as well, but I figured why not just do 2^30 here.

lightning/src/routing/scoring.rs

jkczyz · 2022-07-14T01:53:57Z

lightning/src/routing/scoring.rs

+	/// A fixed penalty in msats to apply to each channel, multiplied by the payment amount.
+	///
+	/// The purpose of the amount penalty is to avoid having fees dominate the channel cost (i.e.,
+	/// fees plus penalty) for large payments. The penalty is computed as the product of this
+	/// multiplier and `2^30`ths of the payment amount.
+	///
+	/// ie `amount_penalty_multiplier_msat * amount_msat / 2^30`
+	///
+	/// Default value: 8,192 msat
+	pub base_penalty_amount_multiplier_msat: u64,


We should note that this is added to base_penalty_msat as it might surprising that these aren't mutually exclusive. But maybe we should make them so? Workaround is to set either to zero, of course.

Eh, its really obvious and trivial to set it to zero, so I figure let's just leave it as-is.

codecov-commenter · 2022-07-14T02:04:02Z

Codecov Report

Merging #1617 (e9d0117) into main (5023ff0) will increase coverage by 0.29%.
The diff coverage is 100.00%.

❗ Current head e9d0117 differs from pull request most recent head 7f80972. Consider uploading reports for the commit 7f80972 to get more accurate results

@@            Coverage Diff             @@
##             main    #1617      +/-   ##
==========================================
+ Coverage   90.82%   91.11%   +0.29%     
==========================================
  Files          80       80              
  Lines       44643    46373    +1730     
  Branches    44643    46373    +1730     
==========================================
+ Hits        40547    42253    +1706     
- Misses       4096     4120      +24

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	`96.13% <100.00%> (+0.03%)`	⬆️
lightning/src/chain/onchaintx.rs	`93.98% <0.00%> (-0.93%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.26% <0.00%> (+0.15%)`	⬆️
lightning/src/debug_sync.rs	`96.04% <0.00%> (+1.24%)`	⬆️
lightning/src/chain/channelmonitor.rs	`92.47% <0.00%> (+1.50%)`	⬆️
lightning/src/ln/peer_handler.rs	`59.04% <0.00%> (+1.97%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.92% <0.00%> (+2.41%)`	⬆️
lightning-net-tokio/src/lib.rs	`82.88% <0.00%> (+5.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5023ff0...7f80972. Read the comment docs.

TheBlueMatt · 2022-07-15T03:45:34Z

Rebased after dependency's dependency got merged.

tnull

Generally the concept is pretty straight forward I think.

I however see the danger that it is slowly getting harder to keep a good understanding of what goes into a score and how much influence the different factors (should) have. Maybe we could add a bit longer dedicated 'design' comment at the start of scorer.rs that provides a higher level overview and rationale on how the score is comprised and how the user should/could adjust the different penalties?

lightning/src/routing/scoring.rs

TheBlueMatt · 2022-07-19T17:56:04Z

Rebased after merge of dependent PR.

lightning/src/routing/scoring.rs

jkczyz · 2022-07-20T13:44:01Z

lightning/src/routing/scoring.rs

+	/// A fixed penalty in msats to apply to each channel, multiplied by the payment amount, in
+	/// excess of the [`base_penalty_msat`].


Given this field is a multiplier, I'd consider formulating this sentence like the docs for liquidity_penalty_multiplier_msat and amount_penalty_multiplier_msat. The penalty is also variable, not fixed.

Hmm, I'd considered it a "fixed" penalty per-channel, fixed for a given payment/amount, vs variable based on some details about the channel. How about A multiplier used with the payment amount to calculate a fixed penalty applied to each channel, in excess of the base_penalty_msat.

lightning/src/routing/scoring.rs

jkczyz · 2022-07-21T15:08:58Z

lightning/src/routing/scoring.rs

@@ -683,7 +683,7 @@ impl<L: Deref<Target = u64>, T: Time, U: Deref<Target = T>> DirectedChannelLiqui
 		}
 	}

-	/// Computes and combines the liquidity and amount penalties.
+	/// Computes the liquidity penalty from the core and amount penalty multipliers.


Let's rename the function liquidity_penalty_msat and just say "from the parameterized multipliers" here.

Hmm, can we table this until https://github.com/lightningdevkit/rust-lightning/pull/1625/files#diff-b00a115e9303b98772e06346a86a0312aa9790d78a73904eb6a20f938cd6da0fR779 ? There it gets used for both the liquidity and historical penalties, so making the function name say liquidity explicitly makes less sense in that context.

I also find the 'core' terminology a bit confusing here, but fine by me if it will be changed soon anyways. 🤷‍♂️

Sure, I'd see at least drop the "core" terminology as I also find that not obvious.

Sure, I was trying to come up with something that meant "the non-amount-penalty" - any suggestions?

Could use "base" and "proportional" if we're trying to mirror fee_base_msat and fee_proportional_millionths.

Right, then we'd be overloading "base" - I was trying to avoid the term "base" because the params are "base, liquidity, and soon historical", and I wanted to capture the "base" part of the liquidity. I just dropped the list in the comment, maybe that's simplest.

Ah, right. Yeah, just updating the comment should be fine. Could change "base" to "fixed" to avoid overloading "base", but I'm somewhat indifferent on that currently.

lightning/src/routing/scoring.rs

tnull

Just did another pass and LGTM.

tnull · 2022-07-22T08:49:57Z

lightning/src/routing/scoring.rs

@@ -683,7 +683,7 @@ impl<L: Deref<Target = u64>, T: Time, U: Deref<Target = T>> DirectedChannelLiqui
 		}
 	}

-	/// Computes and combines the liquidity and amount penalties.
+	/// Computes the liquidity penalty from the core and amount penalty multipliers.


I also find the 'core' terminology a bit confusing here, but fine by me if it will be changed soon anyways. 🤷‍♂️

There's not much reason to not have a per-hop-per-amount penalty in the `ProbabilisticScorer` to go along with the per-hop penalty to let it scale up to larger amounts, so we add one here. Notably, we use a divisor of 2^30 instead of 2^20 (like the equivalent liquidity penalty) as it allows for more flexibility, and there's not really any reason to worry about us not being able to create high enough penalties. Closes lightningdevkit#1616

This makes our `ProbabilisticScorer` field names more consistent, as we add more types of penalties, referring to a penalty as only the "amount penalty" no longer makes sense - we not have several amount multiplier penalties.

TheBlueMatt · 2022-07-25T19:23:19Z

Squashed without further changes. Changes since a few days ago:

$ git diff-tree -U1 14696cc 7f80972e1
diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index 005df8cd6..a587f53c5 100644
--- a/lightning/src/routing/scoring.rs
+++ b/lightning/src/routing/scoring.rs
@@ -685,3 +685,3 @@ impl<L: Deref<Target = u64>, T: Time, U: Deref<Target = T>> DirectedChannelLiqui
 
-	/// Computes the liquidity penalty from the core and amount penalty multipliers.
+	/// Computes the liquidity penalty from the penalty multipliers.
 	#[inline(always)]

TheBlueMatt · 2022-07-25T22:02:06Z

Hmm, looks like backtrace is causing us CI issues, will look into it after this.

TheBlueMatt added this to the 0.0.110 milestone Jul 14, 2022

TheBlueMatt added the blocked on dependent pr label Jul 14, 2022

jkczyz reviewed Jul 14, 2022

View reviewed changes

tnull self-requested a review July 14, 2022 09:29

TheBlueMatt force-pushed the 2022-07-base-ppm branch from db0e626 to e3e7ead Compare July 15, 2022 03:45

tnull reviewed Jul 15, 2022

View reviewed changes

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

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

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

TheBlueMatt force-pushed the 2022-07-base-ppm branch 2 times, most recently from 471e92e to 09e0ad5 Compare July 19, 2022 17:55

TheBlueMatt removed the blocked on dependent pr label Jul 19, 2022

TheBlueMatt assigned tnull and jkczyz Jul 19, 2022

jkczyz reviewed Jul 19, 2022

View reviewed changes

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

jkczyz reviewed Jul 20, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Jul 21, 2022

Track Channel Liquidity History in ProbabilisticScorer #1625

Merged

jkczyz reviewed Jul 21, 2022

View reviewed changes

tnull reviewed Jul 22, 2022

View reviewed changes

jkczyz previously approved these changes Jul 22, 2022

View reviewed changes

TheBlueMatt dismissed jkczyz’s stale review via e9d0117 July 22, 2022 15:35

TheBlueMatt force-pushed the 2022-07-base-ppm branch from 14696cc to e9d0117 Compare July 22, 2022 15:35

jkczyz previously approved these changes Jul 25, 2022

View reviewed changes

TheBlueMatt added 2 commits July 25, 2022 19:22

TheBlueMatt dismissed jkczyz’s stale review via 7f80972 July 25, 2022 19:22

TheBlueMatt force-pushed the 2022-07-base-ppm branch from e9d0117 to 7f80972 Compare July 25, 2022 19:22

tnull approved these changes Jul 25, 2022

View reviewed changes

jkczyz approved these changes Jul 25, 2022

View reviewed changes

TheBlueMatt merged commit 61b0a90 into lightningdevkit:main Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a per-amount base penalty in the ProbabilisticScorer #1617

Add a per-amount base penalty in the ProbabilisticScorer #1617

TheBlueMatt commented Jul 14, 2022

jkczyz commented Jul 14, 2022

TheBlueMatt commented Jul 14, 2022

jkczyz Jul 14, 2022

TheBlueMatt Jul 15, 2022

codecov-commenter commented Jul 14, 2022 •

edited

TheBlueMatt commented Jul 15, 2022

tnull left a comment •

edited

TheBlueMatt commented Jul 19, 2022

jkczyz Jul 20, 2022

TheBlueMatt Jul 20, 2022

jkczyz Jul 21, 2022

TheBlueMatt Jul 21, 2022

tnull Jul 22, 2022

jkczyz Jul 22, 2022

TheBlueMatt Jul 22, 2022

jkczyz Jul 25, 2022

TheBlueMatt Jul 25, 2022

jkczyz Jul 25, 2022

tnull left a comment •

edited

tnull Jul 22, 2022

TheBlueMatt commented Jul 25, 2022

TheBlueMatt commented Jul 25, 2022

		/// A fixed penalty in msats to apply to each channel, multiplied by the payment amount, in
		/// excess of the [`base_penalty_msat`].

Add a per-amount base penalty in the ProbabilisticScorer #1617

Add a per-amount base penalty in the ProbabilisticScorer #1617

Conversation

TheBlueMatt commented Jul 14, 2022

jkczyz commented Jul 14, 2022

TheBlueMatt commented Jul 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 14, 2022 • edited

Codecov Report

TheBlueMatt commented Jul 15, 2022

tnull left a comment • edited

Choose a reason for hiding this comment

TheBlueMatt commented Jul 19, 2022

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 25, 2022

TheBlueMatt commented Jul 25, 2022

codecov-commenter commented Jul 14, 2022 •

edited

tnull left a comment •

edited

tnull left a comment •

edited