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

Remove Shards/Crosslinks from Phase 0 #1428

Merged
merged 24 commits into from Oct 28, 2019

Conversation

@djrtwo
Copy link
Contributor

djrtwo commented Oct 12, 2019

In light of discussions around the alternate phase 1 proposal, this PR removes the notion of Shard and Crosslink entirely from Phase 0 to allow for (1) a clean slate to design phase 1 on top of and (2) to enable client implementers to keep moving forward on phase 0 testnets.

  • Removed all Shard and Crosslink mechanics (Crosslink, AttestationData.crosslink, BeaconState.start_shard, process_crosslinks, get_crosslink_deltas, etc)
  • Added notion of CrosslinkIndex, AttestationData.index, and MAX_COMMITTEES_PER_SLOT. This indexes committees on a per-slot basis. The notion of Shard will map on top of this in phase 1.
  • Add slot to AttestationData. Required in this new context because with the removal shard, we can't reverse engineer the slot. Also we've been planning on doing this anyway.
  • Renamed "crosslink committee" to "beacon committee"
  • Temporarily disabled phase 1 tests from CI to isolate phase 0 for discussion before fixing/removing things from phase 1
  • constants changes
    • SECONDS_PER_SLOT from 6 to 12 -- more events will occur per slot in these modified proposals so 12 to be conservation
    • SLOTS_PER_EPOCH from 64 to 32 -- reduction of 64 to 32 here keeps epochs at 6 minutes and also help with crosslink committee sizes (wrt TARGET_COMMITTEE_SIZE)
    • the combination of the prior two changes keep the same number of epochs per year so the same reward rate
    • BASE_REWARDS_PER_EPOCH from 5 to 4 -- adjustment due to the removal of crosslinks. keeps rewards constant
    • MAX_VALIDATORS_PER_COMMITTEE to 2048 -- TOTAL_ETH_SUPPLY // ETH_PER_VALIDATOR // SLOTS_PER_EPOCH // MAX_COMMITTEES_PER_SLOT -- 2**27 // 32 // 32 // 64
djrtwo added 6 commits Oct 12, 2019
@djrtwo djrtwo requested review from protolambda and JustinDrake and removed request for protolambda Oct 12, 2019
djrtwo added 2 commits Oct 12, 2019
Copy link
Contributor

terencechain left a comment

Looks good so far. Crosschecked by implementing the changes in this Prysm branch. Able to build beacon node & validator client binaries. Haven't tested run time yet

specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
djrtwo added 2 commits Oct 13, 2019
@terencechain

This comment has been minimized.

Copy link
Contributor

terencechain commented Oct 14, 2019

what's the timeline for this? do we plan to roll this out for the next release? : )

djrtwo added 2 commits Oct 16, 2019
@djrtwo

This comment has been minimized.

Copy link
Contributor Author

djrtwo commented Oct 16, 2019

@terencechain, Made a few minor changes today. Still check out on your end?

what's the timeline for this? do we plan to roll this out for the next release? : )

Discussing this now. Want to get this out asap, but also want to keep things moving forward smoothly on the client/testnet side of things

djrtwo added 3 commits Oct 17, 2019
@JustinDrake

This comment has been minimized.

Copy link
Collaborator

JustinDrake commented Oct 17, 2019

I'm loving the cleaner separation between phase 0 and phase 1 👍

Minor comments:

  1. The Shard custom type can go
  2. Transfers can go
  3. Consider renaming get_committees_per_slot to get_committee_count_at_slot ("at slot" is consistent with get_block_root_at_slot; "committee_count" is more correct than "committees")
  4. index should go immediately after slot in AttestationData (remove "Committee Index" comment (with inconsistent capitalisation 😂))
  5. Consider the shorter "a committee index at a slot" over "an index for a committee within a slot"
  6. MAX_COMMITTEES_PER_SLOT => MAX_COMMITTEE_COUNT_PER_SLOT
  7. Should MAX_VALIDATORS_PER_COMMITTEE now be 2**11?
  8. The "Shuffling" section comment in BeaconState is now more accurately "Randomness"
  9. Rename compute_epoch_of_slot to compute_epoch_at_slot (consistent with get_block_root_at_slot and get_committee_count_at_slot)
  10. If we remove the notion of persistent committee, do we need compute_committee?
Copy link
Contributor

protolambda left a comment

The phase 1 documents reference crosslinks still. What is the plan there, leave it as-is till we have "packages" (or a new form of crosslink) for phase 1? In that case we should add a note to top of the documents, and maybe refer to an issue/PR for open discussion.

specs/core/1_shard-data-chains.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Show resolved Hide resolved
specs/core/0_fork-choice.md Outdated Show resolved Hide resolved
specs/core/0_fork-choice.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/1_beacon-chain-misc.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
configs/minimal.yaml Show resolved Hide resolved
@hwwhww hwwhww referenced this pull request Oct 17, 2019
3 of 3 tasks complete
@djrtwo

This comment has been minimized.

Copy link
Contributor Author

djrtwo commented Oct 17, 2019

@JustinDrake

  1. Shard type -- Will move to phase 1 (to be majorly reworked but the type being there keeps build fine for now
  2. Transfers -- will open a separate PR to make this more digestible for implementers
  3. get_committees_per_slot -> get_committee_count_at_slot -- will do
  4. index should go immediately after slot -- Was planning on it, but forgot. thanks!
  5. Consider the shorter "a committee index at a slot" over "an index for a committee within a slot" -- affirmative
  6. MAX_COMMITTEE_COUNT_PER_SLOT -- EDIT: I think it is in line with previous naming -- similar to MAX_VALIDATORS_PER_COMMITTEE -- and it is not quite as long which is good
  7. MAX_VALIDATORS_PER_COMMITTEE now be 2**11 -- will be different for sure. will be defined in relation to the number of committees per slot but also the number of slots per epoch which is up for debate. I'll make the number work in relation to the current constants
  8. "Randomness" -- willl do
  9. compute_epoch_of_slot to compute_epoch_at_slot -- would rather reduce random cleanup changes, but.... okay
  10. Think we should keep compute_committee. We are likely going to have a persistent committee upon which shard block proposers are selected and which serve as the p2p backbone for the shard
djrtwo added 3 commits Oct 18, 2019
@djrtwo djrtwo force-pushed the phase0-simplify branch from 37bdc1e to d5a2535 Oct 18, 2019
@djrtwo

This comment has been minimized.

Copy link
Contributor Author

djrtwo commented Oct 18, 2019

@JustinDrake @protolambda @hwwhww @terencechain
Thank you for the feedback! Comments addressed

@djrtwo djrtwo force-pushed the phase0-simplify branch from 94a9868 to a11b012 Oct 20, 2019
configs/mainnet.yaml Outdated Show resolved Hide resolved
Co-Authored-By: Cayman <caymannava@gmail.com>
@@ -878,61 +855,37 @@ def get_seed(state: BeaconState, epoch: Epoch, domain_type: DomainType) -> Hash:
return hash(domain_type + int_to_bytes(epoch, length=8) + mix)
```

#### `get_committee_count`
#### `get_committee_count_at_slot`

This comment has been minimized.

Copy link
@JustinDrake

JustinDrake Oct 20, 2019

Collaborator

This function is not in the ToC

This comment has been minimized.

Copy link
@djrtwo

djrtwo Oct 20, 2019

Author Contributor

ah, thanks. need to re-run ToC generator

@@ -69,7 +68,7 @@
- [`compute_shuffled_index`](#compute_shuffled_index)
- [`compute_proposer_index`](#compute_proposer_index)
- [`compute_committee`](#compute_committee)
- [`compute_epoch_of_slot`](#compute_epoch_of_slot)
- [`compute_epoch_at_slot`](#compute_epoch_at_slot)
- [`compute_start_slot_of_epoch`](#compute_start_slot_of_epoch)

This comment has been minimized.

Copy link
@JustinDrake

JustinDrake Oct 20, 2019

Collaborator

should be compute_start_slot_at_epoch for consistency with compute_epoch_at_slot, get_block_root_at_slot and get_committee_count_at_slot.

@terencechain

This comment has been minimized.

Copy link
Contributor

terencechain commented Oct 22, 2019

CustodyChunkChallenge still uses crosslink from attestation. I'm guessing It'll be a separate effort to revamp the custody game spec?

@djrtwo

This comment has been minimized.

Copy link
Contributor Author

djrtwo commented Oct 22, 2019

@terencechain, yes. I've currently disabled phase 1 CI as most of it needs to be revamped. Prioritizing getting the phase 0 things out and then we'll circle back to get the phase 1 spec in place

@terencechain

This comment has been minimized.

Copy link
Contributor

terencechain commented Oct 22, 2019

@terencechain, yes. I've currently disabled phase 1 CI as most of it needs to be revamped. Prioritizing getting the phase 0 things out and then we'll circle back to get the phase 1 spec in place

Great. Thank you

djrtwo and others added 3 commits Oct 23, 2019
…to phase0-simplify
@tersec tersec referenced this pull request Oct 24, 2019
@djrtwo djrtwo merged commit 364781b into dev Oct 28, 2019
6 checks passed
6 checks passed
ci/circleci: checkout_specs Your tests passed on CircleCI!
Details
ci/circleci: deposit_contract Your tests passed on CircleCI!
Details
ci/circleci: install_deposit_contract_test Your tests passed on CircleCI!
Details
ci/circleci: install_pyspec_test Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@djrtwo djrtwo deleted the phase0-simplify branch Oct 28, 2019
@terencechain terencechain referenced this pull request Oct 28, 2019
19 of 21 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.