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

Document consensus rules from 3.9 Nullifier Sets #3521

Merged
merged 3 commits into from
Feb 16, 2022

Conversation

conradoplg
Copy link
Collaborator

Motivation

We must document all consensus rules.

Specifications

Designs

Solution

I was going over the remaining consensus rules documentation issues and ended up just doing this, just had to reorganize and reformat. (As well as confirming the implementation was there)

Closes #3214

Review

@dconnolly and/or @upbqdn might want to review

Reviewer Checklist

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

Follow Up Work

@teor2345
Copy link
Contributor

Looks like this failed with bug #3489

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 wrote this code and the original comments, so we might also want to get a review from another cryptographer.

@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #3521 (750ba40) into main (499ae89) will increase coverage by 2.21%.
The diff coverage is 79.81%.

@@            Coverage Diff             @@
##             main    #3521      +/-   ##
==========================================
+ Coverage   78.34%   80.55%   +2.21%     
==========================================
  Files         267      274       +7     
  Lines       31526    32256     +730     
==========================================
+ Hits        24698    25983    +1285     
+ Misses       6828     6273     -555     

upbqdn
upbqdn previously approved these changes Feb 14, 2022
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

One note, otherwise LGTM.

zebra-state/src/service/check/nullifier.rs Show resolved Hide resolved
@upbqdn

This comment was marked as resolved.

@teor2345

This comment was marked as resolved.

@teor2345
Copy link
Contributor

By the way, why does this PR change test coverage when it only edits comments? The bot says there will be 854 new hits.

Our main branch coverage seems to be out of date, maybe we need to start running our coverage job on main again.

I opened ticket #3533 to fix this.

Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

I wrote this code and the original comments, so we might also want to get a review from another cryptographer.

I checked the code, comments and consensus rules, and it all looks good to me.

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Feb 16, 2022

update

✅ Branch has been successfully updated

mergify bot added a commit that referenced this pull request Feb 16, 2022
@mergify mergify bot merged commit f6b0dc7 into main Feb 16, 2022
@mergify mergify bot deleted the document-consensus-3.9-nullifier-sets branch February 16, 2022 10:00
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.

Document consensus rules from Zcash spec: Section 3.9 Nullifier Sets
3 participants