Review comments

Author: wvanlint

lightning/src/ln/channel.rs

@@ -255,7 +255,6 @@ enum HTLCUpdateAwaitingACK {
 /// Note that `PeerDisconnected` can be set on both `ChannelReady` and `FundingSent`.
 /// `ChannelReady` can then get all remaining flags set on it, until we finish shutdown, then we
 /// move on to `ShutdownComplete`, at which point most calls into this channel are disallowed.
-#[derive(Clone, Copy)]
 enum ChannelState {
 	/// Implies we have (or are prepared to) send our open_channel/accept_channel message
 	OurInitSent = 1 << 0,
@@ -319,54 +318,6 @@ const STATE_FLAGS: u32 =
 	ChannelState::OurChannelReady as u32 |
 	ChannelState::AwaitingRemoteRevoke as u32 |
 	ChannelState::WaitingForBatch as u32;
-const CHANNEL_STATE_ENUMS: [ChannelState; 14] = [
-	ChannelState::OurInitSent,
-	ChannelState::TheirInitSent,
-	ChannelState::FundingCreated,
-	ChannelState::FundingSent,
-	ChannelState::TheirChannelReady,
-	ChannelState::OurChannelReady,
-	ChannelState::ChannelReady,
-	ChannelState::PeerDisconnected,
-	ChannelState::MonitorUpdateInProgress,
-	ChannelState::AwaitingRemoteRevoke,
-	ChannelState::RemoteShutdownSent,
-	ChannelState::LocalShutdownSent,
-	ChannelState::ShutdownComplete,
-	ChannelState::WaitingForBatch,
-];
-
-struct ChannelStateInt(u32);
-
-impl IntoIterator for ChannelStateInt {
-	type Item = ChannelState;
-	type IntoIter = Box<dyn Iterator<Item = ChannelState>>;
-
-	fn into_iter(self) -> Self::IntoIter {
-		Box::new(
-			CHANNEL_STATE_ENUMS.iter()
-			.filter(move |&channel_state| self.0 & *channel_state as u32 != 0)
-			.map(|channel_state| channel_state.clone())
-		)
-	}
-}
-
-impl_writeable_tlv_based_enum_upgradable!(ChannelState,
-	(0, OurInitSent) => {},
-	(2, TheirInitSent) => {},
-	(4, FundingCreated) => {},
-	(6, FundingSent) => {},
-	(8, TheirChannelReady) => {},
-	(10, OurChannelReady) => {},
-	(12, ChannelReady) => {},
-	(14, PeerDisconnected) => {},
-	(16, MonitorUpdateInProgress) => {},
-	(18, AwaitingRemoteRevoke) => {},
-	(20, RemoteShutdownSent) => {},
-	(22, LocalShutdownSent) => {},
-	(24, ShutdownComplete) => {},
-	(26, WaitingForBatch) => {},
-);
 
 pub const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
 
@@ -854,6 +805,7 @@ pub(super) struct ChannelContext<SP: Deref> where SP::Target: SignerProvider {
 	pub(crate) channel_transaction_parameters: ChannelTransactionParameters,
 	funding_transaction: Option<Transaction>,
 	funding_txid: Option<Txid>,
+	is_batch_funding: Option<bool>,
 
 	counterparty_cur_commitment_point: Option<PublicKey>,
 	counterparty_prev_commitment_point: Option<PublicKey>,
@@ -1990,7 +1942,7 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 	fn if_unbroadcasted_funding<F, O>(&self, f: F) -> Option<O>
 		where F: Fn() -> Option<O> {
 		if self.channel_state & ChannelState::FundingCreated as u32 != 0 ||
-				self.channel_state & ChannelState::WaitingForBatch as u32 != 0 {
+		   self.channel_state & ChannelState::WaitingForBatch as u32 != 0 {
 			f()
 		} else {
 			None
@@ -2009,6 +1961,11 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 		self.if_unbroadcasted_funding(|| self.funding_txid.clone())
 	}
 
+	/// Returns whether the channel is funded in a batch.
+	pub fn is_batch_funding(&self) -> bool {
+		self.is_batch_funding.unwrap_or(false)
+	}
+
 	/// Gets the latest commitment transaction and any dependent transactions for relay (forcing
 	/// shutdown of this channel - no more calls into this Channel may be made afterwards except
 	/// those explicitly stated to be allowed after shutdown completes, eg some simple getters).
@@ -2552,7 +2509,7 @@ impl<SP: Deref> Channel<SP> where
 	/// Handles a funding_signed message from the remote end.
 	/// If this call is successful, broadcast the funding transaction (and not before!)
 	pub fn funding_signed<L: Deref>(
-		&mut self, msg: &msgs::FundingSigned, best_block: BestBlock, signer_provider: &SP, is_batch_funding: bool, logger: &L
+		&mut self, msg: &msgs::FundingSigned, best_block: BestBlock, signer_provider: &SP, logger: &L
 	) -> Result<ChannelMonitor<<SP::Target as SignerProvider>::Signer>, ChannelError>
 	where
 		L::Target: Logger
@@ -2627,7 +2584,7 @@ impl<SP: Deref> Channel<SP> where
 			counterparty_initial_commitment_tx.to_countersignatory_value_sat(), logger);
 
 		assert_eq!(self.context.channel_state & (ChannelState::MonitorUpdateInProgress as u32), 0); // We have no had any monitor(s) yet to fail update!
-		if is_batch_funding {
+		if self.context.is_batch_funding() {
 			self.context.channel_state = ChannelState::FundingSent as u32 | ChannelState::WaitingForBatch as u32;
 		} else {
 			self.context.channel_state = ChannelState::FundingSent as u32;
@@ -5830,6 +5787,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 				},
 				funding_transaction: None,
 				funding_txid: None,
+				is_batch_funding: None,
 
 				counterparty_cur_commitment_point: None,
 				counterparty_prev_commitment_point: None,
@@ -5890,7 +5848,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 	/// Note that channel_id changes during this call!
 	/// Do NOT broadcast the funding transaction until after a successful funding_signed call!
 	/// If an Err is returned, it is a ChannelError::Close.
-	pub fn get_funding_created<L: Deref>(mut self, funding_transaction: Transaction, funding_txo: OutPoint, logger: &L)
+	pub fn get_funding_created<L: Deref>(mut self, funding_transaction: Transaction, funding_txo: OutPoint, is_batch_funding: bool, logger: &L)
 	-> Result<(Channel<SP>, msgs::FundingCreated), (Self, ChannelError)> where L::Target: Logger {
 		if !self.context.is_outbound() {
 			panic!("Tried to create outbound funding_created message on an inbound channel!");
@@ -5924,6 +5882,7 @@ impl<SP: Deref> OutboundV1Channel<SP> where SP::Target: SignerProvider {
 		self.context.channel_id = funding_txo.to_channel_id();
 		self.context.funding_transaction = Some(funding_transaction);
 		self.context.funding_txid = self.context.funding_transaction.as_ref().map(|tx| tx.txid());
+		self.context.is_batch_funding = Some(is_batch_funding);
 
 		let channel = Channel {
 			context: self.context,
@@ -6474,6 +6433,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
 				},
 				funding_transaction: None,
 				funding_txid: None,
+				is_batch_funding: None,
 
 				counterparty_cur_commitment_point: Some(msg.first_per_commitment_point),
 				counterparty_prev_commitment_point: None,
@@ -7090,9 +7050,9 @@ impl<SP: Deref> Writeable for Channel<SP> where SP::Target: SignerProvider {
 			(35, pending_outbound_skimmed_fees, optional_vec),
 			(37, holding_cell_skimmed_fees, optional_vec),
 			(
-				if self.context.channel_state & ChannelState::WaitingForBatch as u32 != 0 { 38 } else { 39 },
-				ChannelStateInt(self.context.channel_state).into_iter().collect::<Vec<ChannelState>>(),
-				optional_vec
+				if self.context.is_batch_funding() { 38 } else { 39 },
+				self.context.is_batch_funding,
+				option
 			),
 		});
 
@@ -7378,8 +7338,8 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 		let mut pending_outbound_skimmed_fees_opt: Option<Vec<Option<u64>>> = None;
 		let mut holding_cell_skimmed_fees_opt: Option<Vec<Option<u64>>> = None;
 
-		let mut _channel_states_even: Option<Vec<ChannelState>>;
-		let mut _channel_states_uneven: Option<Vec<ChannelState>>;
+		let mut is_batch_funding_even: Option<bool> = None;
+		let mut is_batch_funding_uneven: Option<bool> = None;
 
 		read_tlv_fields!(reader, {
 			(0, announcement_sigs, option),
@@ -7406,8 +7366,8 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 			(31, channel_pending_event_emitted, option),
 			(35, pending_outbound_skimmed_fees_opt, optional_vec),
 			(37, holding_cell_skimmed_fees_opt, optional_vec),
-			(38, _channel_states_even, optional_vec),
-			(39, _channel_states_uneven, optional_vec),
+			(38, is_batch_funding_even, option),
+			(39, is_batch_funding_uneven, option),
 		});
 
 		let (channel_keys_id, holder_signer) = if let Some(channel_keys_id) = channel_keys_id {
@@ -7566,6 +7526,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
 				channel_transaction_parameters: channel_parameters,
 				funding_transaction,
 				funding_txid,
+				is_batch_funding: is_batch_funding_even.or(is_batch_funding_uneven),
 
 				counterparty_cur_commitment_point,
 				counterparty_prev_commitment_point,
@@ -7798,11 +7759,11 @@ mod tests {
 			value: 10000000, script_pubkey: output_script.clone(),
 		}]};
 		let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
-		let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(tx.clone(), funding_outpoint, &&logger).map_err(|_| ()).unwrap();
+		let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(tx.clone(), funding_outpoint, false, &&logger).map_err(|_| ()).unwrap();
 		let (_, funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&keys_provider, &&logger).map_err(|_| ()).unwrap();
 
 		// Node B --> Node A: funding signed
-		let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, false, &&logger).unwrap();
+		let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, &&logger).unwrap();
 
 		// Put some inbound and outbound HTLCs in A's channel.
 		let htlc_amount_msat = 11_092_000; // put an amount below A's effective dust limit but above B's.
@@ -7925,11 +7886,11 @@ mod tests {
 			value: 10000000, script_pubkey: output_script.clone(),
 		}]};
 		let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
-		let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(tx.clone(), funding_outpoint, &&logger).map_err(|_| ()).unwrap();
+		let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(tx.clone(), funding_outpoint, false, &&logger).map_err(|_| ()).unwrap();
 		let (mut node_b_chan, funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&keys_provider, &&logger).map_err(|_| ()).unwrap();
 
 		// Node B --> Node A: funding signed
-		let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, false, &&logger).unwrap();
+		let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, &&logger).unwrap();
 
 		// Now disconnect the two nodes and check that the commitment point in
 		// Node B's channel_reestablish message is sane.
@@ -8113,11 +8074,11 @@ mod tests {
 			value: 10000000, script_pubkey: output_script.clone(),
 		}]};
 		let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
-		let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(tx.clone(), funding_outpoint, &&logger).map_err(|_| ()).unwrap();
+		let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(tx.clone(), funding_outpoint, false, &&logger).map_err(|_| ()).unwrap();
 		let (_, funding_signed_msg, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&keys_provider, &&logger).map_err(|_| ()).unwrap();
 
 		// Node B --> Node A: funding signed
-		let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, false, &&logger).unwrap();
+		let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, &&logger).unwrap();
 
 		// Make sure that receiving a channel update will update the Channel as expected.
 		let update = ChannelUpdate {
@@ -9168,6 +9129,7 @@ mod tests {
 		let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(
 			tx.clone(),
 			funding_outpoint,
+			true,
 			&&logger,
 		).map_err(|_| ()).unwrap();
 		let (mut node_b_chan, funding_signed_msg, _) = node_b_chan.funding_created(
@@ -9190,7 +9152,6 @@ mod tests {
 			&funding_signed_msg,
 			best_block,
 			&&keys_provider,
-			true,
 			&&logger,
 		).unwrap();
 		let node_a_updates = node_a_chan.monitor_updating_restored(