Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On key-generation example (documentation), "rand" should be "rand-std" instead #389

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

vinliao
Copy link
Contributor

@vinliao vinliao commented Jan 27, 2022

I copy-pasted the key-generation example written on the documentation, but it didn't work. It only worked when I used the feature rand-std instead of rand.

To reproduce, boot up a new Rust project, and add this to main.rs:

use secp256k1::rand::rngs::OsRng;
use secp256k1::{Secp256k1, Message};
use secp256k1::hashes::sha256;

let secp = Secp256k1::new();
let mut rng = OsRng::new().expect("OsRng");
let (secret_key, public_key) = secp.generate_keypair(&mut rng);
let message = Message::from_hashed_data::<sha256::Hash>("Hello World!".as_bytes());

let sig = secp.sign_ecdsa(&message, &secret_key);
assert!(secp.verify_ecdsa(&message, &sig, &public_key).is_ok());

Using this dependencies causes error: secp256k1 = {version="0.21.2", features=["rand", "bitcoin_hashes"]}. After replacing rand with rand-std, it works.

apoelstra
apoelstra previously approved these changes Jan 27, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2732891

Yeah, I think OsRng requires std. That's unfortunate.

tcharding
tcharding previously approved these changes Jan 30, 2022
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I actually went to verify this just recently, clearly I should have trusted the code not the docs :) For rand 0.6.5 (which we currently use) the docs for OsRng do not say that std features is needed.

Thanks again.

https://docs.rs/rand/0.6.5/rand/rngs/struct.OsRng.html

tACK 2732891

@vinliao vinliao dismissed stale reviews from tcharding and apoelstra via 89fb2a0 January 31, 2022 08:37
@vinliao
Copy link
Contributor Author

vinliao commented Jan 31, 2022

I've found a few "rand" feature requirement in the documentation and have replaced it with "rand-std" (again).

@apoelstra
Copy link
Member

I don't understand why rand-std is needed for any of these new ones. Can you confirm that they don't work with rand?

@vinliao
Copy link
Contributor Author

vinliao commented Jan 31, 2022

That last commit was not a well-considered commit. I was browsing the documentation and just sort of assumed that it needed to be changed (because it had rand instead of rand-std in it). It was a mistake on my part. I have removed the last commit from the PR.

@tcharding
Copy link
Member

tcharding commented Jan 31, 2022

Don't feel bad for getting confused @vinliao, I recently got merged a heap of rustdoc tests that should fail because I used feature-gate rand instead of rand-std :(

EDIT:

Seems there is a bug in cargo, the tests in key.rs currently on master branch run successfully but AFAICT they should fail. Here is an example, running cargo test --features=rand should make this test fail but it doesn't?

/// Secret 256-bit key used as `x` in an ECDSA signature.
///
/// # Examples
///
/// Basic usage:
///
/// ```
/// # #[cfg(all(feature = "rand", any(feature =  "alloc", feature = "std")))] {
/// use secp256k1::{rand, Secp256k1, SecretKey};
///
/// let secp = Secp256k1::new();
/// let secret_key = SecretKey::new(&mut rand::thread_rng());
/// # }
/// ``` 

@apoelstra
Copy link
Member

@tcharding should it fail because rand/std is not enabled? Interesting..

@tcharding
Copy link
Member

Yes, because we use thread_rng which needs rand/std. If one puts this same code in a crate and only enable rand it doesn't build.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2732891

Curious that my previous ACK, on this exact commit, no longer counts..

@apoelstra apoelstra merged commit 7a3736a into rust-bitcoin:master Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants