X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=blobdiff_plain;f=lightning%2Fsrc%2Fln%2Foutbound_payment.rs;h=2ddd3e2e006a9935a8c7243800a98aa36278d60f;hb=9f41bd7f6492ca146a852c779f072d5b87581fed;hp=490aa90697de9581ab493d2cf71c02fbc88e4300;hpb=30b9d9fbeaa62537beb8d3ea0b2866703d0d7c92;p=rust-lightning diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 490aa906..2ddd3e2e 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; @@ -383,12 +382,14 @@ pub enum PaymentSendFailure { pub(super) struct OutboundPayments { pub(super) pending_outbound_payments: Mutex>, + pub(super) retry_lock: Mutex<()>, } impl OutboundPayments { pub(super) fn new() -> Self { Self { - pending_outbound_payments: Mutex::new(HashMap::new()) + pending_outbound_payments: Mutex::new(HashMap::new()), + retry_lock: Mutex::new(()), } } @@ -396,7 +397,8 @@ impl OutboundPayments { &self, payment_hash: PaymentHash, payment_secret: &Option, payment_id: PaymentId, retry_strategy: Retry, route_params: RouteParameters, router: &R, first_hops: Vec, compute_inflight_htlcs: IH, entropy_source: &ES, - node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: SP, + node_signer: &NS, best_block_height: u32, logger: &L, + pending_events: &Mutex>, send_payment_along_path: SP, ) -> Result<(), PaymentSendFailure> where R::Target: Router, @@ -407,9 +409,9 @@ impl OutboundPayments { SP: Fn(&Vec, &Option, &PaymentHash, &Option, u64, u32, PaymentId, &Option, [u8; 32]) -> Result<(), APIError>, { - self.pay_internal(payment_id, Some((payment_hash, payment_secret, None, retry_strategy)), + self.pay_internal(payment_id, payment_hash, Some((payment_secret, None, retry_strategy)), route_params, router, first_hops, &compute_inflight_htlcs, entropy_source, node_signer, - best_block_height, logger, &send_payment_along_path) + best_block_height, logger, pending_events, &send_payment_along_path) .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } @@ -434,7 +436,8 @@ impl OutboundPayments { &self, payment_preimage: Option, payment_id: PaymentId, retry_strategy: Retry, route_params: RouteParameters, router: &R, first_hops: Vec, inflight_htlcs: IH, entropy_source: &ES, - node_signer: &NS, best_block_height: u32, logger: &L, send_payment_along_path: SP + node_signer: &NS, best_block_height: u32, logger: &L, + pending_events: &Mutex>, send_payment_along_path: SP ) -> Result where R::Target: Router, @@ -448,9 +451,9 @@ impl OutboundPayments { let preimage = payment_preimage .unwrap_or_else(|| PaymentPreimage(entropy_source.get_secure_random_bytes())); let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner()); - self.pay_internal(payment_id, Some((payment_hash, &None, Some(preimage), retry_strategy)), + self.pay_internal(payment_id, payment_hash, Some((&None, Some(preimage), retry_strategy)), route_params, router, first_hops, &inflight_htlcs, entropy_source, node_signer, - best_block_height, logger, &send_payment_along_path) + best_block_height, logger, pending_events, &send_payment_along_path) .map(|()| payment_hash) .map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e }) } @@ -494,14 +497,15 @@ impl OutboundPayments { FH: Fn() -> Vec, L::Target: Logger, { + let _single_thread = self.retry_lock.lock().unwrap(); loop { let mut outbounds = self.pending_outbound_payments.lock().unwrap(); let mut retry_id_route_params = None; for (pmt_id, pmt) in outbounds.iter_mut() { if pmt.is_auto_retryable_now() { - if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, payment_params: Some(params), .. } = pmt { + if let PendingOutboundPayment::Retryable { payment_hash, pending_amt_msat, total_msat, payment_params: Some(params), .. } = pmt { if pending_amt_msat < total_msat { - retry_id_route_params = Some((*pmt_id, RouteParameters { + retry_id_route_params = Some((*pmt_id, *payment_hash, RouteParameters { final_value_msat: *total_msat - *pending_amt_msat, final_cltv_expiry_delta: if let Some(delta) = params.final_cltv_expiry_delta { delta } @@ -517,8 +521,8 @@ impl OutboundPayments { } } core::mem::drop(outbounds); - if let Some((payment_id, route_params)) = retry_id_route_params { - if let Err(e) = self.pay_internal(payment_id, None, route_params, router, first_hops(), &inflight_htlcs, entropy_source, node_signer, best_block_height, logger, &send_payment_along_path) { + if let Some((payment_id, payment_hash, route_params)) = retry_id_route_params { + if let Err(e) = self.pay_internal(payment_id, payment_hash, None, route_params, router, first_hops(), &inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events, &send_payment_along_path) { log_info!(logger, "Errored retrying payment: {:?}", e); // If we error on retry, there is no chance of the payment succeeding and no HTLCs have // been irrevocably committed to, so we can safely abandon. @@ -543,13 +547,19 @@ 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, - initial_send_info: Option<(PaymentHash, &Option, Option, Retry)>, + &self, payment_id: PaymentId, payment_hash: PaymentHash, + initial_send_info: Option<(&Option, Option, Retry)>, route_params: RouteParameters, router: &R, first_hops: Vec, inflight_htlcs: &IH, entropy_source: &ES, node_signer: &NS, best_block_height: u32, - logger: &L, send_payment_along_path: &SP, + logger: &L, pending_events: &Mutex>, send_payment_along_path: &SP, ) -> Result<(), PaymentSendFailure> where R::Target: Router, @@ -575,7 +585,7 @@ impl OutboundPayments { err: format!("Failed to find a route for payment {}: {:?}", log_bytes!(payment_id.0), e), // TODO: add APIError::RouteNotFound }))?; - let res = if let Some((payment_hash, payment_secret, keysend_preimage, retry_strategy)) = initial_send_info { + let res = if let Some((payment_secret, keysend_preimage, retry_strategy)) = initial_send_info { let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, keysend_preimage, &route, Some(retry_strategy), Some(route_params.payment_params.clone()), entropy_source, best_block_height)?; self.pay_route_internal(&route, payment_hash, payment_secret, None, payment_id, None, onion_session_privs, node_signer, best_block_height, send_payment_along_path) } else { @@ -583,7 +593,7 @@ impl OutboundPayments { }; match res { Err(PaymentSendFailure::AllFailedResendSafe(_)) => { - let retry_res = self.pay_internal(payment_id, None, route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, logger, send_payment_along_path); + let retry_res = self.pay_internal(payment_id, payment_hash, None, route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events, send_payment_along_path); log_info!(logger, "Result retrying payment id {}: {:?}", log_bytes!(payment_id.0), retry_res); if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = &retry_res { if err.starts_with("Retries exhausted ") { return res; } @@ -594,7 +604,7 @@ impl OutboundPayments { // Some paths were sent, even if we failed to send the full MPP value our recipient may // misbehave and claim the funds, at which point we have to consider the payment sent, so // return `Ok()` here, ignoring any retry errors. - let retry_res = self.pay_internal(payment_id, None, retry, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, logger, send_payment_along_path); + let retry_res = self.pay_internal(payment_id, payment_hash, None, retry, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events, send_payment_along_path); log_info!(logger, "Result retrying payment id {}: {:?}", log_bytes!(payment_id.0), retry_res); Ok(()) }, @@ -1003,12 +1013,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))] @@ -1018,18 +1029,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 { @@ -1062,24 +1088,26 @@ impl OutboundPayments { }); } - if !payment_is_probe && (!is_retryable_now || !payment_retryable || retry.is_none()) { + if payment_is_probe || !is_retryable_now || !payment_retryable || retry.is_none() { let _ = payment.get_mut().mark_abandoned(); // we'll only Err if it's a legacy payment is_retryable_now = false; } if payment.get().remaining_parts() == 0 { all_paths_failed = true; if payment.get().abandoned() { - full_failure_ev = Some(events::Event::PaymentFailed { - payment_id: *payment_id, - payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"), - }); + if !payment_is_probe { + full_failure_ev = Some(events::Event::PaymentFailed { + payment_id: *payment_id, + payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"), + }); + } payment.remove(); } } 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)); @@ -1109,11 +1137,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), @@ -1134,7 +1160,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( @@ -1253,15 +1279,17 @@ mod tests { final_value_msat: 0, final_cltv_expiry_delta: 0, }; + let pending_events = Mutex::new(Vec::new()); let err = if on_retry { outbound_payments.pay_internal( - PaymentId([0; 32]), None, expired_route_params, &&router, vec![], &|| InFlightHtlcs::new(), - &&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() + PaymentId([0; 32]), PaymentHash([0; 32]), None, expired_route_params, &&router, vec![], + &|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, &pending_events, + &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() } else { outbound_payments.send_payment( PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), expired_route_params, &&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, - |_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() + &pending_events, |_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() }; if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err { assert!(err.contains("Invoice expired")); @@ -1293,18 +1321,20 @@ mod tests { router.expect_find_route(route_params.clone(), Err(LightningError { err: String::new(), action: ErrorAction::IgnoreError })); + let pending_events = Mutex::new(Vec::new()); let err = if on_retry { outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), None, PaymentId([0; 32]), None, &Route { paths: vec![], payment_params: None }, Some(Retry::Attempts(1)), Some(route_params.payment_params.clone()), &&keys_manager, 0).unwrap(); outbound_payments.pay_internal( - PaymentId([0; 32]), None, route_params, &&router, vec![], &|| InFlightHtlcs::new(), - &&keys_manager, &&keys_manager, 0, &&logger, &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() + PaymentId([0; 32]), PaymentHash([0; 32]), None, route_params, &&router, vec![], + &|| InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, &pending_events, + &|_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() } else { outbound_payments.send_payment( PaymentHash([0; 32]), &None, PaymentId([0; 32]), Retry::Attempts(0), route_params, &&router, vec![], || InFlightHtlcs::new(), &&keys_manager, &&keys_manager, 0, &&logger, - |_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() + &pending_events, |_, _, _, _, _, _, _, _, _| Ok(())).unwrap_err() }; if let PaymentSendFailure::ParameterError(APIError::APIMisuseError { err }) = err { assert!(err.contains("Failed to find a route"));