-
Notifications
You must be signed in to change notification settings - Fork 41
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
bls test vectors #397
bls test vectors #397
Conversation
What's going on? Why are we getting PRs with Rust? |
So this is for the test vectors. We were thinking the other day with @tdammers were should the script that creates test vectors for BLS12-381 bindings live. The conclusion was that the best place for that code to live was in the repo where the test-vectors lived, hence cardano-base. However, this is obviously open to discussion if you think it should be outside of the repo. The motivation to put it here is that we could include a test in the CI that checks that the file used for the test vectors is equal to what is generated by the rust script. However, the idea is that we don't put any dependencies on the haskell build with rust. i.e., the rust script only needs to be run by the CI or anyone wanting to re-generate the test vectors. WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a Haskell project. Even tests need to be written in Haskell, not Rust.
In this case looks like the goal is to compare that the functionality matches another implementation.
I would prefer to avoid adding other languages and libraries if they can be avoided. We already depend on libblst
, then why not implement generation of the test vectors in C?
FYI. I don't have anything against Rust, I think it is a great language. But, I do have a problem with addition of unnecessary dependencies! Addition of a new technology increases complexity of a repository significantly.
I understand. The reason why we decided to do the test vectors in rust was to compare the library we use ( I understand wanting to leave rust outside of the build CI, in which case we can upload the rust code in another repo, link to that in the README and only have here the test vectors. If that's your preferred path, we'll go that way 👌 |
I don't understand why would we need any sort of bindings if we wanted to write a C program that generates test vectors. I would expect it to be same as the rust program in this PR, which depends on Rust implementation of bls, except that program would be written in C and depend on then C implementation bls
If you think what I am suggesting is too hard to do then we don't need to waste any time and I am not gonna stand in way of having this code in this repo. After all, it will only be used once to generate the test vectors. If we had to run it on CI and locally every time we had to run the test suite that would be a different story and I would be pushing back much harder against it. |
But we want to create these test vectors with the rust library, which is the 'other' reference implementation of bls12-381. We could do it with the underlying library, but the goal of these test vectors is that they are independently generated.
Then we should discuss this :) my intention was to include a check in CI that the test-vectors file used for tests corresponds to the output of the rust binary. If we don't include such a check in the CI, then I don't see the purpose of including it in the repo, and we can leave it out. |
The output of the Rust program will be checked-in in this repo, no? Then why do we need to run Rust on CI?
Which library are you trying to test? Are you trying to test the My understanding was that the reason for writing this script in Rust was only because it is easier to write Rust, rather than C. |
Apart from checking the output of the Rust program, I was thinking of checking that the test vectors we use are indeed the output of the rust program. Like this we guarantee that the Rust script and the test vectors do not go out of sync.
Yes, the intention is to have tests for the libblst library, as the latter does not contain an extensive test-suite. That's what motivated a different choice of the underlying library. I agree that such tests should not be in the bindings library, but given that we include test vectors in our repo, we might as well have them generated from a different library, and so we kill two birds with one stone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments. Some of the are generic that can be applied in several spots of the script (e.g. the usage of projective point, or the usage of a helper function). Two final comments:
- I'd suggest we put the data files in a different folder
- We now have
bls12-381-test-vectors
that only containstest-vectors-bls12_381
. I'd suggest we put the contents oftest-vectors-bls12_381
directly in the first folder.
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/Cargo.toml
Outdated
Show resolved
Hide resolved
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/Cargo.toml
Outdated
Show resolved
Hide resolved
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/Cargo.toml
Outdated
Show resolved
Hide resolved
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/README.md
Outdated
Show resolved
Hide resolved
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/README.md
Outdated
Show resolved
Hide resolved
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/README.md
Outdated
Show resolved
Hide resolved
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/bls_sig_aug_test_vectors
Outdated
Show resolved
Hide resolved
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/src/main.rs
Outdated
Show resolved
Hide resolved
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/src/main.rs
Outdated
Show resolved
Hide resolved
cardano-crypto-tests/bls12-381-test-vectors/test-vectors-bls12_381/src/main.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@iquerejeta This PR is up your alley. We've already talked about this work, so if you can oversee this work further that'll be great. I suspect Haskell tests will be added in a separate PR, right? |
Haskell tests will be added in a separate PR, yes. I will oversee the PR. If you could approve it to unblock your change request, so that once I approve we can merge. |
1a8c321
to
1fbc1e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some minor suggestions before merging.
Co-authored-by: Iñigo Querejeta Azurmendi <31273774+iquerejeta@users.noreply.github.com>
Include rust script to generate test vectors of the BLS12-381 curve using zkcrypto's implementation of BLS12-381. The generated test vectors themselves are also included as part of cardano-base. Co-authored-by: iquerejeta <querejeta.azurmendi@iohk.io>
Include rust script to generate test vectors of the BLS12-381 curve using zkcrypto's implementation of BLS12-381. The generated test vectors themselves are also included as part of cardano-base. Co-authored-by: iquerejeta <querejeta.azurmendi@iohk.io>
Test vectors for
bls12-381
, still under construction!