Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 480
Fix some onion errors and assert their length is correct#1895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
TheBlueMatt
merged 12 commits into
lightningdevkit:main
from
TheBlueMatt:2022-12-fix-missing-dataDec 6, 2022
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
9a2e26b
Encode HTLC failure packets in a util method on `HTLCFailReason`
TheBlueMatt 6c984bf
Decode `HTLCFailReason`s in a util method on the enum
TheBlueMatt fe3cf29
Fix `impl_writeable_tlv_based_enum` to not require `DecodeError`
TheBlueMatt 4ba8338
Construct from-message `HTLCFailReason` via a constructor method
TheBlueMatt 2485ef3
Move `HTLCFailReason` to `onion_utils`
TheBlueMatt 4011db5
Encapsulate `HTLCFailReason` to not expose struct variants
TheBlueMatt 5e7e3d5
Drop the stale `final_expiry_too_soon` error code
TheBlueMatt be7b82b
Correctly include the `sha256_hash_of_onion` field in BADONION errs
TheBlueMatt 8ec1480
Assert that all onion error messages are correct len in tests
TheBlueMatt 6daf62f
Use `temporary_node_failure` for a phantom HTLC with bogus CLTV
TheBlueMatt 01d299e
Replace `build_first_hop_failure_packet` with `HTLCFailReason`
TheBlueMatt c9fe69f
Correctly handle any `UPDATE` errors to phandom invoices
TheBlueMatt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -49,6 +49,7 @@ use crate::ln::features::InvoiceFeatures; | ||
| use crate::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, RoutePath, RouteParameters}; | ||
| use crate::ln::msgs; | ||
| use crate::ln::onion_utils; | ||
| use crate::ln::onion_utils::HTLCFailReason; | ||
| use crate::ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT}; | ||
| use crate::ln::wire::Encode; | ||
| use crate::chain::keysinterface::{Sign, KeysInterface, KeysManager, Recipient}; | ||
| @@ -276,27 +277,6 @@ impl HTLCSource { | ||
| } | ||
| } | ||
| #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug | ||
| pub(super) enum HTLCFailReason { | ||
| LightningError { | ||
| err: msgs::OnionErrorPacket, | ||
| }, | ||
| Reason { | ||
| failure_code: u16, | ||
| data: Vec<u8>, | ||
| } | ||
| } | ||
| impl HTLCFailReason { | ||
| pub(super) fn reason(failure_code: u16, data: Vec<u8>) -> Self { | ||
| Self::Reason { failure_code, data } | ||
| } | ||
| pub(super) fn from_failure_code(failure_code: u16) -> Self { | ||
| Self::Reason { failure_code, data: Vec::new() } | ||
| } | ||
| } | ||
| struct ReceiveError { | ||
| err_code: u16, | ||
| err_data: Vec<u8>, | ||
| @@ -2062,10 +2042,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F | ||
| // Also, ensure that, in the case of an unknown preimage for the received payment hash, our | ||
| // payment logic has enough time to fail the HTLC backward before our onchain logic triggers a | ||
| // channel closure (see HTLC_FAIL_BACK_BUFFER rationale). | ||
| if (hop_data.outgoing_cltv_value as u64) <= self.best_block.read().unwrap().height() as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 { | ||
| let current_height: u32 = self.best_block.read().unwrap().height(); | ||
| if (hop_data.outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 { | ||
| let mut err_data = Vec::with_capacity(12); | ||
| err_data.extend_from_slice(&amt_msat.to_be_bytes()); | ||
| err_data.extend_from_slice(¤t_height.to_be_bytes()); | ||
valentinewallace marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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", | ||
| }); | ||
| } | ||
| @@ -2173,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), | ||
| })); | ||
| } | ||
| } | ||
| @@ -2238,7 +2222,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F | ||
| // with a short_channel_id of 0. This is important as various things later assume | ||
| // short_channel_id is non-0 in any ::Forward. | ||
| if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing { | ||
| if let Some((err, code, chan_update)) = loop { | ||
| if let Some((err, mut code, chan_update)) = loop { | ||
| let id_option = self.short_to_chan_info.read().unwrap().get(&short_channel_id).cloned(); | ||
| let mut channel_state = self.channel_state.lock().unwrap(); | ||
| let forwarding_id_opt = match id_option { | ||
| @@ -2295,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 | ||
| @@ -2344,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[..]); | ||
| } | ||
| @@ -4049,114 +4042,57 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F | ||
| } else { None }; | ||
| log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0)); | ||
| let path_failure = match &onion_error { | ||
| &HTLCFailReason::LightningError { ref err } => { | ||
| let path_failure = { | ||
| #[cfg(test)] | ||
| let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone()); | ||
| let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_error.decode_onion_failure(&self.secp_ctx, &self.logger, &source); | ||
| #[cfg(not(test))] | ||
| let (network_update, short_channel_id, payment_retryable, _, _) = onion_utils::process_onion_failure(&self.secp_ctx, &self.logger, &source, err.data.clone()); | ||
| if self.payment_is_probe(payment_hash, &payment_id) { | ||
| if !payment_retryable { | ||
| events::Event::ProbeSuccessful { | ||
| payment_id: *payment_id, | ||
| payment_hash: payment_hash.clone(), | ||
| path: path.clone(), | ||
| } | ||
| } else { | ||
| events::Event::ProbeFailed { | ||
| payment_id: *payment_id, | ||
| payment_hash: payment_hash.clone(), | ||
| path: path.clone(), | ||
| short_channel_id, | ||
| } | ||
| } | ||
| } else { | ||
| // TODO: If we decided to blame ourselves (or one of our channels) in | ||
| // process_onion_failure we should close that channel as it implies our | ||
| // next-hop is needlessly blaming us! | ||
| if let Some(scid) = short_channel_id { | ||
| retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid)); | ||
| } | ||
| events::Event::PaymentPathFailed { | ||
| payment_id: Some(*payment_id), | ||
| payment_hash: payment_hash.clone(), | ||
| payment_failed_permanently: !payment_retryable, | ||
| network_update, | ||
| all_paths_failed, | ||
| path: path.clone(), | ||
| short_channel_id, | ||
| retry, | ||
| #[cfg(test)] | ||
| error_code: onion_error_code, | ||
| #[cfg(test)] | ||
| error_data: onion_error_data | ||
| } | ||
| } | ||
| }, | ||
| &HTLCFailReason::Reason { | ||
| #[cfg(test)] | ||
| ref failure_code, | ||
| #[cfg(test)] | ||
| ref data, | ||
| .. } => { | ||
| // we get a fail_malformed_htlc from the first hop | ||
| // TODO: We'd like to generate a NetworkUpdate for temporary | ||
| // failures here, but that would be insufficient as find_route | ||
| // generally ignores its view of our own channels as we provide them via | ||
| // ChannelDetails. | ||
| // TODO: For non-temporary failures, we really should be closing the | ||
| // channel here as we apparently can't relay through them anyway. | ||
| let scid = path.first().unwrap().short_channel_id; | ||
| retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid)); | ||
| if self.payment_is_probe(payment_hash, &payment_id) { | ||
| events::Event::ProbeFailed { | ||
| let (network_update, short_channel_id, payment_retryable, _, _) = onion_error.decode_onion_failure(&self.secp_ctx, &self.logger, &source); | ||
| if self.payment_is_probe(payment_hash, &payment_id) { | ||
| if !payment_retryable { | ||
| events::Event::ProbeSuccessful { | ||
| payment_id: *payment_id, | ||
| payment_hash: payment_hash.clone(), | ||
| path: path.clone(), | ||
| short_channel_id: Some(scid), | ||
| } | ||
| } else { | ||
| events::Event::PaymentPathFailed { | ||
| payment_id: Some(*payment_id), | ||
| events::Event::ProbeFailed { | ||
| payment_id: *payment_id, | ||
| payment_hash: payment_hash.clone(), | ||
| payment_failed_permanently: false, | ||
| network_update: None, | ||
| all_paths_failed, | ||
| path: path.clone(), | ||
| short_channel_id: Some(scid), | ||
| retry, | ||
| #[cfg(test)] | ||
| error_code: Some(*failure_code), | ||
| #[cfg(test)] | ||
| error_data: Some(data.clone()), | ||
| short_channel_id, | ||
| } | ||
| } | ||
| } else { | ||
| // TODO: If we decided to blame ourselves (or one of our channels) in | ||
| // process_onion_failure we should close that channel as it implies our | ||
| // next-hop is needlessly blaming us! | ||
| if let Some(scid) = short_channel_id { | ||
| retry.as_mut().map(|r| r.payment_params.previously_failed_channels.push(scid)); | ||
| } | ||
| events::Event::PaymentPathFailed { | ||
| payment_id: Some(*payment_id), | ||
| payment_hash: payment_hash.clone(), | ||
| payment_failed_permanently: !payment_retryable, | ||
| network_update, | ||
| all_paths_failed, | ||
| path: path.clone(), | ||
| short_channel_id, | ||
| retry, | ||
| #[cfg(test)] | ||
| error_code: onion_error_code, | ||
| #[cfg(test)] | ||
| error_data: onion_error_data | ||
| } | ||
| } | ||
| }; | ||
| let mut pending_events = self.pending_events.lock().unwrap(); | ||
| pending_events.push(path_failure); | ||
| if let Some(ev) = full_failure_ev { pending_events.push(ev); } | ||
| }, | ||
| HTLCSource::PreviousHopData(HTLCPreviousHopData { ref short_channel_id, ref htlc_id, ref incoming_packet_shared_secret, ref phantom_shared_secret, ref outpoint }) => { | ||
| let err_packet = match onion_error { | ||
| HTLCFailReason::Reason { ref failure_code, ref data } => { | ||
| log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards from us with code {}", log_bytes!(payment_hash.0), failure_code); | ||
| if let Some(phantom_ss) = phantom_shared_secret { | ||
| let phantom_packet = onion_utils::build_failure_packet(phantom_ss, *failure_code, &data[..]).encode(); | ||
| let encrypted_phantom_packet = onion_utils::encrypt_failure_packet(phantom_ss, &phantom_packet); | ||
| onion_utils::encrypt_failure_packet(incoming_packet_shared_secret, &encrypted_phantom_packet.data[..]) | ||
| } else { | ||
| let packet = onion_utils::build_failure_packet(incoming_packet_shared_secret, *failure_code, &data[..]).encode(); | ||
| onion_utils::encrypt_failure_packet(incoming_packet_shared_secret, &packet) | ||
| } | ||
| }, | ||
| HTLCFailReason::LightningError { err } => { | ||
| log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards with pre-built LightningError", log_bytes!(payment_hash.0)); | ||
| onion_utils::encrypt_failure_packet(incoming_packet_shared_secret, &err.data) | ||
| } | ||
| }; | ||
| log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards from us with {:?}", log_bytes!(payment_hash.0), onion_error); | ||
| let err_packet = onion_error.get_encrypted_failure_packet(incoming_packet_shared_secret, phantom_shared_secret); | ||
| let mut forward_event = None; | ||
| let mut forward_htlcs = self.forward_htlcs.lock().unwrap(); | ||
| @@ -5084,10 +5020,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, | ||
| @@ -5131,7 +5067,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)) | ||
| } | ||
| @@ -5150,7 +5086,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F | ||
| let chan_err: ChannelError = ChannelError::Close("Got update_fail_malformed_htlc with BADONION not set".to_owned()); | ||
| try_chan_entry!(self, Err(chan_err), chan); | ||
| } | ||
| try_chan_entry!(self, chan.get_mut().update_fail_malformed_htlc(&msg, HTLCFailReason::from_failure_code(msg.failure_code)), chan); | ||
| try_chan_entry!(self, chan.get_mut().update_fail_malformed_htlc(&msg, HTLCFailReason::reason(msg.failure_code, msg.sha256_of_onion.to_vec())), chan); | ||
| Ok(()) | ||
| }, | ||
| hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id)) | ||
| @@ -7033,16 +6969,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)), | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the original more readable 🤷