Avoid resetting splice state on FundingNegotiation::AwaitingSignatures

Otherwise, we won't ever be able to resume a pending negotiation after a
reconnection via `channel_reestablish`.

Along the way, we also merge `should_reset_pending_splice_state` into
`PendingFunding::can_abandon_state` to simplify the logic around when
we're able to reset specific parts of the pending splice state.

Author: wpaulino

lightning/src/ln/channel.rs

@@ -1694,8 +1694,7 @@ where
 				pending_v2_channel.interactive_tx_constructor.take();
 			},
 			ChannelPhase::Funded(funded_channel) => {
-				if funded_channel.should_reset_pending_splice_funding_negotiation().unwrap_or(true)
-				{
+				if funded_channel.should_reset_pending_splice_state() {
 					funded_channel.reset_pending_splice_state();
 				} else {
 					debug_assert!(false, "We should never fail an interactive funding negotiation once we're exchanging tx_signatures");
@@ -1829,8 +1828,10 @@ where
 				pending_v2_channel.interactive_tx_constructor.take().is_some()
 			},
 			ChannelPhase::Funded(funded_channel) => {
-				if let Some(should_reset) =
-					funded_channel.should_reset_pending_splice_funding_negotiation()
+				if let Some(should_reset) = funded_channel
+					.pending_splice
+					.as_ref()
+					.map(|pending_splice| pending_splice.can_abandon_state())
 				{
 					if should_reset {
 						// We may have still tracked the pending funding negotiation state, so we
@@ -2583,13 +2584,13 @@ impl FundingNegotiation {
 }
 
 impl PendingFunding {
-	fn can_abandon_funding_negotiation(&self) -> bool {
+	fn can_abandon_state(&self) -> bool {
 		self.funding_negotiation
 			.as_ref()
 			.map(|funding_negotiation| {
 				!matches!(funding_negotiation, FundingNegotiation::AwaitingSignatures { .. })
 			})
-			.unwrap_or(true)
+			.unwrap_or(self.negotiated_candidates.is_empty())
 	}
 
 	fn check_get_splice_locked<SP: Deref>(
@@ -6773,40 +6774,25 @@ where
 		)
 	}
 
-	/// Returns `None` if there is no [`FundedChannel::pending_splice`], otherwise a boolean
-	/// indicating whether we should reset the splice's [`PendingFunding::funding_negotiation`].
-	fn should_reset_pending_splice_funding_negotiation(&self) -> Option<bool> {
-		self.pending_splice.as_ref().map(|pending_splice| {
-			if pending_splice.can_abandon_funding_negotiation() {
-				true
-			} else {
-				self.context
-					.interactive_tx_signing_session
-					.as_ref()
-					.map(|signing_session| !signing_session.has_received_commitment_signed())
-					.unwrap_or_else(|| {
-						debug_assert!(false);
-						false
-					})
-			}
-		})
-	}
-
+	/// Returns a boolean indicating whether we should reset the splice's
+	/// [`PendingFunding::funding_negotiation`].
 	fn should_reset_pending_splice_state(&self) -> bool {
-		self.should_reset_pending_splice_funding_negotiation().unwrap_or(true)
-			&& self.pending_funding().is_empty()
+		self.pending_splice
+			.as_ref()
+			.map(|pending_splice| pending_splice.can_abandon_state())
+			.unwrap_or(true)
 	}
 
 	fn reset_pending_splice_state(&mut self) -> bool {
-		debug_assert!(self.should_reset_pending_splice_funding_negotiation().unwrap_or(true));
+		debug_assert!(self.should_reset_pending_splice_state());
 		self.context.channel_state.clear_quiescent();
 		self.context.interactive_tx_signing_session.take();
 		let has_funding_negotiation = self
 			.pending_splice
 			.as_mut()
 			.and_then(|pending_splice| pending_splice.funding_negotiation.take())
 			.is_some();
-		if self.should_reset_pending_splice_state() {
+		if self.pending_funding().is_empty() {
 			self.pending_splice.take();
 		}
 		has_funding_negotiation
@@ -8948,7 +8934,7 @@ where
 			}
 			self.context.channel_state.clear_local_stfu_sent();
 			self.context.channel_state.clear_remote_stfu_sent();
-			if self.should_reset_pending_splice_funding_negotiation().unwrap_or(true) {
+			if self.should_reset_pending_splice_state() {
 				// If we were in quiescence but a splice was never negotiated, or the negotiation
 				// failed due to disconnecting, we shouldn't be quiescent anymore upon reconnecting.
 				// If there was a pending splice negotiation that has failed due to disconnecting,
@@ -14021,7 +14007,7 @@ where
 					}
 					channel_state.clear_local_stfu_sent();
 					channel_state.clear_remote_stfu_sent();
-					if self.should_reset_pending_splice_funding_negotiation().unwrap_or(true) {
+					if self.should_reset_pending_splice_state() {
 						// If we were in quiescence but a splice was never negotiated, or the
 						// negotiation failed due to disconnecting, we shouldn't be quiescent
 						// anymore upon reconnecting.
@@ -14389,19 +14375,14 @@ where
 		let holder_commitment_point_next = self.holder_commitment_point.next_point();
 		let holder_commitment_point_pending_next = self.holder_commitment_point.pending_next_point;
 
-		let interactive_tx_signing_session =
-			if self.should_reset_pending_splice_funding_negotiation().unwrap_or(false) {
-				None
+		let (interactive_tx_signing_session, pending_splice) =
+			if self.should_reset_pending_splice_state() {
+				(None, None)
 			} else {
-				self.context.interactive_tx_signing_session.as_ref()
+				// We don't have to worry about resetting the pending `FundingNegotiation` because we
+				// can only read `FundingNegotiation::AwaitingSignatures` variants anyway.
+				(self.context.interactive_tx_signing_session.as_ref(), self.pending_splice.as_ref())
 			};
-		let pending_splice = if self.should_reset_pending_splice_state() {
-			None
-		} else {
-			// We don't have to worry about resetting the pending `FundingNegotiation` because we
-			// can only read `FundingNegotiation::AwaitingSignatures` variants anyway.
-			self.pending_splice.as_ref()
-		};
 
 		write_tlv_fields!(writer, {
 			(0, self.context.announcement_sigs, option),

lightning/src/ln/splicing_tests.rs

@@ -414,9 +414,9 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) {
 		)
 		.unwrap();
 
-	// Attempt a splice negotiation that only goes up to exchanging `tx_complete`. Reconnecting
-	// should implicitly abort the negotiation and reset the splice state such that we're able to
-	// retry another splice later.
+	// Attempt a splice negotiation that ends mid-construction of the funding transaction.
+	// Reconnecting should implicitly abort the negotiation and reset the splice state such that
+	// we're able to retry another splice later.
 	let stfu = get_event_msg!(nodes[0], MessageSendEvent::SendStfu, node_id_1);
 	nodes[1].node.handle_stfu(node_id_0, &stfu);
 	let stfu = get_event_msg!(nodes[1], MessageSendEvent::SendStfu, node_id_0);
@@ -427,18 +427,9 @@ fn do_test_splice_state_reset_on_disconnect(reload: bool) {
 	let splice_ack = get_event_msg!(nodes[1], MessageSendEvent::SendSpliceAck, node_id_0);
 	nodes[0].node.handle_splice_ack(node_id_1, &splice_ack);
 
-	let new_funding_script = chan_utils::make_funding_redeemscript(
-		&splice_init.funding_pubkey,
-		&splice_ack.funding_pubkey,
-	)
-	.to_p2wsh();
-	let _ = complete_interactive_funding_negotiation(
-		&nodes[0],
-		&nodes[1],
-		channel_id,
-		contribution.clone(),
-		new_funding_script,
-	);
+	let tx_add_input = get_event_msg!(nodes[0], MessageSendEvent::SendTxAddInput, node_id_1);
+	nodes[1].node.handle_tx_add_input(node_id_0, &tx_add_input);
+	let _ = get_event_msg!(nodes[1], MessageSendEvent::SendTxComplete, node_id_0);
 
 	if reload {
 		let encoded_monitor_0 = get_monitor!(nodes[0], channel_id).encode();