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 9ceae441b..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] @@ -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", }; diff --git a/src/error.rs b/src/error.rs index 4d95535a7..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. @@ -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.") } diff --git a/src/lib.rs b/src/lib.rs index 3a0526f23..b8ae4c48c 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::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 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::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 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) + } + } } } } @@ -1697,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/payment_store.rs b/src/payment_store.rs index fb18b2efa..c9d867d60 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(); @@ -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, diff --git a/src/test/functional_tests.rs b/src/test/functional_tests.rs index 39da1a634..ba92dfc78 100644 --- a/src/test/functional_tests.rs +++ b/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;