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

Switch to little endian encoding for everything #12

Merged
merged 7 commits into from
May 18, 2018

Conversation

ebfull
Copy link
Collaborator

@ebfull ebfull commented May 17, 2018

@daira
Copy link
Contributor

daira commented May 17, 2018

I'm not sure whether https://github.com/zcash/librustzcash/pull/12/files#diff-745d270700285c73dedf7767652465d7R155 and https://github.com/zcash/librustzcash/pull/12/files#diff-745d270700285c73dedf7767652465d7R159 are correct. They might be, but I recommend dropping the .rev() and implementing a LittleEndianBitIterator.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK

@daira
Copy link
Contributor

daira commented May 17, 2018

My ACK assumes that we will add test vectors generated by @str4d's independent implementation.

@ebfull
Copy link
Collaborator Author

ebfull commented May 18, 2018

We will be adding those test vectors, but this PR will not block on them.

@str4d
Copy link
Contributor

str4d commented May 18, 2018

I have test vectors for key components, but g_d is failing. I'll figure it out tomorrow.

@ebfull
Copy link
Collaborator Author

ebfull commented May 18, 2018

We now have test coverage of Pedersen commitments, hashes, (merkle tree empty roots consistent with zcashd), note commitments, addresses/ivk, group hash, and most (all?) of jubjub.

@str4d
Copy link
Contributor

str4d commented May 18, 2018

g_d was failing in my local test vectors because the first Pedersen base was invalid, due to not rejecting points where v was not in the field. Test vectors in this PR include the fix.

Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

ACK+cov. I generated the test vectors and wrote the tests that use them, so my ACK does not cover those.

@ebfull ebfull merged commit 36d7acf into zcash:master May 18, 2018
str4d referenced this pull request in str4d/librustzcash Aug 28, 2018
str4d referenced this pull request in str4d/librustzcash Aug 28, 2018
ebfull added a commit to ebfull/librustzcash that referenced this pull request Feb 23, 2019
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

3 participants