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

Rename SHARD_COMMITTEE_PERIOD #3942

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ppopth
Copy link
Member

@ppopth ppopth commented Sep 25, 2024

This PR renames SHARD_COMMITTEE_PERIOD to MIN_VALIDATOR_ACTIVE_PERIOD. Since we don't have shard committees anymore, naming things after shard committees doesn't make sense. This PR renames it to MIN_VALIDATOR_ACTIVE_PERIOD so that it means the minimum period that the validator has to be active before exiting.

This commit renames SHARD_COMMITTEE_PERIOD to MIN_VALIDATOR_ACTIVE_PERIOD.
Since we don't have shard committees anymore, naming things after shard
committees doesn't make sense. This commit renames it to
MIN_VALIDATOR_ACTIVE_PERIOD so that it means the minimum period that the
validator has to be active before exiting.
@hwwhww
Copy link
Contributor

hwwhww commented Oct 1, 2024

It's hard to change the phase0 configuration now without solid reasons. 😅 I'd leave this PR open for a while.

/cc @dankrad on thoughts of naming for FullDAS.

@hwwhww hwwhww added the general:presentation Presentation (as opposed to content) label Oct 1, 2024
@@ -72,7 +72,7 @@ SECONDS_PER_ETH1_BLOCK: 14
# 2**8 (= 256) epochs ~27 hours
MIN_VALIDATOR_WITHDRAWABILITY_DELAY: 256
# 2**8 (= 256) epochs ~27 hours
SHARD_COMMITTEE_PERIOD: 256
MIN_VALIDATOR_ACTIVE_PERIOD: 256
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a breaking change and can mess up clients that expect this key to exist in configs. If we merge this, client binaries will have to either:

  • Update their expect config name, work with spec tests, break with existing user configs
  • Not update, break with spec tests and work with existing user configs

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's not do it then.

@Scutua

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:presentation Presentation (as opposed to content)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants