Test the RouteParameters passed to TestRouter

`TestRouter` allows us to simply select the `Route` that will be
returned in the next `find_route` call, but it does so without any
checking of what was *requested* for the call. This makes it a
somewhat dubious test utility as it very helpfully ensures we
ignore errors in the routes we're looking for.

Instead, we require users of `TestRouter` pass a `RouteParameters`
to `expect_find_route`, which we compare against the requested
parameters passed to `find_route`.

Author: TheBlueMatt

lightning/src/ln/outbound_payment.rs

@@ -1189,15 +1189,16 @@ mod tests {
 		let secp_ctx = Secp256k1::new();
 		let keys_manager = test_utils::TestKeysInterface::new(&[0; 32], Network::Testnet);
 
-		router.expect_find_route(Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError }));
-
 		let payment_params = PaymentParameters::from_node_id(
 			PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()), 0);
 		let route_params = RouteParameters {
 			payment_params,
 			final_value_msat: 0,
 			final_cltv_expiry_delta: 0,
 		};
+		router.expect_find_route(route_params.clone(),
+			Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError }));
+
 		let err = if on_retry {
 			outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), None, PaymentId([0; 32]),
 			&Route { paths: vec![], payment_params: None }, Retry::Attempts(1), Some(route_params.clone()),

lightning/src/ln/payment_tests.rs

@@ -1844,7 +1844,7 @@ fn auto_retry_partial_failure() {
 				cltv_expiry_delta: 100,
 			}],
 		],
-		payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)),
+		payment_params: Some(route_params.payment_params.clone()),
 	};
 	let retry_1_route = Route {
 		paths: vec![
@@ -1865,7 +1865,7 @@ fn auto_retry_partial_failure() {
 				cltv_expiry_delta: 100,
 			}],
 		],
-		payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)),
+		payment_params: Some(route_params.payment_params.clone()),
 	};
 	let retry_2_route = Route {
 		paths: vec![
@@ -1878,11 +1878,17 @@ fn auto_retry_partial_failure() {
 				cltv_expiry_delta: 100,
 			}],
 		],
-		payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)),
+		payment_params: Some(route_params.payment_params.clone()),
 	};
-	nodes[0].router.expect_find_route(Ok(send_route));
-	nodes[0].router.expect_find_route(Ok(retry_1_route));
-	nodes[0].router.expect_find_route(Ok(retry_2_route));
+	nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route));
+	nodes[0].router.expect_find_route(RouteParameters {
+			payment_params: route_params.payment_params.clone(),
+			final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV
+		}, Ok(retry_1_route));
+	nodes[0].router.expect_find_route(RouteParameters {
+			payment_params: route_params.payment_params.clone(),
+			final_value_msat: amt_msat / 4, final_cltv_expiry_delta: TEST_FINAL_CLTV
+		}, Ok(retry_2_route));
 
 	// Send a payment that will partially fail on send, then partially fail on retry, then succeed.
 	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(3)).unwrap();
@@ -2074,6 +2080,27 @@ fn retry_multi_path_single_failed_payment() {
 
 	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
 	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
+
+	let amt_msat = 100_010_000;
+
+	let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[1], amt_msat);
+	#[cfg(feature = "std")]
+	let payment_expiry_secs = SystemTime::UNIX_EPOCH.elapsed().unwrap().as_secs() + 60 * 60;
+	#[cfg(not(feature = "std"))]
+	let payment_expiry_secs = 60 * 60;
+	let mut invoice_features = InvoiceFeatures::empty();
+	invoice_features.set_variable_length_onion_required();
+	invoice_features.set_payment_secret_required();
+	invoice_features.set_basic_mpp_optional();
+	let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
+		.with_expiry_time(payment_expiry_secs as u64)
+		.with_features(invoice_features);
+	let route_params = RouteParameters {
+		payment_params: payment_params.clone(),
+		final_value_msat: amt_msat,
+		final_cltv_expiry_delta: TEST_FINAL_CLTV,
+	};
+
 	let chans = nodes[0].node.list_usable_channels();
 	let mut route = Route {
 		paths: vec![
@@ -2094,15 +2121,37 @@ fn retry_multi_path_single_failed_payment() {
 				cltv_expiry_delta: 100,
 			}],
 		],
-		payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)),
+		payment_params: Some(payment_params),
 	};
-	nodes[0].router.expect_find_route(Ok(route.clone()));
+	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
 	// On retry, split the payment across both channels.
 	route.paths[0][0].fee_msat = 50_000_001;
 	route.paths[1][0].fee_msat = 50_000_000;
-	nodes[0].router.expect_find_route(Ok(route.clone()));
+	nodes[0].router.expect_find_route(RouteParameters {
+			payment_params: route.payment_params.clone().unwrap(),
+			// Note that the second request here requests the amount we originally failed to send,
+			// not the amount remaining on the full payment, which should be changed.
+			final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV
+		}, Ok(route.clone()));
 
-	let amt_msat = 100_010_000;
+	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
+	let htlc_msgs = nodes[0].node.get_and_clear_pending_msg_events();
+	assert_eq!(htlc_msgs.len(), 2);
+	check_added_monitors!(nodes[0], 2);
+}
+
+#[test]
+fn immediate_retry_on_failure() {
+	// Tests that we can/will retry immediately after a failure
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
+	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
+
+	let amt_msat = 100_000_001;
 	let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[1], amt_msat);
 	#[cfg(feature = "std")]
 	let payment_expiry_secs = SystemTime::UNIX_EPOCH.elapsed().unwrap().as_secs() + 60 * 60;
@@ -2121,22 +2170,6 @@ fn retry_multi_path_single_failed_payment() {
 		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 
-	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
-	let htlc_msgs = nodes[0].node.get_and_clear_pending_msg_events();
-	assert_eq!(htlc_msgs.len(), 2);
-	check_added_monitors!(nodes[0], 2);
-}
-
-#[test]
-fn immediate_retry_on_failure() {
-	// Tests that we can/will retry immediately after a failure
-	let chanmon_cfgs = create_chanmon_cfgs(2);
-	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
-	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None, None]);
-	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-
-	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
-	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
 	let chans = nodes[0].node.list_usable_channels();
 	let mut route = Route {
 		paths: vec![
@@ -2151,32 +2184,16 @@ fn immediate_retry_on_failure() {
 		],
 		payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)),
 	};
-	nodes[0].router.expect_find_route(Ok(route.clone()));
+	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
 	// On retry, split the payment across both channels.
 	route.paths.push(route.paths[0].clone());
 	route.paths[0][0].short_channel_id = chans[1].short_channel_id.unwrap();
 	route.paths[0][0].fee_msat = 50_000_000;
 	route.paths[1][0].fee_msat = 50_000_001;
-	nodes[0].router.expect_find_route(Ok(route.clone()));
-
-	let amt_msat = 100_010_000;
-	let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[1], amt_msat);
-	#[cfg(feature = "std")]
-	let payment_expiry_secs = SystemTime::UNIX_EPOCH.elapsed().unwrap().as_secs() + 60 * 60;
-	#[cfg(not(feature = "std"))]
-	let payment_expiry_secs = 60 * 60;
-	let mut invoice_features = InvoiceFeatures::empty();
-	invoice_features.set_variable_length_onion_required();
-	invoice_features.set_payment_secret_required();
-	invoice_features.set_basic_mpp_optional();
-	let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
-		.with_expiry_time(payment_expiry_secs as u64)
-		.with_features(invoice_features);
-	let route_params = RouteParameters {
-		payment_params,
-		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
-	};
+	nodes[0].router.expect_find_route(RouteParameters {
+			payment_params: route_params.payment_params.clone(),
+			final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV
+		}, Ok(route.clone()));
 
 	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
 	let htlc_msgs = nodes[0].node.get_and_clear_pending_msg_events();
@@ -2208,6 +2225,25 @@ fn no_extra_retries_on_back_to_back_fail() {
 	let chan_1_scid = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0).0.contents.short_channel_id;
 	let chan_2_scid = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 0).0.contents.short_channel_id;
 
+	let amt_msat = 200_000_000;
+	let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[1], amt_msat);
+	#[cfg(feature = "std")]
+	let payment_expiry_secs = SystemTime::UNIX_EPOCH.elapsed().unwrap().as_secs() + 60 * 60;
+	#[cfg(not(feature = "std"))]
+	let payment_expiry_secs = 60 * 60;
+	let mut invoice_features = InvoiceFeatures::empty();
+	invoice_features.set_variable_length_onion_required();
+	invoice_features.set_payment_secret_required();
+	invoice_features.set_basic_mpp_optional();
+	let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
+		.with_expiry_time(payment_expiry_secs as u64)
+		.with_features(invoice_features);
+	let route_params = RouteParameters {
+		payment_params,
+		final_value_msat: amt_msat,
+		final_cltv_expiry_delta: TEST_FINAL_CLTV,
+	};
+
 	let mut route = Route {
 		paths: vec![
 			vec![RouteHop {
@@ -2243,29 +2279,15 @@ fn no_extra_retries_on_back_to_back_fail() {
 		],
 		payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV)),
 	};
-	nodes[0].router.expect_find_route(Ok(route.clone()));
+	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
 	// On retry, we'll only be asked for one path
 	route.paths.remove(1);
-	nodes[0].router.expect_find_route(Ok(route.clone()));
-
-	let amt_msat = 100_010_000;
-	let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[1], amt_msat);
-	#[cfg(feature = "std")]
-	let payment_expiry_secs = SystemTime::UNIX_EPOCH.elapsed().unwrap().as_secs() + 60 * 60;
-	#[cfg(not(feature = "std"))]
-	let payment_expiry_secs = 60 * 60;
-	let mut invoice_features = InvoiceFeatures::empty();
-	invoice_features.set_variable_length_onion_required();
-	invoice_features.set_payment_secret_required();
-	invoice_features.set_basic_mpp_optional();
-	let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
-		.with_expiry_time(payment_expiry_secs as u64)
-		.with_features(invoice_features);
-	let route_params = RouteParameters {
-		payment_params,
-		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
-	};
+	let mut second_payment_params = route_params.payment_params.clone();
+	second_payment_params.previously_failed_channels = vec![chan_2_scid, chan_2_scid];
+	nodes[0].router.expect_find_route(RouteParameters {
+			payment_params: second_payment_params,
+			final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV,
+		}, Ok(route.clone()));
 
 	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
 	let htlc_updates = SendEvent::from_node(&nodes[0]);

lightning/src/util/test_utils.rs

@@ -77,17 +77,17 @@ impl chaininterface::FeeEstimator for TestFeeEstimator {
 
 pub struct TestRouter<'a> {
 	pub network_graph: Arc<NetworkGraph<&'a TestLogger>>,
-	pub next_routes: Mutex<VecDeque<Result<Route, LightningError>>>,
+	pub next_routes: Mutex<VecDeque<(RouteParameters, Result<Route, LightningError>)>>,
 }
 
 impl<'a> TestRouter<'a> {
 	pub fn new(network_graph: Arc<NetworkGraph<&'a TestLogger>>) -> Self {
 		Self { network_graph, next_routes: Mutex::new(VecDeque::new()), }
 	}
 
-	pub fn expect_find_route(&self, result: Result<Route, LightningError>) {
+	pub fn expect_find_route(&self, query: RouteParameters, result: Result<Route, LightningError>) {
 		let mut expected_routes = self.next_routes.lock().unwrap();
-		expected_routes.push_back(result);
+		expected_routes.push_back((query, result));
 	}
 }
 
@@ -96,8 +96,9 @@ impl<'a> Router for TestRouter<'a> {
 		&self, payer: &PublicKey, params: &RouteParameters, first_hops: Option<&[&channelmanager::ChannelDetails]>,
 		inflight_htlcs: &InFlightHtlcs
 	) -> Result<Route, msgs::LightningError> {
-		if let Some(find_route_res) = self.next_routes.lock().unwrap().pop_front() {
-			return find_route_res
+		if let Some((find_route_query, find_route_res)) = self.next_routes.lock().unwrap().pop_front() {
+			assert_eq!(find_route_query, *params);
+			return find_route_res;
 		}
 		let logger = TestLogger::new();
 		find_route(