Skip to content
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

Jump to bottom

Avoid reusing just-failed channels in the router, making the impossibility penalty configurable #1600

Merged
merged 3 commits into from Jul 15, 2022
Merged

Avoid reusing just-failed channels in the router, making the impossibility penalty configurable #1600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93e645d
Track channels which a given payment part failed to traverse
TheBlueMatt Jul 6, 2022
ec2b1ce
Make the `ProbabilisticScorer` impossibility penalty configurable
TheBlueMatt Jul 6, 2022
a3547e2
Change default "impossibility penalty" to one Bitcoin
TheBlueMatt Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Expand Up @@ -3894,7 +3894,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
return;
}
mem::drop(channel_state_lock);
let retry = if let Some(payment_params_data) = payment_params {
let mut retry = if let Some(payment_params_data) = payment_params {
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
Some(RouteParameters {
payment_params: payment_params_data.clone(),
Expand Down Expand Up @@ -3930,6 +3930,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// TODO: If we decided to blame ourselves (or one of our channels) in
// process_onion_failure we should close that channel as it implies our
// next-hop is needlessly blaming us!
if let Some(scid) = short_channel_id {
retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
}
events::Event::PaymentPathFailed {
payment_id: Some(payment_id),
payment_hash: payment_hash.clone(),
Expand Down Expand Up @@ -3959,14 +3962,16 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// ChannelDetails.
// TODO: For non-temporary failures, we really should be closing the
// channel here as we apparently can't relay through them anyway.
let scid = path.first().unwrap().short_channel_id;
retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
events::Event::PaymentPathFailed {
payment_id: Some(payment_id),
payment_hash: payment_hash.clone(),
rejected_by_dest: path.len() == 1,
network_update: None,
all_paths_failed,
path: path.clone(),
short_channel_id: Some(path.first().unwrap().short_channel_id),
short_channel_id: Some(scid),
retry,
#[cfg(test)]
error_code: Some(*failure_code),
Expand Down
5 changes: 4 additions & 1 deletion lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Expand Up @@ -1492,7 +1492,7 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>(
let mut events = node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
let expected_payment_id = match events.pop().unwrap() {
Event::PaymentPathFailed { payment_hash, rejected_by_dest, path, retry, payment_id, network_update,
Event::PaymentPathFailed { payment_hash, rejected_by_dest, path, retry, payment_id, network_update, short_channel_id,
#[cfg(test)]
error_code,
#[cfg(test)]
Expand All @@ -1502,6 +1502,9 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>(
assert!(retry.is_some(), "expected retry.is_some()");
assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path");
assert_eq!(retry.as_ref().unwrap().payment_params.payee_pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path");
if let Some(scid) = short_channel_id {
assert!(retry.as_ref().unwrap().payment_params.previously_failed_channels.contains(&scid));
}

#[cfg(test)]
{
Expand Down
54 changes: 48 additions & 6 deletions lightning/src/routing/router.rs
View file
Open in desktop
Expand Up @@ -240,6 +240,11 @@ pub struct PaymentParameters {
///
/// Default value: 1
pub max_channel_saturation_power_of_half: u8,

/// A list of SCIDs which this payment was previously attempted over and which caused the
/// payment to fail. Future attempts for the same payment shouldn't be relayed through any of
/// these SCIDs.
pub previously_failed_channels: Vec<u64>,
}

impl_writeable_tlv_based!(PaymentParameters, {
Expand All @@ -250,6 +255,7 @@ impl_writeable_tlv_based!(PaymentParameters, {
(4, route_hints, vec_type),
(5, max_channel_saturation_power_of_half, (default_value, 1)),
(6, expiry_time, option),
(7, previously_failed_channels, vec_type),
});

impl PaymentParameters {
Expand All @@ -263,6 +269,7 @@ impl PaymentParameters {
max_total_cltv_expiry_delta: DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA,
max_path_count: DEFAULT_MAX_PATH_COUNT,
max_channel_saturation_power_of_half: 1,
previously_failed_channels: Vec::new(),
}
}

Expand Down Expand Up @@ -1002,7 +1009,7 @@ where L::Target: Logger {
let contributes_sufficient_value = available_value_contribution_msat >= minimal_value_contribution_msat;
// Do not consider candidate hops that would exceed the maximum path length.
let path_length_to_node = $next_hops_path_length + 1;
let doesnt_exceed_max_path_length = path_length_to_node <= MAX_PATH_LENGTH_ESTIMATE;
let exceeds_max_path_length = path_length_to_node > MAX_PATH_LENGTH_ESTIMATE;

// Do not consider candidates that exceed the maximum total cltv expiry limit.
// In order to already account for some of the privacy enhancing random CLTV
Expand All @@ -1013,7 +1020,7 @@ where L::Target: Logger {
.unwrap_or(payment_params.max_total_cltv_expiry_delta - final_cltv_expiry_delta);
let hop_total_cltv_delta = ($next_hops_cltv_delta as u32)
.saturating_add($candidate.cltv_expiry_delta());
let doesnt_exceed_cltv_delta_limit = hop_total_cltv_delta <= max_total_cltv_expiry_delta;
let exceeds_cltv_delta_limit = hop_total_cltv_delta > max_total_cltv_expiry_delta;

let value_contribution_msat = cmp::min(available_value_contribution_msat, $next_hops_value_contribution);
// Includes paying fees for the use of the following channels.
Expand All @@ -1033,15 +1040,19 @@ where L::Target: Logger {
(amount_to_transfer_over_msat < $next_hops_path_htlc_minimum_msat &&
recommended_value_msat > $next_hops_path_htlc_minimum_msat));

let payment_failed_on_this_channel =
payment_params.previously_failed_channels.contains(&short_channel_id);

// If HTLC minimum is larger than the amount we're going to transfer, we shouldn't
// bother considering this channel. If retrying with recommended_value_msat may
// allow us to hit the HTLC minimum limit, set htlc_minimum_limit so that we go
// around again with a higher amount.
if contributes_sufficient_value && doesnt_exceed_max_path_length &&
doesnt_exceed_cltv_delta_limit && may_overpay_to_meet_path_minimum_msat {
if !contributes_sufficient_value || exceeds_max_path_length ||
exceeds_cltv_delta_limit || payment_failed_on_this_channel {
// Path isn't useful, ignore it and move on.
} else if may_overpay_to_meet_path_minimum_msat {
hit_minimum_limit = true;
} else if contributes_sufficient_value && doesnt_exceed_max_path_length &&
doesnt_exceed_cltv_delta_limit && over_path_minimum_msat {
} else if over_path_minimum_msat {
// Note that low contribution here (limited by available_liquidity_msat)
// might violate htlc_minimum_msat on the hops which are next along the
// payment path (upstream to the payee). To avoid that, we recompute
Expand Down Expand Up @@ -1993,6 +2004,8 @@ mod tests {
use prelude::*;
use sync::{self, Arc};

use core::convert::TryInto;

fn get_channel_details(short_channel_id: Option<u64>, node_id: PublicKey,
features: InitFeatures, outbound_capacity_msat: u64) -> channelmanager::ChannelDetails {
channelmanager::ChannelDetails {
Expand Down Expand Up @@ -5573,6 +5586,35 @@ mod tests {
}
}

#[test]
fn avoids_recently_failed_paths() {
// Ensure that the router always avoids all of the `previously_failed_channels` channels by
// randomly inserting channels into it until we can't find a route anymore.
let (secp_ctx, network, _, _, logger) = build_graph();
let (_, our_id, _, nodes) = get_nodes(&secp_ctx);
let network_graph = network.read_only();

let scorer = test_utils::TestScorer::with_penalty(0);
let mut payment_params = PaymentParameters::from_node_id(nodes[6]).with_route_hints(last_hops(&nodes))
.with_max_path_count(1);
let keys_manager = test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
let random_seed_bytes = keys_manager.get_secure_random_bytes();

// We should be able to find a route initially, and then after we fail a few random
// channels eventually we won't be able to any longer.
assert!(get_route(&our_id, &payment_params, &network_graph, None, 100, 0, Arc::clone(&logger), &scorer, &random_seed_bytes).is_ok());
loop {
if let Ok(route) = get_route(&our_id, &payment_params, &network_graph, None, 100, 0, Arc::clone(&logger), &scorer, &random_seed_bytes) {
TheBlueMatt marked this conversation as resolved.
Show resolved Hide resolved
for chan in route.paths[0].iter() {
assert!(!payment_params.previously_failed_channels.contains(&chan.short_channel_id));
}
let victim = (u64::from_ne_bytes(random_seed_bytes[0..8].try_into().unwrap()) as usize)
tnull marked this conversation as resolved.
Show resolved Hide resolved
% route.paths[0].len();
payment_params.previously_failed_channels.push(route.paths[0][victim].short_channel_id);
} else { break; }
}
}

#[test]
fn limits_path_length() {
let (secp_ctx, network, _, _, logger) = build_line_graph();
Expand Down
60 changes: 42 additions & 18 deletions lightning/src/routing/scoring.rs
View file
Open in desktop
Expand Up @@ -394,6 +394,25 @@ pub struct ProbabilisticScoringParameters {
///
/// Default value: 250 msat
pub anti_probing_penalty_msat: u64,

/// This penalty is applied when the amount we're attempting to send over a channel exceeds our
/// current estimate of the channel's available liquidity.
///
/// Note that in this case all other penalties, including the
/// [`liquidity_penalty_multiplier_msat`] and [`amount_penalty_multiplier_msat`]-based
/// penalties, as well as the [`base_penalty_msat`] and the [`anti_probing_penalty_msat`], if
/// applicable, are still included in the overall penalty.
///
/// If you wish to avoid creating paths with such channels entirely, setting this to a value of
/// `u64::max_value()` will guarantee that.
///
/// Default value: 1_0000_0000_000 msat (1 Bitcoin)
tnull marked this conversation as resolved.
Show resolved Hide resolved
///
/// [`liquidity_penalty_multiplier_msat`]: Self::liquidity_penalty_multiplier_msat
/// [`amount_penalty_multiplier_msat`]: Self::amount_penalty_multiplier_msat
/// [`base_penalty_msat`]: Self::base_penalty_msat
/// [`anti_probing_penalty_msat`]: Self::anti_probing_penalty_msat
pub considered_impossible_penalty_msat: u64,
}

/// Accounting for channel liquidity balance uncertainty.
Expand Down Expand Up @@ -522,6 +541,7 @@ impl ProbabilisticScoringParameters {
amount_penalty_multiplier_msat: 0,
manual_node_penalties: HashMap::new(),
anti_probing_penalty_msat: 0,
considered_impossible_penalty_msat: 0,
}
}

Expand All @@ -543,6 +563,7 @@ impl Default for ProbabilisticScoringParameters {
amount_penalty_multiplier_msat: 256,
manual_node_penalties: HashMap::new(),
anti_probing_penalty_msat: 250,
considered_impossible_penalty_msat: 1_0000_0000_000,
tnull marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down Expand Up @@ -620,17 +641,12 @@ impl<L: Deref<Target = u64>, T: Time, U: Deref<Target = T>> DirectedChannelLiqui
if amount_msat <= min_liquidity_msat {
0
} else if amount_msat >= max_liquidity_msat {
if amount_msat > max_liquidity_msat {
u64::max_value()
} else if max_liquidity_msat != self.capacity_msat {
// Avoid using the failed channel on retry.
u64::max_value()
} else {
// Equivalent to hitting the else clause below with the amount equal to the
// effective capacity and without any certainty on the liquidity upper bound.
let negative_log10_times_2048 = NEGATIVE_LOG10_UPPER_BOUND * 2048;
self.combined_penalty_msat(amount_msat, negative_log10_times_2048, params)
}
// 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.
Comment on lines +644 to +646
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment needs to be updated now since it's not only when equal to the effective capacity. Seems like the earlier change that added this now removed logic is what caused calculates_log10_without_overflowing_u64_max_value to not exercise the correct code path as mentioned earlier on that test (i.e., it doesn't hit the negative_log10_times_2048 case below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I read the comment differently - I read the comment to say "the calculation we're doing here is equivalent to..." which is still true, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah, you're right!

let negative_log10_times_2048 = NEGATIVE_LOG10_UPPER_BOUND * 2048;
self.combined_penalty_msat(amount_msat, negative_log10_times_2048, params)
.saturating_add(params.considered_impossible_penalty_msat)
Comment on lines +647 to +649
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to do the max of these two rather than adding? Is the idea that we want this to be >= anything given in the else clause?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, that's the idea. I suppose we could do a max. Originally I had it not adding the combined_penalty call at all but that seemed to brittle to deal with so switched to adding. I don't honestly have a strong opinion between adding and max, either way the docs can tell users what's going on, but for max i kinda worry users will acidentally set it too low and get no penalty here, which seems strange?

Copy link
Contributor

Choose a reason for hiding this comment

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

They would also need to set the other params lower, though, since max would select the combined penalty over the considered_impossible_penalty_msat if the latter were too low. So it's all kinda relative. Don't feel too strongly either though note that max may make debugging a little easier as otherwise you may need to mentally subtract some combined penalty if it is not obvious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, sure, I just meant it'd be easy for a user to look at the field and think "okay, let me pick something a bit higher than the liquidity offset, forget to multiply by 2 or whatever, and end up with a penalty equal to the liquidity penalty, which seems wrong? I dunno, I'm happy to mentally convert when we're debugging. Unless you feel strongly I'd suggest we leave it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, sure, I just meant it'd be easy for a user to look at the field and think "okay, let me pick something a bit higher than the liquidity offset, forget to multiply by 2 or whatever, and end up with a penalty equal to the liquidity penalty, which seems wrong?

Plus base and amount penalty, FWIW.

I dunno, I'm happy to mentally convert when we're debugging. Unless you feel strongly I'd suggest we leave it.

Sure we can leave it. Though one thing I just realized is that either way now the penalty will be variable across channels depending on the amount. Maybe less so when using max but maybe that's an argument in favor of adding. That way if left to choose only channels exceeding the maximum liquidity, we'd prefer ones that would otherwise be penalized less.

} else {
let numerator = (max_liquidity_msat - amount_msat).saturating_add(1);
let denominator = (max_liquidity_msat - min_liquidity_msat).saturating_add(1);
Expand Down Expand Up @@ -1624,7 +1640,7 @@ mod tests {
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 0);
let usage = ChannelUsage { amount_msat: 102_400, ..usage };
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 47);
let usage = ChannelUsage { amount_msat: 1_024_000, ..usage };
let usage = ChannelUsage { amount_msat: 1_023_999, ..usage };
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);

let usage = ChannelUsage {
Expand Down Expand Up @@ -1654,6 +1670,7 @@ mod tests {
let network_graph = network_graph(&logger);
let params = ProbabilisticScoringParameters {
liquidity_penalty_multiplier_msat: 1_000,
considered_impossible_penalty_msat: u64::max_value(),
..ProbabilisticScoringParameters::zero_penalty()
};
let scorer = ProbabilisticScorer::new(params, &network_graph, &logger)
Expand Down Expand Up @@ -1745,6 +1762,7 @@ mod tests {
let network_graph = network_graph(&logger);
let params = ProbabilisticScoringParameters {
liquidity_penalty_multiplier_msat: 1_000,
considered_impossible_penalty_msat: u64::max_value(),
..ProbabilisticScoringParameters::zero_penalty()
};
let mut scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
Expand Down Expand Up @@ -1811,6 +1829,7 @@ mod tests {
let params = ProbabilisticScoringParameters {
liquidity_penalty_multiplier_msat: 1_000,
liquidity_offset_half_life: Duration::from_secs(10),
considered_impossible_penalty_msat: u64::max_value(),
..ProbabilisticScoringParameters::zero_penalty()
};
let mut scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
Expand All @@ -1820,10 +1839,10 @@ mod tests {
let usage = ChannelUsage {
amount_msat: 0,
inflight_htlc_msat: 0,
effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024, htlc_maximum_msat: Some(1_000) },
effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024, htlc_maximum_msat: Some(1_024) },
};
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 0);
let usage = ChannelUsage { amount_msat: 1_024, ..usage };
let usage = ChannelUsage { amount_msat: 1_023, ..usage };
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);

scorer.payment_path_failed(&payment_path_for_amount(768).iter().collect::<Vec<_>>(), 42);
Expand Down Expand Up @@ -1867,20 +1886,20 @@ mod tests {
let usage = ChannelUsage { amount_msat: 1_023, ..usage };
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
let usage = ChannelUsage { amount_msat: 1_024, ..usage };
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), u64::max_value());

// Fully decay liquidity upper bound.
SinceEpoch::advance(Duration::from_secs(10));
let usage = ChannelUsage { amount_msat: 0, ..usage };
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 0);
let usage = ChannelUsage { amount_msat: 1_024, ..usage };
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), u64::max_value());

SinceEpoch::advance(Duration::from_secs(10));
let usage = ChannelUsage { amount_msat: 0, ..usage };
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 0);
let usage = ChannelUsage { amount_msat: 1_024, ..usage };
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), 2_000);
assert_eq!(scorer.channel_penalty_msat(42, &source, &target, usage), u64::max_value());
}

#[test]
Expand Down Expand Up @@ -1965,6 +1984,7 @@ mod tests {
let params = ProbabilisticScoringParameters {
liquidity_penalty_multiplier_msat: 1_000,
liquidity_offset_half_life: Duration::from_secs(10),
considered_impossible_penalty_msat: u64::max_value(),
..ProbabilisticScoringParameters::zero_penalty()
};
let mut scorer = ProbabilisticScorer::new(params.clone(), &network_graph, &logger);
Expand Down Expand Up @@ -2001,6 +2021,7 @@ mod tests {
let params = ProbabilisticScoringParameters {
liquidity_penalty_multiplier_msat: 1_000,
liquidity_offset_half_life: Duration::from_secs(10),
considered_impossible_penalty_msat: u64::max_value(),
..ProbabilisticScoringParameters::zero_penalty()
};
let mut scorer = ProbabilisticScorer::new(params.clone(), &network_graph, &logger);
Expand Down Expand Up @@ -2171,7 +2192,10 @@ mod tests {
fn accounts_for_inflight_htlc_usage() {
let logger = TestLogger::new();
let network_graph = network_graph(&logger);
let params = ProbabilisticScoringParameters::default();
let params = ProbabilisticScoringParameters {
considered_impossible_penalty_msat: u64::max_value(),
..ProbabilisticScoringParameters::zero_penalty()
};
let scorer = ProbabilisticScorer::new(params, &network_graph, &logger);
let source = source_node_id();
let target = target_node_id();
Expand Down