Abandon payment if retry fails in process_pending_htlcs

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 35a3c96a393486f0bcdf5872f7062bccdc125bba..671b80aca81d538c5e8fb78465341b905018808a 100644 (file)
--- 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 05987e7505d24c32cd7afb0100e18c82a26333dd..dd5cd892648dccd9d2803ed96b5f2e6965062de0 100644 (file)
--- 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 b4cb379c4adf17c32911a92e584cb040cbc4fe63..fcd57d8f0ad1f30e22bead96491da649872695de 100644 (file)
--- 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] {