Skip to content

Commit fadf972

Browse files
jkczyzjkczyz
committed
Add PendingOutboundPayment::AwaitingInvoice
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.
1 parent b8770c0 commit fadf972

File tree

2 files changed

+105
-39
lines changed

2 files changed

+105
-39
lines changed

‎lightning/src/ln/channelmanager.rs‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1268,9 +1268,13 @@ const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_G
12681268
/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until expiry of incomplete MPPs
12691269
pub(crate) const MPP_TIMEOUT_TICKS: u8 = 3;
12701270

1271+
/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until an invoice request without
1272+
/// a response is timed out.
1273+
pub(crate) const INVOICE_REQUEST_TIMEOUT_TICKS: u8 = 3;
1274+
12711275
/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until we time-out the
12721276
/// idempotency of payments by [`PaymentId`]. See
1273-
/// [`OutboundPayments::remove_stale_resolved_payments`].
1277+
/// [`OutboundPayments::remove_stale_payments`].
12741278
pub(crate) const IDEMPOTENCY_TIMEOUT_TICKS: u8 = 7;
12751279

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

4519-
self.pending_outbound_payments.remove_stale_resolved_payments(&self.pending_events);
4531+
self.pending_outbound_payments.remove_stale_payments(&self.pending_events);
45204532

45214533
// Technically we don't need to do this here, but if we have holding cell entries in a
45224534
// channel that need freeing, it's better to do that here and block a background task
@@ -8057,6 +8069,7 @@ where
80578069
session_priv.write(writer)?;
80588070
}
80598071
}
8072+
PendingOutboundPayment::AwaitingInvoice { .. } => {},
80608073
PendingOutboundPayment::Fulfilled { .. } => {},
80618074
PendingOutboundPayment::Abandoned { .. } => {},
80628075
}

‎lightning/src/ln/outbound_payment.rs‎

Lines changed: 89 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey};
1616
use crate::sign::{EntropySource, NodeSigner, Recipient};
1717
use crate::events::{self, PaymentFailureReason};
1818
use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret};
19-
use crate::ln::channelmanager::{ChannelDetails, EventCompletionAction, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, PaymentId};
19+
use crate::ln::channelmanager::{ChannelDetails, EventCompletionAction, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, INVOICE_REQUEST_TIMEOUT_TICKS, PaymentId};
2020
use crate::ln::onion_utils::HTLCFailReason;
2121
use crate::routing::router::{InFlightHtlcs, Path, PaymentParameters, Route, RouteParameters, Router};
2222
use crate::util::errors::APIError;
@@ -38,6 +38,9 @@ pub(crate) enum PendingOutboundPayment {
3838
Legacy {
3939
session_privs: HashSet<[u8; 32]>,
4040
},
41+
AwaitingInvoice {
42+
timer_ticks_without_response: u8,
43+
},
4144
Retryable {
4245
retry_strategy: Option<Retry>,
4346
attempts: PaymentAttempts,
@@ -107,6 +110,12 @@ impl PendingOutboundPayment {
107110
params.previously_failed_channels.push(scid);
108111
}
109112
}
113+
fn is_awaiting_invoice(&self) -> bool {
114+
match self {
115+
PendingOutboundPayment::AwaitingInvoice { .. } => true,
116+
_ => false,
117+
}
118+
}
110119
pub(super) fn is_fulfilled(&self) -> bool {
111120
match self {
112121
PendingOutboundPayment::Fulfilled { .. } => true,
@@ -129,6 +138,7 @@ impl PendingOutboundPayment {
129138
fn payment_hash(&self) -> Option<PaymentHash> {
130139
match self {
131140
PendingOutboundPayment::Legacy { .. } => None,
141+
PendingOutboundPayment::AwaitingInvoice { .. } => None,
132142
PendingOutboundPayment::Retryable { payment_hash, .. } => Some(*payment_hash),
133143
PendingOutboundPayment::Fulfilled { payment_hash, .. } => *payment_hash,
134144
PendingOutboundPayment::Abandoned { payment_hash, .. } => Some(*payment_hash),
@@ -141,8 +151,8 @@ impl PendingOutboundPayment {
141151
PendingOutboundPayment::Legacy { session_privs } |
142152
PendingOutboundPayment::Retryable { session_privs, .. } |
143153
PendingOutboundPayment::Fulfilled { session_privs, .. } |
144-
PendingOutboundPayment::Abandoned { session_privs, .. }
145-
=> session_privs,
154+
PendingOutboundPayment::Abandoned { session_privs, .. } => session_privs,
155+
PendingOutboundPayment::AwaitingInvoice { .. } => return,
146156
});
147157
let payment_hash = self.payment_hash();
148158
*self = PendingOutboundPayment::Fulfilled { session_privs, payment_hash, timer_ticks_without_htlcs: 0 };
@@ -168,7 +178,8 @@ impl PendingOutboundPayment {
168178
PendingOutboundPayment::Fulfilled { session_privs, .. } |
169179
PendingOutboundPayment::Abandoned { session_privs, .. } => {
170180
session_privs.remove(session_priv)
171-
}
181+
},
182+
PendingOutboundPayment::AwaitingInvoice { .. } => false,
172183
};
173184
if remove_res {
174185
if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, ref mut pending_fee_msat, .. } = self {
@@ -188,6 +199,7 @@ impl PendingOutboundPayment {
188199
PendingOutboundPayment::Retryable { session_privs, .. } => {
189200
session_privs.insert(session_priv)
190201
}
202+
PendingOutboundPayment::AwaitingInvoice { .. } => false,
191203
PendingOutboundPayment::Fulfilled { .. } => false,
192204
PendingOutboundPayment::Abandoned { .. } => false,
193205
};
@@ -209,7 +221,8 @@ impl PendingOutboundPayment {
209221
PendingOutboundPayment::Fulfilled { session_privs, .. } |
210222
PendingOutboundPayment::Abandoned { session_privs, .. } => {
211223
session_privs.len()
212-
}
224+
},
225+
PendingOutboundPayment::AwaitingInvoice { .. } => 0,
213226
}
214227
}
215228
}
@@ -626,7 +639,7 @@ impl OutboundPayments {
626639
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
627640
outbounds.retain(|pmt_id, pmt| {
628641
let mut retain = true;
629-
if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 {
642+
if !pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_awaiting_invoice() {
630643
pmt.mark_abandoned(PaymentFailureReason::RetriesExhausted);
631644
if let PendingOutboundPayment::Abandoned { payment_hash, reason, .. } = pmt {
632645
pending_events.lock().unwrap().push_back((events::Event::PaymentFailed {
@@ -644,7 +657,8 @@ impl OutboundPayments {
644657
pub(super) fn needs_abandon(&self) -> bool {
645658
let outbounds = self.pending_outbound_payments.lock().unwrap();
646659
outbounds.iter().any(|(_, pmt)|
647-
!pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_fulfilled())
660+
!pmt.is_auto_retryable_now() && pmt.remaining_parts() == 0 && !pmt.is_fulfilled() &&
661+
!pmt.is_awaiting_invoice())
648662
}
649663

650664
/// Errors immediately on [`RetryableSendFailure`] error conditions. Otherwise, further errors may
@@ -779,6 +793,10 @@ impl OutboundPayments {
779793
log_error!(logger, "Unable to retry payments that were initially sent on LDK versions prior to 0.0.102");
780794
return
781795
},
796+
PendingOutboundPayment::AwaitingInvoice { .. } => {
797+
log_error!(logger, "Payment not yet sent");
798+
return
799+
},
782800
PendingOutboundPayment::Fulfilled { .. } => {
783801
log_error!(logger, "Payment already completed");
784802
return
@@ -951,35 +969,57 @@ impl OutboundPayments {
951969
keysend_preimage: Option<PaymentPreimage>, route: &Route, retry_strategy: Option<Retry>,
952970
payment_params: Option<PaymentParameters>, entropy_source: &ES, best_block_height: u32
953971
) -> Result<Vec<[u8; 32]>, PaymentSendFailure> where ES::Target: EntropySource {
972+
let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
973+
let entry = pending_outbounds.entry(payment_id);
974+
if let hash_map::Entry::Occupied(entry) = &entry {
975+
if let PendingOutboundPayment::AwaitingInvoice { .. } = entry.get() { } else {
976+
return Err(PaymentSendFailure::DuplicatePayment)
977+
}
978+
}
979+
954980
let mut onion_session_privs = Vec::with_capacity(route.paths.len());
955981
for _ in 0..route.paths.len() {
956982
onion_session_privs.push(entropy_source.get_secure_random_bytes());
957983
}
958984

985+
let mut payment = PendingOutboundPayment::Retryable {
986+
retry_strategy,
987+
attempts: PaymentAttempts::new(),
988+
payment_params,
989+
session_privs: HashSet::new(),
990+
pending_amt_msat: 0,
991+
pending_fee_msat: Some(0),
992+
payment_hash,
993+
payment_secret: recipient_onion.payment_secret,
994+
payment_metadata: recipient_onion.payment_metadata,
995+
keysend_preimage,
996+
starting_block_height: best_block_height,
997+
total_msat: route.get_total_amount(),
998+
};
999+
1000+
for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
1001+
assert!(payment.insert(*session_priv_bytes, path));
1002+
}
1003+
1004+
match entry {
1005+
hash_map::Entry::Occupied(mut entry) => { entry.insert(payment); },
1006+
hash_map::Entry::Vacant(entry) => { entry.insert(payment); },
1007+
}
1008+
1009+
Ok(onion_session_privs)
1010+
}
1011+
1012+
#[allow(unused)]
1013+
pub(super) fn add_new_awaiting_invoice(&self, payment_id: PaymentId) -> Result<(), ()> {
9591014
let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
9601015
match pending_outbounds.entry(payment_id) {
961-
hash_map::Entry::Occupied(_) => Err(PaymentSendFailure::DuplicatePayment),
1016+
hash_map::Entry::Occupied(_) => Err(()),
9621017
hash_map::Entry::Vacant(entry) => {
963-
let payment = entry.insert(PendingOutboundPayment::Retryable {
964-
retry_strategy,
965-
attempts: PaymentAttempts::new(),
966-
payment_params,
967-
session_privs: HashSet::new(),
968-
pending_amt_msat: 0,
969-
pending_fee_msat: Some(0),
970-
payment_hash,
971-
payment_secret: recipient_onion.payment_secret,
972-
payment_metadata: recipient_onion.payment_metadata,
973-
keysend_preimage,
974-
starting_block_height: best_block_height,
975-
total_msat: route.get_total_amount(),
1018+
entry.insert(PendingOutboundPayment::AwaitingInvoice {
1019+
timer_ticks_without_response: 0,
9761020
});
9771021

978-
for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
979-
assert!(payment.insert(*session_priv_bytes, path));
980-
}
981-
982-
Ok(onion_session_privs)
1022+
Ok(())
9831023
},
9841024
}
9851025
}
@@ -1188,19 +1228,19 @@ impl OutboundPayments {
11881228
}
11891229
}
11901230

1191-
pub(super) fn remove_stale_resolved_payments(&self,
1192-
pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>)
1231+
pub(super) fn remove_stale_payments(
1232+
&self, pending_events: &Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>)
11931233
{
1194-
// If an outbound payment was completed, and no pending HTLCs remain, we should remove it
1195-
// from the map. However, if we did that immediately when the last payment HTLC is claimed,
1196-
// this could race the user making a duplicate send_payment call and our idempotency
1197-
// guarantees would be violated. Instead, we wait a few timer ticks to do the actual
1198-
// removal. This should be more than sufficient to ensure the idempotency of any
1199-
// `send_payment` calls that were made at the same time the `PaymentSent` event was being
1200-
// processed.
12011234
let mut pending_outbound_payments = self.pending_outbound_payments.lock().unwrap();
1202-
let pending_events = pending_events.lock().unwrap();
1235+
let mut pending_events = pending_events.lock().unwrap();
12031236
pending_outbound_payments.retain(|payment_id, payment| {
1237+
// If an outbound payment was completed, and no pending HTLCs remain, we should remove it
1238+
// from the map. However, if we did that immediately when the last payment HTLC is claimed,
1239+
// this could race the user making a duplicate send_payment call and our idempotency
1240+
// guarantees would be violated. Instead, we wait a few timer ticks to do the actual
1241+
// removal. This should be more than sufficient to ensure the idempotency of any
1242+
// `send_payment` calls that were made at the same time the `PaymentSent` event was being
1243+
// processed.
12041244
if let PendingOutboundPayment::Fulfilled { session_privs, timer_ticks_without_htlcs, .. } = payment {
12051245
let mut no_remaining_entries = session_privs.is_empty();
12061246
if no_remaining_entries {
@@ -1225,6 +1265,16 @@ impl OutboundPayments {
12251265
*timer_ticks_without_htlcs = 0;
12261266
true
12271267
}
1268+
} else if let PendingOutboundPayment::AwaitingInvoice { timer_ticks_without_response, .. } = payment {
1269+
*timer_ticks_without_response += 1;
1270+
if *timer_ticks_without_response <= INVOICE_REQUEST_TIMEOUT_TICKS {
1271+
true
1272+
} else {
1273+
pending_events.push_back(
1274+
(events::Event::InvoiceRequestFailed { payment_id: *payment_id }, None)
1275+
);
1276+
false
1277+
}
12281278
} else { true }
12291279
});
12301280
}
@@ -1429,6 +1479,9 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
14291479
(1, reason, option),
14301480
(2, payment_hash, required),
14311481
},
1482+
(5, AwaitingInvoice) => {
1483+
(0, timer_ticks_without_response, required),
1484+
},
14321485
);
14331486

14341487
#[cfg(test)]

0 commit comments

Comments
 (0)