Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117028
MIME-Version: 1.0
References: <CAMs36HcsGqNanQaro_VTkK_VKHNg-jqtshXpW-aCxdqpxHjMdw@mail.gmail.com>
 <41a1b458-4941-f34e-f1b4-e25b3298b80a@bastelstu.be>
In-Reply-To: <41a1b458-4941-f34e-f1b4-e25b3298b80a@bastelstu.be>
Date: Mon, 14 Feb 2022 23:53:36 +0900
Message-ID: <CAMs36Hc4GCPUp2m+6N1LTP+hPrUv15iZF2QTCHBuAm+KDyXgfw@mail.gmail.com>
To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <timwolla@bastelstu.be>
Cc: internals@lists.php.net
Content-Type: multipart/alternative; boundary="0000000000006e6edd05d7fb95ae"
Subject: Re: [PHP-DEV] [RFC] [Under Discussion] Random Extension 4.0
From: g-kudo@colopl.co.jp (Go Kudo)

--0000000000006e6edd05d7fb95ae
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

2022=E5=B9=B42=E6=9C=8814=E6=97=A5(=E6=9C=88) 20:40 Tim D=C3=BCsterhus <tim=
wolla@bastelstu.be>:

> Hi
>
> On 2/14/22 12:11, Go Kudo wrote:
> > The refreshed RFC and implementation are available at the following URL=
:
> >
> > https://wiki.php.net/rfc/rng_extension
> > https://github.com/php/php-src/pull/8094
> >
> > If there are no specific comments, I would like to start voting as soon
> as
> > the two-week pre-announcement phase is over.
>
> 1) XorShift128+ has a 128 Bit internal state, but takes an integer seed
> within the constructor. Thus only 64 Bits of seed can be provided.
>
> Maybe the seed parameter should be a 16-byte string instead?
> Initializing the generator with a completely random seed would then be:
>
> new XorShift128Plus(\random_bytes(16));
>
> instead of the much more complicated:
>
> new XorShift128Plus(\random_int(\PHP_INT_MIN, \PHP_INT_MAX));
>
> Perhaps the following API would be even clearer:
>
> XorShift128Plus::fromSeed(\random_bytes(16));
> XorShift128Plus::fromGenerator(new Secure()); // Takes 16 bytes from the
> given generator.
>
> 2) I would adjust the 'Randomizer' to use the 'Secure' generator as a
> safe default. If absolute performance or a reproducible sequence is
> required then one can use a custom generator, but the default will be
> the secure CSPRNG, making it harder to misuse.
>
> 3) The RFC is inconsistent in the example code. Is it 'stringShuffle' or
> 'shuffleString'?
>
> 4) The RFC should document the 'NumberGenerator' interface. Specifically
> I'm interested in the return type of the 'generate' method. Does it
> return bytes or integers? Is it legal to implement the interface in
> userland code?
>
> Best regards
> Tim D=C3=BCsterhus
>

Hi

> 1) XorShift128+ has a 128 Bit internal state, but takes an integer seed
within the constructor. Thus only 64 Bits of seed can be provided.

This is for convenience. Other software that uses XorShift128+, such as
Chromium (V8), also uses a 64-bit value for the initial seed value.
I think that 128-bit value seeding with strings is unintuitive and not very
good for performance.

https://chromium.googlesource.com/v8/v8/+/refs/heads/main/src/base/utils/ra=
ndom-number-generator.h

> 2) I would adjust the 'Randomizer' to use the 'Secure' generator as a
safe default. If absolute performance or a reproducible sequence is
required then one can use a custom generator, but the default will be the
secure CSPRNG, making it harder to misuse.

Certainly, this may be appropriate. But, in this case, the Randomizer
generated with the default parameters will not be serializable. Is that
acceptable?

Personally, I think this is a good change.

> 3) The RFC is inconsistent in the example code. Is it 'stringShuffle' or
'shuffleString'?

RFC Fixed :)

> 4) The RFC should document the 'NumberGenerator' interface. Specifically
I'm interested in the return type of the 'generate' method. Does it return
bytes or integers? Is it legal to implement the interface in userland code?

It returns an int. Also, as pointed out in GH, the generated value is
implicitly treated as the size equivalent of PHP_INT_SIZE (zend_long) on
the environment. This means that it is not possible to implement the
userland Mersenne twister (32-bit) in a 64-bit environment.

https://github.com/php/php-src/pull/8094#pullrequestreview-881660425

--0000000000006e6edd05d7fb95ae--