types: followup after validator set changes (#3301)

* fix failure in TestProposerFrequency

* Add test to check priority order after updates

* Changed applyRemovals() and removed Remove()

Changed applyRemovals() similar to applyUpdates()
Removed function Remove()
Updated comments

* review comments

* simplify applyRemovals and add more comments

* small correction in comment

* Fix check in test

* Fix priority check for centering, address review comments

* fix assert for priority centering

* review comments

* review comments

* cleanup and review comments

added upper limit check for validator voting power
moved check for empty validator set earlier
moved panic on potential negative set length in verifyRemovals
added more tests

* review comments

types/validator_set.go

@@ -2,6 +2,7 @@ package types
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"math"
 	"math/big"
@@ -93,7 +94,7 @@ func (vals *ValidatorSet) IncrementProposerPriority(times int) {
 	vals.shiftByAvgProposerPriority()
 
 	var proposer *Validator
-	// call IncrementProposerPriority(1) times times:
+	// Call IncrementProposerPriority(1) times times.
 	for i := 0; i < times; i++ {
 		proposer = vals.incrementProposerPriority()
 	}
@@ -117,9 +118,9 @@ func (vals *ValidatorSet) RescalePriorities(diffMax int64) {
 	// NOTE: This may make debugging priority issues easier as well.
 	diff := computeMaxMinPriorityDiff(vals)
 	ratio := (diff + diffMax - 1) / diffMax
-	if ratio > 1 {
+	if diff > diffMax {
 		for _, val := range vals.Validators {
-			val.ProposerPriority /= ratio
+			val.ProposerPriority = val.ProposerPriority / ratio
 		}
 	}
 }
@@ -130,15 +131,15 @@ func (vals *ValidatorSet) incrementProposerPriority() *Validator {
 		newPrio := safeAddClip(val.ProposerPriority, val.VotingPower)
 		val.ProposerPriority = newPrio
 	}
-	// Decrement the validator with most ProposerPriority:
+	// Decrement the validator with most ProposerPriority.
 	mostest := vals.getValWithMostPriority()
-	// mind underflow
+	// Mind the underflow.
 	mostest.ProposerPriority = safeSubClip(mostest.ProposerPriority, vals.TotalVotingPower())
 
 	return mostest
 }
 
-// should not be called on an empty validator set
+// Should not be called on an empty validator set.
 func (vals *ValidatorSet) computeAvgProposerPriority() int64 {
 	n := int64(len(vals.Validators))
 	sum := big.NewInt(0)
@@ -150,11 +151,11 @@ func (vals *ValidatorSet) computeAvgProposerPriority() int64 {
 		return avg.Int64()
 	}
 
-	// this should never happen: each val.ProposerPriority is in bounds of int64
+	// This should never happen: each val.ProposerPriority is in bounds of int64.
 	panic(fmt.Sprintf("Cannot represent avg ProposerPriority as an int64 %v", avg))
 }
 
-// compute the difference between the max and min ProposerPriority of that set
+// Compute the difference between the max and min ProposerPriority of that set.
 func computeMaxMinPriorityDiff(vals *ValidatorSet) int64 {
 	if vals.IsNilOrEmpty() {
 		panic("empty validator set")
@@ -195,7 +196,7 @@ func (vals *ValidatorSet) shiftByAvgProposerPriority() {
 	}
 }
 
-// Makes a copy of the validator list
+// Makes a copy of the validator list.
 func validatorListCopy(valsList []*Validator) []*Validator {
 	if valsList == nil {
 		return nil
@@ -207,7 +208,7 @@ func validatorListCopy(valsList []*Validator) []*Validator {
 	return valsCopy
 }
 
-// Copy each validator into a new ValidatorSet
+// Copy each validator into a new ValidatorSet.
 func (vals *ValidatorSet) Copy() *ValidatorSet {
 	return &ValidatorSet{
 		Validators:       validatorListCopy(vals.Validators),
@@ -253,21 +254,29 @@ func (vals *ValidatorSet) Size() int {
 	return len(vals.Validators)
 }
 
-// TotalVotingPower returns the sum of the voting powers of all validators.
+// Force recalculation of the set's total voting power.
-func (vals *ValidatorSet) TotalVotingPower() int64 {
+func (vals *ValidatorSet) updateTotalVotingPower() {
-	if vals.totalVotingPower == 0 {
+
-		sum := int64(0)
+	sum := int64(0)
-		for _, val := range vals.Validators {
+	for _, val := range vals.Validators {
-			// mind overflow
+		// mind overflow
-			sum = safeAddClip(sum, val.VotingPower)
+		sum = safeAddClip(sum, val.VotingPower)
-		}
 		if sum > MaxTotalVotingPower {
 			panic(fmt.Sprintf(
 				"Total voting power should be guarded to not exceed %v; got: %v",
 				MaxTotalVotingPower,
 				sum))
 		}
-		vals.totalVotingPower = sum
+	}
+
+	vals.totalVotingPower = sum
+}
+
+// TotalVotingPower returns the sum of the voting powers of all validators.
+// It recomputes the total voting power if required.
+func (vals *ValidatorSet) TotalVotingPower() int64 {
+	if vals.totalVotingPower == 0 {
+		vals.updateTotalVotingPower()
 	}
 	return vals.totalVotingPower
 }
@@ -307,27 +316,6 @@ func (vals *ValidatorSet) Hash() []byte {
 	return merkle.SimpleHashFromByteSlices(bzs)
 }
 
-// Remove deletes the validator with address. It returns the validator removed
-// and true. If returns nil and false if validator is not present in the set.
-func (vals *ValidatorSet) Remove(address []byte) (val *Validator, removed bool) {
-	idx := sort.Search(len(vals.Validators), func(i int) bool {
-		return bytes.Compare(address, vals.Validators[i].Address) <= 0
-	})
-	if idx >= len(vals.Validators) || !bytes.Equal(vals.Validators[idx].Address, address) {
-		return nil, false
-	}
-	removedVal := vals.Validators[idx]
-	newValidators := vals.Validators[:idx]
-	if idx+1 < len(vals.Validators) {
-		newValidators = append(newValidators, vals.Validators[idx+1:]...)
-	}
-	vals.Validators = newValidators
-	// Invalidate cache
-	vals.Proposer = nil
-	vals.totalVotingPower = 0
-	return removedVal, true
-}
-
 // Iterate will run the given function over the set.
 func (vals *ValidatorSet) Iterate(fn func(index int, val *Validator) bool) {
 	for i, val := range vals.Validators {
@@ -338,15 +326,15 @@ func (vals *ValidatorSet) Iterate(fn func(index int, val *Validator) bool) {
 	}
 }
 
-// Checks changes against duplicates, splits the changes in updates and removals, sorts them by address
+// Checks changes against duplicates, splits the changes in updates and removals, sorts them by address.
 //
 // Returns:
 // updates, removals - the sorted lists of updates and removals
 // err - non-nil if duplicate entries or entries with negative voting power are seen
 //
-// No changes are made to 'origChanges'
+// No changes are made to 'origChanges'.
 func processChanges(origChanges []*Validator) (updates, removals []*Validator, err error) {
-	// Make a deep copy of the changes and sort by address
+	// Make a deep copy of the changes and sort by address.
 	changes := validatorListCopy(origChanges)
 	sort.Sort(ValidatorsByAddress(changes))
 
@@ -354,14 +342,19 @@ func processChanges(origChanges []*Validator) (updates, removals []*Validator, e
 	updates = make([]*Validator, 0, len(changes))
 	var prevAddr Address
 
-	// Scan changes by address and append valid validators to updates or removals lists
+	// Scan changes by address and append valid validators to updates or removals lists.
 	for _, valUpdate := range changes {
 		if bytes.Equal(valUpdate.Address, prevAddr) {
 			err = fmt.Errorf("duplicate entry %v in %v", valUpdate, changes)
 			return nil, nil, err
 		}
 		if valUpdate.VotingPower < 0 {
-			err = fmt.Errorf("voting power can't be negative %v", valUpdate)
+			err = fmt.Errorf("voting power can't be negative: %v", valUpdate)
+			return nil, nil, err
+		}
+		if valUpdate.VotingPower > MaxTotalVotingPower {
+			err = fmt.Errorf("to prevent clipping/ overflow, voting power can't be higher than %v: %v ",
+				MaxTotalVotingPower, valUpdate)
 			return nil, nil, err
 		}
 		if valUpdate.VotingPower == 0 {
@@ -376,59 +369,49 @@ func processChanges(origChanges []*Validator) (updates, removals []*Validator, e
 
 // Verifies a list of updates against a validator set, making sure the allowed
 // total voting power would not be exceeded if these updates would be applied to the set.
-// It also computes the total voting power of the set that would result after the updates but
-// before the removals.
 //
 // Returns:
 // updatedTotalVotingPower - the new total voting power if these updates would be applied
+// numNewValidators - number of new validators
 // err - non-nil if the maximum allowed total voting power would be exceeded
 //
-// 'updates' should be a list of proper validator changes, i.e. they have been scanned
+// 'updates' should be a list of proper validator changes, i.e. they have been verified
 // by processChanges for duplicates and invalid values.
 // No changes are made to the validator set 'vals'.
-func verifyUpdates(updates []*Validator, vals *ValidatorSet) (updatedTotalVotingPower int64, err error) {
+func verifyUpdates(updates []*Validator, vals *ValidatorSet) (updatedTotalVotingPower int64, numNewValidators int, err error) {
 
-	// Scan the updates, compute new total voting power, check for overflow
 	updatedTotalVotingPower = vals.TotalVotingPower()
 
 	for _, valUpdate := range updates {
 		address := valUpdate.Address
 		_, val := vals.GetByAddress(address)
 		if val == nil {
-			// new validator, add its voting power the the total
+			// New validator, add its voting power the the total.
 			updatedTotalVotingPower += valUpdate.VotingPower
+			numNewValidators++
 		} else {
-			// updated validator, add the difference in power to the total
+			// Updated validator, add the difference in power to the total.
 			updatedTotalVotingPower += valUpdate.VotingPower - val.VotingPower
 		}
-
-		if updatedTotalVotingPower < 0 {
-			err = fmt.Errorf(
-				"failed to add/update validator with negative voting power %v",
-				valUpdate)
-			return 0, err
-		}
 		overflow := updatedTotalVotingPower > MaxTotalVotingPower
 		if overflow {
 			err = fmt.Errorf(
 				"failed to add/update validator %v, total voting power would exceed the max allowed %v",
 				valUpdate, MaxTotalVotingPower)
-			return 0, err
+			return 0, 0, err
 		}
 	}
 
-	return updatedTotalVotingPower, nil
+	return updatedTotalVotingPower, numNewValidators, nil
 }
 
-// Computes the proposer priority for the validators not present in the set based on 'updatedTotalVotingPower'
+// Computes the proposer priority for the validators not present in the set based on 'updatedTotalVotingPower'.
 // Leaves unchanged the priorities of validators that are changed.
 //
 // 'updates' parameter must be a list of unique validators to be added or updated.
 // No changes are made to the validator set 'vals'.
-func computeNewPriorities(updates []*Validator, vals *ValidatorSet, updatedTotalVotingPower int64) int {
+func computeNewPriorities(updates []*Validator, vals *ValidatorSet, updatedTotalVotingPower int64) {
 
-	numNew := 0
-	// Scan and update the proposerPriority for newly added and updated validators
 	for _, valUpdate := range updates {
 		address := valUpdate.Address
 		_, val := vals.GetByAddress(address)
@@ -442,13 +425,11 @@ func computeNewPriorities(updates []*Validator, vals *ValidatorSet, updatedTotal
 			//
 			// Compute ProposerPriority = -1.125*totalVotingPower == -(updatedVotingPower + (updatedVotingPower >> 3)).
 			valUpdate.ProposerPriority = -(updatedTotalVotingPower + (updatedTotalVotingPower >> 3))
-			numNew++
 		} else {
 			valUpdate.ProposerPriority = val.ProposerPriority
 		}
 	}
 
-	return numNew
 }
 
 // Merges the vals' validator list with the updates list.
@@ -457,20 +438,19 @@ func computeNewPriorities(updates []*Validator, vals *ValidatorSet, updatedTotal
 // must have been validated with verifyUpdates() and priorities computed with computeNewPriorities().
 func (vals *ValidatorSet) applyUpdates(updates []*Validator) {
 
-	existing := make([]*Validator, len(vals.Validators))
+	existing := vals.Validators
-	copy(existing, vals.Validators)
-
 	merged := make([]*Validator, len(existing)+len(updates))
 	i := 0
 
 	for len(existing) > 0 && len(updates) > 0 {
-		if bytes.Compare(existing[0].Address, updates[0].Address) < 0 {
+		if bytes.Compare(existing[0].Address, updates[0].Address) < 0 { // unchanged validator
 			merged[i] = existing[0]
 			existing = existing[1:]
 		} else {
+			// Apply add or update.
 			merged[i] = updates[0]
 			if bytes.Equal(existing[0].Address, updates[0].Address) {
-				// validator present in both, advance existing
+				// Validator is present in both, advance existing.
 				existing = existing[1:]
 			}
 			updates = updates[1:]
@@ -478,18 +458,18 @@ func (vals *ValidatorSet) applyUpdates(updates []*Validator) {
 		i++
 	}
 
+	// Add the elements which are left.
 	for j := 0; j < len(existing); j++ {
 		merged[i] = existing[j]
 		i++
 	}
-
+	// OR add updates which are left.
 	for j := 0; j < len(updates); j++ {
 		merged[i] = updates[j]
 		i++
 	}
 
 	vals.Validators = merged[:i]
-	vals.totalVotingPower = 0
 }
 
 // Checks that the validators to be removed are part of the validator set.
@@ -503,6 +483,9 @@ func verifyRemovals(deletes []*Validator, vals *ValidatorSet) error {
 			return fmt.Errorf("failed to find validator %X to remove", address)
 		}
 	}
+	if len(deletes) > len(vals.Validators) {
+		panic("more deletes than validators")
+	}
 	return nil
 }
 
@@ -510,50 +493,49 @@ func verifyRemovals(deletes []*Validator, vals *ValidatorSet) error {
 // Should not fail as verification has been done before.
 func (vals *ValidatorSet) applyRemovals(deletes []*Validator) {
 
-	for _, valUpdate := range deletes {
+	existing := vals.Validators
-		address := valUpdate.Address
+
-		_, removed := vals.Remove(address)
+	merged := make([]*Validator, len(existing)-len(deletes))
-		if !removed {
+	i := 0
-			// Should never happen
+
-			panic(fmt.Sprintf("failed to remove validator %X", address))
+	// Loop over deletes until we removed all of them.
+	for len(deletes) > 0 {
+		if bytes.Equal(existing[0].Address, deletes[0].Address) {
+			deletes = deletes[1:]
+		} else { // Leave it in the resulting slice.
+			merged[i] = existing[0]
+			i++
 		}
+		existing = existing[1:]
 	}
+
+	// Add the elements which are left.
+	for j := 0; j < len(existing); j++ {
+		merged[i] = existing[j]
+		i++
+	}
+
+	vals.Validators = merged[:i]
 }
 
-// UpdateWithChangeSet attempts to update the validator set with 'changes'
+// Main function used by UpdateWithChangeSet() and NewValidatorSet().
-// It performs the following steps:
+// If 'allowDeletes' is false then delete operations (identified by validators with voting power 0)
-// - validates the changes making sure there are no duplicates and splits them in updates and deletes
+// are not allowed and will trigger an error if present in 'changes'.
-// - verifies that applying the changes will not result in errors
+// The 'allowDeletes' flag is set to false by NewValidatorSet() and to true by UpdateWithChangeSet().
-// - computes the total voting power BEFORE removals to ensure that in the next steps the relative priorities
-//   across old and newly added validators is fair
-// - computes the priorities of new validators against the final set
-// - applies the updates against the validator set
-// - applies the removals against the validator set
-// - performs scaling and centering of priority values
-// If error is detected during verification steps it is returned and the validator set
-// is not changed.
-func (vals *ValidatorSet) UpdateWithChangeSet(changes []*Validator) error {
-	return vals.updateWithChangeSet(changes, true)
-}
-
-// main function used by UpdateWithChangeSet() and NewValidatorSet()
-// If 'allowDeletes' is false then delete operations are not allowed and must be reported if
-// present in 'changes'
 func (vals *ValidatorSet) updateWithChangeSet(changes []*Validator, allowDeletes bool) error {
 
 	if len(changes) <= 0 {
 		return nil
 	}
 
-	// Check for duplicates within changes, split in 'updates' and 'deletes' lists (sorted)
+	// Check for duplicates within changes, split in 'updates' and 'deletes' lists (sorted).
 	updates, deletes, err := processChanges(changes)
 	if err != nil {
 		return err
 	}
 
 	if !allowDeletes && len(deletes) != 0 {
-		err = fmt.Errorf("cannot process validators with voting power 0: %v", deletes)
+		return fmt.Errorf("cannot process validators with voting power 0: %v", deletes)
-		return err
 	}
 
 	// Verify that applying the 'deletes' against 'vals' will not result in error.
@@ -562,29 +544,48 @@ func (vals *ValidatorSet) updateWithChangeSet(changes []*Validator, allowDeletes
 	}
 
 	// Verify that applying the 'updates' against 'vals' will not result in error.
-	updatedTotalVotingPower, err := verifyUpdates(updates, vals)
+	updatedTotalVotingPower, numNewValidators, err := verifyUpdates(updates, vals)
 	if err != nil {
 		return err
 	}
 
-	// Compute the priorities for updates
+	// Check that the resulting set will not be empty.
-	numNewValidators := computeNewPriorities(updates, vals, updatedTotalVotingPower)
+	if numNewValidators == 0 && len(vals.Validators) == len(deletes) {
-	if len(vals.Validators)+numNewValidators <= len(deletes) {
+		return errors.New("applying the validator changes would result in empty set")
-		err = fmt.Errorf("applying the validator changes would result in empty set")
-		return err
 	}
 
-	// Apply updates and removals
+	// Compute the priorities for updates.
+	computeNewPriorities(updates, vals, updatedTotalVotingPower)
+
+	// Apply updates and removals.
 	vals.applyUpdates(updates)
 	vals.applyRemovals(deletes)
 
-	// Scale and center
+	vals.updateTotalVotingPower()
+
+	// Scale and center.
 	vals.RescalePriorities(PriorityWindowSizeFactor * vals.TotalVotingPower())
 	vals.shiftByAvgProposerPriority()
 
 	return nil
 }
 
+// UpdateWithChangeSet attempts to update the validator set with 'changes'.
+// It performs the following steps:
+// - validates the changes making sure there are no duplicates and splits them in updates and deletes
+// - verifies that applying the changes will not result in errors
+// - computes the total voting power BEFORE removals to ensure that in the next steps the priorities
+//   across old and newly added validators are fair
+// - computes the priorities of new validators against the final set
+// - applies the updates against the validator set
+// - applies the removals against the validator set
+// - performs scaling and centering of priority values
+// If an error is detected during verification steps, it is returned and the validator set
+// is not changed.
+func (vals *ValidatorSet) UpdateWithChangeSet(changes []*Validator) error {
+	return vals.updateWithChangeSet(changes, true)
+}
+
 // Verify that +2/3 of the set had signed the given signBytes.
 func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, height int64, commit *Commit) error {
 
@@ -768,7 +769,7 @@ func (vals *ValidatorSet) StringIndented(indent string) string {
 //-------------------------------------
 // Implements sort for sorting validators by address.
 
-// Sort validators by address
+// Sort validators by address.
 type ValidatorsByAddress []*Validator
 
 func (valz ValidatorsByAddress) Len() int {
@@ -786,7 +787,7 @@ func (valz ValidatorsByAddress) Swap(i, j int) {
 }
 
 //----------------------------------------
-// For testing
+// for testing
 
 // RandValidatorSet returns a randomized validator set, useful for testing.
 // NOTE: PrivValidator are in order.
@@ -805,7 +806,7 @@ func RandValidatorSet(numValidators int, votingPower int64) (*ValidatorSet, []Pr
 }
 
 ///////////////////////////////////////////////////////////////////////////////
-// Safe addition/subtraction
+// safe addition/subtraction
 
 func safeAdd(a, b int64) (int64, bool) {
 	if b > 0 && a > math.MaxInt64-b {

types/validator_set_test.go

@@ -4,7 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"math"
-	"math/rand"
+	"sort"
 	"strings"
 	"testing"
 	"testing/quick"
@@ -72,13 +72,6 @@ func TestValidatorSetBasic(t *testing.T) {
 	_, val = vset.GetByAddress(val.Address)
 	assert.Equal(t, proposerPriority, val.ProposerPriority)
 
-	// remove
-	val2, removed := vset.Remove(randValidator_(vset.TotalVotingPower()).Address)
-	assert.Nil(t, val2)
-	assert.False(t, removed)
-	val2, removed = vset.Remove(val.Address)
-	assert.Equal(t, val.Address, val2.Address)
-	assert.True(t, removed)
 }
 
 func TestCopy(t *testing.T) {
@@ -658,6 +651,71 @@ type testVal struct {
 	power int64
 }
 
+func permutation(valList []testVal) []testVal {
+	if len(valList) == 0 {
+		return nil
+	}
+	permList := make([]testVal, len(valList))
+	perm := cmn.RandPerm(len(valList))
+	for i, v := range perm {
+		permList[v] = valList[i]
+	}
+	return permList
+}
+
+func createNewValidatorList(testValList []testVal) []*Validator {
+	valList := make([]*Validator, 0, len(testValList))
+	for _, val := range testValList {
+		valList = append(valList, newValidator([]byte(val.name), val.power))
+	}
+	return valList
+}
+
+func createNewValidatorSet(testValList []testVal) *ValidatorSet {
+	return NewValidatorSet(createNewValidatorList(testValList))
+}
+
+func valSetTotalProposerPriority(valSet *ValidatorSet) int64 {
+	sum := int64(0)
+	for _, val := range valSet.Validators {
+		// mind overflow
+		sum = safeAddClip(sum, val.ProposerPriority)
+	}
+	return sum
+}
+
+func verifyValidatorSet(t *testing.T, valSet *ValidatorSet) {
+	// verify that the capacity and length of validators is the same
+	assert.Equal(t, len(valSet.Validators), cap(valSet.Validators))
+
+	// verify that the set's total voting power has been updated
+	tvp := valSet.totalVotingPower
+	valSet.updateTotalVotingPower()
+	expectedTvp := valSet.TotalVotingPower()
+	assert.Equal(t, expectedTvp, tvp,
+		"expected TVP %d. Got %d, valSet=%s", expectedTvp, tvp, valSet)
+
+	// verify that validator priorities are centered
+	valsCount := int64(len(valSet.Validators))
+	tpp := valSetTotalProposerPriority(valSet)
+	assert.True(t, tpp < valsCount && tpp > -valsCount,
+		"expected total priority in (-%d, %d). Got %d", valsCount, valsCount, tpp)
+
+	// verify that priorities are scaled
+	dist := computeMaxMinPriorityDiff(valSet)
+	assert.True(t, dist <= PriorityWindowSizeFactor*tvp,
+		"expected priority distance < %d. Got %d", PriorityWindowSizeFactor*tvp, dist)
+}
+
+func toTestValList(valList []*Validator) []testVal {
+	testList := make([]testVal, len(valList))
+	for i, val := range valList {
+		testList[i].name = string(val.Address)
+		testList[i].power = val.VotingPower
+	}
+	return testList
+}
+
 func testValSet(nVals int, power int64) []testVal {
 	vals := make([]testVal, nVals)
 	for i := 0; i < nVals; i++ {
@@ -755,6 +813,10 @@ func TestValSetUpdatesOverflows(t *testing.T) {
 			testValSet(2, 10),
 			[]testVal{{"v2", math.MaxInt64}},
 		},
+		{ // add validator leading to overflow
+			testValSet(1, maxVP),
+			[]testVal{{"v2", math.MaxInt64}},
+		},
 		{ // add validator leading to exceed Max
 			testValSet(1, maxVP-1),
 			[]testVal{{"v2", 5}},
@@ -763,6 +825,10 @@ func TestValSetUpdatesOverflows(t *testing.T) {
 			testValSet(2, maxVP/3),
 			[]testVal{{"v3", maxVP / 2}},
 		},
+		{ // add validator leading to exceed Max
+			testValSet(1, maxVP),
+			[]testVal{{"v2", maxVP}},
+		},
 	}
 
 	for i, tt := range testCases {
@@ -837,30 +903,27 @@ func TestValSetUpdatesBasicTestsExecute(t *testing.T) {
 		// create a new set and apply updates, keeping copies for the checks
 		valSet := createNewValidatorSet(tt.startVals)
 		valList := createNewValidatorList(tt.updateVals)
-		valListCopy := validatorListCopy(valList)
 		err := valSet.UpdateWithChangeSet(valList)
 		assert.NoError(t, err, "test %d", i)
 
-		// check the parameter list has not changed
+		valListCopy := validatorListCopy(valSet.Validators)
-		assert.Equal(t, valList, valListCopy, "test %v", i)
+		// check that the voting power in the set's validators is not changing if the voting power
+		// is changed in the list of validators previously passed as parameter to UpdateWithChangeSet.
+		// this is to make sure copies of the validators are made by UpdateWithChangeSet.
+		if len(valList) > 0 {
+			valList[0].VotingPower++
+			assert.Equal(t, toTestValList(valListCopy), toTestValList(valSet.Validators), "test %v", i)
+
+		}
 
 		// check the final validator list is as expected and the set is properly scaled and centered.
-		assert.Equal(t, getValidatorResults(valSet.Validators), tt.expectedVals, "test %v", i)
+		assert.Equal(t, tt.expectedVals, toTestValList(valSet.Validators), "test %v", i)
 		verifyValidatorSet(t, valSet)
 	}
 }
 
-func getValidatorResults(valList []*Validator) []testVal {
-	testList := make([]testVal, len(valList))
-	for i, val := range valList {
-		testList[i].name = string(val.Address)
-		testList[i].power = val.VotingPower
-	}
-	return testList
-}
-
 // Test that different permutations of an update give the same result.
-func TestValSetUpdatesOrderTestsExecute(t *testing.T) {
+func TestValSetUpdatesOrderIndependenceTestsExecute(t *testing.T) {
 	// startVals - initial validators to create the set with
 	// updateVals - a sequence of updates to be applied to the set.
 	// updateVals is shuffled a number of times during testing to check for same resulting validator set.
@@ -973,55 +1036,242 @@ func TestValSetApplyUpdatesTestsExecute(t *testing.T) {
 		valSet.applyUpdates(valList)
 
 		// check the new list of validators for proper merge
-		assert.Equal(t, getValidatorResults(valSet.Validators), tt.expectedVals, "test %v", i)
+		assert.Equal(t, toTestValList(valSet.Validators), tt.expectedVals, "test %v", i)
+	}
+}
+
+type testVSetCfg struct {
+	startVals    []testVal
+	deletedVals  []testVal
+	updatedVals  []testVal
+	addedVals    []testVal
+	expectedVals []testVal
+}
+
+func randTestVSetCfg(t *testing.T, nBase, nAddMax int) testVSetCfg {
+	if nBase <= 0 || nAddMax < 0 {
+		panic(fmt.Sprintf("bad parameters %v %v", nBase, nAddMax))
+	}
+
+	const maxPower = 1000
+	var nOld, nDel, nChanged, nAdd int
+
+	nOld = int(cmn.RandUint()%uint(nBase)) + 1
+	if nBase-nOld > 0 {
+		nDel = int(cmn.RandUint() % uint(nBase-nOld))
+	}
+	nChanged = nBase - nOld - nDel
+
+	if nAddMax > 0 {
+		nAdd = cmn.RandInt()%nAddMax + 1
+	}
+
+	cfg := testVSetCfg{}
+
+	cfg.startVals = make([]testVal, nBase)
+	cfg.deletedVals = make([]testVal, nDel)
+	cfg.addedVals = make([]testVal, nAdd)
+	cfg.updatedVals = make([]testVal, nChanged)
+	cfg.expectedVals = make([]testVal, nBase-nDel+nAdd)
+
+	for i := 0; i < nBase; i++ {
+		cfg.startVals[i] = testVal{fmt.Sprintf("v%d", i), int64(cmn.RandUint()%maxPower + 1)}
+		if i < nOld {
+			cfg.expectedVals[i] = cfg.startVals[i]
+		}
+		if i >= nOld && i < nOld+nChanged {
+			cfg.updatedVals[i-nOld] = testVal{fmt.Sprintf("v%d", i), int64(cmn.RandUint()%maxPower + 1)}
+			cfg.expectedVals[i] = cfg.updatedVals[i-nOld]
+		}
+		if i >= nOld+nChanged {
+			cfg.deletedVals[i-nOld-nChanged] = testVal{fmt.Sprintf("v%d", i), 0}
+		}
+	}
+
+	for i := nBase; i < nBase+nAdd; i++ {
+		cfg.addedVals[i-nBase] = testVal{fmt.Sprintf("v%d", i), int64(cmn.RandUint()%maxPower + 1)}
+		cfg.expectedVals[i-nDel] = cfg.addedVals[i-nBase]
+	}
+
+	sort.Sort(testValsByAddress(cfg.startVals))
+	sort.Sort(testValsByAddress(cfg.deletedVals))
+	sort.Sort(testValsByAddress(cfg.updatedVals))
+	sort.Sort(testValsByAddress(cfg.addedVals))
+	sort.Sort(testValsByAddress(cfg.expectedVals))
+
+	return cfg
+
+}
+
+func applyChangesToValSet(t *testing.T, valSet *ValidatorSet, valsLists ...[]testVal) {
+	changes := make([]testVal, 0)
+	for _, valsList := range valsLists {
+		changes = append(changes, valsList...)
+	}
+	valList := createNewValidatorList(changes)
+	err := valSet.UpdateWithChangeSet(valList)
+	assert.NoError(t, err)
+}
+
+func isAddressInList(address []byte, valsList []testVal) bool {
+	for _, val := range valsList {
+		if bytes.Equal([]byte(val.name), address) {
+			return true
+		}
+	}
+	return false
+}
+
+func TestValSetUpdatePriorityOrderTests(t *testing.T) {
+	const nMaxElections = 5000
+
+	testCases := []testVSetCfg{
+		0: { // remove high power validator, keep old equal lower power validators
+			startVals:    []testVal{{"v1", 1}, {"v2", 1}, {"v3", 1000}},
+			deletedVals:  []testVal{{"v3", 0}},
+			updatedVals:  []testVal{},
+			addedVals:    []testVal{},
+			expectedVals: []testVal{{"v1", 1}, {"v2", 1}},
+		},
+		1: { // remove high power validator, keep old different power validators
+			startVals:    []testVal{{"v1", 1}, {"v2", 10}, {"v3", 1000}},
+			deletedVals:  []testVal{{"v3", 0}},
+			updatedVals:  []testVal{},
+			addedVals:    []testVal{},
+			expectedVals: []testVal{{"v1", 1}, {"v2", 10}},
+		},
+		2: { // remove high power validator, add new low power validators, keep old lower power
+			startVals:    []testVal{{"v1", 1}, {"v2", 2}, {"v3", 1000}},
+			deletedVals:  []testVal{{"v3", 0}},
+			updatedVals:  []testVal{{"v2", 1}},
+			addedVals:    []testVal{{"v4", 40}, {"v5", 50}},
+			expectedVals: []testVal{{"v1", 1}, {"v2", 1}, {"v4", 40}, {"v5", 50}},
+		},
+
+		// generate a configuration with 100 validators,
+		// randomly select validators for updates and deletes, and
+		// generate 10 new validators to be added
+		3: randTestVSetCfg(t, 100, 10),
+
+		4: randTestVSetCfg(t, 1000, 100),
+
+		5: randTestVSetCfg(t, 10, 100),
+
+		6: randTestVSetCfg(t, 100, 1000),
+
+		7: randTestVSetCfg(t, 1000, 1000),
+
+		8: randTestVSetCfg(t, 10000, 1000),
+
+		9: randTestVSetCfg(t, 1000, 10000),
+	}
+
+	for _, cfg := range testCases {
+
+		// create a new validator set
+		valSet := createNewValidatorSet(cfg.startVals)
 		verifyValidatorSet(t, valSet)
+
+		// run election up to nMaxElections times, apply changes and verify that the priority order is correct
+		verifyValSetUpdatePriorityOrder(t, valSet, cfg, nMaxElections)
 	}
 }
 
-func permutation(valList []testVal) []testVal {
+func verifyValSetUpdatePriorityOrder(t *testing.T, valSet *ValidatorSet, cfg testVSetCfg, nMaxElections int) {
-	if len(valList) == 0 {
+
-		return nil
+	// Run election up to nMaxElections times, sort validators by priorities
+	valSet.IncrementProposerPriority(cmn.RandInt()%nMaxElections + 1)
+	origValsPriSorted := validatorListCopy(valSet.Validators)
+	sort.Sort(validatorsByPriority(origValsPriSorted))
+
+	// apply the changes, get the updated validators, sort by priorities
+	applyChangesToValSet(t, valSet, cfg.addedVals, cfg.updatedVals, cfg.deletedVals)
+	updatedValsPriSorted := validatorListCopy(valSet.Validators)
+	sort.Sort(validatorsByPriority(updatedValsPriSorted))
+
+	// basic checks
+	assert.Equal(t, toTestValList(valSet.Validators), cfg.expectedVals)
+	verifyValidatorSet(t, valSet)
+
+	// verify that the added validators have the smallest priority:
+	//  - they should be at the beginning of valListNewPriority since it is sorted by priority
+	if len(cfg.addedVals) > 0 {
+		addedValsPriSlice := updatedValsPriSorted[:len(cfg.addedVals)]
+		sort.Sort(ValidatorsByAddress(addedValsPriSlice))
+		assert.Equal(t, cfg.addedVals, toTestValList(addedValsPriSlice))
+
+		//  - and should all have the same priority
+		expectedPri := addedValsPriSlice[0].ProposerPriority
+		for _, val := range addedValsPriSlice[1:] {
+			assert.Equal(t, expectedPri, val.ProposerPriority)
+		}
 	}
-	permList := make([]testVal, len(valList))
+
-	perm := rand.Perm(len(valList))
+	// check that the priority order for validators that remained is the same
-	for i, v := range perm {
+	// as in the original set
-		permList[v] = valList[i]
+	remainingValsPriSlice := updatedValsPriSorted[len(cfg.addedVals):]
+
+	for len(remainingValsPriSlice) > 0 {
+		addressInChanged := remainingValsPriSlice[0].Address
+		addressInOld := origValsPriSorted[0].Address
+
+		// skip validators in original list that have been removed
+		if isAddressInList(addressInOld, cfg.deletedVals) {
+			origValsPriSorted = origValsPriSorted[1:]
+			continue
+		}
+		assert.Equal(t, addressInOld, addressInChanged, "wrong priority order")
+
+		remainingValsPriSlice = remainingValsPriSlice[1:]
+		origValsPriSorted = origValsPriSorted[1:]
 	}
-	return permList
 }
 
-func createNewValidatorList(testValList []testVal) []*Validator {
+//---------------------
-	valList := make([]*Validator, 0, len(testValList))
+// Sort validators by priority and address
-	for _, val := range testValList {
+type validatorsByPriority []*Validator
-		valList = append(valList, newValidator([]byte(val.name), val.power))
+
+func (valz validatorsByPriority) Len() int {
+	return len(valz)
+}
+
+func (valz validatorsByPriority) Less(i, j int) bool {
+	if valz[i].ProposerPriority < valz[j].ProposerPriority {
+		return true
 	}
-	return valList
+	if valz[i].ProposerPriority > valz[j].ProposerPriority {
+		return false
+	}
+	return bytes.Compare(valz[i].Address, valz[j].Address) < 0
 }
 
-func createNewValidatorSet(testValList []testVal) *ValidatorSet {
+func (valz validatorsByPriority) Swap(i, j int) {
-	valList := createNewValidatorList(testValList)
+	it := valz[i]
-	valSet := NewValidatorSet(valList)
+	valz[i] = valz[j]
-	return valSet
+	valz[j] = it
 }
 
-func verifyValidatorSet(t *testing.T, valSet *ValidatorSet) {
+//-------------------------------------
-	// verify that the vals' tvp is set to the sum of the all vals voting powers
+// Sort testVal-s by address.
-	tvp := valSet.TotalVotingPower()
+type testValsByAddress []testVal
-	assert.Equal(t, valSet.totalVotingPower, tvp,
-		"expected TVP %d. Got %d, valSet=%s", tvp, valSet.totalVotingPower, valSet)
 
-	// verify that validator priorities are centered
+func (tvals testValsByAddress) Len() int {
-	l := int64(len(valSet.Validators))
+	return len(tvals)
-	tpp := valSet.TotalVotingPower()
-	assert.True(t, tpp <= l || tpp >= -l,
-		"expected total priority in (-%d, %d). Got %d", l, l, tpp)
-
-	// verify that priorities are scaled
-	dist := computeMaxMinPriorityDiff(valSet)
-	assert.True(t, dist <= PriorityWindowSizeFactor*tvp,
-		"expected priority distance < %d. Got %d", PriorityWindowSizeFactor*tvp, dist)
 }
 
+func (tvals testValsByAddress) Less(i, j int) bool {
+	return bytes.Compare([]byte(tvals[i].name), []byte(tvals[j].name)) == -1
+}
+
+func (tvals testValsByAddress) Swap(i, j int) {
+	it := tvals[i]
+	tvals[i] = tvals[j]
+	tvals[j] = it
+}
+
+//-------------------------------------
+// Benchmark tests
+//
 func BenchmarkUpdates(b *testing.B) {
 	const (
 		n = 100