Address ChannelState inconsistency throughout splicing

Once a channel open has become locked (i.e., we've entered
`ChannelState::ChannelReady`), the channel is intended to remain within
this state for the rest of its lifetime until shutdown. Previously, we
had assumed a channel being spliced would go through the `ChannelState`
lifecycle again starting from `NegotiatingFunding` but skipping
`AwaitingChannelReady`. This inconsistency departs from what we strive
to achieve with `ChannelState` and also makes the state of a channel
harder to reason about.

This commit ensures a channel undergoing a splice remains in
`ChannelReady`, clearing the quiescent flag once the negotiation is
complete. Dual funding is unaffected by this change as the channel is
being opened and we want to maintain the same `ChannelState` lifecycle.

Author: wpaulino

lightning/src/ln/channel.rs

@@ -718,9 +718,9 @@ enum ChannelState {
 	/// `AwaitingChannelReady`. Note that this is nonsense for an inbound channel as we immediately generate
 	/// `funding_signed` upon receipt of `funding_created`, so simply skip this state.
 	///
-	/// For inbound and outbound interactively funded channels (dual-funding/splicing), this flag indicates
-	/// that interactive transaction construction has been completed and we are now interactively signing
-	/// the funding/splice transaction.
+	/// For inbound and outbound interactively funded channels (dual-funding), this state indicates
+	/// that interactive transaction construction has been completed and we are now interactively
+	/// signing the initial funding transaction.
 	FundingNegotiated(FundingNegotiatedFlags),
 	/// We've received/sent `funding_created` and `funding_signed` and are thus now waiting on the
 	/// funding transaction to confirm.
@@ -1932,6 +1932,14 @@ where
 		let logger = WithChannelContext::from(logger, self.context(), None);
 		match &mut self.phase {
 			ChannelPhase::UnfundedV2(chan) => {
+				debug_assert_eq!(
+					chan.context.channel_state,
+					ChannelState::NegotiatingFunding(
+						NegotiatingFundingFlags::OUR_INIT_SENT
+							| NegotiatingFundingFlags::THEIR_INIT_SENT
+					),
+				);
+
 				let signing_session = chan
 					.interactive_tx_constructor
 					.take()
@@ -6116,7 +6124,6 @@ where
 		funding
 			.channel_transaction_parameters.funding_outpoint = Some(outpoint);
 		self.interactive_tx_signing_session = Some(signing_session);
-		self.channel_state = ChannelState::FundingNegotiated(FundingNegotiatedFlags::new());
 
 		if is_splice {
 			debug_assert_eq!(
@@ -6127,6 +6134,7 @@ where
 			return Err(AbortReason::InternalError("Splicing not yet supported"));
 		} else {
 			self.assert_no_commitment_advancement(holder_commitment_transaction_number, "initial commitment_signed");
+			self.channel_state = ChannelState::FundingNegotiated(FundingNegotiatedFlags::new());
 		}
 
 		let commitment_signed = self.get_initial_commitment_signed_v2(&funding, logger);
@@ -6211,9 +6219,7 @@ where
 		SP::Target: SignerProvider,
 		L::Target: Logger,
 	{
-		assert!(
-			matches!(self.channel_state, ChannelState::FundingNegotiated(_) if self.interactive_tx_signing_session.is_some())
-		);
+		debug_assert!(self.interactive_tx_signing_session.is_some());
 
 		let signature = self.get_initial_counterparty_commitment_signature(funding, logger);
 		if let Some(signature) = signature {
@@ -8573,11 +8579,43 @@ where
 		}
 	}
 
+	fn on_tx_signatures_exchange(&mut self, funding_tx: Transaction) {
+		debug_assert!(!self.context.channel_state.is_monitor_update_in_progress());
+		debug_assert!(!self.context.channel_state.is_awaiting_remote_revoke());
+
+		if let Some(pending_splice) = self.pending_splice.as_mut() {
+			if let Some(FundingNegotiation::AwaitingSignatures { mut funding }) =
+				pending_splice.funding_negotiation.take()
+			{
+				funding.funding_transaction = Some(funding_tx);
+				pending_splice.negotiated_candidates.push(funding);
+			} else {
+				debug_assert!(false);
+			}
+			self.context.channel_state.clear_quiescent();
+		} else {
+			self.funding.funding_transaction = Some(funding_tx);
+			self.context.channel_state =
+				ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
+		}
+	}
+
 	pub fn funding_transaction_signed(
 		&mut self, funding_txid_signed: Txid, witnesses: Vec<Witness>,
 	) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), APIError> {
 		let signing_session =
 			if let Some(signing_session) = self.context.interactive_tx_signing_session.as_mut() {
+				if let Some(pending_splice) = self.pending_splice.as_ref() {
+					debug_assert!(pending_splice
+						.funding_negotiation
+						.as_ref()
+						.map(|funding_negotiation| matches!(
+							funding_negotiation,
+							FundingNegotiation::AwaitingSignatures { .. }
+						))
+						.unwrap_or(false));
+				}
+
 				signing_session
 			} else {
 				let err =
@@ -8620,24 +8658,40 @@ where
 			.provide_holder_witnesses(tx_signatures, &self.context.secp_ctx)
 			.map_err(|err| APIError::APIMisuseError { err })?;
 
-		if funding_tx_opt.is_some() {
-			self.funding.funding_transaction = funding_tx_opt.clone();
-			self.context.channel_state =
-				ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
+		if let Some(funding_tx) = funding_tx_opt.clone() {
+			debug_assert!(tx_signatures_opt.is_some());
+			self.on_tx_signatures_exchange(funding_tx);
 		}
 
 		Ok((tx_signatures_opt, funding_tx_opt))
 	}
 
-	#[rustfmt::skip]
-	pub fn tx_signatures(&mut self, msg: &msgs::TxSignatures) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), ChannelError> {
-		let signing_session = if let Some(signing_session) = self.context.interactive_tx_signing_session.as_mut() {
+	pub fn tx_signatures(
+		&mut self, msg: &msgs::TxSignatures,
+	) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), ChannelError> {
+		let signing_session = if let Some(signing_session) =
+			self.context.interactive_tx_signing_session.as_mut()
+		{
 			if signing_session.has_received_tx_signatures() {
 				return Err(ChannelError::Ignore("Ignoring duplicate tx_signatures".to_owned()));
 			}
 			if !signing_session.has_received_commitment_signed() {
-				return Err(ChannelError::close("Received tx_signatures before initial commitment_signed".to_owned()));
+				return Err(ChannelError::close(
+					"Received tx_signatures before initial commitment_signed".to_owned(),
+				));
+			}
+
+			if let Some(pending_splice) = self.pending_splice.as_ref() {
+				debug_assert!(pending_splice
+					.funding_negotiation
+					.as_ref()
+					.map(|funding_negotiation| matches!(
+						funding_negotiation,
+						FundingNegotiation::AwaitingSignatures { .. }
+					))
+					.unwrap_or(false));
 			}
+
 			signing_session
 		} else {
 			return Err(ChannelError::Ignore("Ignoring unexpected tx_signatures".to_owned()));
@@ -8657,16 +8711,11 @@ where
 			}
 		}
 
-		let (holder_tx_signatures_opt, funding_tx_opt) = signing_session.received_tx_signatures(msg)
-			.map_err(|msg| ChannelError::Warn(msg))?;
+		let (holder_tx_signatures_opt, funding_tx_opt) =
+			signing_session.received_tx_signatures(msg).map_err(|msg| ChannelError::Warn(msg))?;
 
-		if funding_tx_opt.is_some() {
-			// TODO(splicing): Transition back to `ChannelReady` and not `AwaitingChannelReady`
-			// We will also need to use the pending `FundingScope` in the splicing case.
-			//
-			// We have a finalized funding transaction, so we can set the funding transaction.
-			self.funding.funding_transaction = funding_tx_opt.clone();
-			self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
+		if let Some(funding_tx) = funding_tx_opt.clone() {
+			self.on_tx_signatures_exchange(funding_tx);
 		}
 
 		Ok((holder_tx_signatures_opt, funding_tx_opt))