Move pending payment tracking to after the new HTLC flies

If we attempt to send a payment, but the HTLC cannot be send due to
local channel limits, we'll provide the user an error but end up
with an entry in our pending payment map. This will result in a
memory leak as we'll never reclaim the pending payment map entry.

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -402,7 +402,7 @@ struct PendingInboundPayment {
 
 /// Stores the session_priv for each part of a payment that is still pending. For versions 0.0.102
 /// and later, also stores information for retrying the payment.
-enum PendingOutboundPayment {
+pub(crate) enum PendingOutboundPayment {
 	Legacy {
 		session_privs: HashSet<[u8; 32]>,
 	},
@@ -1951,16 +1951,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash);
 
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
-		let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
-		let payment = pending_outbounds.entry(payment_id).or_insert_with(|| PendingOutboundPayment::Retryable {
-			session_privs: HashSet::new(),
-			pending_amt_msat: 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.last().unwrap().fee_msat));
 
 		let err: Result<(), _> = loop {
 			let mut channel_lock = self.channel_state.lock().unwrap();
@@ -1978,12 +1968,27 @@ 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()});
 					}
-					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(),
-						first_hop_htlc_msat: htlc_msat,
-						payment_id,
-					}, onion_packet, &self.logger), channel_state, chan)
+					let send_res = 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(),
+							first_hop_htlc_msat: htlc_msat,
+							payment_id,
+						}, onion_packet, &self.logger),
+					channel_state, chan);
+
+					let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
+					let payment = pending_outbounds.entry(payment_id).or_insert_with(|| PendingOutboundPayment::Retryable {
+						session_privs: HashSet::new(),
+						pending_amt_msat: 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.last().unwrap().fee_msat));
+
+					send_res
 				} {
 					Some((update_add, commitment_signed, monitor_update)) => {
 						if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
@@ -4383,6 +4388,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		self.process_pending_events(&event_handler);
 		events.into_inner()
 	}
+
+	#[cfg(test)]
+	pub fn has_pending_payments(&self) -> bool {
+		!self.pending_outbound_payments.lock().unwrap().is_empty()
+	}
 }
 
 impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> MessageSendEventsProvider for ChannelManager<Signer, M, T, K, F, L>

lightning/src/ln/payment_tests.rs

@@ -225,3 +225,30 @@ fn retry_expired_payment() {
 		panic!("Unexpected error");
 	}
 }
+
+#[test]
+fn no_pending_leak_on_initial_send_failure() {
+	// In an earlier version of our payment tracking, we'd have a retry entry even when the initial
+	// HTLC for payment failed to send due to local channel errors (e.g. peer disconnected). In this
+	// case, the user wouldn't have a PaymentId to retry the payment with, but we'd think we have a
+	// pending payment forever and never time it out.
+	// Here we test exactly that - retrying a payment when a peer was disconnected on the first
+	// try, and then check that no pending payment is being tracked.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+
+	let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000);
+
+	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+	nodes[1].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+
+	unwrap_send_err!(nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)),
+		true, APIError::ChannelUnavailable { ref err },
+		assert_eq!(err, "Peer for first hop currently disconnected/pending monitor update!"));
+
+	assert!(!nodes[0].node.has_pending_payments());
+}