Merge pull request #3029 from TheBlueMatt/2024-04-fix-batch-funding-f…

…ailures

Add error handling for channels which fail to be created in `funding_transaction_generated_intern`

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() {
@@ -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(),
+									}
+								},
+							});
 						});
 				}
 			}

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]

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);
 }