Don't over-penalize channels with inflight HTLCs

Commit df52da7b31494c7ec77a705cca4c44bc840f8a95 modified
ProbabilisticScorer to apply some penalty amount multipliers (e.g.,
liquidity_penalty_amount_multiplier_msat) to the total amount flowing
over the channel (i.e., including inflight HTLCs), not just the payment
in question. This led to over-penalizing in-use channels. Instead, only
apply the total amount when calculating success probability.

diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index b0a3f34a6761a59320e23689dde91766ed348c7d..231fc9e9ca7bf69c8f646a906de841b610372028 100644 (file)
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -7392,9 +7392,12 @@ mod tests {
                        .unwrap();
                let random_seed_bytes = [42; 32];
 
-               // 100,000 sats is less than the available liquidity on each channel, set above.
+               // 75,000 sats is less than the available liquidity on each channel, set above, when
+               // applying max_channel_saturation_power_of_half. This value also ensures the cost of paths
+               // considered when applying max_channel_saturation_power_of_half is less than the cost of
+               // those when it is not applied.
                let route_params = RouteParameters::from_payment_params_and_value(
-                       payment_params, 100_000_000);
+                       payment_params, 75_000_000);
                let route = get_route(&our_id, &route_params, &network_graph.read_only(), None,
                        Arc::clone(&logger), &scorer, &ProbabilisticScoringFeeParameters::default(), &random_seed_bytes).unwrap();
                assert_eq!(route.paths.len(), 2);

diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs
index b27a9f97b02664507a7369e8d18b3245d18e0eb2..e915f392d53ced81e6a592fd1b963a4f0cddd8ec 100644 (file)
--- a/lightning/src/routing/scoring.rs
+++ b/lightning/src/routing/scoring.rs
@@ -524,14 +524,14 @@ pub struct ProbabilisticScoringFeeParameters {
        /// [`liquidity_offset_half_life`]: ProbabilisticScoringDecayParameters::liquidity_offset_half_life
        pub liquidity_penalty_multiplier_msat: u64,
 
-       /// A multiplier used in conjunction with the total amount flowing over a channel and the
-       /// negative `log10` of the channel's success probability for the payment, as determined by our
-       /// latest estimates of the channel's liquidity, to determine the amount penalty.
+       /// A multiplier used in conjunction with the payment amount and the negative `log10` of the
+       /// channel's success probability for the total amount flowing over a channel, as determined by
+       /// our latest estimates of the channel's liquidity, to determine the amount penalty.
        ///
        /// 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^20`ths of the amount flowing over this channel, weighted by the negative
-       /// `log10` of the success probability.
+       /// multiplier and `2^20`ths of the payment amount, weighted by the negative `log10` of the
+       /// success probability.
        ///
        /// `-log10(success_probability) * liquidity_penalty_amount_multiplier_msat * amount_msat / 2^20`
        ///
@@ -560,15 +560,14 @@ pub struct ProbabilisticScoringFeeParameters {
        /// [`liquidity_penalty_multiplier_msat`]: Self::liquidity_penalty_multiplier_msat
        pub historical_liquidity_penalty_multiplier_msat: u64,
 
-       /// A multiplier used in conjunction with the total amount flowing over a channel and the
-       /// negative `log10` of the channel's success probability for the payment, as determined based
-       /// on the history of our estimates of the channel's available liquidity, to determine a
+       /// A multiplier used in conjunction with the payment amount and the negative `log10` of the
+       /// channel's success probability for the total amount flowing over a channel, as determined
+       /// based on the history of our estimates of the channel's available liquidity, to determine a
        /// penalty.
        ///
        /// The purpose of the amount penalty is to avoid having fees dominate the channel cost for
        /// large payments. The penalty is computed as the product of this multiplier and `2^20`ths
-       /// of the amount flowing over this channel, weighted by the negative `log10` of the success
-       /// probability.
+       /// of the payment amount, weighted by the negative `log10` of the success probability.
        ///
        /// This penalty is similar to [`liquidity_penalty_amount_multiplier_msat`], however, instead
        /// of using only our latest estimate for the current liquidity available in the channel, it
@@ -1138,14 +1137,18 @@ impl<L: Deref<Target = u64>, HT: Deref<Target = HistoricalLiquidityTracker>, T:
 DirectedChannelLiquidity< L, HT, T> {
        /// Returns a liquidity penalty for routing the given HTLC `amount_msat` through the channel in
        /// this direction.
-       fn penalty_msat(&self, amount_msat: u64, score_params: &ProbabilisticScoringFeeParameters) -> u64 {
+       fn penalty_msat(
+               &self, amount_msat: u64, inflight_htlc_msat: u64,
+               score_params: &ProbabilisticScoringFeeParameters,
+       ) -> u64 {
+               let total_inflight_amount_msat = amount_msat.saturating_add(inflight_htlc_msat);
                let available_capacity = self.capacity_msat;
                let max_liquidity_msat = self.max_liquidity_msat();
                let min_liquidity_msat = core::cmp::min(self.min_liquidity_msat(), max_liquidity_msat);
 
-               let mut res = if amount_msat <= min_liquidity_msat {
+               let mut res = if total_inflight_amount_msat <= min_liquidity_msat {
                        0
-               } else if amount_msat >= max_liquidity_msat {
+               } else if total_inflight_amount_msat >= max_liquidity_msat {
                        // Equivalent to hitting the else clause below with the amount equal to the effective
                        // capacity and without any certainty on the liquidity upper bound, plus the
                        // impossibility penalty.
@@ -1155,8 +1158,10 @@ DirectedChannelLiquidity< L, HT, T> {
                                        score_params.liquidity_penalty_amount_multiplier_msat)
                                .saturating_add(score_params.considered_impossible_penalty_msat)
                } else {
-                       let (numerator, denominator) = success_probability(amount_msat,
-                               min_liquidity_msat, max_liquidity_msat, available_capacity, score_params, false);
+                       let (numerator, denominator) = success_probability(
+                               total_inflight_amount_msat, min_liquidity_msat, max_liquidity_msat,
+                               available_capacity, score_params, false,
+                       );
                        if denominator - numerator < denominator / PRECISION_LOWER_BOUND_DENOMINATOR {
                                // If the failure probability is < 1.5625% (as 1 - numerator/denominator < 1/64),
                                // don't bother trying to use the log approximation as it gets too noisy to be
@@ -1171,7 +1176,7 @@ DirectedChannelLiquidity< L, HT, T> {
                        }
                };
 
-               if amount_msat >= available_capacity {
+               if total_inflight_amount_msat >= available_capacity {
                        // We're trying to send more than the capacity, use a max penalty.
                        res = res.saturating_add(Self::combined_penalty_msat(amount_msat,
                                NEGATIVE_LOG10_UPPER_BOUND * 2048,
@@ -1184,7 +1189,8 @@ DirectedChannelLiquidity< L, HT, T> {
                   score_params.historical_liquidity_penalty_amount_multiplier_msat != 0 {
                        if let Some(cumulative_success_prob_times_billion) = self.liquidity_history
                                .calculate_success_probability_times_billion(
-                                       score_params, amount_msat, self.capacity_msat)
+                                       score_params, total_inflight_amount_msat, self.capacity_msat
+                               )
                        {
                                let historical_negative_log10_times_2048 =
                                        log_approx::negative_log10_times_2048(cumulative_success_prob_times_billion + 1, 1024 * 1024 * 1024);
@@ -1195,8 +1201,10 @@ DirectedChannelLiquidity< L, HT, T> {
                                // If we don't have any valid points (or, once decayed, we have less than a full
                                // point), redo the non-historical calculation with no liquidity bounds tracked and
                                // the historical penalty multipliers.
-                               let (numerator, denominator) = success_probability(amount_msat, 0,
-                                       available_capacity, available_capacity, score_params, true);
+                               let (numerator, denominator) = success_probability(
+                                       total_inflight_amount_msat, 0, available_capacity, available_capacity,
+                                       score_params, true,
+                               );
                                let negative_log10_times_2048 =
                                        log_approx::negative_log10_times_2048(numerator, denominator);
                                res = res.saturating_add(Self::combined_penalty_msat(amount_msat, negative_log10_times_2048,
@@ -1353,13 +1361,12 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref> ScoreLookUp for Probabilistic
                        _ => {},
                }
 
-               let amount_msat = usage.amount_msat.saturating_add(usage.inflight_htlc_msat);
                let capacity_msat = usage.effective_capacity.as_msat();
                self.channel_liquidities
                        .get(scid)
                        .unwrap_or(&ChannelLiquidity::new(Duration::ZERO))
                        .as_directed(&source, &target, capacity_msat)
-                       .penalty_msat(amount_msat, score_params)
+                       .penalty_msat(usage.amount_msat, usage.inflight_htlc_msat, score_params)
                        .saturating_add(anti_probing_penalty_msat)
                        .saturating_add(base_penalty_msat)
        }
@@ -3269,7 +3276,7 @@ mod tests {
                                short_channel_id: 42,
                        });
 
-                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 2050);
+                       assert_eq!(scorer.channel_penalty_msat(&candidate, usage, &params), 2048);
 
                        let usage = ChannelUsage {
                                amount_msat: 1,