Should there be a fill_bytes method in addition to try_fill?

[pitdicker]

Relevant part of the RFC:

Regarding fill_bytes / try_fill, the little benchmarking done shows no
performance impact of returning a Result. Handling a Result involves some
additional code complexity, but since this function is mostly of interest to
cryptographic code wanting a sequence of bytes, and these users are the ones
requesting error handling, this extra code seems reasonable. It is slightly
unfortunate that any code using try_fill on an infallible PRNG must still do
error handling or use unwrap, but this is probably not a big deal. Therefore
I believe a try_fill(..) -> Result<(), Error> function is sufficient and an
infallible fill_bytes is not needed.

[pitdicker]

I wrote a comment about why I believe cryptographic code wants try_fill, but never posted it:

PRNGs are generally infallible. In my comment about OsRng I found out it is possible to never get an error wile calling fill_bytes. At least there are no documented errors. All the reasons it may fail can be covered in OsRng::new.

ring choose a different design. The new operation should be entirely zero-cost. All the logic and the possibility to error is contained in (its equivalent of) fill_bytes. Even /dev/random is only opened on first read, on the systems that use that interface. Next the file descriptor is stored in a static.

I suspect such a design to work well for ring. Well, I imagine that calls to get a block of random numbers in it do not really happen in inner loops, but more like once per function. Then it does not really matter if you handle the error when initializing, or while reading.

rand should be more flexible. Errors can be handled during initialization. fill_bytes can safely be used in inner loops etc without having to worry about error handling. Of course this all does not matter if the function just try!'s. Note also that both designs can be implemented in terms of the other.

[pitdicker]

Personally I would like to see both a fallible and infallible version try_fill and fill_bytes. Because RNGs that can error are rare, an infallible variant is more egnomomic.

One extra method does not cost us much. PRNG implementations are slightly simpler. Wrappers would have to wrap one extra method. And fallible RNGs would need one extra method that unwraps try_fill. It may even cause fallible RNGs to think a bit more about how to handle errors. Hereby I assume try_fill has a default implementation in terms of fill_bytes:

    fn try_fill(&mut self, dest: &mut [u8]) -> Result<(), Error> {
        Ok(self.fill_bytes(dest))
    }

For me, anything that returns a Result causes me to think about how it should be dealt with. Is unwrapping and panicing ok? Should the error be passed on? Or is it something that can be handled somehow? So while there is no performance overhead, there is a little mental overhead. But this is not a point I want to push to far.

Performance is not a real problem as the RFC notes, as any (external) RNG that can error has enough overhead to it for the unwrap not to matter. And PRNGs should be inlinable.

Should we also have try_next_u32 and try_next_u64? The RFC notes that for these functions the overhead can be higher. It also notes that returning a Result "is most applicable to fill_bytes since external generators commonly provide byte streams ([u8])." So a RNG implementation that can both return integers efficiently and that is fallible, should be rare. Then it seems to me these methods do not have to be part of the trait. A RNG can just implement these methods itself when there is a need.

[dhardy]

Well reasoned argument. Having both try_fill and fill_bytes does allow more consistent usage.

On the other hand, adding try_next_* feels like its going too far (insufficient incentive given the extra lines of code to maintain).

[pitdicker]

Thank you. Yes, I am not proposing try_next_*. Only for something like RDRAND it makes sense, and then the could just be separate methods.

[pitdicker]

Part of the RFC about default implementations:

I argue that the best option is to provide no default implementations, for the following reasons:

• It makes the traits themselves as simple as possible
• It makes the correct way to implement Rng/CryptoRng obvious (no questions about which default implementations are adequate)
• It forces any wrapper implementation to wrap each function explicitly; since throwing away bits is allowed (next_u64() as u32 or fill_bytes implemented via next_u*), a wrapper using a default implementation may output different results (see here and here for more)

Unfortunately this does mean that any implementation of Rng must include several annoying trivial functions (casting or wrapping some provided implementation), but there should not end up being a huge number of users needing to implement Rng (relative to users of Rng).

I completely agree about no default implementations for next_u32, next_u64 and fill_bytes.

An Rng implementation generally has an u8, u32 or u64 as its default output, and the others are derived from that one. There is no default implementation that fits well.

I didn't think about wrappers, and that is especially a strong argument against default implementations.

Do the same arguments apply for a default implementation of try_fill as Ok(fill_bytes(())?

• There is only one obvious default implementation, so the first two do not apply.
• A wrapper will not output different results, but it will panic instead of passing on an error.

I do not like it, but with the wrapper argument try_fill should not have a default implementation.
Because errors are exceptionally rare for RNGs, bad error handling could take a long time to come to light.

And as the RFC notes, no default implementations is not a big problem. There are not all that many implementors of Rng.

[dhardy]

This is merged into upstream master and appears to be well received