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.

Sign up for GitHub

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

Jump to bottom

Align to consensus release v1.4.0-beta.2 #12908

Closed
wants to merge 12 commits into from
Closed

Align to consensus release v1.4.0-beta.2 #12908

wants to merge 12 commits into from

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Sep 15, 2023 •
edited

@terencechain terencechain added the Blocked Blocked by research or external factors label Sep 15, 2023
@terencechain terencechain requested a review from a team as a code owner September 15, 2023 12:26
@terencechain terencechain self-assigned this Sep 15, 2023
@terencechain terencechain added the Deneb PRs or issues for the Deneb upgrade label Sep 15, 2023
@terencechain terencechain added Ready For Review A pull request ready for code review and removed Blocked Blocked by research or external factors labels Sep 19, 2023
terencechain
terencechain commented Sep 19, 2023
View reviewed changes
testing/endtoend/evaluators/operations.go
@@ -29,8 +29,6 @@ import (
"google.golang.org/protobuf/types/known/emptypb"
)

// churnLimit is normally 4 unless the validator set is extremely large.
var churnLimit = 4
Copy link
Member Author

Choose a reason for hiding this comment

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

I am removing this to use the default config

james-prysm
james-prysm reviewed Sep 20, 2023
View reviewed changes
config/params/mainnet_config.go
@@ -270,6 +270,8 @@ var mainnetBeaconConfig = &BeaconChainConfig{

// Subnet value
BlobsidecarSubnetCount: 6,

MaxPerEpochActivationChurnLimit: 8,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not need the min here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

there's no change here

james-prysm
james-prysm reviewed Sep 20, 2023
View reviewed changes
config/params/testnet_e2e_config.go
@@ -96,6 +96,9 @@ func E2EMainnetTestConfig() *BeaconChainConfig {
e2eConfig.CapellaForkVersion = []byte{3, 0, 0, 254}
e2eConfig.DenebForkVersion = []byte{4, 0, 0, 254}

// Deneb changes.
e2eConfig.MinPerEpochChurnLimit = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not need max here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should just use the default which is 8

james-prysm
james-prysm reviewed Sep 20, 2023
View reviewed changes
beacon-chain/core/epoch/epoch_processing.go Outdated
@@ -146,6 +146,10 @@ func ProcessRegistryUpdates(ctx context.Context, state state.BeaconState) (state
if churnLimit < limit {
limit = churnLimit
}
// Cap churn limit to max per epoch churn limit. New in EIP7514.
if limit > params.BeaconConfig().MaxPerEpochActivationChurnLimit {
Copy link
Contributor

Choose a reason for hiding this comment

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

where is the minimum set for this

Copy link
Member Author

Choose a reason for hiding this comment

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

Minimal is used in other places. It's not part of the change besides the value

rauljordan
rauljordan reviewed Sep 21, 2023
View reviewed changes
testing/endtoend/evaluators/operations.go Outdated
@@ -249,8 +247,8 @@ func activatesDepositedValidators(ec *e2etypes.EvaluationContext, conns ...*grpc
return fmt.Errorf("missing %d validators for post-genesis deposits", len(expected))
}

if deposits != churnLimit {
return fmt.Errorf("expected %d deposits to be processed in epoch %d, received %d", churnLimit, epoch, deposits)
if deposits != int(params.BeaconConfig().MinPerEpochChurnLimit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer to cast deposits to uint

rauljordan
rauljordan previously approved these changes Sep 21, 2023
View reviewed changes
nisdas
nisdas reviewed Sep 22, 2023
View reviewed changes
beacon-chain/core/epoch/epoch_processing.go Outdated
@@ -147,6 +148,13 @@ func ProcessRegistryUpdates(ctx context.Context, state state.BeaconState) (state
limit = churnLimit
}

if slots.ToEpoch(state.Slot()) >= params.BeaconConfig().DenebForkEpoch {
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't the state version be better here ? we would only apply this update on deneb states

@terencechain terencechain force-pushed the update-max-per-epoch-churn-limit branch 2 times, most recently from 63553bf to 41366d8 Compare September 25, 2023 13:28
@terencechain terencechain changed the title Update and use max per epoch churn limit Align to consensus release v1.4.0-beta.2 Sep 25, 2023
@terencechain terencechain changed the title Align to consensus release v1.4.0-beta.2 Align to consensus release v1.4.0-beta.2 Sep 25, 2023
@terencechain
Copy link
Member Author

superseded by #12959

@terencechain terencechain mentioned this pull request Sep 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nisdas nisdas nisdas left review comments

@james-prysm james-prysm james-prysm left review comments

@rauljordan rauljordan rauljordan left review comments

@kasey kasey Awaiting requested review from kasey kasey is a code owner automatically assigned from prysmaticlabs/core-team

Assignees

@terencechain terencechain

Labels
Deneb PRs or issues for the Deneb upgrade Ready For Review A pull request ready for code review >1 Approves Required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@terencechain @rauljordan @nisdas @james-prysm @potuz