test, ci: Lower default iteration count to 16

hebasto commented at 10:55 am on August 12, 2024: member

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

Resolves #1561.

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

jonasnick commented at 11:55 am on October 21, 2024: contributor

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 commented at 12:58 pm on October 21, 2024: contributor

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 commented at 6:32 pm on October 21, 2024: contributor

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:

0CONDITIONAL_TEST(35, "test_ecmult_constants_2bit") {
1    test_ecmult_constants_2bit();
2}

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

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

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

0./tests: 80s
1./tests 16: 21s,
2./tests 16 with test_ecmult_constants_2bit turned on: 28s
3./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 commented at 11:15 am on October 22, 2024: contributor

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

real-or-random added the label assurance on Oct 22, 2024

hebasto force-pushed on Oct 22, 2024

hebasto commented at 2:59 pm on October 22, 2024: member

So here’s a concrete suggestion;

Thanks! Implemented.

hebasto renamed this:
~~test, ci: Lower default of iters to 4~~
test, ci: Lower default iteration count to 16
on Oct 22, 2024

real-or-random commented at 8:31 pm on October 22, 2024: contributor

utACK 9743bce984ed3677a35cc21789295b66bc780053

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 commented at 1:55 pm on October 30, 2024: contributor

The difference in the test duration when I replace 32 in

0CONDITIONAL_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 commented at 2:26 pm on October 30, 2024: member

The difference in the test duration when I replace 32 in
0CONDITIONAL_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 @ 9743bce984ed3677a35cc21789295b66bc780053:

0$ time ./build/src/tests
1test count = 16
2random seed = 660c3a508327a7b0913e0a7599218c1e
3Skipping run_sha256_known_output_tests 1000000 (iteration count too low)
4random run = 2edadae86b5b9e8bceac0864645cfb14
5no problems found
6
7real	0m33.367s
8user	0m33.313s
9sys	0m0.051s

With the following diff:

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

it takes a bit longer:

0$ time ./build/src/tests
1test count = 16
2random seed = 59ea2b21267ec0ef0b4d13821292489f
3random run = 2936c044f82c7598a866869b9d954d42
4no problems found
5
6real	0m34.618s
7user	0m34.568s
8sys	0m0.050s

test, ci: Lower default iteration count to 16

The number of test iterations in the CI remains unchanged.

Additionally, the minimum iteration counts to enable the
`test_ecmult_constants_2bit` test is adjusted from 35 to 16, so it is
run by default.

0f73caf7c6

hebasto force-pushed on Oct 30, 2024

hebasto commented at 2:31 pm on October 30, 2024: member

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

Done.

Thank you!

jonasnick approved

jonasnick commented at 6:38 pm on October 30, 2024: contributor

ACK 0f73caf7c62564dd4a28cdf682ea9644d1be0ef4

sipa commented at 6:41 pm on October 30, 2024: contributor

utACK 0f73caf7c62564dd4a28cdf682ea9644d1be0ef4

real-or-random merged this on Nov 1, 2024

real-or-random closed this on Nov 1, 2024

hebasto deleted the branch on Nov 1, 2024

test, ci: Lower default iteration count to 16 #1581