From 5ce4bfc1f6369cde16a88f2d2af416fc32f71082 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 27 Jan 2023 20:31:10 +0000 Subject: [PATCH] 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`. --- lightning/src/ln/outbound_payment.rs | 5 +- lightning/src/ln/payment_tests.rs | 158 +++++++++++++++------------ lightning/src/util/test_utils.rs | 11 +- 3 files changed, 99 insertions(+), 75 deletions(-) diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index a8acb9e8..d6e32ed2 100644 --- 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 5897d11b..c7c8198f 100644 --- 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 1dae61ab..f9a2eafb 100644 --- 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>, - pub next_routes: Mutex>>, + pub next_routes: Mutex)>>, } 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) { + pub fn expect_find_route(&self, query: RouteParameters, result: Result) { 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 { - 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( -- 2.30.2