Handle short_to_id state updates on channel closure via macros

This avoids needing to update channel closure code in many places
as we add multiple SCIDs for each channel and have to track them.

Author: TheBlueMatt

lightning/src/ln/channelmanager.rs

@@ -1405,6 +1405,14 @@ macro_rules! handle_error {
 	}
 }
 
+macro_rules! update_maps_on_chan_removal {
+	($short_to_id: expr, $channel: expr) => {
+		if let Some(short_id) = $channel.get_short_channel_id() {
+			$short_to_id.remove(&short_id);
+		}
+	}
+}
+
 /// Returns (boolean indicating if we should remove the Channel object from memory, a mapped error)
 macro_rules! convert_chan_err {
 	($self: ident, $err: expr, $short_to_id: expr, $channel: expr, $channel_id: expr) => {
@@ -1417,18 +1425,14 @@ macro_rules! convert_chan_err {
 			},
 			ChannelError::Close(msg) => {
 				log_error!($self.logger, "Closing channel {} due to close-required error: {}", log_bytes!($channel_id[..]), msg);
-				if let Some(short_id) = $channel.get_short_channel_id() {
-					$short_to_id.remove(&short_id);
-				}
+				update_maps_on_chan_removal!($short_to_id, $channel);
 				let shutdown_res = $channel.force_shutdown(true);
 				(true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, $channel.get_user_id(),
 					shutdown_res, $self.get_channel_update_for_broadcast(&$channel).ok()))
 			},
 			ChannelError::CloseDelayBroadcast(msg) => {
 				log_error!($self.logger, "Channel {} need to be shutdown but closing transactions not broadcast due to {}", log_bytes!($channel_id[..]), msg);
-				if let Some(short_id) = $channel.get_short_channel_id() {
-					$short_to_id.remove(&short_id);
-				}
+				update_maps_on_chan_removal!($short_to_id, $channel);
 				let shutdown_res = $channel.force_shutdown(false);
 				(true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, $channel.get_user_id(),
 					shutdown_res, $self.get_channel_update_for_broadcast(&$channel).ok()))
@@ -1471,9 +1475,7 @@ macro_rules! remove_channel {
 	($channel_state: expr, $entry: expr) => {
 		{
 			let channel = $entry.remove_entry().1;
-			if let Some(short_id) = channel.get_short_channel_id() {
-				$channel_state.short_to_id.remove(&short_id);
-			}
+			update_maps_on_chan_removal!($channel_state.short_to_id, channel);
 			channel
 		}
 	}
@@ -1484,9 +1486,7 @@ macro_rules! handle_monitor_err {
 		match $err {
 			ChannelMonitorUpdateErr::PermanentFailure => {
 				log_error!($self.logger, "Closing channel {} due to monitor update ChannelMonitorUpdateErr::PermanentFailure", log_bytes!($chan_id[..]));
-				if let Some(short_id) = $chan.get_short_channel_id() {
-					$short_to_id.remove(&short_id);
-				}
+				update_maps_on_chan_removal!($short_to_id, $chan);
 				// TODO: $failed_fails is dropped here, which will cause other channels to hit the
 				// chain in a confused state! We need to move them into the ChannelMonitor which
 				// will be responsible for failing backwards once things confirm on-chain.
@@ -2049,17 +2049,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						return Err(APIError::ChannelUnavailable{err: "No such channel".to_owned()});
 					}
 				}
-				if let Some(short_id) = chan.get().get_short_channel_id() {
-					channel_state.short_to_id.remove(&short_id);
-				}
 				if peer_node_id.is_some() {
 					if let Some(peer_msg) = peer_msg {
 						self.issue_channel_close_events(chan.get(),ClosureReason::CounterpartyForceClosed { peer_msg: peer_msg.to_string() });
 					}
 				} else {
 					self.issue_channel_close_events(chan.get(),ClosureReason::HolderForceClosed);
 				}
-				chan.remove_entry().1
+				remove_channel!(channel_state, chan)
 			} else {
 				return Err(APIError::ChannelUnavailable{err: "No such channel".to_owned()});
 			}
@@ -3206,12 +3203,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 										}
 										ChannelError::Close(msg) => {
 											log_trace!(self.logger, "Closing channel {} due to Close-required error: {}", log_bytes!(chan.key()[..]), msg);
-											let (channel_id, mut channel) = chan.remove_entry();
-											if let Some(short_id) = channel.get_short_channel_id() {
-												channel_state.short_to_id.remove(&short_id);
-											}
+											let mut channel = remove_channel!(channel_state, chan);
 											// ChannelClosed event is generated by handle_error for us.
-											Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, channel.get_user_id(), channel.force_shutdown(true), self.get_channel_update_for_broadcast(&channel).ok()))
+											Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel.channel_id(), channel.get_user_id(), channel.force_shutdown(true), self.get_channel_update_for_broadcast(&channel).ok()))
 										},
 										ChannelError::CloseDelayBroadcast(_) => { panic!("Wait is only generated on receipt of channel_reestablish, which is handled by try_chan_entry, we don't bother to support it here"); }
 									};
@@ -4479,10 +4473,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						// also implies there are no pending HTLCs left on the channel, so we can
 						// fully delete it from tracking (the channel monitor is still around to
 						// watch for old state broadcasts)!
-						if let Some(short_id) = chan_entry.get().get_short_channel_id() {
-							channel_state.short_to_id.remove(&short_id);
-						}
-						(tx, Some(chan_entry.remove_entry().1))
+						(tx, Some(remove_channel!(channel_state, chan_entry)))
 					} else { (tx, None) }
 				},
 				hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
@@ -4920,12 +4911,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					let mut channel_lock = self.channel_state.lock().unwrap();
 					let channel_state = &mut *channel_lock;
 					let by_id = &mut channel_state.by_id;
-					let short_to_id = &mut channel_state.short_to_id;
 					let pending_msg_events = &mut channel_state.pending_msg_events;
-					if let Some(mut chan) = by_id.remove(&funding_outpoint.to_channel_id()) {
-						if let Some(short_id) = chan.get_short_channel_id() {
-							short_to_id.remove(&short_id);
-						}
+					if let hash_map::Entry::Occupied(chan_entry) = by_id.entry(funding_outpoint.to_channel_id()) {
+						let mut chan = remove_channel!(channel_state, chan_entry);
 						failed_channels.push(chan.force_shutdown(false));
 						if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
 							pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
@@ -5054,10 +5042,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 						if let Some(tx) = tx_opt {
 							// We're done with this channel. We got a closing_signed and sent back
 							// a closing_signed with a closing transaction to broadcast.
-							if let Some(short_id) = chan.get_short_channel_id() {
-								short_to_id.remove(&short_id);
-							}
-
 							if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
 								pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
 									msg: update
@@ -5068,6 +5052,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
 							log_info!(self.logger, "Broadcasting {}", log_tx!(tx));
 							self.tx_broadcaster.broadcast_transaction(&tx);
+							update_maps_on_chan_removal!(short_to_id, chan);
 							false
 						} else { true }
 					},
@@ -5587,9 +5572,7 @@ where
 						}
 					}
 				} else if let Err(reason) = res {
-					if let Some(short_id) = channel.get_short_channel_id() {
-						short_to_id.remove(&short_id);
-					}
+					update_maps_on_chan_removal!(short_to_id, channel);
 					// It looks like our counterparty went on-chain or funding transaction was
 					// reorged out of the main chain. Close the channel.
 					failed_channels.push(channel.force_shutdown(true));
@@ -5785,9 +5768,7 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 				log_debug!(self.logger, "Failing all channels with {} due to no_connection_possible", log_pubkey!(counterparty_node_id));
 				channel_state.by_id.retain(|_, chan| {
 					if chan.get_counterparty_node_id() == *counterparty_node_id {
-						if let Some(short_id) = chan.get_short_channel_id() {
-							short_to_id.remove(&short_id);
-						}
+						update_maps_on_chan_removal!(short_to_id, chan);
 						failed_channels.push(chan.force_shutdown(true));
 						if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
 							pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
@@ -5806,9 +5787,7 @@ impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
 					if chan.get_counterparty_node_id() == *counterparty_node_id {
 						chan.remove_uncommitted_htlcs_and_mark_paused(&self.logger);
 						if chan.is_shutdown() {
-							if let Some(short_id) = chan.get_short_channel_id() {
-								short_to_id.remove(&short_id);
-							}
+							update_maps_on_chan_removal!(short_to_id, chan);
 							self.issue_channel_close_events(chan, ClosureReason::DisconnectedPeer);
 							return false;
 						} else {