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] Tree hash #88

Closed
wants to merge 21 commits into from
Closed

[WIP] Tree hash #88

wants to merge 21 commits into from

Conversation

mjkeating
Copy link
Contributor

Issue Addressed

Which issue # does this PR address?
#70

Proposed Changes

Added a Tree_Hash trait to relevant types with supporting functions

Additional Info

Still need to implement lists (Vec) and Container (HashMap - If I understand correctly) types. This a WIP/ sanity review request.

Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Looks like you've been busy and are making good progress!

I've focused on picking up project-consistency type issues here, as I figure this is the type of feedback you're after.

I can see there's going to be a lot of work around implementing tree hashing in it's entirety for all required types. For this reason, I think it might be sensible to re-scope this PR to be around implementing the core recursive hashing logic and only implementing it on a single (recursive) type. CrystallizedState seems to be the best candidate for this.

It appears to me that it would be very valuable to have some test vectors at this point. I will go prod the EF research team and let you know how it goes.

In terms of moving forward, I'll let you process my comments and I'll chase up test vectors. Once we have completed that, we can regroup and move towards a finalized implementation that is provably correct in some aspects.

Of course, feel free to ping me on Gitter with questions :)

beacon_chain/utils/ssz/src/tree_hash.rs Outdated Show resolved Hide resolved
beacon_chain/utils/ssz/src/tree_hash.rs Outdated Show resolved Hide resolved
beacon_chain/utils/ssz/src/tree_hash.rs Outdated Show resolved Hide resolved
beacon_chain/types/src/validator_record.rs Outdated Show resolved Hide resolved
@mjkeating
Copy link
Contributor Author

Updated to the latest spec changes. Still needs tie-out testing when available.

@paulhauner
Copy link
Member

paulhauner commented Dec 18, 2018

FYI, I just followed up on finding a blessed impl from the EF that we can test with.

EDIT: If they don't get something in a timely manner I think we just merge this in and then update later if required.

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Dec 18, 2018
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Changes are great!

@paulhauner
Copy link
Member

paulhauner commented Jan 24, 2019

Would you believe that this PR is finally getting merged??

Oh no, I missed the conflicts! If you can resolve I'll merge :)

@mjkeating
Copy link
Contributor Author

Yeah, I just saw this - sorry, it's pretty out of date. I'll create a new branch, if that's cool. I think it'll be cleaner.

@kirk-baird
Copy link
Member

Moved to #169

@kirk-baird kirk-baird closed this Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress PR is a work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants