From 63fe7bf675da59a9bb46bc1c61a9c5e8fb7dfa42 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 1 Dec 2023 17:54:31 +0400 Subject: [PATCH] types: validate Validator#Address field (#1715) * types: validate Validator#Address field * fix TestProposerSelection3 * add a changelog entry * fix two more tests * Update .changelog/unreleased/improvements/1715-validate-validator-address Co-authored-by: Thane Thomson --------- Co-authored-by: Thane Thomson --- .../improvements/1715-validate-validator-address | 1 + internal/state/store_test.go | 4 +--- internal/store/store_test.go | 2 +- types/validator.go | 5 +++-- types/validator_set_test.go | 14 +++++++++----- types/validator_test.go | 5 +++-- 6 files changed, 18 insertions(+), 13 deletions(-) create mode 100644 .changelog/unreleased/improvements/1715-validate-validator-address diff --git a/.changelog/unreleased/improvements/1715-validate-validator-address b/.changelog/unreleased/improvements/1715-validate-validator-address new file mode 100644 index 0000000000..ec7f2c7da6 --- /dev/null +++ b/.changelog/unreleased/improvements/1715-validate-validator-address @@ -0,0 +1 @@ +- `[types]` Validate `Validator#Address` in `ValidateBasic` ([\#1715](https://github.com/cometbft/cometbft/pull/1715)) diff --git a/internal/state/store_test.go b/internal/state/store_test.go index 690e5354a9..3e27d5ba6a 100644 --- a/internal/state/store_test.go +++ b/internal/state/store_test.go @@ -18,9 +18,7 @@ import ( abci "github.com/cometbft/cometbft/abci/types" cmtstate "github.com/cometbft/cometbft/api/cometbft/state/v1" - "github.com/cometbft/cometbft/crypto" "github.com/cometbft/cometbft/crypto/ed25519" - cmtrand "github.com/cometbft/cometbft/internal/rand" sm "github.com/cometbft/cometbft/internal/state" "github.com/cometbft/cometbft/internal/store" "github.com/cometbft/cometbft/internal/test" @@ -135,7 +133,7 @@ func TestPruneStates(t *testing.T) { // Generate a bunch of state data. Validators change for heights ending with 3, and // parameters when ending with 5. - validator := &types.Validator{Address: cmtrand.Bytes(crypto.AddressSize), VotingPower: 100, PubKey: pk} + validator := &types.Validator{Address: pk.Address(), VotingPower: 100, PubKey: pk} validatorSet := &types.ValidatorSet{ Validators: []*types.Validator{validator}, Proposer: validator, diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 9f41b918b1..8a13a50769 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -608,7 +608,7 @@ func TestPruningService(t *testing.T) { // Generate a bunch of state data. // This is needed because the pruning is expecting to load the state from the database thus // We have to have acceptable values for all fields of the state - validator := &types.Validator{Address: cmtrand.Bytes(crypto.AddressSize), VotingPower: 100, PubKey: pk} + validator := &types.Validator{Address: pk.Address(), VotingPower: 100, PubKey: pk} validatorSet := &types.ValidatorSet{ Validators: []*types.Validator{validator}, Proposer: validator, diff --git a/types/validator.go b/types/validator.go index 43e339c2ad..a1a3452e50 100644 --- a/types/validator.go +++ b/types/validator.go @@ -46,8 +46,9 @@ func (v *Validator) ValidateBasic() error { return errors.New("validator has negative voting power") } - if len(v.Address) != crypto.AddressSize { - return fmt.Errorf("validator address is the wrong size: %v", v.Address) + addr := v.PubKey.Address() + if !bytes.Equal(v.Address, addr) { + return fmt.Errorf("validator address is incorrectly derived from pubkey. Exp: %v, got %v", addr, v.Address) } return nil diff --git a/types/validator_set_test.go b/types/validator_set_test.go index 5ec4a9b972..0c8c135820 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -299,18 +299,22 @@ func TestProposerSelection2(t *testing.T) { } func TestProposerSelection3(t *testing.T) { - vset := NewValidatorSet([]*Validator{ + vals := []*Validator{ newValidator([]byte("avalidator_address12"), 1), newValidator([]byte("bvalidator_address12"), 1), newValidator([]byte("cvalidator_address12"), 1), newValidator([]byte("dvalidator_address12"), 1), - }) + } - proposerOrder := make([]*Validator, 4) for i := 0; i < 4; i++ { - // need to give all validators to have keys pk := ed25519.GenPrivKey().PubKey() - vset.Validators[i].PubKey = pk + vals[i].PubKey = pk + vals[i].Address = pk.Address() + } + sort.Sort(ValidatorsByAddress(vals)) + vset := NewValidatorSet(vals) + proposerOrder := make([]*Validator, 4) + for i := 0; i < 4; i++ { proposerOrder[i] = vset.GetProposer() vset.IncrementProposerPriority(1) } diff --git a/types/validator_test.go b/types/validator_test.go index 5eb2ed7bf1..954e8ec23b 100644 --- a/types/validator_test.go +++ b/types/validator_test.go @@ -1,6 +1,7 @@ package types import ( + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -74,7 +75,7 @@ func TestValidatorValidateBasic(t *testing.T) { Address: nil, }, err: true, - msg: "validator address is the wrong size: ", + msg: fmt.Sprintf("validator address is incorrectly derived from pubkey. Exp: %v, got ", pubKey.Address()), }, { val: &Validator{ @@ -82,7 +83,7 @@ func TestValidatorValidateBasic(t *testing.T) { Address: []byte{'a'}, }, err: true, - msg: "validator address is the wrong size: 61", + msg: fmt.Sprintf("validator address is incorrectly derived from pubkey. Exp: %v, got 61", pubKey.Address()), }, }