Move retry-limiting to retry_payment_with_route and count paths

The documentation for `Retry` is very clear that it counts the
number of failed paths, not discrete retries. When we added
retries internally in `ChannelManager`, that broke and we started
counting the number of discrete retries, even if multiple paths
failed and were replaced with multiple MPP HTLCs.

Because we are now rewriting retries, we take this opportunity to
reduce the places where retries are analyzed, specifically a good
chunk of code is removed from `pay_internal`.

Because we now retry multiple failed paths with one single retry,
we choose somewhat arbitrarily to count the retried paths, not the
failed paths, which leaves the code simpler as we only have to
count the paths in a `Route` in front of us in
`retry_payment_with_route` rather than having to track the number
of original paths and offset the retry count against that while
handling failures.

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -2489,7 +2489,7 @@ where
 	#[cfg(test)]
 	pub(crate) fn test_add_new_pending_payment(&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId, route: &Route) -> Result<Vec<[u8; 32]>, PaymentSendFailure> {
 		let best_block_height = self.best_block.read().unwrap().height();
-		self.pending_outbound_payments.test_add_new_pending_payment(payment_hash, payment_secret, payment_id, route, Retry::Attempts(0), &self.entropy_source, best_block_height)
+		self.pending_outbound_payments.test_add_new_pending_payment(payment_hash, payment_secret, payment_id, route, None, &self.entropy_source, best_block_height)
 	}
 
 
@@ -7357,7 +7357,7 @@ where
 								hash_map::Entry::Vacant(entry) => {
 									let path_fee = path.get_path_fees();
 									entry.insert(PendingOutboundPayment::Retryable {
-										retry_strategy: Retry::Attempts(0),
+										retry_strategy: None,
 										attempts: PaymentAttempts::new(),
 										route_params: None,
 										session_privs: [session_priv_bytes].iter().map(|a| *a).collect(),

lightning/src/ln/outbound_payment.rs

@@ -41,7 +41,7 @@ pub(crate) enum PendingOutboundPayment {
 		session_privs: HashSet<[u8; 32]>,
 	},
 	Retryable {
-		retry_strategy: Retry,
+		retry_strategy: Option<Retry>,
 		attempts: PaymentAttempts,
 		route_params: Option<RouteParameters>,
 		session_privs: HashSet<[u8; 32]>,
@@ -82,11 +82,25 @@ impl PendingOutboundPayment {
 			attempts.count += 1;
 		}
 	}
-	fn is_retryable_now(&self) -> bool {
-		if let PendingOutboundPayment::Retryable { retry_strategy, attempts, .. } = self {
-			return retry_strategy.is_retryable_now(&attempts)
+	fn auto_retries_remaining_now(&self) -> usize {
+		match self {
+			PendingOutboundPayment::Retryable { retry_strategy: Some(strategy), attempts, .. } => {
+				strategy.retries_remaining_now(&attempts)
+			},
+			_ => 0,
+		}
+	}
+	fn is_retryable_now(&self, pending_paths: usize) -> bool {
+		match self {
+			PendingOutboundPayment::Retryable { retry_strategy: None, .. } => {
+				// We're handling retries manually, we can always retry.
+				true
+			},
+			PendingOutboundPayment::Retryable { retry_strategy: Some(strategy), attempts, .. } => {
+				strategy.retries_remaining_now(&attempts) >= pending_paths
+			},
+			_ => false,
 		}
-		false
 	}
 	pub fn insert_previously_failed_scid(&mut self, scid: u64) {
 		if let PendingOutboundPayment::Retryable { route_params: Some(params), .. } = self {
@@ -212,27 +226,27 @@ impl PendingOutboundPayment {
 pub enum Retry {
 	/// Max number of attempts to retry payment.
 	///
-	/// Note that this is the number of *path* failures, not full payment retries. For multi-path
-	/// payments, if this is less than the total number of paths, we will never even retry all of the
-	/// payment's paths.
+	/// This is the number of additional paths along which the payment will be retried after the
+	/// first payment. If two paths fail at around the same time and are retried with a single
+	/// MPP path/HTLC, it will only count as a single retry.
 	Attempts(usize),
 	#[cfg(not(feature = "no-std"))]
 	/// Time elapsed before abandoning retries for a payment.
 	Timeout(core::time::Duration),
 }
 
 impl Retry {
-	pub(crate) fn is_retryable_now(&self, attempts: &PaymentAttempts) -> bool {
+	pub(crate) fn retries_remaining_now(&self, attempts: &PaymentAttempts) -> usize {
 		match (self, attempts) {
 			(Retry::Attempts(max_retry_count), PaymentAttempts { count, .. }) => {
-				max_retry_count > count
+				*max_retry_count - count
 			},
 			#[cfg(all(not(feature = "no-std"), not(test)))]
 			(Retry::Timeout(max_duration), PaymentAttempts { first_attempted_at, .. }) =>
-				*max_duration >= std::time::Instant::now().duration_since(*first_attempted_at),
+				if *max_duration >= std::time::Instant::now().duration_since(*first_attempted_at) { usize::max_value() } else { 0 },
 			#[cfg(all(not(feature = "no-std"), test))]
 			(Retry::Timeout(max_duration), PaymentAttempts { first_attempted_at, .. }) =>
-				*max_duration >= SinceEpoch::now().duration_since(*first_attempted_at),
+				if *max_duration >= SinceEpoch::now().duration_since(*first_attempted_at) { usize::max_value() } else { 0 },
 		}
 	}
 }
@@ -409,7 +423,7 @@ impl OutboundPayments {
 		F: Fn(&Vec<RouteHop>, &Option<PaymentParameters>, &PaymentHash, &Option<PaymentSecret>, u64,
 		   u32, PaymentId, &Option<PaymentPreimage>, [u8; 32]) -> Result<(), APIError>
 	{
-		let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, route, Retry::Attempts(0), None, entropy_source, best_block_height)?;
+		let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, route, None, None, 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)
 			.map_err(|e| { self.remove_outbound_if_all_failed(payment_id, &e); e })
@@ -430,7 +444,7 @@ impl OutboundPayments {
 			None => PaymentPreimage(entropy_source.get_secure_random_bytes()),
 		};
 		let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner());
-		let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, &route, Retry::Attempts(0), None, entropy_source, best_block_height)?;
+		let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, &route, None, None, entropy_source, best_block_height)?;
 
 		match self.pay_route_internal(route, payment_hash, &None, Some(preimage), payment_id, None, onion_session_privs, node_signer, best_block_height, &send_payment_along_path) {
 			Ok(()) => Ok(payment_hash),
@@ -459,11 +473,15 @@ impl OutboundPayments {
 			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_retryable_now() {
+				let retries_remaining = pmt.auto_retries_remaining_now();
+				if retries_remaining > 0 {
 					if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, route_params: Some(params), .. } = pmt {
 						if pending_amt_msat < total_msat {
-							retry_id_route_params = Some((*pmt_id, params.clone()));
-							pmt.increment_attempts();
+							let mut params = params.clone();
+							if params.payment_params.max_path_count as usize > retries_remaining {
+								params.payment_params.max_path_count = retries_remaining as u8;
+							}
+							retry_id_route_params = Some((*pmt_id, params));
 							break
 						}
 					}
@@ -509,35 +527,21 @@ impl OutboundPayments {
 		}))?;
 
 		let res = if let Some((payment_hash, payment_secret, retry_strategy)) = initial_send_info {
-			let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, &route, retry_strategy, Some(route_params.clone()), entropy_source, best_block_height)?;
+			let onion_session_privs = self.add_new_pending_payment(payment_hash, *payment_secret, payment_id, &route, Some(retry_strategy), Some(route_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 {
 			self.retry_payment_with_route(&route, payment_id, entropy_source, node_signer, best_block_height, send_payment_along_path)
 		};
 		match res {
 			Err(PaymentSendFailure::AllFailedResendSafe(_)) => {
-				let mut outbounds = self.pending_outbound_payments.lock().unwrap();
-				if let Some(payment) = outbounds.get_mut(&payment_id) {
-					let retryable = payment.is_retryable_now();
-					if retryable {
-						payment.increment_attempts();
-					} else { return res }
-				} else { return res }
-				core::mem::drop(outbounds);
 				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);
 				log_info!(logger, "Errored retrying payment: {:?}", retry_res);
+				if let Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError { err })) = &retry_res {
+					if err.starts_with("Retries exhausted ") { return res; }
+				}
 				retry_res
 			},
-			Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, .. }) => {
-				let mut outbounds = self.pending_outbound_payments.lock().unwrap();
-				if let Some(payment) = outbounds.get_mut(&payment_id) {
-					let retryable = payment.is_retryable_now();
-					if retryable {
-						payment.increment_attempts();
-					} else { return Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, payment_id }) }
-				} else { return Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, payment_id }) }
-				core::mem::drop(outbounds);
-
+			Err(PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), .. }) => {
 				// 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.
@@ -611,8 +615,14 @@ impl OutboundPayments {
 							}));
 						},
 					};
+					if !payment.is_retryable_now(route.paths.len()) {
+						return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
+							err: format!("Retries exhausted for payment id {}", log_bytes!(payment_id.0)),
+						}))
+					}
 					for (path, session_priv_bytes) in route.paths.iter().zip(onion_session_privs.iter()) {
 						assert!(payment.insert(*session_priv_bytes, path));
+						payment.increment_attempts();
 					}
 					res
 				},
@@ -646,7 +656,7 @@ impl OutboundPayments {
 		}
 
 		let route = Route { paths: vec![hops], payment_params: None };
-		let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, &route, Retry::Attempts(0), None, entropy_source, best_block_height)?;
+		let onion_session_privs = self.add_new_pending_payment(payment_hash, None, payment_id, &route, None, None, entropy_source, best_block_height)?;
 
 		match self.pay_route_internal(&route, payment_hash, &None, None, payment_id, None, onion_session_privs, node_signer, best_block_height, &send_payment_along_path) {
 			Ok(()) => Ok((payment_hash, payment_id)),
@@ -660,14 +670,14 @@ impl OutboundPayments {
 	#[cfg(test)]
 	pub(super) fn test_add_new_pending_payment<ES: Deref>(
 		&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId,
-		route: &Route, retry_strategy: Retry, entropy_source: &ES, best_block_height: u32
+		route: &Route, retry_strategy: Option<Retry>, entropy_source: &ES, best_block_height: u32
 	) -> Result<Vec<[u8; 32]>, PaymentSendFailure> where ES::Target: EntropySource {
 		self.add_new_pending_payment(payment_hash, payment_secret, payment_id, route, retry_strategy, None, entropy_source, best_block_height)
 	}
 
 	pub(super) fn add_new_pending_payment<ES: Deref>(
 		&self, payment_hash: PaymentHash, payment_secret: Option<PaymentSecret>, payment_id: PaymentId,
-		route: &Route, retry_strategy: Retry, route_params: Option<RouteParameters>,
+		route: &Route, retry_strategy: Option<Retry>, route_params: Option<RouteParameters>,
 		entropy_source: &ES, best_block_height: u32
 	) -> Result<Vec<[u8; 32]>, PaymentSendFailure> where ES::Target: EntropySource {
 		let mut onion_session_privs = Vec::with_capacity(route.paths.len());
@@ -969,7 +979,7 @@ impl OutboundPayments {
 				log_trace!(logger, "Received failure of HTLC with payment_hash {} after payment completion", log_bytes!(payment_hash.0));
 				return
 			}
-			let is_retryable_now = payment.get().is_retryable_now();
+			let is_retryable_now = payment.get().auto_retries_remaining_now() > 0;
 			if let Some(scid) = short_channel_id {
 				payment.get_mut().insert_previously_failed_scid(scid);
 			}
@@ -1110,7 +1120,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
 		(0, session_privs, required),
 		(1, pending_fee_msat, option),
 		(2, payment_hash, required),
-		(not_written, retry_strategy, (static_value, Retry::Attempts(0))),
+		(not_written, retry_strategy, (static_value, None)),
 		(4, payment_secret, option),
 		(not_written, attempts, (static_value, PaymentAttempts::new())),
 		(6, total_msat, required),
@@ -1207,7 +1217,7 @@ mod tests {
 
 		let err = if on_retry {
 			outbound_payments.add_new_pending_payment(PaymentHash([0; 32]), None, PaymentId([0; 32]),
-			&Route { paths: vec![], payment_params: None }, Retry::Attempts(1), Some(route_params.clone()),
+			&Route { paths: vec![], payment_params: None }, Some(Retry::Attempts(1)), Some(route_params.clone()),
 			&&keys_manager, 0).unwrap();
 			outbound_payments.pay_internal(
 				PaymentId([0; 32]), None, route_params, &&router, vec![], InFlightHtlcs::new(),

lightning/src/ln/payment_tests.rs

@@ -2134,7 +2134,7 @@ fn retry_multi_path_single_failed_payment() {
 			final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV
 		}, Ok(route.clone()));
 
-	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
+	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(2)).unwrap();
 	let htlc_msgs = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(htlc_msgs.len(), 2);
 	check_added_monitors!(nodes[0], 2);
@@ -2195,7 +2195,7 @@ fn immediate_retry_on_failure() {
 			final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV
 		}, Ok(route.clone()));
 
-	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
+	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(2)).unwrap();
 	let htlc_msgs = nodes[0].node.get_and_clear_pending_msg_events();
 	assert_eq!(htlc_msgs.len(), 2);
 	check_added_monitors!(nodes[0], 2);
@@ -2251,7 +2251,7 @@ fn no_extra_retries_on_back_to_back_fail() {
 				node_features: nodes[1].node.node_features(),
 				short_channel_id: chan_1_scid,
 				channel_features: nodes[1].node.channel_features(),
-				fee_msat: 0,
+				fee_msat: 0, // nodes[1] will fail the payment as we don't pay its fee
 				cltv_expiry_delta: 100,
 			}, RouteHop {
 				pubkey: nodes[2].node.get_our_node_id(),
@@ -2266,7 +2266,7 @@ fn no_extra_retries_on_back_to_back_fail() {
 				node_features: nodes[1].node.node_features(),
 				short_channel_id: chan_1_scid,
 				channel_features: nodes[1].node.channel_features(),
-				fee_msat: 0,
+				fee_msat: 0, // nodes[1] will fail the payment as we don't pay its fee
 				cltv_expiry_delta: 100,
 			}, RouteHop {
 				pubkey: nodes[2].node.get_our_node_id(),
@@ -2280,10 +2280,12 @@ fn no_extra_retries_on_back_to_back_fail() {
 		payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id(), TEST_FINAL_CLTV)),
 	};
 	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
-	// On retry, we'll only be asked for one path
-	route.paths.remove(1);
 	let mut second_payment_params = route_params.payment_params.clone();
 	second_payment_params.previously_failed_channels = vec![chan_2_scid, chan_2_scid];
+	// We'll only have one retry left at the end, so we'll hlepfully get a max_path_count of 1
+	second_payment_params.max_path_count = 1;
+	route.paths.remove(1);
+	route.paths[0][1].fee_msat = amt_msat;
 	nodes[0].router.expect_find_route(RouteParameters {
 			payment_params: second_payment_params,
 			final_value_msat: amt_msat, final_cltv_expiry_delta: TEST_FINAL_CLTV,
@@ -2351,10 +2353,16 @@ fn no_extra_retries_on_back_to_back_fail() {
 
 	// At this point A has sent two HTLCs which both failed due to lack of fee. It now has two
 	// pending `PaymentPathFailed` events, one with `all_paths_failed` unset, and the second
-	// with it set. The first event will use up the only retry we are allowed, with the second
-	// `PaymentPathFailed` being passed up to the user (us, in this case). Previously, we'd
-	// treated this as "HTLC complete" and dropped the retry counter, causing us to retry again
-	// if the final HTLC failed.
+	// with it set.
+	//
+	// Previously, we retried payments in an event consumer, which would retry each
+	// `PaymentPathFailed` individually. In that setup, we had retried the payment in response to
+	// the first `PaymentPathFailed`, then seen the second `PaymentPathFailed` with
+	// `all_paths_failed` set and assumed the payment was completely failed. We ultimately fixed it
+	// by adding the `PaymentFailed` event.
+	//
+	// Because we now retry payments as a batch, we simply return a single-path route in the
+	// second, batched, request, have that fail, then complete the payment via `abandon_payment`.
 	let mut events = nodes[0].node.get_and_clear_pending_events();
 	assert_eq!(events.len(), 4);
 	match events[0] {