Skip to content

Fix per-path minimal value contribution during route finding #1526

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 2 commits into from
Jun 13, 2022
Merged

Fix per-path minimal value contribution during route finding #1526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13b7cd5
Fix min. contrib. depending on `max_mpp_path_count`
tnull Jun 7, 2022
1dfabcb
Add failure test cases for `max_mpp_path_count`.
tnull Jun 9, 2022
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
6 changes: 0 additions & 6 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2529,12 +2529,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
if route.paths.len() < 1 {
return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"}));
}
if route.paths.len() > 10 {
// This limit is completely arbitrary - there aren't any real fundamental path-count
// limits. After we support retrying individual paths we should likely bump this, but
// for now more than 10 paths likely carries too much one-path failure.
return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "Sending over more than 10 paths is not currently supported"}));
}
if payment_secret.is_none() && route.paths.len() > 1 {
return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError{err: "Payment secret is required for multi-path payments".to_string()}));
}
Expand Down
110 changes: 76 additions & 34 deletions lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,11 @@ impl_writeable_tlv_based!(RouteParameters, {
/// Maximum total CTLV difference we allow for a full payment path.
pub const DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA: u32 = 1008;

/// Maximum number of paths we allow an MPP payment to have.
// The default limit is currently set rather arbitrary - there aren't any real fundamental path-count
// limits, but for now more than 10 paths likely carries too much one-path failure.
pub const DEFAULT_MAX_MPP_PATH_COUNT: u8 = 10;

// The median hop CLTV expiry delta currently seen in the network.
const MEDIAN_HOP_CLTV_EXPIRY_DELTA: u32 = 40;

Expand Down Expand Up @@ -214,13 +219,19 @@ pub struct PaymentParameters {
pub expiry_time: Option<u64>,

/// The maximum total CLTV delta we accept for the route.
/// Defaults to [`DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA`].
pub max_total_cltv_expiry_delta: u32,

/// The maximum number of paths that may be used by MPP payments.
/// Defaults to [`DEFAULT_MAX_MPP_PATH_COUNT`].
pub max_mpp_path_count: u8,
}

impl_writeable_tlv_based!(PaymentParameters, {
(0, payee_pubkey, required),
(1, max_total_cltv_expiry_delta, (default_value, DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA)),
(2, features, option),
(3, max_mpp_path_count, (default_value, DEFAULT_MAX_MPP_PATH_COUNT)),
(4, route_hints, vec_type),
(6, expiry_time, option),
});
Expand All @@ -234,6 +245,7 @@ impl PaymentParameters {
route_hints: vec![],
expiry_time: None,
max_total_cltv_expiry_delta: DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA,
max_mpp_path_count: DEFAULT_MAX_MPP_PATH_COUNT,
}
}

Expand Down Expand Up @@ -269,6 +281,13 @@ impl PaymentParameters {
pub fn with_max_total_cltv_expiry_delta(self, max_total_cltv_expiry_delta: u32) -> Self {
Self { max_total_cltv_expiry_delta, ..self }
}

/// Includes a limit for the maximum number of payment paths that may be used by MPP.
///
/// (C-not exported) since bindings don't support move semantics
pub fn with_max_mpp_path_count(self, max_mpp_path_count: u8) -> Self {
Self { max_mpp_path_count, ..self }
}
}

/// A list of hops along a payment path terminating with a channel to the recipient.
Expand Down Expand Up @@ -790,6 +809,11 @@ where L::Target: Logger {
node_info.features.supports_basic_mpp()
} else { false }
} else { false };

if allow_mpp && payment_params.max_mpp_path_count == 0 {
return Err(LightningError{err: "Can't find an MPP route with no paths allowed.".to_owned(), action: ErrorAction::IgnoreError});
}

log_trace!(logger, "Searching for a route from payer {} to payee {} {} MPP and {} first hops {}overriding the network graph", our_node_pubkey,
payment_params.payee_pubkey, if allow_mpp { "with" } else { "without" },
first_hops.map(|hops| hops.len()).unwrap_or(0), if first_hops.is_some() { "" } else { "not " });
Expand Down Expand Up @@ -840,18 +864,30 @@ where L::Target: Logger {
let recommended_value_msat = final_value_msat * ROUTE_CAPACITY_PROVISION_FACTOR as u64;
let mut path_value_msat = final_value_msat;

// Routing Fragmentation Mitigation heuristic:
//
// Routing fragmentation across many payment paths increases the overall routing
// fees as you have irreducible routing fees per-link used (`fee_base_msat`).
// Taking too many smaller paths also increases the chance of payment failure.
// Thus to avoid this effect, we require from our collected links to provide
// at least a minimal contribution to the recommended value yet-to-be-fulfilled.
// This requirement is currently set to be 1/max_mpp_path_count of the payment
// value to ensure we only ever return routes that do not violate this limit.
let minimal_value_contribution_msat: u64 = if allow_mpp {
(final_value_msat + (payment_params.max_mpp_path_count as u64 - 1)) / payment_params.max_mpp_path_count as u64
} else {
final_value_msat
};

// Keep track of how much liquidity has been used in selected channels. Used to determine
// if the channel can be used by additional MPP paths or to inform path finding decisions. It is
// aware of direction *only* to ensure that the correct htlc_maximum_msat value is used. Hence,
// liquidity used in one direction will not offset any used in the opposite direction.
let mut used_channel_liquidities: HashMap<(u64, bool), u64> =
HashMap::with_capacity(network_nodes.len());

// Keeping track of how much value we already collected across other paths. Helps to decide:
// - how much a new path should be transferring (upper bound);
// - whether a channel should be disregarded because
// it's available liquidity is too small comparing to how much more we need to collect;
// - when we want to stop looking for new paths.
// Keeping track of how much value we already collected across other paths. Helps to decide
// when we want to stop looking for new paths.
let mut already_collected_value_msat = 0;

for (_, channels) in first_hop_targets.iter_mut() {
Expand Down Expand Up @@ -913,26 +949,6 @@ where L::Target: Logger {
*used_liquidity_msat
});

// Routing Fragmentation Mitigation heuristic:
//
// Routing fragmentation across many payment paths increases the overall routing
// fees as you have irreducible routing fees per-link used (`fee_base_msat`).
// Taking too many smaller paths also increases the chance of payment failure.
// Thus to avoid this effect, we require from our collected links to provide
// at least a minimal contribution to the recommended value yet-to-be-fulfilled.
//
// This requirement is currently 5% of the remaining-to-be-collected value.
// This means as we successfully advance in our collection,
// the absolute liquidity contribution is lowered,
// thus increasing the number of potential channels to be selected.

// Derive the minimal liquidity contribution with a ratio of 20 (5%, rounded up)
// or 100% if we're not allowed to do multipath payments.
let minimal_value_contribution_msat: u64 = if allow_mpp {
(recommended_value_msat - already_collected_value_msat + 19) / 20
} else {
final_value_msat
};
// Verify the liquidity offered by this channel complies to the minimal contribution.
let contributes_sufficient_value = available_value_contribution_msat >= minimal_value_contribution_msat;
// Do not consider candidate hops that would exceed the maximum path length.
Expand Down Expand Up @@ -1504,10 +1520,8 @@ where L::Target: Logger {
*used_channel_liquidities.entry((victim_scid, true)).or_default() = exhausted;
}

// Track the total amount all our collected paths allow to send so that we:
// - know when to stop looking for more paths
// - know which of the hops are useless considering how much more sats we need
// (contributes_sufficient_value)
// Track the total amount all our collected paths allow to send so that we know
// when to stop looking for more paths
already_collected_value_msat += value_contribution_msat;

payment_paths.push(payment_path);
Expand Down Expand Up @@ -1678,6 +1692,8 @@ where L::Target: Logger {
});
selected_paths.push(path);
}
// Make sure we would never create a route with more paths than we allow.
debug_assert!(selected_paths.len() <= payment_params.max_mpp_path_count.into());

if let Some(features) = &payment_params.features {
for path in selected_paths.iter_mut() {
Expand Down Expand Up @@ -3999,7 +4015,8 @@ mod tests {
let scorer = test_utils::TestScorer::with_penalty(0);
let keys_manager = test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
let random_seed_bytes = keys_manager.get_secure_random_bytes();
let payment_params = PaymentParameters::from_node_id(nodes[2]).with_features(InvoiceFeatures::known());
let payment_params = PaymentParameters::from_node_id(nodes[2])
.with_features(InvoiceFeatures::known());

// We need a route consisting of 3 paths:
// From our node to node2 via node0, node7, node1 (three paths one hop each).
Expand Down Expand Up @@ -4092,15 +4109,39 @@ mod tests {
{
// Attempt to route more than available results in a failure.
if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(
&our_id, &payment_params, &network_graph.read_only(), None, 300_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes) {
assert_eq!(err, "Failed to find a sufficient route to the given destination");
&our_id, &payment_params, &network_graph.read_only(), None, 300_000, 42,
Arc::clone(&logger), &scorer, &random_seed_bytes) {
assert_eq!(err, "Failed to find a sufficient route to the given destination");
} else { panic!(); }
}

{
// Attempt to route while setting max_mpp_path_count to 0 results in a failure.
let zero_payment_params = payment_params.clone().with_max_mpp_path_count(0);
if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(
&our_id, &zero_payment_params, &network_graph.read_only(), None, 100, 42,
Arc::clone(&logger), &scorer, &random_seed_bytes) {
assert_eq!(err, "Can't find an MPP route with no paths allowed.");
} else { panic!(); }
}

{
// Attempt to route while setting max_mpp_path_count to 3 results in a failure.
// This is the case because the minimal_value_contribution_msat would require each path
// to account for 1/3 of the total value, which is violated by 2 out of 3 paths.
let fail_payment_params = payment_params.clone().with_max_mpp_path_count(3);
if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(
&our_id, &fail_payment_params, &network_graph.read_only(), None, 250_000, 42,
Arc::clone(&logger), &scorer, &random_seed_bytes) {
assert_eq!(err, "Failed to find a sufficient route to the given destination");
} else { panic!(); }
}

{
// Now, attempt to route 250 sats (just a bit below the capacity).
// Our algorithm should provide us with these 3 paths.
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 250_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None,
250_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
assert_eq!(route.paths.len(), 3);
let mut total_amount_paid_msat = 0;
for path in &route.paths {
Expand All @@ -4113,7 +4154,8 @@ mod tests {

{
// Attempt to route an exact amount is also fine
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 290_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None,
290_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
assert_eq!(route.paths.len(), 3);
let mut total_amount_paid_msat = 0;
for path in &route.paths {
Expand Down