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

refactor: get rid of default implementation of Hashed trait for writable things #2573

Merged
merged 1 commit into from
Feb 15, 2019

Conversation

JeremyRubin
Copy link
Contributor

This supersedes #2549 as an approach to refactor the Hashed trait. See #2549 for discussion on why this should be done.

For any struct which implements DefaultHashable, it automatically derives the Hashed trait.

We no longer have hash_with in the Hashed trait as it was only used by Hash, and there was no sane implementation for some implementations.

The only downside is we can no longer put things which are just Hashed into a PMMR, as we don't have tuple derivations for Hashed. If this becomes a need in the future, something can be figured out.

In future work, I would still like to rename the trait and it's functions, but it is not done here for minimal diff.

@hashmap
Copy link
Contributor

hashmap commented Feb 14, 2019

Could you elaborate on why we need DefaultHashable? Can we just have a custom derive for Hashed (see https://doc.rust-lang.org/book/ch19-06-macros.html)

@JeremyRubin
Copy link
Contributor Author

Proc macros are kind of annoying and not the easiest to debug/implement.

They also encumber another compiler run to compile the proc macros and then the main codebase.

Probably easier to solve things at the trait level.

The main motivation for DefaultHashable is that structs which don't impl it have something weird going on internally, which should preclude them from being used places where we want to hash (e.g. pmmr).

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

I actually like this a lot.
It's a bit more verbose but I think its a lot clearer and more explicit in terms of what we are doing.

Copy link
Contributor

@ignopeverell ignopeverell left a comment

Choose a reason for hiding this comment

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

Liking it as well 👍

@yeastplume
Copy link
Member

Like this as well, think this has confused each and every person who's started looking into the codebase seriously at some point or another.

@antiochp
Copy link
Member

antiochp commented Feb 15, 2019

I'm testing this locally by the way - we should have a few pairs of eyes on this actually running in various networks before we merge as the hashing impacts everything.

Edit: Looks good, both on mainnet and mining with local usernet.

@antiochp antiochp added this to the 1.0.2 milestone Feb 15, 2019
@antiochp antiochp merged commit 99494c6 into mimblewimble:master Feb 15, 2019
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.

5 participants