From dda8b67f3747538bb560508de22aeef8a2153532 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 4 Jul 2018 12:36:11 +0400 Subject: [PATCH 1/3] state: err if 0 power validator is added to the validator set Closes #1893 --- state/execution.go | 6 ++-- state/execution_test.go | 73 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/state/execution.go b/state/execution.go index 1c0af17a8..a36cbfb72 100644 --- a/state/execution.go +++ b/state/execution.go @@ -5,10 +5,10 @@ import ( fail "github.com/ebuchman/fail-test" abci "github.com/tendermint/tendermint/abci/types" - "github.com/tendermint/tendermint/proxy" - "github.com/tendermint/tendermint/types" dbm "github.com/tendermint/tendermint/libs/db" "github.com/tendermint/tendermint/libs/log" + "github.com/tendermint/tendermint/proxy" + "github.com/tendermint/tendermint/types" ) //----------------------------------------------------------------------------- @@ -280,7 +280,7 @@ func updateValidators(currentSet *types.ValidatorSet, abciUpdates []abci.Validat for _, valUpdate := range updates { address := valUpdate.Address _, val := currentSet.GetByAddress(address) - if val == nil { + if val == nil && valUpdate.VotingPower != 0 { // add val added := currentSet.Add(valUpdate) if !added { diff --git a/state/execution_test.go b/state/execution_test.go index 9c0635dcc..6b8a7e9e6 100644 --- a/state/execution_test.go +++ b/state/execution_test.go @@ -149,6 +149,79 @@ func TestBeginBlockByzantineValidators(t *testing.T) { } } +func TestUpdateValidators(t *testing.T) { + pubkey1 := crypto.GenPrivKeyEd25519().PubKey() + val1 := types.NewValidator(pubkey1, 10) + pubkey2 := crypto.GenPrivKeyEd25519().PubKey() + val2 := types.NewValidator(pubkey2, 20) + + testCases := []struct { + name string + + currentSet *types.ValidatorSet + abciUpdates []abci.Validator + + resultingSet *types.ValidatorSet + shouldErr bool + }{ + { + "adding a validator is OK", + + types.NewValidatorSet([]*types.Validator{val1}), + []abci.Validator{{[]byte{}, types.TM2PB.PubKey(pubkey2), 20}}, + + types.NewValidatorSet([]*types.Validator{val1, val2}), + false, + }, + { + "updating a validator is OK", + + types.NewValidatorSet([]*types.Validator{val1}), + []abci.Validator{{[]byte{}, types.TM2PB.PubKey(pubkey1), 20}}, + + types.NewValidatorSet([]*types.Validator{types.NewValidator(pubkey1, 20)}), + false, + }, + { + "removing a validator is OK", + + types.NewValidatorSet([]*types.Validator{val1, val2}), + []abci.Validator{{[]byte{}, types.TM2PB.PubKey(pubkey2), 0}}, + + types.NewValidatorSet([]*types.Validator{val1}), + false, + }, + + { + "removing a non-existing validator results in error", + + types.NewValidatorSet([]*types.Validator{val1}), + []abci.Validator{{[]byte{}, types.TM2PB.PubKey(pubkey2), 0}}, + + types.NewValidatorSet([]*types.Validator{val1}), + true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := updateValidators(tc.currentSet, tc.abciUpdates) + if tc.shouldErr { + assert.Error(t, err) + } else { + require.Equal(t, tc.resultingSet.Size(), tc.currentSet.Size()) + + assert.Equal(t, tc.resultingSet.TotalVotingPower(), tc.currentSet.TotalVotingPower()) + + assert.Equal(t, tc.resultingSet.Validators[0].Address, tc.currentSet.Validators[0].Address) + if tc.resultingSet.Size() > 1 { + assert.Equal(t, tc.resultingSet.Validators[1].Address, tc.currentSet.Validators[1].Address) + } + } + }) + } +} + //---------------------------------------------------------------------------- // make some bogus txs From c1aeb08e4bcebf8a8641cae054b67c5d9acc8c54 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 4 Jul 2018 13:21:29 +0400 Subject: [PATCH 2/3] return error if power is negative Refs #1893 --- state/execution.go | 4 ++++ state/execution_test.go | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/state/execution.go b/state/execution.go index a36cbfb72..941053316 100644 --- a/state/execution.go +++ b/state/execution.go @@ -278,6 +278,10 @@ func updateValidators(currentSet *types.ValidatorSet, abciUpdates []abci.Validat // these are tendermint types now for _, valUpdate := range updates { + if valUpdate.VotingPower < 0 { + return fmt.Errorf("Voting power can't be negative %v", valUpdate) + } + address := valUpdate.Address _, val := currentSet.GetByAddress(address) if val == nil && valUpdate.VotingPower != 0 { diff --git a/state/execution_test.go b/state/execution_test.go index 6b8a7e9e6..5e0072c30 100644 --- a/state/execution_test.go +++ b/state/execution_test.go @@ -201,6 +201,16 @@ func TestUpdateValidators(t *testing.T) { types.NewValidatorSet([]*types.Validator{val1}), true, }, + + { + "adding a validator with negative power results in error", + + types.NewValidatorSet([]*types.Validator{val1}), + []abci.Validator{{[]byte{}, types.TM2PB.PubKey(pubkey2), -100}}, + + types.NewValidatorSet([]*types.Validator{val1}), + true, + }, } for _, tc := range testCases { From 59f624043c5ea278cdf3690c63587706731ec572 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 4 Jul 2018 20:50:36 +0400 Subject: [PATCH 3/3] reorder statements --- state/execution.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/state/execution.go b/state/execution.go index 941053316..601abec9e 100644 --- a/state/execution.go +++ b/state/execution.go @@ -284,18 +284,18 @@ func updateValidators(currentSet *types.ValidatorSet, abciUpdates []abci.Validat address := valUpdate.Address _, val := currentSet.GetByAddress(address) - if val == nil && valUpdate.VotingPower != 0 { - // add val - added := currentSet.Add(valUpdate) - if !added { - return fmt.Errorf("Failed to add new validator %v", valUpdate) - } - } else if valUpdate.VotingPower == 0 { + if valUpdate.VotingPower == 0 { // remove val _, removed := currentSet.Remove(address) if !removed { return fmt.Errorf("Failed to remove validator %X", address) } + } else if val == nil { + // add val + added := currentSet.Add(valUpdate) + if !added { + return fmt.Errorf("Failed to add new validator %v", valUpdate) + } } else { // update val updated := currentSet.Update(valUpdate)