diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index a088f64f4d1..39887e83840 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -914,6 +914,7 @@ pub fn do_test(data: &[u8], underlying_out: Out) { events::Event::PaymentClaimed { .. } => {}, events::Event::PaymentPathSuccessful { .. } => {}, events::Event::PaymentPathFailed { .. } => {}, + events::Event::PaymentFailed { .. } => {}, events::Event::ProbeSuccessful { .. } | events::Event::ProbeFailed { .. } => { // Even though we don't explicitly send probes, because probes are // detected based on hashing the payment hash+preimage, its rather diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index e1ea256a661..fdb66cc4dc8 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -1752,12 +1752,18 @@ fn test_monitor_update_on_pending_forwards() { commitment_signed_dance!(nodes[0], nodes[1], bs_updates.commitment_signed, false, true); let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 2); + assert_eq!(events.len(), 3); if let Event::PaymentPathFailed { payment_hash, payment_failed_permanently, .. } = events[0] { assert_eq!(payment_hash, payment_hash_1); assert!(payment_failed_permanently); } else { panic!("Unexpected event!"); } match events[1] { + Event::PaymentFailed { payment_hash, .. } => { + assert_eq!(payment_hash, payment_hash_1); + }, + _ => panic!("Unexpected event"), + } + match events[2] { Event::PendingHTLCsForwardable { .. } => { }, _ => panic!("Unexpected event"), }; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a1148b692b1..10d2a69d20f 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1190,9 +1190,9 @@ pub enum RecentPaymentDetails { /// made before LDK version 0.0.104. payment_hash: Option, }, - /// After a payment is explicitly abandoned by calling [`ChannelManager::abandon_payment`], it - /// is marked as abandoned until an [`Event::PaymentFailed`] is generated. A payment could also - /// be marked as abandoned if pathfinding fails repeatedly or retries have been exhausted. + /// After a payment's retries are exhausted per the provided [`Retry`], or it is explicitly + /// abandoned via [`ChannelManager::abandon_payment`], it is marked as abandoned until all + /// pending HTLCs for this payment resolve and an [`Event::PaymentFailed`] is generated. Abandoned { /// Hash of the payment that we have given up trying to send. payment_hash: PaymentHash, @@ -1718,7 +1718,7 @@ where /// /// This can be useful for payments that may have been prepared, but ultimately not sent, as a /// result of a crash. If such a payment exists, is not listed here, and an - /// [`Event::PaymentSent`] has not been received, you may consider retrying the payment. + /// [`Event::PaymentSent`] has not been received, you may consider resending the payment. /// /// [`Event::PaymentSent`]: events::Event::PaymentSent pub fn list_recent_payments(&self) -> Vec { @@ -2475,8 +2475,8 @@ where /// If a pending payment is currently in-flight with the same [`PaymentId`] provided, this /// method will error with an [`APIError::InvalidRoute`]. Note, however, that once a payment /// is no longer pending (either via [`ChannelManager::abandon_payment`], or handling of an - /// [`Event::PaymentSent`]) LDK will not stop you from sending a second payment with the same - /// [`PaymentId`]. + /// [`Event::PaymentSent`] or [`Event::PaymentFailed`]) LDK will not stop you from sending a + /// second payment with the same [`PaymentId`]. /// /// Thus, in order to ensure duplicate payments are not sent, you should implement your own /// tracking of payments, including state to indicate once a payment has completed. Because you @@ -2521,6 +2521,7 @@ where /// [`Route`], we assume the invoice had the basic_mpp feature set. /// /// [`Event::PaymentSent`]: events::Event::PaymentSent + /// [`Event::PaymentFailed`]: events::Event::PaymentFailed /// [`PeerManager::process_events`]: crate::ln::peer_handler::PeerManager::process_events /// [`ChannelMonitorUpdateStatus::InProgress`]: crate::chain::ChannelMonitorUpdateStatus::InProgress pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option, payment_id: PaymentId) -> Result<(), PaymentSendFailure> { @@ -2558,48 +2559,25 @@ where } - /// Retries a payment along the given [`Route`]. + /// Signals that no further retries for the given payment should occur. Useful if you have a + /// pending outbound payment with retries remaining, but wish to stop retrying the payment before + /// retries are exhausted. /// - /// 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, 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> { - let best_block_height = self.best_block.read().unwrap().height(); - self.pending_outbound_payments.retry_payment_with_route(route, payment_id, &self.entropy_source, &self.node_signer, best_block_height, - |path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv| - self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv)) - } - - /// Signals that no further retries for the given payment will occur. - /// - /// After this method returns, no future calls to [`retry_payment`] for the given `payment_id` - /// are allowed. If no [`Event::PaymentFailed`] event had been generated before, one will be - /// generated as soon as there are no remaining pending HTLCs for this payment. + /// If no [`Event::PaymentFailed`] event had been generated before, one 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. /// /// If an [`Event::PaymentFailed`] event is generated and we restart without this - /// [`ChannelManager`] having been persisted, the payment may still be in the pending state - /// upon restart. This allows further calls to [`retry_payment`] (and requiring a second call - /// to [`abandon_payment`] to mark the payment as failed again). Otherwise, future calls to - /// [`retry_payment`] will fail with [`PaymentSendFailure::ParameterError`]. + /// [`ChannelManager`] having been persisted, another [`Event::PaymentFailed`] may be generated. /// - /// [`abandon_payment`]: Self::abandon_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); - if let Some(payment_failed_ev) = self.pending_outbound_payments.abandon_payment(payment_id) { - self.pending_events.lock().unwrap().push(payment_failed_ev); - } + self.pending_outbound_payments.abandon_payment(payment_id, &self.pending_events); } /// Send a spontaneous payment, which is a payment that does not require the recipient to have @@ -3372,7 +3350,8 @@ where let best_block_height = self.best_block.read().unwrap().height(); self.pending_outbound_payments.check_retry_payments(&self.router, || self.list_usable_channels(), - || self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height, &self.logger, + || self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height, + &self.pending_events, &self.logger, |path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv| self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv)); diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 9f888848fe9..f9a2424e080 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -1800,17 +1800,18 @@ macro_rules! expect_payment_failed { } pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>( - node: &'a Node<'b, 'c, 'd>, payment_failed_event: Event, expected_payment_hash: PaymentHash, + payment_failed_events: Vec, expected_payment_hash: PaymentHash, expected_payment_failed_permanently: bool, conditions: PaymentFailedConditions<'e> ) { - let expected_payment_id = match payment_failed_event { + 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, network_update, short_channel_id, #[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_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"); @@ -1839,7 +1840,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>( }, Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }) if chan_closed => { if let Some(scid) = conditions.expected_blamed_scid { - assert_eq!(short_channel_id, scid); + assert_eq!(*short_channel_id, scid); } assert!(is_permanent); }, @@ -1853,10 +1854,7 @@ pub fn expect_payment_failed_conditions_event<'a, 'b, 'c, 'd, 'e>( _ => 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] { + match &payment_failed_events[1] { 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); @@ -1870,9 +1868,8 @@ pub fn expect_payment_failed_conditions<'a, 'b, 'c, 'd, 'e>( node: &'a Node<'b, 'c, 'd>, expected_payment_hash: PaymentHash, expected_payment_failed_permanently: bool, conditions: PaymentFailedConditions<'e> ) { - let mut events = node.node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - expect_payment_failed_conditions_event(node, events.pop().unwrap(), expected_payment_hash, expected_payment_failed_permanently, conditions); + let events = node.node.get_and_clear_pending_events(); + expect_payment_failed_conditions_event(events, expected_payment_hash, expected_payment_failed_permanently, conditions); } 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 { @@ -2157,22 +2154,6 @@ pub fn fail_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expe } pub fn pass_failed_payment_back<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash) { - let expected_payment_id = pass_failed_payment_back_no_abandon(origin_node, expected_paths_slice, skip_last, our_payment_hash); - if !skip_last { - origin_node.node.abandon_payment(expected_payment_id.unwrap()); - 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.unwrap()); - } - _ => panic!("Unexpected second event"), - } - } -} - -pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths_slice: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_hash: PaymentHash) -> Option { let mut expected_paths: Vec<_> = expected_paths_slice.iter().collect(); check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len()); @@ -2196,8 +2177,6 @@ pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b per_path_msgs.sort_unstable_by(|(_, node_id_a), (_, node_id_b)| node_id_a.cmp(node_id_b)); expected_paths.sort_unstable_by(|path_a, path_b| path_a[path_a.len() - 2].node.get_our_node_id().cmp(&path_b[path_b.len() - 2].node.get_our_node_id())); - let mut expected_payment_id = None; - for (i, (expected_route, (path_msgs, next_hop))) in expected_paths.iter().zip(per_path_msgs.drain(..)).enumerate() { let mut next_msgs = Some(path_msgs); let mut expected_next_node = next_hop; @@ -2245,8 +2224,9 @@ pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b assert!(origin_node.node.get_and_clear_pending_msg_events().is_empty()); 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); - expected_payment_id = Some(match events[0] { + 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, .. } => { assert_eq!(payment_hash, our_payment_hash); assert!(payment_failed_permanently); @@ -2257,7 +2237,16 @@ pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b payment_id.unwrap() }, _ => panic!("Unexpected event"), - }); + }; + if i == expected_paths.len() - 1 { + match events[1] { + 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"), + } + } } } @@ -2266,8 +2255,6 @@ pub fn pass_failed_payment_back_no_abandon<'a, 'b, 'c>(origin_node: &Node<'a, 'b assert!(expected_paths[0].last().unwrap().node.get_and_clear_pending_events().is_empty()); assert!(expected_paths[0].last().unwrap().node.get_and_clear_pending_msg_events().is_empty()); check_added_monitors!(expected_paths[0].last().unwrap(), 0); - - expected_payment_id } pub fn fail_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], our_payment_hash: PaymentHash) { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 9055d4e76fe..3da49ca7dc8 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3170,7 +3170,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); let events = nodes[1].node.get_and_clear_pending_events(); - assert_eq!(events.len(), if deliver_bs_raa { 2 + nodes.len() - 1 } else { 3 + nodes.len() }); + assert_eq!(events.len(), if deliver_bs_raa { 3 + nodes.len() - 1 } else { 4 + nodes.len() }); match events[0] { Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => { }, _ => panic!("Unexepected event"), @@ -3181,20 +3181,11 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use }, _ => panic!("Unexpected event"), } - if !deliver_bs_raa { - match events[2] { - Event::PendingHTLCsForwardable { .. } => { }, - _ => panic!("Unexpected event"), - }; - nodes[1].node.abandon_payment(PaymentId(fourth_payment_hash.0)); - let payment_failed_events = nodes[1].node.get_and_clear_pending_events(); - assert_eq!(payment_failed_events.len(), 1); - match payment_failed_events[0] { - Event::PaymentFailed { ref payment_hash, .. } => { - assert_eq!(*payment_hash, fourth_payment_hash); - }, - _ => panic!("Unexpected event"), - } + match events[2] { + Event::PaymentFailed { ref payment_hash, .. } => { + assert_eq!(*payment_hash, fourth_payment_hash); + }, + _ => panic!("Unexpected event"), } nodes[1].node.process_pending_htlc_forwards(); @@ -3242,7 +3233,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use commitment_signed_dance!(nodes[0], nodes[1], commitment_signed, false, true); let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 3); + assert_eq!(events.len(), 6); match events[0] { Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => { assert!(failed_htlcs.insert(payment_hash.0)); @@ -3255,19 +3246,37 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use _ => panic!("Unexpected event"), } match events[1] { + Event::PaymentFailed { ref payment_hash, .. } => { + assert_eq!(*payment_hash, first_payment_hash); + }, + _ => panic!("Unexpected event"), + } + match events[2] { Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => { assert!(failed_htlcs.insert(payment_hash.0)); assert!(network_update.is_some()); }, _ => panic!("Unexpected event"), } - match events[2] { + match events[3] { + Event::PaymentFailed { ref payment_hash, .. } => { + assert_eq!(*payment_hash, second_payment_hash); + }, + _ => panic!("Unexpected event"), + } + match events[4] { Event::PaymentPathFailed { ref payment_hash, ref network_update, .. } => { assert!(failed_htlcs.insert(payment_hash.0)); assert!(network_update.is_some()); }, _ => panic!("Unexpected event"), } + match events[5] { + Event::PaymentFailed { ref payment_hash, .. } => { + assert_eq!(*payment_hash, third_payment_hash); + }, + _ => panic!("Unexpected event"), + } }, _ => panic!("Unexpected event"), } @@ -3354,7 +3363,7 @@ fn fail_backward_pending_htlc_upon_channel_failure() { nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &update_add_htlc); } let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 2); + assert_eq!(events.len(), 3); // Check that Alice fails backward the pending HTLC from the second payment. match events[0] { Event::PaymentPathFailed { payment_hash, .. } => { @@ -3363,6 +3372,12 @@ fn fail_backward_pending_htlc_upon_channel_failure() { _ => panic!("Unexpected event"), } match events[1] { + Event::PaymentFailed { payment_hash, .. } => { + assert_eq!(payment_hash, failed_payment_hash); + }, + _ => panic!("Unexpected event"), + } + match events[2] { Event::ChannelClosed { reason: ClosureReason::ProcessingError { ref err }, .. } => { assert_eq!(err, "Remote side tried to send a 0-msat HTLC"); }, @@ -3594,7 +3609,7 @@ fn test_simple_peer_disconnect() { reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (1, 0), (1, 0), (false, false)); { let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 3); + assert_eq!(events.len(), 4); match events[0] { Event::PaymentSent { payment_preimage, payment_hash, .. } => { assert_eq!(payment_preimage, payment_preimage_3); @@ -3610,6 +3625,12 @@ fn test_simple_peer_disconnect() { _ => panic!("Unexpected event"), } match events[2] { + Event::PaymentFailed { payment_hash, .. } => { + assert_eq!(payment_hash, payment_hash_5); + }, + _ => panic!("Unexpected event"), + } + match events[3] { Event::PaymentPathSuccessful { .. } => {}, _ => panic!("Unexpected event"), } @@ -5123,7 +5144,7 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno } let as_events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(as_events.len(), if announce_latest { 5 } else { 3 }); + assert_eq!(as_events.len(), if announce_latest { 10 } else { 6 }); let mut as_failds = HashSet::new(); let mut as_updates = 0; for event in as_events.iter() { @@ -5137,6 +5158,7 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno if network_update.is_some() { as_updates += 1; } + } else if let &Event::PaymentFailed { .. } = event { } else { panic!("Unexpected event"); } } assert!(as_failds.contains(&payment_hash_1)); @@ -5148,7 +5170,7 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno assert!(as_failds.contains(&payment_hash_6)); let bs_events = nodes[1].node.get_and_clear_pending_events(); - assert_eq!(bs_events.len(), if announce_latest { 4 } else { 3 }); + assert_eq!(bs_events.len(), if announce_latest { 8 } else { 6 }); let mut bs_failds = HashSet::new(); let mut bs_updates = 0; for event in bs_events.iter() { @@ -5162,6 +5184,7 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno if network_update.is_some() { bs_updates += 1; } + } else if let &Event::PaymentFailed { .. } = event { } else { panic!("Unexpected event"); } } assert!(bs_failds.contains(&payment_hash_1)); @@ -5670,7 +5693,7 @@ fn test_fail_holding_cell_htlc_upon_free() { // Check that the payment failed to be sent out. let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); + 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, .. } => { assert_eq!(PaymentId(our_payment_hash.0), *payment_id.as_ref().unwrap()); @@ -5682,6 +5705,12 @@ fn test_fail_holding_cell_htlc_upon_free() { }, _ => panic!("Unexpected event"), } + match &events[1] { + &Event::PaymentFailed { ref payment_hash, .. } => { + assert_eq!(our_payment_hash.clone(), *payment_hash); + }, + _ => panic!("Unexpected event"), + } } // Test that if multiple HTLCs are released from the holding cell and one is @@ -5755,7 +5784,7 @@ fn test_free_and_fail_holding_cell_htlcs() { // Check that the second payment failed to be sent out. let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); + 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, .. } => { assert_eq!(payment_id_2, *payment_id.as_ref().unwrap()); @@ -5767,6 +5796,12 @@ fn test_free_and_fail_holding_cell_htlcs() { }, _ => panic!("Unexpected event"), } + match &events[1] { + &Event::PaymentFailed { ref payment_hash, .. } => { + assert_eq!(payment_hash_2.clone(), *payment_hash); + }, + _ => panic!("Unexpected event"), + } // Complete the first payment and the RAA from the fee update. let (payment_event, send_raa_event) = { @@ -6649,7 +6684,7 @@ fn test_channel_failed_after_message_with_badonion_node_perm_bits_set() { } let events_5 = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events_5.len(), 1); + assert_eq!(events_5.len(), 2); // Expect a PaymentPathFailed event with a ChannelFailure network update for the channel between // the node originating the error to its next hop. @@ -6663,6 +6698,12 @@ fn test_channel_failed_after_message_with_badonion_node_perm_bits_set() { }, _ => panic!("Unexpected event"), } + match events_5[1] { + Event::PaymentFailed { payment_hash, .. } => { + assert_eq!(payment_hash, our_payment_hash); + }, + _ => panic!("Unexpected event"), + } // TODO: Test actual removal of channel from NetworkGraph when it's implemented. } @@ -6734,7 +6775,7 @@ fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) { connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); let events = nodes[0].node.get_and_clear_pending_events(); // Only 2 PaymentPathFailed events should show up, over-dust HTLC has to be failed by timeout tx - assert_eq!(events.len(), 2); + assert_eq!(events.len(), 4); let mut first_failed = false; for event in events { match event { @@ -6745,7 +6786,8 @@ fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) { } else { assert_eq!(payment_hash, payment_hash_2); } - } + }, + Event::PaymentFailed { .. } => {} _ => panic!("Unexpected event"), } } @@ -9034,9 +9076,11 @@ fn do_test_dup_htlc_second_rejected(test_for_second_fail_panic: bool) { commitment_signed_dance!(nodes[0], nodes[1], fail_updates_1.commitment_signed, false); let failure_events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(failure_events.len(), 2); + assert_eq!(failure_events.len(), 4); if let Event::PaymentPathFailed { .. } = failure_events[0] {} else { panic!(); } - if let Event::PaymentPathFailed { .. } = failure_events[1] {} else { panic!(); } + if let Event::PaymentFailed { .. } = failure_events[1] {} else { panic!(); } + if let Event::PaymentPathFailed { .. } = failure_events[2] {} else { panic!(); } + if let Event::PaymentFailed { .. } = failure_events[3] {} else { panic!(); } } else { // Let the second HTLC fail and claim the first expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(nodes[1], vec![HTLCDestination::FailedPayment { payment_hash: our_payment_hash }]); @@ -9047,7 +9091,7 @@ fn do_test_dup_htlc_second_rejected(test_for_second_fail_panic: bool) { nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]); commitment_signed_dance!(nodes[0], nodes[1], fail_updates_1.commitment_signed, false); - expect_payment_failed_conditions(&nodes[0], our_payment_hash, true, PaymentFailedConditions::new().mpp_parts_remain()); + expect_payment_failed_conditions(&nodes[0], our_payment_hash, true, PaymentFailedConditions::new()); claim_payment(&nodes[0], &[&nodes[1]], our_payment_preimage); } @@ -9169,7 +9213,27 @@ fn test_inconsistent_mpp_params() { assert_eq!(events.len(), 1); pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 15_000_000, our_payment_hash, Some(our_payment_secret), events.pop().unwrap(), true, None); - claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, our_payment_preimage); + do_claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, our_payment_preimage); + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 3); + match events[0] { + Event::PaymentSent { payment_hash, .. } => { // The payment was abandoned earlier, so the fee paid will be None + assert_eq!(payment_hash, our_payment_hash); + }, + _ => panic!("Unexpected event") + } + match events[1] { + Event::PaymentPathSuccessful { payment_hash, .. } => { + assert_eq!(payment_hash.unwrap(), our_payment_hash); + }, + _ => panic!("Unexpected event") + } + match events[2] { + Event::PaymentPathSuccessful { payment_hash, .. } => { + assert_eq!(payment_hash.unwrap(), our_payment_hash); + }, + _ => panic!("Unexpected event") + } } #[test] diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index 088cf8661f8..c4435b470ca 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -1241,11 +1241,10 @@ fn do_test_revoked_counterparty_commitment_balances(confirm_htlc_spend_first: bo test_spendable_output(&nodes[1], &as_revoked_txn[0]); let mut payment_failed_events = nodes[1].node.get_and_clear_pending_events(); - expect_payment_failed_conditions_event(&nodes[1], payment_failed_events.pop().unwrap(), - dust_payment_hash, false, PaymentFailedConditions::new()); - expect_payment_failed_conditions_event(&nodes[1], payment_failed_events.pop().unwrap(), + expect_payment_failed_conditions_event(payment_failed_events[..2].to_vec(), missing_htlc_payment_hash, false, PaymentFailedConditions::new()); - assert!(payment_failed_events.is_empty()); + expect_payment_failed_conditions_event(payment_failed_events[2..].to_vec(), + dust_payment_hash, false, PaymentFailedConditions::new()); connect_blocks(&nodes[1], 1); test_spendable_output(&nodes[1], &claim_txn[if confirm_htlc_spend_first { 2 } else { 3 }]); diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index fbc8e1c9e4e..bb69bbb32d5 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -166,7 +166,7 @@ fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: commitment_signed_dance!(nodes[0], nodes[1], update_1_0.commitment_signed, false, true); let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); + 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] { assert_eq!(*payment_failed_permanently, !expected_retryable); assert_eq!(*all_paths_failed, true); @@ -212,10 +212,7 @@ fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: } else { panic!("Unexpected event"); } - nodes[0].node.abandon_payment(payment_id); - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { + match events[1] { Event::PaymentFailed { payment_hash: ev_payment_hash, payment_id: ev_payment_id } => { assert_eq!(*payment_hash, ev_payment_hash); assert_eq!(payment_id, ev_payment_id); diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 31ba9cb371f..490aa90697d 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -163,13 +163,13 @@ impl PendingOutboundPayment { let our_payment_hash; core::mem::swap(&mut session_privs, match self { PendingOutboundPayment::Legacy { .. } | - PendingOutboundPayment::Fulfilled { .. } => + PendingOutboundPayment::Fulfilled { .. } => return Err(()), - PendingOutboundPayment::Retryable { session_privs, payment_hash, .. } | - PendingOutboundPayment::Abandoned { session_privs, payment_hash, .. } => { - our_payment_hash = *payment_hash; - session_privs - }, + 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(()) @@ -323,68 +323,58 @@ pub enum PaymentSendFailure { /// /// You can freely resend the payment in full (with the parameter error fixed). /// - /// Because the payment failed outright, no payment tracking is done, you do not need to call - /// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work - /// for this payment. + /// Because the payment failed outright, no payment tracking is done and no + /// [`Event::PaymentPathFailed`] or [`Event::PaymentFailed`] events will be generated. /// - /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment + /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed + /// [`Event::PaymentFailed`]: crate::util::events::Event::PaymentFailed ParameterError(APIError), /// A parameter in a single path which was passed to send_payment was invalid, preventing us /// from attempting to send the payment at all. /// /// You can freely resend the payment in full (with the parameter error fixed). /// + /// Because the payment failed outright, no payment tracking is done and no + /// [`Event::PaymentPathFailed`] or [`Event::PaymentFailed`] events will be generated. + /// /// The results here are ordered the same as the paths in the route object which was passed to /// send_payment. /// - /// Because the payment failed outright, no payment tracking is done, you do not need to call - /// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work - /// for this payment. - /// - /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment + /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed + /// [`Event::PaymentFailed`]: crate::util::events::Event::PaymentFailed PathParameterError(Vec>), /// All paths which were attempted failed to send, with no channel state change taking place. /// You can freely resend the payment in full (though you probably want to do so over different /// paths than the ones selected). /// - /// Because the payment failed outright, no payment tracking is done, you do not need to call - /// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work - /// for this payment. + /// Because the payment failed outright, no payment tracking is done and no + /// [`Event::PaymentPathFailed`] or [`Event::PaymentFailed`] events will be generated. /// - /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment + /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed + /// [`Event::PaymentFailed`]: crate::util::events::Event::PaymentFailed AllFailedResendSafe(Vec), /// Indicates that a payment for the provided [`PaymentId`] is already in-flight and has not - /// yet completed (i.e. generated an [`Event::PaymentSent`]) or been abandoned (via - /// [`ChannelManager::abandon_payment`]). + /// yet completed (i.e. generated an [`Event::PaymentSent`] or [`Event::PaymentFailed`]). /// /// [`PaymentId`]: crate::ln::channelmanager::PaymentId /// [`Event::PaymentSent`]: crate::util::events::Event::PaymentSent - /// [`ChannelManager::abandon_payment`]: crate::ln::channelmanager::ChannelManager::abandon_payment + /// [`Event::PaymentFailed`]: crate::util::events::Event::PaymentFailed DuplicatePayment, - /// Some paths which were attempted failed to send, though possibly not all. At least some - /// paths have irrevocably committed to the HTLC and retrying the payment in full would result - /// in over-/re-payment. + /// Some paths that were attempted failed to send, though some paths may have succeeded. At least + /// some paths have irrevocably committed to the HTLC. /// - /// The results here are ordered the same as the paths in the route object which was passed to - /// send_payment, and any `Err`s which are not [`APIError::MonitorUpdateInProgress`] can be - /// safely retried via [`ChannelManager::retry_payment`]. + /// The results here are ordered the same as the paths in the route object that was passed to + /// send_payment. /// - /// Any entries which contain `Err(APIError::MonitorUpdateInprogress)` or `Ok(())` MUST NOT be - /// retried as they will result in over-/re-payment. These HTLCs all either successfully sent - /// (in the case of `Ok(())`) or will send once a [`MonitorEvent::Completed`] is provided for - /// the next-hop channel with the latest update_id. + /// Any entries that contain `Err(APIError::MonitorUpdateInprogress)` will send once a + /// [`MonitorEvent::Completed`] is provided for the next-hop channel with the latest update_id. /// - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment /// [`MonitorEvent::Completed`]: crate::chain::channelmonitor::MonitorEvent::Completed PartialFailure { - /// The errors themselves, in the same order as the route hops. + /// The errors themselves, in the same order as the paths from the route. results: Vec>, /// If some paths failed without irrevocably committing to the new HTLC(s), this will - /// contain a [`RouteParameters`] object which can be used to calculate a new route that - /// will pay all remaining unpaid balance. + /// contain a [`RouteParameters`] object for the failing paths. failed_paths_retry: Option, /// The payment id for the payment, which is now at least partially pending. payment_id: PaymentId, @@ -491,7 +481,8 @@ impl OutboundPayments { pub(super) fn check_retry_payments( &self, router: &R, first_hops: FH, inflight_htlcs: IH, entropy_source: &ES, node_signer: &NS, - best_block_height: u32, logger: &L, send_payment_along_path: SP, + best_block_height: u32, pending_events: &Mutex>, logger: &L, + send_payment_along_path: SP, ) where R::Target: Router, @@ -525,15 +516,34 @@ impl OutboundPayments { } } } + core::mem::drop(outbounds); if let Some((payment_id, route_params)) = retry_id_route_params { - core::mem::drop(outbounds); if let Err(e) = self.pay_internal(payment_id, None, route_params, router, first_hops(), &inflight_htlcs, entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) { log_info!(logger, "Errored retrying payment: {:?}", e); + // If we error on retry, there is no chance of the payment succeeding and no HTLCs have + // been irrevocably committed to, so we can safely abandon. + self.abandon_payment(payment_id, pending_events); } } else { break } } + + let mut outbounds = self.pending_outbound_payments.lock().unwrap(); + outbounds.retain(|pmt_id, pmt| { + let mut retain = true; + if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 { + if pmt.mark_abandoned().is_ok() { + pending_events.lock().unwrap().push(events::Event::PaymentFailed { + payment_id: *pmt_id, + payment_hash: pmt.payment_hash().expect("PendingOutboundPayments::Retryable always has a payment hash set"), + }); + retain = false; + } + } + retain + }); } + /// Will return `Ok(())` iff at least one HTLC is sent for the payment. fn pay_internal( &self, payment_id: PaymentId, initial_send_info: Option<(PaymentHash, &Option, Option, Retry)>, @@ -876,8 +886,8 @@ impl OutboundPayments { .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } - // If we failed to send any paths, we should remove the new PaymentId from the - // `pending_outbound_payments` map, as the user isn't expected to `abandon_payment`. + // If we failed to send any paths, remove the new PaymentId from the `pending_outbound_payments` + // map as the payment is free to be resent. fn remove_outbound_if_all_failed(&self, payment_id: PaymentId, err: &PaymentSendFailure) { if let &PaymentSendFailure::AllFailedResendSafe(_) = err { let removed = self.pending_outbound_payments.lock().unwrap().remove(&payment_id).is_some(); @@ -1004,6 +1014,7 @@ impl OutboundPayments { #[cfg(not(test))] let (network_update, short_channel_id, payment_retryable, _, _) = onion_error.decode_onion_failure(secp_ctx, logger, &source); + let payment_is_probe = payment_is_probe(payment_hash, &payment_id, probing_cookie_secret); let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); @@ -1020,7 +1031,7 @@ impl OutboundPayments { log_trace!(logger, "Received failure of HTLC with payment_hash {} after payment completion", log_bytes!(payment_hash.0)); return } - let is_retryable_now = payment.get().is_auto_retryable_now(); + let mut is_retryable_now = payment.get().is_auto_retryable_now(); if let Some(scid) = short_channel_id { payment.get_mut().insert_previously_failed_scid(scid); } @@ -1051,6 +1062,10 @@ impl OutboundPayments { }); } + if !payment_is_probe && (!is_retryable_now || !payment_retryable || retry.is_none()) { + let _ = payment.get_mut().mark_abandoned(); // we'll only Err if it's a legacy payment + is_retryable_now = false; + } if payment.get().remaining_parts() == 0 { all_paths_failed = true; if payment.get().abandoned() { @@ -1070,7 +1085,7 @@ impl OutboundPayments { log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0)); let path_failure = { - if payment_is_probe(payment_hash, &payment_id, probing_cookie_secret) { + if payment_is_probe { if !payment_retryable { events::Event::ProbeSuccessful { payment_id: *payment_id, @@ -1092,7 +1107,9 @@ impl OutboundPayments { if let Some(scid) = short_channel_id { retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid)); } - if payment_retryable && attempts_remaining && retry.is_some() { + // 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 { debug_assert!(full_failure_ev.is_none()); pending_retry_ev = Some(events::Event::PendingHTLCsForwardable { time_forwardable: Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS), @@ -1120,13 +1137,14 @@ impl OutboundPayments { if let Some(ev) = pending_retry_ev { pending_events.push(ev); } } - pub(super) fn abandon_payment(&self, payment_id: PaymentId) -> Option { - let mut failed_ev = None; + pub(super) fn abandon_payment( + &self, payment_id: PaymentId, pending_events: &Mutex> + ) { let mut outbounds = self.pending_outbound_payments.lock().unwrap(); if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) { if let Ok(()) = payment.get_mut().mark_abandoned() { if payment.get().remaining_parts() == 0 { - failed_ev = Some(events::Event::PaymentFailed { + 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"), }); @@ -1134,7 +1152,6 @@ impl OutboundPayments { } } } - failed_ev } #[cfg(test)] diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index b4cb379c4ad..a433f796a3f 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -43,60 +43,6 @@ use { std::time::{SystemTime, Duration} }; -#[test] -fn retry_single_path_payment() { - 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 mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); - - let _chan_0 = create_announced_chan_between_nodes(&nodes, 0, 1); - let chan_1 = create_announced_chan_between_nodes(&nodes, 2, 1); - // Rebalance to find a route - send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); - - let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 100_000); - - // Rebalance so that the first hop fails. - send_payment(&nodes[1], &vec!(&nodes[2])[..], 2_000_000); - - // Make sure the payment fails on the first hop. - let payment_id = PaymentId(payment_hash.0); - nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), payment_id).unwrap(); - check_added_monitors!(nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - let mut payment_event = SendEvent::from_event(events.pop().unwrap()); - nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); - check_added_monitors!(nodes[1], 0); - commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); - expect_pending_htlcs_forwardable!(nodes[1]); - expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[1], vec![HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1.2 }]); - let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); - assert!(htlc_updates.update_add_htlcs.is_empty()); - assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); - assert!(htlc_updates.update_fulfill_htlcs.is_empty()); - assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); - 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_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); - - // Mine two blocks (we expire retries after 3, so this will check that we don't expire early) - connect_blocks(&nodes[0], 2); - - // Retry the payment and make sure it succeeds. - nodes[0].node.retry_payment(&route, payment_id).unwrap(); - check_added_monitors!(nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - pass_along_path(&nodes[0], &[&nodes[1], &nodes[2]], 100_000, payment_hash, Some(payment_secret), events.pop().unwrap(), true, None); - claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage); -} - #[test] fn mpp_failure() { let chanmon_cfgs = create_chanmon_cfgs(4); @@ -136,7 +82,8 @@ fn mpp_retry() { // Rebalance send_payment(&nodes[3], &vec!(&nodes[2])[..], 1_500_000); - let (mut route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[3], 1_000_000); + let amt_msat = 1_000_000; + let (mut route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[3], amt_msat); let path = route.paths[0].clone(); route.paths.push(path); route.paths[0][0].pubkey = nodes[1].node.get_our_node_id(); @@ -148,7 +95,14 @@ fn mpp_retry() { // Initiate the MPP payment. let payment_id = PaymentId(payment_hash.0); - nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), payment_id).unwrap(); + let mut route_params = RouteParameters { + payment_params: route.payment_params.clone().unwrap(), + final_value_msat: amt_msat, + final_cltv_expiry_delta: TEST_FINAL_CLTV, + }; + + nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone())); + nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), payment_id, route_params.clone(), Retry::Attempts(1)).unwrap(); check_added_monitors!(nodes[0], 2); // one monitor per path let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 2); @@ -184,25 +138,23 @@ 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_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain()); + let mut events = nodes[0].node.get_and_clear_pending_events(); + match events[1] { + Event::PendingHTLCsForwardable { .. } => {}, + _ => panic!("Unexpected event") + } + events.remove(1); + expect_payment_failed_conditions_event(events, 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); - // Make sure it errors as expected given a too-large amount. - if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, payment_id) { - assert!(err.contains("over total_payment_amt_msat")); - } else { panic!("Unexpected error"); } - - // Make sure it errors as expected given the wrong payment_id. - if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, PaymentId([0; 32])) { - assert!(err.contains("not found")); - } else { panic!("Unexpected error"); } - // Retry the second half of the payment and make sure it succeeds. - let mut path = route.clone(); - path.paths.remove(0); - nodes[0].node.retry_payment(&path, payment_id).unwrap(); + route.paths.remove(0); + route_params.final_value_msat = 1_000_000; + route_params.payment_params.previously_failed_channels.push(chan_4_update.contents.short_channel_id); + nodes[0].router.expect_find_route(route_params, Ok(route)); + nodes[0].node.process_pending_htlc_forwards(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); @@ -284,55 +236,6 @@ fn mpp_receive_timeout() { do_mpp_receive_timeout(false); } -#[test] -fn retry_expired_payment() { - 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 mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); - - let _chan_0 = create_announced_chan_between_nodes(&nodes, 0, 1); - let chan_1 = create_announced_chan_between_nodes(&nodes, 2, 1); - // Rebalance to find a route - send_payment(&nodes[2], &vec!(&nodes[1])[..], 3_000_000); - - let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 100_000); - - // Rebalance so that the first hop fails. - send_payment(&nodes[1], &vec!(&nodes[2])[..], 2_000_000); - - // Make sure the payment fails on the first hop. - nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap(); - check_added_monitors!(nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 1); - let mut payment_event = SendEvent::from_event(events.pop().unwrap()); - nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); - check_added_monitors!(nodes[1], 0); - commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); - expect_pending_htlcs_forwardable!(nodes[1]); - expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[1], vec![HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1.2 }]); - let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); - assert!(htlc_updates.update_add_htlcs.is_empty()); - assert_eq!(htlc_updates.update_fail_htlcs.len(), 1); - assert!(htlc_updates.update_fulfill_htlcs.is_empty()); - assert!(htlc_updates.update_fail_malformed_htlcs.is_empty()); - 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); - - // Mine blocks so the payment will have expired. - connect_blocks(&nodes[0], 3); - - // Retry the payment and make sure it errors as expected. - if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = nodes[0].node.retry_payment(&route, PaymentId(payment_hash.0)) { - assert!(err.contains("not found")); - } else { - panic!("Unexpected error"); - } -} - #[test] fn no_pending_leak_on_initial_send_failure() { // In an earlier version of our payment tracking, we'd have a retry entry even when the initial @@ -388,9 +291,15 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { // Send two payments - one which will get to nodes[2] and will be claimed, one which we'll time // out and retry. - let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 1_000_000); + let amt_msat = 1_000_000; + let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], amt_msat); let (payment_preimage_1, payment_hash_1, _, payment_id_1) = send_along_route(&nodes[0], route.clone(), &[&nodes[1], &nodes[2]], 1_000_000); - nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap(); + let route_params = RouteParameters { + payment_params: route.payment_params.clone().unwrap(), + final_value_msat: amt_msat, + final_cltv_expiry_delta: TEST_FINAL_CLTV, + }; + nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); @@ -499,7 +408,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { confirm_transaction(&nodes[0], &first_htlc_timeout_tx); } nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); - expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain()); + expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new()); // 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 @@ -525,8 +434,8 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { nodes[1].node.timer_tick_occurred(); } - assert!(nodes[0].node.retry_payment(&new_route, payment_id_1).is_err()); // Shouldn't be allowed to retry a fulfilled payment - nodes[0].node.retry_payment(&new_route, PaymentId(payment_hash.0)).unwrap(); + assert!(nodes[0].node.send_payment(&new_route, payment_hash, &Some(payment_secret), payment_id_1).is_err()); // Shouldn't be allowed to retry a fulfilled payment + nodes[0].node.send_payment(&new_route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap(); check_added_monitors!(nodes[0], 1); let mut events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 1); @@ -660,7 +569,10 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { // If we attempt to retry prior to the HTLC-Timeout (or commitment transaction, for dust HTLCs) // confirming, we will fail as it's considered still-pending... let (new_route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[2], if use_dust { 1_000 } else { 1_000_000 }); - assert!(nodes[0].node.retry_payment(&new_route, payment_id).is_err()); + match nodes[0].node.send_payment(&new_route, payment_hash, &Some(payment_secret), payment_id) { + Err(PaymentSendFailure::DuplicatePayment) => {}, + _ => panic!("Unexpected error") + } assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); // After ANTI_REORG_DELAY confirmations, the HTLC should be failed and we can try the payment @@ -668,14 +580,14 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { // (which should also still work). connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); - // We set mpp_parts_remain to avoid having abandon_payment called - expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new().mpp_parts_remain()); + expect_payment_failed_conditions(&nodes[0], payment_hash, false, PaymentFailedConditions::new()); let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); let chan_1_monitor_serialized = get_monitor!(nodes[0], chan_id_3).encode(); nodes_0_serialized = nodes[0].node.encode(); - assert!(nodes[0].node.retry_payment(&new_route, payment_id).is_ok()); + // After the payment failed, we're free to send it again. + assert!(nodes[0].node.send_payment(&new_route, payment_hash, &Some(payment_secret), payment_id).is_ok()); assert!(!nodes[0].node.get_and_clear_pending_msg_events().is_empty()); reload_node!(nodes[0], test_default_channel_config(), nodes_0_serialized, &[&chan_0_monitor_serialized, &chan_1_monitor_serialized], second_persister, second_new_chain_monitor, second_nodes_0_deserialized); @@ -685,25 +597,32 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { // Now resend the payment, delivering the HTLC and actually claiming it this time. This ensures // the payment is not (spuriously) listed as still pending. - assert!(nodes[0].node.retry_payment(&new_route, payment_id).is_ok()); + assert!(nodes[0].node.send_payment(&new_route, payment_hash, &Some(payment_secret), payment_id).is_ok()); check_added_monitors!(nodes[0], 1); pass_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], if use_dust { 1_000 } else { 1_000_000 }, payment_hash, payment_secret); claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage); - assert!(nodes[0].node.retry_payment(&new_route, payment_id).is_err()); + match nodes[0].node.send_payment(&new_route, payment_hash, &Some(payment_secret), payment_id) { + Err(PaymentSendFailure::DuplicatePayment) => {}, + _ => panic!("Unexpected error") + } assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); let chan_0_monitor_serialized = get_monitor!(nodes[0], chan_id).encode(); let chan_1_monitor_serialized = get_monitor!(nodes[0], chan_id_3).encode(); nodes_0_serialized = nodes[0].node.encode(); - // Ensure that after reload we cannot retry the payment. + // Check that after reload we can send the payment again (though we shouldn't, since it was + // claimed previously). reload_node!(nodes[0], test_default_channel_config(), nodes_0_serialized, &[&chan_0_monitor_serialized, &chan_1_monitor_serialized], third_persister, third_new_chain_monitor, third_nodes_0_deserialized); nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false); reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false)); - assert!(nodes[0].node.retry_payment(&new_route, payment_id).is_err()); + match nodes[0].node.send_payment(&new_route, payment_hash, &Some(payment_secret), payment_id) { + Err(PaymentSendFailure::DuplicatePayment) => {}, + _ => panic!("Unexpected error") + } assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty()); } @@ -1233,20 +1152,17 @@ fn abandoned_send_payment_idempotent() { nodes[1].node.fail_htlc_backwards(&first_payment_hash); expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], [HTLCDestination::FailedPayment { payment_hash: first_payment_hash }]); - pass_failed_payment_back_no_abandon(&nodes[0], &[&[&nodes[1]]], false, first_payment_hash); - check_send_rejected!(); - - // Until we abandon the payment, no matter how many timer ticks pass, we still cannot reuse the + // Until we abandon the payment upon path failure, no matter how many timer ticks pass, we still cannot reuse the // PaymentId. for _ in 0..=IDEMPOTENCY_TIMEOUT_TICKS { nodes[0].node.timer_tick_occurred(); } check_send_rejected!(); - nodes[0].node.abandon_payment(payment_id); - get_event!(nodes[0], Event::PaymentFailed); + pass_failed_payment_back(&nodes[0], &[&[&nodes[1]]], false, first_payment_hash); - // However, we can reuse the PaymentId immediately after we `abandon_payment`. + // However, we can reuse the PaymentId immediately after we `abandon_payment` upon passing the + // failed payment back. nodes[0].node.send_payment(&route, second_payment_hash, &Some(second_payment_secret), payment_id).unwrap(); check_added_monitors!(nodes[0], 1); pass_along_route(&nodes[0], &[&[&nodes[1]]], 100_000, second_payment_hash, second_payment_secret); @@ -1615,6 +1531,7 @@ enum AutoRetry { FailAttempts, FailTimeout, FailOnRestart, + FailOnRetry, } #[test] @@ -1624,6 +1541,7 @@ fn automatic_retries() { do_automatic_retries(AutoRetry::FailAttempts); do_automatic_retries(AutoRetry::FailTimeout); do_automatic_retries(AutoRetry::FailOnRestart); + do_automatic_retries(AutoRetry::FailOnRetry); } fn do_automatic_retries(test: AutoRetry) { // Test basic automatic payment retries in ChannelManager. See individual `test` variant comments @@ -1680,12 +1598,12 @@ fn do_automatic_retries(test: AutoRetry) { check_added_monitors!(&nodes[1], 1); assert!(update_1.update_fail_htlcs.len() == 1); let fail_msg = update_1.update_fail_htlcs[0].clone(); - nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg); commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false); // Ensure the attempt fails and a new PendingHTLCsForwardable event is generated for the retry let mut events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 2); match events[0] { Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently, .. } => { assert_eq!(payment_hash, ev_payment_hash); @@ -1694,12 +1612,18 @@ fn do_automatic_retries(test: AutoRetry) { _ => panic!("Unexpected event"), } if $expect_pending_htlcs_forwardable { - assert_eq!(events.len(), 2); match events[1] { Event::PendingHTLCsForwardable { .. } => {}, _ => panic!("Unexpected event"), } - } else { assert_eq!(events.len(), 1) } + } else { + match events[1] { + Event::PaymentFailed { payment_hash: ev_payment_hash, .. } => { + assert_eq!(payment_hash, ev_payment_hash); + }, + _ => panic!("Unexpected event"), + } + } } } @@ -1751,17 +1675,6 @@ fn do_automatic_retries(test: AutoRetry) { nodes[0].node.process_pending_htlc_forwards(); let mut msg_events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), 0); - - nodes[0].node.abandon_payment(PaymentId(payment_hash.0)); - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentFailed { payment_hash: ref ev_payment_hash, payment_id: ref ev_payment_id } => { - assert_eq!(payment_hash, *ev_payment_hash); - assert_eq!(PaymentId(payment_hash.0), *ev_payment_id); - }, - _ => panic!("Unexpected event"), - } } else if test == AutoRetry::FailTimeout { #[cfg(not(feature = "no-std"))] { // Ensure ChannelManager will not retry a payment if it times out due to Retry::Timeout. @@ -1776,7 +1689,6 @@ fn do_automatic_retries(test: AutoRetry) { let mut msg_events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), 0); - nodes[0].node.abandon_payment(PaymentId(payment_hash.0)); let mut events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { @@ -1811,7 +1723,25 @@ fn do_automatic_retries(test: AutoRetry) { let mut msg_events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), 0); - nodes[0].node.abandon_payment(PaymentId(payment_hash.0)); + let mut events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + Event::PaymentFailed { payment_hash: ref ev_payment_hash, payment_id: ref ev_payment_id } => { + assert_eq!(payment_hash, *ev_payment_hash); + assert_eq!(PaymentId(payment_hash.0), *ev_payment_id); + }, + _ => panic!("Unexpected event"), + } + } else if test == AutoRetry::FailOnRetry { + nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap(); + pass_failed_attempt_with_retry_along_path!(channel_id_2, true); + + // We retry payments in `process_pending_htlc_forwards`. Since our channel closed, we should + // fail to find a route. + nodes[0].node.process_pending_htlc_forwards(); + let mut msg_events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 0); + let mut events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); match events[0] { @@ -2413,7 +2343,7 @@ fn no_extra_retries_on_back_to_back_fail() { // by adding the `PaymentFailed` event. // // Because we now retry payments as a batch, we simply return a single-path route in the - // second, batched, request, have that fail, then complete the payment via `abandon_payment`. + // second, batched, request, have that fail, ensure the payment was abandoned. let mut events = nodes[0].node.get_and_clear_pending_events(); assert_eq!(events.len(), 4); match events[0] { @@ -2450,7 +2380,7 @@ fn no_extra_retries_on_back_to_back_fail() { commitment_signed_dance!(nodes[0], nodes[1], &bs_fail_update.commitment_signed, false, true); let mut events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); + assert_eq!(events.len(), 2); match events[0] { Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently, .. } => { assert_eq!(payment_hash, ev_payment_hash); @@ -2458,10 +2388,7 @@ fn no_extra_retries_on_back_to_back_fail() { }, _ => panic!("Unexpected event"), } - nodes[0].node.abandon_payment(PaymentId(payment_hash.0)); - events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { + match events[1] { Event::PaymentFailed { payment_hash: ref ev_payment_hash, payment_id: ref ev_payment_id } => { assert_eq!(payment_hash, *ev_payment_hash); assert_eq!(PaymentId(payment_hash.0), *ev_payment_id); diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index cfee77a67a2..b4e2b138433 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -333,7 +333,7 @@ impl Readable for Route { /// Parameters needed to find a [`Route`]. /// /// Passed to [`find_route`] and [`build_route_from_hops`], but also provided in -/// [`Event::PaymentPathFailed`] for retrying a failed payment path. +/// [`Event::PaymentPathFailed`]. /// /// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 265e9186611..4d68d01f6ed 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -565,11 +565,9 @@ pub enum Event { /// Note for MPP payments: in rare cases, this event may be preceded by a `PaymentPathFailed` /// event. In this situation, you SHOULD treat this payment as having succeeded. PaymentSent { - /// The id returned by [`ChannelManager::send_payment`] and used with - /// [`ChannelManager::retry_payment`]. + /// The id returned by [`ChannelManager::send_payment`]. /// /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment payment_id: Option, /// The preimage to the hash given to ChannelManager::send_payment. /// Note that this serves as a payment receipt, if you wish to have such a thing, you must @@ -594,16 +592,16 @@ pub enum Event { /// 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 due to [`ChannelManager::abandon_payment`] having been - /// called for the corresponding payment. + /// payment is no longer retryable, due either to the [`Retry`] provided or + /// [`ChannelManager::abandon_payment`] having been called for the corresponding payment. /// + /// [`Retry`]: crate::ln::channelmanager::Retry /// [`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::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`]. @@ -616,11 +614,9 @@ pub enum Event { /// Always generated after [`Event::PaymentSent`] and thus useful for scoring channels. See /// [`Event::PaymentSent`] for obtaining the payment preimage. PaymentPathSuccessful { - /// The id returned by [`ChannelManager::send_payment`] and used with - /// [`ChannelManager::retry_payment`]. + /// The id returned by [`ChannelManager::send_payment`]. /// /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment - /// [`ChannelManager::retry_payment`]: crate::ln::channelmanager::ChannelManager::retry_payment payment_id: PaymentId, /// The hash that was given to [`ChannelManager::send_payment`]. /// @@ -631,24 +627,22 @@ pub enum Event { /// May contain a closed channel if the HTLC sent along the path was fulfilled on chain. path: Vec, }, - /// Indicates an outbound HTLC we sent failed. Probably some intermediary node dropped - /// something. You may wish to retry with a different route. - /// - /// If you have given up retrying this payment and wish to fail it, you MUST call - /// [`ChannelManager::abandon_payment`] at least once for a given [`PaymentId`] or memory - /// related to payment tracking will leak. + /// Indicates an outbound HTLC we sent failed, likely due to an intermediary node being unable to + /// 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`]. /// + /// 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::retry_payment`] and [`ChannelManager::abandon_payment`]. + /// [`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`]. @@ -656,8 +650,8 @@ pub enum Event { /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment payment_hash: PaymentHash, /// Indicates the payment was rejected for some reason by the recipient. This implies that - /// the payment has failed, not just the route in question. If this is not set, you may - /// retry the payment via a different route. + /// the payment has failed, not just the route in question. If this is not set, the payment may + /// be retried via a different route. payment_failed_permanently: bool, /// Any failure information conveyed via the Onion return packet by a node along the failed /// payment route. @@ -670,20 +664,6 @@ 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, @@ -696,12 +676,9 @@ 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 needed to compute a new [`Route`] when retrying the failed payment path. - /// - /// See [`find_route`] for details. + /// Parameters used by LDK to compute a new [`Route`] when retrying the failed payment path. /// /// [`Route`]: crate::routing::router::Route - /// [`find_route`]: crate::routing::router::find_route retry: Option, #[cfg(test)] error_code: Option,