From b43a7e3ef3e88012cf9d6dfd0fa8362a32474fb4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 18 Sep 2020 18:26:12 -0400 Subject: [PATCH] Fix feerate calculation on closing transactions This resolves a number of bugs around how we calculate feerates on closing transactions: * We previously calculated the weight wrong both by always counting two outputs instead of counting the number of outputs that actually exist in the closing transaction and by not counting the witness redeemscript. * We use assertions to check the calculated weight matches what we actually build (with debug_assertions for variable-length sigs). * As an additional sanity check, we really should check that the transaction had at least min-relay-fee when we were the channel initator. --- lightning/src/ln/channel.rs | 59 ++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 14 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 407c9adb..934f155d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1054,8 +1054,30 @@ impl Channel { } #[inline] - fn get_closing_transaction_weight(a_scriptpubkey: &Script, b_scriptpubkey: &Script) -> u64 { - (4 + 1 + 36 + 4 + 1 + 1 + 2*(8+1) + 4 + a_scriptpubkey.len() as u64 + b_scriptpubkey.len() as u64)*4 + 2 + 1 + 1 + 2*(1 + 72) + fn get_closing_transaction_weight(&self, a_scriptpubkey: Option<&Script>, b_scriptpubkey: Option<&Script>) -> u64 { + let mut ret = + (4 + // version + 1 + // input count + 36 + // prevout + 1 + // script length (0) + 4 + // sequence + 1 + // output count + 4 // lock time + )*4 + // * 4 for non-witness parts + 2 + // witness marker and flag + 1 + // witness element count + 4 + // 4 element lengths (2 sigs, multisig dummy, and witness script) + self.get_funding_redeemscript().len() as u64 + // funding witness script + 2*(1 + 71); // two signatures + sighash type flags + if let Some(spk) = a_scriptpubkey { + ret += ((8+1) + // output values and script length + spk.len() as u64) * 4; // scriptpubkey and witness multiplier + } + if let Some(spk) = b_scriptpubkey { + ret += ((8+1) + // output values and script length + spk.len() as u64) * 4; // scriptpubkey and witness multiplier + } + ret } #[inline] @@ -2880,13 +2902,14 @@ impl Channel { if self.feerate_per_kw > proposed_feerate { proposed_feerate = self.feerate_per_kw; } - let tx_weight = Self::get_closing_transaction_weight(&self.get_closing_scriptpubkey(), self.counterparty_shutdown_scriptpubkey.as_ref().unwrap()); + let tx_weight = self.get_closing_transaction_weight(Some(&self.get_closing_scriptpubkey()), Some(self.counterparty_shutdown_scriptpubkey.as_ref().unwrap())); let proposed_total_fee_satoshis = proposed_feerate as u64 * tx_weight / 1000; let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(proposed_total_fee_satoshis, false); let sig = self.holder_keys .sign_closing_transaction(&closing_tx, &self.secp_ctx) .ok(); + assert!(closing_tx.get_weight() as u64 <= tx_weight); if sig.is_none() { return None; } self.last_sent_closing_fee = Some((proposed_feerate, total_fee_satoshis, sig.clone().unwrap())); @@ -3031,9 +3054,14 @@ impl Channel { }, }; + let closing_tx_max_weight = self.get_closing_transaction_weight( + if let Some(oup) = closing_tx.output.get(0) { Some(&oup.script_pubkey) } else { None }, + if let Some(oup) = closing_tx.output.get(1) { Some(&oup.script_pubkey) } else { None }); if let Some((_, last_fee, sig)) = self.last_sent_closing_fee { if last_fee == msg.fee_satoshis { self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig); + assert!(closing_tx.get_weight() as u64 <= closing_tx_max_weight); + debug_assert!(closing_tx.get_weight() as u64 >= closing_tx_max_weight - 2); self.channel_state = ChannelState::ShutdownComplete as u32; self.update_time_counter += 1; return Ok((None, Some(closing_tx))); @@ -3042,11 +3070,12 @@ impl Channel { macro_rules! propose_new_feerate { ($new_feerate: expr) => { - let closing_tx_max_weight = Self::get_closing_transaction_weight(&self.get_closing_scriptpubkey(), self.counterparty_shutdown_scriptpubkey.as_ref().unwrap()); - let (closing_tx, used_total_fee) = self.build_closing_transaction($new_feerate as u64 * closing_tx_max_weight / 1000, false); + let tx_weight = self.get_closing_transaction_weight(Some(&self.get_closing_scriptpubkey()), Some(self.counterparty_shutdown_scriptpubkey.as_ref().unwrap())); + let (closing_tx, used_total_fee) = self.build_closing_transaction($new_feerate as u64 * tx_weight / 1000, false); let sig = self.holder_keys .sign_closing_transaction(&closing_tx, &self.secp_ctx) .map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?; + assert!(closing_tx.get_weight() as u64 <= tx_weight); self.last_sent_closing_fee = Some(($new_feerate, used_total_fee, sig.clone())); return Ok((Some(msgs::ClosingSigned { channel_id: self.channel_id, @@ -3056,10 +3085,10 @@ impl Channel { } } - let proposed_sat_per_kw = msg.fee_satoshis * 1000 / closing_tx.get_weight() as u64; + let mut min_feerate = 253; if self.channel_outbound { let max_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal); - if (proposed_sat_per_kw as u32) > max_feerate { + if (msg.fee_satoshis as u64) > max_feerate as u64 * closing_tx_max_weight / 1000 { if let Some((last_feerate, _, _)) = self.last_sent_closing_fee { if max_feerate <= last_feerate { return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something higher ({}) than our Normal feerate ({})", last_feerate, max_feerate))); @@ -3068,21 +3097,23 @@ impl Channel { propose_new_feerate!(max_feerate); } } else { - let min_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background); - if (proposed_sat_per_kw as u32) < min_feerate { - if let Some((last_feerate, _, _)) = self.last_sent_closing_fee { - if min_feerate >= last_feerate { - return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something lower ({}) than our Background feerate ({}).", last_feerate, min_feerate))); - } + min_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background); + } + if (msg.fee_satoshis as u64) < min_feerate as u64 * closing_tx_max_weight / 1000 { + if let Some((last_feerate, _, _)) = self.last_sent_closing_fee { + if min_feerate >= last_feerate { + return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something lower ({}) than our Background feerate ({}).", last_feerate, min_feerate))); } - propose_new_feerate!(min_feerate); } + propose_new_feerate!(min_feerate); } let sig = self.holder_keys .sign_closing_transaction(&closing_tx, &self.secp_ctx) .map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?; self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig); + assert!(closing_tx.get_weight() as u64 <= closing_tx_max_weight); + debug_assert!(closing_tx.get_weight() as u64 >= closing_tx_max_weight - 2); self.channel_state = ChannelState::ShutdownComplete as u32; self.update_time_counter += 1; -- 2.30.2