refactor(PR 13): clean generic hash for all zerokit's module (prepare for future PoseidonHash2)#424
Merged
Merged
Conversation
Benchmark for 7a3cd66Click to view benchmark
|
Benchmark for 7a3cd66Click to view benchmark
|
sydhds
requested changes
Jul 6, 2026
| /// Defines the interface for a hash function over a prime field, used by all zerokit modules. | ||
| pub trait ZerokitHasher { | ||
| /// Type of the hashed elements, also used as the Merkle tree node type. | ||
| type Fr: Clone + Copy + Eq + Default + Debug + Display + FromStr + Send + Sync; |
Contributor
There was a problem hiding this comment.
Please find another name because the name "Fr" conflicts with the ark type Fr here.
I don't think we need so many trait bounds no?
Contributor
There was a problem hiding this comment.
Is there a particular need to the trait bounds here? You restricted the set but it still seems to me that we don't need any no?
Contributor
Author
There was a problem hiding this comment.
Yes, each one has its use, you can check by removing any of them, the compiler will tell you to put it back
- Copy: tree nodes are passed around by value everywhere (root(), sibling reads, H::hash(&[left, right]))
- PartialEq: verify() compares the recomputed root with the stored one
- Default: ZerokitMerkleTree::default(depth) uses Scalar::default() as the empty-leaf value
- Debug: the tree and proof types derive Debug
- Send + Sync: the rayon parallel hashing path sends nodes across threads
Contributor
There was a problem hiding this comment.
So at the end, you need those trait bounds because you are defining MerkleTree<H: ZerokitHasher> but this would not be the case if you would defined MerkleTree<T, H> where T: Copy, ... , H: Hasher wdyt?
Contributor
Author
There was a problem hiding this comment.
Even if you make the type T separate, you still need T to match the Scalar type in in fn hash():
pub trait ZerokitHasher {
type Scalar;
fn hash(input: &[Self::Scalar]) -> Self::Scalar;
}And this would make the call site look like this:
let tree = FullMerkleTree::<Fr, PoseidonHash>::default(20)?;So I'd say the current implementation is much cleaner for the call site.
let tree = FullMerkleTree::<PoseidonHash>::default(20)?;
Contributor
There was a problem hiding this comment.
Why is let tree = FullMerkleTree::<PoseidonHash>::default(20)?; cleaner than let tree = FullMerkleTree::<Fr, PoseidonHash>::default(20)?;
The later shows, I want a MerkleTree storing Fr types and with PoseidonHash so it clearly states the intent of the dev. Also adding trait bounds for the trait Hasher makes no sense because the trait is meant for dev to impl it.
Also another problem of those trait bounds, is that you cannot impl Hasher for SecretFr and that should not be the case no?
Contributor
Author
There was a problem hiding this comment.
Ok so to resolve this:
- I removed all the bounds from
ZerokitHasher(type Scalar;is now just bare). The tree types add the bounds they actually need withwhere H::Scalar: ..., you right about this one, cleaner split. Scalar = SecretFrstill isn't allowed as a tree node, but that's intentional. Tree nodes are public commitments, whileSecretFris deliberately!Copyso secrets can't accidentally make their way into the generic tree/hash code.- For
<T, H>, each hasher already determines exactly one scalar type, so adding a separateTwould just duplicate information the compiler already knows. I'd keep the single-parameter API.
Contributor
There was a problem hiding this comment.
I still don't understand why you don't want to add the generic T? Any particular reason for this? It does improve the intent as already mentioned but you rejected it without any reason...
Contributor
Author
There was a problem hiding this comment.
The reason I prefer the single-parameter API is that Scalar isn't an independent choice, it's determined by H through the associated type. Any <T, H> design would still need H: ZerokitHasher<Scalar = T>, so T is always duplicate information that H already carries.
One consequence is that a two-parameter API allows expressing combinations that are invalid like:
let tree = FullMerkleTree::<WrongFr, PoseidonHash>::default(20)?;But PoseidonHash already defines its scalar type:
impl ZerokitHasher for PoseidonHash {
type Scalar = Fr;
}We can definitely make PoseidonHash generic over any field element type as well, but that's outside the scope of this refactor, since entire codebase is still use arkworks types (Fr, and SecretFr wrapper).
Benchmark for 0975ccdClick to view benchmark
|
Benchmark for 0975ccdClick to view benchmark
|
sydhds
requested changes
Jul 7, 2026
Benchmark for 150fe1cClick to view benchmark
|
Benchmark for 150fe1cClick to view benchmark
|
…w convention, simplify generic type and trait bound
vinhtc27
force-pushed
the
refactor/pr13-generic-hash
branch
from
July 7, 2026 16:08
1db3048 to
428454e
Compare
Benchmark for 99e8499Click to view benchmark
|
Benchmark for 99e8499Click to view benchmark
|
Benchmark for e8e38f0Click to view benchmark
|
Benchmark for e8e38f0Click to view benchmark
|
Benchmark for 004d4a6Click to view benchmark
|
Benchmark for 004d4a6Click to view benchmark
|
sydhds
approved these changes
Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes: