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

EIP-6110: Supply validator deposits on chain #13944

Merged
merged 92 commits into from
Jun 29, 2024
Merged

EIP-6110: Supply validator deposits on chain #13944

merged 92 commits into from
Jun 29, 2024

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented May 1, 2024

What type of PR is this?

Feature

What does this PR do? Why is it needed?

implements eip6110 as defined in https://eips.ethereum.org/EIPS/eip-6110

  • process deposits was refactored to be aligned with specs
  • packing deposits on validator client was updated
  • saveValidatorIndices was added on parts of the code where state is finalized

TODO

  • adds spec tests spec tests need to be updated in future PR for devnet1

Which issues(s) does this PR fix?

Other notes for review

related PR: #13943

if uint64(len(body.Deposits())) != maxDeposits {
return nil, fmt.Errorf("incorrect outstanding deposits in block body, wanted: %d, got: %d",
maxDeposits, len(body.Deposits()))
if len(body.Deposits()) != 0 {
Copy link
Contributor Author

@james-prysm james-prysm May 2, 2024

Choose a reason for hiding this comment

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

double check as this is changing existing logic or if I need to handle it in a fork specific way

@james-prysm
Copy link
Contributor Author

don't forget about this,

For the read usage side, nothing needs to be done. For the write usage side, we need to call SaveValidatorIndices() when a new finalized state is emitted and when the node starts up. This part is TBD

@@ -10,6 +10,7 @@ go_library(
"effective_balance_updates.go",
"registry_updates.go",
"transition.go",
"transition_no_verify.sig.go",
Copy link
Member

Choose a reason for hiding this comment

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

This filename doesn't make sense for what you've added. It should be called operations.go, in my opinion.

If you want to keep it with this pattern please use underscores for the name and avoid a .sig.go suffix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another good catch was definitely not intentional...

// for_ops(body.execution_payload.withdrawal_requests, process_execution_layer_withdrawal_request)
// for_ops(body.execution_payload.deposit_requests, process_deposit_requests) # [New in Electra:EIP6110]
// for_ops(body.consolidations, process_consolidation) # [New in Electra:EIP7251]
func Operations(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func Operations(
func ProcessOperations(

This name is more appropriate imo

return nil, errors.Wrap(err, "could not process deposit receipts")
}

// TODO: Process consolidations from execution header.
Copy link
Member

Choose a reason for hiding this comment

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

I'll have this ready for review today 💪

Comment on lines 159 to 195
// AddValidatorToRegistry updates the beacon state with validator information
// def add_validator_to_registry(state: BeaconState,
//
// pubkey: BLSPubkey,
// withdrawal_credentials: Bytes32,
// amount: uint64) -> None:
// index = get_index_for_new_validator(state)
// validator = get_validator_from_deposit(pubkey, withdrawal_credentials)
// set_or_append_list(state.validators, index, validator)
// set_or_append_list(state.balances, index, 0) # [Modified in Electra:EIP7251]
// set_or_append_list(state.previous_epoch_participation, index, ParticipationFlags(0b0000_0000))
// set_or_append_list(state.current_epoch_participation, index, ParticipationFlags(0b0000_0000))
// set_or_append_list(state.inactivity_scores, index, uint64(0))
// state.pending_balance_deposits.append(PendingBalanceDeposit(index=index, amount=amount)) # [New in Electra:EIP7251]
func AddValidatorToRegistry(beaconState state.BeaconState, pubKey []byte, withdrawalCredentials []byte, amount uint64) error {
val := GetValidatorFromDeposit(pubKey, withdrawalCredentials)
if err := beaconState.AppendValidator(val); err != nil {
return err
}
index, ok := beaconState.ValidatorIndexByPubkey(bytesutil.ToBytes48(pubKey))
if !ok {
return errors.New("could not find validator in registry")
}
if err := beaconState.AppendBalance(0); err != nil {
return err
}
if err := beaconState.AppendPendingBalanceDeposit(index, amount); err != nil {
return err
}
if err := beaconState.AppendInactivityScore(0); err != nil {
return err
}
if err := beaconState.AppendPreviousParticipationBits(0); err != nil {
return err
}
return beaconState.AppendCurrentParticipationBits(0)
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this go in the validator file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move it, i think it's only used in deposits though

// effective_balance=0, # [Modified in Electra:EIP7251]
//
// )
func GetValidatorFromDeposit(pubKey []byte, withdrawalCredentials []byte) *ethpb.Validator {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func GetValidatorFromDeposit(pubKey []byte, withdrawalCredentials []byte) *ethpb.Validator {
func ValidatorFromDeposit(pubKey []byte, withdrawalCredentials []byte) *ethpb.Validator {

Please use idiomatic go, avoid Get prefix

prestonvanloon
prestonvanloon previously approved these changes Jun 28, 2024
@james-prysm james-prysm added this pull request to the merge queue Jun 28, 2024
@james-prysm james-prysm removed this pull request from the merge queue due to a manual request Jun 28, 2024
@james-prysm james-prysm added this pull request to the merge queue Jun 29, 2024
Merged via the queue into develop with commit 028504a Jun 29, 2024
16 of 17 checks passed
@james-prysm james-prysm deleted the eip6110 branch June 29, 2024 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Electra electra hardfork Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants