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

implement SOS serialization in minimal_ssz.py #1024

Merged
merged 2 commits into from
May 3, 2019
Merged

implement SOS serialization in minimal_ssz.py #1024

merged 2 commits into from
May 3, 2019

Conversation

protolambda
Copy link
Collaborator

@protolambda protolambda commented May 1, 2019

Note: PR into master, to get it into v0.6.1. Will make PRs into special dev branch for future v0.6.x updates after v0.6.1

Implemented SOS serialization for minimal_ssz.py (util of the executable spec).

Some notes:

  • Container encoding differs slightly from previous implementation semantics: I call the helper function encode_series, since it does lists, vectors and containers. Not just containers.
  • Removed unnecessary summing & list concatenation. Worth a few lines to get fast CI + tests.
  • The encoding seems funny, but is correct from what I understand. Example outlined below. Edit: funny encoding was just incorrect.
  • The spec can be improved somewhat:
    • serialize(sum(fixed_lengths + variable_lengths[:i])) is not just inefficient, it is also missing explicit expression of the expected length of the serialization output. Should be BYTES_PER_LENGTH_OFFSET, which is 4, i.e. uint32, something different from most of the other parts of the spec.
    • typo: Reccursively

@protolambda protolambda added the scope:SSZ Simple Serialize label May 1, 2019
@protolambda
Copy link
Collaborator Author

protolambda commented May 1, 2019

@JustinDrake You worked on the SOS wording in the SSZ spec, and are working with bitfields in PR #1019, what do you think about the encoding example above? Fixed related bug, not necessary anymore, see below.

@protolambda
Copy link
Collaborator Author

protolambda commented May 1, 2019

Now that I think about it, the 0x0000... actually has to be 0x04000... To account for the offset itself. Can fix that later. Base point still stands: do we actually want it to be there?
Edit: what seemed funny, was really just incorrect. Blame jetlag. At least I catched it, and it's fixed now, so good to merge after review :)

@hwwhww hwwhww requested a review from pipermerriam May 2, 2019 03:07
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.

Nice!

serialize(sum(fixed_lengths + variable_lengths[:i])) is not just inefficient, it is also missing explicit expression of the expected length of the serialization output. Should be BYTES_PER_LENGTH_OFFSET, which is 4, i.e. uint32, something different from most of the other parts of the spec.

And it appears that the definition of serialize in SSZ takes no explicit type... How do we want to proceed here? Probably just mirror our minimal_ssz with an optional type param.

@djrtwo djrtwo merged commit c011feb into master May 3, 2019
@djrtwo djrtwo deleted the sos_ssz_py branch May 3, 2019 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:SSZ Simple Serialize
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants