Merge pull request #1077 from jkczyz/2021-09-failing-route-hop
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Wed, 13 Oct 2021 01:13:41 +0000 (01:13 +0000)
committerGitHub <noreply@github.com>
Wed, 13 Oct 2021 01:13:41 +0000 (01:13 +0000)
Include short channel id in PaymentPathFailed

1  2 
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_tests.rs
lightning/src/routing/network_graph.rs
lightning/src/util/events.rs

index 4a3c69f9995f882ecf5933117a79720f7cc81561,e2584c4716a3528491b9df3f5586c3770674debe..298f88ce0cc018162cf5f2a3201512641b1cd8fd
@@@ -172,20 -172,20 +172,20 @@@ struct ClaimableHTLC 
        onion_payload: OnionPayload,
  }
  
 -/// A payment identifier used to correlate an MPP payment's per-path HTLC sources internally.
 +/// A payment identifier used to uniquely identify a payment to LDK.
  #[derive(Hash, Copy, Clone, PartialEq, Eq, Debug)]
 -pub(crate) struct MppId(pub [u8; 32]);
 +pub struct PaymentId(pub [u8; 32]);
  
 -impl Writeable for MppId {
 +impl Writeable for PaymentId {
        fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
                self.0.write(w)
        }
  }
  
 -impl Readable for MppId {
 +impl Readable for PaymentId {
        fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
                let buf: [u8; 32] = Readable::read(r)?;
 -              Ok(MppId(buf))
 +              Ok(PaymentId(buf))
        }
  }
  /// Tracks the inbound corresponding to an outbound HTLC
@@@ -198,7 -198,7 +198,7 @@@ pub(crate) enum HTLCSource 
                /// Technically we can recalculate this from the route, but we cache it here to avoid
                /// doing a double-pass on route when we get a failure back
                first_hop_htlc_msat: u64,
 -              mpp_id: MppId,
 +              payment_id: PaymentId,
        },
  }
  #[cfg(test)]
@@@ -208,7 -208,7 +208,7 @@@ impl HTLCSource 
                        path: Vec::new(),
                        session_priv: SecretKey::from_slice(&[1; 32]).unwrap(),
                        first_hop_htlc_msat: 0,
 -                      mpp_id: MppId([2; 32]),
 +                      payment_id: PaymentId([2; 32]),
                }
        }
  }
@@@ -400,65 -400,6 +400,65 @@@ struct PendingInboundPayment 
        min_value_msat: Option<u64>,
  }
  
 +/// Stores the session_priv for each part of a payment that is still pending. For versions 0.0.102
 +/// and later, also stores information for retrying the payment.
 +pub(crate) enum PendingOutboundPayment {
 +      Legacy {
 +              session_privs: HashSet<[u8; 32]>,
 +      },
 +      Retryable {
 +              session_privs: HashSet<[u8; 32]>,
 +              payment_hash: PaymentHash,
 +              payment_secret: Option<PaymentSecret>,
 +              pending_amt_msat: u64,
 +              /// The total payment amount across all paths, used to verify that a retry is not overpaying.
 +              total_msat: u64,
 +              /// Our best known block height at the time this payment was initiated.
 +              starting_block_height: u32,
 +      },
 +}
 +
 +impl PendingOutboundPayment {
 +      fn remove(&mut self, session_priv: &[u8; 32], part_amt_msat: u64) -> bool {
 +              let remove_res = match self {
 +                      PendingOutboundPayment::Legacy { session_privs } |
 +                      PendingOutboundPayment::Retryable { session_privs, .. } => {
 +                              session_privs.remove(session_priv)
 +                      }
 +              };
 +              if remove_res {
 +                      if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
 +                              *pending_amt_msat -= part_amt_msat;
 +                      }
 +              }
 +              remove_res
 +      }
 +
 +      fn insert(&mut self, session_priv: [u8; 32], part_amt_msat: u64) -> bool {
 +              let insert_res = match self {
 +                      PendingOutboundPayment::Legacy { session_privs } |
 +                      PendingOutboundPayment::Retryable { session_privs, .. } => {
 +                              session_privs.insert(session_priv)
 +                      }
 +              };
 +              if insert_res {
 +                      if let PendingOutboundPayment::Retryable { ref mut pending_amt_msat, .. } = self {
 +                              *pending_amt_msat += part_amt_msat;
 +                      }
 +              }
 +              insert_res
 +      }
 +
 +      fn remaining_parts(&self) -> usize {
 +              match self {
 +                      PendingOutboundPayment::Legacy { session_privs } |
 +                      PendingOutboundPayment::Retryable { session_privs, .. } => {
 +                              session_privs.len()
 +                      }
 +              }
 +      }
 +}
 +
  /// SimpleArcChannelManager is useful when you need a ChannelManager with a static lifetime, e.g.
  /// when you're using lightning-net-tokio (since tokio::spawn requires parameters with static
  /// lifetimes). Other times you can afford a reference, which is more efficient, in which case
@@@ -545,7 -486,7 +545,7 @@@ pub struct ChannelManager<Signer: Sign
        /// Locked *after* channel_state.
        pending_inbound_payments: Mutex<HashMap<PaymentHash, PendingInboundPayment>>,
  
 -      /// The session_priv bytes of outbound payments which are pending resolution.
 +      /// The session_priv bytes and retry metadata of outbound payments which are pending resolution.
        /// The authoritative state of these HTLCs resides either within Channels or ChannelMonitors
        /// (if the channel has been force-closed), however we track them here to prevent duplicative
        /// PaymentSent/PaymentPathFailed events. Specifically, in the case of a duplicative
        /// which may generate a claim event, we may receive similar duplicate claim/fail MonitorEvents
        /// after reloading from disk while replaying blocks against ChannelMonitors.
        ///
 -      /// Each payment has each of its MPP part's session_priv bytes in the HashSet of the map (even
 -      /// payments over a single path).
 +      /// See `PendingOutboundPayment` documentation for more info.
        ///
        /// Locked *after* channel_state.
 -      pending_outbound_payments: Mutex<HashMap<MppId, HashSet<[u8; 32]>>>,
 +      pending_outbound_payments: Mutex<HashMap<PaymentId, PendingOutboundPayment>>,
  
        our_network_key: SecretKey,
        our_network_pubkey: PublicKey,
@@@ -1384,18 -1326,6 +1384,18 @@@ impl<Signer: Sign, M: Deref, T: Deref, 
                self.list_channels_with_filter(|&(_, ref channel)| channel.is_live())
        }
  
 +      /// Helper function that issues the channel close events
 +      fn issue_channel_close_events(&self, channel: &Channel<Signer>, closure_reason: ClosureReason) {
 +              let mut pending_events_lock = self.pending_events.lock().unwrap();
 +              match channel.unbroadcasted_funding() {
 +                      Some(transaction) => {
 +                              pending_events_lock.push(events::Event::DiscardFunding { channel_id: channel.channel_id(), transaction })
 +                      },
 +                      None => {},
 +              }
 +              pending_events_lock.push(events::Event::ChannelClosed { channel_id: channel.channel_id(), reason: closure_reason });
 +      }
 +
        fn close_channel_internal(&self, channel_id: &[u8; 32], target_feerate_sats_per_1000_weight: Option<u32>) -> Result<(), APIError> {
                let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
  
                                                                msg: channel_update
                                                        });
                                                }
 -                                              if let Ok(mut pending_events_lock) = self.pending_events.lock() {
 -                                                      pending_events_lock.push(events::Event::ChannelClosed {
 -                                                              channel_id: *channel_id,
 -                                                              reason: ClosureReason::HolderForceClosed
 -                                                      });
 -                                              }
 +                                              self.issue_channel_close_events(&channel, ClosureReason::HolderForceClosed);
                                        }
                                        break Ok(());
                                },
                                if let Some(short_id) = chan.get().get_short_channel_id() {
                                        channel_state.short_to_id.remove(&short_id);
                                }
 -                              let mut pending_events_lock = self.pending_events.lock().unwrap();
                                if peer_node_id.is_some() {
                                        if let Some(peer_msg) = peer_msg {
 -                                              pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, reason: ClosureReason::CounterpartyForceClosed { peer_msg: peer_msg.to_string() } });
 +                                              self.issue_channel_close_events(chan.get(),ClosureReason::CounterpartyForceClosed { peer_msg: peer_msg.to_string() });
                                        }
                                } else {
 -                                      pending_events_lock.push(events::Event::ChannelClosed { channel_id: *channel_id, reason: ClosureReason::HolderForceClosed });
 +                                      self.issue_channel_close_events(chan.get(),ClosureReason::HolderForceClosed);
                                }
                                chan.remove_entry().1
                        } else {
        }
  
        // Only public for testing, this should otherwise never be called direcly
 -      pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32, mpp_id: MppId, keysend_preimage: &Option<PaymentPreimage>) -> Result<(), APIError> {
 +      pub(crate) fn send_payment_along_path(&self, path: &Vec<RouteHop>, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>, total_value: u64, cur_height: u32, payment_id: PaymentId, keysend_preimage: &Option<PaymentPreimage>) -> Result<(), APIError> {
                log_trace!(self.logger, "Attempting to send payment for path with next hop {}", path.first().unwrap().short_channel_id);
                let prng_seed = self.keys_manager.get_secure_random_bytes();
                let session_priv_bytes = self.keys_manager.get_secure_random_bytes();
                let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash);
  
                let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 -              let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
 -              let sessions = pending_outbounds.entry(mpp_id).or_insert(HashSet::new());
 -              assert!(sessions.insert(session_priv_bytes));
  
                let err: Result<(), _> = loop {
                        let mut channel_lock = self.channel_state.lock().unwrap();
                                        if !chan.get().is_live() {
                                                return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!".to_owned()});
                                        }
 -                                      break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
 -                                              path: path.clone(),
 -                                              session_priv: session_priv.clone(),
 -                                              first_hop_htlc_msat: htlc_msat,
 -                                              mpp_id,
 -                                      }, onion_packet, &self.logger), channel_state, chan)
 +                                      let send_res = break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(
 +                                              htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
 +                                                      path: path.clone(),
 +                                                      session_priv: session_priv.clone(),
 +                                                      first_hop_htlc_msat: htlc_msat,
 +                                                      payment_id,
 +                                              }, onion_packet, &self.logger),
 +                                      channel_state, chan);
 +
 +                                      let mut pending_outbounds = self.pending_outbound_payments.lock().unwrap();
 +                                      let payment = pending_outbounds.entry(payment_id).or_insert_with(|| PendingOutboundPayment::Retryable {
 +                                              session_privs: HashSet::new(),
 +                                              pending_amt_msat: 0,
 +                                              payment_hash: *payment_hash,
 +                                              payment_secret: *payment_secret,
 +                                              starting_block_height: self.best_block.read().unwrap().height(),
 +                                              total_msat: total_value,
 +                                      });
 +                                      assert!(payment.insert(session_priv_bytes, path.last().unwrap().fee_msat));
 +
 +                                      send_res
                                } {
                                        Some((update_add, commitment_signed, monitor_update)) => {
                                                if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
        /// If a payment_secret *is* provided, we assume that the invoice had the payment_secret feature
        /// bit set (either as required or as available). If multiple paths are present in the Route,
        /// we assume the invoice had the basic_mpp feature set.
 -      pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>) -> Result<(), PaymentSendFailure> {
 -              self.send_payment_internal(route, payment_hash, payment_secret, None)
 +      pub fn send_payment(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>) -> Result<PaymentId, PaymentSendFailure> {
 +              self.send_payment_internal(route, payment_hash, payment_secret, None, None, None)
        }
  
 -      fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>) -> Result<(), PaymentSendFailure> {
 +      fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: Option<PaymentId>, recv_value_msat: Option<u64>) -> Result<PaymentId, PaymentSendFailure> {
                if route.paths.len() < 1 {
                        return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"}));
                }
                let mut total_value = 0;
                let our_node_id = self.get_our_node_id();
                let mut path_errs = Vec::with_capacity(route.paths.len());
 -              let mpp_id = MppId(self.keys_manager.get_secure_random_bytes());
 +              let payment_id = if let Some(id) = payment_id { id } else { PaymentId(self.keys_manager.get_secure_random_bytes()) };
                'path_check: for path in route.paths.iter() {
                        if path.len() < 1 || path.len() > 20 {
                                path_errs.push(Err(APIError::RouteError{err: "Path didn't go anywhere/had bogus size"}));
                if path_errs.iter().any(|e| e.is_err()) {
                        return Err(PaymentSendFailure::PathParameterError(path_errs));
                }
 +              if let Some(amt_msat) = recv_value_msat {
 +                      debug_assert!(amt_msat >= total_value);
 +                      total_value = amt_msat;
 +              }
  
                let cur_height = self.best_block.read().unwrap().height() + 1;
                let mut results = Vec::new();
                for path in route.paths.iter() {
 -                      results.push(self.send_payment_along_path(&path, &payment_hash, payment_secret, total_value, cur_height, mpp_id, &keysend_preimage));
 +                      results.push(self.send_payment_along_path(&path, &payment_hash, payment_secret, total_value, cur_height, payment_id, &keysend_preimage));
                }
                let mut has_ok = false;
                let mut has_err = false;
                } else if has_err {
                        Err(PaymentSendFailure::AllFailedRetrySafe(results.drain(..).map(|r| r.unwrap_err()).collect()))
                } else {
 -                      Ok(())
 +                      Ok(payment_id)
                }
        }
  
 +      /// Retries a payment along the given [`Route`].
 +      ///
 +      /// Errors returned are a superset of those returned from [`send_payment`], so see
 +      /// [`send_payment`] documentation for more details on errors. This method will also error if the
 +      /// retry amount puts the payment more than 10% over the payment's total amount, or if the payment
 +      /// for the given `payment_id` cannot be found (likely due to timeout or success).
 +      ///
 +      /// [`send_payment`]: [`ChannelManager::send_payment`]
 +      pub fn retry_payment(&self, route: &Route, payment_id: PaymentId) -> Result<(), PaymentSendFailure> {
 +              const RETRY_OVERFLOW_PERCENTAGE: u64 = 10;
 +              for path in route.paths.iter() {
 +                      if path.len() == 0 {
 +                              return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
 +                                      err: "length-0 path in route".to_string()
 +                              }))
 +                      }
 +              }
 +
 +              let (total_msat, payment_hash, payment_secret) = {
 +                      let outbounds = self.pending_outbound_payments.lock().unwrap();
 +                      if let Some(payment) = outbounds.get(&payment_id) {
 +                              match payment {
 +                                      PendingOutboundPayment::Retryable {
 +                                              total_msat, payment_hash, payment_secret, pending_amt_msat, ..
 +                                      } => {
 +                                              let retry_amt_msat: u64 = route.paths.iter().map(|path| path.last().unwrap().fee_msat).sum();
 +                                              if retry_amt_msat + *pending_amt_msat > *total_msat * (100 + RETRY_OVERFLOW_PERCENTAGE) / 100 {
 +                                                      return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
 +                                                              err: format!("retry_amt_msat of {} will put pending_amt_msat (currently: {}) more than 10% over total_payment_amt_msat of {}", retry_amt_msat, pending_amt_msat, total_msat).to_string()
 +                                                      }))
 +                                              }
 +                                              (*total_msat, *payment_hash, *payment_secret)
 +                                      },
 +                                      PendingOutboundPayment::Legacy { .. } => {
 +                                              return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
 +                                                      err: "Unable to retry payments that were initially sent on LDK versions prior to 0.0.102".to_string()
 +                                              }))
 +                                      }
 +                              }
 +                      } else {
 +                              return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError {
 +                                      err: format!("Payment with ID {} not found", log_bytes!(payment_id.0)),
 +                              }))
 +                      }
 +              };
 +              return self.send_payment_internal(route, payment_hash, &payment_secret, None, Some(payment_id), Some(total_msat)).map(|_| ())
 +      }
 +
        /// Send a spontaneous payment, which is a payment that does not require the recipient to have
        /// generated an invoice. Optionally, you may specify the preimage. If you do choose to specify
        /// the preimage, it must be a cryptographically secure random value that no intermediate node
        /// Note that `route` must have exactly one path.
        ///
        /// [`send_payment`]: Self::send_payment
 -      pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option<PaymentPreimage>) -> Result<PaymentHash, PaymentSendFailure> {
 +      pub fn send_spontaneous_payment(&self, route: &Route, payment_preimage: Option<PaymentPreimage>) -> Result<(PaymentHash, PaymentId), PaymentSendFailure> {
                let preimage = match payment_preimage {
                        Some(p) => p,
                        None => PaymentPreimage(self.keys_manager.get_secure_random_bytes()),
                };
                let payment_hash = PaymentHash(Sha256::hash(&preimage.0).into_inner());
 -              match self.send_payment_internal(route, payment_hash, &None, Some(preimage)) {
 -                      Ok(()) => Ok(payment_hash),
 +              match self.send_payment_internal(route, payment_hash, &None, Some(preimage), None, None) {
 +                      Ok(payment_id) => Ok((payment_hash, payment_id)),
                        Err(e) => Err(e)
                }
        }
                                        self.fail_htlc_backwards_internal(channel_state,
                                                htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data});
                                },
 -                              HTLCSource::OutboundRoute { session_priv, mpp_id, path, .. } => {
 +                              HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
                                        let mut session_priv_bytes = [0; 32];
                                        session_priv_bytes.copy_from_slice(&session_priv[..]);
                                        let mut outbounds = self.pending_outbound_payments.lock().unwrap();
 -                                      if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(mpp_id) {
 -                                              if sessions.get_mut().remove(&session_priv_bytes) {
 +                                      if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
 +                                              if payment.get_mut().remove(&session_priv_bytes, path.last().unwrap().fee_msat) {
                                                        self.pending_events.lock().unwrap().push(
                                                                events::Event::PaymentPathFailed {
                                                                        payment_hash,
                                                                        rejected_by_dest: false,
                                                                        network_update: None,
 -                                                                      all_paths_failed: sessions.get().len() == 0,
 +                                                                      all_paths_failed: payment.get().remaining_parts() == 0,
                                                                        path: path.clone(),
+                                                                       short_channel_id: None,
                                                                        #[cfg(test)]
                                                                        error_code: None,
                                                                        #[cfg(test)]
                                                                        error_data: None,
                                                                }
                                                        );
 -                                                      if sessions.get().len() == 0 {
 -                                                              sessions.remove();
 -                                                      }
                                                }
                                        } else {
                                                log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
                // from block_connected which may run during initialization prior to the chain_monitor
                // being fully configured. See the docs for `ChannelManagerReadArgs` for more.
                match source {
 -                      HTLCSource::OutboundRoute { ref path, session_priv, mpp_id, .. } => {
 +                      HTLCSource::OutboundRoute { ref path, session_priv, payment_id, .. } => {
                                let mut session_priv_bytes = [0; 32];
                                session_priv_bytes.copy_from_slice(&session_priv[..]);
                                let mut outbounds = self.pending_outbound_payments.lock().unwrap();
                                let mut all_paths_failed = false;
 -                              if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(mpp_id) {
 -                                      if !sessions.get_mut().remove(&session_priv_bytes) {
 +                              if let hash_map::Entry::Occupied(mut sessions) = outbounds.entry(payment_id) {
 +                                      if !sessions.get_mut().remove(&session_priv_bytes, path.last().unwrap().fee_msat) {
                                                log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
                                                return;
                                        }
 -                                      if sessions.get().len() == 0 {
 +                                      if sessions.get().remaining_parts() == 0 {
                                                all_paths_failed = true;
 -                                              sessions.remove();
                                        }
                                } else {
                                        log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
                                match &onion_error {
                                        &HTLCFailReason::LightningError { ref err } => {
  #[cfg(test)]
-                                               let (network_update, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
+                                               let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
  #[cfg(not(test))]
-                                               let (network_update, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
+                                               let (network_update, short_channel_id, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
                                                // TODO: If we decided to blame ourselves (or one of our channels) in
                                                // process_onion_failure we should close that channel as it implies our
                                                // next-hop is needlessly blaming us!
                                                                network_update,
                                                                all_paths_failed,
                                                                path: path.clone(),
+                                                               short_channel_id,
  #[cfg(test)]
                                                                error_code: onion_error_code,
  #[cfg(test)]
                                                                network_update: None,
                                                                all_paths_failed,
                                                                path: path.clone(),
+                                                               short_channel_id: Some(path.first().unwrap().short_channel_id),
  #[cfg(test)]
                                                                error_code: Some(*failure_code),
  #[cfg(test)]
  
        fn claim_funds_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<Signer>>, source: HTLCSource, payment_preimage: PaymentPreimage, forwarded_htlc_value_msat: Option<u64>, from_onchain: bool) {
                match source {
 -                      HTLCSource::OutboundRoute { session_priv, mpp_id, .. } => {
 +                      HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } => {
                                mem::drop(channel_state_lock);
                                let mut session_priv_bytes = [0; 32];
                                session_priv_bytes.copy_from_slice(&session_priv[..]);
                                let mut outbounds = self.pending_outbound_payments.lock().unwrap();
 -                              let found_payment = if let Some(mut sessions) = outbounds.remove(&mpp_id) {
 -                                      sessions.remove(&session_priv_bytes)
 +                              let found_payment = if let Some(mut sessions) = outbounds.remove(&payment_id) {
 +                                      sessions.remove(&session_priv_bytes, path.last().unwrap().fee_msat)
                                } else { false };
                                if found_payment {
 +                                      let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
                                        self.pending_events.lock().unwrap().push(
 -                                              events::Event::PaymentSent { payment_preimage }
 +                                              events::Event::PaymentSent {
 +                                                      payment_preimage,
 +                                                      payment_hash: payment_hash
 +                                              }
                                        );
                                } else {
                                        log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0));
                                                Err(e) => try_chan_entry!(self, Err(e), channel_state, chan),
                                        };
                                        if let Err(e) = self.chain_monitor.watch_channel(chan.get().get_funding_txo().unwrap(), monitor) {
 -                                              return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, false, false);
 +                                              let mut res = handle_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, false, false);
 +                                              if let Err(MsgHandleErrInternal { ref mut shutdown_finish, .. }) = res {
 +                                                      // We weren't able to watch the channel to begin with, so no updates should be made on
 +                                                      // it. Previously, full_stack_target found an (unreachable) panic when the
 +                                                      // monitor update contained within `shutdown_finish` was applied.
 +                                                      if let Some((ref mut shutdown_finish, _)) = shutdown_finish {
 +                                                              shutdown_finish.0.take();
 +                                                      }
 +                                              }
 +                                              return res
                                        }
                                        funding_tx
                                },
                                        msg: update
                                });
                        }
 -                      self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: msg.channel_id,  reason: ClosureReason::CooperativeClosure });
 +                      self.issue_channel_close_events(&chan, ClosureReason::CooperativeClosure);
                }
                Ok(())
        }
                                                                msg: update
                                                        });
                                                }
 -                                              self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  reason: ClosureReason::CommitmentTxConfirmed });
 +                                              self.issue_channel_close_events(&chan, ClosureReason::CommitmentTxConfirmed);
                                                pending_msg_events.push(events::MessageSendEvent::HandleError {
                                                        node_id: chan.get_counterparty_node_id(),
                                                        action: msgs::ErrorAction::SendErrorMessage {
                                                                });
                                                        }
  
 -                                                      if let Ok(mut pending_events_lock) = self.pending_events.lock() {
 -                                                              pending_events_lock.push(events::Event::ChannelClosed {
 -                                                                      channel_id: *channel_id,
 -                                                                      reason: ClosureReason::CooperativeClosure
 -                                                              });
 -                                                      }
 +                                                      self.issue_channel_close_events(chan, ClosureReason::CooperativeClosure);
  
                                                        log_info!(self.logger, "Broadcasting {}", log_tx!(tx));
                                                        self.tx_broadcaster.broadcast_transaction(&tx);
                self.process_pending_events(&event_handler);
                events.into_inner()
        }
 +
 +      #[cfg(test)]
 +      pub fn has_pending_payments(&self) -> bool {
 +              !self.pending_outbound_payments.lock().unwrap().is_empty()
 +      }
  }
  
  impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> MessageSendEventsProvider for ChannelManager<Signer, M, T, K, F, L>
@@@ -4573,16 -4439,6 +4576,16 @@@ wher
                payment_secrets.retain(|_, inbound_payment| {
                        inbound_payment.expiry_time > header.time as u64
                });
 +
 +              let mut outbounds = self.pending_outbound_payments.lock().unwrap();
 +              outbounds.retain(|_, payment| {
 +                      const PAYMENT_EXPIRY_BLOCKS: u32 = 3;
 +                      if payment.remaining_parts() != 0 { return true }
 +                      if let PendingOutboundPayment::Retryable { starting_block_height, .. } = payment {
 +                              return *starting_block_height + PAYMENT_EXPIRY_BLOCKS > height
 +                      }
 +                      true
 +              });
        }
  
        fn get_relevant_txids(&self) -> Vec<Txid> {
@@@ -4676,7 -4532,7 +4679,7 @@@ wher
                                                        msg: update
                                                });
                                        }
 -                                      self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: channel.channel_id(),  reason: ClosureReason::CommitmentTxConfirmed });
 +                                      self.issue_channel_close_events(channel, ClosureReason::CommitmentTxConfirmed);
                                        pending_msg_events.push(events::MessageSendEvent::HandleError {
                                                node_id: channel.get_counterparty_node_id(),
                                                action: msgs::ErrorAction::SendErrorMessage { msg: e },
@@@ -4867,7 -4723,7 +4870,7 @@@ impl<Signer: Sign, M: Deref , T: Deref 
                                                                msg: update
                                                        });
                                                }
 -                                              self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  reason: ClosureReason::DisconnectedPeer });
 +                                              self.issue_channel_close_events(chan, ClosureReason::DisconnectedPeer);
                                                false
                                        } else {
                                                true
                                                        if let Some(short_id) = chan.get_short_channel_id() {
                                                                short_to_id.remove(&short_id);
                                                        }
 -                                                      self.pending_events.lock().unwrap().push(events::Event::ChannelClosed { channel_id: chan.channel_id(),  reason: ClosureReason::DisconnectedPeer });
 +                                                      self.issue_channel_close_events(chan, ClosureReason::DisconnectedPeer);
                                                        return false;
                                                } else {
                                                        no_channels_remain = false;
@@@ -5226,23 -5082,23 +5229,23 @@@ impl Readable for HTLCSource 
                                let mut session_priv: ::util::ser::OptionDeserWrapper<SecretKey> = ::util::ser::OptionDeserWrapper(None);
                                let mut first_hop_htlc_msat: u64 = 0;
                                let mut path = Some(Vec::new());
 -                              let mut mpp_id = None;
 +                              let mut payment_id = None;
                                read_tlv_fields!(reader, {
                                        (0, session_priv, required),
 -                                      (1, mpp_id, option),
 +                                      (1, payment_id, option),
                                        (2, first_hop_htlc_msat, required),
                                        (4, path, vec_type),
                                });
 -                              if mpp_id.is_none() {
 -                                      // For backwards compat, if there was no mpp_id written, use the session_priv bytes
 +                              if payment_id.is_none() {
 +                                      // For backwards compat, if there was no payment_id written, use the session_priv bytes
                                        // instead.
 -                                      mpp_id = Some(MppId(*session_priv.0.unwrap().as_ref()));
 +                                      payment_id = Some(PaymentId(*session_priv.0.unwrap().as_ref()));
                                }
                                Ok(HTLCSource::OutboundRoute {
                                        session_priv: session_priv.0.unwrap(),
                                        first_hop_htlc_msat: first_hop_htlc_msat,
                                        path: path.unwrap(),
 -                                      mpp_id: mpp_id.unwrap(),
 +                                      payment_id: payment_id.unwrap(),
                                })
                        }
                        1 => Ok(HTLCSource::PreviousHopData(Readable::read(reader)?)),
  impl Writeable for HTLCSource {
        fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::io::Error> {
                match self {
 -                      HTLCSource::OutboundRoute { ref session_priv, ref first_hop_htlc_msat, ref path, mpp_id } => {
 +                      HTLCSource::OutboundRoute { ref session_priv, ref first_hop_htlc_msat, ref path, payment_id } => {
                                0u8.write(writer)?;
 -                              let mpp_id_opt = Some(mpp_id);
 +                              let payment_id_opt = Some(payment_id);
                                write_tlv_fields!(writer, {
                                        (0, session_priv, required),
 -                                      (1, mpp_id_opt, option),
 +                                      (1, payment_id_opt, option),
                                        (2, first_hop_htlc_msat, required),
                                        (4, path, vec_type),
                                 });
@@@ -5304,20 -5160,6 +5307,20 @@@ impl_writeable_tlv_based!(PendingInboun
        (8, min_value_msat, required),
  });
  
 +impl_writeable_tlv_based_enum!(PendingOutboundPayment,
 +      (0, Legacy) => {
 +              (0, session_privs, required),
 +      },
 +      (2, Retryable) => {
 +              (0, session_privs, required),
 +              (2, payment_hash, required),
 +              (4, payment_secret, option),
 +              (6, total_msat, required),
 +              (8, pending_amt_msat, required),
 +              (10, starting_block_height, required),
 +      },
 +;);
 +
  impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable for ChannelManager<Signer, M, T, K, F, L>
        where M::Target: chain::Watch<Signer>,
          T::Target: BroadcasterInterface,
                let pending_outbound_payments = self.pending_outbound_payments.lock().unwrap();
                // For backwards compat, write the session privs and their total length.
                let mut num_pending_outbounds_compat: u64 = 0;
 -              for (_, outbounds) in pending_outbound_payments.iter() {
 -                      num_pending_outbounds_compat += outbounds.len() as u64;
 +              for (_, outbound) in pending_outbound_payments.iter() {
 +                      num_pending_outbounds_compat += outbound.remaining_parts() as u64;
                }
                num_pending_outbounds_compat.write(writer)?;
 -              for (_, outbounds) in pending_outbound_payments.iter() {
 -                      for outbound in outbounds.iter() {
 -                              outbound.write(writer)?;
 +              for (_, outbound) in pending_outbound_payments.iter() {
 +                      match outbound {
 +                              PendingOutboundPayment::Legacy { session_privs } |
 +                              PendingOutboundPayment::Retryable { session_privs, .. } => {
 +                                      for session_priv in session_privs.iter() {
 +                                              session_priv.write(writer)?;
 +                                      }
 +                              }
                        }
                }
  
 +              // Encode without retry info for 0.0.101 compatibility.
 +              let mut pending_outbound_payments_no_retry: HashMap<PaymentId, HashSet<[u8; 32]>> = HashMap::new();
 +              for (id, outbound) in pending_outbound_payments.iter() {
 +                      match outbound {
 +                              PendingOutboundPayment::Legacy { session_privs } |
 +                              PendingOutboundPayment::Retryable { session_privs, .. } => {
 +                                      pending_outbound_payments_no_retry.insert(*id, session_privs.clone());
 +                              }
 +                      }
 +              }
                write_tlv_fields!(writer, {
 -                      (1, pending_outbound_payments, required),
 +                      (1, pending_outbound_payments_no_retry, required),
 +                      (3, pending_outbound_payments, required),
                });
  
                Ok(())
@@@ -5673,16 -5499,6 +5676,16 @@@ impl<'a, Signer: Sign, M: Deref, T: Der
                                None => continue,
                        }
                }
 +              if forward_htlcs_count > 0 {
 +                      // If we have pending HTLCs to forward, assume we either dropped a
 +                      // `PendingHTLCsForwardable` or the user received it but never processed it as they
 +                      // shut down before the timer hit. Either way, set the time_forwardable to a small
 +                      // constant as enough time has likely passed that we should simply handle the forwards
 +                      // now, or at least after the user gets a chance to reconnect to our peers.
 +                      pending_events_read.push(events::Event::PendingHTLCsForwardable {
 +                              time_forwardable: Duration::from_secs(2),
 +                      });
 +              }
  
                let background_event_count: u64 = Readable::read(reader)?;
                let mut pending_background_events_read: Vec<BackgroundEvent> = Vec::with_capacity(cmp::min(background_event_count as usize, MAX_ALLOC_SIZE/mem::size_of::<BackgroundEvent>()));
                }
  
                let pending_outbound_payments_count_compat: u64 = Readable::read(reader)?;
 -              let mut pending_outbound_payments_compat: HashMap<MppId, HashSet<[u8; 32]>> =
 +              let mut pending_outbound_payments_compat: HashMap<PaymentId, PendingOutboundPayment> =
                        HashMap::with_capacity(cmp::min(pending_outbound_payments_count_compat as usize, MAX_ALLOC_SIZE/32));
                for _ in 0..pending_outbound_payments_count_compat {
                        let session_priv = Readable::read(reader)?;
 -                      if pending_outbound_payments_compat.insert(MppId(session_priv), [session_priv].iter().cloned().collect()).is_some() {
 +                      let payment = PendingOutboundPayment::Legacy {
 +                              session_privs: [session_priv].iter().cloned().collect()
 +                      };
 +                      if pending_outbound_payments_compat.insert(PaymentId(session_priv), payment).is_some() {
                                return Err(DecodeError::InvalidValue)
                        };
                }
  
 +              // pending_outbound_payments_no_retry is for compatibility with 0.0.101 clients.
 +              let mut pending_outbound_payments_no_retry: Option<HashMap<PaymentId, HashSet<[u8; 32]>>> = None;
                let mut pending_outbound_payments = None;
                read_tlv_fields!(reader, {
 -                      (1, pending_outbound_payments, option),
 +                      (1, pending_outbound_payments_no_retry, option),
 +                      (3, pending_outbound_payments, option),
                });
 -              if pending_outbound_payments.is_none() {
 +              if pending_outbound_payments.is_none() && pending_outbound_payments_no_retry.is_none() {
                        pending_outbound_payments = Some(pending_outbound_payments_compat);
 +              } else if pending_outbound_payments.is_none() {
 +                      let mut outbounds = HashMap::new();
 +                      for (id, session_privs) in pending_outbound_payments_no_retry.unwrap().drain() {
 +                              outbounds.insert(id, PendingOutboundPayment::Legacy { session_privs });
 +                      }
 +                      pending_outbound_payments = Some(outbounds);
                }
  
                let mut secp_ctx = Secp256k1::new();
@@@ -5795,7 -5599,7 +5798,7 @@@ mod tests 
        use bitcoin::hashes::sha256::Hash as Sha256;
        use core::time::Duration;
        use ln::{PaymentPreimage, PaymentHash, PaymentSecret};
 -      use ln::channelmanager::{MppId, PaymentSendFailure};
 +      use ln::channelmanager::{PaymentId, PaymentSendFailure};
        use ln::features::{InitFeatures, InvoiceFeatures};
        use ln::functional_test_utils::*;
        use ln::msgs;
                let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
                let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph, &nodes[1].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap();
                let (payment_preimage, our_payment_hash, payment_secret) = get_payment_preimage_hash!(&nodes[1]);
 -              let mpp_id = MppId([42; 32]);
 +              let payment_id = PaymentId([42; 32]);
                // Use the utility function send_payment_along_path to send the payment with MPP data which
                // indicates there are more HTLCs coming.
                let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match.
 -              nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, mpp_id, &None).unwrap();
 +              nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
                expect_payment_failed!(nodes[0], our_payment_hash, true);
  
                // Send the second half of the original MPP payment.
 -              nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, mpp_id, &None).unwrap();
 +              nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200_000, cur_height, payment_id, &None).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
                // further events will be generated for subsequence path successes.
                let events = nodes[0].node.get_and_clear_pending_events();
                match events[0] {
 -                      Event::PaymentSent { payment_preimage: ref preimage } => {
 +                      Event::PaymentSent { payment_preimage: ref preimage, payment_hash: ref hash } => {
                                assert_eq!(payment_preimage, *preimage);
 +                              assert_eq!(our_payment_hash, *hash);
                        },
                        _ => panic!("Unexpected event"),
                }
                // To start (2), send a keysend payment but don't claim it.
                let payment_preimage = PaymentPreimage([42; 32]);
                let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph, &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 100_000, TEST_FINAL_CLTV, &logger).unwrap();
 -              let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap();
 +              let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(payment_preimage)).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
  
                let test_preimage = PaymentPreimage([42; 32]);
                let mismatch_payment_hash = PaymentHash([43; 32]);
 -              let _ = nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage)).unwrap();
 +              let _ = nodes[0].node.send_payment_internal(&route, mismatch_payment_hash, &None, Some(test_preimage), None, None).unwrap();
                check_added_monitors!(nodes[0], 1);
  
                let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
                let test_preimage = PaymentPreimage([42; 32]);
                let test_secret = PaymentSecret([43; 32]);
                let payment_hash = PaymentHash(Sha256::hash(&test_preimage.0).into_inner());
 -              let _ = nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage)).unwrap();
 +              let _ = nodes[0].node.send_payment_internal(&route, payment_hash, &Some(test_secret), Some(test_preimage), None, None).unwrap();
                check_added_monitors!(nodes[0], 1);
  
                let updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
index b412cc2bcca57b24fddb8daa2136e68893628e2d,8d0bb1ec38069d8c53770b148dd80654f1fb0526..9817d6bed62a20b1c5818bfceaf077f40a36b08e
@@@ -19,7 -19,7 +19,7 @@@ use chain::transaction::OutPoint
  use chain::keysinterface::BaseSign;
  use ln::{PaymentPreimage, PaymentSecret, PaymentHash};
  use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
 -use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, MppId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
 +use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
  use ln::channel::{Channel, ChannelError};
  use ln::{chan_utils, onion_utils};
  use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
@@@ -2521,8 -2521,8 +2521,8 @@@ fn test_htlc_on_chain_success() 
        send_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 8000000);
        send_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 8000000);
  
 -      let (our_payment_preimage, _payment_hash, _payment_secret) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2]), 3000000);
 -      let (our_payment_preimage_2, _payment_hash_2, _payment_secret_2) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2]), 3000000);
 +      let (our_payment_preimage, payment_hash_1, _payment_secret) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2]), 3000000);
 +      let (our_payment_preimage_2, payment_hash_2, _payment_secret_2) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2]), 3000000);
  
        // Broadcast legit commitment tx from C on B's chain
        // Broadcast HTLC Success transaction by C on received output from C's commitment tx on B's chain
        let mut first_claimed = false;
        for event in events {
                match event {
 -                      Event::PaymentSent { payment_preimage } => {
 -                              if payment_preimage == our_payment_preimage {
 +                      Event::PaymentSent { payment_preimage, payment_hash } => {
 +                              if payment_preimage == our_payment_preimage && payment_hash == payment_hash_1 {
                                        assert!(!first_claimed);
                                        first_claimed = true;
                                } else {
                                        assert_eq!(payment_preimage, our_payment_preimage_2);
 +                                      assert_eq!(payment_hash, payment_hash_2);
                                }
                        },
                        Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => {},
@@@ -3371,7 -3370,7 +3371,7 @@@ fn test_simple_peer_disconnect() 
        nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
        reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
  
 -      let payment_preimage_3 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).0;
 +      let (payment_preimage_3, payment_hash_3, _) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000);
        let payment_preimage_4 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).0;
        let payment_hash_5 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).1;
        let payment_hash_6 = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 1000000).1;
                let events = nodes[0].node.get_and_clear_pending_events();
                assert_eq!(events.len(), 2);
                match events[0] {
 -                      Event::PaymentSent { payment_preimage } => {
 +                      Event::PaymentSent { payment_preimage, payment_hash } => {
                                assert_eq!(payment_preimage, payment_preimage_3);
 +                              assert_eq!(payment_hash, payment_hash_3);
                        },
                        _ => panic!("Unexpected event"),
                }
@@@ -3556,9 -3554,8 +3556,9 @@@ fn do_test_drop_messages_peer_disconnec
                let events_4 = nodes[0].node.get_and_clear_pending_events();
                assert_eq!(events_4.len(), 1);
                match events_4[0] {
 -                      Event::PaymentSent { ref payment_preimage } => {
 +                      Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
                                assert_eq!(payment_preimage_1, *payment_preimage);
 +                              assert_eq!(payment_hash_1, *payment_hash);
                        },
                        _ => panic!("Unexpected event"),
                }
                        let events_4 = nodes[0].node.get_and_clear_pending_events();
                        assert_eq!(events_4.len(), 1);
                        match events_4[0] {
 -                              Event::PaymentSent { ref payment_preimage } => {
 +                              Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
                                        assert_eq!(payment_preimage_1, *payment_preimage);
 +                                      assert_eq!(payment_hash_1, *payment_hash);
                                },
                                _ => panic!("Unexpected event"),
                        }
@@@ -3804,7 -3800,7 +3804,7 @@@ fn test_drop_messages_peer_disconnect_d
        create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
        let logger = test_utils::TestLogger::new();
  
 -      let (payment_preimage_1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
 +      let (payment_preimage_1, payment_hash_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
  
        // Now try to send a second payment which will fail to send
        let (payment_preimage_2, payment_hash_2, payment_secret_2) = get_payment_preimage_hash!(nodes[1]);
                        let events_3 = nodes[0].node.get_and_clear_pending_events();
                        assert_eq!(events_3.len(), 1);
                        match events_3[0] {
 -                              Event::PaymentSent { ref payment_preimage } => {
 +                              Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
                                        assert_eq!(*payment_preimage, payment_preimage_1);
 +                                      assert_eq!(*payment_hash, payment_hash_1);
                                },
                                _ => panic!("Unexpected event"),
                        }
@@@ -3962,8 -3957,8 +3962,8 @@@ fn do_test_htlc_timeout(send_partial_mp
                // Use the utility function send_payment_along_path to send the payment with MPP data which
                // indicates there are more HTLCs coming.
                let cur_height = CHAN_CONFIRM_DEPTH + 1; // route_payment calls send_payment, which adds 1 to the current height. So we do the same here to match.
 -              let mpp_id = MppId([42; 32]);
 -              nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200000, cur_height, mpp_id, &None).unwrap();
 +              let payment_id = PaymentId([42; 32]);
 +              nodes[0].node.send_payment_along_path(&route.paths[0], &our_payment_hash, &Some(payment_secret), 200000, cur_height, payment_id, &None).unwrap();
                check_added_monitors!(nodes[0], 1);
                let mut events = nodes[0].node.get_and_clear_pending_msg_events();
                assert_eq!(events.len(), 1);
@@@ -5256,9 -5251,8 +5256,9 @@@ fn test_duplicate_payment_hash_one_fail
  
        let events = nodes[0].node.get_and_clear_pending_events();
        match events[0] {
 -              Event::PaymentSent { ref payment_preimage } => {
 +              Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
                        assert_eq!(*payment_preimage, our_payment_preimage);
 +                      assert_eq!(*payment_hash, duplicate_payment_hash);
                }
                _ => panic!("Unexpected event"),
        }
@@@ -5760,7 -5754,7 +5760,7 @@@ fn do_htlc_claim_local_commitment_only(
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
        let chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
  
 -      let (our_payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1]], if use_dust { 50000 } else { 3000000 });
 +      let (our_payment_preimage, our_payment_hash, _) = route_payment(&nodes[0], &[&nodes[1]], if use_dust { 50000 } else { 3000000 });
  
        // Claim the payment, but don't deliver A's commitment_signed, resulting in the HTLC only being
        // present in B's local commitment transaction, but none of A's commitment transactions.
        let events = nodes[0].node.get_and_clear_pending_events();
        assert_eq!(events.len(), 1);
        match events[0] {
 -              Event::PaymentSent { payment_preimage } => {
 +              Event::PaymentSent { payment_preimage, payment_hash } => {
                        assert_eq!(payment_preimage, our_payment_preimage);
 +                      assert_eq!(payment_hash, our_payment_hash);
                },
                _ => panic!("Unexpected event"),
        }
@@@ -6075,11 -6068,12 +6075,12 @@@ fn test_fail_holding_cell_htlc_upon_fre
        let events = nodes[0].node.get_and_clear_pending_events();
        assert_eq!(events.len(), 1);
        match &events[0] {
-               &Event::PaymentPathFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref error_code, ref error_data, ref all_paths_failed, path: _ } => {
+               &Event::PaymentPathFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref all_paths_failed, path: _, ref short_channel_id, ref error_code, ref error_data } => {
                        assert_eq!(our_payment_hash.clone(), *payment_hash);
                        assert_eq!(*rejected_by_dest, false);
                        assert_eq!(*all_paths_failed, true);
                        assert_eq!(*network_update, None);
+                       assert_eq!(*short_channel_id, None);
                        assert_eq!(*error_code, None);
                        assert_eq!(*error_data, None);
                },
@@@ -6162,11 -6156,12 +6163,12 @@@ fn test_free_and_fail_holding_cell_htlc
        let events = nodes[0].node.get_and_clear_pending_events();
        assert_eq!(events.len(), 1);
        match &events[0] {
-               &Event::PaymentPathFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref error_code, ref error_data, ref all_paths_failed, path: _ } => {
+               &Event::PaymentPathFailed { ref payment_hash, ref rejected_by_dest, ref network_update, ref all_paths_failed, path: _, ref short_channel_id, ref error_code, ref error_data } => {
                        assert_eq!(payment_hash_2.clone(), *payment_hash);
                        assert_eq!(*rejected_by_dest, false);
                        assert_eq!(*all_paths_failed, true);
                        assert_eq!(*network_update, None);
+                       assert_eq!(*short_channel_id, None);
                        assert_eq!(*error_code, None);
                        assert_eq!(*error_data, None);
                },
        let events = nodes[0].node.get_and_clear_pending_events();
        assert_eq!(events.len(), 1);
        match events[0] {
 -              Event::PaymentSent { ref payment_preimage } => {
 +              Event::PaymentSent { ref payment_preimage, ref payment_hash } => {
                        assert_eq!(*payment_preimage, payment_preimage_1);
 +                      assert_eq!(*payment_hash, payment_hash_1);
                }
                _ => panic!("Unexpected event"),
        }
@@@ -8640,7 -8634,7 +8642,7 @@@ fn test_pre_lockin_no_chan_closed_updat
        let channel_id = ::chain::transaction::OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
        nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id, data: "Hi".to_owned() });
        assert!(nodes[0].chain_monitor.added_monitors.lock().unwrap().is_empty());
 -      check_closed_event!(nodes[0], 1, ClosureReason::CounterpartyForceClosed { peer_msg: "Hi".to_string() });
 +      check_closed_event!(nodes[0], 2, ClosureReason::CounterpartyForceClosed { peer_msg: "Hi".to_string() }, true);
  }
  
  #[test]
@@@ -9216,125 -9210,6 +9218,125 @@@ fn test_tx_confirmed_skipping_blocks_im
        do_test_tx_confirmed_skipping_blocks_immediate_broadcast(true);
  }
  
 +#[test]
 +fn test_forwardable_regen() {
 +      // Tests that if we reload a ChannelManager while forwards are pending we will regenerate the
 +      // PendingHTLCsForwardable event automatically, ensuring we don't forget to forward/receive
 +      // HTLCs.
 +      // We test it for both payment receipt and payment forwarding.
 +
 +      let chanmon_cfgs = create_chanmon_cfgs(3);
 +      let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
 +      let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 +      let persister: test_utils::TestPersister;
 +      let new_chain_monitor: test_utils::TestChainMonitor;
 +      let nodes_1_deserialized: ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>;
 +      let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
 +      create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 +      create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
 +
 +      // First send a payment to nodes[1]
 +      let (route, payment_hash, payment_preimage, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000);
 +      nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
 +      check_added_monitors!(nodes[0], 1);
 +
 +      let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 +      assert_eq!(events.len(), 1);
 +      let payment_event = SendEvent::from_event(events.pop().unwrap());
 +      nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
 +      commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
 +
 +      expect_pending_htlcs_forwardable_ignore!(nodes[1]);
 +
 +      // Next send a payment which is forwarded by nodes[1]
 +      let (route_2, payment_hash_2, payment_preimage_2, payment_secret_2) = get_route_and_payment_hash!(nodes[0], nodes[2], 200_000);
 +      nodes[0].node.send_payment(&route_2, payment_hash_2, &Some(payment_secret_2)).unwrap();
 +      check_added_monitors!(nodes[0], 1);
 +
 +      let mut events = nodes[0].node.get_and_clear_pending_msg_events();
 +      assert_eq!(events.len(), 1);
 +      let payment_event = SendEvent::from_event(events.pop().unwrap());
 +      nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]);
 +      commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
 +
 +      // There is already a PendingHTLCsForwardable event "pending" so another one will not be
 +      // generated
 +      assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 +
 +      // Now restart nodes[1] and make sure it regenerates a single PendingHTLCsForwardable
 +      nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 +      nodes[2].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 +
 +      let nodes_1_serialized = nodes[1].node.encode();
 +      let mut chan_0_monitor_serialized = test_utils::TestVecWriter(Vec::new());
 +      let mut chan_1_monitor_serialized = test_utils::TestVecWriter(Vec::new());
 +      {
 +              let monitors = nodes[1].chain_monitor.chain_monitor.monitors.read().unwrap();
 +              let mut monitor_iter = monitors.iter();
 +              monitor_iter.next().unwrap().1.write(&mut chan_0_monitor_serialized).unwrap();
 +              monitor_iter.next().unwrap().1.write(&mut chan_1_monitor_serialized).unwrap();
 +      }
 +
 +      persister = test_utils::TestPersister::new();
 +      let keys_manager = &chanmon_cfgs[1].keys_manager;
 +      new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[1].chain_source), nodes[1].tx_broadcaster.clone(), nodes[1].logger, node_cfgs[1].fee_estimator, &persister, keys_manager);
 +      nodes[1].chain_monitor = &new_chain_monitor;
 +
 +      let mut chan_0_monitor_read = &chan_0_monitor_serialized.0[..];
 +      let (_, mut chan_0_monitor) = <(BlockHash, ChannelMonitor<EnforcingSigner>)>::read(
 +              &mut chan_0_monitor_read, keys_manager).unwrap();
 +      assert!(chan_0_monitor_read.is_empty());
 +      let mut chan_1_monitor_read = &chan_1_monitor_serialized.0[..];
 +      let (_, mut chan_1_monitor) = <(BlockHash, ChannelMonitor<EnforcingSigner>)>::read(
 +              &mut chan_1_monitor_read, keys_manager).unwrap();
 +      assert!(chan_1_monitor_read.is_empty());
 +
 +      let mut nodes_1_read = &nodes_1_serialized[..];
 +      let (_, nodes_1_deserialized_tmp) = {
 +              let mut channel_monitors = HashMap::new();
 +              channel_monitors.insert(chan_0_monitor.get_funding_txo().0, &mut chan_0_monitor);
 +              channel_monitors.insert(chan_1_monitor.get_funding_txo().0, &mut chan_1_monitor);
 +              <(BlockHash, ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>)>::read(&mut nodes_1_read, ChannelManagerReadArgs {
 +                      default_config: UserConfig::default(),
 +                      keys_manager,
 +                      fee_estimator: node_cfgs[1].fee_estimator,
 +                      chain_monitor: nodes[1].chain_monitor,
 +                      tx_broadcaster: nodes[1].tx_broadcaster.clone(),
 +                      logger: nodes[1].logger,
 +                      channel_monitors,
 +              }).unwrap()
 +      };
 +      nodes_1_deserialized = nodes_1_deserialized_tmp;
 +      assert!(nodes_1_read.is_empty());
 +
 +      assert!(nodes[1].chain_monitor.watch_channel(chan_0_monitor.get_funding_txo().0, chan_0_monitor).is_ok());
 +      assert!(nodes[1].chain_monitor.watch_channel(chan_1_monitor.get_funding_txo().0, chan_1_monitor).is_ok());
 +      nodes[1].node = &nodes_1_deserialized;
 +      check_added_monitors!(nodes[1], 2);
 +
 +      reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
 +      // Note that nodes[1] and nodes[2] resend their funding_locked here since they haven't updated
 +      // the commitment state.
 +      reconnect_nodes(&nodes[1], &nodes[2], (true, true), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
 +
 +      assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 +
 +      expect_pending_htlcs_forwardable!(nodes[1]);
 +      expect_payment_received!(nodes[1], payment_hash, payment_secret, 100_000);
 +      check_added_monitors!(nodes[1], 1);
 +
 +      let mut events = nodes[1].node.get_and_clear_pending_msg_events();
 +      assert_eq!(events.len(), 1);
 +      let payment_event = SendEvent::from_event(events.pop().unwrap());
 +      nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &payment_event.msgs[0]);
 +      commitment_signed_dance!(nodes[2], nodes[1], payment_event.commitment_msg, false);
 +      expect_pending_htlcs_forwardable!(nodes[2]);
 +      expect_payment_received!(nodes[2], payment_hash_2, payment_secret_2, 200_000);
 +
 +      claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
 +      claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_2);
 +}
 +
  #[test]
  fn test_keysend_payments_to_public_node() {
        let chanmon_cfgs = create_chanmon_cfgs(2);
                          nodes[0].logger).unwrap();
  
        let test_preimage = PaymentPreimage([42; 32]);
 -      let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage)).unwrap();
 +      let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage)).unwrap();
        check_added_monitors!(nodes[0], 1);
        let mut events = nodes[0].node.get_and_clear_pending_msg_events();
        assert_eq!(events.len(), 1);
@@@ -9381,7 -9256,7 +9383,7 @@@ fn test_keysend_payments_to_private_nod
                                  nodes[0].logger).unwrap();
  
        let test_preimage = PaymentPreimage([42; 32]);
 -      let payment_hash = nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage)).unwrap();
 +      let (payment_hash, _) = nodes[0].node.send_spontaneous_payment(&route, Some(test_preimage)).unwrap();
        check_added_monitors!(nodes[0], 1);
        let mut events = nodes[0].node.get_and_clear_pending_msg_events();
        assert_eq!(events.len(), 1);
index 2a8c45c3bb47618cd3cec78c4be2d5e00a13b08c,19786947f98d987c465ffdce7dcd0ad4dc281405..961939cb7e6dedd4126c8a9daf7e4d839135bc6e
@@@ -9,7 -9,6 +9,7 @@@
  
  //! The top-level network map tracking logic lives here.
  
 +use bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE;
  use bitcoin::secp256k1::key::PublicKey;
  use bitcoin::secp256k1::Secp256k1;
  use bitcoin::secp256k1;
@@@ -51,75 -50,12 +51,75 @@@ const MAX_EXCESS_BYTES_FOR_RELAY: usiz
  /// This value ensures a reply fits within the 65k payload limit and is consistent with other implementations.
  const MAX_SCIDS_PER_REPLY: usize = 8000;
  
 +/// Represents the compressed public key of a node
 +#[derive(Clone, Copy)]
 +pub struct NodeId([u8; PUBLIC_KEY_SIZE]);
 +
 +impl NodeId {
 +      /// Create a new NodeId from a public key
 +      pub fn from_pubkey(pubkey: &PublicKey) -> Self {
 +              NodeId(pubkey.serialize())
 +      }
 +      
 +      /// Get the public key slice from this NodeId
 +      pub fn as_slice(&self) -> &[u8] {
 +              &self.0
 +      }
 +}
 +
 +impl fmt::Debug for NodeId {
 +      fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
 +              write!(f, "NodeId({})", log_bytes!(self.0))
 +      }
 +}
 +
 +impl core::hash::Hash for NodeId {
 +      fn hash<H: core::hash::Hasher>(&self, hasher: &mut H) {
 +              self.0.hash(hasher);
 +      }
 +}
 +
 +impl Eq for NodeId {}
 +
 +impl PartialEq for NodeId {
 +      fn eq(&self, other: &Self) -> bool {
 +              self.0[..] == other.0[..]
 +      }
 +}
 +
 +impl cmp::PartialOrd for NodeId {
 +      fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
 +              Some(self.cmp(other))
 +      }
 +}
 +
 +impl Ord for NodeId {
 +      fn cmp(&self, other: &Self) -> cmp::Ordering {
 +              self.0[..].cmp(&other.0[..])
 +      }
 +}
 +
 +impl Writeable for NodeId {
 +      fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
 +              writer.write_all(&self.0)?;
 +              Ok(())
 +      }
 +}
 +
 +impl Readable for NodeId {
 +      fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
 +              let mut buf = [0; PUBLIC_KEY_SIZE];
 +              reader.read_exact(&mut buf)?;
 +              Ok(Self(buf))
 +      }
 +}
 +
  /// Represents the network as nodes and channels between them
  pub struct NetworkGraph {
        genesis_hash: BlockHash,
        // Lock order: channels -> nodes
        channels: RwLock<BTreeMap<u64, ChannelInfo>>,
 -      nodes: RwLock<BTreeMap<PublicKey, NodeInfo>>,
 +      nodes: RwLock<BTreeMap<NodeId, NodeInfo>>,
  }
  
  impl Clone for NetworkGraph {
  /// A read-only view of [`NetworkGraph`].
  pub struct ReadOnlyNetworkGraph<'a> {
        channels: RwLockReadGuard<'a, BTreeMap<u64, ChannelInfo>>,
 -      nodes: RwLockReadGuard<'a, BTreeMap<PublicKey, NodeInfo>>,
 +      nodes: RwLockReadGuard<'a, BTreeMap<NodeId, NodeInfo>>,
  }
  
  /// Update to the [`NetworkGraph`] based on payment failure information conveyed via the Onion
@@@ -341,11 -277,11 +341,11 @@@ where C::Target: chain::Access, L::Targ
                let mut result = Vec::with_capacity(batch_amount as usize);
                let nodes = self.network_graph.nodes.read().unwrap();
                let mut iter = if let Some(pubkey) = starting_point {
 -                              let mut iter = nodes.range((*pubkey)..);
 +                              let mut iter = nodes.range(NodeId::from_pubkey(pubkey)..);
                                iter.next();
                                iter
                        } else {
 -                              nodes.range(..)
 +                              nodes.range::<NodeId, _>(..)
                        };
                while result.len() < batch_amount as usize {
                        if let Some((_, ref node)) = iter.next() {
                }
  
                // Check if we need to perform a full synchronization with this peer
 -              if !self.should_request_full_sync(their_node_id) {
 +              if !self.should_request_full_sync(&their_node_id) {
                        return ();
                }
  
@@@ -615,11 -551,11 +615,11 @@@ pub struct ChannelInfo 
        /// Protocol features of a channel communicated during its announcement
        pub features: ChannelFeatures,
        /// Source node of the first direction of a channel
 -      pub node_one: PublicKey,
 +      pub node_one: NodeId,
        /// Details about the first direction of a channel
        pub one_to_two: Option<DirectionalChannelInfo>,
        /// Source node of the second direction of a channel
 -      pub node_two: PublicKey,
 +      pub node_two: NodeId,
        /// Details about the second direction of a channel
        pub two_to_one: Option<DirectionalChannelInfo>,
        /// The channel capacity as seen on-chain, if chain lookup is available.
  impl fmt::Display for ChannelInfo {
        fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
                write!(f, "features: {}, node_one: {}, one_to_two: {:?}, node_two: {}, two_to_one: {:?}",
 -                 log_bytes!(self.features.encode()), log_pubkey!(self.node_one), self.one_to_two, log_pubkey!(self.node_two), self.two_to_one)?;
 +                 log_bytes!(self.features.encode()), log_bytes!(self.node_one.as_slice()), self.one_to_two, log_bytes!(self.node_two.as_slice()), self.two_to_one)?;
                Ok(())
        }
  }
@@@ -788,8 -724,8 +788,8 @@@ impl fmt::Display for NetworkGraph 
                        writeln!(f, " {}: {}", key, val)?;
                }
                writeln!(f, "[Nodes]")?;
 -              for (key, val) in self.nodes.read().unwrap().iter() {
 -                      writeln!(f, " {}: {}", log_pubkey!(key), val)?;
 +              for (&node_id, val) in self.nodes.read().unwrap().iter() {
 +                      writeln!(f, " {}: {}", log_bytes!(node_id.as_slice()), val)?;
                }
                Ok(())
        }
@@@ -844,7 -780,7 +844,7 @@@ impl NetworkGraph 
        }
  
        fn update_node_from_announcement_intern(&self, msg: &msgs::UnsignedNodeAnnouncement, full_msg: Option<&msgs::NodeAnnouncement>) -> Result<(), LightningError> {
 -              match self.nodes.write().unwrap().get_mut(&msg.node_id) {
 +              match self.nodes.write().unwrap().get_mut(&NodeId::from_pubkey(&msg.node_id)) {
                        None => Err(LightningError{err: "No existing channels for node_announcement".to_owned(), action: ErrorAction::IgnoreError}),
                        Some(node) => {
                                if let Some(node_info) = node.announcement_info.as_ref() {
  
                let chan_info = ChannelInfo {
                                features: msg.features.clone(),
 -                              node_one: msg.node_id_1.clone(),
 +                              node_one: NodeId::from_pubkey(&msg.node_id_1),
                                one_to_two: None,
 -                              node_two: msg.node_id_2.clone(),
 +                              node_two: NodeId::from_pubkey(&msg.node_id_2),
                                two_to_one: None,
                                capacity_sats: utxo_value,
                                announcement_message: if msg.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY
                        };
                }
  
 -              add_channel_to_node!(msg.node_id_1);
 -              add_channel_to_node!(msg.node_id_2);
 +              add_channel_to_node!(NodeId::from_pubkey(&msg.node_id_1));
 +              add_channel_to_node!(NodeId::from_pubkey(&msg.node_id_2));
  
                Ok(())
        }
                                if msg.flags & 1 == 1 {
                                        dest_node_id = channel.node_one.clone();
                                        if let Some((sig, ctx)) = sig_info {
 -                                              secp_verify_sig!(ctx, &msg_hash, &sig, &channel.node_two);
 +                                              secp_verify_sig!(ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_two.as_slice()).map_err(|_| LightningError{
 +                                                      err: "Couldn't parse source node pubkey".to_owned(),
 +                                                      action: ErrorAction::IgnoreAndLog(Level::Debug)
 +                                              })?);
                                        }
                                        maybe_update_channel_info!(channel.two_to_one, channel.node_two);
                                } else {
                                        dest_node_id = channel.node_two.clone();
                                        if let Some((sig, ctx)) = sig_info {
 -                                              secp_verify_sig!(ctx, &msg_hash, &sig, &channel.node_one);
 +                                              secp_verify_sig!(ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_one.as_slice()).map_err(|_| LightningError{
 +                                                      err: "Couldn't parse destination node pubkey".to_owned(),
 +                                                      action: ErrorAction::IgnoreAndLog(Level::Debug)
 +                                              })?);
                                        }
                                        maybe_update_channel_info!(channel.one_to_two, channel.node_one);
                                }
                Ok(())
        }
  
 -      fn remove_channel_in_nodes(nodes: &mut BTreeMap<PublicKey, NodeInfo>, chan: &ChannelInfo, short_channel_id: u64) {
 +      fn remove_channel_in_nodes(nodes: &mut BTreeMap<NodeId, NodeInfo>, chan: &ChannelInfo, short_channel_id: u64) {
                macro_rules! remove_from_node {
                        ($node_id: expr) => {
                                if let BtreeEntry::Occupied(mut entry) = nodes.entry($node_id) {
@@@ -1206,7 -1136,7 +1206,7 @@@ impl ReadOnlyNetworkGraph<'_> 
        /// Returns all known nodes' public keys along with announced node info.
        ///
        /// (C-not exported) because we have no mapping for `BTreeMap`s
 -      pub fn nodes(&self) -> &BTreeMap<PublicKey, NodeInfo> {
 +      pub fn nodes(&self) -> &BTreeMap<NodeId, NodeInfo> {
                &*self.nodes
        }
  
        ///
        /// (C-not exported) as there is no practical way to track lifetimes of returned values.
        pub fn get_addresses(&self, pubkey: &PublicKey) -> Option<&Vec<NetAddress>> {
 -              if let Some(node) = self.nodes.get(pubkey) {
 +              if let Some(node) = self.nodes.get(&NodeId::from_pubkey(&pubkey)) {
                        if let Some(node_info) = node.announcement_info.as_ref() {
                                return Some(&node_info.addresses)
                        }
@@@ -1815,6 -1745,7 +1815,7 @@@ mod tests 
                                network_update: Some(NetworkUpdate::ChannelUpdateMessage {
                                        msg: valid_channel_update,
                                }),
+                               short_channel_id: None,
                                error_code: None,
                                error_data: None,
                        });
                                        short_channel_id,
                                        is_permanent: false,
                                }),
+                               short_channel_id: None,
                                error_code: None,
                                error_data: None,
                        });
                                        short_channel_id,
                                        is_permanent: true,
                                }),
+                               short_channel_id: None,
                                error_code: None,
                                error_data: None,
                        });
index 08fc164f4baf10f8a2240a64e09d6bd77b8fa165,fcc625db047e679117aff18af238634401ea1cf1..9f7f4b4e5e00130a3581c5dd397497b3be0ba8ff
@@@ -23,15 -23,13 +23,15 @@@ use util::ser::{BigSize, FixedLengthRea
  use routing::router::RouteHop;
  
  use bitcoin::blockdata::script::Script;
 -
 +use bitcoin::hashes::Hash;
 +use bitcoin::hashes::sha256::Hash as Sha256;
  use bitcoin::secp256k1::key::PublicKey;
  
  use io;
  use prelude::*;
  use core::time::Duration;
  use core::ops::Deref;
 +use bitcoin::Transaction;
  
  /// Some information provided on receipt of payment depends on whether the payment received is a
  /// spontaneous payment or a "conventional" lightning payment that's paying an invoice.
@@@ -150,20 -148,21 +150,20 @@@ pub enum Event 
                user_channel_id: u64,
        },
        /// Indicates we've received money! Just gotta dig out that payment preimage and feed it to
 -      /// ChannelManager::claim_funds to get it....
 -      /// Note that if the preimage is not known or the amount paid is incorrect, you should call
 -      /// ChannelManager::fail_htlc_backwards to free up resources for this HTLC and avoid
 +      /// [`ChannelManager::claim_funds`] to get it....
 +      /// Note that if the preimage is not known, you should call
 +      /// [`ChannelManager::fail_htlc_backwards`] to free up resources for this HTLC and avoid
        /// network congestion.
 -      /// The amount paid should be considered 'incorrect' when it is less than or more than twice
 -      /// the amount expected.
 -      /// If you fail to call either ChannelManager::claim_funds or
 -      /// ChannelManager::fail_htlc_backwards within the HTLC's timeout, the HTLC will be
 +      /// If you fail to call either [`ChannelManager::claim_funds`] or
 +      /// [`ChannelManager::fail_htlc_backwards`] within the HTLC's timeout, the HTLC will be
        /// automatically failed.
 +      ///
 +      /// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds
 +      /// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards
        PaymentReceived {
                /// The hash for which the preimage should be handed to the ChannelManager.
                payment_hash: PaymentHash,
 -              /// The value, in thousandths of a satoshi, that this payment is for. Note that you must
 -              /// compare this to the expected value before accepting the payment (as otherwise you are
 -              /// providing proof-of-payment for less than the value you expected!).
 +              /// The value, in thousandths of a satoshi, that this payment is for.
                amt: u64,
                /// Information for claiming this received payment, based on whether the purpose of the
                /// payment is to pay an invoice or to send a spontaneous payment.
                /// Note that this serves as a payment receipt, if you wish to have such a thing, you must
                /// store it somehow!
                payment_preimage: PaymentPreimage,
 +              /// The hash which was given to [`ChannelManager::send_payment`].
 +              ///
 +              /// [`ChannelManager::send_payment`]: crate::ln::channelmanager::ChannelManager::send_payment
 +              payment_hash: PaymentHash,
        },
        /// Indicates an outbound payment we made failed. Probably some intermediary node dropped
        /// something. You may wish to retry with a different route.
                all_paths_failed: bool,
                /// The payment path that failed.
                path: Vec<RouteHop>,
+               /// The channel responsible for the failed payment path.
+               ///
+               /// If this is `Some`, then the corresponding channel should be avoided when the payment is
+               /// retried. May be `None` for older [`Event`] serializations.
+               short_channel_id: Option<u64>,
  #[cfg(test)]
                error_code: Option<u16>,
  #[cfg(test)]
                channel_id: [u8; 32],
                /// The reason the channel was closed.
                reason: ClosureReason
 +      },
 +      /// Used to indicate to the user that they can abandon the funding transaction and recycle the
 +      /// inputs for another purpose.
 +      DiscardFunding {
 +              /// The channel_id of the channel which has been closed.
 +              channel_id: [u8; 32],
 +              /// The full transaction received from the user
 +              transaction: Transaction
        }
  }
  
@@@ -301,15 -293,14 +306,15 @@@ impl Writeable for Event 
                                        (8, payment_preimage, option),
                                });
                        },
 -                      &Event::PaymentSent { ref payment_preimage } => {
 +                      &Event::PaymentSent { ref payment_preimage, ref payment_hash} => {
                                2u8.write(writer)?;
                                write_tlv_fields!(writer, {
                                        (0, payment_preimage, required),
 +                                      (1, payment_hash, required),
                                });
                        },
                        &Event::PaymentPathFailed { ref payment_hash, ref rejected_by_dest, ref network_update,
-                                                   ref all_paths_failed, ref path,
+                                                   ref all_paths_failed, ref path, ref short_channel_id,
                                #[cfg(test)]
                                ref error_code,
                                #[cfg(test)]
                                        (2, rejected_by_dest, required),
                                        (3, all_paths_failed, required),
                                        (5, path, vec_type),
+                                       (7, short_channel_id, option),
                                });
                        },
                        &Event::PendingHTLCsForwardable { time_forwardable: _ } => {
                                4u8.write(writer)?;
 -                              write_tlv_fields!(writer, {});
 -                              // We don't write the time_fordwardable out at all, as we presume when the user
 -                              // deserializes us at least that much time has elapsed.
 +                              // Note that we now ignore these on the read end as we'll re-generate them in
 +                              // ChannelManager, we write them here only for backwards compatibility.
                        },
                        &Event::SpendableOutputs { ref outputs } => {
                                5u8.write(writer)?;
                                        (2, reason, required)
                                });
                        },
 +                      &Event::DiscardFunding { ref channel_id, ref transaction } => {
 +                              11u8.write(writer)?;
 +                              write_tlv_fields!(writer, {
 +                                      (0, channel_id, required),
 +                                      (2, transaction, required)
 +                              })
 +                      },
                        // Note that, going forward, all new events must only write data inside of
                        // `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write
                        // data via `write_tlv_fields`.
@@@ -409,17 -395,11 +415,17 @@@ impl MaybeReadable for Event 
                        2u8 => {
                                let f = || {
                                        let mut payment_preimage = PaymentPreimage([0; 32]);
 +                                      let mut payment_hash = None;
                                        read_tlv_fields!(reader, {
                                                (0, payment_preimage, required),
 +                                              (1, payment_hash, option),
                                        });
 +                                      if payment_hash.is_none() {
 +                                              payment_hash = Some(PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner()));
 +                                      }
                                        Ok(Some(Event::PaymentSent {
                                                payment_preimage,
 +                                              payment_hash: payment_hash.unwrap(),
                                        }))
                                };
                                f()
                                        let mut network_update = None;
                                        let mut all_paths_failed = Some(true);
                                        let mut path: Option<Vec<RouteHop>> = Some(vec![]);
+                                       let mut short_channel_id = None;
                                        read_tlv_fields!(reader, {
                                                (0, payment_hash, required),
                                                (1, network_update, ignorable),
                                                (2, rejected_by_dest, required),
                                                (3, all_paths_failed, option),
                                                (5, path, vec_type),
+                                               (7, short_channel_id, ignorable),
                                        });
                                        Ok(Some(Event::PaymentPathFailed {
                                                payment_hash,
                                                network_update,
                                                all_paths_failed: all_paths_failed.unwrap(),
                                                path: path.unwrap(),
+                                               short_channel_id,
                                                #[cfg(test)]
                                                error_code,
                                                #[cfg(test)]
                                };
                                f()
                        },
 -                      4u8 => {
 -                              let f = || {
 -                                      read_tlv_fields!(reader, {});
 -                                      Ok(Some(Event::PendingHTLCsForwardable {
 -                                              time_forwardable: Duration::from_secs(0)
 -                                      }))
 -                              };
 -                              f()
 -                      },
 +                      4u8 => Ok(None),
                        5u8 => {
                                let f = || {
                                        let mut outputs = VecReadWrapper(Vec::new());
                                f()
                        },
                        9u8 => {
 -                              let mut channel_id = [0; 32];
 -                              let mut reason = None;
 -                              read_tlv_fields!(reader, {
 -                                      (0, channel_id, required),
 -                                      (2, reason, ignorable),
 -                              });
 -                              if reason.is_none() { return Ok(None); }
 -                              Ok(Some(Event::ChannelClosed { channel_id, reason: reason.unwrap() }))
 +                              let f = || {
 +                                      let mut channel_id = [0; 32];
 +                                      let mut reason = None;
 +                                      read_tlv_fields!(reader, {
 +                                              (0, channel_id, required),
 +                                              (2, reason, ignorable),
 +                                      });
 +                                      if reason.is_none() { return Ok(None); }
 +                                      Ok(Some(Event::ChannelClosed { channel_id, reason: reason.unwrap() }))
 +                              };
 +                              f()
 +                      },
 +                      11u8 => {
 +                              let f = || {
 +                                      let mut channel_id = [0; 32];
 +                                      let mut transaction = Transaction{ version: 2, lock_time: 0, input: Vec::new(), output: Vec::new() };
 +                                      read_tlv_fields!(reader, {
 +                                              (0, channel_id, required),
 +                                              (2, transaction, required),
 +                                      });
 +                                      Ok(Some(Event::DiscardFunding { channel_id, transaction } ))
 +                              };
 +                              f()
                        },
                        // Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue.
                        // Version 0.0.100 failed to properly ignore odd types, possibly resulting in corrupt