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

Implement ZIP-244 authorizing data commitment (auth_digest) #2547

Merged
merged 6 commits into from
Aug 13, 2021
Merged

Implement ZIP-244 authorizing data commitment (auth_digest) #2547

merged 6 commits into from
Aug 13, 2021

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Jul 29, 2021 •
edited by dconnolly
Loading

Motivation

ZIP-244 specifies a new digest called "authorizing data commitment" or "auth_digest" that binds to authorization data (signatures, proofs). This must be implemented for Nu5.

Part of #2048

Specifications

https://zips.z.cash/zip-0244
https://zips.z.cash/zip-0225#modifications-to-zip-244

Designs

N/A

Solution

This adds a new method in Transaction to compute the auth_digest. It uses librustzcash to compute it, just like we do for the transaction ID and sighash.

Review

This is a requirement for ZIP-221 work, but since that's ongoing it's not urgent.

@dconnolly or @teor2345 might want to review, but I think anyone else can review it too.

Reviewer Checklist

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

Follow Up Work

N/A

This change is Reviewable

@teor2345 teor2345 requested a review from dconnolly July 30, 2021 02:59
@teor2345
Copy link
Contributor

I'm happy to leave this review to @dconnolly.

@conradoplg conradoplg marked this pull request as draft August 9, 2021 17:58
@conradoplg conradoplg marked this pull request as ready for review August 9, 2021 20:54
@conradoplg conradoplg mentioned this pull request Aug 11, 2021
Merged
3 tasks
teor2345
teor2345 suggested changes Aug 11, 2021
View reviewed changes
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.

We need to pad auth data merkle trees with zeroes:
https://zips.z.cash/zip-0244#block-header-changes

(The transaction ID merkle tree pads with the previous transaction ID.)

dconnolly
dconnolly suggested changes Aug 12, 2021
View reviewed changes
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.

Mostly small things, but teor++ on making sure we getting the different paddings right between the transaction merkle tree and the auth digest merkle tree, however we want to do that

conradoplg
conradoplg commented Aug 12, 2021
View reviewed changes
Copy link
Collaborator Author

@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.

Fixed the AuthDataRoot computation and added some tests

zebra-chain/src/block/merkle.rs Outdated Show resolved Hide resolved
zebra-chain/src/block/merkle.rs Outdated Show resolved Hide resolved
zebra-chain/src/block/merkle.rs Outdated Show resolved Hide resolved
zebra-chain/src/block/merkle.rs Outdated Show resolved Hide resolved
zebra-chain/src/block/merkle.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction.rs Show resolved Hide resolved
zebra-chain/src/transaction/auth_digest.rs Outdated Show resolved Hide resolved
teor2345
teor2345 approved these changes Aug 12, 2021
View reviewed changes
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 to me.

I have a suggestion about a follow-up optimisation ticket, but it's not blocking.

zebra-chain/src/block/merkle.rs Show resolved Hide resolved
@teor2345 teor2345 mentioned this pull request Aug 13, 2021
Merged
3 tasks
@conradoplg conradoplg mentioned this pull request Aug 13, 2021
Closed
conradoplg
conradoplg commented Aug 13, 2021
View reviewed changes
Copy link
Collaborator Author

@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.

Waiting for @dconnolly's approval

zebra-chain/src/block/merkle.rs Show resolved Hide resolved
dconnolly
dconnolly approved these changes Aug 13, 2021
View reviewed changes
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.

Awesome!

@dconnolly dconnolly merged commit eadca72 into main Aug 13, 2021
@dconnolly dconnolly deleted the auth-digest branch August 13, 2021 16:58
mpguerra added a commit that referenced this pull request Aug 16, 2021
@mpguerra
Update CHANGELOG.md
d3831f7 
Add #2547 to changelog
Clarify partial implementation of ZIP-221
@mpguerra mpguerra mentioned this pull request Aug 16, 2021
Merged
6 tasks
mpguerra added a commit that referenced this pull request Aug 16, 2021
@mpguerra @teor2345
CHANGELOG for Zebra 1.0.0-alpha.15 (#2613)
13651d4 
* WIP: Copy unformatted changelog from github

* Categorise and refactor changelog entries

* Update CHANGELOG.md

Add missing whitespace

Co-authored-by: teor <[email protected]>

* Apply suggestions from teor's review

* Update CHANGELOG.md

Add #2547 to changelog
Clarify partial implementation of ZIP-221

* Update release date

Co-authored-by: teor <[email protected]>
@conradoplg conradoplg mentioned this pull request Aug 20, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daira daira daira left review comments

@teor2345 teor2345 teor2345 approved these changes

@dconnolly dconnolly dconnolly approved these changes

Assignees

@conradoplg conradoplg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@conradoplg @teor2345 @dconnolly @daira