Skip to content

Commit

Permalink
Add PendingOutboundPayment::AwaitingInvoice
Browse files Browse the repository at this point in the history 
When a BOLT 12 invoice has been requested, in order to guarantee
invoice payment idempotency the future payment needs to be tracked. Add
an AwaitingInvoice variant to PendingOutboundPayment such that only
requested invoices are paid only once. Timeout after a few timer ticks
if a request has not been received.
  • Loading branch information
@jkczyz
jkczyz committed Aug 17, 2023
1 parent d3c8613 commit e0bdae9
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 39 deletions.
19 changes: 16 additions & 3 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1268,9 +1268,13 @@ const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_G
/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until expiry of incomplete MPPs
pub(crate) const MPP_TIMEOUT_TICKS: u8 = 3;

/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until an invoice request without
/// a response is timed out.
pub(crate) const INVOICE_REQUEST_TIMEOUT_TICKS: u8 = 3;

/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until we time-out the
/// idempotency of payments by [`PaymentId`]. See
/// [`OutboundPayments::remove_stale_resolved_payments`].
/// [`OutboundPayments::remove_stale_payments`].
pub(crate) const IDEMPOTENCY_TIMEOUT_TICKS: u8 = 7;

/// The number of ticks of [`ChannelManager::timer_tick_occurred`] where a peer is disconnected
Expand Down Expand Up @@ -1615,6 +1619,11 @@ pub enum ChannelShutdownState {
/// These include payments that have yet to find a successful path, or have unresolved HTLCs.
#[derive(Debug, PartialEq)]
pub enum RecentPaymentDetails {
/// When an invoice was requested but not yet received, and thus a payment has not been sent.
AwaitingInvoice {
/// Identifier for the payment to ensure idempotency.
payment_id: PaymentId,
},
/// When a payment is still being sent and awaiting successful delivery.
Pending {
/// Hash of the payment that is currently being sent but has yet to be fulfilled or
Expand Down Expand Up @@ -2344,7 +2353,10 @@ where
/// [`Event::PaymentSent`]: events::Event::PaymentSent
pub fn list_recent_payments(&self) -> Vec<RecentPaymentDetails> {
self.pending_outbound_payments.pending_outbound_payments.lock().unwrap().iter()
.filter_map(|(_, pending_outbound_payment)| match pending_outbound_payment {
.filter_map(|(payment_id, pending_outbound_payment)| match pending_outbound_payment {
PendingOutboundPayment::AwaitingInvoice { .. } => {
Some(RecentPaymentDetails::AwaitingInvoice { payment_id: *payment_id })
},
PendingOutboundPayment::Retryable { payment_hash, total_msat, .. } => {
Some(RecentPaymentDetails::Pending {
payment_hash: *payment_hash,
Expand Down Expand Up @@ -4516,7 +4528,7 @@ where
let _ = handle_error!(self, err, counterparty_node_id);
}

self.pending_outbound_payments.remove_stale_resolved_payments(&self.pending_events);
self.pending_outbound_payments.remove_stale_payments(&self.pending_events);

// Technically we don't need to do this here, but if we have holding cell entries in a
// channel that need freeing, it's better to do that here and block a background task
Expand Down Expand Up @@ -8057,6 +8069,7 @@ where
session_priv.write(writer)?;
}
}
PendingOutboundPayment::AwaitingInvoice { .. } => {},
PendingOutboundPayment::Fulfilled { .. } => {},
PendingOutboundPayment::Abandoned { .. } => {},
}
Expand Down
125 changes: 89 additions & 36 deletions lightning/src/ln/outbound_payment.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
use crate::sign::{EntropySource, NodeSigner, Recipient};
use crate::events::{self, PaymentFailureReason};
use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
use crate::ln::channelmanager::{ChannelDetails, EventCompletionAction, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, PaymentId};
use crate::ln::channelmanager::{ChannelDetails, EventCompletionAction, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, INVOICE_REQUEST_TIMEOUT_TICKS, PaymentId};
use crate::ln::onion_utils::HTLCFailReason;
use crate::routing::router::{InFlightHtlcs, Path, PaymentParameters, Route, RouteParameters, Router};
use crate::util::errors::APIError;
Expand All @@ -38,6 +38,9 @@ pub(crate) enum PendingOutboundPayment {
Legacy {
session_privs: HashSet<[u8; 32]>,
},
AwaitingInvoice {
timer_ticks_without_response: u8,
},
Retryable {
retry_strategy: Option<Retry>,
attempts: PaymentAttempts,
Expand Down Expand Up @@ -107,6 +110,12 @@ impl PendingOutboundPayment {
params.previously_failed_channels.push(scid);
}
}
fn is_awaiting_invoice(&self) -> bool {
match self {
PendingOutboundPayment::AwaitingInvoice { .. } => true,
_ => false,
}
}
pub(super) fn is_fulfilled(&self) -> bool {
match self {
PendingOutboundPayment::Fulfilled { .. } => true,
Expand All @@ -129,6 +138,7 @@ impl PendingOutboundPayment {
fn payment_hash(&self) -> Option<PaymentHash> {
match self {
PendingOutboundPayment::Legacy { .. } => None,
PendingOutboundPayment::AwaitingInvoice { .. } => None,
PendingOutboundPayment::Retryable { payment_hash, .. } => Some(*payment_hash),
PendingOutboundPayment::Fulfilled { payment_hash, .. } => *payment_hash,
PendingOutboundPayment::Abandoned { payment_hash, .. } => Some(*payment_hash),
Expand All @@ -141,8 +151,8 @@ impl PendingOutboundPayment {
PendingOutboundPayment::Legacy { session_privs } |
PendingOutboundPayment::Retryable { session_privs, .. } |
PendingOutboundPayment::Fulfilled { session_privs, .. } |
PendingOutboundPayment::Abandoned { session_privs, .. }
=> session_privs,
PendingOutboundPayment::Abandoned { session_privs, .. } => session_privs,
PendingOutboundPayment::AwaitingInvoice { .. } => return,
});
let payment_hash = self.payment_hash();
*self = PendingOutboundPayment::Fulfilled { session_privs, payment_hash, timer_ticks_without_htlcs: 0 };
Expand All @@ -168,7 +178,8 @@ impl PendingOutboundPayment {
PendingOutboundPayment::Fulfilled { session_privs, .. } |
PendingOutboundPayment::Abandoned { session_privs, .. } => {
session_privs.remove(session_priv)
}
},
PendingOutboundPayment::AwaitingInvoice { .. } => false,
};
if remove_res {
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
Expand All @@ -188,6 +199,7 @@ impl PendingOutboundPayment {
PendingOutboundPayment::Retryable { session_privs, .. } => {
session_privs.insert(session_priv)
}
PendingOutboundPayment::AwaitingInvoice { .. } => false,
PendingOutboundPayment::Fulfilled { .. } => false,
PendingOutboundPayment::Abandoned { .. } => false,
};
Expand All @@ -209,7 +221,8 @@ impl PendingOutboundPayment {
PendingOutboundPayment::Fulfilled { session_privs, .. } |
PendingOutboundPayment::Abandoned { session_privs, .. } => {
session_privs.len()
}
},
PendingOutboundPayment::AwaitingInvoice { .. } => 0,
}
}
}
Expand Down Expand Up @@ -626,7 +639,7 @@ impl OutboundPayments {
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
outbounds.retain(|pmt_id, pmt| {
let mut retain = true;
if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 {
if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_awaiting_invoice() {
pmt.mark_abandoned(PaymentFailureReason::RetriesExhausted);
if let PendingOutboundPayment::Abandoned { payment_hash, reason, .. } = pmt {
pending_events.lock().unwrap().push_back((events::Event::PaymentFailed {
Expand All @@ -644,7 +657,8 @@ impl OutboundPayments {
pub(super) fn needs_abandon(&self) -> bool {
let outbounds = self.pending_outbound_payments.lock().unwrap();
outbounds.iter().any(|(_, pmt)|
!pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_fulfilled())
!pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_fulfilled() &&
!pmt.is_awaiting_invoice())
}

/// Errors immediately on [`RetryableSendFailure`] error conditions. Otherwise, further errors may
Expand Down Expand Up @@ -779,6 +793,10 @@ impl OutboundPayments {
log_error!(logger, "Unable to retry payments that were initially sent on LDK versions prior to 0.0.102");
return
},
PendingOutboundPayment::AwaitingInvoice { .. } => {
log_error!(logger, "Payment not yet sent");
return
},
PendingOutboundPayment::Fulfilled { .. } => {
log_error!(logger, "Payment already completed");
return
Expand Down Expand Up @@ -951,35 +969,57 @@ impl OutboundPayments {
keysend_preimage: Option<PaymentPreimage>, route: &Route, retry_strategy: Option<Retry>,
payment_params: Option<PaymentParameters>, entropy_source: &ES, best_block_height: u32
) -> Result<Vec<[u8; 32]>, PaymentSendFailure> where ES::Target: EntropySource {
let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
let entry = pending_outbounds.entry(payment_id);
if let hash_map::Entry::Occupied(entry) = &entry {
if let PendingOutboundPayment::AwaitingInvoice { .. } = entry.get() { } else {
return Err(PaymentSendFailure::DuplicatePayment)
}
}

let mut onion_session_privs = Vec::with_capacity(route.paths.len());
for _ in 0..route.paths.len() {
onion_session_privs.push(entropy_source.get_secure_random_bytes());
}

let mut payment = PendingOutboundPayment::Retryable {
retry_strategy,
attempts: PaymentAttempts::new(),
payment_params,
session_privs: HashSet::new(),
pending_amt_msat: 0,
pending_fee_msat: Some(0),
payment_hash,
payment_secret: recipient_onion.payment_secret,
payment_metadata: recipient_onion.payment_metadata,
keysend_preimage,
starting_block_height: best_block_height,
total_msat: route.get_total_amount(),
};

for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
assert!(payment.insert(*session_priv_bytes, path));
}

match entry {
hash_map::Entry::Occupied(mut entry) => { entry.insert(payment); },
hash_map::Entry::Vacant(entry) => { entry.insert(payment); },
}

Ok(onion_session_privs)
}

#[allow(unused)]
pub(super) fn add_new_awaiting_invoice(&self, payment_id: PaymentId) -> Result<(), ()> {
let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
match pending_outbounds.entry(payment_id) {
hash_map::Entry::Occupied(_) => Err(PaymentSendFailure::DuplicatePayment),
hash_map::Entry::Occupied(_) => Err(()),
hash_map::Entry::Vacant(entry) => {
let payment = entry.insert(PendingOutboundPayment::Retryable {
retry_strategy,
attempts: PaymentAttempts::new(),
payment_params,
session_privs: HashSet::new(),
pending_amt_msat: 0,
pending_fee_msat: Some(0),
payment_hash,
payment_secret: recipient_onion.payment_secret,
payment_metadata: recipient_onion.payment_metadata,
keysend_preimage,
starting_block_height: best_block_height,
total_msat: route.get_total_amount(),
entry.insert(PendingOutboundPayment::AwaitingInvoice {
timer_ticks_without_response: 0,
});

for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
assert!(payment.insert(*session_priv_bytes, path));
}

Ok(onion_session_privs)
Ok(())
},
}
}
Expand Down Expand Up @@ -1188,19 +1228,19 @@ impl OutboundPayments {
}
}

pub(super) fn remove_stale_resolved_payments(&self,
pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>)
pub(super) fn remove_stale_payments(
&self, pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>)
{
// If an outbound payment was completed, and no pending HTLCs remain, we should remove it
// from the map. However, if we did that immediately when the last payment HTLC is claimed,
// this could race the user making a duplicate send_payment call and our idempotency
// guarantees would be violated. Instead, we wait a few timer ticks to do the actual
// removal. This should be more than sufficient to ensure the idempotency of any
// `send_payment` calls that were made at the same time the `PaymentSent` event was being
// processed.
let mut pending_outbound_payments = self.pending_outbound_payments.lock().unwrap();
let pending_events = pending_events.lock().unwrap();
let mut pending_events = pending_events.lock().unwrap();
pending_outbound_payments.retain(|payment_id, payment| {
// If an outbound payment was completed, and no pending HTLCs remain, we should remove it
// from the map. However, if we did that immediately when the last payment HTLC is claimed,
// this could race the user making a duplicate send_payment call and our idempotency
// guarantees would be violated. Instead, we wait a few timer ticks to do the actual
// removal. This should be more than sufficient to ensure the idempotency of any
// `send_payment` calls that were made at the same time the `PaymentSent` event was being
// processed.
if let PendingOutboundPayment::Fulfilled { session_privs, timer_ticks_without_htlcs, .. } = payment {
let mut no_remaining_entries = session_privs.is_empty();
if no_remaining_entries {
Expand All @@ -1225,6 +1265,16 @@ impl OutboundPayments {
*timer_ticks_without_htlcs = 0;
true
}
} else if let PendingOutboundPayment::AwaitingInvoice { timer_ticks_without_response, .. } = payment {
*timer_ticks_without_response += 1;
if *timer_ticks_without_response <= INVOICE_REQUEST_TIMEOUT_TICKS {
true
} else {
pending_events.push_back(
(events::Event::InvoiceRequestFailed { payment_id: *payment_id }, None)
);
false
}
} else { true }
});
}
Expand Down Expand Up @@ -1429,6 +1479,9 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
(1, reason, option),
(2, payment_hash, required),
},
(5, AwaitingInvoice) => {
(0, timer_ticks_without_response, required),
},
);

#[cfg(test)]
Expand Down

0 comments on commit e0bdae9

Please sign in to comment.