From 753e4ce3c39080abe77e4d53a4ccb067d48f87a8 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 1 Mar 2023 11:46:15 -0500 Subject: [PATCH] Remove PaymentPathFailed::retry We now support automatic retries in ChannelManager and no longer support manual retries, so the field is useless. --- lightning-background-processor/src/lib.rs | 2 -- lightning/src/ln/channelmanager.rs | 4 +-- lightning/src/ln/functional_test_utils.rs | 9 +---- lightning/src/ln/outbound_payment.rs | 43 ++++------------------- lightning/src/util/events.rs | 11 ++---- pending_changelog/2063.txt | 4 +++ 6 files changed, 15 insertions(+), 58 deletions(-) create mode 100644 pending_changelog/2063.txt diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs index f64f1e765..e407c4fdb 100644 --- a/lightning-background-processor/src/lib.rs +++ b/lightning-background-processor/src/lib.rs @@ -1369,7 +1369,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)) @@ -1389,7 +1388,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)) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 6e0cd1709..478e52da4 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3834,9 +3834,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(); } }, diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 1e99a475f..c8d3beb3c 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1868,20 +1868,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"); diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index fdf9cdd0e..544d3e6c9 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/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); @@ -798,7 +790,6 @@ impl OutboundPayments { failure: events::PathFailure::InitialSend { err: e }, path, short_channel_id: failed_scid, - retry: None, #[cfg(test)] error_code: None, #[cfg(test)] @@ -1128,8 +1119,8 @@ impl OutboundPayments { pub(super) fn fail_htlc( &self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, path: &Vec, session_priv: &SecretKey, payment_id: &PaymentId, - payment_params: &Option, probing_cookie_secret: [u8; 32], - secp_ctx: &Secp256k1, pending_events: &Mutex>, logger: &L + probing_cookie_secret: [u8; 32], secp_ctx: &Secp256k1, + pending_events: &Mutex>, 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); @@ -1157,7 +1148,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)); @@ -1169,27 +1159,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; } @@ -1229,12 +1205,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 { @@ -1248,7 +1218,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)] diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 5bc8a5084..8c981fd1c 100644 --- a/lightning/src/util/events.rs +++ b/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, - /// Parameters used by LDK to compute a new [`Route`] when retrying the failed payment path. - /// - /// [`Route`]: crate::routing::router::Route - retry: Option, #[cfg(test)] error_code: Option, #[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::, 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> = 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)] diff --git a/pending_changelog/2063.txt b/pending_changelog/2063.txt new file mode 100644 index 000000000..8ac52a416 --- /dev/null +++ b/pending_changelog/2063.txt @@ -0,0 +1,4 @@ +## API Updates + +- `Event::PaymentPathFailed::retry` will always be `None` if we initiate a payment on 0.0.115 + then downgrade to an earlier version (#2063) -- 2.39.5