Remove retry_payments method

We're no longer supporting manual retries since
ChannelManager::send_payment_with_retry can be parameterized by a retry
strategy

This commit also updates all docs related to retry_payment and abandon_payment.
Since these docs frequently overlap with changes in preceding commits where we
start abandoning payments on behalf of the user, all the docs are updated in
one go.

Author: valentinewallace

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,41 +2559,20 @@ where
 	}
 
 
-	/// Retries a payment along the given [`Route`].
-	///
-	/// 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.
+	/// 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.
 	///
-	/// 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) {

lightning/src/ln/outbound_payment.rs

@@ -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<Result<(), APIError>>),
 	/// 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<APIError>),
 	/// 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.
+	/// 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`].
+	/// 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 which 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<Result<(), APIError>>,
 		/// 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<RouteParameters>,
 		/// The payment id for the payment, which is now at least partially pending.
 		payment_id: PaymentId,
@@ -896,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();

lightning/src/ln/payment_tests.rs

@@ -236,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
@@ -618,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
@@ -648,7 +602,10 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
 	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.send_payment(&new_route, payment_hash, &Some(payment_secret), 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();
@@ -662,7 +619,10 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
 
 	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());
 }
 

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)]