Close channels when `find_funding_output` fails to find an output

In `funding_transaction_generated_intern`, if `find_funding_output`
fails (e.g. due to the require output not being present in the
provided funding transaction) we'd previously not generated a
`ChannelClosed` event which leaves users possibly in a confused
state.

Here we fix this, also fixing the relevant tests to check for the
new event.

Fixes #2843.

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 51555c7a0ac6e199fe25f6ec17315edd48f2cb9b..3a025d069d45e4a1ffcd325fb95e8dd1aa83500d 100644 (file)
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4480,7 +4480,7 @@ where
 
        /// Handles the generation of a funding transaction, optionally (for tests) with a function
        /// which checks the correctness of the funding transaction given the associated channel.
-       fn funding_transaction_generated_intern<FundingOutput: FnMut(&OutboundV1Channel<SP>, &Transaction) -> Result<OutPoint, APIError>>(
+       fn funding_transaction_generated_intern<FundingOutput: FnMut(&OutboundV1Channel<SP>, &Transaction) -> Result<OutPoint, &'static str>>(
                &self, temporary_channel_id: &ChannelId, counterparty_node_id: &PublicKey, funding_transaction: Transaction, is_batch_funding: bool,
                mut find_funding_output: FundingOutput,
        ) -> Result<(), APIError> {
@@ -4493,26 +4493,38 @@ where
                let funding_txo;
                let (mut chan, msg_opt) = match peer_state.channel_by_id.remove(temporary_channel_id) {
                        Some(ChannelPhase::UnfundedOutboundV1(mut chan)) => {
-                               funding_txo = find_funding_output(&chan, &funding_transaction)?;
+                               macro_rules! close_chan { ($err: expr, $api_err: expr, $chan: expr) => { {
+                                       let counterparty;
+                                       let err = if let ChannelError::Close(msg) = $err {
+                                               let channel_id = $chan.context.channel_id();
+                                               counterparty = chan.context.get_counterparty_node_id();
+                                               let reason = ClosureReason::ProcessingError { err: msg.clone() };
+                                               let shutdown_res = $chan.context.force_shutdown(false, reason);
+                                               MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, shutdown_res, None)
+                                       } else { unreachable!(); };
+
+                                       mem::drop(peer_state_lock);
+                                       mem::drop(per_peer_state);
+                                       let _: Result<(), _> = handle_error!(self, Err(err), counterparty);
+                                       Err($api_err)
+                               } } }
+                               match find_funding_output(&chan, &funding_transaction) {
+                                       Ok(found_funding_txo) => funding_txo = found_funding_txo,
+                                       Err(err) => {
+                                               let chan_err = ChannelError::Close(err.to_owned());
+                                               let api_err = APIError::APIMisuseError { err: err.to_owned() };
+                                               return close_chan!(chan_err, api_err, chan);
+                                       },
+                               }
 
                                let logger = WithChannelContext::from(&self.logger, &chan.context);
-                               let funding_res = chan.get_funding_created(funding_transaction, funding_txo, is_batch_funding, &&logger)
-                                       .map_err(|(mut chan, e)| if let ChannelError::Close(msg) = e {
-                                               let channel_id = chan.context.channel_id();
-                                               let reason = ClosureReason::ProcessingError { err: msg.clone() };
-                                               let shutdown_res = chan.context.force_shutdown(false, reason);
-                                               (chan, MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, shutdown_res, None))
-                                       } else { unreachable!(); });
+                               let funding_res = chan.get_funding_created(funding_transaction, funding_txo, is_batch_funding, &&logger);
                                match funding_res {
                                        Ok(funding_msg) => (chan, funding_msg),
-                                       Err((chan, err)) => {
-                                               mem::drop(peer_state_lock);
-                                               mem::drop(per_peer_state);
-                                               let _: Result<(), _> = handle_error!(self, Err(err), chan.context.get_counterparty_node_id());
-                                               return Err(APIError::ChannelUnavailable {
-                                                       err: "Signer refused to sign the initial commitment transaction".to_owned()
-                                               });
-                                       },
+                                       Err((mut chan, chan_err)) => {
+                                               let api_err = APIError::ChannelUnavailable { err: "Signer refused to sign the initial commitment transaction".to_owned() };
+                                               return close_chan!(chan_err, api_err, chan);
+                                       }
                                }
                        },
                        Some(phase) => {
@@ -4677,17 +4689,13 @@ where
                                        for (idx, outp) in tx.output.iter().enumerate() {
                                                if outp.script_pubkey == expected_spk && outp.value == chan.context.get_value_satoshis() {
                                                        if output_index.is_some() {
-                                                               return Err(APIError::APIMisuseError {
-                                                                       err: "Multiple outputs matched the expected script and value".to_owned()
-                                                               });
+                                                               return Err("Multiple outputs matched the expected script and value");
                                                        }
                                                        output_index = Some(idx as u16);
                                                }
                                        }
                                        if output_index.is_none() {
-                                               return Err(APIError::APIMisuseError {
-                                                       err: "No output matched the script_pubkey and value in the FundingGenerationReady event".to_owned()
-                                               });
+                                               return Err("No output matched the script_pubkey and value in the FundingGenerationReady event");
                                        }
                                        let outpoint = OutPoint { txid: tx.txid(), index: output_index.unwrap() };
                                        if let Some(funding_batch_state) = funding_batch_state.as_mut() {

diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs
index 7f004948ffeb5cb10790673e5fa36680950bef23..9a44953f51e3fab76776f323ea91485a59ecc448 100644 (file)
--- a/lightning/src/ln/shutdown_tests.rs
+++ b/lightning/src/ln/shutdown_tests.rs
@@ -1401,8 +1401,8 @@ fn batch_funding_failure() {
        let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
        let nodes = create_network(4, &node_cfgs, &node_chanmgrs);
 
-       exchange_open_accept_chan(&nodes[0], &nodes[1], 1_000_000, 0);
-       exchange_open_accept_chan(&nodes[0], &nodes[2], 1_000_000, 0);
+       let temp_chan_id_a = exchange_open_accept_chan(&nodes[0], &nodes[1], 1_000_000, 0);
+       let temp_chan_id_b = exchange_open_accept_chan(&nodes[0], &nodes[2], 1_000_000, 0);
 
        let events = nodes[0].node.get_and_clear_pending_events();
        assert_eq!(events.len(), 2);
@@ -1419,14 +1419,35 @@ fn batch_funding_failure() {
                } else { panic!(); }
        }
 
-       // We should probably end up with an error for both channels, but currently we don't generate
-       // an error for the failing channel itself.
        let err = "Error in transaction funding: Misuse error: No output matched the script_pubkey and value in the FundingGenerationReady event".to_string();
-       let close = [ExpectedCloseEvent::from_id_reason(ChannelId::v1_from_funding_txid(tx.txid().as_ref(), 0), true, ClosureReason::ProcessingError { err })];
+       let temp_err = "No output matched the script_pubkey and value in the FundingGenerationReady event".to_string();
+       let post_funding_chan_id_a = ChannelId::v1_from_funding_txid(tx.txid().as_ref(), 0);
+       let close = [
+               ExpectedCloseEvent::from_id_reason(post_funding_chan_id_a, true, ClosureReason::ProcessingError { err: err.clone() }),
+               ExpectedCloseEvent::from_id_reason(temp_chan_id_b, false, ClosureReason::ProcessingError { err: temp_err }),
+       ];
 
        nodes[0].node.batch_funding_transaction_generated(&chans, tx).unwrap_err();
 
-       get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
+       let msgs = nodes[0].node.get_and_clear_pending_msg_events();
+       assert_eq!(msgs.len(), 2);
+       // We currently spuriously send `FundingCreated` for the first channel and then immediately
+       // fail both channels, which isn't ideal but should be fine.
+       assert!(msgs.iter().any(|msg| {
+               if let MessageSendEvent::HandleError { action: msgs::ErrorAction::SendErrorMessage {
+                       msg: msgs::ErrorMessage { channel_id, .. }, ..
+               }, .. } = msg {
+                       assert_eq!(*channel_id, temp_chan_id_b);
+                       true
+               } else { false }
+       }));
+       assert!(msgs.iter().any(|msg| {
+               if let MessageSendEvent::SendFundingCreated { msg: msgs::FundingCreated { temporary_channel_id, .. }, .. } = msg {
+                       assert_eq!(*temporary_channel_id, temp_chan_id_a);
+                       true
+               } else { false }
+       }));
+
        check_closed_events(&nodes[0], &close);
        assert_eq!(nodes[0].node.list_channels().len(), 0);
 }