Skip to content

Commit 1e1cb91

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 ab476a9 commit 1e1cb91

File tree

9 files changed

+105
-89
lines changed

9 files changed

+105
-89
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
@@ -6789,14 +6789,14 @@ impl Readable for HTLCSource {
67896789
let mut path: Option<Vec<RouteHop>> = Some(Vec::new());
67906790
let mut payment_id = None;
67916791
let mut payment_secret = None;
6792-
let mut payment_params = None;
6792+
let mut payment_params: Option<PaymentParameters> = None;
67936793
read_tlv_fields!(reader, {
67946794
(0, session_priv, required),
67956795
(1, payment_id, option),
67966796
(2, first_hop_htlc_msat, required),
67976797
(3, payment_secret, option),
67986798
(4, path, vec_type),
6799-
(5, payment_params, option),
6799+
(5, payment_params, (option: ReadableArgs, 0)),
68006800
});
68016801
if payment_id.is_none() {
68026802
// For backwards compat, if there was no payment_id written, use the session_priv bytes
@@ -6807,6 +6807,11 @@ impl Readable for HTLCSource {
68076807
return Err(DecodeError::InvalidValue);
68086808
}
68096809
let path = path.unwrap();
6810+
if let Some(params) = payment_params.as_mut() {
6811+
if params.final_cltv_expiry_delta == 0 {
6812+
params.final_cltv_expiry_delta = path.last().unwrap().cltv_expiry_delta;
6813+
}
6814+
}
68106815
Ok(HTLCSource::OutboundRoute {
68116816
session_priv: session_priv.0.unwrap(),
68126817
first_hop_htlc_msat,
@@ -7988,7 +7993,6 @@ mod tests {
79887993
let route_params = RouteParameters {
79897994
payment_params: PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV),
79907995
final_value_msat: 100_000,
7991-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
79927996
};
79937997
let route = find_route(
79947998
&nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph,
@@ -8079,7 +8083,6 @@ mod tests {
80798083
let route_params = RouteParameters {
80808084
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
80818085
final_value_msat: 10_000,
8082-
final_cltv_expiry_delta: 40,
80838086
};
80848087
let network_graph = nodes[0].network_graph.clone();
80858088
let first_hops = nodes[0].node.list_usable_channels();
@@ -8122,7 +8125,6 @@ mod tests {
81228125
let route_params = RouteParameters {
81238126
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
81248127
final_value_msat: 10_000,
8125-
final_cltv_expiry_delta: 40,
81268128
};
81278129
let network_graph = nodes[0].network_graph.clone();
81288130
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
@@ -9250,7 +9250,6 @@ fn test_keysend_payments_to_public_node() {
92509250
let route_params = RouteParameters {
92519251
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
92529252
final_value_msat: 10000,
9253-
final_cltv_expiry_delta: 40,
92549253
};
92559254
let scorer = test_utils::TestScorer::new();
92569255
let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes();
@@ -9281,7 +9280,6 @@ fn test_keysend_payments_to_private_node() {
92819280
let route_params = RouteParameters {
92829281
payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
92839282
final_value_msat: 10000,
9284-
final_cltv_expiry_delta: 40,
92859283
};
92869284
let network_graph = nodes[0].network_graph.clone();
92879285
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};
@@ -505,12 +505,6 @@ impl OutboundPayments {
505505
if pending_amt_msat < total_msat {
506506
retry_id_route_params = Some((*pmt_id, RouteParameters {
507507
final_value_msat: *total_msat - *pending_amt_msat,
508-
final_cltv_expiry_delta:
509-
if let Some(delta) = params.final_cltv_expiry_delta { delta }
510-
else {
511-
debug_assert!(false, "We always set the final_cltv_expiry_delta when a path fails");
512-
LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA.into()
513-
},
514508
payment_params: params.clone(),
515509
}));
516510
break
@@ -862,9 +856,6 @@ impl OutboundPayments {
862856
Some(RouteParameters {
863857
payment_params: payment_params.clone(),
864858
final_value_msat: pending_amt_unsent,
865-
final_cltv_expiry_delta:
866-
if let Some(delta) = payment_params.final_cltv_expiry_delta { delta }
867-
else { max_unsent_cltv_delta },
868859
})
869860
} else { None }
870861
} else { None },
@@ -1066,23 +1057,14 @@ impl OutboundPayments {
10661057
// `payment_params`) back to the user.
10671058
let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
10681059
if let Some(params) = payment.get_mut().payment_parameters() {
1069-
if params.final_cltv_expiry_delta.is_none() {
1070-
// This should be rare, but a user could provide None for the payment data, and
1071-
// we need it when we go to retry the payment, so fill it in.
1072-
params.final_cltv_expiry_delta = Some(path_last_hop.cltv_expiry_delta);
1073-
}
10741060
retry = Some(RouteParameters {
10751061
payment_params: params.clone(),
10761062
final_value_msat: path_last_hop.fee_msat,
1077-
final_cltv_expiry_delta: params.final_cltv_expiry_delta.unwrap(),
10781063
});
10791064
} else if let Some(params) = payment_params {
10801065
retry = Some(RouteParameters {
10811066
payment_params: params.clone(),
10821067
final_value_msat: path_last_hop.fee_msat,
1083-
final_cltv_expiry_delta:
1084-
if let Some(delta) = params.final_cltv_expiry_delta { delta }
1085-
else { path_last_hop.cltv_expiry_delta },
10861068
});
10871069
}
10881070

@@ -1219,7 +1201,9 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
12191201
(0, session_privs, required),
12201202
(1, pending_fee_msat, option),
12211203
(2, payment_hash, required),
1222-
(3, payment_params, option),
1204+
// Note that while we "default" payment_param's final CLTV expiry delta to 0 we should
1205+
// never see it - `payment_params` was added here after the field was added/required.
1206+
(3, payment_params, (option: ReadableArgs, 0)),
12231207
(4, payment_secret, option),
12241208
(5, keysend_preimage, option),
12251209
(6, total_msat, required),
@@ -1275,7 +1259,6 @@ mod tests {
12751259
let expired_route_params = RouteParameters {
12761260
payment_params,
12771261
final_value_msat: 0,
1278-
final_cltv_expiry_delta: 0,
12791262
};
12801263
let err = if on_retry {
12811264
outbound_payments.pay_internal(
@@ -1312,7 +1295,6 @@ mod tests {
13121295
let route_params = RouteParameters {
13131296
payment_params,
13141297
final_value_msat: 0,
1315-
final_cltv_expiry_delta: 0,
13161298
};
13171299
router.expect_find_route(route_params.clone(),
13181300
Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError }));

‎lightning/src/ln/payment_tests.rs‎

Lines changed: 8 additions & 18 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
@@ -1859,11 +1855,11 @@ fn auto_retry_partial_failure() {
18591855
nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route));
18601856
nodes[0].router.expect_find_route(RouteParameters {
18611857
payment_params: route_params.payment_params.clone(),
1862-
final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV
1858+
final_value_msat: amt_msat / 2,
18631859
}, Ok(retry_1_route));
18641860
nodes[0].router.expect_find_route(RouteParameters {
18651861
payment_params: route_params.payment_params.clone(),
1866-
final_value_msat: amt_msat / 4, final_cltv_expiry_delta: TEST_FINAL_CLTV
1862+
final_value_msat: amt_msat / 4,
18671863
}, Ok(retry_2_route));
18681864

18691865
// Send a payment that will partially fail on send, then partially fail on retry, then succeed.
@@ -1989,7 +1985,6 @@ fn auto_retry_zero_attempts_send_error() {
19891985
let route_params = RouteParameters {
19901986
payment_params,
19911987
final_value_msat: amt_msat,
1992-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
19931988
};
19941989

19951990
chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::PermanentFailure);
@@ -2027,7 +2022,6 @@ fn fails_paying_after_rejected_by_payee() {
20272022
let route_params = RouteParameters {
20282023
payment_params,
20292024
final_value_msat: amt_msat,
2030-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
20312025
};
20322026

20332027
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2074,7 +2068,6 @@ fn retry_multi_path_single_failed_payment() {
20742068
let route_params = RouteParameters {
20752069
payment_params: payment_params.clone(),
20762070
final_value_msat: amt_msat,
2077-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
20782071
};
20792072

20802073
let chans = nodes[0].node.list_usable_channels();
@@ -2107,7 +2100,7 @@ fn retry_multi_path_single_failed_payment() {
21072100
payment_params: route.payment_params.clone().unwrap(),
21082101
// Note that the second request here requests the amount we originally failed to send,
21092102
// not the amount remaining on the full payment, which should be changed.
2110-
final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV
2103+
final_value_msat: 100_000_001,
21112104
}, Ok(route.clone()));
21122105

21132106
{
@@ -2153,7 +2146,6 @@ fn immediate_retry_on_failure() {
21532146
let route_params = RouteParameters {
21542147
payment_params,
21552148
final_value_msat: amt_msat,
2156-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
21572149
};
21582150

21592151
let chans = nodes[0].node.list_usable_channels();
@@ -2178,7 +2170,7 @@ fn immediate_retry_on_failure() {
21782170
route.paths[1][0].fee_msat = 50_000_001;
21792171
nodes[0].router.expect_find_route(RouteParameters {
21802172
payment_params: route_params.payment_params.clone(),
2181-
final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV
2173+
final_value_msat: amt_msat,
21822174
}, Ok(route.clone()));
21832175

21842176
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2227,7 +2219,6 @@ fn no_extra_retries_on_back_to_back_fail() {
22272219
let route_params = RouteParameters {
22282220
payment_params,
22292221
final_value_msat: amt_msat,
2230-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
22312222
};
22322223

22332224
let mut route = Route {
@@ -2273,7 +2264,7 @@ fn no_extra_retries_on_back_to_back_fail() {
22732264
route.paths[0][1].fee_msat = amt_msat;
22742265
nodes[0].router.expect_find_route(RouteParameters {
22752266
payment_params: second_payment_params,
2276-
final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV,
2267+
final_value_msat: amt_msat,
22772268
}, Ok(route.clone()));
22782269

22792270
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2428,7 +2419,6 @@ fn test_simple_partial_retry() {
24282419
let route_params = RouteParameters {
24292420
payment_params,
24302421
final_value_msat: amt_msat,
2431-
final_cltv_expiry_delta: TEST_FINAL_CLTV,
24322422
};
24332423

24342424
let mut route = Route {
@@ -2473,7 +2463,7 @@ fn test_simple_partial_retry() {
24732463
route.paths.remove(0);
24742464
nodes[0].router.expect_find_route(RouteParameters {
24752465
payment_params: second_payment_params,
2476-
final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV,
2466+
final_value_msat: amt_msat / 2,
24772467
}, Ok(route.clone()));
24782468

24792469
nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();

0 commit comments

Comments
 (0)