Merge pull request #96 from tnull/2023-05-list-all-payments

Only avoid duplicate payments if we didn't fail sending (and expose `list_payments`)

Author: tnull

bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt

@@ -181,6 +181,9 @@ class LibraryTest {
         assert(paymentReceivedEvent is Event.PaymentReceived)
         node2.eventHandled()
 
+        assert(node1.listPayments().size == 1)
+        assert(node2.listPayments().size == 1)
+
         node2.closeChannel(channelId, nodeId1)
 
         val channelClosedEvent1 = node1.waitNextEvent()
@@ -196,7 +199,7 @@ class LibraryTest {
         mine(1u)
 
         // Sleep a bit to allow for the block to propagate to esplora
-        Thread.sleep(3_000)
+        Thread.sleep(5_000)
 
         node1.syncWallets()
         node2.syncWallets()

bindings/ldk_node.udl

@@ -67,6 +67,7 @@ interface LDKNode {
 	PaymentDetails? payment([ByRef]PaymentHash payment_hash);
 	[Throws=NodeError]
 	boolean remove_payment([ByRef]PaymentHash payment_hash);
+	sequence<PaymentDetails> list_payments();
 	sequence<PeerDetails> list_peers();
 	sequence<ChannelDetails> list_channels();
 	[Throws=NodeError]
@@ -81,7 +82,7 @@ enum NodeError {
 	"OnchainTxCreationFailed",
 	"ConnectionFailed",
 	"InvoiceCreationFailed",
-	"PaymentFailed",
+	"PaymentSendingFailed",
 	"ChannelCreationFailed",
 	"ChannelClosingFailed",
 	"PersistenceFailed",
@@ -101,7 +102,7 @@ enum NodeError {
 	"InvalidInvoice",
 	"InvalidChannelId",
 	"InvalidNetwork",
-	"NonUniquePaymentHash",
+	"DuplicatePayment",
 	"InsufficientFunds",
 };
 
@@ -122,6 +123,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.
@@ -53,8 +53,8 @@ pub enum Error {
 	InvalidChannelId,
 	/// The given network is invalid.
 	InvalidNetwork,
-	/// Payment of the given invoice has already been intiated.
-	NonUniquePaymentHash,
+	/// A payment with the given hash has already been intiated.
+	DuplicatePayment,
 	/// There are insufficient funds to complete the given operation.
 	InsufficientFunds,
 }
@@ -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."),
@@ -89,7 +89,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::NonUniquePaymentHash => write!(f, "An invoice must not get payed twice."),
+			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

@@ -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::NonUniquePaymentHash);
+		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::NonUniquePaymentHash);
+		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)
+					}
+				}
 			}
 		}
 	}
@@ -1697,6 +1728,11 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> {
 		self.payment_store.list_filter(f)
 	}
 
+	/// Retrieves all payments.
+	pub fn list_payments(&self) -> Vec<PaymentDetails> {
+		self.payment_store.list_filter(|_| true)
+	}
+
 	/// Retrieves a list of known peers.
 	pub fn list_peers(&self) -> Vec<PeerDetails> {
 		let active_connected_peers: Vec<PublicKey> =

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();
@@ -210,13 +209,13 @@ mod tests {
 	use std::sync::Arc;
 
 	#[test]
-	fn persistence_guard_persists_on_drop() {
+	fn payment_info_is_persisted() {
 		let store = Arc::new(TestStore::new());
 		let logger = Arc::new(TestLogger::new());
 		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,9 @@ 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));
+
+	assert_eq!(node_a.list_payments().first().unwrap().hash, payment_hash);
 
 	let outbound_payments_a =
 		node_a.list_payments_with_filter(|p| p.direction == PaymentDirection::Outbound);
@@ -130,8 +133,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_eq!(Err(Error::NonUniquePaymentHash), node_a.send_payment(&invoice));
+	// 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;