Merge rust-bitcoin/rust-bitcoin#1978: Wrap the `bitcoinconsensus` error

29a4f9b114 Wrap the bitcoinconsensus error (Tobin C. Harding)

Pull request description:

  Currently the `bitcoinconsensus` error is part of the public API. This hinders maintainability because changes to the verison of `bitcoinconsensus` force a re-release in `rust-bitcoin`. This is an unnecessary maintenance burden, we can wrap the error instead.

ACKs for top commit:
  apoelstra:
    ACK 29a4f9b114
  sanket1729:
    utACK 29a4f9b114

Tree-SHA512: 36bc1b0ad5f5675d79eea2409844a839d862997c256e301c53c5f1af547edc9a0b83e586bd70e1b8853722cd7ef279e7515e09fbe942660f8049090d1be39d3a

bitcoin/src/consensus/validation.rs

@@ -31,7 +31,7 @@ pub fn verify_script(
     index: usize,
     amount: Amount,
     spending_tx: &[u8],
-) -> Result<(), bitcoinconsensus::Error> {
+) -> Result<(), BitcoinconsensusError> {
     verify_script_with_flags(script, index, amount, spending_tx, bitcoinconsensus::VERIFY_ALL)
 }
 
@@ -50,14 +50,14 @@ pub fn verify_script_with_flags<F: Into<u32>>(
     amount: Amount,
     spending_tx: &[u8],
     flags: F,
-) -> Result<(), bitcoinconsensus::Error> {
+) -> Result<(), BitcoinconsensusError> {
-    bitcoinconsensus::verify_with_flags(
+    Ok(bitcoinconsensus::verify_with_flags(
         script.as_bytes(),
         amount.to_sat(),
         spending_tx,
         index,
         flags.into(),
-    )
+    )?)
 }
 
 /// Verifies that this transaction is able to spend its inputs.
@@ -121,7 +121,7 @@ impl Script {
         index: usize,
         amount: crate::Amount,
         spending_tx: &[u8],
-    ) -> Result<(), bitcoinconsensus::Error> {
+    ) -> Result<(), BitcoinconsensusError> {
         verify_script(self, index, amount, spending_tx)
     }
 
@@ -140,7 +140,7 @@ impl Script {
         amount: crate::Amount,
         spending_tx: &[u8],
         flags: F,
-    ) -> Result<(), bitcoinconsensus::Error> {
+    ) -> Result<(), BitcoinconsensusError> {
         verify_script_with_flags(self, index, amount, spending_tx, flags)
     }
 }
@@ -172,11 +172,40 @@ impl Transaction {
     }
 }
 
+/// Wrapped error from `bitcoinconsensus`.
+// We do this for two reasons:
+// 1. We don't want the error to be part of the public API because we do not want to expose the
+//    unusual versioning used in `bitcoinconsensus` to users of `rust-bitcoin`.
+// 2. We want to implement `std::error::Error` if the "std" feature is enabled in `rust-bitcoin` but
+//    not in `bitcoinconsensus`.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct BitcoinconsensusError(bitcoinconsensus::Error);
+
+impl fmt::Display for BitcoinconsensusError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write_err!(f, "bitcoinconsensus error"; &self.0)
+    }
+}
+
+#[cfg(all(feature = "std", feature = "bitcoinconsensus-std"))]
+impl std::error::Error for BitcoinconsensusError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { Some(&self.0) }
+}
+
+#[cfg(all(feature = "std", not(feature = "bitcoinconsensus-std")))]
+impl std::error::Error for BitcoinconsensusError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None }
+}
+
+impl From<bitcoinconsensus::Error> for BitcoinconsensusError {
+    fn from(e: bitcoinconsensus::Error) -> Self { Self(e) }
+}
+
 /// An error during transaction validation.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum TxVerifyError {
     /// Error validating the script with bitcoinconsensus library.
-    ScriptVerification(bitcoinconsensus::Error),
+    ScriptVerification(BitcoinconsensusError),
     /// Can not find the spent output.
     UnknownSpentOutput(OutPoint),
 }
@@ -186,9 +215,7 @@ impl fmt::Display for TxVerifyError {
         use TxVerifyError::*;
 
         match *self {
-            ScriptVerification(ref e) => {
+            ScriptVerification(ref e) => write_err!(f, "bitcoinconsensus verification failed"; e),
-                write_err!(f, "bitcoinconsensus verification failed"; bitcoinconsensus_hack::wrap_error(e))
-            }
             UnknownSpentOutput(ref p) => write!(f, "unknown spent output: {}", p),
         }
     }
@@ -200,50 +227,12 @@ impl std::error::Error for TxVerifyError {
         use TxVerifyError::*;
 
         match *self {
-            ScriptVerification(ref e) => Some(bitcoinconsensus_hack::wrap_error(e)),
+            ScriptVerification(ref e) => Some(e),
             UnknownSpentOutput(_) => None,
         }
     }
 }
 
-impl From<bitcoinconsensus::Error> for TxVerifyError {
+impl From<BitcoinconsensusError> for TxVerifyError {
-    fn from(e: bitcoinconsensus::Error) -> Self { TxVerifyError::ScriptVerification(e) }
+    fn from(e: BitcoinconsensusError) -> Self { TxVerifyError::ScriptVerification(e) }
-}
-
-// If bitcoinonsensus-std is off but bitcoinconsensus is present we patch the error type to
-// implement `std::error::Error`.
-#[cfg(all(feature = "std", feature = "bitcoinconsensus", not(feature = "bitcoinconsensus-std")))]
-mod bitcoinconsensus_hack {
-    use core::fmt;
-
-    #[repr(transparent)]
-    pub(crate) struct Error(bitcoinconsensus::Error);
-
-    impl fmt::Debug for Error {
-        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Debug::fmt(&self.0, f) }
-    }
-
-    impl fmt::Display for Error {
-        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) }
-    }
-
-    // bitcoinconsensus::Error has no sources at this time
-    impl std::error::Error for Error {}
-
-    pub(crate) fn wrap_error(error: &bitcoinconsensus::Error) -> &Error {
-        // Unfortunately, we cannot have the reference inside `Error` struct because of the 'static
-        // bound on `source` return type, so we have to use unsafe to overcome the limitation.
-        // SAFETY: the type is repr(transparent) and the lifetimes match
-        unsafe { &*(error as *const _ as *const Error) }
-    }
-}
-
-#[cfg(not(all(
-    feature = "std",
-    feature = "bitcoinconsensus",
-    not(feature = "bitcoinconsensus-std")
-)))]
-mod bitcoinconsensus_hack {
-    #[allow(unused_imports)] // conditionally used
-    pub(crate) use core::convert::identity as wrap_error;
 }