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

Partial Merkle tree implementation #156

Merged
merged 9 commits into from
Jun 14, 2023
Merged

Partial Merkle tree implementation #156

merged 9 commits into from
Jun 14, 2023

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented May 30, 2023

Implementation of basic functionality for Partial Merkle tree similar to Sparse Merkle tree.
(De)serialization of this structure will be implemented in future PR.

@Fumuran Fumuran force-pushed the andrew-partial-mt branch 4 times, most recently from ee270c1 to 10b0532 Compare May 30, 2023 19:43
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! This is not a full review yet - but I left some comments which will probably lead to some significant changes - so, I can review other parts once these are implemented.

src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran requested a review from bobbinth June 1, 2023 17:15
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good! I left some more comments inline.

src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/tests.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran requested review from bobbinth and removed request for bobbinth June 2, 2023 18:59
@Fumuran Fumuran force-pushed the andrew-partial-mt branch 2 times, most recently from a2ee4db to 69df6ed Compare June 2, 2023 19:57
@Fumuran Fumuran requested a review from bobbinth June 2, 2023 20:12
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good! I left some more comments inline.

src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/tests.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-partial-mt branch 2 times, most recently from 9197420 to 6f0c749 Compare June 5, 2023 15:15
@Fumuran Fumuran requested a review from bobbinth June 5, 2023 16:20
Copy link
Contributor

@bobbinth bobbinth left a 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 left a few more comments inline.

src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/tests.rs Show resolved Hide resolved
src/merkle/partial_mt/tests.rs Show resolved Hide resolved
src/merkle/partial_mt/tests.rs Show resolved Hide resolved
src/merkle/partial_mt/tests.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good. I left a few more comments inline. Also, a couple of general comments about tests:

  • When a test has several things that it is doing, it would be good to have a brief comment explaining what is being tested.
  • We should probably add tests for error conditions. For example, adding two Merkle paths which resolve to different roots should be an error.

src/merkle/partial_mt/tests.rs Show resolved Hide resolved
src/merkle/partial_mt/tests.rs Show resolved Hide resolved
src/merkle/partial_mt/tests.rs Show resolved Hide resolved
src/merkle/partial_mt/tests.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
src/merkle/partial_mt/mod.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran requested a review from bobbinth June 13, 2023 13:16
Copy link
Contributor

@bobbinth bobbinth left a 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!

For the next PR, let's add some additional methods (e.g., inner_nodes()) and anything else that we'd need to make this a drop-in replacement for MerklePathSet.

@bobbinth bobbinth merged commit 53d52b8 into next Jun 14, 2023
@bobbinth bobbinth deleted the andrew-partial-mt branch June 14, 2023 05:10
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.

2 participants