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

[for discussion] Use all attestation subnets in phase 0 #1804

Merged
merged 3 commits into from
May 14, 2020
Merged

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented May 11, 2020

An attempt to address this.

In Phase 1, all attestation subnets would get used even when committees_per_slot does not equal SHARD_COUNT. In this PR, all attestation subnets are now used by rotating through them all throughout the epoch instead of just using a subset at each slot starting at 0.

This does mimic the behaviour of Phase 1 more directly, but introduces a change to broadcast logic kind of late in the game.

@djrtwo djrtwo requested a review from protolambda May 11, 2020 18:53
@djrtwo
Copy link
Contributor Author

djrtwo commented May 11, 2020

@AgeManning
Copy link
Contributor

This looks fine to us.

@zilm13
Copy link
Contributor

zilm13 commented May 13, 2020

@djrtwo it looks like minor change for subnetwork calculation formula when you choose one to post your attestation, but @jrhea proposal was to decrease number of subnetworks in order to increase size of each subnetwork skeleton. In this one we still have small skeletons in small networks. If we kinda combine it or smth like this, it could serve similarly to #1777, but it would look more complicated than original PR for me.

@jrhea
Copy link
Contributor

jrhea commented May 13, 2020

I just want to make sure I understand what is happening here. Assuming,

  • first slot in a new epoch
  • a constant get_committee_count_at_slot() == 4

the possible subnets for each slot would be:

  • slot 0: [0,3]
  • slot 1: [4,7]
  • etc

Is that right? If so, I have a couple of comments/questions:

  1. isn't get_committee_count_at_slot() always going to be constant for each slot in an epoch? if so, seems like compute_subnet_for_attestation() we could avoid the for loop.

  2. is the point here to mimic the scenario in phase 1 where we don't have enough validators to form crosslinks every slot for each shard?

@djrtwo
Copy link
Contributor Author

djrtwo commented May 14, 2020

@jrhea proposal was to decrease number of subnetworks in order to increase size of each subnetwork skeleton

Yes, I'm aware. We discussed this on the networking call. The desire, if we change it, is to make it more like the distribution we would see with actual shards. I am worried about artificially increasing the size of subnets in phase 0 when in actuality, come phase 1, they are used more like in the PR

@djrtwo
Copy link
Contributor Author

djrtwo commented May 14, 2020

isn't get_committee_count_at_slot() always going to be constant for each slot in an epoch? if so, seems like compute_subnet_for_attestation() we could avoid the for loop.

you're right, the for loop is unnecessary here due to # of committees being equal across all slots in an epoch

is the point here to mimic the scenario in phase 1 where we don't have enough validators to form crosslinks every slot for each shard?

Yes, in low validation numbers, you rotate through shards across slots when you don't have high enough committee numbers. When you get to enough validators to have 64 committees per slot, you just get usage of all subnets at all slots (same as you'd see in the current phase 0 setup)

Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

LGTM. Still have slight doubts about subnet rotation between epochs, but not too critical.

Co-authored-by: Diederik Loerakker <proto@protolambda.com>
@djrtwo djrtwo merged commit 96ab76d into v012x May 14, 2020
@mkalinin
Copy link
Collaborator

The desire, if we change it, is to make it more like the distribution we would see with actual shards. I am worried about artificially increasing the size of subnets in phase 0 when in actuality, come phase 1, they are used more like in the PR

For the record. We might use one subnet for several shards in Phase 1 if it would impact security on the network layer.

@djrtwo djrtwo deleted the use-all-attnets branch May 15, 2020 14:42
@djrtwo
Copy link
Contributor Author

djrtwo commented May 15, 2020

For the record. We might use one subnet for several shards in Phase 1 if it would impact security on the network layer.

True! that's why we have ATTESTATION_SUBNET_COUNT to adjust

@tersec
Copy link
Contributor

tersec commented May 22, 2020

For the record, this looks fine from Nimbus's end as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants