-
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
ATTESTATION_SUBNET_COUNT should be dynamic #1777
Comments
Also we need |
Thanks @jrhea! I had similar thoughts as we were discussing the attnets debugging work you've been doing. I think that predicting the validator set size in some future epoch and accounting for how many committees will be is not worth the added complexity here (especially re: @zilm13's comment about the warm-up). Another couple of notes:
The biggest arguments might lie in being able to find peers more easily in the dht but this might not be a realistic approximation of what comes in phase 1. |
I don't see his comment as really being an issue. The warmup is accounted for in the
My understanding was that we would be using topic discovery in phase 1.
I believe this can be addressed, but it is part of a solution to another concern i have.
Again, i thought we plan on implementing topic discovery for phase 1. if not, i see your point. If we are, then i would consider this argument less compelling. |
I forgot to comment on this. Ya, I figured you might say that and you would be a better judge of the ramifications of this. Your call boss |
This would make sense to use the first option def get_attestation_subnet_count_at_slot(state: BeaconState, slot: Slot) -> uint64:
return min(get_committee_count_at_slot(state,slot), 64) Using the current active validator set seems like a good gauge as any for determining the amount of subnets. Although there might be a possibility of newly added validators biasing the subnets through this period
While that maybe true, the initial genesis validator set would only have 4 committees. Having validators subscribe to the other 60 empty subnets seems a bit pointless. You would have 94% of your validators not participating in a functioning subnet. |
Which is totally fine. If we move to phase 1 with similar number of validators (this is feasible though unlikely), all persistent subnets would then be used, but they would also be the same size as those today. The size of the persistent subnet approximates the phase 1 behaviour more than all of the validators being assigned to subnets that are actually used.
But this does not adequately capture the moment in which the subnet count would jump up and as @zilm13 noted, the subnet would be under-provisioned in number of validators. |
That's why I suggested adding a calculation that gives the worst case possible number of new committees. def get_attestation_subnet_count_at_slot(state: BeaconState, slot: Slot) -> uint64:
max_additional_committees = MIN_PER_EPOCH_CHURN_LIMIT*PERSISTENT_COMMITTEE_PERIOD//SLOTS_PER_EPOCH//TARGET_COMMITTEE_SIZE
return min(get_committee_count_at_slot(state,slot) + max_additional_committees, 64) // COMMITTEES_PER_SUBNET Granted that We change:
to
What am i missing? |
@djrtwo if I remember correctly, one of the possibilities we discussed in the networking call was reverting back to using all 64 committees so that our network is more similar to how it will be in phase 1. Is that still on the table? |
working on that now |
fix merged! thanks @jrhea ! |
ATTESTATION_SUBNET_COUNT
is currently a spec constant that defines the valid range of subnets persistent committees should randomly subscribe to (this value is currently set to 64). When the network has a smaller number of validators, this results in nodes joining subnets with committee counts of 0. Spreading persistent committee members across unused subnets could significantly inhibit our ability to find subnets in a timely fashion.The following table compares active validators to resulting committee counts:
In other words, we don't need 64 attestation subnets until we reach 262,144 active validators.
To fix this, I suggest that we replace
ATTESTATION_SUBNET_COUNT
with a method that calculates it based on the committee count. Consider something like this to start:that seems fine if we assume a static validator set, but we really should consider how many validators could exit or activate within a
PERSISTENT_COMMITTEE_PERIOD
to ensure we create enough attestation subnets. To do that, we should take a look at worst case validator churn limits. From the spec,get_validator_churn_limit()
is defined as follows:In order to have a churn limit > 4 , this would have to be true:
len(active_validator_indices) > 4 * CHURN_LIMIT_QUOTIENT
In other words, we would need 262,144 active validators. From the table above, we know this is the exact amount needed to reach a committee count of 64. This means that we can calculate the max increase/decrease in committee counts within a
PERSISTEN_COMMITTEE_PERIOD
as follows:It's probably safe to only consider the max increase in committee count (worst, worst case we have 2 additional attestation subnets). As a result, we can modify our new method
get_attestation_subnet_count_at_slot()
from above to account for the worst case scenario where 2 additional committees could be added within aPERSISTENT_COMMITTEE_PERIOD
:If we want to retain the ability to control the committee to attestation subnets ratio, we could add a configurable spec constant called
COMMITTEES_PER_SUBNET
that can be set to 1 andget_attestation_subnet_count_at_slot
could be modified as follows:Credit/blame to @mkalinin for talking this out with me and @djrtwo for pointing out that low validator counts could result in persistent committee members joining unused subnets.
The text was updated successfully, but these errors were encountered: