Validate the Router is meeting MPP and max-fee limitations given

When `OutboundPayments` calls the provided `Router` to fetch a
`Route` it passes a `RouteParameters` with a specific max-fee. Here
we validate that the `Route` returned sticks to the limits
provided, and also that it meets the MPP rules of not having any
single MPP part which can be removed while still meeting the
desired payment amount.

Author: TheBlueMatt

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -2309,6 +2309,7 @@ fn test_path_paused_mpp() {
 	route.paths[1].hops[0].pubkey = node_c_id;
 	route.paths[1].hops[0].short_channel_id = chan_2_ann.contents.short_channel_id;
 	route.paths[1].hops[1].short_channel_id = chan_4_id;
+	route.route_params.as_mut().unwrap().final_value_msat *= 2;
 
 	// Set it so that the first monitor update (for the path 0 -> 1 -> 3) succeeds, but the second
 	// (for the path 0 -> 2 -> 3) fails.
@@ -4252,7 +4253,7 @@ fn do_test_partial_claim_mon_update_compl_actions(reload_a: bool, reload_b: bool
 	let chan_4_scid = chan_4_update.contents.short_channel_id;
 
 	let (mut route, payment_hash, preimage, payment_secret) =
-		get_route_and_payment_hash!(&nodes[0], nodes[3], 100000);
+		get_route_and_payment_hash!(&nodes[0], nodes[3], 100_000);
 	let path = route.paths[0].clone();
 	route.paths.push(path);
 	route.paths[0].hops[0].pubkey = node_b_id;
@@ -4261,6 +4262,8 @@ fn do_test_partial_claim_mon_update_compl_actions(reload_a: bool, reload_b: bool
 	route.paths[1].hops[0].pubkey = node_c_id;
 	route.paths[1].hops[0].short_channel_id = chan_2_scid;
 	route.paths[1].hops[1].short_channel_id = chan_4_scid;
+	route.route_params.as_mut().unwrap().final_value_msat *= 2;
+
 	let paths = &[&[&nodes[1], &nodes[3]][..], &[&nodes[2], &nodes[3]][..]];
 	send_along_route_with_secret(&nodes[0], route, paths, 200_000, payment_hash, payment_secret);
 

lightning/src/ln/channelmanager.rs

@@ -20292,6 +20292,7 @@ mod tests {
 		route.paths[1].hops[0].pubkey = nodes[2].node.get_our_node_id();
 		route.paths[1].hops[0].short_channel_id = chan_2_id;
 		route.paths[1].hops[1].short_channel_id = chan_4_id;
+		route.route_params.as_mut().unwrap().final_value_msat *= 2;
 
 		nodes[0].node.send_payment_with_route(route, payment_hash,
 			RecipientOnionFields::spontaneous_empty(), PaymentId(payment_hash.0)).unwrap();

lightning/src/ln/functional_tests.rs

@@ -7205,7 +7205,7 @@ pub fn test_simple_mpp() {
 	let chan_4_id = create_announced_chan_between_nodes(&nodes, 2, 3).0.contents.short_channel_id;
 
 	let (mut route, payment_hash, payment_preimage, payment_secret) =
-		get_route_and_payment_hash!(&nodes[0], nodes[3], 100000);
+		get_route_and_payment_hash!(&nodes[0], nodes[3], 100_000);
 	let path = route.paths[0].clone();
 	route.paths.push(path);
 	route.paths[0].hops[0].pubkey = node_b_id;
@@ -7214,6 +7214,7 @@ pub fn test_simple_mpp() {
 	route.paths[1].hops[0].pubkey = node_c_id;
 	route.paths[1].hops[0].short_channel_id = chan_2_id;
 	route.paths[1].hops[1].short_channel_id = chan_4_id;
+	route.route_params.as_mut().unwrap().final_value_msat = 200_000;
 	let paths: &[&[_]] = &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]];
 	send_along_route_with_secret(&nodes[0], route, paths, 200_000, payment_hash, payment_secret);
 	claim_payment_along_route(ClaimAlongRouteArgs::new(&nodes[0], paths, payment_preimage));

lightning/src/ln/outbound_payment.rs

@@ -894,6 +894,30 @@ impl OutboundPayments {
 	}
 }
 
+/// Validate that a [`Route`] picked by our [`Router`] is sane for the [`RouteParameters`] used to
+/// request it. Failure here indicates a critical bug in the [`Router`].
+fn validate_found_route<L: Logger>(
+	route: &mut Route, route_params: &RouteParameters, logger: &WithContext<L>,
+) -> Result<(), ()> {
+	if route.route_params.as_ref() != Some(route_params) {
+		debug_assert!(
+			false,
+			"Routers are expected to return a Route which includes the requested RouteParameters. Got {:?}, expected {route_params:?}",
+			route.route_params
+		);
+		log_error!(
+			logger,
+			"Routers are expected to return a Route which includes the requested RouteParameters. Got {:?}, expected {route_params:?}",
+			route.route_params
+		);
+		route.route_params = Some(route_params.clone());
+	}
+
+	route.debug_assert_route_meets_params(logger)?;
+
+	Ok(())
+}
+
 impl OutboundPayments {
 	#[rustfmt::skip]
 	pub(super) fn send_payment<R: Router, ES: EntropySource, NS: NodeSigner, IH, SP, L: Logger>(
@@ -1462,12 +1486,8 @@ impl OutboundPayments {
 			RetryableSendFailure::RouteNotFound
 		})?;
 
-		if route.route_params.as_ref() != Some(route_params) {
-			debug_assert!(false,
-				"Routers are expected to return a Route which includes the requested RouteParameters. Got {:?}, expected {:?}",
-				route.route_params, route_params);
-			route.route_params = Some(route_params.clone());
-		}
+		validate_found_route(&mut route, route_params, logger)
+			.map_err(|()| RetryableSendFailure::RouteNotFound)?;
 
 		Ok(route)
 	}
@@ -1552,18 +1572,9 @@ impl OutboundPayments {
 			}
 		};
 
-		if route.route_params.as_ref() != Some(&route_params) {
-			debug_assert!(false,
-				"Routers are expected to return a Route which includes the requested RouteParameters");
-			route.route_params = Some(route_params.clone());
-		}
-
-		for path in route.paths.iter() {
-			if path.hops.len() == 0 {
-				log_error!(logger, "Unusable path in route (path.hops.len() must be at least 1");
-				self.abandon_payment(payment_id, PaymentFailureReason::UnexpectedError, pending_events);
-				return
-			}
+		if validate_found_route(&mut route, &route_params, logger).is_err() {
+			self.abandon_payment(payment_id, PaymentFailureReason::RouteNotFound, pending_events);
+			return
 		}
 
 		macro_rules! abandon_with_entry {
@@ -2967,15 +2978,15 @@ mod tests {
 		let sender_pk = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
 		let receiver_pk = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[43; 32]).unwrap());
 		let payment_params = PaymentParameters::from_node_id(sender_pk, 0);
-		let route_params = RouteParameters::from_payment_params_and_value(payment_params.clone(), 0);
+		let route_params = RouteParameters::from_payment_params_and_value(payment_params.clone(), 1);
 		let failed_scid = 42;
 		let route = Route {
 			paths: vec![Path { hops: vec![RouteHop {
 				pubkey: receiver_pk,
 				node_features: NodeFeatures::empty(),
 				short_channel_id: failed_scid,
 				channel_features: ChannelFeatures::empty(),
-				fee_msat: 0,
+				fee_msat: 1,
 				cltv_expiry_delta: 0,
 				maybe_announced_channel: true,
 			}], blinded_tail: None }],

lightning/src/ln/payment_tests.rs

@@ -97,6 +97,8 @@ fn mpp_failure() {
 	route.paths[1].hops[0].pubkey = node_c_id;
 	route.paths[1].hops[0].short_channel_id = chan_2_id;
 	route.paths[1].hops[1].short_channel_id = chan_4_id;
+	route.route_params.as_mut().unwrap().final_value_msat *= 2;
+
 	let paths: &[&[_]] = &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]];
 	send_along_route_with_secret(&nodes[0], route, paths, 200_000, payment_hash, payment_secret);
 	fail_payment_along_route(&nodes[0], paths, false, payment_hash);
@@ -137,6 +139,7 @@ fn mpp_retry() {
 	route.paths[1].hops[0].pubkey = node_c_id;
 	route.paths[1].hops[0].short_channel_id = chan_2_update.contents.short_channel_id;
 	route.paths[1].hops[1].short_channel_id = chan_4_update.contents.short_channel_id;
+	route.route_params.as_mut().unwrap().final_value_msat *= 2;
 
 	// Initiate the MPP payment.
 	let id = PaymentId(hash.0);
@@ -360,6 +363,7 @@ fn do_mpp_receive_timeout(send_partial_mpp: bool) {
 	route.paths[1].hops[0].pubkey = node_c_id;
 	route.paths[1].hops[0].short_channel_id = chan_2_update.contents.short_channel_id;
 	route.paths[1].hops[1].short_channel_id = chan_4_update.contents.short_channel_id;
+	route.route_params.as_mut().unwrap().final_value_msat *= 2;
 
 	// Initiate the MPP payment.
 	let onion = RecipientOnionFields::secret_only(payment_secret);

lightning/src/routing/router.rs

@@ -633,7 +633,7 @@ impl Path {
 		}
 	}
 
-	/// Gets the final hop's CLTV expiry delta.
+	/// Gets the final hop's CLTV expiry delta, if there's a final non-blinded hop.
 	#[rustfmt::skip]
 	pub fn final_cltv_expiry_delta(&self) -> Option<u32> {
 		match &self.blinded_tail {
@@ -688,6 +688,66 @@ impl Route {
 	pub fn get_total_amount(&self) -> u64 {
 		self.paths.iter().map(|path| path.final_value_msat()).sum()
 	}
+
+	pub(crate) fn debug_assert_route_meets_params<L: Logger>(&self, logger: L) -> Result<(), ()> {
+		if let Some(route_params) = self.route_params.as_ref() {
+			// Check that we actually pay less than the max fee we set.
+			if let Some(max_total_fee) = route_params.max_total_routing_fee_msat {
+				let total_fee = self.get_total_fees();
+				if total_fee > max_total_fee {
+					let err = format!("Router returned an attempt to pay with a higher fee ({total_fee}msat) than we allowed ({max_total_fee}msat). Your router is critically buggy!");
+					debug_assert!(false, "{}", err);
+					log_error!(logger, "{}", err);
+					return Err(());
+				}
+			}
+
+			// Test that we don't contain any "extra" MPP parts - while we're allowed to overshoot
+			// the `final_value_msat` specified in the `route_params`, we aren't allowed to have
+			// any MPP parts which aren't needed to meet `route_params.final_value_msat`.
+			let min_mpp_part = self.paths.iter().map(|h| h.final_value_msat()).min().unwrap_or(0);
+			if self.get_total_amount() - min_mpp_part >= route_params.final_value_msat {
+				let err = format!(
+					"Router returned an attempt to include more MPP parts than needed. The smallest MPP part ({min_mpp_part}msat) was not needed for a payment of {}msat. Your router is critically buggy!",
+					route_params.final_value_msat
+				);
+				debug_assert!(false, "{}", err);
+				log_error!(logger, "{}", err);
+				return Err(());
+			}
+
+			if self.paths.is_empty() {
+				let err = "Selected route had no paths. Your router is buggy!";
+				debug_assert!(false, "{}", err);
+				log_error!(logger, "{}", err);
+				return Err(());
+			}
+
+			for path in self.paths.iter() {
+				if path.hops.is_empty() {
+					let err = "Unusable path in route (path.hops.len() must be at least 1)";
+					debug_assert!(false, "{}", err);
+					log_error!(logger, "{}", err);
+					return Err(());
+				}
+
+				if path.hops.len() > route_params.payment_params.max_path_length.into() {
+					let err = format!(
+						"Path had a length of {}, which is greater than the maximum we're allowed ({})",
+						path.hops.len(),
+						route_params.payment_params.max_path_length,
+					);
+					#[cfg(any(test, feature = "_test_utils"))]
+					debug_assert!(false, "{}", err);
+					log_error!(logger, "{}", err);
+					// This is a bug, but there's not a material safety risk to making this
+					// payment, so we don't bother to error here.
+				}
+			}
+		}
+
+		Ok(())
+	}
 }
 
 impl fmt::Display for Route {
@@ -2491,9 +2551,11 @@ pub fn find_route<L: Logger, GL: Logger, S: ScoreLookUp>(
 	scorer: &S, score_params: &S::ScoreParams, random_seed_bytes: &[u8; 32]
 ) -> Result<Route, &'static str> {
 	let graph_lock = network_graph.read_only();
-	let mut route = get_route(our_node_pubkey, &route_params, &graph_lock, first_hops, logger,
+	let mut route = get_route(our_node_pubkey, &route_params, &graph_lock, first_hops, &logger,
 		scorer, score_params, random_seed_bytes)?;
 	add_random_cltv_offset(&mut route, &route_params.payment_params, &graph_lock, random_seed_bytes);
+	route.debug_assert_route_meets_params(&logger)
+		.map_err(|()| "Generated route doesn't comply with the parameters you specified. This indicates a bug in the router. Please report this bug!")?;
 	Ok(route)
 }