-
Notifications
You must be signed in to change notification settings - Fork 831
Make Inventory and NetworkMessage enums exhaustive #496
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ | |
//! also defines (de)serialization routines for many primitives. | ||
//! | ||
|
||
use std::{io, iter, mem, fmt}; | ||
use std::{fmt, io, iter, mem, str}; | ||
use std::borrow::Cow; | ||
use std::io::Cursor; | ||
|
||
|
@@ -42,21 +42,28 @@ pub const MAX_INV_SIZE: usize = 50_000; | |
#[derive(PartialEq, Eq, Clone, Debug)] | ||
pub struct CommandString(Cow<'static, str>); | ||
|
||
impl fmt::Display for CommandString { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
f.write_str(self.0.as_ref()) | ||
} | ||
} | ||
|
||
impl From<&'static str> for CommandString { | ||
fn from(f: &'static str) -> Self { | ||
CommandString(f.into()) | ||
impl CommandString { | ||
/// Convert from various string types into a [CommandString]. | ||
/// | ||
/// Supported types are: | ||
/// - `&'static str` | ||
/// - `String` | ||
/// | ||
/// Returns an empty error if and only if the string is | ||
/// larger than 12 characters in length. | ||
pub fn try_from<S: Into<Cow<'static, str>>>(s: S) -> Result<CommandString, ()> { | ||
let cow = s.into(); | ||
if cow.as_ref().len() > 12 { | ||
Err(()) | ||
} else { | ||
Ok(CommandString(cow)) | ||
} | ||
} | ||
} | ||
|
||
impl From<String> for CommandString { | ||
fn from(f: String) -> Self { | ||
CommandString(f.into()) | ||
impl fmt::Display for CommandString { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
f.write_str(self.0.as_ref()) | ||
} | ||
} | ||
|
||
|
@@ -74,9 +81,7 @@ impl Encodable for CommandString { | |
) -> Result<usize, encode::Error> { | ||
let mut rawbytes = [0u8; 12]; | ||
let strbytes = self.0.as_bytes(); | ||
if strbytes.len() > 12 { | ||
return Err(encode::Error::NetworkCommandTooLong(self.0.clone().into_owned())); | ||
} | ||
debug_assert!(strbytes.len() <= 12); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure that switching from error type to a panic case is a good practice? However, I understand that with "erlang fail fast" paradigm this can be nice since devs will see that they did something wong — and getting command string from some external data w/o proper filtering is a very bad practice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Afaik this isn't a problem since you can't construct a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you meant "before we have bugs". The fact that if the bugs will happen that may come unseen. But this consideration shouldnt' be a stopper here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bugs in our library should cause panics, not error returns. |
||
rawbytes[..strbytes.len()].clone_from_slice(&strbytes[..]); | ||
rawbytes.consensus_encode(s) | ||
} | ||
|
@@ -104,9 +109,9 @@ pub struct RawNetworkMessage { | |
pub payload: NetworkMessage | ||
} | ||
|
||
#[derive(Clone, PartialEq, Eq, Debug)] | ||
/// A Network message payload. Proper documentation is available on at | ||
/// [Bitcoin Wiki: Protocol Specification](https://en.bitcoin.it/wiki/Protocol_specification) | ||
#[derive(Clone, PartialEq, Eq, Debug)] | ||
pub enum NetworkMessage { | ||
/// `version` | ||
Version(message_network::VersionMessage), | ||
|
@@ -168,10 +173,22 @@ pub enum NetworkMessage { | |
AddrV2(Vec<AddrV2Message>), | ||
/// `sendaddrv2` | ||
SendAddrV2, | ||
|
||
/// Any other message. | ||
Unknown { | ||
/// The command of this message. | ||
command: CommandString, | ||
/// The payload of this message. | ||
payload: Vec<u8>, | ||
} | ||
} | ||
|
||
impl NetworkMessage { | ||
/// Return the message command. This is useful for debug outputs. | ||
/// Return the message command as a static string reference. | ||
/// | ||
/// This returns `"unknown"` for [NetworkMessage::Unknown], | ||
/// regardless of the actual command in the unknown message. | ||
/// Use the [command] method to get the command for unknown messages. | ||
pub fn cmd(&self) -> &'static str { | ||
match *self { | ||
NetworkMessage::Version(_) => "version", | ||
|
@@ -202,17 +219,25 @@ impl NetworkMessage { | |
NetworkMessage::WtxidRelay => "wtxidrelay", | ||
NetworkMessage::AddrV2(_) => "addrv2", | ||
NetworkMessage::SendAddrV2 => "sendaddrv2", | ||
NetworkMessage::Unknown { .. } => "unknown", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this return the internal command and not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well cmd is the one that returns a static string, which is what we use to create CommandString for outgoing messages. Since CommandString is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anther panic does not sound good There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't checked the uses of |
||
} | ||
} | ||
|
||
/// Return the CommandString for the message command. | ||
pub fn command(&self) -> CommandString { | ||
self.cmd().into() | ||
match *self { | ||
NetworkMessage::Unknown { command: ref c, .. } => c.clone(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we return internal command representation, unlike to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah so this one returns CommandString with a |
||
_ => CommandString::try_from(self.cmd()).expect("cmd returns valid commands") | ||
} | ||
} | ||
} | ||
|
||
impl RawNetworkMessage { | ||
/// Return the message command. This is useful for debug outputs. | ||
/// Return the message command as a static string reference. | ||
/// | ||
/// This returns `"unknown"` for [NetworkMessage::Unknown], | ||
/// regardless of the actual command in the unknown message. | ||
/// Use the [command] method to get the command for unknown messages. | ||
pub fn cmd(&self) -> &'static str { | ||
self.payload.cmd() | ||
} | ||
|
@@ -276,8 +301,9 @@ impl Encodable for RawNetworkMessage { | |
| NetworkMessage::SendHeaders | ||
| NetworkMessage::MemPool | ||
| NetworkMessage::GetAddr | ||
| NetworkMessage::WtxidRelay => vec![], | ||
| NetworkMessage::WtxidRelay | ||
| NetworkMessage::SendAddrV2 => vec![], | ||
NetworkMessage::Unknown { payload: ref data, .. } => serialize(data), | ||
}).consensus_encode(&mut s)?; | ||
Ok(len) | ||
} | ||
|
@@ -309,12 +335,11 @@ impl Decodable for HeaderDeserializationWrapper { | |
impl Decodable for RawNetworkMessage { | ||
fn consensus_decode<D: io::Read>(mut d: D) -> Result<Self, encode::Error> { | ||
let magic = Decodable::consensus_decode(&mut d)?; | ||
let cmd = CommandString::consensus_decode(&mut d)?.0; | ||
let cmd = CommandString::consensus_decode(&mut d)?; | ||
let raw_payload = CheckedData::consensus_decode(&mut d)?.0; | ||
let raw_payload_len = raw_payload.len(); | ||
|
||
let mut mem_d = Cursor::new(raw_payload); | ||
let payload = match &cmd[..] { | ||
let payload = match &cmd.0[..] { | ||
"version" => NetworkMessage::Version(Decodable::consensus_decode(&mut mem_d)?), | ||
"verack" => NetworkMessage::Verack, | ||
"addr" => NetworkMessage::Addr(Decodable::consensus_decode(&mut mem_d)?), | ||
|
@@ -345,7 +370,10 @@ impl Decodable for RawNetworkMessage { | |
"wtxidrelay" => NetworkMessage::WtxidRelay, | ||
"addrv2" => NetworkMessage::AddrV2(Decodable::consensus_decode(&mut mem_d)?), | ||
"sendaddrv2" => NetworkMessage::SendAddrV2, | ||
_ => return Err(encode::Error::UnrecognizedNetworkCommand(cmd.into_owned(), 4 + 12 + 4 + 4 + raw_payload_len)), // magic + msg str + payload len + checksum + payload | ||
_ => NetworkMessage::Unknown { | ||
command: cmd, | ||
payload: mem_d.into_inner(), | ||
} | ||
}; | ||
Ok(RawNetworkMessage { | ||
10000 magic: magic, | ||
|
@@ -356,11 +384,10 @@ impl Decodable for RawNetworkMessage { | |
|
||
#[cfg(test)] | ||
mod test { | ||
use std::io; | ||
use std::net::Ipv4Addr; | ||
use super::{RawNetworkMessage, NetworkMessage, CommandString}; | ||
use network::constants::ServiceFlags; | ||
use consensus::encode::{Encodable, deserialize, deserialize_partial, serialize}; | ||
use consensus::encode::{deserialize, deserialize_partial, serialize}; | ||
use hashes::hex::FromHex; | ||
use hashes::sha256d::Hash; | ||
use hashes::Hash as HashTrait; | ||
|
@@ -407,7 +434,7 @@ mod test { | |
NetworkMessage::GetCFCheckpt(GetCFCheckpt{filter_type: 17, stop_hash: hash([25u8; 32]).into()}), | ||
NetworkMessage::CFCheckpt(CFCheckpt{filter_type: 27, stop_hash: hash([77u8; 32]).into(), filter_headers: vec![hash([3u8; 32]).into(), hash([99u8; 32]).into()]}), | ||
NetworkMessage::Alert(vec![45,66,3,2,6,8,9,12,3,130]), | ||
NetworkMessage::Reject(Reject{message: "Test reject".into(), ccode: RejectReason::Duplicate, reason: "Cause".into(), hash: hash([255u8; 32])}), | ||
NetworkMessage::Reject(Reject{message: CommandString::try_from("Test reject").unwrap(), ccode: RejectReason::Duplicate, reason: "Cause".into(), hash: hash([255u8; 32])}), | ||
NetworkMessage::FeeFilter(1000), | ||
NetworkMessage::WtxidRelay, | ||
NetworkMessage::AddrV2(vec![AddrV2Message{ addr: AddrV2::Ipv4(Ipv4Addr::new(127, 0, 0, 1)), port: 0, services: ServiceFlags::NONE, time: 0 }]), | ||
|
@@ -422,21 +449,20 @@ mod test { | |
} | ||
|
||
#[test] | ||
fn serialize_commandstring_test() { | ||
fn commandstring_test() { | ||
// Test converting. | ||
assert_eq!(CommandString::try_from("AndrewAndrew").unwrap().as_ref(), "AndrewAndrew"); | ||
assert!(CommandString::try_from("AndrewAndrewA").is_err()); | ||
|
||
// Test serializing. | ||
let cs = CommandString("Andrew".into()); | ||
assert_eq!(serialize(&cs), vec![0x41u8, 0x6e, 0x64, 0x72, 0x65, 0x77, 0, 0, 0, 0, 0, 0]); | ||
|
||
// Test oversized one. | ||
let mut encoder = io::Cursor::new(vec![]); | ||
assert!(CommandString("AndrewAndrewA".into()).consensus_encode(&mut encoder).is_err()); | ||
} | ||
|
||
#[test] | ||
fn deserialize_commandstring_test() { | ||
// Test deserializing | ||
let cs: Result<CommandString, _> = deserialize(&[0x41u8, 0x6e, 0x64, 0x72, 0x65, 0x77, 0, 0, 0, 0, 0, 0]); | ||
assert!(cs.is_ok()); | ||
assert_eq!(cs.as_ref().unwrap().to_string(), "Andrew".to_owned()); | ||
assert_eq!(cs.unwrap(), "Andrew".into()); | ||
assert_eq!(cs.unwrap(), CommandString::try_from("Andrew").unwrap()); | ||
|
||
let short_cs: Result<CommandString, _> = deserialize(&[0x41u8, 0x6e, 0x64, 0x72, 0x65, 0x77, 0, 0, 0, 0, 0]); | ||
assert!(short_cs.is_err()); | ||
|
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.
Empty errors are bad (they do not implement
std::error::Error
trait since they have not Display), however, with rust 1.29 I think this can work for now)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've done this in the past too, what's the recommended way to handle single-variant errors?
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.
AFAIK s to implement unit structs (i.e. structs w/o any data) and implement Debug+Display for them (I do that with derive and amplify, but yep, for 1.29 you can't do a derive display with amplify)