Skip to content

Commit c86cacd

Browse files
authored
Merge pull request #2625 from tnull/2023-09-moar-tests-n-fixes
Improve test coverage of #2575 router fixes
2 parents 2c51080 + 3a8bf89 commit c86cacd

File tree

4 files changed

+223
-26
lines changed

4 files changed

+223
-26
lines changed

lightning/src/ln/outbound_payment.rs

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -901,12 +901,10 @@ impl OutboundPayments {
901901
RetryableSendFailure::RouteNotFound
902902
})?;
903903

904-
if let Some(route_route_params) = route.route_params.as_mut() {
905-
if route_route_params.final_value_msat != route_params.final_value_msat {
906-
debug_assert!(false,
907-
"Routers are expected to return a route which includes the requested final_value_msat");
908-
route_route_params.final_value_msat = route_params.final_value_msat;
909-
}
904+
if route.route_params.as_ref() != Some(&route_params) {
905+
debug_assert!(false,
906+
"Routers are expected to return a Route which includes the requested RouteParameters");
907+
route.route_params = Some(route_params.clone());
910908
}
911909

912910
let onion_session_privs = self.add_new_pending_payment(payment_hash,
@@ -963,12 +961,10 @@ impl OutboundPayments {
963961
}
964962
};
965963

966-
if let Some(route_route_params) = route.route_params.as_mut() {
967-
if route_route_params.final_value_msat != route_params.final_value_msat {
968-
debug_assert!(false,
969-
"Routers are expected to return a route which includes the requested final_value_msat");
970-
route_route_params.final_value_msat = route_params.final_value_msat;
971-
}
964+
if route.route_params.as_ref() != Some(&route_params) {
965+
debug_assert!(false,
966+
"Routers are expected to return a Route which includes the requested RouteParameters");
967+
route.route_params = Some(route_params.clone());
972968
}
973969

974970
for path in route.paths.iter() {
@@ -1958,7 +1954,9 @@ mod tests {
19581954
router.expect_find_route(route_params.clone(), Ok(route.clone()));
19591955
let mut route_params_w_failed_scid = route_params.clone();
19601956
route_params_w_failed_scid.payment_params.previously_failed_channels.push(failed_scid);
1961-
router.expect_find_route(route_params_w_failed_scid, Ok(route.clone()));
1957+
let mut route_w_failed_scid = route.clone();
1958+
route_w_failed_scid.route_params = Some(route_params_w_failed_scid.clone());
1959+
router.expect_find_route(route_params_w_failed_scid, Ok(route_w_failed_scid));
19621960
router.expect_find_route(route_params.clone(), Ok(route.clone()));
19631961
router.expect_find_route(route_params.clone(), Ok(route.clone()));
19641962

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/routing/router.rs

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7691,6 +7691,203 @@ mod tests {
76917691
assert_eq!(route.paths.len(), 1);
76927692
assert_eq!(route.get_total_amount(), amt_msat);
76937693
}
7694+
7695+
#[test]
7696+
fn first_hop_preferred_over_hint() {
7697+
// Check that if we have a first hop to a peer we'd always prefer that over a route hint
7698+
// they gave us, but we'd still consider all subsequent hints if they are more attractive.
7699+
let secp_ctx = Secp256k1::new();
7700+
let logger = Arc::new(ln_test_utils::TestLogger::new());
7701+
let network_graph = Arc::new(NetworkGraph::new(Network::Testnet, Arc::clone(&logger)));
7702+
let gossip_sync = P2PGossipSync::new(Arc::clone(&network_graph), None, Arc::clone(&logger));
7703+
let scorer = ln_test_utils::TestScorer::new();
7704+
let keys_manager = ln_test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
7705+
let random_seed_bytes = keys_manager.get_secure_random_bytes();
7706+
let config = UserConfig::default();
7707+
7708+
let amt_msat = 1_000_000;
7709+
let (our_privkey, our_node_id, privkeys, nodes) = get_nodes(&secp_ctx);
7710+
7711+
add_channel(&gossip_sync, &secp_ctx, &our_privkey, &privkeys[0],
7712+
ChannelFeatures::from_le_bytes(id_to_feature_flags(1)), 1);
7713+
update_channel(&gossip_sync, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
7714+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
7715+
short_channel_id: 1,
7716+
timestamp: 1,
7717+
flags: 0,
7718+
cltv_expiry_delta: 42,
7719+
htlc_minimum_msat: 1_000,
7720+
htlc_maximum_msat: 10_000_000,
7721+
fee_base_msat: 800,
7722+
fee_proportional_millionths: 0,
7723+
excess_data: Vec::new()
7724+
});
7725+
update_channel(&gossip_sync, &secp_ctx, &privkeys[0], UnsignedChannelUpdate {
7726+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
7727+
short_channel_id: 1,
7728+
timestamp: 1,
7729+
flags: 1,
7730+
cltv_expiry_delta: 42,
7731+
htlc_minimum_msat: 1_000,
7732+
htlc_maximum_msat: 10_000_000,
7733+
fee_base_msat: 800,
7734+
fee_proportional_millionths: 0,
7735+
excess_data: Vec::new()
7736+
});
7737+
7738+
add_channel(&gossip_sync, &secp_ctx, &privkeys[0], &privkeys[1],
7739+
ChannelFeatures::from_le_bytes(id_to_feature_flags(1)), 2);
7740+
update_channel(&gossip_sync, &secp_ctx, &privkeys[0], UnsignedChannelUpdate {
7741+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
7742+
short_channel_id: 2,
7743+
timestamp: 2,
7744+
flags: 0,
7745+
cltv_expiry_delta: 42,
7746+
htlc_minimum_msat: 1_000,
7747+
htlc_maximum_msat: 10_000_000,
7748+
fee_base_msat: 800,
7749+
fee_proportional_millionths: 0,
7750+
excess_data: Vec::new()
7751+
});
7752+
update_channel(&gossip_sync, &secp_ctx, &privkeys[1], UnsignedChannelUpdate {
7753+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
7754+
short_channel_id: 2,
7755+
timestamp: 2,
7756+
flags: 1,
7757+
cltv_expiry_delta: 42,
7758+
htlc_minimum_msat: 1_000,
7759+
htlc_maximum_msat: 10_000_000,
7760+
fee_base_msat: 800,
7761+
fee_proportional_millionths: 0,
7762+
excess_data: Vec::new()
7763+
});
7764+
7765+
let dest_node_id = nodes[2];
7766+
7767+
let route_hint = RouteHint(vec![RouteHintHop {
7768+
src_node_id: our_node_id,
7769+
short_channel_id: 44,
7770+
fees: RoutingFees {
7771+
base_msat: 234,
7772+
proportional_millionths: 0,
7773+
},
7774+
cltv_expiry_delta: 10,
7775+
htlc_minimum_msat: None,
7776+
htlc_maximum_msat: Some(5_000_000),
7777+
},
7778+
RouteHintHop {
7779+
src_node_id: nodes[0],
7780+
short_channel_id: 45,
7781+
fees: RoutingFees {
7782+
base_msat: 123,
7783+
proportional_millionths: 0,
7784+
},
7785+
cltv_expiry_delta: 10,
7786+
htlc_minimum_msat: None,
7787+
htlc_maximum_msat: None,
7788+
}]);
7789+
7790+
let payment_params = PaymentParameters::from_node_id(dest_node_id, 42)
7791+
.with_route_hints(vec![route_hint]).unwrap()
7792+
.with_bolt11_features(channelmanager::provided_invoice_features(&config)).unwrap();
7793+
let route_params = RouteParameters::from_payment_params_and_value(
7794+
payment_params, amt_msat);
7795+
7796+
// First create an insufficient first hop for channel with SCID 1 and check we'd use the
7797+
// route hint.
7798+
let first_hop = get_channel_details(Some(1), nodes[0],
7799+
channelmanager::provided_init_features(&config), 999_999);
7800+
let first_hops = vec![first_hop];
7801+
7802+
let route = get_route(&our_node_id, &route_params.clone(), &network_graph.read_only(),
7803+
Some(&first_hops.iter().collect::<Vec<_>>()), Arc::clone(&logger), &scorer,
7804+
&Default::default(), &random_seed_bytes).unwrap();
7805+
assert_eq!(route.paths.len(), 1);
7806+
assert_eq!(route.get_total_amount(), amt_msat);
7807+
assert_eq!(route.paths[0].hops.len(), 2);
7808+
assert_eq!(route.paths[0].hops[0].short_channel_id, 44);
7809+
assert_eq!(route.paths[0].hops[1].short_channel_id, 45);
7810+
assert_eq!(route.get_total_fees(), 123);
7811+
7812+
// Now check we would trust our first hop info, i.e., fail if we detect the route hint is
7813+
// for a first hop channel.
7814+
let mut first_hop = get_channel_details(Some(1), nodes[0], channelmanager::provided_init_features(&config), 999_999);
7815+
first_hop.outbound_scid_alias = Some(44);
7816+
let first_hops = vec![first_hop];
7817+
7818+
let route_res = get_route(&our_node_id, &route_params.clone(), &network_graph.read_only(),
7819+
Some(&first_hops.iter().collect::<Vec<_>>()), Arc::clone(&logger), &scorer,
7820+
&Default::default(), &random_seed_bytes);
7821+
assert!(route_res.is_err());
7822+
7823+
// Finally check we'd use the first hop if has sufficient outbound capacity. But we'd stil
7824+
// use the cheaper second hop of the route hint.
7825+
let mut first_hop = get_channel_details(Some(1), nodes[0],
7826+
channelmanager::provided_init_features(&config), 10_000_000);
7827+
first_hop.outbound_scid_alias = Some(44);
7828+
let first_hops = vec![first_hop];
7829+
7830+
let route = get_route(&our_node_id, &route_params.clone(), &network_graph.read_only(),
7831+
Some(&first_hops.iter().collect::<Vec<_>>()), Arc::clone(&logger), &scorer,
7832+
&Default::default(), &random_seed_bytes).unwrap();
7833+
assert_eq!(route.paths.len(), 1);
7834+
assert_eq!(route.get_total_amount(), amt_msat);
7835+
assert_eq!(route.paths[0].hops.len(), 2);
7836+
assert_eq!(route.paths[0].hops[0].short_channel_id, 1);
7837+
assert_eq!(route.paths[0].hops[1].short_channel_id, 45);
7838+
assert_eq!(route.get_total_fees(), 123);
7839+
}
7840+
7841+
#[test]
7842+
fn allow_us_being_first_hint() {
7843+
// Check that we consider a route hint even if we are the src of the first hop.
7844+
let secp_ctx = Secp256k1::new();
7845+
let logger = Arc::new(ln_test_utils::TestLogger::new());
7846+
let network_graph = Arc::new(NetworkGraph::new(Network::Testnet, Arc::clone(&logger)));
7847+
let scorer = ln_test_utils::TestScorer::new();
7848+
let keys_manager = ln_test_utils::TestKeysInterface::new(&[0u8; 32], Network::Testnet);
7849+
let random_seed_bytes = keys_manager.get_secure_random_bytes();
7850+
let config = UserConfig::default();
7851+
7852+
let (_, our_node_id, _, nodes) = get_nodes(&secp_ctx);
7853+
7854+
let amt_msat = 1_000_000;
7855+
let dest_node_id = nodes[1];
7856+
7857+
let first_hop = get_channel_details(Some(1), nodes[0], channelmanager::provided_init_features(&config), 10_000_000);
7858+
let first_hops = vec![first_hop];
7859+
7860+
let route_hint = RouteHint(vec![RouteHintHop {
7861+
src_node_id: our_node_id,
7862+
short_channel_id: 44,
7863+
fees: RoutingFees {
7864+
base_msat: 123,
7865+
proportional_millionths: 0,
7866+
},
7867+
cltv_expiry_delta: 10,
7868+
htlc_minimum_msat: None,
7869+
htlc_maximum_msat: None,
7870+
}]);
7871+
7872+
let payment_params = PaymentParameters::from_node_id(dest_node_id, 42)
7873+
.with_route_hints(vec![route_hint]).unwrap()
7874+
.with_bolt11_features(channelmanager::provided_invoice_features(&config)).unwrap();
7875+
7876+
let route_params = RouteParameters::from_payment_params_and_value(
7877+
payment_params, amt_msat);
7878+
7879+
7880+
let route = get_route(&our_node_id, &route_params, &network_graph.read_only(),
7881+
Some(&first_hops.iter().collect::<Vec<_>>()), Arc::clone(&logger), &scorer,
7882+
&Default::default(), &random_seed_bytes).unwrap();
7883+
7884+
assert_eq!(route.paths.len(), 1);
7885+
assert_eq!(route.get_total_amount(), amt_msat);
7886+
assert_eq!(route.get_total_fees(), 0);
7887+
assert_eq!(route.paths[0].hops.len(), 1);
7888+
7889+
assert_eq!(route.paths[0].hops[0].short_channel_id, 44);
7890+
}
76947891
}
76957892

76967893
#[cfg(all(any(test, ldk_bench), not(feature = "no-std")))]

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)