X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=blobdiff_plain;f=lightning%2Fsrc%2Fln%2Fchannelmanager.rs;h=d4209ce9539695138e80e1eeaec5b93671c57c2a;hb=2390dbcb2236888f1a06f8b5666486eb54ace4b1;hp=c4d90480eecd2baeaeca20fd23299b0de2a00d58;hpb=f79ad2efb161d8ea9ccda9d67cf0ee2d63586cbe;p=rust-lightning

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index c4d90480..d4209ce9 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -49,12 +49,13 @@ use crate::ln::features::InvoiceFeatures;
 use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RoutePath, RouteParameters};
 use crate::ln::msgs;
 use crate::ln::onion_utils;
+use crate::ln::onion_utils::HTLCFailReason;
 use crate::ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT};
 use crate::ln::wire::Encode;
 use crate::chain::keysinterface::{Sign, KeysInterface, KeysManager, Recipient};
 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,17 +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>,
-	}
-}
-
 struct ReceiveError {
 	err_code: u16,
 	err_data: Vec<u8>,
@@ -736,9 +726,9 @@ pub struct ChannelManager<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	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>>,
@@ -1876,8 +1866,9 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		};
 
 		for htlc_source in failed_htlcs.drain(..) {
+			let reason = HTLCFailReason::from_failure_code(0x4000 | 8);
 			let receiver = HTLCDestination::NextHopChannel { node_id: Some(*counterparty_node_id), channel_id: *channel_id };
-			self.fail_htlc_backwards_internal(htlc_source.0, &htlc_source.1, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() }, receiver);
+			self.fail_htlc_backwards_internal(&htlc_source.0, &htlc_source.1, &reason, receiver);
 		}
 
 		let _ = handle_error!(self, result, *counterparty_node_id);
@@ -1934,8 +1925,9 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		log_debug!(self.logger, "Finishing force-closure of channel with {} HTLCs to fail", failed_htlcs.len());
 		for htlc_source in failed_htlcs.drain(..) {
 			let (source, payment_hash, counterparty_node_id, channel_id) = htlc_source;
+			let reason = HTLCFailReason::from_failure_code(0x4000 | 8);
 			let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id };
-			self.fail_htlc_backwards_internal(source, &payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() }, receiver);
+			self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 		}
 		if let Some((funding_txo, monitor_update)) = monitor_update_option {
 			// There isn't anything we can do if we get an update failure - we're already
@@ -2041,7 +2033,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 			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
@@ -2050,17 +2042,20 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		// Also, ensure that, in the case of an unknown preimage for the received payment hash, our
 		// payment logic has enough time to fail the HTLC backward before our onchain logic triggers a
 		// channel closure (see HTLC_FAIL_BACK_BUFFER rationale).
-		if (hop_data.outgoing_cltv_value as u64) <= self.best_block.read().unwrap().height() as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1	{
+		let current_height: u32 = self.best_block.read().unwrap().height();
+		if (hop_data.outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 {
+			let mut err_data = Vec::with_capacity(12);
+			err_data.extend_from_slice(&amt_msat.to_be_bytes());
+			err_data.extend_from_slice(&current_height.to_be_bytes());
 			return Err(ReceiveError {
-				err_code: 17,
-				err_data: Vec::new(),
+				err_code: 0x4000 | 15, err_data,
 				msg: "The final CLTV expiry is too soon to handle",
 			});
 		}
 		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",
 			});
 		}
@@ -2161,7 +2156,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 					return PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
 						channel_id: msg.channel_id,
 						htlc_id: msg.htlc_id,
-						reason: onion_utils::build_first_hop_failure_packet(&shared_secret, $err_code, $data),
+						reason: HTLCFailReason::reason($err_code, $data.to_vec())
+							.get_encrypted_failure_packet(&shared_secret, &None),
 					}));
 				}
 			}
@@ -2226,15 +2222,16 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 			// with a short_channel_id of 0. This is important as various things later assume
 			// short_channel_id is non-0 in any ::Forward.
 			if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
-				if let Some((err, code, chan_update)) = loop {
+				if let Some((err, mut code, chan_update)) = loop {
 					let id_option = self.short_to_chan_info.read().unwrap().get(&short_channel_id).cloned();
 					let mut channel_state = self.channel_state.lock().unwrap();
 					let forwarding_id_opt = match id_option {
 						None => { // unknown_next_peer
 							// Note that this is likely a timing oracle for detecting whether an scid is a
 							// phantom or an intercept.
-							if fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash) ||
-							   fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash)
+							if (self.default_configuration.accept_intercept_htlcs &&
+							   fake_scid::is_valid_intercept(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash)) ||
+							   fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, *short_channel_id, &self.genesis_hash)
 							{
 								None
 							} else {
@@ -2282,10 +2279,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 						}
 						chan_update_opt
 					} else {
-						if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 { // incorrect_cltv_expiry
+						if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
+							// We really should set `incorrect_cltv_expiry` here but as we're not
+							// forwarding over a real channel we can't generate a channel_update
+							// for it. Instead we just return a generic temporary_node_failure.
 							break Some((
 								"Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
-								0x1000 | 13, None,
+								0x2000 | 2, None,
 							));
 						}
 						None
@@ -2331,6 +2331,12 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 						(chan_update.serialized_length() as u16 + 2).write(&mut res).expect("Writes cannot fail");
 						msgs::ChannelUpdate::TYPE.write(&mut res).expect("Writes cannot fail");
 						chan_update.write(&mut res).expect("Writes cannot fail");
+					} else if code & 0x1000 == 0x1000 {
+						// If we're trying to return an error that requires a `channel_update` but
+						// we're forwarding to a phantom or intercept "channel" (i.e. cannot
+						// generate an update), just use the generic "temporary_node_failure"
+						// instead.
+						code = 0x2000 | 2;
 					}
 					return_err!(err, code, &res.0[..]);
 				}
@@ -2406,10 +2412,10 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		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);
 
@@ -2426,7 +2432,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 			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()});
@@ -2501,7 +2507,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// 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`].
@@ -2520,7 +2526,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// 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
@@ -2585,7 +2591,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
 	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()}));
@@ -2595,12 +2601,12 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		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;
 				}
 			}
@@ -3057,14 +3063,19 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// Intercepted HTLCs can be useful for Lightning Service Providers (LSPs) to open a just-in-time
 	/// channel to a receiving node if the node lacks sufficient inbound liquidity.
 	///
-	/// To make use of intercepted HTLCs, use [`ChannelManager::get_intercept_scid`] to generate short
-	/// channel id(s) to put in the receiver's invoice route hints. These route hints will signal to
-	/// LDK to generate an [`HTLCIntercepted`] event when it receives the forwarded HTLC, and this
-	/// method or [`ChannelManager::fail_intercepted_htlc`] MUST be called in response to the event.
+	/// To make use of intercepted HTLCs, set [`UserConfig::accept_intercept_htlcs`] and use
+	/// [`ChannelManager::get_intercept_scid`] to generate short channel id(s) to put in the
+	/// receiver's invoice route hints. These route hints will signal to LDK to generate an
+	/// [`HTLCIntercepted`] event when it receives the forwarded HTLC, and this method or
+	/// [`ChannelManager::fail_intercepted_htlc`] MUST be called in response to the event.
 	///
 	/// Note that LDK does not enforce fee requirements in `amt_to_forward_msat`, and will not stop
 	/// you from forwarding more than you received.
 	///
+	/// Errors if the event was not handled in time, in which case the HTLC was automatically failed
+	/// backwards.
+	///
+	/// [`UserConfig::accept_intercept_htlcs`]: crate::util::config::UserConfig::accept_intercept_htlcs
 	/// [`HTLCIntercepted`]: events::Event::HTLCIntercepted
 	// TODO: when we move to deciding the best outbound channel at forward time, only take
 	// `next_node_id` and not `next_hop_channel_id`
@@ -3072,15 +3083,22 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 
 		let next_hop_scid = match self.channel_state.lock().unwrap().by_id.get(next_hop_channel_id) {
-			Some(chan) => 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)
+			Some(chan) => {
+				if !chan.is_usable() {
+					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::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 {
@@ -3106,13 +3124,16 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// Fails the intercepted HTLC indicated by intercept_id. Should only be called in response to
 	/// an [`HTLCIntercepted`] event. See [`ChannelManager::forward_intercepted_htlc`].
 	///
+	/// Errors if the event was not handled in time, in which case the HTLC was automatically failed
+	/// backwards.
+	///
 	/// [`HTLCIntercepted`]: events::Event::HTLCIntercepted
 	pub fn fail_intercepted_htlc(&self, intercept_id: InterceptId) -> Result<(), APIError> {
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 
 		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 {
@@ -3124,9 +3145,9 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 				phantom_shared_secret: None,
 			});
 
-			let failure_reason = HTLCFailReason::Reason { failure_code: 0x4000 | 10, data: Vec::new() };
+			let failure_reason = HTLCFailReason::from_failure_code(0x4000 | 10);
 			let destination = HTLCDestination::UnknownNextHop { requested_forward_scid: short_channel_id };
-			self.fail_htlc_backwards_internal(htlc_source, &payment.forward_info.payment_hash, failure_reason, destination);
+			self.fail_htlc_backwards_internal(&htlc_source, &payment.forward_info.payment_hash, &failure_reason, destination);
 		} else { unreachable!() } // Only `PendingHTLCRouting::Forward`s are intercepted
 
 		Ok(())
@@ -3179,7 +3200,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 												};
 
 												failed_forwards.push((htlc_source, payment_hash,
-													HTLCFailReason::Reason { failure_code: $err_code, data: $err_data },
+													HTLCFailReason::reason($err_code, $err_data),
 													reason
 												));
 												continue;
@@ -3287,7 +3308,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 												}
 												let (failure_code, data) = self.get_htlc_temp_fail_err_and_data(0x1000|7, short_chan_id, chan.get());
 												failed_forwards.push((htlc_source, payment_hash,
-													HTLCFailReason::Reason { failure_code, data },
+													HTLCFailReason::reason(failure_code, data),
 													HTLCDestination::NextHopChannel { node_id: Some(chan.get().get_counterparty_node_id()), channel_id: forward_chan_id }
 												));
 												continue;
@@ -3423,9 +3444,9 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
 								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,
@@ -3434,7 +3455,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 												incoming_packet_shared_secret: $htlc.prev_hop.incoming_packet_shared_secret,
 												phantom_shared_secret,
 											}), payment_hash,
-											HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data },
+											HTLCFailReason::reason(0x4000 | 15, htlc_msat_height_data),
 											HTLCDestination::FailedPayment { payment_hash: $payment_hash },
 										));
 									}
@@ -3448,7 +3469,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 
 								macro_rules! check_total_value {
 									($payment_data: expr, $payment_preimage: expr) => {{
-										let mut payment_received_generated = false;
+										let mut payment_claimable_generated = false;
 										let purpose = || {
 											events::PaymentPurpose::InvoicePayment {
 												payment_preimage: $payment_preimage,
@@ -3487,7 +3508,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 										} 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(),
@@ -3495,14 +3516,14 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 												via_channel_id: Some(prev_channel_id),
 												via_user_channel_id: Some(prev_user_channel_id),
 											});
-											payment_received_generated = true;
+											payment_claimable_generated = true;
 										} else {
 											// Nothing to do - we haven't reached the total
 											// payment value yet, wait until we receive more
 											// MPP parts.
 											htlcs.push(claimable_htlc);
 										}
-										payment_received_generated
+										payment_claimable_generated
 									}}
 								}
 
@@ -3533,7 +3554,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 														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,
@@ -3565,8 +3586,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 												log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap());
 											fail_htlc!(claimable_htlc, payment_hash);
 										} else {
-											let payment_received_generated = check_total_value!(payment_data, inbound_payment.get().payment_preimage);
-											if payment_received_generated {
+											let payment_claimable_generated = check_total_value!(payment_data, inbound_payment.get().payment_preimage);
+											if payment_claimable_generated {
 												inbound_payment.remove_entry();
 											}
 										}
@@ -3583,7 +3604,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		}
 
 		for (htlc_source, payment_hash, failure_reason, destination) in failed_forwards.drain(..) {
-			self.fail_htlc_backwards_internal(htlc_source, &payment_hash, failure_reason, destination);
+			self.fail_htlc_backwards_internal(&htlc_source, &payment_hash, &failure_reason, destination);
 		}
 		self.forward_htlcs(&mut phantom_receives);
 
@@ -3846,8 +3867,10 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 			});
 
 			for htlc_source in timed_out_mpp_htlcs.drain(..) {
+				let source = HTLCSource::PreviousHopData(htlc_source.0.clone());
+				let reason = HTLCFailReason::from_failure_code(23);
 				let receiver = HTLCDestination::FailedPayment { payment_hash: htlc_source.1 };
-				self.fail_htlc_backwards_internal(HTLCSource::PreviousHopData(htlc_source.0.clone()), &htlc_source.1, HTLCFailReason::Reason { failure_code: 23, data: Vec::new() }, receiver );
+				self.fail_htlc_backwards_internal(&source, &htlc_source.1, &reason, receiver);
 			}
 
 			for (err, counterparty_node_id) in handle_errors.drain(..) {
@@ -3861,12 +3884,12 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	}
 
 	/// 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
@@ -3879,13 +3902,12 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		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()));
-				self.fail_htlc_backwards_internal(
-						HTLCSource::PreviousHopData(htlc.prev_hop), payment_hash,
-						HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data },
-						HTLCDestination::FailedPayment { payment_hash: *payment_hash });
+				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 };
+				self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 			}
 		}
 	}
@@ -3944,23 +3966,24 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		&self, mut htlcs_to_fail: Vec<(HTLCSource, PaymentHash)>, channel_id: [u8; 32],
 		counterparty_node_id: &PublicKey
 	) {
-		for (htlc_src, payment_hash) in htlcs_to_fail.drain(..) {
-			let (failure_code, onion_failure_data) =
-				match self.channel_state.lock().unwrap().by_id.entry(channel_id) {
-					hash_map::Entry::Occupied(chan_entry) => {
-						self.get_htlc_inbound_temp_fail_err_and_data(0x1000|7, &chan_entry.get())
-					},
-					hash_map::Entry::Vacant(_) => (0x4000|10, Vec::new())
-				};
+		let (failure_code, onion_failure_data) =
+			match self.channel_state.lock().unwrap().by_id.entry(channel_id) {
+				hash_map::Entry::Occupied(chan_entry) => {
+					self.get_htlc_inbound_temp_fail_err_and_data(0x1000|7, &chan_entry.get())
+				},
+				hash_map::Entry::Vacant(_) => (0x4000|10, Vec::new())
+			};
 
+		for (htlc_src, payment_hash) in htlcs_to_fail.drain(..) {
+			let reason = HTLCFailReason::reason(failure_code, onion_failure_data.clone());
 			let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id.clone()), channel_id };
-			self.fail_htlc_backwards_internal(htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data }, receiver);
+			self.fail_htlc_backwards_internal(&htlc_src, &payment_hash, &reason, receiver);
 		}
 	}
 
 	/// Fails an HTLC backwards to the sender of it to us.
 	/// Note that we do not assume that channels corresponding to failed HTLCs are still available.
-	fn fail_htlc_backwards_internal(&self, source: HTLCSource, payment_hash: &PaymentHash, onion_error: HTLCFailReason,destination: HTLCDestination) {
+	fn fail_htlc_backwards_internal(&self, source: &HTLCSource, payment_hash: &PaymentHash, onion_error: &HTLCFailReason, destination: HTLCDestination) {
 		#[cfg(debug_assertions)]
 		{
 			// Ensure that the `channel_state` lock is not held when calling this function.
@@ -3979,13 +4002,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		// from block_connected which may run during initialization prior to the chain_monitor
 		// being fully configured. See the docs for `ChannelManagerReadArgs` for more.
 		match source {
-			HTLCSource::OutboundRoute { ref path, session_priv, payment_id, ref payment_params, .. } => {
+			HTLCSource::OutboundRoute { ref path, ref session_priv, ref payment_id, ref payment_params, .. } => {
 				let mut session_priv_bytes = [0; 32];
 				session_priv_bytes.copy_from_slice(&session_priv[..]);
 				let mut outbounds = self.pending_outbound_payments.lock().unwrap();
 				let mut all_paths_failed = false;
 				let mut full_failure_ev = None;
-				if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
+				if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(*payment_id) {
 					if !payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
 						log_trace!(self.logger, "Received duplicative fail for HTLC with payment_hash {}", log_bytes!(payment_hash.0));
 						return;
@@ -3998,7 +4021,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 						all_paths_failed = true;
 						if payment.get().abandoned() {
 							full_failure_ev = Some(events::Event::PaymentFailed {
-								payment_id,
+								payment_id: *payment_id,
 								payment_hash: payment.get().payment_hash().expect("PendingOutboundPayments::RetriesExceeded always has a payment hash set"),
 							});
 							payment.remove();
@@ -4018,126 +4041,69 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 				} else { None };
 				log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
 
-				let path_failure = match &onion_error {
-					&HTLCFailReason::LightningError { ref err } => {
+				let path_failure = {
 #[cfg(test)]
-						let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
+					let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_error.decode_onion_failure(&self.secp_ctx, &self.logger, &source);
 #[cfg(not(test))]
-						let (network_update, short_channel_id, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone());
+					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_hash: payment_hash.clone(),
-									path: path.clone(),
-								}
-							} else {
-								events::Event::ProbeFailed {
-									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),
+					if self.payment_is_probe(payment_hash, &payment_id) {
+						if !payment_retryable {
+							events::Event::ProbeSuccessful {
+								payment_id: *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 {
-								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();
 				pending_events.push(path_failure);
 				if let Some(ev) = full_failure_ev { pending_events.push(ev); }
 			},
-			HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret, phantom_shared_secret, outpoint }) => {
-				let err_packet = match onion_error {
-					HTLCFailReason::Reason { failure_code, 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)
-					}
-				};
+			HTLCSource::PreviousHopData(HTLCPreviousHopData { ref short_channel_id, ref htlc_id, ref incoming_packet_shared_secret, ref phantom_shared_secret, ref outpoint }) => {
+				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();
 				if forward_htlcs.is_empty() {
 					forward_event = Some(Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS));
 				}
-				match forward_htlcs.entry(short_channel_id) {
+				match forward_htlcs.entry(*short_channel_id) {
 					hash_map::Entry::Occupied(mut entry) => {
-						entry.get_mut().push(HTLCForwardInfo::FailHTLC { htlc_id, err_packet });
+						entry.get_mut().push(HTLCForwardInfo::FailHTLC { htlc_id: *htlc_id, err_packet });
 					},
 					hash_map::Entry::Vacant(entry) => {
-						entry.insert(vec!(HTLCForwardInfo::FailHTLC { htlc_id, err_packet }));
+						entry.insert(vec!(HTLCForwardInfo::FailHTLC { htlc_id: *htlc_id, err_packet }));
 					}
 				}
 				mem::drop(forward_htlcs);
@@ -4149,13 +4115,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 				}
 				pending_events.push(events::Event::HTLCHandlingFailed {
 					prev_channel_id: outpoint.to_channel_id(),
-					failed_next_destination: destination
+					failed_next_destination: destination,
 				});
 			},
 		}
 	}
 
-	/// 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
@@ -4163,11 +4129,11 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// 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
@@ -4275,13 +4241,12 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 			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()));
-					self.fail_htlc_backwards_internal(
-						HTLCSource::PreviousHopData(htlc.prev_hop), &payment_hash,
-						HTLCFailReason::Reason { failure_code: 0x4000|15, data: htlc_msat_height_data },
-						HTLCDestination::FailedPayment { payment_hash } );
+					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 };
+					self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 				}
 			}
 
@@ -4605,7 +4570,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		self.finalize_claims(finalized_claims);
 		for failure in pending_failures.drain(..) {
 			let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id: funding_txo.to_channel_id() };
-			self.fail_htlc_backwards_internal(failure.0, &failure.1, failure.2, receiver);
+			self.fail_htlc_backwards_internal(&failure.0, &failure.1, &failure.2, receiver);
 		}
 	}
 
@@ -4785,7 +4750,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 					if chan.get().get_counterparty_node_id() != *counterparty_node_id {
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.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))
 			}
@@ -4856,7 +4821,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 					if chan.get().get_counterparty_node_id() != *counterparty_node_id {
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
 					}
-					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),
 					};
@@ -4973,7 +4938,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		};
 		for htlc_source in dropped_htlcs.drain(..) {
 			let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id.clone()), channel_id: msg.channel_id };
-			self.fail_htlc_backwards_internal(htlc_source.0, &htlc_source.1, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() }, receiver);
+			let reason = HTLCFailReason::from_failure_code(0x4000 | 8);
+			self.fail_htlc_backwards_internal(&htlc_source.0, &htlc_source.1, &reason, receiver);
 		}
 
 		let _ = handle_error!(self, result, *counterparty_node_id);
@@ -5052,10 +5018,10 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 						PendingHTLCStatus::Forward(PendingHTLCInfo { ref incoming_shared_secret, .. }) => {
 							let reason = if (error_code & 0x1000) != 0 {
 								let (real_code, error_data) = self.get_htlc_inbound_temp_fail_err_and_data(error_code, chan);
-								onion_utils::build_first_hop_failure_packet(incoming_shared_secret, real_code, &error_data)
+								HTLCFailReason::reason(real_code, error_data)
 							} else {
-								onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &[])
-							};
+								HTLCFailReason::from_failure_code(error_code)
+							}.get_encrypted_failure_packet(incoming_shared_secret, &None);
 							let msg = msgs::UpdateFailHTLC {
 								channel_id: msg.channel_id,
 								htlc_id: msg.htlc_id,
@@ -5099,7 +5065,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 				if chan.get().get_counterparty_node_id() != *counterparty_node_id {
 					return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
 				}
-				try_chan_entry!(self, chan.get_mut().update_fail_htlc(&msg, HTLCFailReason::LightningError { err: msg.reason.clone() }), chan);
+				try_chan_entry!(self, chan.get_mut().update_fail_htlc(&msg, HTLCFailReason::from_msg(msg)), chan);
 			},
 			hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 		}
@@ -5118,7 +5084,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 					let chan_err: ChannelError = ChannelError::Close("Got update_fail_malformed_htlc with BADONION not set".to_owned());
 					try_chan_entry!(self, Err(chan_err), chan);
 				}
-				try_chan_entry!(self, chan.get_mut().update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }), 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))
@@ -5224,7 +5190,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 										});
 
 										failed_intercept_forwards.push((htlc_source, forward_info.payment_hash,
-												HTLCFailReason::Reason { failure_code: 0x4000 | 10, data: Vec::new() },
+												HTLCFailReason::from_failure_code(0x4000 | 10),
 												HTLCDestination::InvalidForward { requested_forward_scid: scid },
 										));
 									}
@@ -5244,7 +5210,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 			}
 
 			for (htlc_source, payment_hash, failure_reason, destination) in failed_intercept_forwards.drain(..) {
-				self.fail_htlc_backwards_internal(htlc_source, &payment_hash, failure_reason, destination);
+				self.fail_htlc_backwards_internal(&htlc_source, &payment_hash, &failure_reason, destination);
 			}
 
 			if !new_intercept_events.is_empty() {
@@ -5319,7 +5285,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 			{
 				for failure in pending_failures.drain(..) {
 					let receiver = HTLCDestination::NextHopChannel { node_id: Some(*counterparty_node_id), channel_id: channel_outpoint.to_channel_id() };
-					self.fail_htlc_backwards_internal(failure.0, &failure.1, failure.2, receiver);
+					self.fail_htlc_backwards_internal(&failure.0, &failure.1, &failure.2, receiver);
 				}
 				self.forward_htlcs(&mut [(short_channel_id, channel_outpoint, user_channel_id, pending_forwards)]);
 				self.finalize_claims(finalized_claim_htlcs);
@@ -5479,7 +5445,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 						} else {
 							log_trace!(self.logger, "Failing HTLC with hash {} from our monitor", log_bytes!(htlc_update.payment_hash.0));
 							let receiver = HTLCDestination::NextHopChannel { node_id: counterparty_node_id, channel_id: funding_outpoint.to_channel_id() };
-							self.fail_htlc_backwards_internal(htlc_update.source, &htlc_update.payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() }, receiver);
+							let reason = HTLCFailReason::from_failure_code(0x4000 | 8);
+							self.fail_htlc_backwards_internal(&htlc_update.source, &htlc_update.payment_hash, &reason, receiver);
 						}
 					},
 					MonitorEvent::CommitmentTxConfirmed(funding_outpoint) |
@@ -5715,8 +5682,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// 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.
@@ -5732,8 +5699,8 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// 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)
@@ -5759,7 +5726,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// 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.
 	///
@@ -5769,7 +5736,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	///
 	/// `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
@@ -5780,9 +5747,9 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	///
 	/// 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`].
@@ -5798,7 +5765,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// 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)
 	}
@@ -5876,7 +5843,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		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());
 				}
@@ -5894,6 +5861,12 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 		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()
@@ -6165,9 +6138,8 @@ where
 				if let Ok((channel_ready_opt, mut timed_out_pending_htlcs, announcement_sigs)) = res {
 					for (source, payment_hash) in timed_out_pending_htlcs.drain(..) {
 						let (failure_code, data) = self.get_htlc_inbound_temp_fail_err_and_data(0x1000|14 /* expiry_too_soon */, &channel);
-						timed_out_htlcs.push((source, payment_hash, HTLCFailReason::Reason {
-							failure_code, data,
-						}, HTLCDestination::NextHopChannel { node_id: Some(channel.get_counterparty_node_id()), channel_id: channel.channel_id() }));
+						timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(failure_code, data),
+							HTLCDestination::NextHopChannel { node_id: Some(channel.get_counterparty_node_id()), channel_id: channel.channel_id() }));
 					}
 					if let Some(channel_ready) = channel_ready_opt {
 						send_channel_ready!(self, pending_msg_events, channel, channel_ready);
@@ -6251,24 +6223,46 @@ where
 					// 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 {
-							failure_code: 0x4000 | 15,
-							data: htlc_msat_height_data
-						}, HTLCDestination::FailedPayment { payment_hash: payment_hash.clone() }));
+						timed_out_htlcs.push((HTLCSource::PreviousHopData(htlc.prev_hop.clone()), payment_hash.clone(),
+							HTLCFailReason::reason(0x4000 | 15, htlc_msat_height_data),
+							HTLCDestination::FailedPayment { payment_hash: payment_hash.clone() }));
 						false
 					} else { true }
 				});
 				!htlcs.is_empty() // Only retain this entry if htlcs has at least one entry.
 			});
+
+			let mut intercepted_htlcs = self.pending_intercepted_htlcs.lock().unwrap();
+			intercepted_htlcs.retain(|_, htlc| {
+				if height >= htlc.forward_info.outgoing_cltv_value - HTLC_FAIL_BACK_BUFFER {
+					let prev_hop_data = HTLCSource::PreviousHopData(HTLCPreviousHopData {
+						short_channel_id: htlc.prev_short_channel_id,
+						htlc_id: htlc.prev_htlc_id,
+						incoming_packet_shared_secret: htlc.forward_info.incoming_shared_secret,
+						phantom_shared_secret: None,
+						outpoint: htlc.prev_funding_outpoint,
+					});
+
+					let requested_forward_scid /* intercept scid */ = match htlc.forward_info.routing {
+						PendingHTLCRouting::Forward { short_channel_id, .. } => short_channel_id,
+						_ => unreachable!(),
+					};
+					timed_out_htlcs.push((prev_hop_data, htlc.forward_info.payment_hash,
+							HTLCFailReason::from_failure_code(0x2000 | 2),
+							HTLCDestination::InvalidForward { requested_forward_scid }));
+					log_trace!(self.logger, "Timing out intercepted HTLC with requested forward scid {}", requested_forward_scid);
+					false
+				} else { true }
+			});
 		}
 
 		self.handle_init_event_channel_failures(failed_channels);
 
 		for (source, payment_hash, reason, destination) in timed_out_htlcs.drain(..) {
-			self.fail_htlc_backwards_internal(source, &payment_hash, reason, destination);
+			self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, destination);
 		}
 	}
 
@@ -6979,16 +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)),
@@ -7366,6 +7350,25 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: 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() {
@@ -7446,16 +7449,6 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 				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>()));
@@ -7566,10 +7559,44 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 							}
 						}
 					}
+					for (htlc_source, htlc) in monitor.get_all_current_outbound_htlcs() {
+						if let HTLCSource::PreviousHopData(prev_hop_data) = htlc_source {
+							// 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);
 
@@ -7755,7 +7782,8 @@ impl<'a, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 		for htlc_source in failed_htlcs.drain(..) {
 			let (source, payment_hash, counterparty_node_id, channel_id) = htlc_source;
 			let receiver = HTLCDestination::NextHopChannel { node_id: Some(counterparty_node_id), channel_id };
-			channel_manager.fail_htlc_backwards_internal(source, &payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 8, data: Vec::new() }, receiver);
+			let reason = HTLCFailReason::from_failure_code(0x4000 | 8);
+			channel_manager.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver);
 		}
 
 		//TODO: Broadcast channel update for closed channels, but only after we've made a
@@ -8502,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);