Skip to content

Commit abf2891

Browse files
committed
Pass Route to send_payment as a reference, not move
ChannelManager::send_payment stopped utilizing its ownership of the Route with MPP (which, for readability, now clone()s the individual paths when creating HTLCSource::OutboundRoute objects). While this isn't ideal, it likely also makes sense to ensure that the user has access to the Route after sending to correlate individual path failures with the paths in the route or, in the future, retry individual paths. Thus, the easiest solution is to just take the Route by reference, allowing the user to retain ownership.
1 parent 2464750 commit abf2891

6 files changed

+71
-71
lines changed

fuzz/src/chanmon_consistency.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -408,7 +408,7 @@ pub fn do_test(data: &[u8]) {
408408
($source: expr, $dest: expr) => { {
409409
let payment_hash = Sha256::hash(&[payment_id; 1]);
410410
payment_id = payment_id.wrapping_add(1);
411-
if let Err(_) = $source.send_payment(Route {
411+
if let Err(_) = $source.send_payment(&Route {
412412
paths: vec![vec![RouteHop {
413413
pubkey: $dest.0.get_our_node_id(),
414414
node_features: NodeFeatures::empty(),
@@ -425,7 +425,7 @@ pub fn do_test(data: &[u8]) {
425425
($source: expr, $middle: expr, $dest: expr) => { {
426426
let payment_hash = Sha256::hash(&[payment_id; 1]);
427427
payment_id = payment_id.wrapping_add(1);
428-
if let Err(_) = $source.send_payment(Route {
428+
if let Err(_) = $source.send_payment(&Route {
429429
paths: vec![vec![RouteHop {
430430
pubkey: $middle.0.get_our_node_id(),
431431
node_features: NodeFeatures::empty(),
@@ -453,7 +453,7 @@ pub fn do_test(data: &[u8]) {
453453
payment_id = payment_id.wrapping_add(1);
454454
let payment_secret = Sha256::hash(&[payment_id; 1]);
455455
payment_id = payment_id.wrapping_add(1);
456-
if let Err(_) = $source.send_payment(Route {
456+
if let Err(_) = $source.send_payment(&Route {
457457
paths: vec![vec![RouteHop {
458458
pubkey: $middle.0.get_our_node_id(),
459459
node_features: NodeFeatures::empty(),

fuzz/src/full_stack.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -401,7 +401,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
401401
sha.input(&payment_hash.0[..]);
402402
payment_hash.0 = Sha256::from_engine(sha).into_inner();
403403
payments_sent += 1;
404-
match channelmanager.send_payment(route, payment_hash, &None) {
404+
match channelmanager.send_payment(&route, payment_hash, &None) {
405405
Ok(_) => {},
406406
Err(_) => return,
407407
}

lightning/src/ln/chanmon_update_fail_tests.rs

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ fn test_simple_monitor_permanent_update_fail() {
3030
let (_, payment_hash_1) = get_payment_preimage_hash!(&nodes[0]);
3131

3232
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::PermanentFailure);
33-
unwrap_send_err!(nodes[0].node.send_payment(route, payment_hash_1, &None), true, APIError::ChannelUnavailable {..}, {});
33+
unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_1, &None), true, APIError::ChannelUnavailable {..}, {});
3434
check_added_monitors!(nodes[0], 2);
3535

3636
let events_1 = nodes[0].node.get_and_clear_pending_msg_events();
@@ -64,7 +64,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
6464

6565
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
6666

67-
unwrap_send_err!(nodes[0].node.send_payment(route.clone(), payment_hash_1, &None), false, APIError::MonitorUpdateFailed, {});
67+
unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_1, &None), false, APIError::MonitorUpdateFailed, {});
6868
check_added_monitors!(nodes[0], 1);
6969

7070
assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
@@ -107,7 +107,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
107107
// Now set it to failed again...
108108
let (_, payment_hash_2) = get_payment_preimage_hash!(&nodes[0]);
109109
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
110-
unwrap_send_err!(nodes[0].node.send_payment(route, payment_hash_2, &None), false, APIError::MonitorUpdateFailed, {});
110+
unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_2, &None), false, APIError::MonitorUpdateFailed, {});
111111
check_added_monitors!(nodes[0], 1);
112112

113113
assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
@@ -170,7 +170,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
170170
let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
171171

172172
*nodes[0].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
173-
unwrap_send_err!(nodes[0].node.send_payment(route.clone(), payment_hash_2, &None), false, APIError::MonitorUpdateFailed, {});
173+
unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash_2, &None), false, APIError::MonitorUpdateFailed, {});
174174
check_added_monitors!(nodes[0], 1);
175175

176176
assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
@@ -497,7 +497,7 @@ fn test_monitor_update_fail_cs() {
497497

498498
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
499499
let (payment_preimage, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
500-
nodes[0].node.send_payment(route, our_payment_hash, &None).unwrap();
500+
nodes[0].node.send_payment(&route, our_payment_hash, &None).unwrap();
501501
check_added_monitors!(nodes[0], 1);
502502

503503
let send_event = SendEvent::from_event(nodes[0].node.get_and_clear_pending_msg_events().remove(0));
@@ -582,7 +582,7 @@ fn test_monitor_update_fail_no_rebroadcast() {
582582

583583
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
584584
let (payment_preimage_1, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
585-
nodes[0].node.send_payment(route, our_payment_hash, &None).unwrap();
585+
nodes[0].node.send_payment(&route, our_payment_hash, &None).unwrap();
586586
check_added_monitors!(nodes[0], 1);
587587

588588
let send_event = SendEvent::from_event(nodes[0].node.get_and_clear_pending_msg_events().remove(0));
@@ -630,13 +630,13 @@ fn test_monitor_update_raa_while_paused() {
630630

631631
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
632632
let (payment_preimage_1, our_payment_hash_1) = get_payment_preimage_hash!(nodes[0]);
633-
nodes[0].node.send_payment(route, our_payment_hash_1, &None).unwrap();
633+
nodes[0].node.send_payment(&route, our_payment_hash_1, &None).unwrap();
634634
check_added_monitors!(nodes[0], 1);
635635
let send_event_1 = SendEvent::from_event(nodes[0].node.get_and_clear_pending_msg_events().remove(0));
636636

637637
let route = nodes[1].router.get_route(&nodes[0].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
638638
let (payment_preimage_2, our_payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
639-
nodes[1].node.send_payment(route, our_payment_hash_2, &None).unwrap();
639+
nodes[1].node.send_payment(&route, our_payment_hash_2, &None).unwrap();
640640
check_added_monitors!(nodes[1], 1);
641641
let send_event_2 = SendEvent::from_event(nodes[1].node.get_and_clear_pending_msg_events().remove(0));
642642

@@ -724,7 +724,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
724724
// holding cell.
725725
let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
726726
let route = nodes[0].router.get_route(&nodes[2].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
727-
nodes[0].node.send_payment(route, payment_hash_2, &None).unwrap();
727+
nodes[0].node.send_payment(&route, payment_hash_2, &None).unwrap();
728728
check_added_monitors!(nodes[0], 1);
729729

730730
let mut send_event = SendEvent::from_event(nodes[0].node.get_and_clear_pending_msg_events().remove(0));
@@ -749,7 +749,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
749749

750750
let (_, payment_hash_3) = get_payment_preimage_hash!(nodes[0]);
751751
let route = nodes[0].router.get_route(&nodes[2].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
752-
nodes[0].node.send_payment(route, payment_hash_3, &None).unwrap();
752+
nodes[0].node.send_payment(&route, payment_hash_3, &None).unwrap();
753753
check_added_monitors!(nodes[0], 1);
754754

755755
*nodes[1].chan_monitor.update_ret.lock().unwrap() = Ok(()); // We succeed in updating the monitor for the first channel
@@ -796,7 +796,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
796796
// Try to route another payment backwards from 2 to make sure 1 holds off on responding
797797
let (payment_preimage_4, payment_hash_4) = get_payment_preimage_hash!(nodes[0]);
798798
let route = nodes[2].router.get_route(&nodes[0].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
799-
nodes[2].node.send_payment(route, payment_hash_4, &None).unwrap();
799+
nodes[2].node.send_payment(&route, payment_hash_4, &None).unwrap();
800800
check_added_monitors!(nodes[2], 1);
801801

802802
send_event = SendEvent::from_event(nodes[2].node.get_and_clear_pending_msg_events().remove(0));
@@ -1047,9 +1047,9 @@ fn raa_no_response_awaiting_raa_state() {
10471047
// immediately after a CS. By setting failing the monitor update failure from the CS (which
10481048
// requires only an RAA response due to AwaitingRAA) we can deliver the RAA and require the CS
10491049
// generation during RAA while in monitor-update-failed state.
1050-
nodes[0].node.send_payment(route.clone(), payment_hash_1, &None).unwrap();
1050+
nodes[0].node.send_payment(&route, payment_hash_1, &None).unwrap();
10511051
check_added_monitors!(nodes[0], 1);
1052-
nodes[0].node.send_payment(route.clone(), payment_hash_2, &None).unwrap();
1052+
nodes[0].node.send_payment(&route, payment_hash_2, &None).unwrap();
10531053
check_added_monitors!(nodes[0], 0);
10541054

10551055
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
@@ -1097,7 +1097,7 @@ fn raa_no_response_awaiting_raa_state() {
10971097
// We send a third payment here, which is somewhat of a redundant test, but the
10981098
// chanmon_fail_consistency test required it to actually find the bug (by seeing out-of-sync
10991099
// commitment transaction states) whereas here we can explicitly check for it.
1100-
nodes[0].node.send_payment(route.clone(), payment_hash_3, &None).unwrap();
1100+
nodes[0].node.send_payment(&route, payment_hash_3, &None).unwrap();
11011101
check_added_monitors!(nodes[0], 0);
11021102
assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
11031103

@@ -1186,7 +1186,7 @@ fn claim_while_disconnected_monitor_update_fail() {
11861186
// the monitor still failed
11871187
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
11881188
let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
1189-
nodes[0].node.send_payment(route, payment_hash_2, &None).unwrap();
1189+
nodes[0].node.send_payment(&route, payment_hash_2, &None).unwrap();
11901190
check_added_monitors!(nodes[0], 1);
11911191

11921192
let as_updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
@@ -1278,7 +1278,7 @@ fn monitor_failed_no_reestablish_response() {
12781278
// on receipt).
12791279
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
12801280
let (payment_preimage_1, payment_hash_1) = get_payment_preimage_hash!(nodes[0]);
1281-
nodes[0].node.send_payment(route, payment_hash_1, &None).unwrap();
1281+
nodes[0].node.send_payment(&route, payment_hash_1, &None).unwrap();
12821282
check_added_monitors!(nodes[0], 1);
12831283

12841284
*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
@@ -1348,7 +1348,7 @@ fn first_message_on_recv_ordering() {
13481348
// can deliver it and fail the monitor update.
13491349
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
13501350
let (payment_preimage_1, payment_hash_1) = get_payment_preimage_hash!(nodes[0]);
1351-
nodes[0].node.send_payment(route, payment_hash_1, &None).unwrap();
1351+
nodes[0].node.send_payment(&route, payment_hash_1, &None).unwrap();
13521352
check_added_monitors!(nodes[0], 1);
13531353

13541354
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
@@ -1370,7 +1370,7 @@ fn first_message_on_recv_ordering() {
13701370
// Route the second payment, generating an update_add_htlc/commitment_signed
13711371
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
13721372
let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
1373-
nodes[0].node.send_payment(route, payment_hash_2, &None).unwrap();
1373+
nodes[0].node.send_payment(&route, payment_hash_2, &None).unwrap();
13741374
check_added_monitors!(nodes[0], 1);
13751375
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
13761376
assert_eq!(events.len(), 1);
@@ -1446,7 +1446,7 @@ fn test_monitor_update_fail_claim() {
14461446

14471447
let route = nodes[2].router.get_route(&nodes[0].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
14481448
let (_, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
1449-
nodes[2].node.send_payment(route, payment_hash_2, &None).unwrap();
1449+
nodes[2].node.send_payment(&route, payment_hash_2, &None).unwrap();
14501450
check_added_monitors!(nodes[2], 1);
14511451

14521452
// Successfully update the monitor on the 1<->2 channel, but the 0<->1 channel should still be
@@ -1527,7 +1527,7 @@ fn test_monitor_update_on_pending_forwards() {
15271527

15281528
let route = nodes[2].router.get_route(&nodes[0].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
15291529
let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
1530-
nodes[2].node.send_payment(route, payment_hash_2, &None).unwrap();
1530+
nodes[2].node.send_payment(&route, payment_hash_2, &None).unwrap();
15311531
check_added_monitors!(nodes[2], 1);
15321532

15331533
let mut events = nodes[2].node.get_and_clear_pending_msg_events();
@@ -1586,7 +1586,7 @@ fn monitor_update_claim_fail_no_response() {
15861586
// Now start forwarding a second payment, skipping the last RAA so B is in AwaitingRAA
15871587
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
15881588
let (payment_preimage_2, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
1589-
nodes[0].node.send_payment(route, payment_hash_2, &None).unwrap();
1589+
nodes[0].node.send_payment(&route, payment_hash_2, &None).unwrap();
15901590
check_added_monitors!(nodes[0], 1);
15911591

15921592
let mut events = nodes[0].node.get_and_clear_pending_msg_events();

lightning/src/ln/channelmanager.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1261,7 +1261,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
12611261
/// If a payment_secret *is* provided, we assume that the invoice had the payment_secret feature
12621262
/// bit set (either as required or as available). If multiple paths are present in the Route,
12631263
/// we assume the invoice had the basic_mpp feature set.
1264-
pub fn send_payment(&self, route: Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>) -> Result<(), PaymentSendFailure> {
1264+
pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>) -> Result<(), PaymentSendFailure> {
12651265
if route.paths.len() < 1 {
12661266
return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"}));
12671267
}

lightning/src/ln/functional_test_utils.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -777,7 +777,7 @@ macro_rules! expect_payment_failed {
777777
}
778778

779779
pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>) {
780-
origin_node.node.send_payment(route, our_payment_hash, &our_payment_secret).unwrap();
780+
origin_node.node.send_payment(&route, our_payment_hash, &our_payment_secret).unwrap();
781781
check_added_monitors!(origin_node, expected_paths.len());
782782

783783
let mut events = origin_node.node.get_and_clear_pending_msg_events();
@@ -953,7 +953,7 @@ pub fn route_over_limit<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_rou
953953
}
954954

955955
let (_, our_payment_hash) = get_payment_preimage_hash!(origin_node);
956-
unwrap_send_err!(origin_node.node.send_payment(route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
956+
unwrap_send_err!(origin_node.node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
957957
assert_eq!(err, "Cannot send value that would put us over the max HTLC value in flight our peer will accept"));
958958
}
959959

0 commit comments

Comments
 (0)