Test for extra locks held in `handle_error` unconditionally

`handle_error` must be called without `per_peer_state` mutex or
`pending_events` mutex locks held or we may risk deadlocks.
Previously we checked this in debug builds in the error path, but
not in the success path.

As it turns out, `funding_transaction_generated`'s error path does
hold a `per_peer_state` lock, which we fix here as well as move the
tests to happen unconditionally.

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index c2ff9da67f38cc169c048ca7279952109f346d3d..ba0f17303372dba3aa4c68d5305181d6173db4a3 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1345,15 +1345,15 @@ pub struct PhantomRouteHints {
 }
 
 macro_rules! handle_error {
-       ($self: ident, $internal: expr, $counterparty_node_id: expr) => {
+       ($self: ident, $internal: expr, $counterparty_node_id: expr) => { {
+               // In testing, ensure there are no deadlocks where the lock is already held upon
+               // entering the macro.
+               debug_assert_ne!($self.pending_events.held_by_thread(), LockHeldState::HeldByThread);
+               debug_assert_ne!($self.per_peer_state.held_by_thread(), LockHeldState::HeldByThread);
+
                match $internal {
                        Ok(msg) => Ok(msg),
                        Err(MsgHandleErrInternal { err, chan_id, shutdown_finish }) => {
-                               // In testing, ensure there are no deadlocks where the lock is already held upon
-                               // entering the macro.
-                               debug_assert_ne!($self.pending_events.held_by_thread(), LockHeldState::HeldByThread);
-                               debug_assert_ne!($self.per_peer_state.held_by_thread(), LockHeldState::HeldByThread);
-
                                let mut msg_events = Vec::with_capacity(2);
 
                                if let Some((shutdown_res, update_option)) = shutdown_finish {
@@ -1392,7 +1392,7 @@ macro_rules! handle_error {
                                Err(err)
                        },
                }
-       }
+       } }
 }
 
 macro_rules! update_maps_on_chan_removal {
@@ -2790,29 +2790,28 @@ where
 
                let mut peer_state_lock = peer_state_mutex.lock().unwrap();
                let peer_state = &mut *peer_state_lock;
-               let (chan, msg) = {
-                       let (res, chan) = {
-                               match peer_state.channel_by_id.remove(temporary_channel_id) {
-                                       Some(mut chan) => {
-                                               let funding_txo = find_funding_output(&chan, &funding_transaction)?;
-
-                                               (chan.get_outbound_funding_created(funding_transaction, funding_txo, &self.logger)
-                                                       .map_err(|e| if let ChannelError::Close(msg) = e {
-                                                               MsgHandleErrInternal::from_finish_shutdown(msg, chan.channel_id(), chan.get_user_id(), chan.force_shutdown(true), None)
-                                                       } else { unreachable!(); })
-                                               , chan)
+               let (msg, chan) = match peer_state.channel_by_id.remove(temporary_channel_id) {
+                       Some(mut chan) => {
+                               let funding_txo = find_funding_output(&chan, &funding_transaction)?;
+
+                               let funding_res = chan.get_outbound_funding_created(funding_transaction, funding_txo, &self.logger)
+                                       .map_err(|e| if let ChannelError::Close(msg) = e {
+                                               MsgHandleErrInternal::from_finish_shutdown(msg, chan.channel_id(), chan.get_user_id(), chan.force_shutdown(true), None)
+                                       } else { unreachable!(); });
+                               match funding_res {
+                                       Ok(funding_msg) => (funding_msg, chan),
+                                       Err(_) => {
+                                               mem::drop(peer_state_lock);
+                                               mem::drop(per_peer_state);
+
+                                               let _ = handle_error!(self, funding_res, chan.get_counterparty_node_id());
+                                               return Err(APIError::ChannelUnavailable {
+                                                       err: "Signer refused to sign the initial commitment transaction".to_owned()
+                                               });
                                        },
-                                       None => { return Err(APIError::ChannelUnavailable { err: format!("Channel with id {} not found for the passed counterparty node_id {}", log_bytes!(*temporary_channel_id), counterparty_node_id) }) },
                                }
-                       };
-                       match handle_error!(self, res, chan.get_counterparty_node_id()) {
-                               Ok(funding_msg) => {
-                                       (chan, funding_msg)
-                               },
-                               Err(_) => { return Err(APIError::ChannelUnavailable {
-                                       err: "Signer refused to sign the initial commitment transaction".to_owned()
-                               }) },
-                       }
+                       },
+                       None => { return Err(APIError::ChannelUnavailable { err: format!("Channel with id {} not found for the passed counterparty node_id {}", log_bytes!(*temporary_channel_id), counterparty_node_id) }) },
                };
 
                peer_state.pending_msg_events.push(events::MessageSendEvent::SendFundingCreated {