From 92dffeb99d253e95742291ae90d900f2bc1a0d43 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 12 May 2023 14:33:03 +0200 Subject: [PATCH 1/4] Rename `NonUniquePaymentHash` error to `DuplicatePayment` --- bindings/ldk_node.udl | 2 +- src/error.rs | 8 +++++--- src/lib.rs | 4 ++-- src/test/functional_tests.rs | 2 +- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/bindings/ldk_node.udl b/bindings/ldk_node.udl index 9ceae441b3..8e85dcaa02 100644 --- a/bindings/ldk_node.udl +++ b/bindings/ldk_node.udl @@ -101,7 +101,7 @@ enum NodeError { "InvalidInvoice", "InvalidChannelId", "InvalidNetwork", - "NonUniquePaymentHash", + "DuplicatePayment", "InsufficientFunds", }; diff --git a/src/error.rs b/src/error.rs index 4d95535a72..04748ddeb7 100644 --- a/src/error.rs +++ b/src/error.rs @@ -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, } @@ -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.") } diff --git a/src/lib.rs b/src/lib.rs index 3a0526f23f..d3acf5c8d3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1404,7 +1404,7 @@ impl Node { if self.payment_store.contains(&payment_hash) { log_error!(self.logger, "Payment error: an invoice must not get paid twice."); - return Err(Error::NonUniquePaymentHash); + return Err(Error::DuplicatePayment); } let payment_secret = Some(*invoice.payment_secret()); @@ -1479,7 +1479,7 @@ 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::NonUniquePaymentHash); + return Err(Error::DuplicatePayment); } let payment_id = PaymentId(invoice.payment_hash().into_inner()); diff --git a/src/test/functional_tests.rs b/src/test/functional_tests.rs index 39da1a634a..32c7a19b73 100644 --- a/src/test/functional_tests.rs +++ b/src/test/functional_tests.rs @@ -131,7 +131,7 @@ fn channel_full_cycle() { 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_eq!(Err(Error::DuplicatePayment), node_a.send_payment(&invoice)); // Test under-/overpayment let invoice_amount_2_msat = 1000_000; From 64cf5f10ba935f23e46c2f02430a01ff83a63cef Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 12 May 2023 15:15:12 +0200 Subject: [PATCH 2/4] 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. --- bindings/ldk_node.udl | 3 +- src/error.rs | 6 +- src/lib.rs | 111 ++++++++++++++++++++++------------- src/payment_store.rs | 15 +++-- src/test/functional_tests.rs | 9 ++- 5 files changed, 91 insertions(+), 53 deletions(-) diff --git a/bindings/ldk_node.udl b/bindings/ldk_node.udl index 8e85dcaa02..ecb4a7cc57 100644 --- a/bindings/ldk_node.udl +++ b/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", }; diff --git a/src/error.rs b/src/error.rs index 04748ddeb7..d14723a02b 100644 --- a/src/error.rs +++ b/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."), diff --git a/src/lib.rs b/src/lib.rs index d3acf5c8d3..c7002db31a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1402,9 +1402,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 be paid twice."); + return Err(Error::DuplicatePayment); + } } let payment_secret = Some(*invoice.payment_secret()); @@ -1437,18 +1439,24 @@ impl Node { } 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 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 be paid twice."); + return Err(Error::DuplicatePayment); + } } let payment_id = PaymentId(invoice.payment_hash().into_inner()); @@ -1532,17 +1542,24 @@ impl Node { 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 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, @@ -1593,17 +1617,24 @@ impl Node { 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) + } + } } } } diff --git a/src/payment_store.rs b/src/payment_store.rs index fb18b2efaf..88aac631a2 100644 --- a/src/payment_store.rs +++ b/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 { 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, diff --git a/src/test/functional_tests.rs b/src/test/functional_tests.rs index 32c7a19b73..2c1ff37704 100644 --- a/src/test/functional_tests.rs +++ b/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; From 806f79994383fd51f17df2f075c6a8b62a5b5714 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 12 May 2023 13:38:51 +0200 Subject: [PATCH 3/4] Expose `list_payments` in bindings --- .../test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt | 5 ++++- bindings/ldk_node.udl | 1 + src/lib.rs | 5 +++++ src/test/functional_tests.rs | 2 ++ 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt b/bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt index b004b0da44..1b875d13d1 100644 --- a/bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt +++ b/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() diff --git a/bindings/ldk_node.udl b/bindings/ldk_node.udl index ecb4a7cc57..db3b0b1afc 100644 --- a/bindings/ldk_node.udl +++ b/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 list_payments(); sequence list_peers(); sequence list_channels(); [Throws=NodeError] diff --git a/src/lib.rs b/src/lib.rs index c7002db31a..b8ae4c48c2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1728,6 +1728,11 @@ impl Node { self.payment_store.list_filter(f) } + /// Retrieves all payments. + pub fn list_payments(&self) -> Vec { + self.payment_store.list_filter(|_| true) + } + /// Retrieves a list of known peers. pub fn list_peers(&self) -> Vec { let active_connected_peers: Vec = diff --git a/src/test/functional_tests.rs b/src/test/functional_tests.rs index 2c1ff37704..ba92dfc783 100644 --- a/src/test/functional_tests.rs +++ b/src/test/functional_tests.rs @@ -106,6 +106,8 @@ fn channel_full_cycle() { 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); assert_eq!(outbound_payments_a.len(), 1); From ecd1b64919f13d4869eab0a6f4f1a2dd3864d62a Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 1 Jun 2023 09:22:11 +0200 Subject: [PATCH 4/4] Rename test case to reflect newer persistence scheme --- src/payment_store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/payment_store.rs b/src/payment_store.rs index 88aac631a2..c9d867d607 100644 --- a/src/payment_store.rs +++ b/src/payment_store.rs @@ -209,7 +209,7 @@ 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);