BREAKING: Make random_mod_core platform-independent (take 2)

[mrdomino]

This introduces a fairly robust platform-independence test for random_mod_core. The test exercises the rejection sampling and also the way multi-word Uints are constructed, which changed in this PR, as you can see in commit d2f4b89:

-            U256::from_be_hex("C54302F2EB1E2F69C3B919AE0D16DF2259CD1A8A9B8EA8E0862878227D4B40A3")
+            U256::from_be_hex("C3B919AE0D16DF2259CD1A8A9B8EA8E0862878227D4B40A3C54302F2EB1E2F69")

The difference, under most circumstances, is just that word order will be rotated; the old entropy gathering approach independently rejection-sampled the high word first, then built up the rest, so the resulting Uint was sort of like (MSW, LSW, LSW+1, …, MSW-1) over the raw entropy stream, except in cases where the high-word rejection sample triggered, in which case it was a trickier transform. The new interpretation is simply little-endian, both bytewise and wordwise, aligned to 64-bit word boundaries.

I elected to make this change for two reasons:

1. It is more aesthetically pleasing to me personally.
2. More importantly, with this change we actually are changing the way we sample entropy in random_mod, particularly in rare cases of multiple rejection samples over the most significant word. Rather than make this a rare, subtle, difficult-to-detect change that will bite people unexpectedly, I elected to make this a big, obvious, easy-to-detect change that people will usually notice.

To work around the apparent compiler bug that required #1010 to be reverted (See #1018 for exploration of the bug), I also elected to make the comparison loop variable-time rather than using ct_lt, since the latter is what seems to trigger the bug. My defense is that random_mod is already variable-time; it leaks RNG state via timing information every time it rejection-samples. (It might be a good idea to rename it to random_mod_vartime, since AFAICT leaking this timing information is unavoidable in the general case.)

By making this change, we are able to carry through the prior copy-elision optimization from master, resulting in an implementation that performs about at parity with master in the benchmarks I ran.

One other note: I elected to take 64-bit rather than 32-bit words as primary here, i.e., 32-bit platforms need to adjust by sampling and discarding an extra word in odd cases rather than 64-bit platforms needing to adjust by only sampling half a word in those cases. Both are doable, but operating in terms of 64-bit word sizes frees us from the constraint of our RNG needing to be "4-byte sequential."

Fixes #1009

[codecov]

Codecov Report

❌ Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.89%. Comparing base (e9f0efd) to head (3d1db8a).

Files with missing lines	Patch %	Lines
src/uint/rand.rs	97.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1021      +/-   ##
==========================================
+ Coverage   79.86%   79.89%   +0.02%     
==========================================
  Files         163      163              
  Lines       17737    17757      +20     
==========================================
+ Hits        14166    14187      +21     
+ Misses       3571     3570       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mrdomino]

Current benchmarks show equivocal performance compared with master, slower in some cases, faster in some cases:

bounded random/random_mod, U1024
                        time:   [72.961 ns 76.993 ns 81.109 ns]
                        change: [+20.725% +25.815% +31.323%] (p = 0.00 < 0.05)
                        Performance has regressed.
Benchmarking bounded random/random_mod, U1024, small bound: Collecting 100 samples in estimated 5.0001 s (251M iter
bounded random/random_mod, U1024, small bound
                        time:   [14.832 ns 15.687 ns 16.576 ns]
                        change: [−16.001% −12.053% −7.9623%] (p = 0.00 < 0.05)
                        Performance has improved.
Benchmarking bounded random/random_mod, U1024, 512 bit bound low: Collecting 100 samples in estimated 5.0002 s (139
bounded random/random_mod, U1024, 512 bit bound low
                        time:   [42.299 ns 44.756 ns 47.282 ns]
                        change: [−10.546% −6.7606% −2.5299%] (p = 0.00 < 0.05)
                        Performance has improved.
Benchmarking bounded random/random_mod, U1024, 512 bit bound hi: Collecting 100 samples in estimated 5.0001 s (68M
bounded random/random_mod, U1024, 512 bit bound hi
                        time:   [71.057 ns 74.627 ns 78.443 ns]
                        change: [+14.196% +18.859% +24.401%] (p = 0.00 < 0.05)
                        Performance has regressed.
Benchmarking bounded random/random_mod, U1024, tiny high limb: Collecting 100 samples in estimated 5.0001 s (124M i
bounded random/random_mod, U1024, tiny high limb
                        time:   [40.052 ns 40.076 ns 40.104 ns]
                        change: [+2.3526% +2.6806% +3.0255%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
  1 (1.00%) low severe
  3 (3.00%) low mild
  2 (2.00%) high mild
  2 (2.00%) high severe

[mrdomino]

@tarcieri any thoughts on this?

I could see the switch from ct_lt being controversial here; and arguably merits renaming this to random_mod_vartime.

[tarcieri]

Sorry I haven't looked at this, it was a holiday weekend here in the US, and I got a bit lost following your other PR with supposed miscompilation? (did you ever get that into some sort of outside-of-CI reproducible state? If so we should report it upstream)

I feel like this is fundamentally trying to something quite similar to #285, but I find the implementation there a bit more comprehensible.

cc @fjarri

[mrdomino]

No worries; miscompilation bug is here:
rust-lang/rust#149522

E.g.:
https://github.com/mrdomino/subtle-repro/actions/runs/19838188507

[mrdomino]

I feel like this is fundamentally trying to something quite similar to #285, but I find the implementation there a bit more comprehensible.

Yes; this implementation is kinda wonky for two reasons:

1. The desire for random_mod benchmarks to be fast, which prevents me just going with 7e0166b.
2. The compiler bug, which prevents me using ct_lt with the optimized version.

[mrdomino]

Yeah actually re-reading #285, I prefer it myself - difference (in terms of values produced / rng state) is that this is (64-bit) word-aligned whereas that is byte-aligned; this wastes more entropy but may provide more opportunities for optimization, whereas that wastes less entropy (but still some; we are not going down to the bit level after all.)

[fjarri]

And Encoding is now available for all Uint, so it can be made simpler

[mrdomino]

@fjarri a take 3, based on Encoding: #1026.