-
Notifications
You must be signed in to change notification settings - Fork 41
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
mmr: added partial mmr #195
Conversation
It is not obvious how the code works at first glance, so here is a visualization:
flowchart BT
id1[1]
id2((2))
id3((3))
id4((4))
id5[5]
id6((6))
id7((7))
id8((8))
id9((9))
id10((10))
id11((11))
id12((12))
id13((13))
id14((14))
id15((15))
id16((16))
id17[17]
id18((18))
id19((19))
id20((20))
id21((21))
id22((22))
id23((23))
id24((24))
id25((25))
id26((26))
id27((27))
id28((28))
id29((29))
id30((30))
id31((31))
id1---id2
id3---id2
id5---id6
id7---id6
id9---id10
id11---id10
id13---id14
id15---id14
id17---id18
id19---id18
id21---id22
id23---id22
id25---id26
id27---id26
id29---id30
id31---id30
id2---id4
id6---id4
id10---id12
id14---id12
id18---id20
id22---id20
id26---id28
id30---id28
id4---id8
id12---id8
id20---id24
id28---id24
id8---id16
id24---id16
classDef unused stroke-width:4px,stroke-dasharray:5,fill:white;
classDef peak fill:yellow;
classDef untracked fill:white;
class id27,id29,id31 unused;
class id26,id30 unused;
class id28 unused;
class id24 unused;
class id16 unused;
class id17,id18,id21,id23 untracked;
class id4,id9,id10,id11,id13,id14,id15 untracked;
class id1,id5 untracked;
class id25,id20,id8 peak;
Updates to the above structure depend on the state of the new full Mmr (not represented). The update procedure works as follows:
Examples:
|
a487c68
to
4495590
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thank you! The trick with in-order indexing is especially cool.
This is not a full review yet (I still need to internalize a few things), but I left some comments inline. Most of them are nits/typos.
4495590
to
6638035
Compare
f3e20b1
to
39f0a0f
Compare
4061285
to
1471309
Compare
1471309
to
2771d26
Compare
one note: a thing that annoyed me while working on this, is that I have used |
2771d26
to
5613a94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I added a few more comments inline - again mostly nits.
One thing that I think would be great to do to improve code clarity/readability is to create a wrapper type for Forest
- but this is probably best left for a subsequent PR (but let's create an issue for this).
5daa919
to
2b74429
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Thank you! One last thing that we should do is update the changelog. Once this is done, we can merge.
2b74429
to
bde20f9
Compare
Describe your changes
Note: I'm adding a few more test, a preliminary review can be useful but I will push some additional code today.
This differs significantly to the #86 . That PR implemented a data structure that could track a single element, i.e. a single authentication path from a Mmr. This implements a structure that can track an arbitrary number of elements (from just the peaks/0 leaves, up to every leaf in the Mmr).
Design choices
Vec<MerklePath>
. Each element in the vector was one authentication path to be updated in isolation. Issues:PartialMmr
one would need to iterate over all theMerklePath
sVec<RpoDigest>
with separate parent/child links asVec<usize>
pointers. Notes:Some additional notes about this implementation: