Skip to content

Don't over-penalize channels with inflight HTLCs #3356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 19, 2024
Merged

Don't over-penalize channels with inflight HTLCs #3356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
866588c
Don't over-penalize channels with inflight HTLCs
jkczyz Oct 10, 2024
7fa6770
Correct base_penalty_amount_multiplier_msat docs
jkczyz Oct 10, 2024
572fab7
Correct comments in avoids_saturating_channels
jkczyz Oct 16, 2024
0305000
Use total_inflight_amount_msat for probability fns
jkczyz Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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);
Expand Down
78 changes: 44 additions & 34 deletions lightning/src/routing/scoring.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
///
Expand All @@ -524,14 +523,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`
///
Expand Down Expand Up @@ -560,15 +559,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
Expand Down Expand Up @@ -1076,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: there is a typo in last commit's description,
mount -> amount.

/// 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
Expand Down Expand Up @@ -1138,14 +1140,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.
Expand All @@ -1155,8 +1161,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
Expand All @@ -1171,7 +1179,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,
Expand All @@ -1184,7 +1192,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);
Expand All @@ -1195,8 +1204,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,
Copy link
Contributor

@G8XSU G8XSU Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doubt: Can you explain why liquidity_penalty only needs to be applied over htlc_amount_msat and not total_inflight?
Previously, this penalty would increase with each new htlc being routed through the same channel, but now, every htlc(whether the 1st or the 10th) will be treated equal for liquidity penalty iiuc.

I understand it might not be working as designed/intended at first but how was it wrong?
Is it because we don't want to penalize twice(once for liquidity_penalty and next for success_probability), since success_probability is already non-linear for increasing amounts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, we want to use the total amount when computing the success probability since in-flight HTLCs will affect this. However, when computing the actual penalties we use the payment amount (i.e., amount_msat) and the success probability. So each successive HTLC in-flight would cause the penalty to increase since the success probability would decrease (assuming the total amount is above the minimum liquidity estimate).

That said, as written, our docs may need to be clearer about what amount is used to compute the success probability (total amount) and what amount is used for the penalty (payment amount w/ success probability).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, as written, our docs may need to be clearer about what amount is used to compute the success probability (total amount) and what amount is used for the penalty (payment amount w/ success probability).

Updated the docs in the latest push, BTW.

Expand Down Expand Up @@ -1353,13 +1364,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)
}
Expand Down Expand Up @@ -1773,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<u64> {
// If historical penalties are enabled, we try to calculate a probability of success
Expand All @@ -1783,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 =
Expand Down Expand Up @@ -3269,7 +3279,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,
Expand Down
Loading