Faster fixed-input ecmult tests

[sipa]

Given how much #920 slowed down the tests with low iteration count, replace it with 3 different similar test:

• count >= 1: a test with 1024 multiplies that tests any pattern of 6 bits in windows not more than 20 bits wide
• count >= 3: a test with 2048 multiplies that tests any pattern of 8 consecutive bits
• count >= 35: the old test (which effectively tests all 2-bit patterns)

[real-or-random]

Is this the first test where we run essentially different checks depending on the count variable?

I'm slightly concerned that it introduces an "unexpected" element in the tests.

[robot-dreams]

ACK ed939de

I verified the expected hashes with an independent implementation: https://gist.github.com/robot-dreams/02d27311448bd4cb79bec3ce155bf21a

I also verified on a Linux VM that it's indeed faster:

New

# time valgrind ./tests 2
real	4m48.751s user	3m55.599s sys	0m52.974s

# time valgrind ./tests 4
real	7m2.431s user	6m1.322s sys	1m0.990s

Old

# time valgrind ./tests 2
real	11m44.887s user	10m49.579s sys	0m54.015s

# time valgrind ./tests 4
real	12m25.258s user	11m32.643s sys	0m52.434s

@real-or-random I see your concern. The way I'm thinking about it is, "this test does a bunch of ecmults and checks the result; count just controls how many ecmults are being done".

I agree it'd be nicer if the relationship between count and the actual number (and choice) of ecmults were more direct, but I don't think it's critical. But if you DO think it's critical, then as an alternative I might suggest having lower count correspond to a lower maximum value of i in the test_ecmult_constants_2bit.

[robot-dreams]

This condition was a little hard to understand. Do you have a script that confirms it?

Alternatively, should we replace this with the following easier-to-understand condition, which is analogous to the one below (this script confirms that it holds: https://gist.github.com/robot-dreams/02d27311448bd4cb79bec3ce155bf21a#file-verify_1049_patterns-py):

    /* For every combination of 6 consecutive bit positions, all 64 bit
     * patterns occur in the input scalars used in this test. */

[real-or-random]

@real-or-random I see your concern. The way I'm thinking about it is, "this test does a bunch of ecmults and checks the result; count just controls how many ecmults are being done".

Indeed but your numbers show that it's really significant on valgrind... Maybe for the paranoid, we should document a number where it's guaranteed that all tests run? I guess anyway all tests should run at 64 but maybe even lower? For example, is there a reason why you choose 35? (And not 32? Sorry, I don't want to start bikeshedding over the number, just trying to understand.)

[sipa]

This is indeed somewhat unusual, at least compared to what we had before, where the code paths used never depend on the count variable.

My justification here is that while the count variable now changes the test code paths, it doesn't (or at least, shouldn't) change the code paths being tested. Increasing count just runs it with more (fixed) inputs.

The reasoning is something like: #920 was probably overkill, but I also don't want to throw it away for cases where we have enough time to still run it. So replace it with a significantly cheaper test that tests the same code paths (and mostly the same table entries; possibly even more), but also still run the old code if count is high enough.

If this feels all too arbitrary (which I find understandable), I'm also fine with just replacing the whole thing with only the consecutive-8-bits 2048-point test.

@robot-dreams

patterns = []
for i in range(1, 1<<20, 2):
    if bin(i).count("1") == 6:
        for j in range(256):
            if (i << j) >> 256 == 0:
                patterns.append(i << j)

then for each pattern in patterns, it should should hold that x & pattern takes on 64 different values as x iterates over the scalars in the test.

[real-or-random]

Concept ACK

The reasoning is something like: #920 was probably overkill, but I also don't want to throw it away for cases where we have enough time to still run it.

This was also my initial intuition and I agree. It's not elegant but it's better than throwing tests away. This will also be helpful for other "overkill" tests, e.g., see the discussion about an "million bytes" SHA256 test: #731 (comment)

Maybe this is the time to add a -h / --help and then mention that not all tests may be run when the count is lower than the default.

[sipa]

For example, is there a reason why you choose 35? (And not 32)?

Specifying count=1 does ~1024 multiplications, count=3 does ~1024×3, count=35 does ~1024×35.

[real-or-random]

utACK ed939de

Fine with merging this as-is but I wonder if we can improve the "grepability" of such tests. At the moment git grep "count >" -- src/tests.c works but it's fragile. I wonder if we can introduce a function/macro with a one-line comment to make it sure the skipped tests can be found easily.

edit: The macro could even print a notice like "Skipping test x due to the low iteration count."

[sipa]

@real-or-random Done.

[real-or-random]

ACK 070e772

[real-or-random]

@robot-dreams quick re-review? :)

[robot-dreams]

ACK 070e772, the addition of the CONDITIONAL_TEST macro is nice.

I also searched for \<count\> in tests.c just to make sure there isn't anything else that should use the macro. The only possible case I found was this one, but I'm okay with leaving it as is because it would always still run at least once (count must be positive).