Return new RetryableSendFailure on auto-retryable send error

valentinewallace · valentinewallace · commit 22698fcbbd0b · 2023-02-06T16:08:23.000-05:00
PaymentSendFailure doesn't work for auto retry send methods because they will
never return partial failure
diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs
@@ -18,7 +18,7 @@ use lightning::chain;
 use lightning::chain::chaininterface::{BroadcasterInterface, FeeEstimator};
 use lightning::chain::keysinterface::{NodeSigner, SignerProvider, EntropySource};
 use lightning::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
-use lightning::ln::channelmanager::{ChannelManager, PaymentId, PaymentSendFailure, Retry};
+use lightning::ln::channelmanager::{ChannelManager, PaymentId, Retry, RetryableSendFailure};
 use lightning::routing::router::{PaymentParameters, RouteParameters, Router};
 use lightning::util::logger::Logger;
 
@@ -249,7 +249,7 @@ pub enum PaymentError {
 	/// An error resulting from the provided [`Invoice`] or payment hash.
 	Invoice(&'static str),
 	/// An error occurring when sending a payment.
-	Sending(PaymentSendFailure),
+	Sending(RetryableSendFailure),
 }
 
 /// A trait defining behavior of an [`Invoice`] payer.
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -76,7 +76,7 @@ use core::time::Duration;
 use core::ops::Deref;
 
 // Re-export this for use in the public API.
-pub use crate::ln::outbound_payment::{PaymentSendFailure, Retry};
+pub use crate::ln::outbound_payment::{PaymentSendFailure, Retry, RetryableSendFailure};
 
 // We hold various information about HTLC relay in the HTLC objects in Channel itself:
 //
@@ -2543,7 +2543,7 @@ where
 
 	/// Similar to [`ChannelManager::send_payment`], but will automatically find a route based on
 	/// `route_params` and retry failed payment paths based on `retry_strategy`.
-	pub fn send_payment_with_retry(&self, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, payment_id: PaymentId, route_params: RouteParameters, retry_strategy: Retry) -> Result<(), PaymentSendFailure> {
+	pub fn send_payment_with_retry(&self, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, payment_id: PaymentId, route_params: RouteParameters, retry_strategy: Retry) -> Result<(), RetryableSendFailure> {
 		let best_block_height = self.best_block.read().unwrap().height();
 		self.pending_outbound_payments
 			.send_payment(payment_hash, payment_secret, payment_id, retry_strategy, route_params,
@@ -2616,7 +2616,7 @@ where
 
 	/// Similar to [`ChannelManager::send_spontaneous_payment`], but will automatically find a route
 	/// based on `route_params` and retry failed payment paths based on `retry_strategy`.
-	pub fn send_spontaneous_payment_with_retry(&self, payment_preimage: Option<PaymentPreimage>, payment_id: PaymentId, route_params: RouteParameters, retry_strategy: Retry) -> Result<PaymentHash, PaymentSendFailure> {
+	pub fn send_spontaneous_payment_with_retry(&self, payment_preimage: Option<PaymentPreimage>, payment_id: PaymentId, route_params: RouteParameters, retry_strategy: Retry) -> Result<PaymentHash, RetryableSendFailure> {
 		let best_block_height = self.best_block.read().unwrap().height();
 		self.pending_outbound_payments.send_spontaneous_payment(payment_preimage, payment_id,
 			retry_strategy, route_params, &self.router, self.list_usable_channels(),
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
@@ -313,9 +313,44 @@ impl<T: Time> Display for PaymentAttemptsUsingTime<T> {
 	}
 }
 
-/// If a payment fails to send, it can be in one of several states. This enum is returned as the
-/// Err() type describing which state the payment is in, see the description of individual enum
-/// states for more.
+/// If a payment fails to send with [`ChannelManager::send_payment_with_retry`], it can be in one of
+/// several states. This enum is returned as the Err() type describing which state the payment is
+/// in, see the description of individual enum states for more.
+///
+/// [`ChannelManager::send_payment_with_retry`]: crate::ln::channelmanager::ChannelManager::send_payment_with_retry
+#[derive(Clone, Debug)]
+pub enum RetryableSendFailure {
+	/// A parameter 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).
+	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).
+	///
+	/// The results here are ordered the same as the paths in the route object which was passed to
+	/// send_payment.
+	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).
+	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 [`Event::PaymentFailed`]).
+	///
+	/// [`PaymentId`]: crate::ln::channelmanager::PaymentId
+	/// [`Event::PaymentSent`]: crate::util::events::Event::PaymentSent
+	/// [`Event::PaymentFailed`]: crate::util::events::Event::PaymentFailed
+	DuplicatePayment,
+}
+
+/// If a payment fails to send with [`ChannelManager::send_payment`], it can be in one of several
+/// states. This enum is returned as the Err() type describing which state the payment is in, see
+/// the description of individual enum states for more.
+///
+/// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment
 #[derive(Clone, Debug)]
 pub enum PaymentSendFailure {
 	/// A parameter which was passed to send_payment was invalid, preventing us from attempting to
@@ -379,7 +414,7 @@ impl OutboundPayments {
 		retry_strategy: Retry, route_params: RouteParameters, router: &R,
 		first_hops: Vec<ChannelDetails>, inflight_htlcs: InFlightHtlcs, entropy_source: &ES,
 		node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F,
-	) -> Result<(), PaymentSendFailure>
+	) -> Result<(), RetryableSendFailure>
 	where
 		R::Target: Router,
 		ES::Target: EntropySource,
@@ -388,10 +423,18 @@ impl OutboundPayments {
 		F: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
 			 u32, PaymentId, &Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>,
 	{
-		self.pay_internal(payment_id, Some((payment_hash, payment_secret, None, retry_strategy)),
+		let res = self.pay_internal(payment_id, Some((payment_hash, payment_secret, None, retry_strategy)),
 			route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer,
 			best_block_height, logger, &send_payment_along_path)
-			.map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e })
+			.map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e });
+		match res {
+			Err(PaymentSendFailure::ParameterError(e)) => Err(RetryableSendFailure::ParameterError(e)),
+			Err(PaymentSendFailure::PathParameterError(e)) => Err(RetryableSendFailure::PathParameterError(e)),
+			Err(PaymentSendFailure::AllFailedResendSafe(e)) => Err(RetryableSendFailure::AllFailedResendSafe(e)),
+			Err(PaymentSendFailure::DuplicatePayment) => Err(RetryableSendFailure::DuplicatePayment),
+			Err(PaymentSendFailure::PartialFailure { .. }) => Ok(()),
+			Ok(()) => Ok(())
+		}
 	}
 
 	pub(super) fn send_payment_with_route<ES: Deref, NS: Deref, F>(
@@ -416,7 +459,7 @@ impl OutboundPayments {
 		retry_strategy: Retry, route_params: RouteParameters, router: &R,
 		first_hops: Vec<ChannelDetails>, inflight_htlcs: InFlightHtlcs, entropy_source: &ES,
 		node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: F
-	) -> Result<PaymentHash, PaymentSendFailure>
+	) -> Result<PaymentHash, RetryableSendFailure>
 	where
 		R::Target: Router,
 		ES::Target: EntropySource,
@@ -428,11 +471,18 @@ impl OutboundPayments {
 		let preimage = payment_preimage
 			.unwrap_or_else(|| PaymentPreimage(entropy_source.get_secure_random_bytes()));
 		let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner());
-		self.pay_internal(payment_id, Some((payment_hash, &None, Some(preimage), retry_strategy)),
+		let res = self.pay_internal(payment_id, Some((payment_hash, &None, Some(preimage), retry_strategy)),
 			route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer,
 			best_block_height, logger, &send_payment_along_path)
-			.map(|()| payment_hash)
-			.map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e })
+			.map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e });
+		match res {
+			Err(PaymentSendFailure::ParameterError(e)) => Err(RetryableSendFailure::ParameterError(e)),
+			Err(PaymentSendFailure::PathParameterError(e)) => Err(RetryableSendFailure::PathParameterError(e)),
+			Err(PaymentSendFailure::AllFailedResendSafe(e)) => Err(RetryableSendFailure::AllFailedResendSafe(e)),
+			Err(PaymentSendFailure::DuplicatePayment) => Err(RetryableSendFailure::DuplicatePayment),
+			Err(PaymentSendFailure::PartialFailure { .. }) => Ok(payment_hash),
+			Ok(()) => Ok(payment_hash),
+		}
 	}
 
 	pub(super) fn send_spontaneous_payment_with_route<ES: Deref, NS: Deref, F>(
@@ -1222,19 +1272,22 @@ mod tests {
 			final_value_msat: 0,
 			final_cltv_expiry_delta: 0,
 		};
-		let err = if on_retry {
-			outbound_payments.pay_internal(
+		if on_retry {
+			let err = outbound_payments.pay_internal(
 				PaymentId([0; 32]), None, expired_route_params, &&router, vec![], InFlightHtlcs::new(),
-				&&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err()
+				&&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err();
+			if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err {
+				assert!(err.contains("Invoice expired"));
+			} else { panic!("Unexpected error"); }
 		} else {
-			outbound_payments.send_payment(
+			let err = outbound_payments.send_payment(
 				PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), expired_route_params,
 				&&router, vec![], InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
-				|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err()
-		};
-		if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err {
-			assert!(err.contains("Invoice expired"));
-		} else { panic!("Unexpected error"); }
+				|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err();
+			if let RetryableSendFailure::ParameterError(APIError::APIMisuseError { err }) = err {
+				assert!(err.contains("Invoice expired"));
+			} else { panic!("Unexpected error"); }
+		}
 	}
 
 	#[test]
@@ -1261,22 +1314,25 @@ mod tests {
 		router.expect_find_route(route_params.clone(),
 			Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError }));
 
-		let err = if on_retry {
+		if on_retry {
 			outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), None, PaymentId([0; 32]), None,
 				&Route { paths: vec![], payment_params: None }, Some(Retry::Attempts(1)),
 				Some(route_params.payment_params.clone()), &&keys_manager, 0).unwrap();
-			outbound_payments.pay_internal(
+			let err = outbound_payments.pay_internal(
 				PaymentId([0; 32]), None, route_params, &&router, vec![], InFlightHtlcs::new(),
-				&&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err()
+				&&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err();
+			if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err {
+				assert!(err.contains("Failed to find a route"));
+			} else { panic!("Unexpected error"); }
 		} else {
-			outbound_payments.send_payment(
+			let err = outbound_payments.send_payment(
 				PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), route_params,
 				&&router, vec![], InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger,
-				|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err()
-		};
-		if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err {
-			assert!(err.contains("Failed to find a route"));
-		} else { panic!("Unexpected error"); }
+				|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err();
+			if let RetryableSendFailure::ParameterError(APIError::APIMisuseError { err }) = err {
+				assert!(err.contains("Failed to find a route"));
+			} else { panic!("Unexpected error"); }
+		}
 	}
 
 	#[test]
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
@@ -15,7 +15,7 @@ use crate::chain::{ChannelMonitorUpdateStatus, Confirm, Listen, Watch};
 use crate::chain::channelmonitor::{ANTI_REORG_DELAY, LATENCY_GRACE_PERIOD_BLOCKS};
 use crate::chain::keysinterface::EntropySource;
 use crate::chain::transaction::OutPoint;
-use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS, RecentPaymentDetails};
+use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChannelManager, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, IDEMPOTENCY_TIMEOUT_TICKS, RecentPaymentDetails, RetryableSendFailure};
 use crate::ln::features::InvoiceFeatures;
 use crate::ln::msgs;
 use crate::ln::msgs::ChannelMessageHandler;
@@ -1924,7 +1924,7 @@ fn auto_retry_zero_attempts_send_error() {
 
 	chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::PermanentFailure);
 	let err = nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap_err();
-	if let PaymentSendFailure::AllFailedResendSafe(_) = err {
+	if let RetryableSendFailure::AllFailedResendSafe(_) = err {
 	} else { panic!("Unexpected error"); }
 	assert_eq!(nodes[0].node.get_and_clear_pending_msg_events().len(), 2); // channel close messages
 	assert_eq!(nodes[0].node.get_and_clear_pending_events().len(), 1); // channel close event
