From 6960210f604964583bfe704d6f956cadddcd15a2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 29 Apr 2024 18:12:59 +0000 Subject: [PATCH 1/2] 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. --- lightning/src/ln/channelmanager.rs | 54 +++++++++++++++++------------- lightning/src/ln/shutdown_tests.rs | 33 ++++++++++++++---- 2 files changed, 58 insertions(+), 29 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 51555c7a0ac..3a025d069d4 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() { diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index 7f004948ffe..9a44953f51e 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,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); } From b811cba74835c3e866354cb778714837ad488d28 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 29 Apr 2024 21:05:52 +0000 Subject: [PATCH 2/2] Send peers error messages for failures due to invalid batch funding If we fail to fund a batch open we'll force-close all channels in the batch, however would previously fail to send error messages to our peers for any channels we were due to test after the one that failed. This commit fixes that issue, sending the required error messages to allow our peers to clean up their state. --- lightning/src/ln/channelmanager.rs | 13 +++++++++++-- lightning/src/ln/functional_tests.rs | 11 +++-------- lightning/src/ln/shutdown_tests.rs | 19 ++++++++++++++----- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3a025d069d4..07fe697a8f6 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4726,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 9a44953f51e..e4b69321eb7 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -1430,23 +1430,32 @@ fn batch_funding_failure() { nodes[0].node.batch_funding_transaction_generated(&chans, tx).unwrap_err(); let msgs = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(msgs.len(), 2); + 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 { - assert_eq!(*channel_id, temp_chan_id_b); - true + *channel_id == temp_chan_id_b } else { false } })); - assert!(msgs.iter().any(|msg| { + 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);