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.

Sign up for GitHub

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

Jump to bottom

get_start_shard proposal #1858

Merged
merged 7 commits into from
Jun 3, 2020
Merged

get_start_shard proposal #1858

merged 7 commits into from
Jun 3, 2020

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented May 29, 2020 •
edited
Loading

Issue

Implement get_start_shard

How did I fix it

  1. Add current_epoch_start_shard to BeaconState.
  2. Implement get_start_shard
    1. Use state.current_epoch_start_shard as the current view, to plus or substract the committee count delta.
    2. get_start_shard and get_committee_count_at_slot results should be cached.
  3. [minimal config] Change INITIAL_ACTIVE_SHARDS from 4 to 2 for more frequent crosslinks

TODO

  • add tests

@hwwhww hwwhww added the phase1 label May 29, 2020
@hwwhww hwwhww changed the title get_start_shard proposal [WIP] get_start_shard proposal May 29, 2020
terencechain
terencechain reviewed May 31, 2020
View reviewed changes
specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase1/beacon-chain.md Show resolved Hide resolved
specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
djrtwo
djrtwo reviewed May 31, 2020
View reviewed changes
specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase1/beacon-chain.md Show resolved Hide resolved
specs/phase1/beacon-chain.md Outdated Show resolved Hide resolved
djrtwo
djrtwo approved these changes Jun 2, 2020
View reviewed changes
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.

looks good to me!

@djrtwo
Copy link
Contributor

djrtwo commented Jun 3, 2020

whoops, realized the todo : "Add tests"

:)

@hwwhww
Add get_start_shard unittests and update minimal config
68e934b 
1. Add unittests for testing `get_start_shard` with better granularity
2. Change `INITIAL_ACTIVE_SHARDS` from `4` to `2` for tight crosslinking
@hwwhww hwwhww changed the title [WIP] get_start_shard proposal get_start_shard proposal Jun 3, 2020
@hwwhww
Copy link
Contributor Author

hwwhww commented Jun 3, 2020

@djrtwo

Added unit tests (not for test generator)!

Also, the minimal config's INITIAL_ACTIVE_SHARDS was set to 4. With test suite default to 256 validators, the committee_count_per_slot would be only 2. So I changed INITIAL_ACTIVE_SHARDS to 2 such that normally, we can crosslink all shards at every slot.

@hwwhww hwwhww requested a review from djrtwo June 3, 2020 15:25
djrtwo
djrtwo approved these changes Jun 3, 2020
View reviewed changes
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.

great!
One style suggestion and then go for the merge

tests/core/pyspec/eth2spec/test/phase_1/unittests/test_get_start_shard.py Outdated Show resolved Hide resolved
@hwwhww @djrtwo
PR feedback from Danny
a685be3 
Co-authored-by: Danny Ryan <[email protected]>
@hwwhww hwwhww merged commit eb21648 into dev Jun 3, 2020
@hwwhww hwwhww deleted the hwwhww/get_start_shard branch June 3, 2020 16:56
@hwwhww hwwhww mentioned this pull request Jun 9, 2020
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terencechain terencechain terencechain left review comments

@djrtwo djrtwo djrtwo approved these changes

Assignees
No one assigned
Labels
phase1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hwwhww @djrtwo @terencechain