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

BLS test vectors #408

Merged
merged 12 commits into from
May 18, 2023
Merged

BLS test vectors #408

merged 12 commits into from
May 18, 2023

Conversation

tdammers
Copy link
Contributor

@tdammers tdammers commented May 8, 2023

Integrate Rust test vectors with the Haskell BLS code and test suite.

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a few comments

cardano-crypto-tests/src/Test/Crypto/EllipticCurve.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/Util.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/Util.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/src/Test/Crypto/Util.hs Outdated Show resolved Hide resolved
cardano-crypto-tests/cardano-crypto-tests.cabal Outdated Show resolved Hide resolved
@lehins
Copy link
Collaborator

lehins commented May 15, 2023

It's quite peculiar.
@tdammers and @iquerejeta do you guys know why are the tests failing on Windows?
In particular, is it a problem with tests or the actual implementation?

@tdammers
Copy link
Contributor Author

tdammers commented May 16, 2023

@tdammers and @iquerejeta do you guys know why are the tests failing on Windows?
In particular, is it a problem with tests or the actual implementation?

I think I know.

The error originates in Base16.decode, specifically here; it occurs when the length of the bytestring to be decoded is not even (which it has to be because each byte encodes to two hex digits).

And the reason it is uneven on Windows is because (as per review comment) we now use ByteString IO instead of String IO and lines from Data.ByteString.Char8 instead of the Prelude one on String. My hypothesis is that it goes like this:

  • git on the Windows CI machines is configured to automatically convert line endings on checkout (I think that's the default on Windows), so the hex files have \r\n line endings on Windows CI, but plain \n on Linux and OSX.
  • We then open the file in binary mode (Data.ByteString.readFile; previously this was System.IO.readFile, which uses text mode), so the \r is retained.
  • Data.ByteString.Char8.lines then splits on \n; it does not consider \r to be a newline character, and does not strip it away.
  • The bytestring we feed to Data.ByteString.Base16.decode now has a trailing \r attached to it on Windows (but not on the other OSes, where git does not inject \r, because plain \n is the native line ending convention).

The quick-and-dirty solution: just cull the \r characters, e.g. using BS8.filter (/= '\r').

The morally correct, but more elaborate and potentially less performant solution: read the file into a Data.Text (this will use openFile, not openBinaryFile), line-split (still on Text), UTF8-encode each line into a ByteString, and then do the hex decoding.

@iquerejeta iquerejeta marked this pull request as ready for review May 16, 2023 13:58
@iquerejeta iquerejeta force-pushed the tdammers/bls-test-vectors branch from 2af6728 to 41d6ece Compare May 16, 2023 14:30
@tdammers tdammers force-pushed the tdammers/bls-test-vectors branch from 41d6ece to 2af6728 Compare May 16, 2023 14:33
@tdammers tdammers force-pushed the tdammers/bls-test-vectors branch from 2af6728 to 24045a4 Compare May 16, 2023 14:33
@lehins
Copy link
Collaborator

lehins commented May 16, 2023

@tdammers Oh yeah, Windows newline strikes again. I always forget about it. In that case we can go back to using String for reading and splitting on newlines: map BS8.pack . Prelude.lines <$> Prelude.readFile f. Hex decoding still needs to be done on ByteStrings.

line-split (still on Text),

I thought about the same thing, but unfortunately text package also ignores the CR character, from haddock: "lines does not treat '\r' (CR, carriage return) as a newline character."

@hamishmack hamishmack force-pushed the tdammers/bls-test-vectors branch from 24045a4 to 33a4941 Compare May 17, 2023 00:09
@iquerejeta iquerejeta requested a review from lehins May 17, 2023 06:38
@tdammers
Copy link
Contributor Author

@tdammers Oh yeah, Windows newline strikes again. I always forget about it. In that case we can go back to using String for reading and splitting on newlines: map BS8.pack . Prelude.lines <$> Prelude.readFile f. Hex decoding still needs to be done on ByteStrings.

line-split (still on Text),

I thought about the same thing, but unfortunately text package also ignores the CR character, from haddock: "lines does not treat '\r' (CR, carriage return) as a newline character."

Indeed it does not, but Data.Text.IO.readFile converts newlines as per the system locale, which is exactly what we want in this case. Then again, the documentation for that also suggests to use decodeUtf8 . Data.ByteString.readFile instead, for performance reasons, conveniently failing to mention that this changes the behavior to no longer perform any line ending conversions...

Bottom line, just stripping \r like I did in the last commit is probably the sanest solution here.

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cardano-crypto-class/cardano-crypto-class.cabal Outdated Show resolved Hide resolved
@lehins
Copy link
Collaborator

lehins commented May 17, 2023

Bottom line, just stripping \r like I did in the last commit is probably the sanest solution here.

Yeah, sounds reasonable.

@angerman angerman force-pushed the tdammers/bls-test-vectors branch from 394c170 to d14d21b Compare May 18, 2023 06:24
@iquerejeta iquerejeta merged commit 37dc906 into master May 18, 2023
lehins pushed a commit that referenced this pull request May 18, 2023
* Include Haskell tests for BLS12-381 test vectors

---------

Co-authored-by: iquerejeta <querejeta.azurmendi@iohk.io>
Co-authored-by: Moritz Angermann <moritz.angermann@gmail.com>
lehins pushed a commit that referenced this pull request May 23, 2023
* Include Haskell tests for BLS12-381 test vectors

---------

Co-authored-by: iquerejeta <querejeta.azurmendi@iohk.io>
Co-authored-by: Moritz Angermann <moritz.angermann@gmail.com>
@tdammers tdammers deleted the tdammers/bls-test-vectors branch June 7, 2023 05:44
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.

4 participants