[Merged by Bors] - Implement SSZ union type #2579

paulhauner · 2021-09-09T13:56:52Z

Issue Addressed

NA

Proposed Changes

Implements the "union" type from the SSZ spec for ssz, ssz_derive, tree_hash and tree_hash_derive so it may be derived for enums:

https://github.com/ethereum/consensus-specs/blob/v1.1.0-beta.3/ssz/simple-serialize.md#union

The union type is required for the merge, since the Transaction type is defined as a single-variant union Union[OpaqueTransaction].

Crate Updates

This PR will (hopefully) cause CI to publish new versions for the following crates:

eth2_ssz_derive: 0.2.1 -> 0.3.0
eth2_ssz: 0.3.0 -> 0.4.0
eth2_ssz_types: 0.2.0 -> 0.2.1
tree_hash: 0.3.0 -> 0.4.0
tree_hash_derive: 0.3.0 -> 0.4.0

These these crates depend on each other, I've had to add a workspace-level [patch] for these crates. A follow-up PR will need to remove this patch, ones the new versions are published.

Union Behaviors

We already had SSZ Encode and TreeHash derive for enums, however it just did a "transparent" pass-through of the inner value. Since the "union" decoding from the spec is in conflict with the transparent method, I've required that all enum have exactly one of the following enum-level attributes:

SSZ

#[ssz(enum_behaviour = "union")]
- matches the spec used for the merge
#[ssz(enum_behaviour = "transparent")]
- maintains existing functionality
- not supported for Decode (never was)

TreeHash

#[tree_hash(enum_behaviour = "union")]
- matches the spec used for the merge
#[tree_hash(enum_behaviour = "transparent")]
- maintains existing functionality

This means that we can maintain the existing transparent behaviour, but all existing users will get a compile-time error until they explicitly opt-in to being transparent.

Legacy Option Encoding

Before this PR, we already had a union-esque encoding for Option<T>. However, this was with the old SSZ spec where the union selector was 4 bytes. During merge specification, the spec was changed to use 1 byte for the selector.

Whilst the 4-byte Option encoding was never used in the spec, we used it in our database. Writing a migrate script for all occurrences of Option in the database would be painful, especially since it's used in the CommitteeCache. To avoid the migrate script, I added a serde-esque #[ssz(with = "module")] field-level attribute to ssz_derive so that we can opt into the 4-byte encoding on a field-by-field basis.

The ssz::legacy::four_byte_impl! macro allows a one-liner to define the module required for the #[ssz(with = "module")] for some Option<T> where T: Encode + Decode.

Notably, I have removed Encode and Decode impls for Option. I've done this to force a break on downstream users. Like I mentioned, Option isn't used in the spec so I don't think it'll be that annoying. I think it's nicer than quietly having two different union implementations or quietly breaking the existing Option impl.

Crate Publish Ordering

I've modified the order in which CI publishes crates to ensure that we don't publish a crate without ensuring we already published a crate that it depends upon.

TODO

Queue a follow-up [patch]-removing PR.

paulhauner · 2021-09-21T00:39:35Z

I've just added a fix to the CommitteeCache legacy serialization. I also rebased on unstable to fix a merge conflict.

michaelsproul

This is impeccable! 👌

testing/ef_tests/src/cases/rewards.rs

paulhauner · 2021-09-25T00:09:39Z

Thanks @michaelsproul! I've addressed your review comment in c9111f4.

I also made a small change in 91d3152 to avoid unchecked arith that was triggering clippy in the types crate.

paulhauner · 2021-09-25T00:13:06Z

Although I made a change since the last review, I believe the change is trivial. I'm keen to get this into unstable so we can tidy the merge-f2f branch and get it passing clippy. As such, I'm going to bors this. @michaelsproul could you please eyeball 91d3152, just in case? 🙏 It's not going into prod until the merge, so it's not an issue in the meantime.

bors r+

## Issue Addressed NA ## Proposed Changes Implements the "union" type from the SSZ spec for `ssz`, `ssz_derive`, `tree_hash` and `tree_hash_derive` so it may be derived for `enums`: https://github.com/ethereum/consensus-specs/blob/v1.1.0-beta.3/ssz/simple-serialize.md#union The union type is required for the merge, since the `Transaction` type is defined as a single-variant union `Union[OpaqueTransaction]`. ### Crate Updates This PR will (hopefully) cause CI to publish new versions for the following crates: - `eth2_ssz_derive`: `0.2.1` -> `0.3.0` - `eth2_ssz`: `0.3.0` -> `0.4.0` - `eth2_ssz_types`: `0.2.0` -> `0.2.1` - `tree_hash`: `0.3.0` -> `0.4.0` - `tree_hash_derive`: `0.3.0` -> `0.4.0` These these crates depend on each other, I've had to add a workspace-level `[patch]` for these crates. A follow-up PR will need to remove this patch, ones the new versions are published. ### Union Behaviors We already had SSZ `Encode` and `TreeHash` derive for enums, however it just did a "transparent" pass-through of the inner value. Since the "union" decoding from the spec is in conflict with the transparent method, I've required that all `enum` have exactly one of the following enum-level attributes: #### SSZ - `#[ssz(enum_behaviour = "union")]` - matches the spec used for the merge - `#[ssz(enum_behaviour = "transparent")]` - maintains existing functionality - not supported for `Decode` (never was) #### TreeHash - `#[tree_hash(enum_behaviour = "union")]` - matches the spec used for the merge - `#[tree_hash(enum_behaviour = "transparent")]` - maintains existing functionality This means that we can maintain the existing transparent behaviour, but all existing users will get a compile-time error until they explicitly opt-in to being transparent. ### Legacy Option Encoding Before this PR, we already had a union-esque encoding for `Option<T>`. However, this was with the *old* SSZ spec where the union selector was 4 bytes. During merge specification, the spec was changed to use 1 byte for the selector. Whilst the 4-byte `Option` encoding was never used in the spec, we used it in our database. Writing a migrate script for all occurrences of `Option` in the database would be painful, especially since it's used in the `CommitteeCache`. To avoid the migrate script, I added a serde-esque `#[ssz(with = "module")]` field-level attribute to `ssz_derive` so that we can opt into the 4-byte encoding on a field-by-field basis. The `ssz::legacy::four_byte_impl!` macro allows a one-liner to define the module required for the `#[ssz(with = "module")]` for some `Option<T> where T: Encode + Decode`. Notably, **I have removed `Encode` and `Decode` impls for `Option`**. I've done this to force a break on downstream users. Like I mentioned, `Option` isn't used in the spec so I don't think it'll be *that* annoying. I think it's nicer than quietly having two different union implementations or quietly breaking the existing `Option` impl. ### Crate Publish Ordering I've modified the order in which CI publishes crates to ensure that we don't publish a crate without ensuring we already published a crate that it depends upon. ## TODO - [ ] Queue a follow-up `[patch]`-removing PR.

bors · 2021-09-25T00:19:37Z

Build failed:

cargo-audit

paulhauner · 2021-09-25T00:25:55Z

Looks like we're blocked on #2625 for the time being.

michaelsproul

I checked the arith and it looks good 👌 Merge at will. Could even batch with the zeroize PR?

paulhauner · 2021-09-25T01:20:37Z

bors r+

## Issue Addressed NA ## Proposed Changes Implements the "union" type from the SSZ spec for `ssz`, `ssz_derive`, `tree_hash` and `tree_hash_derive` so it may be derived for `enums`: https://github.com/ethereum/consensus-specs/blob/v1.1.0-beta.3/ssz/simple-serialize.md#union The union type is required for the merge, since the `Transaction` type is defined as a single-variant union `Union[OpaqueTransaction]`. ### Crate Updates This PR will (hopefully) cause CI to publish new versions for the following crates: - `eth2_ssz_derive`: `0.2.1` -> `0.3.0` - `eth2_ssz`: `0.3.0` -> `0.4.0` - `eth2_ssz_types`: `0.2.0` -> `0.2.1` - `tree_hash`: `0.3.0` -> `0.4.0` - `tree_hash_derive`: `0.3.0` -> `0.4.0` These these crates depend on each other, I've had to add a workspace-level `[patch]` for these crates. A follow-up PR will need to remove this patch, ones the new versions are published. ### Union Behaviors We already had SSZ `Encode` and `TreeHash` derive for enums, however it just did a "transparent" pass-through of the inner value. Since the "union" decoding from the spec is in conflict with the transparent method, I've required that all `enum` have exactly one of the following enum-level attributes: #### SSZ - `#[ssz(enum_behaviour = "union")]` - matches the spec used for the merge - `#[ssz(enum_behaviour = "transparent")]` - maintains existing functionality - not supported for `Decode` (never was) #### TreeHash - `#[tree_hash(enum_behaviour = "union")]` - matches the spec used for the merge - `#[tree_hash(enum_behaviour = "transparent")]` - maintains existing functionality This means that we can maintain the existing transparent behaviour, but all existing users will get a compile-time error until they explicitly opt-in to being transparent. ### Legacy Option Encoding Before this PR, we already had a union-esque encoding for `Option<T>`. However, this was with the *old* SSZ spec where the union selector was 4 bytes. During merge specification, the spec was changed to use 1 byte for the selector. Whilst the 4-byte `Option` encoding was never used in the spec, we used it in our database. Writing a migrate script for all occurrences of `Option` in the database would be painful, especially since it's used in the `CommitteeCache`. To avoid the migrate script, I added a serde-esque `#[ssz(with = "module")]` field-level attribute to `ssz_derive` so that we can opt into the 4-byte encoding on a field-by-field basis. The `ssz::legacy::four_byte_impl!` macro allows a one-liner to define the module required for the `#[ssz(with = "module")]` for some `Option<T> where T: Encode + Decode`. Notably, **I have removed `Encode` and `Decode` impls for `Option`**. I've done this to force a break on downstream users. Like I mentioned, `Option` isn't used in the spec so I don't think it'll be *that* annoying. I think it's nicer than quietly having two different union implementations or quietly breaking the existing `Option` impl. ### Crate Publish Ordering I've modified the order in which CI publishes crates to ensure that we don't publish a crate without ensuring we already published a crate that it depends upon. ## TODO - [ ] Queue a follow-up `[patch]`-removing PR.

bors · 2021-09-25T03:12:36Z

Build failed (retrying...):

release-tests-windows

## Issue Addressed NA ## Proposed Changes Implements the "union" type from the SSZ spec for `ssz`, `ssz_derive`, `tree_hash` and `tree_hash_derive` so it may be derived for `enums`: https://github.com/ethereum/consensus-specs/blob/v1.1.0-beta.3/ssz/simple-serialize.md#union The union type is required for the merge, since the `Transaction` type is defined as a single-variant union `Union[OpaqueTransaction]`. ### Crate Updates This PR will (hopefully) cause CI to publish new versions for the following crates: - `eth2_ssz_derive`: `0.2.1` -> `0.3.0` - `eth2_ssz`: `0.3.0` -> `0.4.0` - `eth2_ssz_types`: `0.2.0` -> `0.2.1` - `tree_hash`: `0.3.0` -> `0.4.0` - `tree_hash_derive`: `0.3.0` -> `0.4.0` These these crates depend on each other, I've had to add a workspace-level `[patch]` for these crates. A follow-up PR will need to remove this patch, ones the new versions are published. ### Union Behaviors We already had SSZ `Encode` and `TreeHash` derive for enums, however it just did a "transparent" pass-through of the inner value. Since the "union" decoding from the spec is in conflict with the transparent method, I've required that all `enum` have exactly one of the following enum-level attributes: #### SSZ - `#[ssz(enum_behaviour = "union")]` - matches the spec used for the merge - `#[ssz(enum_behaviour = "transparent")]` - maintains existing functionality - not supported for `Decode` (never was) #### TreeHash - `#[tree_hash(enum_behaviour = "union")]` - matches the spec used for the merge - `#[tree_hash(enum_behaviour = "transparent")]` - maintains existing functionality This means that we can maintain the existing transparent behaviour, but all existing users will get a compile-time error until they explicitly opt-in to being transparent. ### Legacy Option Encoding Before this PR, we already had a union-esque encoding for `Option<T>`. However, this was with the *old* SSZ spec where the union selector was 4 bytes. During merge specification, the spec was changed to use 1 byte for the selector. Whilst the 4-byte `Option` encoding was never used in the spec, we used it in our database. Writing a migrate script for all occurrences of `Option` in the database would be painful, especially since it's used in the `CommitteeCache`. To avoid the migrate script, I added a serde-esque `#[ssz(with = "module")]` field-level attribute to `ssz_derive` so that we can opt into the 4-byte encoding on a field-by-field basis. The `ssz::legacy::four_byte_impl!` macro allows a one-liner to define the module required for the `#[ssz(with = "module")]` for some `Option<T> where T: Encode + Decode`. Notably, **I have removed `Encode` and `Decode` impls for `Option`**. I've done this to force a break on downstream users. Like I mentioned, `Option` isn't used in the spec so I don't think it'll be *that* annoying. I think it's nicer than quietly having two different union implementations or quietly breaking the existing `Option` impl. ### Crate Publish Ordering I've modified the order in which CI publishes crates to ensure that we don't publish a crate without ensuring we already published a crate that it depends upon. ## TODO - [ ] Queue a follow-up `[patch]`-removing PR.

bors · 2021-09-25T05:10:31Z

This PR was included in a batch that successfully built, but then failed to merge into unstable. It will not be retried.

Additional information:

{"message":"You're not authorized to push to this branch. Visit https://docs.github.com/articles/about-protected-branches/ for more information.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

paulhauner · 2021-09-25T05:58:22Z

bors r+

## Issue Addressed NA ## Proposed Changes Implements the "union" type from the SSZ spec for `ssz`, `ssz_derive`, `tree_hash` and `tree_hash_derive` so it may be derived for `enums`: https://github.com/ethereum/consensus-specs/blob/v1.1.0-beta.3/ssz/simple-serialize.md#union The union type is required for the merge, since the `Transaction` type is defined as a single-variant union `Union[OpaqueTransaction]`. ### Crate Updates This PR will (hopefully) cause CI to publish new versions for the following crates: - `eth2_ssz_derive`: `0.2.1` -> `0.3.0` - `eth2_ssz`: `0.3.0` -> `0.4.0` - `eth2_ssz_types`: `0.2.0` -> `0.2.1` - `tree_hash`: `0.3.0` -> `0.4.0` - `tree_hash_derive`: `0.3.0` -> `0.4.0` These these crates depend on each other, I've had to add a workspace-level `[patch]` for these crates. A follow-up PR will need to remove this patch, ones the new versions are published. ### Union Behaviors We already had SSZ `Encode` and `TreeHash` derive for enums, however it just did a "transparent" pass-through of the inner value. Since the "union" decoding from the spec is in conflict with the transparent method, I've required that all `enum` have exactly one of the following enum-level attributes: #### SSZ - `#[ssz(enum_behaviour = "union")]` - matches the spec used for the merge - `#[ssz(enum_behaviour = "transparent")]` - maintains existing functionality - not supported for `Decode` (never was) #### TreeHash - `#[tree_hash(enum_behaviour = "union")]` - matches the spec used for the merge - `#[tree_hash(enum_behaviour = "transparent")]` - maintains existing functionality This means that we can maintain the existing transparent behaviour, but all existing users will get a compile-time error until they explicitly opt-in to being transparent. ### Legacy Option Encoding Before this PR, we already had a union-esque encoding for `Option<T>`. However, this was with the *old* SSZ spec where the union selector was 4 bytes. During merge specification, the spec was changed to use 1 byte for the selector. Whilst the 4-byte `Option` encoding was never used in the spec, we used it in our database. Writing a migrate script for all occurrences of `Option` in the database would be painful, especially since it's used in the `CommitteeCache`. To avoid the migrate script, I added a serde-esque `#[ssz(with = "module")]` field-level attribute to `ssz_derive` so that we can opt into the 4-byte encoding on a field-by-field basis. The `ssz::legacy::four_byte_impl!` macro allows a one-liner to define the module required for the `#[ssz(with = "module")]` for some `Option<T> where T: Encode + Decode`. Notably, **I have removed `Encode` and `Decode` impls for `Option`**. I've done this to force a break on downstream users. Like I mentioned, `Option` isn't used in the spec so I don't think it'll be *that* annoying. I think it's nicer than quietly having two different union implementations or quietly breaking the existing `Option` impl. ### Crate Publish Ordering I've modified the order in which CI publishes crates to ensure that we don't publish a crate without ensuring we already published a crate that it depends upon. ## TODO - [ ] Queue a follow-up `[patch]`-removing PR.

bors · 2021-09-25T07:53:47Z

Pull request successfully merged into unstable.

Build succeeded:

njgheorghita · 2021-11-15T19:37:14Z

@paulhauner 👋 It looks like the new versions of eth2_ssz / eth2_ssz_derive / eth2_ssz_types weren't published. Do you know if/when you expect them to be available?

paulhauner added the work-in-progress PR is a work-in-progress label Sep 9, 2021

paulhauner mentioned this pull request Sep 10, 2021

Update to union-supporting SSZ #2581

Closed

1 task

paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Sep 13, 2021

paulhauner marked this pull request as ready for review September 13, 2021 07:34

paulhauner added 25 commits September 21, 2021 10:37

Add basic encode impl

a621bb3

Add UnionSelector to ssz encode

03e3fbd

Comment-out existing Option impl

7c2950f

Add non-functional decode impl

a2a3716

Complete Decode impl

7b41553

Tidy comments

0350c47

Remove option code

b02f3f5

Add comments to union selector

0d1d5d8

Bump ssz and ssz_derive versions

16c2be4

Remove tests

1e751e2

Freshen cargo.lock

23af4c4

Freshen cargo.lock

171ff61

Remove unused union impl for ssz encoder

ba9db13

Add basic union tree hash method

522d020

Tidy tree hash

189d025

Fix bugs, bump version

f78a7c4

Add ssz tests

bf7a4a8

Add top-level patch

c09e28d

Add tree_hash_derive tests

2e7a28e

Patch ssz types

0066d05

Update tree hash to 0.4.0

ef2ff5d

Finish tree hash tests

8afd431

Apply enum_behaviour

40c2a8e

Bump eth2_ssz_types

9e84579

Bump ssz, ssz_derive versions

62521dd

paulhauner added 2 commits September 21, 2021 10:38

Add test for decoding genesis state

2c4a47f

Fix shim in committee cache

76964de

paulhauner force-pushed the ssz-union branch from e1317c7 to 76964de Compare September 21, 2021 00:38

michaelsproul approved these changes Sep 24, 2021

View reviewed changes

testing/ef_tests/src/cases/rewards.rs Outdated Show resolved Hide resolved

michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Sep 24, 2021

paulhauner added 2 commits September 25, 2021 10:01

Remove unchecked arith from ssz_derive

91d3152

Remove ssz impl from AllDeltas

c9111f4

michaelsproul approved these changes Sep 25, 2021

View reviewed changes

paulhauner added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 25, 2021

bors bot changed the title ~~Implement SSZ union type~~ [Merged by Bors] - Implement SSZ union type Sep 25, 2021

bors bot closed this Sep 25, 2021

paulhauner mentioned this pull request Sep 27, 2021

Merge F2F #2620

Closed

7 tasks

jacobkaufmann mentioned this pull request Nov 15, 2021

Update message types to match wire protocol test vectors ethereum/trin#181

Merged

[Merged by Bors] - Implement SSZ union type #2579

[Merged by Bors] - Implement SSZ union type #2579

Uh oh!

Conversation

paulhauner commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Crate Updates

Union Behaviors

SSZ

TreeHash

Legacy Option Encoding

Crate Publish Ordering

TODO

Uh oh!

paulhauner commented Sep 21, 2021

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulhauner commented Sep 25, 2021

Uh oh!

paulhauner commented Sep 25, 2021

Uh oh!

bors bot commented Sep 25, 2021

Uh oh!

paulhauner commented Sep 25, 2021

Uh oh!

michaelsproul left a comment

Choose a reason for hiding this comment

Uh oh!

paulhauner commented Sep 25, 2021

Uh oh!

bors bot commented Sep 25, 2021

Uh oh!

bors bot commented Sep 25, 2021

Uh oh!

paulhauner commented Sep 25, 2021

Uh oh!

bors bot commented Sep 25, 2021

Uh oh!

njgheorghita commented Nov 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paulhauner commented Sep 9, 2021 •

edited

Loading