Fix Error type for SimpleDecoder and SimpleEncoder #137

dongcarl · 2018-08-17T15:32:24Z

Separate serialize::Error and network::Error from util::Error
Remove unneeded propagate_err and consume_err

Closes #131

jeandudey

ACK.

apoelstra · 2018-08-20T14:23:37Z

src/internal_macros.rs

@@ -16,15 +16,15 @@ macro_rules! impl_consensus_encoding {
    ($thing:ident, $($field:ident),+) => (
        impl<S: ::network::serialize::SimpleEncoder> ::network::encodable::ConsensusEncodable<S> for $thing {
            #[inline]
-            fn consensus_encode(&self, s: &mut S) -> Result<(), S::Error> {
+            fn consensus_encode(&self, s: &mut S) -> Result<(), serialize::Error> {


Can you use ::network::serialize::Error here? It is annoying to type out the full path but it means that users of the macro don't need mysterious imports (e.g. the one you added to block.rs).

apoelstra · 2018-08-20T14:25:01Z

src/network/message_network.rs

@@ -20,6 +20,7 @@

 use network::constants;
 use network::address::Address;
+use network::serialize;


Can be dropped (see comment on internal_macros.rs)

apoelstra · 2018-08-20T14:25:09Z

src/blockdata/block.rs

@@ -25,7 +25,7 @@ use util::Error::{SpvBadTarget, SpvBadProofOfWork};
 use util::hash::Sha256dHash;
 use util::uint::Uint256;
 use network::encodable::VarInt;
-use network::serialize::BitcoinHash;
+use network::serialize::{self, BitcoinHash};


Can be dropped (see comment on internal_macros.rs)

apoelstra · 2018-08-20T14:28:59Z

src/network/mod.rs

+    /// Not connected to peer
+    SocketNotConnectedToPeer,
+    /// Error propagated from subsystem
+    Detail(String, Box<Error>),


Variant can be dropped (see comments on network/listener.rs which is the only place this was used)

apoelstra · 2018-08-20T14:29:52Z

src/network/listener.rs

@@ -40,7 +40,7 @@ pub trait Listener {
        // Open socket
        let mut ret_sock = Socket::new(self.network());
        if let Err(e) = ret_sock.connect(self.peer(), self.port()) {
-            return Err(util::Error::Detail("listener".to_owned(), Box::new(e)));
+            return Err(util::Error::Network(network::Error::Detail("listener".to_owned(), Box::new(e))));
        }


This whole if should be replaced by ret_sock.connect(self.peer(), self.port())?; because ret_sock.connect already returns a network::Error.

Then you can drop the new self import at the top of this file.

apoelstra · 2018-08-20T14:35:51Z

src/network/serialize.rs

+    /// Parsing error
+    ParseFailed,
+    /// Error propagated from subsystem
+    Detail(String, Box<Error>),


We should drop the Box<Error> from this; the only thing ever packaged in it is Error::ParseFailed. In fact I think we should just make ParseFailed take a String.

In fact it should take a &'static str rather than a String. The only place we dynamically generate a string is in util::hex_bytes, which (a) is only used in unit tests, (b) will be replaced alongside the hashes in the medium-term future.

I believe that the error(&mut self, err: String) -> Error method of RawDecoders also use a dynamic String, I do believe however, that with this overall change, we can eliminate that method and directly return serialize::Errors, does that sound good?

@apoelstra I have replaced those instanced with their own serialize::Error vairants... What do we do with the util::hex_bytes case? Do I do the same?

apoelstra · 2018-08-20T14:37:43Z

src/network/serialize.rs

+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match *self {
+            Error::Io(ref e) => fmt::Display::fmt(e, f),
+            Error::Detail(ref s, ref e) => write!(f, "{}: {}", s, e),


I think every variant (once we merge Detail into ParseFailed) should do something here, and drop the ref x match.

apoelstra · 2018-08-20T14:47:55Z

src/network/message.rs

@@ -174,30 +174,30 @@ impl<S: SimpleEncoder> ConsensusEncodable<S> for RawNetworkMessage {

 // TODO: restriction on D::Error is so that `propagate_err` will work;
 //       is there a more generic way to handle this?


This TODO comment should be deleted.

apoelstra · 2018-08-20T14:50:24Z

src/util/mod.rs

            Error::Secp256k1(ref e) => fmt::Display::fmt(e, f),
+            Error::Serialize(ref e) => fmt::Display::fmt(e, f),


Should add case for Error::Network... and might as well replace ref x in the catchall case with Error::SpvBadTarget | Error::SpvBadProofofWork since there are only the two cases and it'll trigger exhaustiveness checking if we add new ones.

apoelstra · 2018-08-20T14:50:34Z

src/util/mod.rs

            Error::Secp256k1(ref e) => Some(e),
+            Error::Serialize(ref e) => Some(e),
            _ => None


Ditto here.

dongcarl · 2018-08-20T23:59:33Z

@apoelstra Is there any reason we're not using e.fmt(f) in fmt::Display impls?

apoelstra · 2018-08-21T00:01:20Z

@dongcarl I assumed that wouldn't work unless fmt::Display was imported, and I prefer to use the full path fmt::Display everywhere rather than doing so.

In fact if we used e.fmt(f) why would rustc choose fmt::Display::fmt over fmt::Debug::fmt, say?

- Separate serialize::Error and network::Error from util::Error - Remove unneeded propagate_err and consume_err - Change fuzzing code to ignore Err type

- Add serialize::Error::ParseFailed(&'static str) variant for serialization errors without context - Add appropriate variants to replace network::Error::Detail for serialization error with context - Remove error method from SimpleDecoders

dongcarl · 2018-08-21T09:01:32Z

All concerns should have been addressed.

Fix typos

Fix broken CI on master

psbt: satisfy any ToPublicKey key

dongcarl force-pushed the 2018-8-better-errors branch from ab1f4f6 to 09bdda0 Compare August 17, 2018 16:30

jeandudey approved these changes Aug 17, 2018

View reviewed changes

apoelstra reviewed Aug 20, 2018

View reviewed changes

src/util/mod.rs Outdated

Error::Secp256k1(ref e) => Some(e),

Error::Serialize(ref e) => Some(e),

_ => None

Copy link

Member

apoelstra Aug 20, 2018

Choose a reason for hiding this comment

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

Ditto here.

apoelstra added this to the 0.14 milestone Aug 20, 2018

dongcarl added 5 commits August 21, 2018 01:58

Fix Error type for SimpleDecoder and SimpleEncoder

e5b5cbf

- Separate serialize::Error and network::Error from util::Error - Remove unneeded propagate_err and consume_err - Change fuzzing code to ignore Err type

Use full path in macros to to eliminate uses

95303a1

Remove unnecessary network::Error::Detail variant

d12a861

Replace serialize::Error::Detail with variants

0c17294

- Add serialize::Error::ParseFailed(&'static str) variant for serialization errors without context - Add appropriate variants to replace network::Error::Detail for serialization error with context - Remove error method from SimpleDecoders

Replace catch-all match arms with specific ones

bccdd06

dongcarl force-pushed the 2018-8-better-errors branch from 09bdda0 to bccdd06 Compare August 21, 2018 09:00

apoelstra approved these changes Aug 22, 2018

View reviewed changes

apoelstra merged commit a61ad5d into rust-bitcoin:master Aug 22, 2018

apoelstra mentioned this pull request Aug 22, 2018

Some serialization types seem odd. #134

Closed

Davidson-Souza pushed a commit to Davidson-Souza/rust-bitcoin that referenced this pull request Jul 12, 2023

Merge pull request rust-bitcoin#137 from practicalswift/typos

b005089

Fix typos

casey pushed a commit to casey/rust-bitcoin that referenced this pull request Nov 28, 2023

Merge pull request rust-bitcoin#137 from sgeisler/2020-08-fix-ci

fd6c2d9

Fix broken CI on master

yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this pull request Mar 23, 2024

Merge pull request rust-bitcoin#137 from darosior/psbt_satisfy_everyone

c1a54ec

psbt: satisfy any ToPublicKey key

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Error type for SimpleDecoder and SimpleEncoder #137

Fix Error type for SimpleDecoder and SimpleEncoder #137

dongcarl commented Aug 17, 2018 •

edited

jeandudey left a comment

apoelstra Aug 20, 2018

apoelstra Aug 20, 2018

apoelstra Aug 20, 2018

apoelstra Aug 20, 2018

apoelstra Aug 20, 2018

apoelstra Aug 20, 2018

apoelstra Aug 20, 2018

dongcarl Aug 20, 2018

dongcarl Aug 21, 2018

apoelstra Aug 20, 2018

apoelstra Aug 20, 2018

apoelstra Aug 20, 2018

apoelstra Aug 20, 2018

dongcarl commented Aug 20, 2018

apoelstra commented Aug 21, 2018 •

edited

dongcarl commented Aug 21, 2018

		@@ -174,30 +174,30 @@ impl<S: SimpleEncoder> ConsensusEncodable<S> for RawNetworkMessage {

		// TODO: restriction on D::Error is so that `propagate_err` will work;
		// is there a more generic way to handle this?

		Error::Secp256k1(ref e) => fmt::Display::fmt(e, f),
		Error::Serialize(ref e) => fmt::Display::fmt(e, f),

Fix Error type for SimpleDecoder and SimpleEncoder #137

Fix Error type for SimpleDecoder and SimpleEncoder #137

Conversation

dongcarl commented Aug 17, 2018 • edited

jeandudey 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

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

dongcarl commented Aug 20, 2018

apoelstra commented Aug 21, 2018 • edited

dongcarl commented Aug 21, 2018

dongcarl commented Aug 17, 2018 •

edited

apoelstra commented Aug 21, 2018 •

edited