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

@@ -81,7 +81,7 @@ enum NodeError {
 	"OnchainTxCreationFailed",
 	"ConnectionFailed",
 	"InvoiceCreationFailed",
-	"PaymentFailed",
+	"PaymentSendingFailed",
 	"ChannelCreationFailed",
 	"ChannelClosingFailed",
 	"PersistenceFailed",
@@ -122,6 +122,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 channel could not be opened.
 	ChannelCreationFailed,
 	/// A channel could not be closed.
@@ -69,7 +69,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::ChannelCreationFailed => write!(f, "Failed to create channel."),
 			Self::ChannelClosingFailed => write!(f, "Failed to close channel."),
 			Self::PersistenceFailed => write!(f, "Failed to persist data."),

src/lib.rs

@@ -1402,9 +1402,11 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> {
 
 		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 be paid twice.");
+				return Err(Error::DuplicatePayment);
+			}
 		}
 
 		let payment_secret = Some(*invoice.payment_secret());
@@ -1437,18 +1439,24 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> {
 			}
 			Err(payment::PaymentError::Sending(e)) => {
 				log_error!(self.logger, "Failed to send payment: {:?}", e);
-
-				let payment = PaymentDetails {
-					preimage: None,
-					hash: payment_hash,
-					secret: payment_secret,
-					amount_msat: invoice.amount_milli_satoshis(),
-					direction: PaymentDirection::Outbound,
-					status: PaymentStatus::Failed,
-				};
-				self.payment_store.insert(payment)?;
-
-				Err(Error::PaymentFailed)
+				match e {
+					channelmanager::RetryableSendFailure::DuplicatePayment => {
+						Err(Error::DuplicatePayment)
+					}
+					_ => {
+						let payment = PaymentDetails {
+							preimage: None,
+							hash: payment_hash,
+							secret: payment_secret,
+							amount_msat: invoice.amount_milli_satoshis(),
+							direction: PaymentDirection::Outbound,
+							status: PaymentStatus::SendingFailed,
+						};
+
+						self.payment_store.insert(payment)?;
+						Err(Error::PaymentSendingFailed)
+					}
+				}
 			}
 		}
 	}
@@ -1477,9 +1485,11 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> {
 		}
 
 		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 be paid twice.");
+				return Err(Error::DuplicatePayment);
+			}
 		}
 
 		let payment_id = PaymentId(invoice.payment_hash().into_inner());
@@ -1532,17 +1542,24 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> {
 			Err(payment::PaymentError::Sending(e)) => {
 				log_error!(self.logger, "Failed to send payment: {:?}", e);
 
-				let payment = PaymentDetails {
-					hash: payment_hash,
-					preimage: None,
-					secret: payment_secret,
-					amount_msat: Some(amount_msat),
-					direction: PaymentDirection::Outbound,
-					status: PaymentStatus::Failed,
-				};
-				self.payment_store.insert(payment)?;
-
-				Err(Error::PaymentFailed)
+				match e {
+					channelmanager::RetryableSendFailure::DuplicatePayment => {
+						Err(Error::DuplicatePayment)
+					}
+					_ => {
+						let payment = PaymentDetails {
+							hash: payment_hash,
+							preimage: None,
+							secret: payment_secret,
+							amount_msat: Some(amount_msat),
+							direction: PaymentDirection::Outbound,
+							status: PaymentStatus::SendingFailed,
+						};
+						self.payment_store.insert(payment)?;
+
+						Err(Error::PaymentSendingFailed)
+					}
+				}
 			}
 		}
 	}
@@ -1559,6 +1576,13 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> {
 		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,
@@ -1593,17 +1617,24 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> {
 			Err(e) => {
 				log_error!(self.logger, "Failed to send payment: {:?}", e);
 
-				let payment = PaymentDetails {
-					hash: payment_hash,
-					preimage: Some(payment_preimage),
-					secret: None,
-					status: PaymentStatus::Failed,
-					direction: PaymentDirection::Outbound,
-					amount_msat: Some(amount_msat),
-				};
-				self.payment_store.insert(payment)?;
-
-				Err(Error::PaymentFailed)
+				match e {
+					channelmanager::RetryableSendFailure::DuplicatePayment => {
+						Err(Error::DuplicatePayment)
+					}
+					_ => {
+						let payment = PaymentDetails {
+							hash: payment_hash,
+							preimage: Some(payment_preimage),
+							secret: None,
+							status: PaymentStatus::SendingFailed,
+							direction: PaymentDirection::Outbound,
+							amount_msat: Some(amount_msat),
+						};
+
+						self.payment_store.insert(payment)?;
+						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)]
@@ -139,10 +142,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();
@@ -216,7 +215,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

@@ -104,6 +104,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);
@@ -130,8 +131,14 @@ fn channel_full_cycle() {
 	assert_eq!(node_b.payment(&payment_hash).unwrap().direction, PaymentDirection::Inbound);
 	assert_eq!(node_b.payment(&payment_hash).unwrap().amount_msat, Some(invoice_amount_1_msat));
 
-	// Assert we fail duplicate outbound payments.
+	// Assert we fail duplicate outbound payments and check the status hasn't changed.
 	assert_eq!(Err(Error::DuplicatePayment), node_a.send_payment(&invoice));
+	assert_eq!(node_a.payment(&payment_hash).unwrap().status, PaymentStatus::Succeeded);
+	assert_eq!(node_a.payment(&payment_hash).unwrap().direction, PaymentDirection::Outbound);
+	assert_eq!(node_a.payment(&payment_hash).unwrap().amount_msat, Some(invoice_amount_1_msat));
+	assert_eq!(node_b.payment(&payment_hash).unwrap().status, PaymentStatus::Succeeded);
+	assert_eq!(node_b.payment(&payment_hash).unwrap().direction, PaymentDirection::Inbound);
+	assert_eq!(node_b.payment(&payment_hash).unwrap().amount_msat, Some(invoice_amount_1_msat));
 
 	// Test under-/overpayment
 	let invoice_amount_2_msat = 1000_000;