Review comments

Author: wvanlint

lightning/src/events/mod.rs

@@ -841,6 +841,8 @@ pub enum Event {
 	},
 	/// Used to indicate to the user that they can abandon the funding transaction and recycle the
 	/// inputs for another purpose.
+	///
+	/// This event is not guaranteed to be generated for channels that are closed due to a restart.
 	DiscardFunding {
 		/// The channel_id of the channel which has been closed.
 		channel_id: ChannelId,

lightning/src/ln/channel.rs

@@ -305,8 +305,19 @@ enum ChannelState {
 	/// have received funding_signed and have their monitors persisted.
 	WaitingForBatch = 1 << 13,
 }
-const BOTH_SIDES_SHUTDOWN_MASK: u32 = ChannelState::LocalShutdownSent as u32 | ChannelState::RemoteShutdownSent as u32;
-const MULTI_STATE_FLAGS: u32 = BOTH_SIDES_SHUTDOWN_MASK | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32;
+const BOTH_SIDES_SHUTDOWN_MASK: u32 =
+	ChannelState::LocalShutdownSent as u32 |
+	ChannelState::RemoteShutdownSent as u32;
+const MULTI_STATE_FLAGS: u32 =
+	BOTH_SIDES_SHUTDOWN_MASK |
+	ChannelState::PeerDisconnected as u32 |
+	ChannelState::MonitorUpdateInProgress as u32;
+const STATE_FLAGS: u32 =
+	MULTI_STATE_FLAGS |
+	ChannelState::TheirChannelReady as u32 |
+	ChannelState::OurChannelReady as u32 |
+	ChannelState::AwaitingRemoteRevoke as u32 |
+	ChannelState::WaitingForBatch as u32;
 
 pub const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
 
@@ -918,7 +929,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 
 	/// Returns true if we've ever received a message from the remote end for this Channel
 	pub fn have_received_message(&self) -> bool {
-		self.channel_state > (ChannelState::OurInitSent as u32)
+		self.channel_state & !STATE_FLAGS > (ChannelState::OurInitSent as u32)
 	}
 
 	/// Returns true if this channel is fully established and not known to be closing.
@@ -1196,7 +1207,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 	/// Returns true if funding_signed was sent/received and the
 	/// funding transaction has been broadcast if necessary.
 	pub fn is_funding_initiated(&self) -> bool {
-		self.channel_state >= ChannelState::FundingSent as u32 &&
+		self.channel_state & !STATE_FLAGS >= ChannelState::FundingSent as u32 &&
 		self.channel_state & ChannelState::WaitingForBatch as u32 == 0
 	}
 
@@ -2616,6 +2627,8 @@ impl<SP: Deref> Channel<SP> where
 
 		let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
 
+		// If the WaitingForBatch flag is set, we can receive their channel_ready, but our
+		// channel_ready shouldn't have been sent and we shouldn't move to ChannelReady.
 		if non_shutdown_state & !(ChannelState::WaitingForBatch as u32) == ChannelState::FundingSent as u32 {
 			self.context.channel_state |= ChannelState::TheirChannelReady as u32;
 		} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurChannelReady as u32) {
@@ -3115,7 +3128,7 @@ impl<SP: Deref> Channel<SP> where
 	) -> (Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>)
 	where F::Target: FeeEstimator, L::Target: Logger
 	{
-		if self.context.channel_state >= ChannelState::ChannelReady as u32 &&
+		if self.context.channel_state & !STATE_FLAGS >= ChannelState::ChannelReady as u32 &&
 		   (self.context.channel_state & (ChannelState::AwaitingRemoteRevoke as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32)) == 0 {
 			self.free_holding_cell_htlcs(fee_estimator, logger)
 		} else { (None, Vec::new()) }
@@ -3591,7 +3604,7 @@ impl<SP: Deref> Channel<SP> where
 	/// completed.
 	pub fn remove_uncommitted_htlcs_and_mark_paused<L: Deref>(&mut self, logger: &L)  where L::Target: Logger {
 		assert_eq!(self.context.channel_state & ChannelState::ShutdownComplete as u32, 0);
-		if self.context.channel_state < ChannelState::FundingSent as u32 {
+		if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
 			self.context.channel_state = ChannelState::ShutdownComplete as u32;
 			return;
 		}
@@ -3705,13 +3718,13 @@ impl<SP: Deref> Channel<SP> where
 		// (re-)broadcast the funding transaction as we may have declined to broadcast it when we
 		// first received the funding_signed.
 		let mut funding_broadcastable =
-			if self.context.is_outbound() && self.context.channel_state & !MULTI_STATE_FLAGS >= ChannelState::FundingSent as u32 && self.context.channel_state & ChannelState::WaitingForBatch as u32 == 0 {
+			if self.context.is_outbound() && self.context.channel_state & !STATE_FLAGS >= ChannelState::FundingSent as u32 && self.context.channel_state & ChannelState::WaitingForBatch as u32 == 0 {
 				self.context.funding_txid.take();
 				self.context.funding_transaction.take()
 			} else { None };
 		// That said, if the funding transaction is already confirmed (ie we're active with a
 		// minimum_depth over 0) don't bother re-broadcasting the confirmed funding tx.
-		if self.context.channel_state & !MULTI_STATE_FLAGS >= ChannelState::ChannelReady as u32 && self.context.minimum_depth != Some(0) {
+		if self.context.channel_state & !STATE_FLAGS >= ChannelState::ChannelReady as u32 && self.context.minimum_depth != Some(0) {
 			funding_broadcastable = None;
 		}
 
@@ -4214,7 +4227,7 @@ impl<SP: Deref> Channel<SP> where
 		if self.context.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
 			return Err(ChannelError::Close("Peer sent shutdown when we needed a channel_reestablish".to_owned()));
 		}
-		if self.context.channel_state < ChannelState::FundingSent as u32 {
+		if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
 			// 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...
@@ -4639,7 +4652,7 @@ impl<SP: Deref> Channel<SP> where
 
 	/// Returns true if our channel_ready has been sent
 	pub fn is_our_channel_ready(&self) -> bool {
-		(self.context.channel_state & ChannelState::OurChannelReady as u32) != 0 || self.context.channel_state >= ChannelState::ChannelReady as u32
+		(self.context.channel_state & ChannelState::OurChannelReady as u32) != 0 || self.context.channel_state & !STATE_FLAGS >= ChannelState::ChannelReady as u32
 	}
 
 	/// Returns true if our peer has either initiated or agreed to shut down the channel.
@@ -4702,7 +4715,7 @@ impl<SP: Deref> Channel<SP> where
 			// We got a reorg but not enough to trigger a force close, just ignore.
 			false
 		} else {
-			if self.context.funding_tx_confirmation_height != 0 && self.context.channel_state < ChannelState::ChannelReady as u32 {
+			if self.context.funding_tx_confirmation_height != 0 && self.context.channel_state & !STATE_FLAGS < ChannelState::ChannelReady as u32 {
 				// We should never see a funding transaction on-chain until we've received
 				// funding_signed (if we're an outbound channel), or seen funding_generated (if we're
 				// an inbound channel - before that we have no known funding TXID). The fuzzer,
@@ -4863,7 +4876,7 @@ impl<SP: Deref> Channel<SP> where
 		}
 
 		let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
-		if non_shutdown_state >= ChannelState::ChannelReady as u32 ||
+		if non_shutdown_state & !STATE_FLAGS >= ChannelState::ChannelReady as u32 ||
 		   (non_shutdown_state & ChannelState::OurChannelReady as u32) == ChannelState::OurChannelReady as u32 {
 			let mut funding_tx_confirmations = height as i64 - self.context.funding_tx_confirmation_height as i64 + 1;
 			if self.context.funding_tx_confirmation_height == 0 {
@@ -4891,7 +4904,7 @@ impl<SP: Deref> Channel<SP> where
 				height >= self.context.channel_creation_height + FUNDING_CONF_DEADLINE_BLOCKS {
 			log_info!(logger, "Closing channel {} due to funding timeout", &self.context.channel_id);
 			// If funding_tx_confirmed_in is unset, the channel must not be active
-			assert!(non_shutdown_state <= ChannelState::ChannelReady as u32);
+			assert!(non_shutdown_state & !STATE_FLAGS <= ChannelState::ChannelReady as u32);
 			assert_eq!(non_shutdown_state & ChannelState::OurChannelReady as u32, 0);
 			return Err(ClosureReason::FundingTimedOut);
 		}
@@ -5511,7 +5524,7 @@ impl<SP: Deref> Channel<SP> where
 		// If we haven't funded the channel yet, we don't need to bother ensuring the shutdown
 		// script is set, we just force-close and call it a day.
 		let mut chan_closed = false;
-		if self.context.channel_state < ChannelState::FundingSent as u32 {
+		if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
 			chan_closed = true;
 		}
 
@@ -5540,7 +5553,7 @@ impl<SP: Deref> Channel<SP> where
 
 		// From here on out, we may not fail!
 		self.context.target_closing_feerate_sats_per_kw = target_feerate_sats_per_kw;
-		if self.context.channel_state < ChannelState::FundingSent as u32 {
+		if self.context.channel_state & !STATE_FLAGS < ChannelState::FundingSent as u32 {
 			self.context.channel_state = ChannelState::ShutdownComplete as u32;
 		} else {
 			self.context.channel_state |= ChannelState::LocalShutdownSent as u32;
@@ -7339,7 +7352,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 			// If we've gotten to the funding stage of the channel, populate the signer with its
 			// required channel parameters.
 			let non_shutdown_state = channel_state & (!MULTI_STATE_FLAGS);
-			if non_shutdown_state >= (ChannelState::FundingCreated as u32) {
+			if non_shutdown_state & !STATE_FLAGS >= (ChannelState::FundingCreated as u32) {
 				holder_signer.provide_channel_parameters(&channel_parameters);
 			}
 			(channel_keys_id, holder_signer)
@@ -9064,13 +9077,10 @@ mod tests {
 			&config,
 			0,
 			&&logger,
-			42,
+			true,  // Allow node b to send a 0conf channel_ready.
 		).unwrap();
 
-		// Allow node b to send a 0conf channel_ready.
-		node_b_chan.set_0conf();
-
-		let accept_channel_msg = node_b_chan.accept_inbound_channel(0);
+		let accept_channel_msg = node_b_chan.accept_inbound_channel();
 		node_a_chan.accept_channel(
 			&accept_channel_msg,
 			&config.channel_handshake_limits,