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,32 @@ 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::Intermediate);
+		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);
+					let funded_chan = self.as_funded_mut().expect("Channel should be funded");
+					Ok((funded_chan, monitor))
+				},
+				Err((chan, e)) => {
+					self.phase = ChannelPhase::UnfundedOutboundV1(chan);
+					Err(e)
+				},
+			}
+		} else {
+			self.phase = phase;
+			Err(ChannelError::NotFound("Failed to find corresponding channel".to_owned()))
+		}
+	}
 }
 
 impl<SP: Deref> From<OutboundV1Channel<SP>> for Channel<SP>

lightning/src/ln/channelmanager.rs

@@ -8157,49 +8157,29 @@ 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());
+			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_err(|()| {
 								// 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));
+								funded_chan.unset_funding_info(msg.channel_id);
+								ChannelError::close("Channel funding outpoint was a duplicate".to_owned())
+							})
+							.map(|persist_status| (funded_chan, persist_status))
+					})
+				{
+					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) => 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))