Check splice contributions against SignedAmount::MAX_MONEY

Splice contributions should never exceed the total bitcoin supply. This
check prevents a potential overflow when converting the contribution
from sats to msats. The commit additionally begins to store the
contribution using SignedAmount.

Author: jkczyz

lightning/src/ln/channel.rs

@@ -8,7 +8,7 @@
 // licenses.
 
 use bitcoin::absolute::LockTime;
-use bitcoin::amount::Amount;
+use bitcoin::amount::{Amount, SignedAmount};
 use bitcoin::consensus::encode;
 use bitcoin::constants::ChainHash;
 use bitcoin::script::{Builder, Script, ScriptBuf, WScriptHash};
@@ -2254,20 +2254,22 @@ impl FundingScope {
 	/// Constructs a `FundingScope` for splicing a channel.
 	#[cfg(splicing)]
 	fn for_splice<SP: Deref>(
-		prev_funding: &Self, context: &ChannelContext<SP>, our_funding_contribution_sats: i64,
-		their_funding_contribution_sats: i64, counterparty_funding_pubkey: PublicKey,
+		prev_funding: &Self, context: &ChannelContext<SP>, our_funding_contribution: SignedAmount,
+		their_funding_contribution: SignedAmount, counterparty_funding_pubkey: PublicKey,
 	) -> Result<Self, ChannelError>
 	where
 		SP::Target: SignerProvider,
 	{
+		debug_assert!(our_funding_contribution <= SignedAmount::MAX_MONEY);
+
 		let post_channel_value = prev_funding.compute_post_splice_value(
-			our_funding_contribution_sats,
-			their_funding_contribution_sats,
+			our_funding_contribution.to_sat(),
+			their_funding_contribution.to_sat(),
 		);
 
 		let post_value_to_self_msat = AddSigned::checked_add_signed(
 			prev_funding.value_to_self_msat,
-			our_funding_contribution_sats * 1000,
+			our_funding_contribution.to_sat() * 1000,
 		);
 		debug_assert!(post_value_to_self_msat.is_some());
 		let post_value_to_self_msat = post_value_to_self_msat.unwrap();
@@ -5965,7 +5967,7 @@ pub(super) struct FundingNegotiationContext {
 	/// Whether we initiated the funding negotiation.
 	pub is_initiator: bool,
 	/// The amount in satoshis we will be contributing to the channel.
-	pub our_funding_contribution_satoshis: i64,
+	pub our_funding_contribution: SignedAmount,
 	/// The amount in satoshis our counterparty will be contributing to the channel.
 	#[allow(dead_code)] // TODO(dual_funding): Remove once contribution to V2 channels is enabled.
 	pub their_funding_contribution_satoshis: Option<i64>,
@@ -6021,7 +6023,7 @@ impl FundingNegotiationContext {
 		};
 
 		// Optionally add change output
-		if self.our_funding_contribution_satoshis > 0 {
+		if self.our_funding_contribution > SignedAmount::ZERO {
 			let change_value_opt = calculate_change_output_value(
 				&self,
 				self.shared_funding_input.is_some(),
@@ -10636,11 +10638,22 @@ where
 
 		// TODO(splicing): check for quiescence
 
-		if our_funding_contribution_satoshis < 0 {
+		let our_funding_contribution = SignedAmount::from_sat(our_funding_contribution_satoshis);
+		if our_funding_contribution > SignedAmount::MAX_MONEY {
+			return Err(APIError::APIMisuseError {
+				err: format!(
+					"Channel {} cannot be spliced; contribution exceeds total bitcoin supply: {}",
+					self.context.channel_id(),
+					our_funding_contribution,
+				),
+			});
+		}
+
+		if our_funding_contribution < SignedAmount::ZERO {
 			return Err(APIError::APIMisuseError {
 				err: format!(
 				"TODO(splicing): Splice-out not supported, only splice in; channel ID {}, contribution {}",
-				self.context.channel_id(), our_funding_contribution_satoshis,
+				self.context.channel_id(), our_funding_contribution,
 			),
 			});
 		}
@@ -10653,7 +10666,7 @@ where
 
 		// Check that inputs are sufficient to cover our contribution.
 		let _fee = check_v2_funding_inputs_sufficient(
-			our_funding_contribution_satoshis,
+			our_funding_contribution.to_sat(),
 			&our_funding_inputs,
 			true,
 			true,
@@ -10677,7 +10690,7 @@ where
 		let prev_funding_input = self.funding.to_splice_funding_input();
 		let funding_negotiation_context = FundingNegotiationContext {
 			is_initiator: true,
-			our_funding_contribution_satoshis,
+			our_funding_contribution,
 			their_funding_contribution_satoshis: None,
 			funding_tx_locktime: LockTime::from_consensus(locktime),
 			funding_feerate_sat_per_1000_weight: funding_feerate_per_kw,
@@ -10698,7 +10711,7 @@ where
 
 		Ok(msgs::SpliceInit {
 			channel_id: self.context.channel_id,
-			funding_contribution_satoshis: our_funding_contribution_satoshis,
+			funding_contribution_satoshis: our_funding_contribution.to_sat(),
 			funding_feerate_per_kw,
 			locktime,
 			funding_pubkey,
@@ -10709,10 +10722,8 @@ where
 	/// Checks during handling splice_init
 	#[cfg(splicing)]
 	pub fn validate_splice_init(
-		&self, msg: &msgs::SpliceInit, our_funding_contribution_satoshis: i64,
+		&self, msg: &msgs::SpliceInit, our_funding_contribution: SignedAmount,
 	) -> Result<FundingScope, ChannelError> {
-		let their_funding_contribution_satoshis = msg.funding_contribution_satoshis;
-
 		// TODO(splicing): Add check that we are the quiescence acceptor
 
 		// Check if a splice has been initiated already.
@@ -10732,21 +10743,38 @@ where
 			)));
 		}
 
-		if their_funding_contribution_satoshis.saturating_add(our_funding_contribution_satoshis) < 0
-		{
+		if our_funding_contribution > SignedAmount::MAX_MONEY {
+			return Err(ChannelError::WarnAndDisconnect(format!(
+				"Channel {} cannot be spliced; our contribution exceeds total bitcoin supply: {}",
+				self.context.channel_id(),
+				our_funding_contribution,
+			)));
+		}
+
+		let their_funding_contribution = SignedAmount::from_sat(msg.funding_contribution_satoshis);
+		if their_funding_contribution > SignedAmount::MAX_MONEY {
+			return Err(ChannelError::WarnAndDisconnect(format!(
+				"Channel {} cannot be spliced; their contribution exceeds total bitcoin supply: {}",
+				self.context.channel_id(),
+				their_funding_contribution,
+			)));
+		}
+
+		debug_assert_eq!(our_funding_contribution, SignedAmount::ZERO);
+		if their_funding_contribution < SignedAmount::ZERO {
 			return Err(ChannelError::WarnAndDisconnect(format!(
 				"Splice-out not supported, only splice in, contribution is {} ({} + {})",
-				their_funding_contribution_satoshis + our_funding_contribution_satoshis,
-				their_funding_contribution_satoshis,
-				our_funding_contribution_satoshis,
+				their_funding_contribution + our_funding_contribution,
+				their_funding_contribution,
+				our_funding_contribution,
 			)));
 		}
 
 		let splice_funding = FundingScope::for_splice(
 			&self.funding,
 			&self.context,
-			our_funding_contribution_satoshis,
-			their_funding_contribution_satoshis,
+			our_funding_contribution,
+			their_funding_contribution,
 			msg.funding_pubkey,
 		)?;
 
@@ -10771,7 +10799,8 @@ where
 		ES::Target: EntropySource,
 		L::Target: Logger,
 	{
-		let splice_funding = self.validate_splice_init(msg, our_funding_contribution_satoshis)?;
+		let our_funding_contribution = SignedAmount::from_sat(our_funding_contribution_satoshis);
+		let splice_funding = self.validate_splice_init(msg, our_funding_contribution)?;
 
 		log_info!(
 			logger,
@@ -10785,7 +10814,7 @@ where
 		let prev_funding_input = self.funding.to_splice_funding_input();
 		let funding_negotiation_context = FundingNegotiationContext {
 			is_initiator: false,
-			our_funding_contribution_satoshis,
+			our_funding_contribution,
 			their_funding_contribution_satoshis: Some(their_funding_contribution_satoshis),
 			funding_tx_locktime: LockTime::from_consensus(msg.locktime),
 			funding_feerate_sat_per_1000_weight: msg.funding_feerate_per_kw,
@@ -10823,7 +10852,7 @@ where
 
 		Ok(msgs::SpliceAck {
 			channel_id: self.context.channel_id,
-			funding_contribution_satoshis: our_funding_contribution_satoshis,
+			funding_contribution_satoshis: our_funding_contribution.to_sat(),
 			funding_pubkey,
 			require_confirmed_inputs: None,
 		})
@@ -10870,15 +10899,23 @@ where
 			},
 		};
 
-		let our_funding_contribution_satoshis =
-			funding_negotiation_context.our_funding_contribution_satoshis;
-		let their_funding_contribution_satoshis = msg.funding_contribution_satoshis;
+		let our_funding_contribution = funding_negotiation_context.our_funding_contribution;
+		debug_assert!(our_funding_contribution <= SignedAmount::MAX_MONEY);
+
+		let their_funding_contribution = SignedAmount::from_sat(msg.funding_contribution_satoshis);
+		if their_funding_contribution > SignedAmount::MAX_MONEY {
+			return Err(ChannelError::Warn(format!(
+				"Channel {} cannot be spliced; their contribution exceeds total bitcoin supply: {}",
+				self.context.channel_id(),
+				their_funding_contribution,
+			)));
+		}
 
 		let splice_funding = FundingScope::for_splice(
 			&self.funding,
 			&self.context,
-			our_funding_contribution_satoshis,
-			their_funding_contribution_satoshis,
+			our_funding_contribution,
+			their_funding_contribution,
 			msg.funding_pubkey,
 		)?;
 
@@ -12476,7 +12513,7 @@ where
 		};
 		let funding_negotiation_context = FundingNegotiationContext {
 			is_initiator: true,
-			our_funding_contribution_satoshis: funding_satoshis as i64,
+			our_funding_contribution: SignedAmount::from_sat(funding_satoshis as i64),
 			// TODO(dual_funding) TODO(splicing) Include counterparty contribution, once that's enabled
 			their_funding_contribution_satoshis: None,
 			funding_tx_locktime,
@@ -12586,10 +12623,11 @@ where
 			  L::Target: Logger,
 	{
 		// TODO(dual_funding): Take these as input once supported
-		let our_funding_satoshis = 0u64;
+		let (our_funding_contribution, our_funding_contribution_sats) = (SignedAmount::ZERO, 0u64);
 		let our_funding_inputs = Vec::new();
 
-		let channel_value_satoshis = our_funding_satoshis.saturating_add(msg.common_fields.funding_satoshis);
+		let channel_value_satoshis =
+			our_funding_contribution_sats.saturating_add(msg.common_fields.funding_satoshis);
 		let counterparty_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
 			channel_value_satoshis, msg.common_fields.dust_limit_satoshis);
 		let holder_selected_channel_reserve_satoshis = get_v2_channel_reserve_satoshis(
@@ -12616,9 +12654,7 @@ where
 			current_chain_height,
 			logger,
 			false,
-
-			our_funding_satoshis,
-
+			our_funding_contribution_sats,
 			counterparty_pubkeys,
 			channel_type,
 			holder_selected_channel_reserve_satoshis,
@@ -12633,7 +12669,7 @@ where
 
 		let funding_negotiation_context = FundingNegotiationContext {
 			is_initiator: false,
-			our_funding_contribution_satoshis: our_funding_satoshis as i64,
+			our_funding_contribution,
 			their_funding_contribution_satoshis: Some(msg.common_fields.funding_satoshis as i64),
 			funding_tx_locktime: LockTime::from_consensus(msg.locktime),
 			funding_feerate_sat_per_1000_weight: msg.funding_feerate_sat_per_1000_weight,
@@ -12657,7 +12693,7 @@ where
 				is_initiator: false,
 				inputs_to_contribute: our_funding_inputs,
 				shared_funding_input: None,
-				shared_funding_output: SharedOwnedOutput::new(shared_funding_output, our_funding_satoshis),
+				shared_funding_output: SharedOwnedOutput::new(shared_funding_output, our_funding_contribution_sats),
 				outputs_to_contribute: Vec::new(),
 			}
 		).map_err(|err| {
@@ -12738,7 +12774,7 @@ where
 				}),
 				channel_type: Some(self.funding.get_channel_type().clone()),
 			},
-			funding_satoshis: self.funding_negotiation_context.our_funding_contribution_satoshis
+			funding_satoshis: self.funding_negotiation_context.our_funding_contribution.to_sat()
 				as u64,
 			second_per_commitment_point,
 			require_confirmed_inputs: None,

lightning/src/ln/channelmanager.rs

@@ -30,7 +30,7 @@ use bitcoin::hashes::{Hash, HashEngine, HmacEngine};
 
 use bitcoin::secp256k1::Secp256k1;
 use bitcoin::secp256k1::{PublicKey, SecretKey};
-use bitcoin::{secp256k1, Sequence};
+use bitcoin::{secp256k1, Sequence, SignedAmount};
 #[cfg(splicing)]
 use bitcoin::{ScriptBuf, TxIn, Weight};
 
@@ -9198,7 +9198,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 
 					// Inbound V2 channels with contributed inputs are not considered unfunded.
 					if let Some(unfunded_chan) = chan.as_unfunded_v2() {
-						if unfunded_chan.funding_negotiation_context.our_funding_contribution_satoshis > 0 {
+						if unfunded_chan.funding_negotiation_context.our_funding_contribution > SignedAmount::ZERO {
 							continue;
 						}
 					}

lightning/src/ln/interactivetxs.rs

@@ -11,7 +11,7 @@ use crate::io_extras::sink;
 use crate::prelude::*;
 
 use bitcoin::absolute::LockTime as AbsoluteLockTime;
-use bitcoin::amount::Amount;
+use bitcoin::amount::{Amount, SignedAmount};
 use bitcoin::consensus::Encodable;
 use bitcoin::constants::WITNESS_SCALE_FACTOR;
 use bitcoin::policy::MAX_STANDARD_TX_WEIGHT;
@@ -1886,8 +1886,8 @@ pub(super) fn calculate_change_output_value(
 	context: &FundingNegotiationContext, is_splice: bool, shared_output_funding_script: &ScriptBuf,
 	funding_outputs: &Vec<TxOut>, change_output_dust_limit: u64,
 ) -> Result<Option<u64>, AbortReason> {
-	assert!(context.our_funding_contribution_satoshis > 0);
-	let our_funding_contribution_satoshis = context.our_funding_contribution_satoshis as u64;
+	assert!(context.our_funding_contribution > SignedAmount::ZERO);
+	let our_funding_contribution_satoshis = context.our_funding_contribution.to_sat() as u64;
 
 	let mut total_input_satoshis = 0u64;
 	let mut our_funding_inputs_weight = 0u64;
@@ -1964,7 +1964,8 @@ mod tests {
 	use bitcoin::secp256k1::{Keypair, PublicKey, Secp256k1, SecretKey};
 	use bitcoin::transaction::Version;
 	use bitcoin::{
-		OutPoint, PubkeyHash, ScriptBuf, Sequence, Transaction, TxIn, TxOut, WPubkeyHash, Witness,
+		OutPoint, PubkeyHash, ScriptBuf, Sequence, SignedAmount, Transaction, TxIn, TxOut,
+		WPubkeyHash, Witness,
 	};
 	use core::ops::Deref;
 
@@ -2993,7 +2994,7 @@ mod tests {
 		// There is leftover for change
 		let context = FundingNegotiationContext {
 			is_initiator: true,
-			our_funding_contribution_satoshis: our_contributed as i64,
+			our_funding_contribution: SignedAmount::from_sat(our_contributed as i64),
 			their_funding_contribution_satoshis: None,
 			funding_tx_locktime: AbsoluteLockTime::ZERO,
 			funding_feerate_sat_per_1000_weight,
@@ -3016,7 +3017,7 @@ mod tests {
 		// Insufficient inputs, no leftover
 		let context = FundingNegotiationContext {
 			is_initiator: false,
-			our_funding_contribution_satoshis: 130_000,
+			our_funding_contribution: SignedAmount::from_sat(130_000),
 			..context
 		};
 		assert_eq!(
@@ -3027,7 +3028,7 @@ mod tests {
 		// Very small leftover
 		let context = FundingNegotiationContext {
 			is_initiator: false,
-			our_funding_contribution_satoshis: 118_000,
+			our_funding_contribution: SignedAmount::from_sat(118_000),
 			..context
 		};
 		assert_eq!(
@@ -3038,7 +3039,7 @@ mod tests {
 		// Small leftover, but not dust
 		let context = FundingNegotiationContext {
 			is_initiator: false,
-			our_funding_contribution_satoshis: 117_992,
+			our_funding_contribution: SignedAmount::from_sat(117_992),
 			..context
 		};
 		assert_eq!(
@@ -3049,7 +3050,7 @@ mod tests {
 		// Larger fee, smaller change
 		let context = FundingNegotiationContext {
 			is_initiator: true,
-			our_funding_contribution_satoshis: our_contributed as i64,
+			our_funding_contribution: SignedAmount::from_sat(our_contributed as i64),
 			funding_feerate_sat_per_1000_weight: funding_feerate_sat_per_1000_weight * 3,
 			..context
 		};