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

Implements hashing as a blanket trait for instances of CanonicalSerialize #265

Merged
merged 5 commits into from
Apr 23, 2021

Conversation

huitseeker
Copy link
Contributor

  • this saves an alllocation w.r.t the issue-suggested approach by implementing io::Write on the input instance of digest::Digest,
  • note that most instances of digest::Digest already have an io::Write instance, but CanonicalSerialize consuming its io::Write argument prevents its usage,
  • this hence implements io::Write on a cheap newtype wrapper

closes: #86


  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@huitseeker huitseeker force-pushed the hashes branch 2 times, most recently from e15b4c5 to 48847ee Compare April 21, 2021 22:50
…alize`

- this saves an alllocation w.r.t the suggested approach by implementing `io::Write` on the input instance of `digest::Digest`,
- note that most instances of `digest::Digest` [already](https://gist.github.com/huitseeker/e827161413063e347ce5a496b66ff287) have an [`io::Write` instance](https://github.com/rustcrypto/hashes#hashing-readable-objects), but `CanonicalSerialize` consuming its `io::Write` argument prevents its usage,
- this hence implements `io::Write` on a [cheap newtype wrapper](https://rust-unofficial.github.io/patterns/patterns/behavioural/newtype.html)

Fixes arkworks-rs#86
serialize/src/lib.rs Outdated Show resolved Hide resolved
serialize/src/lib.rs Outdated Show resolved Hide resolved
@Pratyush
Copy link
Member

but CanonicalSerialize consuming its io::Write argument prevents its usage,

Is it not possible to work around this by doing &mut writer (&mut W is Write whenever W is)

@huitseeker
Copy link
Contributor Author

huitseeker commented Apr 22, 2021

@Pratyush Sure, though this signature change would change the API of CanonicalSerialize in a backwards-incompatible way.

Moreover, I'm not sure we want to depend on the std::io::Write implementations in RustCrypto/hashes:

  • the Digest trait does not guarantee its existence on its own,
  • it's implemented from unrelated (MAC) trait impls through a macro which future in the code base isn't assured.
  • the no-std context adds an additional wrinkle.

They're here .. just not very reliably so.

@Pratyush
Copy link
Member

Pratyush commented Apr 22, 2021

Ok that makes sense, let's proceed as is. This LGTM modulo the change to CanonicalSerializeHashExt

- rename Hash -> CanonicalSerialize Ext according to [extension trait best practices](https://rust-lang.github.io/rfcs/0445-extension-trait-conventions.html#the-convention)
- add the same structure for hashing uncompressed bytes
@Pratyush
Copy link
Member

Oh @huitseeker if you could add a CHANGELOG entry that would be great (also for #263)

@weikengchen weikengchen merged commit 2b35d51 into arkworks-rs:master Apr 23, 2021
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

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.

Hashing Structures helpers
4 participants