diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 60e263f245a..9b8a86b6490 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -21,10 +21,9 @@ use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::hashes::sha256d::Hash as Sha256d; use bitcoin::hashes::Hash; -use bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE; -use bitcoin::secp256k1::{ecdsa::Signature, Secp256k1}; -use bitcoin::secp256k1::{PublicKey, SecretKey}; -use bitcoin::{secp256k1, sighash}; +use bitcoin::secp256k1::{ + self, constants::PUBLIC_KEY_SIZE, ecdsa::Signature, PublicKey, Secp256k1, SecretKey, +}; use crate::chain::chaininterface::{ fee_for_weight, ConfirmationTarget, FeeEstimator, LowerBoundedFeeEstimator, @@ -2512,75 +2511,80 @@ where fn received_msg(&self) -> &'static str; - #[rustfmt::skip] - fn check_counterparty_commitment_signature( - &self, sig: &Signature, holder_commitment_point: &HolderCommitmentPoint, logger: &L - ) -> Result where L::Target: Logger { - let funding_script = self.funding().get_funding_redeemscript(); - - let commitment_data = self.context().build_commitment_transaction(self.funding(), - holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), - true, false, logger); - let initial_commitment_tx = commitment_data.tx; - let trusted_tx = initial_commitment_tx.trust(); - let initial_commitment_bitcoin_tx = trusted_tx.built_transaction(); - let sighash = initial_commitment_bitcoin_tx.get_sighash_all(&funding_script, self.funding().get_value_satoshis()); - // They sign the holder commitment transaction... - log_trace!(logger, "Checking {} tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} for channel {}.", - self.received_msg(), log_bytes!(sig.serialize_compact()[..]), log_bytes!(self.funding().counterparty_funding_pubkey().serialize()), - encode::serialize_hex(&initial_commitment_bitcoin_tx.transaction), log_bytes!(sighash[..]), - encode::serialize_hex(&funding_script), &self.context().channel_id()); - secp_check!(self.context().secp_ctx.verify_ecdsa(&sighash, sig, self.funding().counterparty_funding_pubkey()), format!("Invalid {} signature from peer", self.received_msg())); - - Ok(initial_commitment_tx) - } - - #[rustfmt::skip] fn initial_commitment_signed( - &mut self, channel_id: ChannelId, counterparty_signature: Signature, holder_commitment_point: &mut HolderCommitmentPoint, - best_block: BestBlock, signer_provider: &SP, logger: &L, - ) -> Result<(ChannelMonitor<::EcdsaSigner>, CommitmentTransaction), ChannelError> + &mut self, channel_id: ChannelId, counterparty_signature: Signature, + holder_commitment_point: &mut HolderCommitmentPoint, best_block: BestBlock, + signer_provider: &SP, logger: &L, + ) -> Result< + (ChannelMonitor<::EcdsaSigner>, CommitmentTransaction), + ChannelError, + > where - L::Target: Logger + L::Target: Logger, { - let initial_commitment_tx = match self.check_counterparty_commitment_signature(&counterparty_signature, holder_commitment_point, logger) { - Ok(res) => res, - Err(ChannelError::Close(e)) => { - // TODO(dual_funding): Update for V2 established channels. - if !self.funding().is_outbound() { - self.funding_mut().channel_transaction_parameters.funding_outpoint = None; - } - return Err(ChannelError::Close(e)); - }, - Err(e) => { - // The only error we know how to handle is ChannelError::Close, so we fall over here - // to make sure we don't continue with an inconsistent state. - panic!("unexpected error type from check_counterparty_commitment_signature {:?}", e); - } - }; let context = self.context(); - let commitment_data = context.build_commitment_transaction(self.funding(), - context.cur_counterparty_commitment_transaction_number, - &context.counterparty_cur_commitment_point.unwrap(), false, false, logger); - let counterparty_initial_commitment_tx = commitment_data.tx; - let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); - let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); - - log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}", - &context.channel_id(), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction)); + let initial_commitment_tx = context + .build_commitment_transaction( + self.funding(), + holder_commitment_point.transaction_number(), + &holder_commitment_point.current_point(), + true, + false, + logger, + ) + .tx; let holder_commitment_tx = HolderCommitmentTransaction::new( initial_commitment_tx, counterparty_signature, Vec::new(), &self.funding().get_holder_pubkeys().funding_pubkey, - &self.funding().counterparty_funding_pubkey() + &self.funding().counterparty_funding_pubkey(), ); - if context.holder_signer.as_ref().validate_holder_commitment(&holder_commitment_tx, Vec::new()).is_err() { + log_trace!( + logger, + "Validating {} commitment in channel {}", + self.received_msg(), + context.channel_id() + ); + if context + .holder_signer + .as_ref() + .validate_holder_commitment( + &self.funding().channel_transaction_parameters, + &holder_commitment_tx, + Vec::new(), + &context.secp_ctx, + ) + .is_err() + { + if !self.funding().is_outbound() { + self.funding_mut().channel_transaction_parameters.funding_outpoint = None; + } return Err(ChannelError::close("Failed to validate our commitment".to_owned())); } + let commitment_data = context.build_commitment_transaction( + self.funding(), + context.cur_counterparty_commitment_transaction_number, + &context.counterparty_cur_commitment_point.unwrap(), + false, + false, + logger, + ); + let counterparty_initial_commitment_tx = commitment_data.tx; + let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); + let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); + + log_trace!( + logger, + "Initial counterparty tx for channel {} is: txid {} tx {}", + &context.channel_id(), + counterparty_initial_bitcoin_tx.txid, + encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction) + ); + // Now that we're past error-generating stuff, update our local state: let is_v2_established = self.is_v2_established(); @@ -2590,35 +2594,61 @@ where assert!(!context.channel_state.is_monitor_update_in_progress()); // We have not had any monitor(s) yet to fail update! if !is_v2_established { if context.is_batch_funding() { - context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::WAITING_FOR_BATCH); + context.channel_state = ChannelState::AwaitingChannelReady( + AwaitingChannelReadyFlags::WAITING_FOR_BATCH, + ); } else { - context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); + context.channel_state = + ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); } } - if holder_commitment_point.advance(&context.holder_signer, &context.secp_ctx, logger).is_err() { + if holder_commitment_point + .advance(&context.holder_signer, &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. // We cannot send accept_channel/open_channel before this has occurred, so if we // err here by the time we receive funding_created/funding_signed, something has gone wrong. - debug_assert!(false, "We should be ready to advance our commitment point by the time we receive {}", self.received_msg()); - return Err(ChannelError::close("Failed to advance holder commitment point".to_owned())); + debug_assert!( + false, + "We should be ready to advance our commitment point by the time we receive {}", + self.received_msg() + ); + return Err(ChannelError::close( + "Failed to advance holder commitment point".to_owned(), + )); } let context = self.context(); let funding = self.funding(); - let obscure_factor = get_commitment_transaction_number_obscure_factor(&funding.get_holder_pubkeys().payment_point, &funding.get_counterparty_pubkeys().payment_point, funding.is_outbound()); - let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner()); + let obscure_factor = get_commitment_transaction_number_obscure_factor( + &funding.get_holder_pubkeys().payment_point, + &funding.get_counterparty_pubkeys().payment_point, + funding.is_outbound(), + ); + let shutdown_script = + context.shutdown_scriptpubkey.clone().map(|script| script.into_inner()); let monitor_signer = signer_provider.derive_channel_signer(context.channel_keys_id); // TODO(RBF): When implementing RBF, the funding_txo passed here must only update // ChannelMonitorImp::first_confirmed_funding_txo during channel establishment, not splicing let channel_monitor = ChannelMonitor::new( - context.secp_ctx.clone(), monitor_signer, shutdown_script, - funding.get_holder_selected_contest_delay(), &context.destination_script, - &funding.channel_transaction_parameters, funding.is_outbound(), obscure_factor, - holder_commitment_tx, best_block, context.counterparty_node_id, context.channel_id(), + context.secp_ctx.clone(), + monitor_signer, + shutdown_script, + funding.get_holder_selected_contest_delay(), + &context.destination_script, + &funding.channel_transaction_parameters, + funding.is_outbound(), + obscure_factor, + holder_commitment_tx, + best_block, + context.counterparty_node_id, + context.channel_id(), ); channel_monitor.provide_initial_counterparty_commitment_tx( - counterparty_initial_commitment_tx.clone(), logger, + counterparty_initial_commitment_tx.clone(), + logger, ); self.context_mut().cur_counterparty_commitment_transaction_number -= 1; @@ -4199,7 +4229,6 @@ where Ok(()) } - #[rustfmt::skip] fn validate_commitment_signed( &self, funding: &FundingScope, holder_commitment_point: &HolderCommitmentPoint, msg: &msgs::CommitmentSigned, logger: &L, @@ -4207,78 +4236,95 @@ where where L::Target: Logger, { - let funding_script = funding.get_funding_redeemscript(); + let commitment_data = self.build_commitment_transaction( + funding, + holder_commitment_point.transaction_number(), + &holder_commitment_point.current_point(), + true, + false, + logger, + ); - let commitment_data = self.build_commitment_transaction(funding, - holder_commitment_point.transaction_number(), &holder_commitment_point.current_point(), - true, false, logger); - let commitment_txid = { - let trusted_tx = commitment_data.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 - }; + if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() { + return Err(ChannelError::close(format!( + "Got wrong number of HTLC signatures ({}) from remote. It must be {}", + msg.htlc_signatures.len(), + commitment_data.tx.nondust_htlcs().len() + ))); + } + + let holder_commitment_tx = HolderCommitmentTransaction::new( + commitment_data.tx, + msg.signature, + msg.htlc_signatures.clone(), + &funding.get_holder_pubkeys().funding_pubkey, + funding.counterparty_funding_pubkey(), + ); + + log_trace!( + logger, + "Validating commitment_signed commitment in channel {}", + self.channel_id() + ); + self.holder_signer + .as_ref() + .validate_holder_commitment( + &funding.channel_transaction_parameters, + &holder_commitment_tx, + commitment_data.outbound_htlc_preimages, + &self.secp_ctx, + ) + .map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?; // 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 }; + } 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_data.stats.remote_balance_before_fee_anchors_msat < commitment_data.stats.total_fee_sat * 1000 + commitment_data.stats.total_anchors_sat * 1000 + counterparty_reserve_we_require_msat { - return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned())); + let counterparty_reserve_we_require_msat = + funding.holder_selected_channel_reserve_satoshis * 1000; + if commitment_data.stats.remote_balance_before_fee_anchors_msat + < commitment_data.stats.total_fee_sat * 1000 + + commitment_data.stats.total_anchors_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(); + 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() + 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_data.stats.total_fee_sat, info.fee / 1000); - } + && info.feerate == self.feerate_per_kw + { + assert_eq!(commitment_data.stats.total_fee_sat, info.fee / 1000); + } } } } - if msg.htlc_signatures.len() != commitment_data.tx.nondust_htlcs().len() { - return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.tx.nondust_htlcs().len()))); - } - - let holder_keys = commitment_data.tx.trust().keys(); - let mut nondust_htlc_sources = Vec::with_capacity(commitment_data.tx.nondust_htlcs().len()); - let mut dust_htlcs = Vec::with_capacity(commitment_data.htlcs_included.len() - commitment_data.tx.nondust_htlcs().len()); - for (idx, (htlc, mut source_opt)) in commitment_data.htlcs_included.into_iter().enumerate() { + let mut nondust_htlc_sources = + Vec::with_capacity(holder_commitment_tx.nondust_htlcs().len()); + let mut dust_htlcs = Vec::with_capacity( + commitment_data.htlcs_included.len() - holder_commitment_tx.nondust_htlcs().len(), + ); + for (htlc, mut source_opt) in commitment_data.htlcs_included.into_iter() { if let Some(_) = htlc.transaction_output_index { - let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.tx.feerate_per_kw(), - funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, funding.get_channel_type(), - &holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key); - - let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, funding.get_channel_type(), &holder_keys); - let htlc_sighashtype = if funding.get_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!(holder_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], &holder_keys.countersignatory_htlc_key.to_public_key()) { - return Err(ChannelError::close("Invalid HTLC tx signature from peer".to_owned())); - } if htlc.offered { if let Some(source) = source_opt.take() { nondust_htlc_sources.push(source.clone()); @@ -4292,17 +4338,6 @@ where debug_assert!(source_opt.is_none(), "HTLCSource should have been put somewhere"); } - let holder_commitment_tx = HolderCommitmentTransaction::new( - commitment_data.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_data.outbound_htlc_preimages) - .map_err(|_| ChannelError::close("Failed to validate our commitment".to_owned()))?; - Ok(LatestHolderCommitmentTXInfo { commitment_tx: holder_commitment_tx, htlc_outputs: dust_htlcs, diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 60aa21bc44e..f3718694f43 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -8892,7 +8892,7 @@ pub fn test_duplicate_funding_err_in_funding() { funding_created_msg.funding_output_index += 10; nodes[1].node.handle_funding_created(node_c_id, &funding_created_msg); get_err_msg(&nodes[1], &node_c_id); - let err = "Invalid funding_created signature from peer".to_owned(); + let err = "Failed to validate our commitment".to_owned(); let reason = ClosureReason::ProcessingError { err }; let expected_closing = ExpectedCloseEvent::from_id_reason(real_channel_id, false, reason); check_closed_events(&nodes[1], &[expected_closing]); diff --git a/lightning/src/sign/mod.rs b/lightning/src/sign/mod.rs index 075cd2b644e..7d397e3d736 100644 --- a/lightning/src/sign/mod.rs +++ b/lightning/src/sign/mod.rs @@ -763,8 +763,9 @@ pub trait ChannelSigner { /// closed. If you wish to make this operation asynchronous, you should instead return `Ok(())` /// and pause future signing operations until this validation completes. fn validate_holder_commitment( - &self, holder_tx: &HolderCommitmentTransaction, - outbound_htlc_preimages: Vec, + &self, channel_parameters: &ChannelTransactionParameters, + holder_tx: &HolderCommitmentTransaction, outbound_htlc_preimages: Vec, + secp_ctx: &Secp256k1, ) -> Result<(), ()>; /// Validate the counterparty's revocation. @@ -1362,9 +1363,68 @@ impl ChannelSigner for InMemorySigner { } fn validate_holder_commitment( - &self, _holder_tx: &HolderCommitmentTransaction, - _outbound_htlc_preimages: Vec, + &self, channel_parameters: &ChannelTransactionParameters, + holder_tx: &HolderCommitmentTransaction, _outbound_htlc_preimages: Vec, + secp_ctx: &secp256k1::Secp256k1, ) -> Result<(), ()> { + let counterparty_funding_pubkey = + &channel_parameters.counterparty_pubkeys().as_ref().unwrap().funding_pubkey; + let funding_script = channel_parameters.make_funding_redeemscript(); + let channel_value_satoshis = channel_parameters.channel_value_satoshis; + let trusted_tx = holder_tx.trust(); + let bitcoin_tx = trusted_tx.built_transaction(); + let sighash = bitcoin_tx.get_sighash_all(&funding_script, channel_value_satoshis); + + secp_ctx + .verify_ecdsa(&sighash, &holder_tx.counterparty_sig, counterparty_funding_pubkey) + .map_err(|_| ())?; + + let holder_keys = trusted_tx.keys(); + + if holder_tx.nondust_htlcs().len() != holder_tx.counterparty_htlc_sigs.len() { + return Err(()); + } + for (htlc, sig) in + holder_tx.nondust_htlcs().iter().zip(holder_tx.counterparty_htlc_sigs.iter()) + { + let htlc_tx = chan_utils::build_htlc_transaction( + &bitcoin_tx.txid, + holder_tx.feerate_per_kw(), + channel_parameters.as_holder_broadcastable().contest_delay(), + &htlc, + &channel_parameters.channel_type_features, + &holder_keys.broadcaster_delayed_payment_key, + &holder_keys.revocation_key, + ); + let htlc_redeemscript = chan_utils::get_htlc_redeemscript( + &htlc, + &channel_parameters.channel_type_features, + &holder_keys, + ); + let htlc_sighashtype = + if channel_parameters.channel_type_features.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()[..] + ); + secp_ctx + .verify_ecdsa( + &htlc_sighash, + &sig, + &holder_keys.countersignatory_htlc_key.to_public_key(), + ) + .map_err(|_| ())?; + } Ok(()) } diff --git a/lightning/src/util/dyn_signer.rs b/lightning/src/util/dyn_signer.rs index 7e9844e2ac0..a245969a812 100644 --- a/lightning/src/util/dyn_signer.rs +++ b/lightning/src/util/dyn_signer.rs @@ -172,8 +172,10 @@ delegate!(DynSigner, ChannelSigner, ) -> Result, fn release_commitment_secret(, idx: u64) -> Result<[u8; 32], ()>, fn validate_holder_commitment(, + channel_parameters: &ChannelTransactionParameters, holder_tx: &HolderCommitmentTransaction, - preimages: Vec + preimages: Vec, + secp_ctx: &Secp256k1 ) -> Result<(), ()>, fn pubkeys(, splice_parent_funding_txid: Option, secp_ctx: &Secp256k1 diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index e619069af50..f5738dcc886 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -191,8 +191,9 @@ impl ChannelSigner for TestChannelSigner { } fn validate_holder_commitment( - &self, holder_tx: &HolderCommitmentTransaction, - _outbound_htlc_preimages: Vec, + &self, channel_parameters: &ChannelTransactionParameters, + holder_tx: &HolderCommitmentTransaction, outbound_htlc_preimages: Vec, + secp_ctx: &Secp256k1, ) -> Result<(), ()> { let mut state = self.state.lock().unwrap(); let idx = holder_tx.commitment_number(); @@ -205,7 +206,12 @@ impl ChannelSigner for TestChannelSigner { ); } state.last_holder_commitment = idx; - Ok(()) + self.inner.validate_holder_commitment( + channel_parameters, + holder_tx, + outbound_htlc_preimages, + secp_ctx, + ) } fn validate_counterparty_revocation(&self, idx: u64, _secret: &SecretKey) -> Result<(), ()> {