Merge pull request #1282 from TheBlueMatt/2022-01-fuzz-overflow
authorvalentinewallace <valentinewallace@users.noreply.github.com>
Thu, 27 Jan 2022 16:42:05 +0000 (11:42 -0500)
committerGitHub <noreply@github.com>
Thu, 27 Jan 2022 16:42:05 +0000 (11:42 -0500)
Avoid overflow in addition when checking counterparty feerates

1  2 
lightning/src/ln/channel.rs

index 72c60e8ed616f0518aca326ec4230d549eeb5a8f,8daea4d7548fda4d1f72d50b4e3f46774d404f8a..fc48a6b9a1ceb777eea964f19b89bd729072b60f
@@@ -162,43 -162,19 +162,43 @@@ enum OutboundHTLCState 
        Committed,
        /// Remote removed this (outbound) HTLC. We're waiting on their commitment_signed to finalize
        /// the change (though they'll need to revoke before we fail the payment).
 -      RemoteRemoved(Option<HTLCFailReason>),
 +      RemoteRemoved(OutboundHTLCOutcome),
        /// Remote removed this and sent a commitment_signed (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// can do any backwards failing. Implies AwaitingRemoteRevoke.
        /// We also have not yet removed this HTLC in a commitment_signed message, and are waiting on a
        /// remote revoke_and_ack on a previous state before we can do so.
 -      AwaitingRemoteRevokeToRemove(Option<HTLCFailReason>),
 +      AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome),
        /// Remote removed this and sent a commitment_signed (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// can do any backwards failing. Implies AwaitingRemoteRevoke.
        /// We have removed this HTLC in our latest commitment_signed and are now just waiting on a
        /// revoke_and_ack to drop completely.
 -      AwaitingRemovedRemoteRevoke(Option<HTLCFailReason>),
 +      AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome),
 +}
 +
 +#[derive(Clone)]
 +enum OutboundHTLCOutcome {
 +      Success(Option<PaymentPreimage>),
 +      Failure(HTLCFailReason),
 +}
 +
 +impl From<Option<HTLCFailReason>> for OutboundHTLCOutcome {
 +      fn from(o: Option<HTLCFailReason>) -> Self {
 +              match o {
 +                      None => OutboundHTLCOutcome::Success(None),
 +                      Some(r) => OutboundHTLCOutcome::Failure(r)
 +              }
 +      }
 +}
 +
 +impl<'a> Into<Option<&'a HTLCFailReason>> for &'a OutboundHTLCOutcome {
 +      fn into(self) -> Option<&'a HTLCFailReason> {
 +              match self {
 +                      OutboundHTLCOutcome::Success(_) => None,
 +                      OutboundHTLCOutcome::Failure(ref r) => Some(r)
 +              }
 +      }
  }
  
  struct OutboundHTLCOutput {
@@@ -305,26 -281,6 +305,26 @@@ pub(super) enum ChannelUpdateStatus 
        Disabled,
  }
  
 +/// We track when we sent an `AnnouncementSignatures` to our peer in a few states, described here.
 +#[derive(PartialEq)]
 +pub enum AnnouncementSigsState {
 +      /// We have not sent our peer an `AnnouncementSignatures` yet, or our peer disconnected since
 +      /// we sent the last `AnnouncementSignatures`.
 +      NotSent,
 +      /// We sent an `AnnouncementSignatures` to our peer since the last time our peer disconnected.
 +      /// This state never appears on disk - instead we write `NotSent`.
 +      MessageSent,
 +      /// We sent a `CommitmentSigned` after the last `AnnouncementSignatures` we sent. Because we
 +      /// only ever have a single `CommitmentSigned` pending at once, if we sent one after sending
 +      /// `AnnouncementSignatures` then we know the peer received our `AnnouncementSignatures` if
 +      /// they send back a `RevokeAndACK`.
 +      /// This state never appears on disk - instead we write `NotSent`.
 +      Committed,
 +      /// We received a `RevokeAndACK`, effectively ack-ing our `AnnouncementSignatures`, at this
 +      /// point we no longer need to re-send our `AnnouncementSignatures` again on reconnect.
 +      PeerReceived,
 +}
 +
  /// An enum indicating whether the local or remote side offered a given HTLC.
  enum HTLCInitiator {
        LocalOffered,
@@@ -350,7 -306,6 +350,7 @@@ struct CommitmentStats<'a> 
        htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction
        local_balance_msat: u64, // local balance before fees but considering dust limits
        remote_balance_msat: u64, // remote balance before fees but considering dust limits
 +      preimages: Vec<PaymentPreimage>, // preimages for successful offered HTLCs since last commitment
  }
  
  /// Used when calculating whether we or the remote can afford an additional HTLC.
@@@ -419,19 -374,6 +419,19 @@@ pub(super) struct MonitorRestoreUpdate
        pub finalized_claimed_htlcs: Vec<HTLCSource>,
        pub funding_broadcastable: Option<Transaction>,
        pub funding_locked: Option<msgs::FundingLocked>,
 +      pub announcement_sigs: Option<msgs::AnnouncementSignatures>,
 +}
 +
 +/// The return value of `channel_reestablish`
 +pub(super) struct ReestablishResponses {
 +      pub funding_locked: Option<msgs::FundingLocked>,
 +      pub raa: Option<msgs::RevokeAndACK>,
 +      pub commitment_update: Option<msgs::CommitmentUpdate>,
 +      pub order: RAACommitmentOrder,
 +      pub mon_update: Option<ChannelMonitorUpdate>,
 +      pub holding_cell_failed_htlcs: Vec<(HTLCSource, PaymentHash)>,
 +      pub announcement_sigs: Option<msgs::AnnouncementSignatures>,
 +      pub shutdown_msg: Option<msgs::Shutdown>,
  }
  
  /// If the majority of the channels funds are to the fundee and the initiator holds only just
@@@ -488,19 -430,6 +488,19 @@@ pub(super) struct Channel<Signer: Sign
  
        channel_id: [u8; 32],
        channel_state: u32,
 +
 +      // When we reach max(6 blocks, minimum_depth), we need to send an AnnouncementSigs message to
 +      // our peer. However, we want to make sure they received it, or else rebroadcast it when we
 +      // next connect.
 +      // We do so here, see `AnnouncementSigsSent` for more details on the state(s).
 +      // Note that a number of our tests were written prior to the behavior here which retransmits
 +      // AnnouncementSignatures until after an RAA completes, so the behavior is short-circuited in
 +      // many tests.
 +      #[cfg(any(test, feature = "_test_utils"))]
 +      pub(crate) announcement_sigs_state: AnnouncementSigsState,
 +      #[cfg(not(any(test, feature = "_test_utils")))]
 +      announcement_sigs_state: AnnouncementSigsState,
 +
        secp_ctx: Secp256k1<secp256k1::All>,
        channel_value_satoshis: u64,
  
@@@ -838,7 -767,6 +838,7 @@@ impl<Signer: Sign> Channel<Signer> 
  
                        channel_id: keys_provider.get_secure_random_bytes(),
                        channel_state: ChannelState::OurInitSent as u32,
 +                      announcement_sigs_state: AnnouncementSigsState::NotSent,
                        secp_ctx,
                        channel_value_satoshis,
  
        fn check_remote_fee<F: Deref>(fee_estimator: &F, feerate_per_kw: u32) -> Result<(), ChannelError>
                where F::Target: FeeEstimator
        {
-               let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-               // Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
-               // occasional issues with feerate disagreements between an initiator that wants a feerate
-               // of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
-               // sat/kw before the comparison here.
-               if feerate_per_kw + 250 < lower_limit {
-                       return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {} (- 250)", feerate_per_kw, lower_limit)));
-               }
                // We only bound the fee updates on the upper side to prevent completely absurd feerates,
                // always accepting up to 25 sat/vByte or 10x our fee estimator's "High Priority" fee.
                // We generally don't care too much if they set the feerate to something very high, but it
                if feerate_per_kw as u64 > upper_limit {
                        return Err(ChannelError::Close(format!("Peer's feerate much too high. Actual: {}. Our expected upper limit: {}", feerate_per_kw, upper_limit)));
                }
+               let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
+               // Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
+               // occasional issues with feerate disagreements between an initiator that wants a feerate
+               // of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
+               // sat/kw before the comparison here.
+               if feerate_per_kw + 250 < lower_limit {
+                       return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {} (- 250)", feerate_per_kw, lower_limit)));
+               }
                Ok(())
        }
  
  
                        channel_id: msg.temporary_channel_id,
                        channel_state: (ChannelState::OurInitSent as u32) | (ChannelState::TheirInitSent as u32),
 +                      announcement_sigs_state: AnnouncementSigsState::NotSent,
                        secp_ctx,
  
                        latest_monitor_update_id: 0,
                        }
                }
  
 +              let mut preimages: Vec<PaymentPreimage> = Vec::new();
 +
                for ref htlc in self.pending_outbound_htlcs.iter() {
                        let (include, state_name) = match htlc.state {
                                OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"),
                                OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) => (false, "AwaitingRemovedRemoteRevoke"),
                        };
  
 +                      let preimage_opt = match htlc.state {
 +                              OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(p)) => p,
 +                              OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(p)) => p,
 +                              OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(p)) => p,
 +                              _ => None,
 +                      };
 +
 +                      if let Some(preimage) = preimage_opt {
 +                              preimages.push(preimage);
 +                      }
 +
                        if include {
                                add_htlc_output!(htlc, true, Some(&htlc.source), state_name);
                                local_htlc_total_msat += htlc.amount_msat;
                        } else {
                                log_trace!(logger, "   ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, log_bytes!(htlc.payment_hash.0), htlc.amount_msat, state_name);
                                match htlc.state {
 -                                      OutboundHTLCState::AwaitingRemoteRevokeToRemove(None)|OutboundHTLCState::AwaitingRemovedRemoteRevoke(None) => {
 +                                      OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_))|OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) => {
                                                value_to_self_msat_offset -= htlc.amount_msat as i64;
                                        },
 -                                      OutboundHTLCState::RemoteRemoved(None) => {
 +                                      OutboundHTLCState::RemoteRemoved(OutboundHTLCOutcome::Success(_)) => {
                                                if !generated_by_local {
                                                        value_to_self_msat_offset -= htlc.amount_msat as i64;
                                                }
                        htlcs_included,
                        local_balance_msat: value_to_self_msat as u64,
                        remote_balance_msat: value_to_remote_msat as u64,
 +                      preimages
                }
        }
  
                log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}",
                        log_bytes!(self.channel_id()), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction));
  
 -              let counterparty_signature = self.holder_signer.sign_counterparty_commitment(&counterparty_initial_commitment_tx, &self.secp_ctx)
 +              let counterparty_signature = self.holder_signer.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), &self.secp_ctx)
                                .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0;
  
                // We sign "counterparty" commitment transaction, allowing them to broadcast the tx if they wish.
                        self.counterparty_funding_pubkey()
                );
  
 -              self.holder_signer.validate_holder_commitment(&holder_commitment_tx)
 +              self.holder_signer.validate_holder_commitment(&holder_commitment_tx, Vec::new())
                        .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?;
  
                // Now that we're past error-generating stuff, update our local state:
                        self.counterparty_funding_pubkey()
                );
  
 -              self.holder_signer.validate_holder_commitment(&holder_commitment_tx)
 +              self.holder_signer.validate_holder_commitment(&holder_commitment_tx, Vec::new())
                        .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?;
  
  
                Ok((channel_monitor, self.funding_transaction.as_ref().cloned().unwrap()))
        }
  
 -      pub fn funding_locked<L: Deref>(&mut self, msg: &msgs::FundingLocked, logger: &L) -> Result<(), ChannelError> where L::Target: Logger {
 +      /// Handles a funding_locked message from our peer. If we've already sent our funding_locked
 +      /// and the channel is now usable (and public), this may generate an announcement_signatures to
 +      /// reply with.
 +      pub fn funding_locked<L: Deref>(&mut self, msg: &msgs::FundingLocked, node_pk: PublicKey, genesis_block_hash: BlockHash, best_block: &BestBlock, logger: &L) -> Result<Option<msgs::AnnouncementSignatures>, ChannelError> where L::Target: Logger {
                if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
                        self.workaround_lnd_bug_4006 = Some(msg.clone());
                        return Err(ChannelError::Ignore("Peer sent funding_locked when we needed a channel_reestablish. The peer is likely lnd, see https://github.com/lightningnetwork/lnd/issues/4006".to_owned()));
                                return Err(ChannelError::Close("Peer sent a reconnect funding_locked with a different point".to_owned()));
                        }
                        // They probably disconnected/reconnected and re-sent the funding_locked, which is required
 -                      return Ok(());
 +                      return Ok(None);
                } else {
                        return Err(ChannelError::Close("Peer sent a funding_locked at a strange time".to_owned()));
                }
  
                log_info!(logger, "Received funding_locked from peer for channel {}", log_bytes!(self.channel_id()));
  
 -              Ok(())
 +              Ok(self.get_announcement_sigs(node_pk, genesis_block_hash, best_block.height(), logger))
        }
  
        /// Returns transaction if there is pending funding transaction that is yet to broadcast
                // transaction).
                let mut removed_outbound_total_msat = 0;
                for ref htlc in self.pending_outbound_htlcs.iter() {
 -                      if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(None) = htlc.state {
 +                      if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) = htlc.state {
                                removed_outbound_total_msat += htlc.amount_msat;
 -                      } else if let OutboundHTLCState::AwaitingRemovedRemoteRevoke(None) = htlc.state {
 +                      } else if let OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) = htlc.state {
                                removed_outbound_total_msat += htlc.amount_msat;
                        }
                }
  
        /// Marks an outbound HTLC which we have received update_fail/fulfill/malformed
        #[inline]
 -      fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, check_preimage: Option<PaymentHash>, fail_reason: Option<HTLCFailReason>) -> Result<&OutboundHTLCOutput, ChannelError> {
 +      fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, check_preimage: Option<PaymentPreimage>, fail_reason: Option<HTLCFailReason>) -> Result<&OutboundHTLCOutput, ChannelError> {
 +              assert!(!(check_preimage.is_some() && fail_reason.is_some()), "cannot fail while we have a preimage");
                for htlc in self.pending_outbound_htlcs.iter_mut() {
                        if htlc.htlc_id == htlc_id {
 -                              match check_preimage {
 -                                      None => {},
 -                                      Some(payment_hash) =>
 +                              let outcome = match check_preimage {
 +                                      None => fail_reason.into(),
 +                                      Some(payment_preimage) => {
 +                                              let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner());
                                                if payment_hash != htlc.payment_hash {
                                                        return Err(ChannelError::Close(format!("Remote tried to fulfill HTLC ({}) with an incorrect preimage", htlc_id)));
                                                }
 +                                              OutboundHTLCOutcome::Success(Some(payment_preimage))
 +                                      }
                                };
                                match htlc.state {
                                        OutboundHTLCState::LocalAnnounced(_) =>
                                                return Err(ChannelError::Close(format!("Remote tried to fulfill/fail HTLC ({}) before it had been committed", htlc_id))),
                                        OutboundHTLCState::Committed => {
 -                                              htlc.state = OutboundHTLCState::RemoteRemoved(fail_reason);
 +                                              htlc.state = OutboundHTLCState::RemoteRemoved(outcome);
                                        },
                                        OutboundHTLCState::AwaitingRemoteRevokeToRemove(_) | OutboundHTLCState::AwaitingRemovedRemoteRevoke(_) | OutboundHTLCState::RemoteRemoved(_) =>
                                                return Err(ChannelError::Close(format!("Remote tried to fulfill/fail HTLC ({}) that they'd already fulfilled/failed", htlc_id))),
                        return Err(ChannelError::Close("Peer sent update_fulfill_htlc when we needed a channel_reestablish".to_owned()));
                }
  
 -              let payment_hash = PaymentHash(Sha256::hash(&msg.payment_preimage.0[..]).into_inner());
 -              self.mark_outbound_htlc_removed(msg.htlc_id, Some(payment_hash), None).map(|htlc| (htlc.source.clone(), htlc.amount_msat))
 +              self.mark_outbound_htlc_removed(msg.htlc_id, Some(msg.payment_preimage), None).map(|htlc| (htlc.source.clone(), htlc.amount_msat))
        }
  
        pub fn update_fail_htlc(&mut self, msg: &msgs::UpdateFailHTLC, fail_reason: HTLCFailReason) -> Result<(), ChannelError> {
                );
  
                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)
 +              self.holder_signer.validate_holder_commitment(&holder_commitment_tx, commitment_stats.preimages)
                        .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);
  
                        }
                }
                for htlc in self.pending_outbound_htlcs.iter_mut() {
 -                      if let Some(fail_reason) = if let &mut OutboundHTLCState::RemoteRemoved(ref mut fail_reason) = &mut htlc.state {
 -                              Some(fail_reason.take())
 -                      } else { None } {
 +                      if let &mut OutboundHTLCState::RemoteRemoved(ref mut outcome) = &mut htlc.state {
                                log_trace!(logger, "Updating HTLC {} to AwaitingRemoteRevokeToRemove due to commitment_signed in channel {}.",
                                        log_bytes!(htlc.payment_hash.0), log_bytes!(self.channel_id));
 -                              htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(fail_reason);
 +                              // Grab the preimage, if it exists, instead of cloning
 +                              let mut reason = OutboundHTLCOutcome::Success(None);
 +                              mem::swap(outcome, &mut reason);
 +                              htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(reason);
                                need_commitment = true;
                        }
                }
                self.counterparty_cur_commitment_point = Some(msg.next_per_commitment_point);
                self.cur_counterparty_commitment_transaction_number -= 1;
  
 +              if self.announcement_sigs_state == AnnouncementSigsState::Committed {
 +                      self.announcement_sigs_state = AnnouncementSigsState::PeerReceived;
 +              }
 +
                log_trace!(logger, "Updating HTLCs on receipt of RAA in channel {}...", log_bytes!(self.channel_id()));
                let mut to_forward_infos = Vec::new();
                let mut revoked_htlcs = Vec::new();
                                } else { true }
                        });
                        pending_outbound_htlcs.retain(|htlc| {
 -                              if let &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref fail_reason) = &htlc.state {
 +                              if let &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref outcome) = &htlc.state {
                                        log_trace!(logger, " ...removing outbound AwaitingRemovedRemoteRevoke {}", log_bytes!(htlc.payment_hash.0));
 -                                      if let Some(reason) = fail_reason.clone() { // We really want take() here, but, again, non-mut ref :(
 +                                      if let OutboundHTLCOutcome::Failure(reason) = outcome.clone() { // We really want take() here, but, again, non-mut ref :(
                                                revoked_htlcs.push((htlc.source.clone(), htlc.payment_hash, reason));
                                        } else {
                                                finalized_claimed_htlcs.push(htlc.source.clone());
                                        log_trace!(logger, " ...promoting outbound LocalAnnounced {} to Committed", log_bytes!(htlc.payment_hash.0));
                                        htlc.state = OutboundHTLCState::Committed;
                                }
 -                              if let Some(fail_reason) = if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut fail_reason) = &mut htlc.state {
 -                                      Some(fail_reason.take())
 -                              } else { None } {
 +                              if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
                                        log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", log_bytes!(htlc.payment_hash.0));
 -                                      htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(fail_reason);
 +                                      // Grab the preimage, if it exists, instead of cloning
 +                                      let mut reason = OutboundHTLCOutcome::Success(None);
 +                                      mem::swap(outcome, &mut reason);
 +                                      htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason);
                                        require_commitment = true;
                                }
                        }
                        self.channel_state = ChannelState::ShutdownComplete as u32;
                        return;
                }
 +
 +              if self.announcement_sigs_state == AnnouncementSigsState::MessageSent || self.announcement_sigs_state == AnnouncementSigsState::Committed {
 +                      self.announcement_sigs_state = AnnouncementSigsState::NotSent;
 +              }
 +
                // Upon reconnect we have to start the closing_signed dance over, but shutdown messages
                // will be retransmitted.
                self.last_sent_closing_fee = None;
        /// Indicates that the latest ChannelMonitor update has been committed by the client
        /// successfully and we should restore normal operation. Returns messages which should be sent
        /// to the remote side.
 -      pub fn monitor_updating_restored<L: Deref>(&mut self, logger: &L) -> MonitorRestoreUpdates where L::Target: Logger {
 +      pub fn monitor_updating_restored<L: Deref>(&mut self, logger: &L, node_pk: PublicKey, genesis_block_hash: BlockHash, best_block_height: u32) -> MonitorRestoreUpdates where L::Target: Logger {
                assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32);
                self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32);
  
                        })
                } else { None };
  
 +              let announcement_sigs = self.get_announcement_sigs(node_pk, genesis_block_hash, best_block_height, logger);
 +
                let mut accepted_htlcs = Vec::new();
                mem::swap(&mut accepted_htlcs, &mut self.monitor_pending_forwards);
                let mut failed_htlcs = Vec::new();
                        self.monitor_pending_commitment_signed = false;
                        return MonitorRestoreUpdates {
                                raa: None, commitment_update: None, order: RAACommitmentOrder::RevokeAndACKFirst,
 -                              accepted_htlcs, failed_htlcs, finalized_claimed_htlcs, funding_broadcastable, funding_locked
 +                              accepted_htlcs, failed_htlcs, finalized_claimed_htlcs, funding_broadcastable, funding_locked, announcement_sigs
                        };
                }
  
                        if commitment_update.is_some() { "a" } else { "no" }, if raa.is_some() { "an" } else { "no" },
                        match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
                MonitorRestoreUpdates {
 -                      raa, commitment_update, order, accepted_htlcs, failed_htlcs, finalized_claimed_htlcs, funding_broadcastable, funding_locked
 +                      raa, commitment_update, order, accepted_htlcs, failed_htlcs, finalized_claimed_htlcs, funding_broadcastable, funding_locked, announcement_sigs
                }
        }
  
  
        /// May panic if some calls other than message-handling calls (which will all Err immediately)
        /// have been called between remove_uncommitted_htlcs_and_mark_paused and this call.
 -      pub fn channel_reestablish<L: Deref>(&mut self, msg: &msgs::ChannelReestablish, logger: &L) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitorUpdate>, RAACommitmentOrder, Vec<(HTLCSource, PaymentHash)>, Option<msgs::Shutdown>), ChannelError> where L::Target: Logger {
 +      pub fn channel_reestablish<L: Deref>(&mut self, msg: &msgs::ChannelReestablish, logger: &L,
 +              node_pk: PublicKey, genesis_block_hash: BlockHash, best_block: &BestBlock)
 +      -> Result<ReestablishResponses, ChannelError> where L::Target: Logger {
                if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 {
                        // While BOLT 2 doesn't indicate explicitly we should error this channel here, it
                        // almost certainly indicates we are going to end up out-of-sync in some way, so we
                        })
                } else { None };
  
 +              let announcement_sigs = self.get_announcement_sigs(node_pk, genesis_block_hash, best_block.height(), logger);
 +
                if self.channel_state & (ChannelState::FundingSent as u32) == ChannelState::FundingSent as u32 {
                        // If we're waiting on a monitor update, we shouldn't re-send any funding_locked's.
                        if self.channel_state & (ChannelState::OurFundingLocked as u32) == 0 ||
                                        return Err(ChannelError::Close("Peer claimed they saw a revoke_and_ack but we haven't sent funding_locked yet".to_owned()));
                                }
                                // Short circuit the whole handler as there is nothing we can resend them
 -                              return Ok((None, None, None, None, RAACommitmentOrder::CommitmentFirst, Vec::new(), shutdown_msg));
 +                              return Ok(ReestablishResponses {
 +                                      funding_locked: None,
 +                                      raa: None, commitment_update: None, mon_update: None,
 +                                      order: RAACommitmentOrder::CommitmentFirst,
 +                                      holding_cell_failed_htlcs: Vec::new(),
 +                                      shutdown_msg, announcement_sigs,
 +                              });
                        }
  
                        // We have OurFundingLocked set!
                        let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
 -                      return Ok((Some(msgs::FundingLocked {
 -                              channel_id: self.channel_id(),
 -                              next_per_commitment_point,
 -                      }), None, None, None, RAACommitmentOrder::CommitmentFirst, Vec::new(), shutdown_msg));
 +                      return Ok(ReestablishResponses {
 +                              funding_locked: Some(msgs::FundingLocked {
 +                                      channel_id: self.channel_id(),
 +                                      next_per_commitment_point,
 +                              }),
 +                              raa: None, commitment_update: None, mon_update: None,
 +                              order: RAACommitmentOrder::CommitmentFirst,
 +                              holding_cell_failed_htlcs: Vec::new(),
 +                              shutdown_msg, announcement_sigs,
 +                      });
                }
  
                let required_revoke = if msg.next_remote_commitment_number + 1 == INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number {
                // the corresponding revoke_and_ack back yet.
                let next_counterparty_commitment_number = INITIAL_COMMITMENT_NUMBER - self.cur_counterparty_commitment_transaction_number + if (self.channel_state & ChannelState::AwaitingRemoteRevoke as u32) != 0 { 1 } else { 0 };
  
 -              let resend_funding_locked = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number == 1 {
 +              let funding_locked = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number == 1 {
                        // We should never have to worry about MonitorUpdateFailed resending FundingLocked
                        let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
                        Some(msgs::FundingLocked {
                                // have received some updates while we were disconnected. Free the holding cell
                                // now!
                                match self.free_holding_cell_htlcs(logger) {
 -                                      Err(ChannelError::Close(msg)) => return Err(ChannelError::Close(msg)),
 +                                      Err(ChannelError::Close(msg)) => Err(ChannelError::Close(msg)),
                                        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));
 +                                      Ok((Some((commitment_update, monitor_update)), holding_cell_failed_htlcs)) => {
 +                                              Ok(ReestablishResponses {
 +                                                      funding_locked, shutdown_msg, announcement_sigs,
 +                                                      raa: required_revoke,
 +                                                      commitment_update: Some(commitment_update),
 +                                                      order: self.resend_order.clone(),
 +                                                      mon_update: Some(monitor_update),
 +                                                      holding_cell_failed_htlcs,
 +                                              })
                                        },
 -                                      Ok((None, htlcs_to_fail)) => {
 -                                              return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), htlcs_to_fail, shutdown_msg));
 +                                      Ok((None, holding_cell_failed_htlcs)) => {
 +                                              Ok(ReestablishResponses {
 +                                                      funding_locked, shutdown_msg, announcement_sigs,
 +                                                      raa: required_revoke,
 +                                                      commitment_update: None,
 +                                                      order: self.resend_order.clone(),
 +                                                      mon_update: None,
 +                                                      holding_cell_failed_htlcs,
 +                                              })
                                        },
                                }
                        } else {
 -                              return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), Vec::new(), shutdown_msg));
 +                              Ok(ReestablishResponses {
 +                                      funding_locked, shutdown_msg, announcement_sigs,
 +                                      raa: required_revoke,
 +                                      commitment_update: None,
 +                                      order: self.resend_order.clone(),
 +                                      mon_update: None,
 +                                      holding_cell_failed_htlcs: Vec::new(),
 +                              })
                        }
                } else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 {
                        if required_revoke.is_some() {
  
                        if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 {
                                self.monitor_pending_commitment_signed = true;
 -                              return Ok((resend_funding_locked, None, None, None, self.resend_order.clone(), Vec::new(), shutdown_msg));
 +                              Ok(ReestablishResponses {
 +                                      funding_locked, shutdown_msg, announcement_sigs,
 +                                      commitment_update: None, raa: None, mon_update: None,
 +                                      order: self.resend_order.clone(),
 +                                      holding_cell_failed_htlcs: Vec::new(),
 +                              })
 +                      } else {
 +                              Ok(ReestablishResponses {
 +                                      funding_locked, shutdown_msg, announcement_sigs,
 +                                      raa: required_revoke,
 +                                      commitment_update: Some(self.get_last_commitment_update(logger)),
 +                                      order: self.resend_order.clone(),
 +                                      mon_update: None,
 +                                      holding_cell_failed_htlcs: Vec::new(),
 +                              })
                        }
 -
 -                      return Ok((resend_funding_locked, required_revoke, Some(self.get_last_commitment_update(logger)), None, self.resend_order.clone(), Vec::new(), shutdown_msg));
                } else {
 -                      return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction".to_owned()));
 +                      Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction".to_owned()))
                }
        }
  
        /// Allowed in any state (including after shutdown)
        pub fn is_usable(&self) -> bool {
                let mask = ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK;
 -              (self.channel_state & mask) == (ChannelState::ChannelFunded as u32)
 +              (self.channel_state & mask) == (ChannelState::ChannelFunded as u32) && !self.monitor_pending_funding_locked
        }
  
        /// Returns true if this channel is currently available for use. This is a superset of
  
                if need_commitment_update {
                        if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
 -                              let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
 -                              return Some(msgs::FundingLocked {
 -                                      channel_id: self.channel_id,
 -                                      next_per_commitment_point,
 -                              });
 +                              if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 {
 +                                      let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
 +                                      return Some(msgs::FundingLocked {
 +                                              channel_id: self.channel_id,
 +                                              next_per_commitment_point,
 +                                      });
 +                              }
                        } else {
                                self.monitor_pending_funding_locked = true;
                        }
        /// When a transaction is confirmed, we check whether it is or spends the funding transaction
        /// In the first case, we store the confirmation height and calculating the short channel id.
        /// In the second, we simply return an Err indicating we need to be force-closed now.
 -      pub fn transactions_confirmed<L: Deref>(&mut self, block_hash: &BlockHash, height: u32, txdata: &TransactionData, logger: &L)
 -      -> Result<Option<msgs::FundingLocked>, ClosureReason> where L::Target: Logger {
 +      pub fn transactions_confirmed<L: Deref>(&mut self, block_hash: &BlockHash, height: u32,
 +              txdata: &TransactionData, genesis_block_hash: BlockHash, node_pk: PublicKey, logger: &L)
 +      -> Result<(Option<msgs::FundingLocked>, Option<msgs::AnnouncementSignatures>), ClosureReason> where L::Target: Logger {
                let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS);
 -              for &(index_in_block, tx) in txdata.iter() {
 -                      if let Some(funding_txo) = self.get_funding_txo() {
 +              if let Some(funding_txo) = self.get_funding_txo() {
 +                      for &(index_in_block, tx) in txdata.iter() {
                                // If we haven't yet sent a funding_locked, but are in FundingSent (ignoring
                                // whether they've sent a funding_locked or not), check if we should send one.
                                if non_shutdown_state & !(ChannelState::TheirFundingLocked as u32) == ChannelState::FundingSent as u32 {
                                        // may have already happened for this block).
                                        if let Some(funding_locked) = self.check_get_funding_locked(height) {
                                                log_info!(logger, "Sending a funding_locked to our peer for channel {}", log_bytes!(self.channel_id));
 -                                              return Ok(Some(funding_locked));
 +                                              let announcement_sigs = self.get_announcement_sigs(node_pk, genesis_block_hash, height, logger);
 +                                              return Ok((Some(funding_locked), announcement_sigs));
                                        }
                                }
                                for inp in tx.input.iter() {
                                }
                        }
                }
 -              Ok(None)
 +              Ok((None, None))
        }
  
        /// When a new block is connected, we check the height of the block against outbound holding
        ///
        /// May return some HTLCs (and their payment_hash) which have timed out and should be failed
        /// back.
 -      pub fn best_block_updated<L: Deref>(&mut self, height: u32, highest_header_time: u32, logger: &L)
 -      -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>), ClosureReason> where L::Target: Logger {
 +      pub fn best_block_updated<L: Deref>(&mut self, height: u32, highest_header_time: u32, genesis_block_hash: BlockHash, node_pk: PublicKey, logger: &L)
 +      -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>, Option<msgs::AnnouncementSignatures>), ClosureReason> where L::Target: Logger {
 +              self.do_best_block_updated(height, highest_header_time, Some((genesis_block_hash, node_pk)), logger)
 +      }
 +
 +      fn do_best_block_updated<L: Deref>(&mut self, height: u32, highest_header_time: u32, genesis_node_pk: Option<(BlockHash, PublicKey)>, logger: &L)
 +      -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>, Option<msgs::AnnouncementSignatures>), ClosureReason> where L::Target: Logger {
                let mut timed_out_htlcs = Vec::new();
                // This mirrors the check in ChannelManager::decode_update_add_htlc_onion, refusing to
                // forward an HTLC when our counterparty should almost certainly just fail it for expiring
                self.update_time_counter = cmp::max(self.update_time_counter, highest_header_time);
  
                if let Some(funding_locked) = self.check_get_funding_locked(height) {
 +                      let announcement_sigs = if let Some((genesis_block_hash, node_pk)) = genesis_node_pk {
 +                              self.get_announcement_sigs(node_pk, genesis_block_hash, height, logger)
 +                      } else { None };
                        log_info!(logger, "Sending a funding_locked to our peer for channel {}", log_bytes!(self.channel_id));
 -                      return Ok((Some(funding_locked), timed_out_htlcs));
 +                      return Ok((Some(funding_locked), timed_out_htlcs, announcement_sigs));
                }
  
                let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS);
                        return Err(ClosureReason::FundingTimedOut);
                }
  
 -              Ok((None, timed_out_htlcs))
 +              let announcement_sigs = if let Some((genesis_block_hash, node_pk)) = genesis_node_pk {
 +                      self.get_announcement_sigs(node_pk, genesis_block_hash, height, logger)
 +              } else { None };
 +              Ok((None, timed_out_htlcs, announcement_sigs))
        }
  
        /// Indicates the funding transaction is no longer confirmed in the main chain. This may
                        // larger. If we don't know that time has moved forward, we can just set it to the last
                        // time we saw and it will be ignored.
                        let best_time = self.update_time_counter;
 -                      match self.best_block_updated(reorg_height, best_time, logger) {
 -                              Ok((funding_locked, timed_out_htlcs)) => {
 +                      match self.do_best_block_updated(reorg_height, best_time, None, logger) {
 +                              Ok((funding_locked, timed_out_htlcs, announcement_sigs)) => {
                                        assert!(funding_locked.is_none(), "We can't generate a funding with 0 confirmations?");
                                        assert!(timed_out_htlcs.is_empty(), "We can't have accepted HTLCs with a timeout before our funding confirmation?");
 +                                      assert!(announcement_sigs.is_none(), "We can't generate an announcement_sigs with 0 confirmations?");
                                        Ok(())
                                },
                                Err(e) => Err(e)
        fn get_outbound_funding_created_signature<L: Deref>(&mut self, logger: &L) -> Result<Signature, ChannelError> where L::Target: Logger {
                let counterparty_keys = self.build_remote_transaction_keys()?;
                let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx;
 -              Ok(self.holder_signer.sign_counterparty_commitment(&counterparty_initial_commitment_tx, &self.secp_ctx)
 +              Ok(self.holder_signer.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), &self.secp_ctx)
                                .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0)
        }
  
                })
        }
  
 -      /// Gets an UnsignedChannelAnnouncement, as well as a signature covering it using our
 -      /// bitcoin_key, if available, for this channel. The channel must be publicly announceable and
 -      /// available for use (have exchanged FundingLocked messages in both directions). Should be used
 -      /// for both loose and in response to an AnnouncementSignatures message from the remote peer.
 +      /// Gets an UnsignedChannelAnnouncement for this channel. The channel must be publicly
 +      /// announceable and available for use (have exchanged FundingLocked messages in both
 +      /// directions). Should be used for both broadcasted announcements and in response to an
 +      /// AnnouncementSignatures message from the remote peer.
 +      ///
        /// Will only fail if we're not in a state where channel_announcement may be sent (including
        /// closing).
 +      ///
        /// Note that the "channel must be funded" requirement is stricter than BOLT 7 requires - see
        /// https://github.com/lightningnetwork/lightning-rfc/issues/468
        ///
        /// This will only return ChannelError::Ignore upon failure.
 -      pub fn get_channel_announcement(&self, node_id: PublicKey, chain_hash: BlockHash) -> Result<(msgs::UnsignedChannelAnnouncement, Signature), ChannelError> {
 +      fn get_channel_announcement(&self, node_id: PublicKey, chain_hash: BlockHash) -> Result<msgs::UnsignedChannelAnnouncement, ChannelError> {
                if !self.config.announced_channel {
                        return Err(ChannelError::Ignore("Channel is not available for public announcements".to_owned()));
                }
 -              if self.channel_state & (ChannelState::ChannelFunded as u32) == 0 {
 -                      return Err(ChannelError::Ignore("Cannot get a ChannelAnnouncement until the channel funding has been locked".to_owned()));
 -              }
 -              if (self.channel_state & (ChannelState::LocalShutdownSent as u32 | ChannelState::ShutdownComplete as u32)) != 0 {
 -                      return Err(ChannelError::Ignore("Cannot get a ChannelAnnouncement once the channel is closing".to_owned()));
 +              if !self.is_usable() {
 +                      return Err(ChannelError::Ignore("Cannot get a ChannelAnnouncement if the channel is not currently usable".to_owned()));
                }
  
                let were_node_one = node_id.serialize()[..] < self.counterparty_node_id.serialize()[..];
                        excess_data: Vec::new(),
                };
  
 -              let sig = self.holder_signer.sign_channel_announcement(&msg, &self.secp_ctx)
 -                      .map_err(|_| ChannelError::Ignore("Signer rejected channel_announcement".to_owned()))?;
 +              Ok(msg)
 +      }
 +
 +      fn get_announcement_sigs<L: Deref>(&mut self, node_pk: PublicKey, genesis_block_hash: BlockHash, best_block_height: u32, logger: &L)
 +      -> Option<msgs::AnnouncementSignatures> where L::Target: Logger {
 +              if self.funding_tx_confirmation_height == 0 || self.funding_tx_confirmation_height + 5 > best_block_height {
 +                      return None;
 +              }
 +
 +              if !self.is_usable() {
 +                      return None;
 +              }
 +
 +              if self.channel_state & ChannelState::PeerDisconnected as u32 != 0 {
 +                      log_trace!(logger, "Cannot create an announcement_signatures as our peer is disconnected");
 +                      return None;
 +              }
 +
 +              if self.announcement_sigs_state != AnnouncementSigsState::NotSent {
 +                      return None;
 +              }
 +
 +              log_trace!(logger, "Creating an announcement_signatures message for channel {}", log_bytes!(self.channel_id()));
 +              let announcement = match self.get_channel_announcement(node_pk, genesis_block_hash) {
 +                      Ok(a) => a,
 +                      Err(_) => {
 +                              log_trace!(logger, "Cannot create an announcement_signatures as channel is not public.");
 +                              return None;
 +                      }
 +              };
 +              let (our_node_sig, our_bitcoin_sig) = match self.holder_signer.sign_channel_announcement(&announcement, &self.secp_ctx) {
 +                      Err(_) => {
 +                              log_error!(logger, "Signer rejected channel_announcement signing. Channel will not be announced!");
 +                              return None;
 +                      },
 +                      Ok(v) => v
 +              };
 +              self.announcement_sigs_state = AnnouncementSigsState::MessageSent;
  
 -              Ok((msg, sig))
 +              Some(msgs::AnnouncementSignatures {
 +                      channel_id: self.channel_id(),
 +                      short_channel_id: self.get_short_channel_id().unwrap(),
 +                      node_signature: our_node_sig,
 +                      bitcoin_signature: our_bitcoin_sig,
 +              })
        }
  
        /// Signs the given channel announcement, returning a ChannelError::Ignore if no keys are
        /// available.
 -      fn sign_channel_announcement(&self, our_node_secret: &SecretKey, our_node_id: PublicKey, msghash: secp256k1::Message, announcement: msgs::UnsignedChannelAnnouncement, our_bitcoin_sig: Signature) -> Result<msgs::ChannelAnnouncement, ChannelError> {
 +      fn sign_channel_announcement(&self, our_node_id: PublicKey, announcement: msgs::UnsignedChannelAnnouncement) -> Result<msgs::ChannelAnnouncement, ChannelError> {
                if let Some((their_node_sig, their_bitcoin_sig)) = self.announcement_sigs {
                        let were_node_one = announcement.node_id_1 == our_node_id;
  
 -                      let our_node_sig = self.secp_ctx.sign(&msghash, our_node_secret);
 +                      let (our_node_sig, our_bitcoin_sig) = self.holder_signer.sign_channel_announcement(&announcement, &self.secp_ctx)
 +                              .map_err(|_| ChannelError::Ignore("Signer rejected channel_announcement".to_owned()))?;
                        Ok(msgs::ChannelAnnouncement {
                                node_signature_1: if were_node_one { our_node_sig } else { their_node_sig },
                                node_signature_2: if were_node_one { their_node_sig } else { our_node_sig },
        /// Processes an incoming announcement_signatures message, providing a fully-signed
        /// channel_announcement message which we can broadcast and storing our counterparty's
        /// signatures for later reconstruction/rebroadcast of the channel_announcement.
 -      pub fn announcement_signatures(&mut self, our_node_secret: &SecretKey, our_node_id: PublicKey, chain_hash: BlockHash, msg: &msgs::AnnouncementSignatures) -> Result<msgs::ChannelAnnouncement, ChannelError> {
 -              let (announcement, our_bitcoin_sig) = self.get_channel_announcement(our_node_id.clone(), chain_hash)?;
 +      pub fn announcement_signatures(&mut self, our_node_id: PublicKey, chain_hash: BlockHash, best_block_height: u32, msg: &msgs::AnnouncementSignatures) -> Result<msgs::ChannelAnnouncement, ChannelError> {
 +              let announcement = self.get_channel_announcement(our_node_id.clone(), chain_hash)?;
  
                let msghash = hash_to_message!(&Sha256d::hash(&announcement.encode()[..])[..]);
  
                }
  
                self.announcement_sigs = Some((msg.node_signature, msg.bitcoin_signature));
 +              if self.funding_tx_confirmation_height == 0 || self.funding_tx_confirmation_height + 5 > best_block_height {
 +                      return Err(ChannelError::Ignore(
 +                              "Got announcement_signatures prior to the required six confirmations - we may not have received a block yet that our peer has".to_owned()));
 +              }
  
 -              self.sign_channel_announcement(our_node_secret, our_node_id, msghash, announcement, our_bitcoin_sig)
 +              self.sign_channel_announcement(our_node_id, announcement)
        }
  
        /// Gets a signed channel_announcement for this channel, if we previously received an
        /// announcement_signatures from our counterparty.
 -      pub fn get_signed_channel_announcement(&self, our_node_secret: &SecretKey, our_node_id: PublicKey, chain_hash: BlockHash) -> Option<msgs::ChannelAnnouncement> {
 -              let (announcement, our_bitcoin_sig) = match self.get_channel_announcement(our_node_id.clone(), chain_hash) {
 +      pub fn get_signed_channel_announcement(&self, our_node_id: PublicKey, chain_hash: BlockHash, best_block_height: u32) -> Option<msgs::ChannelAnnouncement> {
 +              if self.funding_tx_confirmation_height == 0 || self.funding_tx_confirmation_height + 5 > best_block_height {
 +                      return None;
 +              }
 +              let announcement = match self.get_channel_announcement(our_node_id.clone(), chain_hash) {
                        Ok(res) => res,
                        Err(_) => return None,
                };
 -              let msghash = hash_to_message!(&Sha256d::hash(&announcement.encode()[..])[..]);
 -              match self.sign_channel_announcement(our_node_secret, our_node_id, msghash, announcement, our_bitcoin_sig) {
 +              match self.sign_channel_announcement(our_node_id, announcement) {
                        Ok(res) => Some(res),
                        Err(_) => None,
                }
                        }
                }
                for htlc in self.pending_outbound_htlcs.iter_mut() {
 -                      if let Some(fail_reason) = if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut fail_reason) = &mut htlc.state {
 -                              Some(fail_reason.take())
 -                      } else { None } {
 +                      if let &mut OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref mut outcome) = &mut htlc.state {
                                log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", log_bytes!(htlc.payment_hash.0));
 -                              htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(fail_reason);
 +                              // Grab the preimage, if it exists, instead of cloning
 +                              let mut reason = OutboundHTLCOutcome::Success(None);
 +                              mem::swap(outcome, &mut reason);
 +                              htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(reason);
                        }
                }
                if let Some((feerate, update_state)) = self.pending_update_fee {
                        Err(e) => return Err(e),
                };
  
 +              if self.announcement_sigs_state == AnnouncementSigsState::MessageSent {
 +                      self.announcement_sigs_state = AnnouncementSigsState::Committed;
 +              }
 +
                self.latest_monitor_update_id += 1;
                let monitor_update = ChannelMonitorUpdate {
                        update_id: self.latest_monitor_update_id,
                                htlcs.push(htlc);
                        }
  
 -                      let res = self.holder_signer.sign_counterparty_commitment(&commitment_stats.tx, &self.secp_ctx)
 +                      let res = self.holder_signer.sign_counterparty_commitment(&commitment_stats.tx, commitment_stats.preimages, &self.secp_ctx)
                                .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?;
                        signature = res.0;
                        htlc_signatures = res.1;
@@@ -5400,29 -5177,6 +5400,29 @@@ impl Readable for ChannelUpdateStatus 
        }
  }
  
 +impl Writeable for AnnouncementSigsState {
 +      fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
 +              // We only care about writing out the current state as if we had just disconnected, at
 +              // which point we always set anything but AnnouncementSigsReceived to NotSent.
 +              match self {
 +                      AnnouncementSigsState::NotSent => 0u8.write(writer),
 +                      AnnouncementSigsState::MessageSent => 0u8.write(writer),
 +                      AnnouncementSigsState::Committed => 0u8.write(writer),
 +                      AnnouncementSigsState::PeerReceived => 1u8.write(writer),
 +              }
 +      }
 +}
 +
 +impl Readable for AnnouncementSigsState {
 +      fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
 +              Ok(match <u8 as Readable>::read(reader)? {
 +                      0 => AnnouncementSigsState::NotSent,
 +                      1 => AnnouncementSigsState::PeerReceived,
 +                      _ => return Err(DecodeError::InvalidValue),
 +              })
 +      }
 +}
 +
  impl<Signer: Sign> Writeable for Channel<Signer> {
        fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
                // Note that we write out as if remove_uncommitted_htlcs_and_mark_paused had just been
                        }
                }
  
 +              let mut preimages: Vec<&Option<PaymentPreimage>> = vec![];
 +
                (self.pending_outbound_htlcs.len() as u64).write(writer)?;
                for htlc in self.pending_outbound_htlcs.iter() {
                        htlc.htlc_id.write(writer)?;
                                        // resend the claim/fail on reconnect as we all (hopefully) the missing CS.
                                        1u8.write(writer)?;
                                },
 -                              &OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref fail_reason) => {
 +                              &OutboundHTLCState::AwaitingRemoteRevokeToRemove(ref outcome) => {
                                        3u8.write(writer)?;
 -                                      fail_reason.write(writer)?;
 -                              },
 -                              &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref fail_reason) => {
 +                                      if let OutboundHTLCOutcome::Success(preimage) = outcome {
 +                                              preimages.push(preimage);
 +                                      }
 +                                      let reason: Option<&HTLCFailReason> = outcome.into();
 +                                      reason.write(writer)?;
 +                              }
 +                              &OutboundHTLCState::AwaitingRemovedRemoteRevoke(ref outcome) => {
                                        4u8.write(writer)?;
 -                                      fail_reason.write(writer)?;
 -                              },
 +                                      if let OutboundHTLCOutcome::Success(preimage) = outcome {
 +                                              preimages.push(preimage);
 +                                      }
 +                                      let reason: Option<&HTLCFailReason> = outcome.into();
 +                                      reason.write(writer)?;
 +                              }
                        }
                }
  
                        (9, self.target_closing_feerate_sats_per_kw, option),
                        (11, self.monitor_pending_finalized_fulfills, vec_type),
                        (13, self.channel_creation_height, required),
 +                      (15, preimages, vec_type),
 +                      (17, self.announcement_sigs_state, required),
                });
  
                Ok(())
@@@ -5777,18 -5519,9 +5777,18 @@@ impl<'a, Signer: Sign, K: Deref> Readab
                                state: match <u8 as Readable>::read(reader)? {
                                        0 => OutboundHTLCState::LocalAnnounced(Box::new(Readable::read(reader)?)),
                                        1 => OutboundHTLCState::Committed,
 -                                      2 => OutboundHTLCState::RemoteRemoved(Readable::read(reader)?),
 -                                      3 => OutboundHTLCState::AwaitingRemoteRevokeToRemove(Readable::read(reader)?),
 -                                      4 => OutboundHTLCState::AwaitingRemovedRemoteRevoke(Readable::read(reader)?),
 +                                      2 => {
 +                                              let option: Option<HTLCFailReason> = Readable::read(reader)?;
 +                                              OutboundHTLCState::RemoteRemoved(option.into())
 +                                      },
 +                                      3 => {
 +                                              let option: Option<HTLCFailReason> = Readable::read(reader)?;
 +                                              OutboundHTLCState::AwaitingRemoteRevokeToRemove(option.into())
 +                                      },
 +                                      4 => {
 +                                              let option: Option<HTLCFailReason> = Readable::read(reader)?;
 +                                              OutboundHTLCState::AwaitingRemovedRemoteRevoke(option.into())
 +                                      },
                                        _ => return Err(DecodeError::InvalidValue),
                                },
                        });
                // only, so we default to that if none was written.
                let mut channel_type = Some(ChannelTypeFeatures::only_static_remote_key());
                let mut channel_creation_height = Some(serialized_height);
 +              let mut preimages_opt: Option<Vec<Option<PaymentPreimage>>> = None;
 +
 +              // If we read an old Channel, for simplicity we just treat it as "we never sent an
 +              // AnnouncementSignatures" which implies we'll re-send it on reconnect, but that's fine.
 +              let mut announcement_sigs_state = Some(AnnouncementSigsState::NotSent);
 +
                read_tlv_fields!(reader, {
                        (0, announcement_sigs, option),
                        (1, minimum_depth, option),
                        (9, target_closing_feerate_sats_per_kw, option),
                        (11, monitor_pending_finalized_fulfills, vec_type),
                        (13, channel_creation_height, option),
 +                      (15, preimages_opt, vec_type),
 +                      (17, announcement_sigs_state, option),
                });
  
 +              if let Some(preimages) = preimages_opt {
 +                      let mut iter = preimages.into_iter();
 +                      for htlc in pending_outbound_htlcs.iter_mut() {
 +                              match &htlc.state {
 +                                      OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(None)) => {
 +                                              htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?));
 +                                      }
 +                                      OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(None)) => {
 +                                              htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(iter.next().ok_or(DecodeError::InvalidValue)?));
 +                                      }
 +                                      _ => {}
 +                              }
 +                      }
 +                      // We expect all preimages to be consumed above
 +                      if iter.next().is_some() {
 +                              return Err(DecodeError::InvalidValue);
 +                      }
 +              }
 +
                let chan_features = channel_type.as_ref().unwrap();
                if chan_features.supports_unknown_bits() || chan_features.requires_unknown_bits() {
                        // If the channel was written by a new version and negotiated with features we don't
                        config: config.unwrap(),
                        channel_id,
                        channel_state,
 +                      announcement_sigs_state: announcement_sigs_state.unwrap(),
                        secp_ctx,
                        channel_value_satoshis,
  
@@@ -6154,6 -5859,13 +6154,13 @@@ mod tests 
                        "MAX_FUNDING_SATOSHIS is greater than all satoshis in existence");
        }
  
+       #[test]
+       fn test_no_fee_check_overflow() {
+               // Previously, calling `check_remote_fee` with a fee of 0xffffffff would overflow in
+               // arithmetic, causing a panic with debug assertions enabled.
+               assert!(Channel::<InMemorySigner>::check_remote_fee(&&TestFeeEstimator { fee_est: 42 }, u32::max_value()).is_err());
+       }
        struct Keys {
                signer: InMemorySigner,
        }
  
                let mut signer = InMemorySigner::new(
                        &secp_ctx,
 +                      SecretKey::from_slice(&hex::decode("4242424242424242424242424242424242424242424242424242424242424242").unwrap()[..]).unwrap(),
                        SecretKey::from_slice(&hex::decode("30ff4956bbdd3222d44cc5e8a1261dab1e07957bdac5ae88fe3261ef321f3749").unwrap()[..]).unwrap(),
                        SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(),
                        SecretKey::from_slice(&hex::decode("1111111111111111111111111111111111111111111111111111111111111111").unwrap()[..]).unwrap(),