-
Notifications
You must be signed in to change notification settings - Fork 432
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
Universal seeder #554
Universal seeder #554
Conversation
Small issue: this requires |
This proves there are no stupid mistakes, I guess. No surprises really:
|
In general I think this looks great. Two very minor comments that I don't feel strongly about. Could we use a more discoverable name, like Syntax like I haven't reviewed in detail, but happy to do so. |
I originally named it Using an extension trait is technically possible but not my preferred syntax. |
Updated. We don't need to use turbo-fish syntax:
This is possible with an extension trait (like I'm also wondering whether we should re-think |
Still needs a fix for Rust < 1.26 (i128); wait on #571 |
I think converting
#571 was merged. From the code: /// Although the SipHash algorithm is considered to be generally strong,
/// it is not intended for cryptographic purposes. As such, all
/// cryptographic uses of this implementation are _strongly discouraged_. Maybe we want to use a stronger hash algorithm? However, you discourage from using this method of seeding for cryptographic purposes anyway. (Seeding with a low-entropy password might require an expensive key-derivation function, making it unsuitable for |
0x8f6092dd2692af57, 0xbdf362ab8e29260b]; | ||
// for _ in 0..8 { | ||
// println!("0x{:x}, 0x{:x},", rng.next_u64(), rng.next_u64()); | ||
// } |
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.
You can probably uncomment this code. The output will only be shown if the test fails.
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.
In theory it makes the tests slower to run.
|
||
Sip24Rounds::c_rounds(&mut self.state); | ||
|
||
self.state.v0 ^ self.state.v1 ^ self.state.v2 ^ self.state.v3 |
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.
Your comment above refers to this code, right? What is the motivation for this construction?
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.
XORing the state together? No, I guess you mean using two rounds here?
Well, the default construction of SIP hash uses two rounds between each input consumed and four rounds after consuming all input (although this is configurable, and the authors invite others to try attacking the hash function with different constructions, even with zero rounds at the end).
Using two rounds here, and two rounds when switching from input consuming to and output phases, mirrors the input.
I'm not qualified to say anything about the security, but it mirrors the input design and seems a reasonable construction. I was wondering about asking the SIP hash authors for their input on 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.
I should add that all the variants of SIP hash mutate the state somehow between each set of rounds, in addition to mutation by input. Mutating the adjustor (adj
) is my invention to add unique input each time; as I understand the bit propagation of the "rounds" is perhaps good enough alone, but I believe this increases the strength vs any kind of analysis.
Of course, making SIP rng stronger vs crytoanalysis is beyond our requirements, but still seems prudent to do what's simple.
// This is supposed to be d - c rounds (here 4 - 2 = 2) | ||
S::c_rounds(&mut state); | ||
|
||
SipRng::from_state(state) |
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.
Where does this come from and how does it relate to the original algorithm?
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.
This is the original: https://github.com/rust-lang/rust/blob/master/src/libcore/hash/sip.rs#L334
It looks like I missed the tweak to state.v2
before the end rounds — though the state is tweaked before the next two rounds anyway. Might be better with an extra tweak then but not too important probably.
I originally tried to make the first output match the initial one (or the first two from the 128-bit output version), but this wasn't so easy to turn into a nice RNG (because one version mutates two different parts of the state, and the other doesn't mutate any state between these four rounds).
If this is to replace #537, I think we should add some prominent examples in Rand's documentation for the |
#537 ( I still think this PR is interesting but not a priority, so postponing for now. If anyone wants to take this further, you could start by reviewing this PR. |
Note: with four |
It doesn't make sense to merge this new crate into this repo; we are already discussing removing other content to reduce the size of this repo (#885). This code is also not a core part of the Rand project. However, this code may have some use for somebody and seems to be in reasonable shape. Therefore it seems sensible to create a new |
This now lives at https://github.com/rust-random/seeder |
This is much more powerful than
seed_from_u64
(#537).SipRng
codeSipHash is a keyed hash function optimised for speed on short messages. I adapted this to support unlimited length output with
SipRng
.Quality should be roughly crypto-grade, though I haven't attempted any kind of crypto-analysis and would not like to bet on the strength without at least some further review.
Unsurprisingly, PractRand hasn't picked up any issues (two "unusual" items under 64 GiB on this run, none up to 128 GiB with a slightly different construction).
Performance is reasonable though of course not close to fast RNGs (I didn't bother using caching for
next_u32
here; for seeding at least it's not useful):For the intended usage, seeding other RNGs, this performance is perfectly adequate.
Note that the
Seeder
type is mostly for convenience. We could just recommend usingfrom_rng
, except that this method is currently documented as not being value-stable (which thus defeats the whole point of this code).I may still make some tweaks to this, but so far it looks quite nice to me. Comments welcome.