From b6f9e47dba9b784c9a51cec2d182ea913088e724 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 25 Oct 2022 13:35:32 +0200 Subject: [PATCH] Fix `no_std` when `bitcoinconsensus` is enabled `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 --- bitcoin/Cargo.toml | 3 ++- bitcoin/src/blockdata/script.rs | 44 +++++++++++++++++++++++++++++++-- bitcoin/src/lib.rs | 7 ++++-- 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/bitcoin/Cargo.toml b/bitcoin/Cargo.toml index 7e7b87fa..034f52d8 100644 --- a/bitcoin/Cargo.toml +++ b/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 } diff --git a/bitcoin/src/blockdata/script.rs b/bitcoin/src/blockdata/script.rs index 7628da14..0134e06c 100644 --- a/bitcoin/src/blockdata/script.rs +++ b/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)), } } } diff --git a/bitcoin/src/lib.rs b/bitcoin/src/lib.rs index 72d01764..fa725e6b 100644 --- a/bitcoin/src/lib.rs +++ b/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)]