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

BlockHash Refactor #19

Merged
merged 2 commits into from
Feb 14, 2022
Merged

BlockHash Refactor #19

merged 2 commits into from
Feb 14, 2022

Conversation

DieracDelta
Copy link
Contributor

@DieracDelta DieracDelta commented Jan 7, 2022

Closes #10 . This PR adds the following functionality:

  • Renames what was originally BlockHash to InternalHash and makes InternalHash private.
  • Adds five public wrapper types around InternalHash: BlockHash (hash of implementors of BlockContents), LeafHash (hash of Leaf), TransactionHash (hash of BlockContents::Transaction), VerifyHash (special case used to verify QuorumCertificates), StateHash (hash of implementors of State). This is done via the macro gen_hash_wrapper_type since I couldn't seem to think of an easier way to sidestep boilerplate code without exposing the inner InternalHash type.
  • Uses serde_bytes to hash the underlying array during serialization. Note that serde_bytes does not seem to support const generics, so I directly copied in the wrapper given in that issue.
  • Adds a Debug implementation to InternalHash and its wrapper types.

@nmccarty if this is too large, I'm happy to split this in several smaller PRs.

I'm also uncertain if the types I've added make sense. This spot in particular was causing me a bit of confusion. It seems like in extends_from, we track the leaf hashes up the tree. So parent is a LeafHash. And we compare parent to node, which makes node also a LeafHash. I changed the extends_from invocation to reflect that by passing in locked_qc.leaf_hash instead of locked_qc.block_hash. The tests still pass after this change.

@DieracDelta DieracDelta requested a review from nmccarty January 7, 2022 20:34
@DieracDelta DieracDelta marked this pull request as draft January 7, 2022 20:34
@DieracDelta DieracDelta self-assigned this Jan 10, 2022
@DieracDelta DieracDelta marked this pull request as ready for review January 11, 2022 22:16
nmccarty
nmccarty previously approved these changes Feb 14, 2022
@nmccarty nmccarty merged commit cf76388 into main Feb 14, 2022
nmccarty pushed a commit that referenced this pull request Mar 4, 2022
@DieracDelta DieracDelta deleted the justin-hash-types branch March 31, 2022 16: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.

BlockHash overhaul
2 participants