From 09926fee1d2f7fb74fa7e8a44d86757cc2f8005c Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 13 Aug 2025 20:19:49 +0000 Subject: [PATCH 1/7] Finish validation in `splice_ack` before taking a `&mut self` As much as possible, we want to only mutate state once we are done with input validation. This also removes complaints when helper functions during validation take a `&self`. --- lightning/src/ln/channel.rs | 38 ++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 079201e748b..ba55039d30c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -11250,26 +11250,17 @@ 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() { + 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(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)); + Some(FundingNegotiation::ConstructingTransaction(_, _)) + | Some(FundingNegotiation::AwaitingSignatures(_)) => { return Err(ChannelError::WarnAndDisconnect(format!( "Got unexpected splice_ack; splice negotiation already in progress" ))); @@ -11306,6 +11297,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, @@ -11324,8 +11326,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, From 6fd902ab00a039b5437e26b724e84b4827dddcfe Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Tue, 26 Aug 2025 18:40:04 +0000 Subject: [PATCH 2/7] Add `validate_splice_ack` helper function As in `splice_init`, this helps clearly delineate `splice_ack` message validation from the subsequent state mutations. This is a code-move. --- lightning/src/ln/channel.rs | 86 ++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 40 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index ba55039d30c..de87339e702 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -11172,7 +11172,6 @@ where Ok(()) } - /// See also [`validate_splice_init`] #[cfg(splicing)] pub(crate) fn splice_init( &mut self, msg: &msgs::SpliceInit, our_funding_contribution_satoshis: i64, @@ -11240,7 +11239,6 @@ where }) } - /// Handle splice_ack #[cfg(splicing)] pub(crate) fn splice_ack( &mut self, msg: &msgs::SpliceAck, signer_provider: &SP, entropy_source: &ES, @@ -11250,44 +11248,7 @@ where ES::Target: EntropySource, L::Target: Logger, { - // 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(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, @@ -11334,6 +11295,51 @@ where Ok(tx_msg_opt) } + /// Checks during handling splice_ack + #[cfg(splicing)] + fn validate_splice_ack(&self, msg: &msgs::SpliceAck) -> Result { + // 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(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) + + Ok(splice_funding) + } + #[cfg(splicing)] pub fn splice_locked( &mut self, msg: &msgs::SpliceLocked, node_signer: &NS, chain_hash: ChainHash, From e55084cf69398471eaeec20ff509c3b0dea7a5ce Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Tue, 26 Aug 2025 16:49:19 +0000 Subject: [PATCH 3/7] Make `for_splice` infallible --- lightning/src/ln/channel.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index de87339e702..e08a825cf80 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2248,7 +2248,7 @@ impl FundingScope { fn for_splice( prev_funding: &Self, context: &ChannelContext, our_funding_contribution: SignedAmount, their_funding_contribution: SignedAmount, counterparty_funding_pubkey: PublicKey, - ) -> Result + ) -> 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. @@ -11120,7 +11120,7 @@ where 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`. @@ -11332,7 +11332,7 @@ where 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) From 0aa19d9b1f0568304988b8bd74ed170548987f23 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Tue, 26 Aug 2025 17:23:26 +0000 Subject: [PATCH 4/7] Let `validate_splice_contribution` construct the new `FundingScope` We will validate the reserve requirements on the new `FundingScope` in `validate_splice_contribution`. --- lightning/src/ln/channel.rs | 60 +++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e08a825cf80..ef2fb06634c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -11089,6 +11089,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 @@ -11110,33 +11112,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 { if their_funding_contribution > SignedAmount::MAX_MONEY { return Err(ChannelError::WarnAndDisconnect(format!( "Channel {} cannot be spliced in; their {} contribution exceeds the total bitcoin supply", @@ -11169,7 +11156,25 @@ where ))); } - Ok(()) + let splice_funding = FundingScope::for_splice( + &self.funding, + &self.context, + our_funding_contribution, + their_funding_contribution, + counterparty_funding_pubkey, + ); + + // TODO(splicing): Check that channel balance does not go below the channel reserve + + // 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 + + // TODO(splicing): Pre-check for reserve requirement + // (Note: It should also be checked later at tx_complete) + + Ok(splice_funding) } #[cfg(splicing)] @@ -11324,20 +11329,11 @@ where 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, + self.validate_splice_contribution( 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) - - Ok(splice_funding) + ) } #[cfg(splicing)] From 7cb581e190803683d49461a1312df030c1a62872 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Tue, 26 Aug 2025 21:24:11 +0000 Subject: [PATCH 5/7] Add `NextCommitmentStats::get_balances_including_fee` `NextCommitmentStats` provides the commitment transaction fee as a separate value to assist with applying a multiplier on it in `can_accept_incoming_htlc`. Nonetheless in most cases, we want the balances to include the commitment transaction fee, so here we add a helper that gives us these balances. --- lightning/src/sign/tx_builder.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index f9c871d59b5..62f2e995807 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/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, @@ -48,6 +49,26 @@ pub(crate) struct NextCommitmentStats { pub extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat: Option, } +impl NextCommitmentStats { + pub(crate) fn get_balances_including_fee_msat(&self) -> (Option, Option) { + if self.is_outbound_from_holder { + ( + self.holder_balance_before_fee_msat.and_then(|balance_msat| { + balance_msat.checked_sub(self.commit_tx_fee_sat * 1000) + }), + self.counterparty_balance_before_fee_msat, + ) + } else { + ( + self.holder_balance_before_fee_msat, + self.counterparty_balance_before_fee_msat.and_then(|balance_msat| { + balance_msat.checked_sub(self.commit_tx_fee_sat * 1000) + }), + ) + } + } +} + fn excess_fees_on_counterparty_tx_dust_exposure_msat( next_commitment_htlcs: &[HTLCAmountDirection], dust_buffer_feerate: u32, excess_feerate: u32, counterparty_dust_limit_satoshis: u64, dust_htlc_exposure_msat: u64, @@ -280,6 +301,7 @@ impl TxBuilder for SpecTxBuilder { }; NextCommitmentStats { + is_outbound_from_holder, inbound_htlcs_count, inbound_htlcs_value_msat, holder_balance_before_fee_msat, From 79c26452fbc6c60f72c172711e4c68cc4a56a6f0 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 27 Aug 2025 02:04:00 +0000 Subject: [PATCH 6/7] Cleanup the style of `tx_builder::subtract_addl_outputs` Makes it consistent with `get_balances_including_fee`, itself added in the previous commit. --- lightning/src/sign/tx_builder.rs | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs index 62f2e995807..ed3f4746cab 100644 --- a/lightning/src/sign/tx_builder.rs +++ b/lightning/src/sign/tx_builder.rs @@ -147,21 +147,19 @@ fn subtract_addl_outputs( // commitment transaction *before* checking whether the remote party's balance is enough to // cover the total anchor sum. - let local_balance_before_fee_msat = if is_outbound_from_holder { - value_to_self_after_htlcs_msat - .and_then(|balance_msat| balance_msat.checked_sub(total_anchors_sat * 1000)) - } else { - value_to_self_after_htlcs_msat - }; - - let remote_balance_before_fee_msat = if !is_outbound_from_holder { - value_to_remote_after_htlcs_msat - .and_then(|balance_msat| balance_msat.checked_sub(total_anchors_sat * 1000)) + if is_outbound_from_holder { + ( + value_to_self_after_htlcs_msat + .and_then(|balance_msat| balance_msat.checked_sub(total_anchors_sat * 1000)), + value_to_remote_after_htlcs_msat, + ) } else { - value_to_remote_after_htlcs_msat - }; - - (local_balance_before_fee_msat, remote_balance_before_fee_msat) + ( + value_to_self_after_htlcs_msat, + value_to_remote_after_htlcs_msat + .and_then(|balance_msat| balance_msat.checked_sub(total_anchors_sat * 1000)), + ) + } } fn get_dust_buffer_feerate(feerate_per_kw: u32) -> u32 { From 7a567578e5b9a7664a3f27e141fcd21bc164ac03 Mon Sep 17 00:00:00 2001 From: Leo Nash Date: Wed, 27 Aug 2025 02:23:48 +0000 Subject: [PATCH 7/7] Check reserves after `funding_contribution_satoshis` is applied This applies to both `splice_init` and `splice_ack` messages. From BOLT 2: ``` - If `funding_contribution_satoshis` is negative and its absolute value is greater than the sending node's current channel balance: - MUST send a `warning` and close the connection or send an `error` and fail the channel. ``` Further down also: ``` If a side does not meet the reserve requirements, that's OK: but if they take funds out of the channel, they must ensure that they do meet them. If your peer adds a massive amount to the channel, then you only have to add more reserve if you want to contribute to the splice (and you can use `tx_remove_output` and/or `tx_remove_input` part-way through if this happens). ``` Therefore, we run the reserve check anytime the contribution is not equal to zero. --- lightning/src/ln/channel.rs | 85 +++++++++++++++++++++++++++++++++---- 1 file changed, 76 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index ef2fb06634c..e03bc833c7e 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -11147,6 +11147,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 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: {}", @@ -11164,15 +11169,9 @@ where counterparty_funding_pubkey, ); - // TODO(splicing): Check that channel balance does not go below the channel reserve - - // 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 - - // TODO(splicing): Pre-check for reserve requirement - // (Note: It should also be checked later at tx_complete) + if their_funding_contribution != SignedAmount::ZERO { + self.validate_their_funding_contribution_reserve(&splice_funding)?; + } Ok(splice_funding) } @@ -11336,6 +11335,74 @@ where ) } + /// Used to validate a negative `funding_contribution_satoshis` in `splice_init` and `splice_ack` messages. + #[cfg(splicing)] + fn validate_their_funding_contribution_reserve( + &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; + // Make sure that that the funder of the channel can pay the transaction fees for an additional + // nondust HTLC on the channel. + let addl_nondust_htlc_count = 1; + + 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::WarnAndDisconnect(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::WarnAndDisconnect(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( &mut self, msg: &msgs::SpliceLocked, node_signer: &NS, chain_hash: ChainHash,