From 0b3240ee6a6e1baeafaed9e766997098be588358 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 3 Dec 2021 19:57:37 +0000 Subject: [PATCH] Add a variant to `PendingOutboundPayment` for retries-exceeded When a payer gives up trying to retry a payment, they don't know for sure what the current state of the event queue is. Specifically, they cannot be sure that there are not multiple additional `PaymentPathFailed` or even `PaymentSuccess` events pending which they will see later. Thus, they have a very hard time identifying whether a payment has truly failed (and informing the UI of that fact) or if it is still pending. See [1] for more information. In order to avoid this mess, we will resolve it here by having the payer give `ChannelManager` a bit more information - when they have given up on a payment - and using that to generate a `PaymentFailed` event when all paths have failed. This commit adds the neccessary storage and changes for the new state inside `ChannelManager` and a public method to mark a payment as failed, the next few commits will add the new `Event` and use the new features in our `PaymentRetrier`. [1] https://github.com/lightningdevkit/rust-lightning/issues/1164 --- lightning/src/ln/channelmanager.rs | 82 +++++++++++++++++++++++++++--- 1 file changed, 74 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index f9f7b044..1d3b292d 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -454,6 +454,17 @@ pub(crate) enum PendingOutboundPayment { session_privs: HashSet<[u8; 32]>, payment_hash: Option, }, + /// When a payer gives up trying to retry a payment, they inform us, letting us generate a + /// `PaymentFailed` event when all HTLCs have irrevocably failed. This avoids a number of race + /// conditions in MPP-aware payment retriers (1), where the possibility of multiple + /// `PaymentPathFailed` events with `all_paths_failed` can be pending at once, confusing a + /// downstream event handler as to when a payment has actually failed. + /// + /// (1) https://github.com/lightningdevkit/rust-lightning/issues/1164 + Abandoned { + session_privs: HashSet<[u8; 32]>, + payment_hash: PaymentHash, + }, } impl PendingOutboundPayment { @@ -469,6 +480,12 @@ impl PendingOutboundPayment { _ => false, } } + fn abandoned(&self) -> bool { + match self { + PendingOutboundPayment::Abandoned { .. } => true, + _ => false, + } + } fn get_pending_fee_msat(&self) -> Option { match self { PendingOutboundPayment::Retryable { pending_fee_msat, .. } => pending_fee_msat.clone(), @@ -481,6 +498,7 @@ impl PendingOutboundPayment { PendingOutboundPayment::Legacy { .. } => None, PendingOutboundPayment::Retryable { payment_hash, .. } => Some(*payment_hash), PendingOutboundPayment::Fulfilled { payment_hash, .. } => *payment_hash, + PendingOutboundPayment::Abandoned { payment_hash, .. } => Some(*payment_hash), } } @@ -489,19 +507,38 @@ impl PendingOutboundPayment { core::mem::swap(&mut session_privs, match self { PendingOutboundPayment::Legacy { session_privs } | PendingOutboundPayment::Retryable { session_privs, .. } | - PendingOutboundPayment::Fulfilled { session_privs, .. } - => session_privs + PendingOutboundPayment::Fulfilled { session_privs, .. } | + PendingOutboundPayment::Abandoned { session_privs, .. } + => session_privs, }); let payment_hash = self.payment_hash(); *self = PendingOutboundPayment::Fulfilled { session_privs, payment_hash }; } + fn mark_abandoned(&mut self) -> Result<(), ()> { + let mut session_privs = HashSet::new(); + let our_payment_hash; + core::mem::swap(&mut session_privs, match self { + PendingOutboundPayment::Legacy { .. } | + PendingOutboundPayment::Fulfilled { .. } => + return Err(()), + PendingOutboundPayment::Retryable { session_privs, payment_hash, .. } | + PendingOutboundPayment::Abandoned { session_privs, payment_hash, .. } => { + our_payment_hash = *payment_hash; + session_privs + }, + }); + *self = PendingOutboundPayment::Abandoned { session_privs, payment_hash: our_payment_hash }; + Ok(()) + } + /// panics if path is None and !self.is_fulfilled fn remove(&mut self, session_priv: &[u8; 32], path: Option<&Vec>) -> bool { let remove_res = match self { PendingOutboundPayment::Legacy { session_privs } | PendingOutboundPayment::Retryable { session_privs, .. } | - PendingOutboundPayment::Fulfilled { session_privs, .. } => { + PendingOutboundPayment::Fulfilled { session_privs, .. } | + PendingOutboundPayment::Abandoned { session_privs, .. } => { session_privs.remove(session_priv) } }; @@ -524,7 +561,8 @@ impl PendingOutboundPayment { PendingOutboundPayment::Retryable { session_privs, .. } => { session_privs.insert(session_priv) } - PendingOutboundPayment::Fulfilled { .. } => false + PendingOutboundPayment::Fulfilled { .. } => false, + PendingOutboundPayment::Abandoned { .. } => false, }; if insert_res { if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self { @@ -542,7 +580,8 @@ impl PendingOutboundPayment { match self { PendingOutboundPayment::Legacy { session_privs } | PendingOutboundPayment::Retryable { session_privs, .. } | - PendingOutboundPayment::Fulfilled { session_privs, .. } => { + PendingOutboundPayment::Fulfilled { session_privs, .. } | + PendingOutboundPayment::Abandoned { session_privs, .. } => { session_privs.len() } } @@ -2313,10 +2352,12 @@ impl ChannelMana /// /// Errors returned are a superset of those returned from [`send_payment`], so see /// [`send_payment`] documentation for more details on errors. This method will also error if the - /// retry amount puts the payment more than 10% over the payment's total amount, or if the payment - /// for the given `payment_id` cannot be found (likely due to timeout or success). + /// retry amount puts the payment more than 10% over the payment's total amount, if the payment + /// for the given `payment_id` cannot be found (likely due to timeout or success), or if + /// further retries have been disabled with [`abandon_payment`]. /// /// [`send_payment`]: [`ChannelManager::send_payment`] + /// [`abandon_payment`]: [`ChannelManager::abandon_payment`] pub fn retry_payment(&self, route: &Route, payment_id: PaymentId) -> Result<(), PaymentSendFailure> { const RETRY_OVERFLOW_PERCENTAGE: u64 = 10; for path in route.paths.iter() { @@ -2352,6 +2393,11 @@ impl ChannelMana err: "Payment already completed" })); }, + PendingOutboundPayment::Abandoned { .. } => { + return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { + err: "Payment already abandoned (with some HTLCs still pending)".to_owned() + })); + }, } } else { return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { @@ -2362,6 +2408,21 @@ impl ChannelMana return self.send_payment_internal(route, payment_hash, &payment_secret, None, Some(payment_id), Some(total_msat)).map(|_| ()) } + /// Signals that no further retries for the given payment will occur. + /// + /// After this method returns, any future calls to [`retry_payment`] for the given `payment_id` + /// will fail with [`PaymentSendFailure::ParameterError`]. + /// + /// [`retry_payment`]: Self::retry_payment + pub fn abandon_payment(&self, payment_id: PaymentId) { + let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); + + let mut outbounds = self.pending_outbound_payments.lock().unwrap(); + if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) { + let _ = payment.get_mut().mark_abandoned(); + } + } + /// Send a spontaneous payment, which is a payment that does not require the recipient to have /// generated an invoice. Optionally, you may specify the preimage. If you do choose to specify /// the preimage, it must be a cryptographically secure random value that no intermediate node @@ -5605,6 +5666,10 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment, (8, pending_amt_msat, required), (10, starting_block_height, required), }, + (3, Abandoned) => { + (0, session_privs, required), + (2, payment_hash, required), + }, ); impl Writeable for ChannelManager @@ -5698,7 +5763,7 @@ impl Writeable f // For backwards compat, write the session privs and their total length. let mut num_pending_outbounds_compat: u64 = 0; for (_, outbound) in pending_outbound_payments.iter() { - if !outbound.is_fulfilled() { + if !outbound.is_fulfilled() && !outbound.abandoned() { num_pending_outbounds_compat += outbound.remaining_parts() as u64; } } @@ -5712,6 +5777,7 @@ impl Writeable f } } PendingOutboundPayment::Fulfilled { .. } => {}, + PendingOutboundPayment::Abandoned { .. } => {}, } } -- 2.30.2