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

Update wycheproof testvectors to v1 #154

Closed
wants to merge 6 commits into from

Conversation

Sajjon
Copy link
Contributor

@Sajjon Sajjon commented Mar 24, 2023

Update some Wycheproof testvectors (v0) to new testvectors_v1, which in general include more tests per file.

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

If you've made changes to gyb files

  • I've run .script/generate_boilerplate_files_with_gyb and included updated generated files in a commit of this pull request

Motivation:

We ought to use as many and as up to date test vectors as possible. Wycheproofs v0 were created 4 years ago and were due some upgrade.

Modification & Result

(I took the liberty to merge these to sections together)

More tests are now run πŸŽ‰ below is a table over all changes and result.

Testvector Previous name V0 number of tests V1 number of tests Source code unchanged? Test code unchanged?
x25519_test unchanged 87 518 βœ… βœ…
ed25519_test eddsa_test 111 150 βœ… JSON: changed single key
aes_gcm_test unchanged 217 313 βœ… βœ…
chacha20_poly1305_test unchanged 151 325 βœ… βœ…
ecdh_secp256r1_ecpoint_test unchanged 99 355 βœ… βœ…
ecdh_secp256r1_test unchanged 340 612 βœ… Logic: fatalError -> failable init

I can upgrade all...

... but I wanted to hear first that you think it a good idea...

If so, should I do it in a single PR? I.e. add more commits to this PR?

@Sajjon
Copy link
Contributor Author

Sajjon commented Mar 28, 2023

@swift-server-bot test this please

@Sajjon
Copy link
Contributor Author

Sajjon commented Mar 28, 2023

Ah maybe I lack access to trigger swift-server-bot to run tests... one can always try πŸ˜…

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 28, 2023

Good news though, I can tackle them!

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 28, 2023

@swift-server-bot test this please

@Sajjon
Copy link
Contributor Author

Sajjon commented Mar 28, 2023

@Lukasa would you be interested in me continuing with the rest of the vectors? I mean to say, if you don't think it a good idea, best I don't waste the time πŸ˜….

If you think it it a good idea, do you prefer all upgraded in the same PR? Or split across many?

Let me know :)

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 30, 2023

Hey @Sajjon, sorry for letting this sit here. I haven't lost track of it, I promise. I think in general this is something we want to accept, but I think splitting across PRs will make life easier.

@Sajjon
Copy link
Contributor Author

Sajjon commented Mar 30, 2023

Great, I will split into multiple ones and referenced them here and then close this one. Might be some
Days before I can do that though.

Talk soon!

@Sajjon
Copy link
Contributor Author

Sajjon commented Apr 2, 2023

@Lukasa I've split this PR into six new ones, I will continue with the rest later next week. Should we have an umbrella issue for all PRs? Since they are strongly related. I can create that, and then we can close this PR.

Let me know how you wanna proceed! :)

@Lukasa
Copy link
Collaborator

Lukasa commented Apr 3, 2023

The first 6 are merged, thanks!

@Sajjon
Copy link
Contributor Author

Sajjon commented Apr 3, 2023

Great, I can maybe do some more later this week.

@Sajjon
Copy link
Contributor Author

Sajjon commented Apr 4, 2023

Closed in favour of umbrella issue: #165

@Sajjon Sajjon closed this Apr 4, 2023
@Sajjon Sajjon deleted the update_wycheproof_testvectors_to_v1 branch April 4, 2023 08:10
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.

None yet

2 participants