-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[WIP] New shard proposal #1427
Conversation
There was a problem hiding this 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
There was a problem hiding this 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. 👀
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
```python | ||
class ShardState(Container): | ||
slot: Slot | ||
gasprice: Gwei |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
# Type 2: delayed attestations | ||
else: | ||
assert state.slot - slot_to_epoch(data.slot) < EPOCH_LENGTH | ||
assert data.shard_transition_root == Hash() |
There was a problem hiding this comment.
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 emptycustody_bits
andshard_transition_root
, and broadcast it? - It seems the proposer can help transform ReducedAttestation in the previous version?
There was a problem hiding this comment.
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.
There was a problem hiding this 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
### Epoch transition | ||
|
||
```python | ||
def phase_1_epoch_transition(state: BeaconState) -> None: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Hsiao-Wei Wang <[email protected]>
Co-Authored-By: Danny Ryan <[email protected]>
There was a problem hiding this 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]) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 | | |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels safer
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]: |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
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). Opened a PR here: #1483 |
Closing in favor of #1483 |
Still very incomplete and being worked on.
Known TODOs: