Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:117296
User-Agent: Cyrus-JMAP/3.5.0-alpha0-4778-g14fba9972e-fm-20220217.001-g14fba997
Mime-Version: 1.0
Message-ID: <cab85ccc-bef9-43b2-a967-e91553022761@www.fastmail.com>
In-Reply-To: 
 <CAMs36Hco05vXydtM8qUfpcDkj6oKYufifAr-gOr573OwmVtimA@mail.gmail.com>
References: 
 <CAMs36Hco05vXydtM8qUfpcDkj6oKYufifAr-gOr573OwmVtimA@mail.gmail.com>
Date: Wed, 09 Mar 2022 12:06:54 -0600
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] [RFC] [Under Discussion] Random Extension 5.x
From: larry@garfieldtech.com ("Larry Garfield")

On Wed, Mar 9, 2022, at 4:48 AM, Go Kudo wrote:
> Hello internals.
>
> Proposed RFCs and implementations have been reorganized. The main changes
> are as follows
>
> https://wiki.php.net/rfc/rng_extension
> https://github.com/php/php-src/pull/8094
>
> - Remove XorShift128Plus, Xoshiro256StarStar
>     - To avoid confusion, Can be added if needed
> - All classes can now omit the seed argument
>     - Seeding with CSPRNG
> - Throws RuntimeException if number generation fails
>
> The merge window to PHP 8.2 will remain open for some time to come. Due to
> the global turmoil, I will try to delay the start of the RFC voting phase
> as long as possible.
>
> However, I believe that PHP RFCs tend to become more controversial once the
> voting phase begins.
>
> Therefore, I would like to solicit feedback on this RFC, whatever it may
> be. (Just only positive or negative feedback would be okay)
>
> Regards,
> Go Kudo

This looks good to me overall, although I have a few comments.

* There's several places where a sentence or portion of a sentence repeats itself.  I assume this is just an editing error.

* The list headers "implemented of" doesn't really make grammatical sense.  I think you mean "implemented by"?

* The last section "PRNG Shootout" seems to imply that you're only implementing one algorithm, but it doesn't say which one.  The earlier sections, though, show several algorithms that you are implementing.  Again, I assume this is an editing error from many revisions but it's confusing as is.

* If no engine is provided to the randomizer, what gets used?  If the default is listed somewhere in the RFC, I missed it.  I'm assuming there should be one, for usability, but changing it in the future would be an RFC-level change (much like changing the defaults for password_hash()).

* The example of using different algorithms in different environments needs much higher billing.  You're burying the lead there.  The ability to have a bad but predictable random seed for tests but then a CSPRNG for production is *huge*.  The approach is similar to the nearly-done PSR-20 spec in FIG, which does essentially the same thing for the "current time" routines, albeit in userspace.  I would highlight this benefit way more, as it's probably the biggest benefit from these changes that most developers will see and has me more excited about this RFC than anything else in the entire document.

* How do the existing random functions interact with the new API?  Do they shift to be shells over the new classes with an internal shared global, or something else?  

* A side effect of the current design is that SeedableEngine is just a marker interface; there's no actual standardized way to set the seed.  The examples imply that passing it as a constructor is the recommended way (and I agree it probably should be), but that's not explicit.  I'm also unclear then what value the marker interface is, then.  Does the randomizer do something different with a seedable engine?  I'm not against the current design, but its explanation/implications could be improved.

Looking forward to this passing in a few weeks!

--Larry Garfield