Skip to content
This repository was archived by the owner on Oct 31, 2023. It is now read-only.

chore: replace check_membership with compute_merkle_root #143

Merged
TomAFrench merged 9 commits intomasterfrom
calculate-merkle-root
Apr 26, 2023
Merged

chore: replace check_membership with compute_merkle_root #143
TomAFrench merged 9 commits intomasterfrom
calculate-merkle-root

Conversation

@TomAFrench
Copy link
Copy Markdown
Member

@TomAFrench TomAFrench commented Apr 26, 2023

Related to noir-lang/acvm#66

I've also inlined the testing for merkle trees into pwg.rs. A lot of the merkle logic is unnecessary imo as it's related to constructing a merkle tree rather than just proving against an existing one. I'm then planning on removing this in a later PR.

Comment thread acvm_backend_barretenberg/src/acvm_interop/pwg.rs Outdated
Comment thread acvm_backend_barretenberg/src/acvm_interop/pwg.rs Outdated
Comment thread acvm_backend_barretenberg/src/acvm_interop/pwg.rs Outdated
@vezenovm
Copy link
Copy Markdown
Collaborator

A lot of the merkle logic is unnecessary imo as it's related to constructing a merkle tree rather than just proving against an existing one. I'm then planning on removing this in a later PR.

Yeah it is only related to constructing a merkle tree and is really only useful for tests. We should be careful about removing things that we may have to potentially bring back. If we ever implement any sparse merkle tree functions or natively enable merkle trees of different arity we will want to expand upon these merkle tree classes

@TomAFrench TomAFrench changed the title chore: replace check_membership with calculate_merkle_root chore: replace check_membership with compute_merkle_root Apr 26, 2023
@TomAFrench
Copy link
Copy Markdown
Member Author

We should be careful about removing things that we may have to potentially bring back.

I'm of the opinion that that's best left to git to handle. If we need a merkle tree implementation then we can fish it back from the git history, otherwise we need to do all the work to maintain this code all while never using it.

These tests don't feel particularly useful to me as the vast majority of the complexity is actually in the testing code (the merkle tree implementation to construct the tree rather than the function which is verifying it).

* master:
  chore: migrate the verifier contract to `acvm-backend-barretenberg` (#140)
  chore: inline contents of `common/proof.rs` into `acvm-backend-barretenberg` (#141)
@vezenovm
Copy link
Copy Markdown
Collaborator

vezenovm commented Apr 26, 2023

These tests don't feel particularly useful to me as the vast majority of the complexity is actually in the testing code (the merkle tree implementation to construct the tree rather than the function which is verifying it).

For the basic interop tests I agree. But the tests that use compute_merkle_root are useful. I also have used them to update inputs to our nargo integration tests that use merkle trees.

@vezenovm
Copy link
Copy Markdown
Collaborator

I'm of the opinion that that's best left to git to handle. If we need a merkle tree implementation then we can fish it back from the git history, otherwise we need to do all the work to maintain this code all while never using it.

The dead code can be removed in a separate PR

@TomAFrench TomAFrench added this pull request to the merge queue Apr 26, 2023
Merged via the queue into master with commit 9b29e74 Apr 26, 2023
@TomAFrench TomAFrench deleted the calculate-merkle-root branch April 26, 2023 18:04
TomAFrench added a commit that referenced this pull request Apr 26, 2023
* master:
  chore: replace `check_membership` with `compute_merkle_root`  (#143)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants