Merge pull request #2008 from valentinewallace/2023-02-remove-manual-retries

Abandon payments on behalf of the user and remove manual retries

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -914,6 +914,7 @@ pub fn do_test<Out: Output>(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

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"),
 	};

lightning/src/ln/channelmanager.rs

@@ -1190,9 +1190,9 @@ pub enum RecentPaymentDetails {
 		/// made before LDK version 0.0.104.
 		payment_hash: Option<PaymentHash>,
 	},
-	/// 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<RecentPaymentDetails> {
@@ -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<PaymentSecret>, 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));
 

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<Event>, 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<PaymentId> {
 	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)  {