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

57dd6739c3 Do not print error when displaying for std builds (Tobin C. Harding)
b80cfeed85 Bind to error_kind instead of e (Tobin C. Harding)
241ec72497 Bind to b instead of e (Tobin C. Harding)
01f481bf5c Bind to s instead of e (Tobin C. Harding)
5c6d369289 network: Remove unused error variants (Tobin C. Harding)
e67e97bb37 Put From impl below std::error::Error impl (Tobin C. Harding)
6ca98e5275 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](https://github.com/rust-bitcoin/rust-bitcoin/pull/987#issuecomment-1135563287) 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 57dd6739c3
  apoelstra:
    ACK 57dd6739c3
  sanket1729:
    ACK 57dd6739c3. Did not check if we covered all cases. We need to remember to use `write_err!` instead of `write!` in future.

Tree-SHA512: 1ed26b0cc5f9a0f71684c431cbb9f94404c116c9136be696434c56a2f56fd93cb5406b0955edbd0dc6f8612e77345c93fa70a70650118968cc58e680333a41de

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,29 +54,16 @@ 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 {
@@ -84,8 +71,14 @@ impl std::error::Error for Error {
         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),
         }
     }
 }

src/util/key.rs

@@ -46,10 +46,10 @@ pub enum Error {
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            Error::Base58(ref e) => write!(f, "Key base58 error: {}", e),
-            Error::Secp256k1(ref e) => write!(f, "Key secp256k1 error: {}", e),
-            Error::InvalidKeyPrefix(ref e) => write!(f, "Key prefix invalid: {}", e),
-            Error::Hex(ref e) => write!(f, "Key hex decoding error: {}", e)
+            Error::Base58(ref e) => write_err!(f, "key base58 error"; e),
+            Error::Secp256k1(ref e) => write_err!(f, "key secp256k1 error"; e),
+            Error::InvalidKeyPrefix(ref b) => write!(f, "key prefix invalid: {}", b),
+            Error::Hex(ref e) => write_err!(f, "key hex decoding error"; e)
         }
     }
 }

src/util/misc.rs

@@ -64,7 +64,7 @@ mod message_signing {
         fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
             match *self {
                 MessageSignatureError::InvalidLength => write!(f, "length not 65 bytes"),
-                MessageSignatureError::InvalidEncoding(ref e) => write!(f, "invalid encoding: {}", e),
+                MessageSignatureError::InvalidEncoding(ref e) => write_err!(f, "invalid encoding"; e),
                 MessageSignatureError::InvalidBase64 => write!(f, "invalid base64"),
                 MessageSignatureError::UnsupportedAddressType(ref address_type) => write!(f, "unsupported address type: {}", address_type),
             }

src/util/mod.rs

@@ -82,8 +82,8 @@ pub enum Error {
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            Error::Encode(ref e) => fmt::Display::fmt(e, f),
-            Error::Network(ref e) => fmt::Display::fmt(e, f),
+            Error::Encode(ref e) => write_err!(f, "encoding error"; e),
+            Error::Network(ref e) => write_err!(f, "network error"; e),
             Error::BlockBadProofOfWork => f.write_str("block target correct but not attained"),
             Error::BlockBadTarget => f.write_str("block target incorrect"),
         }

src/util/psbt/error.rs

@@ -104,7 +104,7 @@ impl fmt::Display for Error {
             Error::NoMorePairs => f.write_str("no more key-value pairs for this psbt map"),
             Error::UnexpectedUnsignedTx { expected: ref e, actual: ref a } => write!(f, "different unsigned transaction: expected {}, actual {}", e.txid(), a.txid()),
             Error::NonStandardSighashType(ref sht) => write!(f, "non-standard sighash type: {}", sht),
-            Error::HashParseError(e) => write!(f, "Hash Parse Error: {}", e),
+            Error::HashParseError(ref e) => write_err!(f, "hash parse error"; e),
             Error::InvalidPreimageHashPair{ref preimage, ref hash, ref hash_type} => {
                 // directly using debug forms of psbthash enums
                 write!(f, "Preimage {:?} does not match {:?} hash {:?}", preimage, hash_type, hash )

src/util/psbt/mod.rs

@@ -234,9 +234,11 @@ mod display_from_str {
 
     impl Display for PsbtParseError {
         fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-            match self {
-                PsbtParseError::PsbtEncoding(err) => Display::fmt(err, f),
-                PsbtParseError::Base64Encoding(err) => Display::fmt(err, f),
+            use self::PsbtParseError::*;
+
+            match *self {
+                PsbtEncoding(ref e) => write_err!(f, "error in internal PSBT data structure"; e),
+                Base64Encoding(ref e) => write_err!(f, "error in PSBT base64 encoding"; e),
             }
         }
     }
@@ -926,7 +928,10 @@ mod tests {
             let err = hex_psbt!("70736274ff01007d020000000127744ababf3027fe0d6cf23a96eee2efb188ef52301954585883e69b6624b2420000000000ffffffff02887b0100000000001600142382871c7e8421a00093f754d91281e675874b9f606b042a010000002251205a2c2cf5b52cf31f83ad2e8da63ff03183ecd8f609c7510ae8a48e03910a0757000000000001012b00f2052a010000002251205a2c2cf5b52cf31f83ad2e8da63ff03183ecd8f609c7510ae8a48e03910a07570000220702fe349064c98d6e2a853fa3c9b12bd8b304a19c195c60efa7ee2393046d3fa2321900772b2da7560000800100008000000080010000000000000000").unwrap_err();
             assert_eq!(err.to_string(), "parse failed: Invalid xonly public key");
             let err = hex_psbt!("70736274ff01005e02000000019bd48765230bf9a72e662001f972556e54f0c6f97feb56bcb5600d817f6995260100000000ffffffff0148e6052a01000000225120030da4fce4f7db28c2cb2951631e003713856597fe963882cb500e68112cca63000000000001012b00f2052a01000000225120c2247efbfd92ac47f6f40b8d42d169175a19fa9fa10e4a25d7f35eb4dd85b6924214022cb13ac68248de806aa6a3659cf3c03eb6821d09c8114a4e868febde865bb6d2cd970e15f53fc0c82f950fd560ffa919b76172be017368a89913af074f400b094089756aa3739ccc689ec0fcf3a360be32cc0b59b16e93a1e8bb4605726b2ca7a3ff706c4176649632b2cc68e1f912b8a578e3719ce7710885c7a966f49bcd43cb0000").unwrap_err();
-            assert_eq!(err.to_string(), "PSBT error: Hash Parse Error: bad slice length 33 (expected 32)");
+            #[cfg(feature = "std")]
+            assert_eq!(err.to_string(), "PSBT error");
+            #[cfg(not(feature = "std"))]
+            assert_eq!(err.to_string(), "PSBT error: hash parse error: bad slice length 33 (expected 32)");
             let err = hex_psbt!("70736274ff01005e02000000019bd48765230bf9a72e662001f972556e54f0c6f97feb56bcb5600d817f6995260100000000ffffffff0148e6052a01000000225120030da4fce4f7db28c2cb2951631e003713856597fe963882cb500e68112cca63000000000001012b00f2052a01000000225120c2247efbfd92ac47f6f40b8d42d169175a19fa9fa10e4a25d7f35eb4dd85b69241142cb13ac68248de806aa6a3659cf3c03eb6821d09c8114a4e868febde865bb6d2cd970e15f53fc0c82f950fd560ffa919b76172be017368a89913af074f400b094289756aa3739ccc689ec0fcf3a360be32cc0b59b16e93a1e8bb4605726b2ca7a3ff706c4176649632b2cc68e1f912b8a578e3719ce7710885c7a966f49bcd43cb01010000").unwrap_err();
             assert_eq!(err.to_string(), "parse failed: Invalid Schnorr signature length");
             let err = hex_psbt!("70736274ff01005e02000000019bd48765230bf9a72e662001f972556e54f0c6f97feb56bcb5600d817f6995260100000000ffffffff0148e6052a01000000225120030da4fce4f7db28c2cb2951631e003713856597fe963882cb500e68112cca63000000000001012b00f2052a01000000225120c2247efbfd92ac47f6f40b8d42d169175a19fa9fa10e4a25d7f35eb4dd85b69241142cb13ac68248de806aa6a3659cf3c03eb6821d09c8114a4e868febde865bb6d2cd970e15f53fc0c82f950fd560ffa919b76172be017368a89913af074f400b093989756aa3739ccc689ec0fcf3a360be32cc0b59b16e93a1e8bb4605726b2ca7a3ff706c4176649632b2cc68e1f912b8a578e3719ce7710885c7a966f49bcd43cb0000").unwrap_err();

src/util/schnorr.rs

@@ -283,7 +283,7 @@ impl fmt::Display for SchnorrSigError {
             SchnorrSigError::InvalidSighashType(hash_ty) =>
                 write!(f, "Invalid signature hash type {}", hash_ty),
             SchnorrSigError::Secp256k1(ref e) =>
-                write!(f, "Schnorr Signature has correct len, but is malformed : {}", e),
+                write_err!(f, "Schnorr signature has correct len but is malformed"; e),
             SchnorrSigError::InvalidSchnorrSigSize(sz) =>
                 write!(f, "Invalid Schnorr signature size: {}", sz),
         }

src/util/sighash.rs

@@ -212,7 +212,7 @@ 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, "Writer errored: {:?}", e),
+            Error::Io(error_kind) => write!(f, "writer errored: {:?}", error_kind),
             Error::IndexOutOfInputsBounds { index, inputs_size } => write!(f, "Requested index ({}) is greater or equal than the number of transaction inputs ({})", index, inputs_size),
             Error::SingleWithoutCorrespondingOutput { index, outputs_size } => write!(f, "SIGHASH_SINGLE for input ({}) haven't a corresponding output (#outputs:{})", index, outputs_size),
             Error::PrevoutsSize => write!(f, "Number of supplied prevouts differs from the number of inputs in transaction"),

src/util/taproot.rs

@@ -1004,8 +1004,8 @@ impl fmt::Display for TaprootBuilderError {
                 "Attempted to create a tree with two nodes at depth 0. There must\
                 only be a exactly one node at depth 0",
             ),
-            TaprootBuilderError::InvalidInternalKey(e) => {
-                write!(f, "Invalid Internal XOnly key : {}", e)
+            TaprootBuilderError::InvalidInternalKey(ref e) => {
+                write_err!(f, "invalid internal x-only key"; e)
             }
             TaprootBuilderError::IncompleteTree => {
                 write!(f, "Called finalize on an incomplete tree")
@@ -1077,9 +1077,8 @@ impl fmt::Display for TaprootError {
                 "Control Block size({}) must be of the form 33 + 32*m where  0 <= m <= {} ",
                 sz, TAPROOT_CONTROL_MAX_NODE_COUNT
             ),
-            // TODO: add source when in MSRV
-            TaprootError::InvalidInternalKey(e) => write!(f, "Invalid Internal XOnly key : {}", e),
-            TaprootError::InvalidParity(e) => write!(f, "Invalid parity value for internal key: {}", e),
+            TaprootError::InvalidInternalKey(ref e) => write_err!(f, "invalid internal x-only key"; e),
+            TaprootError::InvalidParity(_) => write!(f, "invalid parity value for internal key"),
             TaprootError::EmptyTree => write!(f, "Taproot Tree must contain at least one script"),
         }
     }