rand: implement an unbiased random integer from a range #22499
Conversation
paulidale
added
branch: master
github-actions
bot
added
the
severity: fips change
|
A helper function is a reasonable idea still. |
|
The pathological case will be around 2³¹+1 where just under half the values are in the rejection region. Given the test only needs the third random sample less than one time in 2³², testing isn't practical using less than (at a guess) 2⁶⁴ samples. It would be possible to test this better by reducing the size. E.g. generating 4 bit numbers under 9. A mistake should become obvious in reasonable time. Not useful for our test suite but good for confirmation. χ² is probably good enough here. |
|
I've run a χ² test (p=0.99) for the output of this algorithm modified to work with 4 bit "words" and it seems good. Test harness is attached. There seems to be a slight problem with the χ² test for this discrete uniform distribution. I'm seeing test failures for small samples at a higher than expected rate (gut feel, not measured). For large samples it comes good and the test statistic reduces nicely. I'm pretty sure this is because the sum of uniform distributions is pretty non-normal in nature. As the number increases the central limit theorem kicks in and it tends towards normal. Convergence to normal being, proportional to the square root of the number of samples accumulated (i.e. 100 gives approx 1 digit, 10,000 gives approx 2 digits, 1,000,000 gives approx 3, etc). Given the instability for small sample sizes and the time required to generate large samples, I do not see how this can reasonably be directly tested as part of our CIs. Run using 1,000,000 samples (note the Run using 100,000,000 samples: |
paulidale
added
the
tests: exempted
|
nit: unbiassed -> unbiased |
paulidale
changed the title
|
Addressed comments... |
|
Ping @openssl/committers for reviews. The algorithm isn't as daunting as it first appears. |
|
@mspncp Are you happy that your requested changes have been made? |
|
Yes I am. You may dismiss my previous change request if you like. (I didn't find a way how to do it from the mobile app.) |
tom-cosgrove-arm
dismissed
mspncp’s stale review
Requested changes have been made
tom-cosgrove-arm
added
approval: otc review pending
and removed
tests: exempted
|
OTC: vote to include PR subject to normal review process |
I didn't doubt the fact that it was written from scratch. But except for minor adjustments, it is the same algorithm. As a mathematician, I find it remarkable and non-intuitive that it's only the written program and not the underlying algorithm which is considered an intellectual property. Fortunately, I'm not a lawyer, and if they are happy with this interpretation, I'll not dive down further into this rabbit hole. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
|
The code here is different to the one referenced. The referenced one only generates a second 64 bit value and accepts a 1 in 2^64 chance of bias. This PR, instead, generates 32 bit values and continues to do so until a definitive result is obtained -- this is not covered by the referenced code. Algorithms cannot be copyrighted. They are patentable (unfortunately). |
Refer: swiftlang/swift#39143 for a description of the algorithm. It is optimal in the sense of having: * no divisions * minimal number of blocks of random bits from the generator
|
@tom-cosgrove-arm please reconfirm |
|
Still okay |
|
Merged to the master and 3.2 branches. Thank you. |
Refer: swiftlang/swift#39143 for a description of the algorithm. It is optimal in the sense of having: * no divisions * minimal number of blocks of random bits from the generator Reviewed-by: Tom Cosgrove <[email protected]> Reviewed-by: Matthias St. Pierre <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #22499)
Reviewed-by: Tom Cosgrove <[email protected]> Reviewed-by: Matthias St. Pierre <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #22499)
Reviewed-by: Tom Cosgrove <[email protected]> Reviewed-by: Matthias St. Pierre <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #22499)
Refer: swiftlang/swift#39143 for a description of the algorithm. It is optimal in the sense of having: * no divisions * minimal number of blocks of random bits from the generator Reviewed-by: Tom Cosgrove <[email protected]> Reviewed-by: Matthias St. Pierre <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #22499) (cherry picked from commit 55755fb)
Reviewed-by: Tom Cosgrove <[email protected]> Reviewed-by: Matthias St. Pierre <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #22499) (cherry picked from commit d05e0e4)
Reviewed-by: Tom Cosgrove <[email protected]> Reviewed-by: Matthias St. Pierre <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #22499) (cherry picked from commit dfb26e0)
Refer: swiftlang/swift#39143 for a description of the algorithm. It is optimal in the sense of having: * no divisions * minimal number of blocks of random bits from the generator Reviewed-by: Tom Cosgrove <[email protected]> Reviewed-by: Matthias St. Pierre <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl/openssl#22499) Signed-off-by: fly2x <[email protected]>
Reviewed-by: Tom Cosgrove <[email protected]> Reviewed-by: Matthias St. Pierre <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl/openssl#22499) Signed-off-by: fly2x <[email protected]>
Reviewed-by: Tom Cosgrove <[email protected]> Reviewed-by: Matthias St. Pierre <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl/openssl#22499) Signed-off-by: fly2x <[email protected]>
Refer: swiftlang/swift#39143 for a description of the algorithm.
It is optimal in the sense of having: