Add a per-amount base penalty in the ProbabilisticScorer by TheBlueMatt · Pull Request #1617 · lightningdevkit/rust-lightning

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.

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

+	/// 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?

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

Rebased after merge of dependent PR.

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

+	/// 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.

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

 	}

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

tnull

Just did another pass and LGTM.

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

 	}

-	/// 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. Closeslightningdevkit#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 · 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.

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

+	/// 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?

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

Rebased after merge of dependent PR.

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

+	/// 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.

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

 	}

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