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

SSZ merkle partial #424

Closed
wants to merge 36 commits into from

Conversation

@lightclient
Copy link
Contributor

commented Jul 3, 2019

Still a work in progress for #417, but it would be great to get some feedback on the work so far. To do:

  • Support for structs with only basic members
  • Support for structs with container members, recursively
  • Complete error handling
  • Full test coverage
  • Derive macro

For now, I've manually implemented what I believe the derive macro should produce in the tests module for A. I believe it is necessary to generate the "field overlay" using the NodeIndex to Node mapping. This should be determinable at compile time and will allow the program to traverse the field structure at runtime for loading and extracting partials.

@lightclient

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@paulhauner @michaelsproul please let me know if you see me trying to implement something that you've already implemented elsewhere! I think I'm on the path to reimplementing your BTreeOverlay, so I'm in the process right now of figuring out how to integrate that. I think the caches are different enough to warrant separate implementation -- the TreeHashCache stores the full tree in a Vec<u8> where I'm storing portions of the tree in a HashMap<NodeIndex, Vec<u8>>. Not sure if y'all would prefer to generalize these and combine them in some way.

@paulhauner paulhauner added the WIP label Jul 4, 2019

@paulhauner

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

I'm not sure about generalizing this with the TreeHashCache, yet. I need to do a big refactor on it due to the spec v0.8.0 updates -- that's happening this week/next, so I'll keep partials in mind.

I've found that building complex traits that can be implemented with a proc macro really influences the design of the trait. Having a look through this, I'm thinking there's going to be a lot of work to just make this derive-able.

I'm definitely not saying this design is necessarily bad for proc macros, it's just they're generally a lot of work. If I may, I'd suggest to start applying the proc-macro constraints to this trait as soon as possible. For cached_tree_hash, I followed this approach:

  1. Build a few other proc macros to understand their constraints. (I'd just happened to have already done this; it was very helpful).
  2. Like you've done with struct A, define an example struct and do your best to implement your trait as if it were implemented via derive macro.
  3. Build out the trait, maintaining struct A so you notice when you've broken the derive-ability.
  4. Once you're confident you don't need to modify the trait much more, implement an actual derive macro.

The reason I do (3) instead of just immediately doing (4) is because the compile errors generated in a derive macro are not as rich as others. (Viz., troubleshooting derive macros is painful).

@lightclient lightclient force-pushed the lightclient:ssz-merkle-partial branch from be0f898 to f05dc45 Jul 11, 2019

lightclient added some commits Jul 11, 2019

@lightclient lightclient force-pushed the lightclient:ssz-merkle-partial branch from 1904b77 to 6755bee Jul 16, 2019

lightclient added some commits Jul 16, 2019

@lightclient

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Okay I'm finally ready to have this reviewed! I have written an initial implementation that supports an interface similar to what we discussed in #417.

Here are some current drawbacks of the implementation:

  • There is little link between the partial and its type, e.g. a partial for a A could be loaded into a B partial without causing an error. Although the developer will not be able to do anything useful with this, I believe it can be avoided entirely.
  • get_bytes and set_bytes only work on primitive types.
  • Only general indices less than 2**64 are valid, as any greater and they will overflow the u64 type.
  • Structs can't be utilized via their native API (e.g. a.some_element = "go fish".to_owned();)

The future work on this will include trying to remedy the current drawbacks as well as:

  • Staying up-to-date with the latest merkle partial specifications
  • Performance benchmarking
  • Fuzzing
  • Improving documentation
  • Improve ergonomics
  • Adding support for Bitlists and Bitvectors
  • Replacing the current Cache with a Cached_tree_hash

Please let know what y'all think, I'll make any changes you request ASAP. Thanks 😄

@lightclient lightclient changed the title [WIP] SSZ merkle partial SSZ merkle partial Jul 24, 2019

@lightclient

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Closing for now as I'm going to investigate using bm instead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.