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

Rework initial shard slot (PHASE_1_FORK_SLOT) #1971

Merged
merged 7 commits into from
Jul 28, 2020
Merged

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Jul 16, 2020

Issue

Fix #1877

How did I fix it

  1. Rename PHASE_1_GENESIS_SLOT to PHASE_1_FORK_SLOT: IMHO it's clearer. (Sorry!)
  2. Set initial shard_state.slot to compute_previous_slot(pre.slot), where pre is the beacon state.
  3. Per @mkalinin's suggestion, make PHASE_1_FORK_SLOT = GENESIS_SLOT = 0 for the test vectors generation.
  4. Related to Enable test generation for phase1 #1957 - now we can enable more phase 0 tests to be phase-1-compatible!

TODO

Since it makes PHASE_1_FORK_SLOT = GENESIS_SLOT = 0, we have to add fork boundary tests with a new config with a reasonable value, e.g., PHASE_1_FORK_SLOT := Slot(256). Leave it to future PRs.

@hwwhww hwwhww added the phase1 label Jul 16, 2020
@hwwhww hwwhww force-pushed the hwwhww/phase-1-fork-slot branch from 8148ef2 to 0b81c96 Compare July 16, 2020 16:46
@hwwhww hwwhww requested review from djrtwo and protolambda July 16, 2020 16:47
Copy link
Collaborator

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

I have made a couple of suggestions that IMHO are to increase readability for a little bit.

The other thing might worth mentioning. These changes doesn't allow for shard blocks production during PHASE_1_FORK_SLOT while in the case where PHASE_1_FORK_SLOT > GENESIS_SLOT, e.g. the Mainnet, there would be an opportunity to do that as compute_previous_slot(pre.slot) in shard states initializer would return pre.slot - 1.

Do we want to use this opportunity? If yes then PHASE_1_FORK_SLOT in the places where it addresses off-by-one issue should be replaced with GENESIS_SLOT.

specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
@hwwhww
Copy link
Contributor Author

hwwhww commented Jul 22, 2020

Note that with this PR:

  1. The PHASE_1_FORK_SLOT == Slot(128) case is like:

phase_1_fork_slot_128

  1. the PHASE_1_FORK_SLOT == GENESIS_SLOT case is like:

phase_1_fork_slot_0

The shard block 0 doesn't exist in this case so transition_to_valid_shard_slot helper is for forwarding to Slot(1).

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

excellent!

@hwwhww hwwhww merged commit fc38fc1 into dev Jul 28, 2020
@hwwhww hwwhww deleted the hwwhww/phase-1-fork-slot branch July 28, 2020 16:12
hwwhww added a commit that referenced this pull request Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial shard slot number
3 participants