Skip to content

Commit

Permalink
Add a separate PaymentSendFailure for idempotency violation
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
TheBlueMatt committed Nov 7, 2022
1 parent 3de2d5b commit dd4c94a
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 7 deletions.
5 changes: 5 additions & 0 deletions lightning-invoice/src/payment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -658,6 +659,10 @@ where
Err(PaymentSendFailure::AllFailedResendSafe(_)) => {
self.retry_payment(payment_id, payment_hash, params)
},
Err(PaymentSendFailure::DuplicatePayment) => {
log_info!(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
Expand Down
10 changes: 7 additions & 3 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,12 @@ pub enum PaymentSendFailure {
/// [`ChannelManager::abandon_payment`] and [`ChannelManager::retry_payment`] will *not* work
/// for this payment.
AllFailedResendSafe(Vec<APIError>),
/// 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.
Expand Down Expand Up @@ -2611,9 +2617,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F

let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
match pending_outbounds.entry(payment_id) {
hash_map::Entry::Occupied(_) => 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(),
Expand Down
8 changes: 4 additions & 4 deletions lightning/src/ln/payment_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1275,15 +1275,15 @@ 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),
}

// Further, if we try to send a spontaneous payment with the same payment_id it should
// 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),
}
}
Expand Down Expand Up @@ -1347,15 +1347,15 @@ 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),
}

// Further, if we try to send a spontaneous payment with the same payment_id it should
// 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),
}
}
Expand Down

0 comments on commit dd4c94a

Please sign in to comment.