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.

Sign up for GitHub

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

Jump to bottom

Add iso-Pallas, SWU hash-to-curve, and Sinsemilla #17

Merged
merged 28 commits into from
Apr 29, 2021
Merged

Add iso-Pallas, SWU hash-to-curve, and Sinsemilla #17

merged 28 commits into from
Apr 29, 2021

Conversation

defuse
Copy link
Collaborator

@defuse defuse commented Mar 30, 2021

No description provided.

@defuse @nuttycom
Add iso-Pallas, SWU hash-to-curve, and Sinsemilla
c3a70e2 
Co-authored-by: Kris Nuttycombe <kris.nuttycombe@gmail.com>
@defuse defuse mentioned this pull request Mar 30, 2021
Closed
@defuse
Copy link
Collaborator Author

defuse commented Mar 30, 2021

I tried reducing the duplication between Pallas and iso-Pallas using inheritance or ownership but it turned out to be too error-prone, i.e. hard to be sure that neither curve wasn't accidentally using the operations of the other, so I left the duplication in.

daira
daira reviewed Apr 1, 2021
View reviewed changes
orchard_iso_pallas.py Outdated Show resolved Hide resolved
daira
daira reviewed Apr 1, 2021
View reviewed changes
orchard_pallas.py Outdated Show resolved Hide resolved
daira
daira reviewed Apr 1, 2021
View reviewed changes
orchard_sinsemilla.py Outdated Show resolved Hide resolved
daira
daira reviewed Apr 1, 2021
View reviewed changes
orchard_sinsemilla.py Outdated Show resolved Hide resolved
daira
daira reviewed Apr 1, 2021
View reviewed changes
orchard_sinsemilla.py Outdated Show resolved Hide resolved
daira
daira reviewed Apr 1, 2021
View reviewed changes
orchard_sinsemilla.py Outdated Show resolved Hide resolved
daira
daira reviewed Apr 1, 2021
View reviewed changes
orchard_sinsemilla.py Outdated Show resolved Hide resolved
@daira
Apply suggestions from code review
816cfa0
@daira
Remove trailing spaces, and make orchard_iso_pallas.py and orchard_si…
9184c78 
…nsemilla.py executable.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira
orchard_sinsemilla.py: corrections, and add test vector for Sinsemill…
28370d5 
…aHash.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
dconnolly
dconnolly reviewed Apr 2, 2021
View reviewed changes
orchard_sinsemilla.py Outdated Show resolved Hide resolved
@dconnolly
Copy link
Contributor

Can we use render_* in __main__ to make it easy to get the Rust/zcash/json renderings of these values in a reproducible way? 🙏

defuse
defuse commented Apr 6, 2021
View reviewed changes
orchard_pallas.py Outdated Show resolved Hide resolved
@defuse
Copy link
Collaborator Author

defuse commented Apr 6, 2021 •
edited
Loading

utACK @daira's changes aside from the Fp.ZERO suggested change

@daira @defuse
Update orchard_pallas.py
57c2894 
Replace 0 with Fp.ZERO in `extract`.

Co-authored-by: Taylor Hornby <taylor@defuse.ca>
@str4d
Copy link
Contributor

str4d commented Apr 20, 2021

Can we use render_* in __main__ to make it easy to get the Rust/zcash/json renderings of these values in a reproducible way? 🙏

Agreed; we shouldn't be doing any non-trivial assertions in __main__ here, since this is where we generate the test vectors.

@str4d str4d mentioned this pull request Apr 20, 2021
Merged
@str4d str4d added this to the Core Sprint 2021-14 milestone Apr 20, 2021
daira
daira reviewed Apr 20, 2021
View reviewed changes
orchard_sinsemilla.py Outdated Show resolved Hide resolved
daira
daira previously approved these changes Apr 20, 2021
View reviewed changes
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.

ACK

Base automatically changed from orchard to master April 21, 2021 00:09
str4d
str4d requested changes Apr 21, 2021
View reviewed changes
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.

Reviewed with @therealyingtong.

orchard_iso_pallas.py Show resolved Hide resolved
orchard_sinsemilla.py Outdated Show resolved Hide resolved
orchard_sinsemilla.py Outdated Show resolved Hide resolved
orchard_sinsemilla.py Outdated Show resolved Hide resolved
orchard_sinsemilla.py Outdated Show resolved Hide resolved
orchard_sinsemilla.py Outdated Show resolved Hide resolved
orchard_sinsemilla.py Outdated Show resolved Hide resolved
orchard_sinsemilla.py Outdated Show resolved Hide resolved
orchard_sinsemilla.py Outdated Show resolved Hide resolved
orchard_sinsemilla.py Outdated Show resolved Hide resolved
@daira daira dismissed their stale review April 21, 2021 11:42

Dismissing my review while I check against @str4d's comments.

daira
daira reviewed Apr 21, 2021
View reviewed changes
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.

Confirmed @str4d's comments.

str4d
str4d requested changes Apr 27, 2021
View reviewed changes
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.

utACK once the bug below is fixed.

orchard_pallas.py Outdated Show resolved Hide resolved
@defuse @str4d
Fix broken incomplete addition case check
86c2796 
Co-authored-by: str4d <jack@z.cash>
str4d
str4d approved these changes Apr 27, 2021
View reviewed changes
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.

utACK

daira
daira reviewed Apr 27, 2021
View reviewed changes
orchard_group_hash.py Outdated Show resolved Hide resolved
daira
daira reviewed Apr 27, 2021
View reviewed changes
orchard_group_hash.py Outdated Show resolved Hide resolved
@daira
orchard_group_hash.py: don't apply iso_map as part of map_to_curve_si…
322aff1 
…mple_swu.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira
orchard_group_hash: minor refactoring.
634c599 
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira
orchard_group_hash: add test vectors for map_to_curve_simple_swu.
e981001 
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
daira
daira approved these changes Apr 27, 2021
View reviewed changes
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 with minor comments.

I added three commits for direct testing of map_to_curve_simple_swu; these should also be reviewed.

@dconnolly dconnolly mentioned this pull request Apr 27, 2021
Closed
str4d
str4d requested changes Apr 28, 2021
View reviewed changes
orchard_group_hash.py Outdated Show resolved Hide resolved
orchard_group_hash.py Outdated Show resolved Hide resolved
orchard_group_hash.py Outdated Show resolved Hide resolved
@str4d
Split apart MapToCurveSimpleSWU and GroupHash test vectors
10bdd6c 
The test vector generators in this repository are meant to output data
that can be copy-pasted or piped to a file. Generating multiple sets of
test vectors from a single file interferes with this.
daira
daira approved these changes Apr 29, 2021
View reviewed changes
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 with comments.

@str4d
Fix Rand.u8() to return unsigned integers
c6cd47a 
Existing test vector generators are adjusted to use Rand.i8() so they
generate the same test vectors. We should evaluate these later to
determine whether they should actually use Rand.u8() (and update the
test vectors across the ecosystem).
@str4d
Generate random test vectors for MapToCurve, GroupHash, Sinsemilla
9cb9e0f
@daira
Apply suggestions from code review
f8008a0 
Comments and a minor refactor for consistency.
str4d
str4d approved these changes Apr 29, 2021
View reviewed changes
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 f8008a0

@str4d
Copy link
Contributor

str4d commented Apr 29, 2021

I'm happy that the few remaining open comment threads have resolutions.

@str4d str4d merged commit 0cc3147 into master Apr 29, 2021
@daira daira mentioned this pull request Jun 2, 2021
Closed
4 tasks
@nuttycom nuttycom deleted the sinsemilla-test-vectors branch June 2, 2021 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@therealyingtong therealyingtong therealyingtong left review comments

@str4d str4d str4d approved these changes

@daira daira daira approved these changes

@dconnolly dconnolly dconnolly left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Core Sprint 2021-16
Development

Successfully merging this pull request may close these issues.
6 participants
@defuse @dconnolly @str4d @daira @therealyingtong @r3ld3v