Skip to content

Commit b451b35

Browse files
TheBlueMattTheBlueMatt
committed
Use only the failed amount when retrying payments, not the full amt
When we landed the initial in-`ChannelManager` payment retries, we stored the `RouteParameters` in the payment info, and then re-use it as-is for new payments. `RouteParameters` is intended to store the information specific to the *route*, `PaymentParameters` exists to store information specific to a payment. Worse, because we don't recalculate the amount stored in the `RouteParameters` before fetching a new route with it, we end up attempting to retry the full payment amount, rather than only the failed part. This issue brought to you by having redundant data in datastructures, part 5,001.
1 parent 410e532 commit b451b35

File tree

3 files changed

+216
-22
lines changed

3 files changed

+216
-22
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7359,7 +7359,7 @@ where
73597359
entry.insert(PendingOutboundPayment::Retryable {
73607360
retry_strategy: None,
73617361
attempts: PaymentAttempts::new(),
7362-
route_params: None,
7362+
payment_params: None,
73637363
session_privs: [session_priv_bytes].iter().map(|a| *a).collect(),
73647364
payment_hash: htlc.payment_hash,
73657365
payment_secret,

‎lightning/src/ln/outbound_payment.rs

Lines changed: 56 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ pub(crate) enum PendingOutboundPayment {
4343
Retryable {
4444
retry_strategy: Option<Retry>,
4545
attempts: PaymentAttempts,
46-
route_params: Option<RouteParameters>,
46+
payment_params: Option<PaymentParameters>,
4747
session_privs: HashSet<[u8; 32]>,
4848
payment_hash: PaymentHash,
4949
payment_secret: Option<PaymentSecret>,
@@ -102,9 +102,17 @@ impl PendingOutboundPayment {
102102
_ => false,
103103
}
104104
}
105+
fn payment_parameters(&mut self) -> Option<&mut PaymentParameters> {
106+
match self {
107+
PendingOutboundPayment::Retryable { payment_params: Some(ref mut params), .. } => {
108+
Some(params)
109+
},
110+
_ => None,
111+
}
112+
}
105113
pub fn insert_previously_failed_scid(&mut self, scid: u64) {
106-
if let PendingOutboundPayment::Retryable { route_params: Some(params), .. } = self {
107-
params.payment_params.previously_failed_channels.push(scid);
114+
if let PendingOutboundPayment::Retryable { payment_params: Some(params), .. } = self {
115+
params.previously_failed_channels.push(scid);
108116
}
109117
}
110118
pub(super) fn is_fulfilled(&self) -> bool {
@@ -475,13 +483,22 @@ impl OutboundPayments {
475483
for (pmt_id, pmt) in outbounds.iter_mut() {
476484
let retries_remaining = pmt.auto_retries_remaining_now();
477485
if retries_remaining > 0 {
478-
if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, route_params: Some(params), .. } = pmt {
486+
if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, payment_params: Some(params), .. } = pmt {
479487
if pending_amt_msat < total_msat {
480488
let mut params = params.clone();
481-
if params.payment_params.max_path_count as usize > retries_remaining {
482-
params.payment_params.max_path_count = retries_remaining as u8;
489+
if params.max_path_count as usize > retries_remaining {
490+
params.max_path_count = retries_remaining as u8;
483491
}
484-
retry_id_route_params = Some((*pmt_id, params));
492+
retry_id_route_params = Some((*pmt_id, RouteParameters {
493+
final_value_msat: *total_msat - *pending_amt_msat,
494+
final_cltv_expiry_delta:
495+
if let Some(delta) = params.final_cltv_expiry_delta { delta }
496+
else {
497+
debug_assert!(false, "We always set the final_cltv_expiry_delta when a path fails");
498+
super::channelmanager::MIN_FINAL_CLTV_EXPIRY_DELTA.into()
499+
},
500+
payment_params: params,
501+
}));
485502
break
486503
}
487504
}
@@ -692,7 +709,7 @@ impl OutboundPayments {
692709
let payment = entry.insert(PendingOutboundPayment::Retryable {
693710
retry_strategy,
694711
attempts: PaymentAttempts::new(),
695-
route_params,
712+
payment_params: route_params.map(|p| p.payment_params),
696713
session_privs: HashSet::new(),
697714
pending_amt_msat: 0,
698715
pending_fee_msat: Some(0),
@@ -970,6 +987,7 @@ impl OutboundPayments {
970987
let mut all_paths_failed = false;
971988
let mut full_failure_ev = None;
972989
let mut pending_retry_ev = None;
990+
let mut retry = None;
973991
let attempts_remaining = if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
974992
if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
975993
log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
@@ -983,6 +1001,33 @@ impl OutboundPayments {
9831001
if let Some(scid) = short_channel_id {
9841002
payment.get_mut().insert_previously_failed_scid(scid);
9851003
}
1004+
1005+
// We want to move towards only using the `PaymentParameters` in the outbound payments
1006+
// map. However, for backwards-compatibility, we still need to support passing the
1007+
// `PaymentParameters` data that was shoved in the HTLC (and given to us via
1008+
// `payment_params`) back to the user.
1009+
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
1010+
if let Some(params) = payment.get_mut().payment_parameters() {
1011+
if params.final_cltv_expiry_delta.is_none() {
1012+
// This should be rare, but a user could provide None for the payment data, and
1013+
// we need it when we go to retry the payment, so fill it in.
1014+
params.final_cltv_expiry_delta = Some(path_last_hop.cltv_expiry_delta);
1015+
}
1016+
retry = Some(RouteParameters {
1017+
payment_params: params.clone(),
1018+
final_value_msat: path_last_hop.fee_msat,
1019+
final_cltv_expiry_delta: params.final_cltv_expiry_delta.unwrap(),
1020+
});
1021+
} else if let Some(payment_params_data) = payment_params {
1022+
retry = Some(RouteParameters {
1023+
payment_params: payment_params_data.clone(),
1024+
final_value_msat: path_last_hop.fee_msat,
1025+
final_cltv_expiry_delta:
1026+
if let Some(delta) = payment_params_data.final_cltv_expiry_delta { delta }
1027+
else { path_last_hop.cltv_expiry_delta },
1028+
});
1029+
}
1030+
9861031
if payment.get().remaining_parts() == 0 {
9871032
all_paths_failed = true;
9881033
if payment.get().abandoned() {
@@ -999,16 +1044,6 @@ impl OutboundPayments {
9991044
return
10001045
};
10011046
core::mem::drop(outbounds);
1002-
let mut retry = if let Some(payment_params_data) = payment_params {
1003-
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
1004-
Some(RouteParameters {
1005-
payment_params: payment_params_data.clone(),
1006-
final_value_msat: path_last_hop.fee_msat,
1007-
final_cltv_expiry_delta:
1008-
if let Some(delta) = payment_params_data.final_cltv_expiry_delta { delta }
1009-
else { path_last_hop.cltv_expiry_delta },
1010-
})
1011-
} else { None };
10121047
log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
10131048

10141049
let path_failure = {
@@ -1120,13 +1155,13 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
11201155
(0, session_privs, required),
11211156
(1, pending_fee_msat, option),
11221157
(2, payment_hash, required),
1123-
(not_written, retry_strategy, (static_value, None)),
1158+
(3, payment_params, option),
11241159
(4, payment_secret, option),
1125-
(not_written, attempts, (static_value, PaymentAttempts::new())),
11261160
(6, total_msat, required),
1127-
(not_written, route_params, (static_value, None)),
11281161
(8, pending_amt_msat, required),
11291162
(10, starting_block_height, required),
1163+
(not_written, retry_strategy, (static_value, None)),
1164+
(not_written, attempts, (static_value, PaymentAttempts::new())),
11301165
},
11311166
(3, Abandoned) => {
11321167
(0, session_privs, required),

‎lightning/src/ln/payment_tests.rs

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2418,3 +2418,162 @@ fn no_extra_retries_on_back_to_back_fail() {
24182418
_ => panic!("Unexpected event"),
24192419
}
24202420
}
2421+
2422+
#[test]
2423+
fn test_simple_partial_retry() {
2424+
// In the first version of the in-`ChannelManager` payment retries, retries were sent for the
2425+
// full amount of the payment, rather than only the missing amount. Here we simply test for
2426+
// this by sending a payment with two parts, failing one, and retrying the second. Note that
2427+
// `TestRouter` will check that the `RouteParameters` (which contain the amount) matches the
2428+
// request.
2429+
let chanmon_cfgs = create_chanmon_cfgs(3);
2430+
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
2431+
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
2432+
let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
2433+
2434+
let chan_1_scid = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0).0.contents.short_channel_id;
2435+
let chan_2_scid = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 0).0.contents.short_channel_id;
2436+
2437+
let amt_msat = 200_000_000;
2438+
let (_, payment_hash, _, payment_secret) = get_route_and_payment_hash!(&nodes[0], nodes[2], amt_msat);
2439+
#[cfg(feature = "std")]
2440+
let payment_expiry_secs = SystemTime::UNIX_EPOCH.elapsed().unwrap().as_secs() + 60 * 60;
2441+
#[cfg(not(feature = "std"))]
2442+
let payment_expiry_secs = 60 * 60;
2443+
let mut invoice_features = InvoiceFeatures::empty();
2444+
invoice_features.set_variable_length_onion_required();
2445+
invoice_features.set_payment_secret_required();
2446+
invoice_features.set_basic_mpp_optional();
2447+
let payment_params = PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), TEST_FINAL_CLTV)
2448+
.with_expiry_time(payment_expiry_secs as u64)
2449+
.with_features(invoice_features);
2450+
let route_params = RouteParameters {
2451+
payment_params,
2452+
final_value_msat: amt_msat,
2453+
final_cltv_expiry_delta: TEST_FINAL_CLTV,
2454+
};
2455+
2456+
let mut route = Route {
2457+
paths: vec![
2458+
vec![RouteHop {
2459+
pubkey: nodes[1].node.get_our_node_id(),
2460+
node_features: nodes[1].node.node_features(),
2461+
short_channel_id: chan_1_scid,
2462+
channel_features: nodes[1].node.channel_features(),
2463+
fee_msat: 0, // nodes[1] will fail the payment as we don't pay its fee
2464+
cltv_expiry_delta: 100,
2465+
}, RouteHop {
2466+
pubkey: nodes[2].node.get_our_node_id(),
2467+
node_features: nodes[2].node.node_features(),
2468+
short_channel_id: chan_2_scid,
2469+
channel_features: nodes[2].node.channel_features(),
2470+
fee_msat: 100_000_000,
2471+
cltv_expiry_delta: 100,
2472+
}],
2473+
vec![RouteHop {
2474+
pubkey: nodes[1].node.get_our_node_id(),
2475+
node_features: nodes[1].node.node_features(),
2476+
short_channel_id: chan_1_scid,
2477+
channel_features: nodes[1].node.channel_features(),
2478+
fee_msat: 100_000,
2479+
cltv_expiry_delta: 100,
2480+
}, RouteHop {
2481+
pubkey: nodes[2].node.get_our_node_id(),
2482+
node_features: nodes[2].node.node_features(),
2483+
short_channel_id: chan_2_scid,
2484+
channel_features: nodes[2].node.channel_features(),
2485+
fee_msat: 100_000_000,
2486+
cltv_expiry_delta: 100,
2487+
}]
2488+
],
2489+
payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV)),
2490+
};
2491+
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
2492+
let mut second_payment_params = route_params.payment_params.clone();
2493+
second_payment_params.previously_failed_channels = vec![chan_2_scid];
2494+
// We'll only have one retry left at the end, so we'll hlepfully get a max_path_count of 1
2495+
second_payment_params.max_path_count = 1;
2496+
route.paths.remove(0);
2497+
nodes[0].router.expect_find_route(RouteParameters {
2498+
payment_params: second_payment_params,
2499+
final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV,
2500+
}, Ok(route.clone()));
2501+
2502+
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
2503+
let htlc_updates = SendEvent::from_node(&nodes[0]);
2504+
check_added_monitors!(nodes[0], 1);
2505+
assert_eq!(htlc_updates.msgs.len(), 1);
2506+
2507+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &htlc_updates.msgs[0]);
2508+
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &htlc_updates.commitment_msg);
2509+
check_added_monitors!(nodes[1], 1);
2510+
let (bs_first_raa, bs_first_cs) = get_revoke_commit_msgs!(nodes[1], nodes[0].node.get_our_node_id());
2511+
2512+
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_first_raa);
2513+
check_added_monitors!(nodes[0], 1);
2514+
let second_htlc_updates = SendEvent::from_node(&nodes[0]);
2515+
2516+
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_first_cs);
2517+
check_added_monitors!(nodes[0], 1);
2518+
let as_first_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
2519+
2520+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &second_htlc_updates.msgs[0]);
2521+
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &second_htlc_updates.commitment_msg);
2522+
check_added_monitors!(nodes[1], 1);
2523+
let bs_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
2524+
2525+
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_first_raa);
2526+
check_added_monitors!(nodes[1], 1);
2527+
let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
2528+
2529+
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_raa);
2530+
check_added_monitors!(nodes[0], 1);
2531+
2532+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]);
2533+
nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_fail_update.commitment_signed);
2534+
check_added_monitors!(nodes[0], 1);
2535+
let (as_second_raa, as_third_cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id());
2536+
2537+
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_second_raa);
2538+
check_added_monitors!(nodes[1], 1);
2539+
2540+
nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_third_cs);
2541+
check_added_monitors!(nodes[1], 1);
2542+
2543+
let bs_third_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id());
2544+
2545+
nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_third_raa);
2546+
check_added_monitors!(nodes[0], 1);
2547+
2548+
let mut events = nodes[0].node.get_and_clear_pending_events();
2549+
assert_eq!(events.len(), 2);
2550+
match events[0] {
2551+
Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently, .. } => {
2552+
assert_eq!(payment_hash, ev_payment_hash);
2553+
assert_eq!(payment_failed_permanently, false);
2554+
},
2555+
_ => panic!("Unexpected event"),
2556+
}
2557+
match events[1] {
2558+
Event::PendingHTLCsForwardable { .. } => {},
2559+
_ => panic!("Unexpected event"),
2560+
}
2561+
2562+
nodes[0].node.process_pending_htlc_forwards();
2563+
let retry_htlc_updates = SendEvent::from_node(&nodes[0]);
2564+
check_added_monitors!(nodes[0], 1);
2565+
2566+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &retry_htlc_updates.msgs[0]);
2567+
commitment_signed_dance!(nodes[1], nodes[0], &retry_htlc_updates.commitment_msg, false, true);
2568+
2569+
expect_pending_htlcs_forwardable!(nodes[1]);
2570+
check_added_monitors!(nodes[1], 1);
2571+
2572+
let bs_forward_update = get_htlc_update_msgs!(nodes[1], nodes[2].node.get_our_node_id());
2573+
nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[0]);
2574+
nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &bs_forward_update.update_add_htlcs[1]);
2575+
commitment_signed_dance!(nodes[2], nodes[1], &bs_forward_update.commitment_signed, false);
2576+
2577+
expect_pending_htlcs_forwardable!(nodes[2]);
2578+
expect_payment_claimable!(nodes[2], payment_hash, payment_secret, amt_msat);
2579+
}

0 commit comments

Comments
 (0)