-
Notifications
You must be signed in to change notification settings - Fork 430
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
Add EntropySource wrapper #235
Conversation
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 actually makes NewSeeded
redundant.
src/lib.rs
Outdated
} | ||
|
||
impl EntropySource { | ||
pub fn new() -> Result<Self, Error> { |
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 think it's worth making this infallible and delaying errors so that X::from_rng(EntropySource::new())
works. Since performance isn't very important here we could potentially just try all sources each call, though that's not optimal.
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.
JitterRng
has a pretty high set-up cost, because it runs the timer for every initialization.
Do you mean to add something like a third variant to EntropySourceInner
that none are available, with the Error
?
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.
X::from_rng(EntropySource::new())
works really nice! It will take me until Wednesday or Thursday before I can update this PR.
} | ||
|
||
fn fill_bytes(&mut self, dest: &mut [u8]) { | ||
self.try_fill_bytes(dest).unwrap(); |
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.
unwrap
has to go — but sure, this is a quick implementation
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 was even thinking of just using unimplemented!()
here. The situations where both OsRng
and JitterRng
fail should be extremely rare, so I didn't care much about the unwrap. What do suggest to replace it with?
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.
Okay for now; revisit in #249
src/lib.rs
Outdated
let mut rng2_result = JitterRng::new(); | ||
if let Ok(mut rng2) = rng2_result { | ||
result = rng2.try_fill_bytes(dest); // Can't fail | ||
switch_rng = Some(EntropySourceInner::Jitter(rng2)); |
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.
Probably simpler to recurse here (i.e. call new_self.try_fill_bytes()
).
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.
It took some thinking to get the logic right and the borrow checker happy. But I'll think about it!
Um, so did you come up with this idea independently or see my comment before I finished the review? Either way I like it, plus we can leave out It might be nice to have a global generator function like |
I was surprised by you comment. We thought of the same thing at the same time 😄. |
I am partial about the name. Do you want to pick between I feel that adding an |
fd7c131
to
e32b4c9
Compare
(nothing changed yet, just a rebase to hopefully get the CI happy) |
To quote the OED:
More on topic, I prefer You are right that But having this as well, and making |
Ah, thank you. I was impartial. Now I am starting to get partial to
Yes, I don't like the extra import. But there seems to be no way around it? Without the trait it is necessary to import What did you think of the idea to add a feature flag (in the future) that allows crates to provide a different source for |
Um... I think the extra import could be avoided if Anyway, this "entropy source" will be a full I don't think a feature flag can do that; at best it could enable support for setting another source at run time. |
I also though of renaming If we make I just thought of an issue:
I was thinking of something like this (but have to learn a lot about FFI and linking first): #[cfg(feature = "extern_entropy_source")]
extern {
fn extern_fill_bytes(dest: *mut u8, length: usize) -> u32;
}
#[cfg(feature = "extern_entropy_source")]
impl Rng for EntropySource {
fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
/* call extern_fill_bytes */
}
} Linking in a crate that provides a different source would have to be done by the build script of the final crate. |
I was playing the devil's advocate; I didn't mean putting You're right, linking like that should be possible, but it's not very "Rustic". It relies on |
(I have been busy writing this comment for four days...)
One of the nicest ways to explore options 😄. What do you think of
Would this be more "Rustic"? Moving
Good point. We don't want an application that relies on a source like The second part of the puzzle I had in mind was to make Something like this had been proposed/tried before in #109, #133, #78. It is not optimal, no use of As argued in #109 (comment) this could help make crates to be available for |
If we go in that direction —
The above is quite a big redesign, but on first impressions it sounds good.
I was experimenting with making |
e32b4c9
to
7a07c36
Compare
I have rebased on top of #233 again (basically recreated the branch...). The last commit renames The logic to select an entropy source now is something like this:
Maybe I just made things to difficult, but couldn't quickly see a better way. Will you have another look? I would like to keep future directions for |
The logic in the comment above sounds reasonable to me. I didn't look at the commits themselves; the order seems to be mixed up with commits from #233? Can you do |
I am not sure what is happening with the order of the commits, locally and on github (https://github.com/pitdicker/rand/commits/entropy_source) they aren't mixed. I did not change the commits you already reviewed, only added one extra. |
That's odd. Anyway, your new approach looks fine, although it doesn't check the error kind. If If |
Good point about checking the error kind, I only thought about it partially. I am also not sure using a counter is best. The overhead of the extra system call is only 2%. Especially when doing larger reads with I just realized I will add a description of the fall-back strategy to the documentation of |
Performance isn't critical here, so using
|
Yes, that could work. And every time we retry, we get the reason for failing again from the OS, so we can reset the counter. Something like this? EntropySource::Jitter(ref mut rng) => {
if self.counter == 0 {
match try_os_new(dest) {
Ok(os_rng) => {
switch_rng = Some(EntropySource::Os(os_rng));
}
Err(e) => {
if e.kind() == ErrorKind::Unavailable {
self.counter = 8; // Wait 8 calls before retry
} else {
self.counter = 0; // Retry on next call
}
return rng.try_fill_bytes(dest); // use JitterRng
}
}
} else {
self.counter -= 1;
return rng.try_fill_bytes(dest); // use JitterRng
}
} |
Yes. Although if we only wait 8 calls I wonder if it's worth using a counter at all; I was thinking a much higher count, but there's probably no reason to do so. I guess given how slow |
I don't think using a counter or not matters much. If we set the counter to a high value, and If we set the counter to a low value, and I'll leave it to you: keep things as they are now, remove the counter, or increase it? |
Good argument. So I guess I'll let you decide, while I take another look at #233. |
Looks like this needs a rebase |
41fc616
to
b6339f5
Compare
Rebased and removed the counter |
Okay, I made my own commit to replace usage of
to:
With that change I'm happy to merge this (although I did notice |
Thinking about it some more, the above messages are still not ideal: we never show errors from I keep wondering whether we want a single shared state for these RNGs (i.e. accessed via a function) or not; so far it doesn't appear necessary for |
I think you need something like a static I guess this brings us back to this discussion too. |
I would like to make the changes to |
No, it shouldn't hold this up. Sorry, I'll try to get this merged. |
I tweaked this and merged with master, but it needs a bit more work really. Feel free to rebase, or I might try reworking this a bit more. I'm not sure about the error handling; e.g. if reseeding failed it might be permissible to continue without reseeding? |
Thank you. I wanted to say no need to hurry, it is not as if I was able to do much last week.
Is there a way to see what changes you made? |
Sorry, I've been using kdiff3 to do the merges, and apparently trusting it too much. |
I found a nice way to reduce the size of |
Why did you close — are you going to start fresh with a new PR? |
Yes, I will make a fresh PR. I did not realize re-using this one was even an option... |
You can force-push and re-open if you like — depends whether you want to keep these comments in the same thread really. |
b6339f5
to
69eee53
Compare
Apologies, I understood things wrong and blamed you and merging. I rebased and reopened this PR. Also added an extra commit with logging for And it turned out reopening a PR after a force-push is difficult. |
I'm still not quite sure we've got this right, though re-reading the comment thread the design has evolved significantly since we started! The Since the current code tries to use The logging doesn't seem quite right; currently error details should be printed via Since the above issues are very minor and we've been waiting for this a while, I'll go ahead and merge, and leave #249 and #251 to follow up. |
This is just an idea I am trying. The PR is based on top of #233, only the last 3 commits are new.
I have added an
EntropySource
wrapper that usesOsRng
, and if it errors falls back toJitterRng
. I hope this will help in three situations:NewSeeded
can also be used by user code.Reseeder
trait, providing an RNG for reseeding is enough.EntropySource
use an external RNG. This will make it andNewSeeded
available forno_std
uses.