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

serde for BlockRng, ReseedingRng and ReadRng #1130

Merged
merged 4 commits into from Jun 7, 2021
Merged

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented May 27, 2021

Closes #1101, which also discussed serde for other RNGs.

  • implements for ReadRng: probably useless?
  • implements for ReseedingRng: probably useless since the reseeder will normally be a non-deterministic RNG
  • corrects bounds for BlockRng: required for ReseedingRng and I guess was never picked up in previous tests?
  • rand/serde1 now requires rand_core/serde1, required for ReseedingRng and probably sensible
  • does not implement for OsRng: useless, and easy to skip as a field in structs
  • does not implement for StdRng or SmallRng: as @kazcw points out, non-stable serialisation would be a pit-fall
  • does not implement for Hc128Rng: would require https://crates.io/crates/serde-big-array or some workaround until some future MSRV bump but I doubt anyone cares anyway

I'm tempted to remove the ReadRng and possibly ReseedingRng support but there doesn't seem a good reason either way. Thoughts?

@vks
Copy link
Collaborator

vks commented May 27, 2021

I don't have an opinion on ReadRng, because I don't know what it is used for. If it is used on files, serialization makes sense, but why would anyone do that? For /dev/urandom, serialization does not make sense, but I don't see a point of doing this over using OsRng. To be honest, I don't see a convincing use case for ReadRng and would be tempted to deprecate it.

I think ReseedingRng almost always makes the RNG non-deterministic, so I don't think serialization makes sense?

I agree that the implementation for BlockRng should be added.

@dhardy
Copy link
Member Author

dhardy commented Jun 7, 2021

I think ReadRng is just a left-over component of the old OsRng. I couldn't find any actual usage. It's now deprecated as suggested.

Review?

Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

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

Looks great! Maybe you could also update the changelog?

@dhardy
Copy link
Member Author

dhardy commented Jun 7, 2021

I'll get to the changelog later. Once current PRs are merged I think we should do the next patch release (don't think there's any breaking changes).

@dhardy dhardy merged commit 09d3df3 into rust-random:master Jun 7, 2021
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.

Implement serde::{Serialize, Deserialize} for ChaCha*Rng
2 participants