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

Several improvements #177

Merged
merged 19 commits into from
Jan 27, 2022
Merged

Several improvements #177

merged 19 commits into from
Jan 27, 2022

Conversation

vmx
Copy link
Member

@vmx vmx commented Jan 25, 2022

I'm usually a fan of having a clean Git history. Though in this case I think we want all the changes from the next branch to land on master. Also all changes were reviewed before they were merged into the next branch. So I'm in favour of merging the next branch into master now.

There are still two PRs open that I think make sense to land before we cut a release (they probably need to be polished a bit more). But this can be done after this one is merged.

It would be great if everyone involved (@dignifiedquire, @mriise, @samuelburnham, @Stebalien) in the changes could give a thumbs up for this PR to land on master. (as it really is a lot of (breaking) changes, most notably @mriise excellent long running effort on making const generics work).

mriise and others added 19 commits October 29, 2021 08:14
Rebased and squashed from #116. Closes #114.

BREAKING CHANGE: This change replaces uses of typenum with const
generics.

Importantly, the MultihashDigest, Hasher, and StatefulHasher traits are
now generic over the digest size due to compiler limitations. However,
`Hasher` exposes this size as an associated const, so
`MyHasherImpl::SIZE` can often be used to fill in the size parameter
with some help from type inference.

Additionally:

- All usages of typenum must be replaced with usize constants.
- The new MSRV is 1.51.0 due to const generics.
- IdentityDigest is now a tuple struct of usize and the digest instead
  of u8 and a generic array.
Sometimes the compiler can optimize this, but there's this code isn't
any more complicated.
fix: avoid copying digests where possible
Allow comparing across multihash sizes
1. Add a truncate method to "trim" multihashes. E.g., to generate 20
byte SHA256 hashes.
2. Add a function to "resize" multihashes into different backing buffer.
Enable cargo test without command line arguments
@mriise
Copy link
Contributor

mriise commented Jan 25, 2022

Thanks for the ping @vmx! I would like to hold of just a bit longer. Rust version 1.59 will be introducing const generic defaults which will allow for better ergonomics when dealing with different sized arrays by expanding (or potentially shrinking) the backing array. Users would still need to ensure the Digest type is sized properly but we can add tools to make it easy.

here is a playground link:
https://play.rust-lang.org/?version=beta&mode=debug&edition=2021&gist=138ba86ee23c866228af859d86b7d4ed

There is a downside for this approach however, the const assert hack is very much a hack and spits out potentially confusing error information if types are not what it expected. However I feel that with good documentation, it would be worth for the less confusing interface of types in general.

@mriise
Copy link
Contributor

mriise commented Jan 25, 2022

As a side note, I did some playing around the other day with using const expressions to add the From trait for smaller into larger size and with the plan to add TryFrom for shrinking but it gets a bit too hacky pretty quick. Resizing to a default sized backing array I think will be the most ergonomic for a long time till const generics become more fleshed out.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=4bd521cc2b6d48534e1b0a58aafebb61

the idea of the above being

struct Container<const S: usize> ([u8; S]);

impl<const SMALL: usize, const LARGE: usize> From<Container<SMALL>> for Container<LARGE> 
    where Assert<{SMALL <= LARGE}>: True 
{}

and consuming functions being

fn generic_fn<T, const S: usize>(src: T) where T: Into<Container<S>> {
    let container: Container<S> = src.into();
}

or something similar

But again, const generics is still decently far away from making that happen making the first option a good option in the foreseeable future as well as a good option in the short term future.

@vmx
Copy link
Member Author

vmx commented Jan 26, 2022

@mriise Do I understand correctly, that your proposed changes would change the current API around resizing Multihashes. As this isn't a feature in any released version yet, I don't expect many people using it already. As @Stebalien introduced that change, I guess that he needs that change. Though I don't expect, that it would be a problem to have a breaking change once Rust 1.59 is out.

@mriise would it work for you if we merge the current version (and have a release), but you're free to break the resizing API as soon as Rust 1. (@Stebalien would need to agree on this too).

To explain my rush: rust-multihash is a dependency deep down, and I'd like to update other libraries like rust-cid and libipld to also use this const generics version.

@mriise
Copy link
Contributor

mriise commented Jan 26, 2022

@vmx sorry for the confusion my suggestion had 2 parts, the primary being to change the digest so that it uses a default size and the other to help with resizing the backing array without pulling the digest out of our implementation and putting it back in to fit the generic size. adding an into_generic() along with exposing the methods that it uses to resize.

using a default size for the backing array would be doing exactly what we are doing currently to avoid specifying what size each Digest's backing uses (addressing the whole concern about loss in usability). I will make a PR to next to show what I mean.

@vmx
Copy link
Member Author

vmx commented Jan 26, 2022

using a default size for the backing array would be doing exactly what we are doing currently to avoid specifying what size each Digest's backing uses (addressing the whole concern about loss in usability).

I think that usability concern would also be solved with #156 wouldn't it? I'm in favor of that change, as I also think that people really shouldn't use the hashers directly.

@mriise
Copy link
Contributor

mriise commented Jan 27, 2022

yep, that PR would do much of what I was thinking of doing. After playing around with const generic defaults a bit more I realized its not quite what I expected.

what i wanted to do was

struct Digest<const S: usize = DEFAULT_ALLOC_SIZE> ([u8; S]);

trait Hasher {
    type Digest: Digest;
}

impl<const S: usize> Hasher for Blake2<S> {
    type Digest = Blake2Digest<S>; // override Digest<DEFAULT_ALLOC> with Digest<S>
}

but instead type Digest: Digest seems to just turn into sugar for type Digest: Digest<DEFAULT_ALLOC_SIZE> meaning any impl generic over S will have conflicts, which makes the change essentially pointless.

As for resizing it can be done now with stable rust, but that is a minor change and isn't blocking this.

I would like to see #156 merged so we can reduce the amount of major releases :D

the major drawback is loosing out on newtypes for digests, but that is OK by me to ease const generics

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I have no problems merging this to master now, although I'd consider getting #156 and maybe #159 in first to batch up the rest of the breaking changes.

@vmx
Copy link
Member Author

vmx commented Jan 27, 2022

@mriise @Stebalien Yes, the plan is to just merge this into master. I also want all other breaking changes that make sense in before doing a release.

@mriise #156 depends on this PR. So do I understand correctly that you're also OK merging this one?

@vmx vmx requested a review from mriise January 27, 2022 09:54
@vmx vmx merged commit 22e44c8 into master Jan 27, 2022
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.

5 participants