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 9ceae441b..8e85dcaa0 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 4d95535a7..04748ddeb 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 3a0526f23..d3acf5c8d 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 39da1a634..32c7a19b7 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 8e85dcaa0..ecb4a7cc5 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 04748ddeb..d14723a02 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 d3acf5c8d..c7002db31 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 fb18b2efa..88aac631a 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 32c7a19b7..2c1ff3770 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 b004b0da4..1b875d13d 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 ecb4a7cc5..db3b0b1af 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 c7002db31..b8ae4c48c 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 2c1ff3770..ba92dfc78 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 88aac631a..c9d867d60 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);