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

Switch the rest of the spec to MAX_EFFECTIVE_BALANCE_ELECTRA #3783

Merged
merged 6 commits into from
Jun 14, 2024

Conversation

mkalinin
Copy link
Collaborator

This PR makes the rest of the places of the spec to use the new MAX_EFFECTIVE_BALANCE_ELECTRA preset instead of the old one, namely:

  • initialize_beacon_state_from_eth1
  • get_next_sync_committee_indices

One exception is compute_weak_subjectivity_period function which implements the strategy outlined in the WS research paper thus doesn’t need to be updated.

This change entails the following updates into the spec tests:

  • Use MAX_EFFECTIVE_BALANCE_ELECTRA in the build_mock_validator for post-Electra forks
  • Use MAX_EFFECTIVE_BALANCE_ELECTRA in test_initialize_beacon_state_some_small_balances when post-Electra
  • Initialize validators with MAX_EFFECTIVE_BALANCE_ELECTRA balance for sync committee tests post-Electra affected by the change to get_next_sync_committee_indices

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

good catch @mkalinin ! 🙏

specs/electra/beacon-chain.md Show resolved Hide resolved
Comment on lines +198 to +199
if not is_post_electra(spec):
return misc_balances(spec)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible that, instead of adding new misc_balances_electra, we modify misc_balances by adding new if is_post_electra(spec) logic?

Comment on lines +117 to +118
if not is_post_electra(spec):
return default_balances(spec)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto. Is it possible that instead of adding new default_balances_electra, we modify default_balances by adding new if is_post_electra(spec) logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about this approach but then decided to keep default balances as is because it makes a test run with 32 ETH balance validators which is also important for Electra. Ideally, we should run tests with MIN_ACTIVATION_BALANCE, MAX_EFFECTIVE_BALANCE_ELECTRA and something in between. It is probably better to handle this modification separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably more just the naming problem; when I read default_balances_electra, I thought it is just "the default balances post-electra," but it is actually compatible with pre-electra. But I understand what you wanted to present.

Copy link
Contributor

Choose a reason for hiding this comment

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

what about calling it max_balances instead? To me it explains pretty clearly what it does

Co-authored-by: Hsiao-Wei Wang <[email protected]>
@hwwhww hwwhww mentioned this pull request Jun 4, 2024
10 tasks
Comment on lines +117 to +118
if not is_post_electra(spec):
return default_balances(spec)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably more just the naming problem; when I read default_balances_electra, I thought it is just "the default balances post-electra," but it is actually compatible with pre-electra. But I understand what you wanted to present.

Co-authored-by: Hsiao-Wei Wang <[email protected]>
@hwwhww
Copy link
Contributor

hwwhww commented Jun 14, 2024

I fixed the broken test (aa65fd7). Ideally I should add a new test for Electra initialize_beacon_state_from_eth1, but since it's supposed to be removed (#3663), I didn't add that test case here.

Copy link
Contributor

@fradamt fradamt left a comment

Choose a reason for hiding this comment

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

Generally lgtm

validator = spec.Validator(
pubkey=active_pubkey,
withdrawal_credentials=withdrawal_credentials,
activation_eligibility_epoch=spec.FAR_FUTURE_EPOCH,
activation_epoch=spec.FAR_FUTURE_EPOCH,
exit_epoch=spec.FAR_FUTURE_EPOCH,
withdrawable_epoch=spec.FAR_FUTURE_EPOCH,
effective_balance=min(balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT, spec.MAX_EFFECTIVE_BALANCE)
effective_balance=min(balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT, max_effective_balace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
effective_balance=min(balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT, max_effective_balace)
effective_balance=min(balance - balance % spec.EFFECTIVE_BALANCE_INCREMENT, max_effective_balance)

else:
# insecurely use pubkey as withdrawal key as well
withdrawal_credentials = spec.BLS_WITHDRAWAL_PREFIX + spec.hash(withdrawal_pubkey)[1:]
max_effective_balace = spec.MAX_EFFECTIVE_BALANCE
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
max_effective_balace = spec.MAX_EFFECTIVE_BALANCE
max_effective_balance = spec.MAX_EFFECTIVE_BALANCE

else:
# insecurely use pubkey as withdrawal key as well
withdrawal_credentials = spec.BLS_WITHDRAWAL_PREFIX + spec.hash(withdrawal_pubkey)[1:]
max_effective_balace = spec.MAX_EFFECTIVE_BALANCE_ELECTRA
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
max_effective_balace = spec.MAX_EFFECTIVE_BALANCE_ELECTRA
max_effective_balance = spec.MAX_EFFECTIVE_BALANCE_ELECTRA

Copy link
Contributor

Choose a reason for hiding this comment

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

ooops. will fix it after the release.

Thanks for the review!

@hwwhww hwwhww merged commit ca64850 into ethereum:dev Jun 14, 2024
26 checks passed
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

lgtm! +1 to hww's comments, but it looks like something was sorted

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

Successfully merging this pull request may close these issues.

5 participants