SOS style layout #47

pipermerriam · 2019-03-15T22:33:51Z

WIP with the SOS style layout as specified in ethereum/consensus-specs#787

Depends on #58

What was wrong?

The spec for SSZ has been updated to a new serialization layout.

How was it fixed?

Updated the serialization and deserialization code for all Composite types to comply with the spec.

Some API updates:

Container sedes no longer operate on dictionaries or have any concept of fields names
Converted to using io.ByteIO streams for some decoding rather than tracking the current content index and end index.

Cute Animal Picture

pipermerriam · 2019-04-05T19:10:55Z

pytest.ini

@@ -1,5 +1,5 @@
 [pytest]
-addopts= --showlocals --durations 10


Need to revert this.

pipermerriam · 2019-04-05T19:11:12Z

ssz/sedes/base.py

 )

 TSerializable = TypeVar("TSerializable")
-TDeserialized = TypeVar("TDeserialized")
+TCanonical = TypeVar("TCanonical")


Need to revert this

pipermerriam · 2019-04-05T19:22:31Z

@jannikluhn can you give this a preliminary review. Not really a merge candidate until ethereum/consensus-specs#787 has been merged.

jannikluhn

It seems like that Container, Vector, and List have separate implementations of deserialize, even though they all more or less do the same thing. Is there a reason why we can't just put it in CompositeSedes, maybe relying on some abstract methods implemented in the subclasses such as _iter_sedes?

ssz/sedes/base.py

ssz/sedes/boolean.py

ssz/sedes/base.py

ssz/sedes/container.py

jannikluhn · 2019-04-08T10:26:39Z

ssz/utils.py

+def read_exact(num_bytes: int, stream: IO[bytes]) -> bytes:
+    data = stream.read(num_bytes)
+    if len(data) != num_bytes:
+        raise DeserializationError(f"Tried to read {num_bytes}.  Only got {len(data)} bytes")


Suggested change

raise DeserializationError(f"Tried to read {num_bytes}. Only got {len(data)} bytes")

raise DeserializationError(f"Tried to read {num_bytes}. Only got {len(data)} bytes")

jannikluhn · 2019-04-08T10:27:02Z

ssz/utils.py

-            f"Content size is too large to be encoded in a {SIZE_PREFIX_SIZE} byte prefix",
-        )
+def encode_offset(offset: int) -> bytes:
+    return offset.to_bytes(4, 'little')


Suggested change

return offset.to_bytes(4, 'little')

return offset.to_bytes(OFFSET_SIZE, 'little')

ssz/utils.py

jannikluhn · 2019-04-08T10:31:51Z

ssz/sedes/list.py

+        if len(value):
+            raise ValueError("Cannot compute trie hash for non-empty value using `EmptyList` sedes")
+        # TODO: This can just be a constant value, no need to compute at runtime
+        return merkleize(pack([]))


Suggested change

return merkleize(pack([]))

return mix_in_length(merkleize(pack([])), 0)

Was this resolved on purpose?

jannikluhn · 2019-04-08T10:37:02Z

ssz/sedes/base.py

+    #
+    # Tree hashing
+    #
+    def hash_tree_root(self, value: Sequence[Any]) -> bytes:


It looks like this is only suitable for Vector, but not for List and it would make more sense to have this in Vector directly.

pipermerriam · 2019-04-09T18:40:11Z

@jannikluhn I addressed everything with the following exceptions.

You suggested trying to combine things into a single generic deserialize and have subclass hooks. I'm 👎 on that approach. Each of the different algorithms have sufficient differences that I don't see combining them as being an improvement because I feel it would decreased readability with very marginal reduction in code duplication.
You didn't like the mixing of values and offsets in Container deserialization. I segregated this into a small stand-alone function and have updated it so that the separation of values from offsets is no longer happening inline. I tinkered with an approach that didn't store them alongside each other at any point, but it added complexity at what I assessed to be no added safety since the segregation happens based on the sedes.is_fixed_size flag and any mechanism I thought up to keep them separate was still ultimately depending on that flag.

jannikluhn

👍 looks good

There seem to be a few comments from the previous review that have been marked as resolved, presumably by accident? I reopened them just for double checking, most of them are minor, but at least the mix_in_length one for empty_list seems to be worth fixing.

ssz/sedes/byte.py

ssz/sedes/container.py

+    def _validate_serializable(self, value: Sequence[Any]) -> bytes:
+        if len(value) != len(self.field_sedes):
+            raise SerializationError(
+                f"Not enough elements.  Expected {len(self.field_sedes)}, Got {len(value)}"


jannikluhn · 2019-04-15T00:27:22Z

ssz/sedes/list.py

+        if len(value):
+            raise ValueError("Cannot compute trie hash for non-empty value using `EmptyList` sedes")
+        # TODO: This can just be a constant value, no need to compute at runtime
+        return merkleize(pack([]))


Was this resolved on purpose?

ssz/utils.py

pipermerriam · 2019-04-15T14:36:40Z

I'll update those. I think I force pushed over top of the commits that were made by the github UI in accepting your recommended changes.

pipermerriam · 2019-04-18T21:57:43Z

@jannikluhn I've revived the fixes that were squashed in my force pushes.

I believe this is ready for final review and merge once any issues have been addressed.

hwwhww · 2019-04-19T09:48:32Z

@pipermerriam

For syncing the spec versioning, the procedure we need:

Merge Serializable metaclass cleanups #57, Root and SignedRoot #58
Release py-ssz (sync with spec v0.5.1)
Merge this PR (SOS style layout #47) (spec dev branch)

hwwhww · 2019-04-19T10:59:03Z

ssz/sedes/container.py

+        if not offset_pairs:
+            return fixed_size_values
+
+        dynamic_size_values = self.deserialize_section_1(offset_pairs, stream)


variable v.s. dynamic

I'm fine with both! But we can sync it by either changing the spec or changing py-ssz side.

hwwhww · 2019-04-19T10:59:30Z

ssz/sedes/container.py

-            if next_field_start_index <= field_start_index:
-                raise Exception("Invariant: must always make progress")
-            field_start_index = next_field_start_index
+        # Verify that bothe iterables have been fully consumed.


typo?

Suggested change

# Verify that bothe iterables have been fully consumed.

# Verify that both iterables have been fully consumed.

hwwhww · 2019-04-19T11:05:07Z

ssz/sedes/base.py

+    pass
+
+
+def _compute_section_0_size(element_sedes: Iterable[TSedes]) -> int:


IMO section_0 and section_1 are confusing names. fixed_parts and variable_parts from the current ethereum/consensus-specs#787 PR are better.

hwwhww · 2019-04-19T11:16:56Z

ssz/sedes/list.py

-                raise Exception("Invariant: must always make progress")
-            element_start_index = next_element_start_index
+            num_remaining_offset_bytes = first_offset - stream.tell()
+            if num_remaining_offset_bytes % 4 != 0:


As @jannikluhn mentioned: https://github.com/ethereum/py-ssz/pull/47/files#r272979440
Should we use OFFSET_SIZE instead of 4?

Suggested change

if num_remaining_offset_bytes % 4 != 0:

if num_remaining_offset_bytes % OFFSET_SIZE != 0:

hwwhww · 2019-04-19T11:17:27Z

ssz/sedes/list.py

+            num_remaining_offset_bytes = first_offset - stream.tell()
+            if num_remaining_offset_bytes % 4 != 0:
+                raise DeserializationError(
+                    f"Offset bytes was not a multiple of 4.  Got {num_remaining_offset_bytes}"


ditto

Suggested change

f"Offset bytes was not a multiple of 4. Got {num_remaining_offset_bytes}"

f"Offset bytes was not a multiple of {OFFSET_SIZE}. Got {num_remaining_offset_bytes}"

hwwhww · 2019-04-19T11:18:58Z

ssz/sedes/list.py


-            yield element
+            num_remaining_offsets = num_remaining_offset_bytes // 4


ditto

Suggested change

num_remaining_offsets = num_remaining_offset_bytes // 4

num_remaining_offsets = num_remaining_offset_bytes // OFFSET_SIZE

pipermerriam · 2019-04-19T16:18:51Z

THis has now been rebased on top of #58

pipermerriam · 2019-04-19T17:58:55Z

This PR should be ready to merge once the two dependent PR's are merged.

pipermerriam · 2019-04-23T19:47:44Z

@jannikluhn I've rebased. All tests are passing. This should get one more review pass to be sure everything is fine with you and/or @hwwhww as I had to resolve a number of non-trivial merge conflicts and then resulting test failures.

jannikluhn

Just some nitpicks and a potential bug in list deserialization, other than that LGTM.

jannikluhn · 2019-04-24T08:14:57Z

ssz/constants.py

@@ -9,3 +9,9 @@
 MAX_CONTENT_SIZE = 2 ** (SIZE_PREFIX_SIZE * 8) - 1


Looks like SIZE_PREFIX_SIZE and MAX_CONTENT_SIZE could be removed.

jannikluhn · 2019-04-24T08:28:11Z

ssz/sedes/base.py

+TDeserializedElement = TypeVar("TDeserializedElement")
+
+
+class HomogenousSequence(CompositeSedes[Sequence[Any], Tuple[Any]]):


Suggested change

class HomogenousSequence(CompositeSedes[Sequence[Any], Tuple[Any]]):

class HomogenousSequence(CompositeSedes[Sequence[Any], Tuple[Any, ...]]):

Also, this class doesn't seem to be used (Vector, List, and EmptyList are just CompositeSedes).

jannikluhn · 2019-04-24T08:44:34Z

ssz/sedes/container.py

-            raise ValidationError(
-                f"The following fields are duplicated {','.join(sorted(duplicate_field_names))}"
-            )
+        from ssz.sedes.base import BaseSedes


jannikluhn · 2019-04-24T08:47:36Z

ssz/sedes/list.py

 )

 TSerializable = TypeVar("TSerializable")
 TDeserialized = TypeVar("TDeserialized")

+EMPTY_LIST_HASH_TRIES_ROOT = mix_in_length(merkleize(pack([])), 0)


Suggested change

EMPTY_LIST_HASH_TRIES_ROOT = mix_in_length(merkleize(pack([])), 0)

EMPTY_LIST_HASH_TREE_ROOT = mix_in_length(merkleize(pack([])), 0)

As it's just a normal Merkle tree, not a trie.

Also, this could be moved to constants.py (but I guess here would be fine too as it's very specific).

Keeping it here because I don't want to import merkleize or pack from within constants.py

jannikluhn · 2019-04-24T08:48:37Z

ssz/sedes/list.py


+    def hash_tree_root(self, value: Sequence[TSerializable]) -> bytes:
+        if len(value):
+            raise ValueError("Cannot compute trie hash for non-empty value using `EmptyList` sedes")


Suggested change

raise ValueError("Cannot compute trie hash for non-empty value using `EmptyList` sedes")

raise ValueError("Cannot compute tree hash for non-empty value using `EmptyList` sedes")

jannikluhn · 2019-04-24T08:50:07Z

ssz/sedes/list.py

+            element_size = self.element_sedes.get_fixed_size()
+            data = stream.read()
+            if len(data) % element_size != 0:
+                raise DeserializationError("TODO: INVALID LENGTH")


leftover TODO

jannikluhn · 2019-04-24T08:55:08Z

ssz/sedes/list.py

+                yield self.element_sedes.deserialize(segment)
+        else:
+            try:
+                first_offset = s_decode_offset(stream)


This would also fail if there are more than zero but less than 4 bytes in which case we should not return the empty list but raise an exception.

Fixed and added a test for this case.

pipermerriam · 2019-04-24T13:36:44Z

@hwwhww I just realized I merged when this had "DO NOT MERGE" label on it... feel free to revert or force push master back to previous state.

hwwhww · 2019-04-26T04:40:49Z

Whoops sorry, I need to revert it and add #62 bugfix and #60 tool into the next py-ssz release (mapping to spec v0.5.1).

Better error if bump missing in make notes/release

pipermerriam mentioned this pull request Mar 18, 2019

[WIP] Spec update #46

Closed

jannikluhn mentioned this pull request Mar 21, 2019

Implement serialization and deserialization of basic types #49

Merged

pipermerriam force-pushed the piper/sos-style-layout branch from dc446a7 to 8072a1b Compare April 5, 2019 19:10

pipermerriam commented Apr 5, 2019

View reviewed changes

pytest.ini Outdated

@@ -1,5 +1,5 @@

[pytest]

addopts= --showlocals --durations 10

Copy link

Member Author

pipermerriam Apr 5, 2019

Choose a reason for hiding this comment

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

Need to revert this.

pipermerriam commented Apr 5, 2019

View reviewed changes

pipermerriam requested a review from jannikluhn April 5, 2019 19:22

pipermerriam mentioned this pull request Apr 7, 2019

Update SSZ spec to use SOS style offset based layout. ethereum/consensus-specs#787

Merged

jannikluhn reviewed Apr 8, 2019

View reviewed changes

pipermerriam force-pushed the piper/sos-style-layout branch 2 times, most recently from 22bc497 to dd709d6 Compare April 9, 2019 18:08

jannikluhn approved these changes Apr 15, 2019

View reviewed changes

pipermerriam force-pushed the piper/sos-style-layout branch from 5293899 to 34a330c Compare April 18, 2019 21:51

pipermerriam requested review from jannikluhn and hwwhww April 18, 2019 21:56

hwwhww approved these changes Apr 19, 2019

View reviewed changes

pipermerriam force-pushed the piper/sos-style-layout branch from 34a330c to 4ee2470 Compare April 19, 2019 16:18

pipermerriam changed the title ~~[WIP] SOS style layout~~ SOS style layout Apr 19, 2019

pipermerriam force-pushed the piper/sos-style-layout branch 2 times, most recently from ea4e18c to 69f09ea Compare April 19, 2019 16:27

hwwhww added the DO NOT MERGE label Apr 23, 2019

pipermerriam force-pushed the piper/sos-style-layout branch from 69f09ea to 8574b7b Compare April 23, 2019 16:44

jannikluhn approved these changes Apr 23, 2019

View reviewed changes

SOS style layout for serialization

3a871a4

pipermerriam force-pushed the piper/sos-style-layout branch from 8574b7b to 3a871a4 Compare April 23, 2019 19:39

jannikluhn approved these changes Apr 24, 2019

View reviewed changes

pipermerriam force-pushed the piper/sos-style-layout branch from b5849cc to 2cb7311 Compare April 24, 2019 13:12

PR Feedback

9cafc75

pipermerriam force-pushed the piper/sos-style-layout branch from 2cb7311 to 9cafc75 Compare April 24, 2019 13:14

pipermerriam merged commit 91f0928 into ethereum:master Apr 24, 2019

pipermerriam deleted the piper/sos-style-layout branch April 24, 2019 13:16

hwwhww mentioned this pull request Apr 26, 2019

SOS style layout #63

Merged

carver added a commit that referenced this pull request Aug 18, 2022

Merge pull request #47 from carver/bump-missing-error

54c02fe

Better error if bump missing in make notes/release

	raise DeserializationError(f"Tried to read {num_bytes}. Only got {len(data)} bytes")
	raise DeserializationError(f"Tried to read {num_bytes}. Only got {len(data)} bytes")

	return offset.to_bytes(4, 'little')
	return offset.to_bytes(OFFSET_SIZE, 'little')

	return merkleize(pack([]))
	return mix_in_length(merkleize(pack([])), 0)

	# Verify that bothe iterables have been fully consumed.
	# Verify that both iterables have been fully consumed.

		pass


		def _compute_section_0_size(element_sedes: Iterable[TSedes]) -> int:

	if num_remaining_offset_bytes % 4 != 0:
	if num_remaining_offset_bytes % OFFSET_SIZE != 0:

	f"Offset bytes was not a multiple of 4. Got {num_remaining_offset_bytes}"
	f"Offset bytes was not a multiple of {OFFSET_SIZE}. Got {num_remaining_offset_bytes}"


		yield element
		num_remaining_offsets = num_remaining_offset_bytes // 4

		@@ -9,3 +9,9 @@
		MAX_CONTENT_SIZE = 2 ** (SIZE_PREFIX_SIZE * 8) - 1

		TDeserializedElement = TypeVar("TDeserializedElement")


		class HomogenousSequence(CompositeSedes[Sequence[Any], Tuple[Any]]):

	EMPTY_LIST_HASH_TRIES_ROOT = mix_in_length(merkleize(pack([])), 0)
	EMPTY_LIST_HASH_TREE_ROOT = mix_in_length(merkleize(pack([])), 0)

	raise ValueError("Cannot compute trie hash for non-empty value using `EmptyList` sedes")
	raise ValueError("Cannot compute tree hash for non-empty value using `EmptyList` sedes")

SOS style layout #47

SOS style layout #47

Conversation

pipermerriam commented Mar 15, 2019 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Apr 5, 2019

jannikluhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Apr 9, 2019

jannikluhn left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

pipermerriam commented Apr 15, 2019

pipermerriam commented Apr 18, 2019

hwwhww commented Apr 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Apr 19, 2019

pipermerriam commented Apr 19, 2019

pipermerriam commented Apr 23, 2019

jannikluhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam commented Apr 24, 2019

hwwhww commented Apr 26, 2019

pipermerriam commented Mar 15, 2019 •

edited

Loading