Add a separate PaymentSendFailure for idempotency violation

When a user attempts to send a payment but it fails due to
idempotency key violation, they need to know that this was the
reason as they need to handle the error programmatically
differently from other errors.

Here we simply add a new `PaymentSendFailure` enum variant for
`DuplicatePayment` to allow for that.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -289,6 +289,7 @@ fn check_payment_err(send_err: PaymentSendFailure) {
 		PaymentSendFailure::PartialFailure { results, .. } => {
 			for res in results { if let Err(api_err) = res { check_api_err(api_err); } }
 		},
+		PaymentSendFailure::DuplicatePayment => panic!(),
 	}
 }
 

lightning-invoice/src/payment.rs

@@ -543,6 +543,7 @@ where
 			Err(e) => match e {
 				PaymentSendFailure::ParameterError(_) => Err(e),
 				PaymentSendFailure::PathParameterError(_) => Err(e),
+				PaymentSendFailure::DuplicatePayment => Err(e),
 				PaymentSendFailure::AllFailedResendSafe(_) => {
 					let mut payment_cache = self.payment_cache.lock().unwrap();
 					let payment_info = payment_cache.get_mut(&payment_hash).unwrap();
@@ -658,6 +659,10 @@ where
 			Err(PaymentSendFailure::AllFailedResendSafe(_)) => {
 				self.retry_payment(payment_id, payment_hash, params)
 			},
+			Err(PaymentSendFailure::DuplicatePayment) => {
+				log_error!(self.logger, "Got a DuplicatePayment error when attempting to retry a payment, this shouldn't happen.");
+				Err(())
+			}
 			Err(PaymentSendFailure::PartialFailure { failed_paths_retry, results, .. }) => {
 				// If a `PartialFailure` error contains a result that is an `Ok()`, it means that
 				// part of our payment is retried. When we receive `MonitorUpdateInProgress`, it

lightning/src/ln/channelmanager.rs

@@ -1188,16 +1188,25 @@ impl ChannelDetails {
 #[derive(Clone, Debug)]
 pub enum PaymentSendFailure {
 	/// A parameter which was passed to send_payment was invalid, preventing us from attempting to
-	/// send the payment at all. No channel state has been changed or messages sent to peers, and
-	/// once you've changed the parameter at error, you can freely retry the payment in full.
+	/// 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, you do not need to call
+	/// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work
+	/// for this payment.
 	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. No channel state has been changed or messages
-	/// sent to peers, and once you've changed the parameter at error, you can freely retry the
-	/// payment in full.
+	/// 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.
+	///
+	/// 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.
 	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
@@ -1207,6 +1216,12 @@ pub enum PaymentSendFailure {
 	/// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work
 	/// for this payment.
 	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`]).
+	///
+	/// [`Event::PaymentSent`]: events::Event::PaymentSent
+	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.
@@ -2611,9 +2626,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
 		let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
 		match pending_outbounds.entry(payment_id) {
-			hash_map::Entry::Occupied(_) => Err(PaymentSendFailure::ParameterError(APIError::RouteError {
-				err: "Payment already in progress"
-			})),
+			hash_map::Entry::Occupied(_) => Err(PaymentSendFailure::DuplicatePayment),
 			hash_map::Entry::Vacant(entry) => {
 				let payment = entry.insert(PendingOutboundPayment::Retryable {
 					session_privs: HashSet::new(),

lightning/src/ln/payment_tests.rs

@@ -1275,15 +1275,15 @@ fn claimed_send_payment_idempotent() {
 			// payment_id, it should be rejected.
 			let send_result = nodes[0].node.send_payment(&route, second_payment_hash, &Some(second_payment_secret), payment_id);
 			match send_result {
-				Err(PaymentSendFailure::ParameterError(APIError::RouteError { err: "Payment already in progress" })) => {},
+				Err(PaymentSendFailure::DuplicatePayment) => {},
 				_ => panic!("Unexpected send result: {:?}", send_result),
 			}
 
 			// Further, if we try to send a spontaneous payment with the same payment_id it should
 			// also be rejected.
 			let send_result = nodes[0].node.send_spontaneous_payment(&route, None, payment_id);
 			match send_result {
-				Err(PaymentSendFailure::ParameterError(APIError::RouteError { err: "Payment already in progress" })) => {},
+				Err(PaymentSendFailure::DuplicatePayment) => {},
 				_ => panic!("Unexpected send result: {:?}", send_result),
 			}
 		}
@@ -1347,15 +1347,15 @@ fn abandoned_send_payment_idempotent() {
 			// payment_id, it should be rejected.
 			let send_result = nodes[0].node.send_payment(&route, second_payment_hash, &Some(second_payment_secret), payment_id);
 			match send_result {
-				Err(PaymentSendFailure::ParameterError(APIError::RouteError { err: "Payment already in progress" })) => {},
+				Err(PaymentSendFailure::DuplicatePayment) => {},
 				_ => panic!("Unexpected send result: {:?}", send_result),
 			}
 
 			// Further, if we try to send a spontaneous payment with the same payment_id it should
 			// also be rejected.
 			let send_result = nodes[0].node.send_spontaneous_payment(&route, None, payment_id);
 			match send_result {
-				Err(PaymentSendFailure::ParameterError(APIError::RouteError { err: "Payment already in progress" })) => {},
+				Err(PaymentSendFailure::DuplicatePayment) => {},
 				_ => panic!("Unexpected send result: {:?}", send_result),
 			}
 		}