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

Add tree hash implementation for fixed bytes #27

Conversation

Nomad-Free-Talent
Copy link
Contributor

Relevant to #22, I'm adding the new impl_for_fixed_bytes macro which can be used to implement the TreeHash trait to FixedBytes<N> struct.

It's not available for us to implement it like impl<const N: usize> for FixedBytes<N> because there's already a TreeHash implementation for B256 which is represented as FixedBytes<32>.

impl TreeHash for B256 {
fn tree_hash_type() -> TreeHashType {
TreeHashType::Vector
}
fn tree_hash_packed_encoding(&self) -> PackedEncoding {
PackedEncoding::from_slice(self.as_slice())
}
fn tree_hash_packing_factor() -> usize {
1
}
fn tree_hash_root(&self) -> Hash256 {
*self
}
}

@michaelsproul
Copy link
Member

I think we can probably make a generic impl that covers B256 as well.

@michaelsproul
Copy link
Member

It's weird that we have Vector type and still specify a packed encoding. I think that code might be unreachable, need to check.

@Nomad-Free-Talent
Copy link
Contributor Author

Nomad-Free-Talent commented Feb 2, 2025

I think we can probably make a generic impl that covers B256 as well.

The implementation logics between FixedBytes<48> and B256, which can be assumed as FixedBytes<32>, are different each other.

It's weird that we have Vector type and still specify a packed encoding. I think that code might be unreachable, need to check.

The FixedBytes type is defined as a wrapper of [u8; N] type. I think, in this case, we should define an additional implementation logic.

@michaelsproul
Copy link
Member

The implementation logics between FixedBytes<48> and B256, which can be assumed as FixedBytes<32>, are different each other.

They are different, yes. But I'm not convinced that they should be. I think that might be an oversight

@Nomad-Free-Talent
Copy link
Contributor Author

I'm not convinced that they should be.

Do you think we can just implement the tree_hash implementation logic for FixedBytes<const N: usize>?
In that case, the original B256 related logic will be ignored.

@michaelsproul
Copy link
Member

Yes, see:

I need one of my colleagues to review, but I think we should be Ok with a generic impl.

@michaelsproul
Copy link
Member

I ended up adding the generic implementations in that PR, so I could see all the changes in one place. I can add you as a co-author if you would like?

@Nomad-Free-Talent
Copy link
Contributor Author

I ended up adding the generic implementations in that PR, so I could see all the changes in one place. I can add you as a co-author if you would like?

Thank you for your help. Can we release the v0.9.1 with this change? I need the ssz_types crate with new tree_hash crate imported in it.
https://github.com/sigp/ssz_types/blob/b7f47d95d5cd926a2c299260838f2652a1f962c4/Cargo.toml#L14

And I'd be happy to be a co-author with you.

@michaelsproul
Copy link
Member

Yes I think we can do a backwards-compatible v0.9.1 release, as soon as my PR is reviewed.

I've added a commit with you as co-author so you'll get some credit. I'll close this PR now in favour of:

@Nomad-Free-Talent
Copy link
Contributor Author

Thank you for your help with this task.

@michaelsproul
Copy link
Member

v0.9.1 is out now 🎉

@Nomad-Free-Talent
Copy link
Contributor Author

v0.9.1 is out now 🎉

This is great. I'll create another PR on ssz_types repo with new tree_hash crate version.

@Nomad-Free-Talent
Copy link
Contributor Author

I created a PR towards the ssz_types crate to use the [email protected].
sigp/ssz_types#42

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