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

[wip] next version #30

Closed
wants to merge 13 commits into from
Closed

[wip] next version #30

wants to merge 13 commits into from

Conversation

dignifiedquire
Copy link
Member

@dignifiedquire dignifiedquire commented Nov 6, 2018

  • add support for blake2b and blake2s
  • add support for codes larger than 255
  • add support for streaming based hashing
  • add std feature, and test for no_std compat (needs verification)
  • add asm and simd features to allow enabling those on the underlying hashers
  • minimize overhead of Multihash wrapper type around the actual hash
  • update travis script and ensure features are being tested
  • use proc_macro to do the actual implementation under the hood

@ghost ghost assigned dignifiedquire Nov 6, 2018
@ghost ghost added the in progress label Nov 6, 2018
src/digests.rs Outdated

#[Size = "32"]
#[Digest = "sha2::Sha256"]
Sha2256 = 0x12,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really have to be like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

which part?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the Sha2256. I understand need for no _ but something like Sha256V2, Sha256V3, Sha512V2. Anything to make it valid syntactically.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dignifiedquire however you feel, anyway, just an observation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get it to be SHA2_256 etc.?

Copy link

@laser laser left a comment

Choose a reason for hiding this comment

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

multihash_derive/src/lib.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,229 @@
#![recursion_limit = "1024"]
Copy link

Choose a reason for hiding this comment

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

honest question: what's this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

tldr: it doesn't compile when I remove it

longer form:

as far as I understand the reason is that we are using macros, e.g. quote! inside the definition of another macro, and there is a recursion thing that breaks from the parsing level, but I am not entirely sure and haven't found any good docs on this yet

Copy link
Member Author

Choose a reason for hiding this comment

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

this the best thing I found so far:

Recursion limit

The quote! macro relies on deep recursion so some large invocations may fail with "recursion limit reached" when you compile. If it fails, bump up the recursion limit by adding #![recursion_limit = "128"] to your crate. An even higher limit may be necessary for especially large invocations. You don't need this unless the compiler tells you that you need it.

multihash_derive/src/lib.rs Outdated Show resolved Hide resolved
src/digests.rs Outdated Show resolved Hide resolved
src/digests.rs Outdated Show resolved Hide resolved
}

let code =
fetch_discriminant(&variant.discriminant).expect("Please provide the code as discriminant");
Copy link

Choose a reason for hiding this comment

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

so very polite!

multihash_derive/src/lib.rs Outdated Show resolved Hide resolved
multihash_derive/src/lib.rs Outdated Show resolved Hide resolved
@tomaka
Copy link
Member

tomaka commented Nov 21, 2018

For what it's worth, we have a fork in the rust-libp2p repo that also contains a lot of changes: https://github.com/libp2p/rust-libp2p/tree/master/misc/multihash

(We did that in order to not have to wait for a review every time we needed something)

@dignifiedquire
Copy link
Member Author

@tomaka thanks for reminding me, I would like to make sure to get all things that are important for you back into here. I‘ll review the commits there, but is there anything specific you have as a requirement you want to point out?

@dignifiedquire
Copy link
Member Author

it would also be great to get feedback if this branch is in a state that you could base your work on in rust-libp2p or if not what is missing.

@dvc94ch
Copy link
Collaborator

dvc94ch commented Aug 23, 2019

interesting would be to add a cli app to compute hashes from stdin. would be useful to be able to do something like this: cat file.txt | multihash --hash sha2-256 | multibase --base base32

@dvc94ch
Copy link
Collaborator

dvc94ch commented Aug 23, 2019

So is this getting merged?

@mark-stopka
Copy link

Is this library still being maintained? SHA2 is already on a version 0.8.1; this PR also removes dependency on 0.7.x version of SHA2...

@dvc94ch
Copy link
Collaborator

dvc94ch commented Oct 1, 2019

Yeah it's still being maintained, just currently busy with other things.

@vmx vmx mentioned this pull request Feb 25, 2020
@vmx
Copy link
Member

vmx commented Feb 26, 2020

I'm closing this PR as most of the features got implemented one way or another with the release of 0.10.0.

Missing is the no_std part, but I think that's something that can be tackled later in a separate PR.

@vmx vmx closed this Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants