Updated shuffle implementation to use better hash function

[djns99]

Fixes #1256 by changing the hash function from taus88 to use wyhash. Adds test for a random distribution of numbers

[alliepiper]

@djns99 @RAMitchell Just checking in -- it looks like you two have been iterating on this PR, just let me know when you're ready for me to do a final review and start testing/landing it.

[RAMitchell]

@djns99 I'm happy with your current approach using murmurhash, did you have anything else you wanted to add? Would be good to have a couple of comments with references for CephesFunctions and murmerhash. Please also check the licenses are compatible i.e. we are allowed to use their code in this way.

[djns99]

@djns99 I'm happy with your current approach using murmurhash, did you have anything else you wanted to add? Would be good to have a couple of comments with references for CephesFunctions and murmerhash. Please also check the licenses are compatible i.e. we are allowed to use their code in this way.

@RAMitchell I probably wont have time to do this in the next few days. However, I am still having difficulty obtaining sufficiently random results, even if I increase the number of rounds. Some preliminary investigations indicate that it is likely the murmur hash function with insufficient randomness, however it is a significant improvement over the current results even at 8 rounds.
I would also note that it could be a test bug since I would expect a high enough number of rounds to pass, but I did not see this behaviour

As for licensing, cephes was taken from the NIST suite for testing randomness, which according to https://csrc.nist.gov/Projects/Random-Bit-Generation/Documentation-and-Software is in the public domain. I took murmur hash from the smhasher repo, the licensing information there say it is also in the public domain.

[RAMitchell]

Ok, I think the key thing is for us to make the next toolkit release (11.1?). Let's catch up next week on this.

[djns99]

@RAMitchell I have done a bit of work on this and decided to use the RC5 encryption algorithm https://doi.org/10.1007/3-540-60590-8_7. This uses a 12-20 round (I have it set to 12 at present) feistel network using data dependent rotations on an w-bit register. The only shortfall of this is that it requires us to round to a power of 4 instead of 2. However, it means we can make some guarantees about the strength of our internal function (i.e. any errors are due to an insufficiently random key).
There are still some issues with the test I am in the process of diagnosing

As for licensing, the patent for the algorithm expired in 2015 https://patents.google.com/patent/US5835600A/en so I believe this means we are allowed to use it.

[djns99]

Test issues are resolved.

[RAMitchell]

Regarding the use of RC5, that's up to the thrust team. You might want to have an alternative in mind.

The only shortfall of this is that it requires us to round to a power of 4 instead of 2.

I'm a bit confused, current code still uses nearest power of two in get_cipher_bits.

Looks really good otherwise and glad to see you've got the tests passing at high accuracy!

[djns99]

I'm a bit confused, current code still uses nearest power of two in get_cipher_bits.

Note the change to

m >>= 2u;

This counts the number of pairs of bits now, with w representing one half of the feistel block

[RAMitchell]

I see, thanks! Nearest power of four is not ideal from a performance perspective but I think we are still much faster than alternatives and robustness is priority one.

[djns99]

performance perspective

In theory we could modify the RC5 algorithm slightly and use modulus by a different amount for each register to allow for a power of two instead of 4. However, this would likely require a more comprehensive analysis of the problem to ensure the correct behaviour is maintained, so I dont believe this is the place for this optimisation. There are a range of other candidate ciphers available to consider, but RC5/RC6 (though RC6 rounds to a power of 16) was the main one I found that explicitly supports a variable block width.

[RAMitchell]

@djns99 let us know when you are ready for final review.

[djns99]

If we are ok with RC5 I don't think there is anything left to do at present. So we should be good to go ahead with review and merge.

[alliepiper]

Whatever implementation you two decide on is fine with me, but it sounds like there are some possible concerns with licensing, too?

Can someone summarize the potentially problematic portions of the current PR, along with their licenses, patents, and other IP concerns?

[djns99]

There are two parts that potentially have licensing/IP issues. The first is the Cephes functions used in the test suite, which are directly taken from a NIST test suite for random numbers.
The second potential problem is the round function used for the Feistel Network, of which there are currently two options: RC5 and MurmurHash2

As for licensing, cephes was taken from the NIST suite for testing randomness, which according to https://csrc.nist.gov/Projects/Random-Bit-Generation/Documentation-and-Software is in the public domain. I took murmur hash from the smhasher repo, the licensing information there say it is also in the public domain.

The Cephes functions are fine as is Murmur Hash if we go that route to the best of my understanding.

As for licensing, the patent for the algorithm expired in 2015 https://patents.google.com/patent/US5835600A/en so I believe this means we are allowed to use it.

If we go for RC5 (my preferred function @RAMitchell ) then it is covered by an expired patent, with the implementation being my own (based off the original paper https://doi.org/10.1007/3-540-60590-8_7). My understanding is the expired patent means the algorithm is in the public domain, so it is acceptable to use as we have here, however I don't know enough to say for sure.

To the best of my knowledge there are no other licensing/IP issues.

[brycelelbach]

I'm going to have one of the NVIDIA open source lawyers look at this.

[alliepiper]

@djns99 @RAMitchell I haven't been able to get a straight answer on this from our lawyers, and I get the feeling that the only way this patch will move forward is by rewriting it to avoid anything that is known to be associated with a non-NVIDIA owned patent.

Would you be willing to submit a different implementation that avoids the patent-related algorithms?

[RAMitchell]

Ok, we will look at rewriting with a different hash. @djns99 if you don't have time, I will take a stab at it later this week.

[djns99]

@RAMitchell it would be good if you can. I suspect going back to MurmurHash2 will probably work now I have fixed the tests. Just a bit less academic backing behind it's quality. Should also be able to go to power of two instead of 4 again

[alliepiper]

Thanks. Sorry about this, but it's the only way to get this moving again in a reasonable amount of time.

[alliepiper]

Replaced by #1309 due to IP concerns.