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.

diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index 97d0462eb5010980850c64b53dc37a07c512f476..3b692ac07e2fb1a8036fb262fac084fab58175b7 100644 (file)
--- a/lightning-background-processor/src/lib.rs
+++ b/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,

diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index fd867bc6ab7e07287c0572ae1cdb577e9e290ab8..2786d754abbf8789a675b5c2aec3e8b741eec089 100644 (file)
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/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);
                                        }

diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index d6d2972d0736e3ec642fccee7363e44329560584..32c3b51688a56cc14fe800af080d7d11ec5f158f 100644 (file)
--- a/lightning/src/ln/functional_tests.rs
+++ b/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));
                },

diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
index da5aadb83065af7d6de274c30a021e4b9b05fc23..bc6dd1168bf20863fa1eaed7fa94137dfb025555 100644 (file)
--- a/lightning/src/ln/onion_route_tests.rs
+++ b/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 {

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index a851c8eda1267b1ff04140cf64c16748c604ed15..16037c7f656954d4aeab22a0c3342fc6dc467268 100644 (file)
--- a/lightning/src/ln/outbound_payment.rs
+++ b/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,

diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index 86648e5fb724d39b4e027f2152ea002b32166f79..1d30bceff96125cdb9147ed5838fc2d83ba077db 100644 (file)
--- a/lightning/src/ln/payment_tests.rs
+++ b/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

diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs
index 98239fd487a8e8bb5dfd99baf37b8f1353eda8c3..80ecc22842900fa428c5557723be59379a74d8ad 100644 (file)
--- a/lightning/src/util/events.rs
+++ b/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,

diff --git a/pending_changelog/2043.txt b/pending_changelog/2043.txt
new file mode 100644 (file)
index 0000000..2fc7c75
--- /dev/null
+++ b/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.