From cbe84210d771e62b51a67e3e6d355eba952f0e9e Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 28 Feb 2025 14:34:28 -0600 Subject: [PATCH 1/4] Split commitment_signed handling by check-accept When handling commitment_signed messages, a number of checks are performed before a ChannelMonitorUpdate is created and returned. Once splicing is added, these checks need to be performed on the primary FundingScope and any pending scopes that resulted from splicing or RBF. This commit splits the handling into a check and accept methods, taking &self and &mut self, respectively. This ensures that the ChannelContext is not modified between checks. Once all funding scopes have been checked successfully, the accept portion of the code can then execute. --- lightning/src/chain/channelmonitor.rs | 1 + lightning/src/ln/channel.rs | 55 +++++++++++++++++++-------- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 040ac6b3f2f..a78c8edaf97 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -531,6 +531,7 @@ impl_writeable_tlv_based_enum_upgradable!(OnchainEvent, #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum ChannelMonitorUpdateStep { + // Update LatestHolderCommitmentTXInfo in channel.rs if adding new fields to this variant. LatestHolderCommitmentTXInfo { commitment_tx: HolderCommitmentTransaction, /// Note that LDK after 0.0.115 supports this only containing dust HTLCs (implying the diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e08a0731513..e09eca0c602 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4707,6 +4707,14 @@ struct CommitmentTxInfoCached { feerate: u32, } +/// Partial data from ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo used to simplify the +/// return type of `FundedChannel::validate_commitment_signed`. +struct LatestHolderCommitmentTXInfo { + pub commitment_tx: HolderCommitmentTransaction, + pub htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>, + pub nondust_htlc_sources: Vec, +} + /// Contents of a wire message that fails an HTLC backwards. Useful for [`FundedChannel::fail_htlc`] to /// fail with either [`msgs::UpdateFailMalformedHTLC`] or [`msgs::UpdateFailHTLC`] as needed. trait FailHTLCContents { @@ -5495,22 +5503,9 @@ impl FundedChannel where Ok(channel_monitor) } - pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned, logger: &L) -> Result, ChannelError> + fn validate_commitment_signed(&self, msg: &msgs::CommitmentSigned, logger: &L) -> Result where L::Target: Logger { - if self.context.channel_state.is_quiescent() { - return Err(ChannelError::WarnAndDisconnect("Got commitment_signed message while quiescent".to_owned())); - } - if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { - return Err(ChannelError::close("Got commitment signed message when channel was not in an operational state".to_owned())); - } - if self.context.channel_state.is_peer_disconnected() { - return Err(ChannelError::close("Peer sent commitment_signed when we needed a channel_reestablish".to_owned())); - } - if self.context.channel_state.is_both_sides_shutdown() && self.context.last_sent_closing_fee.is_some() { - return Err(ChannelError::close("Peer sent commitment_signed after we'd started exchanging closing_signeds".to_owned())); - } - let funding_script = self.funding.get_funding_redeemscript(); let keys = self.context.build_holder_transaction_keys(&self.funding, self.holder_commitment_point.current_point()); @@ -5623,6 +5618,31 @@ impl FundedChannel where self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages) .map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?; + Ok(LatestHolderCommitmentTXInfo { + commitment_tx: holder_commitment_tx, + htlc_outputs: htlcs_and_sigs, + nondust_htlc_sources, + }) + } + + pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned, logger: &L) -> Result, ChannelError> + where L::Target: Logger + { + if self.context.channel_state.is_quiescent() { + return Err(ChannelError::WarnAndDisconnect("Got commitment_signed message while quiescent".to_owned())); + } + if !matches!(self.context.channel_state, ChannelState::ChannelReady(_)) { + return Err(ChannelError::close("Got commitment signed message when channel was not in an operational state".to_owned())); + } + if self.context.channel_state.is_peer_disconnected() { + return Err(ChannelError::close("Peer sent commitment_signed when we needed a channel_reestablish".to_owned())); + } + if self.context.channel_state.is_both_sides_shutdown() && self.context.last_sent_closing_fee.is_some() { + return Err(ChannelError::close("Peer sent commitment_signed after we'd started exchanging closing_signeds".to_owned())); + } + + let commitment_tx_info = self.validate_commitment_signed(msg, logger)?; + // Update state now that we've passed all the can-fail calls... let mut need_commitment = false; if let &mut Some((_, ref mut update_state)) = &mut self.context.pending_update_fee { @@ -5662,13 +5682,16 @@ impl FundedChannel where } } + let LatestHolderCommitmentTXInfo { + commitment_tx, htlc_outputs, nondust_htlc_sources, + } = commitment_tx_info; self.context.latest_monitor_update_id += 1; let mut monitor_update = ChannelMonitorUpdate { update_id: self.context.latest_monitor_update_id, counterparty_node_id: Some(self.context.counterparty_node_id), updates: vec![ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo { - commitment_tx: holder_commitment_tx, - htlc_outputs: htlcs_and_sigs, + commitment_tx, + htlc_outputs, claimed_htlcs, nondust_htlc_sources, }], From abdb5c993f79ba29e8b03621e0ac0d900597f2fa Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 28 Feb 2025 15:14:23 -0600 Subject: [PATCH 2/4] Move commitment_signed_check to ChannelContext Now that commitment_signed is split into check and accept methods, move the check portion from FundedChannel to ChannelContext. This allows calling it with a different FundingScope when there are pending splices and RBF attempts. --- lightning/src/ln/channel.rs | 252 ++++++++++++++++++------------------ 1 file changed, 128 insertions(+), 124 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e09eca0c602..dc48018f76f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3453,6 +3453,132 @@ impl ChannelContext where SP::Target: SignerProvider { !matches!(self.channel_state, ChannelState::AwaitingChannelReady(flags) if flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH)) } + fn validate_commitment_signed( + &self, funding: &FundingScope, holder_commitment_point: &HolderCommitmentPoint, + msg: &msgs::CommitmentSigned, logger: &L, + ) -> Result + where + L::Target: Logger, + { + let funding_script = funding.get_funding_redeemscript(); + + let keys = self.build_holder_transaction_keys(funding, holder_commitment_point.current_point()); + + let commitment_stats = self.build_commitment_transaction(funding, holder_commitment_point.transaction_number(), &keys, true, false, logger); + let commitment_txid = { + let trusted_tx = commitment_stats.tx.trust(); + let bitcoin_tx = trusted_tx.built_transaction(); + let sighash = bitcoin_tx.get_sighash_all(&funding_script, funding.get_value_satoshis()); + + log_trace!(logger, "Checking commitment tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}", + log_bytes!(msg.signature.serialize_compact()[..]), + log_bytes!(funding.counterparty_funding_pubkey().serialize()), encode::serialize_hex(&bitcoin_tx.transaction), + log_bytes!(sighash[..]), encode::serialize_hex(&funding_script), &self.channel_id()); + if let Err(_) = self.secp_ctx.verify_ecdsa(&sighash, &msg.signature, &funding.counterparty_funding_pubkey()) { + return Err(ChannelError::close("Invalid commitment tx signature from peer".to_owned())); + } + bitcoin_tx.txid + }; + let mut htlcs_cloned: Vec<_> = commitment_stats.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect(); + + // If our counterparty updated the channel fee in this commitment transaction, check that + // they can actually afford the new fee now. + let update_fee = if let Some((_, update_state)) = self.pending_update_fee { + update_state == FeeUpdateState::RemoteAnnounced + } else { false }; + if update_fee { + debug_assert!(!funding.is_outbound()); + let counterparty_reserve_we_require_msat = funding.holder_selected_channel_reserve_satoshis * 1000; + if commitment_stats.remote_balance_msat < commitment_stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat { + return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned())); + } + } + #[cfg(any(test, fuzzing))] + { + if funding.is_outbound() { + let projected_commit_tx_info = funding.next_local_commitment_tx_fee_info_cached.lock().unwrap().take(); + *funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None; + if let Some(info) = projected_commit_tx_info { + let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len() + + self.holding_cell_htlc_updates.len(); + if info.total_pending_htlcs == total_pending_htlcs + && info.next_holder_htlc_id == self.next_holder_htlc_id + && info.next_counterparty_htlc_id == self.next_counterparty_htlc_id + && info.feerate == self.feerate_per_kw { + assert_eq!(commitment_stats.total_fee_sat, info.fee / 1000); + } + } + } + } + + if msg.htlc_signatures.len() != commitment_stats.num_nondust_htlcs { + return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs))); + } + + // Up to LDK 0.0.115, HTLC information was required to be duplicated in the + // `htlcs_and_sigs` vec and in the `holder_commitment_tx` itself, both of which were passed + // in the `ChannelMonitorUpdate`. In 0.0.115, support for having a separate set of + // outbound-non-dust-HTLCSources in the `ChannelMonitorUpdate` was added, however for + // backwards compatibility, we never use it in production. To provide test coverage, here, + // we randomly decide (in test/fuzzing builds) to use the new vec sometimes. + #[allow(unused_assignments, unused_mut)] + let mut separate_nondust_htlc_sources = false; + #[cfg(all(feature = "std", any(test, fuzzing)))] { + use core::hash::{BuildHasher, Hasher}; + // Get a random value using the only std API to do so - the DefaultHasher + let rand_val = std::collections::hash_map::RandomState::new().build_hasher().finish(); + separate_nondust_htlc_sources = rand_val % 2 == 0; + } + + let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len()); + let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len()); + for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() { + if let Some(_) = htlc.transaction_output_index { + let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.feerate_per_kw, + funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, &self.channel_type, + &keys.broadcaster_delayed_payment_key, &keys.revocation_key); + + let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &self.channel_type, &keys); + let htlc_sighashtype = if self.channel_type.supports_anchors_zero_fee_htlc_tx() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All }; + let htlc_sighash = hash_to_message!(&sighash::SighashCache::new(&htlc_tx).p2wsh_signature_hash(0, &htlc_redeemscript, htlc.to_bitcoin_amount(), htlc_sighashtype).unwrap()[..]); + log_trace!(logger, "Checking HTLC tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}.", + log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(keys.countersignatory_htlc_key.to_public_key().serialize()), + encode::serialize_hex(&htlc_tx), log_bytes!(htlc_sighash[..]), encode::serialize_hex(&htlc_redeemscript), &self.channel_id()); + if let Err(_) = self.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &keys.countersignatory_htlc_key.to_public_key()) { + return Err(ChannelError::close("Invalid HTLC tx signature from peer".to_owned())); + } + if !separate_nondust_htlc_sources { + htlcs_and_sigs.push((htlc, Some(msg.htlc_signatures[idx]), source_opt.take())); + } + } else { + htlcs_and_sigs.push((htlc, None, source_opt.take())); + } + if separate_nondust_htlc_sources { + if let Some(source) = source_opt.take() { + nondust_htlc_sources.push(source); + } + } + debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere"); + } + + let holder_commitment_tx = HolderCommitmentTransaction::new( + commitment_stats.tx, + msg.signature, + msg.htlc_signatures.clone(), + &funding.get_holder_pubkeys().funding_pubkey, + funding.counterparty_funding_pubkey() + ); + + self.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages) + .map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?; + + Ok(LatestHolderCommitmentTXInfo { + commitment_tx: holder_commitment_tx, + htlc_outputs: htlcs_and_sigs, + nondust_htlc_sources, + }) + } + /// Transaction nomenclature is somewhat confusing here as there are many different cases - a /// transaction is referred to as "a's transaction" implying that a will be able to broadcast /// the transaction. Thus, b will generally be sending a signature over such a transaction to @@ -4708,7 +4834,7 @@ struct CommitmentTxInfoCached { } /// Partial data from ChannelMonitorUpdateStep::LatestHolderCommitmentTXInfo used to simplify the -/// return type of `FundedChannel::validate_commitment_signed`. +/// return type of `ChannelContext::validate_commitment_signed`. struct LatestHolderCommitmentTXInfo { pub commitment_tx: HolderCommitmentTransaction, pub htlc_outputs: Vec<(HTLCOutputInCommitment, Option, Option)>, @@ -5503,128 +5629,6 @@ impl FundedChannel where Ok(channel_monitor) } - fn validate_commitment_signed(&self, msg: &msgs::CommitmentSigned, logger: &L) -> Result - where L::Target: Logger - { - let funding_script = self.funding.get_funding_redeemscript(); - - let keys = self.context.build_holder_transaction_keys(&self.funding, self.holder_commitment_point.current_point()); - - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &keys, true, false, logger); - let commitment_txid = { - let trusted_tx = commitment_stats.tx.trust(); - let bitcoin_tx = trusted_tx.built_transaction(); - let sighash = bitcoin_tx.get_sighash_all(&funding_script, self.funding.get_value_satoshis()); - - log_trace!(logger, "Checking commitment tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}", - log_bytes!(msg.signature.serialize_compact()[..]), - log_bytes!(self.funding.counterparty_funding_pubkey().serialize()), encode::serialize_hex(&bitcoin_tx.transaction), - log_bytes!(sighash[..]), encode::serialize_hex(&funding_script), &self.context.channel_id()); - if let Err(_) = self.context.secp_ctx.verify_ecdsa(&sighash, &msg.signature, &self.funding.counterparty_funding_pubkey()) { - return Err(ChannelError::close("Invalid commitment tx signature from peer".to_owned())); - } - bitcoin_tx.txid - }; - let mut htlcs_cloned: Vec<_> = commitment_stats.htlcs_included.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect(); - - // If our counterparty updated the channel fee in this commitment transaction, check that - // they can actually afford the new fee now. - let update_fee = if let Some((_, update_state)) = self.context.pending_update_fee { - update_state == FeeUpdateState::RemoteAnnounced - } else { false }; - if update_fee { - debug_assert!(!self.funding.is_outbound()); - let counterparty_reserve_we_require_msat = self.funding.holder_selected_channel_reserve_satoshis * 1000; - if commitment_stats.remote_balance_msat < commitment_stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat { - return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned())); - } - } - #[cfg(any(test, fuzzing))] - { - if self.funding.is_outbound() { - let projected_commit_tx_info = self.funding.next_local_commitment_tx_fee_info_cached.lock().unwrap().take(); - *self.funding.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None; - if let Some(info) = projected_commit_tx_info { - let total_pending_htlcs = self.context.pending_inbound_htlcs.len() + self.context.pending_outbound_htlcs.len() - + self.context.holding_cell_htlc_updates.len(); - if info.total_pending_htlcs == total_pending_htlcs - && info.next_holder_htlc_id == self.context.next_holder_htlc_id - && info.next_counterparty_htlc_id == self.context.next_counterparty_htlc_id - && info.feerate == self.context.feerate_per_kw { - assert_eq!(commitment_stats.total_fee_sat, info.fee / 1000); - } - } - } - } - - if msg.htlc_signatures.len() != commitment_stats.num_nondust_htlcs { - return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs))); - } - - // Up to LDK 0.0.115, HTLC information was required to be duplicated in the - // `htlcs_and_sigs` vec and in the `holder_commitment_tx` itself, both of which were passed - // in the `ChannelMonitorUpdate`. In 0.0.115, support for having a separate set of - // outbound-non-dust-HTLCSources in the `ChannelMonitorUpdate` was added, however for - // backwards compatibility, we never use it in production. To provide test coverage, here, - // we randomly decide (in test/fuzzing builds) to use the new vec sometimes. - #[allow(unused_assignments, unused_mut)] - let mut separate_nondust_htlc_sources = false; - #[cfg(all(feature = "std", any(test, fuzzing)))] { - use core::hash::{BuildHasher, Hasher}; - // Get a random value using the only std API to do so - the DefaultHasher - let rand_val = std::collections::hash_map::RandomState::new().build_hasher().finish(); - separate_nondust_htlc_sources = rand_val % 2 == 0; - } - - let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len()); - let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len()); - for (idx, (htlc, mut source_opt)) in htlcs_cloned.drain(..).enumerate() { - if let Some(_) = htlc.transaction_output_index { - let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.feerate_per_kw, - self.funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, &self.context.channel_type, - &keys.broadcaster_delayed_payment_key, &keys.revocation_key); - - let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &keys); - let htlc_sighashtype = if self.context.channel_type.supports_anchors_zero_fee_htlc_tx() { EcdsaSighashType::SinglePlusAnyoneCanPay } else { EcdsaSighashType::All }; - let htlc_sighash = hash_to_message!(&sighash::SighashCache::new(&htlc_tx).p2wsh_signature_hash(0, &htlc_redeemscript, htlc.to_bitcoin_amount(), htlc_sighashtype).unwrap()[..]); - log_trace!(logger, "Checking HTLC tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}.", - log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(keys.countersignatory_htlc_key.to_public_key().serialize()), - encode::serialize_hex(&htlc_tx), log_bytes!(htlc_sighash[..]), encode::serialize_hex(&htlc_redeemscript), &self.context.channel_id()); - if let Err(_) = self.context.secp_ctx.verify_ecdsa(&htlc_sighash, &msg.htlc_signatures[idx], &keys.countersignatory_htlc_key.to_public_key()) { - return Err(ChannelError::close("Invalid HTLC tx signature from peer".to_owned())); - } - if !separate_nondust_htlc_sources { - htlcs_and_sigs.push((htlc, Some(msg.htlc_signatures[idx]), source_opt.take())); - } - } else { - htlcs_and_sigs.push((htlc, None, source_opt.take())); - } - if separate_nondust_htlc_sources { - if let Some(source) = source_opt.take() { - nondust_htlc_sources.push(source); - } - } - debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere"); - } - - let holder_commitment_tx = HolderCommitmentTransaction::new( - commitment_stats.tx, - msg.signature, - msg.htlc_signatures.clone(), - &self.funding.get_holder_pubkeys().funding_pubkey, - self.funding.counterparty_funding_pubkey() - ); - - self.context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, commitment_stats.outbound_htlc_preimages) - .map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?; - - Ok(LatestHolderCommitmentTXInfo { - commitment_tx: holder_commitment_tx, - htlc_outputs: htlcs_and_sigs, - nondust_htlc_sources, - }) - } - pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned, logger: &L) -> Result, ChannelError> where L::Target: Logger { @@ -5641,7 +5645,7 @@ impl FundedChannel where return Err(ChannelError::close("Peer sent commitment_signed after we'd started exchanging closing_signeds".to_owned())); } - let commitment_tx_info = self.validate_commitment_signed(msg, logger)?; + let commitment_tx_info = self.context.validate_commitment_signed(&self.funding, &self.holder_commitment_point, msg, logger)?; // Update state now that we've passed all the can-fail calls... let mut need_commitment = false; From 5549a1aa235cf29af2a1b8811d3cf35e84f717a5 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Fri, 28 Feb 2025 15:29:34 -0600 Subject: [PATCH 3/4] Use reference instead of mut reference --- lightning/src/ln/channel.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index dc48018f76f..0f5619a69ef 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2032,7 +2032,7 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide fn received_msg(&self) -> &'static str; fn check_counterparty_commitment_signature( - &self, sig: &Signature, holder_commitment_point: &mut HolderCommitmentPoint, logger: &L + &self, sig: &Signature, holder_commitment_point: &HolderCommitmentPoint, logger: &L ) -> Result where L::Target: Logger { let funding_script = self.funding().get_funding_redeemscript(); From 2be03cee216583c848af49c02e88de25b2d90148 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Mon, 3 Mar 2025 11:32:56 -0600 Subject: [PATCH 4/4] Move holder_commitment_point advancement Now that validate_commitment_signed encapsulates all funding-specific checks, move the holder_commitment_point advancement immediately following the call to it. While there should be any early returns at that point, it's good to have move it earlier in case of future changes. --- lightning/src/ln/channel.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 0f5619a69ef..da26a64ad42 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -5647,6 +5647,20 @@ impl FundedChannel where let commitment_tx_info = self.context.validate_commitment_signed(&self.funding, &self.holder_commitment_point, msg, logger)?; + if self.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger).is_err() { + // We only fail to advance our commitment point/number if we're currently + // waiting for our signer to unblock and provide a commitment point. + // During post-funding channel operation, we only advance our point upon + // receiving a commitment_signed, and our counterparty cannot send us + // another commitment signed until we've provided a new commitment point + // in revoke_and_ack, which requires unblocking our signer and completing + // the advance to the next point. This should be unreachable since + // a new commitment_signed should fail at our signature checks in + // validate_commitment_signed. + debug_assert!(false, "We should be ready to advance our commitment point by the time we receive commitment_signed"); + return Err(ChannelError::close("Failed to advance our commitment point".to_owned())); + } + // Update state now that we've passed all the can-fail calls... let mut need_commitment = false; if let &mut Some((_, ref mut update_state)) = &mut self.context.pending_update_fee { @@ -5702,18 +5716,6 @@ impl FundedChannel where channel_id: Some(self.context.channel_id()), }; - if self.holder_commitment_point.advance(&self.context.holder_signer, &self.context.secp_ctx, logger).is_err() { - // We only fail to advance our commitment point/number if we're currently - // waiting for our signer to unblock and provide a commitment point. - // During post-funding channel operation, we only advance our point upon - // receiving a commitment_signed, and our counterparty cannot send us - // another commitment signed until we've provided a new commitment point - // in revoke_and_ack, which requires unblocking our signer and completing - // the advance to the next point. This should be unreachable since - // a new commitment_signed should fail at our signature checks above. - debug_assert!(false, "We should be ready to advance our commitment point by the time we receive commitment_signed"); - return Err(ChannelError::close("Failed to advance our commitment point".to_owned())); - } self.context.expecting_peer_commitment_signed = false; // Note that if we need_commitment & !AwaitingRemoteRevoke we'll call // build_commitment_no_status_check() next which will reset this to RAAFirst.