Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Bump SSZ #499

Merged
merged 19 commits into from
May 2, 2019
Merged

Bump SSZ #499

merged 19 commits into from
May 2, 2019

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Apr 13, 2019

What was wrong?

Fixed #409, #365
Bump to the new SSZ.

Note that it requires ethereum/py-ssz#64 PR merged.

How was it fixed?

  1. Use Vector sedes
  2. Use py-ssz root property for the hash tree root
  3. Use py-ssz signing_root property
  4. Use ssz.tree_hash_root for calculating BeaconState.latest_active_index_roots
  5. Fix the SSZ objects dependency: the dependency between BeaconBlockHeader, BeaconBlockBody and ProposerSlashing are twisted together. This PR moves BeaconBlockHeader to eth2/beacon/types/block_headers.py so that ProposerSlashing can import eth2.beacon.types.block_headers

Cute Animal Picture

hare-2218452_640

@hwwhww hwwhww force-pushed the bump_ssz branch 2 times, most recently from 19479cc to 0ba687b Compare April 18, 2019 01:13
@hwwhww hwwhww marked this pull request as ready for review April 18, 2019 02:34
@ralexstokes
Copy link
Member

ralexstokes commented Apr 23, 2019

@hwwhww not to delay things but do we want to wait for ethereum/py-ssz#58 to be incorporated -- should let us drop the tree_root and just go use the superclass's root property

it may change this PR to "Bump ssz to 0.1.0a5 so i understand if we don't want to wait for that whole process before merging the rest of this in :)

# SSZ
@pytest.fixture(scope="function", autouse=True)
def override_length(config):
BeaconState._meta.container_sedes.field_name_to_sedes["latest_randao_mixes"].length = config.LATEST_RANDAO_MIXES_LENGTH # noqa: E501
Copy link
Contributor

Choose a reason for hiding this comment

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

How about putting the field-name/vector length pairs in a dict and then setting all of them in a loop?

@hwwhww hwwhww closed this Apr 26, 2019
@hwwhww hwwhww reopened this Apr 26, 2019
@hwwhww hwwhww changed the title Bump ssz to 0.1.0a4 Bump SSZ Apr 26, 2019
@hwwhww hwwhww mentioned this pull request Apr 29, 2019
@@ -80,6 +83,8 @@
beacon_chain_config = BeaconChainConfig(chain_name='TestTestTest', genesis_data=genesis_data)
chain_class = beacon_chain_config.beacon_chain_class

override_vector_length(XIAO_LONG_BAO_CONFIG)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

/cc @ChihChengLiang, @mhchia
After this PR, it requires overriding SSZ Vector length from config before using eth2.beacon.types.* classes.

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

Lots of comments, but almost all of them about the same thing.

eth2/beacon/tools/misc/ssz_vector.py Outdated Show resolved Hide resolved
eth2/beacon/types/attestation_data_and_custody_bits.py Outdated Show resolved Hide resolved
eth2/beacon/types/attestation_data_and_custody_bits.py Outdated Show resolved Hide resolved
def __repr__(self) -> str:
return '<BlockHeader #{0} {1}>'.format(
self.slot,
encode_hex(self.signing_root)[2:10],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
encode_hex(self.signing_root)[2:10],
encode_hex(self.root)[2:10],

IMO it could be confusing if we use the signing root here because it's possible that two headers only differ by their signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I think that's because we now use signing_root as:

  1. previous_block_root
  2. the key in ChainDB

We can output both signing_root and root.

eth2/beacon/types/block_headers.py Outdated Show resolved Hide resolved
eth2/beacon/types/states.py Outdated Show resolved Hide resolved
eth2/beacon/types/states.py Outdated Show resolved Hide resolved
eth2/beacon/types/transfers.py Outdated Show resolved Hide resolved
eth2/beacon/types/voluntary_exits.py Outdated Show resolved Hide resolved
eth2/beacon/types/voluntary_exits.py Outdated Show resolved Hide resolved
@hwwhww hwwhww force-pushed the bump_ssz branch 2 times, most recently from db60596 to 304c96a Compare May 2, 2019 12:00
@hwwhww hwwhww merged commit 140a3eb into ethereum:master May 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify BeaconState and Deposit fields to utilize new ssz fixed length tuples
4 participants