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.

Sign up for GitHub

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

Jump to bottom

Implement OneshotMerkleTree #43

Merged
merged 14 commits into from
Aug 29, 2022
Merged

Implement OneshotMerkleTree #43

merged 14 commits into from
Aug 29, 2022

Conversation

TomTaehoonKim
Copy link
Contributor

No description provided.

posgnu
posgnu reviewed Aug 13, 2022
View reviewed changes
merkle-tree/src/oneshot.rs Outdated Show resolved Hide resolved
merkle-tree/src/oneshot.rs Outdated Show resolved Hide resolved
junha1
junha1 reviewed Aug 15, 2022
View reviewed changes
common/src/lib.rs Outdated
Ok(())
} else {
Err(MerkleProofError::UnmatchedRoot(
std::str::from_utf8(&root.hash).unwrap().to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

hash is not utf8-encoded. use hex::encode() instead to create a readable string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied.

merkle-tree/src/oneshot.rs Outdated
first_hash
};

Hash256::hash([first_hash, second_hash].concat())
Copy link
Member

Choose a reason for hiding this comment

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

Let's add Hash256::aggregate(&self, other: &Self) -> Self and use it here and in MerkleProof

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied.

merkle-tree/src/oneshot.rs Outdated
pub fn create_merkle_proof(&self, mut key: Hash256) -> MerkleProof {
let mut merkle_proof: MerkleProof = MerkleProof { proof: Vec::new() };

// Self::merkle_proof(&self.hash_list, key, proof)
Copy link
Member

Choose a reason for hiding this comment

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

What is this comment for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary comment. It was a code supposed to be deleted due to refactoring. Deleted.

merkle-tree/src/oneshot.rs Outdated
merkle_tree
}

fn hash_pair(pair: &[Hash256]) -> Hash256 {
Copy link
Member

Choose a reason for hiding this comment

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

What does this function do? Why does this takes slice while its name is 'pair'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added comments explaining what the function does. Please check.

posgnu
posgnu previously approved these changes Aug 16, 2022
View reviewed changes
@TomTaehoonKim TomTaehoonKim force-pushed the feature/oneshotmerkletree branch 4 times, most recently from bf62723 to cfd9903 Compare August 20, 2022 12:58
junha1
junha1 reviewed Aug 21, 2022
View reviewed changes
merkle-tree/src/oneshot.rs Outdated
mod test {
use simperby_common::{crypto::Hash256, MerkleProof};

use super::OneshotMerkleTree;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the new line between uses.
Also use use super::* instead, for a test module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

merkle-tree/src/oneshot.rs Outdated
/// 5 6
/// 1 2 3 4
/// ```
/// which is represented as [[1, 2, 3, 4], [5, 6], [7]].
Copy link
Member

Choose a reason for hiding this comment

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

Would you update the example with [1,2,3]? (general case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied

common/src/lib.rs Outdated
} else {
calculated_root = Hash256::aggregate(&calculated_root, hash);
}
println!("{:?}", hex::encode(calculated_root.hash));
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

merkle-tree/src/oneshot.rs Show resolved Hide resolved
common/src/lib.rs Outdated
@@ -102,7 +102,7 @@ impl BlockHeader {

#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)]
pub struct MerkleProof {
// TODO
pub proof: Vec<(Hash256, u8)>,
Copy link
Member

Choose a reason for hiding this comment

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

  1. use bool
  2. put doc-comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@TomTaehoonKim TomTaehoonKim force-pushed the feature/oneshotmerkletree branch 2 times, most recently from 65d8e82 to 0354eba Compare August 21, 2022 15:33
@TomTaehoonKim TomTaehoonKim mentioned this pull request Aug 23, 2022
Closed
@TomTaehoonKim TomTaehoonKim force-pushed the feature/oneshotmerkletree branch 3 times, most recently from d1095b0 to a63893c Compare August 26, 2022 13:16
junha1
junha1 reviewed Aug 28, 2022
View reviewed changes
common/src/lib.rs Outdated
pub enum MerkleTree {
// (pair_hash, is_right_node)
HasSibling(Hash256, bool),
HasNoSibling,
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for my miscommunication, but what I was think of is
HasRightSibling and HasLeftSibling. This will improve the overall readability .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Fixed.

merkle-tree/src/oneshot.rs Outdated
unimplemented!()
///
/// Given a tree [[1, 2, 3], [4, 5], [6]],
/// Merkle proof for 2 is [(1, false), (5, true)] and Merkle proof for 3 is [(OnlyChildNode, true), (4, false)].
Copy link
Member

Choose a reason for hiding this comment

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

Please update the description (no boolean)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

junha1
junha1 reviewed Aug 29, 2022
View reviewed changes
common/src/lib.rs Outdated
}

#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)]
pub enum MerkleTree {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it supposed be MerkleProofEntry?
MerkleTree is more like Vec<Vec<Hash256>>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

common/src/lib.rs Outdated
pub fn verify(&self, root: Hash256, data: &[u8]) -> Result<(), MerkleProofError> {
let mut calculated_root: Hash256 = Hash256::hash(data);
for node in &self.proof {
calculated_root = if let MerkleTree::LeftChild(pair_hash) = node {
Copy link
Member

Choose a reason for hiding this comment

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

Using match will look better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@junha1 junha1 merged commit 276dc16 into postech-dao:main Aug 29, 2022
@TomTaehoonKim TomTaehoonKim deleted the feature/oneshotmerkletree branch August 29, 2022 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junha1 junha1 junha1 approved these changes

@posgnu posgnu posgnu left review comments

Assignees

@posgnu posgnu

@junha1 junha1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@TomTaehoonKim @posgnu @junha1