EspressoSystems / jellyfish

A Rust Implementation of the PLONK ZKP System and Extensions
https://jellyfish.docs.espressosys.com
MIT License
391 stars 96 forks source link

`MerkleProof` should not include position or leaf value #658

Open ggutoski opened 1 month ago

ggutoski commented 1 month ago

This issue is an opinionated suggestion. Others may disagree.

MerkleProof struct contains:

These items should not be in the merkle proof. Instead, MerkleTree::verify should take them as separate args. If the leaf value is large then the size of the merkle proof is unnecessarily large.

First copy of index position is pos field in `MerkleProof struct: https://github.com/EspressoSystems/jellyfish/blob/92714a4cc509fac07b8e8fc321fc0271c5dbe6b6/merkle_tree/src/internal.rs#L139-L150

Digging into MerklePath we see

https://github.com/EspressoSystems/jellyfish/blob/92714a4cc509fac07b8e8fc321fc0271c5dbe6b6/merkle_tree/src/internal.rs#L84

where MerkleNode is an enum

https://github.com/EspressoSystems/jellyfish/blob/92714a4cc509fac07b8e8fc321fc0271c5dbe6b6/merkle_tree/src/internal.rs#L25

with Leaf variant

https://github.com/EspressoSystems/jellyfish/blob/92714a4cc509fac07b8e8fc321fc0271c5dbe6b6/merkle_tree/src/internal.rs#L37-L47

where we find a second copy of pos and the leaf value elem.

If, as I suggest, we remove these occurrences of pos then MerkleProof becomes just a wrapper for MerklePath. In that case we should eliminate the wrapper and just use MerklePath.

I suggest that MerklePath be an opaque struct. No need to expose the fact that it's a Vec<MerkleNode>.

mrain commented 1 month ago

I tend to keep it in the original form. But this comment is valuable:

If the leaf value is large then the size of the merkle proof is unnecessarily large.

ggutoski commented 1 month ago

Forgot to mention: currently MerkleTree::verify takes a redundant arg pos. The current implementation merely checks pos against one of its copies inside MerkleProof:

https://github.com/EspressoSystems/jellyfish/blob/92714a4cc509fac07b8e8fc321fc0271c5dbe6b6/merkle_tree/src/macros.rs#L81-L90

Instead verify args should include both pos and elem or neither of these items. Seems strange to take only one but not the other. Naturally, my suggestion is to include both and then remove them from MerkleProof.

mrain commented 4 weeks ago

My opinion has changed. We should make these changes along with #642

philippecamacho commented 3 weeks ago

We will go with two kind of Merkle proof: with and without leaf.