Merge pull request #1895 from TheBlueMatt/2022-12-fix-missing-data
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Tue, 6 Dec 2022 22:46:04 +0000 (22:46 +0000)
committerGitHub <noreply@github.com>
Tue, 6 Dec 2022 22:46:04 +0000 (22:46 +0000)
Fix some onion errors and assert their length is correct

1  2 
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/onion_route_tests.rs
lightning/src/ln/onion_utils.rs

index 70a746819e0a2a6659bdf1ebf43fff2117f8663b,9f47bacc74de03f3e3d7b234f5db14ddbdb0cdad..f7ea1ba552bb2c5b4979d30d5b8390bff602a149
@@@ -27,14 -27,15 +27,15 @@@ use crate::ln::features::{ChannelTypeFe
  use crate::ln::msgs;
  use crate::ln::msgs::{DecodeError, OptionalField, DataLossProtect};
  use crate::ln::script::{self, ShutdownScript};
- use crate::ln::channelmanager::{self, CounterpartyForwardingInfo, PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
+ use crate::ln::channelmanager::{self, CounterpartyForwardingInfo, PendingHTLCStatus, HTLCSource, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
  use crate::ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, htlc_success_tx_weight, htlc_timeout_tx_weight, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor, ClosingTransaction};
  use crate::ln::chan_utils;
+ use crate::ln::onion_utils::HTLCFailReason;
  use crate::chain::BestBlock;
  use crate::chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBoundedFeeEstimator};
  use crate::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS};
  use crate::chain::transaction::{OutPoint, TransactionData};
 -use crate::chain::keysinterface::{Sign, KeysInterface};
 +use crate::chain::keysinterface::{Sign, KeysInterface, BaseSign};
  use crate::util::events::ClosureReason;
  use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter};
  use crate::util::logger::Logger;
@@@ -737,10 -738,6 +738,10 @@@ pub(super) struct Channel<Signer: Sign
  
        // We track whether we already emitted a `ChannelReady` event.
        channel_ready_event_emitted: bool,
 +
 +      /// The unique identifier used to re-derive the private key material for the channel through
 +      /// [`KeysInterface::derive_channel_signer`].
 +      channel_keys_id: [u8; 32],
  }
  
  #[cfg(any(test, fuzzing))]
@@@ -913,8 -910,7 +914,8 @@@ impl<Signer: Sign> Channel<Signer> 
                let opt_anchors = false; // TODO - should be based on features
  
                let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
 -              let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
 +              let channel_keys_id = keys_provider.generate_channel_keys_id(false, channel_value_satoshis, user_id);
 +              let holder_signer = keys_provider.derive_channel_signer(channel_value_satoshis, channel_keys_id);
                let pubkeys = holder_signer.pubkeys().clone();
  
                if !their_features.supports_wumbo() && channel_value_satoshis > MAX_FUNDING_SATOSHIS_NO_WUMBO {
                        historical_inbound_htlc_fulfills: HashSet::new(),
  
                        channel_type: Self::get_initial_channel_type(&config),
 +                      channel_keys_id,
                })
        }
  
                        return Err(ChannelError::Close("Channel Type was not understood - we require static remote key".to_owned()));
                }
  
 -              let holder_signer = keys_provider.get_channel_signer(true, msg.funding_satoshis);
 +              let channel_keys_id = keys_provider.generate_channel_keys_id(true, msg.funding_satoshis, user_id);
 +              let holder_signer = keys_provider.derive_channel_signer(msg.funding_satoshis, channel_keys_id);
                let pubkeys = holder_signer.pubkeys().clone();
                let counterparty_pubkeys = ChannelPublicKeys {
                        funding_pubkey: msg.funding_pubkey,
                        historical_inbound_htlc_fulfills: HashSet::new(),
  
                        channel_type,
 +                      channel_keys_id,
                };
  
                Ok(chan)
        /// our counterparty!)
        /// The result is a transaction which we can revoke broadcastership of (ie a "local" transaction)
        /// TODO Some magic rust shit to compile-time check this?
 -      fn build_holder_transaction_keys(&self, commitment_number: u64) -> Result<TxCreationKeys, ChannelError> {
 +      fn build_holder_transaction_keys(&self, commitment_number: u64) -> TxCreationKeys {
                let per_commitment_point = self.holder_signer.get_per_commitment_point(commitment_number, &self.secp_ctx);
                let delayed_payment_base = &self.get_holder_pubkeys().delayed_payment_basepoint;
                let htlc_basepoint = &self.get_holder_pubkeys().htlc_basepoint;
                let counterparty_pubkeys = self.get_counterparty_pubkeys();
  
 -              Ok(secp_check!(TxCreationKeys::derive_new(&self.secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint), "Local tx keys generation got bogus keys".to_owned()))
 +              TxCreationKeys::derive_new(&self.secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint)
        }
  
        #[inline]
        /// Creates a set of keys for build_commitment_transaction to generate a transaction which we
        /// will sign and send to our counterparty.
        /// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created)
 -      fn build_remote_transaction_keys(&self) -> Result<TxCreationKeys, ChannelError> {
 +      fn build_remote_transaction_keys(&self) -> TxCreationKeys {
                //TODO: Ensure that the payment_key derived here ends up in the library users' wallet as we
                //may see payments to it!
                let revocation_basepoint = &self.get_holder_pubkeys().revocation_basepoint;
                let htlc_basepoint = &self.get_holder_pubkeys().htlc_basepoint;
                let counterparty_pubkeys = self.get_counterparty_pubkeys();
  
 -              Ok(secp_check!(TxCreationKeys::derive_new(&self.secp_ctx, &self.counterparty_cur_commitment_point.unwrap(), &counterparty_pubkeys.delayed_payment_basepoint, &counterparty_pubkeys.htlc_basepoint, revocation_basepoint, htlc_basepoint), "Remote tx keys generation got bogus keys".to_owned()))
 +              TxCreationKeys::derive_new(&self.secp_ctx, &self.counterparty_cur_commitment_point.unwrap(), &counterparty_pubkeys.delayed_payment_basepoint, &counterparty_pubkeys.htlc_basepoint, revocation_basepoint, htlc_basepoint)
        }
  
        /// Gets the redeemscript for the funding transaction output (ie the funding transaction output
        fn funding_created_signature<L: Deref>(&mut self, sig: &Signature, logger: &L) -> Result<(Txid, CommitmentTransaction, Signature), ChannelError> where L::Target: Logger {
                let funding_script = self.get_funding_redeemscript();
  
 -              let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
 +              let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number);
                let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, logger).tx;
                {
                        let trusted_tx = initial_commitment_tx.trust();
                        secp_check!(self.secp_ctx.verify_ecdsa(&sighash, &sig, self.counterparty_funding_pubkey()), "Invalid funding_created signature from peer".to_owned());
                }
  
 -              let counterparty_keys = self.build_remote_transaction_keys()?;
 +              let counterparty_keys = self.build_remote_transaction_keys();
                let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx;
  
                let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
                &self.get_counterparty_pubkeys().funding_pubkey
        }
  
 -      pub fn funding_created<L: Deref>(&mut self, msg: &msgs::FundingCreated, best_block: BestBlock, logger: &L) -> Result<(msgs::FundingSigned, ChannelMonitor<Signer>, Option<msgs::ChannelReady>), ChannelError> where L::Target: Logger {
 +      pub fn funding_created<K: Deref, L: Deref>(
 +              &mut self, msg: &msgs::FundingCreated, best_block: BestBlock, keys_source: &K, logger: &L
 +      ) -> Result<(msgs::FundingSigned, ChannelMonitor<<K::Target as KeysInterface>::Signer>, Option<msgs::ChannelReady>), ChannelError>
 +      where
 +              K::Target: KeysInterface,
 +              L::Target: Logger
 +      {
                if self.is_outbound() {
                        return Err(ChannelError::Close("Received funding_created for an outbound channel?".to_owned()));
                }
                self.channel_transaction_parameters.funding_outpoint = Some(funding_txo);
                // This is an externally observable change before we finish all our checks.  In particular
                // funding_created_signature may fail.
 -              self.holder_signer.ready_channel(&self.channel_transaction_parameters);
 +              self.holder_signer.provide_channel_parameters(&self.channel_transaction_parameters);
  
                let (counterparty_initial_commitment_txid, initial_commitment_tx, signature) = match self.funding_created_signature(&msg.signature, logger) {
                        Ok(res) => res,
                let funding_txo_script = funding_redeemscript.to_v0_p2wsh();
                let obscure_factor = get_commitment_transaction_number_obscure_factor(&self.get_holder_pubkeys().payment_point, &self.get_counterparty_pubkeys().payment_point, self.is_outbound());
                let shutdown_script = self.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
 -              let channel_monitor = ChannelMonitor::new(self.secp_ctx.clone(), self.holder_signer.clone(),
 +              let mut monitor_signer = keys_source.derive_channel_signer(self.channel_value_satoshis, self.channel_keys_id);
 +              monitor_signer.provide_channel_parameters(&self.channel_transaction_parameters);
 +              let channel_monitor = ChannelMonitor::new(self.secp_ctx.clone(), monitor_signer,
                                                          shutdown_script, self.get_holder_selected_contest_delay(),
                                                          &self.destination_script, (funding_txo, funding_txo_script.clone()),
                                                          &self.channel_transaction_parameters,
  
        /// Handles a funding_signed message from the remote end.
        /// If this call is successful, broadcast the funding transaction (and not before!)
 -      pub fn funding_signed<L: Deref>(&mut self, msg: &msgs::FundingSigned, best_block: BestBlock, logger: &L) -> Result<(ChannelMonitor<Signer>, Transaction, Option<msgs::ChannelReady>), ChannelError> where L::Target: Logger {
 +      pub fn funding_signed<K: Deref, L: Deref>(
 +              &mut self, msg: &msgs::FundingSigned, best_block: BestBlock, keys_source: &K, logger: &L
 +      ) -> Result<(ChannelMonitor<<K::Target as KeysInterface>::Signer>, Transaction, Option<msgs::ChannelReady>), ChannelError>
 +      where
 +              K::Target: KeysInterface,
 +              L::Target: Logger
 +      {
                if !self.is_outbound() {
                        return Err(ChannelError::Close("Received funding_signed for an inbound channel?".to_owned()));
                }
  
                let funding_script = self.get_funding_redeemscript();
  
 -              let counterparty_keys = self.build_remote_transaction_keys()?;
 +              let counterparty_keys = self.build_remote_transaction_keys();
                let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx;
                let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust();
                let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction();
                log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}",
                        log_bytes!(self.channel_id()), counterparty_initial_bitcoin_tx.txid, encode::serialize_hex(&counterparty_initial_bitcoin_tx.transaction));
  
 -              let holder_signer = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
 +              let holder_signer = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number);
                let initial_commitment_tx = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &holder_signer, true, false, logger).tx;
                {
                        let trusted_tx = initial_commitment_tx.trust();
                let funding_txo_script = funding_redeemscript.to_v0_p2wsh();
                let obscure_factor = get_commitment_transaction_number_obscure_factor(&self.get_holder_pubkeys().payment_point, &self.get_counterparty_pubkeys().payment_point, self.is_outbound());
                let shutdown_script = self.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
 -              let channel_monitor = ChannelMonitor::new(self.secp_ctx.clone(), self.holder_signer.clone(),
 +              let mut monitor_signer = keys_source.derive_channel_signer(self.channel_value_satoshis, self.channel_keys_id);
 +              monitor_signer.provide_channel_parameters(&self.channel_transaction_parameters);
 +              let channel_monitor = ChannelMonitor::new(self.secp_ctx.clone(), monitor_signer,
                                                          shutdown_script, self.get_holder_selected_contest_delay(),
                                                          &self.destination_script, (funding_txo, funding_txo_script),
                                                          &self.channel_transaction_parameters,
  
                let funding_script = self.get_funding_redeemscript();
  
 -              let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number).map_err(|e| (None, e))?;
 +              let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number);
  
                let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, false, logger);
                let commitment_txid = {
                // Before proposing a feerate update, check that we can actually afford the new fee.
                let inbound_stats = self.get_inbound_pending_htlc_stats(Some(feerate_per_kw));
                let outbound_stats = self.get_outbound_pending_htlc_stats(Some(feerate_per_kw));
 -              let keys = if let Ok(keys) = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number) { keys } else { return None; };
 +              let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number);
                let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger);
                let buffer_fee_msat = Channel::<Signer>::commit_tx_fee_sat(feerate_per_kw, commitment_stats.num_nondust_htlcs + outbound_stats.on_holder_tx_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, self.opt_anchors()) * 1000;
                let holder_balance_msat = commitment_stats.local_balance_msat - outbound_stats.holding_cell_msat;
  
        /// If an Err is returned, it is a ChannelError::Close (for get_outbound_funding_created)
        fn get_outbound_funding_created_signature<L: Deref>(&mut self, logger: &L) -> Result<Signature, ChannelError> where L::Target: Logger {
 -              let counterparty_keys = self.build_remote_transaction_keys()?;
 +              let counterparty_keys = self.build_remote_transaction_keys();
                let counterparty_initial_commitment_tx = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, false, logger).tx;
                Ok(self.holder_signer.sign_counterparty_commitment(&counterparty_initial_commitment_tx, Vec::new(), &self.secp_ctx)
                                .map_err(|_| ChannelError::Close("Failed to get signatures for new commitment_signed".to_owned()))?.0)
                }
  
                self.channel_transaction_parameters.funding_outpoint = Some(funding_txo);
 -              self.holder_signer.ready_channel(&self.channel_transaction_parameters);
 +              self.holder_signer.provide_channel_parameters(&self.channel_transaction_parameters);
  
                let signature = match self.get_outbound_funding_created_signature(logger) {
                        Ok(res) => res,
                        return Err(ChannelError::Ignore(format!("Cannot send value that would put us over the max HTLC value in flight our peer will accept ({})", self.counterparty_max_htlc_value_in_flight_msat)));
                }
  
 -              let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number)?;
 +              let keys = self.build_holder_transaction_keys(self.cur_holder_commitment_transaction_number);
                let commitment_stats = self.build_commitment_transaction(self.cur_holder_commitment_transaction_number, &keys, true, true, logger);
                if !self.is_outbound() {
                        // Check that we won't violate the remote channel reserve by adding this HTLC.
        /// Only fails in case of bad keys. Used for channel_reestablish commitment_signed generation
        /// when we shouldn't change HTLC/channel state.
        fn send_commitment_no_state_update<L: Deref>(&self, logger: &L) -> Result<(msgs::CommitmentSigned, (Txid, Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>)), ChannelError> where L::Target: Logger {
 -              let counterparty_keys = self.build_remote_transaction_keys()?;
 +              let counterparty_keys = self.build_remote_transaction_keys();
                let commitment_stats = self.build_commitment_transaction(self.cur_counterparty_commitment_transaction_number, &counterparty_keys, false, true, logger);
                let counterparty_commitment_txid = commitment_stats.tx.trust().txid();
                let (signature, htlc_signatures);
                (monitor_update, dropped_outbound_htlcs)
        }
  
 -      pub fn inflight_htlc_sources(&self) -> impl Iterator<Item=&HTLCSource> {
 +      pub fn inflight_htlc_sources(&self) -> impl Iterator<Item=(&HTLCSource, &PaymentHash)> {
                self.holding_cell_htlc_updates.iter()
                        .flat_map(|htlc_update| {
                                match htlc_update {
 -                                      HTLCUpdateAwaitingACK::AddHTLC { source, .. } => { Some(source) }
 -                                      _ => None
 +                                      HTLCUpdateAwaitingACK::AddHTLC { source, payment_hash, .. }
 +                                              => Some((source, payment_hash)),
 +                                      _ => None,
                                }
                        })
 -                      .chain(self.pending_outbound_htlcs.iter().map(|htlc| &htlc.source))
 +                      .chain(self.pending_outbound_htlcs.iter().map(|htlc| (&htlc.source, &htlc.payment_hash)))
        }
  }
  
 -const SERIALIZATION_VERSION: u8 = 2;
 +const SERIALIZATION_VERSION: u8 = 3;
  const MIN_SERIALIZATION_VERSION: u8 = 2;
  
  impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
@@@ -6021,7 -5997,7 +6022,7 @@@ impl<Signer: Sign> Writeable for Channe
                // Note that we write out as if remove_uncommitted_htlcs_and_mark_paused had just been
                // called.
  
 -              write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
 +              write_ver_prefix!(writer, MIN_SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
  
                // `user_id` used to be a single u64 value. In order to remain backwards compatible with
                // versions prior to 0.0.113, the u128 is serialized as two separate u64 values. We write
                        (21, self.outbound_scid_alias, required),
                        (23, channel_ready_event_emitted, option),
                        (25, user_id_high_opt, option),
 +                      (27, self.channel_keys_id, required),
                });
  
                Ok(())
@@@ -6340,20 -6315,16 +6341,20 @@@ impl<'a, K: Deref> ReadableArgs<(&'a K
  
                let latest_monitor_update_id = Readable::read(reader)?;
  
 -              let keys_len: u32 = Readable::read(reader)?;
 -              let mut keys_data = Vec::with_capacity(cmp::min(keys_len as usize, MAX_ALLOC_SIZE));
 -              while keys_data.len() != keys_len as usize {
 -                      // Read 1KB at a time to avoid accidentally allocating 4GB on corrupted channel keys
 -                      let mut data = [0; 1024];
 -                      let read_slice = &mut data[0..cmp::min(1024, keys_len as usize - keys_data.len())];
 -                      reader.read_exact(read_slice)?;
 -                      keys_data.extend_from_slice(read_slice);
 +              let mut keys_data = None;
 +              if ver <= 2 {
 +                      // Read the serialize signer bytes. We'll choose to deserialize them or not based on whether
 +                      // the `channel_keys_id` TLV is present below.
 +                      let keys_len: u32 = Readable::read(reader)?;
 +                      keys_data = Some(Vec::with_capacity(cmp::min(keys_len as usize, MAX_ALLOC_SIZE)));
 +                      while keys_data.as_ref().unwrap().len() != keys_len as usize {
 +                              // Read 1KB at a time to avoid accidentally allocating 4GB on corrupted channel keys
 +                              let mut data = [0; 1024];
 +                              let read_slice = &mut data[0..cmp::min(1024, keys_len as usize - keys_data.as_ref().unwrap().len())];
 +                              reader.read_exact(read_slice)?;
 +                              keys_data.as_mut().unwrap().extend_from_slice(read_slice);
 +                      }
                }
 -              let holder_signer = keys_source.read_chan_signer(&keys_data)?;
  
                // Read the old serialization for shutdown_pubkey, preferring the TLV field later if set.
                let mut shutdown_scriptpubkey = match <PublicKey as Readable>::read(reader) {
                let mut channel_ready_event_emitted = None;
  
                let mut user_id_high_opt: Option<u64> = None;
 +              let mut channel_keys_id: Option<[u8; 32]> = None;
  
                read_tlv_fields!(reader, {
                        (0, announcement_sigs, option),
                        (21, outbound_scid_alias, option),
                        (23, channel_ready_event_emitted, option),
                        (25, user_id_high_opt, option),
 +                      (27, channel_keys_id, option),
                });
  
 +              let (channel_keys_id, holder_signer) = if let Some(channel_keys_id) = channel_keys_id {
 +                      let mut holder_signer = keys_source.derive_channel_signer(channel_value_satoshis, channel_keys_id);
 +                      // If we've gotten to the funding stage of the channel, populate the signer with its
 +                      // required channel parameters.
 +                      let non_shutdown_state = channel_state & (!MULTI_STATE_FLAGS);
 +                      if non_shutdown_state >= (ChannelState::FundingCreated as u32) {
 +                              holder_signer.provide_channel_parameters(&channel_parameters);
 +                      }
 +                      (channel_keys_id, holder_signer)
 +              } else {
 +                      // `keys_data` can be `None` if we had corrupted data.
 +                      let keys_data = keys_data.ok_or(DecodeError::InvalidValue)?;
 +                      let holder_signer = keys_source.read_chan_signer(&keys_data)?;
 +                      (holder_signer.channel_keys_id(), holder_signer)
 +              };
 +
                if let Some(preimages) = preimages_opt {
                        let mut iter = preimages.into_iter();
                        for htlc in pending_outbound_htlcs.iter_mut() {
                        historical_inbound_htlc_fulfills,
  
                        channel_type: channel_type.unwrap(),
 +                      channel_keys_id,
                })
        }
  }
@@@ -6784,7 -6736,7 +6785,7 @@@ mod tests 
        use crate::ln::chan_utils::{htlc_success_tx_weight, htlc_timeout_tx_weight};
        use crate::chain::BestBlock;
        use crate::chain::chaininterface::{FeeEstimator, LowerBoundedFeeEstimator, ConfirmationTarget};
 -      use crate::chain::keysinterface::{InMemorySigner, Recipient, KeyMaterial, KeysInterface};
 +      use crate::chain::keysinterface::{BaseSign, InMemorySigner, Recipient, KeyMaterial, KeysInterface};
        use crate::chain::transaction::OutPoint;
        use crate::util::config::UserConfig;
        use crate::util::enforcing_trait_impls::EnforcingSigner;
                        ShutdownScript::new_p2wpkh_from_pubkey(PublicKey::from_secret_key(&secp_ctx, &channel_close_key))
                }
  
 -              fn get_channel_signer(&self, _inbound: bool, _channel_value_satoshis: u64) -> InMemorySigner {
 +              fn generate_channel_keys_id(&self, _inbound: bool, _channel_value_satoshis: u64, _user_channel_id: u128) -> [u8; 32] {
 +                      self.signer.channel_keys_id()
 +              }
 +              fn derive_channel_signer(&self, _channel_value_satoshis: u64, _channel_keys_id: [u8; 32]) -> Self::Signer {
                        self.signer.clone()
                }
                fn get_secure_random_bytes(&self) -> [u8; 32] { [0; 32] }
                }]};
                let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
                let funding_created_msg = node_a_chan.get_outbound_funding_created(tx.clone(), funding_outpoint, &&logger).unwrap();
 -              let (funding_signed_msg, _, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&logger).unwrap();
 +              let (funding_signed_msg, _, _) = node_b_chan.funding_created(&funding_created_msg, best_block, &&keys_provider, &&logger).unwrap();
  
                // Node B --> Node A: funding signed
 -              let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&logger);
 +              let _ = node_a_chan.funding_signed(&funding_signed_msg, best_block, &&keys_provider, &&logger);
  
                // Now disconnect the two nodes and check that the commitment point in
                // Node B's channel_reestablish message is sane.
                                selected_contest_delay: 144
                        });
                chan.channel_transaction_parameters.funding_outpoint = Some(funding_info);
 -              signer.ready_channel(&chan.channel_transaction_parameters);
 +              signer.provide_channel_parameters(&chan.channel_transaction_parameters);
  
                assert_eq!(counterparty_pubkeys.payment_point.serialize()[..],
                           hex::decode("032c0b7cf95324a07d05398b240174dc0c2be444d96b159aa6c7f7b1e668680991").unwrap()[..]);
                let per_commitment_secret = SecretKey::from_slice(&hex::decode("1f1e1d1c1b1a191817161514131211100f0e0d0c0b0a09080706050403020100").unwrap()[..]).unwrap();
                let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
                let htlc_basepoint = &chan.holder_signer.pubkeys().htlc_basepoint;
 -              let keys = TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint).unwrap();
 +              let keys = TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, delayed_payment_base, htlc_basepoint, &counterparty_pubkeys.revocation_basepoint, &counterparty_pubkeys.htlc_basepoint);
  
                macro_rules! test_commitment {
                        ( $counterparty_sig_hex: expr, $sig_hex: expr, $tx_hex: expr, $($remain:tt)* ) => {
                let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret);
                assert_eq!(per_commitment_point.serialize()[..], hex::decode("025f7117a78150fe2ef97db7cfc83bd57b2e2c0d0dd25eaf467a4a1c2a45ce1486").unwrap()[..]);
  
 -              assert_eq!(chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &base_point).unwrap().serialize()[..],
 +              assert_eq!(chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &base_point).serialize()[..],
                                hex::decode("0235f2dbfaa89b57ec7b055afe29849ef7ddfeb1cefdb9ebdc43f5494984db29e5").unwrap()[..]);
  
 -              assert_eq!(chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &base_secret).unwrap(),
 +              assert_eq!(chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &base_secret),
                                SecretKey::from_slice(&hex::decode("cbced912d3b21bf196a766651e436aff192362621ce317704ea2f75d87e7be0f").unwrap()[..]).unwrap());
  
 -              assert_eq!(chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &base_point).unwrap().serialize()[..],
 +              assert_eq!(chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &base_point).serialize()[..],
                                hex::decode("02916e326636d19c33f13e8c0c3a03dd157f332f3e99c317c141dd865eb01f8ff0").unwrap()[..]);
  
 -              assert_eq!(chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_secret, &base_secret).unwrap(),
 +              assert_eq!(chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_secret, &base_secret),
                                SecretKey::from_slice(&hex::decode("d09ffff62ddb2297ab000cc85bcb4283fdeb6aa052affbc9dddcf33b61078110").unwrap()[..]).unwrap());
        }
  
index 5c4e5394375a4df4327e3ae1669ea1508a5a3217,6e5df15908adb097ea5b96655b234e46ac66907a..d4209ce9539695138e80e1eeaec5b93671c57c2a
@@@ -49,12 -49,13 +49,13 @@@ use crate::ln::features::InvoiceFeature
  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};
  use crate::util::config::{UserConfig, ChannelConfig};
  use crate::util::events::{Event, EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination};
 -use crate::util::{byte_utils, events};
 +use crate::util::events;
  use crate::util::wakers::{Future, Notifier};
  use crate::util::scid_utils::fake_scid;
  use crate::util::ser::{BigSize, FixedLengthReader, Readable, ReadableArgs, MaybeReadable, Writeable, Writer, VecWriter};
@@@ -276,27 -277,6 +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>,
@@@ -746,9 -726,9 +726,9 @@@ pub struct ChannelManager<M: Deref, T: 
        channel_state: Mutex<ChannelHolder<<K::Target as KeysInterface>::Signer>>,
  
        /// Storage for PaymentSecrets and any requirements on future inbound payments before we will
 -      /// expose them to users via a PaymentReceived event. HTLCs which do not meet the requirements
 +      /// expose them to users via a PaymentClaimable event. HTLCs which do not meet the requirements
        /// here are failed when we process them as pending-forwardable-HTLCs, and entries are removed
 -      /// after we generate a PaymentReceived upon receipt of all MPP parts or when they time out.
 +      /// after we generate a PaymentClaimable upon receipt of all MPP parts or when they time out.
        ///
        /// See `ChannelManager` struct-level documentation for lock order requirements.
        pending_inbound_payments: Mutex<HashMap<PaymentHash, PendingInboundPayment>>,
@@@ -2053,7 -2033,7 +2033,7 @@@ impl<M: Deref, T: Deref, K: Deref, F: D
                        return Err(ReceiveError {
                                msg: "Upstream node set CLTV to the wrong value",
                                err_code: 18,
 -                              err_data: byte_utils::be32_to_array(cltv_expiry).to_vec()
 +                              err_data: cltv_expiry.to_be_bytes().to_vec()
                        })
                }
                // final_expiry_too_soon
                // 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",
                        });
                }
                if hop_data.amt_to_forward > amt_msat {
                        return Err(ReceiveError {
                                err_code: 19,
 -                              err_data: byte_utils::be64_to_array(amt_msat).to_vec(),
 +                              err_data: amt_msat.to_be_bytes().to_vec(),
                                msg: "Upstream node sent less than we were supposed to receive in payment",
                        });
                }
                                        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),
                                        }));
                                }
                        }
                        // 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 {
                                                }
                                                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
                                                (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[..]);
                                }
                let session_priv = SecretKey::from_slice(&session_priv_bytes[..]).expect("RNG is busted");
  
                let onion_keys = onion_utils::construct_onion_keys(&self.secp_ctx, &path, &session_priv)
 -                      .map_err(|_| APIError::RouteError{err: "Pubkey along hop was maliciously selected"})?;
 +                      .map_err(|_| APIError::InvalidRoute{err: "Pubkey along hop was maliciously selected"})?;
                let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(path, total_value, payment_secret, cur_height, keysend_preimage)?;
                if onion_utils::route_size_insane(&onion_payloads) {
 -                      return Err(APIError::RouteError{err: "Route size too large considering onion data"});
 +                      return Err(APIError::InvalidRoute{err: "Route size too large considering onion data"});
                }
                let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash);
  
                        if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
                                match {
                                        if chan.get().get_counterparty_node_id() != path.first().unwrap().pubkey {
 -                                              return Err(APIError::RouteError{err: "Node ID mismatch on first hop!"});
 +                                              return Err(APIError::InvalidRoute{err: "Node ID mismatch on first hop!"});
                                        }
                                        if !chan.get().is_live() {
                                                return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!".to_owned()});
        /// fields for more info.
        ///
        /// If a pending payment is currently in-flight with the same [`PaymentId`] provided, this
 -      /// method will error with an [`APIError::RouteError`]. Note, however, that once a payment
 +      /// method will error with an [`APIError::InvalidRoute`]. Note, however, that once a payment
        /// is no longer pending (either via [`ChannelManager::abandon_payment`], or handling of an
        /// [`Event::PaymentSent`]) LDK will not stop you from sending a second payment with the same
        /// [`PaymentId`].
        /// PaymentSendFailure for more info.
        ///
        /// In general, a path may raise:
 -      ///  * [`APIError::RouteError`] when an invalid route or forwarding parameter (cltv_delta, fee,
 +      ///  * [`APIError::InvalidRoute`] when an invalid route or forwarding parameter (cltv_delta, fee,
        ///    node public key) is specified.
        ///  * [`APIError::ChannelUnavailable`] if the next-hop channel is not available for updates
        ///    (including due to previous monitor update failure or new permanent monitor update
  
        fn send_payment_internal(&self, route: &Route, payment_hash: PaymentHash, payment_secret: &Option<PaymentSecret>, keysend_preimage: Option<PaymentPreimage>, payment_id: PaymentId, recv_value_msat: Option<u64>, onion_session_privs: Vec<[u8; 32]>) -> Result<(), PaymentSendFailure> {
                if route.paths.len() < 1 {
 -                      return Err(PaymentSendFailure::ParameterError(APIError::RouteError{err: "There must be at least one path to send over"}));
 +                      return Err(PaymentSendFailure::ParameterError(APIError::InvalidRoute{err: "There must be at least one path to send over"}));
                }
                if payment_secret.is_none() && route.paths.len() > 1 {
                        return Err(PaymentSendFailure::ParameterError(APIError::APIMisuseError{err: "Payment secret is required for multi-path payments".to_string()}));
                let mut path_errs = Vec::with_capacity(route.paths.len());
                '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"}));
 +                              path_errs.push(Err(APIError::InvalidRoute{err: "Path didn't go anywhere/had bogus size"}));
                                continue 'path_check;
                        }
                        for (idx, hop) in path.iter().enumerate() {
                                if idx != path.len() - 1 && hop.pubkey == our_node_id {
 -                                      path_errs.push(Err(APIError::RouteError{err: "Path went through us but wasn't a simple rebalance loop to us"}));
 +                                      path_errs.push(Err(APIError::InvalidRoute{err: "Path went through us but wasn't a simple rebalance loop to us"}));
                                        continue 'path_check;
                                }
                        }
                let next_hop_scid = match self.channel_state.lock().unwrap().by_id.get(next_hop_channel_id) {
                        Some(chan) => {
                                if !chan.is_usable() {
 -                                      return Err(APIError::APIMisuseError {
 -                                              err: format!("Channel with id {:?} not fully established", next_hop_channel_id)
 +                                      return Err(APIError::ChannelUnavailable {
 +                                              err: format!("Channel with id {} not fully established", log_bytes!(*next_hop_channel_id))
                                        })
                                }
                                chan.get_short_channel_id().unwrap_or(chan.outbound_scid_alias())
                        },
 -                      None => return Err(APIError::APIMisuseError {
 -                              err: format!("Channel with id {:?} not found", next_hop_channel_id)
 +                      None => return Err(APIError::ChannelUnavailable {
 +                              err: format!("Channel with id {} not found", log_bytes!(*next_hop_channel_id))
                        })
                };
  
                let payment = self.pending_intercepted_htlcs.lock().unwrap().remove(&intercept_id)
                        .ok_or_else(|| APIError::APIMisuseError {
 -                              err: format!("Payment with intercept id {:?} not found", intercept_id.0)
 +                              err: format!("Payment with intercept id {} not found", log_bytes!(intercept_id.0))
                        })?;
  
                let routing = match payment.forward_info.routing {
  
                let payment = self.pending_intercepted_htlcs.lock().unwrap().remove(&intercept_id)
                        .ok_or_else(|| APIError::APIMisuseError {
 -                              err: format!("Payment with InterceptId {:?} not found", intercept_id)
 +                              err: format!("Payment with intercept id {} not found", log_bytes!(intercept_id.0))
                        })?;
  
                if let PendingHTLCRouting::Forward { short_channel_id, .. } = payment.forward_info.routing {
  
                                                                macro_rules! fail_htlc {
                                                                        ($htlc: expr, $payment_hash: expr) => {
 -                                                                              let mut htlc_msat_height_data = byte_utils::be64_to_array($htlc.value).to_vec();
 +                                                                              let mut htlc_msat_height_data = $htlc.value.to_be_bytes().to_vec();
                                                                                htlc_msat_height_data.extend_from_slice(
 -                                                                                      &byte_utils::be32_to_array(self.best_block.read().unwrap().height()),
 +                                                                                      &self.best_block.read().unwrap().height().to_be_bytes(),
                                                                                );
                                                                                failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData {
                                                                                                short_channel_id: $htlc.prev_hop.short_channel_id,
  
                                                                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,
                                                                                } else if total_value == $payment_data.total_msat {
                                                                                        let prev_channel_id = prev_funding_outpoint.to_channel_id();
                                                                                        htlcs.push(claimable_htlc);
 -                                                                                      new_events.push(events::Event::PaymentReceived {
 +                                                                                      new_events.push(events::Event::PaymentClaimable {
                                                                                                receiver_node_id: Some(receiver_node_id),
                                                                                                payment_hash,
                                                                                                purpose: purpose(),
                                                                                                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
                                                                        }}
                                                                }
  
                                                                                                                let purpose = events::PaymentPurpose::SpontaneousPayment(preimage);
                                                                                                                e.insert((purpose.clone(), vec![claimable_htlc]));
                                                                                                                let prev_channel_id = prev_funding_outpoint.to_channel_id();
 -                                                                                                              new_events.push(events::Event::PaymentReceived {
 +                                                                                                              new_events.push(events::Event::PaymentClaimable {
                                                                                                                        receiver_node_id: Some(receiver_node_id),
                                                                                                                        payment_hash,
                                                                                                                        amount_msat: outgoing_amt_msat,
                                                                                                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();
                                                                                        }
                                                                                }
        }
  
        /// Indicates that the preimage for payment_hash is unknown or the received amount is incorrect
 -      /// after a PaymentReceived event, failing the HTLC back to its origin and freeing resources
 +      /// after a PaymentClaimable event, failing the HTLC back to its origin and freeing resources
        /// along the path (including in our own channel on which we received it).
        ///
        /// Note that in some cases around unclean shutdown, it is possible the payment may have
        /// already been claimed by you via [`ChannelManager::claim_funds`] prior to you seeing (a
 -      /// second copy of) the [`events::Event::PaymentReceived`] event. Alternatively, the payment
 +      /// second copy of) the [`events::Event::PaymentClaimable`] event. Alternatively, the payment
        /// may have already been failed automatically by LDK if it was nearing its expiration time.
        ///
        /// While LDK will never claim a payment automatically on your behalf (i.e. without you calling
                let removed_source = self.claimable_htlcs.lock().unwrap().remove(payment_hash);
                if let Some((_, mut sources)) = removed_source {
                        for htlc in sources.drain(..) {
 -                              let mut htlc_msat_height_data = byte_utils::be64_to_array(htlc.value).to_vec();
 -                              htlc_msat_height_data.extend_from_slice(&byte_utils::be32_to_array(
 -                                              self.best_block.read().unwrap().height()));
 +                              let mut htlc_msat_height_data = htlc.value.to_be_bytes().to_vec();
 +                              htlc_msat_height_data.extend_from_slice(&self.best_block.read().unwrap().height().to_be_bytes());
                                let source = HTLCSource::PreviousHopData(htlc.prev_hop);
                                let reason = HTLCFailReason::reason(0x4000 | 15, htlc_msat_height_data);
                                let receiver = HTLCDestination::FailedPayment { payment_hash: *payment_hash };
                                } 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();
                                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();
                }
        }
  
 -      /// Provides a payment preimage in response to [`Event::PaymentReceived`], generating any
 +      /// Provides a payment preimage in response to [`Event::PaymentClaimable`], generating any
        /// [`MessageSendEvent`]s needed to claim the payment.
        ///
        /// Note that calling this method does *not* guarantee that the payment has been claimed. You
        /// provided to your [`EventHandler`] when [`process_pending_events`] is next called.
        ///
        /// Note that if you did not set an `amount_msat` when calling [`create_inbound_payment`] or
 -      /// [`create_inbound_payment_for_hash`] you must check that the amount in the `PaymentReceived`
 +      /// [`create_inbound_payment_for_hash`] you must check that the amount in the `PaymentClaimable`
        /// event matches your expectation. If you fail to do so and call this method, you may provide
        /// the sender "proof-of-payment" when they did not fulfill the full expected payment.
        ///
 -      /// [`Event::PaymentReceived`]: crate::util::events::Event::PaymentReceived
 +      /// [`Event::PaymentClaimable`]: crate::util::events::Event::PaymentClaimable
        /// [`Event::PaymentClaimed`]: crate::util::events::Event::PaymentClaimed
        /// [`process_pending_events`]: EventsProvider::process_pending_events
        /// [`create_inbound_payment`]: Self::create_inbound_payment
                        mem::drop(channel_state_lock);
                        if !valid_mpp {
                                for htlc in sources.drain(..) {
 -                                      let mut htlc_msat_height_data = byte_utils::be64_to_array(htlc.value).to_vec();
 -                                      htlc_msat_height_data.extend_from_slice(&byte_utils::be32_to_array(
 -                                              self.best_block.read().unwrap().height()));
 +                                      let mut htlc_msat_height_data = htlc.value.to_be_bytes().to_vec();
 +                                      htlc_msat_height_data.extend_from_slice(&self.best_block.read().unwrap().height().to_be_bytes());
                                        let source = HTLCSource::PreviousHopData(htlc.prev_hop);
                                        let reason = HTLCFailReason::reason(0x4000 | 15, htlc_msat_height_data);
                                        let receiver = HTLCDestination::FailedPayment { payment_hash };
                                        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.temporary_channel_id));
                                        }
 -                                      (try_chan_entry!(self, chan.get_mut().funding_created(msg, best_block, &self.logger), chan), chan.remove())
 +                                      (try_chan_entry!(self, chan.get_mut().funding_created(msg, best_block, &self.keys_manager, &self.logger), chan), chan.remove())
                                },
                                hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.temporary_channel_id))
                        }
                                        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));
                                        }
 -                                      let (monitor, funding_tx, channel_ready) = match chan.get_mut().funding_signed(&msg, best_block, &self.logger) {
 +                                      let (monitor, funding_tx, channel_ready) = match chan.get_mut().funding_signed(&msg, best_block, &self.keys_manager, &self.logger) {
                                                Ok(update) => update,
                                                Err(e) => try_chan_entry!(self, Err(e), chan),
                                        };
                                                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,
                                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))
                }
                                        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))
        /// This differs from [`create_inbound_payment_for_hash`] only in that it generates the
        /// [`PaymentHash`] and [`PaymentPreimage`] for you.
        ///
 -      /// The [`PaymentPreimage`] will ultimately be returned to you in the [`PaymentReceived`], which
 -      /// will have the [`PaymentReceived::payment_preimage`] field filled in. That should then be
 +      /// The [`PaymentPreimage`] will ultimately be returned to you in the [`PaymentClaimable`], which
 +      /// will have the [`PaymentClaimable::payment_preimage`] field filled in. That should then be
        /// passed directly to [`claim_funds`].
        ///
        /// See [`create_inbound_payment_for_hash`] for detailed documentation on behavior and requirements.
        /// Errors if `min_value_msat` is greater than total bitcoin supply.
        ///
        /// [`claim_funds`]: Self::claim_funds
 -      /// [`PaymentReceived`]: events::Event::PaymentReceived
 -      /// [`PaymentReceived::payment_preimage`]: events::Event::PaymentReceived::payment_preimage
 +      /// [`PaymentClaimable`]: events::Event::PaymentClaimable
 +      /// [`PaymentClaimable::payment_preimage`]: events::Event::PaymentClaimable::payment_preimage
        /// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
        pub fn create_inbound_payment(&self, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> Result<(PaymentHash, PaymentSecret), ()> {
                inbound_payment::create(&self.inbound_payment_key, min_value_msat, invoice_expiry_delta_secs, &self.keys_manager, self.highest_seen_timestamp.load(Ordering::Acquire) as u64)
        /// Gets a [`PaymentSecret`] for a given [`PaymentHash`], for which the payment preimage is
        /// stored external to LDK.
        ///
 -      /// A [`PaymentReceived`] event will only be generated if the [`PaymentSecret`] matches a
 +      /// A [`PaymentClaimable`] event will only be generated if the [`PaymentSecret`] matches a
        /// payment secret fetched via this method or [`create_inbound_payment`], and which is at least
        /// the `min_value_msat` provided here, if one is provided.
        ///
        ///
        /// `min_value_msat` should be set if the invoice being generated contains a value. Any payment
        /// received for the returned [`PaymentHash`] will be required to be at least `min_value_msat`
 -      /// before a [`PaymentReceived`] event will be generated, ensuring that we do not provide the
 +      /// before a [`PaymentClaimable`] event will be generated, ensuring that we do not provide the
        /// sender "proof-of-payment" unless they have paid the required amount.
        ///
        /// `invoice_expiry_delta_secs` describes the number of seconds that the invoice is valid for
        ///
        /// Note that we use block header time to time-out pending inbound payments (with some margin
        /// to compensate for the inaccuracy of block header timestamps). Thus, in practice we will
 -      /// accept a payment and generate a [`PaymentReceived`] event for some time after the expiry.
 +      /// accept a payment and generate a [`PaymentClaimable`] event for some time after the expiry.
        /// If you need exact expiry semantics, you should enforce them upon receipt of
 -      /// [`PaymentReceived`].
 +      /// [`PaymentClaimable`].
        ///
        /// Note that invoices generated for inbound payments should have their `min_final_cltv_expiry`
        /// set to at least [`MIN_FINAL_CLTV_EXPIRY`].
        /// Errors if `min_value_msat` is greater than total bitcoin supply.
        ///
        /// [`create_inbound_payment`]: Self::create_inbound_payment
 -      /// [`PaymentReceived`]: events::Event::PaymentReceived
 +      /// [`PaymentClaimable`]: events::Event::PaymentClaimable
        pub fn create_inbound_payment_for_hash(&self, payment_hash: PaymentHash, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> Result<PaymentSecret, ()> {
                inbound_payment::create_from_hash(&self.inbound_payment_key, min_value_msat, payment_hash, invoice_expiry_delta_secs, self.highest_seen_timestamp.load(Ordering::Acquire) as u64)
        }
                let mut inflight_htlcs = InFlightHtlcs::new();
  
                for chan in self.channel_state.lock().unwrap().by_id.values() {
 -                      for htlc_source in chan.inflight_htlc_sources() {
 +                      for (htlc_source, _) in chan.inflight_htlc_sources() {
                                if let HTLCSource::OutboundRoute { path, .. } = htlc_source {
                                        inflight_htlcs.process_path(path, self.get_our_node_id());
                                }
                events.into_inner()
        }
  
 +      #[cfg(test)]
 +      pub fn pop_pending_event(&self) -> Option<events::Event> {
 +              let mut events = self.pending_events.lock().unwrap();
 +              if events.is_empty() { None } else { Some(events.remove(0)) }
 +      }
 +
        #[cfg(test)]
        pub fn has_pending_payments(&self) -> bool {
                !self.pending_outbound_payments.lock().unwrap().is_empty()
@@@ -6287,8 -6219,8 +6223,8 @@@ wher
                                        // number of blocks we generally consider it to take to do a commitment update,
                                        // just give up on it and fail the HTLC.
                                        if height >= htlc.cltv_expiry - HTLC_FAIL_BACK_BUFFER {
 -                                              let mut htlc_msat_height_data = byte_utils::be64_to_array(htlc.value).to_vec();
 -                                              htlc_msat_height_data.extend_from_slice(&byte_utils::be32_to_array(height));
 +                                              let mut htlc_msat_height_data = htlc.value.to_be_bytes().to_vec();
 +                                              htlc_msat_height_data.extend_from_slice(&height.to_be_bytes());
  
                                                timed_out_htlcs.push((HTLCSource::PreviousHopData(htlc.prev_hop.clone()), payment_hash.clone(),
                                                        HTLCFailReason::reason(0x4000 | 15, htlc_msat_height_data),
@@@ -7037,16 -6969,6 +6973,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)),
@@@ -7424,25 -7346,6 +7350,25 @@@ impl<'a, M: Deref, T: Deref, K: Deref, 
                                                user_channel_id: channel.get_user_id(),
                                                reason: ClosureReason::OutdatedChannelManager
                                        });
 +                                      for (channel_htlc_source, payment_hash) in channel.inflight_htlc_sources() {
 +                                              let mut found_htlc = false;
 +                                              for (monitor_htlc_source, _) in monitor.get_all_current_outbound_htlcs() {
 +                                                      if *channel_htlc_source == monitor_htlc_source { found_htlc = true; break; }
 +                                              }
 +                                              if !found_htlc {
 +                                                      // If we have some HTLCs in the channel which are not present in the newer
 +                                                      // ChannelMonitor, they have been removed and should be failed back to
 +                                                      // ensure we don't forget them entirely. Note that if the missing HTLC(s)
 +                                                      // were actually claimed we'd have generated and ensured the previous-hop
 +                                                      // claim update ChannelMonitor updates were persisted prior to persising
 +                                                      // the ChannelMonitor update for the forward leg, so attempting to fail the
 +                                                      // backwards leg of the HTLC will simply be rejected.
 +                                                      log_info!(args.logger,
 +                                                              "Failing HTLC with hash {} as it is missing in the ChannelMonitor for channel {} but was present in the (stale) ChannelManager",
 +                                                              log_bytes!(channel.channel_id()), log_bytes!(payment_hash.0));
 +                                                      failed_htlcs.push((channel_htlc_source.clone(), *payment_hash, channel.get_counterparty_node_id(), channel.channel_id()));
 +                                              }
 +                                      }
                                } else {
                                        log_info!(args.logger, "Successfully loaded channel {}", log_bytes!(channel.channel_id()));
                                        if let Some(short_channel_id) = channel.get_short_channel_id() {
                                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>()));
                                                        }
                                                }
                                        }
 +                                      for (htlc_source, htlc) in monitor.get_all_current_outbound_htlcs() {
 +                                              if let HTLCSource::PreviousHopData(prev_hop_data) = htlc_source {
 +                                                      // 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.
 +                                                      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
 +                                                                              {
 +                                                                                      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
 +                                                                              } else { true }
 +                                                                      } else { true }
 +                                                              });
 +                                                              !forwards.is_empty()
 +                                                      })
 +                                              }
 +                                      }
                                }
                        }
                }
  
 +              if !forward_htlcs.is_empty() {
 +                      // 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 inbound_pmt_key_material = args.keys_manager.get_inbound_payment_key_material();
                let expanded_inbound_key = inbound_payment::ExpandedKey::new(&inbound_pmt_key_material);
  
@@@ -8604,7 -8483,7 +8530,7 @@@ pub mod bench 
                                $node_b.handle_revoke_and_ack(&$node_a.get_our_node_id(), &get_event_msg!(NodeHolder { node: &$node_a }, MessageSendEvent::SendRevokeAndACK, $node_b.get_our_node_id()));
  
                                expect_pending_htlcs_forwardable!(NodeHolder { node: &$node_b });
 -                              expect_payment_received!(NodeHolder { node: &$node_b }, payment_hash, payment_secret, 10_000);
 +                              expect_payment_claimable!(NodeHolder { node: &$node_b }, payment_hash, payment_secret, 10_000);
                                $node_b.claim_funds(payment_preimage);
                                expect_payment_claimed!(NodeHolder { node: &$node_b }, payment_hash, 10_000);
  
index 48b8f05b3e2b5fe0099d6a9a1203c05137e24cd2,7978114c0ff6c90c902ae1a5f5c5adc26f6f0ea4..ca2fd76584174a5e1be7b2c405c28a0cf29541d3
@@@ -25,7 -25,7 +25,7 @@@ use crate::ln::msgs::{ChannelMessageHan
  use crate::ln::wire::Encode;
  use crate::util::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
  use crate::util::ser::{Writeable, Writer};
 -use crate::util::{byte_utils, test_utils};
 +use crate::util::test_utils;
  use crate::util::config::{UserConfig, ChannelConfig};
  use crate::util::errors::APIError;
  
@@@ -125,7 -125,7 +125,7 @@@ fn run_onion_failure_test_with_fail_int
  
                        if test_case == 2 || test_case == 200 {
                                expect_htlc_forward!(&nodes[2]);
 -                              expect_event!(&nodes[2], Event::PaymentReceived);
 +                              expect_event!(&nodes[2], Event::PaymentClaimable);
                                callback_node();
                                expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], vec![HTLCDestination::FailedPayment { payment_hash: payment_hash.clone() }]);
                        }
@@@ -548,7 -548,7 +548,7 @@@ fn test_onion_failure() 
                connect_blocks(&nodes[0], height - nodes[0].best_block_info().1);
                connect_blocks(&nodes[1], height - nodes[1].best_block_info().1);
                connect_blocks(&nodes[2], height - nodes[2].best_block_info().1);
-       }, || {}, true, Some(17), None, None);
+       }, || {}, false, Some(0x4000 | 15), None, None);
  
        run_onion_failure_test("final_incorrect_cltv_expiry", 1, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || {
                for (_, pending_forwards) in nodes[1].node.forward_htlcs.lock().unwrap().iter_mut() {
@@@ -1053,8 -1053,8 +1053,8 @@@ fn test_phantom_final_incorrect_cltv_ex
        commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
  
        // Ensure the payment fails with the expected error.
 -      let expected_cltv = 82;
 -      let error_data = byte_utils::be32_to_array(expected_cltv).to_vec();
 +      let expected_cltv: u32 = 82;
 +      let error_data = expected_cltv.to_be_bytes().to_vec();
        let mut fail_conditions = PaymentFailedConditions::new()
                .blamed_scid(phantom_scid)
                .expected_htlc_error_data(18, &error_data);
@@@ -1099,10 -1099,93 +1099,93 @@@ fn test_phantom_failure_too_low_cltv() 
        commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
  
        // Ensure the payment fails with the expected error.
-       let error_data = Vec::new();
+       let mut error_data = recv_value_msat.to_be_bytes().to_vec();
+       error_data.extend_from_slice(
+               &nodes[0].node.best_block.read().unwrap().height().to_be_bytes(),
+       );
+       let mut fail_conditions = PaymentFailedConditions::new()
+               .blamed_scid(phantom_scid)
+               .expected_htlc_error_data(0x4000 | 15, &error_data);
+       expect_payment_failed_conditions(&nodes[0], payment_hash, true, fail_conditions);
+ }
+ #[test]
+ fn test_phantom_failure_modified_cltv() {
+       // Test that we fail back phantoms if the upstream node fiddled with the CLTV too much with the
+       // correct error code.
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       let channel = create_announced_chan_between_nodes(&nodes, 0, 1, channelmanager::provided_init_features(), channelmanager::provided_init_features());
+       // Get the route.
+       let recv_value_msat = 10_000;
+       let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat));
+       let (mut route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel);
+       // Route the HTLC through to the destination.
+       nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+       let mut update_add = update_0.update_add_htlcs[0].clone();
+       // Modify the route to have a too-low cltv.
+       update_add.cltv_expiry -= 10;
+       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
+       commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);
+       let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+       assert!(update_1.update_fail_htlcs.len() == 1);
+       let fail_msg = update_1.update_fail_htlcs[0].clone();
+       nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
+       commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
+       // Ensure the payment fails with the expected error.
+       let mut fail_conditions = PaymentFailedConditions::new()
+               .blamed_scid(phantom_scid)
+               .expected_htlc_error_data(0x2000 | 2, &[]);
+       expect_payment_failed_conditions(&nodes[0], payment_hash, false, fail_conditions);
+ }
+ #[test]
+ fn test_phantom_failure_expires_too_soon() {
+       // Test that we fail back phantoms if the HTLC got delayed and we got blocks in between with
+       // the correct error code.
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       let channel = create_announced_chan_between_nodes(&nodes, 0, 1, channelmanager::provided_init_features(), channelmanager::provided_init_features());
+       // Get the route.
+       let recv_value_msat = 10_000;
+       let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat));
+       let (mut route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel);
+       // Route the HTLC through to the destination.
+       nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret), PaymentId(payment_hash.0)).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
+       let mut update_add = update_0.update_add_htlcs[0].clone();
+       connect_blocks(&nodes[1], CLTV_FAR_FAR_AWAY);
+       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
+       commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true);
+       let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
+       assert!(update_1.update_fail_htlcs.len() == 1);
+       let fail_msg = update_1.update_fail_htlcs[0].clone();
+       nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
+       commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
+       // Ensure the payment fails with the expected error.
        let mut fail_conditions = PaymentFailedConditions::new()
                .blamed_scid(phantom_scid)
-               .expected_htlc_error_data(17, &error_data);
+               .expected_htlc_error_data(0x2000 | 2, &[]);
        expect_payment_failed_conditions(&nodes[0], payment_hash, false, fail_conditions);
  }
  
@@@ -1144,8 -1227,10 +1227,8 @@@ fn test_phantom_failure_too_low_recv_am
        commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
  
        // Ensure the payment fails with the expected error.
 -      let mut error_data = byte_utils::be64_to_array(bad_recv_amt_msat).to_vec();
 -      error_data.extend_from_slice(
 -              &byte_utils::be32_to_array(nodes[1].node.best_block.read().unwrap().height()),
 -      );
 +      let mut error_data = bad_recv_amt_msat.to_be_bytes().to_vec();
 +      error_data.extend_from_slice(&nodes[1].node.best_block.read().unwrap().height().to_be_bytes());
        let mut fail_conditions = PaymentFailedConditions::new()
                .blamed_scid(phantom_scid)
                .expected_htlc_error_data(0x4000 | 15, &error_data);
@@@ -1227,7 -1312,7 +1310,7 @@@ fn test_phantom_failure_reject_payment(
        nodes[1].node.process_pending_htlc_forwards();
        expect_pending_htlcs_forwardable_ignore!(nodes[1]);
        nodes[1].node.process_pending_htlc_forwards();
 -      expect_payment_received!(nodes[1], payment_hash, payment_secret, recv_amt_msat, None, route.paths[0].last().unwrap().pubkey);
 +      expect_payment_claimable!(nodes[1], payment_hash, payment_secret, recv_amt_msat, None, route.paths[0].last().unwrap().pubkey);
        nodes[1].node.fail_htlc_backwards(&payment_hash);
        expect_pending_htlcs_forwardable_and_htlc_handling_failed_ignore!(nodes[1], vec![HTLCDestination::FailedPayment { payment_hash }]);
        nodes[1].node.process_pending_htlc_forwards();
        commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false);
  
        // Ensure the payment fails with the expected error.
 -      let mut error_data = byte_utils::be64_to_array(recv_amt_msat).to_vec();
 -      error_data.extend_from_slice(
 -              &byte_utils::be32_to_array(nodes[1].node.best_block.read().unwrap().height()),
 -      );
 +      let mut error_data = recv_amt_msat.to_be_bytes().to_vec();
 +      error_data.extend_from_slice(&nodes[1].node.best_block.read().unwrap().height().to_be_bytes());
        let mut fail_conditions = PaymentFailedConditions::new()
                .blamed_scid(phantom_scid)
                .expected_htlc_error_data(0x4000 | 15, &error_data);
index 6f85e2db8d60f2684bc867bb5636cfa22b00f168,76a39c698017d0c030913c22df0e5d43ac05cb3d..1abaf5920a51d8476d56b6f65968f725bd1a781a
@@@ -15,7 -15,7 +15,7 @@@ use crate::routing::gossip::NetworkUpda
  use crate::routing::router::RouteHop;
  use crate::util::chacha20::{ChaCha20, ChaChaReader};
  use crate::util::errors::{self, APIError};
- use crate::util::ser::{Readable, ReadableArgs, Writeable, LengthCalculatingWriter};
+ use crate::util::ser::{Readable, ReadableArgs, Writeable, Writer, LengthCalculatingWriter};
  use crate::util::logger::Logger;
  
  use bitcoin::hashes::{Hash, HashEngine};
@@@ -182,11 -182,11 +182,11 @@@ pub(super) fn build_onion_payloads(path
                });
                cur_value_msat += hop.fee_msat;
                if cur_value_msat >= 21000000 * 100000000 * 1000 {
 -                      return Err(APIError::RouteError{err: "Channel fees overflowed?"});
 +                      return Err(APIError::InvalidRoute{err: "Channel fees overflowed?"});
                }
                cur_cltv += hop.cltv_expiry_delta as u32;
                if cur_cltv >= 500000000 {
 -                      return Err(APIError::RouteError{err: "Channel CLTV overflowed?"});
 +                      return Err(APIError::InvalidRoute{err: "Channel CLTV overflowed?"});
                }
                last_short_channel_id = hop.short_channel_id;
        }
@@@ -382,7 -382,7 +382,7 @@@ pub(super) fn build_failure_packet(shar
        packet
  }
  
- #[inline]
+ #[cfg(test)]
  pub(super) fn build_first_hop_failure_packet(shared_secret: &[u8], failure_type: u16, failure_data: &[u8]) -> msgs::OnionErrorPacket {
        let failure_packet = build_failure_packet(shared_secret, failure_type, failure_data);
        encrypt_failure_packet(shared_secret, &failure_packet.encode()[..])
@@@ -592,6 -592,146 +592,146 @@@ pub(super) fn process_onion_failure<T: 
        } else { unreachable!(); }
  }
  
+ #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
+ pub(super) struct HTLCFailReason(HTLCFailReasonRepr);
+ #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
+ enum HTLCFailReasonRepr {
+       LightningError {
+               err: msgs::OnionErrorPacket,
+       },
+       Reason {
+               failure_code: u16,
+               data: Vec<u8>,
+       }
+ }
+ impl core::fmt::Debug for HTLCFailReason {
+       fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
+               match self.0 {
+                       HTLCFailReasonRepr::Reason { ref failure_code, .. } => {
+                               write!(f, "HTLC error code {}", failure_code)
+                       },
+                       HTLCFailReasonRepr::LightningError { .. } => {
+                               write!(f, "pre-built LightningError")
+                       }
+               }
+       }
+ }
+ impl Writeable for HTLCFailReason {
+       fn write<W: Writer>(&self, writer: &mut W) -> Result<(), crate::io::Error> {
+               self.0.write(writer)
+       }
+ }
+ impl Readable for HTLCFailReason {
+       fn read<R: Read>(reader: &mut R) -> Result<Self, msgs::DecodeError> {
+               Ok(Self(Readable::read(reader)?))
+       }
+ }
+ impl_writeable_tlv_based_enum!(HTLCFailReasonRepr,
+       (0, LightningError) => {
+               (0, err, required),
+       },
+       (1, Reason) => {
+               (0, failure_code, required),
+               (2, data, vec_type),
+       },
+ ;);
+ impl HTLCFailReason {
+       pub(super) fn reason(failure_code: u16, data: Vec<u8>) -> Self {
+               const BADONION: u16 = 0x8000;
+               const PERM: u16 = 0x4000;
+               const NODE: u16 = 0x2000;
+               const UPDATE: u16 = 0x1000;
+                    if failure_code == 1  | PERM { debug_assert!(data.is_empty()) }
+               else if failure_code == 2  | NODE { debug_assert!(data.is_empty()) }
+               else if failure_code == 2  | PERM | NODE { debug_assert!(data.is_empty()) }
+               else if failure_code == 3  | PERM | NODE { debug_assert!(data.is_empty()) }
+               else if failure_code == 4  | BADONION | PERM { debug_assert_eq!(data.len(), 32) }
+               else if failure_code == 5  | BADONION | PERM { debug_assert_eq!(data.len(), 32) }
+               else if failure_code == 6  | BADONION | PERM { debug_assert_eq!(data.len(), 32) }
+               else if failure_code == 7  | UPDATE {
+                       debug_assert_eq!(data.len() - 2, u16::from_be_bytes(data[0..2].try_into().unwrap()) as usize) }
+               else if failure_code == 8  | PERM { debug_assert!(data.is_empty()) }
+               else if failure_code == 9  | PERM { debug_assert!(data.is_empty()) }
+               else if failure_code == 10 | PERM { debug_assert!(data.is_empty()) }
+               else if failure_code == 11 | UPDATE {
+                       debug_assert_eq!(data.len() - 2 - 8, u16::from_be_bytes(data[8..10].try_into().unwrap()) as usize) }
+               else if failure_code == 12 | UPDATE {
+                       debug_assert_eq!(data.len() - 2 - 8, u16::from_be_bytes(data[8..10].try_into().unwrap()) as usize) }
+               else if failure_code == 13 | UPDATE {
+                       debug_assert_eq!(data.len() - 2 - 4, u16::from_be_bytes(data[4..6].try_into().unwrap()) as usize) }
+               else if failure_code == 14 | UPDATE {
+                       debug_assert_eq!(data.len() - 2, u16::from_be_bytes(data[0..2].try_into().unwrap()) as usize) }
+               else if failure_code == 15 | PERM { debug_assert_eq!(data.len(), 12) }
+               else if failure_code == 18 { debug_assert_eq!(data.len(), 4) }
+               else if failure_code == 19 { debug_assert_eq!(data.len(), 8) }
+               else if failure_code == 20 | UPDATE {
+                       debug_assert_eq!(data.len() - 2 - 2, u16::from_be_bytes(data[2..4].try_into().unwrap()) as usize) }
+               else if failure_code == 21 { debug_assert!(data.is_empty()) }
+               else if failure_code == 22 | PERM { debug_assert!(data.len() <= 11) }
+               else if failure_code == 23 { debug_assert!(data.is_empty()) }
+               else if failure_code & BADONION != 0 {
+                       // We set some bogus BADONION failure codes in test, so ignore unknown ones.
+               }
+               else { debug_assert!(false, "Unknown failure code: {}", failure_code) }
+               Self(HTLCFailReasonRepr::Reason { failure_code, data })
+       }
+       pub(super) fn from_failure_code(failure_code: u16) -> Self {
+               Self::reason(failure_code, Vec::new())
+       }
+       pub(super) fn from_msg(msg: &msgs::UpdateFailHTLC) -> Self {
+               Self(HTLCFailReasonRepr::LightningError { err: msg.reason.clone() })
+       }
+       pub(super) fn get_encrypted_failure_packet(&self, incoming_packet_shared_secret: &[u8; 32], phantom_shared_secret: &Option<[u8; 32]>)
+       -> msgs::OnionErrorPacket {
+               match self.0 {
+                       HTLCFailReasonRepr::Reason { ref failure_code, ref data } => {
+                               if let Some(phantom_ss) = phantom_shared_secret {
+                                       let phantom_packet = build_failure_packet(phantom_ss, *failure_code, &data[..]).encode();
+                                       let encrypted_phantom_packet = encrypt_failure_packet(phantom_ss, &phantom_packet);
+                                       encrypt_failure_packet(incoming_packet_shared_secret, &encrypted_phantom_packet.data[..])
+                               } else {
+                                       let packet = build_failure_packet(incoming_packet_shared_secret, *failure_code, &data[..]).encode();
+                                       encrypt_failure_packet(incoming_packet_shared_secret, &packet)
+                               }
+                       },
+                       HTLCFailReasonRepr::LightningError { ref err } => {
+                               encrypt_failure_packet(incoming_packet_shared_secret, &err.data)
+                       }
+               }
+       }
+       pub(super) fn decode_onion_failure<T: secp256k1::Signing, L: Deref>(
+               &self, secp_ctx: &Secp256k1<T>, logger: &L, htlc_source: &HTLCSource
+       ) -> (Option<NetworkUpdate>, Option<u64>, bool, Option<u16>, Option<Vec<u8>>)
+       where L::Target: Logger {
+               match self.0 {
+                       HTLCFailReasonRepr::LightningError { ref err } => {
+                               process_onion_failure(secp_ctx, logger, &htlc_source, err.data.clone())
+                       },
+                       HTLCFailReasonRepr::Reason { ref failure_code, 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.
+                               if let &HTLCSource::OutboundRoute { ref path, .. } = htlc_source {
+                                       (None, Some(path.first().unwrap().short_channel_id), true, Some(*failure_code), Some(data.clone()))
+                               } else { unreachable!(); }
+                       }
+               }
+       }
+ }
  /// Allows `decode_next_hop` to return the next hop packet bytes for either payments or onion
  /// message forwards.
  pub(crate) trait NextPacketBytes: AsMut<[u8]> {