Isaac refactoring #4

pitdicker · 2017-09-07T08:35:18Z

You already said you are not likely to merge any ISAAC+ modifications. But I still give it a try, because most changes should be useful.

The first commit is just refactoring.
I have replaced some macro's by functions. This appearently made it possible to remove the unsafe indexing without a performance loss.

Over time the implementation of ISAAC and ISAAC-64 have diverged a bit, now they are as similar as possible (for easier comparison). IsaacRng now does 34% better in the benchmark :-)!

I have tested the code against the previous implementation, and the reference implementation.

The second commit does the modifications for ISAAC+. I can easily remove it if you want. But the problem is, that I do like to sometimes contribute to an existing project, but don't want to commit to a (small) library of myself.

The ISAAC+ algorithm is described in the paper On the pseudo-random generator ISAAC by Jean-Philippe Aumasson. The specification is however not complete. The definition of f'(a,i) is missing. But the text says that in that step we should "perform rotations instead of shifts, so as to get more diffusion from the state bits". So f'(a,i) is f(a,i) with shifts replaced by rotations.

It mentions two other changes: in the rngstep function one addition should be replaced by a XOR. And there should be one extra XOR with a.

Finally the algorithm in the paper shows that in the ind function we should not do shifts, but rotations. But performing a rotation-right of 2 or 10 on an u32, and than only using the last 8 bits, is no different than a shift right. I suppose it makes it easier to describe the new algorithm as 'all shifts are replaced by rotations'.

I found one other (correct) implementation of ISAAC+: konovod/prngs@749c33b

dhardy · 2017-09-07T08:51:38Z

But the problem is, that I do like to sometimes contribute to an existing project, but don't want to commit to a (small) library of myself.

Ok, fair point that you don't want an extra library to maintain. Possibly I will merge then; it remains to be decided exactly what happens with the RNG implementations (several people would like them all moved to separate crates).

What's your source for the sequences in the "true values" tests?

I think ISAAC+ should be implemented via new types, not modifying the existing ones (first, because there's very little literature review of the algorithm and none of the code yet, so many people will be wary of it and won't want it forced on them; second, because people explicitly using the existing impls might actually want reproducible results).

I will try to review this; links to any external resources used would be great.

pitdicker · 2017-09-07T12:55:42Z

it remains to be decided exactly what happens with the RNG implementations (several people would like them all moved to separate crates).

I think it would be a good idea to keep a number of good-quality RNG's together in a standard crate. Because otherwise well-known RNG's could be used a lot, while they may not be the best idea. But that is more a discussion for RFC thread.

What's your source for the sequences in the "true values" tests?

I have to admit, this are just the results of this implementation. I don't really see a way to get them from somewhere else. The only other good implementation I could find is not in C, so it is not easy to hook it up with the seeds here.

I think ISAAC+ should be implemented via new types, not modifying the existing ones

I think that is reasonable. One thing that made me unsure about adding it as a new type, was that it duplicates a lot of code. You now not only make sure IsaacRng and Isaac64Rng remain in sync, but also two other types. But maybe just a comment at the top of the file is enough.

I will try to review this; links to any external resources used would be great.

I was thinking about writing some large comments, describing

the algorithm,
choices made for optimization
differences between ISAAC, ISAAC-64 and ISAAC+
If it would make things easier to review, just say so and I'll add it (in a few days...)

Most useful for me where the following links:
http://burtleburtle.net/bob/rand/isaac.html#ISAAC (description of algorithm and optimisations)
http://burtleburtle.net/bob/rand/isaac.html#Introduction (notes about the different constants in ISAAC-64)
http://burtleburtle.net/bob/c/readable.c (ISAAC reference implementation)
http://burtleburtle.net/bob/c/isaac64.c (ISAAC-64 reference implementation)
http://eprint.iacr.org/2006/438 (Paper with good description of ISAAC algorithm, weaknesses, and ISAAC+)
https://ccodearchive.net/info/isaac.html (good collection of notes / history)
https://github.com/moschles/isaac-gen/wiki (discussing whether ISAAC is secure enough for cryptography)

dhardy · 2017-09-07T13:49:58Z

I have to admit, this are just the results of this implementation.

That's still much better than nothing, but please write comment in the code. I'll try comparing results with the Crystal impl when I get to this (I'm more focussed on the core traits for now).

Thanks for the links; again, please put some of those in as comments. I'm mainly thinking of anyone looking at the code in the future; it's not always easy finding PRs like this one (especially since this will likely get rebased and moved to a different repo at some point).

pitdicker · 2017-09-08T05:19:37Z

You are doing good work designing the core traits 👍 .
It is not something I feel ready to contribute to...

I have changed my thoughts about ISAAC+. It may be okay, but there are just to many little problems, as I've written in rust-lang/rfcs#2106 (comment).

I will add more comments to the normal code.

For into `isaac` and `isaac64` for easier comparison. Split of `isaac_word` to generate correct the documentation (TODO). Nothing is changed here, just split in three files.

This removes the unsafe blocks from the main isaac and isaac64 routines. Some macro's are replaced by functions. Over time the implementation of ISAAC and ISAAC-64 have diverged a bit, now they are as similar as possible (for easier comparison). The code is tested against the previous implementation, and the reference implementation. IsaacRng now does 34% better in the benchmark.

pitdicker · 2017-09-11T20:39:51Z

I have added a little comments, and better documentation.

Also I have split isaac.rs into three files. While working on the code, I found it helpful to have ISAAC and ISAAC64 side-by-side to see the differences. And if it was useful to may, maybe it also is for others in the future.

I split IsaacWordRng to a different file, and changed it to not be a simple re-export. This helps for documentation. I just noticed that part does not work yet, but the rest should be ready for review.

pitdicker · 2017-09-12T06:14:47Z

IsaacWordRng should now be ok. I made it a newtype around IsaacRng or Isaac64Rng. This way we can clearly explain its purpose in the documentation. Otherwise it would often have the same documentation as Isaac64Rng, is the platform most documentation is generated on.

pitdicker · 2017-09-12T06:24:37Z

src/prng/isaac64.rs

+        let mut m2 = MIDPOINT;
+        for i in (0..MIDPOINT/4).map(|i| i * 4) {
+            rngstep(self, !(a ^ (a << 21)), &mut a, &mut b, i + 0, m, m2);
+            rngstep(self,   a ^ (a >> 5 ),  &mut a, &mut b, i + 1, m, m2);


Here the mix value is completely calculated, instead of passing only a shift to the rngstep* macro like the previous code.
For ISAAC this worked ok, although it needed two different macros for the shift right and shift left. But for ISAAC-64 it was starting to make the code harder to read, because the first step should also invert the mix.

pitdicker · 2017-09-12T06:29:01Z

src/prng/isaac64.rs

+            *a = mix + ctx.mem[base + m2];
+            let y = *a + *b + ind(&ctx.mem, x, 3);
+            ctx.mem[base + m] = y;
+            *b = x + ind(&ctx.mem, y, 3 + RAND_SIZE_LEN);


In the previous code it was less clear how large the shift was. ind!() in ISAAC was always shifting by 2, and in ISAAC-64 by 3. A further shift was applied by the calling code when calculating b. Now we just always pass the amount to shift to ind().

dhardy

That's a lot cleaner and better documented, thanks @pitdicker!

Any chance you could extend the "true value" tests to go further in the future? I think right now they only test the output of the first buffer fill. It doesn't need to test all intermediate values, so doesn't need a huge block of numbers in the code.

dhardy · 2017-09-12T07:29:08Z

src/prng/isaac.rs

+///
+/// Where fast random numbers are needed which should still be secure, but where
+/// speed is more important than absolute (cryptographic) security (e.g. to
+/// initialise hashes in the std library), a generator like ISAAC should be


should is very assertive language. Unless you're sure about this it might be better to write something like "may be a good choice".

I have copied this line from your RFC ;-)

dhardy · 2017-09-12T07:31:24Z

src/prng/isaac.rs

+///
+/// In 2006 an improvement to ISAAC was suggested by Jean-Philippe Aumasson,
+/// named ISAAC+[3]. But because the specification is not complete, there is no
+/// good implementation, and because the suggested bias may not exist, it is not


reference implementation
There's also the problem of lack of third-party review.

Yes, true. But I tried to not say too much about an algorithm we don't implement.

dhardy · 2017-09-12T07:32:27Z

src/prng/isaac.rs

+/// Input: a, b, c, s[256] // state
+/// Output: r[256]         // results
+///
+/// mix(a,i) = a ^ a << 13   i = 0 mod 4


I think there should be an "if" before i = ...

dhardy · 2017-09-12T07:34:48Z

src/prng/isaac.rs

+/// in 1996[1][2].
+///
+/// Although ISAAC is designed to be cryptographically secure, its design is not
+/// founded in cryptographic theory. Therefore it should _not_ be used for


Maybe better to say is not recommended for? Or are you quoting Jenkins?

dhardy · 2017-09-12T07:36:35Z

src/prng/isaac.rs

-/// cryptographic purposes, but this implementation has not be
-/// verified as such. Prefer a generator like `OsRng` that defers to
-/// the operating system for cases that need high security.
+/// ISAAC therefore needs comparatively much memory. 2 * 256 * 4 = 2 kb to hold


ISAAC therefore needs a lot of memory, relative to non-crypto RNGs.

[As I understand, most crypto RNGs need a lot of memory.]

dhardy · 2017-09-12T07:43:27Z

src/prng/isaac_word.rs

+/// depending on the pointer size of the target architecture.
+///
+/// In general a random number generator that internally uses the word size of
+/// the target architecture is faster than one that doesn't. Choosing


Until we get to things like x32. And then modern CPUs support so much SIMD that we now have a pretty good u128 type. Leave this as-is for now, but it might be worth thinking about.

pitdicker · 2017-09-13T04:35:37Z

That where good comments, thank you. Updated.

dhardy · 2017-09-13T08:38:59Z

Merged. I created a few conflicts so figured I should solve them. I also removed a few #[inline] attributes (in theory the compiler should work out when to do this itself most of the time) and removed the SeedableRng impl on IsaacWordRng (because that's not reproducible cross platforms).

pitdicker force-pushed the isaac-rewrite branch from 4b99f3e to f454dfc Compare September 7, 2017 08:40

pitdicker force-pushed the isaac-rewrite branch from f454dfc to 036cb72 Compare September 8, 2017 05:14

Paul Dicker added 2 commits September 11, 2017 20:55

Split isaac into 3 modules

ec7e6bf

For into `isaac` and `isaac64` for easier comparison. Split of `isaac_word` to generate correct the documentation (TODO). Nothing is changed here, just split in three files.

pitdicker force-pushed the isaac-rewrite branch from 036cb72 to fdf3c83 Compare September 11, 2017 20:34

pitdicker force-pushed the isaac-rewrite branch from fdf3c83 to 705cd05 Compare September 12, 2017 05:22

pitdicker commented Sep 12, 2017

View reviewed changes

dhardy reviewed Sep 12, 2017

View reviewed changes

pitdicker force-pushed the isaac-rewrite branch from 705cd05 to 1081a70 Compare September 13, 2017 04:29

Add some extra documentation

14d0561

pitdicker force-pushed the isaac-rewrite branch from 1081a70 to 14d0561 Compare September 13, 2017 04:33

dhardy merged commit 14d0561 into dhardy:master Sep 13, 2017

pitdicker deleted the isaac-rewrite branch October 21, 2017 14:40

dhardy mentioned this pull request Oct 22, 2017

Seeding PRNGs (traits and bit size) #18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isaac refactoring #4

Isaac refactoring #4

pitdicker commented Sep 7, 2017

dhardy commented Sep 7, 2017

pitdicker commented Sep 7, 2017 •

edited by dhardy

Loading

dhardy commented Sep 7, 2017 •

edited

Loading

pitdicker commented Sep 8, 2017

pitdicker commented Sep 11, 2017 •

edited

Loading

pitdicker commented Sep 12, 2017

pitdicker Sep 12, 2017 •

edited

Loading

pitdicker Sep 12, 2017

dhardy left a comment

dhardy Sep 12, 2017

pitdicker Sep 13, 2017

dhardy Sep 12, 2017

pitdicker Sep 13, 2017

dhardy Sep 12, 2017

dhardy Sep 12, 2017

dhardy Sep 12, 2017

dhardy Sep 12, 2017

pitdicker commented Sep 13, 2017

dhardy commented Sep 13, 2017

Isaac refactoring #4

Isaac refactoring #4

Conversation

pitdicker commented Sep 7, 2017

dhardy commented Sep 7, 2017

pitdicker commented Sep 7, 2017 • edited by dhardy Loading

dhardy commented Sep 7, 2017 • edited Loading

pitdicker commented Sep 8, 2017

pitdicker commented Sep 11, 2017 • edited Loading

pitdicker commented Sep 12, 2017

pitdicker Sep 12, 2017 • edited Loading

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

pitdicker commented Sep 13, 2017

dhardy commented Sep 13, 2017

pitdicker commented Sep 7, 2017 •

edited by dhardy

Loading

dhardy commented Sep 7, 2017 •

edited

Loading

pitdicker commented Sep 11, 2017 •

edited

Loading

pitdicker Sep 12, 2017 •

edited

Loading