From 9a2e26b9b7f3c167ec31a45859ed5e05636bf2f6 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2022 18:56:17 +0000 Subject: [PATCH 01/12] Encode HTLC failure packets in a util method on `HTLCFailReason` --- lightning/src/ln/channelmanager.rs | 50 ++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 97dbb4175c5..fa792cc072f 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -287,6 +287,19 @@ pub(super) enum HTLCFailReason { } } +impl core::fmt::Debug for HTLCFailReason { + fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> { + match self { + HTLCFailReason::Reason { ref failure_code, .. } => { + write!(f, "HTLC error code {}", failure_code) + }, + HTLCFailReason::LightningError { .. } => { + write!(f, "pre-built LightningError") + } + } + } +} + impl HTLCFailReason { pub(super) fn reason(failure_code: u16, data: Vec) -> Self { Self::Reason { failure_code, data } @@ -295,6 +308,24 @@ impl HTLCFailReason { pub(super) fn from_failure_code(failure_code: u16) -> Self { Self::Reason { failure_code, data: Vec::new() } } + + fn get_encrypted_failure_packet(&self, incoming_packet_shared_secret: &[u8; 32], phantom_shared_secret: &Option<[u8; 32]>) -> msgs::OnionErrorPacket { + match self { + HTLCFailReason::Reason { ref failure_code, ref data } => { + 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 } => { + onion_utils::encrypt_failure_packet(incoming_packet_shared_secret, &err.data) + } + } + } } struct ReceiveError { @@ -4140,23 +4171,8 @@ impl ChannelManager { - 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(); From 6c984bf50d2d22d979d5b4f7f75b5a6b428dc980 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2022 19:08:53 +0000 Subject: [PATCH 02/12] Decode `HTLCFailReason`s in a util method on the enum --- lightning/src/ln/channelmanager.rs | 124 ++++++++++++----------------- 1 file changed, 51 insertions(+), 73 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index fa792cc072f..3b1c9d54eea 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -326,6 +326,26 @@ impl HTLCFailReason { } } } + + fn decode_onion_failure(&self, secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource) -> (Option, Option, bool, Option, Option>) where L::Target: Logger { + match self { + HTLCFailReason::LightningError { ref err } => { + onion_utils::process_onion_failure(secp_ctx, logger, &htlc_source, err.data.clone()) + }, + HTLCFailReason::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. + // TODO: For non-temporary failures, we really should be closing the + // channel here as we apparently can't relay through them anyway. + 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!(); } + } + } + } } struct ReceiveError { @@ -4080,90 +4100,48 @@ impl ChannelManager { + 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(); From fe3cf29595caa9e783aafaf603e1b55c380e4921 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2022 19:14:43 +0000 Subject: [PATCH 03/12] Fix `impl_writeable_tlv_based_enum` to not require `DecodeError` `impl_writeable_tlv_based_enum` shouldn't be assuming that `DecodeError` is in scope, which we address here. --- lightning/src/util/ser_macros.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 3e1d8a9280d..caed542c19b 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -679,7 +679,7 @@ macro_rules! impl_writeable_tlv_based_enum { Ok($st::$tuple_variant_name(Readable::read(reader)?)) }),* _ => { - Err(DecodeError::UnknownRequiredFeature) + Err($crate::ln::msgs::DecodeError::UnknownRequiredFeature) }, } } From 4ba83381b12449dcdf14b0adc145290f55ebaf21 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2022 19:18:16 +0000 Subject: [PATCH 04/12] Construct from-message `HTLCFailReason` via a constructor method --- lightning/src/ln/channelmanager.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 3b1c9d54eea..eddb5589e4f 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -309,6 +309,10 @@ impl HTLCFailReason { Self::Reason { failure_code, data: Vec::new() } } + pub(super) fn from_msg(msg: &msgs::UpdateFailHTLC) -> Self { + Self::LightningError { err: msg.reason.clone() } + } + fn get_encrypted_failure_packet(&self, incoming_packet_shared_secret: &[u8; 32], phantom_shared_secret: &Option<[u8; 32]>) -> msgs::OnionErrorPacket { match self { HTLCFailReason::Reason { ref failure_code, ref data } => { @@ -5125,7 +5129,7 @@ impl ChannelManager return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id)) } From 2485ef38c344f34efec7adf0413836130dd2a138 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2022 19:20:19 +0000 Subject: [PATCH 05/12] Move `HTLCFailReason` to `onion_utils` Now that it's entirely abstracted, there's no reason for `HTLCFailReason` to be in `channelmanager`, it's really an onion-level abstraction. --- lightning/src/ln/channel.rs | 3 +- lightning/src/ln/channelmanager.rs | 87 +---------------------------- lightning/src/ln/onion_utils.rs | 88 ++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 87 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 5cc4f4a364b..9f47bacc74d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -27,9 +27,10 @@ use crate::ln::features::{ChannelTypeFeatures, InitFeatures}; 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}; diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index eddb5589e4f..7fed7b2f427 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -49,6 +49,7 @@ use crate::ln::features::InvoiceFeatures; use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RoutePath, RouteParameters}; use crate::ln::msgs; use crate::ln::onion_utils; +use crate::ln::onion_utils::HTLCFailReason; use crate::ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT}; use crate::ln::wire::Encode; use crate::chain::keysinterface::{Sign, KeysInterface, KeysManager, Recipient}; @@ -276,82 +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, - } -} - -impl core::fmt::Debug for HTLCFailReason { - fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> { - match self { - HTLCFailReason::Reason { ref failure_code, .. } => { - write!(f, "HTLC error code {}", failure_code) - }, - HTLCFailReason::LightningError { .. } => { - write!(f, "pre-built LightningError") - } - } - } -} - -impl HTLCFailReason { - pub(super) fn reason(failure_code: u16, data: Vec) -> Self { - Self::Reason { failure_code, data } - } - - pub(super) fn from_failure_code(failure_code: u16) -> Self { - Self::Reason { failure_code, data: Vec::new() } - } - - pub(super) fn from_msg(msg: &msgs::UpdateFailHTLC) -> Self { - Self::LightningError { err: msg.reason.clone() } - } - - fn get_encrypted_failure_packet(&self, incoming_packet_shared_secret: &[u8; 32], phantom_shared_secret: &Option<[u8; 32]>) -> msgs::OnionErrorPacket { - match self { - HTLCFailReason::Reason { ref failure_code, ref data } => { - 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 } => { - onion_utils::encrypt_failure_packet(incoming_packet_shared_secret, &err.data) - } - } - } - - fn decode_onion_failure(&self, secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource) -> (Option, Option, bool, Option, Option>) where L::Target: Logger { - match self { - HTLCFailReason::LightningError { ref err } => { - onion_utils::process_onion_failure(secp_ctx, logger, &htlc_source, err.data.clone()) - }, - HTLCFailReason::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. - // TODO: For non-temporary failures, we really should be closing the - // channel here as we apparently can't relay through them anyway. - 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!(); } - } - } - } -} - struct ReceiveError { err_code: u16, err_data: Vec, @@ -7031,16 +6956,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)), diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 23dc556cfac..a44e9b37d04 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -592,6 +592,94 @@ pub(super) fn process_onion_failure(secp_ctx: & } else { unreachable!(); } } +#[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, + } +} + +impl core::fmt::Debug for HTLCFailReason { + fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> { + match self { + HTLCFailReason::Reason { ref failure_code, .. } => { + write!(f, "HTLC error code {}", failure_code) + }, + HTLCFailReason::LightningError { .. } => { + write!(f, "pre-built LightningError") + } + } + } +} + +impl_writeable_tlv_based_enum!(HTLCFailReason, + (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) -> Self { + Self::Reason { failure_code, data } + } + + pub(super) fn from_failure_code(failure_code: u16) -> Self { + Self::Reason { failure_code, data: Vec::new() } + } + + pub(super) fn from_msg(msg: &msgs::UpdateFailHTLC) -> Self { + Self::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 { + HTLCFailReason::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) + } + }, + HTLCFailReason::LightningError { err } => { + encrypt_failure_packet(incoming_packet_shared_secret, &err.data) + } + } + } + + pub(super) fn decode_onion_failure( + &self, secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource + ) -> (Option, Option, bool, Option, Option>) + where L::Target: Logger { + match self { + HTLCFailReason::LightningError { ref err } => { + process_onion_failure(secp_ctx, logger, &htlc_source, err.data.clone()) + }, + HTLCFailReason::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]> { From 4011db57f7e0023da0fe041cfacf09fc32b18fcd Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2022 19:28:32 +0000 Subject: [PATCH 06/12] Encapsulate `HTLCFailReason` to not expose struct variants Now that `HTLCFailReason` is opaque and in `onion_utils`, we should encapsulate it so that `ChannelManager` can no longer directly access its inner fields. --- lightning/src/ln/onion_utils.rs | 44 ++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index a44e9b37d04..c12c63d5233 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -15,7 +15,7 @@ use crate::routing::gossip::NetworkUpdate; 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}; @@ -593,7 +593,10 @@ pub(super) fn process_onion_failure(secp_ctx: & } #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug -pub(super) enum HTLCFailReason { +pub(super) struct HTLCFailReason(HTLCFailReasonRepr); + +#[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug +enum HTLCFailReasonRepr { LightningError { err: msgs::OnionErrorPacket, }, @@ -605,18 +608,29 @@ pub(super) enum HTLCFailReason { impl core::fmt::Debug for HTLCFailReason { fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> { - match self { - HTLCFailReason::Reason { ref failure_code, .. } => { + match self.0 { + HTLCFailReasonRepr::Reason { ref failure_code, .. } => { write!(f, "HTLC error code {}", failure_code) }, - HTLCFailReason::LightningError { .. } => { + HTLCFailReasonRepr::LightningError { .. } => { write!(f, "pre-built LightningError") } } } } -impl_writeable_tlv_based_enum!(HTLCFailReason, +impl Writeable for HTLCFailReason { + fn write(&self, writer: &mut W) -> Result<(), crate::io::Error> { + self.0.write(writer) + } +} +impl Readable for HTLCFailReason { + fn read(reader: &mut R) -> Result { + Ok(Self(Readable::read(reader)?)) + } +} + +impl_writeable_tlv_based_enum!(HTLCFailReasonRepr, (0, LightningError) => { (0, err, required), }, @@ -628,21 +642,21 @@ impl_writeable_tlv_based_enum!(HTLCFailReason, impl HTLCFailReason { pub(super) fn reason(failure_code: u16, data: Vec) -> Self { - Self::Reason { failure_code, data } + Self(HTLCFailReasonRepr::Reason { failure_code, data }) } pub(super) fn from_failure_code(failure_code: u16) -> Self { - Self::Reason { failure_code, data: Vec::new() } + Self(HTLCFailReasonRepr::Reason { failure_code, data: Vec::new() }) } pub(super) fn from_msg(msg: &msgs::UpdateFailHTLC) -> Self { - Self::LightningError { err: msg.reason.clone() } + 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 { - HTLCFailReason::Reason { ref failure_code, ref data } => { + 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); @@ -652,7 +666,7 @@ impl HTLCFailReason { encrypt_failure_packet(incoming_packet_shared_secret, &packet) } }, - HTLCFailReason::LightningError { err } => { + HTLCFailReasonRepr::LightningError { ref err } => { encrypt_failure_packet(incoming_packet_shared_secret, &err.data) } } @@ -662,11 +676,11 @@ impl HTLCFailReason { &self, secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource ) -> (Option, Option, bool, Option, Option>) where L::Target: Logger { - match self { - HTLCFailReason::LightningError { ref err } => { + match self.0 { + HTLCFailReasonRepr::LightningError { ref err } => { process_onion_failure(secp_ctx, logger, &htlc_source, err.data.clone()) }, - HTLCFailReason::Reason { ref failure_code, ref data, .. } => { + 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 From 5e7e3d57bf00c75a2be2b5475d39a8a4f6ec1453 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2022 20:25:33 +0000 Subject: [PATCH 07/12] Drop the stale `final_expiry_too_soon` error code This replaces `final_expiry_too_soon` with `incorrect_or_unknown_payment` as was done in https://github.com/lightning/bolts/pull/608. Note that the rationale for this (that it may expose whether you are the final recipient for the payment or not) does not currently apply to us - we don't apply different final CLTV values to different payments. However, we might in the future, and this will make us slightly more consistent with other nodes. --- lightning/src/ln/channelmanager.rs | 9 ++++++--- lightning/src/ln/onion_route_tests.rs | 11 +++++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 7fed7b2f427..36886645ab8 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2042,10 +2042,13 @@ impl ChannelManager Date: Thu, 1 Dec 2022 20:30:45 +0000 Subject: [PATCH 08/12] Correctly include the `sha256_hash_of_onion` field in BADONION errs The spec mandates that we copy the `sha256_hash_of_onion` field from the `UpdateFailMalformedHTLC` message into the error message we send back to the sender, however we simply ignored it. Here we copy it into the message correctly. --- lightning/src/ln/channelmanager.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 36886645ab8..9ec1c43cfa3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -5076,7 +5076,7 @@ impl ChannelManager return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id)) From 8ec148072437b3a64ddddee89088a77db271312e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2022 20:31:52 +0000 Subject: [PATCH 09/12] Assert that all onion error messages are correct len in tests When we're constructing an HTLCFailReason, we should check that we set the data to at least the correct length for the given failure code, which we do here. --- lightning/src/ln/onion_utils.rs | 40 ++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index c12c63d5233..5f55714a624 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -642,11 +642,49 @@ impl_writeable_tlv_based_enum!(HTLCFailReasonRepr, impl HTLCFailReason { pub(super) fn reason(failure_code: u16, data: Vec) -> 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(HTLCFailReasonRepr::Reason { failure_code, data: Vec::new() }) + Self::reason(failure_code, Vec::new()) } pub(super) fn from_msg(msg: &msgs::UpdateFailHTLC) -> Self { From 6daf62fea3399061b2bd55da7e7e129186e4bd09 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2022 23:39:28 +0000 Subject: [PATCH 10/12] Use `temporary_node_failure` for a phantom HTLC with bogus CLTV When we receive a phantom HTLC with a bogus/modified CLTV, we should fail back with `incorrect_cltv_expiry`, but that requires a `channel_update`, which we cannot generate for a phantom HTLC which has no corresponding channel. Thus, instead, we have to fall back to `incorrect_cltv_expiry`. Fixes #1879 --- lightning/src/ln/channelmanager.rs | 7 +++-- lightning/src/ln/onion_route_tests.rs | 41 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 9ec1c43cfa3..d6f9d94f444 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2278,10 +2278,13 @@ impl ChannelManager Date: Thu, 1 Dec 2022 23:30:04 +0000 Subject: [PATCH 11/12] Replace `build_first_hop_failure_packet` with `HTLCFailReason` This ensures we always hit our new debug assertions while building failure packets in the immediately-fail pipeline while processing an inbound HTLC. --- lightning/src/ln/channelmanager.rs | 9 +++++---- lightning/src/ln/onion_utils.rs | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index d6f9d94f444..e0a54841e5d 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2156,7 +2156,8 @@ impl ChannelManager ChannelManager { 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, diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 5f55714a624..76a39c69801 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -382,7 +382,7 @@ pub(super) fn build_failure_packet(shared_secret: &[u8], failure_type: u16, fail 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()[..]) From c9fe69fa5fe2ce50dde6e5b59a16d5783a29a9d7 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 2 Dec 2022 21:12:47 +0000 Subject: [PATCH 12/12] Correctly handle any `UPDATE` errors to phandom invoices If we try to send any onion error with the `UPDATE` flag in response to a phantom receipt, we should always swap it for something generic that doesn't require a `channel_update` in it. Here we use `temporary_node_failure`. Test provided by Valentine Wallace --- lightning/src/ln/channelmanager.rs | 8 +++++- lightning/src/ln/onion_route_tests.rs | 39 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index e0a54841e5d..6e5df15908a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2222,7 +2222,7 @@ impl ChannelManager ChannelManager