[RFC] [Draft] Add RNG extension and deprecate mt_srand() View as Markdown

1. I'm still not a fan of exposing the algorithms as separate classes in
   PHP. Their names are confusing and I don't see how exposing them gives any
   benefits. I would prefer that, like in password_hash, let the user chose
   the algorithm using a constant then ask for seed as a second argument
   to Randomizer.

This may be useful if you are implementing a specific RNG in userland.
It also gets quite complicated when trying to implement extensions that add
corresponding algorithms.
Since we're going all out, I thought it might be appropriate to use the OOP
approach. What do you think?

2. Why is there a getter on Randomizer for the source?

Some Source objects are serializable and can store state, to take advantage
of this feature.

3. Why is the interface called source?

I couldn't think of an appropriate name. Do you have any ideas?

4. You have generateInt() twice. Wouldn't it be better to have it only
   once but make the arguments nullable or provide PHP_MIN_INT and PHP_MAX_INT
   as defaults?

This is to maintain compatibility with the mt_rand() function.

5. Why do we need to deprecate the old functions now? What's the rush?

Keeping old implementations may lead to code being written that
unintentionally relies on the PHPs global state.
Deprecating them reduces the possibility that such code will be written
anew.

2021年5月19日(水) 2:29 Kamil Tekiela tekiela246@gmail.com:

Hi Go Kudo,

I think this proposal is much better than the one before. I believe we
need such functionality in PHP.
I have some questions I would like you to consider:

1. I'm still not a fan of exposing the algorithms as separate classes in
   PHP. Their names are confusing and I don't see how exposing them gives any
   benefits. I would prefer that, like in password_hash, let the user chose
   the algorithm using a constant then ask for seed as a second argument
   to Randomizer.
2. Why is there a getter on Randomizer for the source?
3. Why is the interface called source?
4. You have generateInt() twice. Wouldn't it be better to have it only
   once but make the arguments nullable or provide PHP_MIN_INT and PHP_MAX_INT
   as defaults?
5. Why do we need to deprecate the old functions now? What's the rush?

Regards,
Kamil

Thanks.

Deprecation will be done only to prevent unintended MT state dependent
code. We don't plan to go as far as removing the implementation for now.

Firstly, making these functions independent of mt_srand() is a breaking
change, so cannot happen until PHP 9.0 at the earliest.

To be honest, I don't understand how far PHP is willing to go to accept
disruptive changes.

For example, mt_rand(), when called in its uninitialized state, was
initialized using Combined LCG (time + PID/TID) before PHP 8.0.

This can bias the results of mt_rand(), so I sent a PR on GitHub and it
was changed to use php_random_bytes() if possible.

https://github.com/php/php-src/pull/6520

It was in PHP 7.1 that the shuffle(), str_shuffle(), and array_rand()
functions were changed to use MT-based random numbers instead of
libc-derived random numbers. This was clearly disruptive, and left no way
to even maintain backward compatibility.

Compared to that, this proposal is not disruptive in the sense that it
provides a complete alternative.

2021年5月19日(水) 2:43 Rowan Tommins rowan.collins@gmail.com:

I have created a draft of the RFC.

https://wiki.php.net/rfc/rng_extension

Hi,

At a glance, I think this looks like a good clear approach.

I think deprecating mt_srand makes sense, but could do with a heading to
make sure it's not overlooked, and include a clear statement of when it
would be removed (9.0? 10.0?). It could potentially even be a separate
vote, as keeping it doesn't actually harm users of the new classes.

I have a few concerns with the third part of the proposal:

Also, change the following function to use the same method as
random_byte() (the php_random_bytes() internal function) for processing,
instead of PHP's global state.

Firstly, making these functions independent of mt_srand() is a breaking
change, so cannot happen until PHP 9.0 at the earliest. This could
happen at the same time as mt_srand() is removed, but that connection
needs to be clear in the proposal.

Secondly, it seems inconsistent to make these functions use the
crypto-strong randomness source, but retain rand() and mt_rand() using
PRNGs. In fact, I don't see the need to change them at all - they can be
documented as not for cryptographic use, as mt_rand is, and users
pointed to the new API if they need something stronger.

Regards,

--
Rowan Tommins
[IMSoP]

--

To unsubscribe, visit: https://www.php.net/unsub.php

Sorry. I couldn't make it in time for my lunch break.

How many bytes is Source::next() supposed to generate? Is it even
intended to be called directly (vs Randomizer::generateBytes(int $length))?

No, this is not intended to be used directly.
In fact, as many people have pointed out, I think it would be more
appropriate to merge it into a single class.

As per the original implementation (ext/orng), I didn't think it needed to
be separated originally. One of the comments on a previous RFC suggested
that it would probably be better to separate them, and I took that into
account.

About the MT19937PHP variant: The PHP manual for mt_srand() describes
the MT_RAND_PHP mode like this: "Uses an incorrect Mersenne Twister
implementation which was used as the default up till PHP 7.1.0. This mode
is available for backward compatibility."; do we really need/want to port
it to this new extension?

This is for backward compatibility. If it is not needed, we would like to
eliminate the implementation itself.
Well, to keep the language core clean, maybe this should be provided by an
appropriate external extension.

• PlatformProvided feels like the "odd one out" here: its constructor
  [assuming same as previous RFC] doesn't take a seed, and it isn't
  serializable; I understand why, but the main point of the RFC is "RNG
  reproducibility", which this class cannot achieve.

You're absolutely right. I would like to remove this.

Technical, about Randomizer::generateFloat(): Is the returned float
guaranteed to be finite (i.e. never NAN nor +/-INF)? And may it ever return
-0.0 (negative zero)? and even if not, is 0.0 twice as likely as other
floats?
Also, no generateFloat(float $min, float $max) variant like for
generateInt()?

As a matter of fact, we believe that the functions provided by this class
are sufficient for mt_rand(), shuffle(), str_shuffle(), and
array_rand() equivalents. At most, the equivalent of random_byte()
might be provided.
I'll reorganize these.

Thanks for the detailed remarks. Based on these, I would like to clean up
the RFC.

Regards,
Go Kudo

2021年5月19日(水) 18:46 Guilliam Xavier guilliam.xavier@gmail.com:

Hello internals.

I have created a draft of the RFC.

https://wiki.php.net/rfc/rng_extension

Hello Go Kudo,

First of all, thank you for the (re)work.

I think the API looks better, but I still have some remarks/questions
(roughly in order of appearance, with some overlap):

• How many bytes is Source::next() supposed to generate? Is it even
  intended to be called directly (vs Randomizer::generateBytes(int $length))?

• The stubs in "Proposal" should show __construct() signatures for the
  classes implementing Source.

• About the MT19937PHP variant: The PHP manual for mt_srand()
  describes the MT_RAND_PHP mode like this: "Uses an incorrect Mersenne
  Twister implementation which was used as the default up till PHP 7.1.0.
  This mode is available for backward compatibility."; do we really need/want
  to port it to this new extension?

• I think you said in the past that XorShift128Plus should be the
  preferred/default implementation, and MT19937 is only provided for
  compatibility/migration? But if so, that's not clear at all in the RFC.

• PlatformProvided feels like the "odd one out" here: its constructor
  [assuming same as previous RFC] doesn't take a seed, and it isn't
  serializable; I understand why, but the main point of the RFC is "RNG
  reproducibility", which this class cannot achieve.
  Also, we already have random_bytes()/random_int(), and you're
  proposing to change shuffle()/str_shuffle()/array_rand()'s internal
  RNG from mt_rand()-like to random_bytes()-like, so what reason would
  there be to use this class? only generateFloat()? but couldn't that be a
  new global function random_float()?
  All in all, wouldn't it be simpler to drop PlatformProvided, and have
  all Source implementations take a seed on construction and be
  serializable?

• Technical, about Randomizer::generateFloat(): Is the returned float
  guaranteed to be finite (i.e. never NAN nor +/-INF)? And may it ever return
  -0.0 (negative zero)? and even if not, is 0.0 twice as likely as other
  floats?
  Also, no generateFloat(float $min, float $max) variant like for
  generateInt()?

• About main usage of the API: To get e.g. $n (say 3) pseudo-random
  integers between $min (say 0) and $max (say 100) using the "best"
  implementation (which we have to know/guess is XorShift128+, isn't it?), we
  must: first choose a $seed (time(), maybe?), then do $source = new XorShift128Plus($seed); and $rng = new Randomizer($source); [even if
  $source (and $seed) could be inlined, that's still several "steps"], then
  we can finally call $rng->generateInt($min, $max); $n times. Is that
  correct?
  If so, it may make sense from the implementer side, but from the user
  side it doesn't look like the most friendly.
  Also, that lets the possibility of constructing a second Randomizer
  using the same $source (or $rng->getSource()), which could be undesirable
  for reproducibility, isn't it?
  Wouldn't it be better to "merge" Randomizer into the interface/classes?
  Actually I see that's the case in your other project ext/orng (
  https://github.com/zeriyoshi/php-ext-orng/blob/master/rng/rnginterface.stub.php
  and
  https://github.com/zeriyoshi/php-ext-orng/blob/master/rng/xorshift128plus.stub.php
  ), why the different approach for ext/rng?
  Maybe even better (and already suggested), have Randomizer constructor
  take an optional algorithm (string, default XorShift128Plus::class?) and
  an optional seed (int, default time()?) and construct the source
  internally (and don't expose it)? So we could do just $rng = new Randomizer(); to rely on the defaults (then call $rng->generateInt($min, $max); $n times).

• Finally, I think the vote should be split into 3 parts: the new classes,
  the deprecations (which itself could be split between [mt_]srand and
  [mt_]rand), and the change of internal RNG for
  shuffle/str_shuffle/array_rand.

Best regards,

--
Guilliam Xavier