Remove all_paths_failed from PaymentPathFailed

This field was previous useful in manual retries for users to know when all
paths of a payment have failed and it is safe to retry. Now that we support
automatic retries in ChannelManager and no longer support manual retries, the
field is no longer useful.

For backwards compat, we now always write false for this field. If we didn't do
this, previous versions would default this field's value to true, which can be
problematic because some clients have relied on the field to indicate when a
full payment retry is safe.

Author: valentinewallace

lightning-background-processor/src/lib.rs

@@ -1366,7 +1366,6 @@ mod tests {
 			payment_hash: PaymentHash([42; 32]),
 			payment_failed_permanently: false,
 			network_update: None,
-			all_paths_failed: true,
 			path: path.clone(),
 			short_channel_id: Some(scored_scid),
 			retry: None,
@@ -1387,7 +1386,6 @@ mod tests {
 			payment_hash: PaymentHash([42; 32]),
 			payment_failed_permanently: true,
 			network_update: None,
-			all_paths_failed: true,
 			path: path.clone(),
 			short_channel_id: None,
 			retry: None,

lightning/src/ln/functional_test_utils.rs

@@ -2240,10 +2240,9 @@ pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe
 			if i == expected_paths.len() - 1 { assert_eq!(events.len(), 2); } else { assert_eq!(events.len(), 1); }
 
 			let expected_payment_id = match events[0] {
-				Event::PaymentPathFailed { payment_hash, payment_failed_permanently, all_paths_failed, ref path, ref payment_id, .. } => {
+				Event::PaymentPathFailed { payment_hash, payment_failed_permanently, ref path, ref payment_id, .. } => {
 					assert_eq!(payment_hash, our_payment_hash);
 					assert!(payment_failed_permanently);
-					assert_eq!(all_paths_failed, i == expected_paths.len() - 1);
 					for (idx, hop) in expected_route.iter().enumerate() {
 						assert_eq!(hop.node.get_our_node_id(), path[idx].pubkey);
 					}

lightning/src/ln/functional_tests.rs

@@ -5695,11 +5695,10 @@ fn test_fail_holding_cell_htlc_upon_free() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match &events[0] {
-		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, .. } => {
+		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref short_channel_id, .. } => {
 			assert_eq!(PaymentId(our_payment_hash.0), *payment_id.as_ref().unwrap());
 			assert_eq!(our_payment_hash.clone(), *payment_hash);
 			assert_eq!(*payment_failed_permanently, false);
-			assert_eq!(*all_paths_failed, true);
 			assert_eq!(*network_update, None);
 			assert_eq!(*short_channel_id, Some(route.paths[0][0].short_channel_id));
 		},
@@ -5786,11 +5785,10 @@ fn test_free_and_fail_holding_cell_htlcs() {
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
 	match &events[0] {
-		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, .. } => {
+		&Event::PaymentPathFailed { ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update, ref short_channel_id, .. } => {
 			assert_eq!(payment_id_2, *payment_id.as_ref().unwrap());
 			assert_eq!(payment_hash_2.clone(), *payment_hash);
 			assert_eq!(*payment_failed_permanently, false);
-			assert_eq!(*all_paths_failed, true);
 			assert_eq!(*network_update, None);
 			assert_eq!(*short_channel_id, Some(route_2.paths[0][0].short_channel_id));
 		},

lightning/src/ln/onion_route_tests.rs

@@ -167,9 +167,8 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 
 	let events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 2);
-	if let &Event::PaymentPathFailed { ref payment_failed_permanently, ref network_update, ref all_paths_failed, ref short_channel_id, ref error_code, .. } = &events[0] {
+	if let &Event::PaymentPathFailed { ref payment_failed_permanently, ref network_update, ref short_channel_id, ref error_code, .. } = &events[0] {
 		assert_eq!(*payment_failed_permanently, !expected_retryable);
-		assert_eq!(*all_paths_failed, true);
 		assert_eq!(*error_code, expected_error_code);
 		if expected_channel_update.is_some() {
 			match network_update {

lightning/src/ln/outbound_payment.rs

@@ -794,7 +794,6 @@ impl OutboundPayments {
 					payment_hash,
 					payment_failed_permanently: false,
 					network_update: None,
-					all_paths_failed: false,
 					path,
 					short_channel_id: failed_scid,
 					retry: None,
@@ -1157,7 +1156,6 @@ impl OutboundPayments {
 			awaiting_retry
 		});
 
-		let mut all_paths_failed = false;
 		let mut full_failure_ev = None;
 		let mut pending_retry_ev = false;
 		let mut retry = None;
@@ -1206,7 +1204,6 @@ impl OutboundPayments {
 				is_retryable_now = false;
 			}
 			if payment.get().remaining_parts() == 0 {
-				all_paths_failed = true;
 				if payment.get().abandoned() {
 					if !payment_is_probe {
 						full_failure_ev = Some(events::Event::PaymentFailed {
@@ -1259,7 +1256,6 @@ impl OutboundPayments {
 					payment_hash: payment_hash.clone(),
 					payment_failed_permanently: !payment_retryable,
 					network_update,
-					all_paths_failed,
 					path: path.clone(),
 					short_channel_id,
 					retry,

lightning/src/ln/payment_tests.rs

@@ -2139,7 +2139,7 @@ fn retry_multi_path_single_failed_payment() {
 	assert_eq!(events.len(), 1);
 	match events[0] {
 		Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false,
-			network_update: None, all_paths_failed: false, short_channel_id: Some(expected_scid), .. } => {
+			network_update: None, short_channel_id: Some(expected_scid), .. } => {
 			assert_eq!(payment_hash, ev_payment_hash);
 			assert_eq!(expected_scid, route.paths[1][0].short_channel_id);
 		},
@@ -2212,7 +2212,7 @@ fn immediate_retry_on_failure() {
 	assert_eq!(events.len(), 1);
 	match events[0] {
 		Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently: false,
-		network_update: None, all_paths_failed: false, short_channel_id: Some(expected_scid), .. } => {
+		network_update: None, short_channel_id: Some(expected_scid), .. } => {
 			assert_eq!(payment_hash, ev_payment_hash);
 			assert_eq!(expected_scid, route.paths[1][0].short_channel_id);
 		},
@@ -2226,7 +2226,8 @@ fn immediate_retry_on_failure() {
 #[test]
 fn no_extra_retries_on_back_to_back_fail() {
 	// In a previous release, we had a race where we may exceed the payment retry count if we
-	// get two failures in a row with the second having `all_paths_failed` set.
+	// get two failures in a row with the second indicating that all paths had failed (this field,
+	// `all_paths_failed`, has since been removed).
 	// Generally, when we give up trying to retry a payment, we don't know for sure what the
 	// current state of the ChannelManager event queue is. Specifically, we cannot be sure that
 	// there are not multiple additional `PaymentPathFailed` or even `PaymentSent` events

lightning/src/util/events.rs

@@ -630,13 +630,12 @@ pub enum Event {
 	/// handle the HTLC.
 	///
 	/// Note that this does *not* indicate that all paths for an MPP payment have failed, see
-	/// [`Event::PaymentFailed`] and [`all_paths_failed`].
+	/// [`Event::PaymentFailed`].
 	///
 	/// See [`ChannelManager::abandon_payment`] for giving up on this payment before its retries have
 	/// been exhausted.
 	///
 	/// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment
-	/// [`all_paths_failed`]: Self::PaymentPathFailed::all_paths_failed
 	PaymentPathFailed {
 		/// The id returned by [`ChannelManager::send_payment`] and used with
 		/// [`ChannelManager::abandon_payment`].
@@ -660,10 +659,6 @@ pub enum Event {
 		///
 		/// [`NetworkGraph`]: crate::routing::gossip::NetworkGraph
 		network_update: Option<NetworkUpdate>,
-		/// For both single-path and multi-path payments, this is set if all paths of the payment have
-		/// failed. This will be set to false if (1) this is an MPP payment and (2) other parts of the
-		/// larger MPP payment were still in flight when this event was generated.
-		all_paths_failed: bool,
 		/// The payment path that failed.
 		path: Vec<RouteHop>,
 		/// The channel responsible for the failed payment path.
@@ -966,7 +961,7 @@ impl Writeable for Event {
 			},
 			&Event::PaymentPathFailed {
 				ref payment_id, ref payment_hash, ref payment_failed_permanently, ref network_update,
-				ref all_paths_failed, ref path, ref short_channel_id, ref retry,
+				ref path, ref short_channel_id, ref retry,
 				#[cfg(test)]
 				ref error_code,
 				#[cfg(test)]
@@ -981,7 +976,7 @@ impl Writeable for Event {
 					(0, payment_hash, required),
 					(1, network_update, option),
 					(2, payment_failed_permanently, required),
-					(3, all_paths_failed, required),
+					(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),
@@ -1198,7 +1193,6 @@ impl MaybeReadable for Event {
 					let mut payment_hash = PaymentHash([0; 32]);
 					let mut payment_failed_permanently = false;
 					let mut network_update = None;
-					let mut all_paths_failed = Some(true);
 					let mut path: Option<Vec<RouteHop>> = Some(vec![]);
 					let mut short_channel_id = None;
 					let mut retry = None;
@@ -1207,7 +1201,6 @@ impl MaybeReadable for Event {
 						(0, payment_hash, required),
 						(1, network_update, ignorable),
 						(2, payment_failed_permanently, required),
-						(3, all_paths_failed, option),
 						(5, path, vec_type),
 						(7, short_channel_id, option),
 						(9, retry, option),
@@ -1218,7 +1211,6 @@ impl MaybeReadable for Event {
 						payment_hash,
 						payment_failed_permanently,
 						network_update,
-						all_paths_failed: all_paths_failed.unwrap(),
 						path: path.unwrap(),
 						short_channel_id,
 						retry,

pending_changelog/2043.txt

@@ -0,0 +1,11 @@
+## API Updates
+- `Event::PaymentPathFailed::all_paths_failed` has been removed, as we've dropped support for manual
+	payment retries.
+
+## Backwards Compatibility
+- If downgrading from 0.0.114 to a previous version, `Event::PaymentPathFailed::all_paths_failed`
+	will always be set to `false`. Users who wish to support downgrading and currently rely on the
+	field should should first migrate to always calling `ChannelManager::abandon_payment` and awaiting
+	`PaymentFailed` events before retrying (see the field docs for more info on this approach:
+	<https://docs.rs/lightning/0.0.113/lightning/util/events/enum.Event.html#variant.PaymentPathFailed.field.all_paths_failed>),
+	and then migrate to 0.0.114.