Merge rust-bitcoin/rust-bitcoin#1003: Improve error Display implementations

57dd673 Do not print error when displaying for std builds (Tobin C. Harding)
b80cfee Bind to error_kind instead of e (Tobin C. Harding)
241ec72 Bind to b instead of e (Tobin C. Harding)
01f481b Bind to s instead of e (Tobin C. Harding)
5c6d369 network: Remove unused error variants (Tobin C. Harding)
e67e97b Put From impl below std::error::Error impl (Tobin C. Harding)
6ca98e5 Remove error TODO (Tobin C. Harding)

Pull request description:

  As part of the ongoing error improvement work and as a direct result of [this comment](rust-bitcoin/rust-bitcoin#987 (comment)) improve the `Display` implementations of all our error types so as to not repeat the source error when printing.

  The first 5 patches are trivial clean ups around the errors. Patch 6 is the real work.

  EDIT: ~CC @Kixunil, have I got the right idea here bro?~ Patch 6 now includes a macro as suggested.

ACKs for top commit:
  Kixunil:
    ACK 57dd673
  apoelstra:
    ACK 57dd673
  sanket1729:
    ACK 57dd673. Did not check if we covered all cases. We need to remember to use `write_err!` instead of `write!` in future.

Tree-SHA512: 1ed26b0cc5f9a0f71684c431cbb9f94404c116c9136be696434c56a2f56fd93cb5406b0955edbd0dc6f8612e77345c93fa70a70650118968cc58e680333a41de

Author: ChallengeDev210

src/blockdata/script.rs

@@ -211,7 +211,7 @@ impl std::error::Error for Error {
             NonMinimalPush
             | EarlyEndOfScript
             | NumericOverflow
-            | BitcoinConsensus(_) // TODO: This should return `Some` but bitcoinconsensus::Error does not implement Error.
+            | BitcoinConsensus(_)
             | UnknownSpentOutput(_)
             | SerializationError => None,
         }

src/blockdata/transaction.rs

@@ -135,8 +135,8 @@ pub enum ParseOutPointError {
 impl fmt::Display for ParseOutPointError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            ParseOutPointError::Txid(ref e) => write!(f, "error parsing TXID: {}", e),
-            ParseOutPointError::Vout(ref e) => write!(f, "error parsing vout: {}", e),
+            ParseOutPointError::Txid(ref e) => write_err!(f, "error parsing TXID"; e),
+            ParseOutPointError::Vout(ref e) => write_err!(f, "error parsing vout"; e),
             ParseOutPointError::Format => write!(f, "OutPoint not in <txid>:<vout> format"),
             ParseOutPointError::TooLong => write!(f, "vout should be at most 10 digits"),
             ParseOutPointError::VoutNotCanonical => write!(f, "no leading zeroes or + allowed in vout part"),

src/consensus/encode.rs

@@ -87,8 +87,8 @@ pub enum Error {
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            Error::Io(ref e) => write!(f, "I/O error: {}", e),
-            Error::Psbt(ref e) => write!(f, "PSBT error: {}", e),
+            Error::Io(ref e) => write_err!(f, "IO error"; e),
+            Error::Psbt(ref e) => write_err!(f, "PSBT error"; e),
             Error::UnexpectedNetworkMagic { expected: ref e, actual: ref a } => write!(f,
                 "unexpected network magic: expected {}, actual {}", e, a),
             Error::OversizedVectorAllocation { requested: ref r, max: ref m } => write!(f,
@@ -97,7 +97,7 @@ impl fmt::Display for Error {
                 "invalid checksum: expected {}, actual {}", e.to_hex(), a.to_hex()),
             Error::NonMinimalVarInt => write!(f, "non-minimal varint"),
             Error::UnknownNetworkMagic(ref m) => write!(f, "unknown network magic: {}", m),
-            Error::ParseFailed(ref e) => write!(f, "parse failed: {}", e),
+            Error::ParseFailed(ref s) => write!(f, "parse failed: {}", s),
             Error::UnsupportedSegwitFlag(ref swflag) => write!(f,
                 "unsupported segwit version: {}", swflag),
         }

src/internal_macros.rs

@@ -574,3 +574,22 @@ macro_rules! user_enum {
         }
     );
 }
+
+/// Formats error. If `std` feature is OFF appends error source (delimited by `: `). We do this
+/// because `e.source()` is only available in std builds, without this macro the error source is
+/// lost for no-std builds.
+macro_rules! write_err {
+    ($writer:expr, $string:literal $(, $args:expr),*; $source:expr) => {
+        {
+            #[cfg(feature = "std")]
+            {
+                let _ = &$source;   // Prevents clippy warnings.
+                write!($writer, $string $(, $args)*)
+            }
+            #[cfg(not(feature = "std"))]
+            {
+                write!($writer, concat!($string, ": {}") $(, $args)*, $source)
+            }
+        }
+    }
+}

src/network/mod.rs

@@ -54,38 +54,31 @@ pub mod stream_reader;
 pub enum Error {
     /// And I/O error
     Io(io::Error),
-    /// Socket mutex was poisoned
-    SocketMutexPoisoned,
-    /// Not connected to peer
-    SocketNotConnectedToPeer,
 }
 
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            Error::Io(ref e) => fmt::Display::fmt(e, f),
-            Error::SocketMutexPoisoned => f.write_str("socket mutex was poisoned"),
-            Error::SocketNotConnectedToPeer => f.write_str("not connected to peer"),
+            Error::Io(ref e) => write_err!(f, "IO error"; e),
         }
     }
 }
 
-#[doc(hidden)]
-impl From<io::Error> for Error {
-    fn from(err: io::Error) -> Self {
-        Error::Io(err)
-    }
-}
-
 #[cfg(feature = "std")]
 #[cfg_attr(docsrs, doc(cfg(feature = "std")))]
 impl std::error::Error for Error {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         use self::Error::*;
 
         match self {
-            Io(e) => Some(e),
-            SocketMutexPoisoned | SocketNotConnectedToPeer => None,
+            Io(e) => Some(e)
         }
     }
 }
+
+#[doc(hidden)]
+impl From<io::Error> for Error {
+    fn from(err: io::Error) -> Self {
+        Error::Io(err)
+    }
+}

src/util/address.rs

@@ -87,12 +87,12 @@ pub enum Error {
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            Error::Base58(_) => write!(f, "base58 address encoding error"),
-            Error::Bech32(_) => write!(f, "bech32 address encoding error"),
+            Error::Base58(ref e) => write_err!(f, "base58 address encoding error"; e),
+            Error::Bech32(ref e) => write_err!(f, "bech32 address encoding error"; e),
             Error::EmptyBech32Payload => write!(f, "the bech32 payload was empty"),
             Error::InvalidBech32Variant { expected, found } => write!(f, "invalid bech32 checksum variant found {:?} when {:?} was expected", found, expected),
             Error::InvalidWitnessVersion(v) => write!(f, "invalid witness script version: {}", v),
-            Error::UnparsableWitnessVersion(_) => write!(f, "incorrect format of a witness version byte"),
+            Error::UnparsableWitnessVersion(ref e) => write_err!(f, "incorrect format of a witness version byte"; e),
             Error::MalformedWitnessVersion => f.write_str("bitcoin script opcode does not match any known witness version, the script is malformed"),
             Error::InvalidWitnessProgramLength(l) => write!(f, "the witness program must be between 2 and 40 bytes in length: length={}", l),
             Error::InvalidSegwitV0ProgramLength(l) => write!(f, "a v0 witness program must be either of length 20 or 32 bytes: length={}", l),

src/util/base58.rs

@@ -61,8 +61,8 @@ impl fmt::Display for Error {
             Error::InvalidExtendedKeyVersion(ref v) => write!(f, "extended key version {:#04x?} is invalid for this base58 type", v),
             Error::InvalidAddressVersion(ref v) => write!(f, "address version {} is invalid for this base58 type", v),
             Error::TooShort(_) => write!(f, "base58ck data not even long enough for a checksum"),
-            Error::Secp256k1(ref e) => fmt::Display::fmt(&e, f),
-            Error::Hex(ref e) => write!(f, "Hexadecimal decoding error: {}", e)
+            Error::Secp256k1(ref e) => write_err!(f, "secp256k1 error while parsing secret key"; e),
+            Error::Hex(ref e) => write_err!(f, "hexadecimal decoding error"; e)
         }
     }
 }

src/util/bip158.rs

@@ -78,7 +78,7 @@ impl Display for Error {
     fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> {
         match *self {
             Error::UtxoMissing(ref coin) => write!(f, "unresolved UTXO {}", coin),
-            Error::Io(ref io) => write!(f, "{}", io)
+            Error::Io(ref e) => write_err!(f, "IO error"; e),
         }
     }
 }

src/util/bip32.rs

@@ -483,14 +483,14 @@ impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
             Error::CannotDeriveFromHardenedKey => f.write_str("cannot derive hardened key from public key"),
-            Error::Secp256k1(ref e) => fmt::Display::fmt(e, f),
+            Error::Secp256k1(ref e) => write_err!(f, "secp256k1 error"; e),
             Error::InvalidChildNumber(ref n) => write!(f, "child number {} is invalid (not within [0, 2^31 - 1])", n),
             Error::InvalidChildNumberFormat => f.write_str("invalid child number format"),
             Error::InvalidDerivationPathFormat => f.write_str("invalid derivation path format"),
             Error::UnknownVersion(ref bytes) => write!(f, "unknown version magic bytes: {:?}", bytes),
             Error::WrongExtendedKeyLength(ref len) => write!(f, "encoded extended key data has wrong length {}", len),
-            Error::Base58(ref err) => write!(f, "base58 encoding error: {}", err),
-            Error::Hex(ref e) => write!(f, "Hexadecimal decoding error: {}", e)
+            Error::Base58(ref e) => write_err!(f, "base58 encoding error"; e),
+            Error::Hex(ref e) => write_err!(f, "Hexadecimal decoding error"; e),
         }
     }
 }

src/util/ecdsa.rs

@@ -104,14 +104,14 @@ pub enum EcdsaSigError {
 impl fmt::Display for EcdsaSigError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            EcdsaSigError::HexEncoding(e) =>
-                write!(f, "EcdsaSig hex encoding error: {}", e),
+            EcdsaSigError::HexEncoding(ref e) =>
+                write_err!(f, "EcdsaSig hex encoding error"; e),
             EcdsaSigError::NonStandardSighashType(hash_ty) =>
                 write!(f, "Non standard signature hash type {}", hash_ty),
             EcdsaSigError::EmptySignature =>
                 write!(f, "Empty ECDSA signature"),
             EcdsaSigError::Secp256k1(ref e) =>
-                write!(f, "Invalid Ecdsa signature: {}", e),
+                write_err!(f, "invalid ECDSA signature"; e),
         }
     }
 }