f also push failed scids on PartialFailure

Author: valentinewallace

lightning/src/ln/outbound_payment.rs

@@ -747,11 +747,11 @@ impl OutboundPayments {
 	{
 		match err {
 			PaymentSendFailure::AllFailedResendSafe(errs) => {
-				Self::push_payment_path_failed_evs(payment_id, payment_hash, Some(&mut route_params), route.paths, errs.into_iter().map(|e| Err(e)), pending_events);
+				Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut route_params, route.paths, errs.into_iter().map(|e| Err(e)), pending_events);
 				self.retry_payment_internal(payment_id, route_params, router, first_hops, inflight_htlcs, entropy_source, node_signer, best_block_height, logger, pending_events, send_payment_along_path);
 			},
-			PaymentSendFailure::PartialFailure { failed_paths_retry: Some(retry), results, .. } => {
-				Self::push_payment_path_failed_evs(payment_id, payment_hash, None, route.paths, results.into_iter(), pending_events);
+			PaymentSendFailure::PartialFailure { failed_paths_retry: Some(mut retry), results, .. } => {
+				Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut retry, route.paths, results.into_iter(), pending_events);
 				// 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.
@@ -763,7 +763,7 @@ impl OutboundPayments {
 				// initial HTLC-Add messages yet.
 			},
 			PaymentSendFailure::PathParameterError(results) => {
-				Self::push_payment_path_failed_evs(payment_id, payment_hash, None, route.paths, results.into_iter(), pending_events);
+				Self::push_path_failed_evs_and_scids(payment_id, payment_hash, &mut route_params, route.paths, results.into_iter(), pending_events);
 				self.abandon_payment(payment_id, pending_events);
 			},
 			PaymentSendFailure::ParameterError(e) => {
@@ -774,10 +774,9 @@ impl OutboundPayments {
 		}
 	}
 
-	fn push_payment_path_failed_evs<I: ExactSizeIterator + Iterator<Item = Result<(), APIError>>>(
-		payment_id: PaymentId, payment_hash: PaymentHash,
-		mut route_params: Option<&mut RouteParameters>, paths: Vec<Vec<RouteHop>>, path_results: I,
-		pending_events: &Mutex<Vec<events::Event>>
+	fn push_path_failed_evs_and_scids<I: ExactSizeIterator + Iterator<Item = Result<(), APIError>>>(
+		payment_id: PaymentId, payment_hash: PaymentHash, route_params: &mut RouteParameters,
+		paths: Vec<Vec<RouteHop>>, path_results: I, pending_events: &Mutex<Vec<events::Event>>
 	) {
 		let mut events = pending_events.lock().unwrap();
 		debug_assert_eq!(paths.len(), path_results.len());
@@ -787,9 +786,7 @@ impl OutboundPayments {
 					None
 				} else {
 					let scid = path[0].short_channel_id;
-					if let Some(ref mut params) = route_params {
-						params.payment_params.previously_failed_channels.push(scid);
-					}
+					route_params.payment_params.previously_failed_channels.push(scid);
 					Some(scid)
 				};
 				events.push(events::Event::PaymentPathFailed {

lightning/src/ln/payment_tests.rs

@@ -1857,13 +1857,15 @@ fn auto_retry_partial_failure() {
 		payment_params: Some(route_params.payment_params.clone()),
 	};
 	nodes[0].router.expect_find_route(route_params.clone(), Ok(send_route));
+	let mut payment_params = route_params.payment_params.clone();
+	payment_params.previously_failed_channels.push(chan_2_id);
 	nodes[0].router.expect_find_route(RouteParameters {
-			payment_params: route_params.payment_params.clone(),
-			final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV
+			payment_params, final_value_msat: amt_msat / 2, final_cltv_expiry_delta: TEST_FINAL_CLTV
 		}, Ok(retry_1_route));
+	let mut payment_params = route_params.payment_params.clone();
+	payment_params.previously_failed_channels.push(chan_3_id);
 	nodes[0].router.expect_find_route(RouteParameters {
-			payment_params: route_params.payment_params.clone(),
-			final_value_msat: amt_msat / 4, final_cltv_expiry_delta: TEST_FINAL_CLTV
+			payment_params, final_value_msat: amt_msat / 4, final_cltv_expiry_delta: TEST_FINAL_CLTV
 		}, Ok(retry_2_route));
 
 	// Send a payment that will partially fail on send, then partially fail on retry, then succeed.
@@ -2113,8 +2115,10 @@ fn retry_multi_path_single_failed_payment() {
 	// On retry, split the payment across both channels.
 	route.paths[0][0].fee_msat = 50_000_001;
 	route.paths[1][0].fee_msat = 50_000_000;
+	let mut pay_params = route.payment_params.clone().unwrap();
+	pay_params.previously_failed_channels.push(chans[1].short_channel_id.unwrap());
 	nodes[0].router.expect_find_route(RouteParameters {
-			payment_params: route.payment_params.clone().unwrap(),
+			payment_params: pay_params,
 			// Note that the second request here requests the amount we originally failed to send,
 			// not the amount remaining on the full payment, which should be changed.
 			final_value_msat: 100_000_001, final_cltv_expiry_delta: TEST_FINAL_CLTV