From: Valentine Wallace Date: Sun, 12 Feb 2023 00:38:48 +0000 (-0500) Subject: Remove all_paths_failed from PaymentPathFailed X-Git-Tag: v0.0.114-beta~3^2~8 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=2037a241f4aa5ff976bfd556bc61c12b73890b3e;p=rust-lightning 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 97d0462e..3b692ac0 100644 --- 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 fd867bc6..2786d754 100644 --- 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 d6d2972d..32c3b516 100644 --- 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 da5aadb8..bc6dd116 100644 --- 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(_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 a851c8ed..16037c7f 100644 --- 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 86648e5f..1d30bcef 100644 --- 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 98239fd4..80ecc228 100644 --- 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, - /// 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, /// 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> = 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 index 00000000..2fc7c750 --- /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: + ), + and then migrate to 0.0.114.