-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove OpenSSL tests #635
Remove OpenSSL tests #635
Conversation
I don't follow your argument: The openssl tests are largely separated so although they are additional things, they don't increase complexity of other things-- their cost primarily just their maintenance, which has been fairly minimal. If you are getting valgrind errors from openssl it means your openssl was not complied with -DPURIFY, you should go scream at your distribution to fix that-- many distributions compile their openssl with -DPURIFY. The tests with openssl are valgrind clean for me on fedora, for example. An argument I'd make for removing it would be that it's possible that its presence falsely causes people to believe that openssl is a dependency of the library and then causes them to use less well reviewed alternatives. I don't think we have any evidence that this is currently happening, but I'm not sure we would. An argument I'd make against removing it is that we have many useful randomized tests which lack a ground truth. These tests lose a lot of their testing power unless they operate as a comparison against and independent implementation. One possible alternative would be to include a tiny independent implementation (potentially uses GMP for its numbers), has it's own distinct group law (perhaps an entirely affine naive implementation). This would be useful as it would be essentially nearly maintenance-free (GMP api is quite stable) and could plausibly be more independent than openssl (which could have some bugs in common with us). It would also generalize to more functions that we implement that openssl does not. The downside would be that it would take a little effort to create. |
Some history: while openssl was considered consensus critical in Bitcoin testing against it was essential. Now that it isn't -- it isn't. But testing two implementations against each other on 'random' inputs (maybe very non-uniformly random) is a very powerful testing methodology which we use intentionally, have found real issues with, and I think if anything we should be increasing our use of... This, however, doesn't itself require openssl-- we just had that due to the history and because it was easy. |
Okay I'm convinced that cross-testing implementations is a good thing. But then I think the OpenSSL tests are good for that purpose, i.e., it is not worth the time and effort to replace them by some alternative implementation. (If someone wants to give it a try, I'm not against it of course.) |
@elichai Maybe https://github.com/elichai/ecc-secp256k1 is a candidate? ;) |
What kind of tests would you like to see added? DER tests? |
This is for discussion obviously.
think we don't need the tests anymore, and they do somehwat more harm than good, e.g., they make our tests more complex, and valgrind will complain about the use of uninitialized memory in the tests (which is done on purpose in OpenSSL).