f reset retries to be per-send not per-path again

Author: TheBlueMatt

lightning/src/ln/outbound_payment.rs

@@ -82,22 +82,22 @@ impl PendingOutboundPayment {
 			attempts.count += 1;
 		}
 	}
-	fn auto_retries_remaining_now(&self) -> usize {
+	fn is_auto_retryable_now(&self) -> bool {
 		match self {
 			PendingOutboundPayment::Retryable { retry_strategy: Some(strategy), attempts, .. } => {
-				strategy.retries_remaining_now(&attempts)
+				strategy.is_retryable_now(&attempts)
 			},
-			_ => 0,
+			_ => false,
 		}
 	}
-	fn is_retryable_now(&self, pending_paths: usize) -> bool {
+	fn is_retryable_now(&self) -> 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
+				strategy.is_retryable_now(&attempts)
 			},
 			_ => false,
 		}
@@ -236,17 +236,17 @@ pub enum Retry {
 }
 
 impl Retry {
-	pub(crate) fn retries_remaining_now(&self, attempts: &PaymentAttempts) -> usize {
+	pub(crate) fn is_retryable_now(&self, attempts: &PaymentAttempts) -> bool {
 		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, .. }) =>
-				if *max_duration >= std::time::Instant::now().duration_since(*first_attempted_at) { usize::max_value() } else { 0 },
+				*max_duration >= std::time::Instant::now().duration_since(*first_attempted_at),
 			#[cfg(all(not(feature = "no-std"), test))]
 			(Retry::Timeout(max_duration), PaymentAttempts { first_attempted_at, .. }) =>
-				if *max_duration >= SinceEpoch::now().duration_since(*first_attempted_at) { usize::max_value() } else { 0 },
+				*max_duration >= SinceEpoch::now().duration_since(*first_attempted_at),
 		}
 	}
 }
@@ -473,15 +473,10 @@ 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() {
-				let retries_remaining = pmt.auto_retries_remaining_now();
-				if retries_remaining > 0 {
+				if pmt.is_auto_retryable_now() {
 					if let PendingOutboundPayment::Retryable { pending_amt_msat, total_msat, route_params: Some(params), .. } = pmt {
 						if pending_amt_msat < total_msat {
-							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));
+							retry_id_route_params = Some((*pmt_id, params.clone()));
 							break
 						}
 					}
@@ -615,14 +610,14 @@ impl OutboundPayments {
 							}));
 						},
 					};
-					if !payment.is_retryable_now(route.paths.len()) {
+					if !payment.is_retryable_now() {
 						return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
 							err: format!("Retries exhausted for payment id {}", log_bytes!(payment_id.0)),
 						}))
 					}
+					payment.increment_attempts();
 					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
 				},
@@ -979,7 +974,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().auto_retries_remaining_now() > 0;
+			let is_retryable_now = payment.get().is_auto_retryable_now();
 			if let Some(scid) = short_channel_id {
 				payment.get_mut().insert_previously_failed_scid(scid);
 			}

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(2)).unwrap();
+	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).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(2)).unwrap();
+	nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).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);
@@ -2282,8 +2282,7 @@ fn no_extra_retries_on_back_to_back_fail() {
 	nodes[0].router.expect_find_route(route_params.clone(), Ok(route.clone()));
 	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;
+	// On retry, we'll only return one path
 	route.paths.remove(1);
 	route.paths[0][1].fee_msat = amt_msat;
 	nodes[0].router.expect_find_route(RouteParameters {