diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 892e4f6622b..783bbc06aa9 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -283,12 +283,13 @@ fn check_payment_err(send_err: PaymentSendFailure) { PaymentSendFailure::PathParameterError(per_path_results) => { for res in per_path_results { if let Err(api_err) = res { check_api_err(api_err); } } }, - PaymentSendFailure::AllFailedRetrySafe(per_path_results) => { + PaymentSendFailure::AllFailedResendSafe(per_path_results) => { for api_err in per_path_results { check_api_err(api_err); } }, 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 7d5b87cf457..0d1279710be 100644 --- a/lightning-invoice/src/payment.rs +++ b/lightning-invoice/src/payment.rs @@ -543,7 +543,8 @@ where Err(e) => match e { PaymentSendFailure::ParameterError(_) => Err(e), PaymentSendFailure::PathParameterError(_) => Err(e), - PaymentSendFailure::AllFailedRetrySafe(_) => { + 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(); payment_info.attempts.count += 1; @@ -655,9 +656,13 @@ where log_trace!(self.logger, "Failed to retry for payment {} due to bogus route/payment data, not retrying.", log_bytes!(payment_hash.0)); Err(()) }, - Err(PaymentSendFailure::AllFailedRetrySafe(_)) => { + 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 63e0bb5ea79..7884ff0da3a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1188,24 +1188,40 @@ 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 retry the payment in full (though you probably want to do so over different + /// You can freely resend the payment in full (though you probably want to do so over different /// paths than the ones selected). /// - /// [`ChannelManager::abandon_payment`] does *not* need to be called for this payment and - /// [`ChannelManager::retry_payment`] will *not* work for this payment. - AllFailedRetrySafe(Vec), + /// 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. + 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. @@ -2610,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(), @@ -2726,7 +2740,7 @@ impl ChannelManager { match &$res { - &Err(PaymentSendFailure::AllFailedRetrySafe(ref fails)) if $all_failed => { + &Err(PaymentSendFailure::AllFailedResendSafe(ref fails)) if $all_failed => { assert_eq!(fails.len(), 1); match fails[0] { $type => { $check }, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index b95356a3dd7..e60cee80a8a 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1333,7 +1333,7 @@ fn test_basic_channel_reserve() { let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], max_can_send + 1); let err = nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret), PaymentId(our_payment_hash.0)).err().unwrap(); match err { - PaymentSendFailure::AllFailedRetrySafe(ref fails) => { + PaymentSendFailure::AllFailedResendSafe(ref fails) => { match &fails[0] { &APIError::ChannelUnavailable{ref err} => assert!(regex::Regex::new(r"Cannot send value that would put our balance under counterparty-announced channel reserve value \(\d+\)").unwrap().is_match(err)), diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index a65da368709..d3146a6e686 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), } }