From 866588c20f497c2fc5d2307e9f2313728022bff8 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 10 Oct 2024 18:01:23 -0500 Subject: [PATCH 1/4] 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. --- lightning/src/routing/router.rs | 7 +++-- lightning/src/routing/scoring.rs | 51 ++++++++++++++++++-------------- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index b0a3f34a676..231fc9e9ca7 100644 --- 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 b27a9f97b02..e915f392d53 100644 --- 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, HT: Deref, 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>, 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, ¶ms), 2050); + assert_eq!(scorer.channel_penalty_msat(&candidate, usage, ¶ms), 2048); let usage = ChannelUsage { amount_msat: 1, From 7fa6770a7af85580802a3c356d0990e44db4ac67 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 10 Oct 2024 18:20:25 -0500 Subject: [PATCH 2/4] Correct base_penalty_amount_multiplier_msat docs Commit df52da7b31494c7ec77a705cca4c44bc840f8a95 modified ProbabilisticScorer to apply some penalty amount multipliers to the total amount flowing over the channel. However, the commit updated the docs for base_penalty_amount_multiplier_msat even though that behavior didn't change. This commit reverts those docs. --- lightning/src/routing/scoring.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index e915f392d53..de89dd96692 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -491,13 +491,12 @@ pub struct ProbabilisticScoringFeeParameters { /// Default value: 500 msat pub base_penalty_msat: u64, - /// A multiplier used with the total amount flowing over a channel to calculate a fixed penalty - /// applied to each channel, in excess of the [`base_penalty_msat`]. + /// A multiplier used with the payment amount to calculate a fixed penalty applied to each + /// channel, in excess of the [`base_penalty_msat`]. /// /// 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 total amount flowing over a channel (i.e. the payment - /// amount plus the amount of any other HTLCs flowing we sent over the same channel). + /// multiplier and `2^30`ths of the payment amount. /// /// ie `base_penalty_amount_multiplier_msat * amount_msat / 2^30` /// From 572fab75918c8165b6de4cb947c25f0e124e6c63 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 16 Oct 2024 15:18:17 -0500 Subject: [PATCH 3/4] Correct comments in avoids_saturating_channels --- lightning/src/routing/router.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 231fc9e9ca7..299d3b1810c 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -7357,7 +7357,7 @@ mod tests { let decay_params = ProbabilisticScoringDecayParameters::default(); let scorer = ProbabilisticScorer::new(decay_params, &*network_graph, Arc::clone(&logger)); - // Set the fee on channel 13 to 100% to match channel 4 giving us two equivalent paths (us + // Set the fee on channel 13 to 0% to match channel 4 giving us two equivalent paths (us // -> node 7 -> node2 and us -> node 1 -> node 2) which we should balance over. update_channel(&gossip_sync, &secp_ctx, &privkeys[1], UnsignedChannelUpdate { chain_hash: ChainHash::using_genesis_block(Network::Testnet), From 0305000850f125a42b192a2632c60670fd43725b Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 18 Oct 2024 17:42:18 -0500 Subject: [PATCH 4/4] Use total_inflight_amount_msat for probability fns Rename parameters used when calculating success probability to make it clear that the total mount in-flight should be used rather than the payment amount. --- lightning/src/routing/scoring.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lightning/src/routing/scoring.rs b/lightning/src/routing/scoring.rs index de89dd96692..11fa94cd43e 100644 --- a/lightning/src/routing/scoring.rs +++ b/lightning/src/routing/scoring.rs @@ -1074,26 +1074,30 @@ fn three_f64_pow_3(a: f64, b: f64, c: f64) -> (f64, f64, f64) { /// /// Must not return a numerator or denominator greater than 2^31 for arguments less than 2^31. /// +/// `total_inflight_amount_msat` includes the amount of the HTLC and any HTLCs in flight over the +/// channel. +/// /// min_zero_implies_no_successes signals that a `min_liquidity_msat` of 0 means we've not /// (recently) seen an HTLC successfully complete over this channel. #[inline(always)] fn success_probability( - amount_msat: u64, min_liquidity_msat: u64, max_liquidity_msat: u64, capacity_msat: u64, - params: &ProbabilisticScoringFeeParameters, min_zero_implies_no_successes: bool, + total_inflight_amount_msat: u64, min_liquidity_msat: u64, max_liquidity_msat: u64, + capacity_msat: u64, params: &ProbabilisticScoringFeeParameters, + min_zero_implies_no_successes: bool, ) -> (u64, u64) { - debug_assert!(min_liquidity_msat <= amount_msat); - debug_assert!(amount_msat < max_liquidity_msat); + debug_assert!(min_liquidity_msat <= total_inflight_amount_msat); + debug_assert!(total_inflight_amount_msat < max_liquidity_msat); debug_assert!(max_liquidity_msat <= capacity_msat); let (numerator, mut denominator) = if params.linear_success_probability { - (max_liquidity_msat - amount_msat, + (max_liquidity_msat - total_inflight_amount_msat, (max_liquidity_msat - min_liquidity_msat).saturating_add(1)) } else { let capacity = capacity_msat as f64; let min = (min_liquidity_msat as f64) / capacity; let max = (max_liquidity_msat as f64) / capacity; - let amount = (amount_msat as f64) / capacity; + let amount = (total_inflight_amount_msat as f64) / capacity; // Assume the channel has a probability density function of (x - 0.5)^2 for values from // 0 to 1 (where 1 is the channel's full capacity). The success probability given some @@ -1779,7 +1783,7 @@ mod bucketed_history { #[inline] pub(super) fn calculate_success_probability_times_billion( - &self, params: &ProbabilisticScoringFeeParameters, amount_msat: u64, + &self, params: &ProbabilisticScoringFeeParameters, total_inflight_amount_msat: u64, capacity_msat: u64 ) -> Option { // If historical penalties are enabled, we try to calculate a probability of success @@ -1789,7 +1793,7 @@ mod bucketed_history { // state). For each pair, we calculate the probability as if the bucket's corresponding // min- and max- liquidity bounds were our current liquidity bounds and then multiply // that probability by the weight of the selected buckets. - let payment_pos = amount_to_pos(amount_msat, capacity_msat); + let payment_pos = amount_to_pos(total_inflight_amount_msat, capacity_msat); if payment_pos >= POSITION_TICKS { return None; } let min_liquidity_offset_history_buckets =