Merge pull request #1994 from TheBlueMatt/2023-01-1916-followups

1916 Followups Part 1

Author: TheBlueMatt

fuzz/src/full_stack.rs

@@ -512,7 +512,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 			},
 			4 => {
 				let final_value_msat = slice_to_be24(get_slice!(3)) as u64;
-				let payment_params = PaymentParameters::from_node_id(get_pubkey!());
+				let payment_params = PaymentParameters::from_node_id(get_pubkey!(), 42);
 				let params = RouteParameters {
 					payment_params,
 					final_value_msat,
@@ -536,7 +536,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 			},
 			15 => {
 				let final_value_msat = slice_to_be24(get_slice!(3)) as u64;
-				let payment_params = PaymentParameters::from_node_id(get_pubkey!());
+				let payment_params = PaymentParameters::from_node_id(get_pubkey!(), 42);
 				let params = RouteParameters {
 					payment_params,
 					final_value_msat,

fuzz/src/router.rs

@@ -263,10 +263,13 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 				let scorer = FixedPenaltyScorer::with_penalty(0);
 				let random_seed_bytes: [u8; 32] = [get_slice!(1)[0]; 32];
 				for target in node_pks.iter() {
+					let final_value_msat = slice_to_be64(get_slice!(8));
+					let final_cltv_expiry_delta = slice_to_be32(get_slice!(4));
 					let route_params = RouteParameters {
-						payment_params: PaymentParameters::from_node_id(*target).with_route_hints(last_hops.clone()),
-						final_value_msat: slice_to_be64(get_slice!(8)),
-						final_cltv_expiry_delta: slice_to_be32(get_slice!(4)),
+						payment_params: PaymentParameters::from_node_id(*target, final_cltv_expiry_delta)
+							.with_route_hints(last_hops.clone()),
+						final_value_msat,
+						final_cltv_expiry_delta,
 					};
 					let _ = find_route(&our_pubkey, &route_params, &net_graph,
 						first_hops.map(|c| c.iter().collect::<Vec<_>>()).as_ref().map(|a| a.as_slice()),

lightning-invoice/src/payment.rs

@@ -422,7 +422,8 @@ where
 		};
 
 		let payment_secret = Some(invoice.payment_secret().clone());
-		let mut payment_params = PaymentParameters::from_node_id(invoice.recover_payee_pub_key())
+		let mut payment_params = PaymentParameters::from_node_id(invoice.recover_payee_pub_key(),
+				invoice.min_final_cltv_expiry_delta() as u32)
 			.with_expiry_time(expiry_time_from_unix_epoch(&invoice).as_secs())
 			.with_route_hints(invoice.route_hints());
 		if let Some(features) = invoice.features() {
@@ -486,7 +487,7 @@ where
 		};
 
 		let route_params = RouteParameters {
-			payment_params: PaymentParameters::for_keysend(pubkey),
+			payment_params: PaymentParameters::for_keysend(pubkey, final_cltv_expiry_delta),
 			final_value_msat: amount_msats,
 			final_cltv_expiry_delta,
 		};
@@ -1377,7 +1378,7 @@ mod tests {
 		assert_eq!(*payer.attempts.borrow(), 1);
 
 		let retry = RouteParameters {
-			payment_params: PaymentParameters::for_keysend(pubkey),
+			payment_params: PaymentParameters::for_keysend(pubkey, final_cltv_expiry_delta),
 			final_value_msat,
 			final_cltv_expiry_delta,
 		};
@@ -1655,7 +1656,8 @@ mod tests {
 		}
 
 		fn retry_for_invoice(invoice: &Invoice) -> RouteParameters {
-			let mut payment_params = PaymentParameters::from_node_id(invoice.recover_payee_pub_key())
+			let mut payment_params = PaymentParameters::from_node_id(
+					invoice.recover_payee_pub_key(), invoice.min_final_cltv_expiry_delta() as u32)
 				.with_expiry_time(expiry_time_from_unix_epoch(invoice).as_secs())
 				.with_route_hints(invoice.route_hints());
 			if let Some(features) = invoice.features() {
@@ -2071,7 +2073,7 @@ mod tests {
 					cltv_expiry_delta: 100,
 				}],
 			],
-			payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id())),
+			payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), 100)),
 		};
 		let router = ManualRouter(RefCell::new(VecDeque::new()));
 		router.expect_find_route(Ok(route.clone()));
@@ -2114,7 +2116,7 @@ mod tests {
 					cltv_expiry_delta: 100,
 				}],
 			],
-			payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id())),
+			payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id(), 100)),
 		};
 		let router = ManualRouter(RefCell::new(VecDeque::new()));
 		router.expect_find_route(Ok(route.clone()));
@@ -2194,7 +2196,7 @@ mod tests {
 					cltv_expiry_delta: 100,
 				}]
 			],
-			payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id())),
+			payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), 100)),
 		};
 		let router = ManualRouter(RefCell::new(VecDeque::new()));
 		router.expect_find_route(Ok(route.clone()));

lightning-invoice/src/utils.rs

@@ -725,7 +725,8 @@ mod test {
 		assert_eq!(invoice.route_hints()[0].0[0].htlc_minimum_msat, chan.inbound_htlc_minimum_msat);
 		assert_eq!(invoice.route_hints()[0].0[0].htlc_maximum_msat, chan.inbound_htlc_maximum_msat);
 
-		let payment_params = PaymentParameters::from_node_id(invoice.recover_payee_pub_key())
+		let payment_params = PaymentParameters::from_node_id(invoice.recover_payee_pub_key(),
+				invoice.min_final_cltv_expiry_delta() as u32)
 			.with_features(invoice.features().unwrap().clone())
 			.with_route_hints(invoice.route_hints());
 		let route_params = RouteParameters {
@@ -1088,7 +1089,8 @@ mod test {
 		assert_eq!(invoice.expiry_time(), Duration::from_secs(non_default_invoice_expiry_secs.into()));
 		assert!(!invoice.features().unwrap().supports_basic_mpp());
 
-		let payment_params = PaymentParameters::from_node_id(invoice.recover_payee_pub_key())
+		let payment_params = PaymentParameters::from_node_id(invoice.recover_payee_pub_key(),
+				invoice.min_final_cltv_expiry_delta() as u32)
 			.with_features(invoice.features().unwrap().clone())
 			.with_route_hints(invoice.route_hints());
 		let params = RouteParameters {

lightning/src/ln/channelmanager.rs

@@ -245,6 +245,10 @@ pub(crate) enum HTLCSource {
 		first_hop_htlc_msat: u64,
 		payment_id: PaymentId,
 		payment_secret: Option<PaymentSecret>,
+		/// Note that this is now "deprecated" - we write it for forwards (and read it for
+		/// backwards) compatibility reasons, but prefer to use the data in the
+		/// [`super::outbound_payment`] module, which stores per-payment data once instead of in
+		/// each HTLC.
 		payment_params: Option<PaymentParameters>,
 	},
 }
@@ -2473,7 +2477,7 @@ where
 		self.pending_outbound_payments
 			.send_payment(payment_hash, payment_secret, payment_id, retry_strategy, route_params,
 				&self.router, self.list_usable_channels(), self.compute_inflight_htlcs(),
-				&self.entropy_source, &self.node_signer, best_block_height,
+				&self.entropy_source, &self.node_signer, best_block_height, &self.logger,
 				|path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv|
 				self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv))
 	}
@@ -2489,7 +2493,7 @@ where
 	#[cfg(test)]
 	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
 		let best_block_height = self.best_block.read().unwrap().height();
-		self.pending_outbound_payments.test_add_new_pending_payment(payment_hash, payment_secret, payment_id, route, Retry::Attempts(0), &self.entropy_source, best_block_height)
+		self.pending_outbound_payments.test_add_new_pending_payment(payment_hash, payment_secret, payment_id, route, None, &self.entropy_source, best_block_height)
 	}
 
 
@@ -7357,9 +7361,9 @@ where
 								hash_map::Entry::Vacant(entry) => {
 									let path_fee = path.get_path_fees();
 									entry.insert(PendingOutboundPayment::Retryable {
-										retry_strategy: Retry::Attempts(0),
+										retry_strategy: None,
 										attempts: PaymentAttempts::new(),
-										route_params: None,
+										payment_params: None,
 										session_privs: [session_priv_bytes].iter().map(|a| *a).collect(),
 										payment_hash: htlc.payment_hash,
 										payment_secret,
@@ -7871,7 +7875,7 @@ mod tests {
 
 		// Next, attempt a keysend payment and make sure it fails.
 		let route_params = RouteParameters {
-			payment_params: PaymentParameters::for_keysend(expected_route.last().unwrap().node.get_our_node_id()),
+			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,
 		};
@@ -7964,7 +7968,7 @@ mod tests {
 
 		let _chan = create_chan_between_nodes(&nodes[0], &nodes[1]);
 		let route_params = RouteParameters {
-			payment_params: PaymentParameters::for_keysend(payee_pubkey),
+			payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
 			final_value_msat: 10_000,
 			final_cltv_expiry_delta: 40,
 		};
@@ -8009,7 +8013,7 @@ mod tests {
 
 		let _chan = create_chan_between_nodes(&nodes[0], &nodes[1]);
 		let route_params = RouteParameters {
-			payment_params: PaymentParameters::for_keysend(payee_pubkey),
+			payment_params: PaymentParameters::for_keysend(payee_pubkey, 40),
 			final_value_msat: 10_000,
 			final_cltv_expiry_delta: 40,
 		};
@@ -8574,7 +8578,7 @@ pub mod bench {
 		macro_rules! send_payment {
 			($node_a: expr, $node_b: expr) => {
 				let usable_channels = $node_a.list_usable_channels();
-				let payment_params = PaymentParameters::from_node_id($node_b.get_our_node_id())
+				let payment_params = PaymentParameters::from_node_id($node_b.get_our_node_id(), TEST_FINAL_CLTV)
 					.with_features($node_b.invoice_features());
 				let scorer = test_utils::TestScorer::with_penalty(0);
 				let seed = [3u8; 32];