test, ci: Lower default iteration count to 16

[hebasto]

The number of test iterations in the CI remains the same.

Resolves #1561.

$ ./build/src/tests 
test count = 16
random seed = 59ea2b21267ec0ef0b4d13821292489f
random run = 2936c044f82c7598a866869b9d954d42
no problems found

[jonasnick]

As I mentioned in the linked issue, I don't think this is a great idea. The tests have been written with a higher test iterations in mind. Lowering the test iterations can result in some crucial tests being missed with some probability, for example, because a run of the tests may always draw points with an even Y coordinate. As your test output shows, some tests are even skipped explicitly.

On the other hand, always testing with 64 iterations is overkill. Hence, it would be reasonable to reduce the default test iterations to some number higher than 4. For example, 16 or 8. I am not sure what impact this has on the runtime of the test suite.

[real-or-random]

On the other hand, always testing with 64 iterations is overkill. Hence, it would be reasonable to reduce the default test iterations to some number higher than 4. For example, 16 or 8. I am not sure what impact this has on the runtime of the test suite.

I agree, this will be the easiest. If Core is happy with 16 or 8, we should just this. (Also happy to increase skip limit of run_sha256_known_output_tests 1000000 above the lowered default. This test is a bit overkill then.)

Alternatively, you could reduce the number in Core if that's easy. I'm not sure if it's easy to pass a command line argument if CMake's test stuff handles all of this automatically. But Core could set SECP256K1_TEST_ITERS, that should be easy in the build system.

[jonasnick]

This PR was discussed during our IRC meeting. Even with iters = 16, the "iteration count too low"-warnings do not go away. This is misleading because it suggests to the user that something went wrong.

The test_ecmult_constants_2bit warning will be printed for any iters < 35, due to these lines:

CONDITIONAL_TEST(35, "test_ecmult_constants_2bit") {
    test_ecmult_constants_2bit();
}

However, this is only an optimization to make the tests run faster for lower iters; test_ecmult_constants_2bit does not read the test iterations. Thus, one possible approach is to just remove the CONDITIONAL_TEST and let test_ecmult_constants_2bit(); run always. We could also skip the test without printing a warning. That's possible, but quite fragile and hacky.

Similarly, to make the tests run with iters = 8 without warnings, we would have to remove the conditional
test macro from the line

CONDITIONAL_TEST(16, "run_sha256_known_output_tests 1000000") ninputs++;`

Quick benchmarks (--enable-dev-mode) on my dev machine (please verify):

./tests: 80s
./tests 16: 21s,
./tests 16 with test_ecmult_constants_2bit turned on: 28s
./tests 8 with both test_ecmult_constants_2bit and run_sha256_known_output_tests turned on: 20s

So we could get a significant speedup relatively easily, without losing the ecmult_constants_2bit and sha256_known_output tests.

However, now that I am thinking about it again, running the tests always will also lead to the CI jobs taking longer. We're reducing the iters significantly, sometimes to only iter = 2 for the valgrind tests. Maybe instead of removing the CONDITIONALS entirely, we should reduce the required iterations to 16 or 8.

In any case, this approach is just quick and dirty solution to reduce the default test duration. In the future, we should rethink and rework the tests to make them both much faster and exhaustive.

[real-or-random]

./tests 16 with test_ecmult_constants_2bit turned on: 28s

This one sounds reasonable. The 1000000 bytes test for SHA256 is really a bit overkill for low iters.

However, now that I am thinking about it again, running the tests always will also lead to the CI jobs taking longer. We're reducing the iters significantly, sometimes to only iter = 2 for the valgrind tests. Maybe instead of removing the CONDITIONALS entirely, we should reduce the required iterations to 16 or 8.

I tend to think that the "skip" messages should be kept. They can be helpful, and I don't think these scare anyone. We don't even use the word warning. We could also prefix them with "note: ".

So here's a concrete suggestion;

• reduce the default to 16 iterations
• keep using 64 in our CI
• CONDITIONAL_TEST(16, "test_ecmult_constants_2bit") (so it's run by default)
• CONDITIONAL_TEST(32, "run_sha256_known_output_tests 1000000") (so it's not run by default)
• Keep the "skip" messages.

[hebasto]

So here's a concrete suggestion;

Thanks! Implemented.

[real-or-random]

utACK 9743bce

I'm obviously Concept ACK for my own suggestion, but I totally don't have a strong opinion here. Okay, this reads like an invitation for bikeshedding. It's just that I made the call, and now that I look at the meeting notes again, it appears to me that I ignored points brought up in the meeting, e.g., "if a test is not run by default, it should not produce a warning when not run".

[jonasnick]

The difference in the test duration when I replace 32 in

CONDITIONAL_TEST(32, "run_sha256_known_output_tests 1000000") ninputs++;

with 16 is less than 1 second on my machine. Can someone confirm? If that's true, we can keep it at 16 (as in master) and do not print the message.

[hebasto]

The difference in the test duration when I replace 32 in

CONDITIONAL_TEST(32, "run_sha256_known_output_tests 1000000") ninputs++;

with 16 is less than 1 second on my machine. Can someone confirm? If that's true, we can keep it at 16 (as in master) and do not print the message.

I can confirm.

On this PR branch @ 9743bce:

$ time ./build/src/tests
test count = 16
random seed = 660c3a508327a7b0913e0a7599218c1e
Skipping run_sha256_known_output_tests 1000000 (iteration count too low)
random run = 2edadae86b5b9e8bceac0864645cfb14
no problems found

real	0m33.367s
user	0m33.313s
sys	0m0.051s

With the following diff:

--- a/src/tests.c
+++ b/src/tests.c
@@ -475,7 +475,7 @@ static void run_sha256_known_output_tests(void) {
 
     /* Skip last input vector for low iteration counts */
     ninputs = sizeof(inputs)/sizeof(inputs[0]) - 1;
-    CONDITIONAL_TEST(32, "run_sha256_known_output_tests 1000000") ninputs++;
+    CONDITIONAL_TEST(16, "run_sha256_known_output_tests 1000000") ninputs++;
 
     for (i = 0; i < ninputs; i++) {
         unsigned char out[32];

it takes a bit longer:

$ time ./build/src/tests
test count = 16
random seed = 59ea2b21267ec0ef0b4d13821292489f
random run = 2936c044f82c7598a866869b9d954d42
no problems found

real	0m34.618s
user	0m34.568s
sys	0m0.050s

[hebasto]

If that's true, we can keep it at 16 (as in master) and do not print the message.

Done.

Thank you!

[jonasnick]

ACK 0f73caf

[sipa]

utACK 0f73caf