Skip to content
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

Remove MT19937 PRNG #4636

Merged
merged 1 commit into from Jul 18, 2017
Merged

Remove MT19937 PRNG #4636

merged 1 commit into from Jul 18, 2017

Conversation

konovod
Copy link
Contributor

@konovod konovod commented Jun 29, 2017

fixes #4634

@sdogruyol
Copy link
Member

sdogruyol commented Jun 30, 2017

I think this is good to merge as it's pretty straightforward 😄 Thanks @konovod 👍

@mverzilli
Copy link

mverzilli commented Jun 30, 2017

Thanks @konovod! Let's take a few days to let the discussions in #4634 mature before merging this (which LGTM otherwise!).

@funny-falcon
Copy link
Contributor

funny-falcon commented Jul 13, 2017

I don't think it is good to remove them.
Having them indeed is a really good thing for users who need stronger random source than PCG.

And there is another point for leaving them: they are really good reference implementations for those algorithms. They are so much more readable than versions on other language, so they could be used as a promotion for Crystal :-)

@RX14
Copy link
Contributor

RX14 commented Jul 13, 2017

@funny-falcon did you see #4634? We're just moving them to a n external shard.

@konovod
Copy link
Contributor Author

konovod commented Jul 13, 2017

MT19937 is much worse for poker games or other things where PCG can be attack target. Despite it's high period it is completely predictable after seeing ~600 consequent numbers. So IMHO it should be removed.

About ISAAC - i think that there could be a secure PCG with higher period, something intermediate between safe but slow System::Random and fast PCG32 . Maybe ChaCha12, but ISAAC doesn't seems bad too.

@funny-falcon
Copy link
Contributor

funny-falcon commented Jul 13, 2017

i think that there could be a secure PCG

I'd better use Xorshift1024+ or Xorshift4096+ - very fast, huge period.
While raw XorshiftN is as predictable as any other LSFR (and MT19937), its + variant is much more sane.

And ISAAC looks to be reasonably fast as well.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Jul 15, 2017

  • MT19937 must be removed. PCG32 is much better.

  • ISAAC could be kept, but ChaCha20 is reportedly better, and seems usually more trusted and used. I would vote to replace ISAAC with ChaCha20.

@funny-falcon
Copy link
Contributor

funny-falcon commented Jul 15, 2017

@ysbaddaden , then ChaCha12 . The best known "theoretical attack" is against 7 rounds of ChaCha. So ChaCha12 is really far from being compromised.

I was really disappointed that ChaCha20 and not ChaCha12 were wildely accepted. ChaCha20 were original conservative author's choice. But analyses of ChaCha became public a long before standartisation, so it is quite surprising ChaCha12 were not preffered choice, because ChaCha12 has same speed as hardware optimized AES, but ChaCha20 is 1.5x slower.

I'd even use ChaCha8 for non-secure RNG - not compromised yet, and very fast.

I think, it is same story as with SipHash: author recommended SipHash24 as concervative choice for secure applications in original paper, and everyone rush to use it as hash function for hash tables. But SipHash13 is safe enough for such use case. And I spend a lot of effort to convince Rust and Ruby to switch to SipHash13. (but I still think, SipHash is overkill for this (even SipHash13), so I propose simpler function for Crystal).

Note: for ChaCha to be fast Crystal have to provide SSE intrisicts.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Jul 16, 2017

  1. Do we agree that MT19937 must be gone?

  2. Do we agree we'd like to keep a secure PRNG? Maybe introduce ChaCha12 or keep ISAAC. Both are secure and trusted; both can certainly be optimized (at the expense of some readability for ISAAC).

If we agree on 1. let's update this PR to drop MT19937 only, and keep ISAAC for the time being. We can continue discussing in #4634 whether we want to keep distributing a secure PRNG in stdlib and which one. I'm fine with this idea, and like either ISAAC or ChaCha12.

@RX14
Copy link
Contributor

RX14 commented Jul 16, 2017

Removing MT19937 sounds like a good idea to me.

@konovod konovod changed the title Remove MT19937 and ISAAC PRNGs Remove MT19937 PRNG Jul 16, 2017
@konovod
Copy link
Contributor Author

konovod commented Jul 16, 2017

I've updated PR to remove only Mersenne.

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Jul 18, 2017

Thank you! Let's continue under #4634 now.

@ysbaddaden ysbaddaden merged commit abbb8de into crystal-lang:master Jul 18, 2017
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants