Skip to content
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

Conversation

real-or-random
Copy link
Contributor

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).

@gmaxwell
Copy link
Contributor

gmaxwell commented Jun 5, 2019

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.

@gmaxwell
Copy link
Contributor

gmaxwell commented Jun 5, 2019

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.

@real-or-random
Copy link
Contributor Author

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.)

@real-or-random
Copy link
Contributor Author

@elichai Maybe https://github.com/elichai/ecc-secp256k1 is a candidate? ;)

@elichai
Copy link
Contributor

elichai commented Jun 18, 2019

What kind of tests would you like to see added?
Random private key and data, then signing and verifying by both libraries?
(and if both are deterministic we can even compare sigs)

DER tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants