Only avoid duplicate payments if we failed sending

Previously, we denied retrying any payment for which we already had seen
the payment hash. However, this is rather invasive as payments might
fail due to local connections issues or unavailability of usable
channels, in which case the invoices/payment hashes would be 'burnt' and
couldn't get retried.

Here, we introduce a new `PaymentStatus::SendingFailed` to differentiate
send failures and allow retrying tracked payments if they failed to send
immediately. We also rename some error types accordingly for further
clarification.

Author: tnull

bindings/ldk_node.udl

@@ -69,7 +69,7 @@ enum NodeError {
 	"OnchainTxCreationFailed",
 	"ConnectionFailed",
 	"InvoiceCreationFailed",
-	"PaymentFailed",
+	"PaymentSendingFailed",
 	"PeerInfoParseFailed",
 	"ChannelCreationFailed",
 	"ChannelClosingFailed",
@@ -107,6 +107,7 @@ enum PaymentDirection {
 
 enum PaymentStatus {
 	"Pending",
+	"SendingFailed",
 	"Succeeded",
 	"Failed",
 };

src/error.rs

@@ -13,8 +13,8 @@ pub enum Error {
 	ConnectionFailed,
 	/// Invoice creation failed.
 	InvoiceCreationFailed,
-	/// An attempted payment has failed.
-	PaymentFailed,
+	/// Sending a payment has failed.
+	PaymentSendingFailed,
 	/// A given peer info could not be parsed.
 	PeerInfoParseFailed,
 	/// A channel could not be opened.
@@ -63,7 +63,7 @@ impl fmt::Display for Error {
 			}
 			Self::ConnectionFailed => write!(f, "Network connection closed."),
 			Self::InvoiceCreationFailed => write!(f, "Failed to create invoice."),
-			Self::PaymentFailed => write!(f, "Failed to send the given payment."),
+			Self::PaymentSendingFailed => write!(f, "Failed to send the given payment."),
 			Self::PeerInfoParseFailed => write!(f, "Failed to parse the given peer information."),
 			Self::ChannelCreationFailed => write!(f, "Failed to create channel."),
 			Self::ChannelClosingFailed => write!(f, "Failed to close channel."),
@@ -80,7 +80,9 @@ impl fmt::Display for Error {
 			Self::InvalidInvoice => write!(f, "The given invoice is invalid."),
 			Self::InvalidChannelId => write!(f, "The given channel ID is invalid."),
 			Self::InvalidNetwork => write!(f, "The given network is invalid."),
-			Self::DuplicatePayment => write!(f, "A payment with the given hash has already been initiated."),
+			Self::DuplicatePayment => {
+				write!(f, "A payment with the given hash has already been initiated.")
+			}
 			Self::InsufficientFunds => {
 				write!(f, "There are insufficient funds to complete the given operation.")
 			}

src/lib.rs

@@ -1143,9 +1143,11 @@ impl Node {
 
 		let payment_hash = PaymentHash((*invoice.payment_hash()).into_inner());
 
-		if self.payment_store.contains(&payment_hash) {
-			log_error!(self.logger, "Payment error: an invoice must not get paid twice.");
-			return Err(Error::DuplicatePayment);
+		if let Some(payment) = self.payment_store.get(&payment_hash) {
+			if payment.status != PaymentStatus::SendingFailed {
+				log_error!(self.logger, "Payment error: an invoice must not get paid twice.");
+				return Err(Error::DuplicatePayment);
+			}
 		}
 
 		let payment_secret = Some(*invoice.payment_secret());
@@ -1185,11 +1187,16 @@ impl Node {
 					secret: payment_secret,
 					amount_msat: invoice.amount_milli_satoshis(),
 					direction: PaymentDirection::Outbound,
-					status: PaymentStatus::Failed,
+					status: PaymentStatus::SendingFailed,
 				};
 				self.payment_store.insert(payment)?;
 
-				Err(Error::PaymentFailed)
+				match e {
+					channelmanager::RetryableSendFailure::DuplicatePayment => {
+						Err(Error::DuplicatePayment)
+					}
+					_ => Err(Error::PaymentSendingFailed),
+				}
 			}
 		}
 	}
@@ -1218,9 +1225,11 @@ impl Node {
 		}
 
 		let payment_hash = PaymentHash((*invoice.payment_hash()).into_inner());
-		if self.payment_store.contains(&payment_hash) {
-			log_error!(self.logger, "Payment error: an invoice must not get paid twice.");
-			return Err(Error::DuplicatePayment);
+		if let Some(payment) = self.payment_store.get(&payment_hash) {
+			if payment.status != PaymentStatus::SendingFailed {
+				log_error!(self.logger, "Payment error: an invoice must not get paid twice.");
+				return Err(Error::DuplicatePayment);
+			}
 		}
 
 		let payment_id = PaymentId(invoice.payment_hash().into_inner());
@@ -1279,11 +1288,16 @@ impl Node {
 					secret: payment_secret,
 					amount_msat: Some(amount_msat),
 					direction: PaymentDirection::Outbound,
-					status: PaymentStatus::Failed,
+					status: PaymentStatus::SendingFailed,
 				};
 				self.payment_store.insert(payment)?;
 
-				Err(Error::PaymentFailed)
+				match e {
+					channelmanager::RetryableSendFailure::DuplicatePayment => {
+						Err(Error::DuplicatePayment)
+					}
+					_ => Err(Error::PaymentSendingFailed),
+				}
 			}
 		}
 	}
@@ -1300,6 +1314,13 @@ impl Node {
 		let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
 		let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
 
+		if let Some(payment) = self.payment_store.get(&payment_hash) {
+			if payment.status != PaymentStatus::SendingFailed {
+				log_error!(self.logger, "Payment error: must not send duplicate payments.");
+				return Err(Error::DuplicatePayment);
+			}
+		}
+
 		let route_params = RouteParameters {
 			payment_params: PaymentParameters::from_node_id(
 				node_id,
@@ -1338,13 +1359,18 @@ impl Node {
 					hash: payment_hash,
 					preimage: Some(payment_preimage),
 					secret: None,
-					status: PaymentStatus::Failed,
+					status: PaymentStatus::SendingFailed,
 					direction: PaymentDirection::Outbound,
 					amount_msat: Some(amount_msat),
 				};
 				self.payment_store.insert(payment)?;
 
-				Err(Error::PaymentFailed)
+				match e {
+					channelmanager::RetryableSendFailure::DuplicatePayment => {
+						Err(Error::DuplicatePayment)
+					}
+					_ => Err(Error::PaymentSendingFailed),
+				}
 			}
 		}
 	}

src/payment_store.rs

@@ -57,16 +57,19 @@ impl_writeable_tlv_based_enum!(PaymentDirection,
 pub enum PaymentStatus {
 	/// The payment is still pending.
 	Pending,
+	/// The sending of the payment failed and is safe to be retried.
+	SendingFailed,
 	/// The payment suceeded.
 	Succeeded,
-	/// The payment failed.
+	/// The payment failed and is not retryable.
 	Failed,
 }
 
 impl_writeable_tlv_based_enum!(PaymentStatus,
 	(0, Pending) => {},
-	(1, Succeeded) => {},
-	(2, Failed) => {};
+	(2, SendingFailed) => {},
+	(4, Succeeded) => {},
+	(6, Failed) => {};
 );
 
 #[derive(Clone, Debug, PartialEq, Eq)]
@@ -141,10 +144,6 @@ where
 		self.payments.lock().unwrap().get(hash).cloned()
 	}
 
-	pub(crate) fn contains(&self, hash: &PaymentHash) -> bool {
-		self.payments.lock().unwrap().contains_key(hash)
-	}
-
 	pub(crate) fn update(&self, update: &PaymentDetailsUpdate) -> Result<bool, Error> {
 		let mut updated = false;
 		let mut locked_payments = self.payments.lock().unwrap();
@@ -237,7 +236,7 @@ mod tests {
 		let payment_store = PaymentStore::new(Vec::new(), Arc::clone(&store), logger);
 
 		let hash = PaymentHash([42u8; 32]);
-		assert!(!payment_store.contains(&hash));
+		assert!(!payment_store.get(&hash).is_some());
 
 		let payment = PaymentDetails {
 			hash,

src/test/functional_tests.rs

@@ -95,6 +95,7 @@ fn channel_full_cycle() {
 
 	println!("\nA send_payment");
 	let payment_hash = node_a.send_payment(&invoice).unwrap();
+	assert_eq!(node_a.send_payment(&invoice), Err(Error::DuplicatePayment));
 
 	let outbound_payments_a =
 		node_a.list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound);