Merge pull request #2529 from TheBlueMatt/2023-08-shutdown-remove-early-sign
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Tue, 14 Nov 2023 19:09:46 +0000 (19:09 +0000)
committerGitHub <noreply@github.com>
Tue, 14 Nov 2023 19:09:46 +0000 (19:09 +0000)
Don't send init `closing_signed` too early after final HTLC removal

1  2 
lightning/src/ln/channel.rs
lightning/src/ln/shutdown_tests.rs

index 16e8ed3ef69503cb9e9ed56172a6749b3747254b,f367774673e69200a23cf69136e5a8a393905512..87f3c43aab0db738541192c42cc66d830664e780
@@@ -39,7 -39,7 +39,7 @@@ use crate::chain::transaction::{OutPoin
  use crate::sign::{EcdsaChannelSigner, WriteableEcdsaChannelSigner, EntropySource, ChannelSigner, SignerProvider, NodeSigner, Recipient};
  use crate::events::ClosureReason;
  use crate::routing::gossip::NodeId;
 -use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter};
 +use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer};
  use crate::util::logger::Logger;
  use crate::util::errors::APIError;
  use crate::util::config::{UserConfig, ChannelConfig, LegacyChannelConfig, ChannelHandshakeConfig, ChannelHandshakeLimits, MaxDustHTLCExposure};
@@@ -48,7 -48,6 +48,7 @@@ use crate::util::scid_utils::scid_from_
  use crate::io;
  use crate::prelude::*;
  use core::{cmp,mem,fmt};
 +use core::convert::TryInto;
  use core::ops::Deref;
  #[cfg(any(test, fuzzing, debug_assertions))]
  use crate::sync::Mutex;
@@@ -816,6 -815,19 +816,19 @@@ pub(super) struct ChannelContext<SP: De
        #[cfg(not(test))]
        closing_fee_limits: Option<(u64, u64)>,
  
+       /// If we remove an HTLC (or fee update), commit, and receive our counterparty's
+       /// `revoke_and_ack`, we remove all knowledge of said HTLC (or fee update). However, the latest
+       /// local commitment transaction that we can broadcast still contains the HTLC (or old fee)
+       /// until we receive a further `commitment_signed`. Thus we are not eligible for initiating the
+       /// `closing_signed` negotiation if we're expecting a counterparty `commitment_signed`.
+       ///
+       /// To ensure we don't send a `closing_signed` too early, we track this state here, waiting
+       /// until we see a `commitment_signed` before doing so.
+       ///
+       /// We don't bother to persist this - we anticipate this state won't last longer than a few
+       /// milliseconds, so any accidental force-closes here should be exceedingly rare.
+       expecting_peer_commitment_signed: bool,
        /// The hash of the block in which the funding transaction was included.
        funding_tx_confirmed_in: Option<BlockHash>,
        funding_tx_confirmation_height: u32,
@@@ -1196,8 -1208,8 +1209,8 @@@ impl<SP: Deref> ChannelContext<SP> wher
                match self.config.options.max_dust_htlc_exposure {
                        MaxDustHTLCExposure::FeeRateMultiplier(multiplier) => {
                                let feerate_per_kw = fee_estimator.bounded_sat_per_1000_weight(
 -                                      ConfirmationTarget::OnChainSweep);
 -                              feerate_per_kw as u64 * multiplier
 +                                      ConfirmationTarget::OnChainSweep) as u64;
 +                              feerate_per_kw.saturating_mul(multiplier)
                        },
                        MaxDustHTLCExposure::FixedLimitMsat(limit) => limit,
                }
                         context.holder_dust_limit_satoshis       + dust_buffer_feerate * htlc_timeout_tx_weight(context.get_channel_type()) / 1000)
                };
                let on_counterparty_dust_htlc_exposure_msat = inbound_stats.on_counterparty_tx_dust_exposure_msat + outbound_stats.on_counterparty_tx_dust_exposure_msat;
 -              if on_counterparty_dust_htlc_exposure_msat as i64 + htlc_success_dust_limit as i64 * 1000 - 1 > max_dust_htlc_exposure_msat as i64 {
 +              if on_counterparty_dust_htlc_exposure_msat as i64 + htlc_success_dust_limit as i64 * 1000 - 1 > max_dust_htlc_exposure_msat.try_into().unwrap_or(i64::max_value()) {
                        remaining_msat_below_dust_exposure_limit =
                                Some(max_dust_htlc_exposure_msat.saturating_sub(on_counterparty_dust_htlc_exposure_msat));
                        dust_exposure_dust_limit_msat = cmp::max(dust_exposure_dust_limit_msat, htlc_success_dust_limit * 1000);
                }
  
                let on_holder_dust_htlc_exposure_msat = inbound_stats.on_holder_tx_dust_exposure_msat + outbound_stats.on_holder_tx_dust_exposure_msat;
 -              if on_holder_dust_htlc_exposure_msat as i64 + htlc_timeout_dust_limit as i64 * 1000 - 1 > max_dust_htlc_exposure_msat as i64 {
 +              if on_holder_dust_htlc_exposure_msat as i64 + htlc_timeout_dust_limit as i64 * 1000 - 1 > max_dust_htlc_exposure_msat.try_into().unwrap_or(i64::max_value()) {
                        remaining_msat_below_dust_exposure_limit = Some(cmp::min(
                                remaining_msat_below_dust_exposure_limit.unwrap_or(u64::max_value()),
                                max_dust_htlc_exposure_msat.saturating_sub(on_holder_dust_htlc_exposure_msat)));
@@@ -3249,6 -3261,7 +3262,7 @@@ impl<SP: Deref> Channel<SP> wher
                };
  
                self.context.cur_holder_commitment_transaction_number -= 1;
+               self.context.expecting_peer_commitment_signed = false;
                // Note that if we need_commitment & !AwaitingRemoteRevoke we'll call
                // build_commitment_no_status_check() next which will reset this to RAAFirst.
                self.context.resend_order = RAACommitmentOrder::CommitmentFirst;
                        // Take references explicitly so that we can hold multiple references to self.context.
                        let pending_inbound_htlcs: &mut Vec<_> = &mut self.context.pending_inbound_htlcs;
                        let pending_outbound_htlcs: &mut Vec<_> = &mut self.context.pending_outbound_htlcs;
+                       let expecting_peer_commitment_signed = &mut self.context.expecting_peer_commitment_signed;
  
                        // We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug)
                        pending_inbound_htlcs.retain(|htlc| {
                                        if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
                                                value_to_self_msat_diff += htlc.amount_msat as i64;
                                        }
+                                       *expecting_peer_commitment_signed = true;
                                        false
                                } else { true }
                        });
                                if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
                                        log_trace!(logger, " ...promoting outbound LocalAnnounced {} to Committed", &htlc.payment_hash);
                                        htlc.state = OutboundHTLCState::Committed;
+                                       *expecting_peer_commitment_signed = true;
                                }
                                if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
                                        log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", &htlc.payment_hash);
                                        log_trace!(logger, " ...promoting outbound fee update {} to Committed", feerate);
                                        self.context.feerate_per_kw = feerate;
                                        self.context.pending_update_fee = None;
+                                       self.context.expecting_peer_commitment_signed = true;
                                },
                                FeeUpdateState::RemoteAnnounced => { debug_assert!(!self.context.is_outbound()); },
                                FeeUpdateState::AwaitingRemoteRevokeToAnnounce => {
                -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>, Option<ShutdownResult>), ChannelError>
                where F::Target: FeeEstimator, L::Target: Logger
        {
+               // If we're waiting on a monitor persistence, that implies we're also waiting to send some
+               // message to our counterparty (probably a `revoke_and_ack`). In such a case, we shouldn't
+               // initiate `closing_signed` negotiation until we're clear of all pending messages. Note
+               // that closing_negotiation_ready checks this case (as well as a few others).
                if self.context.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() {
                        return Ok((None, None, None));
                }
                        return Ok((None, None, None));
                }
  
+               // If we're waiting on a counterparty `commitment_signed` to clear some updates from our
+               // local commitment transaction, we can't yet initiate `closing_signed` negotiation.
+               if self.context.expecting_peer_commitment_signed {
+                       return Ok((None, None, None));
+               }
                let (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator);
  
                assert!(self.context.shutdown_scriptpubkey.is_some());
@@@ -5877,7 -5904,7 +5905,7 @@@ impl<SP: Deref> OutboundV1Channel<SP> w
        pub fn new<ES: Deref, F: Deref>(
                fee_estimator: &LowerBoundedFeeEstimator<F>, entropy_source: &ES, signer_provider: &SP, counterparty_node_id: PublicKey, their_features: &InitFeatures,
                channel_value_satoshis: u64, push_msat: u64, user_id: u128, config: &UserConfig, current_chain_height: u32,
 -              outbound_scid_alias: u64
 +              outbound_scid_alias: u64, temporary_channel_id: Option<ChannelId>
        ) -> Result<OutboundV1Channel<SP>, APIError>
        where ES::Target: EntropySource,
              F::Target: FeeEstimator
                        Err(_) => return Err(APIError::ChannelUnavailable { err: "Failed to get destination script".to_owned()}),
                };
  
 -              let temporary_channel_id = ChannelId::temporary_from_entropy_source(entropy_source);
 +              let temporary_channel_id = temporary_channel_id.unwrap_or_else(|| ChannelId::temporary_from_entropy_source(entropy_source));
  
                Ok(Self {
                        context: ChannelContext {
  
                                last_sent_closing_fee: None,
                                pending_counterparty_closing_signed: None,
+                               expecting_peer_commitment_signed: false,
                                closing_fee_limits: None,
                                target_closing_feerate_sats_per_kw: None,
  
@@@ -6637,6 -6665,7 +6666,7 @@@ impl<SP: Deref> InboundV1Channel<SP> wh
  
                                last_sent_closing_fee: None,
                                pending_counterparty_closing_signed: None,
+                               expecting_peer_commitment_signed: false,
                                closing_fee_limits: None,
                                target_closing_feerate_sats_per_kw: None,
  
  }
  
  const SERIALIZATION_VERSION: u8 = 3;
 -const MIN_SERIALIZATION_VERSION: u8 = 2;
 +const MIN_SERIALIZATION_VERSION: u8 = 3;
  
  impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
        (0, FailRelay),
@@@ -6973,6 -7002,14 +7003,6 @@@ impl<SP: Deref> Writeable for Channel<S
  
                self.context.latest_monitor_update_id.write(writer)?;
  
 -              let mut key_data = VecWriter(Vec::new());
 -              // TODO (taproot|arik): Introduce serialization distinction for non-ECDSA signers.
 -              self.context.holder_signer.as_ecdsa().expect("Only ECDSA signers may be serialized").write(&mut key_data)?;
 -              assert!(key_data.0.len() < core::usize::MAX);
 -              assert!(key_data.0.len() < core::u32::MAX as usize);
 -              (key_data.0.len() as u32).write(writer)?;
 -              writer.write_all(&key_data.0[..])?;
 -
                // Write out the old serialization for shutdown_pubkey for backwards compatibility, if
                // deserialized from that format.
                match self.context.shutdown_scriptpubkey.as_ref().and_then(|script| script.as_legacy_pubkey()) {
@@@ -7708,6 -7745,7 +7738,7 @@@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> 
  
                                last_sent_closing_fee: None,
                                pending_counterparty_closing_signed: None,
+                               expecting_peer_commitment_signed: false,
                                closing_fee_limits: None,
                                target_closing_feerate_sats_per_kw,
  
@@@ -7894,7 -7932,7 +7925,7 @@@ mod tests 
                let secp_ctx = Secp256k1::new();
                let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
 -              match OutboundV1Channel::<&TestKeysInterface>::new(&LowerBoundedFeeEstimator::new(&TestFeeEstimator { fee_est: 253 }), &&keys_provider, &&keys_provider, node_id, &features, 10000000, 100000, 42, &config, 0, 42) {
 +              match OutboundV1Channel::<&TestKeysInterface>::new(&LowerBoundedFeeEstimator::new(&TestFeeEstimator { fee_est: 253 }), &&keys_provider, &&keys_provider, node_id, &features, 10000000, 100000, 42, &config, 0, 42, None) {
                        Err(APIError::IncompatibleShutdownScript { script }) => {
                                assert_eq!(script.into_inner(), non_v0_segwit_shutdown_script.into_inner());
                        },
  
                let node_a_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
 -              let node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&bounded_fee_estimator, &&keys_provider, &&keys_provider, node_a_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42).unwrap();
 +              let node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&bounded_fee_estimator, &&keys_provider, &&keys_provider, node_a_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None).unwrap();
  
                // Now change the fee so we can check that the fee in the open_channel message is the
                // same as the old fee.
                // Create Node A's channel pointing to Node B's pubkey
                let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
 -              let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42).unwrap();
 +              let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None).unwrap();
  
                // Create Node B's channel by receiving Node A's open_channel message
                // Make sure A's dust limit is as we expect.
  
                let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
 -              let mut chan = OutboundV1Channel::<&TestKeysInterface>::new(&fee_est, &&keys_provider, &&keys_provider, node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42).unwrap();
 +              let mut chan = OutboundV1Channel::<&TestKeysInterface>::new(&fee_est, &&keys_provider, &&keys_provider, node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None).unwrap();
  
                let commitment_tx_fee_0_htlcs = commit_tx_fee_msat(chan.context.feerate_per_kw, 0, chan.context.get_channel_type());
                let commitment_tx_fee_1_htlc = commit_tx_fee_msat(chan.context.feerate_per_kw, 1, chan.context.get_channel_type());
                // Create Node A's channel pointing to Node B's pubkey
                let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
 -              let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42).unwrap();
 +              let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None).unwrap();
  
                // Create Node B's channel by receiving Node A's open_channel message
                let open_channel_msg = node_a_chan.get_open_channel(chain_hash);
                // Test that `OutboundV1Channel::new` creates a channel with the correct value for
                // `holder_max_htlc_value_in_flight_msat`, when configured with a valid percentage value,
                // which is set to the lower bound + 1 (2%) of the `channel_value`.
 -              let chan_1 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_2_percent), 10000000, 100000, 42, &config_2_percent, 0, 42).unwrap();
 +              let chan_1 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_2_percent), 10000000, 100000, 42, &config_2_percent, 0, 42, None).unwrap();
                let chan_1_value_msat = chan_1.context.channel_value_satoshis * 1000;
                assert_eq!(chan_1.context.holder_max_htlc_value_in_flight_msat, (chan_1_value_msat as f64 * 0.02) as u64);
  
                // Test with the upper bound - 1 of valid values (99%).
 -              let chan_2 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_99_percent), 10000000, 100000, 42, &config_99_percent, 0, 42).unwrap();
 +              let chan_2 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_99_percent), 10000000, 100000, 42, &config_99_percent, 0, 42, None).unwrap();
                let chan_2_value_msat = chan_2.context.channel_value_satoshis * 1000;
                assert_eq!(chan_2.context.holder_max_htlc_value_in_flight_msat, (chan_2_value_msat as f64 * 0.99) as u64);
  
  
                // Test that `OutboundV1Channel::new` uses the lower bound of the configurable percentage values (1%)
                // if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a value less than 1.
 -              let chan_5 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_0_percent), 10000000, 100000, 42, &config_0_percent, 0, 42).unwrap();
 +              let chan_5 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_0_percent), 10000000, 100000, 42, &config_0_percent, 0, 42, None).unwrap();
                let chan_5_value_msat = chan_5.context.channel_value_satoshis * 1000;
                assert_eq!(chan_5.context.holder_max_htlc_value_in_flight_msat, (chan_5_value_msat as f64 * 0.01) as u64);
  
                // Test that `OutboundV1Channel::new` uses the upper bound of the configurable percentage values
                // (100%) if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a larger value
                // than 100.
 -              let chan_6 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_101_percent), 10000000, 100000, 42, &config_101_percent, 0, 42).unwrap();
 +              let chan_6 = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&config_101_percent), 10000000, 100000, 42, &config_101_percent, 0, 42, None).unwrap();
                let chan_6_value_msat = chan_6.context.channel_value_satoshis * 1000;
                assert_eq!(chan_6.context.holder_max_htlc_value_in_flight_msat, chan_6_value_msat);
  
  
                let mut outbound_node_config = UserConfig::default();
                outbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (outbound_selected_channel_reserve_perc * 1_000_000.0) as u32;
 -              let chan = OutboundV1Channel::<&TestKeysInterface>::new(&&fee_est, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&outbound_node_config), channel_value_satoshis, 100_000, 42, &outbound_node_config, 0, 42).unwrap();
 +              let chan = OutboundV1Channel::<&TestKeysInterface>::new(&&fee_est, &&keys_provider, &&keys_provider, outbound_node_id, &channelmanager::provided_init_features(&outbound_node_config), channel_value_satoshis, 100_000, 42, &outbound_node_config, 0, 42, None).unwrap();
  
                let expected_outbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.context.channel_value_satoshis as f64 * outbound_selected_channel_reserve_perc) as u64);
                assert_eq!(chan.context.holder_selected_channel_reserve_satoshis, expected_outbound_selected_chan_reserve);
                // Create Node A's channel pointing to Node B's pubkey
                let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
 -              let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42).unwrap();
 +              let mut node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider, node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None).unwrap();
  
                // Create Node B's channel by receiving Node A's open_channel message
                // Make sure A's dust limit is as we expect.
                let counterparty_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let mut config = UserConfig::default();
                config.channel_handshake_config.announced_channel = false;
 -              let mut chan = OutboundV1Channel::<&Keys>::new(&LowerBoundedFeeEstimator::new(&feeest), &&keys_provider, &&keys_provider, counterparty_node_id, &channelmanager::provided_init_features(&config), 10_000_000, 0, 42, &config, 0, 42).unwrap(); // Nothing uses their network key in this test
 +              let mut chan = OutboundV1Channel::<&Keys>::new(&LowerBoundedFeeEstimator::new(&feeest), &&keys_provider, &&keys_provider, counterparty_node_id, &channelmanager::provided_init_features(&config), 10_000_000, 0, 42, &config, 0, 42, None).unwrap(); // Nothing uses their network key in this test
                chan.context.holder_dust_limit_satoshis = 546;
                chan.context.counterparty_selected_channel_reserve_satoshis = Some(0); // Filled in in accept_channel
  
                let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
                let node_a_chan = OutboundV1Channel::<&TestKeysInterface>::new(&feeest, &&keys_provider, &&keys_provider,
 -                      node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42).unwrap();
 +                      node_b_node_id, &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42, None).unwrap();
  
                let mut channel_type_features = ChannelTypeFeatures::only_static_remote_key();
                channel_type_features.set_zero_conf_required();
                let channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
                        &fee_estimator, &&keys_provider, &&keys_provider, node_id_b,
                        &channelmanager::provided_init_features(&UserConfig::default()), 10000000, 100000, 42,
 -                      &config, 0, 42
 +                      &config, 0, 42, None
                ).unwrap();
                assert!(!channel_a.context.channel_type.supports_anchors_zero_fee_htlc_tx());
  
  
                let channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
                        &fee_estimator, &&keys_provider, &&keys_provider, node_id_b,
 -                      &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42
 +                      &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42,
 +                      None
                ).unwrap();
  
                let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
  
                let channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
                        &fee_estimator, &&keys_provider, &&keys_provider, node_id_b,
 -                      &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42
 +                      &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42,
 +                      None
                ).unwrap();
  
                // Set `channel_type` to `None` to force the implicit feature negotiation.
                // B as it's not supported by LDK.
                let channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
                        &fee_estimator, &&keys_provider, &&keys_provider, node_id_b,
 -                      &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42
 +                      &channelmanager::provided_init_features(&config), 10000000, 100000, 42, &config, 0, 42,
 +                      None
                ).unwrap();
  
                let mut open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
                // LDK.
                let mut channel_a = OutboundV1Channel::<&TestKeysInterface>::new(
                        &fee_estimator, &&keys_provider, &&keys_provider, node_id_b, &simple_anchors_init,
 -                      10000000, 100000, 42, &config, 0, 42
 +                      10000000, 100000, 42, &config, 0, 42, None
                ).unwrap();
  
                let open_channel_msg = channel_a.get_open_channel(ChainHash::using_genesis_block(network));
                        &config,
                        0,
                        42,
 +                      None
                ).unwrap();
  
                let open_channel_msg = node_a_chan.get_open_channel(ChainHash::using_genesis_block(network));
index de57e5878ec7d8b43d7f079f1d491dfc72723435,bbfcf4f283cc057102c64b95b29fd4b4d6e83682..664af77d412e646890abd529ca30e6d17ec5e06a
@@@ -10,8 -10,9 +10,9 @@@
  //! Tests of our shutdown and closing_signed negotiation logic.
  
  use crate::sign::{EntropySource, SignerProvider};
+ use crate::chain::ChannelMonitorUpdateStatus;
  use crate::chain::transaction::OutPoint;
- use crate::events::{MessageSendEvent, MessageSendEventsProvider, ClosureReason};
+ use crate::events::{MessageSendEvent, HTLCDestination, MessageSendEventsProvider, ClosureReason};
  use crate::ln::channelmanager::{self, PaymentSendFailure, PaymentId, RecipientOnionFields, ChannelShutdownState, ChannelDetails};
  use crate::routing::router::{PaymentParameters, get_route, RouteParameters};
  use crate::ln::msgs;
@@@ -261,7 -262,7 +262,7 @@@ fn shutdown_on_unfunded_channel() 
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
  
 -      nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 1_000_000, 100_000, 0, None).unwrap();
 +      nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 1_000_000, 100_000, 0, None, None).unwrap();
        let open_chan = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
  
        // Create a dummy P2WPKH script
@@@ -770,7 -771,7 +771,7 @@@ fn test_unsupported_anysegwit_upfront_s
                .into_script();
  
        // Check script when handling an open_channel message
 -      nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
 +      nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None, None).unwrap();
        let mut open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
        open_channel.shutdown_scriptpubkey = Some(anysegwit_shutdown_script.clone());
        nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel);
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
  
        // Check script when handling an accept_channel message
 -      nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
 +      nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None, None).unwrap();
        let open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
        nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel);
        let mut accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
@@@ -820,7 -821,7 +821,7 @@@ fn test_invalid_upfront_shutdown_script
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
  
 -      nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
 +      nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None, None).unwrap();
  
        // Use a segwit v0 script with an unsupported witness program
        let mut open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
@@@ -1237,3 -1238,102 +1238,102 @@@ fn simple_target_feerate_shutdown() 
        check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure, [nodes[1].node.get_our_node_id()], 100000);
        check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure, [nodes[0].node.get_our_node_id()], 100000);
  }
+ fn do_outbound_update_no_early_closing_signed(use_htlc: bool) {
+       // Previously, if we have a pending inbound HTLC (or fee update) on a channel which has
+       // initiated shutdown, we'd send our initial closing_signed immediately after receiving the
+       // peer's last RAA to remove the HTLC/fee update, but before receiving their final
+       // commitment_signed for a commitment without the HTLC/with the new fee. This caused at least
+       // LDK peers to force-close as we initiated closing_signed prior to the channel actually being
+       // fully empty of pending updates/HTLCs.
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       let chan_id = create_announced_chan_between_nodes(&nodes, 0, 1).2;
+       send_payment(&nodes[0], &[&nodes[1]], 1_000_000);
+       let payment_hash_opt = if use_htlc {
+               Some(route_payment(&nodes[1], &[&nodes[0]], 10_000).1)
+       } else {
+               None
+       };
+       if use_htlc {
+               nodes[0].node.fail_htlc_backwards(&payment_hash_opt.unwrap());
+               expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[0],
+                       [HTLCDestination::FailedPayment { payment_hash: payment_hash_opt.unwrap() }]);
+       } else {
+               *chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap() *= 10;
+               nodes[0].node.timer_tick_occurred();
+       }
+       let updates = get_htlc_update_msgs(&nodes[0], &nodes[1].node.get_our_node_id());
+       check_added_monitors(&nodes[0], 1);
+       nodes[1].node.close_channel(&chan_id, &nodes[0].node.get_our_node_id()).unwrap();
+       let node_0_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());
+       nodes[0].node.close_channel(&chan_id, &nodes[1].node.get_our_node_id()).unwrap();
+       let node_1_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id());
+       nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &node_0_shutdown);
+       nodes[1].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &node_1_shutdown);
+       if use_htlc {
+               nodes[1].node.handle_update_fail_htlc(&nodes[0].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
+       } else {
+               nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &updates.update_fee.unwrap());
+       }
+       nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &updates.commitment_signed);
+       check_added_monitors(&nodes[1], 1);
+       let (bs_raa, bs_cs) = get_revoke_commit_msgs(&nodes[1], &nodes[0].node.get_our_node_id());
+       nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa);
+       check_added_monitors(&nodes[0], 1);
+       // At this point the Channel on nodes[0] has no record of any HTLCs but the latest
+       // broadcastable commitment does contain the HTLC (but only the ChannelMonitor knows this).
+       // Thus, the channel should not yet initiate closing_signed negotiation (but previously did).
+       assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());
+       chanmon_cfgs[0].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
+       nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_cs);
+       check_added_monitors(&nodes[0], 1);
+       assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());
+       expect_channel_shutdown_state!(nodes[0], chan_id, ChannelShutdownState::ResolvingHTLCs);
+       assert_eq!(nodes[0].node.get_and_clear_pending_msg_events(), Vec::new());
+       let (outpoint, latest_update, _) = nodes[0].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
+       nodes[0].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
+       let as_raa_closing_signed = nodes[0].node.get_and_clear_pending_msg_events();
+       assert_eq!(as_raa_closing_signed.len(), 2);
+       if let MessageSendEvent::SendRevokeAndACK { msg, .. } = &as_raa_closing_signed[0] {
+               nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &msg);
+               check_added_monitors(&nodes[1], 1);
+               if use_htlc {
+                       expect_payment_failed!(nodes[1], payment_hash_opt.unwrap(), true);
+               }
+       } else { panic!("Unexpected message {:?}", as_raa_closing_signed[0]); }
+       if let MessageSendEvent::SendClosingSigned { msg, .. } = &as_raa_closing_signed[1] {
+               nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &msg);
+       } else { panic!("Unexpected message {:?}", as_raa_closing_signed[1]); }
+       let bs_closing_signed = get_event_msg!(nodes[1], MessageSendEvent::SendClosingSigned, nodes[0].node.get_our_node_id());
+       nodes[0].node.handle_closing_signed(&nodes[1].node.get_our_node_id(), &bs_closing_signed);
+       let (_, as_2nd_closing_signed) = get_closing_signed_broadcast!(nodes[0].node, nodes[1].node.get_our_node_id());
+       nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &as_2nd_closing_signed.unwrap());
+       let (_, node_1_none) = get_closing_signed_broadcast!(nodes[1].node, nodes[0].node.get_our_node_id());
+       assert!(node_1_none.is_none());
+       check_closed_event!(nodes[0], 1, ClosureReason::CooperativeClosure, [nodes[1].node.get_our_node_id()], 100000);
+       check_closed_event!(nodes[1], 1, ClosureReason::CooperativeClosure, [nodes[0].node.get_our_node_id()], 100000);
+ }
+ #[test]
+ fn outbound_update_no_early_closing_signed() {
+       do_outbound_update_no_early_closing_signed(true);
+       do_outbound_update_no_early_closing_signed(false);
+ }