Validate negative funding contributions in splice_init and splice_ack messages

lightning/src/ln/channel.rs

@@ -2248,7 +2248,7 @@ impl FundingScope {
 	fn for_splice<SP: Deref>(
 		prev_funding: &Self, context: &ChannelContext<SP>, our_funding_contribution: SignedAmount,
 		their_funding_contribution: SignedAmount, counterparty_funding_pubkey: PublicKey,
-	) -> Result<Self, ChannelError>
+	) -> Self
 	where
 		SP::Target: SignerProvider,
 	{
@@ -2298,7 +2298,7 @@ impl FundingScope {
 		let holder_selected_channel_reserve_satoshis =
 			get_v2_channel_reserve_satoshis(post_channel_value, MIN_CHAN_DUST_LIMIT_SATOSHIS);
 
-		Ok(Self {
+		Self {
 			channel_transaction_parameters: post_channel_transaction_parameters,
 			value_to_self_msat: post_value_to_self_msat,
 			funding_transaction: None,
@@ -2322,7 +2322,7 @@ impl FundingScope {
 			funding_tx_confirmed_in: None,
 			minimum_depth_override: None,
 			short_channel_id: None,
-		})
+		}
 	}
 
 	/// Compute the post-splice channel value from each counterparty's contributions.
@@ -10944,6 +10944,8 @@ where
 			)));
 		}
 
+		// TODO(splicing): Once splice acceptor can contribute, check that inputs are sufficient,
+		// similarly to the check in `splice_channel`.
 		debug_assert_eq!(our_funding_contribution, SignedAmount::ZERO);
 
 		// TODO(splicing): Move this check once user-provided contributions are supported for
@@ -10965,33 +10967,18 @@ where
 		}
 
 		let their_funding_contribution = SignedAmount::from_sat(msg.funding_contribution_satoshis);
-		self.validate_splice_contribution(their_funding_contribution)?;
-
-		// TODO(splicing): Check that channel balance does not go below the channel reserve
-
-		let splice_funding = FundingScope::for_splice(
-			&self.funding,
-			&self.context,
+		self.validate_splice_contribution(
 			our_funding_contribution,
 			their_funding_contribution,
 			msg.funding_pubkey,
-		)?;
-
-		// TODO(splicing): Once splice acceptor can contribute, check that inputs are sufficient,
-		// similarly to the check in `splice_channel`.
-
-		// Note on channel reserve requirement pre-check: as the splice acceptor does not contribute,
-		// it can't go below reserve, therefore no pre-check is done here.
-
-		// TODO(splicing): Early check for reserve requirement
-
-		Ok(splice_funding)
+		)
 	}
 
 	#[cfg(splicing)]
 	fn validate_splice_contribution(
-		&self, their_funding_contribution: SignedAmount,
-	) -> Result<(), ChannelError> {
+		&self, our_funding_contribution: SignedAmount, their_funding_contribution: SignedAmount,
+		counterparty_funding_pubkey: PublicKey,
+	) -> Result<FundingScope, ChannelError> {
 		if their_funding_contribution > SignedAmount::MAX_MONEY {
 			return Err(ChannelError::WarnAndDisconnect(format!(
 				"Channel {} cannot be spliced in; their {} contribution exceeds the total bitcoin supply",
@@ -11015,6 +11002,11 @@ where
 			their_funding_contribution.to_sat(),
 		);
 
+		// While we check that the remote can afford the HTLCs, anchors, and the reserve
+		// after creating the new `FundingScope` below, we MUST do a basic check here to
+		// make sure that their funding contribution doesn't completely exhaust their
+		// balance here because an invariant of `FundingScope` is that `value_to_self_msat`
+		// MUST be smaller than or equal to `channel_value_satoshis` * 1000.
 		if post_channel_balance.is_none() {
 			return Err(ChannelError::WarnAndDisconnect(format!(
 				"Channel {} cannot be spliced out; their {} contribution exhausts their channel balance: {}",
@@ -11024,7 +11016,19 @@ where
 			)));
 		}
 
-		Ok(())
+		let splice_funding = FundingScope::for_splice(
+			&self.funding,
+			&self.context,
+			our_funding_contribution,
+			their_funding_contribution,
+			counterparty_funding_pubkey,
+		);
+
+		if their_funding_contribution.is_negative() {
+			self.validate_their_negative_funding_contribution(&splice_funding)?;
+		}
+
+		Ok(splice_funding)
 	}
 
 	/// See also [`validate_splice_init`]
@@ -11095,7 +11099,7 @@ where
 		})
 	}
 
-	/// Handle splice_ack
+	/// See also [`validate_splice_ack`]
 	#[cfg(splicing)]
 	pub(crate) fn splice_ack<ES: Deref, L: Deref>(
 		&mut self, msg: &msgs::SpliceAck, signer_provider: &SP, entropy_source: &ES,
@@ -11105,53 +11109,7 @@ where
 		ES::Target: EntropySource,
 		L::Target: Logger,
 	{
-		let pending_splice = if let Some(ref mut pending_splice) = &mut self.pending_splice {
-			pending_splice
-		} else {
-			return Err(ChannelError::Ignore(format!("Channel is not in pending splice")));
-		};
-
-		// TODO(splicing): Add check that we are the splice (quiescence) initiator
-
-		let funding_negotiation_context = match pending_splice.funding_negotiation.take() {
-			Some(FundingNegotiation::AwaitingAck(context)) => context,
-			Some(FundingNegotiation::ConstructingTransaction(funding, constructor)) => {
-				pending_splice.funding_negotiation =
-					Some(FundingNegotiation::ConstructingTransaction(funding, constructor));
-				return Err(ChannelError::WarnAndDisconnect(format!(
-					"Got unexpected splice_ack; splice negotiation already in progress"
-				)));
-			},
-			Some(FundingNegotiation::AwaitingSignatures(funding)) => {
-				pending_splice.funding_negotiation =
-					Some(FundingNegotiation::AwaitingSignatures(funding));
-				return Err(ChannelError::WarnAndDisconnect(format!(
-					"Got unexpected splice_ack; splice negotiation already in progress"
-				)));
-			},
-			None => {
-				return Err(ChannelError::Ignore(format!(
-					"Got unexpected splice_ack; no splice negotiation in progress"
-				)));
-			},
-		};
-
-		let our_funding_contribution = funding_negotiation_context.our_funding_contribution;
-		debug_assert!(our_funding_contribution.abs() <= SignedAmount::MAX_MONEY);
-
-		let their_funding_contribution = SignedAmount::from_sat(msg.funding_contribution_satoshis);
-		self.validate_splice_contribution(their_funding_contribution)?;
-
-		let splice_funding = FundingScope::for_splice(
-			&self.funding,
-			&self.context,
-			our_funding_contribution,
-			their_funding_contribution,
-			msg.funding_pubkey,
-		)?;
-
-		// TODO(splicing): Pre-check for reserve requirement
-		// (Note: It should also be checked later at tx_complete)
+		let splice_funding = self.validate_splice_ack(msg)?;
 
 		log_info!(
 			logger,
@@ -11161,6 +11119,17 @@ where
 			self.funding.get_value_satoshis(),
 		);
 
+		let pending_splice =
+			self.pending_splice.as_mut().expect("We should have returned an error earlier!");
+		// TODO: Good candidate for a let else statement once MSRV >= 1.65
+		let funding_negotiation_context = if let Some(FundingNegotiation::AwaitingAck(context)) =
+			pending_splice.funding_negotiation.take()
+		{
+			context
+		} else {
+			panic!("We should have returned an error earlier!");
+		};
+
 		let mut interactive_tx_constructor = funding_negotiation_context
 			.into_interactive_tx_constructor(
 				&self.context,
@@ -11179,8 +11148,6 @@ where
 
 		debug_assert!(self.interactive_tx_signing_session.is_none());
 
-		let pending_splice =
-			self.pending_splice.as_mut().expect("pending_splice should still be set");
 		pending_splice.funding_negotiation = Some(FundingNegotiation::ConstructingTransaction(
 			splice_funding,
 			interactive_tx_constructor,
@@ -11189,6 +11156,108 @@ where
 		Ok(tx_msg_opt)
 	}
 
+	/// Checks during handling splice_ack
+	#[cfg(splicing)]
+	fn validate_splice_ack(&self, msg: &msgs::SpliceAck) -> Result<FundingScope, ChannelError> {
+		// TODO(splicing): Add check that we are the splice (quiescence) initiator
+
+		let funding_negotiation_context = match &self
+			.pending_splice
+			.as_ref()
+			.ok_or(ChannelError::Ignore(format!("Channel is not in pending splice")))?
+			.funding_negotiation
+		{
+			Some(FundingNegotiation::AwaitingAck(context)) => context,
+			Some(FundingNegotiation::ConstructingTransaction(_, _))
+			| Some(FundingNegotiation::AwaitingSignatures(_)) => {
+				return Err(ChannelError::WarnAndDisconnect(format!(
+					"Got unexpected splice_ack; splice negotiation already in progress"
+				)));
+			},
+			None => {
+				return Err(ChannelError::Ignore(format!(
+					"Got unexpected splice_ack; no splice negotiation in progress"
+				)));
+			},
+		};
+
+		let our_funding_contribution = funding_negotiation_context.our_funding_contribution;
+		debug_assert!(our_funding_contribution.abs() <= SignedAmount::MAX_MONEY);
+
+		let their_funding_contribution = SignedAmount::from_sat(msg.funding_contribution_satoshis);
+		self.validate_splice_contribution(
+			our_funding_contribution,
+			their_funding_contribution,
+			msg.funding_pubkey,
+		)
+	}
+
+	/// Used to validate a negative `funding_contribution_satoshis` in `splice_init` and `splice_ack` messages.
+	#[cfg(splicing)]
+	fn validate_their_negative_funding_contribution(
+		&self, splice_funding: &FundingScope,
+	) -> Result<(), ChannelError> {
+		// We don't care about the exact value of `dust_exposure_limiting_feerate` here as
+		// we do not validate dust exposure below, but we want to avoid triggering a debug
+		// assert.
+		//
+		// TODO: clean this up here and elsewhere.
+		let dust_exposure_limiting_feerate =
+			if splice_funding.get_channel_type().supports_anchor_zero_fee_commitments() {
+				None
+			} else {
+				Some(self.context.feerate_per_kw)
+			};
+		// This *should* have no effect because no HTLC updates should be pending, but even if it does,
+		// the result may be a failed negotiation (and not a force-close), so we choose to include them.
+		let include_remote_unknown_htlcs = true;
+		let addl_nondust_htlc_count = 0;
+
+		let validate_stats = |stats: NextCommitmentStats| {
+			let (_, remote_balance_incl_fee_msat) = stats.get_balances_including_fee_msat();
+			let splice_remote_balance_msat = remote_balance_incl_fee_msat
+				.ok_or(ChannelError::Warn(format!("Remote balance does not cover the sum of HTLCs, anchors, and commitment transaction fee")))?;
+
+			// Check if the remote's new balance is under the specified reserve
+			if splice_remote_balance_msat
+				< splice_funding.holder_selected_channel_reserve_satoshis * 1000
+			{
+				return Err(ChannelError::Warn(format!(
+					"Remote balance below reserve mandated by holder: {} vs {}",
+					splice_remote_balance_msat,
+					splice_funding.holder_selected_channel_reserve_satoshis * 1000,
+				)));
+			}
+			Ok(())
+		};
+
+		// Reserve check on local commitment transaction
+
+		let splice_local_commitment_stats = self.context.get_next_local_commitment_stats(
+			splice_funding,
+			None, // htlc_candidate
+			include_remote_unknown_htlcs,
+			addl_nondust_htlc_count,
+			self.context.feerate_per_kw,
+			dust_exposure_limiting_feerate,
+		);
+
+		validate_stats(splice_local_commitment_stats)?;
+
+		// Reserve check on remote commitment transaction
+
+		let splice_remote_commitment_stats = self.context.get_next_remote_commitment_stats(
+			splice_funding,
+			None, // htlc_candidate
+			include_remote_unknown_htlcs,
+			addl_nondust_htlc_count,
+			self.context.feerate_per_kw,
+			dust_exposure_limiting_feerate,
+		);
+
+		validate_stats(splice_remote_commitment_stats)
+	}
+
 	#[cfg(splicing)]
 	pub fn splice_locked<NS: Deref, L: Deref>(
 		&mut self, msg: &msgs::SpliceLocked, node_signer: &NS, chain_hash: ChainHash,

lightning/src/sign/tx_builder.rs

@@ -35,6 +35,7 @@ impl HTLCAmountDirection {
 }
 
 pub(crate) struct NextCommitmentStats {
+	pub is_outbound_from_holder: bool,
 	pub inbound_htlcs_count: usize,
 	pub inbound_htlcs_value_msat: u64,
 	pub holder_balance_before_fee_msat: Option<u64>,
@@ -48,6 +49,26 @@ pub(crate) struct NextCommitmentStats {
 	pub extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat: Option<u64>,
 }
 
+impl NextCommitmentStats {
+	pub(crate) fn get_balances_including_fee_msat(&self) -> (Option<u64>, Option<u64>) {
+		let holder_balance_incl_fee_msat = if self.is_outbound_from_holder {
+			self.holder_balance_before_fee_msat
+				.and_then(|balance| balance.checked_sub(self.commit_tx_fee_sat * 1000))
+		} else {
+			self.holder_balance_before_fee_msat
+		};
+
+		let counterparty_balance_incl_fee_msat = if self.is_outbound_from_holder {
+			self.counterparty_balance_before_fee_msat
+		} else {
+			self.counterparty_balance_before_fee_msat
+				.and_then(|balance| balance.checked_sub(self.commit_tx_fee_sat * 1000))
+		};
+
+		(holder_balance_incl_fee_msat, counterparty_balance_incl_fee_msat)
+	}
+}
+
 #[rustfmt::skip]
 fn excess_fees_on_counterparty_tx_dust_exposure_msat(
 	next_commitment_htlcs: &[HTLCAmountDirection], dust_buffer_feerate: u32,
@@ -243,6 +264,7 @@ impl TxBuilder for SpecTxBuilder {
 			};
 
 		NextCommitmentStats {
+			is_outbound_from_holder,
 			inbound_htlcs_count,
 			inbound_htlcs_value_msat,
 			holder_balance_before_fee_msat,