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

refactor(anchorSapling): Change type to force consensus rule validation #3544

Merged
merged 8 commits into from
Feb 17, 2022

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to change the anchorSapling type similar to orchard so range consensus rule is automatically validated.

Close #3528

Solution

Change the type from bytes into jubjub::Fq.

Review

There is a lot of changes needed at different files but they are almost all trivial to me expect the one about sorting. I will like some input maybe from @conradoplg there before marking the pull request as ready for review. I made a comment for this in the code.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@oxarbitrage oxarbitrage marked this pull request as draft February 15, 2022 22:03
Copy link
Contributor

@teor2345 teor2345 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, I checked for panics and serialization.

But I want someone else to double-check the cryptography.

zebra-chain/src/block/commitment.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/arbitrary.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/shielded_data.rs Show resolved Hide resolved
zebra-chain/src/sapling/spend.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/spend.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/tree.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction/serialize.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #3544 (d08cfd3) into main (499ae89) will increase coverage by 2.12%.
The diff coverage is 78.71%.

@@            Coverage Diff             @@
##             main    #3544      +/-   ##
==========================================
+ Coverage   78.34%   80.46%   +2.12%     
==========================================
  Files         267      274       +7     
  Lines       31526    32276     +750     
==========================================
+ Hits        24698    25971    +1273     
+ Misses       6828     6305     -523     

Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks great! It just needs a couple of fixes

zebra-chain/src/sapling/shielded_data.rs Show resolved Hide resolved
zebra-chain/src/block/commitment.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/arbitrary.rs Outdated Show resolved Hide resolved
zebra-chain/src/sapling/arbitrary.rs Outdated Show resolved Hide resolved
@oxarbitrage oxarbitrage marked this pull request as ready for review February 16, 2022 18:11
@conradoplg conradoplg mentioned this pull request Feb 16, 2022
2 tasks
Copy link
Collaborator

@conradoplg conradoplg 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!

@conradoplg
Copy link
Collaborator

It seems there's a fake-activation-heights test failing, I can help track it down tomorrow if needed. (There's also another CI failure that seems devops-related)

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

Looks great!

@mergify mergify bot merged commit 137ae4e into ZcashFoundation:main Feb 17, 2022
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.

Check if anchorSapling is in the correct range
4 participants