From 9a1157465e9205fbec4026299faf058b8b3bad98 Mon Sep 17 00:00:00 2001 From: Ivan Martinez Date: Thu, 7 May 2020 14:15:51 -0400 Subject: [PATCH] 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 | 35 +-- .../core/blocks/block_operations_fuzz_test.go | 2 +- .../core/blocks/block_operations_test.go | 2 +- beacon-chain/core/epoch/epoch_processing.go | 8 +- beacon-chain/core/helpers/validators.go | 17 +- beacon-chain/core/helpers/validators_test.go | 257 +++++++++++++----- beacon-chain/rpc/beacon/validators.go | 4 +- beacon-chain/rpc/validator/exit.go | 2 +- beacon-chain/state/getters.go | 8 +- beacon-chain/sync/validate_voluntary_exit.go | 2 +- 10 files changed, 233 insertions(+), 104 deletions(-) diff --git a/beacon-chain/core/blocks/block_operations.go b/beacon-chain/core/blocks/block_operations.go index a615e66c690c..17c46b45733a 100644 --- a/beacon-chain/core/blocks/block_operations.go +++ b/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 diff --git a/beacon-chain/core/blocks/block_operations_fuzz_test.go b/beacon-chain/core/blocks/block_operations_fuzz_test.go index 22b29cd4ef7c..fd23a694744a 100644 --- a/beacon-chain/core/blocks/block_operations_fuzz_test.go +++ b/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 := ð.SignedVoluntaryExit{} - val := ð.Validator{} + val := &beaconstate.ReadOnlyValidator{} fork := &pb.Fork{} var slot uint64 diff --git a/beacon-chain/core/blocks/block_operations_test.go b/beacon-chain/core/blocks/block_operations_test.go index 5a53040e2266..115edf485db9 100644 --- a/beacon-chain/core/blocks/block_operations_test.go +++ b/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) diff --git a/beacon-chain/core/epoch/epoch_processing.go b/beacon-chain/core/epoch/epoch_processing.go index fad0a6a4fe0f..558bf0aebb7b 100644 --- a/beacon-chain/core/epoch/epoch_processing.go +++ b/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 diff --git a/beacon-chain/core/helpers/validators.go b/beacon-chain/core/helpers/validators.go index 3504bcf72dcc..c4422d62e270 100644 --- a/beacon-chain/core/helpers/validators.go +++ b/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 diff --git a/beacon-chain/core/helpers/validators_test.go b/beacon-chain/core/helpers/validators_test.go index 8648953f87eb..d249f1c9c2ab 100644 --- a/beacon-chain/core/helpers/validators_test.go +++ b/beacon-chain/core/helpers/validators_test.go @@ -34,88 +34,205 @@ func TestIsActiveValidator_OK(t *testing.T) { } } -func TestIsSlashableValidator_Active(t *testing.T) { - activeValidator := ðpb.Validator{ - WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, - } - - slashableValidator := IsSlashableValidator(activeValidator, 0) - if !slashableValidator { - t.Errorf("Expected active validator to be slashable, received false") - } -} - -func TestIsSlashableValidator_BeforeWithdrawable(t *testing.T) { - beforeWithdrawableValidator := ðpb.Validator{ - WithdrawableEpoch: 5, - } - - slashableValidator := IsSlashableValidator(beforeWithdrawableValidator, 3) - if !slashableValidator { - t.Errorf("Expected before withdrawable validator to be slashable, received false") - } -} - -func TestIsSlashableValidator_Inactive(t *testing.T) { - inactiveValidator := ðpb.Validator{ - ActivationEpoch: 5, - WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, - } - - slashableValidator := IsSlashableValidator(inactiveValidator, 2) - if slashableValidator { - t.Errorf("Expected inactive validator to not be slashable, received true") - } -} - -func TestIsSlashableValidator_AfterWithdrawable(t *testing.T) { - afterWithdrawableValidator := ðpb.Validator{ - WithdrawableEpoch: 3, - } - - slashableValidator := IsSlashableValidator(afterWithdrawableValidator, 3) - if slashableValidator { - t.Errorf("Expected after withdrawable validator to not be slashable, received true") +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}, } -} - -func TestIsSlashableValidator_SlashedWithdrawalble(t *testing.T) { - slashedValidator := ðpb.Validator{ - Slashed: true, - ExitEpoch: params.BeaconConfig().FarFutureEpoch, - WithdrawableEpoch: 1, + val := ðpb.Validator{ActivationEpoch: 10, ExitEpoch: 100} + beaconState, err := beaconstate.InitializeFromProto(&pb.BeaconState{Validators: []*ethpb.Validator{val}}) + if err != nil { + t.Fatal(err) } - - slashableValidator := IsSlashableValidator(slashedValidator, 2) - if slashableValidator { - t.Errorf("Expected slashable validator to not be slashable, received true") + 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_Slashed(t *testing.T) { - slashedValidator2 := ðpb.Validator{ - Slashed: true, - ExitEpoch: params.BeaconConfig().FarFutureEpoch, - WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, +func TestIsSlashableValidator_OK(t *testing.T) { + tests := []struct { + name string + validator *ethpb.Validator + epoch uint64 + slashable bool + }{ + { + name: "Unset withdrawable, slashable", + validator: ðpb.Validator{ + WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, + }, + epoch: 0, + slashable: true, + }, + { + name: "before withdrawable, slashable", + validator: ðpb.Validator{ + WithdrawableEpoch: 5, + }, + epoch: 3, + slashable: true, + }, + { + name: "inactive, not slashable", + validator: ðpb.Validator{ + ActivationEpoch: 5, + WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, + }, + epoch: 2, + slashable: false, + }, + { + name: "after withdrawable, not slashable", + validator: ðpb.Validator{ + WithdrawableEpoch: 3, + }, + epoch: 3, + slashable: false, + }, + { + name: "slashed and withdrawable, not slashable", + validator: ðpb.Validator{ + Slashed: true, + ExitEpoch: params.BeaconConfig().FarFutureEpoch, + WithdrawableEpoch: 1, + }, + epoch: 2, + slashable: false, + }, + { + name: "slashed, not slashable", + validator: ðpb.Validator{ + Slashed: true, + ExitEpoch: params.BeaconConfig().FarFutureEpoch, + WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, + }, + epoch: 2, + slashable: false, + }, + { + name: "inactive and slashed, not slashable", + validator: ðpb.Validator{ + Slashed: true, + ActivationEpoch: 4, + ExitEpoch: params.BeaconConfig().FarFutureEpoch, + WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, + }, + epoch: 2, + slashable: false, + }, } - slashableValidator := IsSlashableValidator(slashedValidator2, 2) - if slashableValidator { - t.Errorf("Expected slashable validator to not be slashable, received true") + 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_InactiveSlashed(t *testing.T) { - slashedValidator2 := ðpb.Validator{ - Slashed: true, - ActivationEpoch: 4, - ExitEpoch: params.BeaconConfig().FarFutureEpoch, - 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: ðpb.Validator{ + WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, + }, + epoch: 0, + slashable: true, + }, + { + name: "before withdrawable, slashable", + validator: ðpb.Validator{ + WithdrawableEpoch: 5, + }, + epoch: 3, + slashable: true, + }, + { + name: "inactive, not slashable", + validator: ðpb.Validator{ + ActivationEpoch: 5, + WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, + }, + epoch: 2, + slashable: false, + }, + { + name: "after withdrawable, not slashable", + validator: ðpb.Validator{ + WithdrawableEpoch: 3, + }, + epoch: 3, + slashable: false, + }, + { + name: "slashed and withdrawable, not slashable", + validator: ðpb.Validator{ + Slashed: true, + ExitEpoch: params.BeaconConfig().FarFutureEpoch, + WithdrawableEpoch: 1, + }, + epoch: 2, + slashable: false, + }, + { + name: "slashed, not slashable", + validator: ðpb.Validator{ + Slashed: true, + ExitEpoch: params.BeaconConfig().FarFutureEpoch, + WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, + }, + epoch: 2, + slashable: false, + }, + { + name: "inactive and slashed, not slashable", + validator: ðpb.Validator{ + Slashed: true, + ActivationEpoch: 4, + ExitEpoch: params.BeaconConfig().FarFutureEpoch, + WithdrawableEpoch: params.BeaconConfig().FarFutureEpoch, + }, + epoch: 2, + slashable: false, + }, } - 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) + } + }) } } diff --git a/beacon-chain/rpc/beacon/validators.go b/beacon-chain/rpc/beacon/validators.go index b9f4e981a8af..cc9b43f37483 100644 --- a/beacon-chain/rpc/beacon/validators.go +++ b/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 diff --git a/beacon-chain/rpc/validator/exit.go b/beacon-chain/rpc/validator/exit.go index c0750beb92f1..7d75791877c8 100644 --- a/beacon-chain/rpc/validator/exit.go +++ b/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") } diff --git a/beacon-chain/state/getters.go b/beacon-chain/state/getters.go index 02b69318b31d..3b45b940a421 100644 --- a/beacon-chain/state/getters.go +++ b/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 } diff --git a/beacon-chain/sync/validate_voluntary_exit.go b/beacon-chain/sync/validate_voluntary_exit.go index fa7b164c36bd..31fd143956c0 100644 --- a/beacon-chain/sync/validate_voluntary_exit.go +++ b/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 }