This removes the ability to test against OpenSSL, as well as the OpenSSL verification benchmark.
The motivation is that OpenSSL 3 is deprecating part of the API used here (see #869), and I’m not sure it’s worth maintaining. We do lose the fact that this is the only test that verifies randomly-generated cases against an independent implementation. On the other hand, there are tons of existing fixed tests now that test all kinds of edge cases already.
Hm yeah, this has a history. I’d probably be fine with removing this but when we last wanted to get rid of the OpenSSL tests (https://github.com/bitcoin-core/secp256k1/pull/635), the conclusion was that we rather want to replace it by some other implementation (https://github.com/bitcoin-core/secp256k1/issues/691). @elichai has two old PRs with different implementations (https://github.com/bitcoin-core/secp256k1/pull/738 and #641). Now there’s also https://github.com/LLFourn/secp256kfun by @LLFourn, which would be another good option (but needs Rust).
I also suggested removing the benchmarks in #736. This issue didn’t get a lot of feedback but I assume removing the benchmark is not controversial.
Concept ACK
Now with #984, the tests even fail. This means that not only the API of OpenSSL has changed v3 but also the behavior. This makes the tests unsuitable for us.
This PR should also remove this change to Makefile.am https://github.com/bitcoin-core/secp256k1/pull/735/files#diff-0462e381b2fb3286568215681c8983490a37ac9ae0f0c5ee304df7fa6426d4af
I agree that cross-testing is useful. I don’t see such a problem with adapting the code calling into a library after a major release. But fine with me to remove it for now. Since the alternatives for cross-testing have tradeoffs as well, the easiest solution may be to reintroduce openssl 3 at some point. Perhaps once it is more popular. I don’t have easy access to v3 right now to play with it.
Another quick fix would be to check that OPENSSL_VERSION_NUMBER in the AC_COMPILE_IFELSE statement of SECP_OPENSSL_CHECK is less than 3.
The commit lgtm.
@real-or-random Added this:
0 bench_verify_LDADD = libsecp256k1.la $(SECP_LIBS) $(SECP_TEST_LIBS) $(COMMON_LIB)
1-# SECP_TEST_INCLUDES are only used here for CRYPTO_CPPFLAGS
2-bench_verify_CPPFLAGS = $(SECP_TEST_INCLUDES)
3 bench_sign_SOURCES = src/bench_sign.c
Could just restrict it to the older version of OpenSSL?
Yeah, we could. My thinking is that maintaining them wouldn’t help too much. The cross tests helped a lot when the library was young but now it’s unclear how much we gain from them. It’s probably better to replace them by another independently written library.
