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

phase 1 validator guide (round 1) #1704

Merged
merged 24 commits into from
Jun 17, 2020
Merged

phase 1 validator guide (round 1) #1704

merged 24 commits into from
Jun 17, 2020

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Apr 2, 2020

Add phase 1 validator guide.

Add the following checked items. Leave the remaining for a subsequent PR(s)

  • Block proposal
  • Attesting
    • Join shard block subnet in addition to attestation subnet
    • On-time attestation
    • Late attestation (no custody_bits_blocks, stub shard_transition, when to broadcast, when to aggregate)
    • [Think about] Split shard_transition from attestation_data in p2p for deduplication gains (OR split attestation_data from attestation...)
  • Light client committee
    • clean up beacon chain spec and add validator duties
  • Shard duties
    • Persistent committee subnets and lookahead (replacement for random subnets)
    • Shard block creation and broadcast
  • Additional slashing avoidance

@djrtwo djrtwo force-pushed the phase1-validator branch from e90d4e3 to d789f3d Compare April 2, 2020 21:10
@djrtwo djrtwo changed the base branch from phase1-tests to dev April 3, 2020 16:47
specs/phase1/validator.md Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
@djrtwo djrtwo force-pushed the phase1-validator branch from e4d6e46 to b1ff00a Compare June 3, 2020 18:18

##### Head shard root

Set `attestation_data.head_shard_root = hash_tree_root(head_shard_block)`.

##### Shard transition

Set `shard_transition` to the value returned by `get_shard_transition()`.
Set `shard_transition` to the value returned by `get_shard_transition(head_state, shard, shard_blocks)`.
Copy link
Contributor Author

@djrtwo djrtwo Jun 3, 2020

Choose a reason for hiding this comment

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

@hwwhww I moved get_shard_transition from shard-transition.md to the validator guide (I think the appropriate place).

I also greatly reduced the complexity of what is going on here. I think previously, much of the helper functions were implementing some weird version of the fork choice. Now, we assume that shard_blocks is the already validated sequence of blocks received from the fork choice leading to the current head of the shard chain.

One important thing is that I am currently assuming that the fork choice is only going to give me heads of chains that fit the expected offset_slots. The forkchoice doesn't currently provide this safety mechanism, but I think that it probably should.

Can you take a look at this attestation data section (mainly the mods I made to get_shard_transition) and the proposed addition of offset_slots being enforced by fork choice? Happy to hop on a quick call to help you go over this and discuss in more depth

Copy link
Contributor

Choose a reason for hiding this comment

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

Good points!

Added changes at #1773

  1. Added rules to enforce the shard block is at offset slots 49cbc7c
  2. Added get_pending_shard_blocks helper that returns the shard_blocks at offset slots till head beacon slot: 465f230 returns correct data.

specs/phase1/validator.md Outdated Show resolved Hide resolved
@hwwhww hwwhww mentioned this pull request Jun 4, 2020
4 tasks
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.

Great work!! ✨

I reviewed the shard transition part. will review other parts later.

specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved

##### Head shard root

Set `attestation_data.head_shard_root = hash_tree_root(head_shard_block)`.

##### Shard transition

Set `shard_transition` to the value returned by `get_shard_transition()`.
Set `shard_transition` to the value returned by `get_shard_transition(head_state, shard, shard_blocks)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good points!

Added changes at #1773

  1. Added rules to enforce the shard block is at offset slots 49cbc7c
  2. Added get_pending_shard_blocks helper that returns the shard_blocks at offset slots till head beacon slot: 465f230 returns correct data.

specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
@djrtwo djrtwo force-pushed the phase1-validator branch from 6a69e0e to 8a9ccc4 Compare June 5, 2020 18:12
@hwwhww
Copy link
Contributor

hwwhww commented Jun 5, 2020

Followup note of #1704 (comment):

These are some cases I see for attesting shard_block_100 and shard_block_101:
image

If head_state is transitioned to the slot of current time, Slot(state.slot + 1) works for these cases. Any other case I missed?

EDIT: added more lines

hwwhww added a commit that referenced this pull request Jun 5, 2020
@djrtwo djrtwo force-pushed the phase1-validator branch from c915fb6 to 7f680df Compare June 5, 2020 20:09
@terencechain
Copy link
Contributor

Followup note of #1704 (comment):

These are some cases I see for attesting shard_block_100 and shard_block_101:
If head_state is transitioned to the slot of current time, Slot(state.slot + 1) works for these cases. Any other case I missed?
Screen Shot 2020-06-05 at 5 50 53 PM

Do you consider 4a and 4b as 2 different cases? If yes, does it break current transition?

4a: B100 crosslinks S99
4b: B100 did not crosslinks S99, B101 crosslinks S99.

@hwwhww
Copy link
Contributor

hwwhww commented Jun 8, 2020

@terencechain

Thank! It looks like an extended case!
I wanted to use "B100 crosslinks S99" to represent "the latest crosslink at the beginning". Rewrite it to:

chain_06_fail_crosslink_and_skipped_shard

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.

Another round :)

specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Show resolved Hide resolved
specs/phase1/validator.md Outdated Show resolved Hide resolved
specs/phase1/validator.md Show resolved Hide resolved
specs/phase1/validator.md Show resolved Hide resolved
@djrtwo djrtwo changed the title [wip] phase 1 validator guide phase 1 validator guide (round 1) Jun 15, 2020
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.

Updated the domain types table a bit.

LGTM except for an is_in_next_light_client_committee question.
Good job! 💯

specs/phase1/validator.md Outdated Show resolved Hide resolved
tests/core/pyspec/eth2spec/test/helpers/shard_block.py Outdated Show resolved Hide resolved
Comment on lines 421 to 423
current_source_epoch = compute_committee_source_epoch(get_current_epoch(state), LIGHT_CLIENT_COMMITTEE_PERIOD)
next_source_epoch = current_source_epoch + LIGHT_CLIENT_COMMITTEE_PERIOD
next_committee = get_light_client_committee(state, next_source_epoch)
Copy link
Contributor

Choose a reason for hiding this comment

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

The input epoch of get_light_client_committee should be the epoch that the light clients have to vote for, not the source epoch.

I suppose it should be:

Suggested change
current_source_epoch = compute_committee_source_epoch(get_current_epoch(state), LIGHT_CLIENT_COMMITTEE_PERIOD)
next_source_epoch = current_source_epoch + LIGHT_CLIENT_COMMITTEE_PERIOD
next_committee = get_light_client_committee(state, next_source_epoch)
next_committee = get_light_client_committee(state,get_current_epoch(state) + LIGHT_CLIENT_COMMITTEE_PERIOD)

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'd actually argue that the above logic is the same, and I considered writing it as you had originally but wasn't sure if the logic was clear.

Making the change regardless. (curious if I am incorrect in saying that you'll get the same result either way)

Copy link
Contributor

@hwwhww hwwhww Jun 17, 2020

Choose a reason for hiding this comment

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

@djrtwo
No, they are not the same result. I argued the function logic correctness:

  • get_light_client_committee itself will compute the correct source epoch internally, so we don't have to pass the source epoch to it.
  • The epoch we pass to get_light_client_committee should have been the epoch that the light client is going to vote for. In this case, setting it to get_current_epoch(state) + LIGHT_CLIENT_COMMITTEE_PERIOD can make it forward to the next period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But getting the current_source_epoch and adding LIGHT_CLIENT_COMMITTEE_PERIOD to it will put you certainly in the next period. Just as get_current_epoch + LIGHT_CLIENT_COMMITTEE_PERIOD will also put you in the next period

@djrtwo djrtwo force-pushed the phase1-validator branch from 20ee12b to 401a37f Compare June 16, 2020 22:27
Remove `ShardState.transition_digest`
@djrtwo djrtwo force-pushed the phase1-validator branch from 401a37f to a21f936 Compare June 16, 2020 22:41
@djrtwo djrtwo force-pushed the phase1-validator branch from f6364d4 to fbf10a0 Compare June 17, 2020 04:32
@djrtwo djrtwo merged commit 8e30ee5 into dev Jun 17, 2020
@djrtwo djrtwo deleted the phase1-validator branch June 17, 2020 04:41
@hwwhww
Copy link
Contributor

hwwhww commented Jun 17, 2020

But getting the current_source_epoch and adding LIGHT_CLIENT_COMMITTEE_PERIOD to it will put you certainly in the next period. Just as get_current_epoch + LIGHT_CLIENT_COMMITTEE_PERIOD will also put you in the next period

No, because compute_committee_source_epoch(epoch, period) has a lookahead inside and get_light_client_committee(beacon_state, epoch) already calls compute_committee_source_epoch(epoch, LIGHT_CLIENT_COMMITTEE_PERIOD) internally. So current_source_epoch + LIGHT_CLIENT_COMMITTEE_PERIOD is the next_source_epoch point at the diagram below, then get_light_client_committee(next_source_epoch) would output the committee of period 2. I believe we want to get the committee of period 3, right?

image

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.

3 participants