Router: discard invalid PaymentPaths that violate a hop's max_htlc

See new tests, the fuzzer found a debug panic in two of these cases, one where
we overpay to meet a hop's min_htlc and one where a previously found path is
already taking up some of a hop's liquidity.

Author: valentinewallace

lightning/src/routing/router.rs

@@ -2300,12 +2300,19 @@ where L::Target: Logger {
 					.chain(payment_path.hops.iter().map(|(hop, _)| &hop.node_id));
 				for (prev_hop, (hop, _)) in prev_hop_iter.zip(payment_path.hops.iter()) {
 					let spent_on_hop_msat = value_contribution_msat + hop.next_hops_fee_msat;
+					let hop_max_msat = max_htlc_from_capacity(
+						hop.candidate.effective_capacity(), channel_saturation_pow_half
+					);
+					let used_liquidity_candidate = used_liquidities
+						.get(&hop.candidate.id(*prev_hop < hop.node_id))
+						.map_or(spent_on_hop_msat, |used_liquidity_msat|
+							used_liquidity_msat + spent_on_hop_msat);
+					if used_liquidity_candidate > hop_max_msat { break 'path_construction }
+
 					let used_liquidity_msat = used_liquidities
 						.entry(hop.candidate.id(*prev_hop < hop.node_id))
 						.and_modify(|used_liquidity_msat| *used_liquidity_msat += spent_on_hop_msat)
 						.or_insert(spent_on_hop_msat);
-					let hop_capacity = hop.candidate.effective_capacity();
-					let hop_max_msat = max_htlc_from_capacity(hop_capacity, channel_saturation_pow_half);
 					if *used_liquidity_msat == hop_max_msat {
 						// If this path used all of this channel's available liquidity, we know
 						// this path will not be selected again in the next loop iteration.
@@ -7185,6 +7192,134 @@ mod tests {
 			assert_eq!(err, "Failed to find a path to the given destination");
 		} else { panic!() }
 	}
+
+	#[test]
+	fn min_htlc_overpay_violates_max_htlc() {
+		// Test that if overpaying to meet a later hop's min_htlc and causes us to violate an earlier
+		// hop's max_htlc, we discard that invalid path. Previously we would consider the path to be
+		// valid and hit a debug panic asserting that the used liquidity for a hop was less than its
+		// available liquidity limit.
+		let secp_ctx = Secp256k1::new();
+		let logger = Arc::new(ln_test_utils::TestLogger::new());
+		let network_graph = Arc::new(NetworkGraph::new(Network::Testnet, Arc::clone(&logger)));
+		let scorer = ln_test_utils::TestScorer::new();
+		let keys_manager = ln_test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
+		let random_seed_bytes = keys_manager.get_secure_random_bytes();
+		let config = UserConfig::default();
+
+		// Values are taken from the fuzz input that uncovered this panic.
+		let amt_msat = 7_4009_8048;
+		let (_, our_id, _, nodes) = get_nodes(&secp_ctx);
+		let first_hop_outbound_capacity = 2_7345_2000;
+		let first_hops = vec![get_channel_details(
+			Some(200), nodes[0], channelmanager::provided_init_features(&config),
+			first_hop_outbound_capacity
+		)];
+
+		let route_hint = RouteHint(vec![RouteHintHop {
+			src_node_id: nodes[0],
+			short_channel_id: 44,
+			fees: RoutingFees {
+				base_msat: 1_6778_3453,
+				proportional_millionths: 0,
+			},
+			cltv_expiry_delta: 10,
+			htlc_minimum_msat: Some(2_5165_8240),
+			htlc_maximum_msat: Some(251_6582_4000),
+		}]);
+
+		let payment_params = PaymentParameters::from_node_id(nodes[1], 42)
+			.with_route_hints(vec![route_hint]).unwrap()
+			.with_bolt11_features(channelmanager::provided_invoice_features(&config)).unwrap();
+
+		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(
+			&our_id, &route_params, &netgraph, Some(&first_hops.iter().collect::<Vec<_>>()),
+			Arc::clone(&logger), &scorer, &(), &random_seed_bytes
+		) {
+			assert_eq!(err, "Failed to find a path to the given destination");
+		} else { panic!() }
+	}
+
+	#[test]
+	fn previously_used_liquidity_violates_max_htlc() {
+		// Test that if a previously found path causes us to violate a newly found path hop's max_htlc,
+		// we will discard that invalid path. Previously we would consider the path to be valid and hit
+		// a debug panic asserting that the used liquidity for a hop was less than its available
+		// liquidity limit.
+		let secp_ctx = Secp256k1::new();
+		let logger = Arc::new(ln_test_utils::TestLogger::new());
+		let network_graph = Arc::new(NetworkGraph::new(Network::Testnet, Arc::clone(&logger)));
+		let scorer = ln_test_utils::TestScorer::new();
+		let keys_manager = ln_test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
+		let random_seed_bytes = keys_manager.get_secure_random_bytes();
+		let config = UserConfig::default();
+
+		// Values are taken from the fuzz input that uncovered this panic.
+		let amt_msat = 52_4288;
+		let (_, our_id, _, nodes) = get_nodes(&secp_ctx);
+		let first_hops = vec![get_channel_details(
+			Some(161), nodes[0], channelmanager::provided_init_features(&config), 486_4000
+		), get_channel_details(
+			Some(122), nodes[0], channelmanager::provided_init_features(&config), 179_5000
+		)];
+
+		let route_hints = vec![RouteHint(vec![RouteHintHop {
+			src_node_id: nodes[0],
+			short_channel_id: 42,
+			fees: RoutingFees {
+				base_msat: 0,
+				proportional_millionths: 0,
+			},
+			cltv_expiry_delta: 10,
+			htlc_minimum_msat: Some(1_4392),
+			htlc_maximum_msat: Some(143_9200),
+		}]), RouteHint(vec![RouteHintHop {
+			src_node_id: nodes[0],
+			short_channel_id: 43,
+			fees: RoutingFees {
+				base_msat: 425_9840,
+				proportional_millionths: 0,
+			},
+			cltv_expiry_delta: 10,
+			htlc_minimum_msat: Some(19_7401),
+			htlc_maximum_msat: Some(1974_0100),
+		}]), RouteHint(vec![RouteHintHop {
+			src_node_id: nodes[0],
+			short_channel_id: 44,
+			fees: RoutingFees {
+				base_msat: 0,
+				proportional_millionths: 0,
+			},
+			cltv_expiry_delta: 10,
+			htlc_minimum_msat: Some(1027),
+			htlc_maximum_msat: Some(10_2700),
+		}]), RouteHint(vec![RouteHintHop {
+			src_node_id: nodes[0],
+			short_channel_id: 45,
+			fees: RoutingFees {
+				base_msat: 0,
+				proportional_millionths: 0,
+			},
+			cltv_expiry_delta: 10,
+			htlc_minimum_msat: Some(6_5535),
+			htlc_maximum_msat: Some(655_3500),
+		}])];
+
+		let payment_params = PaymentParameters::from_node_id(nodes[1], 42)
+			.with_route_hints(route_hints).unwrap()
+			.with_bolt11_features(channelmanager::provided_invoice_features(&config)).unwrap();
+
+		let netgraph = network_graph.read_only();
+		let route_params = RouteParameters::from_payment_params_and_value(
+			payment_params, amt_msat);
+		assert!(get_route(
+			&our_id, &route_params, &netgraph, Some(&first_hops.iter().collect::<Vec<_>>()),
+			Arc::clone(&logger), &scorer, &(), &random_seed_bytes
+		).is_ok());
+	}
 }
 
 #[cfg(all(any(test, ldk_bench), not(feature = "no-std")))]