Move forward_htlcs into standalone lock

ViktorTigerstrom · ViktorTigerstrom · commit df12df354ebd · 2022-09-18T23:13:56.000+02:00
As we are eventually removing the `channel_state` lock, this commit
moves the `forward_htlcs` map out of the `channel_state` lock, to ease
that process.
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -401,16 +401,6 @@ pub(super) struct ChannelHolder<Signer: Sign> {
 	/// SCIDs being added once the funding transaction is confirmed at the channel's required
 	/// confirmation depth.
 	pub(super) short_to_chan_info: HashMap<u64, (PublicKey, [u8; 32])>,
-	/// SCID/SCID Alias -> forward infos. Key of 0 means payments received.
-	///
-	/// Note that because we may have an SCID Alias as the key we can have two entries per channel,
-	/// though in practice we probably won't be receiving HTLCs for a channel both via the alias
-	/// and via the classic SCID.
-	///
-	/// Note that while this is held in the same mutex as the channels themselves, no consistency
-	/// guarantees are made about the existence of a channel with the short id here, nor the short
-	/// ids in the PendingHTLCInfo!
-	pub(super) forward_htlcs: HashMap<u64, Vec<HTLCForwardInfo>>,
 	/// Map from payment hash to the payment data and any HTLCs which are to us and can be
 	/// failed/claimed by the user.
 	///
@@ -722,6 +712,19 @@ pub struct ChannelManager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref,
 	/// Locked *after* channel_state.
 	pending_outbound_payments: Mutex<HashMap<PaymentId, PendingOutboundPayment>>,
 
+	/// SCID/SCID Alias -> forward infos. Key of 0 means payments received.
+	///
+	/// Note that because we may have an SCID Alias as the key we can have two entries per channel,
+	/// though in practice we probably won't be receiving HTLCs for a channel both via the alias
+	/// and via the classic SCID.
+	///
+	/// Note that no consistency guarantees are made about the existence of a channel with the
+	/// `short_channel_id` here, nor the `short_channel_id` in the `PendingHTLCInfo`!
+	#[cfg(test)]
+	pub(super) forward_htlcs: Mutex<HashMap<u64, Vec<HTLCForwardInfo>>>,
+	#[cfg(not(test))]
+	forward_htlcs: Mutex<HashMap<u64, Vec<HTLCForwardInfo>>>,
+
 	/// The set of outbound SCID aliases across all our channels, including unconfirmed channels
 	/// and some closed channels which reached a usable state prior to being closed. This is used
 	/// only to avoid duplicates, and is not persisted explicitly to disk, but rebuilt from the
@@ -1595,13 +1598,13 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			channel_state: Mutex::new(ChannelHolder{
 				by_id: HashMap::new(),
 				short_to_chan_info: HashMap::new(),
-				forward_htlcs: HashMap::new(),
 				claimable_htlcs: HashMap::new(),
 				pending_msg_events: Vec::new(),
 			}),
 			outbound_scid_aliases: Mutex::new(HashSet::new()),
 			pending_inbound_payments: Mutex::new(HashMap::new()),
 			pending_outbound_payments: Mutex::new(HashMap::new()),
+			forward_htlcs: Mutex::new(HashMap::new()),
 			id_to_peer: Mutex::new(HashMap::new()),
 
 			our_network_key: keys_manager.get_node_secret(Recipient::Node).unwrap(),
@@ -3005,7 +3008,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			let mut channel_state_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_state_lock;
 
-			for (short_chan_id, mut pending_forwards) in channel_state.forward_htlcs.drain() {
+			let mut forward_htlcs = HashMap::new();
+			mem::swap(&mut forward_htlcs, &mut self.forward_htlcs.lock().unwrap());
+
+			for (short_chan_id, mut pending_forwards) in forward_htlcs {
 				if short_chan_id != 0 {
 					let forward_chan_id = match channel_state.short_to_chan_info.get(&short_chan_id) {
 						Some((_cp_id, chan_id)) => chan_id.clone(),
@@ -3904,17 +3910,19 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				};
 
 				let mut forward_event = None;
-				if channel_state_lock.forward_htlcs.is_empty() {
+				let mut forward_htlcs = self.forward_htlcs.lock().unwrap();
+				if forward_htlcs.is_empty() {
 					forward_event = Some(Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS));
 				}
-				match channel_state_lock.forward_htlcs.entry(short_channel_id) {
+				match forward_htlcs.entry(short_channel_id) {
 					hash_map::Entry::Occupied(mut entry) => {
 						entry.get_mut().push(HTLCForwardInfo::FailHTLC { htlc_id, err_packet });
 					},
 					hash_map::Entry::Vacant(entry) => {
 						entry.insert(vec!(HTLCForwardInfo::FailHTLC { htlc_id, err_packet }));
 					}
 				}
+				mem::drop(forward_htlcs);
 				mem::drop(channel_state_lock);
 				let mut pending_events = self.pending_events.lock().unwrap();
 				if let Some(time) = forward_event {
@@ -4862,11 +4870,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			let mut forward_event = None;
 			if !pending_forwards.is_empty() {
 				let mut channel_state = self.channel_state.lock().unwrap();
-				if channel_state.forward_htlcs.is_empty() {
+				let mut forward_htlcs = self.forward_htlcs.lock().unwrap();
+				if forward_htlcs.is_empty() {
 					forward_event = Some(Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS))
 				}
 				for (forward_info, prev_htlc_id) in pending_forwards.drain(..) {
-					match channel_state.forward_htlcs.entry(match forward_info.routing {
+					match forward_htlcs.entry(match forward_info.routing {
 							PendingHTLCRouting::Forward { short_channel_id, .. } => short_channel_id,
 							PendingHTLCRouting::Receive { .. } => 0,
 							PendingHTLCRouting::ReceiveKeysend { .. } => 0,
@@ -6552,8 +6561,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
 			}
 		}
 
-		(channel_state.forward_htlcs.len() as u64).write(writer)?;
-		for (short_channel_id, pending_forwards) in channel_state.forward_htlcs.iter() {
+		let forward_htlcs = self.forward_htlcs.lock().unwrap();
+		(forward_htlcs.len() as u64).write(writer)?;
+		for (short_channel_id, pending_forwards) in forward_htlcs.iter() {
 			short_channel_id.write(writer)?;
 			(pending_forwards.len() as u64).write(writer)?;
 			for forward in pending_forwards {
@@ -7165,14 +7175,14 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 			channel_state: Mutex::new(ChannelHolder {
 				by_id,
 				short_to_chan_info,
-				forward_htlcs,
 				claimable_htlcs,
 				pending_msg_events: Vec::new(),
 			}),
 			inbound_payment_key: expanded_inbound_key,
 			pending_inbound_payments: Mutex::new(pending_inbound_payments),
 			pending_outbound_payments: Mutex::new(pending_outbound_payments.unwrap()),
 
+			forward_htlcs: Mutex::new(forward_htlcs),
 			outbound_scid_aliases: Mutex::new(outbound_scid_aliases),
 			id_to_peer: Mutex::new(id_to_peer),
 			fake_scid_rand_bytes: fake_scid_rand_bytes.unwrap(),
diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs
@@ -540,7 +540,7 @@ fn test_onion_failure() {
 	}, || {}, true, Some(17), None, None);
 
 	run_onion_failure_test("final_incorrect_cltv_expiry", 1, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || {
-		for (_, pending_forwards) in nodes[1].node.channel_state.lock().unwrap().forward_htlcs.iter_mut() {
+		for (_, pending_forwards) in nodes[1].node.forward_htlcs.lock().unwrap().iter_mut() {
 			for f in pending_forwards.iter_mut() {
 				match f {
 					&mut HTLCForwardInfo::AddHTLC { ref mut forward_info, .. } =>
@@ -553,7 +553,7 @@ fn test_onion_failure() {
 
 	run_onion_failure_test("final_incorrect_htlc_amount", 1, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || {
 		// violate amt_to_forward > msg.amount_msat
-		for (_, pending_forwards) in nodes[1].node.channel_state.lock().unwrap().forward_htlcs.iter_mut() {
+		for (_, pending_forwards) in nodes[1].node.forward_htlcs.lock().unwrap().iter_mut() {
 			for f in pending_forwards.iter_mut() {
 				match f {
 					&mut HTLCForwardInfo::AddHTLC { ref mut forward_info, .. } =>
@@ -1021,8 +1021,8 @@ fn test_phantom_onion_hmac_failure() {
 
 	// Modify the payload so the phantom hop's HMAC is bogus.
 	let sha256_of_onion = {
-		let mut channel_state = nodes[1].node.channel_state.lock().unwrap();
-		let mut pending_forward = channel_state.forward_htlcs.get_mut(&phantom_scid).unwrap();
+		let mut forward_htlcs = nodes[1].node.forward_htlcs.lock().unwrap();
+		let mut pending_forward = forward_htlcs.get_mut(&phantom_scid).unwrap();
 		match pending_forward[0] {
 			HTLCForwardInfo::AddHTLC {
 				forward_info: PendingHTLCInfo {
@@ -1081,7 +1081,7 @@ fn test_phantom_invalid_onion_payload() {
 	commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);
 
 	// Modify the onion packet to have an invalid payment amount.
-	for (_, pending_forwards) in nodes[1].node.channel_state.lock().unwrap().forward_htlcs.iter_mut() {
+	for (_, pending_forwards) in nodes[1].node.forward_htlcs.lock().unwrap().iter_mut() {
 		for f in pending_forwards.iter_mut() {
 			match f {
 				&mut HTLCForwardInfo::AddHTLC {
@@ -1152,7 +1152,7 @@ fn test_phantom_final_incorrect_cltv_expiry() {
 	commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);
 
 	// Modify the payload so the phantom hop's HMAC is bogus.
-	for (_, pending_forwards) in nodes[1].node.channel_state.lock().unwrap().forward_htlcs.iter_mut() {
+	for (_, pending_forwards) in nodes[1].node.forward_htlcs.lock().unwrap().iter_mut() {
 		for f in pending_forwards.iter_mut() {
 			match f {
 				&mut HTLCForwardInfo::AddHTLC {
