Remove AllPathsFailed outbounds at send_payment_internal callsites instead

This makes it easier to retry payments if all paths fail on initial send, in
in which case we'll want to hold off on removing the pending payment

Author: valentinewallace

lightning/src/ln/outbound_payment.rs

@@ -380,7 +380,14 @@ impl OutboundPayments {
 		   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)?;
-		self.send_payment_internal(route, payment_hash, payment_secret, None, payment_id, None, onion_session_privs, node_signer, best_block_height, send_payment_along_path)
+		self.send_payment_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| { // TODO: use inspect_err instead when it's within MSRV
+				if let PaymentSendFailure::AllFailedResendSafe(_) = e {
+					self.all_failed_remove_outbound(payment_id);
+				}
+				e
+			})
 	}
 
 	pub(super) fn send_spontaneous_payment<ES: Deref, NS: Deref, F>(
@@ -402,7 +409,12 @@ impl OutboundPayments {
 
 		match self.send_payment_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),
-			Err(e) => Err(e)
+			Err(e) => {
+				if let PaymentSendFailure::AllFailedResendSafe(_) = e {
+					self.all_failed_remove_outbound(payment_id);
+				}
+				Err(e)
+			}
 		}
 	}
 
@@ -501,7 +513,12 @@ impl OutboundPayments {
 
 		match self.send_payment_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)),
-			Err(e) => Err(e)
+			Err(e) => {
+				if let PaymentSendFailure::AllFailedResendSafe(_) = e {
+					self.all_failed_remove_outbound(payment_id);
+				}
+				Err(e)
+			}
 		}
 	}
 
@@ -648,10 +665,6 @@ impl OutboundPayments {
 				} else { None },
 			})
 		} else if has_err {
-			// If we failed to send any paths, we should remove the new PaymentId from the
-			// `pending_outbound_payments` map, as the user isn't expected to `abandon_payment`.
-			let removed = self.pending_outbound_payments.lock().unwrap().remove(&payment_id).is_some();
-			debug_assert!(removed, "We should always have a pending payment to remove here");
 			Err(PaymentSendFailure::AllFailedResendSafe(results.drain(..).map(|r| r.unwrap_err()).collect()))
 		} else {
 			Ok(())
@@ -673,6 +686,19 @@ impl OutboundPayments {
 		self.send_payment_internal(route, payment_hash, payment_secret, keysend_preimage, payment_id,
 			recv_value_msat, onion_session_privs, node_signer, best_block_height,
 			send_payment_along_path)
+			.map_err(|e| { // TODO: use inspect_err instead when it's within MSRV
+				if let PaymentSendFailure::AllFailedResendSafe(_) = e {
+					self.all_failed_remove_outbound(payment_id);
+				}
+				e
+			})
+	}
+
+	// If we failed to send any paths, we should remove the new PaymentId from the
+	// `pending_outbound_payments` map, as the user isn't expected to `abandon_payment`.
+	fn all_failed_remove_outbound(&self, payment_id: PaymentId) {
+		let removed = self.pending_outbound_payments.lock().unwrap().remove(&payment_id).is_some();
+		debug_assert!(removed, "We should always have a pending payment to remove here");
 	}
 
 	pub(super) fn claim_htlc<L: Deref>(