Skip to content

Commit 28a94c1

Browse files
TheBlueMattTheBlueMatt
committed
Remove the final_cltv_expiry_delta in RouteParameters entirely
fbc0847 purported to "move" the `final_cltv_expiry_delta` field to `PaymentParamters` from `RouteParameters`. However, for naive backwards-compatibility reasons it left the existing on in place and only added a new, redundant field in `PaymentParameters`. It turns out there's really no reason for this - if we take a more critical eye towards backwards compatibility we can figure out the correct value in every `PaymentParameters` while deserializing. We do this here - making `PaymentParameters` a `ReadableArgs` taking a "default" `cltv_expiry_delta` when it goes to read. This allows existing `RouteParameters` objects to pass the read `final_cltv_expiry_delta` field in to be used if the new field wasn't present.
1 parent 222777d commit 28a94c1

File tree

9 files changed

+106
-90
lines changed

9 files changed

+106
-90
lines changed

‎fuzz/src/full_stack.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -513,7 +513,6 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
513513
let params = RouteParameters {
514514
payment_params,
515515
final_value_msat,
516-
final_cltv_expiry_delta: 42,
517516
};
518517
let random_seed_bytes: [u8; 32] = keys_manager.get_secure_random_bytes();
519518
let route = match find_route(&our_id, &params, &network_graph, None, Arc::clone(&logger), &scorer, &random_seed_bytes) {
@@ -537,7 +536,6 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
537536
let params = RouteParameters {
538537
payment_params,
539538
final_value_msat,
540-
final_cltv_expiry_delta: 42,
541539
};
542540
let random_seed_bytes: [u8; 32] = keys_manager.get_secure_random_bytes();
543541
let mut route = match find_route(&our_id, &params, &network_graph, None, Arc::clone(&logger), &scorer, &random_seed_bytes) {

‎fuzz/src/router.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,6 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
301301
payment_params: PaymentParameters::from_node_id(*target, final_cltv_expiry_delta)
302302
.with_route_hints(last_hops.clone()),
303303
final_value_msat,
304-
final_cltv_expiry_delta,
305304
};
306305
let _ = find_route(&our_pubkey, &route_params, &net_graph,
307306
first_hops.map(|c| c.iter().collect::<Vec<_>>()).as_ref().map(|a| a.as_slice()),

‎lightning-invoice/src/payment.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,6 @@ fn pay_invoice_using_amount<P: Deref>(
156156
let route_params = RouteParameters {
157157
payment_params,
158158
final_value_msat: amount_msats,
159-
final_cltv_expiry_delta: invoice.min_final_cltv_expiry_delta() as u32,
160159
};
161160

162161
payer.send_payment(payment_hash, &payment_secret, payment_id, route_params, retry_strategy)

‎lightning-invoice/src/utils.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -686,7 +686,6 @@ mod test {
686686
let route_params = RouteParameters {
687687
payment_params,
688688
final_value_msat: invoice.amount_milli_satoshis().unwrap(),
689-
final_cltv_expiry_delta: invoice.min_final_cltv_expiry_delta() as u32,
690689
};
691690
let first_hops = nodes[0].node.list_usable_channels();
692691
let network_graph = &node_cfgs[0].network_graph;
@@ -1050,7 +1049,6 @@ mod test {
10501049
let params = RouteParameters {
10511050
payment_params,
10521051
final_value_msat: invoice.amount_milli_satoshis().unwrap(),
1053-
final_cltv_expiry_delta: invoice.min_final_cltv_expiry_delta() as u32,
10541052
};
10551053
let first_hops = nodes[0].node.list_usable_channels();
10561054
let network_graph = &node_cfgs[0].network_graph;

‎lightning/src/ln/channelmanager.rs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6748,14 +6748,14 @@ impl Readable for HTLCSource {
67486748
let mut path: Option<Vec<RouteHop>> = Some(Vec::new());
67496749
let mut payment_id = None;
67506750
let mut payment_secret = None;
6751-
let mut payment_params = None;
6751+
let mut payment_params: Option<PaymentParameters> = None;
67526752
read_tlv_fields!(reader, {
67536753
(0, session_priv, required),
67546754
(1, payment_id, option),
67556755
(2, first_hop_htlc_msat, required),
67566756
(3, payment_secret, option),
67576757
(4, path, vec_type),
6758-
(5, payment_params, option),
6758+
(5, payment_params, (option: ReadableArgs, 0)),
67596759
});
67606760
if payment_id.is_none() {
67616761
// For backwards compat, if there was no payment_id written, use the session_priv bytes
@@ -6766,6 +6766,11 @@ impl Readable for HTLCSource {
67666766
return Err(DecodeError::InvalidValue);
67676767
}
67686768
let path = path.unwrap();
6769+
if let Some(params) = payment_params.as_mut() {
6770+
if params.final_cltv_expiry_delta == 0 {
6771+
params.final_cltv_expiry_delta = path.last().unwrap().cltv_expiry_delta;
6772+
}
6773+
}
67696774
Ok(HTLCSource::OutboundRoute {
67706775
session_priv: session_priv.0.unwrap(),
67716776
first_hop_htlc_msat,
@@ -7967,7 +7972,6 @@ mod tests {
79677972
let route_params = RouteParameters {
79687973
payment_params: PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV),
79697974
final_value_msat: 100_000,
7970-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
79717975
};
79727976
let route = find_route(
79737977
&nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph,
@@ -8058,7 +8062,6 @@ mod tests {
80588062
let route_params = RouteParameters {
80598063
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
80608064
final_value_msat: 10_000,
8061-
final_cltv_expiry_delta: 40,
80628065
};
80638066
let network_graph = nodes[0].network_graph.clone();
80648067
let first_hops = nodes[0].node.list_usable_channels();
@@ -8101,7 +8104,6 @@ mod tests {
81018104
let route_params = RouteParameters {
81028105
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
81038106
final_value_msat: 10_000,
8104-
final_cltv_expiry_delta: 40,
81058107
};
81068108
let network_graph = nodes[0].network_graph.clone();
81078109
let first_hops = nodes[0].node.list_usable_channels();

‎lightning/src/ln/functional_tests.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9241,7 +9241,6 @@ fn test_keysend_payments_to_public_node() {
92419241
let route_params = RouteParameters {
92429242
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
92439243
final_value_msat: 10000,
9244-
final_cltv_expiry_delta: 40,
92459244
};
92469245
let scorer = test_utils::TestScorer::new();
92479246
let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes();
@@ -9272,7 +9271,6 @@ fn test_keysend_payments_to_private_node() {
92729271
let route_params = RouteParameters {
92739272
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
92749273
final_value_msat: 10000,
9275-
final_cltv_expiry_delta: 40,
92769274
};
92779275
let network_graph = nodes[0].network_graph.clone();
92789276
let first_hops = nodes[0].node.list_usable_channels();

‎lightning/src/ln/outbound_payment.rs

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
1616
use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
1717
use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
1818
use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, PaymentId};
19-
use crate::ln::channelmanager::MIN_FINAL_CLTV_EXPIRY_DELTA as LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA;
2019
use crate::ln::onion_utils::HTLCFailReason;
2120
use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters, RoutePath, Router};
2221
use crate::util::errors::APIError;
@@ -25,6 +24,7 @@ use crate::util::logger::Logger;
2524
use crate::util::time::Time;
2625
#[cfg(all(not(feature = "no-std"), test))]
2726
use crate::util::time::tests::SinceEpoch;
27+
use crate::util::ser::ReadableArgs;
2828

2929
use core::cmp;
3030
use core::fmt::{self, Display, Formatter};
@@ -528,12 +528,6 @@ impl OutboundPayments {
528528
if pending_amt_msat < total_msat {
529529
retry_id_route_params = Some((*pmt_id, RouteParameters {
530530
final_value_msat: *total_msat - *pending_amt_msat,
531-
final_cltv_expiry_delta:
532-
if let Some(delta) = params.final_cltv_expiry_delta { delta }
533-
else {
534-
debug_assert!(false, "We always set the final_cltv_expiry_delta when a path fails");
535-
LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA.into()
536-
},
537531
payment_params: params.clone(),
538532
}));
539533
break
@@ -976,9 +970,6 @@ impl OutboundPayments {
976970
Some(RouteParameters {
977971
payment_params: payment_params.clone(),
978972
final_value_msat: pending_amt_unsent,
979-
final_cltv_expiry_delta:
980-
if let Some(delta) = payment_params.final_cltv_expiry_delta { delta }
981-
else { max_unsent_cltv_delta },
982973
})
983974
} else { None }
984975
} else { None },
@@ -1179,23 +1170,14 @@ impl OutboundPayments {
11791170
// `payment_params`) back to the user.
11801171
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
11811172
if let Some(params) = payment.get_mut().payment_parameters() {
1182-
if params.final_cltv_expiry_delta.is_none() {
1183-
// This should be rare, but a user could provide None for the payment data, and
1184-
// we need it when we go to retry the payment, so fill it in.
1185-
params.final_cltv_expiry_delta = Some(path_last_hop.cltv_expiry_delta);
1186-
}
11871173
retry = Some(RouteParameters {
11881174
payment_params: params.clone(),
11891175
final_value_msat: path_last_hop.fee_msat,
1190-
final_cltv_expiry_delta: params.final_cltv_expiry_delta.unwrap(),
11911176
});
11921177
} else if let Some(params) = payment_params {
11931178
retry = Some(RouteParameters {
11941179
payment_params: params.clone(),
11951180
final_value_msat: path_last_hop.fee_msat,
1196-
final_cltv_expiry_delta:
1197-
if let Some(delta) = params.final_cltv_expiry_delta { delta }
1198-
else { path_last_hop.cltv_expiry_delta },
11991181
});
12001182
}
12011183

@@ -1330,7 +1312,9 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
13301312
(0, session_privs, required),
13311313
(1, pending_fee_msat, option),
13321314
(2, payment_hash, required),
1333-
(3, payment_params, option),
1315+
// Note that while we "default" payment_param's final CLTV expiry delta to 0 we should
1316+
// never see it - `payment_params` was added here after the field was added/required.
1317+
(3, payment_params, (option: ReadableArgs, 0)),
13341318
(4, payment_secret, option),
13351319
(5, keysend_preimage, option),
13361320
(6, total_msat, required),
@@ -1386,7 +1370,6 @@ mod tests {
13861370
let expired_route_params = RouteParameters {
13871371
payment_params,
13881372
final_value_msat: 0,
1389-
final_cltv_expiry_delta: 0,
13901373
};
13911374
let pending_events = Mutex::new(Vec::new());
13921375
if on_retry {
@@ -1428,7 +1411,6 @@ mod tests {
14281411
let route_params = RouteParameters {
14291412
payment_params,
14301413
final_value_msat: 0,
1431-
final_cltv_expiry_delta: 0,
14321414
};
14331415
router.expect_find_route(route_params.clone(),
14341416
Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError }));

‎lightning/src/ln/payment_tests.rs

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,6 @@ fn mpp_retry() {
9898
let mut route_params = RouteParameters {
9999
payment_params: route.payment_params.clone().unwrap(),
100100
final_value_msat: amt_msat,
101-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
102101
};
103102

104103
nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
@@ -297,7 +296,6 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
297296
let route_params = RouteParameters {
298297
payment_params: route.payment_params.clone().unwrap(),
299298
final_value_msat: amt_msat,
300-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
301299
};
302300
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
303301
check_added_monitors!(nodes[0], 1);
@@ -1387,12 +1385,12 @@ fn do_test_intercepted_payment(test: InterceptTest) {
13871385
let route_params = RouteParameters {
13881386
payment_params,
13891387
final_value_msat: amt_msat,
1390-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
13911388
};
13921389
let route = get_route(
13931390
&nodes[0].node.get_our_node_id(), &route_params.payment_params,
13941391
&nodes[0].network_graph.read_only(), None, route_params.final_value_msat,
1395-
route_params.final_cltv_expiry_delta, nodes[0].logger, &scorer, &random_seed_bytes
1392+
route_params.payment_params.final_cltv_expiry_delta, nodes[0].logger, &scorer,
1393+
&random_seed_bytes,
13961394
).unwrap();
13971395

13981396
let (payment_hash, payment_secret) = nodes[2].node.create_inbound_payment(Some(amt_msat), 60 * 60, None).unwrap();
@@ -1577,7 +1575,6 @@ fn do_automatic_retries(test: AutoRetry) {
15771575
let route_params = RouteParameters {
15781576
payment_params,
15791577
final_value_msat: amt_msat,
1580-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
15811578
};
15821579
let (_, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], amt_msat);
15831580

@@ -1787,7 +1784,6 @@ fn auto_retry_partial_failure() {
17871784
let route_params = RouteParameters {
17881785
payment_params,
17891786
final_value_msat: amt_msat,
1790-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
17911787
};
17921788

17931789
// Ensure the first monitor update (for the initial send path1 over chan_1) succeeds, but the
@@ -1860,12 +1856,12 @@ fn auto_retry_partial_failure() {
18601856
let mut payment_params = route_params.payment_params.clone();
18611857
payment_params.previously_failed_channels.push(chan_2_id);
18621858
nodes[0].router.expect_find_route(RouteParameters {
1863-
payment_params, final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV
1859+
payment_params, final_value_msat: amt_msat / 2,
18641860
}, Ok(retry_1_route));
18651861
let mut payment_params = route_params.payment_params.clone();
18661862
payment_params.previously_failed_channels.push(chan_3_id);
18671863
nodes[0].router.expect_find_route(RouteParameters {
1868-
payment_params, final_value_msat: amt_msat / 4, final_cltv_expiry_delta: TEST_FINAL_CLTV
1864+
payment_params, final_value_msat: amt_msat / 4,
18691865
}, Ok(retry_2_route));
18701866

18711867
// Send a payment that will partially fail on send, then partially fail on retry, then succeed.
@@ -1999,7 +1995,6 @@ fn auto_retry_zero_attempts_send_error() {
19991995
let route_params = RouteParameters {
20001996
payment_params,
20011997
final_value_msat: amt_msat,
2002-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
20031998
};
20041999

20052000
chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::PermanentFailure);
@@ -2039,7 +2034,6 @@ fn fails_paying_after_rejected_by_payee() {
20392034
let route_params = RouteParameters {
20402035
payment_params,
20412036
final_value_msat: amt_msat,
2042-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
20432037
};
20442038

20452039
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2086,7 +2080,6 @@ fn retry_multi_path_single_failed_payment() {
20862080
let route_params = RouteParameters {
20872081
payment_params: payment_params.clone(),
20882082
final_value_msat: amt_msat,
2089-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
20902083
};
20912084

20922085
let chans = nodes[0].node.list_usable_channels();
@@ -2121,7 +2114,7 @@ fn retry_multi_path_single_failed_payment() {
21212114
payment_params: pay_params,
21222115
// Note that the second request here requests the amount we originally failed to send,
21232116
// not the amount remaining on the full payment, which should be changed.
2124-
final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV
2117+
final_value_msat: 100_000_001,
21252118
}, Ok(route.clone()));
21262119

21272120
{
@@ -2180,7 +2173,6 @@ fn immediate_retry_on_failure() {
21802173
let route_params = RouteParameters {
21812174
payment_params,
21822175
final_value_msat: amt_msat,
2183-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
21842176
};
21852177

21862178
let chans = nodes[0].node.list_usable_channels();
@@ -2207,7 +2199,6 @@ fn immediate_retry_on_failure() {
22072199
pay_params.previously_failed_channels.push(chans[0].short_channel_id.unwrap());
22082200
nodes[0].router.expect_find_route(RouteParameters {
22092201
payment_params: pay_params, final_value_msat: amt_msat,
2210-
final_cltv_expiry_delta: TEST_FINAL_CLTV
22112202
}, Ok(route.clone()));
22122203

22132204
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2270,7 +2261,6 @@ fn no_extra_retries_on_back_to_back_fail() {
22702261
let route_params = RouteParameters {
22712262
payment_params,
22722263
final_value_msat: amt_msat,
2273-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
22742264
};
22752265

22762266
let mut route = Route {
@@ -2316,7 +2306,7 @@ fn no_extra_retries_on_back_to_back_fail() {
23162306
route.paths[0][1].fee_msat = amt_msat;
23172307
nodes[0].router.expect_find_route(RouteParameters {
23182308
payment_params: second_payment_params,
2319-
final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV,
2309+
final_value_msat: amt_msat,
23202310
}, Ok(route.clone()));
23212311

23222312
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2471,7 +2461,6 @@ fn test_simple_partial_retry() {
24712461
let route_params = RouteParameters {
24722462
payment_params,
24732463
final_value_msat: amt_msat,
2474-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
24752464
};
24762465

24772466
let mut route = Route {
@@ -2516,7 +2505,7 @@ fn test_simple_partial_retry() {
25162505
route.paths.remove(0);
25172506
nodes[0].router.expect_find_route(RouteParameters {
25182507
payment_params: second_payment_params,
2519-
final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV,
2508+
final_value_msat: amt_msat / 2,
25202509
}, Ok(route.clone()));
25212510

25222511
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2637,7 +2626,6 @@ fn test_threaded_payment_retries() {
26372626
let mut route_params = RouteParameters {
26382627
payment_params,
26392628
final_value_msat: amt_msat,
2640-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
26412629
};
26422630

26432631
let mut route = Route {

0 commit comments

Comments
 (0)