make test count iteration configurable by environment variable #851

pull apoelstra wants to merge 2 commits into bitcoin-core:master from apoelstra:2020-11--test-iters changing 1 files +9 −0

apoelstra commented at 1:08 AM on November 23, 2020: contributor

You can set the test iteration count by command-line parameter but it is very hard to control the iteration count used by make check and even harder to control the count when this is run through Bitcoin Core's build system. Using an environment var solves both problems.
make test count iteration configurable by environment variable 0ce4554881
real-or-random commented at 8:38 AM on November 23, 2020: contributor
Nice, that's indeed useful.

ACK 0ce45548813709d828cb3abcc7db4c9ce6e26907 I inspected the diff

Here are a few small annoying things that you can add to this PR if you want. If not, let's consider this a list for another PR:
- Add check that aborts if not count > 0? Technically count == 0 is fine but I think noone wants to run this on purpose as it just disables some tests arbitrary.
- I was tempted to say that we should have a ITERS also for the exhaustive tests but I think that's not true. The default of 2 makes a lot of sense for these, because the randomness is only used to randomize the representation in jacobian coordinates. But we should add the same count > 0 check there.
- Document the environment variables. Since we don't have a .md for tests, the canonical place is in the "usage" output. (This is not optimal because the benchmarks have an env variable but no "usage" output. Yeah, ok.)
jonasnick approved
jonasnick commented at 1:30 PM on November 23, 2020: contributor

ACK 0ce45548813709d828cb3abcc7db4c9ce6e26907
elichai commented at 6:59 PM on November 23, 2020: contributor
utACK 0ce45548813709d828cb3abcc7db4c9ce6e26907
Add check that aborts if not count > 0? Technically count == 0 is fine but I think noone wants to run this on purpose as it just disables some tests arbitrary.
I also wanted to do some sanitizing on the bench env var but hadn't gotten to doing it yet https://github.com/bitcoin-core/secp256k1/blob/ca4906b02e69644d83e13b3c09cc9147fc1232a5/src/bench.h#L124-L131
apoelstra commented at 7:00 PM on November 23, 2020: contributor

deadalnix was using count==0 :) https://github.com/bitcoin-core/secp256k1/issues/528
elichai commented at 7:02 PM on November 23, 2020: contributor

deadalnix was using count==0 :) #528

lol I didn't fix yet because I think those env vars are for "advanced users only" and only tests/benchmarks so it doesn't matter too much
apoelstra commented at 7:17 PM on November 23, 2020: contributor

I'm happy to add a count>0 check if people think it's valuable.

I don't think it's worth exposing a way to change the exhaustive test iteration count. I didn't even know there was one.

Unfortunately there is no usage text for the tests binary :). I could add an unconditional message above the "count =" line.
sipa commented at 7:47 PM on November 23, 2020: contributor

The count variable for the exhaustive tests changes the z coordinate for gej elements (which is randomized, not exhaustive).

I don't think there is much need to changing the default, though.
real-or-random commented at 9:44 AM on November 24, 2020: contributor

deadalnix was using count==0 :) #528

yeah but even he says:

My concern here isn't really that 0 is a very useful thing to test for, but that it put the system is some weird state that reveal a bug either in libsecp256k1 or possibly in gcc's optimizer.

So I think we should just enforce > 0. But either way is fine.

I don't think there is much need to changing the default, though.

Indeed.

in src/tests.c:5635 in 1316092d68 outdated

5630 | +        const char* env = getenv("SECP256K1_TEST_ITERS");
5631 | +        if (env) {
5632 | +            count = strtol(env, NULL, 0);
5633 | +        }
5634 | +    }
5635 | +    if (count == 0) {

real-or-random commented at 9:26 AM on November 25, 2020:

Maybe just

    if (count <= 0) {

real-or-random approved
real-or-random commented at 11:29 AM on November 26, 2020: contributor

ACK a729a6c6bfb07929c56222fc43cba790d9e5c083 I inspected the diff but didn't test

in src/tests.c:5636 in a729a6c6bf outdated

5631 | +        if (env) {
5632 | +            count = strtol(env, NULL, 0);
5633 | +        }
5634 | +    }
5635 | +    if (count <= 0) {
5636 | +        fputs("An iteration count of 0 is not allowed.\n", stderr);

jonasnick commented at 2:20 PM on November 30, 2020:

nit: <= 0

jonasnick commented at 2:21 PM on November 30, 2020: contributor

ACK mod nit
forbid a test iteration of 0 or less f4fa8d226a
apoelstra force-pushed on Nov 30, 2020
jonasnick approved
jonasnick commented at 8:26 PM on November 30, 2020: contributor

ACK f4fa8d226a95e42b252c07edb425c446370e01c0
real-or-random approved
real-or-random commented at 8:12 AM on December 1, 2020: contributor

ACK f4fa8d226a95e42b252c07edb425c446370e01c0
jonasnick merged this on Dec 1, 2020
jonasnick closed this on Dec 1, 2020
apoelstra deleted the branch on Dec 3, 2020
Fabcien referenced this in commit bd3ec0df37 on Apr 8, 2021
deadalnix referenced this in commit ecfb8204d1 on Apr 9, 2021

Contributors

Linked (view graph)

#528 Test with count == 0 and optimization disabled trip a verify