Uh oh!
There was an error while loading. Please reload this page.
Avoid saturating channels before we split payments#1605
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -225,6 +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, { | ||
| @@ -233,6 +248,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)), | ||
Contributor There was a problem hiding this comment. Curious why 5 was skipped before :) CollaboratorAuthor There was a problem hiding this comment. Odd and even numbers have different semantics (odds are ignored if an old version doesnt understand it, evens cause a read failure) so they aren't interchangeable. Contributor There was a problem hiding this comment. Ah of course, part of the "okay to be odd" rule. | ||
| (6, expiry_time, option), | ||
| }); | ||
| @@ -246,6 +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, | ||
| } | ||
| } | ||
| @@ -288,6 +305,13 @@ impl PaymentParameters { | ||
| 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 @@ impl<'a> CandidateRouteHop<'a> { | ||
| } | ||
| } | ||
| fn fees(&self) -> RoutingFees { | ||
| match self { | ||
| CandidateRouteHop::FirstHop { .. } => RoutingFees { | ||
| @@ -464,6 +478,33 @@ impl<'a> CandidateRouteHop<'a> { | ||
| } | ||
| } | ||
| #[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 @@ 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 beyond what was selected during the initial | ||
| // routing passes. | ||
| fn update_value_and_recompute_fees(&mut self, value_msat: u64) { | ||
| 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 @@ 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, | ||
| @@ -934,7 +979,8 @@ where L::Target: Logger { | ||
| // - 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 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 | ||
| @@ -1084,7 +1130,7 @@ where L::Target: Logger { | ||
| let channel_usage = ChannelUsage { | ||
| amount_msat: amount_to_transfer_over_msat, | ||
| inflight_htlc_msat: used_liquidity_msat, | ||
| effective_capacity, | ||
| }; | ||
| let channel_penalty_msat = scorer.channel_penalty_msat( | ||
| short_channel_id, &$src_node_id, &$dest_node_id, channel_usage | ||
| @@ -1505,12 +1551,14 @@ where L::Target: Logger { | ||
| .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); | ||
| let hop_capacity = hop.candidate.effective_capacity(); | ||
| let hop_max_msat = max_htlc_from_capacity(hop_capacity, channel_saturation_pow_half); | ||
tnull marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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_max_msat); | ||
| } | ||
| if !prevented_redundant_path_selection { | ||
| // If we weren't capped by hitting a liquidity limit on a channel in the path, | ||
| @@ -1551,6 +1599,10 @@ where L::Target: Logger { | ||
| } | ||
| if !allow_mpp { | ||
| if !found_new_path && channel_saturation_pow_half != 0 { | ||
| channel_saturation_pow_half = 0; | ||
tnull marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| continue 'paths_collection; | ||
| } | ||
| // If we don't support MPP, no use trying to gather more value ever. | ||
| break 'paths_collection; | ||
| } | ||
| @@ -1560,7 +1612,9 @@ where L::Target: Logger { | ||
| // iteration. | ||
| // In the latter case, making another path finding attempt won't help, | ||
| // because we deterministically terminated the search due to low liquidity. | ||
| 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; | ||
| @@ -1676,8 +1730,32 @@ where L::Target: Logger { | ||
| // 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 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)})?, | ||
| @@ -4763,17 +4841,18 @@ mod tests { | ||
| // Get a route for 100 sats and check that we found the MPP route no problem and didn't | ||
| // overpay at all. | ||
| 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); | ||
| 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); | ||
| } | ||
| @@ -4787,7 +4866,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()) | ||
| .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). | ||
| @@ -5235,12 +5315,13 @@ mod tests { | ||
| 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].fee_msat, 50_000); | ||
| assert_eq!(route.paths[1][0].pubkey, nodes[0]); | ||
| assert_eq!(route.paths[1][0].fee_msat, 50_000); | ||
| } | ||
| @@ -5641,6 +5722,50 @@ mod tests { | ||
| } | ||
| } | ||
| #[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. | ||
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think$\frac{1}{2}^x$ should generally work fine, but why don't we allow to just define a simple percentage or share $x \in [0,1]$ of the channel capacity here? Seems more straight forward and probably also more readable when not familiar with how this knob works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cause I really hate the idea of doing a divide in the scorer. Divides are slow, man, shifts are cheap. Quite likely it doesn't actually matter, but still.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I assumed this to be the main reason for it. While there might be ways to work around the floating point division, your solution is probably cleaner, even though it has the drawback that no saturation between 0.5 and 1.0 times the capacity can be specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, if anything I'm more worried about nothing between 1/8 and 1/4 than 1/2 and 1 :)