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`.

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index a8acb9e80ba2796bca53288006cf02ecca59e76c..d6e32ed2519e030e49ebd2a3c6d909c500a344b1 100644 (file)
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -1189,8 +1189,6 @@ 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 {
@@ -1198,6 +1196,9 @@ mod tests {
                        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()),

diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index 5897d11be03de4cc8b2fb270f8e83d0dd2f17573..c7c8198f6c4cfbea3f643c9d17d143125ae6097e 100644 (file)
--- a/lightning/src/ln/payment_tests.rs
+++ b/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]);

diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs
index 1dae61ab3ef5b1841241b069612f71c7e5cc8f38..f9a2eafb82d51739011dc1e80d922655a0f329bb 100644 (file)
--- a/lightning/src/util/test_utils.rs
+++ b/lightning/src/util/test_utils.rs
@@ -77,7 +77,7 @@ 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> {
@@ -85,9 +85,9 @@ impl<'a> TestRouter<'a> {
                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(