Merge pull request #988 from TheBlueMatt/2021-07-chan-details-usability
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Thu, 8 Jul 2021 17:25:53 +0000 (17:25 +0000)
committerGitHub <noreply@github.com>
Thu, 8 Jul 2021 17:25:53 +0000 (17:25 +0000)
Improve ChannelDetails readability significantly.

1  2 
lightning/src/ln/channelmanager.rs

index bbd32994e875138ff5574cbb6e45f381e43ebb2a,e38b232e92d7089ec08c6184a130e052fe7c2c1c..2f51fbab6bb735c574d6dcaf6d246baebeb6de6f
@@@ -602,6 -602,29 +602,29 @@@ const CHECK_CLTV_EXPIRY_SANITY: u32 = M
  #[allow(dead_code)]
  const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER;
  
+ /// Channel parameters which apply to our counterparty. These are split out from [`ChannelDetails`]
+ /// to better separate parameters.
+ #[derive(Clone, Debug, PartialEq)]
+ pub struct ChannelCounterparty {
+       /// The node_id of our counterparty
+       pub node_id: PublicKey,
+       /// The Features the channel counterparty provided upon last connection.
+       /// Useful for routing as it is the most up-to-date copy of the counterparty's features and
+       /// many routing-relevant features are present in the init context.
+       pub features: InitFeatures,
+       /// The value, in satoshis, that must always be held in the channel for our counterparty. This
+       /// value ensures that if our counterparty broadcasts a revoked state, we can punish them by
+       /// claiming at least this value on chain.
+       ///
+       /// This value is not included in [`inbound_capacity_msat`] as it can never be spent.
+       ///
+       /// [`inbound_capacity_msat`]: ChannelDetails::inbound_capacity_msat
+       pub unspendable_punishment_reserve: u64,
+       /// Information on the fees and requirements that the counterparty requires when forwarding
+       /// payments to us through this channel.
+       pub forwarding_info: Option<CounterpartyForwardingInfo>,
+ }
  /// Details of a channel, as returned by ChannelManager::list_channels and ChannelManager::list_usable_channels
  #[derive(Clone, Debug, PartialEq)]
  pub struct ChannelDetails {
        /// Note that this means this value is *not* persistent - it can change once during the
        /// lifetime of the channel.
        pub channel_id: [u8; 32],
+       /// Parameters which apply to our counterparty. See individual fields for more information.
+       pub counterparty: ChannelCounterparty,
        /// The Channel's funding transaction output, if we've negotiated the funding transaction with
        /// our counterparty already.
        ///
        /// The position of the funding transaction in the chain. None if the funding transaction has
        /// not yet been confirmed and the channel fully opened.
        pub short_channel_id: Option<u64>,
-       /// The node_id of our counterparty
-       pub remote_network_id: PublicKey,
-       /// The Features the channel counterparty provided upon last connection.
-       /// Useful for routing as it is the most up-to-date copy of the counterparty's features and
-       /// many routing-relevant features are present in the init context.
-       pub counterparty_features: InitFeatures,
        /// The value, in satoshis, of this channel as appears in the funding output
        pub channel_value_satoshis: u64,
        /// The value, in satoshis, that must always be held in the channel for us. This value ensures
        /// This value will be `None` for outbound channels until the counterparty accepts the channel.
        ///
        /// [`outbound_capacity_msat`]: ChannelDetails::outbound_capacity_msat
-       pub to_self_reserve_satoshis: Option<u64>,
-       /// The value, in satoshis, that must always be held in the channel for our counterparty. This
-       /// value ensures that if our counterparty broadcasts a revoked state, we can punish them by
-       /// claiming at least this value on chain.
-       ///
-       /// This value is not included in [`inbound_capacity_msat`] as it can never be spent.
-       ///
-       /// [`inbound_capacity_msat`]: ChannelDetails::inbound_capacity_msat
-       pub to_remote_reserve_satoshis: u64,
+       pub unspendable_punishment_reserve: Option<u64>,
        /// The user_id passed in to create_channel, or 0 if the channel was inbound.
        pub user_id: u64,
        /// The available outbound capacity for sending HTLCs to the remote peer. This does not include
        /// time to claim our non-HTLC-encumbered funds.
        ///
        /// This value will be `None` for outbound channels until the counterparty accepts the channel.
-       pub spend_csv_on_our_commitment_funds: Option<u16>,
+       pub force_close_spend_delay: Option<u16>,
        /// True if the channel was initiated (and thus funded) by us.
        pub is_outbound: bool,
        /// True if the channel is confirmed, funding_locked messages have been exchanged, and the
        pub is_usable: bool,
        /// True if this channel is (or will be) publicly-announced.
        pub is_public: bool,
-       /// Information on the fees and requirements that the counterparty requires when forwarding
-       /// payments to us through this channel.
-       pub counterparty_forwarding_info: Option<CounterpartyForwardingInfo>,
  }
  
  /// If a payment fails to send, it can be in one of several states. This enum is returned as the
@@@ -800,7 -808,7 +808,7 @@@ macro_rules! convert_chan_err 
                                        $short_to_id.remove(&short_id);
                                }
                                let shutdown_res = $channel.force_shutdown(true);
 -                              (true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $self.get_channel_update(&$channel).ok()))
 +                              (true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $self.get_channel_update_for_broadcast(&$channel).ok()))
                        },
                        ChannelError::CloseDelayBroadcast(msg) => {
                                log_error!($self.logger, "Channel {} need to be shutdown but closing transactions not broadcast due to {}", log_bytes!($channel_id[..]), msg);
                                        $short_to_id.remove(&short_id);
                                }
                                let shutdown_res = $channel.force_shutdown(false);
 -                              (true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $self.get_channel_update(&$channel).ok()))
 +                              (true, MsgHandleErrInternal::from_finish_shutdown(msg, *$channel_id, shutdown_res, $self.get_channel_update_for_broadcast(&$channel).ok()))
                        }
                }
        }
@@@ -864,8 -872,7 +872,8 @@@ macro_rules! handle_monitor_err 
                                // splitting hairs we'd prefer to claim payments that were to us, but we haven't
                                // given up the preimage yet, so might as well just wait until the payment is
                                // retried, avoiding the on-chain fees.
 -                              let res: Result<(), _> = Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure".to_owned(), *$chan_id, $chan.force_shutdown(true), $self.get_channel_update(&$chan).ok()));
 +                              let res: Result<(), _> = Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure".to_owned(), *$chan_id,
 +                                              $chan.force_shutdown(true), $self.get_channel_update_for_broadcast(&$chan).ok() ));
                                (res, true)
                        },
                        ChannelMonitorUpdateErr::TemporaryFailure => {
@@@ -1171,30 -1178,32 +1179,32 @@@ impl<Signer: Sign, M: Deref, T: Deref, 
                                        channel.get_holder_counterparty_selected_channel_reserve_satoshis();
                                res.push(ChannelDetails {
                                        channel_id: (*channel_id).clone(),
+                                       counterparty: ChannelCounterparty {
+                                               node_id: channel.get_counterparty_node_id(),
+                                               features: InitFeatures::empty(),
+                                               unspendable_punishment_reserve: to_remote_reserve_satoshis,
+                                               forwarding_info: channel.counterparty_forwarding_info(),
+                                       },
                                        funding_txo: channel.get_funding_txo(),
                                        short_channel_id: channel.get_short_channel_id(),
-                                       remote_network_id: channel.get_counterparty_node_id(),
-                                       counterparty_features: InitFeatures::empty(),
                                        channel_value_satoshis: channel.get_value_satoshis(),
-                                       to_self_reserve_satoshis,
-                                       to_remote_reserve_satoshis,
+                                       unspendable_punishment_reserve: to_self_reserve_satoshis,
                                        inbound_capacity_msat,
                                        outbound_capacity_msat,
                                        user_id: channel.get_user_id(),
                                        confirmations_required: channel.minimum_depth(),
-                                       spend_csv_on_our_commitment_funds: channel.get_counterparty_selected_contest_delay(),
+                                       force_close_spend_delay: channel.get_counterparty_selected_contest_delay(),
                                        is_outbound: channel.is_outbound(),
                                        is_funding_locked: channel.is_usable(),
                                        is_usable: channel.is_live(),
                                        is_public: channel.should_announce(),
-                                       counterparty_forwarding_info: channel.counterparty_forwarding_info(),
                                });
                        }
                }
                let per_peer_state = self.per_peer_state.read().unwrap();
                for chan in res.iter_mut() {
-                       if let Some(peer_state) = per_peer_state.get(&chan.remote_network_id) {
-                               chan.counterparty_features = peer_state.lock().unwrap().latest_features.clone();
+                       if let Some(peer_state) = per_peer_state.get(&chan.counterparty.node_id) {
+                               chan.counterparty.features = peer_state.lock().unwrap().latest_features.clone();
                        }
                }
                res
                        self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_source.0, &htlc_source.1, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
                }
                let chan_update = if let Some(chan) = chan_option {
 -                      if let Ok(update) = self.get_channel_update(&chan) {
 -                              Some(update)
 -                      } else { None }
 +                      self.get_channel_update_for_broadcast(&chan).ok()
                } else { None };
  
                if let Some(update) = chan_update {
                };
                log_error!(self.logger, "Force-closing channel {}", log_bytes!(channel_id[..]));
                self.finish_force_close_channel(chan.force_shutdown(true));
 -              if let Ok(update) = self.get_channel_update(&chan) {
 +              if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
                        let mut channel_state = self.channel_state.lock().unwrap();
                        channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
                                msg: update
                                        // hopefully an attacker trying to path-trace payments cannot make this occur
                                        // on a small/per-node/per-channel scale.
                                        if !chan.is_live() { // channel_disabled
 -                                              break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
 +                                              break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
                                        if *amt_to_forward < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
 -                                              break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
 +                                              break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
                                        let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_holder_fee_base_msat(&self.fee_estimator) as u64) });
                                        if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 -                                              break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
 +                                              break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
                                        if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + chan.get_cltv_expiry_delta() as u64 { // incorrect_cltv_expiry
 -                                              break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update(chan).unwrap())));
 +                                              break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
                                        let cur_height = self.best_block.read().unwrap().height() + 1;
                                        // Theoretically, channel counterparty shouldn't send us a HTLC expiring now, but we want to be robust wrt to counterparty
                                        // packet sanitization (see HTLC_FAIL_BACK_BUFFER rational)
                                        if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
 -                                              break Some(("CLTV expiry is too close", 0x1000 | 14, Some(self.get_channel_update(chan).unwrap())));
 +                                              break Some(("CLTV expiry is too close", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
                                        if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
                                                break Some(("CLTV expiry is too far in the future", 21, None));
                                        }
 -                                      // In theory, we would be safe against unitentional channel-closure, if we only required a margin of LATENCY_GRACE_PERIOD_BLOCKS.
 -                                      // But, to be safe against policy reception, we use a longuer delay.
 +                                      // In theory, we would be safe against unintentional channel-closure, if we only required a margin of LATENCY_GRACE_PERIOD_BLOCKS.
 +                                      // But, to be safe against policy reception, we use a longer delay.
                                        if (*outgoing_cltv_value) as u64 <= (cur_height + HTLC_FAIL_BACK_BUFFER) as u64 {
 -                                              break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, Some(self.get_channel_update(chan).unwrap())));
 +                                              break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
  
                                        break None;
                (pending_forward_info, channel_state.unwrap())
        }
  
 -      /// only fails if the channel does not yet have an assigned short_id
 +      /// Gets the current channel_update for the given channel. This first checks if the channel is
 +      /// public, and thus should be called whenever the result is going to be passed out in a
 +      /// [`MessageSendEvent::BroadcastChannelUpdate`] event.
 +      ///
 +      /// May be called with channel_state already locked!
 +      fn get_channel_update_for_broadcast(&self, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> {
 +              if !chan.should_announce() {
 +                      return Err(LightningError {
 +                              err: "Cannot broadcast a channel_update for a private channel".to_owned(),
 +                              action: msgs::ErrorAction::IgnoreError
 +                      });
 +              }
 +              log_trace!(self.logger, "Attempting to generate broadcast channel update for channel {}", log_bytes!(chan.channel_id()));
 +              self.get_channel_update_for_unicast(chan)
 +      }
 +
 +      /// Gets the current channel_update for the given channel. This does not check if the channel
 +      /// is public (only returning an Err if the channel does not yet have an assigned short_id),
 +      /// and thus MUST NOT be called unless the recipient of the resulting message has already
 +      /// provided evidence that they know about the existence of the channel.
        /// May be called with channel_state already locked!
 -      fn get_channel_update(&self, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> {
 +      fn get_channel_update_for_unicast(&self, chan: &Channel<Signer>) -> Result<msgs::ChannelUpdate, LightningError> {
 +              log_trace!(self.logger, "Attempting to generate channel update for channel {}", log_bytes!(chan.channel_id()));
                let short_channel_id = match chan.get_short_channel_id() {
                        None => return Err(LightningError{err: "Channel not yet established".to_owned(), action: msgs::ErrorAction::IgnoreError}),
                        Some(id) => id,
                        if let Some(msg) = chan.get_signed_channel_announcement(&self.our_network_key, self.get_our_node_id(), self.genesis_hash.clone()) {
                                channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelAnnouncement {
                                        msg,
 -                                      update_msg: match self.get_channel_update(chan) {
 +                                      update_msg: match self.get_channel_update_for_broadcast(chan) {
                                                Ok(msg) => msg,
                                                Err(_) => continue,
                                        },
                                                                                        } else {
                                                                                                panic!("Stated return value requirements in send_htlc() were not met");
                                                                                        }
 -                                                                                      let chan_update = self.get_channel_update(chan.get()).unwrap();
 +                                                                                      let chan_update = self.get_channel_update_for_unicast(chan.get()).unwrap();
                                                                                        failed_forwards.push((htlc_source, payment_hash,
                                                                                                HTLCFailReason::Reason { failure_code: 0x1000 | 7, data: chan_update.encode_with_len() }
                                                                                        ));
                                                                                        if let Some(short_id) = channel.get_short_channel_id() {
                                                                                                channel_state.short_to_id.remove(&short_id);
                                                                                        }
 -                                                                                      Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, channel.force_shutdown(true), self.get_channel_update(&channel).ok()))
 +                                                                                      Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel_id, channel.force_shutdown(true), self.get_channel_update_for_broadcast(&channel).ok()))
                                                                                },
                                                                                ChannelError::CloseDelayBroadcast(_) => { panic!("Wait is only generated on receipt of channel_reestablish, which is handled by try_chan_entry, we don't bother to support it here"); }
                                                                        };
                                        ChannelUpdateStatus::DisabledStaged if chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Enabled),
                                        ChannelUpdateStatus::EnabledStaged if !chan.is_live() => chan.set_channel_update_status(ChannelUpdateStatus::Disabled),
                                        ChannelUpdateStatus::DisabledStaged if !chan.is_live() => {
 -                                              if let Ok(update) = self.get_channel_update(&chan) {
 +                                              if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
                                                        channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
                                                                msg: update
                                                        });
                                                chan.set_channel_update_status(ChannelUpdateStatus::Disabled);
                                        },
                                        ChannelUpdateStatus::EnabledStaged if chan.is_live() => {
 -                                              if let Ok(update) = self.get_channel_update(&chan) {
 +                                              if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
                                                        channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
                                                                msg: update
                                                        });
                                        let (failure_code, onion_failure_data) =
                                                match self.channel_state.lock().unwrap().by_id.entry(channel_id) {
                                                        hash_map::Entry::Occupied(chan_entry) => {
 -                                                              if let Ok(upd) = self.get_channel_update(&chan_entry.get()) {
 +                                                              if let Ok(upd) = self.get_channel_update_for_unicast(&chan_entry.get()) {
                                                                        (0x1000|7, upd.encode_with_len())
                                                                } else {
                                                                        (0x4000|10, Vec::new())
        pub fn channel_monitor_updated(&self, funding_txo: &OutPoint, highest_applied_update_id: u64) {
                let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
  
 -              let (mut pending_failures, chan_restoration_res) = {
 +              let chan_restoration_res;
 +              let mut pending_failures = {
                        let mut channel_lock = self.channel_state.lock().unwrap();
                        let channel_state = &mut *channel_lock;
                        let mut channel = match channel_state.by_id.entry(funding_txo.to_channel_id()) {
                        }
  
                        let (raa, commitment_update, order, pending_forwards, pending_failures, funding_broadcastable, funding_locked) = channel.get_mut().monitor_updating_restored(&self.logger);
 -                      (pending_failures, handle_chan_restoration_locked!(self, channel_lock, channel_state, channel, raa, commitment_update, order, None, pending_forwards, funding_broadcastable, funding_locked))
 +                      let channel_update = if funding_locked.is_some() && channel.get().is_usable() && !channel.get().should_announce() {
 +                              // We only send a channel_update in the case where we are just now sending a
 +                              // funding_locked and the channel is in a usable state. Further, we rely on the
 +                              // normal announcement_signatures process to send a channel_update for public
 +                              // channels, only generating a unicast channel_update if this is a private channel.
 +                              Some(events::MessageSendEvent::SendChannelUpdate {
 +                                      node_id: channel.get().get_counterparty_node_id(),
 +                                      msg: self.get_channel_update_for_unicast(channel.get()).unwrap(),
 +                              })
 +                      } else { None };
 +                      chan_restoration_res = handle_chan_restoration_locked!(self, channel_lock, channel_state, channel, raa, commitment_update, order, None, pending_forwards, funding_broadcastable, funding_locked);
 +                      if let Some(upd) = channel_update {
 +                              channel_state.pending_msg_events.push(upd);
 +                      }
 +                      pending_failures
                };
                post_handle_chan_restoration!(self, chan_restoration_res);
                for failure in pending_failures.drain(..) {
                                                node_id: counterparty_node_id.clone(),
                                                msg: announcement_sigs,
                                        });
 +                              } else if chan.get().is_usable() {
 +                                      channel_state.pending_msg_events.push(events::MessageSendEvent::SendChannelUpdate {
 +                                              node_id: counterparty_node_id.clone(),
 +                                              msg: self.get_channel_update_for_unicast(chan.get()).unwrap(),
 +                                      });
                                }
                                Ok(())
                        },
                        self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), htlc_source.0, &htlc_source.1, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() });
                }
                if let Some(chan) = chan_option {
 -                      if let Ok(update) = self.get_channel_update(&chan) {
 +                      if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
                                let mut channel_state = self.channel_state.lock().unwrap();
                                channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
                                        msg: update
                        self.tx_broadcaster.broadcast_transaction(&broadcast_tx);
                }
                if let Some(chan) = chan_option {
 -                      if let Ok(update) = self.get_channel_update(&chan) {
 +                      if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
                                let mut channel_state = self.channel_state.lock().unwrap();
                                channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
                                        msg: update
                                        // want to reject the new HTLC and fail it backwards instead of forwarding.
                                        match pending_forward_info {
                                                PendingHTLCStatus::Forward(PendingHTLCInfo { ref incoming_shared_secret, .. }) => {
 -                                                      let reason = if let Ok(upd) = self.get_channel_update(chan) {
 +                                                      let reason = if let Ok(upd) = self.get_channel_update_for_unicast(chan) {
                                                                onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &{
                                                                        let mut res = Vec::with_capacity(8 + 128);
                                                                        // TODO: underspecified, follow https://github.com/lightningnetwork/lightning-rfc/issues/791
  
                                channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelAnnouncement {
                                        msg: try_chan_entry!(self, chan.get_mut().announcement_signatures(&self.our_network_key, self.get_our_node_id(), self.genesis_hash.clone(), msg), channel_state, chan),
 -                                      update_msg: self.get_channel_update(chan.get()).unwrap(), // can only fail if we're not in a ready state
 +                                      // Note that announcement_signatures fails if the channel cannot be announced,
 +                                      // so get_channel_update_for_broadcast will never fail by the time we get here.
 +                                      update_msg: self.get_channel_update_for_broadcast(chan.get()).unwrap(),
                                });
                        },
                        hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
                                        }
                                        return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a channel_update for a channel from the wrong node - it shouldn't know about our private channels!".to_owned(), chan_id));
                                }
 -                              try_chan_entry!(self, chan.get_mut().channel_update(&msg), channel_state, chan);
 +                              let were_node_one = self.get_our_node_id().serialize()[..] < chan.get().get_counterparty_node_id().serialize()[..];
 +                              let msg_from_node_one = msg.contents.flags & 1 == 0;
 +                              if were_node_one == msg_from_node_one {
 +                                      return Ok(NotifyOption::SkipPersist);
 +                              } else {
 +                                      try_chan_entry!(self, chan.get_mut().channel_update(&msg), channel_state, chan);
 +                              }
                        },
                        hash_map::Entry::Vacant(_) => unreachable!()
                }
        }
  
        fn internal_channel_reestablish(&self, counterparty_node_id: &PublicKey, msg: &msgs::ChannelReestablish) -> Result<(), MsgHandleErrInternal> {
 -              let (htlcs_failed_forward, need_lnd_workaround, chan_restoration_res) = {
 +              let chan_restoration_res;
 +              let (htlcs_failed_forward, need_lnd_workaround) = {
                        let mut channel_state_lock = self.channel_state.lock().unwrap();
                        let channel_state = &mut *channel_state_lock;
  
                                        // add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here.
                                        let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, order, htlcs_failed_forward, shutdown) =
                                                try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan);
 +                                      let mut channel_update = None;
                                        if let Some(msg) = shutdown {
                                                channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
                                                        node_id: counterparty_node_id.clone(),
                                                        msg,
                                                });
 +                                      } else if chan.get().is_usable() {
 +                                              // If the channel is in a usable state (ie the channel is not being shut
 +                                              // down), send a unicast channel_update to our counterparty to make sure
 +                                              // they have the latest channel parameters.
 +                                              channel_update = Some(events::MessageSendEvent::SendChannelUpdate {
 +                                                      node_id: chan.get().get_counterparty_node_id(),
 +                                                      msg: self.get_channel_update_for_unicast(chan.get()).unwrap(),
 +                                              });
                                        }
                                        let need_lnd_workaround = chan.get_mut().workaround_lnd_bug_4006.take();
 -                                      (htlcs_failed_forward, need_lnd_workaround,
 -                                              handle_chan_restoration_locked!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), None, funding_locked))
 +                                      chan_restoration_res = handle_chan_restoration_locked!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), None, funding_locked);
 +                                      if let Some(upd) = channel_update {
 +                                              channel_state.pending_msg_events.push(upd);
 +                                      }
 +                                      (htlcs_failed_forward, need_lnd_workaround)
                                },
                                hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
                        }
                                                        short_to_id.remove(&short_id);
                                                }
                                                failed_channels.push(chan.force_shutdown(false));
 -                                              if let Ok(update) = self.get_channel_update(&chan) {
 +                                              if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
                                                        pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
                                                                msg: update
                                                        });
@@@ -4020,7 -3970,7 +4030,7 @@@ wher
                                let res = f(channel);
                                if let Ok((chan_res, mut timed_out_pending_htlcs)) = res {
                                        for (source, payment_hash) in timed_out_pending_htlcs.drain(..) {
 -                                              let chan_update = self.get_channel_update(&channel).map(|u| u.encode_with_len()).unwrap(); // Cannot add/recv HTLCs before we have a short_id so unwrap is safe
 +                                              let chan_update = self.get_channel_update_for_unicast(&channel).map(|u| u.encode_with_len()).unwrap(); // Cannot add/recv HTLCs before we have a short_id so unwrap is safe
                                                timed_out_htlcs.push((source, payment_hash,  HTLCFailReason::Reason {
                                                        failure_code: 0x1000 | 14, // expiry_too_soon, or at least it is now
                                                        data: chan_update,
                                                                node_id: channel.get_counterparty_node_id(),
                                                                msg: announcement_sigs,
                                                        });
 +                                              } else if channel.is_usable() {
 +                                                      log_trace!(self.logger, "Sending funding_locked WITHOUT announcement_signatures but with private channel_update for our counterparty on channel {}", log_bytes!(channel.channel_id()));
 +                                                      pending_msg_events.push(events::MessageSendEvent::SendChannelUpdate {
 +                                                              node_id: channel.get_counterparty_node_id(),
 +                                                              msg: self.get_channel_update_for_unicast(channel).unwrap(),
 +                                                      });
                                                } else {
                                                        log_trace!(self.logger, "Sending funding_locked WITHOUT announcement_signatures for {}", log_bytes!(channel.channel_id()));
                                                }
                                        // It looks like our counterparty went on-chain or funding transaction was
                                        // reorged out of the main chain. Close the channel.
                                        failed_channels.push(channel.force_shutdown(true));
 -                                      if let Ok(update) = self.get_channel_update(&channel) {
 +                                      if let Ok(update) = self.get_channel_update_for_broadcast(&channel) {
                                                pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
                                                        msg: update
                                                });
@@@ -4245,7 -4189,7 +4255,7 @@@ impl<Signer: Sign, M: Deref , T: Deref 
                                                        short_to_id.remove(&short_id);
                                                }
                                                failed_channels.push(chan.force_shutdown(true));
 -                                              if let Ok(update) = self.get_channel_update(&chan) {
 +                                              if let Ok(update) = self.get_channel_update_for_broadcast(&chan) {
                                                        pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate {
                                                                msg: update
                                                        });
                                        &events::MessageSendEvent::BroadcastChannelAnnouncement { .. } => true,
                                        &events::MessageSendEvent::BroadcastNodeAnnouncement { .. } => true,
                                        &events::MessageSendEvent::BroadcastChannelUpdate { .. } => true,
 +                                      &events::MessageSendEvent::SendChannelUpdate { ref node_id, .. } => node_id != counterparty_node_id,
                                        &events::MessageSendEvent::HandleError { ref node_id, .. } => node_id != counterparty_node_id,
                                        &events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => true,
                                        &events::MessageSendEvent::SendChannelRangeQuery { .. } => false,
  
                if msg.channel_id == [0; 32] {
                        for chan in self.list_channels() {
-                               if chan.remote_network_id == *counterparty_node_id {
+                               if chan.counterparty.node_id == *counterparty_node_id {
                                        // Untrusted messages from peer, we throw away the error if id points to a non-existent channel
                                        let _ = self.force_close_channel_with_peer(&chan.channel_id, Some(counterparty_node_id));
                                }
@@@ -5022,31 -4965,6 +5032,31 @@@ mod tests 
                // At this point the channel info given by peers should still be the same.
                assert_eq!(nodes[0].node.list_channels()[0], node_a_chan_info);
                assert_eq!(nodes[1].node.list_channels()[0], node_b_chan_info);
 +
 +              // An earlier version of handle_channel_update didn't check the directionality of the
 +              // update message and would always update the local fee info, even if our peer was
 +              // (spuriously) forwarding us our own channel_update.
 +              let as_node_one = nodes[0].node.get_our_node_id().serialize()[..] < nodes[1].node.get_our_node_id().serialize()[..];
 +              let as_update = if as_node_one == (chan.0.contents.flags & 1 == 0 /* chan.0 is from node one */) { &chan.0 } else { &chan.1 };
 +              let bs_update = if as_node_one == (chan.0.contents.flags & 1 == 0 /* chan.0 is from node one */) { &chan.1 } else { &chan.0 };
 +
 +              // First deliver each peers' own message, checking that the node doesn't need to be
 +              // persisted and that its channel info remains the same.
 +              nodes[0].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &as_update);
 +              nodes[1].node.handle_channel_update(&nodes[0].node.get_our_node_id(), &bs_update);
 +              assert!(!nodes[0].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +              assert!(!nodes[1].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +              assert_eq!(nodes[0].node.list_channels()[0], node_a_chan_info);
 +              assert_eq!(nodes[1].node.list_channels()[0], node_b_chan_info);
 +
 +              // Finally, deliver the other peers' message, ensuring each node needs to be persisted and
 +              // the channel info has updated.
 +              nodes[0].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &bs_update);
 +              nodes[1].node.handle_channel_update(&nodes[0].node.get_our_node_id(), &as_update);
 +              assert!(nodes[0].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +              assert!(nodes[1].node.await_persistable_update_timeout(Duration::from_millis(1)));
 +              assert_ne!(nodes[0].node.list_channels()[0], node_a_chan_info);
 +              assert_ne!(nodes[1].node.list_channels()[0], node_b_chan_info);
        }
  }
  
@@@ -5147,19 -5065,7 +5157,19 @@@ pub mod bench 
                Listen::block_connected(&node_b, &block, 1);
  
                node_a.handle_funding_locked(&node_b.get_our_node_id(), &get_event_msg!(node_b_holder, MessageSendEvent::SendFundingLocked, node_a.get_our_node_id()));
 -              node_b.handle_funding_locked(&node_a.get_our_node_id(), &get_event_msg!(node_a_holder, MessageSendEvent::SendFundingLocked, node_b.get_our_node_id()));
 +              let msg_events = node_a.get_and_clear_pending_msg_events();
 +              assert_eq!(msg_events.len(), 2);
 +              match msg_events[0] {
 +                      MessageSendEvent::SendFundingLocked { ref msg, .. } => {
 +                              node_b.handle_funding_locked(&node_a.get_our_node_id(), msg);
 +                              get_event_msg!(node_b_holder, MessageSendEvent::SendChannelUpdate, node_a.get_our_node_id());
 +                      },
 +                      _ => panic!(),
 +              }
 +              match msg_events[1] {
 +                      MessageSendEvent::SendChannelUpdate { .. } => {},
 +                      _ => panic!(),
 +              }
  
                let dummy_graph = NetworkGraph::new(genesis_hash);