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

[WIP] New shard proposal #1427

Closed
wants to merge 47 commits into from
Closed

[WIP] New shard proposal #1427

wants to merge 47 commits into from

Conversation

vbuterin
Copy link
Contributor

@vbuterin vbuterin commented Oct 12, 2019

Still very incomplete and being worked on.

Known TODOs:

  • Flesh out fraud proofs into a complete specification

@vbuterin vbuterin changed the title Added new shards [WIP] New shard proposal Oct 12, 2019
@hwwhww hwwhww added the phase1 label Oct 12, 2019
@ethereum ethereum deleted a comment from Good2no1 Oct 15, 2019
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 start! We'll need a little bit of back and forth on cleaning up and figuring out how to best integrate this as an executable extension to Phase 0.

Take a look at the more question focused items at least.
From there I can do a round of cleanup

specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

👍👍
First look before going to bed. 👀

specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
vbuterin and others added 2 commits November 5, 2019 10:31
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
vbuterin and others added 8 commits November 5, 2019 12:15
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
```python
class ShardState(Container):
slot: Slot
gasprice: Gwei
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to set initial gasprice value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think no need. It's zero by default, but then after one slot get_updated_gasprice pushes it up to MIN_GASPRICE.

specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Show resolved Hide resolved
# Type 2: delayed attestations
else:
assert state.slot - slot_to_epoch(data.slot) < EPOCH_LENGTH
assert data.shard_transition_root == Hash()
Copy link
Contributor

@hwwhww hwwhww Nov 6, 2019

Choose a reason for hiding this comment

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

  • Is it a kind of reduced form of attestation data?
  • If so, does that mean the late attesters have to rebuild AttestationData of the old slot with empty custody_bits and shard_transition_root, and broadcast it?
  • It seems the proposer can help transform ReducedAttestation in the previous version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a reduced form, and late attesters do have to rebroadcast.

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.

I think the approach here is reasonable. I'm on board from a high level perspective. We'll need many cleanups and testing to get where we're ultimately going.

I can take a pass on getting the linter happy and a couple of sanity tests running after you address @hwwhww's comments

specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
specs/core/1_new_shards.md Outdated Show resolved Hide resolved
### Epoch transition

```python
def phase_1_epoch_transition(state: BeaconState) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

These are additional functions to the existing epoch transition?

Copy link
Contributor

Choose a reason for hiding this comment

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

We really need to figure out the build process.
This spec currently has 3 different approaches to modifying phase 0

  • function/container replacement and modifications
  • field addition to containers
  • additional functions for epoch/block processing

vbuterin and others added 6 commits November 6, 2019 15:53
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
Copy link
Contributor

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Nice work! Implemented all the helpers in Prysm and here's feedback on those

```python
def get_online_indices(state: BeaconState) -> Set[ValidatorIndex]:
active_validators = get_active_validator_indices(state, get_current_epoch(state))
return set([i for i in active_validators if state.online_countdown[i] != 0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is set really necessary? Trying to optimize from client implementation's perspective

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not, but when things are unordered and unique we declare it as Set in the spec. Here it would be a free conversion for a client to a set object anyway (or any validator indices container).

| `SHARD_BLOCK_OFFSETS` | `[1, 2, 3, 5, 8, 13, 21, 34, 55, 89, 144, 233]` | |
| `MAX_SHARD_BLOCKS_PER_ATTESTATION` | `len(SHARD_BLOCK_OFFSETS)` | |
| `EMPTY_CHUNK_ROOT` | `hash_tree_root(BytesN[SHARD_BLOCK_CHUNK_SIZE]())` | |
| `MAX_GASPRICE` | `2**14` (= 16,384) | Gwei | |
Copy link
Contributor

Choose a reason for hiding this comment

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

gasprice or gas_price? not sure which is more semantic 🤔

"""
Given a state and a list of validator indices, outputs the CompactCommittee representing them.
"""
validators = [state.validators[i] for i in committee]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can simplify to one loop without sacrificing much readability. Example:

	compactValidators := make([]uint64, len(committee))
	pubKeys := make([][]byte, len(committee))

	for i := 0; i < len(committee); i++ {
		v := state.Validators[committee[i]]
		compactValidators[i] = PackCompactValidator(committee[i], v.Slashed, v.EffectiveBalance/params.BeaconConfig().EffectiveBalanceIncrement)
		pubKeys[i] = v.PublicKey
	}

```python
def get_shard_committee(beacon_state: BeaconState, epoch: Epoch, shard: Shard) -> Sequence[ValidatorIndex]:
source_epoch = epoch - epoch % SHARD_COMMITTEE_PERIOD
if source_epoch > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels safer

Suggested change
if source_epoch > 0:
if source_epoch >= SHARD_COMMITTEE_PERIOD:

### `get_shard_committee`

```python
def get_shard_committee(beacon_state: BeaconState, epoch: Epoch, shard: Shard) -> Sequence[ValidatorIndex]:
Copy link
Contributor

Choose a reason for hiding this comment

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

shard: Shard is not used


```python
def get_shard_proposer_index(beacon_state: BeaconState, slot: Slot, shard: Shard) -> ValidatorIndex:
committee = get_shard_committee(beacon_state, slot_to_epoch(slot), shard)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
committee = get_shard_committee(beacon_state, slot_to_epoch(slot), shard)
committee = get_shard_committee(beacon_state, compute_epoch_at_slot(slot), shard)


```python
def get_shard(state: BeaconState, attestation: Attestation) -> Shard:
return Shard((attestation.data.index + get_start_shard(state, data.slot)) % ACTIVE_SHARDS)
Copy link
Contributor

Choose a reason for hiding this comment

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

get_start_shard is no longer defined in beacon chain spec, I think we have to reintroduce it here


```python
def get_shard(state: BeaconState, attestation: Attestation) -> Shard:
return Shard((attestation.data.index + get_start_shard(state, data.slot)) % ACTIVE_SHARDS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return Shard((attestation.data.index + get_start_shard(state, data.slot)) % ACTIVE_SHARDS)
return Shard((attestation.data.index + get_start_shard(state, attestation.data.slot)) % ACTIVE_SHARDS)

### `AttestationData`

```python
class AttestationData(Container):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to add shard to the field? can avoid get_shard

Copy link
Contributor

Choose a reason for hiding this comment

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

Would still need to validate the shard against the committee index for the slot, but I see value in including it here explicitly (similar to including slot even though it used to be able to be calculated from the constituent parts)

@protolambda
Copy link
Collaborator

Took the latest work and started on the necessary changes to make it run as part of the pyspec, and just organize things (start with custody spec cleanup, integrate with the rest of the spec).
Also rebased on the latest dev, with v0.9.1 changes too. And then some minor misc fixes in this code (blockbody vs block, loop indentation, etc.).

Opened a PR here: #1483
I'll try to merge any continued commits here, but it would be great if we can coordinate to continue phase 1 changes on a fork of this new PR.

@djrtwo
Copy link
Contributor

djrtwo commented Nov 20, 2019

Closing in favor of #1483

@djrtwo djrtwo closed this Nov 20, 2019
@djrtwo djrtwo mentioned this pull request Dec 5, 2019
7 tasks
@djrtwo djrtwo deleted the vitalik71 branch May 20, 2020 23:23
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.

6 participants