Fix feature gating #520

tcharding · 2022-11-17T23:14:56Z

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

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

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".

apoelstra

ACK 9c74855

apoelstra · 2022-11-17T23:36:39Z

Will wait on @Kixunil to sanity-check before merging. This is a big PR but it's almost entirely mechanical and all the "real" changes are in Cargo.toml.

tcharding · 2022-11-18T00:01:31Z

Will wait on @Kixunil to sanity-check before merging. This is a big PR but it's almost entirely mechanical and all the "real" changes are in Cargo.toml.

And I could easily have missed some places, I was relying on grep heavily to find lines of code that needed patching.

Kixunil

ACK 9c74855

Very nice cleanup!

All of my suggestions are intended for possible future PRs.

Kixunil · 2022-11-18T12:08:13Z

Cargo.toml

@@ -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"


FTR, IIRC I had WIP changes that add rand_core feature so this one would have to go too. But it's OK now.

Kixunil · 2022-11-18T12:15:54Z

src/ecdh.rs

-/// # #[cfg(all(feature = "std", feature = "rand-std"))] {
-/// # use secp256k1::Secp256k1;
+/// # #[cfg(feature = "rand-std")] {
+/// # use secp256k1::{rand, Secp256k1};


Not for this PR but this looks very confusing - it looks as if #[cfg] was controlling use, not the whole block. If we don't want to indent it I suggest we put { on the next line - this is unusual in Rust but much more readable in this case.

Agreed. The old formatting was chosen with "this line will be hidden" in mind.

I have a mild preference for indenting vs putting { on the next line.

Its already laborious to write example code in comments because of zero tooling support, we should be careful what we choose because we (I) will have to go back and change every example in the whole codebase.

Ok, then let's do { on the next line since that's easiest :P and say that this is a very low priority thing.

Kixunil · 2022-11-18T12:56:50Z

src/lib.rs

+    let mut ret = [0u8; 32];
+    rng.fill(&mut ret);
+    ret
+}


This is completely OK for this PR but I don't like non-generic 32_bytes much. For the future I was thinking of some alternative designs: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3123fb72993eabdeeb4a51eb2558be88

This function is only available within the crate, where we routinely need 32 uniformly random bytes and never need any other quantity of differently-random bytes.

Kixunil · 2022-11-18T13:02:26Z

src/lib.rs

-    use crate::ffi::types::AlignedType;
-    use crate::ffi::{self};
+    #[cfg(feature = "alloc")]
+    use crate::{ffi, PublicKey, Secp256k1, SecretKey};


IIUC these do not require alloc but only tests requiring alloc happen to use them, right?

It's not very readable but the only somewhat reasonable idea beyond adding a comment that comes to my mind is having test_std, test_alloc, test_rand... etc modules and the modules have appropriate cfg. Maybe not having same cfgs multiple times is good enough benefit for it?

I think this is a pretty common pattern in test modules to have feature-gated imports because there are feature-gated tests.

Kixunil · 2022-11-18T13:07:55Z

src/scalar.rs

    #[cfg_attr(docsrs, doc(cfg(feature = "rand-std")))]
    pub fn random() -> Self { Self::random_custom(rand::thread_rng()) }

    /// Generates a random scalar using supplied RNG
-    #[cfg(any(test, feature = "rand"))]
+    #[cfg(feature = "rand")]
    #[cfg_attr(docsrs, doc(cfg(feature = "rand")))]
    pub fn random_custom<R: rand::Rng>(mut rng: R) -> Self {


Oh, I should've implemented Distribution<Scalar> for Standard. 🤦‍♂️

ITYM for Uniform, but honestly this seems like more complexity (and reading rand docs) (and hoping they never change) than it's worth.

I think for both Standard and Uniform. If rand is still too far away from stabilizing maybe we should just put it off until then.

apoelstra · 2022-11-18T19:00:56Z

Thanks for your review and (somewhat hesitant) ACK! We'll address things in followup PRs.

tcharding · 2022-11-18T19:18:33Z

Thanks fellas, feels like the feature gating saga is going to be an ongoing one :)

Kixunil · 2022-11-18T23:12:47Z

Should we release a new version so that we unblock development in bitcoin?

apoelstra · 2022-11-18T23:32:21Z

I'd like to get #518 in as well since I believe it is also semver-breaking.

Kixunil · 2022-11-19T07:59:27Z

Oh, yes, we had "remove deprecated" commits. Would it make sense to backport this for point release?

apoelstra · 2022-11-19T13:53:20Z

I think so. Normally changes to feature flags are breaking but I think this is purely additive (except for dev-dependencies). Similarly I think all the changes to feature-gating of functionality is purely additive (things are now available that didn't use to be).

Because it touches so many LOC it'd be a real pain to backport, I think. In particular #499 will be really annoying to backport past :).

I think it'd be less work to get #518 in and then cut a release -- it's almost ready.

Kixunil · 2022-11-20T19:43:21Z

Oh, OK. Yes, this change is backwards-compatible.

apoelstra · 2022-11-21T14:59:31Z

Ok, 518 is in. We can just do a major release and pull that into rust-bitcoin I think.

apoelstra · 2022-11-21T14:59:43Z

cc @tcharding can you open a release PR so that I can ack and merge it?

tcharding · 2022-11-21T19:33:35Z

Sure thing!

tcharding mentioned this pull request Nov 17, 2022

Correct bench feature gating #519

Closed

apoelstra approved these changes Nov 17, 2022

View reviewed changes

Kixunil approved these changes Nov 18, 2022

View reviewed changes

This was referenced Nov 18, 2022

impl Distribution<T> for Standard for relevant T #521

Open

Consider an alternative to random_32_bytes #522

Open

apoelstra merged commit 29c0549 into rust-bitcoin:master Nov 18, 2022

tcharding deleted the 11-18-fix-feature-gating branch November 21, 2022 19:48

tcharding mentioned this pull request Nov 21, 2022

cargo test --no-default-features --features=rand-std fails #494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix feature gating #520

Fix feature gating #520

tcharding commented Nov 17, 2022

apoelstra left a comment

apoelstra commented Nov 17, 2022

tcharding commented Nov 18, 2022

Kixunil left a comment

Kixunil Nov 18, 2022

Kixunil Nov 18, 2022

apoelstra Nov 18, 2022

tcharding Nov 18, 2022

apoelstra Nov 18, 2022

Kixunil Nov 18, 2022

apoelstra Nov 18, 2022

Kixunil Nov 18, 2022

apoelstra Nov 18, 2022

Kixunil Nov 18, 2022

apoelstra Nov 18, 2022

Kixunil Nov 18, 2022

apoelstra commented Nov 18, 2022

tcharding commented Nov 18, 2022

Kixunil commented Nov 18, 2022

apoelstra commented Nov 18, 2022

Kixunil commented Nov 19, 2022

apoelstra commented Nov 19, 2022

Kixunil commented Nov 20, 2022

apoelstra commented Nov 21, 2022

apoelstra commented Nov 21, 2022

tcharding commented Nov 21, 2022

Fix feature gating #520

Fix feature gating #520

Conversation

tcharding commented Nov 17, 2022

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Nov 17, 2022

tcharding commented Nov 18, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Nov 18, 2022

tcharding commented Nov 18, 2022

Kixunil commented Nov 18, 2022

apoelstra commented Nov 18, 2022

Kixunil commented Nov 19, 2022

apoelstra commented Nov 19, 2022

Kixunil commented Nov 20, 2022

apoelstra commented Nov 21, 2022

apoelstra commented Nov 21, 2022

tcharding commented Nov 21, 2022