Skip to content

Conversation

jonas-lj
Copy link
Contributor

No description provided.

@jonas-lj jonas-lj requested a review from kchalkias June 21, 2023 12:52
@jonas-lj jonas-lj marked this pull request as ready for review June 21, 2023 14:34
@jonas-lj jonas-lj requested a review from benr-ml July 24, 2023 06:39
//! # use fastcrypto::hash::Sha256;
//! # use fastcrypto_data::merkle_tree::*;
//! let elements = [[1u8], [2u8], [3u8]];
//! let mut tree = MerkleTree::<32, Sha256, [u8; 1]>::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

const like Sha256Length?

}

impl<const DIGEST_LENGTH: usize, H: HashFunction<DIGEST_LENGTH>, T: AsRef<[u8]>>
MerkleTree<DIGEST_LENGTH, H, T>
Copy link
Contributor

Choose a reason for hiding this comment

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

pls merge with the impl of this struct above

///
/// To avoid second-preimage attacks, a 0x00 byte is prepended to the hash data for leaf nodes (see
/// [LeafHasher]), and 0x01 is prepended when computing internal node hashes (see [InternalNodeHasher]).
pub struct MerkleTree<const DIGEST_LENGTH: usize, H: HashFunction<DIGEST_LENGTH>, T: AsRef<[u8]>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need T here? can we instead have generic insert()?

///
/// To avoid second-preimage attacks, a 0x00 byte is prepended to the hash data for leaf nodes (see
/// [LeafHasher]), and 0x01 is prepended when computing internal node hashes (see [InternalNodeHasher]).
pub struct MerkleTree<const DIGEST_LENGTH: usize, H: HashFunction<DIGEST_LENGTH>, T: AsRef<[u8]>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of requiring AsRef, let's require trait Hashable or just Serialize
(AsRef requires using OnceCell in some cases, which we better avoid unless have to).

MerkleTree<DIGEST_LENGTH, H, T>
{
/// Hash an element using the hash function used for this tree.
pub fn hash(element: &T) -> [u8; DIGEST_LENGTH] {
Copy link
Contributor

Choose a reason for hiding this comment

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

when will this be used?

//! assert!(verifier.verify(index, &elements[index], &proof));
//! ```

use std::borrow::Borrow;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case for this data structure? there are many variants of merkle trees, e.g., key-value, with deletions, etc.
Also, what is the expected number of objects, and the expected insertion/deletion pattern?

Copy link
Collaborator

@kchalkias kchalkias left a comment

Choose a reason for hiding this comment

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

Let's resurrect that PR and get some timings as well (part of our light client story)

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.

3 participants