Skip to content

Commit 102e088

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 82ecda7 commit 102e088

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
@@ -302,7 +302,6 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
302302
payment_params: PaymentParameters::from_node_id(*target, final_cltv_expiry_delta)
303303
.with_route_hints(last_hops.clone()),
304304
final_value_msat,
305-
final_cltv_expiry_delta,
306305
};
307306
let _ = find_route(&our_pubkey, &route_params, &net_graph,
308307
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
@@ -9248,7 +9248,6 @@ fn test_keysend_payments_to_public_node() {
92489248
let route_params = RouteParameters {
92499249
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
92509250
final_value_msat: 10000,
9251-
final_cltv_expiry_delta: 40,
92529251
};
92539252
let scorer = test_utils::TestScorer::new();
92549253
let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes();
@@ -9279,7 +9278,6 @@ fn test_keysend_payments_to_private_node() {
92799278
let route_params = RouteParameters {
92809279
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
92819280
final_value_msat: 10000,
9282-
final_cltv_expiry_delta: 40,
92839281
};
92849282
let network_graph = nodes[0].network_graph.clone();
92859283
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::msgs::DecodeError;
2120
use crate::ln::onion_utils::HTLCFailReason;
2221
use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters, RoutePath, Router};
@@ -26,6 +25,7 @@ use crate::util::logger::Logger;
2625
use crate::util::time::Time;
2726
#[cfg(all(not(feature = "no-std"), test))]
2827
use crate::util::time::tests::SinceEpoch;
28+
use crate::util::ser::ReadableArgs;
2929

3030
use core::cmp;
3131
use core::fmt::{self, Display, Formatter};
@@ -529,12 +529,6 @@ impl OutboundPayments {
529529
if pending_amt_msat < total_msat {
530530
retry_id_route_params = Some((*pmt_id, RouteParameters {
531531
final_value_msat: *total_msat - *pending_amt_msat,
532-
final_cltv_expiry_delta:
533-
if let Some(delta) = params.final_cltv_expiry_delta { delta }
534-
else {
535-
debug_assert!(false, "We always set the final_cltv_expiry_delta when a path fails");
536-
LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA.into()
537-
},
538532
payment_params: params.clone(),
539533
}));
540534
break
@@ -978,9 +972,6 @@ impl OutboundPayments {
978972
Some(RouteParameters {
979973
payment_params: payment_params.clone(),
980974
final_value_msat: pending_amt_unsent,
981-
final_cltv_expiry_delta:
982-
if let Some(delta) = payment_params.final_cltv_expiry_delta { delta }
983-
else { max_unsent_cltv_delta },
984975
})
985976
} else { None }
986977
} else { None },
@@ -1182,23 +1173,14 @@ impl OutboundPayments {
11821173
// `payment_params`) back to the user.
11831174
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
11841175
if let Some(params) = payment.get_mut().payment_parameters() {
1185-
if params.final_cltv_expiry_delta.is_none() {
1186-
// This should be rare, but a user could provide None for the payment data, and
1187-
// we need it when we go to retry the payment, so fill it in.
1188-
params.final_cltv_expiry_delta = Some(path_last_hop.cltv_expiry_delta);
1189-
}
11901176
retry = Some(RouteParameters {
11911177
payment_params: params.clone(),
11921178
final_value_msat: path_last_hop.fee_msat,
1193-
final_cltv_expiry_delta: params.final_cltv_expiry_delta.unwrap(),
11941179
});
11951180
} else if let Some(params) = payment_params {
11961181
retry = Some(RouteParameters {
11971182
payment_params: params.clone(),
11981183
final_value_msat: path_last_hop.fee_msat,
1199-
final_cltv_expiry_delta:
1200-
if let Some(delta) = params.final_cltv_expiry_delta { delta }
1201-
else { path_last_hop.cltv_expiry_delta },
12021184
});
12031185
}
12041186

@@ -1335,7 +1317,9 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
13351317
(0, session_privs, required),
13361318
(1, pending_fee_msat, option),
13371319
(2, payment_hash, required),
1338-
(3, payment_params, option),
1320+
// Note that while we "default" payment_param's final CLTV expiry delta to 0 we should
1321+
// never see it - `payment_params` was added here after the field was added/required.
1322+
(3, payment_params, (option: ReadableArgs, 0)),
13391323
(4, payment_secret, option),
13401324
(5, keysend_preimage, option),
13411325
(6, total_msat, required),
@@ -1391,7 +1375,6 @@ mod tests {
13911375
let expired_route_params = RouteParameters {
13921376
payment_params,
13931377
final_value_msat: 0,
1394-
final_cltv_expiry_delta: 0,
13951378
};
13961379
let pending_events = Mutex::new(Vec::new());
13971380
if on_retry {
@@ -1434,7 +1417,6 @@ mod tests {
14341417
let route_params = RouteParameters {
14351418
payment_params,
14361419
final_value_msat: 0,
1437-
final_cltv_expiry_delta: 0,
14381420
};
14391421
router.expect_find_route(route_params.clone(),
14401422
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
{
@@ -2177,7 +2170,6 @@ fn immediate_retry_on_failure() {
21772170
let route_params = RouteParameters {
21782171
payment_params,
21792172
final_value_msat: amt_msat,
2180-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
21812173
};
21822174

21832175
let chans = nodes[0].node.list_usable_channels();
@@ -2204,7 +2196,6 @@ fn immediate_retry_on_failure() {
22042196
pay_params.previously_failed_channels.push(chans[0].short_channel_id.unwrap());
22052197
nodes[0].router.expect_find_route(RouteParameters {
22062198
payment_params: pay_params, final_value_msat: amt_msat,
2207-
final_cltv_expiry_delta: TEST_FINAL_CLTV
22082199
}, Ok(route.clone()));
22092200

22102201
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2263,7 +2254,6 @@ fn no_extra_retries_on_back_to_back_fail() {
22632254
let route_params = RouteParameters {
22642255
payment_params,
22652256
final_value_msat: amt_msat,
2266-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
22672257
};
22682258

22692259
let mut route = Route {
@@ -2309,7 +2299,7 @@ fn no_extra_retries_on_back_to_back_fail() {
23092299
route.paths[0][1].fee_msat = amt_msat;
23102300
nodes[0].router.expect_find_route(RouteParameters {
23112301
payment_params: second_payment_params,
2312-
final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV,
2302+
final_value_msat: amt_msat,
23132303
}, Ok(route.clone()));
23142304

23152305
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2464,7 +2454,6 @@ fn test_simple_partial_retry() {
24642454
let route_params = RouteParameters {
24652455
payment_params,
24662456
final_value_msat: amt_msat,
2467-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
24682457
};
24692458

24702459
let mut route = Route {
@@ -2509,7 +2498,7 @@ fn test_simple_partial_retry() {
25092498
route.paths.remove(0);
25102499
nodes[0].router.expect_find_route(RouteParameters {
25112500
payment_params: second_payment_params,
2512-
final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV,
2501+
final_value_msat: amt_msat / 2,
25132502
}, Ok(route.clone()));
25142503

25152504
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2630,7 +2619,6 @@ fn test_threaded_payment_retries() {
26302619
let mut route_params = RouteParameters {
26312620
payment_params,
26322621
final_value_msat: amt_msat,
2633-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
26342622
};
26352623

26362624
let mut route = Route {

0 commit comments

Comments
 (0)