Skip to content

Commit 85077fe

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 b5d60df commit 85077fe

File tree

3 files changed

+214
-20
lines changed

3 files changed

+214
-20
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: 54 additions & 19 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 {
@@ -474,9 +482,18 @@ impl OutboundPayments {
474482
let mut retry_id_route_params = None;
475483
for (pmt_id, pmt) in outbounds.iter_mut() {
476484
if pmt.is_auto_retryable_now() {
477-
if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, route_params: Some(params), .. } = pmt {
485+
if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, payment_params: Some(params), .. } = pmt {
478486
if pending_amt_msat < total_msat {
479-
retry_id_route_params = Some((*pmt_id, params.clone()));
487+
retry_id_route_params = Some((*pmt_id, RouteParameters {
488+
final_value_msat: *total_msat - *pending_amt_msat,
489+
final_cltv_expiry_delta:
490+
if let Some(delta) = params.final_cltv_expiry_delta { delta }
491+
else {
492+
debug_assert!(false, "We always set the final_cltv_expiry_delta when a path fails");
493+
super::channelmanager::MIN_FINAL_CLTV_EXPIRY_DELTA.into()
494+
},
495+
payment_params: params.clone(),
496+
}));
480497
break
481498
}
482499
}
@@ -687,7 +704,7 @@ impl OutboundPayments {
687704
let payment = entry.insert(PendingOutboundPayment::Retryable {
688705
retry_strategy,
689706
attempts: PaymentAttempts::new(),
690-
route_params,
707+
payment_params: route_params.map(|p| p.payment_params),
691708
session_privs: HashSet::new(),
692709
pending_amt_msat: 0,
693710
pending_fee_msat: Some(0),
@@ -965,6 +982,7 @@ impl OutboundPayments {
965982
let mut all_paths_failed = false;
966983
let mut full_failure_ev = None;
967984
let mut pending_retry_ev = None;
985+
let mut retry = None;
968986
let attempts_remaining = if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
969987
if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
970988
log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
@@ -978,6 +996,33 @@ impl OutboundPayments {
978996
if let Some(scid) = short_channel_id {
979997
payment.get_mut().insert_previously_failed_scid(scid);
980998
}
999+
1000+
// We want to move towards only using the `PaymentParameters` in the outbound payments
1001+
// map. However, for backwards-compatibility, we still need to support passing the
1002+
// `PaymentParameters` data that was shoved in the HTLC (and given to us via
1003+
// `payment_params`) back to the user.
1004+
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
1005+
if let Some(params) = payment.get_mut().payment_parameters() {
1006+
if params.final_cltv_expiry_delta.is_none() {
1007+
// This should be rare, but a user could provide None for the payment data, and
1008+
// we need it when we go to retry the payment, so fill it in.
1009+
params.final_cltv_expiry_delta = Some(path_last_hop.cltv_expiry_delta);
1010+
}
1011+
retry = Some(RouteParameters {
1012+
payment_params: params.clone(),
1013+
final_value_msat: path_last_hop.fee_msat,
1014+
final_cltv_expiry_delta: params.final_cltv_expiry_delta.unwrap(),
1015+
});
1016+
} else if let Some(payment_params_data) = payment_params {
1017+
retry = Some(RouteParameters {
1018+
payment_params: payment_params_data.clone(),
1019+
final_value_msat: path_last_hop.fee_msat,
1020+
final_cltv_expiry_delta:
1021+
if let Some(delta) = payment_params_data.final_cltv_expiry_delta { delta }
1022+
else { path_last_hop.cltv_expiry_delta },
1023+
});
1024+
}
1025+
9811026
if payment.get().remaining_parts() == 0 {
9821027
all_paths_failed = true;
9831028
if payment.get().abandoned() {
@@ -994,16 +1039,6 @@ impl OutboundPayments {
9941039
return
9951040
};
9961041
core::mem::drop(outbounds);
997-
let mut retry = if let Some(payment_params_data) = payment_params {
998-
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
999-
Some(RouteParameters {
1000-
payment_params: payment_params_data.clone(),
1001-
final_value_msat: path_last_hop.fee_msat,
1002-
final_cltv_expiry_delta:
1003-
if let Some(delta) = payment_params_data.final_cltv_expiry_delta { delta }
1004-
else { path_last_hop.cltv_expiry_delta },
1005-
})
1006-
} else { None };
10071042
log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
10081043

10091044
let path_failure = {
@@ -1115,13 +1150,13 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
11151150
(0, session_privs, required),
11161151
(1, pending_fee_msat, option),
11171152
(2, payment_hash, required),
1118-
(not_written, retry_strategy, (static_value, None)),
1153+
(3, payment_params, option),
11191154
(4, payment_secret, option),
1120-
(not_written, attempts, (static_value, PaymentAttempts::new())),
11211155
(6, total_msat, required),
1122-
(not_written, route_params, (static_value, None)),
11231156
(8, pending_amt_msat, required),
11241157
(10, starting_block_height, required),
1158+
(not_written, retry_strategy, (static_value, None)),
1159+
(not_written, attempts, (static_value, PaymentAttempts::new())),
11251160
},
11261161
(3, Abandoned) => {
11271162
(0, session_privs, required),

‎lightning/src/ln/payment_tests.rs

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

0 commit comments

Comments
 (0)