Remove PaymentPathFailed::retry

We now support automatic retries in ChannelManager and no longer support manual
retries, so the field is useless.

Author: valentinewallace

lightning-background-processor/src/lib.rs

@@ -1367,7 +1367,6 @@ mod tests {
 			failure: PathFailure::OnPath { network_update: None },
 			path: path.clone(),
 			short_channel_id: Some(scored_scid),
-			retry: None,
 		});
 		let event = receiver
 			.recv_timeout(Duration::from_secs(EVENT_DEADLINE))
@@ -1387,7 +1386,6 @@ mod tests {
 			failure: PathFailure::OnPath { network_update: None },
 			path: path.clone(),
 			short_channel_id: None,
-			retry: None,
 		});
 		let event = receiver
 			.recv_timeout(Duration::from_secs(EVENT_DEADLINE))

lightning/src/ln/channelmanager.rs

@@ -3782,9 +3782,9 @@ where
 		// from block_connected which may run during initialization prior to the chain_monitor
 		// being fully configured. See the docs for `ChannelManagerReadArgs` for more.
 		match source {
-			HTLCSource::OutboundRoute { ref path, ref session_priv, ref payment_id, ref payment_params, .. } => {
+			HTLCSource::OutboundRoute { ref path, ref session_priv, ref payment_id, .. } => {
 				if self.pending_outbound_payments.fail_htlc(source, payment_hash, onion_error, path,
-					session_priv, payment_id, payment_params, self.probing_cookie_secret, &self.secp_ctx,
+					session_priv, payment_id, self.probing_cookie_secret, &self.secp_ctx,
 					&self.pending_events, &self.logger)
 				{ self.push_pending_forwards_ev(); }
 			},

lightning/src/ln/functional_test_utils.rs

@@ -1818,20 +1818,13 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>(
 ) {
 	if conditions.expected_mpp_parts_remain { assert_eq!(payment_failed_events.len(), 1); } else { assert_eq!(payment_failed_events.len(), 2); }
 	let expected_payment_id = match &payment_failed_events[0] {
-		Event::PaymentPathFailed { payment_hash, payment_failed_permanently, path, retry, payment_id, failure, short_channel_id,
+		Event::PaymentPathFailed { payment_hash, payment_failed_permanently, payment_id, failure,
 			#[cfg(test)]
 			error_code,
 			#[cfg(test)]
 			error_data, .. } => {
 			assert_eq!(*payment_hash, expected_payment_hash, "unexpected payment_hash");
 			assert_eq!(*payment_failed_permanently, expected_payment_failed_permanently, "unexpected payment_failed_permanently value");
-			assert!(retry.is_some(), "expected retry.is_some()");
-			assert_eq!(retry.as_ref().unwrap().final_value_msat, path.last().unwrap().fee_msat, "Retry amount should match last hop in path");
-			assert_eq!(retry.as_ref().unwrap().payment_params.payee_pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path");
-			if let Some(scid) = short_channel_id {
-				assert!(retry.as_ref().unwrap().payment_params.previously_failed_channels.contains(&scid));
-			}
-
 			#[cfg(test)]
 			{
 				assert!(error_code.is_some(), "expected error_code.is_some() = true");

lightning/src/ln/outbound_payment.rs

@@ -97,14 +97,6 @@ impl PendingOutboundPayment {
 			_ => false,
 		}
 	}
-	fn payment_parameters(&mut self) -> Option<&mut PaymentParameters> {
-		match self {
-			PendingOutboundPayment::Retryable { payment_params: Some(ref mut params), .. } => {
-				Some(params)
-			},
-			_ => None,
-		}
-	}
 	pub fn insert_previously_failed_scid(&mut self, scid: u64) {
 		if let PendingOutboundPayment::Retryable { payment_params: Some(params), .. } = self {
 			params.previously_failed_channels.push(scid);
@@ -790,7 +782,6 @@ impl OutboundPayments {
 					failure: events::PathFailure::InitialSend { err: e },
 					path,
 					short_channel_id: failed_scid,
-					retry: None,
 					#[cfg(test)]
 					error_code: None,
 					#[cfg(test)]
@@ -1120,8 +1111,8 @@ impl OutboundPayments {
 	pub(super) fn fail_htlc<L: Deref>(
 		&self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason,
 		path: &Vec<RouteHop>, session_priv: &SecretKey, payment_id: &PaymentId,
-		payment_params: &Option<PaymentParameters>, probing_cookie_secret: [u8; 32],
-		secp_ctx: &Secp256k1<secp256k1::All>, pending_events: &Mutex<Vec<events::Event>>, logger: &L
+		probing_cookie_secret: [u8; 32], secp_ctx: &Secp256k1<secp256k1::All>,
+		pending_events: &Mutex<Vec<events::Event>>, logger: &L
 	) -> bool where L::Target: Logger {
 		#[cfg(test)]
 		let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_error.decode_onion_failure(secp_ctx, logger, &source);
@@ -1149,7 +1140,6 @@ impl OutboundPayments {
 
 		let mut full_failure_ev = None;
 		let mut pending_retry_ev = false;
-		let mut retry = None;
 		let attempts_remaining = if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
 			if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
 				log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
@@ -1161,27 +1151,13 @@ impl OutboundPayments {
 			}
 			let mut is_retryable_now = payment.get().is_auto_retryable_now();
 			if let Some(scid) = short_channel_id {
+				// TODO: If we decided to blame ourselves (or one of our channels) in
+				// process_onion_failure we should close that channel as it implies our
+				// next-hop is needlessly blaming us!
 				payment.get_mut().insert_previously_failed_scid(scid);
 			}
 
-			// We want to move towards only using the `PaymentParameters` in the outbound payments
-			// map. However, for backwards-compatibility, we still need to support passing the
-			// `PaymentParameters` data that was shoved in the HTLC (and given to us via
-			// `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() {
-				retry = Some(RouteParameters {
-					payment_params: params.clone(),
-					final_value_msat: path_last_hop.fee_msat,
-				});
-			} else if let Some(params) = payment_params {
-				retry = Some(RouteParameters {
-					payment_params: params.clone(),
-					final_value_msat: path_last_hop.fee_msat,
-				});
-			}
-
-			if payment_is_probe || !is_retryable_now || !payment_retryable || retry.is_none() {
+			if payment_is_probe || !is_retryable_now || !payment_retryable {
 				let _ = payment.get_mut().mark_abandoned(); // we'll only Err if it's a legacy payment
 				is_retryable_now = false;
 			}
@@ -1221,12 +1197,6 @@ impl OutboundPayments {
 					}
 				}
 			} else {
-				// TODO: If we decided to blame ourselves (or one of our channels) in
-				// process_onion_failure we should close that channel as it implies our
-				// next-hop is needlessly blaming us!
-				if let Some(scid) = short_channel_id {
-					retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
-				}
 				// If we miss abandoning the payment above, we *must* generate an event here or else the
 				// payment will sit in our outbounds forever.
 				if attempts_remaining && !already_awaiting_retry {
@@ -1240,7 +1210,6 @@ impl OutboundPayments {
 					failure: events::PathFailure::OnPath { network_update },
 					path: path.clone(),
 					short_channel_id,
-					retry,
 					#[cfg(test)]
 					error_code: onion_error_code,
 					#[cfg(test)]

lightning/src/util/events.rs

@@ -701,10 +701,6 @@ pub enum Event {
 		/// If this is `Some`, then the corresponding channel should be avoided when the payment is
 		/// retried. May be `None` for older [`Event`] serializations.
 		short_channel_id: Option<u64>,
-		/// Parameters used by LDK to compute a new [`Route`] when retrying the failed payment path.
-		///
-		/// [`Route`]: crate::routing::router::Route
-		retry: Option<RouteParameters>,
 #[cfg(test)]
 		error_code: Option<u16>,
 #[cfg(test)]
@@ -992,7 +988,7 @@ impl Writeable for Event {
 			},
 			&Event::PaymentPathFailed {
 				ref payment_id, ref payment_hash, ref payment_failed_permanently, ref failure,
-				ref path, ref short_channel_id, ref retry,
+				ref path, ref short_channel_id,
 				#[cfg(test)]
 				ref error_code,
 				#[cfg(test)]
@@ -1010,7 +1006,7 @@ impl Writeable for Event {
 					(3, false, required), // all_paths_failed in LDK versions prior to 0.0.114
 					(5, *path, vec_type),
 					(7, short_channel_id, option),
-					(9, retry, option),
+					(9, None::<RouteParameters>, option), // retry in LDK versions prior to 0.0.115
 					(11, payment_id, option),
 					(13, failure, required),
 				});
@@ -1227,7 +1223,6 @@ impl MaybeReadable for Event {
 					let mut network_update = None;
 					let mut path: Option<Vec<RouteHop>> = Some(vec![]);
 					let mut short_channel_id = None;
-					let mut retry = None;
 					let mut payment_id = None;
 					let mut failure_opt = None;
 					read_tlv_fields!(reader, {
@@ -1236,7 +1231,6 @@ impl MaybeReadable for Event {
 						(2, payment_failed_permanently, required),
 						(5, path, vec_type),
 						(7, short_channel_id, option),
-						(9, retry, option),
 						(11, payment_id, option),
 						(13, failure_opt, upgradable_option),
 					});
@@ -1248,7 +1242,6 @@ impl MaybeReadable for Event {
 						failure,
 						path: path.unwrap(),
 						short_channel_id,
-						retry,
 						#[cfg(test)]
 						error_code,
 						#[cfg(test)]

pending_changelog/2063.txt

@@ -0,0 +1,4 @@
+## API Updates
+
+- `Event::PaymentPathFailed::retry` will always be `None` if an 0.0.115-generated event is read by a
+	prior version of LDK (#2063)