Use read only validators on ApplyToEveryValidator (#14426)

* Use read only validators on ApplyToEveryValidator

* Use ReadFromEveryValidator on slashing

* change changelog

* Revert "Use ReadFromEveryValidator on slashing"

This reverts commit 74c055bddb56e0573075c71df8a40f1c6a9bfdfd.

CHANGELOG.md

@@ -30,6 +30,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve
 - `grpc-gateway-corsdomain` is renamed to http-cors-domain. The old name can still be used as an alias.
 - `api-timeout` is changed from int flag to duration flag, default value updated.
 - Light client support: abstracted out the light client headers with different versions.
+- `ApplyToEveryValidator` has been changed to prevent misuse bugs, it takes a closure that takes a `ReadOnlyValidator` and returns a raw pointer to a `Validator`. 
 
 ### Deprecated
 - `--disable-grpc-gateway` flag is deprecated due to grpc gateway removal.
@@ -48,6 +49,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve
 - validator registration log changed to debug, and the frequency of validator registration calls are reduced
 - Core: Fix process effective balance update to safe copy validator for Electra.
 - `== nil` checks before calling `IsNil()` on interfaces to prevent panics.
+- Core: Fixed slash processing causing extra hashing
 
 ### Security
 

beacon-chain/core/electra/effective_balance_updates.go

@@ -30,21 +30,21 @@ import (
 //	            or validator.effective_balance + UPWARD_THRESHOLD < balance
 //	        ):
 //	            validator.effective_balance = min(balance - balance % EFFECTIVE_BALANCE_INCREMENT, EFFECTIVE_BALANCE_LIMIT)
-func ProcessEffectiveBalanceUpdates(state state.BeaconState) error {
+func ProcessEffectiveBalanceUpdates(st state.BeaconState) error {
 	effBalanceInc := params.BeaconConfig().EffectiveBalanceIncrement
 	hysteresisInc := effBalanceInc / params.BeaconConfig().HysteresisQuotient
 	downwardThreshold := hysteresisInc * params.BeaconConfig().HysteresisDownwardMultiplier
 	upwardThreshold := hysteresisInc * params.BeaconConfig().HysteresisUpwardMultiplier
 
-	bals := state.Balances()
+	bals := st.Balances()
 
 	// Update effective balances with hysteresis.
-	validatorFunc := func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
+	validatorFunc := func(idx int, val state.ReadOnlyValidator) (newVal *ethpb.Validator, err error) {
 		if val == nil {
-			return false, nil, fmt.Errorf("validator %d is nil in state", idx)
+			return nil, fmt.Errorf("validator %d is nil in state", idx)
 		}
 		if idx >= len(bals) {
-			return false, nil, fmt.Errorf("validator index exceeds validator length in state %d >= %d", idx, len(state.Balances()))
+			return nil, fmt.Errorf("validator index exceeds validator length in state %d >= %d", idx, len(st.Balances()))
 		}
 		balance := bals[idx]
 
@@ -53,14 +53,13 @@ func ProcessEffectiveBalanceUpdates(state state.BeaconState) error {
 			effectiveBalanceLimit = params.BeaconConfig().MaxEffectiveBalanceElectra
 		}
 
-		if balance+downwardThreshold < val.EffectiveBalance || val.EffectiveBalance+upwardThreshold < balance {
+		if balance+downwardThreshold < val.EffectiveBalance() || val.EffectiveBalance()+upwardThreshold < balance {
 			effectiveBal := min(balance-balance%effBalanceInc, effectiveBalanceLimit)
-			newVal := ethpb.CopyValidator(val)
+			newVal = val.Copy()
 			newVal.EffectiveBalance = effectiveBal
-			return true, newVal, nil
 		}
-		return false, val, nil
+		return newVal, nil
 	}
 
-	return state.ApplyToEveryValidator(validatorFunc)
+	return st.ApplyToEveryValidator(validatorFunc)
 }

beacon-chain/core/epoch/epoch_processing.go

@@ -153,9 +153,9 @@ func ProcessRegistryUpdates(ctx context.Context, st state.BeaconState) (state.Be
 //	          penalty_numerator = validator.effective_balance // increment * adjusted_total_slashing_balance
 //	          penalty = penalty_numerator // total_balance * increment
 //	          decrease_balance(state, ValidatorIndex(index), penalty)
-func ProcessSlashings(state state.BeaconState, slashingMultiplier uint64) (state.BeaconState, error) {
-	currentEpoch := time.CurrentEpoch(state)
-	totalBalance, err := helpers.TotalActiveBalance(state)
+func ProcessSlashings(st state.BeaconState, slashingMultiplier uint64) (state.BeaconState, error) {
+	currentEpoch := time.CurrentEpoch(st)
+	totalBalance, err := helpers.TotalActiveBalance(st)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not get total active balance")
 	}
@@ -164,7 +164,7 @@ func ProcessSlashings(state state.BeaconState, slashingMultiplier uint64) (state
 	exitLength := params.BeaconConfig().EpochsPerSlashingsVector
 
 	// Compute the sum of state slashings
-	slashings := state.Slashings()
+	slashings := st.Slashings()
 	totalSlashing := uint64(0)
 	for _, slashing := range slashings {
 		totalSlashing, err = math.Add64(totalSlashing, slashing)
@@ -177,19 +177,18 @@ func ProcessSlashings(state state.BeaconState, slashingMultiplier uint64) (state
 	// below equally.
 	increment := params.BeaconConfig().EffectiveBalanceIncrement
 	minSlashing := math.Min(totalSlashing*slashingMultiplier, totalBalance)
-	err = state.ApplyToEveryValidator(func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
-		correctEpoch := (currentEpoch + exitLength/2) == val.WithdrawableEpoch
-		if val.Slashed && correctEpoch {
-			penaltyNumerator := val.EffectiveBalance / increment * minSlashing
+	err = st.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (newVal *ethpb.Validator, err error) {
+		correctEpoch := (currentEpoch + exitLength/2) == val.WithdrawableEpoch()
+		if val.Slashed() && correctEpoch {
+			penaltyNumerator := val.EffectiveBalance() / increment * minSlashing
 			penalty := penaltyNumerator / totalBalance * increment
-			if err := helpers.DecreaseBalance(state, primitives.ValidatorIndex(idx), penalty); err != nil {
-				return false, val, err
+			if err = helpers.DecreaseBalance(st, primitives.ValidatorIndex(idx), penalty); err != nil {
+				return
 			}
-			return true, val, nil
 		}
-		return false, val, nil
+		return
 	})
-	return state, err
+	return st, err
 }
 
 // ProcessEth1DataReset processes updates to ETH1 data votes during epoch processing.
@@ -231,45 +230,43 @@ func ProcessEth1DataReset(state state.BeaconState) (state.BeaconState, error) {
 //	          or validator.effective_balance + UPWARD_THRESHOLD < balance
 //	      ):
 //	          validator.effective_balance = min(balance - balance % EFFECTIVE_BALANCE_INCREMENT, MAX_EFFECTIVE_BALANCE)
-func ProcessEffectiveBalanceUpdates(state state.BeaconState) (state.BeaconState, error) {
+func ProcessEffectiveBalanceUpdates(st state.BeaconState) (state.BeaconState, error) {
 	effBalanceInc := params.BeaconConfig().EffectiveBalanceIncrement
 	maxEffBalance := params.BeaconConfig().MaxEffectiveBalance
 	hysteresisInc := effBalanceInc / params.BeaconConfig().HysteresisQuotient
 	downwardThreshold := hysteresisInc * params.BeaconConfig().HysteresisDownwardMultiplier
 	upwardThreshold := hysteresisInc * params.BeaconConfig().HysteresisUpwardMultiplier
 
-	bals := state.Balances()
+	bals := st.Balances()
 
 	// Update effective balances with hysteresis.
-	validatorFunc := func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
+	validatorFunc := func(idx int, val state.ReadOnlyValidator) (newVal *ethpb.Validator, err error) {
 		if val == nil {
-			return false, nil, fmt.Errorf("validator %d is nil in state", idx)
+			return nil, fmt.Errorf("validator %d is nil in state", idx)
 		}
 		if idx >= len(bals) {
-			return false, nil, fmt.Errorf("validator index exceeds validator length in state %d >= %d", idx, len(state.Balances()))
+			return nil, fmt.Errorf("validator index exceeds validator length in state %d >= %d", idx, len(st.Balances()))
 		}
 		balance := bals[idx]
 
-		if balance+downwardThreshold < val.EffectiveBalance || val.EffectiveBalance+upwardThreshold < balance {
+		if balance+downwardThreshold < val.EffectiveBalance() || val.EffectiveBalance()+upwardThreshold < balance {
 			effectiveBal := maxEffBalance
 			if effectiveBal > balance-balance%effBalanceInc {
 				effectiveBal = balance - balance%effBalanceInc
 			}
-			if effectiveBal != val.EffectiveBalance {
-				newVal := ethpb.CopyValidator(val)
+			if effectiveBal != val.EffectiveBalance() {
+				newVal = val.Copy()
 				newVal.EffectiveBalance = effectiveBal
-				return true, newVal, nil
 			}
-			return false, val, nil
 		}
-		return false, val, nil
+		return
 	}
 
-	if err := state.ApplyToEveryValidator(validatorFunc); err != nil {
+	if err := st.ApplyToEveryValidator(validatorFunc); err != nil {
 		return nil, err
 	}
 
-	return state, nil
+	return st, nil
 }
 
 // ProcessSlashingsReset processes the total slashing balances updates during epoch processing.

beacon-chain/core/epoch/precompute/slashing.go

@@ -44,17 +44,16 @@ func ProcessSlashingsPrecompute(s state.BeaconState, pBal *Balance) error {
 	}
 
 	increment := params.BeaconConfig().EffectiveBalanceIncrement
-	validatorFunc := func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
-		correctEpoch := epochToWithdraw == val.WithdrawableEpoch
-		if val.Slashed && correctEpoch {
-			penaltyNumerator := val.EffectiveBalance / increment * minSlashing
+	validatorFunc := func(idx int, val state.ReadOnlyValidator) (newVal *ethpb.Validator, err error) {
+		correctEpoch := epochToWithdraw == val.WithdrawableEpoch()
+		if val.Slashed() && correctEpoch {
+			penaltyNumerator := val.EffectiveBalance() / increment * minSlashing
 			penalty := penaltyNumerator / pBal.ActiveCurrentEpoch * increment
-			if err := helpers.DecreaseBalance(s, primitives.ValidatorIndex(idx), penalty); err != nil {
-				return false, val, err
+			if err = helpers.DecreaseBalance(s, primitives.ValidatorIndex(idx), penalty); err != nil {
+				return
 			}
-			return true, val, nil
 		}
-		return false, val, nil
+		return
 	}
 
 	return s.ApplyToEveryValidator(validatorFunc)

beacon-chain/state/interfaces.go

@@ -117,6 +117,7 @@ type ReadOnlyValidator interface {
 	ExitEpoch() primitives.Epoch
 	PublicKey() [fieldparams.BLSPubkeyLength]byte
 	GetWithdrawalCredentials() []byte
+	Copy() *ethpb.Validator
 	Slashed() bool
 	IsNil() bool
 }
@@ -258,7 +259,7 @@ type WriteOnlyEth1Data interface {
 // WriteOnlyValidators defines a struct which only has write access to validators methods.
 type WriteOnlyValidators interface {
 	SetValidators(val []*ethpb.Validator) error
-	ApplyToEveryValidator(f func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error)) error
+	ApplyToEveryValidator(f func(idx int, val ReadOnlyValidator) (*ethpb.Validator, error)) error
 	UpdateValidatorAtIndex(idx primitives.ValidatorIndex, val *ethpb.Validator) error
 	AppendValidator(val *ethpb.Validator) error
 }

beacon-chain/state/state-native/readonly_validator.go

@@ -92,3 +92,19 @@ func (v readOnlyValidator) Slashed() bool {
 func (v readOnlyValidator) IsNil() bool {
 	return v.validator == nil
 }
+
+// Copy returns a new validator from the read only validator
+func (v readOnlyValidator) Copy() *ethpb.Validator {
+	pubKey := v.PublicKey()
+	withdrawalCreds := v.GetWithdrawalCredentials()
+	return &ethpb.Validator{
+		PublicKey:                  pubKey[:],
+		WithdrawalCredentials:      withdrawalCreds,
+		EffectiveBalance:           v.EffectiveBalance(),
+		Slashed:                    v.Slashed(),
+		ActivationEligibilityEpoch: v.ActivationEligibilityEpoch(),
+		ActivationEpoch:            v.ActivationEpoch(),
+		ExitEpoch:                  v.ExitEpoch(),
+		WithdrawableEpoch:          v.WithdrawableEpoch(),
+	}
+}

beacon-chain/state/state-native/references_test.go

@@ -865,8 +865,8 @@ func TestValidatorReferences_RemainsConsistent_Phase0(t *testing.T) {
 
 	assert.DeepNotEqual(t, a.Validators()[0], b.Validators()[0], "validators are equal when they are supposed to be different")
 	// Modify all validators from copied state.
-	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
-		return true, &ethpb.Validator{PublicKey: []byte{'V'}}, nil
+	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) {
+		return &ethpb.Validator{PublicKey: []byte{'V'}}, nil
 	}))
 
 	// Ensure reference is properly accounted for.
@@ -900,8 +900,8 @@ func TestValidatorReferences_RemainsConsistent_Altair(t *testing.T) {
 
 	assert.DeepNotEqual(t, a.Validators()[0], b.Validators()[0], "validators are equal when they are supposed to be different")
 	// Modify all validators from copied state.
-	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
-		return true, &ethpb.Validator{PublicKey: []byte{'V'}}, nil
+	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) {
+		return &ethpb.Validator{PublicKey: []byte{'V'}}, nil
 	}))
 
 	// Ensure reference is properly accounted for.
@@ -935,8 +935,8 @@ func TestValidatorReferences_RemainsConsistent_Capella(t *testing.T) {
 
 	assert.DeepNotEqual(t, a.Validators()[0], b.Validators()[0], "validators are equal when they are supposed to be different")
 	// Modify all validators from copied state.
-	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
-		return true, &ethpb.Validator{PublicKey: []byte{'V'}}, nil
+	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) {
+		return &ethpb.Validator{PublicKey: []byte{'V'}}, nil
 	}))
 
 	// Ensure reference is properly accounted for.
@@ -970,8 +970,8 @@ func TestValidatorReferences_RemainsConsistent_Deneb(t *testing.T) {
 
 	assert.DeepNotEqual(t, a.Validators()[0], b.Validators()[0], "validators are equal when they are supposed to be different")
 	// Modify all validators from copied state.
-	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
-		return true, &ethpb.Validator{PublicKey: []byte{'V'}}, nil
+	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) {
+		return &ethpb.Validator{PublicKey: []byte{'V'}}, nil
 	}))
 
 	// Ensure reference is properly accounted for.
@@ -1005,8 +1005,8 @@ func TestValidatorReferences_RemainsConsistent_Bellatrix(t *testing.T) {
 
 	assert.DeepNotEqual(t, a.Validators()[0], b.Validators()[0], "validators are equal when they are supposed to be different")
 	// Modify all validators from copied state.
-	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
-		return true, &ethpb.Validator{PublicKey: []byte{'V'}}, nil
+	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) {
+		return &ethpb.Validator{PublicKey: []byte{'V'}}, nil
 	}))
 
 	// Ensure reference is properly accounted for.
@@ -1041,15 +1041,14 @@ func TestValidatorReferences_ApplyValidator_BalancesRead(t *testing.T) {
 	require.Equal(t, true, ok)
 
 	// Modify all validators from copied state, it should not deadlock.
-	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
+	assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) {
 		b, err := b.BalanceAtIndex(0)
 		if err != nil {
-			return false, nil, err
+			return nil, err
 		}
-		newVal := ethpb.CopyValidator(val)
+		newVal := val.Copy()
 		newVal.EffectiveBalance += b
-		val.EffectiveBalance += b
-		return true, val, nil
+		return newVal, nil
 	}))
 }
 

beacon-chain/state/state-native/setters_validator.go

@@ -2,6 +2,7 @@ package state_native
 
 import (
 	"github.com/pkg/errors"
+	"github.com/prysmaticlabs/prysm/v5/beacon-chain/state"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/state/state-native/types"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/state/stateutil"
 	"github.com/prysmaticlabs/prysm/v5/config/features"
@@ -38,7 +39,7 @@ func (b *BeaconState) SetValidators(val []*ethpb.Validator) error {
 
 // ApplyToEveryValidator applies the provided callback function to each validator in the
 // validator registry.
-func (b *BeaconState) ApplyToEveryValidator(f func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error)) error {
+func (b *BeaconState) ApplyToEveryValidator(f func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error)) error {
 	var changedVals []uint64
 	if features.Get().EnableExperimentalState {
 		l := b.validatorsMultiValue.Len(b)
@@ -47,11 +48,15 @@ func (b *BeaconState) ApplyToEveryValidator(f func(idx int, val *ethpb.Validator
 			if err != nil {
 				return err
 			}
-			changed, newVal, err := f(i, v)
+			ro, err := NewValidator(v)
 			if err != nil {
 				return err
 			}
-			if changed {
+			newVal, err := f(i, ro)
+			if err != nil {
+				return err
+			}
+			if newVal != nil {
 				changedVals = append(changedVals, uint64(i))
 				if err = b.validatorsMultiValue.UpdateAt(b, uint64(i), newVal); err != nil {
 					return errors.Wrapf(err, "could not update validator at index %d", i)
@@ -71,11 +76,15 @@ func (b *BeaconState) ApplyToEveryValidator(f func(idx int, val *ethpb.Validator
 		b.lock.Unlock()
 
 		for i, val := range v {
-			changed, newVal, err := f(i, val)
+			ro, err := NewValidator(val)
 			if err != nil {
 				return err
 			}
-			if changed {
+			newVal, err := f(i, ro)
+			if err != nil {
+				return err
+			}
+			if newVal != nil {
 				changedVals = append(changedVals, uint64(i))
 				v[i] = newVal
 			}
@@ -89,8 +98,10 @@ func (b *BeaconState) ApplyToEveryValidator(f func(idx int, val *ethpb.Validator
 	b.lock.Lock()
 	defer b.lock.Unlock()
 
-	b.markFieldAsDirty(types.Validators)
-	b.addDirtyIndices(types.Validators, changedVals)
+	if len(changedVals) > 0 {
+		b.markFieldAsDirty(types.Validators)
+		b.addDirtyIndices(types.Validators, changedVals)
+	}
 	return nil
 }
 

beacon-chain/sync/validate_bls_to_execution_change_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/p2p/encoder"
 	mockp2p "github.com/prysmaticlabs/prysm/v5/beacon-chain/p2p/testing"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/startup"
+	"github.com/prysmaticlabs/prysm/v5/beacon-chain/state"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/state/stategen"
 	mockSync "github.com/prysmaticlabs/prysm/v5/beacon-chain/sync/initial-sync/testing"
 	"github.com/prysmaticlabs/prysm/v5/beacon-chain/verification"
@@ -293,12 +294,13 @@ func TestService_ValidateBlsToExecutionChange(t *testing.T) {
 				s.cfg.clock = startup.NewClock(time.Now(), [32]byte{'A'})
 				s.initCaches()
 				st, keys := util.DeterministicGenesisStateCapella(t, 128)
-				assert.NoError(t, st.ApplyToEveryValidator(func(idx int, val *ethpb.Validator) (bool, *ethpb.Validator, error) {
+				assert.NoError(t, st.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) {
 					newCreds := make([]byte, 32)
 					newCreds[0] = params.BeaconConfig().ETH1AddressWithdrawalPrefixByte
 					copy(newCreds[12:], wantedExecAddress)
-					val.WithdrawalCredentials = newCreds
-					return true, val, nil
+					newVal := val.Copy()
+					newVal.WithdrawalCredentials = newCreds
+					return newVal, nil
 				}))
 				s.cfg.chain = &mockChain.ChainService{
 					State:   st,