Add max epoch activation churn limit (EIP-7514) to Deneb #3499
Conversation
dapplion
force-pushed
the
limit-churn-inbound
branch
from
89fd4d0 to
f691ed6
Compare
dapplion
force-pushed
the
limit-churn-inbound
branch
from
f691ed6 to
e6f7c99
Compare
hwwhww
force-pushed
the
limit-churn-inbound
branch
from
ade1e1b to
fd37ffc
Compare
|
|
||
| | Name | Value | | ||
| | - | - | | ||
| | `MAX_PER_EPOCH_INBOUND_CHURN_LIMIT` | `uint64(12)` (= 12) | |
There was a problem hiding this comment.
would like to examine lower values here
There was a problem hiding this comment.
I projected different levels of ETH staked for each constant and activation time. ethereum/EIPs#7668
What other reasoning do you propose to inform the decision for a specific lower value? Lower values will increase the effectiveness of this feature, but may raise more fairness concerns. I haven't found a strong framework to inform the decision of a specific value
dapplion
force-pushed
the
limit-churn-inbound
branch
from
9482cb9 to
298a630
Compare
|
I like this change a lot more, I think exits should continue to scale with the validator set size. If we choose not to go with this for whatever reason, I would be in favor of still implementing this in the next fork. These are some charts showing the rate of new deposits per epoch over different time spans. I think 8 is actually a good number based on these. I think it's pretty likely the activation queue empties by the time deneb is deployed and at that point the most immediate downside of a decrease in churn limit (increased queue times) isn't felt.
|
dapplion
changed the title
|
No blockers getting this done for Deneb from nimbus' side |
hwwhww
changed the title
tests/core/pyspec/eth2spec/test/deneb/epoch_processing/test_process_registry_updates.py
Outdated
Show resolved
Hide resolved
Co-authored-by: danny <dannyjryan@gmail.com>
|
|
||
| | Name | Value | | ||
| | - | - | | ||
| | `MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT` | `uint64(2**3)` (= 8) | |
There was a problem hiding this comment.
just a nit: since this new param is only applied from Deneb, should it be called DENEB_MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT ?
There might be confusion in testnets with a Bellatrix genesis and a MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT set to 1 and you may think that it is immediately applied but you actually have to wait until Deneb
There was a problem hiding this comment.
Hmm, but it would break the current code conventions for the new constants/presets/configurations. 🤔
There was a problem hiding this comment.
🤔 ... It actually should be MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT_DENEB, similar to
MAX_REQUEST_BLOCKS_DENEB, even if the phase0 counterpart MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT won't exist.
There was a problem hiding this comment.
I see your point, but we do have MAX_REQUEST_BLOCKS in phase0! Adding a suffix to a new preset breaks the current code conventions.
|
Merging the PR now for cooking the release. |
Cap activations per epoch according to EIP-7514: - https://eips.ethereum.org/EIPS/eip-7514 - ethereum/consensus-specs#3499
* implement EIP-7514 for Deneb: Add Max Epoch Churn Limit Cap activations per epoch according to EIP-7514: - https://eips.ethereum.org/EIPS/eip-7514 - ethereum/consensus-specs#3499 * apply proposer boost to first block in case of equivocation Implement spec changes to fork choice; this only affects equivocation when multiple blocks are signed for the same slot. Regular operation is not changed. - ethereum/consensus-specs#3352 * bump test vectors to v1.4.0-beta.2-hotfix --------- Co-authored-by: tersec <tersec@users.noreply.github.com>
Alternative to #3448 applying a max only to inbound churn. Only requires to modify a single epoch transition function, so implementation complexity is low.
Note that having different inbound / outbound churn is not a well studied configuration AFAIK. However, inbound and outbound churn are independent problems, as long as both are bounded they don't need to be equal.
TODOs