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

Spec update: remove BeaconState.shard_committees_at_slots in favor of crosslink committees; validator_registry_latest_change_slot -> validator_registry_update_slot mechanical renaming #73

Merged
merged 2 commits into from
Jan 28, 2019

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Jan 26, 2019

This works more or less, but fails at slot 128 due to some epoch boundary ethereum/consensus-specs#492. https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#get_crosslink_committees_at_slot has assert state_epoch_slot <= slot + EPOCH_LENGTH i.e. assert state.slot - (state.slot % EPOCH_LENGTH) <= slot + EPOCH_LENGTH and https://github.com/ethereum/eth2.0-specs/blob/master/specs/core/0_beacon-chain.md#crosslinks has For every slot in range(state.slot - 2 * EPOCH_LENGTH, state.slot), let crosslink_committees_at_slot = get_crosslink_committees_at_slot(state, slot).

At state.slot == 128, these conflict, resolving to For every slot in range(0, 64), let crosslink_committees_at_slot = get_crosslink_committees_at_slot(state, slot) which enumerates through get_crosslink_committees_at_slot(state, 0), but assert state.slot - (state.slot % EPOCH_LENGTH) <= slot + EPOCH_LENGTH i.e. assert 128 - 0 <= 0 + 64 then fails.

A previous variation on this bug also showed up at ethereum/consensus-specs#409

I don't want this PR to include them, but next steps are:
(1) when ethereum/consensus-specs#492 or something similar is merged, pull that in, to fix the slot 128 issue.

(2) address the ugliness and clumsiness around ShardCommittee being replaced by this by-hand-constructed, hierarchically composed type. It's why I named the fields a and b; otherwise they're just too verbose in type signatures, etc.

(3) Incrementally remove the remaining references to/adapters for the previous ShardCommittee-based world.

…mmittees; validator_registry_latest_change_slot -> validator_registry_update_slot mechanical renaming
@tersec tersec requested review from zah and mratsim January 26, 2019 19:41
Copy link
Contributor

@mratsim mratsim left a comment

Choose a reason for hiding this comment

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

LGTM, hopefully has a spurious side-effect it also fixes simulation on windows

@zah zah merged commit 7a79035 into master Jan 28, 2019
etan-status pushed a commit that referenced this pull request May 12, 2022
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