Fix a minor memory leak on PermanentFailure mon errs when sending

If we send a payment and fail to update the first-hop channel state
with a `PermanentFailure` ChannelMonitorUpdateErr, we would have an
entry in our pending payments map, but possibly not return the
PaymentId back to the user to retry the payment, leading to a (rare
and relatively minor) memory leak.

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 763a293fa5bcae790e21c7e2b5206c1e0d31ad76..444d443facf98dbb2915172a33c754ea42abb0fe 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2075,6 +2075,21 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                Some(id) => id.clone(),
                        };
 
+                       macro_rules! insert_outbound_payment {
+                               () => {
+                                       let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
+                                               session_privs: HashSet::new(),
+                                               pending_amt_msat: 0,
+                                               pending_fee_msat: Some(0),
+                                               payment_hash: *payment_hash,
+                                               payment_secret: *payment_secret,
+                                               starting_block_height: self.best_block.read().unwrap().height(),
+                                               total_msat: total_value,
+                                       });
+                                       assert!(payment.insert(session_priv_bytes, path));
+                               }
+                       }
+
                        let channel_state = &mut *channel_lock;
                        if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
                                match {
@@ -2084,7 +2099,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        if !chan.get().is_live() {
                                                return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!".to_owned()});
                                        }
-                                       let send_res = break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(
+                                       break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(
                                                htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
                                                        path: path.clone(),
                                                        session_priv: session_priv.clone(),
@@ -2093,20 +2108,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        payment_secret: payment_secret.clone(),
                                                        payee: payee.clone(),
                                                }, onion_packet, &self.logger),
-                                       channel_state, chan);
-
-                                       let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
-                                               session_privs: HashSet::new(),
-                                               pending_amt_msat: 0,
-                                               pending_fee_msat: Some(0),
-                                               payment_hash: *payment_hash,
-                                               payment_secret: *payment_secret,
-                                               starting_block_height: self.best_block.read().unwrap().height(),
-                                               total_msat: total_value,
-                                       });
-                                       assert!(payment.insert(session_priv_bytes, path));
-
-                                       send_res
+                                       channel_state, chan)
                                } {
                                        Some((update_add, commitment_signed, monitor_update)) => {
                                                if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
@@ -2116,8 +2118,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        // is restored. Therefore, we must return an error indicating that
                                                        // it is unsafe to retry the payment wholesale, which we do in the
                                                        // send_payment check for MonitorUpdateFailed, below.
+                                                       insert_outbound_payment!(); // Only do this after possibly break'ing on Perm failure above.
                                                        return Err(APIError::MonitorUpdateFailed);
                                                }
+                                               insert_outbound_payment!();
 
                                                log_debug!(self.logger, "Sending payment along path resulted in a commitment_signed for channel {}", log_bytes!(chan.get().channel_id()));
                                                channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
@@ -2132,7 +2136,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        },
                                                });
                                        },
-                                       None => {},
+                                       None => { insert_outbound_payment!(); },
                                }
                        } else { unreachable!(); }
                        return Ok(());
@@ -2249,6 +2253,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                if has_err && has_ok {
                        Err(PaymentSendFailure::PartialFailure(results))
                } else if has_err {
+                       // If we failed to send any paths, we shouldn't have inserted the new PaymentId into
+                       // our `pending_outbound_payments` map at all.
+                       debug_assert!(self.pending_outbound_payments.lock().unwrap().get(&payment_id).is_none());
                        Err(PaymentSendFailure::AllFailedRetrySafe(results.drain(..).map(|r| r.unwrap_err()).collect()))
                } else {
                        Ok(payment_id)