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

Cleanup containers #1156

Merged
merged 7 commits into from
Jun 17, 2019
Merged

Cleanup containers #1156

merged 7 commits into from
Jun 17, 2019

Conversation

JustinDrake
Copy link
Contributor

@JustinDrake JustinDrake commented Jun 9, 2019

  1. Use custom types in container definitions (fix Use custom types for container definitions #1153)
  2. Renamings
  • deposit_index => eth1_deposit_index (more precise and future proof; consistent with eth1_data and eth1_data_votes in "Eth1" grouping)
  • previous_epoch_attestations => previous_attestations (consistent with previous_crosslinks, previous_justified_epoch and previous_justified_root; shorter)
  • validator_registry => validators (cleaner; more semantically correct—the registry is now both validators and balances; shorter)
  • Remove most latest_ prefixes (cleaner; shorter; the prefix does not add much other than clutter; we make an exception for latest_block_header)
  1. Add description to not-immediately-obvious container fields
  2. Semantically group BeaconState fields, following a logical progression (see item 14 in Phase 0 state transition checklist—take 5 #1054)
    # Versioning
    genesis_time: uint64
    slot: Slot
    fork: Fork
    # History
    latest_block_header: BeaconBlockHeader
    block_roots: Vector[Hash, SLOTS_PER_HISTORICAL_ROOT]
    state_roots: Vector[Hash, SLOTS_PER_HISTORICAL_ROOT]
    historical_roots: List[Hash]
    # Eth1
    eth1_data: Eth1Data
    eth1_data_votes: List[Eth1Data]
    eth1_deposit_index: uint64
    # Registry
    validators: List[Validator]
    balances: List[Gwei]
    # Shuffling
    start_shard: Shard
    randao_mixes: Vector[Hash, RANDAO_MIXES_LENGTH]
    active_index_roots: Vector[Hash, ACTIVE_INDEX_ROOTS_LENGTH]
    # Slashings
    slashed_balances: Vector[Gwei, SLASHED_EXIT_LENGTH]
    # Attestations
    previous_epoch_attestations: List[PendingAttestation]
    current_epoch_attestations: List[PendingAttestation]
    # Crosslinks
    previous_crosslinks: Vector[Crosslink, SHARD_COUNT]
    current_crosslinks: Vector[Crosslink, SHARD_COUNT]
    # Justification
    previous_justified_epoch: Epoch
    previous_justified_root: Hash
    current_justified_epoch: Epoch
    current_justified_root: Hash
    justification_bitfield: uint64
    # Finality
    finalized_epoch: Epoch
    finalized_root: Hash

This was referenced Jun 9, 2019
@djrtwo
Copy link
Contributor

djrtwo commented Jun 10, 2019

validator_registry => validators

We debated this long ago and decided that validator_registry was more explicit -- it is the entire registry of validators rather than just a list of validators

@djrtwo
Copy link
Contributor

djrtwo commented Jun 10, 2019

previous_epoch_attestations => previous_attestations (consistent with previous_crosslinks, previous_justified_epoch and previous_justified_root; shorter)

There is a difference between previous_epoch_attestations and previous_crosslinks/previous_justified_epoch.

previous_epoch_attestations are the attestations for the previous epoch. Not the attestations collected during such an epoch.

Whereas previous_crosslinks were the crosslinks during the previous epoch. Same with previous_justified_epoch -- this was the latest justified_epoch during the previous epoch.

@JustinDrake
Copy link
Contributor Author

We debated this long ago and decided that validator_registry was more explicit

I was in favour of validator_registry in that debate. Since then the situation has changed with the registry split being into two parts:

    # Registry
    validators: List[Validator]
    balances: List[Gwei]

specs/core/0_fork-choice.md Outdated Show resolved Hide resolved
slot: Slot
fork: Fork
# History
parent_block_header: BeaconBlockHeader
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the semantics here.
The latest_block_header seen should be the parent of the next block, but it is not a parent when recorded and not during any skipped slots when it is recorded into block_roots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that naming is awkward here. Brainstorming:

  • block_header (matches the type nicely; vagueness hides difficult-to-explain subtleties)
  • partial_block_header (a bit more accurate)
  • latest_block_header (make an exception here for "latest" prefix)
  • latest_partial_block_header (a bit of mouthful)

Copy link
Contributor

Choose a reason for hiding this comment

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

latest_ seems reasonable and purposefully descriptive here (as opposed to it just being thrown on all the history arrays).

Copy link
Contributor

Choose a reason for hiding this comment

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

bump @JustinDrake

Just handled merge conflict. let's resolve this and get merged.

@JustinDrake
Copy link
Contributor Author

@protolambda, @hwwhww Looks like there is a race condition in the linter CI. I didn't change phase 1 yet the CI is complaining about it.

@djrtwo I think I've addressed all issues, and I've added a bunch of descriptions to field containers. Feel free to edit/add/remove those as you feel appropriate :)

@protolambda
Copy link
Contributor

protolambda commented Jun 15, 2019

@JustinDrake I'll take a closer look. Best guess would be that you changed something in phase 0, and it wound up as part of phase 1.
Edit: yes, matching errors in phase-0 and phase-1 spec. Not a race condition. Just your code being phase-1 compatible, with the linting issues included...

@protolambda
Copy link
Contributor

@JustinDrake fixed linting in 75b4692 . Not a race condition, just one missing space. You can run make -B pyspec and make lint to check your changes locally. 👍

@JustinDrake JustinDrake added the milestone:June 30 freeze 🥶 Phase 0 spec freeze for long-lived cross-client testnet label Jun 15, 2019
@djrtwo djrtwo merged commit c70643e into dev Jun 17, 2019
@djrtwo djrtwo deleted the container-cleanup branch June 17, 2019 19:30
@hwwhww hwwhww mentioned this pull request Jun 17, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
milestone:June 30 freeze 🥶 Phase 0 spec freeze for long-lived cross-client testnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use custom types for container definitions
3 participants