From: Valentine Wallace <vwallace@protonmail.com>
Date: Fri, 3 Feb 2023 17:49:07 +0000 (-0500)
Subject: Abandon payment if retry fails in process_pending_htlcs
X-Git-Tag: v0.0.114-beta~17^2~4
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=07dd8b2794be52734ba0106c3fac395ff0cb2853;p=rust-lightning

Abandon payment if retry fails in process_pending_htlcs
---

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35a3c96a..671b80ac 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3370,7 +3370,8 @@ where
 
 		let best_block_height = self.best_block.read().unwrap().height();
 		self.pending_outbound_payments.check_retry_payments(&self.router, || self.list_usable_channels(),
-			|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height, &self.logger,
+			|| self.compute_inflight_htlcs(), &self.entropy_source, &self.node_signer, best_block_height,
+			&self.pending_events, &self.logger,
 			|path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv|
 			self.send_payment_along_path(path, payment_params, payment_hash, payment_secret, total_value, cur_height, payment_id, keysend_preimage, session_priv));
 
diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs
index 05987e75..dd5cd892 100644
--- a/lightning/src/ln/outbound_payment.rs
+++ b/lightning/src/ln/outbound_payment.rs
@@ -491,7 +491,8 @@ impl OutboundPayments {
 
 	pub(super) fn check_retry_payments<R: Deref, ES: Deref, NS: Deref, SP, IH, FH, L: Deref>(
 		&self, router: &R, first_hops: FH, inflight_htlcs: IH, entropy_source: &ES, node_signer: &NS,
-		best_block_height: u32, logger: &L, send_payment_along_path: SP,
+		best_block_height: u32, pending_events: &Mutex<Vec<events::Event>>, logger: &L,
+		send_payment_along_path: SP,
 	)
 	where
 		R::Target: Router,
@@ -525,15 +526,19 @@ impl OutboundPayments {
 					}
 				}
 			}
+			core::mem::drop(outbounds);
 			if let Some((payment_id, route_params)) = retry_id_route_params {
-				core::mem::drop(outbounds);
 				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) {
 					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.
+					self.abandon_payment(payment_id, pending_events);
 				}
 			} else { break }
 		}
 	}
 
+	/// 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)>,
diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs
index b4cb379c..fcd57d8f 100644
--- a/lightning/src/ln/payment_tests.rs
+++ b/lightning/src/ln/payment_tests.rs
@@ -1615,6 +1615,7 @@ enum AutoRetry {
 	FailAttempts,
 	FailTimeout,
 	FailOnRestart,
+	FailOnRetry,
 }
 
 #[test]
@@ -1624,6 +1625,7 @@ fn automatic_retries() {
 	do_automatic_retries(AutoRetry::FailAttempts);
 	do_automatic_retries(AutoRetry::FailTimeout);
 	do_automatic_retries(AutoRetry::FailOnRestart);
+	do_automatic_retries(AutoRetry::FailOnRetry);
 }
 fn do_automatic_retries(test: AutoRetry) {
 	// Test basic automatic payment retries in ChannelManager. See individual `test` variant comments
@@ -1812,6 +1814,25 @@ fn do_automatic_retries(test: AutoRetry) {
 		assert_eq!(msg_events.len(), 0);
 
 		nodes[0].node.abandon_payment(PaymentId(payment_hash.0));
+		let mut events = nodes[0].node.get_and_clear_pending_events();
+		assert_eq!(events.len(), 1);
+		match events[0] {
+			Event::PaymentFailed { payment_hash: ref ev_payment_hash, payment_id: ref ev_payment_id } => {
+				assert_eq!(payment_hash, *ev_payment_hash);
+				assert_eq!(PaymentId(payment_hash.0), *ev_payment_id);
+			},
+			_ => panic!("Unexpected event"),
+		}
+	} else if test == AutoRetry::FailOnRetry {
+		nodes[0].node.send_payment_with_retry(payment_hash, &Some(payment_secret), PaymentId(payment_hash.0), route_params, Retry::Attempts(1)).unwrap();
+		pass_failed_attempt_with_retry_along_path!(channel_id_2, true);
+
+		// We retry payments in `process_pending_htlc_forwards`. Since our channel closed, we should
+		// fail to find a route.
+		nodes[0].node.process_pending_htlc_forwards();
+		let mut msg_events = nodes[0].node.get_and_clear_pending_msg_events();
+		assert_eq!(msg_events.len(), 0);
+
 		let mut events = nodes[0].node.get_and_clear_pending_events();
 		assert_eq!(events.len(), 1);
 		match events[0] {