Move funding_signed phase transition to Channel

Now that ChannelPhase is encapsulated in Channel, phase transitions can
be moved from ChannelManager to Channel. Update the funding_signed phase
transition accordingly. This allows for simpler logic in ChannelManager
since the channel does not need to removed and then readded into the
channel_by_id map.

Author: jkczyz

lightning/src/ln/channel.rs

@@ -1211,10 +1211,6 @@ impl<SP: Deref> Channel<SP> where
 		matches!(self.phase, ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_))
 	}
 
-	pub fn is_unfunded_outbound_v1(&self) -> bool {
-		matches!(self.phase, ChannelPhase::UnfundedOutboundV1(_))
-	}
-
 	pub fn into_unfunded_outbound_v1(self) -> Result<OutboundV1Channel<SP>, Self> {
 		if let ChannelPhase::UnfundedOutboundV1(channel) = self.phase {
 			Ok(channel)
@@ -1378,6 +1374,57 @@ impl<SP: Deref> Channel<SP> where
 			},
 		}
 	}
+
+	pub fn funding_signed<L: Deref>(
+		&mut self, msg: &msgs::FundingSigned, best_block: BestBlock, signer_provider: &SP, logger: &L
+	) -> Result<(&mut FundedChannel<SP>, ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>), ChannelError>
+	where
+		L::Target: Logger
+	{
+		let phase = core::mem::replace(&mut self.phase, ChannelPhase::Undefined);
+		let result = if let ChannelPhase::UnfundedOutboundV1(chan) = phase {
+			let logger = WithChannelContext::from(logger, &chan.context, None);
+			match chan.funding_signed(msg, best_block, signer_provider, &&logger) {
+				Ok((chan, monitor)) => {
+					self.phase = ChannelPhase::Funded(chan);
+					Ok(monitor)
+				},
+				Err((chan, e)) => {
+					self.phase = ChannelPhase::UnfundedOutboundV1(chan);
+					Err(e)
+				},
+			}
+		} else {
+			self.phase = phase;
+			Err(ChannelError::SendError("Failed to find corresponding UnfundedOutboundV1 channel".to_owned()))
+		};
+
+		debug_assert!(!matches!(self.phase, ChannelPhase::Undefined));
+		result.map(|monitor| (self.as_funded_mut().expect("Channel should be funded"), monitor))
+	}
+
+	pub fn unset_funding_info(&mut self) {
+		let phase = core::mem::replace(&mut self.phase, ChannelPhase::Undefined);
+		if let ChannelPhase::Funded(mut funded_chan) = phase {
+			funded_chan.unset_funding_info();
+
+			let context = funded_chan.context;
+			let unfunded_context = UnfundedChannelContext {
+				unfunded_channel_age_ticks: 0,
+				holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx),
+			};
+			let unfunded_chan = OutboundV1Channel {
+				context,
+				unfunded_context,
+				signer_pending_open_channel: false,
+			};
+			self.phase = ChannelPhase::UnfundedOutboundV1(unfunded_chan);
+		} else {
+			self.phase = phase;
+		};
+
+		debug_assert!(!matches!(self.phase, ChannelPhase::Undefined));
+	}
 }
 
 impl<SP: Deref> From<OutboundV1Channel<SP>> for Channel<SP>
@@ -4952,12 +4999,15 @@ impl<SP: Deref> FundedChannel<SP> where
 	///
 	/// 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) {
+	pub fn unset_funding_info(&mut self) {
 		debug_assert!(matches!(
 			self.context.channel_state, ChannelState::AwaitingChannelReady(_)
 		));
 		self.context.channel_transaction_parameters.funding_outpoint = None;
-		self.context.channel_id = temporary_channel_id;
+		self.context.channel_id = self.context.temporary_channel_id.expect(
+			"temporary_channel_id should be set since unset_funding_info is only called on funded \
+			 channels that were unfunded immediately beforehand"
+		);
 	}
 
 	/// Handles a channel_ready message from our peer. If we've already sent our channel_ready

lightning/src/ln/channelmanager.rs

@@ -8096,7 +8096,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 			// `locked_close_channel`), 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);
+			chan.unset_funding_info();
 			return Err(convert_channel_err!(self, peer_state, err, chan.context, &funded_channel_id, UNFUNDED_CHANNEL).1);
 		} } }
 
@@ -8157,49 +8157,31 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 		let mut peer_state_lock = peer_state_mutex.lock().unwrap();
 		let peer_state = &mut *peer_state_lock;
 		match peer_state.channel_by_id.entry(msg.channel_id) {
-			hash_map::Entry::Occupied(chan_entry) => {
-				if chan_entry.get().is_unfunded_outbound_v1() {
-					let chan = if let Ok(chan) = chan_entry.remove().into_unfunded_outbound_v1() { chan } else { unreachable!() };
-					let logger = WithContext::from(
-						&self.logger,
-						Some(chan.context.get_counterparty_node_id()),
-						Some(chan.context.channel_id()),
-						None
-					);
-					let res =
-						chan.funding_signed(&msg, best_block, &self.signer_provider, &&logger);
-					match res {
-						Ok((mut chan, monitor)) => {
-							if let Ok(persist_status) = self.chain_monitor.watch_channel(chan.context.get_funding_txo().unwrap(), monitor) {
-								// We really should be able to insert here without doing a second
-								// lookup, but sadly rust stdlib doesn't currently allow keeping
-								// the original Entry around with the value removed.
-								let chan = peer_state.channel_by_id.entry(msg.channel_id).or_insert(Channel::from(chan));
-								if let Some(funded_chan) = chan.as_funded_mut() {
-									handle_new_monitor_update!(self, persist_status, peer_state_lock, peer_state, per_peer_state, funded_chan, INITIAL_MONITOR);
-								} else { unreachable!(); }
-								Ok(())
-							} else {
-								let e = ChannelError::close("Channel funding outpoint was a duplicate".to_owned());
-								// We weren't able to watch the channel to begin with, so no
-								// updates should be made on it. Previously, full_stack_target
-								// found an (unreachable) panic when the monitor update contained
-								// within `shutdown_finish` was applied.
-								chan.unset_funding_info(msg.channel_id);
-								return Err(convert_channel_err!(self, peer_state, e, chan, &msg.channel_id, FUNDED_CHANNEL).1);
-							}
-						},
-						Err((mut chan, e)) => {
-							debug_assert!(matches!(e, ChannelError::Close(_)),
-								"We don't have a channel anymore, so the error better have expected close");
-							// We've already removed this outbound 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.
-							return Err(convert_channel_err!(self, peer_state, e, chan.context, &msg.channel_id, UNFUNDED_CHANNEL).1);
-						}
-					}
-				} else {
-					return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id));
+			hash_map::Entry::Occupied(mut chan_entry) => {
+				let chan = chan_entry.get_mut();
+				match chan
+					.funding_signed(&msg, best_block, &self.signer_provider, &self.logger)
+					.and_then(|(funded_chan, monitor)| {
+						self.chain_monitor
+							.watch_channel(funded_chan.context.get_funding_txo().unwrap(), monitor)
+							.map(|persist_status| (funded_chan, persist_status))
+							.map_err(|()| {
+								ChannelError::close("Channel funding outpoint was a duplicate".to_owned())
+							})
+					})
+				{
+					Ok((funded_chan, persist_status)) => {
+						handle_new_monitor_update!(self, persist_status, peer_state_lock, peer_state, per_peer_state, funded_chan, INITIAL_MONITOR);
+						Ok(())
+					},
+					Err(e) => {
+						// We weren't able to watch the channel to begin with, so no
+						// updates should be made on it. Previously, full_stack_target
+						// found an (unreachable) panic when the monitor update contained
+						// within `shutdown_finish` was applied.
+						chan.unset_funding_info();
+						try_channel_entry!(self, peer_state, Err(e), chan_entry)
+					},
 				}
 			},
 			hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))

lightning/src/ln/functional_tests.rs

@@ -9069,7 +9069,7 @@ fn test_duplicate_conflicting_funding_from_second_peer() {
 	check_added_monitors!(nodes[0], 1);
 	get_err_msg(&nodes[0], &nodes[1].node.get_our_node_id());
 	let err_reason = ClosureReason::ProcessingError { err: "Channel funding outpoint was a duplicate".to_owned() };
-	check_closed_events(&nodes[0], &[ExpectedCloseEvent::from_id_reason(funding_signed_msg.channel_id, true, err_reason)]);
+	check_closed_events(&nodes[0], &[ExpectedCloseEvent::from_id_reason(temp_chan_id, true, err_reason)]);
 }
 
 #[test]