f various dedup fixes, enum helper, etc

Author: dunxen

lightning/src/ln/channelmanager.rs

@@ -2073,10 +2073,15 @@ macro_rules! handle_new_monitor_update {
 	($self: ident, $update_res: expr, $peer_state_lock: expr, $peer_state: expr, $per_peer_state_lock: expr, $chan_entry: expr, INITIAL_MONITOR) => {
 		if let ChannelPhase::Funded(chan) = $chan_entry.get_mut() {
 			handle_new_monitor_update!($self, $update_res, $peer_state_lock, $peer_state, $per_peer_state_lock, chan, MANUALLY_REMOVING_INITIAL_MONITOR, { 
-				if let ChannelPhase::Funded(chan) = $chan_entry.remove() { chan } else { unreachable!() }
+				unwrap_enum_value!($chan_entry.remove(), ChannelPhase::Funded(chan) => chan)
 			})
 		} else {
-			unreachable!();
+			// We're not supposed to handle monitor updates for unfunded channels (they have no monitors to
+			// update).
+			debug_assert!(true);
+			Err(MsgHandleErrInternal::send_err_msg_no_close(
+				"Cannot update monitor for unfunded channels as they don't have monitors yet".into(),
+				*$chan_entry.key()))
 		}
 	};
 	($self: ident, $funding_txo: expr, $update: expr, $peer_state_lock: expr, $peer_state: expr, $per_peer_state_lock: expr, $chan: expr, MANUALLY_REMOVING, $remove: expr) => { {
@@ -2529,47 +2534,38 @@ where
 
 				match peer_state.channel_by_id.entry(channel_id.clone()) {
 					hash_map::Entry::Occupied(mut chan_phase_entry) => {
-						match chan_phase_entry.get_mut() {
-							ChannelPhase::Funded(chan) => {
-								let funding_txo_opt = chan.context.get_funding_txo();
-								let their_features = &peer_state.latest_features;
-								let (shutdown_msg, mut monitor_update_opt, htlcs) =
-									chan.get_shutdown(&self.signer_provider, their_features, target_feerate_sats_per_1000_weight, override_shutdown_script)?;
-								failed_htlcs = htlcs;
-
-								// We can send the `shutdown` message before updating the `ChannelMonitor`
-								// here as we don't need the monitor update to complete until we send a
-								// `shutdown_signed`, which we'll delay if we're pending a monitor update.
-								peer_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
-									node_id: *counterparty_node_id,
-									msg: shutdown_msg,
-								});
+						if let ChannelPhase::Funded(chan) = chan_phase_entry.get_mut() {
+							let funding_txo_opt = chan.context.get_funding_txo();
+							let their_features = &peer_state.latest_features;
+							let (shutdown_msg, mut monitor_update_opt, htlcs) =
+								chan.get_shutdown(&self.signer_provider, their_features, target_feerate_sats_per_1000_weight, override_shutdown_script)?;
+							failed_htlcs = htlcs;
+
+							// We can send the `shutdown` message before updating the `ChannelMonitor`
+							// here as we don't need the monitor update to complete until we send a
+							// `shutdown_signed`, which we'll delay if we're pending a monitor update.
+							peer_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
+								node_id: *counterparty_node_id,
+								msg: shutdown_msg,
+							});
 
-								// Update the monitor with the shutdown script if necessary.
-								if let Some(monitor_update) = monitor_update_opt.take() {
-									break handle_new_monitor_update!(self, funding_txo_opt.unwrap(), monitor_update,
-										peer_state_lock, peer_state, per_peer_state, chan_phase_entry).map(|_| ());
-								}
+							// Update the monitor with the shutdown script if necessary.
+							if let Some(monitor_update) = monitor_update_opt.take() {
+								break handle_new_monitor_update!(self, funding_txo_opt.unwrap(), monitor_update,
+									peer_state_lock, peer_state, per_peer_state, chan_phase_entry).map(|_| ());
+							}
 
-								if chan.is_shutdown() {
-									if let ChannelPhase::Funded(chan) = remove_channel_phase!(self, chan_phase_entry) {
-										if let Ok(channel_update) = self.get_channel_update_for_broadcast(&chan) {
-											peer_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
-												msg: channel_update
-											});
-										}
-										self.issue_channel_close_events(&chan.context, ClosureReason::HolderForceClosed);
+							if chan.is_shutdown() {
+								if let ChannelPhase::Funded(chan) = remove_channel_phase!(self, chan_phase_entry) {
+									if let Ok(channel_update) = self.get_channel_update_for_broadcast(&chan) {
+										peer_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
+											msg: channel_update
+										});
 									}
+									self.issue_channel_close_events(&chan.context, ClosureReason::HolderForceClosed);
 								}
-								break Ok(());
-							},
-							_ => {
-								// If we reach this point, it means that the channel_id either refers to an unfunded channel or
-								// it does not exist for this peer. Either way, we can attempt to force-close it.
-								//
-								// An appropriate error will be returned for non-existence of the channel if that's the case.
-								return self.force_close_channel_with_peer(&channel_id, counterparty_node_id, None, false).map(|_| ())
-							},
+							}
+							break Ok(());
 						}
 					},
 					hash_map::Entry::Vacant(_) => (),
@@ -2580,8 +2576,6 @@ where
 			//
 			// An appropriate error will be returned for non-existence of the channel if that's the case.
 			return self.force_close_channel_with_peer(&channel_id, counterparty_node_id, None, false).map(|_| ())
-			// TODO(dunxen): This is still not ideal as we're doing some extra lookups.
-			// Fix this with https://github.com/lightningdevkit/rust-lightning/issues/2422
 		};
 
 		for htlc_source in failed_htlcs.drain(..) {
@@ -3043,10 +3037,13 @@ where
 				}
 				let mut peer_state_lock = peer_state_mutex_opt.unwrap().lock().unwrap();
 				let peer_state = &mut *peer_state_lock;
-				let opt_chan_phase = peer_state.channel_by_id.get_mut(&forwarding_id);
-				let chan = match opt_chan_phase.iter().find_map(
+				let chan = match peer_state.channel_by_id.get_mut(&forwarding_id).map(
 					|chan_phase| if let ChannelPhase::Funded(chan) = chan_phase { Some(chan) } else { None }
-				) {
+				).flatten() {
+				// let opt_chan_phase = peer_state.channel_by_id.get_mut(&forwarding_id);
+				// let chan = match opt_chan_phase.iter().find_map(
+				// 	|chan_phase| if let ChannelPhase::Funded(chan) = chan_phase { Some(chan) } else { None }
+				// ) {
 					None => {
 						// Channel was removed. The short_to_chan_info and channel_by_id maps
 						// have no consistency guarantees.
@@ -3857,7 +3854,7 @@ where
 						next_hop_channel_id, next_node_id)
 				}),
 				None => return Err(APIError::ChannelUnavailable {
-					err: format!("Funded channel with id {} not found for the passed counterparty node_id {}.",
+					err: format!("Channel with id {} not found for the passed counterparty node_id {}.",
 						next_hop_channel_id, next_node_id)
 				})
 			}
@@ -4417,10 +4414,10 @@ where
 							let mut peer_state_lock = peer_state_mutex.lock().unwrap();
 							let peer_state = &mut *peer_state_lock;
 							match peer_state.channel_by_id.entry(funding_txo.to_channel_id()) {
-								hash_map::Entry::Occupied(mut chan_phase_phase) => {
+								hash_map::Entry::Occupied(mut chan_phase) => {
 									updated_chan = true;
 									handle_new_monitor_update!(self, funding_txo, update.clone(),
-										peer_state_lock, peer_state, per_peer_state, chan_phase_phase).map(|_| ())
+										peer_state_lock, peer_state, per_peer_state, chan_phase).map(|_| ())
 								},
 								hash_map::Entry::Vacant(_) => Ok(()),
 							}

lightning/src/ln/payment_tests.rs

@@ -1705,7 +1705,7 @@ fn do_test_intercepted_payment(test: InterceptTest) {
 	// Check for unknown channel id error.
 	let unknown_chan_id_err = nodes[1].node.forward_intercepted_htlc(intercept_id, &ChannelId::from_bytes([42; 32]), nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap_err();
 	assert_eq!(unknown_chan_id_err , APIError::ChannelUnavailable  {
-		err: format!("Funded channel with id {} not found for the passed counterparty node_id {}.",
+		err: format!("Channel with id {} not found for the passed counterparty node_id {}.",
 			log_bytes!([42; 32]), nodes[2].node.get_our_node_id()) });
 
 	if test == InterceptTest::Fail {
@@ -1731,7 +1731,7 @@ fn do_test_intercepted_payment(test: InterceptTest) {
 		let temp_chan_id = nodes[1].node.create_channel(nodes[2].node.get_our_node_id(), 100_000, 0, 42, None).unwrap();
 		let unusable_chan_err = nodes[1].node.forward_intercepted_htlc(intercept_id, &temp_chan_id, nodes[2].node.get_our_node_id(), expected_outbound_amount_msat).unwrap_err();
 		assert_eq!(unusable_chan_err , APIError::ChannelUnavailable {
-			err: format!("Funded channel with id {} not found for the passed counterparty node_id {}.",
+			err: format!("Channel with id {} not found for the passed counterparty node_id {}.",
 				temp_chan_id, nodes[2].node.get_our_node_id()) });
 		assert_eq!(nodes[1].node.get_and_clear_pending_msg_events().len(), 1);
 

lightning/src/util/enum_utils.rs

@@ -0,0 +1,22 @@
+// This file is Copyright its original authors, visible in version control
+// history.
+//
+// This file is licensed under the Apache License, Version 2.0 <LICENSE-APACHE
+// or http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your option.
+// You may not use this file except in accordance with one or both of these
+// licenses.
+
+//! Utilities for making working with enums, their variants, and inner values more convenient.
+
+/// Returns an inner variant value from an enum with a pattern.
+/// Panics if the pattern does not match the actual enum value.
+#[macro_export]
+macro_rules! unwrap_enum_value {
+  ($enum:expr, $pattern:pat => $inner_value:expr) => {
+    match $enum {
+      $pattern => $inner_value,
+      _ => panic!("Failed to unwrap enum value as variant was unexpected!"),
+    }
+  };
+}

lightning/src/util/mod.rs

@@ -58,3 +58,6 @@ pub mod test_utils;
 #[cfg(any(test, feature = "_test_utils"))]
 pub mod enforcing_trait_impls;
 
+/// Enum macro utilities.
+#[macro_use]
+pub(crate) mod enum_utils;