From: Matt Corallo <git@bluematt.me>
Date: Mon, 29 Apr 2024 21:05:52 +0000 (+0000)
Subject: Send peers error messages for failures due to invalid batch funding
X-Git-Tag: v0.0.123-rc1~1^2~4^2
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=refs%2Fheads%2F2024-04-fix-batch-funding-failures;p=rust-lightning

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.
---

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 3a025d069..07fe697a8 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 465d6288d..151626f94 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 9a44953f5..e4b69321e 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);