-
Notifications
You must be signed in to change notification settings - Fork 160
deps: Bump to rand v0.9 where possible #480
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
Conversation
#### Problem The repo is still on rand v0.8 in a lot of places, but that version isn't very compatible with rust 2024 due to its usage of `gen` as a function name, which is becoming a keyword. #### Summary of changes Follow https://rust-random.github.io/book/update-0.9.html to perform the upgrade to rand v0.9 where possible. Most crypto libraries are still on rand v0.8 / rand_core v0.6, and the blstrs and ecdsa implementations of `random` aren't as simple as just taking 32 or 64 random bytes, so I left those back.
alexpyattaev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the crates I know:
- address
- frozen-abi
- keypair
- serde-varint
Overall the change looks good, but I'd prefer if we pinned some version for frozen-abi. Technically, changing the rand version for frozen-abi at this point would require a major version bump (assuming someone has already started using the version with ABI checks).
| # to avoid version skew and keep digest computation stable regardless of consumer dependencies. | ||
| rand = "0.8.5" | ||
| rand_chacha = "0.3.1" | ||
| rand = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we pin the version of rand here to avoid ABI hashes from breaking accidentally? None of this runs in prod, so perf is a non-issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The frozen-abi tests in this repo would catch that, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add those tests. But yes, they should catch this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How different would they be from these? https://github.com/anza-xyz/solana-sdk/blob/master/scripts/test-frozen-abi.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub fn new() -> Self { | ||
| let mut rng = OsRng; | ||
| Self(ed25519_dalek::SigningKey::generate(&mut rng)) | ||
| let secret_bytes = rand::random::<[u8; Self::SECRET_KEY_LENGTH]>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, this changes from explicitly using OsRng to using ThreadRng. Not sure it matters, just pointing it out. It is probably for the best.
Right, it's a little annoying, but probably for the best |
Unfortunately. I have spoken with Alex and we agreed that perhaps it would be better if we switch If you think this is the good move then please leave |
I don't understand, is the separate PR mainly to do the contribution in a separate PR? The changes are here and passing CI, why force me to spend extra time to revert parts of this PR? |
Sorry! I didn't notice that you adjusted the interface definition as well. All good! 🙏 |
alexpyattaev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for crates:
-
address -
frozen-abi -
keypair -
serde-varint
|
@samkim-crypto can you review the other crates not covered by @alexpyattaev ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I checked the rest of the crates and things look good to me.
- we should keep rand to v0.8 for crates that depend on blstrs and ecdsa implementations
- double checked the
getrandomversion update forsdk-wasm-js - the other crates seem pretty straightforward. There seems to be some places where we can write things in slightly more idiomatic way, but very minor nits that you can choose to update only if you want to
serialize-utils/src/cursor.rs
Outdated
| let test_value = if rand::random_bool(0.5) { | ||
| Some(rand::random::<u64>()) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let test_value = if rand::random_bool(0.5) { | |
| Some(rand::random::<u64>()) | |
| } else { | |
| None | |
| }; | |
| let test_value = rand::rng() | |
| .random_bool(0.5) | |
| .then(|| rand::random::<u64>()); |
Very minor nit: I guess this could be written slightly more idiomatically.
vote-interface/src/state/mod.rs
Outdated
| .sorted_by_key(|lockout| lockout.slot()) | ||
| .collect(); | ||
| let root = rng.gen_ratio(1, 2).then(|| { | ||
| let root = rng.random_ratio(1, 2).then(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Seems like random_ratio should be used for fractions like 1/3 or 5/7. For this particular case, it seems like rng.random_bool(0.5) is probably more idiomatic.
vote-interface/src/state/mod.rs
Outdated
| }); | ||
| let timestamp = rng.gen_ratio(1, 2).then(|| rng.gen()); | ||
| let hash = Hash::from(rng.gen::<[u8; 32]>()); | ||
| let timestamp = rng.random_ratio(1, 2).then(|| rng.random()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: same here
0b27d3f
|
Addressed the nits, let me know what you think! |
samkim-crypto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Problem
The repo is still on rand v0.8 in a lot of places, but that version isn't very compatible with rust 2024 due to its usage of
genas a function name, which is becoming a keyword.Summary of changes
Follow https://rust-random.github.io/book/update-0.9.html to perform the upgrade to rand v0.9 where possible.
Most crypto libraries are still on rand v0.8 / rand_core v0.6, and the blstrs and ecdsa implementations of
randomaren't as simple as just taking 32 or 64 random bytes, so I left those back.cc @puhtaytow