Merge rust-bitcoin/rust-bitcoin#1344: Fix `no_std` when `bitcoinconsensus` is enabled

b6f9e47dba Fix `no_std` when `bitcoinconsensus` is enabled (Martin Habovstiak)

Pull request description:

  `default-features = false` was missing previously but blindly adding it would lead to subtle risk of breaking when a crate not needing `std` depends on `bitcoinconsensus` and simultaneously another crate not needing `bitcoinconsensus` depends on `std` and another crate depends on them both.

  This change fixes it by introducing `bitcoinconsensus-std` feature flag and provides a fallback if the flag is off. Unfortunately the fallback has to use a bit of reasonable `unsafe` due to limitations of upcasting.

  The only safe alternatives are not do it and provide worse experience for crates that are affected by the problem above or break the API, which couldn't be backported and would be more annoying to use.

  Closes #1343

  This is considered PoC PR as I realized the possibility of the hack (and necessity of `unsafe`) at the last moment. Things like tests and modifying CONTRIBUTING to change the stance on `unsafe` will be added if `unsafe` is ACKed.

ACKs for top commit:
  tcharding:
    tACK b6f9e47dba
  apoelstra:
    ACK b6f9e47dba

Tree-SHA512: 3a2845f4701c94ff6214749fa490aecf3fd96089df31b15f9d3e0afe3c74329ff2b9054d51244358a79f928aa9d4cf4001fc3ec40a9b0e189323544c4480c709

bitcoin/Cargo.toml

@@ -19,6 +19,7 @@ rand = ["secp256k1/rand-std"]
 serde = ["actual-serde", "bitcoin_hashes/serde", "secp256k1/serde"]
 secp-lowmemory = ["secp256k1/lowmemory"]
 secp-recovery = ["secp256k1/recovery"]
+bitcoinconsensus-std = ["bitcoinconsensus/std", "std"]
 
 # At least one of std, no-std must be enabled.
 #
@@ -40,7 +41,7 @@ secp256k1 = { version = "0.24.0", default-features = false, features = ["bitcoin
 core2 = { version = "0.3.0", optional = true, default-features = false }
 
 base64 = { version = "0.13.0", optional = true }
-bitcoinconsensus = { version = "0.20.2-0.5.0", optional = true }
+bitcoinconsensus = { version = "0.20.2-0.5.0", optional = true, default-features = false }
 # Do NOT use this as a feature! Use the `serde` feature instead.
 actual-serde = { package = "serde", version = "1.0.103", default-features = false, features = [ "derive", "alloc" ], optional = true }
 hashbrown = { version = "0.8", optional = true }

bitcoin/src/blockdata/script.rs

@@ -170,6 +170,46 @@ pub enum Error {
     Serialization
 }
 
+// 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;
+}
+
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
@@ -177,7 +217,7 @@ impl fmt::Display for Error {
             Error::EarlyEndOfScript => f.write_str("unexpected end of script"),
             Error::NumericOverflow => f.write_str("numeric overflow (number on stack larger than 4 bytes)"),
             #[cfg(feature = "bitcoinconsensus")]
-            Error::BitcoinConsensus(ref e) => write_err!(f, "bitcoinconsensus verification failed"; e),
+            Error::BitcoinConsensus(ref e) => write_err!(f, "bitcoinconsensus verification failed"; bitcoinconsensus_hack::wrap_error(e)),
             Error::UnknownSpentOutput(ref point) => write!(f, "unknown spent output: {}", point),
             Error::Serialization => f.write_str("can not serialize the spending transaction in Transaction::verify()"),
         }
@@ -197,7 +237,7 @@ impl std::error::Error for Error {
             | UnknownSpentOutput(_)
             | Serialization => None,
             #[cfg(feature = "bitcoinconsensus")]
-            BitcoinConsensus(ref e) => Some(e),
+            BitcoinConsensus(ref e) => Some(bitcoinconsensus_hack::wrap_error(e)),
         }
     }
 }

bitcoin/src/lib.rs

@@ -26,14 +26,17 @@
 //! * `no-std` - enables additional features required for this crate to be usable
 //!              without std. Does **not** disable `std`. Depends on `hashbrown`
 //!              and `core2`.
-//!
+//! * `bitcoinconsensus-std` - enables `std` in `bitcoinconsensus` and communicates it
+//!                            to this crate so it knows how to implement
+//!                            `std::error::Error`. At this time there's a hack to
+//!                            achieve the same without this feature but it could
+//!                            happen the implementations diverge one day.
 
 #![cfg_attr(all(not(feature = "std"), not(test)), no_std)]
 // Experimental features we need.
 #![cfg_attr(bench, feature(test))]
 #![cfg_attr(docsrs, feature(doc_cfg))]
 // Coding conventions
-#![forbid(unsafe_code)]
 #![deny(non_upper_case_globals)]
 #![deny(non_camel_case_types)]
 #![deny(non_snake_case)]