diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 51555c7a0ac..07fe697a8f6 100644 --- 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, &Transaction) -> Result>( + fn funding_transaction_generated_intern, &Transaction) -> Result>( &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() { @@ -4718,11 +4726,20 @@ where for (channel_id, counterparty_node_id) in channels_to_remove { per_peer_state.get(&counterparty_node_id) .map(|peer_state_mutex| peer_state_mutex.lock().unwrap()) - .and_then(|mut peer_state| peer_state.channel_by_id.remove(&channel_id)) - .map(|mut chan| { + .and_then(|mut peer_state| peer_state.channel_by_id.remove(&channel_id).map(|chan| (chan, peer_state))) + .map(|(mut chan, mut peer_state)| { update_maps_on_chan_removal!(self, &chan.context()); let closure_reason = ClosureReason::ProcessingError { err: e.clone() }; shutdown_results.push(chan.context_mut().force_shutdown(false, closure_reason)); + peer_state.pending_msg_events.push(events::MessageSendEvent::HandleError { + node_id: counterparty_node_id, + action: msgs::ErrorAction::SendErrorMessage { + msg: msgs::ErrorMessage { + channel_id, + data: "Failed to fund channel".to_owned(), + } + }, + }); }); } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 465d6288d9d..151626f94ab 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -10110,14 +10110,9 @@ fn test_non_final_funding_tx() { }, _ => panic!() } - let events = nodes[0].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::ChannelClosed { channel_id, .. } => { - assert_eq!(channel_id, temp_channel_id); - }, - _ => panic!("Unexpected event"), - } + let err = "Error in transaction funding: Misuse error: Funding transaction absolute timelock is non-final".to_owned(); + check_closed_events(&nodes[0], &[ExpectedCloseEvent::from_id_reason(temp_channel_id, false, ClosureReason::ProcessingError { err })]); + assert_eq!(get_err_msg(&nodes[0], &nodes[1].node.get_our_node_id()).data, "Failed to fund channel"); } #[test] diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index 7f004948ffe..e4b69321eb7 100644 --- 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,44 @@ 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(), 3); + // 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 { + *channel_id == temp_chan_id_b + } else { false } + })); + let funding_created_pos = msgs.iter().position(|msg| { + if let MessageSendEvent::SendFundingCreated { msg: msgs::FundingCreated { temporary_channel_id, .. }, .. } = msg { + assert_eq!(*temporary_channel_id, temp_chan_id_a); + true + } else { false } + }).unwrap(); + let funded_channel_close_pos = msgs.iter().position(|msg| { + if let MessageSendEvent::HandleError { action: msgs::ErrorAction::SendErrorMessage { + msg: msgs::ErrorMessage { channel_id, .. }, .. + }, .. } = msg { + *channel_id == post_funding_chan_id_a + } else { false } + }).unwrap(); + + // The error message uses the funded channel_id so must come after the funding_created + assert!(funded_channel_close_pos > funding_created_pos); + check_closed_events(&nodes[0], &close); assert_eq!(nodes[0].node.list_channels().len(), 0); }