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

epoch transition at start of epoch #732

Merged
merged 7 commits into from
Mar 8, 2019
Merged

epoch transition at start of epoch #732

merged 7 commits into from
Mar 8, 2019

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Mar 7, 2019

Having epoch transitions during the last slot of the epoch leaves the state transitioned halfway between the two epochs. Fields related to shuffling, seed, justification, etc are updated for the following epoch but the state.slot still is set for the current epoch. This disparity between parts of the state splitting across epochs creates a number of off-by-one issues throughout helpers and verification.

For example, the attestation off by one issue that was fixed with a kludge was due to this disparity (#627).

A more important issue that Prysmatic has run into is that the shuffling assignments break when in this in between state. At the end of an epoch, say slot 63, all of the fields related to shuffling (current_shuffling_seed, current_shuffling_start_shard, etc) have been updated to reflect values for epoch starting at 64, but the state is still at slot 63. If in this state and you call something like get_crosslink_committees_for_slot for slot 64, the helper thinks you are requesting for the next_epoch and regenerates incorrect seeds.

changelog

  • Move epoch transition before slot transition and handle at the start of each rather than at the end of each epoch.
  • Create new subsection of state transition called "state-root caching". This is done before everything including epoch & slot transitions.
  • Collapse state root verification section into the end of the block processing section
  • remove attestation kludge fix off by one attestaton issue #627

note: the diff looks nasty but this is primarily just a reordering of components of the state transition function. removing the attestation kludge (+ 1) is the only non-ordering change

For clarity, the state transition now looks like this:
https://github.com/ethereum/research/blob/d15e5c78a9a296c2ebe641034c3a3998e9a9c87f/spec_pythonizer/state_transition.py#L75-L84

@terencechain
Copy link
Contributor

Can we get some background on why we need this change?

@djrtwo djrtwo changed the title [WIP] epoch transition at start of epoch epoch transition at start of epoch Mar 7, 2019
djrtwo added a commit to ethereum/research that referenced this pull request Mar 7, 2019
@JustinDrake
Copy link
Collaborator

Minor comments:

  • The sections "Block header" and "State root verification" can now be merged. (If merged, the "Block header" section would go at the very end.)
  • get_state_root is used in exactly one place. It's also a pain because the assert is preventing us from doing state.slot += 1 at the end of advance_slot, simplifying two state.slot - 1 with state.slot. I'm tempted to nuke get_state_root and do the various simplifications.

@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 8, 2019

The get helpers are particularly useful being defined for validators/clients outside of the state transition.

We can just do the array access directly within the state transition here knowing that it is safe in this context if you really want to move these lines around.

@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 8, 2019

The sections "Block header" and "State root verification" can now be merged. (If merged, the "Block header" section would go at the very end.)

Seems like 'eth1 data', 'randao', and 'block header' can be processed in parallel to all of the beacon transactions with just a final state root check at the end. I think if we merge the header and state root sections, we might encourage missing this parallelization opportunity.

djrtwo added a commit to ethereum/research that referenced this pull request Mar 8, 2019
djrtwo added a commit to ethereum/research that referenced this pull request Mar 8, 2019
djrtwo added a commit to ethereum/research that referenced this pull request Mar 8, 2019
@djrtwo djrtwo merged commit 0bcc350 into dev Mar 8, 2019
@djrtwo djrtwo deleted the epoch-start branch March 8, 2019 15:59
@djrtwo djrtwo restored the epoch-start branch March 8, 2019 16:31
@djrtwo djrtwo deleted the epoch-start branch March 8, 2019 16:31
arnetheduck added a commit to status-im/nimbus-eth2 that referenced this pull request Mar 8, 2019
* set epoch_boundary_root - chain finalizes!
* fix slotStart to offset GENESIS_SLOT
* work around bug that will be fixed by
ethereum/consensus-specs#732
* compile with debug info (there was a GC-related crash in C land)
tersec pushed a commit to status-im/nimbus-eth2 that referenced this pull request Mar 9, 2019
* set epoch_boundary_root - chain finalizes!
* fix slotStart to offset GENESIS_SLOT
* work around bug that will be fixed by
ethereum/consensus-specs#732
* compile with debug info (there was a GC-related crash in C land)
mratsim added a commit to status-im/nimbus-eth2 that referenced this pull request Mar 20, 2019
and ethereum/consensus-specs#732 epoch transition work around
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.

3 participants