bench: Fix bench_whitelist hang#347
Conversation
real-or-random
left a comment
real-or-random
left a comment
There was a problem hiding this comment.
No other benchmark uses randomness, so it's clear that we should remove it here, too, even just for consistency. But one advantage of using random keys is that the compiler can't optimize because inputs are known. (A "perfect" compiler would optimize away the entire benchmark and replace it with the precomputed results...)
But this is an issue in all benchmarks, including those in upstream, so let's maybe not care about it here unless you're interested in looking into it.
| /* Start with subkey */ | ||
| random_scalar_order(&ssub); | ||
| secp256k1_scalar_get_b32(data.csub, &ssub); | ||
| memset(data.csub, 1, 32); |
There was a problem hiding this comment.
This key has an exceptionally low Hamming weight, and variable-time algorithms such as secp256k1_ecmult may make use of this fact. It's probably a good idea to use "random" keys for a benchmark.
We could still keep the entire benchmark deterministic, e.g., we could hash a counter and use the resulting hashes as keys. (I'm aware that this is similar to using testrand but I believe it's still a good idea to remove the dependency on testrand.)
There was a problem hiding this comment.
Hashing is a good idea. I used an approach similar to that in bench_ecmult.
Fixed in d54270e
|
The interesting question here is whether this hasn't been noticed on CI:
These things should be fixed, too. |
mllwchrry
force-pushed
the
fix-bench-whitelist
branch
2 times, most recently
from
26b745c to
d54270e
Compare
|
I will fix CI to include -zkp benchmarks in a separate PR. |
| secp256k1_scalar_set_b32(scalar, seckey, &overflow); | ||
| CHECK(!overflow); |
There was a problem hiding this comment.
There's also secp256k1_scalar_set_b32_seckey
| random_scalar_order(&son); | ||
| secp256k1_scalar_get_b32(data.online_seckey[i], &son); | ||
| /* Create two keys using different counter values to ensure different keys */ | ||
| generate_scalar(i + 1, &son, data.online_seckey[i]); |
There was a problem hiding this comment.
There's a convention that output arguments go first. (This is documented in secp256k1.h for API functions, and we use the same convention internally.)
There was a problem hiding this comment.
This and the secp256k1_scalar_set_b32_seckey usage should also be changed in bench_ecmult.c. Since it's a minor issue, it is probably not worth opening a PR to upstream. However, we can keep this in mind and contribute it upstream separately if there's an opportunity (e.g., a PR with other style fixes).
mllwchrry
force-pushed
the
fix-bench-whitelist
branch
from
d54270e to
be075fe
Compare
2 participants
Addresses #346