From efd9ad22fc7d7c3ab131a7fc08b2aa1e1f46c1c0 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Thu, 18 Nov 2021 21:23:41 -0500 Subject: [PATCH] Introduce CommitmentStats --- lightning/src/ln/channel.rs | 138 ++++++++++++++++++++---------------- 1 file changed, 76 insertions(+), 62 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 88007e3bc..fcb13031a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -296,6 +296,17 @@ struct HTLCStats { holding_cell_msat: u64, } +/// 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) + 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 but considering dust limits + remote_balance_msat: u64, // remote balance before fees but considering dust limits +} + /// Used when calculating whether we or the remote can afford an additional HTLC. struct HTLCCandidate { amount_msat: u64, @@ -1118,14 +1129,10 @@ impl Channel { /// have not yet committed it. Such HTLCs will only be included in transactions which are being /// 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. - /// Returns (the transaction info, the number of HTLC outputs which were present in the - /// transaction, the list of HTLCs which were not ignored when building the transaction). - /// Note that below-dust HTLCs are included in the fourth return value, but not the third, and - /// sources are provided only for outbound HTLCs in the fourth return value. - /// Note that fifth and sixth return values are respectively anticipated holder and counterparty - /// balance, not the value actually paid on-chain, which may be zero if it got rounded to dust. #[inline] - fn build_commitment_transaction(&self, commitment_number: u64, keys: &TxCreationKeys, local: bool, generated_by_local: bool, logger: &L) -> (CommitmentTransaction, u32, usize, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>, u64, u64) where L::Target: Logger { + fn build_commitment_transaction(&self, commitment_number: u64, keys: &TxCreationKeys, local: bool, generated_by_local: bool, logger: &L) -> CommitmentStats + where L::Target: Logger + { let mut included_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::new(); let num_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len(); let mut included_non_dust_htlcs: Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)> = Vec::with_capacity(num_htlcs); @@ -1244,13 +1251,13 @@ impl Channel { } } - let value_to_self_msat: i64 = (self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset; + let mut value_to_self_msat: i64 = (self.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 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset; + let mut value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset; assert!(value_to_remote_msat >= 0); #[cfg(debug_assertions)] @@ -1316,7 +1323,19 @@ impl Channel { htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap()); htlcs_included.append(&mut included_dust_htlcs); - (tx, feerate_per_kw, num_nondust_htlcs, htlcs_included, value_to_self_msat as u64, value_to_remote_msat as u64) + // For the stats, trimmed-to-0 the value in msats accordingly + value_to_self_msat = if (value_to_self_msat * 1000) < broadcaster_dust_limit_satoshis as i64 { 0 } else { value_to_self_msat }; + value_to_remote_msat = if (value_to_remote_msat * 1000) < broadcaster_dust_limit_satoshis as i64 { 0 } else { value_to_remote_msat }; + + CommitmentStats { + tx, + feerate_per_kw, + total_fee_sat, + num_nondust_htlcs, + htlcs_included, + local_balance_msat: value_to_self_msat as u64, + remote_balance_msat: value_to_remote_msat as u64, + } } #[inline] @@ -1770,7 +1789,7 @@ impl Channel { let funding_script = self.get_funding_redeemscript(); let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?; - let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, logger).0; + let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, logger).tx; { let trusted_tx = initial_commitment_tx.trust(); let initial_commitment_bitcoin_tx = trusted_tx.built_transaction(); @@ -1784,7 +1803,7 @@ impl Channel { } let counterparty_keys = self.build_remote_transaction_keys()?; - let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).0; + let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); @@ -1895,7 +1914,7 @@ impl Channel { let funding_script = self.get_funding_redeemscript(); let counterparty_keys = self.build_remote_transaction_keys()?; - let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).0; + let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); @@ -1903,7 +1922,7 @@ impl Channel { log_bytes!(self.channel_id()), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction)); let holder_signer = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?; - let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &holder_signer, true, false, logger).0; + let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &holder_signer, true, false, logger).tx; { let trusted_tx = initial_commitment_tx.trust(); let initial_commitment_bitcoin_tx = trusted_tx.built_transaction(); @@ -2495,36 +2514,32 @@ impl Channel { let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number).map_err(|e| (None, e))?; - let (num_htlcs, mut htlcs_cloned, commitment_tx, commitment_txid, feerate_per_kw, _, counterparty_balance_msat) = { - let commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, logger); - let commitment_txid = { - let trusted_tx = commitment_tx.0.trust(); - let bitcoin_tx = trusted_tx.built_transaction(); - let sighash = bitcoin_tx.get_sighash_all(&funding_script, self.channel_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.counterparty_funding_pubkey().serialize()), encode::serialize_hex(&bitcoin_tx.transaction), - log_bytes!(sighash[..]), encode::serialize_hex(&funding_script), log_bytes!(self.channel_id())); - if let Err(_) = self.secp_ctx.verify(&sighash, &msg.signature, &self.counterparty_funding_pubkey()) { - return Err((None, ChannelError::Close("Invalid commitment tx signature from peer".to_owned()))); - } - bitcoin_tx.txid - }; - let htlcs_cloned: Vec<_> = commitment_tx.3.iter().map(|htlc| (htlc.0.clone(), htlc.1.map(|h| h.clone()))).collect(); - (commitment_tx.2, htlcs_cloned, commitment_tx.0, commitment_txid, commitment_tx.1, commitment_tx.4, commitment_tx.5) + let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_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.channel_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.counterparty_funding_pubkey().serialize()), encode::serialize_hex(&bitcoin_tx.transaction), + log_bytes!(sighash[..]), encode::serialize_hex(&funding_script), log_bytes!(self.channel_id())); + if let Err(_) = self.secp_ctx.verify(&sighash, &msg.signature, &self.counterparty_funding_pubkey()) { + return Err((None, 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!(!self.is_outbound()); } - let total_fee_sat = Channel::::commit_tx_fee_sat(feerate_per_kw, num_htlcs); if update_fee { - let counterparty_reserve_we_require = Channel::::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis); - if counterparty_balance_msat < total_fee_sat * 1000 + counterparty_reserve_we_require * 1000 { + debug_assert!(!self.is_outbound()); + let counterparty_reserve_we_require_msat = Channel::::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis) * 1000; + if commitment_stats.remote_balance_msat < commitment_stats.total_fee_sat * 1000 + counterparty_reserve_we_require_msat { return Err((None, ChannelError::Close("Funding remote cannot afford proposed new fee".to_owned()))); } } @@ -2540,21 +2555,21 @@ impl Channel { && 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!(total_fee_sat, info.fee / 1000); + assert_eq!(commitment_stats.total_fee_sat, info.fee / 1000); } } } } - if msg.htlc_signatures.len() != num_htlcs { - return Err((None, ChannelError::Close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), num_htlcs)))); + if msg.htlc_signatures.len() != commitment_stats.num_nondust_htlcs { + return Err((None, ChannelError::Close(format!("Got wrong number of HTLC signatures ({}) from remote. It must be {}", msg.htlc_signatures.len(), commitment_stats.num_nondust_htlcs)))); } // TODO: Sadly, we pass HTLCs twice to ChannelMonitor: once via the HolderCommitmentTransaction and once via the update let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len()); for (idx, (htlc, source)) in htlcs_cloned.drain(..).enumerate() { if let Some(_) = htlc.transaction_output_index { - let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, feerate_per_kw, + let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.feerate_per_kw, self.get_counterparty_selected_contest_delay().unwrap(), &htlc, &keys.broadcaster_delayed_payment_key, &keys.revocation_key); @@ -2573,7 +2588,7 @@ impl Channel { } let holder_commitment_tx = HolderCommitmentTransaction::new( - commitment_tx, + commitment_stats.tx, msg.signature, msg.htlc_signatures.clone(), &self.get_holder_pubkeys().funding_pubkey, @@ -3083,10 +3098,10 @@ impl Channel { let inbound_stats = self.get_inbound_pending_htlc_stats(Some(feerate_per_kw)); let outbound_stats = self.get_outbound_pending_htlc_stats(Some(feerate_per_kw)); let keys = if let Ok(keys) = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number) { keys } else { return None; }; - let (_, _, num_htlcs, _, holder_balance_msat, _) = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger); - let total_fee_sat = Channel::::commit_tx_fee_sat(feerate_per_kw, num_htlcs + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize); - let holder_balance_msat = holder_balance_msat - outbound_stats.holding_cell_msat; - if holder_balance_msat < total_fee_sat * 1000 + self.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { + let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger); + let buffer_fee_msat = Channel::::commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize) * 1000; + let holder_balance_msat = commitment_stats.local_balance_msat - outbound_stats.holding_cell_msat; + if holder_balance_msat < buffer_fee_msat + self.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); return None; @@ -4424,7 +4439,7 @@ impl Channel { /// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created) fn get_outbound_funding_created_signature(&mut self, logger: &L) -> Result where L::Target: Logger { let counterparty_keys = self.build_remote_transaction_keys()?; - let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).0; + let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx; Ok(self.holder_signer.sign_counterparty_commitment(&counterparty_initial_commitment_tx, &self.secp_ctx) .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0) } @@ -4681,13 +4696,13 @@ impl Channel { } let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?; + let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger); if !self.is_outbound() { // Check that we won't violate the remote channel reserve by adding this HTLC. - let counterparty_balance_msat = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger).5; let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered); let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(htlc_candidate, None); let holder_selected_chan_reserve_msat = Channel::::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis) * 1000; - if counterparty_balance_msat < counterparty_commit_tx_fee_msat + holder_selected_chan_reserve_msat { + if commitment_stats.remote_balance_msat < counterparty_commit_tx_fee_msat + holder_selected_chan_reserve_msat { return Err(ChannelError::Ignore("Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_owned())); } } @@ -4710,7 +4725,7 @@ impl Channel { } } - let holder_balance_msat = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger).4 as u64 - outbound_stats.holding_cell_msat; + let holder_balance_msat = commitment_stats.local_balance_msat - outbound_stats.holding_cell_msat; if holder_balance_msat < amount_msat { return Err(ChannelError::Ignore(format!("Cannot send value that would overdraw remaining funds. Amount: {}, pending value to self {}", amount_msat, holder_balance_msat))); } @@ -4861,9 +4876,8 @@ impl Channel { /// 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 { let counterparty_keys = self.build_remote_transaction_keys()?; - let counterparty_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger); - let feerate_per_kw = counterparty_commitment_tx.1; - let counterparty_commitment_txid = counterparty_commitment_tx.0.trust().txid(); + let commitment_stats = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger); + let counterparty_commitment_txid = commitment_stats.tx.trust().txid(); let (signature, htlc_signatures); #[cfg(any(test, feature = "fuzztarget"))] @@ -4877,7 +4891,7 @@ impl Channel { && 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 { - let actual_fee = self.commit_tx_fee_msat(counterparty_commitment_tx.2); + let actual_fee = self.commit_tx_fee_msat(commitment_stats.num_nondust_htlcs); assert_eq!(actual_fee, info.fee); } } @@ -4885,24 +4899,24 @@ impl Channel { } { - let mut htlcs = Vec::with_capacity(counterparty_commitment_tx.3.len()); - for &(ref htlc, _) in counterparty_commitment_tx.3.iter() { + 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 = self.holder_signer.sign_counterparty_commitment(&counterparty_commitment_tx.0, &self.secp_ctx) + let res = self.holder_signer.sign_counterparty_commitment(&commitment_stats.tx, &self.secp_ctx) .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?; signature = res.0; htlc_signatures = res.1; log_trace!(logger, "Signed remote commitment tx {} (txid {}) with redeemscript {} -> {} in channel {}", - encode::serialize_hex(&counterparty_commitment_tx.0.trust().built_transaction().transaction), + encode::serialize_hex(&commitment_stats.tx.trust().built_transaction().transaction), &counterparty_commitment_txid, encode::serialize_hex(&self.get_funding_redeemscript()), log_bytes!(signature.serialize_compact()[..]), log_bytes!(self.channel_id())); for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(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, feerate_per_kw, self.get_holder_selected_contest_delay(), htlc, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), + encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.feerate_per_kw, self.get_holder_selected_contest_delay(), htlc, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)), encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &counterparty_keys)), log_bytes!(counterparty_keys.broadcaster_htlc_key.serialize()), log_bytes!(htlc_sig.serialize_compact()[..]), log_bytes!(self.channel_id())); @@ -4913,7 +4927,7 @@ impl Channel { channel_id: self.channel_id, signature, htlc_signatures, - }, (counterparty_commitment_txid, counterparty_commitment_tx.3))) + }, (counterparty_commitment_txid, commitment_stats.htlcs_included))) } /// Adds a pending outbound HTLC to this channel, and creates a signed commitment transaction @@ -6147,12 +6161,12 @@ mod tests { $( { $htlc_idx: expr, $counterparty_htlc_sig_hex: expr, $htlc_sig_hex: expr, $htlc_tx_hex: expr } ), * } ) => { { let (commitment_tx, htlcs): (_, Vec) = { - let mut res = chan.build_commitment_transaction(0xffffffffffff - 42, &keys, true, false, &logger); + let mut commitment_stats = chan.build_commitment_transaction(0xffffffffffff - 42, &keys, true, false, &logger); - let htlcs = res.3.drain(..) + let htlcs = commitment_stats.htlcs_included.drain(..) .filter_map(|(htlc, _)| if htlc.transaction_output_index.is_some() { Some(htlc) } else { None }) .collect(); - (res.0, htlcs) + (commitment_stats.tx, htlcs) }; let trusted_tx = commitment_tx.trust(); let unsigned_tx = trusted_tx.built_transaction(); -- 2.39.5