Merge pull request #1605 from TheBlueMatt/2022-07-smaller-mpp-parts
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Thu, 14 Jul 2022 18:33:53 +0000 (18:33 +0000)
committerGitHub <noreply@github.com>
Thu, 14 Jul 2022 18:33:53 +0000 (18:33 +0000)
Avoid saturating channels before we split payments

1  2 
lightning/src/ln/functional_tests.rs
lightning/src/routing/gossip.rs
lightning/src/routing/router.rs

index e58b541723c203329a01ea9085314a91e1b8625a,f39cb15fa94a7ea9d710d5e1ccc45ae3449dd639..4894ed02b3bb5412bb6da0eaf0b01dbfc5af2352
@@@ -13,7 -13,6 +13,7 @@@
  
  use chain;
  use chain::{Confirm, Listen, Watch};
 +use chain::chaininterface::LowerBoundedFeeEstimator;
  use chain::channelmonitor;
  use chain::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
  use chain::transaction::OutPoint;
@@@ -1824,9 -1823,12 +1824,12 @@@ fn test_channel_reserve_holding_cell_ht
  
        // attempt to send amt_msat > their_max_htlc_value_in_flight_msat
        {
-               let (mut route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], recv_value_0);
+               let payment_params = PaymentParameters::from_node_id(nodes[2].node.get_our_node_id())
+                       .with_features(InvoiceFeatures::known()).with_max_channel_saturation_power_of_half(0);
+               let (mut route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], payment_params, recv_value_0, TEST_FINAL_CLTV);
                route.paths[0].last_mut().unwrap().fee_msat += 1;
                assert!(route.paths[0].iter().rev().skip(1).all(|h| h.fee_msat == feemsat));
                unwrap_send_err!(nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)), true, APIError::ChannelUnavailable { ref err },
                        assert!(regex::Regex::new(r"Cannot send value that would put us over the max HTLC value in flight our peer will accept \(\d+\)").unwrap().is_match(err)));
                assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
                if stat01.value_to_self_msat < stat01.channel_reserve_msat + commit_tx_fee_all_htlcs + ensure_htlc_amounts_above_dust_buffer + amt_msat {
                        break;
                }
-               send_payment(&nodes[0], &vec![&nodes[1], &nodes[2]][..], recv_value_0);
+               let payment_params = PaymentParameters::from_node_id(nodes[2].node.get_our_node_id())
+                       .with_features(InvoiceFeatures::known()).with_max_channel_saturation_power_of_half(0);
+               let route = get_route!(nodes[0], payment_params, recv_value_0, TEST_FINAL_CLTV).unwrap();
+               let (payment_preimage, ..) = send_along_route(&nodes[0], route, &[&nodes[1], &nodes[2]], recv_value_0);
+               claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage);
  
                let (stat01_, stat11_, stat12_, stat22_) = (
                        get_channel_value_stat!(nodes[0], chan_1.2),
@@@ -3490,7 -3497,7 +3498,7 @@@ fn test_force_close_fail_back() 
        // Now check that if we add the preimage to ChannelMonitor it broadcasts our HTLC-Success..
        {
                get_monitor!(nodes[2], payment_event.commitment_msg.channel_id)
 -                      .provide_payment_preimage(&our_payment_hash, &our_payment_preimage, &node_cfgs[2].tx_broadcaster, &node_cfgs[2].fee_estimator, &node_cfgs[2].logger);
 +                      .provide_payment_preimage(&our_payment_hash, &our_payment_preimage, &node_cfgs[2].tx_broadcaster, &LowerBoundedFeeEstimator::new(node_cfgs[2].fee_estimator), &node_cfgs[2].logger);
        }
        mine_transaction(&nodes[2], &tx);
        let node_txn = nodes[2].tx_broadcaster.txn_broadcasted.lock().unwrap();
@@@ -7343,7 -7350,7 +7351,7 @@@ fn test_user_configurable_csv_delay() 
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
  
        // We test config.our_to_self > BREAKDOWN_TIMEOUT is enforced in Channel::new_outbound()
 -      if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) },
 +      if let Err(error) = Channel::new_outbound(&LowerBoundedFeeEstimator::new(&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }),
                &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), 1000000, 1000000, 0,
                &low_our_to_self_config, 0, 42)
        {
        nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap();
        let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id());
        open_channel.to_self_delay = 200;
 -      if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) },
 +      if let Err(error) = Channel::new_from_req(&LowerBoundedFeeEstimator::new(&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }),
                &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0,
                &low_our_to_self_config, 0, &nodes[0].logger, 42)
        {
        nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap();
        let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id());
        open_channel.to_self_delay = 200;
 -      if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) },
 +      if let Err(error) = Channel::new_from_req(&LowerBoundedFeeEstimator::new(&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }),
                &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), &InitFeatures::known(), &open_channel, 0,
                &high_their_to_self_config, 0, &nodes[0].logger, 42)
        {
index b03acfb8970fc07dc6c8eefdb98251348fd62746,55887623c4032c225befe3d9dfd33fb3a7b1c788..10ce74b971e6eebd13054257826aa78cbec94276
@@@ -816,10 -816,6 +816,6 @@@ impl<'a> DirectedChannelInfoWithUpdate<
        /// Returns the [`EffectiveCapacity`] of the channel in the direction.
        #[inline]
        pub(super) fn effective_capacity(&self) -> EffectiveCapacity { self.inner.effective_capacity() }
-       /// Returns the maximum HTLC amount allowed over the channel in the direction.
-       #[inline]
-       pub(super) fn htlc_maximum_msat(&self) -> u64 { self.inner.htlc_maximum_msat() }
  }
  
  impl<'a> fmt::Debug for DirectedChannelInfoWithUpdate<'a> {
@@@ -1649,11 -1645,6 +1645,11 @@@ impl ReadOnlyNetworkGraph<'_> 
                &*self.channels
        }
  
 +      /// Returns information on a channel with the given id.
 +      pub fn channel(&self, short_channel_id: u64) -> Option<&ChannelInfo> {
 +              self.channels.get(&short_channel_id)
 +      }
 +
        /// Returns all known nodes' public keys along with announced node info.
        ///
        /// (C-not exported) because we have no mapping for `BTreeMap`s
                &*self.nodes
        }
  
 +      /// Returns information on a node with the given id.
 +      pub fn node(&self, node_id: &NodeId) -> Option<&NodeInfo> {
 +              self.nodes.get(node_id)
 +      }
 +
        /// Get network addresses by node id.
        /// Returns None if the requested node is completely unknown,
        /// or if node announcement for the node was never received.
index 289221601c5c82e92cd2db457dbfff69a5eef460,40cd439b1df261c6c1a4c433cf4ea892a0864448..27a21a1899e1014be3c7280f2e1b09a2e8459f60
@@@ -225,6 -225,21 +225,21 @@@ pub struct PaymentParameters 
        /// The maximum number of paths that may be used by (MPP) payments.
        /// Defaults to [`DEFAULT_MAX_PATH_COUNT`].
        pub max_path_count: u8,
+       /// Selects the maximum share of a channel's total capacity which will be sent over a channel,
+       /// as a power of 1/2. A higher value prefers to send the payment using more MPP parts whereas
+       /// a lower value prefers to send larger MPP parts, potentially saturating channels and
+       /// increasing failure probability for those paths.
+       ///
+       /// Note that this restriction will be relaxed during pathfinding after paths which meet this
+       /// restriction have been found. While paths which meet this criteria will be searched for, it
+       /// is ultimately up to the scorer to select them over other paths.
+       ///
+       /// 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
+       pub max_channel_saturation_power_of_half: u8,
  }
  
  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)),
        (6, expiry_time, option),
  });
  
@@@ -246,6 -262,7 +262,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,
                }
        }
  
        pub fn with_max_path_count(self, max_path_count: u8) -> Self {
                Self { max_path_count, ..self }
        }
+       /// Includes a limit for the maximum number of payment paths that may be used.
+       ///
+       /// (C-not exported) since bindings don't support move semantics
+       pub fn with_max_channel_saturation_power_of_half(self, max_channel_saturation_power_of_half: u8) -> Self {
+               Self { max_channel_saturation_power_of_half, ..self }
+       }
  }
  
  /// A list of hops along a payment path terminating with a channel to the recipient.
@@@ -433,16 -457,6 +457,6 @@@ impl<'a> CandidateRouteHop<'a> 
                }
        }
  
-       fn htlc_maximum_msat(&self) -> u64 {
-               match self {
-                       CandidateRouteHop::FirstHop { details } => details.next_outbound_htlc_limit_msat,
-                       CandidateRouteHop::PublicHop { info, .. } => info.htlc_maximum_msat(),
-                       CandidateRouteHop::PrivateHop { hint } => {
-                               hint.htlc_maximum_msat.unwrap_or(u64::max_value())
-                       },
-               }
-       }
        fn fees(&self) -> RoutingFees {
                match self {
                        CandidateRouteHop::FirstHop { .. } => RoutingFees {
        }
  }
  
+ #[inline]
+ fn max_htlc_from_capacity(capacity: EffectiveCapacity, max_channel_saturation_power_of_half: u8) -> u64 {
+       let saturation_shift: u32 = max_channel_saturation_power_of_half as u32;
+       match capacity {
+               EffectiveCapacity::ExactLiquidity { liquidity_msat } => liquidity_msat,
+               EffectiveCapacity::Infinite => u64::max_value(),
+               EffectiveCapacity::Unknown => EffectiveCapacity::Unknown.as_msat(),
+               EffectiveCapacity::MaximumHTLC { amount_msat } =>
+                       amount_msat.checked_shr(saturation_shift).unwrap_or(0),
+               EffectiveCapacity::Total { capacity_msat, htlc_maximum_msat: None } =>
+                       capacity_msat.checked_shr(saturation_shift).unwrap_or(0),
+               EffectiveCapacity::Total { capacity_msat, htlc_maximum_msat: Some(htlc_max) } =>
+                       cmp::min(capacity_msat.checked_shr(saturation_shift).unwrap_or(0), htlc_max),
+       }
+ }
+ fn iter_equal<I1: Iterator, I2: Iterator>(mut iter_a: I1, mut iter_b: I2)
+ -> bool where I1::Item: PartialEq<I2::Item> {
+       loop {
+               let a = iter_a.next();
+               let b = iter_b.next();
+               if a.is_none() && b.is_none() { return true; }
+               if a.is_none() || b.is_none() { return false; }
+               if a.unwrap().ne(&b.unwrap()) { return false; }
+       }
+ }
  /// It's useful to keep track of the hops associated with the fees required to use them,
  /// so that we can choose cheaper paths (as per Dijkstra's algorithm).
  /// Fee values should be updated only in the context of the whole path, see update_value_and_recompute_fees.
@@@ -571,10 -612,9 +612,9 @@@ impl<'a> PaymentPath<'a> 
        // to the fees being paid not lining up with the actual limits.
        //
        // Note that this function is not aware of the available_liquidity limit, and thus does not
-       // support increasing the value being transferred.
+       // support increasing the value being transferred beyond what was selected during the initial
+       // routing passes.
        fn update_value_and_recompute_fees(&mut self, value_msat: u64) {
-               assert!(value_msat <= self.hops.last().unwrap().0.fee_msat);
                let mut total_fee_paid_msat = 0 as u64;
                for i in (0..self.hops.len()).rev() {
                        let last_hop = i == self.hops.len() - 1;
@@@ -882,6 -922,11 +922,11 @@@ where L::Target: Logger 
                final_value_msat
        };
  
+       // When we start collecting routes we enforce the max_channel_saturation_power_of_half
+       // requirement strictly. After we've collected enough (or if we fail to find new routes) we
+       // drop the requirement by setting this to 0.
+       let mut channel_saturation_pow_half = payment_params.max_channel_saturation_power_of_half;
        // 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,
                        // - for first and last hops early in get_route
                        if $src_node_id != $dest_node_id {
                                let short_channel_id = $candidate.short_channel_id();
-                               let htlc_maximum_msat = $candidate.htlc_maximum_msat();
+                               let effective_capacity = $candidate.effective_capacity();
+                               let htlc_maximum_msat = max_htlc_from_capacity(effective_capacity, channel_saturation_pow_half);
  
                                // It is tricky to subtract $next_hops_fee_msat from available liquidity here.
                                // It may be misleading because we might later choose to reduce the value transferred
                                                        let channel_usage = ChannelUsage {
                                                                amount_msat: amount_to_transfer_over_msat,
                                                                inflight_htlc_msat: used_liquidity_msat,
-                                                               effective_capacity: $candidate.effective_capacity(),
+                                                               effective_capacity,
                                                        };
                                                        let channel_penalty_msat = scorer.channel_penalty_msat(
                                                                short_channel_id, &$src_node_id, &$dest_node_id, channel_usage
                                                .entry((hop.candidate.short_channel_id(), *prev_hop < hop.node_id))
                                                .and_modify(|used_liquidity_msat| *used_liquidity_msat += spent_on_hop_msat)
                                                .or_insert(spent_on_hop_msat);
-                                       if *used_liquidity_msat == hop.candidate.htlc_maximum_msat() {
+                                       let hop_capacity = hop.candidate.effective_capacity();
+                                       let hop_max_msat = max_htlc_from_capacity(hop_capacity, channel_saturation_pow_half);
+                                       if *used_liquidity_msat == hop_max_msat {
                                                // If this path used all of this channel's available liquidity, we know
                                                // this path will not be selected again in the next loop iteration.
                                                prevented_redundant_path_selection = true;
                                        }
-                                       debug_assert!(*used_liquidity_msat <= hop.candidate.htlc_maximum_msat());
+                                       debug_assert!(*used_liquidity_msat <= hop_max_msat);
                                }
                                if !prevented_redundant_path_selection {
                                        // If we weren't capped by hitting a liquidity limit on a channel in the path,
                }
  
                if !allow_mpp {
+                       if !found_new_path && channel_saturation_pow_half != 0 {
+                               channel_saturation_pow_half = 0;
+                               continue 'paths_collection;
+                       }
                        // If we don't support MPP, no use trying to gather more value ever.
                        break 'paths_collection;
                }
                // iteration.
                // In the latter case, making another path finding attempt won't help,
                // because we deterministically terminated the search due to low liquidity.
-               if already_collected_value_msat >= recommended_value_msat || !found_new_path {
+               if !found_new_path && channel_saturation_pow_half != 0 {
+                       channel_saturation_pow_half = 0;
+               } else if already_collected_value_msat >= recommended_value_msat || !found_new_path {
                        log_trace!(logger, "Have now collected {} msat (seeking {} msat) in paths. Last path loop {} a new path.",
                                already_collected_value_msat, recommended_value_msat, if found_new_path { "found" } else { "did not find" });
                        break 'paths_collection;
        // 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();
+       // 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
+       // across nodes.
+       selected_route.sort_unstable_by_key(|path| {
+               let mut key = [0u64; MAX_PATH_LENGTH_ESTIMATE as usize];
+               debug_assert!(path.hops.len() <= key.len());
+               for (scid, key) in path.hops.iter().map(|h| h.0.candidate.short_channel_id()).zip(key.iter_mut()) {
+                       *key = scid;
+               }
+               key
+       });
+       for idx in 0..(selected_route.len() - 1) {
+               if idx + 1 >= selected_route.len() { break; }
+               if iter_equal(selected_route[idx    ].hops.iter().map(|h| (h.0.candidate.short_channel_id(), h.0.node_id)),
+                             selected_route[idx + 1].hops.iter().map(|h| (h.0.candidate.short_channel_id(), h.0.node_id))) {
+                       let new_value = selected_route[idx].get_value_msat() + selected_route[idx + 1].get_value_msat();
+                       selected_route[idx].update_value_and_recompute_fees(new_value);
+                       selected_route.remove(idx + 1);
+               }
+       }
        let mut selected_paths = Vec::<Vec<Result<RouteHop, LightningError>>>::new();
-       for payment_path in drawn_routes.first().unwrap() {
+       for payment_path in selected_route {
                let mut path = payment_path.hops.iter().map(|(payment_hop, node_features)| {
                        Ok(RouteHop {
                                pubkey: PublicKey::from_slice(payment_hop.node_id.as_slice()).map_err(|_| LightningError{err: format!("Public key {:?} is invalid", &payment_hop.node_id), action: ErrorAction::IgnoreAndLog(Level::Trace)})?,
@@@ -1882,7 -1960,7 +1960,7 @@@ fn build_route_from_hops_internal<L: De
  
  #[cfg(test)]
  mod tests {
 -      use routing::gossip::{NetworkGraph, P2PGossipSync, NodeId};
 +      use routing::gossip::{NetworkGraph, P2PGossipSync, NodeId, EffectiveCapacity};
        use routing::router::{get_route, build_route_from_hops_internal, add_random_cltv_offset, default_node_features,
                PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RoutingFees,
                DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA, MAX_PATH_LENGTH_ESTIMATE};
  
                // Get a route for 100 sats and check that we found the MPP route no problem and didn't
                // overpay at all.
-               let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 100_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
+               let mut route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 100_000, 42, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap();
                assert_eq!(route.paths.len(), 2);
-               // Paths are somewhat randomly ordered, but:
-               // * the first is channel 2 (1 msat fee) -> channel 4 -> channel 42
-               // * the second is channel 1 (0 fee, but 99 sat maximum) -> channel 3 -> channel 42
-               assert_eq!(route.paths[0][0].short_channel_id, 2);
-               assert_eq!(route.paths[0][0].fee_msat, 1);
-               assert_eq!(route.paths[0][2].fee_msat, 1_000);
-               assert_eq!(route.paths[1][0].short_channel_id, 1);
-               assert_eq!(route.paths[1][0].fee_msat, 0);
-               assert_eq!(route.paths[1][2].fee_msat, 99_000);
+               route.paths.sort_by_key(|path| path[0].short_channel_id);
+               // Paths are manually ordered ordered by SCID, so:
+               // * the first is channel 1 (0 fee, but 99 sat maximum) -> channel 3 -> channel 42
+               // * the second is channel 2 (1 msat fee) -> channel 4 -> channel 42
+               assert_eq!(route.paths[0][0].short_channel_id, 1);
+               assert_eq!(route.paths[0][0].fee_msat, 0);
+               assert_eq!(route.paths[0][2].fee_msat, 99_000);
+               assert_eq!(route.paths[1][0].short_channel_id, 2);
+               assert_eq!(route.paths[1][0].fee_msat, 1);
+               assert_eq!(route.paths[1][2].fee_msat, 1_000);
                assert_eq!(route.get_total_fees(), 1);
                assert_eq!(route.get_total_amount(), 100_000);
        }
                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())
+                       .with_max_channel_saturation_power_of_half(0);
  
                // We need a route consisting of 3 paths:
                // From our node to node2 via node0, node7, node1 (three paths one hop each).
                        assert_eq!(route.paths[0].len(), 1);
                        assert_eq!(route.paths[1].len(), 1);
  
+                       assert!((route.paths[0][0].short_channel_id == 3 && route.paths[1][0].short_channel_id == 2) ||
+                               (route.paths[0][0].short_channel_id == 2 && route.paths[1][0].short_channel_id == 3));
                        assert_eq!(route.paths[0][0].pubkey, nodes[0]);
-                       assert_eq!(route.paths[0][0].short_channel_id, 3);
                        assert_eq!(route.paths[0][0].fee_msat, 50_000);
  
                        assert_eq!(route.paths[1][0].pubkey, nodes[0]);
-                       assert_eq!(route.paths[1][0].short_channel_id, 2);
                        assert_eq!(route.paths[1][0].fee_msat, 50_000);
                }
  
                }
        }
  
+       #[test]
+       fn avoids_saturating_channels() {
+               let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph();
+               let (_, our_id, privkeys, nodes) = get_nodes(&secp_ctx);
+               let scorer = ProbabilisticScorer::new(Default::default(), &*network_graph, Arc::clone(&logger));
+               // Set the fee on channel 13 to 100% 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: genesis_block(Network::Testnet).header.block_hash(),
+                       short_channel_id: 4,
+                       timestamp: 2,
+                       flags: 0,
+                       cltv_expiry_delta: (4 << 4) | 1,
+                       htlc_minimum_msat: 0,
+                       htlc_maximum_msat: OptionalField::Present(200_000_000),
+                       fee_base_msat: 0,
+                       fee_proportional_millionths: 0,
+                       excess_data: Vec::new()
+               });
+               update_channel(&gossip_sync, &secp_ctx, &privkeys[7], UnsignedChannelUpdate {
+                       chain_hash: genesis_block(Network::Testnet).header.block_hash(),
+                       short_channel_id: 13,
+                       timestamp: 2,
+                       flags: 0,
+                       cltv_expiry_delta: (13 << 4) | 1,
+                       htlc_minimum_msat: 0,
+                       htlc_maximum_msat: OptionalField::Present(200_000_000),
+                       fee_base_msat: 0,
+                       fee_proportional_millionths: 0,
+                       excess_data: Vec::new()
+               });
+               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();
+               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));
+       }
        #[cfg(not(feature = "no-std"))]
        pub(super) fn random_init_seed() -> u64 {
                // Because the default HashMap in std pulls OS randomness, we can use it as a (bad) RNG.
        }
  
        #[test]
 -      fn avoids_banned_nodes() {
 +      fn honors_manual_penalties() {
                let (secp_ctx, network_graph, _, _, logger) = build_line_graph();
                let (_, our_id, _, nodes) = get_nodes(&secp_ctx);
  
                let scorer_params = ProbabilisticScoringParameters::default();
                let mut scorer = ProbabilisticScorer::new(scorer_params, Arc::clone(&network_graph), Arc::clone(&logger));
  
 -              // First check we can get a route.
 +              // First check set manual penalties are returned by the scorer.
 +              let usage = ChannelUsage {
 +                      amount_msat: 0,
 +                      inflight_htlc_msat: 0,
 +                      effective_capacity: EffectiveCapacity::Total { capacity_msat: 1_024_000, htlc_maximum_msat: Some(1_000) },
 +              };
 +              scorer.set_manual_penalty(&NodeId::from_pubkey(&nodes[3]), 123);
 +              scorer.set_manual_penalty(&NodeId::from_pubkey(&nodes[4]), 456);
 +              assert_eq!(scorer.channel_penalty_msat(42, &NodeId::from_pubkey(&nodes[3]), &NodeId::from_pubkey(&nodes[4]), usage), 456);
 +
 +              // Then check we can get a normal route
                let payment_params = PaymentParameters::from_node_id(nodes[10]);
                let route = get_route(&our_id, &payment_params, &network_graph.read_only(), None, 100, 42, Arc::clone(&logger), &scorer, &random_seed_bytes);
                assert!(route.is_ok());