X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=blobdiff_plain;f=lightning%2Fsrc%2Fln%2Foutbound_payment.rs;h=2ddd3e2e006a9935a8c7243800a98aa36278d60f;hb=9f41bd7f6492ca146a852c779f072d5b87581fed;hp=715a041dcf3512b0fdfb83244d7418b148b3cc80;hpb=5ea433f71f2db64d38048143c9c9a7d6b8ed3aeb;p=rust-lightning

diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index 715a041d..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;
@@ -398,7 +397,8 @@ impl OutboundPayments {
 		&self, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, payment_id: PaymentId,
 		retry_strategy: Retry, route_params: RouteParameters, router: &R,
 		first_hops: Vec<ChannelDetails>, 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<Vec<events::Event>>, send_payment_along_path: SP,
 	) -> Result<(), PaymentSendFailure>
 	where
 		R::Target: Router,
@@ -409,9 +409,9 @@ impl OutboundPayments {
 		SP: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
 			 u32, PaymentId, &Option<PaymentPreimage>, [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 })
 	}
 
@@ -436,7 +436,8 @@ impl OutboundPayments {
 		&self, payment_preimage: Option<PaymentPreimage>, payment_id: PaymentId,
 		retry_strategy: Retry, route_params: RouteParameters, router: &R,
 		first_hops: Vec<ChannelDetails>, 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<Vec<events::Event>>, send_payment_along_path: SP
 	) -> Result<PaymentHash, PaymentSendFailure>
 	where
 		R::Target: Router,
@@ -450,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 })
 	}
@@ -502,9 +503,9 @@ impl OutboundPayments {
 			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 }
@@ -520,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.
@@ -554,11 +555,11 @@ impl OutboundPayments {
 
 	/// Will return `Ok(())` iff at least one HTLC is sent for the payment.
 	fn pay_internal<R: Deref, NS: Deref, ES: Deref, IH, SP, L: Deref>(
-		&self, payment_id: PaymentId,
-		initial_send_info: Option<(PaymentHash, &Option<PaymentSecret>, Option<PaymentPreimage>, Retry)>,
+		&self, payment_id: PaymentId, payment_hash: PaymentHash,
+		initial_send_info: Option<(&Option<PaymentSecret>, Option<PaymentPreimage>, Retry)>,
 		route_params: RouteParameters, router: &R, first_hops: Vec<ChannelDetails>,
 		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<Vec<events::Event>>, send_payment_along_path: &SP,
 	) -> Result<(), PaymentSendFailure>
 	where
 		R::Target: Router,
@@ -584,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 {
@@ -592,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; }
@@ -603,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(())
 			},
@@ -1012,12 +1013,13 @@ impl OutboundPayments {
 		});
 	}
 
+	// Returns a bool indicating whether a PendingHTLCsForwardable event should be generated.
 	pub(super) fn fail_htlc<L: Deref>(
 		&self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason,
 		path: &Vec<RouteHop>, session_priv: &SecretKey, payment_id: &PaymentId,
 		payment_params: &Option<PaymentParameters>, probing_cookie_secret: [u8; 32],
 		secp_ctx: &Secp256k1<secp256k1::All>, pending_events: &Mutex<Vec<events::Event>>, 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))]
@@ -1027,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 {
@@ -1090,7 +1107,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));
@@ -1120,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),
@@ -1145,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(
@@ -1264,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"));
@@ -1304,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"));