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

Generic FixedBytes implementation and bug fixes #28

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Feb 2, 2025

This PR removes the implementations of tree_hash_packed_encoding and tree_hash_packing_factor for the vector types Hash256 and Address. It then generalises the TreeHash implementation to cover all lengths of FixedBytes.

Removing packing brings the implementations in line with the spec, and other implementations of TreeHash for types with tree_hash_type == Vector.

Downstream users of the TreeHash trait should take care not to call the *packed methods on types other than "basic" types, as can be seen in ssz_types:

https://github.com/sigp/ssz_types/blob/b7f47d95d5cd926a2c299260838f2652a1f962c4/src/tree_hash.rs#L11-L41

And in milhouse:

https://github.com/sigp/milhouse/blob/00220dab8b92ed6bbbac48e4543a04923ce1f3eb/src/utils.rs#L67-L72

Arguably the trait is ill-designed in that it has these methods that are guaranteed to panic. I think we should overhaul this in future for a type-safe design, if that can be achieved with minimal performance penalty.

Copy link

codecov bot commented Feb 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.85%. Comparing base (e9708cd) to head (bdbcfc1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
+ Coverage   79.35%   85.85%   +6.50%     
==========================================
  Files           6        6              
  Lines         310      297      -13     
==========================================
+ Hits          246      255       +9     
+ Misses         64       42      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michaelsproul
Copy link
Member Author

This PR was motivated by this other PR, which highlighted the difference between our Bytes<32> implementation, and the implementations for other lengths of byte vector:

If this change is correct, we can use const generics to implement TreeHash for all byte vectors.

@michaelsproul michaelsproul changed the title Remove packing implementations for Vector types Generic FixedBytes implementation and bug fixes Feb 2, 2025
@michaelsproul michaelsproul changed the title Generic FixedBytes implementation and bug fixes Generic FixedBytes implementation and bug fixes Feb 3, 2025
Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -100,7 +72,7 @@ impl TreeHash for [u8; 48] {

fn tree_hash_root(&self) -> Hash256 {
let values_per_chunk = BYTES_PER_CHUNK;
let minimum_chunk_count = (48 + values_per_chunk - 1) / values_per_chunk;
let minimum_chunk_count = (N + values_per_chunk - 1) / values_per_chunk;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick and not directly related to your changes, but we should probably replace this with N.div_ceil(BYTES_PER_CHUNK) for clarity. This pattern is also used in several other places, so that can probably be a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, div_ceil is a godsend that didn't exist when this was first written. Agree we should use it here (and turn on clippy on CI)

@michaelsproul michaelsproul merged commit 3c54a52 into main Feb 4, 2025
7 checks passed
@michaelsproul michaelsproul deleted the remove-vector-packing branch February 4, 2025 01:48
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