Implement `BlockRngCore` ISAAC and ISAAC-64 #325

pitdicker · 2018-03-22T15:37:44Z

Updated description:

This is the part I left out of #281.

This PR implements BlockRngCore for ISAAC and ISAAC-64. This meant changes in both rand and rand_core.
rand_core:

Added a BlockRng64 wrapper, because BlockRng only works for u32. The implementation is mostly just of copy of the one in Isaac64Rng. Only the fill_bytes part uses the same direct filling trick as BlockRng. This does not have any advantage at all for Isaac64Rng, because it generates 2kb at a time, and the copy cost is negligible compared to the time that takes. But as this is supposed to be a generic wrapper, it seems to make sense.
Added the serde-1 feature to derive Serialize and Deserialize for the BlockRng wrappers.

rand:

Added a generic IsaacArray wrapper to support AsRef and other traits on the BlockRng::Results type of Isaac{64}Rng.
Renamed isaac_serde to isaac_array, because both deal with the problem of bad support for large arrays.
The code from IsaacRng and Isaac64Rng is as much as possible just a refactor of existing functionality. I believe the only small intended change is that the results type does not use Wrapping<T>.

First I thought this PR had problems that would take me forever to figure out, but thanks to help from @dtolnay they vanished overnight 😄. Now I think it would be very nice to get this in the 0.5 release, for three reasons:

Serialization support for BlockRng. We recommend RNGs to implement Serde, then we should also make it possible to do so.
This is a breaking change to one of our new features: serialization of Isaac{64}Rng.
This finishes the BlockRngCore PR by implementing it for ISAAC (making it possible to also use it with ReseedingRng etc.).

Original post:

This is the part I left out of #281.

There are a few things which make implementing BlockRngCore not straight-forward.

Added a BlockRng64 wrapper, because BlockRng only works for u32.
Asref() is not implemented for arrays with more than 32 elements. This adds an IsaacArray as work-around.
I am not sure how to re-implement serialisation. Do we want to make BlockRng in rand-core serialisable? I did some effort, but implementing serialize, and especially deserialize, by hand becomes ugly quickly. cc @LinearZoetrope

So this is not yet mergeable, but I am hoping for some input 😄.

dhardy · 2018-03-22T15:42:41Z

rand-core/src/impls.rs

+///
+/// This is similar to [`BlockRng`], but specialized for algorithms that operate
+/// on `u64` values (rare). Hopefully specialization will one day allow
+/// `BlockRng` to support both, so that this wrapper can be removed.


The "rare" part may change... I also don't know that it's worth talking about this future direction here. Also since all the implementations are different, is there any point making BlockRng support both?

I'll remove the speculation...

The idea was once that things like ReseedingRng can then be used for both 32- and 64-bit RNGs.

Which is a problem in itself. I wonder whether many people will complain about the new ReseedingRng being incompatible with many PRNGs... maybe not: I'm struggling to find any references which are not simply copies of the source

I found a grand total of two:

Gotham

Servo

The servo one is little more than a reshuffle of Rand code, so more like one, and that's reseeding ChaCha, so we're fine.

vks · 2018-03-23T14:10:08Z

Is ISAAC going to be deprecated over HC128?

pitdicker · 2018-03-23T14:11:33Z

In my opinion it already is, with StdRng and ThreadRng having switched.

vks · 2018-03-23T14:16:47Z

Well, at least it's not yet official according to the documentation.

pitdicker · 2018-03-23T14:22:00Z

Good point, I'll make a reference to HC-128 in the documentation

UserAB1236872 · 2018-03-24T01:32:46Z

Have you tried just deriving Serialize and Deserialize on Block Core? It should automagically only derive where R and R::Results are Ser/De. Then any types using this that we want to enable serde on just require their own derive.

pitdicker · 2018-03-25T15:15:32Z

@LinearZoetrope I have pushed my (unsuccessful) attempt to derive Serialize and Deserialize on BlockRng. It did not work 'automagically' enough... It fails with (repeated 3 times):

error[E0277]: the trait bound `<R as BlockRngCore>::Results: serde::Serialize` is not satisfied
   --> rand_core/src/impls.rs:187:38
    |
187 | #[cfg_attr(feature="serde-1", derive(Serialize,Deserialize))]
    |                                      ^^^^^^^^^ the trait `serde::Serialize` is not implemented for `<R as BlockRngCore>::Results`
    |
    = help: consider adding a `where <R as BlockRngCore>::Results: serde::Serialize` bound
    = note: required by `serde::ser::SerializeStruct::serialize_field`

error[E0277]: the trait bound `<R as BlockRngCore>::Results: serde::Deserialize<'_>` is not satisfied
   --> rand_core/src/impls.rs:187:48
    |
187 | #[cfg_attr(feature="serde-1", derive(Serialize,Deserialize))]
    |                                                ^^^^^^^^^^^ the trait `serde::Deserialize<'_>` is not implemented for `<R as BlockRngCore>::Results`
    |
    = help: consider adding a `where <R as BlockRngCore>::Results: serde::Deserialize<'_>` bound
    = note: required by `serde::de::SeqAccess::next_element`

Are you interested in taking a look?

pitdicker · 2018-04-03T06:54:29Z

🎉 Finally have Serde re-implemented and a green bill from the CI. Updated the description in the first post.

(and relative links are really inconsistent)

dtolnay · 2018-04-03T10:12:02Z

src/prng/isaac_array.rs

+#[derive(Copy, Clone)]
+#[allow(missing_debug_implementations)]
+#[cfg_attr(feature="serde-1", derive(Serialize, Deserialize))]
+pub struct IsaacArray<T> where T: Default + Copy {


Default and Copy should never be used in trait bounds on a data structure -- only on impls and methods. The only code that requires these bounds are the Deserialize and Default impls, so the bounds should be on those impls.

Thank you for reviewing, I hope I got them right now. It compiles...

dtolnay · 2018-04-03T10:13:23Z

rand_core/src/lib.rs

@@ -44,6 +44,8 @@

 #[cfg(feature="std")] extern crate core;
 #[cfg(all(feature = "alloc", not(feature="std")))] extern crate alloc;
+#[cfg(feature="serde-1")] extern crate serde;


I have a weak preference for serde1 as the name of the Serde feature instead of serde-1, to be consistent with crates that enable optional Serde support through an optional dependency on the serde1 shim.

From what I see serde-1 seems to be used by a couple of crates to add the optional dependency. serde1 does not seem to have any reverse dependencies?

I don't like serde-1, and serde1 also does not seem ideal compared to just serde.
For rand_core, we could name the feature just serde with:
serde = { version = "1", optional = true, features = ["derive"] }.

For rand I see the serde feature as temporary, until we split of the PRNGs into a separate crate. There seems to be some movement in rust-lang/cargo#1286.

@dhardy What do you think? Does it make sense to use serde as the feature name in rand_core, and at some point in the future both crates can use that name?

Well if serde never has another breaking release we don't need to append any number, but since it had quite a few before 1.0 appending a number seemed prudent. If @dtolnay prefers serde1 that is fine, except that it's a breaking change (we added the serde-1 feature for the Rand 0.4 release).

except that it's a breaking change (we added the serde-1 feature for the Rand 0.4 release).

I don't think we did? https://github.com/rust-lang-nursery/rand/blob/0.4/Cargo.toml

Okay, then we aren't constrained and can use serde1 as suggested.

dhardy

What do you think about making the public fields private? I think the only accessors actually needed should be to core and a "reset" function (for ChaCha::set_counter).

dhardy · 2018-04-04T09:08:05Z

rand_core/src/impls.rs

+        serialize = "R::Results: Serialize",
+        deserialize = "R::Results: Deserialize<'de>")))]
+    pub results: R::Results,
+    pub index: usize,


Comment that 0 is never stored? next_u32 relies on this.

dhardy · 2018-04-04T09:09:50Z

rand_core/src/impls.rs

+    #[cfg_attr(feature="serde-1", serde(bound(
+        serialize = "R::Results: Serialize",
+        deserialize = "R::Results: Deserialize<'de>")))]
+    pub results: R::Results,


Public fields should be documented — but I think perhaps we should not make these public (just add necessary accessor fns and a "reset" function).

But than I have to do things properly... But it really cleans things up, so good idea.

dhardy · 2018-04-04T09:15:30Z

rand_core/src/impls.rs

+        deserialize = "R::Results: Deserialize<'de>")))]
+    pub results: R::Results,
+    pub index: usize,
+    pub half_used: bool, // true if only half of the previous result is used


I think fill_bytes should reset half_used = false ? In fact I think the x86 version could also use the left-over bytes easily (because LE means the bytes are contiguous); the other version could also but would need an extra copy.

Good catch. I think it only happened to pass the tests because it then it generates a new set of results in-between, which resets half_used.

I don't like to change the logic at the moment t.b.h.

I don't think tests could catch this anyway; there's no memory unsafety, just the possibility of skipping one word of output and using another twice.

Oops, I though we tested for it in test_isaac64_true_values_mixed, but that tests only mixing next_u64 and next_u32.

dhardy · 2018-04-04T09:21:28Z

rand_core/src/lib.rs

@@ -44,6 +44,8 @@

 #[cfg(feature="std")] extern crate core;
 #[cfg(all(feature = "alloc", not(feature="std")))] extern crate alloc;
+#[cfg(feature="serde-1")] extern crate serde;


Okay, then we aren't constrained and can use serde1 as suggested.

dhardy · 2018-04-04T16:08:06Z

rand_core/src/impls.rs

+
+    /// Return a mutable reference the wrapped `BlockRngCore`.
+    pub fn inner(&mut self) -> &mut R {
+        &mut self.core


Really we should have mutable and immutable accessors, i.e. fn inner(&self) -> &R and fn inner_mut.

I'm surprised you didn't call this core though (you can shadow the name).

I did initially, but I then thought this would make more sense for users, who don't see the field name.

I just thought it aligned well with BlockRngCore but doesn't matter.

dhardy · 2018-04-04T16:09:48Z

rand_core/src/impls.rs

@@ -424,6 +463,7 @@ where <R as BlockRngCore>::Results: AsRef<[u64]>
    #[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
    fn fill_bytes(&mut self, dest: &mut [u8]) {
        let mut filled = 0;
+        self.half_used = false;


You need this in the other implementation of fill_bytes too!

dhardy · 2018-04-04T16:15:29Z

src/prng/isaac64.rs

-            index: RAND_SIZE, // generate on first use
-            half_used: false,
-        })
+        Isaac64Rng(BlockRng64::new(Isaac64Core::new_from_u64(seed)))


You don't need the separate Isaac64Core::new_from_u64 function, but if you do want it you don't need key on the line above.

Sloppy work... I am sorry. Modified the commit.

dhardy · 2018-04-04T16:53:36Z

Looks good 👍

Implement `BlockRngCore` ISAAC and ISAAC-64

dhardy reviewed Mar 22, 2018

View reviewed changes

pitdicker added the T-RNG label Mar 22, 2018

pitdicker force-pushed the isaac_blockrng branch from c43e348 to 6912a12 Compare March 25, 2018 15:11

pitdicker mentioned this pull request Mar 29, 2018

SeedableRng cannot be implemented for large RNGs using the recommended seed type #354

Closed

Implement BlockRngCore for ISAAC

602ff6f

pitdicker force-pushed the isaac_blockrng branch from 6912a12 to c48e4cc Compare April 2, 2018 18:27

pitdicker mentioned this pull request Apr 2, 2018

Can you help me figure out how to add serde to this struct? serde-rs/serde#1207

Closed

pitdicker force-pushed the isaac_blockrng branch 4 times, most recently from 23d85d5 to 8a2deb0 Compare April 3, 2018 06:39

pitdicker added 3 commits April 3, 2018 08:41

Add BlockRng64 wrapper

dc00192

Implement BlockRngCore for ISAAC-64

84f6ae9

Add serde to rand_core::impl::{BlockRng, BlockRng64}

610e975

pitdicker force-pushed the isaac_blockrng branch from 8a2deb0 to 58fd61d Compare April 3, 2018 06:42

Add back serde to IsaacRng and Isaac64Rng

94fbf76

pitdicker force-pushed the isaac_blockrng branch from 58fd61d to 4d09662 Compare April 3, 2018 07:40

Make isaac and isaac64 modules public

bbd1b38

(and relative links are really inconsistent)

pitdicker force-pushed the isaac_blockrng branch from 4d09662 to bbd1b38 Compare April 3, 2018 07:45

dtolnay reviewed Apr 3, 2018

View reviewed changes

Don't put bounds on a data structure

c5ffc61

dhardy reviewed Apr 4, 2018

View reviewed changes

pitdicker force-pushed the isaac_blockrng branch from a2f85ec to 1d6cd6c Compare April 4, 2018 15:35

pitdicker force-pushed the isaac_blockrng branch from 1d6cd6c to 35ddc87 Compare April 4, 2018 16:11

dhardy reviewed Apr 4, 2018

View reviewed changes

pitdicker added 2 commits April 4, 2018 18:25

Make BlockRng{64} fields private

28238e0

Rename serde-1 to serde1

f9f0ea6

pitdicker force-pushed the isaac_blockrng branch from 35ddc87 to f9f0ea6 Compare April 4, 2018 16:27

pitdicker merged commit baac1ca into rust-random:master Apr 4, 2018

pitdicker deleted the isaac_blockrng branch April 4, 2018 16:55

pitdicker added a commit that referenced this pull request Apr 4, 2018

Merge pull request #325 from pitdicker/isaac_blockrng

73ea10f

Implement `BlockRngCore` ISAAC and ISAAC-64

pitdicker mentioned this pull request Apr 11, 2018

Use the derive feature of serde in rand_core #387

Closed

pitdicker mentioned this pull request Apr 29, 2018

Rand core changes #419

Merged

Implement BlockRngCore ISAAC and ISAAC-64 #325

Implement BlockRngCore ISAAC and ISAAC-64 #325

Conversation

pitdicker commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vks commented Mar 23, 2018

pitdicker commented Mar 23, 2018

vks commented Mar 23, 2018

pitdicker commented Mar 23, 2018

UserAB1236872 commented Mar 24, 2018 • edited Loading

pitdicker commented Mar 25, 2018

pitdicker commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhardy commented Apr 4, 2018

Implement `BlockRngCore` ISAAC and ISAAC-64 #325

Implement `BlockRngCore` ISAAC and ISAAC-64 #325

pitdicker commented Mar 22, 2018 •

edited

Loading

UserAB1236872 commented Mar 24, 2018 •

edited

Loading