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

GENESIS_SLOT == 0 #896

Merged
merged 15 commits into from
Apr 18, 2019
Merged

GENESIS_SLOT == 0 #896

merged 15 commits into from
Apr 18, 2019

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Apr 11, 2019

  • Move genesis slot and epoch to 0
  • ensure get_previous_epoch cannot underflow and that usage of this function during GENESIS_EPOCH is correct
  • remove check for greater than GENESIS_SLOT in process_attestation because there is nothing less than GENESIS_SLOT
  • add blockers to the following functions
    • update_justification_and_finalization at and before GENESIS_EPOCH+1
    • apply_rewards at GENESIS_EPOCH
  • add sanity test for processing finality through first 5 epochs

Currently blocked by #920

@djrtwo djrtwo changed the title [WIP] GENESIS_SLOT == 0 GENESIS_SLOT == 0 Apr 12, 2019
if state.slot == GENESIS_SLOT:
validator.activation_epoch = GENESIS_EPOCH
else:
validator.activation_epoch = get_delayed_activation_exit_epoch(get_current_epoch(state))
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember that @vbuterin chose to use the flag as a cleaner solution (#374 (comment)). :p I'm fine either way!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine either way. But if we are going to have the Genesis slot be 0 why not just remove the constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which, GENESIS_EPOCH?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for removing the constants gensis-slot and genesis-epoch. 0 is perfectly clear.

specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Show resolved Hide resolved
JustinDrake added a commit that referenced this pull request Apr 14, 2019
* Remove unnecessary per-field comments, focus on field grouping
* Group `Validator` fields relevant to light-clients together (small optimisation for light clients)
* Rework grouping of `BeaconState` fields
* `deposit_index` => `eth1_deposit_index`

Do not merge before:

* #695 which specifies custom types for individual container fields, offsetting the removal of comments in some instances
* #896 and #843 so that we don't have to continuously maintain the genesis `BeaconState`
* #874 which introduces `current_crosslinks` and `previous_crosslinks`
@JustinDrake JustinDrake mentioned this pull request Apr 14, 2019
@hwwhww
Copy link
Contributor

hwwhww commented Apr 17, 2019

After merging with #925, one of the new tests assert state.current_justified_root == prev_state.current_justified_root fails

https://github.com/ethereum/eth2.0-specs/blob/3c8d1b23a52c8be12c01628d8546a4443a217ca7/tests/phase0/test_sanity.py#L65-L66

Because

  1. state.current_justified_root was updated in https://github.com/ethereum/eth2.0-specs/blob/a93d34b8e4c46196a52afce3047897fa0bea1b86/specs/core/0_beacon-chain.md#justification:
        state.current_justified_epoch = get_current_epoch(state)
        state.current_justified_root = get_block_root(state, get_epoch_start_slot(state.current_justified_epoch))
  2. state.current_justified_epoch is 0, but state.current_justified_root NOT not initial value ZERO_HASH.

I think it does make sense to say when state.current_justified_epoch == 0, the current_justified_root is the genesis_block_root. However, it's no way to set it in genesis_state since genesis_block_root is built with genesis_state_root...
The easy solution might be either:

  1. Take it as a special case: add some condition check if in update_justification_and_finalization; if it's genesis epoch, don't use get_block_root to update current_justified_root.
  2. As before Decouple justification and finalization processing #925, only call get_block_root when state.current_justified_epoc does change.

@djrtwo
Copy link
Contributor Author

djrtwo commented Apr 17, 2019

@hwwhww I simply made the update_justification_and_finalization exit if if get_current_epoch(state) <= GENESIS_EPOCH + 1: rather than just on GENESIS_EPOCH. This avoids updating justification to GENESIS_EPOCH and finalization to GENESIS_EPOCH as well. The first epoch for either than can be written to state is epoch 1

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

5 participants