diff --git a/CHANGELOG.md b/CHANGELOG.md index 42fad7ce6f..b5858dcd92 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/beacon-chain/core/electra/effective_balance_updates.go b/beacon-chain/core/electra/effective_balance_updates.go index bc5a73b9f1..c87d232adb 100644 --- a/beacon-chain/core/electra/effective_balance_updates.go +++ b/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) } diff --git a/beacon-chain/core/epoch/epoch_processing.go b/beacon-chain/core/epoch/epoch_processing.go index 1fe1293b53..499b3baada 100644 --- a/beacon-chain/core/epoch/epoch_processing.go +++ b/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. diff --git a/beacon-chain/core/epoch/precompute/slashing.go b/beacon-chain/core/epoch/precompute/slashing.go index 03e0cd6ef9..69bf6d7147 100644 --- a/beacon-chain/core/epoch/precompute/slashing.go +++ b/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) diff --git a/beacon-chain/state/interfaces.go b/beacon-chain/state/interfaces.go index e7b4915e1c..ba7d00fdac 100644 --- a/beacon-chain/state/interfaces.go +++ b/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 } diff --git a/beacon-chain/state/state-native/readonly_validator.go b/beacon-chain/state/state-native/readonly_validator.go index f5029049ed..c26b212ba8 100644 --- a/beacon-chain/state/state-native/readonly_validator.go +++ b/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 ðpb.Validator{ + PublicKey: pubKey[:], + WithdrawalCredentials: withdrawalCreds, + EffectiveBalance: v.EffectiveBalance(), + Slashed: v.Slashed(), + ActivationEligibilityEpoch: v.ActivationEligibilityEpoch(), + ActivationEpoch: v.ActivationEpoch(), + ExitEpoch: v.ExitEpoch(), + WithdrawableEpoch: v.WithdrawableEpoch(), + } +} diff --git a/beacon-chain/state/state-native/references_test.go b/beacon-chain/state/state-native/references_test.go index 82596e11cc..d58f43e991 100644 --- a/beacon-chain/state/state-native/references_test.go +++ b/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, ðpb.Validator{PublicKey: []byte{'V'}}, nil + assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) { + return ðpb.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, ðpb.Validator{PublicKey: []byte{'V'}}, nil + assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) { + return ðpb.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, ðpb.Validator{PublicKey: []byte{'V'}}, nil + assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) { + return ðpb.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, ðpb.Validator{PublicKey: []byte{'V'}}, nil + assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) { + return ðpb.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, ðpb.Validator{PublicKey: []byte{'V'}}, nil + assert.NoError(t, b.ApplyToEveryValidator(func(idx int, val state.ReadOnlyValidator) (*ethpb.Validator, error) { + return ðpb.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 })) } diff --git a/beacon-chain/state/state-native/setters_validator.go b/beacon-chain/state/state-native/setters_validator.go index a30986c9cc..6569a08065 100644 --- a/beacon-chain/state/state-native/setters_validator.go +++ b/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 } diff --git a/beacon-chain/sync/validate_bls_to_execution_change_test.go b/beacon-chain/sync/validate_bls_to_execution_change_test.go index c6fb8d43c9..2552790480 100644 --- a/beacon-chain/sync/validate_bls_to_execution_change_test.go +++ b/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,