-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
backport: Use FastRandomContext for all tests #3511
backport: Use FastRandomContext for all tests #3511
Conversation
(I will rebase accordingly as to give attribution to chromatic and ryanofsky as the review process continues.) |
ef63819
to
1fe5082
Compare
Force pushed to give attribution to ryanofsky. Just noticing an extraneous |
1fe5082
to
af576f9
Compare
Force pushed to remove extraneous boost headers |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: the commit message for f9cd19f needs some work: it's no longer a real cherry pick because it's been stripped of a lot of changes from that commit. So to prevent future confusion maybe not do it like this.
Suggest to:
- Just take ownership of that commit
- Describe what it actually does
backported from:
orinspired by:
rather thancherry-picked from:
?
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
f9cd19f
to
b3e1661
Compare
change sys/random.h to random.h in AC_MSG_CHECK remove definitions that are applicable to macos which include unistd.h additionally change comparator argument from NULL to nullptr when evaluating &getentropy Inspired by: c13c97dbf846cf0e6a5581ac414ef96a215b0dc6
b3e1661
to
d693d4a
Compare
Still waking up but I think I've addressed your suggestions. Might need to reword the commit message more though. Let me know if what I wrote meets your approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested ACK - tested on arm64 macOS Sonoma and x86_64 Ubuntu Jammy
Note: scope is greater than just tests (and bench), this also affects randomness calls when:
- Selecting random peers from our peer database (
CAddrman::Select_()
) - Replacing a random entry in a maxed out list of peers to send out (
CNode::PushAddress()
) - Random order of selecting available utxos in wallet (
ApproximateBestSubset()
inwallet.cpp
)
This is a follow up to #3487
and #3497 which are requirementswhich is a requirement needed in order to get #3229 in.