Change the signature of consensus_encode to return io::Error's #494

stevenroose · 2020-10-09T15:01:04Z

This is instead of encode::Errors because the encoders should
not be allowed to return errors that don't originate in the writer
they are writing into.

This is a part of the method definition that has been relied upon for a
while already.

apoelstra · 2020-10-09T15:03:51Z

Nice!

I think this should get its own version bump, separate from other changes, because it requires some invasive but mechanical changes for downstream users.

dr-orlovsky

Pls see one command with consideration of doing FromStr impl as well, otherwise utACK

dr-orlovsky · 2020-10-09T15:08:43Z

src/network/message.rs

+    ///
+    /// Returns an empty error if and only if the string is
+    /// larger than 12 characters in length.
+    pub fn from_str<S: Into<Cow<'static, str>>>(s: S) -> Result<CommandString, ()> {


Some methods rely on FromStr implementation to do a conversion; for instance if use clap for parsing commands from command-line parameters (which may be a case for cli tools operating with bitcoin P2P network). So I propose also to add impl FromStr for CommandString and from it call this method directly

stevenroose · 2020-10-11T20:16:01Z

Nice!

I think this should get its own version bump, separate from other changes, because it requires some invasive but mechanical changes for downstream users.

Can it not just go with the next breaking version bump? I mean it doesn't really matter if it has it's own bump, right? At some point people will have to do it if they want to keep updating.

dr-orlovsky · 2020-10-11T21:58:57Z

src/network/message.rs

    }
 }

+#[deprecated(since = "0.26.0", note = "use CommandString::from")]


Trying to understand why this and above are depricated. The message will be really surprising in practice: instead of something.from() users are asked to use something::<CommandString>.from(), but even that is not clear from the message... Because in most cases method is called on the object, without additional qualifiers, and it will be up to the compiler to decide wich implementation to call.

It seams reasonable just to remove this methods, and nothing bad will happen.

Yeah I removed it. I pushed a new commit cleaning up the CommandString API.

elichai · 2020-10-13T09:38:54Z

src/network/message.rs

+        if f.len() > 12 {
+            CommandString(f[0..12].into())


I don't like this, I'd rather remove the From impl or panic here then do unexpected things

oh I see you deprecated it, hmm idk what's the right thing to do
also, deprecating a trait impl doesn't do anything UX wise:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c67bc53c2372c636208adbfab79fa02e
rust-lang/rust#39935

Hmm, that seems to be a cargo bug. I would prefer not to panic, so perhaps just removing the method is the best option.

K I pushed a new commit that cleans up the CommandString API. I thin it's such a infrequently used type that all the fancy construction methods are not needed. So I removed all of them.

elichai · 2020-10-13T09:39:33Z

src/network/message.rs

+        // Don't use CommandString::from to avoid allocating when
+        // it's not needed.


You're saying don't call CommandString::from to avoid allocating but you then call to_owned() which allocates?

Hmm, that might have been related to an earlier version of the impl or wrongly formulated.. I don't remember. I think we can't use CommandString::from because &'a str doesn't implement Into<Cow<'static, str>>, but I'm not sure, let me try.

Ah k so

72 | Ok(CommandString(s.into())) | ^^^^^^^^ = note: expected `std::borrow::Cow<'static, _>` found `std::borrow::Cow<'_, _>`

I was right, and I don't know what to do here :| I'd be in favor of dropping FromStr as well for this type and just have the basic constructor which has Into<Cow<..>>.

stevenroose · 2020-10-25T13:48:12Z

Rebased and added commit to clean up the CommandString API.

Kixunil · 2020-10-25T19:13:14Z

Just noticed this randomly and I have to say I'm not a huge fan of io::Error. I wrote my reasons on reddit recently.

I don't know the exact context here, so feel free to ignore me if it's unrelated.

stevenroose · 2020-10-31T19:28:17Z

Just noticed this randomly and I have to say I'm not a huge fan of io::Error. I wrote my reasons on reddit recently.

Yeah I don't think that's quite related. Like currently it's about the difference between returning an io::Error or an encoding::Error::Io(io::Error). The latter case is confusing because encode::Error has other variants that the method should never return.

Also, this PR is really neutral about io::Error itself. It just happens to be the error that io::Write and io::Read return. Which are the traits used in the encode::Encodable and encode::Decodable traits.

src/network/message.rs

elichai · 2020-11-01T09:24:23Z

src/network/message.rs

+    ///
+    /// Returns an empty error if and only if the string is
+    /// larger than 12 characters in length.
+    pub fn from<S: Into<Cow<'static, str>>>(s: S) -> Result<CommandString, ()> {


nit, Calling the function from outside the From trait is somewhat weird

Yeah I couldn't think of a better name. I thought of create but it's quite conventional to use from or from_xxx for converter methods. I also considered just new. Open to opinions, though, don't feel strongly about this. I just thought from was good enough.

Open to opinions

Personally, new would probably be the first thing I would look for when trying to construct one of these.

Another thing to consider might be to name it try_from given that you are returning a Result.

According to rust API guidelines we should use with_str as a function name: https://rust-lang.github.io/api-guidelines/naming.html#casing-conforms-to-rfc-430-c-case

I'm not sure, the guildlines say with_more_details, however in this case intput is not "more details", it's literally the source of informtion. My understanding is that with_more_details is used for special cases (with_capacity is an obvious one)

The function header also kinda leaks internal representation but I guess it's fine?
Implementing TryFrom would be best, but AFAIR it's not in supported MSRV.

stevenroose · 2020-11-01T12:07:05Z

Rebased.

elichai

ACK

apoelstra · 2020-11-05T17:51:34Z

Nice!
I think this should get its own version bump, separate from other changes, because it requires some invasive but mechanical changes for downstream users.

Can it not just go with the next breaking version bump? I mean it doesn't really matter if it has it's own bump, right? At some point people will have to do it if they want to keep updating.

Yes, but if we do it in the same bump as a bunch of other stuff, then they have to do these changes in a single commit alongside a bunch of other stuff.

dr-orlovsky

Seems like there a few things potentially reducing stability of the software that I missed in my previous review

dr-orlovsky · 2020-12-15T16:17:18Z

src/network/message.rs

+    ///
+    /// Returns an empty error if and only if the string is
+    /// larger than 12 characters in length.
+    pub fn from<S: Into<Cow<'static, str>>>(s: S) -> Result<CommandString, ()> {


According to rust API guidelines we should use with_str as a function name: https://rust-lang.github.io/api-guidelines/naming.html#casing-conforms-to-rfc-430-c-case

dr-orlovsky · 2020-12-15T16:19:46Z

src/network/message.rs

-        if strbytes.len() > 12 {
-            return Err(encode::Error::UnrecognizedNetworkCommand(self.0.clone().into_owned()));
-        }


This change will lead to a panic if the length of the CommandString is larger than 12. Why not to use ENAMETOOLONG io::Error equivalent?

I don't think so. The PR removes the uncheck instantiation path for CommandString through From and replaces it with CommandString::new, which checks that the string is <=12 bytes long.

dr-orlovsky · 2020-12-15T16:21:00Z

src/network/message.rs

@@ -202,7 +206,7 @@ impl NetworkMessage {

    /// Return the CommandString for the message command.
    pub fn command(&self) -> CommandString {
-        self.cmd().into()
+        CommandString::from(self.cmd()).expect("valid cmd string")


This opens an attack vector to the rust-bitcoin-based software by just sending them invalid command string causing a crash

I don't think so, the return value of .cmd() is a static string defines by the enum variant, not attacker controlled.

I think the message in expect should say invalid. The complete message sounding like "program panicked because cmd string is valid" is strange.

sgeisler

This seems to be on hold for now but I really like the changes, does away with a lot of weirdness. Will do a more thorough review once it's merge-ready.

sgeisler · 2020-12-15T18:36:45Z

src/network/message.rs

@@ -202,7 +206,7 @@ impl NetworkMessage {

    /// Return the CommandString for the message command.
    pub fn command(&self) -> CommandString {
-        self.cmd().into()
+        CommandString::from(self.cmd()).expect("valid cmd string")


I don't think so, the return value of .cmd() is a static string defines by the enum variant, not attacker controlled.

sgeisler · 2020-12-15T18:39:55Z

src/network/message.rs

-        if strbytes.len() > 12 {
-            return Err(encode::Error::UnrecognizedNetworkCommand(self.0.clone().into_owned()));
-        }


I don't think so. The PR removes the uncheck instantiation path for CommandString through From and replaces it with CommandString::new, which checks that the string is <=12 bytes long.

apoelstra · 2021-01-11T18:32:44Z

Setting to 0.26 milestone. Originally we had planned to punt to 0.27 because it is an API-breaking-only change, but I this we should prioritize it since our code currently unwraps these objects whenever we encode into hash engines, for example, so it would be better to eliminate theoretical panics.

On the other hand if this winds up blocking 0.26 then I'll probably move it to 0.27 :)

Needs rebase.

This is instead of encode::Errors because the encoders should not be allowed to return errors that don't originate in the writer they are writing into. This is a part of the method definition that has been relied upon for a while already.

apoelstra

ack 61918df

JeremyRubin · 2021-01-12T19:31:04Z

post-ack

apoelstra added the API break This PR requires a version bump for the next release label Oct 9, 2020

dr-orlovsky reviewed Oct 9, 2020

View reviewed changes

stevenroose mentioned this pull request Oct 10, 2020

Make Inventory and NetworkMessage enums exhaustive #496

Merged

stevenroose force-pushed the encode-io-err branch from 108528f to 446553d Compare October 11, 2020 20:25

dr-orlovsky reviewed Oct 11, 2020

View reviewed changes

elichai reviewed Oct 13, 2020

View reviewed changes

stevenroose force-pushed the encode-io-err branch from 464196c to 9c20cf8 Compare October 25, 2020 13:47

elichai reviewed Nov 1, 2020

View reviewed changes

src/network/message.rs Outdated Show resolved Hide resolved

elichai reviewed Nov 1, 2020

View reviewed changes

stevenroose force-pushed the encode-io-err branch from 9c20cf8 to 876651b Compare November 1, 2020 12:06

stevenroose force-pushed the encode-io-err branch 2 times, most recently from 29ab439 to 0b54429 Compare November 1, 2020 12:46

elichai previously approved these changes Nov 1, 2020

View reviewed changes

apoelstra mentioned this pull request Nov 27, 2020

Rearrange all the modules #525

Closed

stevenroose mentioned this pull request Dec 15, 2020

Flush unrecognized network messages from the read buffer #529

Merged

dr-orlovsky suggested changes Dec 15, 2020

View reviewed changes

sgeisler reviewed Dec 15, 2020

View reviewed changes

apoelstra added this to the 0.26.0 milestone Jan 11, 2021

dr-orlovsky mentioned this pull request Jan 12, 2021

bump version to 0.26 #533

Merged

stevenroose dismissed elichai’s stale review via 61918df January 12, 2021 17:42

stevenroose force-pushed the encode-io-err branch from 0b54429 to 61918df Compare January 12, 2021 17:42

apoelstra approved these changes Jan 12, 2021

View reviewed changes

elichai approved these changes Jan 12, 2021

View reviewed changes

elichai merged commit d2344d3 into rust-bitcoin:master Jan 12, 2021

		// Don't use CommandString::from to avoid allocating when
		// it's not needed.

Change the signature of consensus_encode to return io::Error's #494

Change the signature of consensus_encode to return io::Error's #494

Conversation

stevenroose commented Oct 9, 2020

apoelstra commented Oct 9, 2020

dr-orlovsky left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Oct 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose Oct 25, 2020 • edited Loading

Choose a reason for hiding this comment

stevenroose commented Oct 25, 2020

Kixunil commented Oct 25, 2020

stevenroose commented Oct 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Nov 1, 2020

elichai left a comment

Choose a reason for hiding this comment

apoelstra commented Nov 5, 2020

dr-orlovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgeisler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Jan 11, 2021

apoelstra left a comment

Choose a reason for hiding this comment

JeremyRubin commented Jan 12, 2021

dr-orlovsky left a comment •

edited

Loading

stevenroose Oct 25, 2020 •

edited

Loading