Clean up and split up is_pre_funded_state

lightning/src/ln/channel.rs

@@ -804,11 +804,11 @@ impl ChannelState {
 		}
 	}
 
-	fn is_pre_funded_state(&self) -> bool {
+	fn can_resume_on_reconnect(&self) -> bool {
 		match self {
-			ChannelState::NegotiatingFunding(_) => true,
-			ChannelState::FundingNegotiated(flags) => !flags.is_interactive_signing(),
-			_ => false,
+			ChannelState::NegotiatingFunding(_) => false,
+			ChannelState::FundingNegotiated(flags) => flags.is_interactive_signing(),
+			_ => true,
 		}
 	}
 
@@ -4076,11 +4076,28 @@ where
 		self.is_manual_broadcast = true;
 	}
 
+	/// Returns true if this channel can be resume after a restart, implying its past the initial
+	/// funding negotiation stages (and any assocated batch channels are similarly past initial
+	/// funding negotiation).
+	///
+	/// This is equivalent to saying the channel can be persisted to disk.
+	pub fn can_resume_on_restart(&self) -> bool {
+		self.channel_state.can_resume_on_reconnect()
+			&& match self.channel_state {
+				ChannelState::AwaitingChannelReady(flags) => !flags.is_waiting_for_batch(),
+				_ => true,
+			}
+	}
+
 	/// Returns true if funding_signed was sent/received and the
 	/// funding transaction has been broadcast if necessary.
-	pub fn is_funding_broadcast(&self) -> bool {
-		!self.channel_state.is_pre_funded_state()
-			&& !matches!(self.channel_state, ChannelState::AwaitingChannelReady(flags) if flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH))
+	fn is_funding_broadcast(&self) -> bool {
+		match self.channel_state {
+			ChannelState::NegotiatingFunding(_) => false,
+			ChannelState::FundingNegotiated(flags) => !flags.is_our_tx_signatures_ready(),
+			ChannelState::AwaitingChannelReady(flags) => !flags.is_waiting_for_batch(),
+			_ => true,
+		}
 	}
 
 	#[rustfmt::skip]
@@ -5372,15 +5389,14 @@ where
 		}
 
 		let monitor_update = if let Some(funding_txo) = funding.get_funding_txo() {
-			// If we haven't yet exchanged funding signatures (ie channel_state < AwaitingChannelReady),
-			// returning a channel monitor update here would imply a channel monitor update before
-			// we even registered the channel monitor to begin with, which is invalid.
-			// Thus, if we aren't actually at a point where we could conceivably broadcast the
-			// funding transaction, don't return a funding txo (which prevents providing the
-			// monitor update to the user, even if we return one).
-			// See test_duplicate_chan_id and test_pre_lockin_no_chan_closed_update for more.
-			if !self.channel_state.is_pre_funded_state() {
+			// We should only generate a closing `ChannelMonitorUpdate` if we already have a
+			// `ChannelMonitor` for the disk (i.e. `cur_counterparty_commitment_transaction_number`
+			// has been decremented once, which hapens when we generate the initial
+			// `ChannelMonitor`).  Otherwise, that would imply a channel monitor update before we
+			// even registered the channel monitor to begin with, which is invalid.
+			if self.cur_counterparty_commitment_transaction_number != INITIAL_COMMITMENT_NUMBER {
 				self.latest_monitor_update_id = self.get_latest_unblocked_monitor_update_id() + 1;
+
 				let update = ChannelMonitorUpdate {
 					update_id: self.latest_monitor_update_id,
 					updates: vec![ChannelMonitorUpdateStep::ChannelForceClosed {
@@ -8116,12 +8132,12 @@ where
 	#[rustfmt::skip]
 	fn remove_uncommitted_htlcs_and_mark_paused<L: Deref>(&mut self, logger: &L) -> Result<(), ()> where L::Target: Logger {
 		assert!(!matches!(self.context.channel_state, ChannelState::ShutdownComplete));
-		if self.context.channel_state.is_pre_funded_state() {
+		if !self.context.channel_state.can_resume_on_reconnect() {
 			return Err(())
 		}
 
 		// We only clear `peer_disconnected` if we were able to reestablish the channel. We always
- 		// reset our awaiting response in case we failed reestablishment and are disconnecting.
+		// reset our awaiting response in case we failed reestablishment and are disconnecting.
 		self.context.sent_message_awaiting_response = None;
 
 		if self.context.channel_state.is_peer_disconnected() {
@@ -9107,13 +9123,20 @@ where
 				"Peer sent shutdown when we needed a channel_reestablish".to_owned(),
 			));
 		}
-		if self.context.channel_state.is_pre_funded_state() {
+		let mut not_broadcasted =
+			matches!(self.context.channel_state, ChannelState::NegotiatingFunding(_));
+		if let ChannelState::FundingNegotiated(flags) = &self.context.channel_state {
+			if !flags.is_our_tx_signatures_ready() {
+				// If we're a V1 channel or we haven't yet sent our `tx_signatures`, the funding tx
+				// couldn't be broadcasted yet, so just short-circuit the shutdown logic.
+				not_broadcasted = true;
+			}
+		}
+		if not_broadcasted {
 			// Spec says we should fail the connection, not the channel, but that's nonsense, there
 			// are plenty of reasons you may want to fail a channel pre-funding, and spec says you
 			// can do that via error message without getting a connection fail anyway...
-			return Err(ChannelError::close(
-				"Peer sent shutdown pre-funding generation".to_owned(),
-			));
+			return Err(ChannelError::close("Shutdown before funding was broadcasted".to_owned()));
 		}
 		for htlc in self.context.pending_inbound_htlcs.iter() {
 			if let InboundHTLCState::RemoteAnnounced(_) = htlc.state {

lightning/src/ln/channelmanager.rs

@@ -15199,7 +15199,7 @@ where
 				number_of_funded_channels += peer_state.channel_by_id
 					.values()
 					.filter_map(Channel::as_funded)
-					.filter(|chan| chan.context.is_funding_broadcast())
+					.filter(|chan| chan.context.can_resume_on_restart())
 					.count();
 			}
 
@@ -15211,7 +15211,7 @@ where
 				for channel in peer_state.channel_by_id
 					.values()
 					.filter_map(Channel::as_funded)
-					.filter(|channel| channel.context.is_funding_broadcast())
+					.filter(|channel| channel.context.can_resume_on_restart())
 				{
 					channel.write(writer)?;
 				}