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

Always pick the best paths, rather than (poorly) attempting to randomize #1610

Merged
merged 3 commits into from Jul 19, 2022
Merged

Always pick the best paths, rather than (poorly) attempting to randomize #1610

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
0c3a47c
Fix tracking of collected value across pathfinding iterations
TheBlueMatt Jul 12, 2022
985153e
Make route path selection optimize strictly for `cost / amount`
TheBlueMatt Jul 12, 2022
ff8d3f7
Reduce default max_channel_saturation_power_of_half to 2 (max 1/4)
TheBlueMatt Jul 12, 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
23 changes: 0 additions & 23 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Expand Up @@ -1059,26 +1059,6 @@ fn fake_network_test() {
fail_payment(&nodes[1], &vec!(&nodes[3], &nodes[2], &nodes[1])[..], payment_hash_2);
claim_payment(&nodes[1], &vec!(&nodes[2], &nodes[3], &nodes[1])[..], payment_preimage_1);

// Add a duplicate new channel from 2 to 4
let chan_5 = create_announced_chan_between_nodes(&nodes, 1, 3, InitFeatures::known(), InitFeatures::known());

// Send some payments across both channels
let payment_preimage_3 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
let payment_preimage_4 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;
let payment_preimage_5 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000).0;


route_over_limit(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], 3000000);
let events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 0);
nodes[0].logger.assert_log_regex("lightning::ln::channelmanager".to_string(), regex::Regex::new(r"Cannot send value that would put us over the max HTLC value in flight our peer will accept \(\d+\)").unwrap(), 1);

//TODO: Test that routes work again here as we've been notified that the channel is full

claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_3);
claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_4);
claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[3])[..], payment_preimage_5);

// Close down the channels...
close_channel(&nodes[0], &nodes[1], &chan_1.2, chan_1.3, true);
check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure);
Expand All @@ -1092,9 +1072,6 @@ fn fake_network_test() {
close_channel(&nodes[1], &nodes[3], &chan_4.2, chan_4.3, false);
check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure);
check_closed_event!(nodes[3], 1, ClosureReason::CooperativeClosure);
close_channel(&nodes[1], &nodes[3], &chan_5.2, chan_5.3, false);
check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure);
check_closed_event!(nodes[3], 1, ClosureReason::CooperativeClosure);
}

#[test]
Expand Down
155 changes: 58 additions & 97 deletions lightning/src/routing/router.rs
View file
Open in desktop
Expand Up @@ -238,7 +238,7 @@ pub struct PaymentParameters {
/// A value of 0 will allow payments up to and including a channel's total announced usable
/// capacity, a value of one will only use up to half its capacity, two 1/4, etc.
///
/// Default value: 1
/// Default value: 2
pub max_channel_saturation_power_of_half: u8,

/// A list of SCIDs which this payment was previously attempted over and which caused the
Expand All @@ -253,7 +253,7 @@ impl_writeable_tlv_based!(PaymentParameters, {
(2, features, option),
(3, max_path_count, (default_value, DEFAULT_MAX_PATH_COUNT)),
(4, route_hints, vec_type),
(5, max_channel_saturation_power_of_half, (default_value, 1)),
(5, max_channel_saturation_power_of_half, (default_value, 2)),
(6, expiry_time, option),
(7, previously_failed_channels, vec_type),
});
Expand All @@ -268,7 +268,7 @@ impl PaymentParameters {
expiry_time: None,
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,
max_channel_saturation_power_of_half: 2,
previously_failed_channels: Vec::new(),
}
}
Expand Down Expand Up @@ -754,7 +754,7 @@ where L::Target: Logger, GL::Target: Logger {
pub(crate) fn get_route<L: Deref, S: Score>(
our_node_pubkey: &PublicKey, payment_params: &PaymentParameters, network_graph: &ReadOnlyNetworkGraph,
first_hops: Option<&[&ChannelDetails]>, final_value_msat: u64, final_cltv_expiry_delta: u32,
logger: L, scorer: &S, random_seed_bytes: &[u8; 32]
logger: L, scorer: &S, _random_seed_bytes: &[u8; 32]
Copy link
Contributor

Choose a reason for hiding this comment

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

So, now that we don't use it anymore and probably won't going forward, should we remove random_seed_bytes from the signature of get_route entirely? As discussed, #495 will likely happen in the Scorer and #1482 can be done 'on top' in a similar vein as add_random_cltv_offset.

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, good question. The first version of this patch did, and while it didn't change the public interface, it touched a ton of test code. I dropped it cause I wasn't sure if we would end up using it again and it doesn't change the public interface so I don't care too much about an unused bit of code. I don't feel super strongly but until we're in a place we're confident in with the router I kinda feel like leaving it. One thing, at least, that we may do, is add the random seed as a "per route" input to the scorer.

Copy link
Contributor

@tnull tnull Jul 19, 2022 •
edited

Choose a reason for hiding this comment

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

Hm, but I assume if we needed randomization in the Scorer, we'd pass it directly to it before giving it to find_route? That said, I also don't think it's very pressing to remove it right now. Especially since I still like the idea of eventually introducing a proper Rand interface, which may require a larger refactoring anyways. So feel free to leave as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe? I dunno, I haven't really thought about it, its just not clear to me how we'd do it, so I figured not worry too much about removing the field right now.

) -> Result<Route, LightningError>
where L::Target: Logger {
let payee_node_id = NodeId::from_pubkey(&payment_params.payee_pubkey);
Expand Down Expand Up @@ -796,11 +796,11 @@ where L::Target: Logger {
// 4. See if we managed to collect paths which aggregately are able to transfer target value
// (not recommended value).
// 5. If yes, proceed. If not, fail routing.
// 6. Randomly combine paths into routes having enough to fulfill the payment. (TODO: knapsack)
// 7. Of all the found paths, select only those with the lowest total fee.
// 8. The last path in every selected route is likely to be more than we need.
// Reduce its value-to-transfer and recompute fees.
// 9. Choose the best route by the lowest total fee.
// 6. Select the paths which have the lowest cost (fee plus scorer penalty) per amount
// transferred up to the transfer target value.
// 7. Reduce the value of the last path until we are sending only the target value.
// 8. If our maximum channel saturation limit caused us to pick two identical paths, combine
// them so that we're not sending two HTLCs along the same path.

// As for the actual search algorithm,
// we do a payee-to-payer pseudo-Dijkstra's sorting by each node's distance from the payee
Expand Down Expand Up @@ -1476,7 +1476,7 @@ where L::Target: Logger {
// Both these cases (and other cases except reaching recommended_value_msat) mean that
// paths_collection will be stopped because found_new_path==false.
// This is not necessarily a routing failure.
'path_construction: while let Some(RouteGraphNode { node_id, lowest_fee_to_node, total_cltv_delta, value_contribution_msat, path_htlc_minimum_msat, path_penalty_msat, path_length_to_node, .. }) = targets.pop() {
'path_construction: while let Some(RouteGraphNode { node_id, lowest_fee_to_node, total_cltv_delta, mut value_contribution_msat, path_htlc_minimum_msat, path_penalty_msat, path_length_to_node, .. }) = targets.pop() {

// Since we're going payee-to-payer, hitting our node as a target means we should stop
// traversing the graph and arrange the path out of what we found.
Expand Down Expand Up @@ -1542,7 +1542,9 @@ where L::Target: Logger {
// on some channels we already passed (assuming dest->source direction). Here, we
// recompute the fees again, so that if that's the case, we match the currently
// underpaid htlc_minimum_msat with fees.
payment_path.update_value_and_recompute_fees(cmp::min(value_contribution_msat, final_value_msat));
debug_assert_eq!(payment_path.get_value_msat(), value_contribution_msat);
value_contribution_msat = cmp::min(value_contribution_msat, final_value_msat);
TheBlueMatt marked this conversation as resolved.
Show resolved Hide resolved
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 be possible to formulate a test that catches the incorrect behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, so with the current code we end up relying on it being correct a good bit more, so reverting the first patch here causes existing tests to fail. As for building a fresh test for it...its pretty convoluted and we'd be testing side-effects - we can construct a path that requires up to 2x overpayment, but we're trying to gather 3x total paths, so if we try to build a test that fails to find a path it won't work. We could maybe get away with something where it finds a path but not the optimal one because an optimal one was found later, but that's...complicated and kinda a strange test? I dunno.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be alright if other tests are catching it now.

payment_path.update_value_and_recompute_fees(value_contribution_msat);

// Since a path allows to transfer as much value as
// the smallest channel it has ("bottleneck"), we should recompute
Expand Down Expand Up @@ -1653,96 +1655,55 @@ where L::Target: Logger {
return Err(LightningError{err: "Failed to find a sufficient route to the given destination".to_owned(), action: ErrorAction::IgnoreError});
}

// Sort by total fees and take the best paths.
payment_paths.sort_unstable_by_key(|path| path.get_total_fee_paid_msat());
if payment_paths.len() > 50 {
payment_paths.truncate(50);
}
// Step (6).
let mut selected_route = payment_paths;
tnull marked this conversation as resolved.
Show resolved Hide resolved

// Draw multiple sufficient routes by randomly combining the selected paths.
let mut drawn_routes = Vec::new();
let mut prng = ChaCha20::new(random_seed_bytes, &[0u8; 12]);
let mut random_index_bytes = [0u8; ::core::mem::size_of::<usize>()];
debug_assert_eq!(selected_route.iter().map(|p| p.get_value_msat()).sum::<u64>(), already_collected_value_msat);
let mut overpaid_value_msat = already_collected_value_msat - final_value_msat;

let num_permutations = payment_paths.len();
for _ in 0..num_permutations {
let mut cur_route = Vec::<PaymentPath>::new();
let mut aggregate_route_value_msat = 0;
// First, sort by the cost-per-value of the path, dropping the paths that cost the most for
// the value they contribute towards the payment amount.
// We sort in descending order as we will remove from the front in `retain`, next.
selected_route.sort_unstable_by(|a, b|
(((b.get_cost_msat() as u128) << 64) / (b.get_value_msat() as u128))
.cmp(&(((a.get_cost_msat() as u128) << 64) / (a.get_value_msat() as u128)))
);

// Step (6).
// Do a Fisher-Yates shuffle to create a random permutation of the payment paths
for cur_index in (1..payment_paths.len()).rev() {
prng.process_in_place(&mut random_index_bytes);
let random_index = usize::from_be_bytes(random_index_bytes).wrapping_rem(cur_index+1);
payment_paths.swap(cur_index, random_index);
// We should make sure that at least 1 path left.
let mut paths_left = selected_route.len();
selected_route.retain(|path| {
if paths_left == 1 {
return true
}
let path_value_msat = path.get_value_msat();
if path_value_msat <= overpaid_value_msat {
overpaid_value_msat -= path_value_msat;
paths_left -= 1;
return false;
}
true
});
debug_assert!(selected_route.len() > 0);

if overpaid_value_msat != 0 {
// Step (7).
for payment_path in &payment_paths {
cur_route.push(payment_path.clone());
aggregate_route_value_msat += payment_path.get_value_msat();
if aggregate_route_value_msat > final_value_msat {
// Last path likely overpaid. Substract it from the most expensive
// (in terms of proportional fee) path in this route and recompute fees.
// This might be not the most economically efficient way, but fewer paths
// also makes routing more reliable.
let mut overpaid_value_msat = aggregate_route_value_msat - final_value_msat;

// First, we drop some expensive low-value paths entirely if possible, since fewer
// paths is better: the payment is less likely to fail. In order to do so, we sort
// by value and fall back to total fees paid, i.e., in case of equal values we
// prefer lower cost paths.
cur_route.sort_unstable_by(|a, b| {
a.get_value_msat().cmp(&b.get_value_msat())
// Reverse ordering for cost, so we drop higher-cost paths first
.then_with(|| b.get_cost_msat().cmp(&a.get_cost_msat()))
});

// We should make sure that at least 1 path left.
let mut paths_left = cur_route.len();
cur_route.retain(|path| {
if paths_left == 1 {
return true
}
let mut keep = true;
let path_value_msat = path.get_value_msat();
if path_value_msat <= overpaid_value_msat {
keep = false;
overpaid_value_msat -= path_value_msat;
paths_left -= 1;
}
keep
});

if overpaid_value_msat == 0 {
break;
}
// Now, subtract the remaining overpaid value from the most-expensive path.
// TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
// so that the sender pays less fees overall. And also htlc_minimum_msat.
selected_route.sort_unstable_by(|a, b| {
let a_f = a.hops.iter().map(|hop| hop.0.candidate.fees().proportional_millionths as u64).sum::<u64>();
tnull marked this conversation as resolved.
Show resolved Hide resolved
let b_f = b.hops.iter().map(|hop| hop.0.candidate.fees().proportional_millionths as u64).sum::<u64>();
a_f.cmp(&b_f).then_with(|| b.get_cost_msat().cmp(&a.get_cost_msat()))
});
let expensive_payment_path = selected_route.first_mut().unwrap();

assert!(cur_route.len() > 0);

// Step (8).
// Now, subtract the overpaid value from the most-expensive path.
// TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
// so that the sender pays less fees overall. And also htlc_minimum_msat.
cur_route.sort_unstable_by_key(|path| { path.hops.iter().map(|hop| hop.0.candidate.fees().proportional_millionths as u64).sum::<u64>() });
let expensive_payment_path = cur_route.first_mut().unwrap();

// We already dropped all the small value paths above, meaning all the
// remaining paths are larger than remaining overpaid_value_msat.
// Thus, this can't be negative.
let expensive_path_new_value_msat = expensive_payment_path.get_value_msat() - overpaid_value_msat;
expensive_payment_path.update_value_and_recompute_fees(expensive_path_new_value_msat);
break;
}
}
drawn_routes.push(cur_route);
// We already dropped all the paths with value below `overpaid_value_msat` above, thus this
// can't go negative.
let expensive_path_new_value_msat = expensive_payment_path.get_value_msat() - overpaid_value_msat;
expensive_payment_path.update_value_and_recompute_fees(expensive_path_new_value_msat);
}

// Step (9).
// Select the best route by lowest total cost.
drawn_routes.sort_unstable_by_key(|paths| paths.iter().map(|path| path.get_cost_msat()).sum::<u64>());
let selected_route = drawn_routes.first_mut().unwrap();

// Step (8).
// Sort by the path itself and combine redundant paths.
// Note that we sort by SCIDs alone as its simpler but when combining we have to ensure we
// compare both SCIDs and NodeIds as individual nodes may use random aliases causing collisions
Expand Down Expand Up @@ -4796,7 +4757,7 @@ mod tests {
cltv_expiry_delta: 42,
htlc_minimum_msat: None,
htlc_maximum_msat: None,
}])]);
}])]).with_max_channel_saturation_power_of_half(0);

// Keep only two paths from us to nodes[2], both with a 99sat HTLC maximum, with one with
// no fee and one with a 1msat fee. Previously, trying to route 100 sats to nodes[2] here
Expand Down Expand Up @@ -5780,7 +5741,7 @@ mod tests {
flags: 0,
cltv_expiry_delta: (4 << 4) | 1,
htlc_minimum_msat: 0,
htlc_maximum_msat: OptionalField::Present(200_000_000),
htlc_maximum_msat: OptionalField::Present(250_000_000),
fee_base_msat: 0,
fee_proportional_millionths: 0,
excess_data: Vec::new()
Expand All @@ -5792,7 +5753,7 @@ mod tests {
flags: 0,
cltv_expiry_delta: (13 << 4) | 1,
htlc_minimum_msat: 0,
htlc_maximum_msat: OptionalField::Present(200_000_000),
htlc_maximum_msat: OptionalField::Present(250_000_000),
fee_base_msat: 0,
fee_proportional_millionths: 0,
excess_data: Vec::new()
Expand All @@ -5801,8 +5762,8 @@ mod tests {
let payment_params = PaymentParameters::from_node_id(nodes[2]).with_features(InvoiceFeatures::known());
let keys_manager = test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
let random_seed_bytes = keys_manager.get_secure_random_bytes();
// 150,000 sat is less than the available liquidity on each channel, set above.
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 150_000_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
// 100,000 sats is less than the available liquidity on each channel, set above.
let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 100_000_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
assert_eq!(route.paths.len(), 2);
assert!((route.paths[0][1].short_channel_id == 4 && route.paths[1][1].short_channel_id == 13) ||
(route.paths[1][1].short_channel_id == 4 && route.paths[0][1].short_channel_id == 13));
Expand Down