Merge pull request #1922 from wpaulino/avoid-remaining-redundant-commitment-broadcasts
[rust-lightning] / lightning / src / ln / channelmanager.rs
index fa38e54ed768b2c8a443bf4a043c529857741a21..b8372ad9b10302738882fcc7a35665135cee8e73 100644 (file)
@@ -49,6 +49,7 @@ use crate::ln::features::InvoiceFeatures;
 use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RoutePath, RouteParameters};
 use crate::ln::msgs;
 use crate::ln::onion_utils;
+use crate::ln::onion_utils::HTLCFailReason;
 use crate::ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT};
 use crate::ln::wire::Encode;
 use crate::chain::keysinterface::{Sign, KeysInterface, KeysManager, Recipient};
@@ -276,27 +277,6 @@ impl HTLCSource {
        }
 }
 
-#[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
-pub(super) enum HTLCFailReason {
-       LightningError {
-               err: msgs::OnionErrorPacket,
-       },
-       Reason {
-               failure_code: u16,
-               data: Vec<u8>,
-       }
-}
-
-impl HTLCFailReason {
-       pub(super) fn reason(failure_code: u16, data: Vec<u8>) -> Self {
-               Self::Reason { failure_code, data }
-       }
-
-       pub(super) fn from_failure_code(failure_code: u16) -> Self {
-               Self::Reason { failure_code, data: Vec::new() }
-       }
-}
-
 struct ReceiveError {
        err_code: u16,
        err_data: Vec<u8>,
@@ -2091,10 +2071,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                // Also, ensure that, in the case of an unknown preimage for the received payment hash, our
                // payment logic has enough time to fail the HTLC backward before our onchain logic triggers a
                // channel closure (see HTLC_FAIL_BACK_BUFFER rationale).
-               if (hop_data.outgoing_cltv_value as u64) <= self.best_block.read().unwrap().height() as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1  {
+               let current_height: u32 = self.best_block.read().unwrap().height();
+               if (hop_data.outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 {
+                       let mut err_data = Vec::with_capacity(12);
+                       err_data.extend_from_slice(&amt_msat.to_be_bytes());
+                       err_data.extend_from_slice(&current_height.to_be_bytes());
                        return Err(ReceiveError {
-                               err_code: 17,
-                               err_data: Vec::new(),
+                               err_code: 0x4000 | 15, err_data,
                                msg: "The final CLTV expiry is too soon to handle",
                        });
                }
@@ -2202,7 +2185,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                        return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
                                                channel_id: msg.channel_id,
                                                htlc_id: msg.htlc_id,
-                                               reason: onion_utils::build_first_hop_failure_packet(&shared_secret, $err_code, $data),
+                                               reason: HTLCFailReason::reason($err_code, $data.to_vec())
+                                                       .get_encrypted_failure_packet(&shared_secret, &None),
                                        }));
                                }
                        }
@@ -2267,7 +2251,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                        // with a short_channel_id of 0. This is important as various things later assume
                        // short_channel_id is non-0 in any ::Forward.
                        if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
-                               if let Some((err, code, chan_update)) = loop {
+                               if let Some((err, mut code, chan_update)) = loop {
                                        let id_option = self.short_to_chan_info.read().unwrap().get(&short_channel_id).cloned();
                                        let mut channel_state = self.channel_state.lock().unwrap();
                                        let forwarding_id_opt = match id_option {
@@ -2324,10 +2308,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                                }
                                                chan_update_opt
                                        } else {
-                                               if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 { // incorrect_cltv_expiry
+                                               if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
+                                                       // We really should set `incorrect_cltv_expiry` here but as we're not
+                                                       // forwarding over a real channel we can't generate a channel_update
+                                                       // for it. Instead we just return a generic temporary_node_failure.
                                                        break Some((
                                                                "Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
-                                                               0x1000 | 13, None,
+                                                               0x2000 | 2, None,
                                                        ));
                                                }
                                                None
@@ -2373,6 +2360,12 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                                (chan_update.serialized_length() as u16 + 2).write(&mut res).expect("Writes cannot fail");
                                                msgs::ChannelUpdate::TYPE.write(&mut res).expect("Writes cannot fail");
                                                chan_update.write(&mut res).expect("Writes cannot fail");
+                                       } else if code & 0x1000 == 0x1000 {
+                                               // If we're trying to return an error that requires a `channel_update` but
+                                               // we're forwarding to a phantom or intercept "channel" (i.e. cannot
+                                               // generate an update), just use the generic "temporary_node_failure"
+                                               // instead.
+                                               code = 0x2000 | 2;
                                        }
                                        return_err!(err, code, &res.0[..]);
                                }
@@ -2797,15 +2790,21 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
        /// Signals that no further retries for the given payment will occur.
        ///
-       /// After this method returns, any future calls to [`retry_payment`] for the given `payment_id`
-       /// will fail with [`PaymentSendFailure::ParameterError`]. If no such event has been generated,
-       /// an [`Event::PaymentFailed`] event will be generated as soon as there are no remaining
-       /// pending HTLCs for this payment.
+       /// After this method returns, no future calls to [`retry_payment`] for the given `payment_id`
+       /// are allowed. If no [`Event::PaymentFailed`] event had been generated before, one will be
+       /// generated as soon as there are no remaining pending HTLCs for this payment.
        ///
        /// Note that calling this method does *not* prevent a payment from succeeding. You must still
        /// wait until you receive either a [`Event::PaymentFailed`] or [`Event::PaymentSent`] event to
        /// determine the ultimate status of a payment.
        ///
+       /// If an [`Event::PaymentFailed`] event is generated and we restart without this
+       /// [`ChannelManager`] having been persisted, the payment may still be in the pending state
+       /// upon restart. This allows further calls to [`retry_payment`] (and requiring a second call
+       /// to [`abandon_payment`] to mark the payment as failed again). Otherwise, future calls to
+       /// [`retry_payment`] will fail with [`PaymentSendFailure::ParameterError`].
+       ///
+       /// [`abandon_payment`]: Self::abandon_payment
        /// [`retry_payment`]: Self::retry_payment
        /// [`Event::PaymentFailed`]: events::Event::PaymentFailed
        /// [`Event::PaymentSent`]: events::Event::PaymentSent
@@ -3199,7 +3198,6 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                let mut new_events = Vec::new();
                let mut failed_forwards = Vec::new();
                let mut phantom_receives: Vec<(u64, OutPoint, u128, Vec<(PendingHTLCInfo, u64)>)> = Vec::new();
-               let mut handle_errors = Vec::new();
                {
                        let mut forward_htlcs = HashMap::new();
                        mem::swap(&mut forward_htlcs, &mut self.forward_htlcs.lock().unwrap());
@@ -3315,8 +3313,6 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                                        continue;
                                                },
                                                hash_map::Entry::Occupied(mut chan) => {
-                                                       let mut add_htlc_msgs = Vec::new();
-                                                       let mut fail_htlc_msgs = Vec::new();
                                                        for forward_info in pending_forwards.drain(..) {
                                                                match forward_info {
                                                                        HTLCForwardInfo::AddHTLC(PendingAddHTLCInfo {
@@ -3335,34 +3331,21 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                                                                        // Phantom payments are only PendingHTLCRouting::Receive.
                                                                                        phantom_shared_secret: None,
                                                                                });
-                                                                               match chan.get_mut().send_htlc(outgoing_amt_msat, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet, &self.logger) {
-                                                                                       Err(e) => {
-                                                                                               if let ChannelError::Ignore(msg) = e {
-                                                                                                       log_trace!(self.logger, "Failed to forward HTLC with payment_hash {}: {}", log_bytes!(payment_hash.0), msg);
-                                                                                               } else {
-                                                                                                       panic!("Stated return value requirements in send_htlc() were not met");
-                                                                                               }
-                                                                                               let (failure_code, data) = self.get_htlc_temp_fail_err_and_data(0x1000|7, short_chan_id, chan.get());
-                                                                                               failed_forwards.push((htlc_source, payment_hash,
-                                                                                                       HTLCFailReason::reason(failure_code, data),
-                                                                                                       HTLCDestination::NextHopChannel { node_id: Some(chan.get().get_counterparty_node_id()), channel_id: forward_chan_id }
-                                                                                               ));
-                                                                                               continue;
-                                                                                       },
-                                                                                       Ok(update_add) => {
-                                                                                               match update_add {
-                                                                                                       Some(msg) => { add_htlc_msgs.push(msg); },
-                                                                                                       None => {
-                                                                                                               // Nothing to do here...we're waiting on a remote
-                                                                                                               // revoke_and_ack before we can add anymore HTLCs. The Channel
-                                                                                                               // will automatically handle building the update_add_htlc and
-                                                                                                               // commitment_signed messages when we can.
-                                                                                                               // TODO: Do some kind of timer to set the channel as !is_live()
-                                                                                                               // as we don't really want others relying on us relaying through
-                                                                                                               // this channel currently :/.
-                                                                                                       }
-                                                                                               }
+                                                                               if let Err(e) = chan.get_mut().queue_add_htlc(outgoing_amt_msat,
+                                                                                       payment_hash, outgoing_cltv_value, htlc_source.clone(),
+                                                                                       onion_packet, &self.logger)
+                                                                               {
+                                                                                       if let ChannelError::Ignore(msg) = e {
+                                                                                               log_trace!(self.logger, "Failed to forward HTLC with payment_hash {}: {}", log_bytes!(payment_hash.0), msg);
+                                                                                       } else {
+                                                                                               panic!("Stated return value requirements in send_htlc() were not met");
                                                                                        }
+                                                                                       let (failure_code, data) = self.get_htlc_temp_fail_err_and_data(0x1000|7, short_chan_id, chan.get());
+                                                                                       failed_forwards.push((htlc_source, payment_hash,
+                                                                                               HTLCFailReason::reason(failure_code, data),
+                                                                                               HTLCDestination::NextHopChannel { node_id: Some(chan.get().get_counterparty_node_id()), channel_id: forward_chan_id }
+                                                                                       ));
+                                                                                       continue;
                                                                                }
                                                                        },
                                                                        HTLCForwardInfo::AddHTLC { .. } => {
@@ -3370,77 +3353,22 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                                                        },
                                                                        HTLCForwardInfo::FailHTLC { htlc_id, err_packet } => {
                                                                                log_trace!(self.logger, "Failing HTLC back to channel with short id {} (backward HTLC ID {}) after delay", short_chan_id, htlc_id);
-                                                                               match chan.get_mut().get_update_fail_htlc(htlc_id, err_packet, &self.logger) {
-                                                                                       Err(e) => {
-                                                                                               if let ChannelError::Ignore(msg) = e {
-                                                                                                       log_trace!(self.logger, "Failed to fail HTLC with ID {} backwards to short_id {}: {}", htlc_id, short_chan_id, msg);
-                                                                                               } else {
-                                                                                                       panic!("Stated return value requirements in get_update_fail_htlc() were not met");
-                                                                                               }
-                                                                                               // fail-backs are best-effort, we probably already have one
-                                                                                               // pending, and if not that's OK, if not, the channel is on
-                                                                                               // the chain and sending the HTLC-Timeout is their problem.
-                                                                                               continue;
-                                                                                       },
-                                                                                       Ok(Some(msg)) => { fail_htlc_msgs.push(msg); },
-                                                                                       Ok(None) => {
-                                                                                               // Nothing to do here...we're waiting on a remote
-                                                                                               // revoke_and_ack before we can update the commitment
-                                                                                               // transaction. The Channel will automatically handle
-                                                                                               // building the update_fail_htlc and commitment_signed
-                                                                                               // messages when we can.
-                                                                                               // We don't need any kind of timer here as they should fail
-                                                                                               // the channel onto the chain if they can't get our
-                                                                                               // update_fail_htlc in time, it's not our problem.
+                                                                               if let Err(e) = chan.get_mut().queue_fail_htlc(
+                                                                                       htlc_id, err_packet, &self.logger
+                                                                               ) {
+                                                                                       if let ChannelError::Ignore(msg) = e {
+                                                                                               log_trace!(self.logger, "Failed to fail HTLC with ID {} backwards to short_id {}: {}", htlc_id, short_chan_id, msg);
+                                                                                       } else {
+                                                                                               panic!("Stated return value requirements in queue_fail_htlc() were not met");
                                                                                        }
+                                                                                       // fail-backs are best-effort, we probably already have one
+                                                                                       // pending, and if not that's OK, if not, the channel is on
+                                                                                       // the chain and sending the HTLC-Timeout is their problem.
+                                                                                       continue;
                                                                                }
                                                                        },
                                                                }
                                                        }
-
-                                                       if !add_htlc_msgs.is_empty() || !fail_htlc_msgs.is_empty() {
-                                                               let (commitment_msg, monitor_update) = match chan.get_mut().send_commitment(&self.logger) {
-                                                                       Ok(res) => res,
-                                                                       Err(e) => {
-                                                                               // We surely failed send_commitment due to bad keys, in that case
-                                                                               // close channel and then send error message to peer.
-                                                                               let counterparty_node_id = chan.get().get_counterparty_node_id();
-                                                                               let err: Result<(), _>  = match e {
-                                                                                       ChannelError::Ignore(_) | ChannelError::Warn(_) => {
-                                                                                               panic!("Stated return value requirements in send_commitment() were not met");
-                                                                                       }
-                                                                                       ChannelError::Close(msg) => {
-                                                                                               log_trace!(self.logger, "Closing channel {} due to Close-required error: {}", log_bytes!(chan.key()[..]), msg);
-                                                                                               let mut channel = remove_channel!(self, chan);
-                                                                                               // ChannelClosed event is generated by handle_error for us.
-                                                                                               Err(MsgHandleErrInternal::from_finish_shutdown(msg, channel.channel_id(), channel.get_user_id(), channel.force_shutdown(true), self.get_channel_update_for_broadcast(&channel).ok()))
-                                                                                       },
-                                                                               };
-                                                                               handle_errors.push((counterparty_node_id, err));
-                                                                               continue;
-                                                                       }
-                                                               };
-                                                               match self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
-                                                                       ChannelMonitorUpdateStatus::Completed => {},
-                                                                       e => {
-                                                                               handle_errors.push((chan.get().get_counterparty_node_id(), handle_monitor_update_res!(self, e, chan, RAACommitmentOrder::CommitmentFirst, false, true)));
-                                                                               continue;
-                                                                       }
-                                                               }
-                                                               log_debug!(self.logger, "Forwarding HTLCs resulted in a commitment update with {} HTLCs added and {} HTLCs failed for channel {}",
-                                                                       add_htlc_msgs.len(), fail_htlc_msgs.len(), log_bytes!(chan.get().channel_id()));
-                                                               channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
-                                                                       node_id: chan.get().get_counterparty_node_id(),
-                                                                       updates: msgs::CommitmentUpdate {
-                                                                               update_add_htlcs: add_htlc_msgs,
-                                                                               update_fulfill_htlcs: Vec::new(),
-                                                                               update_fail_htlcs: fail_htlc_msgs,
-                                                                               update_fail_malformed_htlcs: Vec::new(),
-                                                                               update_fee: None,
-                                                                               commitment_signed: commitment_msg,
-                                                                       },
-                                                               });
-                                                       }
                                                }
                                        }
                                } else {
@@ -3505,7 +3433,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
                                                                macro_rules! check_total_value {
                                                                        ($payment_data: expr, $payment_preimage: expr) => {{
-                                                                               let mut payment_received_generated = false;
+                                                                               let mut payment_claimable_generated = false;
                                                                                let purpose = || {
                                                                                        events::PaymentPurpose::InvoicePayment {
                                                                                                payment_preimage: $payment_preimage,
@@ -3556,14 +3484,14 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                                                                                via_channel_id: Some(prev_channel_id),
                                                                                                via_user_channel_id: Some(prev_user_channel_id),
                                                                                        });
-                                                                                       payment_received_generated = true;
+                                                                                       payment_claimable_generated = true;
                                                                                } else {
                                                                                        // Nothing to do - we haven't reached the total
                                                                                        // payment value yet, wait until we receive more
                                                                                        // MPP parts.
                                                                                        htlcs.push(claimable_htlc);
                                                                                }
-                                                                               payment_received_generated
+                                                                               payment_claimable_generated
                                                                        }}
                                                                }
 
@@ -3631,8 +3559,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                                                                                log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap());
                                                                                        fail_htlc!(claimable_htlc, payment_hash);
                                                                                } else {
-                                                                                       let payment_received_generated = check_total_value!(payment_data, inbound_payment.get().payment_preimage);
-                                                                                       if payment_received_generated {
+                                                                                       let payment_claimable_generated = check_total_value!(payment_data, inbound_payment.get().payment_preimage);
+                                                                                       if payment_claimable_generated {
                                                                                                inbound_payment.remove_entry();
                                                                                        }
                                                                                }
@@ -3653,9 +3581,11 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                }
                self.forward_htlcs(&mut phantom_receives);
 
-               for (counterparty_node_id, err) in handle_errors.drain(..) {
-                       let _ = handle_error!(self, err, counterparty_node_id);
-               }
+               // Freeing the holding cell here is relatively redundant - in practice we'll do it when we
+               // next get a `get_and_clear_pending_msg_events` call, but some tests rely on it, and it's
+               // nice to do the work now if we can rather than while we're trying to get messages in the
+               // network stack.
+               self.check_free_holding_cells();
 
                if new_events.is_empty() { return }
                let mut events = self.pending_events.lock().unwrap();
@@ -3693,59 +3623,24 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                self.process_background_events();
        }
 
-       fn update_channel_fee(&self, pending_msg_events: &mut Vec<events::MessageSendEvent>, chan_id: &[u8; 32], chan: &mut Channel<<K::Target as KeysInterface>::Signer>, new_feerate: u32) -> (bool, NotifyOption, Result<(), MsgHandleErrInternal>) {
-               if !chan.is_outbound() { return (true, NotifyOption::SkipPersist, Ok(())); }
+       fn update_channel_fee(&self, chan_id: &[u8; 32], chan: &mut Channel<<K::Target as KeysInterface>::Signer>, new_feerate: u32) -> NotifyOption {
+               if !chan.is_outbound() { return NotifyOption::SkipPersist; }
                // If the feerate has decreased by less than half, don't bother
                if new_feerate <= chan.get_feerate() && new_feerate * 2 > chan.get_feerate() {
                        log_trace!(self.logger, "Channel {} does not qualify for a feerate change from {} to {}.",
                                log_bytes!(chan_id[..]), chan.get_feerate(), new_feerate);
-                       return (true, NotifyOption::SkipPersist, Ok(()));
+                       return NotifyOption::SkipPersist;
                }
                if !chan.is_live() {
                        log_trace!(self.logger, "Channel {} does not qualify for a feerate change from {} to {} as it cannot currently be updated (probably the peer is disconnected).",
                                log_bytes!(chan_id[..]), chan.get_feerate(), new_feerate);
-                       return (true, NotifyOption::SkipPersist, Ok(()));
+                       return NotifyOption::SkipPersist;
                }
                log_trace!(self.logger, "Channel {} qualifies for a feerate change from {} to {}.",
                        log_bytes!(chan_id[..]), chan.get_feerate(), new_feerate);
 
-               let mut retain_channel = true;
-               let res = match chan.send_update_fee_and_commit(new_feerate, &self.logger) {
-                       Ok(res) => Ok(res),
-                       Err(e) => {
-                               let (drop, res) = convert_chan_err!(self, e, chan, chan_id);
-                               if drop { retain_channel = false; }
-                               Err(res)
-                       }
-               };
-               let ret_err = match res {
-                       Ok(Some((update_fee, commitment_signed, monitor_update))) => {
-                               match self.chain_monitor.update_channel(chan.get_funding_txo().unwrap(), monitor_update) {
-                                       ChannelMonitorUpdateStatus::Completed => {
-                                               pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
-                                                       node_id: chan.get_counterparty_node_id(),
-                                                       updates: msgs::CommitmentUpdate {
-                                                               update_add_htlcs: Vec::new(),
-                                                               update_fulfill_htlcs: Vec::new(),
-                                                               update_fail_htlcs: Vec::new(),
-                                                               update_fail_malformed_htlcs: Vec::new(),
-                                                               update_fee: Some(update_fee),
-                                                               commitment_signed,
-                                                       },
-                                               });
-                                               Ok(())
-                                       },
-                                       e => {
-                                               let (res, drop) = handle_monitor_update_res!(self, e, chan, RAACommitmentOrder::CommitmentFirst, chan_id, COMMITMENT_UPDATE_ONLY);
-                                               if drop { retain_channel = false; }
-                                               res
-                                       }
-                               }
-                       },
-                       Ok(None) => Ok(()),
-                       Err(e) => Err(e),
-               };
-               (retain_channel, NotifyOption::DoPersist, ret_err)
+               chan.queue_update_fee(new_feerate, &self.logger);
+               NotifyOption::DoPersist
        }
 
        #[cfg(fuzzing)]
@@ -3759,19 +3654,10 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
                        let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
 
-                       let mut handle_errors = Vec::new();
-                       {
-                               let mut channel_state_lock = self.channel_state.lock().unwrap();
-                               let channel_state = &mut *channel_state_lock;
-                               let pending_msg_events = &mut channel_state.pending_msg_events;
-                               channel_state.by_id.retain(|chan_id, chan| {
-                                       let (retain_channel, chan_needs_persist, err) = self.update_channel_fee(pending_msg_events, chan_id, chan, new_feerate);
-                                       if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
-                                       if err.is_err() {
-                                               handle_errors.push(err);
-                                       }
-                                       retain_channel
-                               });
+                       let mut channel_state = self.channel_state.lock().unwrap();
+                       for (chan_id, chan) in channel_state.by_id.iter_mut() {
+                               let chan_needs_persist = self.update_channel_fee(chan_id, chan, new_feerate);
+                               if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
                        }
 
                        should_persist
@@ -3836,20 +3722,15 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
                        let new_feerate = self.fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
 
-                       let mut handle_errors = Vec::new();
+                       let mut handle_errors: Vec<(Result<(), _>, _)> = Vec::new();
                        let mut timed_out_mpp_htlcs = Vec::new();
                        {
                                let mut channel_state_lock = self.channel_state.lock().unwrap();
                                let channel_state = &mut *channel_state_lock;
                                let pending_msg_events = &mut channel_state.pending_msg_events;
                                channel_state.by_id.retain(|chan_id, chan| {
-                                       let counterparty_node_id = chan.get_counterparty_node_id();
-                                       let (retain_channel, chan_needs_persist, err) = self.update_channel_fee(pending_msg_events, chan_id, chan, new_feerate);
+                                       let chan_needs_persist = self.update_channel_fee(chan_id, chan, new_feerate);
                                        if chan_needs_persist == NotifyOption::DoPersist { should_persist = NotifyOption::DoPersist; }
-                                       if err.is_err() {
-                                               handle_errors.push((err, counterparty_node_id));
-                                       }
-                                       if !retain_channel { return false; }
 
                                        if let Err(e) = chan.timer_check_closing_negotiation_progress() {
                                                let (needs_close, err) = convert_chan_err!(self, e, chan, chan_id);
@@ -3924,6 +3805,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
                        self.remove_stale_resolved_payments();
 
+                       // Technically we don't need to do this here, but if we have holding cell entries in a
+                       // channel that need freeing, it's better to do that here and block a background task
+                       // than block the message queueing pipeline.
+                       if self.check_free_holding_cells() {
+                               should_persist = NotifyOption::DoPersist;
+                       }
+
                        should_persist
                });
        }
@@ -4086,90 +3974,48 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                } else { None };
                                log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
 
-                               let path_failure = match &onion_error {
-                                       &HTLCFailReason::LightningError { ref err } => {
+                               let path_failure = {
 #[cfg(test)]
-                                               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());
+                                       let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_error.decode_onion_failure(&self.secp_ctx, &self.logger, &source);
 #[cfg(not(test))]
-                                               let (network_update, short_channel_id, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
-
-                                               if self.payment_is_probe(payment_hash, &payment_id) {
-                                                       if !payment_retryable {
-                                                               events::Event::ProbeSuccessful {
-                                                                       payment_id: *payment_id,
-                                                                       payment_hash: payment_hash.clone(),
-                                                                       path: path.clone(),
-                                                               }
-                                                       } else {
-                                                               events::Event::ProbeFailed {
-                                                                       payment_id: *payment_id,
-                                                                       payment_hash: payment_hash.clone(),
-                                                                       path: path.clone(),
-                                                                       short_channel_id,
-                                                               }
-                                                       }
-                                               } else {
-                                                       // 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!
-                                                       if let Some(scid) = short_channel_id {
-                                                               retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
-                                                       }
-                                                       events::Event::PaymentPathFailed {
-                                                               payment_id: Some(*payment_id),
-                                                               payment_hash: payment_hash.clone(),
-                                                               payment_failed_permanently: !payment_retryable,
-                                                               network_update,
-                                                               all_paths_failed,
-                                                               path: path.clone(),
-                                                               short_channel_id,
-                                                               retry,
-                                                               #[cfg(test)]
-                                                               error_code: onion_error_code,
-                                                               #[cfg(test)]
-                                                               error_data: onion_error_data
-                                                       }
-                                               }
-                                       },
-                                       &HTLCFailReason::Reason {
-#[cfg(test)]
-                                                       ref failure_code,
-#[cfg(test)]
-                                                       ref data,
-                                                       .. } => {
-                                               // we get a fail_malformed_htlc from the first hop
-                                               // TODO: We'd like to generate a NetworkUpdate for temporary
-                                               // failures here, but that would be insufficient as find_route
-                                               // generally ignores its view of our own channels as we provide them via
-                                               // ChannelDetails.
-                                               // TODO: For non-temporary failures, we really should be closing the
-                                               // channel here as we apparently can't relay through them anyway.
-                                               let scid = path.first().unwrap().short_channel_id;
-                                               retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
-
-                                               if self.payment_is_probe(payment_hash, &payment_id) {
-                                                       events::Event::ProbeFailed {
+                                       let (network_update, short_channel_id, payment_retryable, _, _) = onion_error.decode_onion_failure(&self.secp_ctx, &self.logger, &source);
+
+                                       if self.payment_is_probe(payment_hash, &payment_id) {
+                                               if !payment_retryable {
+                                                       events::Event::ProbeSuccessful {
                                                                payment_id: *payment_id,
                                                                payment_hash: payment_hash.clone(),
                                                                path: path.clone(),
-                                                               short_channel_id: Some(scid),
                                                        }
                                                } else {
-                                                       events::Event::PaymentPathFailed {
-                                                               payment_id: Some(*payment_id),
+                                                       events::Event::ProbeFailed {
+                                                               payment_id: *payment_id,
                                                                payment_hash: payment_hash.clone(),
-                                                               payment_failed_permanently: false,
-                                                               network_update: None,
-                                                               all_paths_failed,
                                                                path: path.clone(),
-                                                               short_channel_id: Some(scid),
-                                                               retry,
-#[cfg(test)]
-                                                               error_code: Some(*failure_code),
-#[cfg(test)]
-                                                               error_data: Some(data.clone()),
+                                                               short_channel_id,
                                                        }
                                                }
+                                       } else {
+                                               // 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!
+                                               if let Some(scid) = short_channel_id {
+                                                       retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid));
+                                               }
+                                               events::Event::PaymentPathFailed {
+                                                       payment_id: Some(*payment_id),
+                                                       payment_hash: payment_hash.clone(),
+                                                       payment_failed_permanently: !payment_retryable,
+                                                       network_update,
+                                                       all_paths_failed,
+                                                       path: path.clone(),
+                                                       short_channel_id,
+                                                       retry,
+                                                       #[cfg(test)]
+                                                       error_code: onion_error_code,
+                                                       #[cfg(test)]
+                                                       error_data: onion_error_data
+                                               }
                                        }
                                };
                                let mut pending_events = self.pending_events.lock().unwrap();
@@ -4177,23 +4023,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                if let Some(ev) = full_failure_ev { pending_events.push(ev); }
                        },
                        HTLCSource::PreviousHopData(HTLCPreviousHopData { ref short_channel_id, ref htlc_id, ref incoming_packet_shared_secret, ref phantom_shared_secret, ref outpoint }) => {
-                               let err_packet = match onion_error {
-                                       HTLCFailReason::Reason { ref failure_code, ref data } => {
-                                               log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards from us with code {}", log_bytes!(payment_hash.0), failure_code);
-                                               if let Some(phantom_ss) = phantom_shared_secret {
-                                                       let phantom_packet = onion_utils::build_failure_packet(phantom_ss, *failure_code, &data[..]).encode();
-                                                       let encrypted_phantom_packet = onion_utils::encrypt_failure_packet(phantom_ss, &phantom_packet);
-                                                       onion_utils::encrypt_failure_packet(incoming_packet_shared_secret, &encrypted_phantom_packet.data[..])
-                                               } else {
-                                                       let packet = onion_utils::build_failure_packet(incoming_packet_shared_secret, *failure_code, &data[..]).encode();
-                                                       onion_utils::encrypt_failure_packet(incoming_packet_shared_secret, &packet)
-                                               }
-                                       },
-                                       HTLCFailReason::LightningError { err } => {
-                                               log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards with pre-built LightningError", log_bytes!(payment_hash.0));
-                                               onion_utils::encrypt_failure_packet(incoming_packet_shared_secret, &err.data)
-                                       }
-                               };
+                               log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards from us with {:?}", log_bytes!(payment_hash.0), onion_error);
+                               let err_packet = onion_error.get_encrypted_failure_packet(incoming_packet_shared_secret, phantom_shared_secret);
 
                                let mut forward_event = None;
                                let mut forward_htlcs = self.forward_htlcs.lock().unwrap();
@@ -4240,7 +4071,6 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
        /// [`process_pending_events`]: EventsProvider::process_pending_events
        /// [`create_inbound_payment`]: Self::create_inbound_payment
        /// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
-       /// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
        pub fn claim_funds(&self, payment_preimage: PaymentPreimage) {
                let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
 
@@ -4426,6 +4256,10 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                        match self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
                                                ChannelMonitorUpdateStatus::Completed => {},
                                                e => {
+                                                       // TODO: This needs to be handled somehow - if we receive a monitor update
+                                                       // with a preimage we *must* somehow manage to propagate it to the upstream
+                                                       // channel, or we must have an ability to receive the same update and try
+                                                       // again on restart.
                                                        log_given_level!(self.logger, if e == ChannelMonitorUpdateStatus::PermanentFailure { Level::Error } else { Level::Info },
                                                                "Failed to update channel monitor with preimage {:?} immediately prior to force-close: {:?}",
                                                                payment_preimage, e);
@@ -5142,10 +4976,10 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                                PendingHTLCStatus::Forward(PendingHTLCInfo { ref incoming_shared_secret, .. }) => {
                                                        let reason = if (error_code & 0x1000) != 0 {
                                                                let (real_code, error_data) = self.get_htlc_inbound_temp_fail_err_and_data(error_code, chan);
-                                                               onion_utils::build_first_hop_failure_packet(incoming_shared_secret, real_code, &error_data)
+                                                               HTLCFailReason::reason(real_code, error_data)
                                                        } else {
-                                                               onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &[])
-                                                       };
+                                                               HTLCFailReason::from_failure_code(error_code)
+                                                       }.get_encrypted_failure_packet(incoming_shared_secret, &None);
                                                        let msg = msgs::UpdateFailHTLC {
                                                                channel_id: msg.channel_id,
                                                                htlc_id: msg.htlc_id,
@@ -5189,7 +5023,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                if chan.get().get_counterparty_node_id() != *counterparty_node_id {
                                        return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
                                }
-                               try_chan_entry!(self, chan.get_mut().update_fail_htlc(&msg, HTLCFailReason::LightningError { err: msg.reason.clone() }), chan);
+                               try_chan_entry!(self, chan.get_mut().update_fail_htlc(&msg, HTLCFailReason::from_msg(msg)), chan);
                        },
                        hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
                }
@@ -5208,7 +5042,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
                                        let chan_err: ChannelError = ChannelError::Close("Got update_fail_malformed_htlc with BADONION not set".to_owned());
                                        try_chan_entry!(self, Err(chan_err), chan);
                                }
-                               try_chan_entry!(self, chan.get_mut().update_fail_malformed_htlc(&msg, HTLCFailReason::from_failure_code(msg.failure_code)), chan);
+                               try_chan_entry!(self, chan.get_mut().update_fail_malformed_htlc(&msg, HTLCFailReason::reason(msg.failure_code, msg.sha256_of_onion.to_vec())), chan);
                                Ok(())
                        },
                        hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
@@ -5626,11 +5460,6 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
        /// Check the holding cell in each channel and free any pending HTLCs in them if possible.
        /// Returns whether there were any updates such as if pending HTLCs were freed or a monitor
        /// update was applied.
-       ///
-       /// This should only apply to HTLCs which were added to the holding cell because we were
-       /// waiting on a monitor update to finish. In that case, we don't want to free the holding cell
-       /// directly in `channel_monitor_updated` as it may introduce deadlocks calling back into user
-       /// code to inform them of a channel monitor update.
        fn check_free_holding_cells(&self) -> bool {
                let mut has_monitor_update = false;
                let mut failed_htlcs = Vec::new();
@@ -7097,16 +6926,6 @@ impl Writeable for HTLCSource {
        }
 }
 
-impl_writeable_tlv_based_enum!(HTLCFailReason,
-       (0, LightningError) => {
-               (0, err, required),
-       },
-       (1, Reason) => {
-               (0, failure_code, required),
-               (2, data, vec_type),
-       },
-;);
-
 impl_writeable_tlv_based!(PendingAddHTLCInfo, {
        (0, forward_info, required),
        (1, prev_user_channel_id, (default_value, 0)),
@@ -7708,17 +7527,19 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                                        }
                                        for (htlc_source, htlc) in monitor.get_all_current_outbound_htlcs() {
                                                if let HTLCSource::PreviousHopData(prev_hop_data) = htlc_source {
+                                                       let pending_forward_matches_htlc = |info: &PendingAddHTLCInfo| {
+                                                               info.prev_funding_outpoint == prev_hop_data.outpoint &&
+                                                                       info.prev_htlc_id == prev_hop_data.htlc_id
+                                                       };
                                                        // The ChannelMonitor is now responsible for this HTLC's
                                                        // failure/success and will let us know what its outcome is. If we
-                                                       // still have an entry for this HTLC in `forward_htlcs`, we were
-                                                       // apparently not persisted after the monitor was when forwarding
-                                                       // the payment.
+                                                       // still have an entry for this HTLC in `forward_htlcs` or
+                                                       // `pending_intercepted_htlcs`, we were apparently not persisted after
+                                                       // the monitor was when forwarding the payment.
                                                        forward_htlcs.retain(|_, forwards| {
                                                                forwards.retain(|forward| {
                                                                        if let HTLCForwardInfo::AddHTLC(htlc_info) = forward {
-                                                                               if htlc_info.prev_short_channel_id == prev_hop_data.short_channel_id &&
-                                                                                       htlc_info.prev_htlc_id == prev_hop_data.htlc_id
-                                                                               {
+                                                                               if pending_forward_matches_htlc(&htlc_info) {
                                                                                        log_info!(args.logger, "Removing pending to-forward HTLC with hash {} as it was forwarded to the closed channel {}",
                                                                                                log_bytes!(htlc.payment_hash.0), log_bytes!(monitor.get_funding_txo().0.to_channel_id()));
                                                                                        false
@@ -7726,7 +7547,19 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                                                                        } else { true }
                                                                });
                                                                !forwards.is_empty()
-                                                       })
+                                                       });
+                                                       pending_intercepted_htlcs.as_mut().unwrap().retain(|intercepted_id, htlc_info| {
+                                                               if pending_forward_matches_htlc(&htlc_info) {
+                                                                       log_info!(args.logger, "Removing pending intercepted HTLC with hash {} as it was forwarded to the closed channel {}",
+                                                                               log_bytes!(htlc.payment_hash.0), log_bytes!(monitor.get_funding_txo().0.to_channel_id()));
+                                                                       pending_events_read.retain(|event| {
+                                                                               if let Event::HTLCIntercepted { intercept_id: ev_id, .. } = event {
+                                                                                       intercepted_id != ev_id
+                                                                               } else { true }
+                                                                       });
+                                                                       false
+                                                               } else { true }
+                                                       });
                                                }
                                        }
                                }