Merge #520: Fix feature gating

9c74855 Fix feature gating (Tobin C. Harding)

Pull request description:

  Currently we have a few problems with our feature gating, attempt to audit all feature gating and fix it by doing:

  1. Do not enable features on optional dependencies (`rand` and `bitcoin-hashes`) in dev-dependencies, doing so hides broken feature gating in unit tests.
  2. Do not use unnecessary feature combinations when one feature enables another e.g. `any(feature = "std", feature = "alloc")`.
  3. Enable "std" from "rand-std" and "bitcoin-std" (and fix features gating as for point 2).
  4. Clean up code around `rand::thread_rng`, this is part of this patch because `thread_rng` requires the "rand-std" feature.
  5. Clean up CI test script to test each feature individually now that "rand-std" and "bitcoin-hashes-std" enable "std".

ACKs for top commit:
  apoelstra:
    ACK 9c74855
  Kixunil:
    ACK 9c74855

Tree-SHA512: 3f8e464eeab1fe279249b5097082774c3de076ec9537162f367013098bde45957b12067a7434389e520bfacd3b415274f897917bf000f332d52cf960b1d4aecf

Author: apoelstra

Cargo.toml

@@ -23,8 +23,8 @@ std = ["alloc", "secp256k1-sys/std"]
 # allow use of Secp256k1::new and related API that requires an allocator
 alloc = ["secp256k1-sys/alloc"]
 bitcoin-hashes = ["bitcoin_hashes"] # Feature alias because of the underscore.
-bitcoin-hashes-std = ["bitcoin-hashes", "bitcoin_hashes/std"]
-rand-std = ["rand/std", "rand/std_rng"]
+bitcoin-hashes-std = ["std", "bitcoin_hashes/std"]
+rand-std = ["std", "rand/std", "rand/std_rng"]
 recovery = ["secp256k1-sys/recovery"]
 lowmemory = ["secp256k1-sys/lowmemory"]
 global-context = ["std"]
@@ -46,10 +46,8 @@ bitcoin_hashes = { version = "0.11", default-features = false, optional = true }
 rand = { version = "0.8", default-features = false, optional = true }
 
 [dev-dependencies]
-rand = "0.8"
 rand_core = "0.6"
 serde_test = "1.0"
-bitcoin_hashes = "0.11"
 bincode = "1.3.3"
 
 # cbor does not build on WASM, we use it in a single trivial test (an example of when
@@ -64,15 +62,15 @@ getrandom = { version = "0.2", features = ["js"] }
 
 [[example]]
 name = "sign_verify_recovery"
-required-features = ["std", "recovery"]
+required-features = ["recovery", "bitcoin-hashes-std"]
 
 [[example]]
 name = "sign_verify"
-required-features = ["std"]
+required-features = ["bitcoin-hashes-std"]
 
 [[example]]
 name = "generate_keys"
-required-features = ["std", "rand-std"]
+required-features = ["rand-std"]
 
 [workspace]
 members = ["secp256k1-sys"]

contrib/test.sh

@@ -2,10 +2,7 @@
 
 set -ex
 
-FEATURES="bitcoin-hashes global-context lowmemory rand recovery serde std alloc"
-# These features are typically enabled along with the 'std' feature, so we test
-# them together with 'std'.
-STD_FEATURES="rand-std bitcoin-hashes-std"
+FEATURES="bitcoin-hashes global-context lowmemory rand recovery serde std alloc bitcoin-hashes-std rand-std"
 
 cargo --version
 rustc --version
@@ -49,17 +46,16 @@ if [ "$DO_FEATURE_MATRIX" = true ]; then
     RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all
     RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all --features="$FEATURES"
     cargo test --all --features="rand serde"
-    cargo test --features="$STD_FEATURES"
 
     if [ "$NIGHTLY" = true ]; then
         cargo test --all --all-features
         RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all --all-features
     fi
 
     # Examples
-    cargo run --example sign_verify --features=std
-    cargo run --example sign_verify_recovery --features=std,recovery
-    cargo run --example generate_keys --features=std,rand-std
+    cargo run --example sign_verify --features=bitcoin-hashes-std
+    cargo run --example sign_verify_recovery --features=recovery,bitcoin-hashes-std
+    cargo run --example generate_keys --features=rand-std
 fi
 
 # Build the docs if told to (this only works with the nightly toolchain)
@@ -96,7 +92,7 @@ fi
 # Bench if told to, only works with non-stable toolchain (nightly, beta).
 if [ "$DO_BENCH" = true ]
 then
-    RUSTFLAGS='--cfg=bench' cargo bench --features=recovery
+    RUSTFLAGS='--cfg=bench' cargo bench --features=recovery,rand-std
 fi
 
 exit 0

examples/generate_keys.rs

@@ -1,11 +1,10 @@
 extern crate secp256k1;
 
-use secp256k1::rand::thread_rng;
 use secp256k1::{PublicKey, Secp256k1, SecretKey};
 
 fn main() {
     let secp = Secp256k1::new();
-    let mut rng = thread_rng();
+    let mut rng = rand::thread_rng();
     // First option:
     let (seckey, pubkey) = secp.generate_keypair(&mut rng);
 

src/context.rs

@@ -31,8 +31,7 @@ pub mod global {
     /// ```
     /// # #[cfg(all(feature = "global-context", feature = "rand-std"))] {
     /// use secp256k1::{PublicKey, SECP256K1};
-    /// use secp256k1::rand::thread_rng;
-    /// let _ = SECP256K1.generate_keypair(&mut thread_rng());
+    /// let _ = SECP256K1.generate_keypair(&mut rand::thread_rng());
     /// # }
     /// ```
     pub static SECP256K1: &GlobalContext = &GlobalContext { __private: () };
@@ -106,7 +105,7 @@ mod private {
 }
 
 #[cfg(feature = "alloc")]
-#[cfg_attr(docsrs, doc(cfg(any(feature = "alloc"))))]
+#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
 mod alloc_only {
     use core::marker::PhantomData;
 
@@ -176,7 +175,7 @@ mod alloc_only {
         /// If `rand-std` feature is enabled, context will have been randomized using `thread_rng`.
         /// If `rand-std` feature is not enabled please consider randomizing the context as follows:
         /// ```
-        /// # #[cfg(all(feature = "std", feature = "rand-std"))] {
+        /// # #[cfg(feature = "rand-std")] {
         /// # use secp256k1::Secp256k1;
         /// # use secp256k1::rand::{thread_rng, RngCore};
         /// let mut ctx = Secp256k1::new();

src/ecdh.rs

@@ -32,13 +32,12 @@ const SHARED_SECRET_SIZE: usize = constants::SECRET_KEY_SIZE;
 /// # Examples
 ///
 /// ```
-/// # #[cfg(all(feature = "std", feature = "rand-std"))] {
-/// # use secp256k1::Secp256k1;
+/// # #[cfg(feature = "rand-std")] {
+/// # use secp256k1::{rand, Secp256k1};
 /// # use secp256k1::ecdh::SharedSecret;
-/// # use secp256k1::rand::thread_rng;
 /// let s = Secp256k1::new();
-/// let (sk1, pk1) = s.generate_keypair(&mut thread_rng());
-/// let (sk2, pk2) = s.generate_keypair(&mut thread_rng());
+/// let (sk1, pk1) = s.generate_keypair(&mut rand::thread_rng());
+/// let (sk2, pk2) = s.generate_keypair(&mut rand::thread_rng());
 /// let sec1 = SharedSecret::new(&pk2, &sk1);
 /// let sec2 = SharedSecret::new(&pk1, &sk2);
 /// assert_eq!(sec1, sec2);
@@ -122,14 +121,13 @@ impl AsRef<[u8]> for SharedSecret {
 ///
 /// # Examples
 /// ```
-/// # #[cfg(all(feature = "bitcoin-hashes-std", feature = "rand-std", feature = "std"))] {
-/// # use secp256k1::{ecdh, Secp256k1, PublicKey, SecretKey};
+/// # #[cfg(all(feature = "bitcoin-hashes-std", feature = "rand-std"))] {
+/// # use secp256k1::{ecdh, rand, Secp256k1, PublicKey, SecretKey};
 /// # use secp256k1::hashes::{Hash, sha512};
-/// # use secp256k1::rand::thread_rng;
 ///
 /// let s = Secp256k1::new();
-/// let (sk1, pk1) = s.generate_keypair(&mut thread_rng());
-/// let (sk2, pk2) = s.generate_keypair(&mut thread_rng());
+/// let (sk1, pk1) = s.generate_keypair(&mut rand::thread_rng());
+/// let (sk2, pk2) = s.generate_keypair(&mut rand::thread_rng());
 ///
 /// let point1 = ecdh::shared_secret_point(&pk2, &sk1);
 /// let secret1 = sha512::Hash::hash(&point1);
@@ -201,19 +199,18 @@ impl<'de> ::serde::Deserialize<'de> for SharedSecret {
 #[cfg(test)]
 #[allow(unused_imports)]
 mod tests {
-    use rand::thread_rng;
     #[cfg(target_arch = "wasm32")]
     use wasm_bindgen_test::wasm_bindgen_test as test;
 
     use super::SharedSecret;
     use crate::Secp256k1;
 
     #[test]
-    #[cfg(all(feature = "rand-std", any(feature = "alloc", feature = "std")))]
+    #[cfg(feature = "rand-std")]
     fn ecdh() {
         let s = Secp256k1::signing_only();
-        let (sk1, pk1) = s.generate_keypair(&mut thread_rng());
-        let (sk2, pk2) = s.generate_keypair(&mut thread_rng());
+        let (sk1, pk1) = s.generate_keypair(&mut rand::thread_rng());
+        let (sk2, pk2) = s.generate_keypair(&mut rand::thread_rng());
 
         let sec1 = SharedSecret::new(&pk2, &sk1);
         let sec2 = SharedSecret::new(&pk1, &sk2);
@@ -241,15 +238,15 @@ mod tests {
 
     #[test]
     #[cfg(not(fuzzing))]
-    #[cfg(all(feature = "std", feature = "rand-std", feature = "bitcoin-hashes-std"))]
+    #[cfg(all(feature = "bitcoin-hashes-std", feature = "rand-std"))]
     fn bitcoin_hashes_and_sys_generate_same_secret() {
         use bitcoin_hashes::{sha256, Hash, HashEngine};
 
         use crate::ecdh::shared_secret_point;
 
         let s = Secp256k1::signing_only();
-        let (sk1, _) = s.generate_keypair(&mut thread_rng());
-        let (_, pk2) = s.generate_keypair(&mut thread_rng());
+        let (sk1, _) = s.generate_keypair(&mut rand::thread_rng());
+        let (_, pk2) = s.generate_keypair(&mut rand::thread_rng());
 
         let secret_sys = SharedSecret::new(&pk2, &sk1);
 
@@ -266,7 +263,7 @@ mod tests {
     }
 
     #[test]
-    #[cfg(all(feature = "serde", any(feature = "alloc", feature = "std")))]
+    #[cfg(all(feature = "serde", feature = "alloc"))]
     fn serde() {
         use serde_test::{assert_tokens, Configure, Token};
         #[rustfmt::skip]
@@ -291,8 +288,8 @@ mod tests {
 }
 
 #[cfg(bench)]
+#[cfg(feature = "rand-std")]    // Currently only a single bench that requires "rand-std".
 mod benches {
-    use rand::thread_rng;
     use test::{black_box, Bencher};
 
     use super::SharedSecret;
@@ -301,7 +298,7 @@ mod benches {
     #[bench]
     pub fn bench_ecdh(bh: &mut Bencher) {
         let s = Secp256k1::signing_only();
-        let (sk, pk) = s.generate_keypair(&mut thread_rng());
+        let (sk, pk) = s.generate_keypair(&mut rand::thread_rng());
 
         bh.iter(|| {
             let res = SharedSecret::new(&pk, &sk);

src/ecdsa/mod.rs

@@ -364,12 +364,11 @@ impl<C: Verification> Secp256k1<C> {
     /// verify-capable context.
     ///
     /// ```rust
-    /// # #[cfg(all(feature = "std", feature = "rand-std"))] {
-    /// # use secp256k1::rand::thread_rng;
-    /// # use secp256k1::{Secp256k1, Message, Error};
+    /// # #[cfg(feature = "rand-std")] {
+    /// # use secp256k1::{rand, Secp256k1, Message, Error};
     /// #
     /// # let secp = Secp256k1::new();
-    /// # let (secret_key, public_key) = secp.generate_keypair(&mut thread_rng());
+    /// # let (secret_key, public_key) = secp.generate_keypair(&mut rand::thread_rng());
     /// #
     /// let message = Message::from_slice(&[0xab; 32]).expect("32 bytes");
     /// let sig = secp.sign_ecdsa(&message, &secret_key);