From: Matt Corallo Date: Wed, 2 Nov 2022 23:25:34 +0000 (+0000) Subject: Add a separate PaymentSendFailure for idempotency violation X-Git-Tag: v0.0.113~38^2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=refs%2Fheads%2F2022-10-idempotency-err;p=rust-lightning 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. --- diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index aa6a3faf0..783bbc06a 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 118ad5da6..0d1279710 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 7de2da89c..7884ff0da 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 a65da3687..d3146a6e6 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), } }