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
Concept ACK
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.
Concept ACK, I agree there's no need to wait for a replacement for cross testing to get in before we remove OpenSSL from the tests and benchmarks (But maybe it's a good time to re-inatiate this discussion in the dedicated issue)
@real-or-random Added this:
bench_verify_LDADD = libsecp256k1.la $(SECP_LIBS) $(SECP_TEST_LIBS) $(COMMON_LIB)
-# SECP_TEST_INCLUDES are only used here for CRYPTO_CPPFLAGS
-bench_verify_CPPFLAGS = $(SECP_TEST_INCLUDES)
bench_sign_SOURCES = src/bench_sign.c
Could just restrict it to the older version of OpenSSL?
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.
ACK bc08599e776aff33c834ef829843ec5f629d1f39
ACK bc08599e776aff33c834ef829843ec5f629d1f39
tACK bc08599
I'm new to a bunch of this but I know I can succeed