Skip to content

remove decode_error macro only used once #478

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.

Sign up for GitHub

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
merged 1 commit into from
Feb 8, 2020
Merged

remove decode_error macro only used once #478

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 28 additions & 32 deletions lightning/src/ln/peer_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,37 +517,6 @@ impl<Descriptor: SocketDescriptor, CM: Deref> PeerManager<Descriptor, CM> where
}
}

macro_rules! try_potential_decodeerror {
($thing: expr) => {
match $thing {
Ok(x) => x,
Err(e) => {
match e {
msgs::DecodeError::UnknownVersion => return Err(PeerHandleError{ no_connection_possible: false }),
msgs::DecodeError::UnknownRequiredFeature => {
log_debug!(self, "Got a channel/node announcement with an known required feature flag, you may want to update!");
continue;
},
msgs::DecodeError::InvalidValue => {
log_debug!(self, "Got an invalid value while deserializing message");
return Err(PeerHandleError{ no_connection_possible: false });
},
msgs::DecodeError::ShortRead => {
log_debug!(self, "Deserialization failed due to shortness of message");
return Err(PeerHandleError{ no_connection_possible: false });
},
msgs::DecodeError::ExtraAddressesPerType => {
log_debug!(self, "Error decoding message, ignoring due to lnd spec incompatibility. See https://github.com/lightningnetwork/lnd/issues/1407");
continue;
},
msgs::DecodeError::BadLengthDescriptor => return Err(PeerHandleError{ no_connection_possible: false }),
msgs::DecodeError::Io(_) => return Err(PeerHandleError{ no_connection_possible: false }),
}
}
};
}
}

macro_rules! insert_node_id {
() => {
match peers.node_id_to_descriptor.entry(peer.their_node_id.unwrap()) {
Expand Down Expand Up @@ -613,7 +582,34 @@ impl<Descriptor: SocketDescriptor, CM: Deref> PeerManager<Descriptor, CM> where
peer.pending_read_is_header = true;

let mut reader = ::std::io::Cursor::new(&msg_data[..]);
let message = try_potential_decodeerror!(wire::read(&mut reader));
let message_result = wire::read(&mut reader);
let message = match message_result {
Comment on lines +585 to +586
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify these two lines as:

let message = match wire::read(&mut reader) {

Or use the name message in both assignments to shadow the first variable, as that is idiomatic in Rust.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Although I would argue it might be easier for readers and debuggers to have the match input be a variable as opposed to a method call. What do you think?

Copy link
Contributor

@jkczyz jkczyz Feb 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on that. For the readability standpoint, the let x = match <exp> { Ok(x) => x, Err(e) => { ... } } pattern is common enough to know that the expression evaluates to a Result. And given that, one variable removes redundancy. Regarding debugging, knowing which arm is hit should be enough.

Anyhow, feel free to leave as is if you feel strongly.

Ok(x) => x,
Err(e) => {
match e {
msgs::DecodeError::UnknownVersion => return Err(PeerHandleError { no_connection_possible: false }),
msgs::DecodeError::UnknownRequiredFeature => {
log_debug!(self, "Got a channel/node announcement with an known required feature flag, you may want to update!");
continue;
}
msgs::DecodeError::InvalidValue => {
log_debug!(self, "Got an invalid value while deserializing message");
return Err(PeerHandleError { no_connection_possible: false });
}
msgs::DecodeError::ShortRead => {
log_debug!(self, "Deserialization failed due to shortness of message");
return Err(PeerHandleError { no_connection_possible: false });
}
msgs::DecodeError::ExtraAddressesPerType => {
log_debug!(self, "Error decoding message, ignoring due to lnd spec incompatibility. See https://github.com/lightningnetwork/lnd/issues/1407");
continue;
}
msgs::DecodeError::BadLengthDescriptor => return Err(PeerHandleError { no_connection_possible: false }),
msgs::DecodeError::Io(_) => return Err(PeerHandleError { no_connection_possible: false }),
}
}
};

log_trace!(self, "Received message of type {} from {}", message.type_id(), log_pubkey!(peer.their_node_id.unwrap()));

// Need an Init as first message
Expand Down