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

Digest-generic MerkleTreeGadget #167

Merged
merged 6 commits into from
Jan 20, 2023
Merged

Digest-generic MerkleTreeGadget #167

merged 6 commits into from
Jan 20, 2023

Conversation

tessico
Copy link
Contributor

@tessico tessico commented Dec 19, 2022

Description

Making the circuit equivalent of MerkleTree generic over a digest algorithm is more tricky, because both the MerkleTreeGadget and DigestAlgorithmGadget need to be implemented for PlonkCircuit.

I'm proposing two designs here. One is to add it as a generic parameter to MerkleTreeGadget, see the last commit. The other option is to add DigestAlgorithmGadget as an associated type on the MerkleTreeGadget trait, see here. Both options work, it's just a matter of convenience and ergonomics. Please let me know your opinions.

(Note that initially I thought that the associated type would not work in the case where in the future we need to provide multiple implementations of the MerkleTreeGadget trait. But since the trait already is generic over MerkleTreeScheme, which, implicitly, will be using the counterpart of DigestAlgorithmGadget, it should still be possible to provide multiple impl even with DigestAlgorithmGadget being an associated type. See the rust book for more explanation on the difference between the two.)

closes: #142


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@tessico tessico requested a review from a team December 19, 2022 15:18
mrain
mrain previously approved these changes Dec 19, 2022
Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

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

Generally LGTM.

@tessico
Copy link
Contributor Author

tessico commented Dec 26, 2022

As per the discussion on the first commit, I've gone back to the initial solution by reverting the second commit.

@tessico tessico requested review from alxiong and mrain December 26, 2022 11:12
@tessico tessico added blocked Blocked by some other issues and removed blocked Blocked by some other issues labels Jan 11, 2023
@tessico tessico requested a review from alxiong January 18, 2023 17:11
Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

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

Excellent work! Will approve if comments are addressed.

@tessico tessico requested a review from mrain January 19, 2023 21:10
Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

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

LGTM

@tessico tessico merged commit 5c3735e into main Jan 20, 2023
@tessico tessico deleted the digest-generic-mt branch January 20, 2023 07:36
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.

Refactor MerkleTreeGadget to be generic over HashGadget
3 participants