Remove the final_cltv_expiry_delta in RouteParameters entirely

TheBlueMatt · TheBlueMatt · commit f106a826263a · 2023-02-16T00:56:16.000Z
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.
diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs
@@ -156,7 +156,6 @@ fn pay_invoice_using_amount<P: Deref>(
 	let route_params = RouteParameters {
 		payment_params,
 		final_value_msat: amount_msats,
-		final_cltv_expiry_delta: invoice.min_final_cltv_expiry_delta() as u32,
 	};
 
 	payer.send_payment(payment_hash, &payment_secret, payment_id, route_params, retry_strategy)
diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs
@@ -686,7 +686,6 @@ mod test {
 		let route_params = RouteParameters {
 			payment_params,
 			final_value_msat: invoice.amount_milli_satoshis().unwrap(),
-			final_cltv_expiry_delta: invoice.min_final_cltv_expiry_delta() as u32,
 		};
 		let first_hops = nodes[0].node.list_usable_channels();
 		let network_graph = &node_cfgs[0].network_graph;
@@ -1050,7 +1049,6 @@ mod test {
 		let params = RouteParameters {
 			payment_params,
 			final_value_msat: invoice.amount_milli_satoshis().unwrap(),
-			final_cltv_expiry_delta: invoice.min_final_cltv_expiry_delta() as u32,
 		};
 		let first_hops = nodes[0].node.list_usable_channels();
 		let network_graph = &node_cfgs[0].network_graph;
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -6793,14 +6793,14 @@ impl Readable for HTLCSource {
 				let mut path: Option<Vec<RouteHop>> = Some(Vec::new());
 				let mut payment_id = None;
 				let mut payment_secret = None;
-				let mut payment_params = None;
+				let mut payment_params: Option<PaymentParameters> = None;
 				read_tlv_fields!(reader, {
 					(0, session_priv, required),
 					(1, payment_id, option),
 					(2, first_hop_htlc_msat, required),
 					(3, payment_secret, option),
 					(4, path, vec_type),
-					(5, payment_params, option),
+					(5, payment_params, (option: ReadableArgs, 0)),
 				});
 				if payment_id.is_none() {
 					// For backwards compat, if there was no payment_id written, use the session_priv bytes
@@ -6811,6 +6811,11 @@ impl Readable for HTLCSource {
 					return Err(DecodeError::InvalidValue);
 				}
 				let path = path.unwrap();
+				if let Some(params) = payment_params.as_mut() {
+					if params.final_cltv_expiry_delta == 0 {
+						params.final_cltv_expiry_delta = path.last().unwrap().cltv_expiry_delta;
+					}
+				}
 				Ok(HTLCSource::OutboundRoute {
 					session_priv: session_priv.0.unwrap(),
 					first_hop_htlc_msat,
@@ -7991,7 +7996,6 @@ mod tests {
 		let route_params = RouteParameters {
 			payment_params: PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id(), TEST_FINAL_CLTV),
 			final_value_msat: 100_000,
-			final_cltv_expiry_delta: TEST_FINAL_CLTV,
 		};
 		let route = find_route(
 			&nodes[0].node.get_our_node_id(), &route_params, &nodes[0].network_graph,
@@ -8082,7 +8086,6 @@ mod tests {
 		let route_params = RouteParameters {
 			payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
 			final_value_msat: 10_000,
-			final_cltv_expiry_delta: 40,
 		};
 		let network_graph = nodes[0].network_graph.clone();
 		let first_hops = nodes[0].node.list_usable_channels();
@@ -8125,7 +8128,6 @@ mod tests {
 		let route_params = RouteParameters {
 			payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
 			final_value_msat: 10_000,
-			final_cltv_expiry_delta: 40,
 		};
 		let network_graph = nodes[0].network_graph.clone();
 		let first_hops = nodes[0].node.list_usable_channels();
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
@@ -9250,7 +9250,6 @@ fn test_keysend_payments_to_public_node() {
 	let route_params = RouteParameters {
 		payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
 		final_value_msat: 10000,
-		final_cltv_expiry_delta: 40,
 	};
 	let scorer = test_utils::TestScorer::new();
 	let random_seed_bytes = chanmon_cfgs[1].keys_manager.get_secure_random_bytes();
@@ -9281,7 +9280,6 @@ fn test_keysend_payments_to_private_node() {
 	let route_params = RouteParameters {
 		payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
 		final_value_msat: 10000,
-		final_cltv_expiry_delta: 40,
 	};
 	let network_graph = nodes[0].network_graph.clone();
 	let first_hops = nodes[0].node.list_usable_channels();
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
@@ -16,7 +16,6 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
 use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient};
 use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, MIN_HTLC_RELAY_HOLDING_CELL_MILLIS, PaymentId};
-use crate::ln::channelmanager::MIN_FINAL_CLTV_EXPIRY_DELTA as LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA;
 use crate::ln::msgs::DecodeError;
 use crate::ln::onion_utils::HTLCFailReason;
 use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RouteParameters, RoutePath, Router};
@@ -26,6 +25,7 @@ use crate::util::logger::Logger;
 use crate::util::time::Time;
 #[cfg(all(not(feature = "no-std"), test))]
 use crate::util::time::tests::SinceEpoch;
+use crate::util::ser::ReadableArgs;
 
 use core::cmp;
 use core::fmt::{self, Display, Formatter};
@@ -503,12 +503,6 @@ impl OutboundPayments {
 						if pending_amt_msat < total_msat {
 							retry_id_route_params = Some((*pmt_id, RouteParameters {
 								final_value_msat: *total_msat - *pending_amt_msat,
-								final_cltv_expiry_delta:
-									if let Some(delta) = params.final_cltv_expiry_delta { delta }
-									else {
-										debug_assert!(false, "We always set the final_cltv_expiry_delta when a path fails");
-										LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA.into()
-									},
 								payment_params: params.clone(),
 							}));
 							break
@@ -854,9 +848,6 @@ impl OutboundPayments {
 						Some(RouteParameters {
 							payment_params: payment_params.clone(),
 							final_value_msat: pending_amt_unsent,
-							final_cltv_expiry_delta:
-								if let Some(delta) = payment_params.final_cltv_expiry_delta { delta }
-								else { max_unsent_cltv_delta },
 						})
 					} else { None }
 				} else { None },
@@ -1042,23 +1033,14 @@ impl OutboundPayments {
 			// `payment_params`) back to the user.
 			let path_last_hop = path.last().expect("Outbound payments must have had a valid path");
 			if let Some(params) = payment.get_mut().payment_parameters() {
-				if params.final_cltv_expiry_delta.is_none() {
-					// This should be rare, but a user could provide None for the payment data, and
-					// we need it when we go to retry the payment, so fill it in.
-					params.final_cltv_expiry_delta = Some(path_last_hop.cltv_expiry_delta);
-				}
 				retry = Some(RouteParameters {
 					payment_params: params.clone(),
 					final_value_msat: path_last_hop.fee_msat,
-					final_cltv_expiry_delta: params.final_cltv_expiry_delta.unwrap(),
 				});
 			} else if let Some(params) = payment_params {
 				retry = Some(RouteParameters {
 					payment_params: params.clone(),
 					final_value_msat: path_last_hop.fee_msat,
-					final_cltv_expiry_delta:
-						if let Some(delta) = params.final_cltv_expiry_delta { delta }
-						else { path_last_hop.cltv_expiry_delta },
 				});
 			}
 
@@ -1195,7 +1177,9 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
 		(0, session_privs, required),
 		(1, pending_fee_msat, option),
 		(2, payment_hash, required),
-		(3, payment_params, option),
+		// Note that while we "default" payment_param's final CLTV expiry delta to 0 we should
+		// never see it - `payment_params` was added here after the field was added/required.
+		(3, payment_params, (option: ReadableArgs, 0)),
 		(4, payment_secret, option),
 		(5, keysend_preimage, option),
 		(6, total_msat, required),
@@ -1251,7 +1235,6 @@ mod tests {
 		let expired_route_params = RouteParameters {
 			payment_params,
 			final_value_msat: 0,
-			final_cltv_expiry_delta: 0,
 		};
 		let err = if on_retry {
 			outbound_payments.pay_internal(
@@ -1288,7 +1271,6 @@ mod tests {
 		let route_params = RouteParameters {
 			payment_params,
 			final_value_msat: 0,
-			final_cltv_expiry_delta: 0,
 		};
 		router.expect_find_route(route_params.clone(),
 			Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError }));
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
@@ -1384,12 +1384,12 @@ fn do_test_intercepted_payment(test: InterceptTest) {
 	let route_params = RouteParameters {
 		payment_params,
 		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 	let route = get_route(
 		&nodes[0].node.get_our_node_id(), &route_params.payment_params,
 		&nodes[0].network_graph.read_only(), None, route_params.final_value_msat,
-		route_params.final_cltv_expiry_delta, nodes[0].logger, &scorer, &random_seed_bytes
+		route_params.payment_params.final_cltv_expiry_delta, nodes[0].logger, &scorer,
+		&random_seed_bytes,
 	).unwrap();
 
 	let (payment_hash, payment_secret) = nodes[2].node.create_inbound_payment(Some(amt_msat), 60 * 60, None).unwrap();
@@ -1574,7 +1574,6 @@ fn do_automatic_retries(test: AutoRetry) {
 	let route_params = RouteParameters {
 		payment_params,
 		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 	let (_, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], amt_msat);
 
@@ -1783,7 +1782,6 @@ fn auto_retry_partial_failure() {
 	let route_params = RouteParameters {
 		payment_params,
 		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 
 	// Ensure the first monitor update (for the initial send path1 over chan_1) succeeds, but the
@@ -1855,11 +1853,11 @@ fn auto_retry_partial_failure() {
 	nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route));
 	nodes[0].router.expect_find_route(RouteParameters {
 			payment_params: route_params.payment_params.clone(),
-			final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV
+			final_value_msat: amt_msat / 2,
 		}, Ok(retry_1_route));
 	nodes[0].router.expect_find_route(RouteParameters {
 			payment_params: route_params.payment_params.clone(),
-			final_value_msat: amt_msat / 4, final_cltv_expiry_delta: TEST_FINAL_CLTV
+			final_value_msat: amt_msat / 4,
 		}, Ok(retry_2_route));
 
 	// Send a payment that will partially fail on send, then partially fail on retry, then succeed.
@@ -1985,7 +1983,6 @@ fn auto_retry_zero_attempts_send_error() {
 	let route_params = RouteParameters {
 		payment_params,
 		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 
 	chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::PermanentFailure);
@@ -2023,7 +2020,6 @@ fn fails_paying_after_rejected_by_payee() {
 	let route_params = RouteParameters {
 		payment_params,
 		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 
 	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2070,7 +2066,6 @@ fn retry_multi_path_single_failed_payment() {
 	let route_params = RouteParameters {
 		payment_params: payment_params.clone(),
 		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 
 	let chans = nodes[0].node.list_usable_channels();
@@ -2103,7 +2098,7 @@ fn retry_multi_path_single_failed_payment() {
 			payment_params: route.payment_params.clone().unwrap(),
 			// Note that the second request here requests the amount we originally failed to send,
 			// not the amount remaining on the full payment, which should be changed.
-			final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV
+			final_value_msat: 100_000_001,
 		}, Ok(route.clone()));
 
 	{
@@ -2149,7 +2144,6 @@ fn immediate_retry_on_failure() {
 	let route_params = RouteParameters {
 		payment_params,
 		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 
 	let chans = nodes[0].node.list_usable_channels();
@@ -2174,7 +2168,7 @@ fn immediate_retry_on_failure() {
 	route.paths[1][0].fee_msat = 50_000_001;
 	nodes[0].router.expect_find_route(RouteParameters {
 			payment_params: route_params.payment_params.clone(),
-			final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV
+			final_value_msat: amt_msat,
 		}, Ok(route.clone()));
 
 	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2223,7 +2217,6 @@ fn no_extra_retries_on_back_to_back_fail() {
 	let route_params = RouteParameters {
 		payment_params,
 		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 
 	let mut route = Route {
@@ -2269,7 +2262,7 @@ fn no_extra_retries_on_back_to_back_fail() {
 	route.paths[0][1].fee_msat = amt_msat;
 	nodes[0].router.expect_find_route(RouteParameters {
 			payment_params: second_payment_params,
-			final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV,
+			final_value_msat: amt_msat,
 		}, Ok(route.clone()));
 
 	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
@@ -2428,7 +2421,6 @@ fn test_simple_partial_retry() {
 	let route_params = RouteParameters {
 		payment_params,
 		final_value_msat: amt_msat,
-		final_cltv_expiry_delta: TEST_FINAL_CLTV,
 	};
 
 	let mut route = Route {
@@ -2473,7 +2465,7 @@ fn test_simple_partial_retry() {
 	route.paths.remove(0);
 	nodes[0].router.expect_find_route(RouteParameters {
 			payment_params: second_payment_params,
-			final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV,
+			final_value_msat: amt_msat / 2,
 		}, Ok(route.clone()));
 
 	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
