From: Jeffrey Czyz Date: Tue, 17 Oct 2023 14:59:39 +0000 (-0500) Subject: Await for invoices using an absolute expiry X-Git-Tag: v0.0.118~11^2~3 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=11fb9c486b2ebbd57ed41ad92b91a7f92b55236a;p=rust-lightning Await for invoices using an absolute expiry PendingOutboundPayment::AwaitingInvoice counts the number of timer ticks that have passed awaiting a Bolt12Invoice for an InvoiceRequest. When a constant INVOICE_REQUEST_TIMEOUT_TICKS has passed, the payment is forgotten. However, this mechanism is insufficient for the Refund scenario, where the Refund's expiration should be used instead. Change AwaitingInvoice to store an absolute expiry instead. When removing stale payments, pass the `SystemTime` in `std` and the highest block time minus two hours in `no-std`. --- diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index d1a1208c8..3abbd4779 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4791,6 +4791,10 @@ where /// with the current [`ChannelConfig`]. /// * Removing peers which have disconnected but and no longer have any channels. /// * Force-closing and removing channels which have not completed establishment in a timely manner. + /// * Forgetting about stale outbound payments, either those that have already been fulfilled + /// or those awaiting an invoice that hasn't been delivered in the necessary amount of time. + /// The latter is determined using the system clock in `std` and the block time minus two + /// hours in `no-std`. /// /// Note that this may cause reentrancy through [`chain::Watch::update_channel`] calls or feerate /// estimate fetches. @@ -5019,7 +5023,18 @@ where self.finish_close_channel(shutdown_res); } - self.pending_outbound_payments.remove_stale_payments(&self.pending_events); + #[cfg(feature = "std")] + let duration_since_epoch = std::time::SystemTime::now() + .duration_since(std::time::SystemTime::UNIX_EPOCH) + .expect("SystemTime::now() should come after SystemTime::UNIX_EPOCH"); + #[cfg(not(feature = "std"))] + let duration_since_epoch = Duration::from_secs( + self.highest_seen_timestamp.load(Ordering::Acquire).saturating_sub(7200) as u64 + ); + + self.pending_outbound_payments.remove_stale_payments( + duration_since_epoch, &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 diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index d6d9f7aac..d679036c1 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -29,6 +29,7 @@ use crate::util::ser::ReadableArgs; use core::fmt::{self, Display, Formatter}; use core::ops::Deref; +use core::time::Duration; use crate::prelude::*; use crate::sync::Mutex; @@ -39,12 +40,6 @@ use crate::sync::Mutex; /// [`ChannelManager::timer_tick_occurred`]: crate::ln::channelmanager::ChannelManager::timer_tick_occurred pub(crate) const IDEMPOTENCY_TIMEOUT_TICKS: u8 = 7; -/// The number of ticks of [`ChannelManager::timer_tick_occurred`] until an invoice request without -/// a response is timed out. -/// -/// [`ChannelManager::timer_tick_occurred`]: crate::ln::channelmanager::ChannelManager::timer_tick_occurred -const INVOICE_REQUEST_TIMEOUT_TICKS: u8 = 3; - /// Stores the session_priv for each part of a payment that is still pending. For versions 0.0.102 /// and later, also stores information for retrying the payment. pub(crate) enum PendingOutboundPayment { @@ -52,7 +47,7 @@ pub(crate) enum PendingOutboundPayment { session_privs: HashSet<[u8; 32]>, }, AwaitingInvoice { - timer_ticks_without_response: u8, + absolute_expiry: Duration, retry_strategy: Retry, max_total_routing_fee_msat: Option, }, @@ -1274,15 +1269,16 @@ impl OutboundPayments { } #[allow(unused)] - pub(super) fn add_new_awaiting_invoice( - &self, payment_id: PaymentId, retry_strategy: Retry, max_total_routing_fee_msat: Option + fn add_new_awaiting_invoice( + &self, payment_id: PaymentId, absolute_expiry: Duration, retry_strategy: Retry, + max_total_routing_fee_msat: Option ) -> Result<(), ()> { let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap(); match pending_outbounds.entry(payment_id) { hash_map::Entry::Occupied(_) => Err(()), hash_map::Entry::Vacant(entry) => { entry.insert(PendingOutboundPayment::AwaitingInvoice { - timer_ticks_without_response: 0, + absolute_expiry, retry_strategy, max_total_routing_fee_msat, }); @@ -1511,14 +1507,15 @@ impl OutboundPayments { } pub(super) fn remove_stale_payments( - &self, pending_events: &Mutex)>>) + &self, duration_since_epoch: Duration, + pending_events: &Mutex)>>) { let mut pending_outbound_payments = self.pending_outbound_payments.lock().unwrap(); #[cfg(not(invreqfailed))] let pending_events = pending_events.lock().unwrap(); #[cfg(invreqfailed)] let mut pending_events = pending_events.lock().unwrap(); - pending_outbound_payments.retain(|payment_id, payment| { + pending_outbound_payments.retain(|payment_id, payment| match 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 @@ -1526,7 +1523,7 @@ impl OutboundPayments { // 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 { + PendingOutboundPayment::Fulfilled { session_privs, timer_ticks_without_htlcs, .. } => { let mut no_remaining_entries = session_privs.is_empty(); if no_remaining_entries { for (ev, _) in pending_events.iter() { @@ -1550,9 +1547,9 @@ 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 { + }, + PendingOutboundPayment::AwaitingInvoice { absolute_expiry, .. } => { + if duration_since_epoch < *absolute_expiry { true } else { #[cfg(invreqfailed)] @@ -1561,7 +1558,8 @@ impl OutboundPayments { ); false } - } else { true } + }, + _ => true, }); } @@ -1778,7 +1776,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment, (2, payment_hash, required), }, (5, AwaitingInvoice) => { - (0, timer_ticks_without_response, required), + (0, absolute_expiry, required), (2, retry_strategy, required), (4, max_total_routing_fee_msat, option), }, @@ -1794,14 +1792,14 @@ mod tests { use bitcoin::network::constants::Network; use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; + use core::time::Duration; + use crate::events::{Event, PathFailure, PaymentFailureReason}; use crate::ln::PaymentHash; use crate::ln::channelmanager::{PaymentId, RecipientOnionFields}; use crate::ln::features::{ChannelFeatures, NodeFeatures}; use crate::ln::msgs::{ErrorAction, LightningError}; use crate::ln::outbound_payment::{Bolt12PaymentError, OutboundPayments, Retry, RetryableSendFailure}; - #[cfg(invreqfailed)] - use crate::ln::outbound_payment::INVOICE_REQUEST_TIMEOUT_TICKS; use crate::offers::invoice::DEFAULT_RELATIVE_EXPIRY; use crate::offers::offer::OfferBuilder; use crate::offers::test_utils::*; @@ -2011,20 +2009,28 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); let outbound_payments = OutboundPayments::new(); let payment_id = PaymentId([0; 32]); + let absolute_expiry = 100; + let tick_interval = 10; assert!(!outbound_payments.has_pending_payments()); assert!( - outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok() + outbound_payments.add_new_awaiting_invoice( + payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), None + ).is_ok() ); assert!(outbound_payments.has_pending_payments()); - for _ in 0..INVOICE_REQUEST_TIMEOUT_TICKS { - outbound_payments.remove_stale_payments(&pending_events); + for seconds_since_epoch in (0..absolute_expiry).step_by(tick_interval) { + let duration_since_epoch = Duration::from_secs(seconds_since_epoch); + outbound_payments.remove_stale_payments(duration_since_epoch, &pending_events); + assert!(outbound_payments.has_pending_payments()); assert!(pending_events.lock().unwrap().is_empty()); } - outbound_payments.remove_stale_payments(&pending_events); + let duration_since_epoch = Duration::from_secs(absolute_expiry); + outbound_payments.remove_stale_payments(duration_since_epoch, &pending_events); + assert!(!outbound_payments.has_pending_payments()); assert!(!pending_events.lock().unwrap().is_empty()); assert_eq!( @@ -2034,13 +2040,16 @@ mod tests { assert!(pending_events.lock().unwrap().is_empty()); assert!( - outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok() + outbound_payments.add_new_awaiting_invoice( + payment_id, Duration::from_secs(absolute_expiry + 1), Retry::Attempts(0), None + ).is_ok() ); assert!(outbound_payments.has_pending_payments()); assert!( - outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None) - .is_err() + outbound_payments.add_new_awaiting_invoice( + payment_id, Duration::from_secs(absolute_expiry + 1), Retry::Attempts(0), None + ).is_err() ); } @@ -2050,10 +2059,13 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); let outbound_payments = OutboundPayments::new(); let payment_id = PaymentId([0; 32]); + let absolute_expiry = 100; assert!(!outbound_payments.has_pending_payments()); assert!( - outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok() + outbound_payments.add_new_awaiting_invoice( + payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), None + ).is_ok() ); assert!(outbound_payments.has_pending_payments()); @@ -2081,9 +2093,12 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); let outbound_payments = OutboundPayments::new(); let payment_id = PaymentId([0; 32]); + let absolute_expiry = 100; assert!( - outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), None).is_ok() + outbound_payments.add_new_awaiting_invoice( + payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), None + ).is_ok() ); assert!(outbound_payments.has_pending_payments()); @@ -2129,6 +2144,7 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); let outbound_payments = OutboundPayments::new(); let payment_id = PaymentId([0; 32]); + let absolute_expiry = 100; let invoice = OfferBuilder::new("foo".into(), recipient_pubkey()) .amount_msats(1000) @@ -2140,9 +2156,11 @@ mod tests { .build().unwrap() .sign(recipient_sign).unwrap(); - assert!(outbound_payments.add_new_awaiting_invoice( - payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000)) - .is_ok() + assert!( + outbound_payments.add_new_awaiting_invoice( + payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), + Some(invoice.amount_msats() / 100 + 50_000) + ).is_ok() ); assert!(outbound_payments.has_pending_payments()); @@ -2185,6 +2203,7 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); let outbound_payments = OutboundPayments::new(); let payment_id = PaymentId([0; 32]); + let absolute_expiry = 100; let invoice = OfferBuilder::new("foo".into(), recipient_pubkey()) .amount_msats(1000) @@ -2196,9 +2215,11 @@ mod tests { .build().unwrap() .sign(recipient_sign).unwrap(); - assert!(outbound_payments.add_new_awaiting_invoice( - payment_id, Retry::Attempts(0), Some(invoice.amount_msats() / 100 + 50_000)) - .is_ok() + assert!( + outbound_payments.add_new_awaiting_invoice( + payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), + Some(invoice.amount_msats() / 100 + 50_000) + ).is_ok() ); assert!(outbound_payments.has_pending_payments()); @@ -2241,6 +2262,7 @@ mod tests { let pending_events = Mutex::new(VecDeque::new()); let outbound_payments = OutboundPayments::new(); let payment_id = PaymentId([0; 32]); + let absolute_expiry = 100; let invoice = OfferBuilder::new("foo".into(), recipient_pubkey()) .amount_msats(1000) @@ -2292,7 +2314,9 @@ mod tests { assert!(pending_events.lock().unwrap().is_empty()); assert!( - outbound_payments.add_new_awaiting_invoice(payment_id, Retry::Attempts(0), Some(1234)).is_ok() + outbound_payments.add_new_awaiting_invoice( + payment_id, Duration::from_secs(absolute_expiry), Retry::Attempts(0), Some(1234) + ).is_ok() ); assert!(outbound_payments.has_pending_payments());