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

Implement gadgets for append-only MT with rate-3 Rescue #144

Merged
merged 25 commits into from
Nov 22, 2022

Conversation

tessico
Copy link
Contributor

@tessico tessico commented Nov 22, 2022

Description

Replacement for #138 after mt-refactor has been merged.


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

alxiong and others added 25 commits November 8, 2022 00:16
* Initial commit

* structured + impl

* Base merkle tree impl

* merkle_tree_impl

* generic indextype

* Refactored code for further SMT impl.

* Batch insertion implementation.

* Major refactoring & forget implementation.

* Remember implemented.

* cargo fmt

* Big refactor. Leaf now digest index to prevent forge attack.

* unit test for Forgettable merkle tree.

* cleanup

* Clear Arity & replace F::zero() with default()

* trait bound refactor

* New example.

* cargo fmt

* More example.

* ElementType->Element; rename trait, impl and module; remove missing license check

* IndexType refactor

* Refactor DigestAlgorithm

* Pull out Element,Index,NodeValue as traits; add trait bound to unconstrainted generic struct/fn

* Refactor capacity interface

* Renaming & better crate import.

* var renaming

* MerkleProof API refactor

Co-authored-by: Alex Xiong <[email protected]>
* A new macro for general merkle tree implementation
* Better trait bound for data types
* New Universal Merkle tree implementation, including non-membership proof
* Better comments for examples.
prelude now includes 2 canonical instantiation of rescue merkle tree.
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. Well done 👍

Copy link
Contributor

@chancharles92 chancharles92 left a comment

Choose a reason for hiding this comment

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

Also LGTM 👍

@tessico tessico merged commit f0fe27f into main Nov 22, 2022
@tessico tessico deleted the implement-mt-gadgets branch November 22, 2022 20:07
Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

Sorry for my late review, but I feel that this PR didn't address the ergonomic issue I raised up in the first place in #88.

other problems include: while our MT impl has become very generic, our gadget is only supports RescueMerkleTree::<F> <-- this is not reflective just from our gadget API.

Concretely, the gadget API can be more intuitive imo. See my comments here: #88 (comment)

Comment on lines +105 to +106
/// Circuit variable for an accumulated element.
pub struct CommittedElemVar {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a good name? what about AccumulatedElemVar? (I'm not sure what does "committed" mean in this context")

Alternatively, I think removing this struct completely is probably also not a bad idea. Sometimes I feel this is an unnecessary abstraction.

@tessico
Copy link
Contributor Author

tessico commented Nov 25, 2022

@alxiong You're right in that it's a gadget very specific for one concrete instantiation of the MT. As discussed here, we are still planning to make it more generic.

I think you have a good point in re-opening the issue. This PR was intended more as a refresh of our old MT gadget (which was also for a concrete instantiation of an MT) to match with the new API introduced in mt-refactor branch, rather than re-designing the APIs.

Some related tasks:

  1. WIP PR for refactoring the sponge APIs: Sponge API refactor #146
  2. issue for making MerkleTreeGadget more generic: Refactor MerkleTreeGadget to be generic over HashGadget #142
  3. now the re-opened [API] Refactor Merkle tree and its gadget  #88

I guess the above order is only natural given how they depend on one another, with potentially some though on doing 2nd and 3rd in parallel to avoid too much redesign.

@alxiong
Copy link
Contributor

alxiong commented Nov 25, 2022

I see, makes sense! thanks for the clarification.

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.

4 participants