diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index d47fbe0babd..77a335fed99 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3737,16 +3737,19 @@ where // being fully configured. See the docs for `ChannelManagerReadArgs` for more. match source { HTLCSource::OutboundRoute { ref path, ref session_priv, ref payment_id, ref payment_params, .. } => { - self.pending_outbound_payments.fail_htlc(source, payment_hash, onion_error, path, session_priv, payment_id, payment_params, self.probing_cookie_secret, &self.secp_ctx, &self.pending_events, &self.logger); + if self.pending_outbound_payments.fail_htlc(source, payment_hash, onion_error, path, + session_priv, payment_id, payment_params, self.probing_cookie_secret, &self.secp_ctx, + &self.pending_events, &self.logger) + { self.push_pending_forwards_ev(); } }, HTLCSource::PreviousHopData(HTLCPreviousHopData { ref short_channel_id, ref htlc_id, ref incoming_packet_shared_secret, ref phantom_shared_secret, ref outpoint }) => { log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards from us with {:?}", log_bytes!(payment_hash.0), onion_error); let err_packet = onion_error.get_encrypted_failure_packet(incoming_packet_shared_secret, phantom_shared_secret); - let mut forward_event = None; + let mut push_forward_ev = false; let mut forward_htlcs = self.forward_htlcs.lock().unwrap(); if forward_htlcs.is_empty() { - forward_event = Some(Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS)); + push_forward_ev = true; } match forward_htlcs.entry(*short_channel_id) { hash_map::Entry::Occupied(mut entry) => { @@ -3757,12 +3760,8 @@ where } } mem::drop(forward_htlcs); + if push_forward_ev { self.push_pending_forwards_ev(); } let mut pending_events = self.pending_events.lock().unwrap(); - if let Some(time) = forward_event { - pending_events.push(events::Event::PendingHTLCsForwardable { - time_forwardable: time - }); - } pending_events.push(events::Event::HTLCHandlingFailed { prev_channel_id: outpoint.to_channel_id(), failed_next_destination: destination, @@ -4839,7 +4838,7 @@ where #[inline] fn forward_htlcs(&self, per_source_pending_forwards: &mut [(u64, OutPoint, u128, Vec<(PendingHTLCInfo, u64)>)]) { for &mut (prev_short_channel_id, prev_funding_outpoint, prev_user_channel_id, ref mut pending_forwards) in per_source_pending_forwards { - let mut forward_event = None; + let mut push_forward_event = false; let mut new_intercept_events = Vec::new(); let mut failed_intercept_forwards = Vec::new(); if !pending_forwards.is_empty() { @@ -4897,7 +4896,7 @@ where // We don't want to generate a PendingHTLCsForwardable event if only intercepted // payments are being processed. if forward_htlcs_empty { - forward_event = Some(Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS)); + push_forward_event = true; } entry.insert(vec!(HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo { prev_short_channel_id, prev_funding_outpoint, prev_htlc_id, prev_user_channel_id, forward_info }))); @@ -4915,16 +4914,21 @@ where let mut events = self.pending_events.lock().unwrap(); events.append(&mut new_intercept_events); } + if push_forward_event { self.push_pending_forwards_ev() } + } + } - match forward_event { - Some(time) => { - let mut pending_events = self.pending_events.lock().unwrap(); - pending_events.push(events::Event::PendingHTLCsForwardable { - time_forwardable: time - }); - } - None => {}, - } + // We only want to push a PendingHTLCsForwardable event if no others are queued. + fn push_pending_forwards_ev(&self) { + let mut pending_events = self.pending_events.lock().unwrap(); + let forward_ev_exists = pending_events.iter() + .find(|ev| if let events::Event::PendingHTLCsForwardable { .. } = ev { true } else { false }) + .is_some(); + if !forward_ev_exists { + pending_events.push(events::Event::PendingHTLCsForwardable { + time_forwardable: + Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS), + }); } } @@ -7526,7 +7530,8 @@ where } } - if !forward_htlcs.is_empty() { + let pending_outbounds = OutboundPayments { pending_outbound_payments: Mutex::new(pending_outbound_payments.unwrap()), retry_lock: Mutex::new(()) }; + if !forward_htlcs.is_empty() || pending_outbounds.needs_abandon() { // If we have pending HTLCs to forward, assume we either dropped a // `PendingHTLCsForwardable` or the user received it but never processed it as they // shut down before the timer hit. Either way, set the time_forwardable to a small @@ -7694,7 +7699,7 @@ where inbound_payment_key: expanded_inbound_key, pending_inbound_payments: Mutex::new(pending_inbound_payments), - pending_outbound_payments: OutboundPayments { pending_outbound_payments: Mutex::new(pending_outbound_payments.unwrap()), retry_lock: Mutex::new(()), }, + pending_outbound_payments: pending_outbounds, pending_intercepted_htlcs: Mutex::new(pending_intercepted_htlcs.unwrap()), forward_htlcs: Mutex::new(forward_htlcs), diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 6ebd7bc547f..f252d88ac36 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -15,7 +15,7 @@ use bitcoin::secp256k1::{self, Secp256k1, SecretKey}; use crate::chain::keysinterface::{EntropySource, NodeSigner, Recipient}; use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret}; -use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, MIN_HTLC_RELAY_HOLDING_CELL_MILLIS, PaymentId}; +use crate::ln::channelmanager::{ChannelDetails, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, PaymentId}; use crate::ln::channelmanager::MIN_FINAL_CLTV_EXPIRY_DELTA as LDK_DEFAULT_MIN_FINAL_CLTV_EXPIRY_DELTA; use crate::ln::msgs::DecodeError; use crate::ln::onion_utils::HTLCFailReason; @@ -30,7 +30,6 @@ use crate::util::time::tests::SinceEpoch; use core::cmp; use core::fmt::{self, Display, Formatter}; use core::ops::Deref; -use core::time::Duration; use crate::prelude::*; use crate::sync::Mutex; @@ -546,6 +545,12 @@ 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()) + } + /// Will return `Ok(())` iff at least one HTLC is sent for the payment. fn pay_internal( &self, payment_id: PaymentId, @@ -1006,12 +1011,13 @@ impl OutboundPayments { }); } + // Returns a bool indicating whether a PendingHTLCsForwardable event should be generated. pub(super) fn fail_htlc( &self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, path: &Vec, session_priv: &SecretKey, payment_id: &PaymentId, payment_params: &Option, probing_cookie_secret: [u8; 32], secp_ctx: &Secp256k1, pending_events: &Mutex>, logger: &L - ) where L::Target: Logger { + ) -> bool where L::Target: Logger { #[cfg(test)] let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_error.decode_onion_failure(secp_ctx, logger, &source); #[cfg(not(test))] @@ -1021,18 +1027,33 @@ impl OutboundPayments { let mut session_priv_bytes = [0; 32]; session_priv_bytes.copy_from_slice(&session_priv[..]); let mut outbounds = self.pending_outbound_payments.lock().unwrap(); + + // If any payments already need retry, there's no need to generate a redundant + // `PendingHTLCsForwardable`. + let already_awaiting_retry = outbounds.iter().any(|(_, pmt)| { + let mut awaiting_retry = false; + if pmt.is_auto_retryable_now() { + if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, .. } = pmt { + if pending_amt_msat < total_msat { + awaiting_retry = true; + } + } + } + awaiting_retry + }); + let mut all_paths_failed = false; let mut full_failure_ev = None; - let mut pending_retry_ev = None; + let mut pending_retry_ev = false; let mut retry = None; let attempts_remaining = if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) { if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) { log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); - return + return false } if payment.get().is_fulfilled() { log_trace!(logger, "Received failure of HTLC with payment_hash {} after payment completion", log_bytes!(payment_hash.0)); - return + return false } let mut is_retryable_now = payment.get().is_auto_retryable_now(); if let Some(scid) = short_channel_id { @@ -1084,7 +1105,7 @@ impl OutboundPayments { is_retryable_now } else { log_trace!(logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0)); - return + return false }; core::mem::drop(outbounds); log_trace!(logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0)); @@ -1114,11 +1135,9 @@ impl OutboundPayments { } // If we miss abandoning the payment above, we *must* generate an event here or else the // payment will sit in our outbounds forever. - if attempts_remaining { + if attempts_remaining && !already_awaiting_retry { debug_assert!(full_failure_ev.is_none()); - pending_retry_ev = Some(events::Event::PendingHTLCsForwardable { - time_forwardable: Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS), - }); + pending_retry_ev = true; } events::Event::PaymentPathFailed { payment_id: Some(*payment_id), @@ -1139,7 +1158,7 @@ impl OutboundPayments { let mut pending_events = pending_events.lock().unwrap(); pending_events.push(path_failure); if let Some(ev) = full_failure_ev { pending_events.push(ev); } - if let Some(ev) = pending_retry_ev { pending_events.push(ev); } + pending_retry_ev } pub(super) fn abandon_payment( diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 4c228a3226a..3285cf4d7a5 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -1721,8 +1721,9 @@ fn do_automatic_retries(test: AutoRetry) { let chan_1_monitor_serialized = get_monitor!(nodes[0], channel_id_1).encode(); reload_node!(nodes[0], node_encoded, &[&chan_1_monitor_serialized], persister, new_chain_monitor, node_0_deserialized); + let mut events = nodes[0].node.get_and_clear_pending_events(); + expect_pending_htlcs_forwardable_from_events!(nodes[0], events, true); // Make sure we don't retry again. - nodes[0].node.process_pending_htlc_forwards(); let mut msg_events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(msg_events.len(), 0); @@ -2348,7 +2349,7 @@ fn no_extra_retries_on_back_to_back_fail() { // Because we now retry payments as a batch, we simply return a single-path route in the // second, batched, request, have that fail, ensure the payment was abandoned. let mut events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 4); + assert_eq!(events.len(), 3); match events[0] { Event::PaymentPathFailed { payment_hash: ev_payment_hash, payment_failed_permanently, .. } => { assert_eq!(payment_hash, ev_payment_hash); @@ -2367,10 +2368,6 @@ fn no_extra_retries_on_back_to_back_fail() { }, _ => panic!("Unexpected event"), } - match events[3] { - Event::PendingHTLCsForwardable { .. } => {}, - _ => panic!("Unexpected event"), - } nodes[0].node.process_pending_htlc_forwards(); let retry_htlc_updates = SendEvent::from_node(&nodes[0]);