From fcf73f0f45d05e0bb2e6118c6d05291adcb5bc0b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 2 Nov 2022 23:25:34 +0000 Subject: [PATCH] Add a separate PaymentSendFailure for idempotency violation When a user attempts to send a payment but it fails due to idempotency key violation, they need to know that this was the reason as they need to handle the error programmatically differently from other errors. Here we simply add a new `PaymentSendFailure` enum variant for `DuplicatePayment` to allow for that. --- fuzz/src/chanmon_consistency.rs | 1 + lightning-invoice/src/payment.rs | 5 +++++ lightning/src/ln/channelmanager.rs | 29 +++++++++++++++++++++-------- lightning/src/ln/payment_tests.rs | 8 ++++---- 4 files changed, 31 insertions(+), 12 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index aa6a3faf..783bbc06 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -289,6 +289,7 @@ fn check_payment_err(send_err: PaymentSendFailure) { PaymentSendFailure::PartialFailure { results, .. } => { for res in results { if let Err(api_err) = res { check_api_err(api_err); } } }, + PaymentSendFailure::DuplicatePayment => panic!(), } } diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs index 118ad5da..0d127971 100644 --- a/lightning-invoice/src/payment.rs +++ b/lightning-invoice/src/payment.rs @@ -543,6 +543,7 @@ where Err(e) => match e { PaymentSendFailure::ParameterError(_) => Err(e), PaymentSendFailure::PathParameterError(_) => Err(e), + PaymentSendFailure::DuplicatePayment => Err(e), PaymentSendFailure::AllFailedResendSafe(_) => { let mut payment_cache = self.payment_cache.lock().unwrap(); let payment_info = payment_cache.get_mut(&payment_hash).unwrap(); @@ -658,6 +659,10 @@ where Err(PaymentSendFailure::AllFailedResendSafe(_)) => { self.retry_payment(payment_id, payment_hash, params) }, + Err(PaymentSendFailure::DuplicatePayment) => { + log_error!(self.logger, "Got a DuplicatePayment error when attempting to retry a payment, this shouldn't happen."); + Err(()) + } Err(PaymentSendFailure::PartialFailure { failed_paths_retry, results, .. }) => { // If a `PartialFailure` error contains a result that is an `Ok()`, it means that // part of our payment is retried. When we receive `MonitorUpdateInProgress`, it diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 7de2da89..7884ff0d 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1188,16 +1188,25 @@ impl ChannelDetails { #[derive(Clone, Debug)] pub enum PaymentSendFailure { /// A parameter which was passed to send_payment was invalid, preventing us from attempting to - /// send the payment at all. No channel state has been changed or messages sent to peers, and - /// once you've changed the parameter at error, you can freely retry the payment in full. + /// send the payment at all. + /// + /// You can freely resend the payment in full (with the parameter error fixed). + /// + /// Because the payment failed outright, no payment tracking is done, you do not need to call + /// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work + /// for this payment. ParameterError(APIError), /// A parameter in a single path which was passed to send_payment was invalid, preventing us - /// from attempting to send the payment at all. No channel state has been changed or messages - /// sent to peers, and once you've changed the parameter at error, you can freely retry the - /// payment in full. + /// from attempting to send the payment at all. + /// + /// You can freely resend the payment in full (with the parameter error fixed). /// /// The results here are ordered the same as the paths in the route object which was passed to /// send_payment. + /// + /// Because the payment failed outright, no payment tracking is done, you do not need to call + /// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work + /// for this payment. PathParameterError(Vec>), /// All paths which were attempted failed to send, with no channel state change taking place. /// You can freely resend the payment in full (though you probably want to do so over different @@ -1207,6 +1216,12 @@ pub enum PaymentSendFailure { /// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work /// for this payment. AllFailedResendSafe(Vec), + /// Indicates that a payment for the provided [`PaymentId`] is already in-flight and has not + /// yet completed (i.e. generated an [`Event::PaymentSent`]) or been abandoned (via + /// [`ChannelManager::abandon_payment`]). + /// + /// [`Event::PaymentSent`]: events::Event::PaymentSent + DuplicatePayment, /// Some paths which were attempted failed to send, though possibly not all. At least some /// paths have irrevocably committed to the HTLC and retrying the payment in full would result /// in over-/re-payment. @@ -2611,9 +2626,7 @@ impl ChannelManager Err(PaymentSendFailure::ParameterError(APIError::RouteError { - err: "Payment already in progress" - })), + hash_map::Entry::Occupied(_) => Err(PaymentSendFailure::DuplicatePayment), hash_map::Entry::Vacant(entry) => { let payment = entry.insert(PendingOutboundPayment::Retryable { session_privs: HashSet::new(), diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index a65da368..d3146a6e 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -1275,7 +1275,7 @@ fn claimed_send_payment_idempotent() { // payment_id, it should be rejected. let send_result = nodes[0].node.send_payment(&route, second_payment_hash, &Some(second_payment_secret), payment_id); match send_result { - Err(PaymentSendFailure::ParameterError(APIError::RouteError { err: "Payment already in progress" })) => {}, + Err(PaymentSendFailure::DuplicatePayment) => {}, _ => panic!("Unexpected send result: {:?}", send_result), } @@ -1283,7 +1283,7 @@ fn claimed_send_payment_idempotent() { // also be rejected. let send_result = nodes[0].node.send_spontaneous_payment(&route, None, payment_id); match send_result { - Err(PaymentSendFailure::ParameterError(APIError::RouteError { err: "Payment already in progress" })) => {}, + Err(PaymentSendFailure::DuplicatePayment) => {}, _ => panic!("Unexpected send result: {:?}", send_result), } } @@ -1347,7 +1347,7 @@ fn abandoned_send_payment_idempotent() { // payment_id, it should be rejected. let send_result = nodes[0].node.send_payment(&route, second_payment_hash, &Some(second_payment_secret), payment_id); match send_result { - Err(PaymentSendFailure::ParameterError(APIError::RouteError { err: "Payment already in progress" })) => {}, + Err(PaymentSendFailure::DuplicatePayment) => {}, _ => panic!("Unexpected send result: {:?}", send_result), } @@ -1355,7 +1355,7 @@ fn abandoned_send_payment_idempotent() { // also be rejected. let send_result = nodes[0].node.send_spontaneous_payment(&route, None, payment_id); match send_result { - Err(PaymentSendFailure::ParameterError(APIError::RouteError { err: "Payment already in progress" })) => {}, + Err(PaymentSendFailure::DuplicatePayment) => {}, _ => panic!("Unexpected send result: {:?}", send_result), } } -- 2.30.2