Skip to content

Commit 5d5d640

Browse files
tnulltnull
committed
Assert equality of route params in tests
Previously we only asserted the `final_value_msat` matches. Looking at it again we can _of course_ assert the full equality of looked-for and included route params after all (duh, not sure what I was thinking...). This cleans up the prior misunderstanding and fixes a bunch of tests that would now fail otherwise.
1 parent 6016101 commit 5d5d640

File tree

3 files changed

+26
-26
lines changed

3 files changed

+26
-26
lines changed

‎lightning/src/ln/outbound_payment.rs

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -885,12 +885,10 @@ impl OutboundPayments {
885885
RetryableSendFailure::RouteNotFound
886886
})?;
887887

888-
if let Some(route_route_params) = route.route_params.as_mut() {
889-
if route_route_params.final_value_msat != route_params.final_value_msat {
890-
debug_assert!(false,
891-
"Routers are expected to return a route which includes the requested final_value_msat");
892-
route_route_params.final_value_msat = route_params.final_value_msat;
893-
}
888+
if route.route_params.as_ref() != Some(&route_params) {
889+
debug_assert!(false,
890+
"Routers are expected to return a Route which includes the requested RouteParameters");
891+
route.route_params = Some(route_params.clone());
894892
}
895893

896894
let onion_session_privs = self.add_new_pending_payment(payment_hash,
@@ -947,12 +945,10 @@ impl OutboundPayments {
947945
}
948946
};
949947

950-
if let Some(route_route_params) = route.route_params.as_mut() {
951-
if route_route_params.final_value_msat != route_params.final_value_msat {
952-
debug_assert!(false,
953-
"Routers are expected to return a route which includes the requested final_value_msat");
954-
route_route_params.final_value_msat = route_params.final_value_msat;
955-
}
948+
if route.route_params.as_ref() != Some(&route_params) {
949+
debug_assert!(false,
950+
"Routers are expected to return a Route which includes the requested RouteParameters");
951+
route.route_params = Some(route_params.clone());
956952
}
957953

958954
for path in route.paths.iter() {
@@ -1935,7 +1931,9 @@ mod tests {
19351931
router.expect_find_route(route_params.clone(), Ok(route.clone()));
19361932
let mut route_params_w_failed_scid = route_params.clone();
19371933
route_params_w_failed_scid.payment_params.previously_failed_channels.push(failed_scid);
1938-
router.expect_find_route(route_params_w_failed_scid, Ok(route.clone()));
1934+
let mut route_w_failed_scid = route.clone();
1935+
route_w_failed_scid.route_params = Some(route_params_w_failed_scid.clone());
1936+
router.expect_find_route(route_params_w_failed_scid, Ok(route_w_failed_scid));
19391937
router.expect_find_route(route_params.clone(), Ok(route.clone()));
19401938
router.expect_find_route(route_params.clone(), Ok(route.clone()));
19411939

‎lightning/src/ln/payment_tests.rs

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,7 @@ fn mpp_retry() {
147147
// Check the remaining max total routing fee for the second attempt is 50_000 - 1_000 msat fee
148148
// used by the first path
149149
route_params.max_total_routing_fee_msat = Some(max_total_routing_fee_msat - 1_000);
150+
route.route_params = Some(route_params.clone());
150151
nodes[0].router.expect_find_route(route_params, Ok(route));
151152
nodes[0].node.process_pending_htlc_forwards();
152153
check_added_monitors!(nodes[0], 1);
@@ -253,12 +254,12 @@ fn mpp_retry_overpay() {
253254

254255
route.paths.remove(0);
255256
route_params.final_value_msat -= first_path_value;
256-
route.route_params.as_mut().map(|p| p.final_value_msat -= first_path_value);
257257
route_params.payment_params.previously_failed_channels.push(chan_4_update.contents.short_channel_id);
258-
259258
// Check the remaining max total routing fee for the second attempt accounts only for 1_000 msat
260259
// base fee, but not for overpaid value of the first try.
261260
route_params.max_total_routing_fee_msat.as_mut().map(|m| *m -= 1000);
261+
262+
route.route_params = Some(route_params.clone());
262263
nodes[0].router.expect_find_route(route_params, Ok(route));
263264
nodes[0].node.process_pending_htlc_forwards();
264265

@@ -2738,7 +2739,7 @@ fn retry_multi_path_single_failed_payment() {
27382739

27392740
let mut retry_params = RouteParameters::from_payment_params_and_value(pay_params, 100_000_000);
27402741
retry_params.max_total_routing_fee_msat = None;
2741-
route.route_params.as_mut().unwrap().final_value_msat = 100_000_000;
2742+
route.route_params = Some(retry_params.clone());
27422743
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
27432744

27442745
{
@@ -2809,9 +2810,7 @@ fn immediate_retry_on_failure() {
28092810
maybe_announced_channel: true,
28102811
}], blinded_tail: None },
28112812
],
2812-
route_params: Some(RouteParameters::from_payment_params_and_value(
2813-
PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV),
2814-
100_000_001)),
2813+
route_params: Some(route_params.clone()),
28152814
};
28162815
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
28172816
// On retry, split the payment across both channels.
@@ -2821,9 +2820,9 @@ fn immediate_retry_on_failure() {
28212820
route.paths[1].hops[0].fee_msat = 50_000_001;
28222821
let mut pay_params = route_params.payment_params.clone();
28232822
pay_params.previously_failed_channels.push(chans[0].short_channel_id.unwrap());
2824-
nodes[0].router.expect_find_route(
2825-
RouteParameters::from_payment_params_and_value(pay_params, amt_msat),
2826-
Ok(route.clone()));
2823+
let retry_params = RouteParameters::from_payment_params_and_value(pay_params, amt_msat);
2824+
route.route_params = Some(retry_params.clone());
2825+
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
28272826

28282827
nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret),
28292828
PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2933,6 +2932,7 @@ fn no_extra_retries_on_back_to_back_fail() {
29332932
route.paths[0].hops[1].fee_msat = amt_msat;
29342933
let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat);
29352934
retry_params.max_total_routing_fee_msat = None;
2935+
route.route_params = Some(retry_params.clone());
29362936
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
29372937

29382938
nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret),
@@ -3137,7 +3137,7 @@ fn test_simple_partial_retry() {
31373137
route.paths.remove(0);
31383138
let mut retry_params = RouteParameters::from_payment_params_and_value(second_payment_params, amt_msat / 2);
31393139
retry_params.max_total_routing_fee_msat = None;
3140-
route.route_params.as_mut().unwrap().final_value_msat = amt_msat / 2;
3140+
route.route_params = Some(retry_params.clone());
31413141
nodes[0].router.expect_find_route(retry_params, Ok(route.clone()));
31423142

31433143
nodes[0].node.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret),
@@ -3316,7 +3316,7 @@ fn test_threaded_payment_retries() {
33163316

33173317
// from here on out, the retry `RouteParameters` amount will be amt/1000
33183318
route_params.final_value_msat /= 1000;
3319-
route.route_params.as_mut().unwrap().final_value_msat /= 1000;
3319+
route.route_params = Some(route_params.clone());
33203320
route.paths.pop();
33213321

33223322
let end_time = Instant::now() + Duration::from_secs(1);
@@ -3358,6 +3358,7 @@ fn test_threaded_payment_retries() {
33583358
new_route_params.payment_params.previously_failed_channels = previously_failed_channels.clone();
33593359
new_route_params.max_total_routing_fee_msat.as_mut().map(|m| *m -= 100_000);
33603360
route.paths[0].hops[1].short_channel_id += 1;
3361+
route.route_params = Some(new_route_params.clone());
33613362
nodes[0].router.expect_find_route(new_route_params, Ok(route.clone()));
33623363

33633364
let bs_fail_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
@@ -3720,7 +3721,7 @@ fn test_retry_custom_tlvs() {
37203721
send_payment(&nodes[2], &vec!(&nodes[1])[..], 1_500_000);
37213722

37223723
let amt_msat = 1_000_000;
3723-
let (route, payment_hash, payment_preimage, payment_secret) =
3724+
let (mut route, payment_hash, payment_preimage, payment_secret) =
37243725
get_route_and_payment_hash!(nodes[0], nodes[2], amt_msat);
37253726

37263727
// Initiate the payment
@@ -3772,6 +3773,7 @@ fn test_retry_custom_tlvs() {
37723773

37733774
// Retry the payment and make sure it succeeds
37743775
route_params.payment_params.previously_failed_channels.push(chan_2_update.contents.short_channel_id);
3776+
route.route_params = Some(route_params.clone());
37753777
nodes[0].router.expect_find_route(route_params, Ok(route));
37763778
nodes[0].node.process_pending_htlc_forwards();
37773779
check_added_monitors!(nodes[0], 1);

‎lightning/src/util/test_utils.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ impl<'a> Router for TestRouter<'a> {
124124
if let Some((find_route_query, find_route_res)) = self.next_routes.lock().unwrap().pop_front() {
125125
assert_eq!(find_route_query, *params);
126126
if let Ok(ref route) = find_route_res {
127-
assert_eq!(route.route_params.as_ref().unwrap().final_value_msat, find_route_query.final_value_msat);
127+
assert_eq!(route.route_params, Some(find_route_query));
128128
let scorer = self.scorer.read().unwrap();
129129
let scorer = ScorerAccountingForInFlightHtlcs::new(scorer, &inflight_htlcs);
130130
for path in &route.paths {

0 commit comments

Comments
 (0)