Use FastRandomContext for all tests #10321

sipa · 2017-05-02T23:03:10Z

Currently, the unit tests rely on a mix of an encapsulated FastRandomContext, and some of the Rand*() functions for randomness. This is inefficient, and will become more inefficient when the Rand* functions are changed to always provide strong randomness.

laanwj · 2017-05-03T05:25:14Z

Concept ACK, also been meaning to do this. There is no need for strong randomness in tests and speed is very important.

jnewbery · 2017-05-03T13:59:39Z

src/random.cpp

@@ -247,6 +247,27 @@ void FastRandomContext::RandomSeed()
    requires_seed = false;
 }

+uint256 FastRandomContext::rand256()


Any particular reason why this is defined in random.cpp, and rand64(), which is almost identical, is defined in the header file?

It doesn't feel worthwhile to inline rand256() to me.

jnewbery · 2017-05-03T15:23:17Z

Tested ACK 7666694 with one question.

I've completely unscientifically benchmarked this locally by running the tests which were updated (excluding the wallet tests). This seems to marginally speed up test run time:

master:

→ for i in {1..10}; do test_bitcoin --run_test=DoS_tests,blockencodings_tests,bloom_tests,checkqueue_tests,coins_tests,dbwrapper_tests,miner_tests,pow_tests,sighash_tests --log_level=test_suite 2>/dev/null | grep "testing time" | cut -d' ' -f7 | rev | cut -c 4- | rev | paste -sd+ - | bc;done
9805962
10284966
9880339
9470773
9775485
10186809
10072789
9522348
9297190
9433797

avg: 9.773045.8s

PR10321:

→ for i in {1..10}; do test_bitcoin --run_test=DoS_tests,blockencodings_tests,bloom_tests,checkqueue_tests,coins_tests,dbwrapper_tests,miner_tests,pow_tests,sighash_tests --log_level=test_suite 2>/dev/null | grep "testing time" | cut -d' ' -f7 | rev | cut -c 4- | rev | paste -sd+ - | bc;done
9597130
9677749
9644620
9597136
9393690
9542741
9417484
9128269
9600153
9382188

avg: 9.498116s

TheBlueMatt · 2017-05-05T21:18:23Z

Can you scripted-diff this? should be rather trivial to s/GetRandHash/insecure_rand256/?

sipa · 2017-05-23T23:33:18Z

Significant update: the scope is expanded (some more constructions are replaced with more efficient ones), test_random.h was merged into test_bitcoin.h (why was it separate? the code for both was in test_bitcoin.cpp), and some of the changes are done as scripted diffs.

TheBlueMatt · 2017-05-29T19:51:36Z

src/random.cpp

+{
+    std::vector<unsigned char> ret;
+    if (len > 0) {
+        ret.resize(len);


its an unsigned type - just create the vector with the size constructor?

TheBlueMatt · 2017-05-29T19:54:48Z

src/test/cuckoocache_tests.cpp

@@ -23,18 +23,18 @@
 *  using BOOST_CHECK_CLOSE to fail.
 *
 */
-FastRandomContext insecure_rand(true);
+static FastRandomContext local_rand_ctx(true);


In "Rename cuckoo tests' local rand context": Note that you need to make it "scripted-diff" not "script-diff", and also note that on this line its not scripted, should probably separate the static or rename the commit to avoid confusing review.

sipa · 2017-05-30T21:43:50Z

Addressed @TheBlueMatt's comments.

FastRandomContext now provides all functionality that the real Rand* functions provide.

-BEGIN VERIFY SCRIPT- sed -i 's/insecure_rand/local_rand_ctx/g' src/test/cuckoocache_tests.cpp -END VERIFY SCRIPT-

-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-

sipa · 2017-06-05T19:46:47Z

Rebased after merge of #10514.

ryanofsky

utACK b09bd02e3cbbb03758b1467fc2a4bb847142c35d

ryanofsky · 2017-06-07T17:50:32Z

src/test/test_bitcoin.h

+static inline uint64_t insecure_randbits(int bits) { return insecure_rand_ctx.randbits(bits); }
+static inline uint64_t insecure_randrange(uint64_t range) { return insecure_rand_ctx.randrange(range); }
+static inline bool insecure_randbool() { return insecure_rand_ctx.randbool(); }
+static inline std::vector<unsigned char> insecure_randbytes(size_t len) { return insecure_rand_ctx.randbytes(len); }


The new functions could be made camelcase to conform to style guide.

ryanofsky · 2017-06-07T17:54:30Z

src/test/coins_tests.cpp

@@ -155,11 +155,11 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test)
                newcoin.out.nValue = insecure_rand();
                newcoin.nHeight = 1;
                if (insecure_randrange(16) == 0 && coin.IsSpent()) {
-                    newcoin.out.scriptPubKey.assign(1 + (insecure_rand() & 0x3F), OP_RETURN);
+                    newcoin.out.scriptPubKey.assign(1 + insecure_randbits(6), OP_RETURN);


In commit "Replace more rand() % NUM by randranges"

There are a few randbits replacements mixed up in this commit.

Moved to the next commit.

ryanofsky · 2017-06-07T18:15:18Z

src/test/prevector_tests.cpp

                test.insert(insecure_randrange(test.size() + 1), insecure_rand());
            }
-            if (test.size() > 0 && ((r >> 2) % 4) == 1) {
+            if (test.size() > 0 && insecure_randbits(2) == 1) {


In commit "Use randbits instead of ad-hoc emulation in prevector tests"

Can all these == 1, == 2, == 3 checks be replaced with == 0? That would seem like a simpler style.

Let's do that some other time.

-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-

-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-

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

e945848 scripted-diff: Use new naming style for insecure_rand* functions (Pieter Wuille) 2fcd9cc scripted-diff: Use randbits/bool instead of randrange where possible (Pieter Wuille) 2ada678 Use randbits instead of ad-hoc emulation in prevector tests (Pieter Wuille) 5f0b04e Replace rand() & ((1 << N) - 1) with randbits(N) (Pieter Wuille) 3ecabae Replace more rand() % NUM by randranges (Pieter Wuille) efee1db scripted-diff: use insecure_rand256/randrange more (Pieter Wuille) 1119927 Add various insecure_rand wrappers for tests (Pieter Wuille) 124d13a Merge test_random.h into test_bitcoin.h (Pieter Wuille) 90620d6 scripted-diff: Rename cuckoo tests' local rand context (Pieter Wuille) 37e864e Add FastRandomContext::rand256() and ::randbytes() (Pieter Wuille) Tree-SHA512: d09705a3ec718ae792f7d66a75401903ba7b9c9d3fc36669d6e3b9242f0194738106be26baefc8a8e3fa6df7c9a35978c71c0c430278a028b331df23a3ea3070

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

8c2f4b8 Expose more parallelism with relaxed atomics (suggested in bitcoin#9938). Fix a test to check the exclusive or of two properties rather than just or. (Jeremy Rubin) Pull request description: This PR is in response to bitcoin#10026 and some feedback on bitcoin#9938. ~Locally, all the checkqueue tests ran 3.2X faster on my machine. The worst offender, `test_CheckQueue_Correct_Random` ran 3.4X faster.~ 1. ~Removes `GetRand()` and replaces it with a single deterministic FastRandomContext instance.~ bitcoin#10321 replicated this 1. Exposes more parallelism with relaxed atomics, increasing chance of catching a bug. This does not change performance on my machine. 1. Makes one test case more restrictive (xor instead of or, see bitcoin#9938). Tree-SHA512: a59dfbee0273c713525a130dfedc1c7ff26f50c2aaca1e94ef5d759b1d6ea6338ffbd97f863b9f6209750d8a788a15fa8ae1bf26774ed2473c520811337e6b00

e945848 scripted-diff: Use new naming style for insecure_rand* functions (Pieter Wuille) 2fcd9cc scripted-diff: Use randbits/bool instead of randrange where possible (Pieter Wuille) 2ada678 Use randbits instead of ad-hoc emulation in prevector tests (Pieter Wuille) 5f0b04e Replace rand() & ((1 << N) - 1) with randbits(N) (Pieter Wuille) 3ecabae Replace more rand() % NUM by randranges (Pieter Wuille) efee1db scripted-diff: use insecure_rand256/randrange more (Pieter Wuille) 1119927 Add various insecure_rand wrappers for tests (Pieter Wuille) 124d13a Merge test_random.h into test_bitcoin.h (Pieter Wuille) 90620d6 scripted-diff: Rename cuckoo tests' local rand context (Pieter Wuille) 37e864e Add FastRandomContext::rand256() and ::randbytes() (Pieter Wuille) Tree-SHA512: d09705a3ec718ae792f7d66a75401903ba7b9c9d3fc36669d6e3b9242f0194738106be26baefc8a8e3fa6df7c9a35978c71c0c430278a028b331df23a3ea3070

Some mistakes where done while backporting bitcoin/bitcoin#10321 Compilation is fixed here

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

Some mistakes where done while backporting bitcoin/bitcoin#10321 Compilation is fixed here

Signed-off-by: pasta <pasta@dashboost.org>

laanwj added the Tests label May 3, 2017

jnewbery reviewed May 3, 2017

View reviewed changes

sipa force-pushed the fast_rand_tests branch from 7666694 to 3413faf Compare May 23, 2017 23:32

sipa force-pushed the fast_rand_tests branch 3 times, most recently from 43566f8 to b68e6a7 Compare May 24, 2017 00:17

TheBlueMatt reviewed May 29, 2017

View reviewed changes

sipa force-pushed the fast_rand_tests branch from b68e6a7 to fcca2c4 Compare May 30, 2017 21:41

sipa mentioned this pull request May 30, 2017

Improve commit-check-script.sh #10480

Merged

sipa force-pushed the fast_rand_tests branch 2 times, most recently from 957999d to e67f766 Compare June 2, 2017 23:46

sipa mentioned this pull request Jun 4, 2017

scripted-diff: Use the C++11 keyword nullptr to denote the pointer literal instead of the macro NULL #10483

Merged

sipa added 5 commits June 5, 2017 12:44

Add FastRandomContext::rand256() and ::randbytes()

37e864e

FastRandomContext now provides all functionality that the real Rand* functions provide.

scripted-diff: Rename cuckoo tests' local rand context

90620d6

-BEGIN VERIFY SCRIPT- sed -i 's/insecure_rand/local_rand_ctx/g' src/test/cuckoocache_tests.cpp -END VERIFY SCRIPT-

Merge test_random.h into test_bitcoin.h

124d13a

Add various insecure_rand wrappers for tests

1119927

sipa force-pushed the fast_rand_tests branch from e67f766 to b09bd02 Compare June 5, 2017 19:46

ryanofsky reviewed Jun 7, 2017

View reviewed changes

sipa added 5 commits June 7, 2017 11:34

Replace more rand() % NUM by randranges

3ecabae

Replace rand() & ((1 << N) - 1) with randbits(N)

5f0b04e

Use randbits instead of ad-hoc emulation in prevector tests

2ada678

Warrows added a commit to Warrows/PIVX that referenced this pull request Jul 8, 2019

[Tests] Fix compilation

091ce25

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

Warrows added a commit to Warrows/PIVX that referenced this pull request Jul 9, 2019

[Tests] Fix compilation

a4fd5ea

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

Warrows added a commit to Warrows/PIVX that referenced this pull request Jul 9, 2019

[Tests] Fix compilation

40fb155

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

Warrows added a commit to Warrows/PIVX that referenced this pull request Jul 31, 2019

[Tests] Fix compilation

8e19443

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

sickpig mentioned this pull request Oct 24, 2019

[port] Update BU random number generator BitcoinUnlimited/BitcoinUnlimited#1997

Merged

CaveSpectre11 pushed a commit to CaveSpectre11/PIVX that referenced this pull request Dec 14, 2019

[Tests] Fix compilation

37b6a22

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

str4d mentioned this pull request Jan 29, 2020

util: CBufferedFile fixes zcash/zcash#4265

Merged

wqking pushed a commit to wqking-temp/Vitae that referenced this pull request Apr 22, 2020

[Tests] Fix compilation

9f6412a

Some mistakes where done while backporting bitcoin/bitcoin#10321 Compilation is fixed here

Kokary pushed a commit to Kokary/wagerr that referenced this pull request May 26, 2020

Fix compilation

03d792f

Some mistakes where done while backporting bitcoin/bitcoin#10321 Compilation is fixed here

Kokary pushed a commit to Kokary/wagerr that referenced this pull request Nov 13, 2020

Fix compilation

306c924

Some mistakes where done while backporting bitcoin/bitcoin#10321 Compilation is fixed here

Kokary pushed a commit to Kokary/wagerr that referenced this pull request Nov 17, 2020

Fix compilation

2f17052

Some mistakes where done while backporting bitcoin/bitcoin#10321 Compilation is fixed here

Kokary pushed a commit to Kokary/wagerr that referenced this pull request Nov 17, 2020

Fix compilation

a080c3f

Some mistakes where done while backporting bitcoin/bitcoin#10321 Compilation is fixed here

KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Nov 21, 2020

[Tests] Fix compilation

fff2534

Some mistakes where done while backporting bitcoin#10321 Compilation is fixed here

Kokary pushed a commit to wagerr/wagerr that referenced this pull request Nov 24, 2020

Fix compilation

e971485

Some mistakes where done while backporting bitcoin/bitcoin#10321 Compilation is fixed here

Cryptarchist pushed a commit to wagerr/wagerr that referenced this pull request Nov 30, 2020

Fix compilation

fa8dfab

Some mistakes where done while backporting bitcoin/bitcoin#10321 Compilation is fixed here

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jul 17, 2021

tests: reenable when backporting Bitcoin bitcoin#10321

87a9465

Signed-off-by: pasta <pasta@dashboost.org>

UdjinM6 pushed a commit to dashpay/dash that referenced this pull request Jul 19, 2021

tests: reenable when backporting Bitcoin bitcoin#10321 (#4273)

4d80e28

Signed-off-by: pasta <pasta@dashboost.org>

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Use FastRandomContext for all tests #10321

Use FastRandomContext for all tests #10321

Uh oh!

Conversation

sipa commented May 2, 2017

Uh oh!

laanwj commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented May 3, 2017

Uh oh!

TheBlueMatt commented May 5, 2017

Uh oh!

sipa commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 30, 2017

Uh oh!

sipa commented Jun 5, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented May 3, 2017 •

edited

Loading