diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index d6fb676d294..887df61f3af 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -38,7 +38,7 @@ use crate::types::features::ChannelTypeFeatures; use crate::types::payment::{PaymentHash, PaymentPreimage}; use crate::ln::msgs::DecodeError; use crate::ln::channel_keys::{DelayedPaymentKey, DelayedPaymentBasepoint, HtlcBasepoint, HtlcKey, RevocationKey, RevocationBasepoint}; -use crate::ln::chan_utils::{self,CommitmentTransaction, CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCClaim, ChannelTransactionParameters, HolderCommitmentTransaction, TxCreationKeys}; +use crate::ln::chan_utils::{self,CommitmentTransaction, CounterpartyCommitmentSecrets, HTLCOutputInCommitment, HTLCClaim, ChannelTransactionParameters, HolderCommitmentTransaction}; use crate::ln::channelmanager::{HTLCSource, SentHTLCId, PaymentClaimDetails}; use crate::chain; use crate::chain::{BestBlock, WatchedOutput}; @@ -3506,20 +3506,9 @@ impl ChannelMonitorImpl { to_broadcaster_value: u64, to_countersignatory_value: u64, feerate_per_kw: u32, mut nondust_htlcs: Vec<(HTLCOutputInCommitment, Option>)> ) -> CommitmentTransaction { - let broadcaster_keys = - &self.funding.channel_parameters.counterparty_parameters.as_ref().unwrap().pubkeys; - let countersignatory_keys = &self.funding.channel_parameters.holder_pubkeys; - - let broadcaster_funding_key = broadcaster_keys.funding_pubkey; - let countersignatory_funding_key = countersignatory_keys.funding_pubkey; - let keys = TxCreationKeys::from_channel_static_keys(&their_per_commitment_point, - &broadcaster_keys, &countersignatory_keys, &self.onchain_tx_handler.secp_ctx); let channel_parameters = &self.funding.channel_parameters.as_counterparty_broadcastable(); - - CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, - to_broadcaster_value, to_countersignatory_value, broadcaster_funding_key, - countersignatory_funding_key, keys, feerate_per_kw, &mut nondust_htlcs, - channel_parameters) + CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, their_per_commitment_point, + to_broadcaster_value, to_countersignatory_value, feerate_per_kw, &mut nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx) } fn counterparty_commitment_txs_from_update(&self, update: &ChannelMonitorUpdate) -> Vec { diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index af5e7d7101b..88efc9e3e3c 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -1180,13 +1180,6 @@ impl HolderCommitmentTransaction { let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); let dummy_sig = sign(&secp_ctx, &secp256k1::Message::from_digest([42; 32]), &SecretKey::from_slice(&[42; 32]).unwrap()); - let keys = TxCreationKeys { - per_commitment_point: dummy_key.clone(), - revocation_key: RevocationKey::from_basepoint(&secp_ctx, &RevocationBasepoint::from(dummy_key), &dummy_key), - broadcaster_htlc_key: HtlcKey::from_basepoint(&secp_ctx, &HtlcBasepoint::from(dummy_key), &dummy_key), - countersignatory_htlc_key: HtlcKey::from_basepoint(&secp_ctx, &HtlcBasepoint::from(dummy_key), &dummy_key), - broadcaster_delayed_payment_key: DelayedPaymentKey::from_basepoint(&secp_ctx, &DelayedPaymentBasepoint::from(dummy_key), &dummy_key), - }; let channel_pubkeys = ChannelPublicKeys { funding_pubkey: dummy_key.clone(), revocation_basepoint: RevocationBasepoint::from(dummy_key), @@ -1208,7 +1201,7 @@ impl HolderCommitmentTransaction { for _ in 0..htlcs.len() { counterparty_htlc_sigs.push(dummy_sig); } - let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, 0, 0, dummy_key.clone(), dummy_key.clone(), keys, 0, htlcs, &channel_parameters.as_counterparty_broadcastable()); + let inner = CommitmentTransaction::new_with_auxiliary_htlc_data(0, &dummy_key, 0, 0, 0, htlcs, &channel_parameters.as_counterparty_broadcastable(), &secp_ctx); htlcs.sort_by_key(|htlc| htlc.0.transaction_output_index); HolderCommitmentTransaction { inner, @@ -1518,12 +1511,13 @@ impl CommitmentTransaction { /// Only include HTLCs that are above the dust limit for the channel. /// /// This is not exported to bindings users due to the generic though we likely should expose a version without - pub fn new_with_auxiliary_htlc_data(commitment_number: u64, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, broadcaster_funding_key: PublicKey, countersignatory_funding_key: PublicKey, keys: TxCreationKeys, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> CommitmentTransaction { + pub fn new_with_auxiliary_htlc_data(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1) -> CommitmentTransaction { let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat); let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat); + let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx); // Sort outputs and populate output indices while keeping track of the auxiliary data - let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters, &broadcaster_funding_key, &countersignatory_funding_key).unwrap(); + let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters); let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(commitment_number, channel_parameters); let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); @@ -1552,11 +1546,11 @@ impl CommitmentTransaction { self } - fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result { + fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters) -> BuiltCommitmentTransaction { let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters); let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect(); - let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters, broadcaster_funding_key, countersignatory_funding_key)?; + let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters); let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs); let txid = transaction.compute_txid(); @@ -1564,7 +1558,7 @@ impl CommitmentTransaction { transaction, txid }; - Ok(built_transaction) + built_transaction } fn make_transaction(obscured_commitment_transaction_number: u64, txins: Vec, outputs: Vec) -> Transaction { @@ -1580,17 +1574,20 @@ impl CommitmentTransaction { // - initial sorting of outputs / HTLCs in the constructor, in which case T is auxiliary data the // caller needs to have sorted together with the HTLCs so it can keep track of the output index // - building of a bitcoin transaction during a verify() call, in which case T is just () - fn internal_build_outputs(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<(Vec, Vec), ()> { - let countersignatory_pubkeys = channel_parameters.countersignatory_pubkeys(); + fn internal_build_outputs(keys: &TxCreationKeys, to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, htlcs_with_aux: &mut Vec<(HTLCOutputInCommitment, T)>, channel_parameters: &DirectedChannelTransactionParameters) -> (Vec, Vec) { + let countersignatory_payment_point = &channel_parameters.countersignatory_pubkeys().payment_point; + let countersignatory_funding_key = &channel_parameters.countersignatory_pubkeys().funding_pubkey; + let broadcaster_funding_key = &channel_parameters.broadcaster_pubkeys().funding_pubkey; + let channel_type = channel_parameters.channel_type_features(); let contest_delay = channel_parameters.contest_delay(); let mut txouts: Vec<(TxOut, Option<&mut HTLCOutputInCommitment>)> = Vec::new(); if to_countersignatory_value_sat > Amount::ZERO { - let script = if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() { - get_to_countersigner_keyed_anchor_redeemscript(&countersignatory_pubkeys.payment_point).to_p2wsh() + let script = if channel_type.supports_anchors_zero_fee_htlc_tx() { + get_to_countersigner_keyed_anchor_redeemscript(countersignatory_payment_point).to_p2wsh() } else { - ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_pubkeys.payment_point.serialize()).into()) + ScriptBuf::new_p2wpkh(&Hash160::hash(&countersignatory_payment_point.serialize()).into()) }; txouts.push(( TxOut { @@ -1616,7 +1613,7 @@ impl CommitmentTransaction { )); } - if channel_parameters.channel_type_features().supports_anchors_zero_fee_htlc_tx() { + if channel_type.supports_anchors_zero_fee_htlc_tx() { if to_broadcaster_value_sat > Amount::ZERO || !htlcs_with_aux.is_empty() { let anchor_script = get_keyed_anchor_redeemscript(broadcaster_funding_key); txouts.push(( @@ -1642,7 +1639,7 @@ impl CommitmentTransaction { let mut htlcs = Vec::with_capacity(htlcs_with_aux.len()); for (htlc, _) in htlcs_with_aux { - let script = get_htlc_redeemscript(&htlc, &channel_parameters.channel_type_features(), &keys); + let script = get_htlc_redeemscript(htlc, channel_type, keys); let txout = TxOut { script_pubkey: script.to_p2wsh(), value: htlc.to_bitcoin_amount(), @@ -1674,7 +1671,7 @@ impl CommitmentTransaction { } outputs.push(out.0); } - Ok((outputs, htlcs)) + (outputs, htlcs) } fn internal_build_inputs(commitment_number: u64, channel_parameters: &DirectedChannelTransactionParameters) -> (u64, Vec) { @@ -1753,14 +1750,14 @@ impl CommitmentTransaction { /// /// An external validating signer must call this method before signing /// or using the built transaction. - pub fn verify(&self, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_keys: &ChannelPublicKeys, countersignatory_keys: &ChannelPublicKeys, secp_ctx: &Secp256k1) -> Result { + pub fn verify(&self, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1) -> Result { // This is the only field of the key cache that we trust - let per_commitment_point = self.keys.per_commitment_point; - let keys = TxCreationKeys::from_channel_static_keys(&per_commitment_point, broadcaster_keys, countersignatory_keys, secp_ctx); + let per_commitment_point = &self.keys.per_commitment_point; + let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx); if keys != self.keys { return Err(()); } - let tx = self.internal_rebuild_transaction(&keys, channel_parameters, &broadcaster_keys.funding_pubkey, &countersignatory_keys.funding_pubkey)?; + let tx = self.internal_rebuild_transaction(&keys, channel_parameters); if self.built.transaction != tx.transaction || self.built.txid != tx.txid { return Err(()); } @@ -1967,8 +1964,8 @@ pub fn get_commitment_transaction_number_obscure_factor( mod tests { use super::{CounterpartyCommitmentSecrets, ChannelPublicKeys}; use crate::chain; - use crate::ln::chan_utils::{get_htlc_redeemscript, get_to_countersigner_keyed_anchor_redeemscript, CommitmentTransaction, TxCreationKeys, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, HTLCOutputInCommitment}; - use bitcoin::secp256k1::{PublicKey, SecretKey, Secp256k1}; + use crate::ln::chan_utils::{get_htlc_redeemscript, get_to_countersigner_keyed_anchor_redeemscript, CommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, HTLCOutputInCommitment}; + use bitcoin::secp256k1::{self, PublicKey, SecretKey, Secp256k1}; use crate::util::test_utils; use crate::sign::{ChannelSigner, SignerProvider}; use bitcoin::{Network, Txid, ScriptBuf, CompressedPublicKey}; @@ -1983,13 +1980,12 @@ mod tests { struct TestCommitmentTxBuilder { commitment_number: u64, - holder_funding_pubkey: PublicKey, - counterparty_funding_pubkey: PublicKey, - keys: TxCreationKeys, + per_commitment_point: PublicKey, feerate_per_kw: u32, htlcs_with_aux: Vec<(HTLCOutputInCommitment, ())>, channel_parameters: ChannelTransactionParameters, counterparty_pubkeys: ChannelPublicKeys, + secp_ctx: Secp256k1::, } impl TestCommitmentTxBuilder { @@ -2014,32 +2010,23 @@ mod tests { channel_type_features: ChannelTypeFeatures::only_static_remote_key(), channel_value_satoshis: 3000, }; - let directed_parameters = channel_parameters.as_holder_broadcastable(); - let keys = TxCreationKeys::from_channel_static_keys( - &per_commitment_point, directed_parameters.broadcaster_pubkeys(), - directed_parameters.countersignatory_pubkeys(), &secp_ctx, - ); let htlcs_with_aux = Vec::new(); Self { commitment_number: 0, - holder_funding_pubkey: holder_pubkeys.funding_pubkey, - counterparty_funding_pubkey: counterparty_pubkeys.funding_pubkey, - keys, + per_commitment_point, feerate_per_kw: 1, htlcs_with_aux, channel_parameters, counterparty_pubkeys, + secp_ctx, } } fn build(&mut self, to_broadcaster_sats: u64, to_countersignatory_sats: u64) -> CommitmentTransaction { CommitmentTransaction::new_with_auxiliary_htlc_data( - self.commitment_number, to_broadcaster_sats, to_countersignatory_sats, - self.holder_funding_pubkey.clone(), - self.counterparty_funding_pubkey.clone(), - self.keys.clone(), self.feerate_per_kw, - &mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable() + self.commitment_number, &self.per_commitment_point, to_broadcaster_sats, to_countersignatory_sats, self.feerate_per_kw, + &mut self.htlcs_with_aux, &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx ) } } @@ -2087,7 +2074,7 @@ mod tests { builder.channel_parameters.channel_type_features = ChannelTypeFeatures::only_static_remote_key(); builder.htlcs_with_aux = vec![(received_htlc.clone(), ()), (offered_htlc.clone(), ())]; let tx = builder.build(3000, 0); - let keys = &builder.keys.clone(); + let keys = tx.trust().keys(); assert_eq!(tx.built.transaction.output.len(), 3); assert_eq!(tx.built.transaction.output[0].script_pubkey, get_htlc_redeemscript(&received_htlc, &ChannelTypeFeatures::only_static_remote_key(), &keys).to_p2wsh()); assert_eq!(tx.built.transaction.output[1].script_pubkey, get_htlc_redeemscript(&offered_htlc, &ChannelTypeFeatures::only_static_remote_key(), &keys).to_p2wsh()); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 9268d07449d..d6fbd162388 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -40,7 +40,7 @@ use crate::ln::script::{self, ShutdownScript}; use crate::ln::channel_state::{ChannelShutdownState, CounterpartyForwardingInfo, InboundHTLCDetails, InboundHTLCStateDetails, OutboundHTLCDetails, OutboundHTLCStateDetails}; use crate::ln::channelmanager::{self, OpenChannelMessage, PendingHTLCStatus, HTLCSource, SentHTLCId, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentClaimDetails, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT}; use crate::ln::chan_utils::{ - CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, htlc_success_tx_weight, + CounterpartyCommitmentSecrets, HTLCOutputInCommitment, htlc_success_tx_weight, htlc_timeout_tx_weight, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, max_htlcs, @@ -877,19 +877,22 @@ struct HTLCStats { on_holder_tx_outbound_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included } -/// An enum gathering stats on commitment transaction, either local or remote. -struct CommitmentStats<'a> { - tx: CommitmentTransaction, // the transaction info - feerate_per_kw: u32, // the feerate included to build the transaction - total_fee_sat: u64, // the total fee included in the transaction - num_nondust_htlcs: usize, // the number of HTLC outputs (dust HTLCs *non*-included) +/// A struct gathering data on a commitment, either local or remote. +struct CommitmentData<'a> { + stats: CommitmentStats, htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction - local_balance_msat: u64, // local balance before fees *not* considering dust limits - remote_balance_msat: u64, // remote balance before fees *not* considering dust limits outbound_htlc_preimages: Vec, // preimages for successful offered HTLCs since last commitment inbound_htlc_preimages: Vec, // preimages for successful received HTLCs since last commitment } +/// A struct gathering stats on a commitment transaction, either local or remote. +struct CommitmentStats { + tx: CommitmentTransaction, // the transaction info + total_fee_sat: u64, // the total fee included in the transaction + local_balance_before_fee_msat: u64, // local balance before fees *not* considering dust limits + remote_balance_before_fee_msat: u64, // remote balance before fees *not* considering dust limits +} + /// Used when calculating whether we or the remote can afford an additional HTLC. struct HTLCCandidate { amount_msat: u64, @@ -2045,8 +2048,10 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide ) -> Result where L::Target: Logger { let funding_script = self.funding().get_funding_redeemscript(); - let keys = self.context().build_holder_transaction_keys(&self.funding(), holder_commitment_point.current_point()); - let initial_commitment_tx = self.context().build_commitment_transaction(self.funding(), holder_commitment_point.transaction_number(), &keys, true, false, logger).tx; + 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.stats.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()); @@ -2083,8 +2088,10 @@ trait InitialRemoteCommitmentReceiver where SP::Target: SignerProvide } }; let context = self.context(); - let counterparty_keys = context.build_remote_transaction_keys(self.funding()); - let counterparty_initial_commitment_tx = context.build_commitment_transaction(self.funding(), context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; + 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.stats.tx; let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); @@ -3481,11 +3488,11 @@ impl ChannelContext where SP::Target: SignerProvider { { 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_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_stats.tx.trust(); + let trusted_tx = commitment_data.stats.tx.trust(); let bitcoin_tx = trusted_tx.built_transaction(); let sighash = bitcoin_tx.get_sighash_all(&funding_script, funding.get_value_satoshis()); @@ -3498,7 +3505,7 @@ impl ChannelContext where SP::Target: SignerProvider { } 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(); + let mut htlcs_cloned: Vec<_> = commitment_data.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. @@ -3508,7 +3515,7 @@ impl ChannelContext where SP::Target: SignerProvider { 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 { + if commitment_data.stats.remote_balance_before_fee_msat < commitment_data.stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat { return Err(ChannelError::close("Funding remote cannot afford proposed new fee".to_owned())); } } @@ -3524,14 +3531,14 @@ impl ChannelContext where SP::Target: SignerProvider { && 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); + assert_eq!(commitment_data.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))); + if msg.htlc_signatures.len() != commitment_data.stats.tx.htlcs().len() { + return Err(ChannelError::close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_data.stats.tx.htlcs().len()))); } // Up to LDK 0.0.115, HTLC information was required to be duplicated in the @@ -3551,19 +3558,20 @@ impl ChannelContext where SP::Target: SignerProvider { let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len()); let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len()); + let holder_keys = commitment_data.stats.tx.trust().keys(); 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, + let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_data.stats.tx.feerate_per_kw(), funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, &self.channel_type, - &keys.broadcaster_delayed_payment_key, &keys.revocation_key); + &holder_keys.broadcaster_delayed_payment_key, &holder_keys.revocation_key); - let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &self.channel_type, &keys); + let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &self.channel_type, &holder_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()), + 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], &keys.countersignatory_htlc_key.to_public_key()) { + 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 !separate_nondust_htlc_sources { @@ -3581,14 +3589,14 @@ impl ChannelContext where SP::Target: SignerProvider { } let holder_commitment_tx = HolderCommitmentTransaction::new( - commitment_stats.tx, + commitment_data.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) + 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 { @@ -3612,7 +3620,7 @@ impl ChannelContext where SP::Target: SignerProvider { /// generated by the peer which proposed adding the HTLCs, and thus we need to understand both /// which peer generated this transaction and "to whom" this transaction flows. #[inline] - fn build_commitment_transaction(&self, funding: &FundingScope, commitment_number: u64, keys: &TxCreationKeys, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats + fn build_commitment_transaction(&self, funding: &FundingScope, commitment_number: u64, per_commitment_point: &PublicKey, local: bool, generated_by_local: bool, logger: &L) -> CommitmentData where L::Target: Logger { let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new(); @@ -3706,13 +3714,9 @@ impl ChannelContext where SP::Target: SignerProvider { } else { log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name); match &htlc.state { - &InboundHTLCState::LocalRemoved(ref reason) => { - if generated_by_local { - if let &InboundHTLCRemovalReason::Fulfill(preimage) = reason { - inbound_htlc_preimages.push(preimage); - value_to_self_msat_offset += htlc.amount_msat as i64; - } - } + &InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(preimage)) => { + inbound_htlc_preimages.push(preimage); + value_to_self_msat_offset += htlc.amount_msat as i64; }, _ => {}, } @@ -3748,27 +3752,27 @@ impl ChannelContext where SP::Target: SignerProvider { } else { log_trace!(logger, " ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name); match htlc.state { - OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_))|OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) => { - value_to_self_msat_offset -= htlc.amount_msat as i64; - }, + OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) | + OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) | OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_)) => { - if !generated_by_local { - value_to_self_msat_offset -= htlc.amount_msat as i64; - } + value_to_self_msat_offset -= htlc.amount_msat as i64; }, _ => {}, } } } - let value_to_self_msat: i64 = (funding.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset; - assert!(value_to_self_msat >= 0); - // Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie - // AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to - // "violate" their reserve value by couting those against it. Thus, we have to convert - // everything to i64 before subtracting as otherwise we can overflow. - let value_to_remote_msat: i64 = (funding.get_value_satoshis() * 1000) as i64 - (funding.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset; - assert!(value_to_remote_msat >= 0); + // # Panics + // + // While we expect `value_to_self_msat_offset` to be negative in some cases, the value going + // to each party MUST be 0 or positive, even if all HTLCs pending in the commitment clear by + // failure. + + // TODO: When MSRV >= 1.66.0, use u64::checked_add_signed + let mut value_to_self_msat = (funding.value_to_self_msat as i64 + value_to_self_msat_offset).try_into().unwrap(); + let mut value_to_remote_msat = (funding.get_value_satoshis() * 1000).checked_sub(value_to_self_msat).unwrap(); + value_to_self_msat = value_to_self_msat.checked_sub(local_htlc_total_msat).unwrap(); + value_to_remote_msat = value_to_remote_msat.checked_sub(remote_htlc_total_msat).unwrap(); #[cfg(debug_assertions)] { @@ -3779,99 +3783,74 @@ impl ChannelContext where SP::Target: SignerProvider { } else { funding.counterparty_max_commitment_tx_output.lock().unwrap() }; - debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat as u64 || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap() as i64); - broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat as u64); - debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat as u64 || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis as i64); - broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat as u64); + debug_assert!(broadcaster_max_commitment_tx_output.0 <= value_to_self_msat || value_to_self_msat / 1000 >= funding.counterparty_selected_channel_reserve_satoshis.unwrap()); + broadcaster_max_commitment_tx_output.0 = cmp::max(broadcaster_max_commitment_tx_output.0, value_to_self_msat); + debug_assert!(broadcaster_max_commitment_tx_output.1 <= value_to_remote_msat || value_to_remote_msat / 1000 >= funding.holder_selected_channel_reserve_satoshis); + broadcaster_max_commitment_tx_output.1 = cmp::max(broadcaster_max_commitment_tx_output.1, value_to_remote_msat); } + // We MUST use saturating subs here, as the funder's balance is not guaranteed to be greater + // than or equal to the sum of `total_fee_sat` and `anchors_val`. + // + // This is because when the remote party sends an `update_fee` message, we build the new + // commitment transaction *before* checking whether the remote party's balance is enough to + // cover the total fee and the anchors. + let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, included_non_dust_htlcs.len(), &funding.channel_transaction_parameters.channel_type_features); - let anchors_val = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 } as i64; + let anchors_val = if funding.channel_transaction_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 }; let (value_to_self, value_to_remote) = if funding.is_outbound() { - (value_to_self_msat / 1000 - anchors_val - total_fee_sat as i64, value_to_remote_msat / 1000) + ((value_to_self_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat), value_to_remote_msat / 1000) } else { - (value_to_self_msat / 1000, value_to_remote_msat / 1000 - anchors_val - total_fee_sat as i64) + (value_to_self_msat / 1000, (value_to_remote_msat / 1000).saturating_sub(anchors_val).saturating_sub(total_fee_sat)) }; - let mut value_to_a = if local { value_to_self } else { value_to_remote }; - let mut value_to_b = if local { value_to_remote } else { value_to_self }; - let (funding_pubkey_a, funding_pubkey_b) = if local { - (funding.get_holder_pubkeys().funding_pubkey, funding.get_counterparty_pubkeys().funding_pubkey) - } else { - (funding.get_counterparty_pubkeys().funding_pubkey, funding.get_holder_pubkeys().funding_pubkey) - }; + let mut to_broadcaster_value_sat = if local { value_to_self } else { value_to_remote }; + let mut to_countersignatory_value_sat = if local { value_to_remote } else { value_to_self }; - if value_to_a >= (broadcaster_dust_limit_satoshis as i64) { - log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a); + if to_broadcaster_value_sat >= broadcaster_dust_limit_satoshis { + log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, to_broadcaster_value_sat); } else { - value_to_a = 0; + to_broadcaster_value_sat = 0; } - if value_to_b >= (broadcaster_dust_limit_satoshis as i64) { - log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, value_to_b); + if to_countersignatory_value_sat >= broadcaster_dust_limit_satoshis { + log_trace!(logger, " ...including {} output with value {}", if local { "to_remote" } else { "to_local" }, to_countersignatory_value_sat); } else { - value_to_b = 0; + to_countersignatory_value_sat = 0; } - let num_nondust_htlcs = included_non_dust_htlcs.len(); - let channel_parameters = if local { funding.channel_transaction_parameters.as_holder_broadcastable() } else { funding.channel_transaction_parameters.as_counterparty_broadcastable() }; let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, - value_to_a as u64, - value_to_b as u64, - funding_pubkey_a, - funding_pubkey_b, - keys.clone(), + per_commitment_point, + to_broadcaster_value_sat, + to_countersignatory_value_sat, feerate_per_kw, &mut included_non_dust_htlcs, - &channel_parameters + &channel_parameters, + &self.secp_ctx, ); let mut htlcs_included = included_non_dust_htlcs; // The unwrap is safe, because all non-dust HTLCs have been assigned an output index htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap()); htlcs_included.append(&mut included_dust_htlcs); - CommitmentStats { + let stats = CommitmentStats { tx, - feerate_per_kw, total_fee_sat, - num_nondust_htlcs, + local_balance_before_fee_msat: value_to_self_msat, + remote_balance_before_fee_msat: value_to_remote_msat, + }; + + CommitmentData { + stats, htlcs_included, - local_balance_msat: value_to_self_msat as u64, - remote_balance_msat: value_to_remote_msat as u64, inbound_htlc_preimages, outbound_htlc_preimages, } } - #[inline] - /// Creates a set of keys for build_commitment_transaction to generate a transaction which our - /// counterparty will sign (ie DO NOT send signatures over a transaction created by this to - /// our counterparty!) - /// The result is a transaction which we can revoke broadcastership of (ie a "local" transaction) - /// TODO Some magic rust shit to compile-time check this? - fn build_holder_transaction_keys(&self, funding: &FundingScope, per_commitment_point: PublicKey) -> TxCreationKeys { - let delayed_payment_base = &funding.get_holder_pubkeys().delayed_payment_basepoint; - let htlc_basepoint = &funding.get_holder_pubkeys().htlc_basepoint; - let counterparty_pubkeys = funding.get_counterparty_pubkeys(); - - TxCreationKeys::derive_new(&self.secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint) - } - - #[inline] - /// Creates a set of keys for build_commitment_transaction to generate a transaction which we - /// will sign and send to our counterparty. - /// If an Err is returned, it is a ChannelError::Close (for get_funding_created) - fn build_remote_transaction_keys(&self, funding: &FundingScope) -> TxCreationKeys { - let revocation_basepoint = &funding.get_holder_pubkeys().revocation_basepoint; - let htlc_basepoint = &funding.get_holder_pubkeys().htlc_basepoint; - let counterparty_pubkeys = funding.get_counterparty_pubkeys(); - - TxCreationKeys::derive_new(&self.secp_ctx, &self.counterparty_cur_commitment_point.unwrap(), &counterparty_pubkeys.delayed_payment_basepoint, &counterparty_pubkeys.htlc_basepoint, revocation_basepoint, htlc_basepoint) - } - pub fn get_feerate_sat_per_1000_weight(&self) -> u32 { self.feerate_per_kw } @@ -4654,9 +4633,10 @@ impl ChannelContext where SP::Target: SignerProvider { SP::Target: SignerProvider, L::Target: Logger { - let counterparty_keys = self.build_remote_transaction_keys(funding); - let counterparty_initial_commitment_tx = self.build_commitment_transaction( - funding, self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; + let commitment_data = self.build_commitment_transaction(funding, + self.cur_counterparty_commitment_transaction_number, + &self.counterparty_cur_commitment_point.unwrap(), false, false, logger); + let counterparty_initial_commitment_tx = commitment_data.stats.tx; match self.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ref ecdsa) => { @@ -6369,10 +6349,11 @@ impl FundedChannel where // Before proposing a feerate update, check that we can actually afford the new fee. let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); let htlc_stats = self.context.get_pending_htlc_stats(Some(feerate_per_kw), dust_exposure_limiting_feerate); - 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, true, logger); - let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; - let holder_balance_msat = commitment_stats.local_balance_msat - htlc_stats.outbound_holding_cell_msat; + let commitment_data = self.context.build_commitment_transaction(&self.funding, + self.holder_commitment_point.transaction_number(), + &self.holder_commitment_point.current_point(), true, true, logger); + let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.stats.tx.htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.context.get_channel_type()) * 1000; + let holder_balance_msat = commitment_data.stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat; if holder_balance_msat < buffer_fee_msat + self.funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { //TODO: auto-close after a number of failures? log_debug!(logger, "Cannot afford to send new feerate at {}", feerate_per_kw); @@ -6683,8 +6664,10 @@ impl FundedChannel where self.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger); } let funding_signed = if self.context.signer_pending_funding && !self.funding.is_outbound() { - let counterparty_keys = self.context.build_remote_transaction_keys(&self.funding); - let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number + 1, &counterparty_keys, false, false, logger).tx; + let commitment_data = self.context.build_commitment_transaction(&self.funding, + self.context.cur_counterparty_commitment_transaction_number + 1, + &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger); + let counterparty_initial_commitment_tx = commitment_data.stats.tx; self.context.get_funding_signed_msg(&self.funding.channel_transaction_parameters, logger, counterparty_initial_commitment_tx) } else { None }; // Provide a `channel_ready` message if we need to, but only if we're _not_ still pending @@ -6869,7 +6852,7 @@ impl FundedChannel where log_trace!(logger, "Regenerating latest commitment update in channel {} with{} {} update_adds, {} update_fulfills, {} update_fails, and {} update_fail_malformeds", &self.context.channel_id(), if update_fee.is_some() { " update_fee," } else { "" }, update_add_htlcs.len(), update_fulfill_htlcs.len(), update_fail_htlcs.len(), update_fail_malformed_htlcs.len()); - let commitment_signed = if let Ok(update) = self.send_commitment_no_state_update(logger).map(|(cu, _)| cu) { + let commitment_signed = if let Ok(update) = self.send_commitment_no_state_update(logger) { if self.context.signer_pending_commitment_update { log_trace!(logger, "Commitment update generated: clearing signer_pending_commitment_update"); self.context.signer_pending_commitment_update = false; @@ -8757,9 +8740,10 @@ impl FundedChannel where -> (Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>, CommitmentTransaction) where L::Target: Logger { - let counterparty_keys = self.context.build_remote_transaction_keys(&self.funding); - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger); - let counterparty_commitment_tx = commitment_stats.tx; + let commitment_data = self.context.build_commitment_transaction(&self.funding, + self.context.cur_counterparty_commitment_transaction_number, + &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger); + let counterparty_commitment_tx = commitment_data.stats.tx; #[cfg(any(test, fuzzing))] { @@ -8772,69 +8756,68 @@ impl FundedChannel where && 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 { - let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, commitment_stats.num_nondust_htlcs, self.context.get_channel_type()) * 1000; + let actual_fee = commit_tx_fee_sat(self.context.feerate_per_kw, counterparty_commitment_tx.htlcs().len(), self.context.get_channel_type()) * 1000; assert_eq!(actual_fee, info.fee); } } } } - (commitment_stats.htlcs_included, counterparty_commitment_tx) + (commitment_data.htlcs_included, counterparty_commitment_tx) } /// Only fails in case of signer rejection. Used for channel_reestablish commitment_signed /// generation when we shouldn't change HTLC/channel state. - fn send_commitment_no_state_update(&self, logger: &L) -> Result<(msgs::CommitmentSigned, (Txid, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>)), ChannelError> where L::Target: Logger { + fn send_commitment_no_state_update(&self, logger: &L) -> Result where L::Target: Logger { // Get the fee tests from `build_commitment_no_state_update` #[cfg(any(test, fuzzing))] self.build_commitment_no_state_update(logger); - let counterparty_keys = self.context.build_remote_transaction_keys(&self.funding); - let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger); - let counterparty_commitment_txid = commitment_stats.tx.trust().txid(); + let commitment_data = self.context.build_commitment_transaction(&self.funding, + self.context.cur_counterparty_commitment_transaction_number, + &self.context.counterparty_cur_commitment_point.unwrap(), false, true, logger); + let counterparty_commitment_tx = commitment_data.stats.tx; match &self.context.holder_signer { ChannelSignerType::Ecdsa(ecdsa) => { let (signature, htlc_signatures); { - let mut htlcs = Vec::with_capacity(commitment_stats.htlcs_included.len()); - for &(ref htlc, _) in commitment_stats.htlcs_included.iter() { - htlcs.push(htlc); - } - let res = ecdsa.sign_counterparty_commitment( &self.funding.channel_transaction_parameters, - &commitment_stats.tx, - commitment_stats.inbound_htlc_preimages, - commitment_stats.outbound_htlc_preimages, + &counterparty_commitment_tx, + commitment_data.inbound_htlc_preimages, + commitment_data.outbound_htlc_preimages, &self.context.secp_ctx, ).map_err(|_| ChannelError::Ignore("Failed to get signatures for new commitment_signed".to_owned()))?; signature = res.0; htlc_signatures = res.1; + let trusted_tx = counterparty_commitment_tx.trust(); log_trace!(logger, "Signed remote commitment tx {} (txid {}) with redeemscript {} -> {} in channel {}", - encode::serialize_hex(&commitment_stats.tx.trust().built_transaction().transaction), - &counterparty_commitment_txid, encode::serialize_hex(&self.funding.get_funding_redeemscript()), + encode::serialize_hex(&trusted_tx.built_transaction().transaction), + &trusted_tx.txid(), encode::serialize_hex(&self.funding.get_funding_redeemscript()), log_bytes!(signature.serialize_compact()[..]), &self.context.channel_id()); - for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) { + let counterparty_keys = trusted_tx.keys(); + debug_assert_eq!(htlc_signatures.len(), trusted_tx.htlcs().len()); + for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(trusted_tx.htlcs()) { log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}", - encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.feerate_per_kw, self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), + encode::serialize_hex(&chan_utils::build_htlc_transaction(&trusted_tx.txid(), trusted_tx.feerate_per_kw(), self.funding.get_holder_selected_contest_delay(), htlc, &self.context.channel_type, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &self.context.channel_type, &counterparty_keys)), log_bytes!(counterparty_keys.broadcaster_htlc_key.to_public_key().serialize()), log_bytes!(htlc_sig.serialize_compact()[..]), &self.context.channel_id()); } } - Ok((msgs::CommitmentSigned { + Ok(msgs::CommitmentSigned { channel_id: self.context.channel_id, signature, htlc_signatures, batch: None, #[cfg(taproot)] partial_signature_with_nonce: None, - }, (counterparty_commitment_txid, commitment_stats.htlcs_included))) + }) }, // TODO (taproot|arik) #[cfg(taproot)] @@ -9274,8 +9257,10 @@ impl OutboundV1Channel where SP::Target: SignerProvider { /// Only allowed after [`FundingScope::channel_transaction_parameters`] is set. fn get_funding_created_msg(&mut self, logger: &L) -> Option where L::Target: Logger { - let counterparty_keys = self.context.build_remote_transaction_keys(&self.funding); - let counterparty_initial_commitment_tx = self.context.build_commitment_transaction(&self.funding, self.context.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; + let commitment_data = self.context.build_commitment_transaction(&self.funding, + self.context.cur_counterparty_commitment_transaction_number, + &self.context.counterparty_cur_commitment_point.unwrap(), false, false, logger); + let counterparty_initial_commitment_tx = commitment_data.stats.tx; let signature = match &self.context.holder_signer { // TODO (taproot|arik): move match into calling method for Taproot ChannelSignerType::Ecdsa(ecdsa) => { @@ -11839,7 +11824,7 @@ mod tests { use bitcoin::secp256k1::Message; use crate::sign::{ChannelDerivationParameters, HTLCDescriptor, ecdsa::EcdsaChannelSigner}; use crate::types::payment::PaymentPreimage; - use crate::ln::channel::{HTLCOutputInCommitment ,TxCreationKeys}; + use crate::ln::channel::HTLCOutputInCommitment; use crate::ln::channel_keys::{DelayedPaymentBasepoint, HtlcBasepoint}; use crate::ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters}; use crate::util::logger::Logger; @@ -11907,11 +11892,6 @@ mod tests { // build_commitment_transaction. let per_commitment_secret = SecretKey::from_slice(&>::from_hex("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap(); let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret); - let directed_params = chan.funding.channel_transaction_parameters.as_holder_broadcastable(); - let keys = TxCreationKeys::from_channel_static_keys( - &per_commitment_point, directed_params.broadcaster_pubkeys(), - directed_params.countersignatory_pubkeys(), &secp_ctx, - ); macro_rules! test_commitment { ( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $($remain:tt)* ) => { @@ -11931,14 +11911,9 @@ mod tests { ( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $opt_anchors: expr, { $( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), * } ) => { { - let (commitment_tx, htlcs): (_, Vec) = { - let mut commitment_stats = chan.context.build_commitment_transaction(&chan.funding, 0xffffffffffff - 42, &keys, true, false, &logger); - - let htlcs = commitment_stats.htlcs_included.drain(..) - .filter_map(|(htlc, _)| if htlc.transaction_output_index.is_some() { Some(htlc) } else { None }) - .collect(); - (commitment_stats.tx, htlcs) - }; + let commitment_data = chan.context.build_commitment_transaction(&chan.funding, + 0xffffffffffff - 42, &per_commitment_point, true, false, &logger); + let commitment_tx = commitment_data.stats.tx; let trusted_tx = commitment_tx.trust(); let unsigned_tx = trusted_tx.built_transaction(); let redeemscript = chan.funding.get_funding_redeemscript(); @@ -11953,10 +11928,10 @@ mod tests { counterparty_htlc_sigs.clear(); // Don't warn about excess mut for no-HTLC calls $({ let remote_signature = Signature::from_der(&>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap(); - per_htlc.push((htlcs[$htlc_idx].clone(), Some(remote_signature))); + per_htlc.push((commitment_tx.htlcs()[$htlc_idx].clone(), Some(remote_signature))); counterparty_htlc_sigs.push(remote_signature); })* - assert_eq!(htlcs.len(), per_htlc.len()); + assert_eq!(commitment_tx.htlcs().len(), per_htlc.len()); let holder_commitment_tx = HolderCommitmentTransaction::new( commitment_tx.clone(), @@ -11979,7 +11954,8 @@ mod tests { log_trace!(logger, "verifying htlc {}", $htlc_idx); let remote_signature = Signature::from_der(&>::from_hex($counterparty_htlc_sig_hex).unwrap()[..]).unwrap(); - let ref htlc = htlcs[$htlc_idx]; + let ref htlc = commitment_tx.htlcs()[$htlc_idx]; + let keys = commitment_tx.trust().keys(); let mut htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.context.feerate_per_kw, chan.funding.get_counterparty_selected_contest_delay().unwrap(), &htlc, $opt_anchors, &keys.broadcaster_delayed_payment_key, &keys.revocation_key); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 3a365919a46..4cfd63ab2d6 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -732,32 +732,14 @@ pub fn test_update_fee_that_funder_cannot_afford() { const INITIAL_COMMITMENT_NUMBER: u64 = 281474976710654; - // Get the TestChannelSigner for each channel, which will be used to (1) get the keys - // needed to sign the new commitment tx and (2) sign the new commitment tx. - let (local_revocation_basepoint, local_htlc_basepoint, local_funding) = { - let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); - let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); - let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); - let chan_signer = local_chan.get_signer(); - let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx); - (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, - pubkeys.funding_pubkey) - }; - let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = { + let remote_point = { let per_peer_state = nodes[1].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap(); let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let chan_signer = remote_chan.get_signer(); - let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx); - (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(), - pubkeys.funding_pubkey) + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap() }; - // Assemble the set of keys we can use for signatures for our commitment_signed message. - let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint, - &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint); - let res = { let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); @@ -766,13 +748,13 @@ pub fn test_update_fee_that_funder_cannot_afford() { let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![]; let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data( INITIAL_COMMITMENT_NUMBER - 1, + &remote_point, push_sats, channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0, &channel_type_features) / 1000, - local_funding, remote_funding, - commit_tx_keys.clone(), non_buffer_feerate + 4, &mut htlcs, - &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable() + &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(), + &secp_ctx, ); local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment( &local_chan.funding.channel_transaction_parameters, &commitment_tx, Vec::new(), @@ -807,6 +789,90 @@ pub fn test_update_fee_that_funder_cannot_afford() { [nodes[0].node.get_our_node_id()], channel_value); } +#[xtest(feature = "_externalize_tests")] +pub fn test_update_fee_that_saturates_subs() { + // Check that when a remote party sends us an `update_fee` message that results in a total fee + // on the commitment transaction that is greater than her balance, we saturate the subtractions, + // and force close the channel. + + let mut default_config = test_default_channel_config(); + let secp_ctx = Secp256k1::new(); + + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(default_config), Some(default_config)]); + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let chan_id = create_chan_between_nodes_with_value(&nodes[0], &nodes[1], 10_000, 8_500_000).3; + + const FEERATE: u32 = 250 * 10; // 10sat/vb + + // Assert that the new feerate will completely exhaust the balance of node 0, and saturate the + // subtraction of the total fee from node 0's balance. + let total_fee_sat = chan_utils::commit_tx_fee_sat(FEERATE, 0, &ChannelTypeFeatures::empty()); + assert!(total_fee_sat > 1500); + + const INITIAL_COMMITMENT_NUMBER: u64 = 281474976710654; + + // We build a commitment transcation here only to pass node 1's check of node 0's signature + // in `commitment_signed`. + + let remote_point = { + let per_peer_state = nodes[1].node.per_peer_state.read().unwrap(); + let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap(); + let remote_chan = chan_lock.channel_by_id.get(&chan_id).and_then(Channel::as_funded).unwrap(); + let chan_signer = remote_chan.get_signer(); + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER, &secp_ctx).unwrap() + }; + + let res = { + let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); + let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); + let local_chan = local_chan_lock.channel_by_id.get(&chan_id).and_then(Channel::as_funded).unwrap(); + let local_chan_signer = local_chan.get_signer(); + let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![]; + let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data( + INITIAL_COMMITMENT_NUMBER, + &remote_point, + 8500, + // Set a zero balance here: this is the transaction that node 1 will expect a signature for, as + // he will do a saturating subtraction of the total fees from node 0's balance. + 0, + FEERATE, + &mut htlcs, + &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(), + &secp_ctx, + ); + local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment( + &local_chan.funding.channel_transaction_parameters, &commitment_tx, Vec::new(), + Vec::new(), &secp_ctx, + ).unwrap() + }; + + let commit_signed_msg = msgs::CommitmentSigned { + channel_id: chan_id, + signature: res.0, + htlc_signatures: res.1, + batch: None, + #[cfg(taproot)] + partial_signature_with_nonce: None, + }; + + let update_fee = msgs::UpdateFee { + channel_id: chan_id, + feerate_per_kw: FEERATE, + }; + + nodes[1].node.handle_update_fee(nodes[0].node.get_our_node_id(), &update_fee); + + nodes[1].node.handle_commitment_signed(nodes[0].node.get_our_node_id(), &commit_signed_msg); + nodes[1].logger.assert_log_contains("lightning::ln::channelmanager", "Funding remote cannot afford proposed new fee", 3); + check_added_monitors!(nodes[1], 1); + check_closed_broadcast!(nodes[1], true); + check_closed_event!(nodes[1], 1, ClosureReason::ProcessingError { err: String::from("Funding remote cannot afford proposed new fee") }, + [nodes[0].node.get_our_node_id()], 10_000); +} + #[xtest(feature = "_externalize_tests")] pub fn test_update_fee_with_fundee_update_add_htlc() { let chanmon_cfgs = create_chanmon_cfgs(2); @@ -1460,9 +1526,7 @@ pub fn test_fee_spike_violation_fails_htlc() { const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1; - // Get the TestChannelSigner for each channel, which will be used to (1) get the keys - // needed to sign the new commitment tx and (2) sign the new commitment tx. - let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point, local_funding) = { + let (local_secret, next_local_point) = { let per_peer_state = nodes[0].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap(); let local_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); @@ -1470,27 +1534,17 @@ pub fn test_fee_spike_violation_fails_htlc() { // Make the signer believe we validated another commitment, so we can release the secret chan_signer.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1; - let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx); - (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(), - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap(), - chan_signer.as_ref().pubkeys(None, &secp_ctx).funding_pubkey) + (chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(), + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap()) }; - let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = { + let remote_point = { let per_peer_state = nodes[1].node.per_peer_state.read().unwrap(); let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap(); let remote_chan = chan_lock.channel_by_id.get(&chan.2).and_then(Channel::as_funded).unwrap(); let chan_signer = remote_chan.get_signer(); - let pubkeys = chan_signer.as_ref().pubkeys(None, &secp_ctx); - (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, - chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap(), - chan_signer.as_ref().pubkeys(None, &secp_ctx).funding_pubkey) + chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx).unwrap() }; - // Assemble the set of keys we can use for signatures for our commitment_signed message. - let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint, - &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint); - // Build the remote commitment transaction so we can sign it, and then later use the // signature for the commitment_signed message. let local_chan_balance = 1313; @@ -1512,13 +1566,13 @@ pub fn test_fee_spike_violation_fails_htlc() { let local_chan_signer = local_chan.get_signer(); let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data( commitment_number, + &remote_point, 95000, local_chan_balance, - local_funding, remote_funding, - commit_tx_keys.clone(), feerate_per_kw, &mut vec![(accepted_htlc_info, ())], - &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable() + &local_chan.funding.channel_transaction_parameters.as_counterparty_broadcastable(), + &secp_ctx, ); local_chan_signer.as_ecdsa().unwrap().sign_counterparty_commitment( &local_chan.funding.channel_transaction_parameters, &commitment_tx, Vec::new(), diff --git a/lightning/src/util/test_channel_signer.rs b/lightning/src/util/test_channel_signer.rs index 018c027a831..ea6451157cc 100644 --- a/lightning/src/util/test_channel_signer.rs +++ b/lightning/src/util/test_channel_signer.rs @@ -549,12 +549,7 @@ impl TestChannelSigner { commitment_tx: &'a CommitmentTransaction, secp_ctx: &Secp256k1, ) -> TrustedCommitmentTransaction<'a> { commitment_tx - .verify( - &channel_parameters.as_counterparty_broadcastable(), - channel_parameters.counterparty_pubkeys().unwrap(), - &channel_parameters.holder_pubkeys, - secp_ctx, - ) + .verify(&channel_parameters.as_counterparty_broadcastable(), secp_ctx) .expect("derived different per-tx keys or built transaction") } @@ -563,12 +558,7 @@ impl TestChannelSigner { commitment_tx: &'a CommitmentTransaction, secp_ctx: &Secp256k1, ) -> TrustedCommitmentTransaction<'a> { commitment_tx - .verify( - &channel_parameters.as_holder_broadcastable(), - &channel_parameters.holder_pubkeys, - channel_parameters.counterparty_pubkeys().unwrap(), - secp_ctx, - ) + .verify(&channel_parameters.as_holder_broadcastable(), secp_ctx) .expect("derived different per-tx keys or built transaction") } }