Merge pull request #1610 from TheBlueMatt/2022-07-no-rand-path-selection
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Tue, 19 Jul 2022 16:56:36 +0000 (16:56 +0000)
committerGitHub <noreply@github.com>
Tue, 19 Jul 2022 16:56:36 +0000 (16:56 +0000)
lightning/src/ln/functional_tests.rs
lightning/src/routing/router.rs

index 4894ed02b3bb5412bb6da0eaf0b01dbfc5af2352..1da45bf2c9916c2455e80f6f2b818c51580468d0 100644 (file)
@@ -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);
@@ -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]
index a97e1b6ffa45db704b89505cfce7d6c58cc1f16b..1b14e9c735f145138ed2ad536f6fbf9e8674a9c0 100644 (file)
@@ -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
@@ -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),
 });
@@ -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(),
                }
        }
@@ -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]
 ) -> Result<Route, LightningError>
 where L::Target: Logger {
        let payee_node_id = NodeId::from_pubkey(&payment_params.payee_pubkey);
@@ -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
@@ -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.
@@ -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);
+                               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
@@ -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;
 
-       // 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>();
+                       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
@@ -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
@@ -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()
@@ -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()
@@ -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));