backport: Use FastRandomContext for all tests #3511
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(I will rebase accordingly as to give attribution to chromatic and ryanofsky as the review process continues.) |
xanimo
force-pushed
the
1.15.0-dev-91e3035-10321
branch
from
ef63819
to
1fe5082
Compare
|
Force pushed to give attribution to ryanofsky. Just noticing an extraneous |
xanimo
force-pushed
the
1.15.0-dev-91e3035-10321
branch
from
1fe5082
to
af576f9
Compare
|
Force pushed to remove extraneous boost headers |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
xanimo
force-pushed
the
1.15.0-dev-91e3035-10321
branch
2 times, most recently
from
78b6207
to
1873fb8
Compare
|
Force pushed to add squashed commit db50410 to 28b9d89 and cherry-picks of ee2d10a and bitcoin/bitcoin@c13c97db. I tried to add attribution to chromatic for their cherry-picks but it seems it didn't work. |
There was a problem hiding this comment.
- 7977dd7: why is this commit in this PR / why is this change relevant for the preceding / succeeding commits?
- 1552823: this scripted diff fails for me (GNU sed 4.8). Either remove the script and the "scripted-diff:" annotation from the commit
, or make it so that the script works. - nit: you've missed some pick references on the first couple commits (would be appreciated to have those, so that I can compare the picks with the originals.
Post-rebase commit checklist:
- 1: dc48e55 = 1: 95cd061 Introduce FastRandomContext::randbool()
- 2: caba928 = 2: ed5ff1b FastRandom benchmark
- 3: 1fd572e = 3: e9a3781 Add ChaCha20
- 4: 85aae7d ! 4: 28b9d89 util: Specific GetOSRandom for Linux/FreeBSD/OpenBSD
- 5: db50410 < -: ---------- squashme: comment that NUM_OS_RANDOM_BYTES should not be changed lightly
- 6: aef0947 = 5: 814a15a sanity: Move OS random to sanity check function
- 7: 1fad144 = 6: 1cf8374 random: Add fallback if getrandom syscall not available
- 8: e402411 = 7: 2ad516b Switch FastRandomContext to ChaCha20
- 9: ca9200c = 8: 9b8d168 Add a FastRandomContext::randrange and use it
- 10: f92324c = 9: 7977dd7 Make test_bitcoin.cpp compatible with Qt Test framework
- 11: 7ee01e0 = 10: 18e7aa6 Add FastRandomContext::rand256() and ::randbytes()
- 12: 0328722 = 11: 6d749d1 Merge test_random.h into test_bitcoin.h
- 13: 16758ea = 12: 1c00a41 Add various insecure_rand wrappers for tests
- 14: 37c30e7 = 13: 5fa8490 scripted-diff: use insecure_rand256/randrange more
- 15: b4236f7 = 14: 64a1b72 Replace more rand() % NUM by randranges
- 16: 50f01c7 = 15: e319e21 Replace rand() & ((1 << N) - 1) with randbits(N)
- 17: 7009588 = 16: d804294 Use randbits instead of ad-hoc emulation in prevector tests
- 18: af576f9 = 17: 1552823 scripted-diff: Use randbits/bool instead of randrange where possible
- -: ---------- > 18: bd83403 Check if sys/random.h is required for getentropy on OSX.
- -: ---------- > 19: 1873fb8 random: getentropy on macOS does not need unistd.h
Cherry-picked from: c21cbe6
Cherry-picked from: 663fbae
Cherry-picked from: e04326f
These are available in sandboxes without access to files or devices. Also [they are safer and more straightforward](https://en.wikipedia.org/wiki/Entropy-supplying_system_calls) to use than `/dev/urandom` as reading from a file has quite a few edge cases: - Linux: `getrandom(buf, buflen, 0)`. [getrandom(2)](http://man7.org/linux/man-pages/man2/getrandom.2.html) was introduced in version 3.17 of the Linux kernel. - OpenBSD: `getentropy(buf, buflen)`. The [getentropy(2)](http://man.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man2/getentropy.2) function appeared in OpenBSD 5.6. - FreeBSD and NetBSD: `sysctl(KERN_ARND)`. Not sure when this was added but it has existed for quite a while. Alternatives: - Linux has sysctl `CTL_KERN` / `KERN_RANDOM` / `RANDOM_UUID` which gives 16 bytes of randomness. This may be available on older kernels, however [sysctl is deprecated on Linux](https://lwn.net/Articles/605392/) and even removed in some distros so we shouldn't use it. Add tests for `GetOSRand()`: - Test that no error happens (otherwise `RandFailure()` which aborts) - Test that all 32 bytes are overwritten (initialize with zeros, try multiple times) Discussion: - When to use these? Currently they are always used when available. Another option would be to use them only when `/dev/urandom` is not available. But this would mean these code paths receive less testing, and I'm not sure there is any reason to prefer `/dev/urandom`. Closes: #9676 Cherry-picked from: 224e6eb Contains squashed commit of aa09ccb squashme: comment that NUM_OS_RANDOM_BYTES should not be changed lightly
Move the OS random test to a sanity check function that is called every time bitcoind is initialized. Keep `src/test/random_tests.cpp` for the case that later random tests are added, and keep a rudimentary test that just calls the sanity check. Cherry-picked from: 7cad849
If the code was compiled with newer (>=3.17) kernel headers but executed on a system without the system call, every use of random would crash the program. Add a fallback for that case. Cherry-picked from: 7e6dcd9
Cherry-picked from: 1632922
Cherry-picked from: 4fd2d2f
FastRandomContext now provides all functionality that the real Rand* functions provide. Cherry-picked from: 37e864e
Cherry-picked from: 124d13a
Cherry-picked from: 1119927
-BEGIN VERIFY SCRIPT- sed -i "s/\<GetRandHash(/insecure_rand256(/" src/test/*_tests.cpp sed -i "s/\<GetRand(/insecure_randrange(/" src/test/*_tests.cpp src/test/test_bitcoin.cpp sed -i 's/\<insecure_rand() % \([0-9]\+\)/insecure_randrange(\1)/g' src/test/*_tests.cpp -END VERIFY SCRIPT- Cherry-picked from: efee1db
Cherry-picked from: 3ecabae
Cherry-picked from: 5f0b04e
Cherry-picked from: 2ada678
-BEGIN VERIFY SCRIPT- sed -i 's/insecure_randbits(1)/insecure_randbool()/g' src/test/*_tests.cpp sed -i 's/insecure_randrange(2)/insecure_randbool()/g' src/test/*_tests.cpp sed -i 's/insecure_randrange(4)/insecure_randbits(2)/g' src/test/*_tests.cpp sed -i 's/insecure_randrange(32)/insecure_randbits(5)/g' src/test/*_tests.cpp sed -i 's/insecure_randrange(256)/insecure_randbits(8)/g' src/test/*_tests.cpp -END VERIFY SCRIPT- Cherry-picked from: 2fcd9cc
-BEGIN VERIFY SCRIPT- sed -i 's/\<insecure_randbits(/InsecureRandBits(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_randbool(/InsecureRandBool(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_randrange(/InsecureRandRange(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_randbytes(/InsecureRandBytes(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_rand256(/InsecureRand256(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<insecure_rand(/InsecureRand32(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp sed -i 's/\<seed_insecure_rand(/SeedInsecureRand(/g' src/test/*.cpp src/test/*.h src/wallet/test/*.cpp -END VERIFY SCRIPT- Cherry-picked from: e945848
Cherry-picked from: ee2d10a
Remove it. Make this change, so in a future commit, we can combine #ifdefs, and avoid duplicate <sys/random.h> includes once we switch to using getrandom directly. Also remove the comment about macOS 10.12. We already require macOS > 10.15, so it is redundant. Cherry-picked from: c13c97dbf846cf0e6a5581ac414ef96a215b0dc6
xanimo
force-pushed
the
1.15.0-dev-91e3035-10321
branch
from
1873fb8
to
f9cd19f
Compare
|
Force pushed reworked branch. I think I addressed all of the above but let me know if I missed anything. |
|
I've tested performance for all tests that were edited between this PR at f9cd19f and 1.15.0-dev at 497c602 (except random tests as that's new) on an M1 with Apple clang 14.
script used icw for t in DoS_tests addrman_tests blockencodings_tests bloom_tests checkqueue_tests coins_tests \
crypto_tests cuckoocache_tests dbwrapper_tests merkle_tests miner_tests pmt_tests \
pow_tests PrevectorTests script_tests sighash_tests skiplist_tests util_tests versionbits_tests; do
echo "'src/test/test_dogecoin -t $t'";
done | xargs -n1 hyperfine --warmup 2 -r 100 --style none --export-csv /dev/stdout --This is consistent with what I measured with bench earlier and potentially can get a speedup from an unroll later, as discussed in #3487. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow up to #3487
and #3497 which are requirementswhich is a requirement needed in order to get #3229 in.