Refactor MT & Generic Implementation #120

mrain · 2022-09-15T14:25:08Z

Description

Part of: #88

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

primitives/src/merkle_tree/merkle_tree_impl.rs

jbearer

Just commenting on the interface, haven't looked at the implementation yet.

One thing I think is missing, that we use extensively in wallet/EsQS/validator interactions, is the ability to extract a commitment and frontier from a MerkleTree, and then later rebuild a MerkleTree from the commitment/frontier. The actual commitment/frontier combo can be opaque (e.g. an associated type), the important thing is that there is some commitment operation which gives you something concise which you can then use to reinstantiate a MerkleTree.

primitives/src/merkle_tree/merkle_tree_traits.rs

primitives/src/merkle_tree/merkle_tree_impl.rs

primitives/src/merkle_tree/merkle_tree_traits.rs

alxiong

Good work to kickstart more discussion and refactoring work. 👍

One thing I struggle with, is the unclear abstraction of our trait MT, should we incline towards a Set accumulator or a Vector accumulator?

API like append() or push() is more on the "vector accumulator" side, whereas (unordered) insert is leaning towards "set accumulator"
Our Interval Merkle Tree is insertion order dependent, thus should more behave like a Vector accumulator. (quite honestly, so is our current merkle_tree impl as an append-only MT)

alxiong · 2022-09-20T12:58:31Z

primitives/src/merkle_tree/mod.rs

+mod merkle_tree_impl;
+mod merkle_tree_traits;
+
+pub use merkle_tree_traits::*;


imo,

bring trait definitions up, directly in this merkle_tree/mod.rs (as we did for pcs/mod.rs)

rename trait MerkleTreeScheme or MerkleTreeTrait ?

the actual implementation can be in files such as: standard_mt.rs append_only_mt.rs or maybe forgettable_mt.rs with struct name struct MerkleTree

alxiong · 2022-09-20T13:01:45Z

primitives/src/merkle_tree/merkle_tree_traits.rs

+    /// Merkle tree element type
+    type ElementType: ElementType<F>;


what about type Element: Default + ... + AsRef<[F]>; and remove ElementType?

again,

inline this and also IndexType

add a trait ToFields or AsFieldSlice or sth to capture the extra interface requirement

rename to type Element, similarly type Index

alxiong · 2022-09-20T13:42:57Z

primitives/src/merkle_tree/merkle_tree_traits.rs

+pub trait Hasher<F: Field> {
+    /// Digest a list of values
+    fn digest(data: &[F]) -> F;
+}


some thoughts (debatable):

if we expect a stateless hash function (i.e. no &self or &mut self as input), then we can completely remove this trait, and just accept type Hash: FnOnce(input: &GenericArray<F, Self::LeafArity>) -> F

naming it trait Hasher will potentially collide with std/libcore's Hasher, also, Hasher should be a trait that our RescueHash should implement.

a better name could derive from trait Digest and borrow some API design there.

my current thinking is in favor of option 1.

primitives/src/merkle_tree/merkle_tree_traits.rs

alxiong · 2022-09-20T13:45:59Z

primitives/src/merkle_tree/merkle_tree_traits.rs

+    /// Exsistential proof
+    type Proof;
+    /// Batch proof
+    type BatchProof;


should we call it just Proof, orMerkleProof, MembershipProof, or else?

why the need for BatchProof here? in another word, should batch proof a struct-specific types & API, should it be a universal one?

Proofs should at least type Proof: Clone + Serialize + ..

I hesitate on the word "existential". where do we put non-membership proof then?

alxiong · 2022-09-20T13:49:34Z

primitives/src/merkle_tree/merkle_tree_traits.rs

+pub enum LookupResult<F, P> {
+    /// The value at the given index, and a proof of validity
+    Ok(F, P),
+    /// The index is valid but we do not have the leaf in memory


without trait bound, <F, P> is quite unintelligible.

either explicitly add trait bounds to them

or generic over LookupResult<T: MerkleTreeScheme<F>, F: Field> and Ok(F, T::Proof), since this enum is meaningless without association with a specific MT scheme.

primitives/src/merkle_tree/merkle_tree_traits.rs

alxiong · 2022-09-20T14:04:02Z

primitives/src/merkle_tree/merkle_tree_traits.rs

+pub trait UpdatableMerkleTree<F: Field>: MerkleTree<F> {
+    /// Update the leaf value at a given position
+    /// * `pos` - zero-based index of the leaf in the tree
+    /// * `elem` - newly updated element
+    fn update(&mut self, pos: u64, elem: &Self::ElementType) -> Result<(), PrimitivesError>;
+}


imo, fn update should not be in the trait,

classical categorization of accumulators,
normally you only support insert, if you further support delete, then you are a dynamic accumulator
(I guess "update" really means insert or delete for an accumulator)

This is more about a sparse(key/value) Merkle tree. Where you have to provide a key/index to insert an element.

Or should we call it RandomAccessMerkleTree?
With 2 interfaces provided.

insert(pos, elem) will error if given position is occupied.

overwrite(pos, elem) will ignore whether it's occupied.

mrain · 2022-09-20T14:11:19Z

Good work to kickstart more discussion and refactoring work. 👍

One thing I struggle with, is the unclear abstraction of our trait MT, should we incline towards a Set accumulator or a Vector accumulator?

API like append() or push() is more on the "vector accumulator" side, whereas (unordered) insert is leaning towards "set accumulator"

IMO it's more like a vector/map accumulator. To access an element you need to specify a position to it.

Our Interval Merkle Tree is insertion order dependent, thus should more behave like a Vector accumulator. (quite honestly, so is our current merkle_tree impl as an append-only MT)

UptablableMerkleTree is just implemented. See latest commitment.

alxiong · 2022-10-04T15:20:33Z

primitives/src/merkle_tree/merkle_tree_traits.rs

+    fn verify(
+        &self,
+        pos: Self::IndexType,
+        proof: impl Borrow<Self::MembershipProof>,
+    ) -> Result<bool, PrimitivesError>;


Should we have pos as input param? Intuitively just a proof is enough and practically, the pos info is embedded in proof.

alxiong · 2022-10-06T13:45:06Z

As we discuss before, we want to make our trait MerkleTree and our implementation generic beyond just <F: Field>, I have some ideas, we should discuss and pair-program on this change. @mrain

…icense check

…trainted generic struct/fn

…ems/jellyfish into chengyu/mt-refactor

* enable CI run on mt-refactor branch * Refactor MT & Generic Implementation (#120) * 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 <alex.xiong.tech@gmail.com> * Universal Merkle tree implementation (#132) * 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. * Address comments * add back the module * randomize the lookup test * fixup rebase * WIP * adapt after rebase * Fix the order of hashing elements, zero goes first as per spec * test for all fields * add back the failure case test for merkle root * fix warnings, clean up unused code * further cleanup * consistency in u32 vs u64 type definition in tests * Address review comments: better naming * fix formatting * Add prelude; prelude now includes 2 canonical instantiation of rescue merkle tree. * Adopted from sub PR. * Add an explicit comment about concrete MT type used * rename variables * add back the benches * Merge artefacts cleanup Co-authored-by: Alex Xiong <alex.xiong.tech@gmail.com> Co-authored-by: MRain <linmrain@gmail.com>

mrain added 2 commits September 12, 2022 10:46

Initial commit

5fa7ce2

structured + impl

83900a9

mrain linked an issue Sep 15, 2022 that may be closed by this pull request

[API] Refactor Merkle tree and its gadget #88

Closed

alxiong self-requested a review September 15, 2022 16:52

mrain added 2 commits September 16, 2022 09:21

Base merkle tree impl

84f33a9

merkle_tree_impl

1181952

chancharles92 reviewed Sep 19, 2022

View reviewed changes

primitives/src/merkle_tree/merkle_tree_impl.rs Outdated Show resolved Hide resolved

primitives/src/merkle_tree/merkle_tree_impl.rs Outdated Show resolved Hide resolved

mrain requested a review from tri-joe September 19, 2022 15:26

alxiong requested review from fkrell, jbearer and kmchicoine September 19, 2022 16:09

jbearer reviewed Sep 19, 2022

View reviewed changes

generic indextype

75f4063

alxiong reviewed Sep 20, 2022

View reviewed changes

mrain added 8 commits September 22, 2022 10:52

Refactored code for further SMT impl.

a123229

Batch insertion implementation.

eea99b3

Major refactoring & forget implementation.

e063069

Remember implemented.

88f201b

cargo fmt

a6dd6e5

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

7172b7c

unit test for Forgettable merkle tree.

49c1416

cleanup

813eff4

mrain marked this pull request as ready for review October 3, 2022 17:55

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

772de83

alxiong reviewed Oct 4, 2022

View reviewed changes

mrain added 3 commits October 13, 2022 11:41

trait bound refactor

d565a18

New example.

326f554

cargo fmt

25c9ec4

mrain and others added 13 commits October 18, 2022 01:25

More example.

de914f9

Merge remote-tracking branch 'origin/main' into chengyu/mt-refactor

9b29217

ElementType->Element; rename trait, impl and module; remove missing l…

8f28cfe

…icense check

IndexType refactor

64f4aae

Refactor DigestAlgorithm

1b0dadf

Merge remote-tracking branch 'origin/main' into chengyu/mt-refactor

c1f88cc

Merge remote-tracking branch 'origin/main' into chengyu/mt-refactor

31fd577

Pull out Element,Index,NodeValue as traits; add trait bound to uncons…

bb9b95e

…trainted generic struct/fn

Merge branch 'chengyu/mt-refactor' of https://github.com/EspressoSyst…

d356fff

…ems/jellyfish into chengyu/mt-refactor

Refactor capacity interface

6f3b96f

Renaming & better crate import.

906319b

var renaming

a2ddedc

MerkleProof API refactor

c8577f7

alxiong changed the base branch from main to mt-refactor November 7, 2022 16:16

Merge branch 'mt-refactor' into chengyu/mt-refactor

a3ae277

alxiong merged commit 98059c3 into mt-refactor Nov 7, 2022

alxiong deleted the chengyu/mt-refactor branch November 7, 2022 16:19

mrain mentioned this pull request Nov 9, 2022

[Constraint] MT batch insertion #101

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MT & Generic Implementation #120

Refactor MT & Generic Implementation #120

mrain commented Sep 15, 2022 •

edited by alxiong

Loading

jbearer left a comment •

edited

Loading

alxiong left a comment

alxiong Sep 20, 2022

alxiong Sep 20, 2022

alxiong Oct 4, 2022

alxiong Sep 20, 2022

alxiong Sep 20, 2022

alxiong Sep 20, 2022

alxiong Sep 20, 2022

mrain Sep 20, 2022

mrain Sep 20, 2022

mrain commented Sep 20, 2022

alxiong Oct 4, 2022

alxiong commented Oct 6, 2022 •

edited

Loading

		/// Merkle tree element type
		type ElementType: ElementType<F>;

Refactor MT & Generic Implementation #120

Refactor MT & Generic Implementation #120

Conversation

mrain commented Sep 15, 2022 • edited by alxiong Loading

Description

jbearer left a comment • edited Loading

Choose a reason for hiding this comment

alxiong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrain commented Sep 20, 2022

Choose a reason for hiding this comment

alxiong commented Oct 6, 2022 • edited Loading

mrain commented Sep 15, 2022 •

edited by alxiong

Loading

jbearer left a comment •

edited

Loading

alxiong commented Oct 6, 2022 •

edited

Loading