Merge pull request #1047 from TheBlueMatt/2021-08-985-followups
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Thu, 9 Sep 2021 09:23:08 +0000 (09:23 +0000)
committerGitHub <noreply@github.com>
Thu, 9 Sep 2021 09:23:08 +0000 (09:23 +0000)
1  2 
lightning/src/ln/channel.rs

index 0481a93fcd97e6fc566436dbbad9fad95f3d871c,f8cbc26a2b73a984fb2be03d2633b75c4c431498..58384f3855e6ceddd7a5a4da646a5e1454e0c45c
@@@ -255,6 -255,8 +255,6 @@@ enum ChannelState 
        RemoteShutdownSent = 1 << 10,
        /// Flag which is set on ChannelFunded or FundingSent after sending a shutdown message. At this
        /// point, we may not add any new HTLCs to the channel.
 -      /// TODO: Investigate some kind of timeout mechanism by which point the remote end must provide
 -      /// us their shutdown.
        LocalShutdownSent = 1 << 11,
        /// We've successfully negotiated a closing_signed dance. At this point ChannelManager is about
        /// to drop us, but we store this anyway.
@@@ -422,22 -424,18 +422,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,
        /// Max to_local and to_remote outputs in a remote-generated commitment transaction
        counterparty_max_commitment_tx_output: Mutex<(u64, u64)>,
  
 -      last_sent_closing_fee: Option<(u32, u64, Signature)>, // (feerate, fee, holder_sig)
 +      last_sent_closing_fee: Option<(u64, Signature)>, // (fee, holder_sig)
 +      target_closing_feerate_sats_per_kw: Option<u32>,
 +
 +      /// If our counterparty sent us a closing_signed while we were waiting for a `ChannelMonitor`
 +      /// update, we need to delay processing it until later. We do that here by simply storing the
 +      /// 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`.
 +      #[cfg(test)]
 +      pub(crate) closing_fee_limits: Option<(u64, u64)>,
 +      #[cfg(not(test))]
 +      closing_fee_limits: Option<(u64, u64)>,
  
        /// The hash of the block in which the funding transaction was included.
        funding_tx_confirmed_in: Option<BlockHash>,
        commitment_secrets: CounterpartyCommitmentSecrets,
  
        channel_update_status: ChannelUpdateStatus,
 +      /// Once we reach `closing_negotiation_ready`, we set this, indicating if closing_signed does
 +      /// not complete within a single timer tick (one minute), we should force-close the channel.
 +      /// This prevents us from keeping unusable channels around forever if our counterparty wishes
 +      /// to DoS us.
 +      /// Note that this field is reset to false on deserialization to give us a chance to connect to
 +      /// our peer and start the closing_signed negotiation fresh.
 +      closing_signed_in_flight: bool,
  
        /// Our counterparty's channel_announcement signatures provided in announcement_signatures.
        /// This can be used to rebroadcast the channel_announcement message later.
@@@ -567,9 -545,7 +563,9 @@@ const COMMITMENT_TX_WEIGHT_PER_HTLC: u6
  #[cfg(test)]
  pub const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172;
  
 -/// Maximmum `funding_satoshis` value, according to the BOLT #2 specification
 +pub const ANCHOR_OUTPUT_VALUE_SATOSHI: u64 = 330;
 +
 +/// Maximum `funding_satoshis` value, according to the BOLT #2 specification
  /// it's 2^24.
  pub const MAX_FUNDING_SATOSHIS: u64 = 1 << 24;
  
@@@ -594,7 -570,6 +590,7 @@@ pub const MIN_DUST_LIMIT_SATOSHIS: u64 
  /// channel_id in ChannelManager.
  pub(super) enum ChannelError {
        Ignore(String),
 +      Warn(String),
        Close(String),
        CloseDelayBroadcast(String),
  }
@@@ -603,7 -578,6 +599,7 @@@ impl fmt::Debug for ChannelError 
        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
                match self {
                        &ChannelError::Ignore(ref e) => write!(f, "Ignore : {}", e),
 +                      &ChannelError::Warn(ref e) => write!(f, "Warn : {}", e),
                        &ChannelError::Close(ref e) => write!(f, "Close : {}", e),
                        &ChannelError::CloseDelayBroadcast(ref e) => write!(f, "CloseDelayBroadcast : {}", e)
                }
@@@ -715,9 -689,6 +711,9 @@@ impl<Signer: Sign> Channel<Signer> 
                        counterparty_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
  
                        last_sent_closing_fee: None,
 +                      pending_counterparty_closing_signed: None,
 +                      closing_fee_limits: None,
 +                      target_closing_feerate_sats_per_kw: None,
  
                        funding_tx_confirmed_in: None,
                        funding_tx_confirmation_height: 0,
                        commitment_secrets: CounterpartyCommitmentSecrets::new(),
  
                        channel_update_status: ChannelUpdateStatus::Enabled,
 +                      closing_signed_in_flight: false,
  
                        announcement_sigs: None,
  
                        counterparty_max_commitment_tx_output: Mutex::new((msg.push_msat, msg.funding_satoshis * 1000 - msg.push_msat)),
  
                        last_sent_closing_fee: None,
 +                      pending_counterparty_closing_signed: None,
 +                      closing_fee_limits: None,
 +                      target_closing_feerate_sats_per_kw: None,
  
                        funding_tx_confirmed_in: None,
                        funding_tx_confirmation_height: 0,
                        commitment_secrets: CounterpartyCommitmentSecrets::new(),
  
                        channel_update_status: ChannelUpdateStatus::Enabled,
 +                      closing_signed_in_flight: false,
  
                        announcement_sigs: None,
  
  
                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 {
 +                      (self.get_holder_pubkeys().funding_pubkey, self.get_counterparty_pubkeys().funding_pubkey)
 +              } else {
 +                      (self.get_counterparty_pubkeys().funding_pubkey, self.get_holder_pubkeys().funding_pubkey)
 +              };
  
                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);
                let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
                                                                             value_to_a as u64,
                                                                             value_to_b as u64,
 +                                                                           false,
 +                                                                           funding_pubkey_a,
 +                                                                           funding_pubkey_b,
                                                                             keys.clone(),
                                                                             feerate_per_kw,
                                                                             &mut included_non_dust_htlcs,
                        self.counterparty_funding_pubkey()
                );
  
 +              self.holder_signer.validate_holder_commitment(&holder_commitment_tx)
 +                      .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?;
 +
                // Now that we're past error-generating stuff, update our local state:
  
                let funding_redeemscript = self.get_funding_redeemscript();
                        self.counterparty_funding_pubkey()
                );
  
 +              self.holder_signer.validate_holder_commitment(&holder_commitment_tx)
 +                      .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?;
 +
  
                let funding_redeemscript = self.get_funding_redeemscript();
                let funding_txo = self.get_funding_txo().unwrap();
                Ok(())
        }
  
 -      pub fn commitment_signed<F: Deref, L: Deref>(&mut self, msg: &msgs::CommitmentSigned, fee_estimator: &F, logger: &L) -> Result<(msgs::RevokeAndACK, Option<msgs::CommitmentSigned>, Option<msgs::ClosingSigned>, ChannelMonitorUpdate), (Option<ChannelMonitorUpdate>, ChannelError)>
 -      where F::Target: FeeEstimator,
 -                              L::Target: Logger
 +      pub fn commitment_signed<L: Deref>(&mut self, msg: &msgs::CommitmentSigned, logger: &L) -> Result<(msgs::RevokeAndACK, Option<msgs::CommitmentSigned>, ChannelMonitorUpdate), (Option<ChannelMonitorUpdate>, ChannelError)>
 +              where L::Target: Logger
        {
                if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) {
                        return Err((None, ChannelError::Close("Got commitment signed message when channel was not in an operational state".to_owned())));
                );
  
                let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number - 1, &self.secp_ctx);
 +              self.holder_signer.validate_holder_commitment(&holder_commitment_tx)
 +                      .map_err(|_| (None, ChannelError::Close("Failed to validate our commitment".to_owned())))?;
                let per_commitment_secret = self.holder_signer.release_commitment_secret(self.cur_holder_commitment_transaction_number + 1);
  
                // Update state now that we've passed all the can-fail calls...
                        }
                        log_debug!(logger, "Received valid commitment_signed from peer in channel {}, updated HTLC state but awaiting a monitor update resolution to reply.",
                                log_bytes!(self.channel_id));
 -                      // TODO: Call maybe_propose_first_closing_signed on restoration (or call it here and
 -                      // re-send the message on restoration)
                        return Err((Some(monitor_update), ChannelError::Ignore("Previous monitor update failure prevented generation of RAA".to_owned())));
                }
  
 -              let (commitment_signed, closing_signed) = if need_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
 +              let commitment_signed = if need_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
                        // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok -
                        // we'll send one right away when we get the revoke_and_ack when we
                        // free_holding_cell_htlcs().
                        // strictly increasing by one, so decrement it here.
                        self.latest_monitor_update_id = monitor_update.update_id;
                        monitor_update.updates.append(&mut additional_update.updates);
 -                      (Some(msg), None)
 -              } else if !need_commitment {
 -                      (None, self.maybe_propose_first_closing_signed(fee_estimator))
 -              } else { (None, None) };
 +                      Some(msg)
 +              } else { None };
  
                log_debug!(logger, "Received valid commitment_signed from peer in channel {}, updating HTLC state and responding with{} a revoke_and_ack.",
                        log_bytes!(self.channel_id()), if commitment_signed.is_some() { " our own commitment_signed and" } else { "" });
                        channel_id: self.channel_id,
                        per_commitment_secret,
                        next_per_commitment_point,
 -              }, commitment_signed, closing_signed, monitor_update))
 +              }, commitment_signed, monitor_update))
        }
  
        /// Public version of the below, checking relevant preconditions first.
        /// waiting on this revoke_and_ack. The generation of this new commitment_signed may also fail,
        /// generating an appropriate error *after* the channel state has been updated based on the
        /// revoke_and_ack message.
 -      pub fn revoke_and_ack<F: Deref, L: Deref>(&mut self, msg: &msgs::RevokeAndACK, fee_estimator: &F, logger: &L) -> Result<(Option<msgs::CommitmentUpdate>, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Option<msgs::ClosingSigned>, ChannelMonitorUpdate, Vec<(HTLCSource, PaymentHash)>), ChannelError>
 -              where F::Target: FeeEstimator,
 -                                      L::Target: Logger,
 +      pub fn revoke_and_ack<L: Deref>(&mut self, msg: &msgs::RevokeAndACK, logger: &L) -> Result<(Option<msgs::CommitmentUpdate>, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, ChannelMonitorUpdate, Vec<(HTLCSource, PaymentHash)>), ChannelError>
 +              where L::Target: Logger,
        {
                if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) {
                        return Err(ChannelError::Close("Got revoke/ACK message when channel was not in an operational state".to_owned()));
                        return Err(ChannelError::Close("Peer sent revoke_and_ack after we'd started exchanging closing_signeds".to_owned()));
                }
  
 +              let secret = secp_check!(SecretKey::from_slice(&msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret".to_owned());
 +
                if let Some(counterparty_prev_commitment_point) = self.counterparty_prev_commitment_point {
 -                      if PublicKey::from_secret_key(&self.secp_ctx, &secp_check!(SecretKey::from_slice(&msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret".to_owned())) != counterparty_prev_commitment_point {
 +                      if PublicKey::from_secret_key(&self.secp_ctx, &secret) != counterparty_prev_commitment_point {
                                return Err(ChannelError::Close("Got a revoke commitment secret which didn't correspond to their current pubkey".to_owned()));
                        }
                }
                        *self.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None;
                }
  
 +              self.holder_signer.validate_counterparty_revocation(
 +                      self.cur_counterparty_commitment_transaction_number + 1,
 +                      &secret
 +              ).map_err(|_| ChannelError::Close("Failed to validate revocation from peer".to_owned()))?;
 +
                self.commitment_secrets.provide_secret(self.cur_counterparty_commitment_transaction_number + 1, msg.per_commitment_secret)
                        .map_err(|_| ChannelError::Close("Previous secrets did not match new one".to_owned()))?;
                self.latest_monitor_update_id += 1;
                        self.monitor_pending_forwards.append(&mut to_forward_infos);
                        self.monitor_pending_failures.append(&mut revoked_htlcs);
                        log_debug!(logger, "Received a valid revoke_and_ack for channel {} but awaiting a monitor update resolution to reply.", log_bytes!(self.channel_id()));
 -                      return Ok((None, Vec::new(), Vec::new(), None, monitor_update, Vec::new()))
 +                      return Ok((None, Vec::new(), Vec::new(), monitor_update, Vec::new()))
                }
  
                match self.free_holding_cell_htlcs(logger)? {
                                self.latest_monitor_update_id = monitor_update.update_id;
                                monitor_update.updates.append(&mut additional_update.updates);
  
 -                              Ok((Some(commitment_update), to_forward_infos, revoked_htlcs, None, monitor_update, htlcs_to_fail))
 +                              Ok((Some(commitment_update), to_forward_infos, revoked_htlcs, monitor_update, htlcs_to_fail))
                        },
                        (None, htlcs_to_fail) => {
                                if require_commitment {
                                                update_fail_malformed_htlcs,
                                                update_fee: None,
                                                commitment_signed
 -                                      }), to_forward_infos, revoked_htlcs, None, monitor_update, htlcs_to_fail))
 +                                      }), to_forward_infos, revoked_htlcs, monitor_update, htlcs_to_fail))
                                } else {
                                        log_debug!(logger, "Received a valid revoke_and_ack for channel {} with no reply necessary.", log_bytes!(self.channel_id()));
 -                                      Ok((None, to_forward_infos, revoked_htlcs, self.maybe_propose_first_closing_signed(fee_estimator), monitor_update, htlcs_to_fail))
 +                                      Ok((None, to_forward_infos, revoked_htlcs, monitor_update, htlcs_to_fail))
                                }
                        }
                }
 -
        }
  
        /// Adds a pending update to this channel. See the doc for send_htlc for
                // Upon reconnect we have to start the closing_signed dance over, but shutdown messages
                // will be retransmitted.
                self.last_sent_closing_fee = None;
 +              self.pending_counterparty_closing_signed = None;
 +              self.closing_fee_limits = None;
  
                let mut inbound_drop_count = 0;
                self.pending_inbound_htlcs.retain(|htlc| {
                                // now!
                                match self.free_holding_cell_htlcs(logger) {
                                        Err(ChannelError::Close(msg)) => return Err(ChannelError::Close(msg)),
 -                                      Err(ChannelError::Ignore(_)) | Err(ChannelError::CloseDelayBroadcast(_)) => panic!("Got non-channel-failing result from free_holding_cell_htlcs"),
 +                                      Err(ChannelError::Warn(_)) | Err(ChannelError::Ignore(_)) | Err(ChannelError::CloseDelayBroadcast(_)) =>
 +                                              panic!("Got non-channel-failing result from free_holding_cell_htlcs"),
                                        Ok((Some((commitment_update, monitor_update)), htlcs_to_fail)) => {
                                                return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(monitor_update), self.resend_order.clone(), htlcs_to_fail, shutdown_msg));
                                        },
                }
        }
  
 -      fn maybe_propose_first_closing_signed<F: Deref>(&mut self, fee_estimator: &F) -> Option<msgs::ClosingSigned>
 +      /// Calculates and returns our minimum and maximum closing transaction fee amounts, in whole
 +      /// satoshis. The amounts remain consistent unless a peer disconnects/reconnects or we restart,
 +      /// at which point they will be recalculated.
 +      fn calculate_closing_fee_limits<F: Deref>(&mut self, fee_estimator: &F) -> (u64, u64)
                where F::Target: FeeEstimator
        {
 -              if !self.is_outbound() || !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() ||
 -                              self.channel_state & (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32) != BOTH_SIDES_SHUTDOWN_MASK ||
 -                              self.last_sent_closing_fee.is_some() || self.pending_update_fee.is_some() {
 -                      return None;
 -              }
 +              if let Some((min, max)) = self.closing_fee_limits { return (min, max); }
  
 +              // Propose a range from our current Background feerate to our Normal feerate plus our
 +              // force_close_avoidance_max_fee_satoshis.
 +              // If we fail to come to consensus, we'll have to force-close.
                let mut proposed_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
 -              if self.feerate_per_kw > proposed_feerate {
 -                      proposed_feerate = self.feerate_per_kw;
 -              }
 -              assert!(self.shutdown_scriptpubkey.is_some());
 +              let normal_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
 +              let mut proposed_max_feerate = if self.is_outbound() { normal_feerate } else { u32::max_value() };
 +
 +              // The spec requires that (when the channel does not have anchors) we only send absolute
 +              // channel fees no greater than the absolute channel fee on the current commitment
 +              // transaction. It's unclear *which* commitment transaction this refers to, and there isn't
 +              // very good reason to apply such a limit in any case. We don't bother doing so, risking
 +              // some force-closure by old nodes, but we wanted to close the channel anyway.
 +
 +              if let Some(target_feerate) = self.target_closing_feerate_sats_per_kw {
 +                      let min_feerate = if self.is_outbound() { target_feerate } else { cmp::min(self.feerate_per_kw, target_feerate) };
 +                      proposed_feerate = cmp::max(proposed_feerate, min_feerate);
 +                      proposed_max_feerate = cmp::max(proposed_max_feerate, min_feerate);
 +              }
 +
 +              // Note that technically we could end up with a lower minimum fee if one sides' balance is
 +              // below our dust limit, causing the output to disappear. We don't bother handling this
 +              // case, however, as this should only happen if a channel is closed before any (material)
 +              // payments have been made on it. This may cause slight fee overpayment and/or failure to
 +              // come to consensus with our counterparty on appropriate fees, however it should be a
 +              // relatively rare case. We can revisit this later, though note that in order to determine
 +              // if the funders' output is dust we have to know the absolute fee we're going to use.
                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 proposed_max_total_fee_satoshis = if self.is_outbound() {
 +                              // We always add force_close_avoidance_max_fee_satoshis to our normal
 +                              // feerate-calculated fee, but allow the max to be overridden if we're using a
 +                              // target feerate-calculated fee.
 +                              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.closing_fee_limits = Some((proposed_total_fee_satoshis, proposed_max_total_fee_satoshis));
 +              self.closing_fee_limits.clone().unwrap()
 +      }
 +
 +      /// Returns true if we're ready to commence the closing_signed negotiation phase. This is true
 +      /// after both sides have exchanged a `shutdown` message and all HTLCs have been drained. At
 +      /// this point if we're the funder we should send the initial closing_signed, and in any case
 +      /// shutdown should complete within a reasonable timeframe.
 +      fn closing_negotiation_ready(&self) -> bool {
 +              self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() &&
 +                      self.channel_state &
 +                              (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32 |
 +                               ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32)
 +                              == BOTH_SIDES_SHUTDOWN_MASK &&
 +                      self.pending_update_fee.is_none()
 +      }
 +
 +      /// Checks if the closing_signed negotiation is making appropriate progress, possibly returning
 +      /// an Err if no progress is being made and the channel should be force-closed instead.
 +      /// Should be called on a one-minute timer.
 +      pub fn timer_check_closing_negotiation_progress(&mut self) -> Result<(), ChannelError> {
 +              if self.closing_negotiation_ready() {
 +                      if self.closing_signed_in_flight {
 +                              return Err(ChannelError::Close("closing_signed negotiation failed to finish within two timer ticks".to_owned()));
 +                      } else {
 +                              self.closing_signed_in_flight = true;
 +                      }
 +              }
 +              Ok(())
 +      }
 +
 +      pub fn maybe_propose_closing_signed<F: Deref, L: Deref>(&mut self, fee_estimator: &F, logger: &L)
 +              -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>), ChannelError>
 +              where F::Target: FeeEstimator, L::Target: Logger
 +      {
 +              if self.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() {
 +                      return Ok((None, None));
 +              }
 +
 +              if !self.is_outbound() {
 +                      if let Some(msg) = &self.pending_counterparty_closing_signed.take() {
 +                              return self.closing_signed(fee_estimator, &msg);
 +                      }
 +                      return Ok((None, None));
 +              }
 +
 +              let (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator);
 +
 +              assert!(self.shutdown_scriptpubkey.is_some());
 +              let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(our_min_fee, false);
 +              log_trace!(logger, "Proposing initial closing_signed for our counterparty with a fee range of {}-{} sat (with initial proposal {} sats)",
 +                      our_min_fee, our_max_fee, total_fee_satoshis);
  
 -              let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(proposed_total_fee_satoshis, false);
                let sig = self.holder_signer
                        .sign_closing_transaction(&closing_tx, &self.secp_ctx)
 -                      .ok();
 -              assert!(closing_tx.get_weight() as u64 <= tx_weight);
 -              if sig.is_none() { return None; }
 +                      .map_err(|()| ChannelError::Close("Failed to get signature for closing transaction.".to_owned()))?;
  
 -              self.last_sent_closing_fee = Some((proposed_feerate, total_fee_satoshis, sig.clone().unwrap()));
 -              Some(msgs::ClosingSigned {
 +              self.last_sent_closing_fee = Some((total_fee_satoshis, sig.clone()));
 +              Ok((Some(msgs::ClosingSigned {
                        channel_id: self.channel_id,
                        fee_satoshis: total_fee_satoshis,
 -                      signature: sig.unwrap(),
 -              })
 +                      signature: sig,
 +                      fee_range: Some(msgs::ClosingSignedFeeRange {
 +                              min_fee_satoshis: our_min_fee,
 +                              max_fee_satoshis: our_max_fee,
 +                      }),
 +              }), None))
        }
  
 -      pub fn shutdown<F: Deref, K: Deref>(
 -              &mut self, fee_estimator: &F, keys_provider: &K, their_features: &InitFeatures, msg: &msgs::Shutdown
 -      ) -> Result<(Option<msgs::Shutdown>, Option<msgs::ClosingSigned>, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>), ChannelError>
 -      where
 -              F::Target: FeeEstimator,
 -              K::Target: KeysInterface<Signer = Signer>
 +      pub fn shutdown<K: Deref>(
 +              &mut self, keys_provider: &K, their_features: &InitFeatures, msg: &msgs::Shutdown
 +      ) -> Result<(Option<msgs::Shutdown>, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>), ChannelError>
 +      where K::Target: KeysInterface<Signer = Signer>
        {
                if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
                        return Err(ChannelError::Close("Peer sent shutdown when we needed a channel_reestablish".to_owned()));
                self.channel_state |= ChannelState::LocalShutdownSent as u32;
                self.update_time_counter += 1;
  
 -              Ok((shutdown, self.maybe_propose_first_closing_signed(fee_estimator), monitor_update, dropped_outbound_htlcs))
 +              Ok((shutdown, monitor_update, dropped_outbound_htlcs))
        }
  
        fn build_signed_closing_transaction(&self, tx: &mut Transaction, counterparty_sig: &Signature, sig: &Signature) {
                        return Err(ChannelError::Close("Remote tried to send us a closing tx with > 21 million BTC fee".to_owned()));
                }
  
 +              if self.is_outbound() && self.last_sent_closing_fee.is_none() {
 +                      return Err(ChannelError::Close("Remote tried to send a closing_signed when we were supposed to propose the first one".to_owned()));
 +              }
 +
 +              if self.channel_state & ChannelState::MonitorUpdateFailed as u32 != 0 {
 +                      self.pending_counterparty_closing_signed = Some(msg.clone());
 +                      return Ok((None, None));
 +              }
 +
                let funding_redeemscript = self.get_funding_redeemscript();
                let (mut closing_tx, used_total_fee) = self.build_closing_transaction(msg.fee_satoshis, false);
                if used_total_fee != msg.fee_satoshis {
 -                      return Err(ChannelError::Close(format!("Remote sent us a closing_signed with a fee greater than the value they can claim. Fee in message: {}", 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 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 {
 +              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);
 -                              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)));
                        }
                }
  
 -              macro_rules! propose_new_feerate {
 -                      ($new_feerate: expr) => {
 -                              assert!(self.shutdown_scriptpubkey.is_some());
 -                              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 (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator);
 +
 +              macro_rules! propose_fee {
 +                      ($new_fee: expr) => {
 +                              let (mut 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(&closing_tx, &self.secp_ctx)
 +                                      .sign_closing_transaction(&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()));
 +
 +                              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);
 +                                      Some(tx)
 +                              } else { None };
 +
 +                              self.last_sent_closing_fee = Some((used_fee, sig.clone()));
                                return Ok((Some(msgs::ClosingSigned {
                                        channel_id: self.channel_id,
 -                                      fee_satoshis: used_total_fee,
 +                                      fee_satoshis: used_fee,
                                        signature: sig,
 -                              }), None))
 +                                      fee_range: Some(msgs::ClosingSignedFeeRange {
 +                                              min_fee_satoshis: our_min_fee,
 +                                              max_fee_satoshis: our_max_fee,
 +                                      }),
 +                              }), signed_tx))
                        }
                }
  
 -              let mut min_feerate = 253;
 -              if self.is_outbound() {
 -                      let max_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
 -                      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)));
 -                                      }
 +              if let Some(msgs::ClosingSignedFeeRange { min_fee_satoshis, max_fee_satoshis }) = msg.fee_range {
 +                      if msg.fee_satoshis < min_fee_satoshis || msg.fee_satoshis > max_fee_satoshis {
 +                              return Err(ChannelError::Close(format!("Peer sent a bogus closing_signed - suggested fee of {} sat was not in their desired range of {} sat - {} sat", msg.fee_satoshis, min_fee_satoshis, max_fee_satoshis)));
 +                      }
 +                      if max_fee_satoshis < our_min_fee {
 +                              return Err(ChannelError::Warn(format!("Unable to come to consensus about closing feerate, remote's max fee ({} sat) was smaller than our min fee ({} sat)", max_fee_satoshis, our_min_fee)));
 +                      }
 +                      if min_fee_satoshis > our_max_fee {
 +                              return Err(ChannelError::Warn(format!("Unable to come to consensus about closing feerate, remote's min fee ({} sat) was greater than our max fee ({} sat)", min_fee_satoshis, our_max_fee)));
 +                      }
 +
 +                      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
 +                              propose_fee!(cmp::min(max_fee_satoshis, our_max_fee));
 +                      } else {
 +                              if msg.fee_satoshis < our_min_fee || msg.fee_satoshis > our_max_fee {
 +                                      return Err(ChannelError::Close(format!("Peer sent a bogus closing_signed - suggested fee of {} sat was not in our desired range of {} sat - {} sat after we informed them of our range.",
 +                                              msg.fee_satoshis, our_min_fee, our_max_fee)));
                                }
 -                              propose_new_feerate!(max_feerate);
 +                              // The proposed fee is in our acceptable range, accept it and broadcast!
 +                              propose_fee!(msg.fee_satoshis);
                        }
                } else {
 -                      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)));
 +                      // Old fee style negotiation. We don't bother to enforce whether they are complying
 +                      // with the "making progress" requirements, we just comply and hope for the best.
 +                      if let Some((last_fee, _)) = self.last_sent_closing_fee {
 +                              if msg.fee_satoshis > last_fee {
 +                                      if msg.fee_satoshis < our_max_fee {
 +                                              propose_fee!(msg.fee_satoshis);
 +                                      } else if last_fee < our_max_fee {
 +                                              propose_fee!(our_max_fee);
 +                                      } else {
 +                                              return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) higher than our max fee ({} sat)", msg.fee_satoshis, our_max_fee)));
 +                                      }
 +                              } else {
 +                                      if msg.fee_satoshis > our_min_fee {
 +                                              propose_fee!(msg.fee_satoshis);
 +                                      } else if last_fee > our_min_fee {
 +                                              propose_fee!(our_min_fee);
 +                                      } else {
 +                                              return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) lower than our min fee ({} sat)", msg.fee_satoshis, our_min_fee)));
 +                                      }
 +                              }
 +                      } else {
 +                              if msg.fee_satoshis < our_min_fee {
 +                                      propose_fee!(our_min_fee);
 +                              } else if msg.fee_satoshis > our_max_fee {
 +                                      propose_fee!(our_max_fee);
 +                              } else {
 +                                      propose_fee!(msg.fee_satoshis);
                                }
                        }
 -                      propose_new_feerate!(min_feerate);
                }
 -
 -              let sig = self.holder_signer
 -                      .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;
 -
 -              Ok((Some(msgs::ClosingSigned {
 -                      channel_id: self.channel_id,
 -                      fee_satoshis: msg.fee_satoshis,
 -                      signature: sig,
 -              }), Some(closing_tx)))
        }
  
        // Public utilities:
                // 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);
                }
                self.channel_state >= ChannelState::FundingSent as u32
        }
  
 +      /// Returns true if our peer has either initiated or agreed to shut down the channel.
 +      pub fn received_shutdown(&self) -> bool {
 +              (self.channel_state & ChannelState::RemoteShutdownSent as u32) != 0
 +      }
 +
 +      /// Returns true if we either initiated or agreed to shut down the channel.
 +      pub fn sent_shutdown(&self) -> bool {
 +              (self.channel_state & ChannelState::LocalShutdownSent as u32) != 0
 +      }
 +
        /// Returns true if this channel is fully shut down. True here implies that no further actions
        /// may/will be taken on this channel, and thus this object should be freed. Any future changes
        /// will be handled appropriately by the chain monitor.
  
        /// Begins the shutdown process, getting a message for the remote peer and returning all
        /// holding cell HTLCs for payment failure.
 -      pub fn get_shutdown<K: Deref>(&mut self, keys_provider: &K, their_features: &InitFeatures) -> Result<(msgs::Shutdown, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>), APIError>
 +      pub fn get_shutdown<K: Deref>(&mut self, keys_provider: &K, their_features: &InitFeatures, target_feerate_sats_per_kw: Option<u32>)
 +      -> Result<(msgs::Shutdown, Option<ChannelMonitorUpdate>, Vec<(HTLCSource, PaymentHash)>), APIError>
        where K::Target: KeysInterface<Signer = Signer> {
                for htlc in self.pending_outbound_htlcs.iter() {
                        if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
                };
  
                // From here on out, we may not fail!
 +              self.target_closing_feerate_sats_per_kw = target_feerate_sats_per_kw;
                if self.channel_state < ChannelState::FundingSent as u32 {
                        self.channel_state = ChannelState::ShutdownComplete as u32;
                } else {
@@@ -5124,9 -4943,10 +5122,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)?;
                self.update_time_counter.write(writer)?;
                self.feerate_per_kw.write(writer)?;
  
 -              match self.last_sent_closing_fee {
 -                      Some((feerate, fee, sig)) => {
 -                              1u8.write(writer)?;
 -                              feerate.write(writer)?;
 -                              fee.write(writer)?;
 -                              sig.write(writer)?;
 -                      },
 -                      None => 0u8.write(writer)?,
 -              }
 +              // Versions prior to 0.0.100 expected to read the fields of `last_sent_closing_fee` here,
 +              // however we are supposed to restart shutdown fee negotiation on reconnect (and wipe
 +              // `last_send_closing_fee` in `remove_uncommitted_htlcs_and_mark_paused`) so we should never
 +              // consider the stale state on reload.
 +              0u8.write(writer)?;
  
                self.funding_tx_confirmed_in.write(writer)?;
                self.funding_tx_confirmation_height.write(writer)?;
                        (3, self.counterparty_selected_channel_reserve_satoshis, option),
                        (5, self.config, required),
                        (7, self.shutdown_scriptpubkey, option),
 +                      (9, self.target_closing_feerate_sats_per_kw, option),
                });
  
                Ok(())
@@@ -5349,19 -5172,11 +5348,19 @@@ impl<'a, Signer: Sign, K: Deref> Readab
                let update_time_counter = Readable::read(reader)?;
                let feerate_per_kw = Readable::read(reader)?;
  
 -              let last_sent_closing_fee = match <u8 as Readable>::read(reader)? {
 -                      0 => None,
 -                      1 => Some((Readable::read(reader)?, Readable::read(reader)?, Readable::read(reader)?)),
 +              // Versions prior to 0.0.100 expected to read the fields of `last_sent_closing_fee` here,
 +              // however we are supposed to restart shutdown fee negotiation on reconnect (and wipe
 +              // `last_send_closing_fee` in `remove_uncommitted_htlcs_and_mark_paused`) so we should never
 +              // consider the stale state on reload.
 +              match <u8 as Readable>::read(reader)? {
 +                      0 => {},
 +                      1 => {
 +                              let _: u32 = Readable::read(reader)?;
 +                              let _: u64 = Readable::read(reader)?;
 +                              let _: Signature = Readable::read(reader)?;
 +                      },
                        _ => return Err(DecodeError::InvalidValue),
 -              };
 +              }
  
                let funding_tx_confirmed_in = Readable::read(reader)?;
                let funding_tx_confirmation_height = Readable::read(reader)?;
                };
  
                let mut announcement_sigs = None;
 +              let mut target_closing_feerate_sats_per_kw = None;
                read_tlv_fields!(reader, {
                        (0, announcement_sigs, option),
                        (1, minimum_depth, option),
                        (3, counterparty_selected_channel_reserve_satoshis, option),
                        (5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
                        (7, shutdown_scriptpubkey, option),
 +                      (9, target_closing_feerate_sats_per_kw, option),
                });
  
                let mut secp_ctx = Secp256k1::new();
                        #[cfg(debug_assertions)]
                        counterparty_max_commitment_tx_output: Mutex::new((0, 0)),
  
 -                      last_sent_closing_fee,
 +                      last_sent_closing_fee: None,
 +                      pending_counterparty_closing_signed: None,
 +                      closing_fee_limits: None,
 +                      target_closing_feerate_sats_per_kw,
  
                        funding_tx_confirmed_in,
                        funding_tx_confirmation_height,
                        commitment_secrets,
  
                        channel_update_status,
 +                      closing_signed_in_flight: false,
  
                        announcement_sigs,