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

Rewrite SSZ spec #696

Merged
merged 22 commits into from
Mar 5, 2019
Merged

Rewrite SSZ spec #696

merged 22 commits into from
Mar 5, 2019

Conversation

JustinDrake
Copy link
Collaborator

@JustinDrake JustinDrake commented Feb 27, 2019

  • Simplify presentation where appropriate. For example, deserialisation is implicit from serialisation (similar to bls_sign being implicit from bls_verify) and is left as an implementation exercise.
  • Dramatically reduce spec size and hopefully improve readability.

Spec changes:

  • Support for tuples (see Full support for tuples #665)
  • Reduce chunk-size reduction 32 bytes (see SSZ TLDR #679)
  • Simplify logic of length prefix serialisation (the same logic is used for lists and containers)
  • Restrict the uintN type (N must be in [8, 16, 32, 64, 128, 256]).
  • Modify signed_root to take single ssz object, returning the hash_tree_root of the object less the last field
  • Do not length pre-fix fixed sized containers (i.e. does not embed a list)

* Implement tuples and a chunk-size reduction to 32 bytes (see #665 and #679)
* Simplify presentation where appropriate. For example, deserialisation is implicit from serialisation (similar to `bls_sign` being implicit from `bls_verify`) and is left as an implementation exercise.
* Dramatically reduce spec size and hopefully improve readability.
Copy link
Contributor

@jannikluhn jannikluhn left a comment

Choose a reason for hiding this comment

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

👍 it's much easier to read now.

I wasn't sure if Python conventions are relevant here, but if so we might want to

  • replace reduce(lambda x, y: x + y, serialized_elements) etc. with ''.join(serialized_elements).
  • renamce object to something else like value as object is a built-in.

specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

Awesome! I like the brevity but might argue for a little more verbosity in a couple of places.

specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
@djrtwo
Copy link
Contributor

djrtwo commented Feb 28, 2019

"Implement tuples and a chunk-size reduction to 32 bytes (see #665 and #679)"
^ Are these the only actual changes that an implementer will need to make?

@JustinDrake

@JustinDrake
Copy link
Collaborator Author

JustinDrake commented Feb 28, 2019

I've updated the description with the changes:

Spec changes:

  • Support for tuples (see Full support for tuples #665)
  • Reduce chunk-size reduction 32 bytes (see SSZ TLDR #679)
  • Simplify logic of length prefix serialisation (the same logic is used for lists and containers)
  • Restrict the uintN type (N must be in [8, 16, 32, 64, 128, 256]).

@zilm13
Copy link
Contributor

zilm13 commented Feb 28, 2019

I'm trying to implement hash_tree_root according to this spec and have few questions/thoughts:

  1. Given ordered objects of the same basic type
    Could we clarify here that type and size, just to make it clear

  2. Merkleize the chunks, and return the root
    It's not clear from spec, what is merkleization

  3. Given the very simple container {i=1}, it's hash_tree_root is 0100000000.. is it ok? So, hash function is never applied. Or did I miss something?

| `uintN` | Type of `N` bits unsigned integer, where ``N % 8 == 0``. |
* **container**: ordered heterogenous collection of values
* key-pair curly bracket notation `{}`, e.g. `{'foo': "uint64", 'bar': "bool"}`
* **tuple**: ordered fixed-length homogeneous collection of values
Copy link
Contributor

Choose a reason for hiding this comment

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

aren't these better called array:s? tuples tend to allow heterogeneous types

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't care either way. Implementations can use whatever terminology works best. For example, the natural terminology for Go is "array" (instead of tuple) and "slice" (instead of list). Notice that in Python lists also allow heterogeneous types 🤷‍♂️

wemeetagain added a commit to ChainSafe/ssz-js that referenced this pull request Mar 1, 2019
This originated as a new codebase, following the currently unreleased
ssz rewrite ethereum/consensus-specs#696
@wemeetagain wemeetagain mentioned this pull request Mar 1, 2019
5 tasks
wemeetagain added a commit to ChainSafe/ssz-js that referenced this pull request Mar 1, 2019
This originated as a new codebase, following the currently unreleased
ssz rewrite ethereum/consensus-specs#696
@mjkeating
Copy link

hash_tree_root(value) is referenced several times in this new spec, but it appears to no longer be defined in this new spec. Maybe I'm missing something.

@JustinDrake
Copy link
Collaborator Author

@mjkeating "We now define Merkleization hash_tree_root(value)"

@djrtwo djrtwo merged commit f93e6fe into dev Mar 5, 2019
@djrtwo djrtwo deleted the JustinDrake-patch-4 branch March 5, 2019 16:22
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.

7 participants