Exhaustive test improvements + exhaustive schnorrsig tests

[sipa]

A few miscellaneous improvements:

• Just use EXHAUSTIVE_TEST_ORDER as order everywhere, rather than a variable
• Move exhaustive tests for recovery module to the recovery module directory
• Make secp256k1_scalar_set_b32 detect overflow correctly for scalar_low (a comment in the recovery exhaustive test indicated why this was the case, but this looks incorrect).
• Change the small test groups so that they include a point with X coordinate 1.
• Initialize the RNG seed, allowing configurating from the cmdline, and report it.
• Permit changing the number of iterations (re-randomizing for each).
• Support splitting the work across cores from the cmdline.

And a big one:

• Add exhaustive tests for schnorrsig module (and limited ones for extrakeys).

[jonasnick]

ACK 634b3e9

I don't really see the point of removing the order variable - was less strenous to read.

Did you notice the low order scalar_set_b32 issue in the exhaustive schnorrsig tests?
I couldn't find a historic reason for the old comment about the infinite loop but the test clearly show that it's wrong.

[sipa]

I don't really see the point of removing the order variable - was less strenous to read.

I'm also ok with replacing it with order everywhere, but in the current code it's a mix, which makes no sense IMO. In theory using a compile-time constant should be faster (modulus operations are very slow for the CPU, but modulus with a known constant can be rewritten using multiply/shift/subtract by the compiler), so I chose that side. I can't measure a difference in performance though, so it probably doesn't matter much.

Did you notice the low order scalar_set_b32 issue in the exhaustive schnorrsig tests?

Sort of, I wanted to add a test that uses an out-of-range s value, but expected it would break things with the low-order scalar implementation that ignored overflows. In trying to fix that, I noticed it wasn't actually necessary in the first place.

[sipa]

Added a commit that changes the exhaustive test groups so they include a point with X coordinate 1 (suggested by @gmaxwell).

[gmaxwell]

nice, utack

[jonasnick]

Nice idea to create groups with an X=1 point!

[sipa]

I've updated https://github.com/sipa/secp256k1/commits/202009_schnorrsig_exhaustive to build on this, including tests with signatures that have R.x=(fieldsize+1). It now catches removing the check on the return value of secp256k1_fe_set_b32(&rx, &sig64[0]).

[real-or-random]

ACK aa48ca9 careful code inspection

My two comments are strictly speaking not this PR. If these are real issues, I guess you could address them here or in another PR, either is fine.

[jonasnick]

ACK aa48ca9

[sipa]

I made another change, making the exhaustive tests correctly initialize the RNG, and adding a way to split the workload (invoke with ./exhaustive_tests 1 "" 8 0 on CPU 0, ./exhaustive_tests 1 "" 8 1 on CPU 1, etc).

I'm done expanding the scope of this PR now. If this is too much I'm happy to split stuff off.

[real-or-random]

Valgrind fails on Travis:
https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/725488761#L573

I made another change, making the exhaustive tests correctly initialize the RNG, and adding a way to split the workload (invoke with ./exhaustive_tests 1 "" 8 0 on CPU 0, ./exhaustive_tests 1 "" 8 1 on CPU 1, etc).

Really invoke the seed with "" ? Doesn't every instance get a different random seed then?

I'm done expanding the scope of this PR now. If this is too much I'm happy to split stuff off.

I think that's ok.

[gmaxwell]

I don't think exhaustive_tests should be actually random. There is a need for 'random' values, but I don't see a lot of gain in having every run use different ones... and they make reproducing issues harder. Is there a reason to not just give it a constant seed?

[sipa]

Really invoke the seed with "" ? Doesn't every instance get a different random seed then?

Yes, for the random part. You can also specify a fixed seed by passing a non-empty hex string.

I don't think exhaustive_tests should be actually random. There is a need for 'random' values, but I don't see a lot of gain in having every run use different ones... and they make reproducing issues harder. Is there a reason to not just give it a constant seed?

Unsure.

At least the gej tests are explicitly rescaled with a random z in every run, and unfortunately, those cannot be selected from an tractably-sized set for exhaustive tests.

The main thing these tests hope to reveal shouldn't be dependent on the choice of the seed, though, so perhaps it's ok to just pick a fixed seed. If so, we can probably also do without the iteration count.

[real-or-random]

Really invoke the seed with "" ? Doesn't every instance get a different random seed then?

Yes, for the random part. You can also specify a fixed seed by passing a non-empty hex string.

Ok, yes. I somehow wrongly assumed that the randomness affects the mapping of work to CPUs, but it does not of course.

I don't think exhaustive_tests should be actually random. There is a need for 'random' values, but I don't see a lot of gain in having every run use different ones... and they make reproducing issues harder. Is there a reason to not just give it a constant seed?

Unsure.

At least the gej tests are explicitly rescaled with a random z in every run, and unfortunately, those cannot be selected from an tractably-sized set for exhaustive tests.

The main thing these tests hope to reveal shouldn't be dependent on the choice of the seed, though, so perhaps it's ok to just pick a fixed seed. If so, we can probably also do without the iteration count.

"Unsure" was my first thought, too. Yes, the main thing here does not depend of on the choice of the seed but I still believe that randomness helps more than it hurts here. For example, it could detect a bug that occurs as a combination of special coordinate value 1 and some z value that we hit with probability 1/10000. This could be missed by the normal tests, even though they're randomized.

[gmaxwell]

I suppose it doesn't hurt. If a user reports a bug that only happens with some particular seeds and they fail to provide their seed-- well that's a bug which wouldn't have otherwise been detected at all.

Just please no "expected failures", the situation in CI (common in bitcoin core but has happened here too) where people are responding to failures by retrying the CI rather than treating the failure as an emergency is a dangerous practice.

[sipa]

Adding setbuf(stdout, NULL); seems to have fixed it...

[real-or-random]

ACK dd29d2e code review and tests pass

[sipa]

Rebased on the now-merged #558, and added exhaustive tests for schnorrsig module too.