Change non-mutating validator accesses to ReadOnly (#5776)

* Change instances of ValidatorAtIndex to ReadOnly where possible

* Use ReadOnly for VerifyExit and Slashings

* Move length check to before lock

* Improve readonly tests

* undo process attester changes

* Fix test

beacon-chain/core/blocks/block_operations.go

@@ -212,7 +212,7 @@ func ProcessBlockHeader(
 
 // VerifyBlockHeaderSignature verifies the proposer signature of a beacon block.
 func VerifyBlockHeaderSignature(beaconState *stateTrie.BeaconState, block *ethpb.SignedBeaconBlock) error {
-	proposer, err := beaconState.ValidatorAtIndex(block.Block.ProposerIndex)
+	proposer, err := beaconState.ValidatorAtIndexReadOnly(block.Block.ProposerIndex)
 	if err != nil {
 		return err
 	}
@@ -222,7 +222,8 @@ func VerifyBlockHeaderSignature(beaconState *stateTrie.BeaconState, block *ethpb
 	if err != nil {
 		return err
 	}
-	return helpers.VerifyBlockSigningRoot(block.Block, proposer.PublicKey, block.Signature, domain)
+	proposerPubKey := proposer.PublicKey()
+	return helpers.VerifyBlockSigningRoot(block.Block, proposerPubKey[:], block.Signature, domain)
 }
 
 // ProcessBlockHeaderNoVerify validates a block by its header but skips proposer
@@ -278,11 +279,11 @@ func ProcessBlockHeaderNoVerify(
 			block.ParentRoot, parentRoot)
 	}
 
-	proposer, err := beaconState.ValidatorAtIndex(idx)
+	proposer, err := beaconState.ValidatorAtIndexReadOnly(idx)
 	if err != nil {
 		return nil, err
 	}
-	if proposer.Slashed {
+	if proposer.Slashed() {
 		return nil, fmt.Errorf("proposer at index %d was previously slashed", idx)
 	}
 
@@ -446,12 +447,12 @@ func VerifyProposerSlashing(
 	if proto.Equal(slashing.Header_1, slashing.Header_2) {
 		return errors.New("expected slashing headers to differ")
 	}
-	proposer, err := beaconState.ValidatorAtIndex(slashing.Header_1.Header.ProposerIndex)
+	proposer, err := beaconState.ValidatorAtIndexReadOnly(slashing.Header_1.Header.ProposerIndex)
 	if err != nil {
 		return err
 	}
-	if !helpers.IsSlashableValidator(proposer, helpers.SlotToEpoch(beaconState.Slot())) {
-		return fmt.Errorf("validator with key %#x is not slashable", proposer.PublicKey)
+	if !helpers.IsSlashableValidatorUsingTrie(proposer, helpers.SlotToEpoch(beaconState.Slot())) {
+		return fmt.Errorf("validator with key %#x is not slashable", proposer.PublicKey())
 	}
 	// Using headerEpoch1 here because both of the headers should have the same epoch.
 	domain, err := helpers.Domain(beaconState.Fork(), helpers.SlotToEpoch(slashing.Header_1.Header.Slot), params.BeaconConfig().DomainBeaconProposer, beaconState.GenesisValidatorRoot())
@@ -460,7 +461,8 @@ func VerifyProposerSlashing(
 	}
 	headers := []*ethpb.SignedBeaconBlockHeader{slashing.Header_1, slashing.Header_2}
 	for _, header := range headers {
-		if err := helpers.VerifySigningRoot(header.Header, proposer.PublicKey, header.Signature, domain); err != nil {
+		proposerPubKey := proposer.PublicKey()
+		if err := helpers.VerifySigningRoot(header.Header, proposerPubKey[:], header.Signature, domain); err != nil {
 			return errors.Wrap(err, "could not verify beacon block header")
 		}
 	}
@@ -1054,7 +1056,7 @@ func ProcessVoluntaryExits(
 				beaconState.NumValidators(),
 			)
 		}
-		val, err := beaconState.ValidatorAtIndex(exit.Exit.ValidatorIndex)
+		val, err := beaconState.ValidatorAtIndexReadOnly(exit.Exit.ValidatorIndex)
 		if err != nil {
 			return nil, err
 		}
@@ -1109,7 +1111,7 @@ func ProcessVoluntaryExitsNoVerify(
 //    # Verify signature
 //    domain = get_domain(state, DOMAIN_VOLUNTARY_EXIT, exit.epoch)
 //    assert bls_verify(validator.pubkey, signing_root(exit), exit.signature, domain)
-func VerifyExit(validator *ethpb.Validator, currentSlot uint64, fork *pb.Fork, signed *ethpb.SignedVoluntaryExit, genesisRoot []byte) error {
+func VerifyExit(validator *stateTrie.ReadOnlyValidator, currentSlot uint64, fork *pb.Fork, signed *ethpb.SignedVoluntaryExit, genesisRoot []byte) error {
 	if signed == nil || signed.Exit == nil {
 		return errors.New("nil exit")
 	}
@@ -1117,30 +1119,31 @@ func VerifyExit(validator *ethpb.Validator, currentSlot uint64, fork *pb.Fork, s
 	exit := signed.Exit
 	currentEpoch := helpers.SlotToEpoch(currentSlot)
 	// Verify the validator is active.
-	if !helpers.IsActiveValidator(validator, currentEpoch) {
+	if !helpers.IsActiveValidatorUsingTrie(validator, currentEpoch) {
 		return errors.New("non-active validator cannot exit")
 	}
 	// Verify the validator has not yet exited.
-	if validator.ExitEpoch != params.BeaconConfig().FarFutureEpoch {
-		return fmt.Errorf("validator has already exited at epoch: %v", validator.ExitEpoch)
+	if validator.ExitEpoch() != params.BeaconConfig().FarFutureEpoch {
+		return fmt.Errorf("validator has already exited at epoch: %v", validator.ExitEpoch())
 	}
 	// Exits must specify an epoch when they become valid; they are not valid before then.
 	if currentEpoch < exit.Epoch {
 		return fmt.Errorf("expected current epoch >= exit epoch, received %d < %d", currentEpoch, exit.Epoch)
 	}
 	// Verify the validator has been active long enough.
-	if currentEpoch < validator.ActivationEpoch+params.BeaconConfig().PersistentCommitteePeriod {
+	if currentEpoch < validator.ActivationEpoch()+params.BeaconConfig().PersistentCommitteePeriod {
 		return fmt.Errorf(
 			"validator has not been active long enough to exit, wanted epoch %d >= %d",
 			currentEpoch,
-			validator.ActivationEpoch+params.BeaconConfig().PersistentCommitteePeriod,
+			validator.ActivationEpoch()+params.BeaconConfig().PersistentCommitteePeriod,
 		)
 	}
 	domain, err := helpers.Domain(fork, exit.Epoch, params.BeaconConfig().DomainVoluntaryExit, genesisRoot)
 	if err != nil {
 		return err
 	}
-	if err := helpers.VerifySigningRoot(exit, validator.PublicKey, signed.Signature, domain); err != nil {
+	valPubKey := validator.PublicKey()
+	if err := helpers.VerifySigningRoot(exit, valPubKey[:], signed.Signature, domain); err != nil {
 		return helpers.ErrSigFailedToVerify
 	}
 	return nil

beacon-chain/core/blocks/block_operations_fuzz_test.go

@@ -410,7 +410,7 @@ func TestFuzzProcessVoluntaryExitsNoVerify_10000(t *testing.T) {
 func TestFuzzVerifyExit_10000(t *testing.T) {
 	fuzzer := fuzz.NewWithSeed(0)
 	ve := &eth.SignedVoluntaryExit{}
-	val := &eth.Validator{}
+	val := &beaconstate.ReadOnlyValidator{}
 	fork := &pb.Fork{}
 	var slot uint64
 

beacon-chain/core/blocks/block_operations_test.go

@@ -549,7 +549,7 @@ func TestProcessProposerSlashings_ValidatorNotSlashable(t *testing.T) {
 	}
 	want := fmt.Sprintf(
 		"validator with key %#x is not slashable",
-		beaconState.Validators()[0].PublicKey,
+		bytesutil.ToBytes48(beaconState.Validators()[0].PublicKey),
 	)
 
 	_, err = blocks.ProcessProposerSlashings(context.Background(), beaconState, block.Body)

beacon-chain/core/epoch/epoch_processing.go

@@ -363,11 +363,11 @@ func unslashedAttestingIndices(state *stateTrie.BeaconState, atts []*pb.PendingA
 	sort.Slice(setIndices, func(i, j int) bool { return setIndices[i] < setIndices[j] })
 	// Remove the slashed validator indices.
 	for i := 0; i < len(setIndices); i++ {
-		v, err := state.ValidatorAtIndex(setIndices[i])
+		v, err := state.ValidatorAtIndexReadOnly(setIndices[i])
 		if err != nil {
 			return nil, errors.Wrap(err, "failed to look up validator")
 		}
-		if v != nil && v.Slashed {
+		if v != nil && v.Slashed() {
 			setIndices = append(setIndices[:i], setIndices[i+1:]...)
 		}
 	}
@@ -391,11 +391,11 @@ func BaseReward(state *stateTrie.BeaconState, index uint64) (uint64, error) {
 	if err != nil {
 		return 0, errors.Wrap(err, "could not calculate active balance")
 	}
-	val, err := state.ValidatorAtIndex(index)
+	val, err := state.ValidatorAtIndexReadOnly(index)
 	if err != nil {
 		return 0, err
 	}
-	effectiveBalance := val.EffectiveBalance
+	effectiveBalance := val.EffectiveBalance()
 	baseReward := effectiveBalance * params.BeaconConfig().BaseRewardFactor /
 		mathutil.IntegerSquareRoot(totalBalance) / params.BeaconConfig().BaseRewardsPerEpoch
 	return baseReward, nil

beacon-chain/core/helpers/validators.go

@@ -42,10 +42,19 @@ func checkValidatorActiveStatus(activationEpoch uint64, exitEpoch uint64, epoch
 //  Check if ``validator`` is slashable.
 //  """
 //  return (not validator.slashed) and (validator.activation_epoch <= epoch < validator.withdrawable_epoch)
-func IsSlashableValidator(validator *ethpb.Validator, epoch uint64) bool {
-	active := validator.ActivationEpoch <= epoch
-	beforeWithdrawable := epoch < validator.WithdrawableEpoch
-	return beforeWithdrawable && active && !validator.Slashed
+func IsSlashableValidator(val *ethpb.Validator, epoch uint64) bool {
+	return checkValidatorSlashable(val.ActivationEpoch, val.WithdrawableEpoch, val.Slashed, epoch)
+}
+
+// IsSlashableValidatorUsingTrie checks if a read only validator is slashable.
+func IsSlashableValidatorUsingTrie(val *stateTrie.ReadOnlyValidator, epoch uint64) bool {
+	return checkValidatorSlashable(val.ActivationEpoch(), val.WithdrawableEpoch(), val.Slashed(), epoch)
+}
+
+func checkValidatorSlashable(activationEpoch uint64, withdrawableEpoch uint64, slashed bool, epoch uint64) bool {
+	active := activationEpoch <= epoch
+	beforeWithdrawable := epoch < withdrawableEpoch
+	return beforeWithdrawable && active && !slashed
 }
 
 // ActiveValidatorIndices filters out active validators based on validator status

beacon-chain/core/helpers/validators_test.go

@@ -34,88 +34,205 @@ func TestIsActiveValidator_OK(t *testing.T) {
 	}
 }
 
-func TestIsSlashableValidator_Active(t *testing.T) {
-	activeValidator := &ethpb.Validator{
-		WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+func TestIsActiveValidatorUsingTrie_OK(t *testing.T) {
+	tests := []struct {
+		a uint64
+		b bool
+	}{
+		{a: 0, b: false},
+		{a: 10, b: true},
+		{a: 100, b: false},
+		{a: 1000, b: false},
+		{a: 64, b: true},
 	}
-
-	slashableValidator := IsSlashableValidator(activeValidator, 0)
-	if !slashableValidator {
-		t.Errorf("Expected active validator to be slashable, received false")
+	val := &ethpb.Validator{ActivationEpoch: 10, ExitEpoch: 100}
+	beaconState, err := beaconstate.InitializeFromProto(&pb.BeaconState{Validators: []*ethpb.Validator{val}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, test := range tests {
+		readOnlyVal, err := beaconState.ValidatorAtIndexReadOnly(0)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if IsActiveValidatorUsingTrie(readOnlyVal, test.a) != test.b {
+			t.Errorf("IsActiveValidatorUsingTrie(%d) = %v, want = %v",
+				test.a, IsActiveValidatorUsingTrie(readOnlyVal, test.a), test.b)
+		}
 	}
 }
 
-func TestIsSlashableValidator_BeforeWithdrawable(t *testing.T) {
-	beforeWithdrawableValidator := &ethpb.Validator{
-		WithdrawableEpoch: 5,
+func TestIsSlashableValidator_OK(t *testing.T) {
+	tests := []struct {
+		name      string
+		validator *ethpb.Validator
+		epoch     uint64
+		slashable bool
+	}{
+		{
+			name: "Unset withdrawable, slashable",
+			validator: &ethpb.Validator{
+				WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+			},
+			epoch:     0,
+			slashable: true,
+		},
+		{
+			name: "before withdrawable, slashable",
+			validator: &ethpb.Validator{
+				WithdrawableEpoch: 5,
+			},
+			epoch:     3,
+			slashable: true,
+		},
+		{
+			name: "inactive, not slashable",
+			validator: &ethpb.Validator{
+				ActivationEpoch:   5,
+				WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+			},
+			epoch:     2,
+			slashable: false,
+		},
+		{
+			name: "after withdrawable, not slashable",
+			validator: &ethpb.Validator{
+				WithdrawableEpoch: 3,
+			},
+			epoch:     3,
+			slashable: false,
+		},
+		{
+			name: "slashed and withdrawable, not slashable",
+			validator: &ethpb.Validator{
+				Slashed:           true,
+				ExitEpoch:         params.BeaconConfig().FarFutureEpoch,
+				WithdrawableEpoch: 1,
+			},
+			epoch:     2,
+			slashable: false,
+		},
+		{
+			name: "slashed, not slashable",
+			validator: &ethpb.Validator{
+				Slashed:           true,
+				ExitEpoch:         params.BeaconConfig().FarFutureEpoch,
+				WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+			},
+			epoch:     2,
+			slashable: false,
+		},
+		{
+			name: "inactive and slashed, not slashable",
+			validator: &ethpb.Validator{
+				Slashed:           true,
+				ActivationEpoch:   4,
+				ExitEpoch:         params.BeaconConfig().FarFutureEpoch,
+				WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+			},
+			epoch:     2,
+			slashable: false,
+		},
 	}
 
-	slashableValidator := IsSlashableValidator(beforeWithdrawableValidator, 3)
-	if !slashableValidator {
-		t.Errorf("Expected before withdrawable validator to be slashable, received false")
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			slashableValidator := IsSlashableValidator(test.validator, test.epoch)
+			if test.slashable != slashableValidator {
+				t.Errorf("Expected active validator slashable to be %t, received %t", test.slashable, slashableValidator)
+			}
+		})
 	}
 }
 
-func TestIsSlashableValidator_Inactive(t *testing.T) {
-	inactiveValidator := &ethpb.Validator{
-		ActivationEpoch:   5,
-		WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+func TestIsSlashableValidatorUsingTrie_OK(t *testing.T) {
+	tests := []struct {
+		name      string
+		validator *ethpb.Validator
+		epoch     uint64
+		slashable bool
+	}{
+		{
+			name: "Unset withdrawable, slashable",
+			validator: &ethpb.Validator{
+				WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+			},
+			epoch:     0,
+			slashable: true,
+		},
+		{
+			name: "before withdrawable, slashable",
+			validator: &ethpb.Validator{
+				WithdrawableEpoch: 5,
+			},
+			epoch:     3,
+			slashable: true,
+		},
+		{
+			name: "inactive, not slashable",
+			validator: &ethpb.Validator{
+				ActivationEpoch:   5,
+				WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+			},
+			epoch:     2,
+			slashable: false,
+		},
+		{
+			name: "after withdrawable, not slashable",
+			validator: &ethpb.Validator{
+				WithdrawableEpoch: 3,
+			},
+			epoch:     3,
+			slashable: false,
+		},
+		{
+			name: "slashed and withdrawable, not slashable",
+			validator: &ethpb.Validator{
+				Slashed:           true,
+				ExitEpoch:         params.BeaconConfig().FarFutureEpoch,
+				WithdrawableEpoch: 1,
+			},
+			epoch:     2,
+			slashable: false,
+		},
+		{
+			name: "slashed, not slashable",
+			validator: &ethpb.Validator{
+				Slashed:           true,
+				ExitEpoch:         params.BeaconConfig().FarFutureEpoch,
+				WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+			},
+			epoch:     2,
+			slashable: false,
+		},
+		{
+			name: "inactive and slashed, not slashable",
+			validator: &ethpb.Validator{
+				Slashed:           true,
+				ActivationEpoch:   4,
+				ExitEpoch:         params.BeaconConfig().FarFutureEpoch,
+				WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
+			},
+			epoch:     2,
+			slashable: false,
+		},
 	}
 
-	slashableValidator := IsSlashableValidator(inactiveValidator, 2)
-	if slashableValidator {
-		t.Errorf("Expected inactive validator to not be slashable, received true")
-	}
-}
-
-func TestIsSlashableValidator_AfterWithdrawable(t *testing.T) {
-	afterWithdrawableValidator := &ethpb.Validator{
-		WithdrawableEpoch: 3,
-	}
-
-	slashableValidator := IsSlashableValidator(afterWithdrawableValidator, 3)
-	if slashableValidator {
-		t.Errorf("Expected after withdrawable validator to not be slashable, received true")
-	}
-}
-
-func TestIsSlashableValidator_SlashedWithdrawalble(t *testing.T) {
-	slashedValidator := &ethpb.Validator{
-		Slashed:           true,
-		ExitEpoch:         params.BeaconConfig().FarFutureEpoch,
-		WithdrawableEpoch: 1,
-	}
-
-	slashableValidator := IsSlashableValidator(slashedValidator, 2)
-	if slashableValidator {
-		t.Errorf("Expected slashable validator to not be slashable, received true")
-	}
-}
-
-func TestIsSlashableValidator_Slashed(t *testing.T) {
-	slashedValidator2 := &ethpb.Validator{
-		Slashed:           true,
-		ExitEpoch:         params.BeaconConfig().FarFutureEpoch,
-		WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
-	}
-
-	slashableValidator := IsSlashableValidator(slashedValidator2, 2)
-	if slashableValidator {
-		t.Errorf("Expected slashable validator to not be slashable, received true")
-	}
-}
-
-func TestIsSlashableValidator_InactiveSlashed(t *testing.T) {
-	slashedValidator2 := &ethpb.Validator{
-		Slashed:           true,
-		ActivationEpoch:   4,
-		ExitEpoch:         params.BeaconConfig().FarFutureEpoch,
-		WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch,
-	}
-
-	slashableValidator := IsSlashableValidator(slashedValidator2, 2)
-	if slashableValidator {
-		t.Errorf("Expected slashable validator to not be slashable, received true")
+	for _, test := range tests {
+		beaconState, err := beaconstate.InitializeFromProto(&pb.BeaconState{Validators: []*ethpb.Validator{test.validator}})
+		if err != nil {
+			t.Fatal(err)
+		}
+		readOnlyVal, err := beaconState.ValidatorAtIndexReadOnly(0)
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Run(test.name, func(t *testing.T) {
+			slashableValidator := IsSlashableValidatorUsingTrie(readOnlyVal, test.epoch)
+			if test.slashable != slashableValidator {
+				t.Errorf("Expected active validator slashable to be %t, received %t", test.slashable, slashableValidator)
+			}
+		})
 	}
 }
 

beacon-chain/rpc/beacon/validators.go

@@ -959,12 +959,12 @@ func (bs *Server) GetValidatorPerformance(
 			missingValidators = append(missingValidators, key)
 			continue
 		}
-		val, err := headState.ValidatorAtIndex(idx)
+		val, err := headState.ValidatorAtIndexReadOnly(idx)
 		if err != nil {
 			return nil, status.Errorf(codes.Internal, "could not get validator: %v", err)
 		}
 		currentEpoch := helpers.CurrentEpoch(headState)
-		if !helpers.IsActiveValidator(val, currentEpoch) {
+		if !helpers.IsActiveValidatorUsingTrie(val, currentEpoch) {
 			// Inactive validator; treat it as missing.
 			missingValidators = append(missingValidators, key)
 			continue

beacon-chain/rpc/validator/exit.go

@@ -31,7 +31,7 @@ func (vs *Server) ProposeExit(ctx context.Context, req *ethpb.SignedVoluntaryExi
 	}
 
 	// Confirm the validator is eligible to exit with the parameters provided.
-	val, err := s.ValidatorAtIndex(req.Exit.ValidatorIndex)
+	val, err := s.ValidatorAtIndexReadOnly(req.Exit.ValidatorIndex)
 	if err != nil {
 		return nil, status.Error(codes.InvalidArgument, "validator index exceeds validator set length")
 	}

beacon-chain/state/getters.go

@@ -429,7 +429,7 @@ func (b *BeaconState) ValidatorAtIndex(idx uint64) (*ethpb.Validator, error) {
 	}, nil
 }
 
-// ValidatorAtIndexReadOnly is the validator at the provided index.This method
+// ValidatorAtIndexReadOnly is the validator at the provided index. This method
 // doesn't clone the validator.
 func (b *BeaconState) ValidatorAtIndexReadOnly(idx uint64) (*ReadOnlyValidator, error) {
 	if !b.HasInnerState() {
@@ -438,13 +438,13 @@ func (b *BeaconState) ValidatorAtIndexReadOnly(idx uint64) (*ReadOnlyValidator,
 	if b.state.Validators == nil {
 		return &ReadOnlyValidator{}, nil
 	}
+	if uint64(len(b.state.Validators)) <= idx {
+		return nil, fmt.Errorf("index %d out of range", idx)
+	}
 
 	b.lock.RLock()
 	defer b.lock.RUnlock()
 
-	if len(b.state.Validators) <= int(idx) {
-		return nil, fmt.Errorf("index %d out of range", idx)
-	}
 	return &ReadOnlyValidator{b.state.Validators[idx]}, nil
 }
 

beacon-chain/sync/validate_voluntary_exit.go

@@ -57,7 +57,7 @@ func (r *Service) validateVoluntaryExit(ctx context.Context, pid peer.ID, msg *p
 	if int(exit.Exit.ValidatorIndex) >= s.NumValidators() {
 		return false
 	}
-	val, err := s.ValidatorAtIndex(exit.Exit.ValidatorIndex)
+	val, err := s.ValidatorAtIndexReadOnly(exit.Exit.ValidatorIndex)
 	if err != nil {
 		return false
 	}