Merge pull request #1072 from TheBlueMatt/2021-09-tighter-max_fee-constant
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Mon, 13 Sep 2021 16:42:36 +0000 (16:42 +0000)
committerGitHub <noreply@github.com>
Mon, 13 Sep 2021 16:42:36 +0000 (16:42 +0000)
Reduce our stated max closing-transaction fee to be the true value

1  2 
lightning/src/ln/channel.rs

index bd97326ac12b7102568ff0e31f124aad62959c0f,767cf130db162f70c3e391eb2365120a89a3734a..8ed7072dd8aafe1ec79871255676084f797cd089
@@@ -8,7 -8,7 +8,7 @@@
  // licenses.
  
  use bitcoin::blockdata::script::{Script,Builder};
 -use bitcoin::blockdata::transaction::{TxIn, TxOut, Transaction, SigHashType};
 +use bitcoin::blockdata::transaction::{Transaction, SigHashType};
  use bitcoin::util::bip143;
  use bitcoin::consensus::encode;
  
@@@ -28,13 -28,14 +28,13 @@@ use ln::msgs
  use ln::msgs::{DecodeError, OptionalField, DataLossProtect};
  use ln::script::ShutdownScript;
  use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
 -use ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor};
 +use ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor, ClosingTransaction};
  use ln::chan_utils;
  use chain::BestBlock;
  use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
  use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER};
  use chain::transaction::{OutPoint, TransactionData};
  use chain::keysinterface::{Sign, KeysInterface};
 -use util::transaction_utils;
  use util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter};
  use util::logger::Logger;
  use util::errors::APIError;
@@@ -421,18 -422,22 +421,18 @@@ pub(super) struct Channel<Signer: Sign
        monitor_pending_forwards: Vec<(PendingHTLCInfo, u64)>,
        monitor_pending_failures: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
  
 -      // pending_update_fee is filled when sending and receiving update_fee
 -      // For outbound channel, feerate_per_kw is updated with the value from
 -      // pending_update_fee when revoke_and_ack is received
 +      // pending_update_fee is filled when sending and receiving update_fee.
        //
 -      // For inbound channel, feerate_per_kw is updated when it receives
 -      // commitment_signed and revoke_and_ack is generated
 -      // The pending value is kept when another pair of update_fee and commitment_signed
 -      // is received during AwaitingRemoteRevoke and relieved when the expected
 -      // revoke_and_ack is received and new commitment_signed is generated to be
 -      // sent to the funder. Otherwise, the pending value is removed when receiving
 -      // commitment_signed.
 +      // Because it follows the same commitment flow as HTLCs, `FeeUpdateState` is either `Outbound`
 +      // or matches a subset of the `InboundHTLCOutput` variants. It is then updated/used when
 +      // generating new commitment transactions with exactly the same criteria as inbound/outbound
 +      // HTLCs with similar state.
        pending_update_fee: Option<(u32, FeeUpdateState)>,
 -      // update_fee() during ChannelState::AwaitingRemoteRevoke is hold in
 -      // holdina_cell_update_fee then moved to pending_udpate_fee when revoke_and_ack
 -      // is received. holding_cell_update_fee is updated when there are additional
 -      // update_fee() during ChannelState::AwaitingRemoteRevoke.
 +      // If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place
 +      // it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
 +      // `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into
 +      // `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by
 +      // further `send_update_fee` calls, dropping the previous holding cell update entirely.
        holding_cell_update_fee: Option<u32>,
        next_holder_htlc_id: u64,
        next_counterparty_htlc_id: u64,
        /// closing_signed message and handling it in `maybe_propose_closing_signed`.
        pending_counterparty_closing_signed: Option<msgs::ClosingSigned>,
  
-       /// The minimum and maximum absolute fee we are willing to place on the closing transaction.
-       /// These are set once we reach `closing_negotiation_ready`.
+       /// The minimum and maximum absolute fee, in satoshis, we are willing to place on the closing
+       /// transaction. These are set once we reach `closing_negotiation_ready`.
        #[cfg(test)]
        pub(crate) closing_fee_limits: Option<(u64, u64)>,
        #[cfg(not(test))]
@@@ -1281,38 -1286,63 +1281,38 @@@ impl<Signer: Sign> Channel<Signer> 
        }
  
        #[inline]
 -      fn build_closing_transaction(&self, proposed_total_fee_satoshis: u64, skip_remote_output: bool) -> (Transaction, u64) {
 -              let txins = {
 -                      let mut ins: Vec<TxIn> = Vec::new();
 -                      ins.push(TxIn {
 -                              previous_output: self.funding_outpoint().into_bitcoin_outpoint(),
 -                              script_sig: Script::new(),
 -                              sequence: 0xffffffff,
 -                              witness: Vec::new(),
 -                      });
 -                      ins
 -              };
 -
 +      fn build_closing_transaction(&self, proposed_total_fee_satoshis: u64, skip_remote_output: bool) -> (ClosingTransaction, u64) {
                assert!(self.pending_inbound_htlcs.is_empty());
                assert!(self.pending_outbound_htlcs.is_empty());
                assert!(self.pending_update_fee.is_none());
 -              let mut txouts: Vec<(TxOut, ())> = Vec::new();
  
                let mut total_fee_satoshis = proposed_total_fee_satoshis;
 -              let value_to_self: i64 = (self.value_to_self_msat as i64) / 1000 - if self.is_outbound() { total_fee_satoshis as i64 } else { 0 };
 -              let value_to_remote: i64 = ((self.channel_value_satoshis * 1000 - self.value_to_self_msat) as i64 / 1000) - if self.is_outbound() { 0 } else { total_fee_satoshis as i64 };
 +              let mut value_to_holder: i64 = (self.value_to_self_msat as i64) / 1000 - if self.is_outbound() { total_fee_satoshis as i64 } else { 0 };
 +              let mut value_to_counterparty: i64 = ((self.channel_value_satoshis * 1000 - self.value_to_self_msat) as i64 / 1000) - if self.is_outbound() { 0 } else { total_fee_satoshis as i64 };
  
 -              if value_to_self < 0 {
 +              if value_to_holder < 0 {
                        assert!(self.is_outbound());
 -                      total_fee_satoshis += (-value_to_self) as u64;
 -              } else if value_to_remote < 0 {
 +                      total_fee_satoshis += (-value_to_holder) as u64;
 +              } else if value_to_counterparty < 0 {
                        assert!(!self.is_outbound());
 -                      total_fee_satoshis += (-value_to_remote) as u64;
 +                      total_fee_satoshis += (-value_to_counterparty) as u64;
                }
  
 -              if !skip_remote_output && value_to_remote as u64 > self.holder_dust_limit_satoshis {
 -                      txouts.push((TxOut {
 -                              script_pubkey: self.counterparty_shutdown_scriptpubkey.clone().unwrap(),
 -                              value: value_to_remote as u64
 -                      }, ()));
 +              if skip_remote_output || value_to_counterparty as u64 <= self.holder_dust_limit_satoshis {
 +                      value_to_counterparty = 0;
                }
  
 -              assert!(self.shutdown_scriptpubkey.is_some());
 -              if value_to_self as u64 > self.holder_dust_limit_satoshis {
 -                      txouts.push((TxOut {
 -                              script_pubkey: self.get_closing_scriptpubkey(),
 -                              value: value_to_self as u64
 -                      }, ()));
 +              if value_to_holder as u64 <= self.holder_dust_limit_satoshis {
 +                      value_to_holder = 0;
                }
  
 -              transaction_utils::sort_outputs(&mut txouts, |_, _| { cmp::Ordering::Equal }); // Ordering doesnt matter if they used our pubkey...
 -
 -              let mut outputs: Vec<TxOut> = Vec::new();
 -              for out in txouts.drain(..) {
 -                      outputs.push(out.0);
 -              }
 +              assert!(self.shutdown_scriptpubkey.is_some());
 +              let holder_shutdown_script = self.get_closing_scriptpubkey();
 +              let counterparty_shutdown_script = self.counterparty_shutdown_scriptpubkey.clone().unwrap();
 +              let funding_outpoint = self.funding_outpoint().into_bitcoin_outpoint();
  
 -              (Transaction {
 -                      version: 2,
 -                      lock_time: 0,
 -                      input: txins,
 -                      output: outputs,
 -              }, total_fee_satoshis)
 +              let closing_transaction = ClosingTransaction::new(value_to_holder as u64, value_to_counterparty as u64, holder_shutdown_script, counterparty_shutdown_script, funding_outpoint);
 +              (closing_transaction, total_fee_satoshis)
        }
  
        fn funding_outpoint(&self) -> OutPoint {
                                cmp::max(normal_feerate as u64 * tx_weight / 1000 + self.config.force_close_avoidance_max_fee_satoshis,
                                        proposed_max_feerate as u64 * tx_weight / 1000)
                        } else {
-                               u64::max_value()
+                               self.channel_value_satoshis - (self.value_to_self_msat + 999) / 1000
                        };
  
                self.closing_fee_limits = Some((proposed_total_fee_satoshis, proposed_max_total_fee_satoshis));
                Ok((shutdown, monitor_update, dropped_outbound_htlcs))
        }
  
 -      fn build_signed_closing_transaction(&self, tx: &mut Transaction, counterparty_sig: &Signature, sig: &Signature) {
 -              if tx.input.len() != 1 { panic!("Tried to sign closing transaction that had input count != 1!"); }
 -              if tx.input[0].witness.len() != 0 { panic!("Tried to re-sign closing transaction"); }
 -              if tx.output.len() > 2 { panic!("Tried to sign bogus closing transaction"); }
 +      fn build_signed_closing_transaction(&self, closing_tx: &ClosingTransaction, counterparty_sig: &Signature, sig: &Signature) -> Transaction {
 +              let mut tx = closing_tx.trust().built_transaction().clone();
  
                tx.input[0].witness.push(Vec::new()); // First is the multisig dummy
  
                tx.input[0].witness[2].push(SigHashType::All as u8);
  
                tx.input[0].witness.push(self.get_funding_redeemscript().into_bytes());
 +              tx
        }
  
        pub fn closing_signed<F: Deref>(&mut self, fee_estimator: &F, msg: &msgs::ClosingSigned) -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>), ChannelError>
                if used_total_fee != msg.fee_satoshis {
                        return Err(ChannelError::Close(format!("Remote sent us a closing_signed with a fee other than the value they can claim. Fee in message: {}. Actual closing tx fee: {}", msg.fee_satoshis, used_total_fee)));
                }
 -              let mut sighash = hash_to_message!(&bip143::SigHashCache::new(&closing_tx).signature_hash(0, &funding_redeemscript, self.channel_value_satoshis, SigHashType::All)[..]);
 +              let sighash = closing_tx.trust().get_sighash_all(&funding_redeemscript, self.channel_value_satoshis);
  
                match self.secp_ctx.verify(&sighash, &msg.signature, &self.get_counterparty_pubkeys().funding_pubkey) {
                        Ok(_) => {},
                                // The remote end may have decided to revoke their output due to inconsistent dust
                                // limits, so check for that case by re-checking the signature here.
                                closing_tx = self.build_closing_transaction(msg.fee_satoshis, true).0;
 -                              sighash = hash_to_message!(&bip143::SigHashCache::new(&closing_tx).signature_hash(0, &funding_redeemscript, self.channel_value_satoshis, SigHashType::All)[..]);
 +                              let sighash = closing_tx.trust().get_sighash_all(&funding_redeemscript, self.channel_value_satoshis);
                                secp_check!(self.secp_ctx.verify(&sighash, &msg.signature, self.counterparty_funding_pubkey()), "Invalid closing tx signature from peer".to_owned());
                        },
                };
                assert!(self.shutdown_scriptpubkey.is_some());
                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);
 +                              let tx = self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig);
                                self.channel_state = ChannelState::ShutdownComplete as u32;
                                self.update_time_counter += 1;
 -                              return Ok((None, Some(closing_tx)));
 +                              return Ok((None, Some(tx)));
                        }
                }
  
  
                macro_rules! propose_fee {
                        ($new_fee: expr) => {
 -                              let (mut tx, used_fee) = if $new_fee == msg.fee_satoshis {
 +                              let (closing_tx, used_fee) = if $new_fee == msg.fee_satoshis {
                                        (closing_tx, $new_fee)
                                } else {
                                        self.build_closing_transaction($new_fee, false)
                                };
  
                                let sig = self.holder_signer
 -                                      .sign_closing_transaction(&tx, &self.secp_ctx)
 +                                      .sign_closing_transaction(&closing_tx, &self.secp_ctx)
                                        .map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?;
  
                                let signed_tx = if $new_fee == msg.fee_satoshis {
                                        self.channel_state = ChannelState::ShutdownComplete as u32;
                                        self.update_time_counter += 1;
 -                                      self.build_signed_closing_transaction(&mut tx, &msg.signature, &sig);
 +                                      let tx = self.build_signed_closing_transaction(&closing_tx, &msg.signature, &sig);
                                        Some(tx)
                                } else { None };
  
  
                        if !self.is_outbound() {
                                // They have to pay, so pick the highest fee in the overlapping range.
-                               debug_assert_eq!(our_max_fee, u64::max_value()); // We should never set an upper bound
+                               // We should never set an upper bound aside from their full balance
+                               debug_assert_eq!(our_max_fee, self.channel_value_satoshis - (self.value_to_self_msat + 999) / 1000);
                                propose_fee!(cmp::min(max_fee_satoshis, our_max_fee));
                        } else {
                                if msg.fee_satoshis < our_min_fee || msg.fee_satoshis > our_max_fee {
                // more dust balance if the feerate increases when we have several HTLCs pending
                // which are near the dust limit.
                let mut feerate_per_kw = self.feerate_per_kw;
 +              // If there's a pending update fee, use it to ensure we aren't under-estimating
 +              // potential feerate updates coming soon.
                if let Some((feerate, _)) = self.pending_update_fee {
                        feerate_per_kw = cmp::max(feerate_per_kw, feerate);
                }
@@@ -5095,10 -5125,9 +5096,10 @@@ impl<Signer: Sign> Writeable for Channe
                if self.is_outbound() {
                        self.pending_update_fee.map(|(a, _)| a).write(writer)?;
                } else if let Some((feerate, FeeUpdateState::AwaitingRemoteRevokeToAnnounce)) = self.pending_update_fee {
 -                      // As for inbound HTLCs, if the update was only announced and never committed, drop it.
                        Some(feerate).write(writer)?;
                } else {
 +                      // As for inbound HTLCs, if the update was only announced and never committed in a
 +                      // commitment_signed, drop it.
                        None::<u32>.write(writer)?;
                }
                self.holding_cell_update_fee.write(writer)?;