Bump rand to 0.8, rand_chacha to 0.3, getrandom to 0.2 #32871

joncinque · 2023-08-17T13:44:42Z

Problem

In order to properly address the cargo audit error in #32836, we'll need to bump ed25519-dalek everywhere, which also entails upgrading the various rand dependencies.

Summary of Changes

Since we expose a dependency to the rand 0.7 types through functions like Keypair::generate, we need to be careful about performing this bump to avoid breaking downstream users. This PR does not break downstream users and avoids touching ed25519-dalek / libsecp256k1, except in tests. That work can be done in other PRs.

Here are the Solana-specific changes in this PR:

deprecate hash::new_rand() in favor of hash::new_with_thread_rng(), since just about everywhere that used new_rand was just passing thread_rng, and it's only used in tests
keep the dependency to 0.7 in all places that have a strong dependency on ed25519-dalek: solana-zk-token-sdk, solana_sdk::signer::keypair::Keypair, solana_sdk::ed25519_instruction, solana_sdk::secp256k1_instruction
inline keypair generation in tests to use the new rand with the old libraries

Here are the rand-required changes in the PR:

gen_range now takes a range, so gen_range(1, 2) becomes gen_range(1..2)
use map(char::from) when generating a random string
use Fill instead of AsByteSliceMut
don't depend on ThreadRng implementing Copy

More information on rand at https://rust-random.github.io/book/update-0.8.html

You should be able to follow the commits pretty linearly, except when I wrongly created Keypair::from_secret_key_bytes, only to realize that it was just keypair_from_seed a few commits later. The concept was:

bump the crates in be1e6db and 18ebdf3
Do git grep -l gen_range | xargs sed -i'' -e 's/gen_range(\(\S*\), /gen_range(\1../' to fix most gen_range instances in 99a0f1e, then clean it up in f78cef6
Do git grep -l hash::new_rand | xargs sed -i'' -e 's/hash::new_rand([^)]*/hash::new_with_thread_rng(/' to deprecate hash::new_rand everywhere in 5f9675a
After that, each commit goes through errors that were triggered in each crate

Please tag anyone else who might be good to review this!

Fixes #

perf/src/test_tx.rs

joncinque · 2023-08-17T13:51:25Z

sdk/src/signer/keypair.rs

@@ -25,6 +25,9 @@ use {
 pub struct Keypair(ed25519_dalek::Keypair);

 impl Keypair {
+    /// Can be used for generating a Keypair without a dependency on `rand` types


This comment might be too specific, but it shows why I needed it

hardcode 64 and static_assertions::const_assert_eq!(SECRET_KEY_LENGTH, ed25519_dalek::SECRET_KEY_LENGTH) to keep dalek out of the api?

Even better, will do!

Done in db48acc

brooksprumo

Overall looks good to me!

For the changes on the actual cryptographic bits, I think another set of eyes with proper experience would be a good idea.

perf/src/test_tx.rs

ledger/src/shred.rs

sdk/src/hash.rs

ledger/src/shredder.rs

t-nelson

this whole change set makes me want to take a hardline that traits have no business being versioned with logic

sdk/src/hash.rs

sdk/src/signer/keypair.rs

perf/src/test_tx.rs

streamer/src/nonblocking/quic.rs

t-nelson · 2023-08-17T16:53:35Z

accounts-db/src/append_vec/test_utils.rs

@@ -25,6 +25,7 @@ pub fn get_append_vec_path(path: &str) -> TempFile {
    let out_dir = get_append_vec_dir();
    let rand_string: String = rand::thread_rng()
        .sample_iter(&Alphanumeric)
+        .map(char::from)


this whole method seems ripe for replacement with tempfile

t-nelson · 2023-08-17T16:57:14Z

sdk/src/signer/keypair.rs

@@ -25,6 +25,9 @@ use {
 pub struct Keypair(ed25519_dalek::Keypair);

 impl Keypair {
+    /// Can be used for generating a Keypair without a dependency on `rand` types


hardcode 64 and static_assertions::const_assert_eq!(SECRET_KEY_LENGTH, ed25519_dalek::SECRET_KEY_LENGTH) to keep dalek out of the api?

ledger/src/shred.rs

t-nelson · 2023-08-17T17:53:19Z

core/src/gen_keys.rs

@@ -4,7 +4,7 @@ use {
    rand::{Rng, SeedableRng},


til this module exists. think i might need a sick day 🤢

Run `git grep -l gen_range | xargs sed -i'' -e 's/gen_range(\(\S*\), /gen_range(\1../'

Run: ``` git grep -l hash::new_rand | xargs sed -i'' -e 's/hash::new_rand([^)]*/hash::new_with_thread_rng(/' ```

joncinque · 2023-08-17T22:09:04Z

ledger/src/shred/merkle.rs

+        let mut seed = [0u8; Keypair::SECRET_KEY_LENGTH];
+        rng.fill(&mut seed[..]);
+        let keypair = keypair_from_seed(&seed).unwrap();


This one isn't using chacha, so changing to Keypair::new

joncinque · 2023-08-17T22:10:19Z

ledger/src/shred/merkle.rs

+        let mut seed = [0u8; Keypair::SECRET_KEY_LENGTH];
+        rng.fill(&mut seed[..]);
+        let keypair = keypair_from_seed(&seed).unwrap();


This one is also using thread_rng, so switching to Keypair::new

joncinque · 2023-08-17T22:30:48Z

sdk/src/signer/keypair.rs

@@ -25,6 +25,9 @@ use {
 pub struct Keypair(ed25519_dalek::Keypair);

 impl Keypair {
+    /// Can be used for generating a Keypair without a dependency on `rand` types


Done in db48acc

joncinque · 2023-08-17T22:34:11Z

Ok, I think all comments have been addressed, thanks for your careful and quick reviews!

brooksprumo

Lgtm - all my concerns have been resolved. Thanks!

t-nelson · 2023-08-18T16:11:57Z

bloom/src/bloom.rs

@@ -308,9 +308,16 @@ mod test {
        );
    }

+    fn generate_random_hash() -> Hash {


lol... Hash::new_unique() is broken. it should be hashing the counter, not using it straight up 🙃

I can try changing it to that and see if the bloom filter test passes. On the flipside, and I might be understanding this wrong, but it seems like the test is meant for random hashes.

nah don't mess with it here

joncinque · 2023-08-18T16:36:35Z

Trying to resolve the additional rebuilds of solana-program by unifying features some more. Bear with me!

joncinque · 2023-08-18T18:54:05Z

I couldn't get a combination that linked successfully and also kept the rebuilds at the same number, so I had to increase from 14 to 15 rebuilds, which feels not-too-bad!

t-nelson · 2023-08-18T21:27:35Z

I couldn't get a combination that linked successfully and also kept the rebuilds at the same number, so I had to increase from 14 to 15 rebuilds, which feels not-too-bad!

presumably we're now linking something against both versions of rand, so expected?

joncinque · 2023-08-18T22:24:36Z

presumably we're now linking something against both versions of rand, so expected?

Yeah probably... all my efforts were useless since they just reintroduced the linking error, which is strange because I couldn't reproduce it on any local machine this time, whereas I could before.

Anyway, had to increase the number of rebuilds to 17 🙃

codecov · 2023-08-19T00:27:48Z

Codecov Report

Merging #32871 (7054869) into master (a4c8cc3) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is 96.7%.

@@           Coverage Diff           @@
##           master   #32871   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         785      784    -1     
  Lines      212387   212437   +50     
=======================================
+ Hits       174190   174245   +55     
+ Misses      38197    38192    -5

joncinque · 2023-08-19T00:29:18Z

It finally passed woo!

joncinque requested review from t-nelson and brooksprumo August 17, 2023 13:44

joncinque commented Aug 17, 2023

View reviewed changes

brooksprumo reviewed Aug 17, 2023

View reviewed changes

perf/src/test_tx.rs Outdated Show resolved Hide resolved

ledger/src/shred.rs Show resolved Hide resolved

sdk/src/hash.rs Outdated Show resolved Hide resolved

ledger/src/shredder.rs Outdated Show resolved Hide resolved

t-nelson reviewed Aug 17, 2023

View reviewed changes

joncinque added 23 commits August 18, 2023 00:26

sdk: Add concurrent support for rand 0.7 and 0.8

7cb815a

Update rand, rand_chacha, and getrandom versions

0821cb7

Run command to replace gen_range

4c99792

Run `git grep -l gen_range | xargs sed -i'' -e 's/gen_range(\(\S*\), /gen_range(\1../'

sdk: Fix users of older gen_range

bdb5465

Replace hash::new_rand with hash::new_with_thread_rng

2e9c550

Run: ``` git grep -l hash::new_rand | xargs sed -i'' -e 's/hash::new_rand([^)]*/hash::new_with_thread_rng(/' ```

perf: Use Keypair::new() instead of generate

9a0a8ec

Use older rand version in zk-token-sdk

5d67cfc

program-runtime: Inline random key generation

6b8f63e

bloom: Fix clippy warnings in tests

d4a6f09

streamer: Scope rng usage correctly

53d12e3

perf: Fix clippy warning

79fb8d3

accounts-db: Map to char to generate a random string

c8c1059

Remove from_secret_key_bytes, it's just keypair_from_seed

b87dd11

ledger: Generate keypairs by hand

775da77

ed25519-tests: Use new rand

cab9648

runtime: Use new rand in all tests

2c99f5f

gossip: Clean up clippy and inline keypair generators

05bb142

core: Inline keypair generation for tests

6bf9c1d

Push sbf lockfile change

f21bd05

sdk: Sort dependencies correctly

0b10db3

Remove hash::new_with_thread_rng, use Hash::new_unique()

3b2597c

Use Keypair::new where chacha isn't used

37bb533

sdk: Fix build by marking rand 0.7 optional

147c1e9

joncinque force-pushed the rand8 branch from 823149a to 147c1e9 Compare August 17, 2023 22:26

Hardcode secret key length, add static assertion

db48acc

joncinque commented Aug 17, 2023

View reviewed changes

Unify getrandom crate usage to fix linking errors

39b5adf

brooksprumo previously approved these changes Aug 18, 2023

View reviewed changes

bloom: Fix tests that require a random hash

fd77bc2

joncinque dismissed brooksprumo’s stale review via fd77bc2 August 18, 2023 15:44

t-nelson reviewed Aug 18, 2023

View reviewed changes

Remove some dependencies, try to unify others

884dc52

joncinque added 4 commits August 18, 2023 19:08

Remove unnecessary uses of rand and rand_core

2b45df5

Update lockfiles

295023a

Add back some dependencies to reduce rebuilds

7099108

Increase max rebuilds from 14 to 15

2e09e57

joncinque added 3 commits August 19, 2023 00:07

frozen-abi: Remove getrandom

9d44709

Bump rebuilds to 17

2b00f23

Remove getrandom from zk-token-proof

4718c51

Merge branch 'master' into rand8

7054869

t-nelson approved these changes Aug 21, 2023

View reviewed changes

brooksprumo mentioned this pull request Aug 21, 2023

Bump ed25519-dalek to 2.0.0 #32836

Closed

joncinque merged commit 0fe902c into solana-labs:master Aug 21, 2023

joncinque deleted the rand8 branch August 21, 2023 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump rand to 0.8, rand_chacha to 0.3, getrandom to 0.2 #32871

Bump rand to 0.8, rand_chacha to 0.3, getrandom to 0.2 #32871

joncinque commented Aug 17, 2023

joncinque Aug 17, 2023

t-nelson Aug 17, 2023

joncinque Aug 17, 2023

joncinque Aug 17, 2023

brooksprumo left a comment

t-nelson left a comment

t-nelson Aug 17, 2023

joncinque Aug 17, 2023

t-nelson Aug 17, 2023

t-nelson Aug 17, 2023

joncinque Aug 17, 2023

joncinque Aug 17, 2023

joncinque Aug 17, 2023

joncinque commented Aug 17, 2023

brooksprumo left a comment

t-nelson Aug 18, 2023

joncinque Aug 18, 2023

t-nelson Aug 18, 2023

joncinque commented Aug 18, 2023

joncinque commented Aug 18, 2023

t-nelson commented Aug 18, 2023

joncinque commented Aug 18, 2023

codecov bot commented Aug 19, 2023

joncinque commented Aug 19, 2023

Bump rand to 0.8, rand_chacha to 0.3, getrandom to 0.2 #32871

Bump rand to 0.8, rand_chacha to 0.3, getrandom to 0.2 #32871

Conversation

joncinque commented Aug 17, 2023

Problem

Summary of Changes

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

brooksprumo left a comment

Choose a reason for hiding this comment

t-nelson 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

joncinque commented Aug 17, 2023

brooksprumo 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

joncinque commented Aug 18, 2023

joncinque commented Aug 18, 2023

t-nelson commented Aug 18, 2023

joncinque commented Aug 18, 2023

codecov bot commented Aug 19, 2023

Codecov Report

joncinque commented Aug 19, 2023