Consistently clean up when failing in internal_funding_created

When we fail to accept a counterparty's funding for various
reasons, we should ensure we call the correct cleanup methods in
`internal_funding_created` to remove the temporary data for the
channel in our various internal structs (primarily the SCID alias
map).

This adds the missing cleanup, using `convert_chan_phase_err`
consistently in all the error paths.

This also ensures we get a `ChannelClosed` event when relevant.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -2920,6 +2920,20 @@ impl<SP: Deref> Channel<SP> where
 		self.context.channel_state.clear_waiting_for_batch();
 	}
 
+	/// Unsets the existing funding information.
+	///
+	/// This must only be used if the channel has not yet completed funding and has not been used.
+	///
+	/// Further, the channel must be immediately shut down after this with a call to
+	/// [`ChannelContext::force_shutdown`].
+	pub fn unset_funding_info(&mut self, temporary_channel_id: ChannelId) {
+		debug_assert!(matches!(
+			self.context.channel_state, ChannelState::AwaitingChannelReady(_)
+		));
+		self.context.channel_transaction_parameters.funding_outpoint = None;
+		self.context.channel_id = temporary_channel_id;
+	}
+
 	/// Handles a channel_ready message from our peer. If we've already sent our channel_ready
 	/// and the channel is now usable (and public), this may generate an announcement_signatures to
 	/// reply with.

lightning/src/ln/channelmanager.rs

@@ -1251,7 +1251,10 @@ where
 	/// required to access the channel with the `counterparty_node_id`.
 	///
 	/// See `ChannelManager` struct-level documentation for lock order requirements.
+	#[cfg(not(test))]
 	outpoint_to_peer: Mutex<HashMap<OutPoint, PublicKey>>,
+	#[cfg(test)]
+	pub(crate) outpoint_to_peer: Mutex<HashMap<OutPoint, PublicKey>>,
 
 	/// SCIDs (and outbound SCID aliases) -> `counterparty_node_id`s and `channel_id`s.
 	///
@@ -6205,44 +6208,55 @@ where
 
 		let mut peer_state_lock = peer_state_mutex.lock().unwrap();
 		let peer_state = &mut *peer_state_lock;
-		let (chan, funding_msg_opt, monitor) =
+		let (mut chan, funding_msg_opt, monitor) =
 			match peer_state.channel_by_id.remove(&msg.temporary_channel_id) {
 				Some(ChannelPhase::UnfundedInboundV1(inbound_chan)) => {
 					let logger = WithChannelContext::from(&self.logger, &inbound_chan.context);
 					match inbound_chan.funding_created(msg, best_block, &self.signer_provider, &&logger) {
 						Ok(res) => res,
-						Err((mut inbound_chan, err)) => {
+						Err((inbound_chan, err)) => {
 							// We've already removed this inbound channel from the map in `PeerState`
 							// above so at this point we just need to clean up any lingering entries
 							// concerning this channel as it is safe to do so.
-							update_maps_on_chan_removal!(self, &inbound_chan.context);
-							let user_id = inbound_chan.context.get_user_id();
-							let shutdown_res = inbound_chan.context.force_shutdown(false);
-							return Err(MsgHandleErrInternal::from_finish_shutdown(format!("{}", err),
-								msg.temporary_channel_id, user_id, shutdown_res, None, inbound_chan.context.get_value_satoshis()));
+							debug_assert!(matches!(err, ChannelError::Close(_)));
+							// Really we should be returning the channel_id the peer expects based
+							// on their funding info here, but they're horribly confused anyway, so
+							// there's not a lot we can do to save them.
+							return Err(convert_chan_phase_err!(self, err, &mut ChannelPhase::UnfundedInboundV1(inbound_chan), &msg.temporary_channel_id).1);
 						},
 					}
 				},
-				Some(ChannelPhase::Funded(_)) | Some(ChannelPhase::UnfundedOutboundV1(_)) => {
-					return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id), msg.temporary_channel_id));
+				Some(mut phase) => {
+					let err_msg = format!("Got an unexpected funding_created message from peer with counterparty_node_id {}", counterparty_node_id);
+					let err = ChannelError::Close(err_msg);
+					return Err(convert_chan_phase_err!(self, err, &mut phase, &msg.temporary_channel_id).1);
 				},
 				None => return Err(MsgHandleErrInternal::send_err_msg_no_close(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", counterparty_node_id), msg.temporary_channel_id))
 			};
 
-		match peer_state.channel_by_id.entry(chan.context.channel_id()) {
+		let funded_channel_id = chan.context.channel_id();
+
+		macro_rules! fail_chan { ($err: expr) => { {
+			// Note that at this point we've filled in the funding outpoint on our
+			// channel, but its actually in conflict with another channel. Thus, if
+			// we call `convert_chan_phase_err` immediately (thus calling
+			// `update_maps_on_chan_removal`), we'll remove the existing channel
+			// from `outpoint_to_peer`. Thus, we must first unset the funding outpoint
+			// on the channel.
+			let err = ChannelError::Close($err.to_owned());
+			chan.unset_funding_info(msg.temporary_channel_id);
+			return Err(convert_chan_phase_err!(self, err, chan, &funded_channel_id, UNFUNDED_CHANNEL).1);
+		} } }
+
+		match peer_state.channel_by_id.entry(funded_channel_id) {
 			hash_map::Entry::Occupied(_) => {
-				Err(MsgHandleErrInternal::send_err_msg_no_close(
-					"Already had channel with the new channel_id".to_owned(),
-					chan.context.channel_id()
-				))
+				fail_chan!("Already had channel with the new channel_id");
 			},
 			hash_map::Entry::Vacant(e) => {
 				let mut outpoint_to_peer_lock = self.outpoint_to_peer.lock().unwrap();
 				match outpoint_to_peer_lock.entry(monitor.get_funding_txo().0) {
 					hash_map::Entry::Occupied(_) => {
-						return Err(MsgHandleErrInternal::send_err_msg_no_close(
-							"The funding_created message had the same funding_txid as an existing channel - funding is not possible".to_owned(),
-							chan.context.channel_id()))
+						fail_chan!("The funding_created message had the same funding_txid as an existing channel - funding is not possible");
 					},
 					hash_map::Entry::Vacant(i_e) => {
 						let monitor_res = self.chain_monitor.watch_channel(monitor.get_funding_txo().0, monitor);
@@ -6271,13 +6285,7 @@ where
 						} else {
 							let logger = WithChannelContext::from(&self.logger, &chan.context);
 							log_error!(logger, "Persisting initial ChannelMonitor failed, implying the funding outpoint was duplicated");
-							let channel_id = match funding_msg_opt {
-								Some(msg) => msg.channel_id,
-								None => chan.context.channel_id(),
-							};
-							return Err(MsgHandleErrInternal::send_err_msg_no_close(
-								"The funding_created message had the same funding_txid as an existing channel - funding is not possible".to_owned(),
-								channel_id));
+							fail_chan!("Duplicate funding outpoint");
 						}
 					}
 				}

lightning/src/ln/functional_test_utils.rs

@@ -1536,6 +1536,18 @@ pub struct ExpectedCloseEvent {
 	pub reason: Option<ClosureReason>,
 }
 
+impl ExpectedCloseEvent {
+	pub fn from_id_reason(channel_id: ChannelId, discard_funding: bool, reason: ClosureReason) -> Self {
+		Self {
+			channel_capacity_sats: None,
+			channel_id: Some(channel_id),
+			counterparty_node_id: None,
+			discard_funding,
+			reason: Some(reason),
+		}
+	}
+}
+
 /// Check that multiple channel closing events have been issued.
 pub fn check_closed_events(node: &Node, expected_close_events: &[ExpectedCloseEvent]) {
 	let closed_events_count = expected_close_events.len();

lightning/src/ln/functional_tests.rs

@@ -8970,6 +8970,54 @@ fn test_duplicate_temporary_channel_id_from_different_peers() {
 	}
 }
 
+#[test]
+fn test_duplicate_funding_err_in_funding() {
+	// Test that if we have a live channel with one peer, then another peer comes along and tries
+	// to create a second channel with the same txid we'll fail and not overwrite the
+	// outpoint_to_peer map in `ChannelManager`.
+	//
+	// This was previously broken.
+	let chanmon_cfgs = create_chanmon_cfgs(3);
+	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+	let (_, _, _, real_channel_id, funding_tx) = create_chan_between_nodes(&nodes[0], &nodes[1]);
+	let real_chan_funding_txo = chain::transaction::OutPoint { txid: funding_tx.txid(), index: 0 };
+	assert_eq!(real_chan_funding_txo.to_channel_id(), real_channel_id);
+
+	nodes[2].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 0, 42, None, None).unwrap();
+	let mut open_chan_msg = get_event_msg!(nodes[2], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+	let node_c_temp_chan_id = open_chan_msg.temporary_channel_id;
+	open_chan_msg.temporary_channel_id = real_channel_id;
+	nodes[1].node.handle_open_channel(&nodes[2].node.get_our_node_id(), &open_chan_msg);
+	let mut accept_chan_msg = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[2].node.get_our_node_id());
+	accept_chan_msg.temporary_channel_id = node_c_temp_chan_id;
+	nodes[2].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_chan_msg);
+
+	// Now that we have a second channel with the same funding txo, send a bogus funding message
+	// and let nodes[1] remove the inbound channel.
+	let (_, funding_tx, _) = create_funding_transaction(&nodes[2], &nodes[1].node.get_our_node_id(), 100_000, 42);
+
+	nodes[2].node.funding_transaction_generated(&node_c_temp_chan_id, &nodes[1].node.get_our_node_id(), funding_tx).unwrap();
+
+	let mut funding_created_msg = get_event_msg!(nodes[2], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
+	funding_created_msg.temporary_channel_id = real_channel_id;
+	// Make the signature invalid by changing the funding output
+	funding_created_msg.funding_output_index += 10;
+	nodes[1].node.handle_funding_created(&nodes[2].node.get_our_node_id(), &funding_created_msg);
+	get_err_msg(&nodes[1], &nodes[2].node.get_our_node_id());
+	let err = "Invalid funding_created signature from peer".to_owned();
+	let reason = ClosureReason::ProcessingError { err };
+	let expected_closing = ExpectedCloseEvent::from_id_reason(real_channel_id, false, reason);
+	check_closed_events(&nodes[1], &[expected_closing]);
+
+	assert_eq!(
+		*nodes[1].node.outpoint_to_peer.lock().unwrap().get(&real_chan_funding_txo).unwrap(),
+		nodes[0].node.get_our_node_id()
+	);
+}
+
 #[test]
 fn test_duplicate_chan_id() {
 	// Test that if a given peer tries to open a channel with the same channel_id as one that is
@@ -9080,6 +9128,12 @@ fn test_duplicate_chan_id() {
 	// without trying to persist the `ChannelMonitor`.
 	check_added_monitors!(nodes[1], 0);
 
+	check_closed_events(&nodes[1], &[
+		ExpectedCloseEvent::from_id_reason(channel_id, false, ClosureReason::ProcessingError {
+			err: "Already had channel with the new channel_id".to_owned()
+		})
+	]);
+
 	// ...still, nodes[1] will reject the duplicate channel.
 	{
 		let events = nodes[1].node.get_and_clear_pending_msg_events();