From d88198b09ce037507b9ab1746a59491de268ff0b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 3 Feb 2025 20:41:21 +0000 Subject: [PATCH 01/10] Ignore fees on first-hop channels from the public network graph If we have a first-hop channel from a first-hop hint, we'll ignore the fees on it as we won't charge ourselves fees. However, if we have a first-hop channel from the network graph, we should do the same. We do so here, also teeing up a coming commit which will remove much of the custom codepath for first-hop hints and start using this common codepath as well. --- lightning/src/routing/router.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index d87b5597c48..1cc8b292e95 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2514,8 +2514,15 @@ where L::Target: Logger { let curr_min = cmp::max( $next_hops_path_htlc_minimum_msat, htlc_minimum_msat ); - let candidate_fees = $candidate.fees(); let src_node_counter = $candidate.src_node_counter(); + let mut candidate_fees = $candidate.fees(); + if src_node_counter == payer_node_counter { + // We do not charge ourselves a fee to use our own channels. + candidate_fees = RoutingFees { + proportional_millionths: 0, + base_msat: 0, + }; + } let path_htlc_minimum_msat = compute_fees_saturating(curr_min, candidate_fees) .saturating_add(curr_min); From 4f2ea6096921467a8beaeeff8408c4f79b8c9ce4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 3 Feb 2025 21:19:54 +0000 Subject: [PATCH 02/10] Clean up `long_mpp_route_test` and `mpp_cheaper_route_test` These tests are a bit annoying to deal with and ultimately work on almost the same graph subset, so it makes sense to combine their graph layout logic and then call it twice. We do that here, combining them and also cleaning up the possible paths as there actually are paths that the router could select which don't meet the tests requirements. --- lightning/src/routing/router.rs | 287 ++++++-------------------------- 1 file changed, 55 insertions(+), 232 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 1cc8b292e95..f78e4500dcc 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -5740,187 +5740,33 @@ mod tests { } #[test] - fn long_mpp_route_test() { - let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph(); - let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); - let scorer = ln_test_utils::TestScorer::new(); - let random_seed_bytes = [42; 32]; - let config = UserConfig::default(); - let payment_params = PaymentParameters::from_node_id(nodes[3], 42) - .with_bolt11_features(channelmanager::provided_bolt11_invoice_features(&config)) - .unwrap(); - - // We need a route consisting of 3 paths: - // From our node to node3 via {node0, node2}, {node7, node2, node4} and {node7, node2}. - // Note that these paths overlap (channels 5, 12, 13). - // We will route 300 sats. - // Each path will have 100 sats capacity, those channels which - // are used twice will have 200 sats capacity. - - // Disable other potential paths. - update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 2, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 2, - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 100_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 7, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 2, - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 100_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - - // Path via {node0, node2} is channels {1, 3, 5}. - update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 1, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 0, - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 100_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - update_channel(&gossip_sync, &secp_ctx, &privkeys[0], UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 3, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 0, - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 100_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - - // Capacity of 200 sats because this channel will be used by 3rd path as well. - add_channel(&gossip_sync, &secp_ctx, &privkeys[2], &privkeys[3], ChannelFeatures::from_le_bytes(id_to_feature_flags(5)), 5); - update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 5, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 0, - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 200_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - update_channel(&gossip_sync, &secp_ctx, &privkeys[3], UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 5, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 3, // disable direction 1 - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 200_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - - // Path via {node7, node2, node4} is channels {12, 13, 6, 11}. - // Add 100 sats to the capacities of {12, 13}, because these channels - // are also used for 3rd path. 100 sats for the rest. Total capacity: 100 sats. - update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 12, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 0, - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 200_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - update_channel(&gossip_sync, &secp_ctx, &privkeys[7], UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 13, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 0, - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 200_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - - update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 6, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 0, - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 100_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - update_channel(&gossip_sync, &secp_ctx, &privkeys[4], UnsignedChannelUpdate { - chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 11, - timestamp: 2, - message_flags: 1, // Only must_be_one - channel_flags: 0, - cltv_expiry_delta: 0, - htlc_minimum_msat: 0, - htlc_maximum_msat: 100_000, - fee_base_msat: 0, - fee_proportional_millionths: 0, - excess_data: Vec::new() - }); - - // Path via {node7, node2} is channels {12, 13, 5}. - // We already limited them to 200 sats (they are used twice for 100 sats). - // Nothing to do here. - + fn mpp_tests() { + let secp_ctx = Secp256k1::new(); + let (_, _, _, nodes) = get_nodes(&secp_ctx); { - // Attempt to route more than available results in a failure. - let route_params = RouteParameters::from_payment_params_and_value( - payment_params.clone(), 350_000); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( - &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), - &scorer, &Default::default(), &random_seed_bytes) { - assert_eq!(err, "Failed to find a sufficient route to the given destination"); - } else { panic!(); } - } + // Check that if we have two cheaper paths and a more expensive (fewer hops) path, we + // choose the two cheaper paths: + let route = do_mpp_route_tests(180_000).unwrap(); + assert_eq!(route.paths.len(), 2); + let mut total_value_transferred_msat = 0; + let mut total_paid_msat = 0; + for path in &route.paths { + assert_eq!(path.hops.last().unwrap().pubkey, nodes[3]); + total_value_transferred_msat += path.final_value_msat(); + for hop in &path.hops { + total_paid_msat += hop.fee_msat; + } + } + // If we paid fee, this would be higher. + assert_eq!(total_value_transferred_msat, 180_000); + let total_fees_paid = total_paid_msat - total_value_transferred_msat; + assert_eq!(total_fees_paid, 0); + } { - // Now, attempt to route 300 sats (exact amount we can route). - // Our algorithm should provide us with these 3 paths, 100 sats each. - let route_params = RouteParameters::from_payment_params_and_value( - payment_params, 300_000); - let route = get_route(&our_id, &route_params, &network_graph.read_only(), None, - Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap(); + // Check that if we use the same channels but need to send more than we could fit in + // the cheaper paths we select all three paths: + let route = do_mpp_route_tests(300_000).unwrap(); assert_eq!(route.paths.len(), 3); let mut total_amount_paid_msat = 0; @@ -5930,11 +5776,11 @@ mod tests { } assert_eq!(total_amount_paid_msat, 300_000); } - + // Check that trying to pay more than our available liquidity fails. + assert!(do_mpp_route_tests(300_001).is_err()); } - #[test] - fn mpp_cheaper_route_test() { + fn do_mpp_route_tests(amt: u64) -> Result { let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); let scorer = ln_test_utils::TestScorer::new(); @@ -5944,21 +5790,17 @@ mod tests { .with_bolt11_features(channelmanager::provided_bolt11_invoice_features(&config)) .unwrap(); - // This test checks that if we have two cheaper paths and one more expensive path, - // so that liquidity-wise any 2 of 3 combination is sufficient, - // two cheaper paths will be taken. - // These paths have equal available liquidity. - - // We need a combination of 3 paths: - // From our node to node3 via {node0, node2}, {node7, node2, node4} and {node7, node2}. - // Note that these paths overlap (channels 5, 12, 13). - // Each path will have 100 sats capacity, those channels which - // are used twice will have 200 sats capacity. + // Build a setup where we have three potential paths from us to node3: + // {node0, node2, node4} (channels 1, 3, 6, 11), fee 0 msat, + // {node7, node2, node4} (channels 12, 13, 6, 11), fee 0 msat, and + // {node1} (channel 2, then a new channel 16), fee 1000 msat. + // Note that these paths overlap on channels 6 and 11. + // Each channel will have 100 sats capacity except for 6 and 11, which have 200. // Disable other potential paths. - update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate { + update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate { chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 2, + short_channel_id: 7, timestamp: 2, message_flags: 1, // Only must_be_one channel_flags: 2, @@ -5969,9 +5811,9 @@ mod tests { fee_proportional_millionths: 0, excess_data: Vec::new() }); - update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate { + update_channel(&gossip_sync, &secp_ctx, &privkeys[1], UnsignedChannelUpdate { chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 7, + short_channel_id: 4, timestamp: 2, message_flags: 1, // Only must_be_one channel_flags: 2, @@ -6011,31 +5853,30 @@ mod tests { excess_data: Vec::new() }); - // Capacity of 200 sats because this channel will be used by 3rd path as well. - add_channel(&gossip_sync, &secp_ctx, &privkeys[2], &privkeys[3], ChannelFeatures::from_le_bytes(id_to_feature_flags(5)), 5); - update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate { + add_channel(&gossip_sync, &secp_ctx, &privkeys[1], &privkeys[3], ChannelFeatures::from_le_bytes(id_to_feature_flags(16)), 16); + update_channel(&gossip_sync, &secp_ctx, &privkeys[1], UnsignedChannelUpdate { chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 5, + short_channel_id: 16, timestamp: 2, message_flags: 1, // Only must_be_one channel_flags: 0, cltv_expiry_delta: 0, htlc_minimum_msat: 0, - htlc_maximum_msat: 200_000, - fee_base_msat: 0, + htlc_maximum_msat: 100_000, + fee_base_msat: 1_000, fee_proportional_millionths: 0, excess_data: Vec::new() }); update_channel(&gossip_sync, &secp_ctx, &privkeys[3], UnsignedChannelUpdate { chain_hash: ChainHash::using_genesis_block(Network::Testnet), - short_channel_id: 5, + short_channel_id: 16, timestamp: 2, message_flags: 1, // Only must_be_one channel_flags: 3, // disable direction 1 cltv_expiry_delta: 0, htlc_minimum_msat: 0, - htlc_maximum_msat: 200_000, - fee_base_msat: 0, + htlc_maximum_msat: 100_000, + fee_base_msat: 1_000, fee_proportional_millionths: 0, excess_data: Vec::new() }); @@ -6051,7 +5892,7 @@ mod tests { channel_flags: 0, cltv_expiry_delta: 0, htlc_minimum_msat: 0, - htlc_maximum_msat: 200_000, + htlc_maximum_msat: 100_000, fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: Vec::new() @@ -6064,7 +5905,7 @@ mod tests { channel_flags: 0, cltv_expiry_delta: 0, htlc_minimum_msat: 0, - htlc_maximum_msat: 200_000, + htlc_maximum_msat: 100_000, fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: Vec::new() @@ -6078,8 +5919,8 @@ mod tests { channel_flags: 0, cltv_expiry_delta: 0, htlc_minimum_msat: 0, - htlc_maximum_msat: 100_000, - fee_base_msat: 1_000, + htlc_maximum_msat: 200_000, + fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: Vec::new() }); @@ -6091,7 +5932,7 @@ mod tests { channel_flags: 0, cltv_expiry_delta: 0, htlc_minimum_msat: 0, - htlc_maximum_msat: 100_000, + htlc_maximum_msat: 200_000, fee_base_msat: 0, fee_proportional_millionths: 0, excess_data: Vec::new() @@ -6101,29 +5942,11 @@ mod tests { // We already limited them to 200 sats (they are used twice for 100 sats). // Nothing to do here. - { - // Now, attempt to route 180 sats. - // Our algorithm should provide us with these 2 paths. - let route_params = RouteParameters::from_payment_params_and_value( - payment_params, 180_000); - let route = get_route(&our_id, &route_params, &network_graph.read_only(), None, - Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes).unwrap(); - assert_eq!(route.paths.len(), 2); - - let mut total_value_transferred_msat = 0; - let mut total_paid_msat = 0; - for path in &route.paths { - assert_eq!(path.hops.last().unwrap().pubkey, nodes[3]); - total_value_transferred_msat += path.final_value_msat(); - for hop in &path.hops { - total_paid_msat += hop.fee_msat; - } - } - // If we paid fee, this would be higher. - assert_eq!(total_value_transferred_msat, 180_000); - let total_fees_paid = total_paid_msat - total_value_transferred_msat; - assert_eq!(total_fees_paid, 0); - } + let route_params = RouteParameters::from_payment_params_and_value( + payment_params, amt); + let res = get_route(&our_id, &route_params, &network_graph.read_only(), None, + Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes); + res } #[test] From 9d17f5532f58e61748d737332a8517b931b70ea8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 3 Feb 2025 21:30:41 +0000 Subject: [PATCH 03/10] Track `node_counter` in `RouteGraphNode` In a coming commit we'll start calling `add_entries_to_cheapest_to_target_node` without always having a public-graph node entry in order to process last- and first-hops via a common codepath. In order to do so, we always need the `node_counter` for the node, however, and thus we track them in `RouteGraphNode` and pass them through to `add_entries_to_cheapest_to_target_node` here. We also take this opportunity to swap the node preference logic to look at the counters, which is slightly less computational work, though it does require some unrelated test changes. --- lightning/src/ln/reload_tests.rs | 1 + lightning/src/routing/router.rs | 15 ++++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 09e9c8278c2..24d5b0987a9 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -964,6 +964,7 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) { // Ensure that the remaining channel is fully operation and not blocked (and that after a // cycle of commitment updates the payment preimage is ultimately pruned). + nodes[0].node.peer_disconnected(nodes[1].node.get_our_node_id()); send_payment(&nodes[0], &[&nodes[2], &nodes[3]], 100_000); assert!(!get_monitor!(nodes[3], chan_id_not_persisted).get_stored_preimages().contains_key(&payment_hash)); } diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index f78e4500dcc..fbf093c5348 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1164,6 +1164,7 @@ impl_writeable_tlv_based!(RouteHintHop, { #[repr(align(64))] // Force the size to 64 bytes struct RouteGraphNode { node_id: NodeId, + node_counter: u32, score: u64, // The maximum value a yet-to-be-constructed payment path might flow through this node. // This value is upper-bounded by us by: @@ -1178,7 +1179,7 @@ struct RouteGraphNode { impl cmp::Ord for RouteGraphNode { fn cmp(&self, other: &RouteGraphNode) -> cmp::Ordering { - other.score.cmp(&self.score).then_with(|| other.node_id.cmp(&self.node_id)) + other.score.cmp(&self.score).then_with(|| other.node_counter.cmp(&self.node_counter)) } } @@ -2625,6 +2626,7 @@ where L::Target: Logger { if !old_entry.was_processed && new_cost < old_cost { let new_graph_node = RouteGraphNode { node_id: src_node_id, + node_counter: src_node_counter, score: cmp::max(total_fee_msat, path_htlc_minimum_msat).saturating_add(path_penalty_msat), total_cltv_delta: hop_total_cltv_delta, value_contribution_msat, @@ -2703,7 +2705,7 @@ where L::Target: Logger { // meaning how much will be paid in fees after this node (to the best of our knowledge). // This data can later be helpful to optimize routing (pay lower fees). macro_rules! add_entries_to_cheapest_to_target_node { - ( $node: expr, $node_id: expr, $next_hops_value_contribution: expr, + ( $node: expr, $node_counter: expr, $node_id: expr, $next_hops_value_contribution: expr, $next_hops_cltv_delta: expr, $next_hops_path_length: expr ) => { let fee_to_target_msat; let next_hops_path_htlc_minimum_msat; @@ -2843,7 +2845,9 @@ where L::Target: Logger { // If not, targets.pop() will not even let us enter the loop in step 2. None => {}, Some(node) => { - add_entries_to_cheapest_to_target_node!(node, payee, path_value_msat, 0, 0); + add_entries_to_cheapest_to_target_node!( + node, node.node_counter, payee, path_value_msat, 0, 0 + ); }, }); @@ -3071,7 +3075,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, total_cltv_delta, mut value_contribution_msat, path_length_to_node, .. }) = targets.pop() { + 'path_construction: while let Some(RouteGraphNode { node_id, node_counter, total_cltv_delta, mut value_contribution_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. @@ -3209,7 +3213,8 @@ where L::Target: Logger { match network_nodes.get(&node_id) { None => {}, Some(node) => { - add_entries_to_cheapest_to_target_node!(node, node_id, + add_entries_to_cheapest_to_target_node!( + node, node_counter, node_id, value_contribution_msat, total_cltv_delta, path_length_to_node); }, From ff09619a22118c77f421f0627bf6bf9a36f9e7f4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 3 Feb 2025 21:40:04 +0000 Subject: [PATCH 04/10] Prefer higher-value, shorter equal-cost paths when routing This likely only impacts very rare edge cases, but if we have two equal-cost paths, we should likely prefer ones which contribute more value (avoiding cases where we use paths which are amount-limited but equal fee to higher-amount paths) and then paths with fewer hops (which may complete faster). It does make test behavior more robust against router changes, which comes in handy over the coming commits. --- lightning/src/routing/router.rs | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index fbf093c5348..30f62505e16 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1179,7 +1179,10 @@ struct RouteGraphNode { impl cmp::Ord for RouteGraphNode { fn cmp(&self, other: &RouteGraphNode) -> cmp::Ordering { - other.score.cmp(&self.score).then_with(|| other.node_counter.cmp(&self.node_counter)) + other.score.cmp(&self.score) + .then_with(|| self.value_contribution_msat.cmp(&other.value_contribution_msat)) + .then_with(|| other.path_length_to_node.cmp(&self.path_length_to_node)) + .then_with(|| other.node_counter.cmp(&self.node_counter)) } } @@ -1809,11 +1812,7 @@ struct PathBuildingHop<'a> { /// The value will be actually deducted from the counterparty balance on the previous link. hop_use_fee_msat: u64, - #[cfg(all(not(ldk_bench), any(test, fuzzing)))] - // In tests, we apply further sanity checks on cases where we skip nodes we already processed - // to ensure it is specifically in cases where the fee has gone down because of a decrease in - // value_contribution_msat, which requires tracking it here. See comments below where it is - // used for more info. + /// The quantity of funds we're willing to route over this channel value_contribution_msat: u64, } @@ -1834,9 +1833,7 @@ impl<'a> core::fmt::Debug for PathBuildingHop<'a> { .field("total_fee_msat - (next_hops_fee_msat + hop_use_fee_msat)", &(&self.total_fee_msat.saturating_sub(self.next_hops_fee_msat).saturating_sub(self.hop_use_fee_msat))) .field("path_penalty_msat", &self.path_penalty_msat) .field("path_htlc_minimum_msat", &self.path_htlc_minimum_msat) - .field("cltv_expiry_delta", &self.candidate.cltv_expiry_delta()); - #[cfg(all(not(ldk_bench), any(test, fuzzing)))] - let debug_struct = debug_struct + .field("cltv_expiry_delta", &self.candidate.cltv_expiry_delta()) .field("value_contribution_msat", &self.value_contribution_msat); debug_struct.finish() } @@ -2546,7 +2543,6 @@ where L::Target: Logger { path_penalty_msat: u64::max_value(), was_processed: false, is_first_hop_target: false, - #[cfg(all(not(ldk_bench), any(test, fuzzing)))] value_contribution_msat, }); dist_entry.as_mut().unwrap() @@ -2622,8 +2618,11 @@ where L::Target: Logger { .saturating_add(old_entry.path_penalty_msat); let new_cost = cmp::max(total_fee_msat, path_htlc_minimum_msat) .saturating_add(path_penalty_msat); + let should_replace = + new_cost < old_cost + || (new_cost == old_cost && old_entry.value_contribution_msat < value_contribution_msat); - if !old_entry.was_processed && new_cost < old_cost { + if !old_entry.was_processed && should_replace { let new_graph_node = RouteGraphNode { node_id: src_node_id, node_counter: src_node_counter, @@ -2640,10 +2639,7 @@ where L::Target: Logger { old_entry.fee_msat = 0; // This value will be later filled with hop_use_fee_msat of the following channel old_entry.path_htlc_minimum_msat = path_htlc_minimum_msat; old_entry.path_penalty_msat = path_penalty_msat; - #[cfg(all(not(ldk_bench), any(test, fuzzing)))] - { - old_entry.value_contribution_msat = value_contribution_msat; - } + old_entry.value_contribution_msat = value_contribution_msat; hop_contribution_amt_msat = Some(value_contribution_msat); } else if old_entry.was_processed && new_cost < old_cost { #[cfg(all(not(ldk_bench), any(test, fuzzing)))] @@ -2814,7 +2810,6 @@ where L::Target: Logger { path_penalty_msat: u64::max_value(), was_processed: false, is_first_hop_target: true, - #[cfg(all(not(ldk_bench), any(test, fuzzing)))] value_contribution_msat: 0, }); } From 0351a24722e206ca3808943b1bd31b56331a04f1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 3 Feb 2025 22:21:18 +0000 Subject: [PATCH 05/10] Move last-hop route handling to the common "normal" hop codepath When we handle the unblinded last-hop route hints from an invoice, we had a good bit of code dedicated to handling fee propagation through the (potentially) multiple last-hops and connecting them to potentially directly-connected first-hops. This was a good bit of code that was almost never used, and it turns out was also buggy - we could process a route hint with multiple hops, committing to one path through nodes A, B, to C, then process another route hint (or public channel) which changes our best path from B to C, making the A entry invalid. Here we remove the whole maze, utilizing the normal hop-processing logic in `add_entries_to_cheapest_to_target_node` for last-hops as well. It requires tracking which nodes connect to last-hop hints similar to the way we do with `is_first_hop_target` in `PathBuildingHop`, storing the `CandidateRouteHop`s in a new map, and always calling `add_entries_to_cheapest_to_target_node` on the payee node, whether its public or not. --- lightning/src/routing/router.rs | 381 +++++++++++++------------------- 1 file changed, 150 insertions(+), 231 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 30f62505e16..4f247033e86 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1792,6 +1792,8 @@ struct PathBuildingHop<'a> { /// updated after being initialized - it is set at the start of a route-finding pass and only /// read thereafter. is_first_hop_target: bool, + /// Identical to the above, but for handling unblinded last-hops rather than first-hops. + is_last_hop_target: bool, /// Used to compare channels when choosing the for routing. /// Includes paying for the use of a hop and the following hops, as well as /// an estimated cost of reaching this hop. @@ -1827,6 +1829,7 @@ impl<'a> core::fmt::Debug for PathBuildingHop<'a> { .field("target_node_id", &self.candidate.target()) .field("short_channel_id", &self.candidate.short_channel_id()) .field("is_first_hop_target", &self.is_first_hop_target) + .field("is_last_hop_target", &self.is_last_hop_target) .field("total_fee_msat", &self.total_fee_msat) .field("next_hops_fee_msat", &self.next_hops_fee_msat) .field("hop_use_fee_msat", &self.hop_use_fee_msat) @@ -2264,8 +2267,10 @@ where L::Target: Logger { // Step (1). Prepare first and last hop targets. // - // First cache all our direct channels so that we can insert them in the heap at startup. - // Then process any blinded routes, resolving their introduction node and caching it. + // For unblinded first- and last-hop channels, cache them in maps so that we can detect them as + // we walk the graph and incorporate them into our candidate set. + // For blinded last-hop paths, look up their introduction point and cache the node counters + // identifying them. let mut first_hop_targets: HashMap<_, (Vec<&ChannelDetails>, u32)> = hash_map_with_capacity(if first_hops.is_some() { first_hops.as_ref().unwrap().len() } else { 0 }); if let Some(hops) = first_hops { @@ -2297,6 +2302,48 @@ where L::Target: Logger { &payment_params, &node_counters, network_graph, &logger, our_node_id, &first_hop_targets, )?; + let mut last_hop_candidates = + hash_map_with_capacity(payment_params.payee.unblinded_route_hints().len()); + for route in payment_params.payee.unblinded_route_hints().iter() + .filter(|route| !route.0.is_empty()) + { + let hop_iter = route.0.iter().rev(); + let prev_hop_iter = core::iter::once(&maybe_dummy_payee_pk).chain( + route.0.iter().skip(1).rev().map(|hop| &hop.src_node_id)); + + for (hop, prev_hop_id) in hop_iter.zip(prev_hop_iter) { + let (target, private_target_node_counter) = + node_counters.private_node_counter_from_pubkey(&prev_hop_id) + .expect("node_counter_from_pubkey is called on all unblinded_route_hints keys above, so is always Some here"); + let (_src_id, private_source_node_counter) = + node_counters.private_node_counter_from_pubkey(&hop.src_node_id) + .expect("node_counter_from_pubkey is called on all unblinded_route_hints keys above, so is always Some here"); + + if let Some((first_channels, _)) = first_hop_targets.get(target) { + if first_channels.iter().any(|d| d.outbound_scid_alias == Some(hop.short_channel_id)) { + log_trace!(logger, "Ignoring route hint with SCID {} (and any previous) due to it being a direct channel of ours.", + hop.short_channel_id); + break; + } + } + + let candidate = network_channels + .get(&hop.short_channel_id) + .and_then(|channel| channel.as_directed_to(target)) + .map(|(info, _)| CandidateRouteHop::PublicHop(PublicHopCandidate { + info, + short_channel_id: hop.short_channel_id, + })) + .unwrap_or_else(|| CandidateRouteHop::PrivateHop(PrivateHopCandidate { + hint: hop, target_node_id: target, + source_node_counter: *private_source_node_counter, + target_node_counter: *private_target_node_counter, + })); + + last_hop_candidates.entry(private_target_node_counter).or_insert_with(Vec::new).push(candidate); + } + } + // The main heap containing all candidate next-hops sorted by their score (max(fee, // htlc_minimum)). Ideally this would be a heap which allowed cheap score reduction instead of // adding duplicate entries when we find a better path to a given node. @@ -2543,6 +2590,7 @@ where L::Target: Logger { path_penalty_msat: u64::max_value(), was_processed: false, is_first_hop_target: false, + is_last_hop_target: false, value_contribution_msat, }); dist_entry.as_mut().unwrap() @@ -2706,14 +2754,15 @@ where L::Target: Logger { let fee_to_target_msat; let next_hops_path_htlc_minimum_msat; let next_hops_path_penalty_msat; - let is_first_hop_target; - let skip_node = if let Some(elem) = &mut dist[$node.node_counter as usize] { + let (is_first_hop_target, is_last_hop_target); + let skip_node = if let Some(elem) = &mut dist[$node_counter as usize] { let was_processed = elem.was_processed; elem.was_processed = true; fee_to_target_msat = elem.total_fee_msat; next_hops_path_htlc_minimum_msat = elem.path_htlc_minimum_msat; next_hops_path_penalty_msat = elem.path_penalty_msat; is_first_hop_target = elem.is_first_hop_target; + is_last_hop_target = elem.is_last_hop_target; was_processed } else { // Entries are added to dist in add_entry!() when there is a channel from a node. @@ -2725,17 +2774,28 @@ where L::Target: Logger { next_hops_path_htlc_minimum_msat = 0; next_hops_path_penalty_msat = 0; is_first_hop_target = false; + is_last_hop_target = false; false }; if !skip_node { + if is_last_hop_target { + if let Some(candidates) = last_hop_candidates.get(&$node_counter) { + for candidate in candidates { + add_entry!(candidate, fee_to_target_msat, + $next_hops_value_contribution, + next_hops_path_htlc_minimum_msat, next_hops_path_penalty_msat, + $next_hops_cltv_delta, $next_hops_path_length); + } + } + } if is_first_hop_target { if let Some((first_channels, peer_node_counter)) = first_hop_targets.get(&$node_id) { for details in first_channels { - debug_assert_eq!(*peer_node_counter, $node.node_counter); + debug_assert_eq!(*peer_node_counter, $node_counter); let candidate = CandidateRouteHop::FirstHop(FirstHopCandidate { details, payer_node_id: &our_node_id, payer_node_counter, - target_node_counter: $node.node_counter, + target_node_counter: $node_counter, }); add_entry!(&candidate, fee_to_target_msat, $next_hops_value_contribution, @@ -2745,29 +2805,31 @@ where L::Target: Logger { } } - let features = if let Some(node_info) = $node.announcement_info.as_ref() { - &node_info.features() - } else { - &default_node_features - }; + if let Some(node) = $node { + let features = if let Some(node_info) = node.announcement_info.as_ref() { + &node_info.features() + } else { + &default_node_features + }; - if !features.requires_unknown_bits() { - for chan_id in $node.channels.iter() { - let chan = network_channels.get(chan_id).unwrap(); - if !chan.features.requires_unknown_bits() { - if let Some((directed_channel, source)) = chan.as_directed_to(&$node_id) { - if first_hops.is_none() || *source != our_node_id { - if directed_channel.direction().enabled { - let candidate = CandidateRouteHop::PublicHop(PublicHopCandidate { - info: directed_channel, - short_channel_id: *chan_id, - }); - add_entry!(&candidate, - fee_to_target_msat, - $next_hops_value_contribution, - next_hops_path_htlc_minimum_msat, - next_hops_path_penalty_msat, - $next_hops_cltv_delta, $next_hops_path_length); + if !features.requires_unknown_bits() { + for chan_id in node.channels.iter() { + let chan = network_channels.get(chan_id).unwrap(); + if !chan.features.requires_unknown_bits() { + if let Some((directed_channel, source)) = chan.as_directed_to(&$node_id) { + if first_hops.is_none() || *source != our_node_id { + if directed_channel.direction().enabled { + let candidate = CandidateRouteHop::PublicHop(PublicHopCandidate { + info: directed_channel, + short_channel_id: *chan_id, + }); + add_entry!(&candidate, + fee_to_target_msat, + $next_hops_value_contribution, + next_hops_path_htlc_minimum_msat, + next_hops_path_penalty_msat, + $next_hops_cltv_delta, $next_hops_path_length); + } } } } @@ -2788,13 +2850,23 @@ where L::Target: Logger { for e in dist.iter_mut() { *e = None; } + + // Step (2). + // Add entries for first-hop and last-hop channel hints to `dist` and add the payee node as + // the best entry via `add_entry`. + // For first- and last-hop hints we need only add dummy entries in `dist` with the relevant + // flags set. As we walk the graph in `add_entries_to_cheapest_to_target_node` we'll check + // those flags and add the channels described by the hints. + // We then either add the payee using `add_entries_to_cheapest_to_target_node` or add the + // blinded paths to the payee using `add_entry`, filling `targets` and setting us up for + // our graph walk. for (_, (chans, peer_node_counter)) in first_hop_targets.iter() { // In order to avoid looking up whether each node is a first-hop target, we store a // dummy entry in dist for each first-hop target, allowing us to do this lookup for // free since we're already looking at the `was_processed` flag. // - // Note that all the fields (except `is_first_hop_target`) will be overwritten whenever - // we find a path to the target, so are left as dummies here. + // Note that all the fields (except `is_{first,last}_hop_target`) will be overwritten + // whenever we find a path to the target, so are left as dummies here. dist[*peer_node_counter as usize] = Some(PathBuildingHop { candidate: CandidateRouteHop::FirstHop(FirstHopCandidate { details: &chans[0], @@ -2810,46 +2882,56 @@ where L::Target: Logger { path_penalty_msat: u64::max_value(), was_processed: false, is_first_hop_target: true, + is_last_hop_target: false, value_contribution_msat: 0, }); } - hit_minimum_limit = false; - - // If first hop is a private channel and the only way to reach the payee, this is the only - // place where it could be added. - payee_node_id_opt.map(|payee| first_hop_targets.get(&payee).map(|(first_channels, peer_node_counter)| { - debug_assert_eq!(*peer_node_counter, payee_node_counter); - for details in first_channels { - let candidate = CandidateRouteHop::FirstHop(FirstHopCandidate { - details, payer_node_id: &our_node_id, payer_node_counter, - target_node_counter: payee_node_counter, + for (target_node_counter, candidates) in last_hop_candidates.iter() { + // In order to avoid looking up whether each node is a last-hop target, we store a + // dummy entry in dist for each last-hop target, allowing us to do this lookup for + // free since we're already looking at the `was_processed` flag. + // + // Note that all the fields (except `is_{first,last}_hop_target`) will be overwritten + // whenever we find a path to the target, so are left as dummies here. + debug_assert!(!candidates.is_empty()); + if candidates.is_empty() { continue } + let entry = &mut dist[**target_node_counter as usize]; + if let Some(hop) = entry { + hop.is_last_hop_target = true; + } else { + *entry = Some(PathBuildingHop { + candidate: candidates[0].clone(), + fee_msat: 0, + next_hops_fee_msat: u64::max_value(), + hop_use_fee_msat: u64::max_value(), + total_fee_msat: u64::max_value(), + path_htlc_minimum_msat: u64::max_value(), + path_penalty_msat: u64::max_value(), + was_processed: false, + is_first_hop_target: false, + is_last_hop_target: true, + value_contribution_msat: 0, }); - let added = add_entry!(&candidate, 0, path_value_msat, - 0, 0u64, 0, 0).is_some(); - log_trace!(logger, "{} direct route to payee via {}", - if added { "Added" } else { "Skipped" }, LoggedCandidateHop(&candidate)); } - })); + } + hit_minimum_limit = false; - // Add the payee as a target, so that the payee-to-payer - // search algorithm knows what to start with. - payee_node_id_opt.map(|payee| match network_nodes.get(&payee) { - // The payee is not in our network graph, so nothing to add here. - // There is still a chance of reaching them via last_hops though, - // so don't yet fail the payment here. - // If not, targets.pop() will not even let us enter the loop in step 2. - None => {}, - Some(node) => { - add_entries_to_cheapest_to_target_node!( - node, node.node_counter, payee, path_value_msat, 0, 0 - ); - }, - }); + if let Some(payee) = payee_node_id_opt { + if let Some(entry) = &mut dist[payee_node_counter as usize] { + // If we built a dummy entry above we need to reset the values to represent 0 fee + // from the target "to the target". + entry.next_hops_fee_msat = 0; + entry.hop_use_fee_msat = 0; + entry.total_fee_msat = 0; + entry.path_htlc_minimum_msat = 0; + entry.path_penalty_msat = 0; + entry.value_contribution_msat = path_value_msat; + } + add_entries_to_cheapest_to_target_node!( + network_nodes.get(&payee), payee_node_counter, payee, path_value_msat, 0, 0 + ); + } - // Step (2). - // If a caller provided us with last hops, add them to routing targets. Since this happens - // earlier than general path finding, they will be somewhat prioritized, although currently - // it matters only if the fees are exactly the same. debug_assert_eq!( payment_params.payee.blinded_route_hints().len(), introduction_node_id_cache.len(), @@ -2895,165 +2977,6 @@ where L::Target: Logger { } } } - for route in payment_params.payee.unblinded_route_hints().iter() - .filter(|route| !route.0.is_empty()) - { - let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id); - let first_hop_src_is_reachable = - // Only add the hops in this route to our candidate set if either we are part of - // the first hop, we have a direct channel to the first hop, or the first hop is in - // the regular network graph. - our_node_id == first_hop_src_id || - first_hop_targets.get(&first_hop_src_id).is_some() || - network_nodes.get(&first_hop_src_id).is_some(); - if first_hop_src_is_reachable { - // We start building the path from reverse, i.e., from payee - // to the first RouteHintHop in the path. - let hop_iter = route.0.iter().rev(); - let prev_hop_iter = core::iter::once(&maybe_dummy_payee_pk).chain( - route.0.iter().skip(1).rev().map(|hop| &hop.src_node_id)); - let mut hop_used = true; - let mut aggregate_next_hops_fee_msat: u64 = 0; - let mut aggregate_next_hops_path_htlc_minimum_msat: u64 = 0; - let mut aggregate_next_hops_path_penalty_msat: u64 = 0; - let mut aggregate_next_hops_cltv_delta: u32 = 0; - let mut aggregate_next_hops_path_length: u8 = 0; - let mut aggregate_path_contribution_msat = path_value_msat; - - for (idx, (hop, prev_hop_id)) in hop_iter.zip(prev_hop_iter).enumerate() { - let (target, private_target_node_counter) = - node_counters.private_node_counter_from_pubkey(&prev_hop_id) - .expect("node_counter_from_pubkey is called on all unblinded_route_hints keys during setup, so is always Some here"); - let (_src_id, private_source_node_counter) = - node_counters.private_node_counter_from_pubkey(&hop.src_node_id) - .expect("node_counter_from_pubkey is called on all unblinded_route_hints keys during setup, so is always Some here"); - - if let Some((first_channels, _)) = first_hop_targets.get(target) { - if first_channels.iter().any(|d| d.outbound_scid_alias == Some(hop.short_channel_id)) { - log_trace!(logger, "Ignoring route hint with SCID {} (and any previous) due to it being a direct channel of ours.", - hop.short_channel_id); - break; - } - } - - let candidate = network_channels - .get(&hop.short_channel_id) - .and_then(|channel| channel.as_directed_to(target)) - .map(|(info, _)| CandidateRouteHop::PublicHop(PublicHopCandidate { - info, - short_channel_id: hop.short_channel_id, - })) - .unwrap_or_else(|| CandidateRouteHop::PrivateHop(PrivateHopCandidate { - hint: hop, target_node_id: target, - source_node_counter: *private_source_node_counter, - target_node_counter: *private_target_node_counter, - })); - - if let Some(hop_used_msat) = add_entry!(&candidate, - aggregate_next_hops_fee_msat, aggregate_path_contribution_msat, - aggregate_next_hops_path_htlc_minimum_msat, aggregate_next_hops_path_penalty_msat, - aggregate_next_hops_cltv_delta, aggregate_next_hops_path_length) - { - aggregate_path_contribution_msat = hop_used_msat; - } else { - // If this hop was not used then there is no use checking the preceding - // hops in the RouteHint. We can break by just searching for a direct - // channel between last checked hop and first_hop_targets. - hop_used = false; - } - - let used_liquidity_msat = used_liquidities - .get(&candidate.id()).copied() - .unwrap_or(0); - let channel_usage = ChannelUsage { - amount_msat: final_value_msat + aggregate_next_hops_fee_msat, - inflight_htlc_msat: used_liquidity_msat, - effective_capacity: candidate.effective_capacity(), - }; - let channel_penalty_msat = scorer.channel_penalty_msat( - &candidate, channel_usage, score_params - ); - aggregate_next_hops_path_penalty_msat = aggregate_next_hops_path_penalty_msat - .saturating_add(channel_penalty_msat); - - aggregate_next_hops_cltv_delta = aggregate_next_hops_cltv_delta - .saturating_add(hop.cltv_expiry_delta as u32); - - aggregate_next_hops_path_length = aggregate_next_hops_path_length - .saturating_add(1); - - // Searching for a direct channel between last checked hop and first_hop_targets - if let Some((first_channels, peer_node_counter)) = first_hop_targets.get_mut(target) { - sort_first_hop_channels( - first_channels, &used_liquidities, recommended_value_msat, our_node_pubkey - ); - for details in first_channels { - let first_hop_candidate = CandidateRouteHop::FirstHop(FirstHopCandidate { - details, payer_node_id: &our_node_id, payer_node_counter, - target_node_counter: *peer_node_counter, - }); - add_entry!(&first_hop_candidate, - aggregate_next_hops_fee_msat, aggregate_path_contribution_msat, - aggregate_next_hops_path_htlc_minimum_msat, aggregate_next_hops_path_penalty_msat, - aggregate_next_hops_cltv_delta, aggregate_next_hops_path_length); - } - } - - if !hop_used { - break; - } - - // In the next values of the iterator, the aggregate fees already reflects - // the sum of value sent from payer (final_value_msat) and routing fees - // for the last node in the RouteHint. We need to just add the fees to - // route through the current node so that the preceding node (next iteration) - // can use it. - let hops_fee = compute_fees(aggregate_next_hops_fee_msat + final_value_msat, hop.fees) - .map_or(None, |inc| inc.checked_add(aggregate_next_hops_fee_msat)); - aggregate_next_hops_fee_msat = if let Some(val) = hops_fee { val } else { break; }; - - // The next channel will need to relay this channel's min_htlc *plus* the fees taken by - // this route hint's source node to forward said min over this channel. - aggregate_next_hops_path_htlc_minimum_msat = { - let curr_htlc_min = cmp::max( - candidate.htlc_minimum_msat(), aggregate_next_hops_path_htlc_minimum_msat - ); - let curr_htlc_min_fee = if let Some(val) = compute_fees(curr_htlc_min, hop.fees) { val } else { break }; - if let Some(min) = curr_htlc_min.checked_add(curr_htlc_min_fee) { min } else { break } - }; - - if idx == route.0.len() - 1 { - // The last hop in this iterator is the first hop in - // overall RouteHint. - // If this hop connects to a node with which we have a direct channel, - // ignore the network graph and, if the last hop was added, add our - // direct channel to the candidate set. - // - // Note that we *must* check if the last hop was added as `add_entry` - // always assumes that the third argument is a node to which we have a - // path. - if let Some((first_channels, peer_node_counter)) = first_hop_targets.get_mut(&NodeId::from_pubkey(&hop.src_node_id)) { - sort_first_hop_channels( - first_channels, &used_liquidities, recommended_value_msat, our_node_pubkey - ); - for details in first_channels { - let first_hop_candidate = CandidateRouteHop::FirstHop(FirstHopCandidate { - details, payer_node_id: &our_node_id, payer_node_counter, - target_node_counter: *peer_node_counter, - }); - add_entry!(&first_hop_candidate, - aggregate_next_hops_fee_msat, - aggregate_path_contribution_msat, - aggregate_next_hops_path_htlc_minimum_msat, - aggregate_next_hops_path_penalty_msat, - aggregate_next_hops_cltv_delta, - aggregate_next_hops_path_length); - } - } - } - } - } - } log_trace!(logger, "Starting main path collection loop with {} nodes pre-filled from first/last hops.", targets.len()); @@ -3205,15 +3128,11 @@ where L::Target: Logger { // Otherwise, since the current target node is not us, // keep "unrolling" the payment graph from payee to payer by // finding a way to reach the current target from the payer side. - match network_nodes.get(&node_id) { - None => {}, - Some(node) => { - add_entries_to_cheapest_to_target_node!( - node, node_counter, node_id, - value_contribution_msat, - total_cltv_delta, path_length_to_node); - }, - } + add_entries_to_cheapest_to_target_node!( + network_nodes.get(&node_id), node_counter, node_id, + value_contribution_msat, + total_cltv_delta, path_length_to_node + ); } if !allow_mpp { From b2c635b8e0e6c5f7588b8322fc160cc56c22df63 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 2 Feb 2025 23:52:49 +0000 Subject: [PATCH 06/10] Add direct hops to intros after all blinded paths in pathfinding When we do pathfinding with blinded paths, we start each pathfinding iteration by inserting all the blinded paths into our nodes map as last-hops to the destination. As we do that, we check if any of the introduction points happen to be nodes we have direct chanels with, as we want to use the local info for such channels and support finding a path even if that channel is not publicly announced. However, as we iterate the blinded paths, we may find a second blinded path from the same introduction point which we prefer over the first. If this happens, we would already have added info from us over the local channel to that intro point and end up with calculations for the first hop to a blinded path that we no longer prefer. This is ultimately fixed here in two ways: (a) we process the first-hop channels to blinded path introduction points in a separate loop after we've processed all blinded paths, ensuring we only ever consider a channel to the blinded path we will ultimately prefer. (b) In the next commit, we add we add a new tracking bool in `PathBuildingHop` called `best_path_from_hop_selected` which we set when we process a channel backwards from a node, indicating that we've committed to the best path to the node and check when we add a new path to a node. This would have resulted in a much earlier debug-assertion in fuzzing or several tests. --- lightning/src/routing/router.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 4f247033e86..9f130556f81 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2937,6 +2937,7 @@ where L::Target: Logger { introduction_node_id_cache.len(), "introduction_node_id_cache was built by iterating the blinded_route_hints, so they should be the same len" ); + let mut blind_intros_added = hash_map_with_capacity(payment_params.payee.blinded_route_hints().len()); for (hint_idx, hint) in payment_params.payee.blinded_route_hints().iter().enumerate() { // Only add the hops in this route to our candidate set if either // we have a direct channel to the first hop or the first hop is @@ -2951,12 +2952,21 @@ where L::Target: Logger { } else { CandidateRouteHop::Blinded(BlindedPathCandidate { source_node_counter, source_node_id, hint, hint_idx }) }; - let mut path_contribution_msat = path_value_msat; if let Some(hop_used_msat) = add_entry!(&candidate, - 0, path_contribution_msat, 0, 0_u64, 0, 0) + 0, path_value_msat, 0, 0_u64, 0, 0) { - path_contribution_msat = hop_used_msat; + blind_intros_added.insert(source_node_id, (hop_used_msat, candidate)); } else { continue } + } + // If we added a blinded path from an introduction node to the destination, where the + // introduction node is one of our direct peers, we need to scan our `first_channels` + // to detect this. However, doing so immediately after calling `add_entry`, above, could + // result in incorrect behavior if we, in a later loop iteration, update the fee from the + // same introduction point to the destination (due to a different blinded path with the + // same introduction point having a lower score). + // Thus, we track the nodes that we added paths from in `blind_intros_added` and scan for + // introduction points we have a channel with after processing all blinded paths. + for (source_node_id, (path_contribution_msat, candidate)) in blind_intros_added { if let Some((first_channels, peer_node_counter)) = first_hop_targets.get_mut(source_node_id) { sort_first_hop_channels( first_channels, &used_liquidities, recommended_value_msat, our_node_pubkey From 303143a957acd8616bbbc329ecf501372644f57c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 3 Feb 2025 22:06:27 +0000 Subject: [PATCH 07/10] Add `PathBuildingHop::best_path_from_hop_selected` When we process a path backwards from a node during pathfinding, we implicitly commit to the path up to that node. Any changes to the preferred path up to that node will make the newly processed path's state invalid. In the previous few commits we fixed cases for this in last-hop paths (both blinded and unblinded). Here we add assertions to enforce this, tracked in a new bool in `PathBuildingHop`. --- lightning/src/routing/router.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 9f130556f81..ebfa37601d2 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1784,6 +1784,12 @@ struct PathBuildingHop<'a> { /// decrease as well. Thus, we have to explicitly track which nodes have been processed and /// avoid processing them again. was_processed: bool, + /// If we've already processed a channel backwards from a target node, we shouldn't update our + /// selected best path from that node to the destination. This should never happen, but with + /// multiple codepaths processing channels we've had issues here in the past, so in debug-mode + /// we track it and assert on it when processing a node. + #[cfg(all(not(ldk_bench), any(test, fuzzing)))] + best_path_from_hop_selected: bool, /// When processing a node as the next best-score candidate, we want to quickly check if it is /// a direct counterparty of ours, using our local channel information immediately if we can. /// @@ -2427,6 +2433,19 @@ where L::Target: Logger { // We "return" whether we updated the path at the end, and how much we can route via // this channel, via this: let mut hop_contribution_amt_msat = None; + + #[cfg(all(not(ldk_bench), any(test, fuzzing)))] + if let Some(counter) = $candidate.target_node_counter() { + // Once we are adding paths backwards from a given target, we've selected the best + // path from that target to the destination and it should no longer change. We thus + // set the best-path selected flag and check that it doesn't change below. + if let Some(node) = &mut dist[counter as usize] { + node.best_path_from_hop_selected = true; + } else if counter != payee_node_counter { + panic!("No dist entry for target node counter {}", counter); + } + } + // Channels to self should not be used. This is more of belt-and-suspenders, because in // practice these cases should be caught earlier: // - for regular channels at channel announcement (TODO) @@ -2591,6 +2610,8 @@ where L::Target: Logger { was_processed: false, is_first_hop_target: false, is_last_hop_target: false, + #[cfg(all(not(ldk_bench), any(test, fuzzing)))] + best_path_from_hop_selected: false, value_contribution_msat, }); dist_entry.as_mut().unwrap() @@ -2671,6 +2692,11 @@ where L::Target: Logger { || (new_cost == old_cost && old_entry.value_contribution_msat < value_contribution_msat); if !old_entry.was_processed && should_replace { + #[cfg(all(not(ldk_bench), any(test, fuzzing)))] + { + assert!(!old_entry.best_path_from_hop_selected); + } + let new_graph_node = RouteGraphNode { node_id: src_node_id, node_counter: src_node_counter, @@ -2884,6 +2910,8 @@ where L::Target: Logger { is_first_hop_target: true, is_last_hop_target: false, value_contribution_msat: 0, + #[cfg(all(not(ldk_bench), any(test, fuzzing)))] + best_path_from_hop_selected: false, }); } for (target_node_counter, candidates) in last_hop_candidates.iter() { @@ -2911,6 +2939,8 @@ where L::Target: Logger { is_first_hop_target: false, is_last_hop_target: true, value_contribution_msat: 0, + #[cfg(all(not(ldk_bench), any(test, fuzzing)))] + best_path_from_hop_selected: false, }); } } From 83ce6c463d9e0e01d19e97e7f1b4343932bd4c5f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 10 Feb 2025 20:22:28 +0000 Subject: [PATCH 08/10] Replace a few router `expect`s with `debug_assert` + `Err`-returns The router is a somewhat complicated beast, and though the last few commits removed some code from it, a complicated beast it remains. Thus, having `expect`s in it is somewhat risky, so we take this opportunity to replace some of them with `debug_assert!(false)`s and an `Err`-return. --- lightning/src/routing/router.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index ebfa37601d2..8c25492c161 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2320,10 +2320,16 @@ where L::Target: Logger { for (hop, prev_hop_id) in hop_iter.zip(prev_hop_iter) { let (target, private_target_node_counter) = node_counters.private_node_counter_from_pubkey(&prev_hop_id) - .expect("node_counter_from_pubkey is called on all unblinded_route_hints keys above, so is always Some here"); + .ok_or_else(|| { + debug_assert!(false); + LightningError { err: "We should always have private target node counters available".to_owned(), action: ErrorAction::IgnoreError } + })?; let (_src_id, private_source_node_counter) = node_counters.private_node_counter_from_pubkey(&hop.src_node_id) - .expect("node_counter_from_pubkey is called on all unblinded_route_hints keys above, so is always Some here"); + .ok_or_else(|| { + debug_assert!(false); + LightningError { err: "We should always have private source node counters available".to_owned(), action: ErrorAction::IgnoreError } + })?; if let Some((first_channels, _)) = first_hop_targets.get(target) { if first_channels.iter().any(|d| d.outbound_scid_alias == Some(hop.short_channel_id)) { From b3c1b6cda42d923294a37036919b2882fbf62f00 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 10 Feb 2025 20:26:24 +0000 Subject: [PATCH 09/10] More completely ignore route hints which are for our own channels When we see a channel come into the router as a route-hint, but its for a direct channel of ours, we'd like to ignore the route-hint as we have more information in the first-hop channel info. We do this by matching SCIDs, but only considered outbound SCID aliases. Here we change to consider both outbound SCID aliases and the full channel SCID, which some nodes may use in their invoices. --- lightning/src/routing/router.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 8c25492c161..3b6d6e0b7a0 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -2332,7 +2332,9 @@ where L::Target: Logger { })?; if let Some((first_channels, _)) = first_hop_targets.get(target) { - if first_channels.iter().any(|d| d.outbound_scid_alias == Some(hop.short_channel_id)) { + let matches_an_scid = |d: &&ChannelDetails| + d.outbound_scid_alias == Some(hop.short_channel_id) || d.short_channel_id == Some(hop.short_channel_id); + if first_channels.iter().any(matches_an_scid) { log_trace!(logger, "Ignoring route hint with SCID {} (and any previous) due to it being a direct channel of ours.", hop.short_channel_id); break; From 0f3c4d26dfdc45625e3329963cd8311b74ad0021 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 10 Feb 2025 20:43:01 +0000 Subject: [PATCH 10/10] Stop wrapping router errors in `LightningError` `LightningError` is an error type for returning errors back to the `PeerHandler` when handling P2P messages. However, it used to be more broadly used, in a way that never made any sense. Here we remove on vestige of this, using a `&'static str` for router errors rather than `LightningError` with a constant `action`. --- fuzz/src/chanmon_consistency.rs | 4 +- fuzz/src/full_stack.rs | 9 +- lightning/src/ln/functional_test_utils.rs | 4 +- lightning/src/ln/outbound_payment.rs | 14 +-- lightning/src/routing/router.rs | 141 ++++++++++------------ lightning/src/util/test_utils.rs | 7 +- 6 files changed, 81 insertions(+), 98 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 59918139af5..54c9a7e481b 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -53,7 +53,7 @@ use lightning::ln::channelmanager::{ use lightning::ln::functional_test_utils::*; use lightning::ln::inbound_payment::ExpandedKey; use lightning::ln::msgs::{ - self, ChannelMessageHandler, CommitmentUpdate, DecodeError, Init, UpdateAddHTLC, + ChannelMessageHandler, CommitmentUpdate, DecodeError, Init, UpdateAddHTLC, }; use lightning::ln::script::ShutdownScript; use lightning::ln::types::ChannelId; @@ -118,7 +118,7 @@ impl Router for FuzzRouter { fn find_route( &self, _payer: &PublicKey, _params: &RouteParameters, _first_hops: Option<&[&ChannelDetails]>, _inflight_htlcs: InFlightHtlcs, - ) -> Result { + ) -> Result { unreachable!() } diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 74eacdabd5e..7b9a9ae1d00 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -44,7 +44,7 @@ use lightning::ln::channelmanager::{ }; use lightning::ln::functional_test_utils::*; use lightning::ln::inbound_payment::ExpandedKey; -use lightning::ln::msgs::{self, DecodeError}; +use lightning::ln::msgs::DecodeError; use lightning::ln::peer_handler::{ IgnoringMessageHandler, MessageHandler, PeerManager, SocketDescriptor, }; @@ -151,11 +151,8 @@ impl Router for FuzzRouter { fn find_route( &self, _payer: &PublicKey, _params: &RouteParameters, _first_hops: Option<&[&ChannelDetails]>, _inflight_htlcs: InFlightHtlcs, - ) -> Result { - Err(msgs::LightningError { - err: String::from("Not implemented"), - action: msgs::ErrorAction::IgnoreError, - }) + ) -> Result { + Err("Not implemented") } fn create_blinded_payment_paths( diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 4ec2fa78a66..b753d3f06c0 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -2169,7 +2169,7 @@ macro_rules! get_payment_preimage_hash { } /// Gets a route from the given sender to the node described in `payment_params`. -pub fn get_route(send_node: &Node, route_params: &RouteParameters) -> Result { +pub fn get_route(send_node: &Node, route_params: &RouteParameters) -> Result { let scorer = TestScorer::new(); let keys_manager = TestKeysInterface::new(&[0u8; 32], Network::Testnet); let random_seed_bytes = keys_manager.get_secure_random_bytes(); @@ -2181,7 +2181,7 @@ pub fn get_route(send_node: &Node, route_params: &RouteParameters) -> Result Result { +pub fn find_route(send_node: &Node, route_params: &RouteParameters) -> Result { let scorer = TestScorer::new(); let keys_manager = TestKeysInterface::new(&[0u8; 32], Network::Testnet); let random_seed_bytes = keys_manager.get_secure_random_bytes(); diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index f51d3b8f11f..7bf91656c08 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -2430,7 +2430,6 @@ mod tests { use crate::ln::channelmanager::{PaymentId, RecipientOnionFields}; use crate::ln::inbound_payment::ExpandedKey; use crate::types::features::{Bolt12InvoiceFeatures, ChannelFeatures, NodeFeatures}; - use crate::ln::msgs::{ErrorAction, LightningError}; use crate::ln::outbound_payment::{Bolt12PaymentError, OutboundPayments, PendingOutboundPayment, Retry, RetryableSendFailure, StaleExpiration}; #[cfg(feature = "std")] use crate::offers::invoice::DEFAULT_RELATIVE_EXPIRY; @@ -2532,8 +2531,7 @@ mod tests { let payment_params = PaymentParameters::from_node_id( PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()), 0); let route_params = RouteParameters::from_payment_params_and_value(payment_params, 0); - router.expect_find_route(route_params.clone(), - Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError })); + router.expect_find_route(route_params.clone(), Err("")); let pending_events = Mutex::new(VecDeque::new()); if on_retry { @@ -2863,13 +2861,11 @@ mod tests { ); assert!(outbound_payments.has_pending_payments()); - router.expect_find_route( - RouteParameters::from_payment_params_and_value( - PaymentParameters::from_bolt12_invoice(&invoice), - invoice.amount_msats(), - ), - Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError }), + let route_params = RouteParameters::from_payment_params_and_value( + PaymentParameters::from_bolt12_invoice(&invoice), + invoice.amount_msats(), ); + router.expect_find_route(route_params, Err("")); assert_eq!( outbound_payments.send_payment_for_bolt12_invoice( diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 3b6d6e0b7a0..8fa9968bc70 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -17,7 +17,7 @@ use crate::types::payment::{PaymentHash, PaymentPreimage}; use crate::ln::channel_state::ChannelDetails; use crate::ln::channelmanager::{PaymentId, MIN_FINAL_CLTV_EXPIRY_DELTA, RecipientOnionFields}; use crate::types::features::{BlindedHopFeatures, Bolt11InvoiceFeatures, Bolt12InvoiceFeatures, ChannelFeatures, NodeFeatures}; -use crate::ln::msgs::{DecodeError, ErrorAction, LightningError, MAX_VALUE_MSAT}; +use crate::ln::msgs::{DecodeError, MAX_VALUE_MSAT}; use crate::ln::onion_utils; #[cfg(async_payments)] use crate::offers::static_invoice::StaticInvoice; @@ -27,7 +27,7 @@ use crate::routing::scoring::{ChannelUsage, LockableScore, ScoreLookUp}; use crate::sign::EntropySource; use crate::sync::Mutex; use crate::util::ser::{Writeable, Readable, ReadableArgs, Writer}; -use crate::util::logger::{Level, Logger}; +use crate::util::logger::Logger; use crate::crypto::chacha20::ChaCha20; use crate::io; @@ -82,7 +82,7 @@ impl>, L: Deref, ES: Deref, S: Deref, SP: Size params: &RouteParameters, first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: InFlightHtlcs - ) -> Result { + ) -> Result { let random_seed_bytes = self.entropy_source.get_secure_random_bytes(); find_route( payer, params, &self.network_graph, first_hops, &*self.logger, @@ -204,13 +204,8 @@ impl Router for FixedRouter { fn find_route( &self, _payer: &PublicKey, _route_params: &RouteParameters, _first_hops: Option<&[&ChannelDetails]>, _inflight_htlcs: InFlightHtlcs - ) -> Result { - self.route.lock().unwrap().take().ok_or_else(|| { - LightningError { - err: "Can't use this router to return multiple routes".to_owned(), - action: ErrorAction::IgnoreError, - } - }) + ) -> Result { + self.route.lock().unwrap().take().ok_or("Can't use this router to return multiple routes") } fn create_blinded_payment_paths< @@ -234,7 +229,7 @@ pub trait Router { fn find_route( &self, payer: &PublicKey, route_params: &RouteParameters, first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: InFlightHtlcs - ) -> Result; + ) -> Result; /// Finds a [`Route`] for a payment between the given `payer` and a payee. /// @@ -247,7 +242,7 @@ pub trait Router { &self, payer: &PublicKey, route_params: &RouteParameters, first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: InFlightHtlcs, _payment_hash: PaymentHash, _payment_id: PaymentId - ) -> Result { + ) -> Result { self.find_route(payer, route_params, first_hops, inflight_htlcs) } @@ -1675,7 +1670,7 @@ fn calculate_blinded_path_intro_points<'a, L: Deref>( payment_params: &PaymentParameters, node_counters: &'a NodeCounters, network_graph: &ReadOnlyNetworkGraph, logger: &L, our_node_id: NodeId, first_hop_targets: &HashMap, u32)>, -) -> Result>, LightningError> +) -> Result>, &'static str> where L::Target: Logger { let introduction_node_id_cache = payment_params.payee.blinded_route_hints().iter() .map(|path| { @@ -1705,21 +1700,18 @@ where L::Target: Logger { for route in route_hints.iter() { for hop in &route.0 { if hop.src_node_id == *node_id { - return Err(LightningError { - err: "Route hint cannot have the payee as the source.".to_owned(), - action: ErrorAction::IgnoreError - }); + return Err("Route hint cannot have the payee as the source."); } } } }, Payee::Blinded { route_hints, .. } => { if introduction_node_id_cache.iter().all(|info_opt| info_opt.map(|(a, _)| a) == Some(&our_node_id)) { - return Err(LightningError{err: "Cannot generate a route to blinded paths if we are the introduction node to all of them".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Cannot generate a route to blinded paths if we are the introduction node to all of them"); } for (blinded_path, info_opt) in route_hints.iter().zip(introduction_node_id_cache.iter()) { if blinded_path.blinded_hops().len() == 0 { - return Err(LightningError{err: "0-hop blinded path provided".to_owned(), action: ErrorAction::IgnoreError}); + return Err("0-hop blinded path provided"); } let introduction_node_id = match info_opt { None => continue, @@ -1733,7 +1725,7 @@ where L::Target: Logger { .filter(|(p, _)| p.blinded_hops().len() == 1) .any(|(_, iter_info_opt)| iter_info_opt.is_some() && iter_info_opt != info_opt) { - return Err(LightningError{err: "1-hop blinded paths must all have matching introduction node ids".to_string(), action: ErrorAction::IgnoreError}); + return Err("1-hop blinded paths must all have matching introduction node ids"); } } } @@ -2112,7 +2104,7 @@ pub fn find_route( our_node_pubkey: &PublicKey, route_params: &RouteParameters, network_graph: &NetworkGraph, first_hops: Option<&[&ChannelDetails]>, logger: L, scorer: &S, score_params: &S::ScoreParams, random_seed_bytes: &[u8; 32] -) -> Result +) -> Result where L::Target: Logger, GL::Target: Logger { let graph_lock = network_graph.read_only(); let mut route = get_route(our_node_pubkey, &route_params, &graph_lock, first_hops, logger, @@ -2125,7 +2117,7 @@ pub(crate) fn get_route( our_node_pubkey: &PublicKey, route_params: &RouteParameters, network_graph: &ReadOnlyNetworkGraph, first_hops: Option<&[&ChannelDetails]>, logger: L, scorer: &S, score_params: &S::ScoreParams, _random_seed_bytes: &[u8; 32] -) -> Result +) -> Result where L::Target: Logger { let payment_params = &route_params.payment_params; @@ -2141,23 +2133,23 @@ where L::Target: Logger { let our_node_id = NodeId::from_pubkey(&our_node_pubkey); if payee_node_id_opt.map_or(false, |payee| payee == our_node_id) { - return Err(LightningError{err: "Cannot generate a route to ourselves".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Cannot generate a route to ourselves"); } if our_node_id == maybe_dummy_payee_node_id { - return Err(LightningError{err: "Invalid origin node id provided, use a different one".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Invalid origin node id provided, use a different one"); } if final_value_msat > MAX_VALUE_MSAT { - return Err(LightningError{err: "Cannot generate a route of more value than all existing satoshis".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Cannot generate a route of more value than all existing satoshis"); } if final_value_msat == 0 { - return Err(LightningError{err: "Cannot send a payment of 0 msat".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Cannot send a payment of 0 msat"); } let final_cltv_expiry_delta = payment_params.payee.final_cltv_expiry_delta().unwrap_or(0); if payment_params.max_total_cltv_expiry_delta <= final_cltv_expiry_delta { - return Err(LightningError{err: "Can't find a route where the maximum total CLTV expiry delta is below the final CLTV expiry.".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Can't find a route where the maximum total CLTV expiry delta is below the final CLTV expiry."); } // The general routing idea is the following: @@ -2220,7 +2212,7 @@ where L::Target: Logger { let network_nodes = network_graph.nodes(); if payment_params.max_path_count == 0 { - return Err(LightningError{err: "Can't find a route with no paths allowed.".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Can't find a route with no paths allowed."); } // Allow MPP only if we have a features set from somewhere that indicates the payee supports @@ -2285,7 +2277,7 @@ where L::Target: Logger { panic!("first_hops should be filled in with usable channels, not pending ones"); } if chan.counterparty.node_id == *our_node_pubkey { - return Err(LightningError{err: "First hop cannot have our_node_pubkey as a destination.".to_owned(), action: ErrorAction::IgnoreError}); + return Err("First hop cannot have our_node_pubkey as a destination."); } let counterparty_id = NodeId::from_pubkey(&chan.counterparty.node_id); first_hop_targets @@ -2298,7 +2290,7 @@ where L::Target: Logger { .0.push(chan); } if first_hop_targets.is_empty() { - return Err(LightningError{err: "Cannot route when there are no outbound routes away from us".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Cannot route when there are no outbound routes away from us"); } } @@ -2322,13 +2314,13 @@ where L::Target: Logger { node_counters.private_node_counter_from_pubkey(&prev_hop_id) .ok_or_else(|| { debug_assert!(false); - LightningError { err: "We should always have private target node counters available".to_owned(), action: ErrorAction::IgnoreError } + "We should always have private target node counters available" })?; let (_src_id, private_source_node_counter) = node_counters.private_node_counter_from_pubkey(&hop.src_node_id) .ok_or_else(|| { debug_assert!(false); - LightningError { err: "We should always have private source node counters available".to_owned(), action: ErrorAction::IgnoreError } + "We should always have private source node counters available" })?; if let Some((first_channels, _)) = first_hop_targets.get(target) { @@ -3236,11 +3228,11 @@ where L::Target: Logger { // Step (5). if payment_paths.len() == 0 { - return Err(LightningError{err: "Failed to find a path to the given destination".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Failed to find a path to the given destination"); } if already_collected_value_msat < final_value_msat { - return Err(LightningError{err: "Failed to find a sufficient route to the given destination".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Failed to find a sufficient route to the given destination"); } // Step (6). @@ -3339,7 +3331,7 @@ where L::Target: Logger { }; hops.push(RouteHop { - pubkey: PublicKey::from_slice(target.as_slice()).map_err(|_| LightningError{err: format!("Public key {:?} is invalid", &target), action: ErrorAction::IgnoreAndLog(Level::Trace)})?, + pubkey: PublicKey::from_slice(target.as_slice()).map_err(|_| "A PublicKey in NetworkGraph is invalid!")?, node_features: node_features.clone(), short_channel_id: hop.candidate.short_channel_id().unwrap(), channel_features: hop.candidate.features(), @@ -3382,8 +3374,7 @@ where L::Target: Logger { // Make sure we would never create a route whose total fees exceed max_total_routing_fee_msat. if let Some(max_total_routing_fee_msat) = route_params.max_total_routing_fee_msat { if route.get_total_fees() > max_total_routing_fee_msat { - return Err(LightningError{err: format!("Failed to find route that adheres to the maximum total fee limit of {}msat", - max_total_routing_fee_msat), action: ErrorAction::IgnoreError}); + return Err("Failed to find route that adheres to the maximum total fee limit"); } } @@ -3488,7 +3479,7 @@ fn add_random_cltv_offset(route: &mut Route, payment_params: &PaymentParameters, pub fn build_route_from_hops( our_node_pubkey: &PublicKey, hops: &[PublicKey], route_params: &RouteParameters, network_graph: &NetworkGraph, logger: L, random_seed_bytes: &[u8; 32] -) -> Result +) -> Result where L::Target: Logger, GL::Target: Logger { let graph_lock = network_graph.read_only(); let mut route = build_route_from_hops_internal(our_node_pubkey, hops, &route_params, @@ -3500,7 +3491,7 @@ where L::Target: Logger, GL::Target: Logger { fn build_route_from_hops_internal( our_node_pubkey: &PublicKey, hops: &[PublicKey], route_params: &RouteParameters, network_graph: &ReadOnlyNetworkGraph, logger: L, random_seed_bytes: &[u8; 32], -) -> Result where L::Target: Logger { +) -> Result where L::Target: Logger { struct HopScorer { our_node_id: NodeId, @@ -3535,7 +3526,7 @@ fn build_route_from_hops_internal( } if hops.len() > MAX_PATH_LENGTH_ESTIMATE.into() { - return Err(LightningError{err: "Cannot build a route exceeding the maximum path length.".to_owned(), action: ErrorAction::IgnoreError}); + return Err("Cannot build a route exceeding the maximum path length."); } let our_node_id = NodeId::from_pubkey(our_node_pubkey); @@ -3564,7 +3555,7 @@ mod tests { use crate::ln::channel_state::{ChannelCounterparty, ChannelDetails, ChannelShutdownState}; use crate::ln::types::ChannelId; use crate::types::features::{BlindedHopFeatures, ChannelFeatures, InitFeatures, NodeFeatures}; - use crate::ln::msgs::{ErrorAction, LightningError, UnsignedChannelUpdate, MAX_VALUE_MSAT}; + use crate::ln::msgs::{UnsignedChannelUpdate, MAX_VALUE_MSAT}; use crate::ln::channelmanager; use crate::util::config::UserConfig; use crate::util::test_utils as ln_test_utils; @@ -3661,7 +3652,7 @@ mod tests { let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 0); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, + if let Err(err) = get_route(&our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Cannot send a payment of 0 msat"); @@ -3705,7 +3696,7 @@ mod tests { let our_chans = vec![get_channel_details(Some(2), our_id, InitFeatures::from_le_bytes(vec![0b11]), 100000)]; let route_params = RouteParameters::from_payment_params_and_value(payment_params, 100); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, + if let Err(err) = get_route(&our_id, &route_params, &network_graph.read_only(), Some(&our_chans.iter().collect::>()), Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "First hop cannot have our_node_pubkey as a destination."); @@ -3828,7 +3819,7 @@ mod tests { // Not possible to send 199_999_999, because the minimum on channel=2 is 200_000_000. let route_params = RouteParameters::from_payment_params_and_value( payment_params, 199_999_999); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, + if let Err(err) = get_route(&our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a path to the given destination"); @@ -4058,10 +4049,10 @@ mod tests { let mut route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 5_000); route_params.max_total_routing_fee_msat = Some(9_999); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, + if let Err(err) = get_route(&our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { - assert_eq!(err, "Failed to find route that adheres to the maximum total fee limit of 9999msat"); + assert_eq!(err, "Failed to find route that adheres to the maximum total fee limit"); } else { panic!(); } let mut route_params = RouteParameters::from_payment_params_and_value( @@ -4110,7 +4101,7 @@ mod tests { // If all the channels require some features we don't understand, route should fail let mut route_params = RouteParameters::from_payment_params_and_value(payment_params, 100); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, + if let Err(err) = get_route(&our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a path to the given destination"); @@ -4157,7 +4148,7 @@ mod tests { // If all nodes require some features we don't understand, route should fail let route_params = RouteParameters::from_payment_params_and_value(payment_params, 100); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, + if let Err(err) = get_route(&our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a path to the given destination"); @@ -4356,7 +4347,7 @@ mod tests { let payment_params = PaymentParameters::from_node_id(nodes[6], 42) .with_route_hints(invalid_last_hops).unwrap(); let route_params = RouteParameters::from_payment_params_and_value(payment_params, 100); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, + if let Err(err) = get_route(&our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Route hint cannot have the payee as the source."); @@ -4877,7 +4868,7 @@ mod tests { assert_eq!(route.paths[0].hops[4].channel_features.le_flags(), &Vec::::new()); // We can't learn any flags from invoices, sadly } - fn do_unannounced_path_test(last_hop_htlc_max: Option, last_hop_fee_prop: u32, outbound_capacity_msat: u64, route_val: u64) -> Result { + fn do_unannounced_path_test(last_hop_htlc_max: Option, last_hop_fee_prop: u32, outbound_capacity_msat: u64, route_val: u64) -> Result { let source_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&>::from_hex(&format!("{:02}", 41).repeat(32)).unwrap()[..]).unwrap()); let middle_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&>::from_hex(&format!("{:02}", 42).repeat(32)).unwrap()[..]).unwrap()); let target_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&>::from_hex(&format!("{:02}", 43).repeat(32)).unwrap()[..]).unwrap()); @@ -5031,7 +5022,7 @@ mod tests { // Attempt to route more than available results in a failure. let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 250_000_001); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -5074,7 +5065,7 @@ mod tests { // Attempt to route more than available results in a failure. let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 200_000_001); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), Some(&our_chans.iter().collect::>()), Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { @@ -5131,7 +5122,7 @@ mod tests { // Attempt to route more than available results in a failure. let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 15_001); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -5211,7 +5202,7 @@ mod tests { // Attempt to route more than available results in a failure. let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 15_001); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -5250,7 +5241,7 @@ mod tests { // Attempt to route more than available results in a failure. let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 10_001); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -5375,7 +5366,7 @@ mod tests { // Attempt to route more than available results in a failure. let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 60_000); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -5616,7 +5607,7 @@ mod tests { // Attempt to route more than available results in a failure. let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 300_000); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -5628,7 +5619,7 @@ mod tests { let zero_payment_params = payment_params.clone().with_max_path_count(0); let route_params = RouteParameters::from_payment_params_and_value( zero_payment_params, 100); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Can't find a route with no paths allowed."); @@ -5642,7 +5633,7 @@ mod tests { let fail_payment_params = payment_params.clone().with_max_path_count(3); let route_params = RouteParameters::from_payment_params_and_value( fail_payment_params, 250_000); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -5747,7 +5738,7 @@ mod tests { assert!(do_mpp_route_tests(300_001).is_err()); } - fn do_mpp_route_tests(amt: u64) -> Result { + fn do_mpp_route_tests(amt: u64) -> Result { let (secp_ctx, network_graph, gossip_sync, _, logger) = build_graph(); let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx); let scorer = ln_test_utils::TestScorer::new(); @@ -6092,7 +6083,7 @@ mod tests { // Attempt to route more than available results in a failure. let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 210_000); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -6103,7 +6094,7 @@ mod tests { // Attempt to route while setting max_total_routing_fee_msat to 149_999 results in a failure. let route_params = RouteParameters { payment_params: payment_params.clone(), final_value_msat: 200_000, max_total_routing_fee_msat: Some(149_999) }; - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -6349,7 +6340,7 @@ mod tests { // Attempt to route more than available results in a failure. let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), 150_000); - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &network_graph.read_only(), None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a sufficient route to the given destination"); @@ -6887,7 +6878,7 @@ mod tests { let scorer = BadNodeScorer { node_id: NodeId::from_pubkey(&nodes[2]) }; match get_route( &our_id, &route_params, &network_graph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { - Err(LightningError { err, .. } ) => { + Err(err) => { assert_eq!(err, "Failed to find a path to the given destination"); }, Ok(_) => panic!("Expected error"), @@ -6994,7 +6985,7 @@ mod tests { match get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { - Err(LightningError { err, .. } ) => { + Err(err) => { assert_eq!(err, "Failed to find a path to the given destination"); }, Ok(_) => panic!("Expected error"), @@ -7061,7 +7052,7 @@ mod tests { match get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { - Err(LightningError { err, .. } ) => { + Err(err) => { assert_eq!(err, "Failed to find a path to the given destination"); }, Ok(_) => panic!("Expected error"), @@ -7388,7 +7379,7 @@ mod tests { let mut route_params = RouteParameters::from_payment_params_and_value( payment_params, max_htlc_msat + 1); route_params.max_total_routing_fee_msat = None; - if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, + if let Err(err) = get_route(&our_id, &route_params, &netgraph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { @@ -7733,7 +7724,7 @@ mod tests { match get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { - Err(LightningError { err, .. }) => { + Err(err) => { assert_eq!(err, "1-hop blinded paths must all have matching introduction node ids"); }, _ => panic!("Expected error") @@ -7745,7 +7736,7 @@ mod tests { match get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { - Err(LightningError { err, .. }) => { + Err(err) => { assert_eq!(err, "Cannot generate a route to blinded paths if we are the introduction node to all of them"); }, _ => panic!("Expected error") @@ -7758,7 +7749,7 @@ mod tests { match get_route(&our_id, &route_params, &network_graph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { - Err(LightningError { err, .. }) => { + Err(err) => { assert_eq!(err, "0-hop blinded path provided"); }, _ => panic!("Expected error") @@ -7887,7 +7878,7 @@ mod tests { let netgraph = network_graph.read_only(); let route_params = RouteParameters::from_payment_params_and_value( payment_params.clone(), amt_msat); - if let Err(LightningError { err, .. }) = get_route(&nodes[0], &route_params, &netgraph, + if let Err(err) = get_route(&nodes[0], &route_params, &netgraph, Some(&first_hops.iter().collect::>()), Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes) { assert_eq!(err, "Failed to find a path to the given destination"); @@ -7996,7 +7987,7 @@ mod tests { let netgraph = network_graph.read_only(); let route_params = RouteParameters::from_payment_params_and_value( payment_params, amt_msat); - if let Err(LightningError { err, .. }) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &netgraph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes ) { assert_eq!(err, "Failed to find a path to the given destination"); @@ -8043,7 +8034,7 @@ mod tests { let netgraph = network_graph.read_only(); let route_params = RouteParameters::from_payment_params_and_value( payment_params, amt_msat); - if let Err(LightningError { err, .. }) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &netgraph, None, Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes ) { assert_eq!(err, "Failed to find a path to the given destination"); @@ -8112,7 +8103,7 @@ mod tests { let netgraph = network_graph.read_only(); let route_params = RouteParameters::from_payment_params_and_value( payment_params, amt_msat); - if let Err(LightningError { err, .. }) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &netgraph, Some(&first_hops.iter().collect::>()), Arc::clone(&logger), &scorer, &Default::default(), &random_seed_bytes ) { @@ -8254,7 +8245,7 @@ mod tests { payment_params, amt_msat); let netgraph = network_graph.read_only(); - if let Err(LightningError { err, .. }) = get_route( + if let Err(err) = get_route( &our_id, &route_params, &netgraph, Some(&first_hops.iter().collect::>()), Arc::clone(&logger), &scorer, &ProbabilisticScoringFeeParameters::default(), &random_seed_bytes diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 7ebbf1c5734..9dec872aacb 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -28,7 +28,6 @@ use crate::ln::chan_utils::CommitmentTransaction; use crate::ln::channel_state::ChannelDetails; use crate::ln::channelmanager; use crate::ln::inbound_payment::ExpandedKey; -use crate::ln::msgs::LightningError; use crate::ln::script::ShutdownScript; use crate::ln::types::ChannelId; use crate::ln::{msgs, wire}; @@ -139,7 +138,7 @@ pub struct TestRouter<'a> { TestScorer, >, pub network_graph: Arc>, - pub next_routes: Mutex>)>>, + pub next_routes: Mutex>)>>, pub next_blinded_payment_paths: Mutex>, pub scorer: &'a RwLock, } @@ -167,7 +166,7 @@ impl<'a> TestRouter<'a> { } } - pub fn expect_find_route(&self, query: RouteParameters, result: Result) { + pub fn expect_find_route(&self, query: RouteParameters, result: Result) { let mut expected_routes = self.next_routes.lock().unwrap(); expected_routes.push_back((query, Some(result))); } @@ -187,7 +186,7 @@ impl<'a> Router for TestRouter<'a> { fn find_route( &self, payer: &PublicKey, params: &RouteParameters, first_hops: Option<&[&ChannelDetails]>, inflight_htlcs: InFlightHtlcs, - ) -> Result { + ) -> Result { let route_res; let next_route_opt = self.next_routes.lock().unwrap().pop_front(); if let Some((find_route_query, find_route_res)) = next_route_opt {