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

feat: add keccak256 merkle tree #685

Merged
merged 2 commits into from
Oct 28, 2024
Merged

feat: add keccak256 merkle tree #685

merged 2 commits into from
Oct 28, 2024

Conversation

alxiong
Copy link
Contributor

@alxiong alxiong commented Sep 18, 2024

This PR:

  • introduce macro rules impl_mt_hash_256! for implementing boilerplate for node and digest for MT that has 32 bytes output size
  • impl for both Sha3 and Keccak256 under this macro
  • expose a MT and lightweight MT for keccak256

@mrain do you remember why we pick rate 3 for MT? is that an assumption for our current MT internals?


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 relevant changelog entries to the CHANGELOG.md of touched crates.
  • Re-reviewed Files changed in the GitHub PR explorer

@mrain
Copy link
Contributor

mrain commented Sep 18, 2024

@mrain do you remember why we pick rate 3 for MT? is that an assumption for our current MT internals?

Rescue hash has a rate of 3.

@sveitser I think it's better to instantiate a MT in the application since it's simple enough. If we do it here, people may think that we are enforcing some default instantiations while we are not.

@alxiong
Copy link
Contributor Author

alxiong commented Sep 18, 2024

Rescue hash has a rate of 3.

but our Sha3MT doesn't have to use rate of 3, right? also our implementation is generic of ARITY. I don't know why our Sha3Tree also choose Arity of 3

@sveitser I think it's better to instantiate a MT in the application since it's simple enough. If we do it here, people may think that we are enforcing some default instantiations while we are not.

Softly disagree, the fact that our API is generic already offers flexibility. prelude::* offers good out-of-box options that can be safely ignored.
I don't see downside of adding Keccak256MerkleTree in jellyfish actually.

@mrain
Copy link
Contributor

mrain commented Sep 18, 2024

but our Sha3MT doesn't have to use rate of 3, right? also our implementation is generic of ARITY. I don't know why our Sha3Tree also choose Arity of 3

I just picked a random number because it was supposed to be only an example. If we are making it public we'd better make it generic to ARITY.

Softly disagree, the fact that our API is generic already offers flexibility. prelude::* offers good out-of-box options that can be safely ignored. I don't see downside of adding Keccak256MerkleTree in jellyfish actually.

We could go either way.

mrain
mrain previously approved these changes Oct 24, 2024
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. I think we could include this change in version 0.2.0, and make a new tag after merge.

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!

@alxiong alxiong merged commit 5f06aa1 into main Oct 28, 2024
5 checks passed
@alxiong alxiong deleted the keccak-mt branch October 28, 2024 15:16
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.

2 participants