f test

Author: TheBlueMatt

lightning/src/routing/router.rs

@@ -2964,7 +2964,7 @@ where L::Target: Logger {
 							// Ignore hops if augmenting the current path to them would put us over `max_total_routing_fee_msat`
 							if total_fee_msat > max_total_routing_fee_msat {
 								if should_log_candidate {
-									log_trace!(logger, "Ignoring {} due to exceeding max total routing fee limit.", LoggedCandidateHop(&$candidate));
+									log_trace!(logger, "Ignoring {} with fee {total_fee_msat} due to exceeding max total routing fee limit {max_total_routing_fee_msat}.", LoggedCandidateHop(&$candidate));
 
 									if let Some(_) = first_hop_details {
 										log_trace!(logger,
@@ -9036,6 +9036,178 @@ mod tests {
 
 		assert_eq!(route.paths[0].hops[0].short_channel_id, 44);
 	}
+
+	#[test]
+	fn prefers_paths_by_cost_amt_ratio() {
+		// Previously, we preferred paths during MPP selection based on their absolute cost, rather
+		// than the cost-per-amount-transferred. This could result in selecting many MPP paths with
+		// relatively low value contribution, rather than one large path which is ultimately
+		// cheaper. While this is a tradeoff (and not universally better), in practice the old
+		// behavior was problematic, so we shifted to a proportional cost.
+		//
+		// Here we check that the proportional cost is being used in a somewhat absurd setup where
+		// we have one good path and several cheaper, but smaller paths.
+		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];
+
+		// Enable channel 1
+		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: (1 << 4) | 1,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: 10_000_000,
+			fee_base_msat: 0,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new()
+		});
+
+		// Set the fee on channel 3 to 1 sat, max HTLC to 1M msat
+		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: (3 << 4) | 1,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: 1_000_000,
+			fee_base_msat: 1_000,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new()
+		});
+
+		// Set the fee on channel 13 to 1 sat, max HTLC to 1M msat
+		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: (13 << 4) | 1,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: 1_000_000,
+			fee_base_msat: 1_000,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new()
+		});
+
+		// Set the fee on channel 4 to 1 sat, max HTLC to 1M msat
+		update_channel(&gossip_sync, &secp_ctx, &privkeys[1], UnsignedChannelUpdate {
+			chain_hash: ChainHash::using_genesis_block(Network::Testnet),
+			short_channel_id: 4,
+			timestamp: 2,
+			message_flags: 1, // Only must_be_one
+			channel_flags: 0,
+			cltv_expiry_delta: (4 << 4) | 1,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: 1_000_000,
+			fee_base_msat: 1_000,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new()
+		});
+
+		// The router will attempt to gather 3x the requested amount, and if it finds the new path
+		// through channel 16, added below, it'll always prefer that, even prior to the changes
+		// which introduced this test.
+		// Instead, we add 6 additional channels so that the pathfinder always just gathers useless
+		// paths first.
+		for i in 0..6 {
+			// Finally, create a single channel with fee of 2 sat from node 1 to node 2 which allows
+			// for a larger payment.
+			add_channel(&gossip_sync, &secp_ctx, &privkeys[7], &privkeys[2], ChannelFeatures::from_le_bytes(vec![]), i + 42);
+
+			// Set the fee on channel 16 to 2 sats, max HTLC to 3M msat
+			update_channel(&gossip_sync, &secp_ctx, &privkeys[7], UnsignedChannelUpdate {
+				chain_hash: ChainHash::using_genesis_block(Network::Testnet),
+				short_channel_id: i + 42,
+				timestamp: 2,
+				message_flags: 1, // Only must_be_one
+				channel_flags: 0,
+				cltv_expiry_delta: (42 << 4) | 1,
+				htlc_minimum_msat: 0,
+				htlc_maximum_msat: 1_000_000,
+				fee_base_msat: 1_000,
+				fee_proportional_millionths: 0,
+				excess_data: Vec::new()
+			});
+
+			// Enable channel 16 by providing an update in both directions
+			update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate {
+				chain_hash: ChainHash::using_genesis_block(Network::Testnet),
+				short_channel_id: i + 42,
+				timestamp: 2,
+				message_flags: 1, // Only must_be_one
+				channel_flags: 1,
+				cltv_expiry_delta: (42 << 4) | 1,
+				htlc_minimum_msat: 0,
+				htlc_maximum_msat: 10_000_000,
+				fee_base_msat: u32::MAX,
+				fee_proportional_millionths: 0,
+				excess_data: Vec::new()
+			});
+		}
+
+		// Ensure that we can build a route for 3M msat across the three paths to node 2.
+		let config = UserConfig::default();
+		let mut payment_params = PaymentParameters::from_node_id(nodes[2], 42)
+			.with_bolt11_features(channelmanager::provided_bolt11_invoice_features(&config))
+			.unwrap();
+		payment_params.max_channel_saturation_power_of_half = 0;
+		let route_params = RouteParameters::from_payment_params_and_value(payment_params, 3_000_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(), 3);
+		for path in route.paths {
+			assert_eq!(path.hops.len(), 2);
+		}
+
+		// Finally, create a single channel with fee of 2 sat from node 1 to node 2 which allows
+		// for a larger payment.
+		add_channel(&gossip_sync, &secp_ctx, &privkeys[1], &privkeys[2], ChannelFeatures::from_le_bytes(id_to_feature_flags(16)), 16);
+
+		// Set the fee on channel 16 to 2 sats, max HTLC to 3M msat
+		update_channel(&gossip_sync, &secp_ctx, &privkeys[1], UnsignedChannelUpdate {
+			chain_hash: ChainHash::using_genesis_block(Network::Testnet),
+			short_channel_id: 16,
+			timestamp: 2,
+			message_flags: 1, // Only must_be_one
+			channel_flags: 0,
+			cltv_expiry_delta: (16 << 4) | 1,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: 3_000_000,
+			fee_base_msat: 2_000,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new()
+		});
+
+		// Enable channel 16 by providing an update in both directions
+		update_channel(&gossip_sync, &secp_ctx, &privkeys[2], UnsignedChannelUpdate {
+			chain_hash: ChainHash::using_genesis_block(Network::Testnet),
+			short_channel_id: 16,
+			timestamp: 2,
+			message_flags: 1, // Only must_be_one
+			channel_flags: 1,
+			cltv_expiry_delta: (16 << 4) | 1,
+			htlc_minimum_msat: 0,
+			htlc_maximum_msat: 10_000_000,
+			fee_base_msat: u32::MAX,
+			fee_proportional_millionths: 0,
+			excess_data: Vec::new()
+		});
+
+		// Ensure that we now build a route for 3M msat across just the new path
+		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(), 1);
+		assert_eq!(route.paths[0].hops.len(), 2);
+		assert_eq!(route.paths[0].hops[1].short_channel_id, 16);
+	}
 }
 
 #[cfg(any(test, ldk_bench))]

lightning/src/routing/test_utils.rs

@@ -112,7 +112,7 @@ pub(crate) fn update_channel(
 
 	match gossip_sync.handle_channel_update(Some(node_pubkey), &valid_channel_update) {
 		Ok(res) => assert!(res),
-		Err(_) => panic!()
+		Err(e) => panic!("{e:?}")
 	};
 }