From 8c9615e8d6be6d4e3ff2d861a8a30266217672b2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 4 Dec 2021 23:41:01 +0000 Subject: [PATCH 1/5] DRY up payment failure macros in functional_test_utils ... with a more extensible expectation-checking framework for them. --- lightning/src/ln/functional_test_utils.rs | 117 +++++++++++++++------- lightning/src/ln/functional_tests.rs | 2 +- lightning/src/ln/monitor_tests.rs | 1 - lightning/src/ln/reorg_tests.rs | 1 - lightning/src/ln/shutdown_tests.rs | 1 - 5 files changed, 81 insertions(+), 41 deletions(-) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 78bf834a628..84f4860d47a 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -336,10 +336,11 @@ pub fn create_chan_between_nodes_with_value<'a, 'b, 'c, 'd>(node_a: &'a Node<'b, } #[macro_export] +/// Gets an RAA and CS which were sent in response to a commitment update macro_rules! get_revoke_commit_msgs { ($node: expr, $node_id: expr) => { { - use util::events::MessageSendEvent; + use $crate::util::events::MessageSendEvent; let events = $node.node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 2); (match events[0] { @@ -400,8 +401,8 @@ macro_rules! get_event { } } -#[cfg(test)] #[macro_export] +/// Gets an UpdateHTLCs MessageSendEvent macro_rules! get_htlc_update_msgs { ($node: expr, $node_id: expr) => { { @@ -933,6 +934,9 @@ impl SendEvent { } } +#[macro_export] +/// Performs the "commitment signed dance" - the series of message exchanges which occur after a +/// commitment update. macro_rules! commitment_signed_dance { ($node_a: expr, $node_b: expr, $commitment_signed: expr, $fail_backwards: expr, true /* skip last step */) => { { @@ -999,7 +1003,7 @@ macro_rules! commitment_signed_dance { { commitment_signed_dance!($node_a, $node_b, $commitment_signed, $fail_backwards, true); if $fail_backwards { - expect_pending_htlcs_forwardable!($node_a); + $crate::expect_pending_htlcs_forwardable!($node_a); check_added_monitors!($node_a, 1); let channel_state = $node_a.node.channel_state.lock().unwrap(); @@ -1057,6 +1061,8 @@ macro_rules! get_route_and_payment_hash { }} } +#[macro_export] +/// Clears (and ignores) a PendingHTLCsForwardable event macro_rules! expect_pending_htlcs_forwardable_ignore { ($node: expr) => {{ let events = $node.node.get_and_clear_pending_events(); @@ -1068,9 +1074,11 @@ macro_rules! expect_pending_htlcs_forwardable_ignore { }} } +#[macro_export] +/// Handles a PendingHTLCsForwardable event macro_rules! expect_pending_htlcs_forwardable { ($node: expr) => {{ - expect_pending_htlcs_forwardable_ignore!($node); + $crate::expect_pending_htlcs_forwardable_ignore!($node); $node.node.process_pending_htlc_forwards(); // Ensure process_pending_htlc_forwards is idempotent. @@ -1199,60 +1207,95 @@ macro_rules! expect_payment_forwarded { } } +pub struct PaymentFailedConditions<'a> { + pub(crate) expected_htlc_error_data: Option<(u16, &'a [u8])>, + pub(crate) expected_blamed_scid: Option, + pub(crate) expected_blamed_chan_closed: Option, +} + +impl<'a> PaymentFailedConditions<'a> { + pub fn new() -> Self { + Self { + expected_htlc_error_data: None, + expected_blamed_scid: None, + expected_blamed_chan_closed: None, + } + } + pub fn blamed_scid(mut self, scid: u64) -> Self { + self.expected_blamed_scid = Some(scid); + self + } + pub fn blamed_chan_closed(mut self, closed: bool) -> Self { + self.expected_blamed_chan_closed = Some(closed); + self + } + pub fn expected_htlc_error_data(mut self, code: u16, data: &'a [u8]) -> Self { + self.expected_htlc_error_data = Some((code, data)); + self + } +} + #[cfg(test)] macro_rules! expect_payment_failed_with_update { ($node: expr, $expected_payment_hash: expr, $rejected_by_dest: expr, $scid: expr, $chan_closed: expr) => { - let events = $node.node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref network_update, ref error_code, ref error_data, ref path, ref retry, .. } => { - assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash"); - assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest 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().payee.pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path"); - assert!(error_code.is_some(), "expected error_code.is_some() = true"); - assert!(error_data.is_some(), "expected error_data.is_some() = true"); - match network_update { - &Some(NetworkUpdate::ChannelUpdateMessage { ref msg }) if !$chan_closed => { - assert_eq!(msg.contents.short_channel_id, $scid); - assert_eq!(msg.contents.flags & 2, 0); - }, - &Some(NetworkUpdate::ChannelClosed { short_channel_id, is_permanent }) if $chan_closed => { - assert_eq!(short_channel_id, $scid); - assert!(is_permanent); - }, - Some(_) => panic!("Unexpected update type"), - None => panic!("Expected update"), - } - }, - _ => panic!("Unexpected event"), - } + expect_payment_failed_conditions!($node, $expected_payment_hash, $rejected_by_dest, + $crate::ln::functional_test_utils::PaymentFailedConditions::new().blamed_scid($scid).blamed_chan_closed($chan_closed)); } } #[cfg(test)] macro_rules! expect_payment_failed { ($node: expr, $expected_payment_hash: expr, $rejected_by_dest: expr $(, $expected_error_code: expr, $expected_error_data: expr)*) => { + let mut conditions = $crate::ln::functional_test_utils::PaymentFailedConditions::new(); + $( + conditions = conditions.expected_htlc_error_data($expected_error_code, &$expected_error_data); + )* + expect_payment_failed_conditions!($node, $expected_payment_hash, $rejected_by_dest, conditions); + }; +} + +#[cfg(test)] +macro_rules! expect_payment_failed_conditions { + ($node: expr, $expected_payment_hash: expr, $rejected_by_dest: expr, $conditions: expr) => { let events = $node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { - Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, network_update: _, ref error_code, ref error_data, ref path, ref retry, .. } => { + Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref error_code, ref error_data, ref path, ref retry, ref network_update, .. } => { assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash"); assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest 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().payee.pubkey, path.last().unwrap().pubkey, "Retry payee node_id should match last hop in path"); + assert!(error_code.is_some(), "expected error_code.is_some() = true"); assert!(error_data.is_some(), "expected error_data.is_some() = true"); - $( - assert_eq!(error_code.unwrap(), $expected_error_code, "unexpected error code"); - assert_eq!(&error_data.as_ref().unwrap()[..], $expected_error_data, "unexpected error data"); - )* + if let Some((code, data)) = $conditions.expected_htlc_error_data { + assert_eq!(error_code.unwrap(), code, "unexpected error code"); + assert_eq!(&error_data.as_ref().unwrap()[..], data, "unexpected error data"); + } + + if let Some(chan_closed) = $conditions.expected_blamed_chan_closed { + match network_update { + &Some($crate::routing::network_graph::NetworkUpdate::ChannelUpdateMessage { ref msg }) if !chan_closed => { + if let Some(scid) = $conditions.expected_blamed_scid { + assert_eq!(msg.contents.short_channel_id, scid); + } + assert_eq!(msg.contents.flags & 2, 0); + }, + &Some($crate::routing::network_graph::NetworkUpdate::ChannelClosed { short_channel_id, is_permanent }) if chan_closed => { + if let Some(scid) = $conditions.expected_blamed_scid { + assert_eq!(short_channel_id, scid); + } + assert!(is_permanent); + }, + Some(_) => panic!("Unexpected update type"), + None => panic!("Expected update"), + } + } }, _ => panic!("Unexpected event"), - } - } + }; + }; } pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret) -> PaymentId { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 3a30cb7f1d0..1bf4f67de76 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -23,7 +23,7 @@ use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAAC use ln::channel::{Channel, ChannelError}; use ln::{chan_utils, onion_utils}; use ln::chan_utils::{HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, HTLCOutputInCommitment}; -use routing::network_graph::{NetworkUpdate, RoutingFees}; +use routing::network_graph::RoutingFees; use routing::router::{Payee, Route, RouteHop, RouteHint, RouteHintHop, RouteParameters, find_route, get_route}; use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures}; use ln::msgs; diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index d836b736a98..a3733fb5eda 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -16,7 +16,6 @@ use ln::channelmanager::BREAKDOWN_TIMEOUT; use ln::features::InitFeatures; use ln::msgs::ChannelMessageHandler; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason}; -use routing::network_graph::NetworkUpdate; use bitcoin::blockdata::script::Builder; use bitcoin::blockdata::opcodes; diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 570dc3d0eee..fe8fd68274b 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -15,7 +15,6 @@ use chain::{Confirm, Watch}; use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs}; use ln::features::InitFeatures; use ln::msgs::ChannelMessageHandler; -use routing::network_graph::NetworkUpdate; use util::enforcing_trait_impls::EnforcingSigner; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason}; use util::test_utils; diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index ffd809bc70d..791e87377f0 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -13,7 +13,6 @@ use chain::keysinterface::KeysInterface; use chain::transaction::OutPoint; use ln::channelmanager::PaymentSendFailure; use routing::router::{Payee, get_route}; -use routing::network_graph::NetworkUpdate; use ln::features::{InitFeatures, InvoiceFeatures}; use ln::msgs; use ln::msgs::{ChannelMessageHandler, ErrorAction}; From 0b3240ee6a6e1baeafaed9e766997098be588358 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 3 Dec 2021 19:57:37 +0000 Subject: [PATCH 2/5] 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 f9f7b044022..1d3b292da87 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 { .. } => {}, } } From 7782d0a1ef9fb4e9d26f78a042da16939708d697 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 10 Dec 2021 00:28:24 +0000 Subject: [PATCH 3/5] Expose an event when a payment has failed and retries complete When a payment fails, a payer needs to know when they can consider a payment as fully-failed, and when only some of the HTLCs in the payment have failed. This isn't possible with the current event scheme, as discovered recently and as described in the previous commit. This adds a new event which describes when a payment is fully and irrevocably failed, generating it only after the payment has expired or been marked as expired with `ChannelManager::mark_retries_exceeded` *and* all HTLCs for it have failed. With this, a payer can more simply deduce when a payment has failed and use that to remove payment state or finalize a payment failure. --- lightning/src/ln/channelmanager.rs | 147 ++++++++++++++-------- lightning/src/ln/functional_test_utils.rs | 44 ++++++- lightning/src/ln/functional_tests.rs | 29 ++++- lightning/src/ln/payment_tests.rs | 6 +- lightning/src/util/events.rs | 67 +++++++++- 5 files changed, 225 insertions(+), 68 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 1d3b292da87..4caa5435eac 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -838,6 +838,10 @@ const CHECK_CLTV_EXPIRY_SANITY: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRA #[allow(dead_code)] const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER; +/// The number of blocks before we consider an outbound payment for expiry if it doesn't have any +/// pending HTLCs in flight. +pub(crate) const PAYMENT_EXPIRY_BLOCKS: u32 = 3; + /// Information needed for constructing an invoice route hint for this channel. #[derive(Clone, Debug, PartialEq)] pub struct CounterpartyForwardingInfo { @@ -2411,15 +2415,31 @@ impl ChannelMana /// 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`]. + /// will fail with [`PaymentSendFailure::ParameterError`]. If no such event has been generated, + /// an [`Event::PaymentFailed`] event will be generated as soon as there are no remaining + /// pending HTLCs for this payment. + /// + /// Note that calling this method does *not* prevent a payment from succeeding. You must still + /// wait until you receive either a [`Event::PaymentFailed`] or [`Event::PaymentSent`] event to + /// determine the ultimate status of a payment. /// /// [`retry_payment`]: Self::retry_payment + /// [`Event::PaymentFailed`]: events::Event::PaymentFailed + /// [`Event::PaymentSent`]: events::Event::PaymentSent 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(); + if let Ok(()) = payment.get_mut().mark_abandoned() { + if payment.get().remaining_parts() == 0 { + self.pending_events.lock().unwrap().push(events::Event::PaymentFailed { + payment_id, + payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"), + }); + payment.remove(); + } + } } } @@ -3250,22 +3270,28 @@ impl ChannelMana final_cltv_expiry_delta: path_last_hop.cltv_expiry_delta, }) } else { None }; - self.pending_events.lock().unwrap().push( - events::Event::PaymentPathFailed { - payment_id: Some(payment_id), - payment_hash, - rejected_by_dest: false, - network_update: None, - all_paths_failed: payment.get().remaining_parts() == 0, - path: path.clone(), - short_channel_id: None, - retry, - #[cfg(test)] - error_code: None, - #[cfg(test)] - error_data: None, - } - ); + let mut pending_events = self.pending_events.lock().unwrap(); + pending_events.push(events::Event::PaymentPathFailed { + payment_id: Some(payment_id), + payment_hash, + rejected_by_dest: false, + network_update: None, + all_paths_failed: payment.get().remaining_parts() == 0, + path: path.clone(), + short_channel_id: None, + retry, + #[cfg(test)] + error_code: None, + #[cfg(test)] + error_data: None, + }); + if payment.get().abandoned() && payment.get().remaining_parts() == 0 { + pending_events.push(events::Event::PaymentFailed { + payment_id, + payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"), + }); + payment.remove(); + } } } else { log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); @@ -3296,6 +3322,7 @@ impl ChannelMana session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); let mut all_paths_failed = false; + let mut full_failure_ev = None; if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) { if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) { log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); @@ -3307,6 +3334,13 @@ impl ChannelMana } if payment.get().remaining_parts() == 0 { all_paths_failed = true; + if payment.get().abandoned() { + full_failure_ev = Some(events::Event::PaymentFailed { + payment_id, + payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"), + }); + payment.remove(); + } } } else { log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); @@ -3322,7 +3356,8 @@ impl ChannelMana }) } else { None }; log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0)); - match &onion_error { + + let path_failure = match &onion_error { &HTLCFailReason::LightningError { ref err } => { #[cfg(test)] let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone()); @@ -3331,22 +3366,20 @@ impl ChannelMana // 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! - self.pending_events.lock().unwrap().push( - events::Event::PaymentPathFailed { - payment_id: Some(payment_id), - payment_hash: payment_hash.clone(), - rejected_by_dest: !payment_retryable, - network_update, - all_paths_failed, - path: path.clone(), - short_channel_id, - retry, + events::Event::PaymentPathFailed { + payment_id: Some(payment_id), + payment_hash: payment_hash.clone(), + rejected_by_dest: !payment_retryable, + network_update, + all_paths_failed, + path: path.clone(), + short_channel_id, + retry, #[cfg(test)] - error_code: onion_error_code, + error_code: onion_error_code, #[cfg(test)] - error_data: onion_error_data - } - ); + error_data: onion_error_data + } }, &HTLCFailReason::Reason { #[cfg(test)] @@ -3361,24 +3394,25 @@ impl ChannelMana // ChannelDetails. // TODO: For non-temporary failures, we really should be closing the // channel here as we apparently can't relay through them anyway. - self.pending_events.lock().unwrap().push( - events::Event::PaymentPathFailed { - payment_id: Some(payment_id), - payment_hash: payment_hash.clone(), - rejected_by_dest: path.len() == 1, - network_update: None, - all_paths_failed, - path: path.clone(), - short_channel_id: Some(path.first().unwrap().short_channel_id), - retry, + events::Event::PaymentPathFailed { + payment_id: Some(payment_id), + payment_hash: payment_hash.clone(), + rejected_by_dest: path.len() == 1, + network_update: None, + all_paths_failed, + path: path.clone(), + short_channel_id: Some(path.first().unwrap().short_channel_id), + retry, #[cfg(test)] - error_code: Some(*failure_code), + error_code: Some(*failure_code), #[cfg(test)] - error_data: Some(data.clone()), - } - ); + error_data: Some(data.clone()), + } } - } + }; + let mut pending_events = self.pending_events.lock().unwrap(); + pending_events.push(path_failure); + if let Some(ev) = full_failure_ev { pending_events.push(ev); } }, HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret, .. }) => { let err_packet = match onion_error { @@ -4907,14 +4941,19 @@ where inbound_payment.expiry_time > header.time as u64 }); + let mut pending_events = self.pending_events.lock().unwrap(); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); - outbounds.retain(|_, payment| { - const PAYMENT_EXPIRY_BLOCKS: u32 = 3; + outbounds.retain(|payment_id, payment| { if payment.remaining_parts() != 0 { return true } - if let PendingOutboundPayment::Retryable { starting_block_height, .. } = payment { - return *starting_block_height + PAYMENT_EXPIRY_BLOCKS > height - } - true + if let PendingOutboundPayment::Retryable { starting_block_height, payment_hash, .. } = payment { + if *starting_block_height + PAYMENT_EXPIRY_BLOCKS <= height { + log_info!(self.logger, "Timing out payment with id {} and hash {}", log_bytes!(payment_id.0), log_bytes!(payment_hash.0)); + pending_events.push(events::Event::PaymentFailed { + payment_id: *payment_id, payment_hash: *payment_hash, + }); + false + } else { true } + } else { true } }); } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 84f4860d47a..8ff793ed08b 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1211,6 +1211,7 @@ pub struct PaymentFailedConditions<'a> { pub(crate) expected_htlc_error_data: Option<(u16, &'a [u8])>, pub(crate) expected_blamed_scid: Option, pub(crate) expected_blamed_chan_closed: Option, + pub(crate) expected_mpp_parts_remain: bool, } impl<'a> PaymentFailedConditions<'a> { @@ -1219,8 +1220,13 @@ impl<'a> PaymentFailedConditions<'a> { expected_htlc_error_data: None, expected_blamed_scid: None, expected_blamed_chan_closed: None, + expected_mpp_parts_remain: false, } } + pub fn mpp_parts_remain(mut self) -> Self { + self.expected_mpp_parts_remain = true; + self + } pub fn blamed_scid(mut self, scid: u64) -> Self { self.expected_blamed_scid = Some(scid); self @@ -1246,6 +1252,7 @@ macro_rules! expect_payment_failed_with_update { #[cfg(test)] macro_rules! expect_payment_failed { ($node: expr, $expected_payment_hash: expr, $rejected_by_dest: expr $(, $expected_error_code: expr, $expected_error_data: expr)*) => { + #[allow(unused_mut)] let mut conditions = $crate::ln::functional_test_utils::PaymentFailedConditions::new(); $( conditions = conditions.expected_htlc_error_data($expected_error_code, &$expected_error_data); @@ -1259,8 +1266,8 @@ macro_rules! expect_payment_failed_conditions { ($node: expr, $expected_payment_hash: expr, $rejected_by_dest: expr, $conditions: expr) => { let events = $node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref error_code, ref error_data, ref path, ref retry, ref network_update, .. } => { + let expected_payment_id = match events[0] { + Event::PaymentPathFailed { ref payment_hash, rejected_by_dest, ref error_code, ref error_data, ref path, ref retry, ref payment_id, ref network_update, .. } => { assert_eq!(*payment_hash, $expected_payment_hash, "unexpected payment_hash"); assert_eq!(rejected_by_dest, $rejected_by_dest, "unexpected rejected_by_dest value"); assert!(retry.is_some(), "expected retry.is_some()"); @@ -1292,10 +1299,24 @@ macro_rules! expect_payment_failed_conditions { None => panic!("Expected update"), } } + + payment_id.unwrap() }, _ => panic!("Unexpected event"), }; - }; + if !$conditions.expected_mpp_parts_remain { + $node.node.abandon_payment(expected_payment_id); + let events = $node.node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + Event::PaymentFailed { ref payment_hash, ref payment_id } => { + assert_eq!(*payment_hash, $expected_payment_hash, "unexpected second payment_hash"); + assert_eq!(*payment_id, expected_payment_id); + } + _ => panic!("Unexpected second event"), + } + } + } } pub fn send_along_route_with_secret<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route, expected_paths: &[&[&Node<'a, 'b, 'c>]], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: PaymentSecret) -> PaymentId { @@ -1598,16 +1619,29 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe commitment_signed_dance!(origin_node, prev_node, next_msgs.as_ref().unwrap().1, false); let events = origin_node.node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentPathFailed { payment_hash, rejected_by_dest, all_paths_failed, ref path, .. } => { + let expected_payment_id = match events[0] { + Event::PaymentPathFailed { payment_hash, rejected_by_dest, all_paths_failed, ref path, ref payment_id, .. } => { assert_eq!(payment_hash, our_payment_hash); assert!(rejected_by_dest); 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); } + payment_id.unwrap() }, _ => panic!("Unexpected event"), + }; + if i == expected_paths.len() - 1 { + origin_node.node.abandon_payment(expected_payment_id); + let events = origin_node.node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + Event::PaymentFailed { ref payment_hash, ref payment_id } => { + assert_eq!(*payment_hash, our_payment_hash, "unexpected second payment_hash"); + assert_eq!(*payment_id, expected_payment_id); + } + _ => panic!("Unexpected second event"), + } } } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 1bf4f67de76..d693c41d07e 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -19,7 +19,7 @@ use chain::transaction::OutPoint; use chain::keysinterface::BaseSign; use ln::{PaymentPreimage, PaymentSecret, PaymentHash}; use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC, CONCURRENT_INBOUND_HTLC_FEE_BUFFER, FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE, MIN_AFFORDABLE_HTLC_COUNT}; -use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA}; +use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, PAYMENT_EXPIRY_BLOCKS }; use ln::channel::{Channel, ChannelError}; use ln::{chan_utils, onion_utils}; use ln::chan_utils::{HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, HTLCOutputInCommitment}; @@ -3149,9 +3149,10 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use mine_transaction(&nodes[1], &revoked_local_txn[0]); check_added_monitors!(nodes[1], 1); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + assert!(ANTI_REORG_DELAY > PAYMENT_EXPIRY_BLOCKS); // We assume payments will also expire let events = nodes[1].node.get_and_clear_pending_events(); - assert_eq!(events.len(), if deliver_bs_raa { 2 } else { 3 }); + assert_eq!(events.len(), if deliver_bs_raa { 2 } else { 4 }); match events[0] { Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => { }, _ => panic!("Unexepected event"), @@ -3164,6 +3165,12 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use } if !deliver_bs_raa { match events[2] { + Event::PaymentFailed { ref payment_hash, .. } => { + assert_eq!(*payment_hash, fourth_payment_hash); + }, + _ => panic!("Unexpected event"), + } + match events[3] { Event::PendingHTLCsForwardable { .. } => { }, _ => panic!("Unexpected event"), }; @@ -4181,7 +4188,14 @@ fn do_test_holding_cell_htlc_add_timeouts(forwarded_htlc: bool) { } expect_payment_failed_with_update!(nodes[0], second_payment_hash, false, chan_2.0.contents.short_channel_id, false); } else { - expect_payment_failed!(nodes[1], second_payment_hash, true); + let events = nodes[1].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2); + if let Event::PaymentPathFailed { ref payment_hash, .. } = events[0] { + assert_eq!(*payment_hash, second_payment_hash); + } else { panic!("Unexpected event"); } + if let Event::PaymentFailed { ref payment_hash, .. } = events[1] { + assert_eq!(*payment_hash, second_payment_hash); + } else { panic!("Unexpected event"); } } } @@ -5837,7 +5851,14 @@ fn do_htlc_claim_previous_remote_commitment_only(use_dust: bool, check_revoke_no check_added_monitors!(nodes[0], 1); check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed); } else { - expect_payment_failed!(nodes[0], our_payment_hash, true); + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2); + if let Event::PaymentPathFailed { ref payment_hash, .. } = events[0] { + assert_eq!(*payment_hash, our_payment_hash); + } else { panic!("Unexpected event"); } + if let Event::PaymentFailed { ref payment_hash, .. } = events[1] { + assert_eq!(*payment_hash, our_payment_hash); + } else { panic!("Unexpected event"); } } } diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 518ccd3b0ef..82c14645fb8 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -67,7 +67,7 @@ fn retry_single_path_payment() { check_added_monitors!(nodes[1], 1); nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], htlc_updates.commitment_signed, false); - expect_payment_failed!(nodes[0], payment_hash, false); + expect_payment_failed_conditions!(nodes[0], payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain()); // Rebalance the channel so the retry succeeds. send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); @@ -170,7 +170,7 @@ fn mpp_retry() { check_added_monitors!(nodes[2], 1); nodes[0].node.handle_update_fail_htlc(&nodes[2].node.get_our_node_id(), &htlc_updates.update_fail_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[2], htlc_updates.commitment_signed, false); - expect_payment_failed!(nodes[0], payment_hash, false); + expect_payment_failed_conditions!(nodes[0], payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain()); // Rebalance the channel so the second half of the payment can succeed. send_payment(&nodes[3], &vec!(&nodes[2])[..], 1_500_000); @@ -437,7 +437,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { confirm_transaction(&nodes[0], &as_htlc_timeout_txn[0]); } nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); - expect_payment_failed!(nodes[0], payment_hash, false); + expect_payment_failed_conditions!(nodes[0], payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain()); // Finally, retry the payment (which was reloaded from the ChannelMonitor when nodes[0] was // reloaded) via a route over the new channel, which work without issue and eventually be diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 959b180d510..5184a02031d 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -225,14 +225,20 @@ pub enum Event { /// [`Route::get_total_fees`]: crate::routing::router::Route::get_total_fees fee_paid_msat: Option, }, - /// Indicates an outbound payment we made failed. Probably some intermediary node dropped + /// Indicates an outbound HTLC we sent failed. Probably some intermediary node dropped /// something. You may wish to retry with a different route. + /// + /// Note that this does *not* indicate that all paths for an MPP payment have failed, see + /// [`Event::PaymentFailed`] and [`all_paths_failed`]. + /// + /// [`all_paths_failed`]: Self::all_paths_failed PaymentPathFailed { /// The id returned by [`ChannelManager::send_payment`] and used with - /// [`ChannelManager::retry_payment`]. + /// [`ChannelManager::retry_payment`] and [`ChannelManager::abandon_payment`]. /// /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment + /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment payment_id: Option, /// The hash that was given to [`ChannelManager::send_payment`]. /// @@ -254,6 +260,20 @@ pub enum Event { /// 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. + /// + /// Note that if you are retrying individual MPP parts, using this value to determine if a + /// payment has fully failed is race-y. Because multiple failures can happen prior to events + /// being processed, you may retry in response to a first failure, with a second failure + /// (with `all_paths_failed` set) still pending. Then, when the second failure is processed + /// you will see `all_paths_failed` set even though the retry of the first failure still + /// has an associated in-flight HTLC. See (1) for an example of such a failure. + /// + /// If you wish to retry individual MPP parts and learn when a payment has failed, you must + /// call [`ChannelManager::abandon_payment`] and wait for a [`Event::PaymentFailed`] event. + /// + /// (1) + /// + /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment all_paths_failed: bool, /// The payment path that failed. path: Vec, @@ -274,6 +294,27 @@ pub enum Event { #[cfg(test)] error_data: Option>, }, + /// Indicates an outbound payment failed. Individual [`Event::PaymentPathFailed`] events + /// provide failure information for each MPP part in the payment. + /// + /// This event is provided once there are no further pending HTLCs for the payment and the + /// payment is no longer retryable, either due to a several-block timeout or because + /// [`ChannelManager::abandon_payment`] was previously called for the corresponding payment. + /// + /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment + PaymentFailed { + /// The id returned by [`ChannelManager::send_payment`] and used with + /// [`ChannelManager::retry_payment`] and [`ChannelManager::abandon_payment`]. + /// + /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment + /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment + /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment + payment_id: PaymentId, + /// The hash that was given to [`ChannelManager::send_payment`]. + /// + /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment + payment_hash: PaymentHash, + }, /// Used to indicate that [`ChannelManager::process_pending_htlc_forwards`] should be called at /// a time in the future. /// @@ -462,6 +503,13 @@ impl Writeable for Event { (4, path, vec_type) }) }, + &Event::PaymentFailed { ref payment_id, ref payment_hash } => { + 15u8.write(writer)?; + write_tlv_fields!(writer, { + (0, payment_id, required), + (2, payment_hash, required), + }) + }, // Note that, going forward, all new events must only write data inside of // `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write // data via `write_tlv_fields`. @@ -639,6 +687,21 @@ impl MaybeReadable for Event { }; f() }, + 15u8 => { + let f = || { + let mut payment_hash = PaymentHash([0; 32]); + let mut payment_id = PaymentId([0; 32]); + read_tlv_fields!(reader, { + (0, payment_id, required), + (2, payment_hash, required), + }); + Ok(Some(Event::PaymentFailed { + payment_id, + payment_hash, + })) + }; + f() + }, // Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue. // Version 0.0.100 failed to properly ignore odd types, possibly resulting in corrupt // reads. From 3086bd8c8e3f4c1d2c5fa99279f516817ce05587 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 4 Dec 2021 23:41:37 +0000 Subject: [PATCH 4/5] Use `Event::PaymentFailed` in `InvoicePayer` to remove retry count This finally fixes the bug described in the previous commits where we retry a payment after its retry count has expired due to early removal of the payment from the retry count tracking map. A test is also added which demonstrates the bug in previous versions and which passes now. Fixes #1164. --- lightning-invoice/src/payment.rs | 201 +++++++++++++++++++++++++++++-- lightning-invoice/src/utils.rs | 4 + 2 files changed, 196 insertions(+), 9 deletions(-) diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs index e785bb39264..d08f38facd4 100644 --- a/lightning-invoice/src/payment.rs +++ b/lightning-invoice/src/payment.rs @@ -65,6 +65,7 @@ //! # fn retry_payment( //! # &self, route: &Route, payment_id: PaymentId //! # ) -> Result<(), PaymentSendFailure> { unimplemented!() } +//! # fn abandon_payment(&self, payment_id: PaymentId) { unimplemented!() } //! # } //! # //! # struct FakeRouter {} @@ -187,6 +188,9 @@ pub trait Payer { /// Retries a failed payment path for the [`PaymentId`] using the given [`Route`]. fn retry_payment(&self, route: &Route, payment_id: PaymentId) -> Result<(), PaymentSendFailure>; + + /// Signals that no further retries for the given payment will occur. + fn abandon_payment(&self, payment_id: PaymentId); } /// A trait defining behavior for routing an [`Invoice`] payment. @@ -462,26 +466,30 @@ where fn handle_event(&self, event: &Event) { match event { Event::PaymentPathFailed { - all_paths_failed, payment_id, payment_hash, rejected_by_dest, path, - short_channel_id, retry, .. + payment_id, payment_hash, rejected_by_dest, path, short_channel_id, retry, .. } => { if let Some(short_channel_id) = short_channel_id { let path = path.iter().collect::>(); self.scorer.lock().payment_path_failed(&path, *short_channel_id); } - if *rejected_by_dest { - log_trace!(self.logger, "Payment {} rejected by destination; not retrying", log_bytes!(payment_hash.0)); - } else if payment_id.is_none() { + if payment_id.is_none() { log_trace!(self.logger, "Payment {} has no id; not retrying", log_bytes!(payment_hash.0)); + } else if *rejected_by_dest { + log_trace!(self.logger, "Payment {} rejected by destination; not retrying", log_bytes!(payment_hash.0)); + self.payer.abandon_payment(payment_id.unwrap()); } else if retry.is_none() { log_trace!(self.logger, "Payment {} missing retry params; not retrying", log_bytes!(payment_hash.0)); + self.payer.abandon_payment(payment_id.unwrap()); } else if self.retry_payment(payment_id.unwrap(), *payment_hash, retry.as_ref().unwrap()).is_ok() { // We retried at least somewhat, don't provide the PaymentPathFailed event to the user. return; + } else { + self.payer.abandon_payment(payment_id.unwrap()); } - - if *all_paths_failed { self.payment_cache.lock().unwrap().remove(payment_hash); } + }, + Event::PaymentFailed { payment_hash, .. } => { + self.remove_cached_payment(&payment_hash); }, Event::PaymentPathSuccessful { path, .. } => { let path = path.iter().collect::>(); @@ -511,12 +519,12 @@ mod tests { use lightning::ln::PaymentPreimage; use lightning::ln::features::{ChannelFeatures, NodeFeatures, InitFeatures}; use lightning::ln::functional_test_utils::*; - use lightning::ln::msgs::{ErrorAction, LightningError}; + use lightning::ln::msgs::{ChannelMessageHandler, ErrorAction, LightningError}; use lightning::routing::network_graph::NodeId; use lightning::routing::router::{Payee, Route, RouteHop}; use lightning::util::test_utils::TestLogger; use lightning::util::errors::APIError; - use lightning::util::events::{Event, MessageSendEventsProvider}; + use lightning::util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider}; use secp256k1::{SecretKey, PublicKey, Secp256k1}; use std::cell::RefCell; use std::collections::VecDeque; @@ -1447,6 +1455,8 @@ mod tests { self.check_value_msats(Amount::OnRetry(route.get_total_amount())); self.check_attempts().map(|_| ()) } + + fn abandon_payment(&self, _payment_id: PaymentId) { } } // *** Full Featured Functional Tests with a Real ChannelManager *** @@ -1569,4 +1579,177 @@ mod tests { assert_eq!(htlc_msgs.len(), 2); check_added_monitors!(nodes[0], 2); } + + #[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. + // 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 + // pending which we will see later. Thus, when we previously removed the retry tracking map + // entry after a `all_paths_failed` `PaymentPathFailed` event, we may have dropped the + // retry entry even though more events for the same payment were still pending. This led to + // us retrying a payment again even though we'd already given up on it. + // + // We now have a separate event - `PaymentFailed` which indicates no HTLCs remain and which + // is used to remove the payment retry counter entries instead. This tests for the specific + // excess-retry case while also testing `PaymentFailed` generation. + + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + let chan_1_scid = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + let chan_2_scid = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000_000, 0, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; + + let mut route = Route { + paths: vec![ + vec![RouteHop { + pubkey: nodes[1].node.get_our_node_id(), + node_features: NodeFeatures::known(), + short_channel_id: chan_1_scid, + channel_features: ChannelFeatures::known(), + fee_msat: 0, + cltv_expiry_delta: 100, + }, RouteHop { + pubkey: nodes[2].node.get_our_node_id(), + node_features: NodeFeatures::known(), + short_channel_id: chan_2_scid, + channel_features: ChannelFeatures::known(), + fee_msat: 100_000_000, + cltv_expiry_delta: 100, + }], + vec![RouteHop { + pubkey: nodes[1].node.get_our_node_id(), + node_features: NodeFeatures::known(), + short_channel_id: chan_1_scid, + channel_features: ChannelFeatures::known(), + fee_msat: 0, + cltv_expiry_delta: 100, + }, RouteHop { + pubkey: nodes[2].node.get_our_node_id(), + node_features: NodeFeatures::known(), + short_channel_id: chan_2_scid, + channel_features: ChannelFeatures::known(), + fee_msat: 100_000_000, + cltv_expiry_delta: 100, + }] + ], + payee: Some(Payee::from_node_id(nodes[2].node.get_our_node_id())), + }; + let router = ManualRouter(RefCell::new(VecDeque::new())); + router.expect_find_route(Ok(route.clone())); + // On retry, we'll only be asked for one path + route.paths.remove(1); + router.expect_find_route(Ok(route.clone())); + + let expected_events: RefCell> = RefCell::new(VecDeque::new()); + let event_handler = |event: &Event| { + let event_checker = expected_events.borrow_mut().pop_front().unwrap(); + event_checker(event); + }; + let scorer = RefCell::new(TestScorer::new()); + let invoice_payer = InvoicePayer::new(nodes[0].node, router, &scorer, nodes[0].logger, event_handler, RetryAttempts(1)); + + assert!(invoice_payer.pay_invoice(&create_invoice_from_channelmanager( + &nodes[1].node, nodes[1].keys_manager, Currency::Bitcoin, Some(100_010_000), "Invoice".to_string()).unwrap()) + .is_ok()); + let htlc_updates = SendEvent::from_node(&nodes[0]); + check_added_monitors!(nodes[0], 1); + assert_eq!(htlc_updates.msgs.len(), 1); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &htlc_updates.msgs[0]); + nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &htlc_updates.commitment_msg); + check_added_monitors!(nodes[1], 1); + let (bs_first_raa, bs_first_cs) = get_revoke_commit_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + + nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_first_raa); + check_added_monitors!(nodes[0], 1); + let second_htlc_updates = SendEvent::from_node(&nodes[0]); + + nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_first_cs); + check_added_monitors!(nodes[0], 1); + let as_first_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id()); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &second_htlc_updates.msgs[0]); + nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &second_htlc_updates.commitment_msg); + check_added_monitors!(nodes[1], 1); + let bs_second_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id()); + + nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_first_raa); + check_added_monitors!(nodes[1], 1); + let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + + nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_second_raa); + check_added_monitors!(nodes[0], 1); + + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]); + nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_fail_update.commitment_signed); + check_added_monitors!(nodes[0], 1); + let (as_second_raa, as_third_cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + + nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_second_raa); + check_added_monitors!(nodes[1], 1); + let bs_second_fail_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + + nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_third_cs); + check_added_monitors!(nodes[1], 1); + let bs_third_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id()); + + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_second_fail_update.update_fail_htlcs[0]); + nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_second_fail_update.commitment_signed); + check_added_monitors!(nodes[0], 1); + + nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_third_raa); + check_added_monitors!(nodes[0], 1); + let (as_third_raa, as_fourth_cs) = get_revoke_commit_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + + nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_third_raa); + check_added_monitors!(nodes[1], 1); + nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_fourth_cs); + check_added_monitors!(nodes[1], 1); + let bs_fourth_raa = get_event_msg!(nodes[1], MessageSendEvent::SendRevokeAndACK, nodes[0].node.get_our_node_id()); + + nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_fourth_raa); + check_added_monitors!(nodes[0], 1); + + // At this point A has sent two HTLCs which both failed due to lack of fee. It now has two + // pending `PaymentPathFailed` events, one with `all_paths_failed` unset, and the second + // with it set. The first event will use up the only retry we are allowed, with the second + // `PaymentPathFailed` being passed up to the user (us, in this case). Previously, we'd + // treated this as "HTLC complete" and dropped the retry counter, causing us to retry again + // if the final HTLC failed. + expected_events.borrow_mut().push_back(&|ev: &Event| { + if let Event::PaymentPathFailed { rejected_by_dest, all_paths_failed, .. } = ev { + assert!(!rejected_by_dest); + assert!(all_paths_failed); + } else { panic!("Unexpected event"); } + }); + nodes[0].node.process_pending_events(&invoice_payer); + assert!(expected_events.borrow().is_empty()); + + let retry_htlc_updates = SendEvent::from_node(&nodes[0]); + check_added_monitors!(nodes[0], 1); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &retry_htlc_updates.msgs[0]); + commitment_signed_dance!(nodes[1], nodes[0], &retry_htlc_updates.commitment_msg, false, true); + let bs_fail_update = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_fail_update.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], &bs_fail_update.commitment_signed, false, true); + + expected_events.borrow_mut().push_back(&|ev: &Event| { + if let Event::PaymentPathFailed { rejected_by_dest, all_paths_failed, .. } = ev { + assert!(!rejected_by_dest); + assert!(all_paths_failed); + } else { panic!("Unexpected event"); } + }); + expected_events.borrow_mut().push_back(&|ev: &Event| { + if let Event::PaymentFailed { .. } = ev { + } else { panic!("Unexpected event"); } + }); + nodes[0].node.process_pending_events(&invoice_payer); + assert!(expected_events.borrow().is_empty()); + } } diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs index f4cc7e713f2..1aa3a8923d2 100644 --- a/lightning-invoice/src/utils.rs +++ b/lightning-invoice/src/utils.rs @@ -152,6 +152,10 @@ where ) -> Result<(), PaymentSendFailure> { self.retry_payment(route, payment_id) } + + fn abandon_payment(&self, payment_id: PaymentId) { + self.abandon_payment(payment_id) + } } #[cfg(test)] From 05d7a33a581076fbea1e9d626c40e0c3c44031b1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 13 Dec 2021 17:41:36 +0000 Subject: [PATCH 5/5] Make attempting to retry a succeeded payment an APIError, not Route This is symmetric with the new failure once a payment is abandoned. --- lightning/src/ln/channelmanager.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 4caa5435eac..bd3b3683e89 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2393,8 +2393,8 @@ impl ChannelMana })) }, PendingOutboundPayment::Fulfilled { .. } => { - return Err(PaymentSendFailure::ParameterError(APIError::RouteError { - err: "Payment already completed" + return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { + err: "Payment already completed".to_owned() })); }, PendingOutboundPayment::Abandoned { .. } => {