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

Always use getrandom to seed RNG #86

Closed
wants to merge 1 commit into from
Closed

Conversation

cod10129
Copy link

This PR makes the default random RNG seed always use getrandom.
The original code looked like this:

fn random_seed() -> Option<u64> {
    use std::collections::hash_map::DefaultHasher;
    use std::hash::{Hash, Hasher};
    use std::thread;
    use std::time::Instant;

    let mut hasher = DefaultHasher::new();
    Instant::now().hash(&mut hasher);
    thread::current().id().hash(&mut hasher);
    Some(hasher.finish())
}

DefaultHasher doesn't actually have any randomness in it, so you're just hashing the current time and thread ID to seed RNG.
This works, but it seemed wrong to me when I saw it. There's an implementation with getrandom right there.

A few other things I noticed:
The #[cfg(target_pointer_width = "128")] at lib.rs line 622 is generating a warning, which has been failing CI since May, after the unexpected_cfgs lint was added in Rust 1.80.
Unrelated but I also really like the cat logo.

@notgull
Copy link
Member

notgull commented Jul 31, 2024

This works, but it seemed wrong to me when I saw it. There's an implementation with getrandom right there.

It doesn't really matter, this is a pseudorandom number generator, so it doesn't matter if the seed has cryptographic level strength. It's just that some seed is nice to have here. Unfortunately the thread::id() and time tricks aren't available on WASM, hence the getrandom implementation.

I would also rather keep fastrand dependency free if possible.

The #[cfg(target_pointer_width = "128")] at lib.rs line 622 is generating a warning, which has been failing CI since May, after the unexpected_cfgs lint was added in Rust 1.80.

This is tracked in #85

Unrelated but I also really like the cat logo.

Thanks! It was drawn by NebulousStar on Twitter.

@cod10129
Copy link
Author

Alright, fair points. Seeding with the time is often used, but does it also need the thread ID? Note the main thread is always ThreadId(1).

I would also rather keep fastrand dependency free if possible.

I was actually wondering if the dependency was the reason. getrandom also pulls in cfg-if and (on Unix) libc. It is a nice cargo tree to just see a self-contained fastrand crate.

@notgull
Copy link
Member

notgull commented Jul 31, 2024

Note the main thread is always ThreadId(1).

Keep in mind GLOBAL_RNG is a thread-local variable, so it will be initialized with a different thread ID every time.

I was actually wondering if the dependency was the reason. getrandom also pulls in cfg-if and (on Unix) libc. It is a nice cargo tree to just see a self-contained fastrand crate.

Agreed, this is my problem with getrandom, in addition to the fact that it pulls libc instead of rustix on Linux.

cc rust-random/getrandom#401

@cod10129
Copy link
Author

The RNG is thread-local, but two different threads aren't initializing their RNGs at the same nanosecond.
I went through the performance implications:

  • Accessing thread-local std::thread::CURRENT
  • Cloning an Arc (Thread.inner)
  • (and then accessing the thread ID from behind that Arc)

Seems negligible, since there's also hashing and Instant::now happening.
I'm thinking I'll just close this PR, unless there's anything you want to say?

@notgull
Copy link
Member

notgull commented Aug 1, 2024

The RNG is thread-local, but two different threads aren't initializing their RNGs at the same nanosecond.

This isn't guaranteed.

I'm thinking I'll just close this PR, unless there's anything you want to say?

Yeah, I'd agree with closing the PR.

@notgull notgull closed this Aug 1, 2024
@notgull
Copy link
Member

notgull commented Aug 1, 2024

Thanks anyways!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants