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 the
user not being able to retry their payment as we'll consider it
still "in-flight".

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]>,
 	},
@@ -557,6 +557,9 @@ pub struct ChannelManager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref,
 	/// See `PendingOutboundPayment` documentation for more info.
 	///
 	/// Locked *after* channel_state.
+	#[cfg(test)] // Exposed so that we can check there aren't dangling entries
+	pub(crate) pending_outbound_payments: Mutex<HashMap<PaymentId, PendingOutboundPayment>>,
+	#[cfg(not(test))]
 	pending_outbound_payments: Mutex<HashMap<PaymentId, PendingOutboundPayment>>,
 
 	our_network_key: SecretKey,
@@ -1951,16 +1954,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 +1971,28 @@ 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) {

lightning/src/ln/payment_tests.rs

@@ -225,3 +225,30 @@ fn retry_expired_payment() {
 		panic!("Unexpected error");
 	}
 }
+
+#[test]
+fn retry_on_init_fail() {
+	// 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 (eg 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 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.pending_outbound_payments.lock().unwrap().is_empty());
+}