Merge PR #1565: Validator Cliff Updates

CHANGELOG.md

@@ -118,6 +118,9 @@ BUG FIXES
 * \#887  - limit the size of rationals that can be passed in from user input
 * \#1461 - CLI tests now no longer reset your local environment data
 * \#1505 - `gaiacli stake validator` no longer panics if validator doesn't exist
+* [x/stake] fix revoke bytes ordering (was putting revoked candidates at the top of the list)
+* [x/stake] bond count was counting revoked validators as bonded, fixed
+* \#1565 - fix cliff validator persisting when validator set shrinks from max
 
 ## 0.19.0
 

x/stake/handler_test.go

@@ -560,6 +560,181 @@ func TestTransitiveRedelegation(t *testing.T) {
 	require.True(t, got.IsOK(), "expected no error")
 }
 
+func TestUnbondingWhenExcessValidators(t *testing.T) {
+	ctx, _, keeper := keep.CreateTestInput(t, false, 1000)
+	validatorAddr1, validatorAddr2, validatorAddr3 := keep.Addrs[0], keep.Addrs[1], keep.Addrs[2]
+
+	// set the unbonding time
+	params := keeper.GetParams(ctx)
+	params.UnbondingTime = 0
+	params.MaxValidators = 2
+	keeper.SetParams(ctx, params)
+
+	// add three validators
+	msgCreateValidator := newTestMsgCreateValidator(validatorAddr1, keep.PKs[0], 50)
+	got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+	require.Equal(t, 1, len(keeper.GetValidatorsBonded(ctx)))
+
+	msgCreateValidator = newTestMsgCreateValidator(validatorAddr2, keep.PKs[1], 30)
+	got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+	require.Equal(t, 2, len(keeper.GetValidatorsBonded(ctx)))
+
+	msgCreateValidator = newTestMsgCreateValidator(validatorAddr3, keep.PKs[2], 10)
+	got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+	require.Equal(t, 2, len(keeper.GetValidatorsBonded(ctx)))
+
+	// unbond the valdator-2
+	msgBeginUnbonding := NewMsgBeginUnbonding(validatorAddr2, validatorAddr2, sdk.NewRat(30))
+	got = handleMsgBeginUnbonding(ctx, msgBeginUnbonding, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgBeginUnbonding")
+
+	// because there are extra validators waiting to get in, the queued
+	// validator (aka. validator-1) should make it into the bonded group, thus
+	// the total number of validators should stay the same
+	vals := keeper.GetValidatorsBonded(ctx)
+	require.Equal(t, 2, len(vals), "vals %v", vals)
+	val1, found := keeper.GetValidator(ctx, validatorAddr1)
+	require.True(t, found)
+	require.Equal(t, sdk.Bonded, val1.Status(), "%v", val1)
+}
+
+func TestJoiningAsCliffValidator(t *testing.T) {
+	ctx, _, keeper := keep.CreateTestInput(t, false, 1000)
+	validatorAddr1, validatorAddr2 := keep.Addrs[0], keep.Addrs[1]
+
+	// make sure that the cliff validator is nil to begin with
+	cliffVal := keeper.GetCliffValidator(ctx)
+	require.Equal(t, []byte(nil), cliffVal)
+
+	// set the unbonding time
+	params := keeper.GetParams(ctx)
+	params.UnbondingTime = 0
+	params.MaxValidators = 2
+	keeper.SetParams(ctx, params)
+
+	// add the first validator
+	msgCreateValidator := newTestMsgCreateValidator(validatorAddr1, keep.PKs[0], 50)
+	got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+
+	// cliff validator should still be nil
+	cliffVal = keeper.GetCliffValidator(ctx)
+	require.Equal(t, []byte(nil), cliffVal)
+
+	// Add the second validator
+	msgCreateValidator = newTestMsgCreateValidator(validatorAddr2, keep.PKs[1], 30)
+	got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+
+	// now that we've reached maximum validators, the val-2 should be added to the cliff (top)
+	cliffVal = keeper.GetCliffValidator(ctx)
+	require.Equal(t, validatorAddr2.Bytes(), cliffVal)
+}
+
+func TestJoiningToCreateFirstCliffValidator(t *testing.T) {
+	ctx, _, keeper := keep.CreateTestInput(t, false, 1000)
+	validatorAddr1, validatorAddr2 := keep.Addrs[0], keep.Addrs[1]
+
+	// make sure that the cliff validator is nil to begin with
+	cliffVal := keeper.GetCliffValidator(ctx)
+	require.Equal(t, []byte(nil), cliffVal)
+
+	// set the unbonding time
+	params := keeper.GetParams(ctx)
+	params.UnbondingTime = 0
+	params.MaxValidators = 2
+	keeper.SetParams(ctx, params)
+
+	// add the first validator
+	msgCreateValidator := newTestMsgCreateValidator(validatorAddr1, keep.PKs[0], 50)
+	got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+
+	// cliff validator should still be nil
+	cliffVal = keeper.GetCliffValidator(ctx)
+	require.Equal(t, []byte(nil), cliffVal)
+
+	// Add the second validator
+	msgCreateValidator = newTestMsgCreateValidator(validatorAddr2, keep.PKs[1], 60)
+	got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+
+	// now that we've reached maximum validators, validator-1 should be added to the cliff (top)
+	cliffVal = keeper.GetCliffValidator(ctx)
+	require.Equal(t, validatorAddr1.Bytes(), cliffVal)
+}
+
+func TestCliffValidator(t *testing.T) {
+	ctx, _, keeper := keep.CreateTestInput(t, false, 1000)
+	validatorAddr1, validatorAddr2, validatorAddr3 := keep.Addrs[0], keep.Addrs[1], keep.Addrs[2]
+
+	// make sure that the cliff validator is nil to begin with
+	cliffVal := keeper.GetCliffValidator(ctx)
+	require.Equal(t, []byte(nil), cliffVal)
+
+	// set the unbonding time
+	params := keeper.GetParams(ctx)
+	params.UnbondingTime = 0
+	params.MaxValidators = 2
+	keeper.SetParams(ctx, params)
+
+	// add the first validator
+	msgCreateValidator := newTestMsgCreateValidator(validatorAddr1, keep.PKs[0], 50)
+	got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+
+	// cliff validator should still be nil
+	cliffVal = keeper.GetCliffValidator(ctx)
+	require.Equal(t, []byte(nil), cliffVal)
+
+	// Add the second validator
+	msgCreateValidator = newTestMsgCreateValidator(validatorAddr2, keep.PKs[1], 30)
+	got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+
+	// now that we've reached maximum validators, validator-2 should be added to the cliff (top)
+	cliffVal = keeper.GetCliffValidator(ctx)
+	require.Equal(t, validatorAddr2.Bytes(), cliffVal)
+
+	// add the third validator, which should not make it to being bonded,
+	// so the cliff validator should not change because nobody has been kicked out
+	msgCreateValidator = newTestMsgCreateValidator(validatorAddr3, keep.PKs[2], 10)
+	got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")
+
+	cliffVal = keeper.GetCliffValidator(ctx)
+	require.Equal(t, validatorAddr2.Bytes(), cliffVal)
+
+	// unbond valdator-2
+	msgBeginUnbonding := NewMsgBeginUnbonding(validatorAddr2, validatorAddr2, sdk.NewRat(30))
+	got = handleMsgBeginUnbonding(ctx, msgBeginUnbonding, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgBeginUnbonding")
+
+	vals := keeper.GetValidatorsBonded(ctx)
+	require.Equal(t, 2, len(vals))
+
+	// now the validator set should be updated,
+	// where val-3 enters the validator set on the cliff
+	cliffVal = keeper.GetCliffValidator(ctx)
+	require.Equal(t, validatorAddr3.Bytes(), cliffVal)
+
+	// unbond valdator-1
+	msgBeginUnbonding = NewMsgBeginUnbonding(validatorAddr1, validatorAddr1, sdk.NewRat(50))
+	got = handleMsgBeginUnbonding(ctx, msgBeginUnbonding, keeper)
+	require.True(t, got.IsOK(), "expected no error on runMsgBeginUnbonding")
+
+	// get bonded validators - should just be one
+	vals = keeper.GetValidatorsBonded(ctx)
+	require.Equal(t, 1, len(vals))
+
+	// cliff now should be empty
+	cliffVal = keeper.GetCliffValidator(ctx)
+	require.Equal(t, []byte(nil), cliffVal)
+}
+
 func TestBondUnbondRedelegateSlashTwice(t *testing.T) {
 	ctx, _, keeper := keep.CreateTestInput(t, false, 1000)
 	valA, valB, del := keep.Addrs[0], keep.Addrs[1], keep.Addrs[2]

x/stake/keeper/key.go

@@ -66,16 +66,17 @@ func GetValidatorsByPowerIndexKey(validator types.Validator, pool types.Pool) []
 }
 
 // get the power ranking of a validator
+// NOTE the larger values are of higher value
 func getValidatorPowerRank(validator types.Validator, pool types.Pool) []byte {
 
 	power := validator.EquivalentBondedShares(pool)
 	powerBytes := []byte(power.ToLeftPadded(maxDigitsForAccount)) // power big-endian (more powerful validators first)
 
 	revokedBytes := make([]byte, 1)
 	if validator.Revoked {
-		revokedBytes[0] = byte(0x01)
-	} else {
 		revokedBytes[0] = byte(0x00)
+	} else {
+		revokedBytes[0] = byte(0x01)
 	}
 
 	// heightBytes and counterBytes represent strings like powerBytes does

x/stake/keeper/validator.go

@@ -249,7 +249,7 @@ func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) type
 
 	// efficiency case:
 	// if was unbonded/or is a new validator - and the new power is less than the cliff validator
-	cliffPower := k.getCliffValidatorPower(ctx)
+	cliffPower := k.GetCliffValidatorPower(ctx)
 	if cliffPower != nil &&
 		(!oldFound || (oldFound && oldValidator.Status() == sdk.Unbonded)) &&
 		bytes.Compare(valPower, cliffPower) == -1 { //(valPower < cliffPower
@@ -285,12 +285,12 @@ func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) type
 //
 // Optionally also return the validator from a retrieve address if the validator has been bonded
 func (k Keeper) UpdateBondedValidators(ctx sdk.Context,
-	newValidator types.Validator) (updatedVal types.Validator) {
+	affectedValidator types.Validator) (updatedVal types.Validator) {
 
 	store := ctx.KVStore(k.storeKey)
 
 	kickCliffValidator := false
-	oldCliffValidatorAddr := k.getCliffValidator(ctx)
+	oldCliffValidatorAddr := k.GetCliffValidator(ctx)
 
 	// add the actual validator power sorted store
 	maxValidators := k.GetParams(ctx).MaxValidators
@@ -303,6 +303,8 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context,
 			// TODO benchmark if we should read the current power and not write if it's the same
 			if bondedValidatorsCount == int(maxValidators) { // is cliff validator
 				k.setCliffValidator(ctx, validator, k.GetPool(ctx))
+			} else if len(oldCliffValidatorAddr) > 0 {
+				k.clearCliffValidator(ctx)
 			}
 			break
 		}
@@ -312,8 +314,8 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context,
 		// provided because it has not yet been updated in the main validator
 		// store
 		ownerAddr := iterator.Value()
-		if bytes.Equal(ownerAddr, newValidator.Owner) {
-			validator = newValidator
+		if bytes.Equal(ownerAddr, affectedValidator.Owner) {
+			validator = affectedValidator
 		} else {
 			var found bool
 			validator, found = k.GetValidator(ctx, ownerAddr)
@@ -328,15 +330,17 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context,
 			kickCliffValidator = true
 
 			validator = k.bondValidator(ctx, validator)
-			if bytes.Equal(ownerAddr, newValidator.Owner) {
+			if bytes.Equal(ownerAddr, affectedValidator.Owner) {
 				updatedVal = validator
 			}
 		}
 
-		if validator.Revoked && validator.Status() == sdk.Bonded {
-			panic(fmt.Sprintf("revoked validator cannot be bonded, address: %v\n", ownerAddr))
-		} else {
+		if !validator.Revoked {
 			bondedValidatorsCount++
+		} else {
+			if validator.Status() == sdk.Bonded {
+				panic(fmt.Sprintf("revoked validator cannot be bonded, address: %v\n", ownerAddr))
+			}
 		}
 
 		iterator.Next()
@@ -405,10 +409,12 @@ func (k Keeper) UpdateBondedValidatorsFull(ctx sdk.Context) {
 			validator = k.bondValidator(ctx, validator)
 		}
 
-		if validator.Revoked && validator.Status() == sdk.Bonded {
-			panic(fmt.Sprintf("revoked validator cannot be bonded, address: %v\n", ownerAddr))
-		} else {
+		if !validator.Revoked {
 			bondedValidatorsCount++
+		} else {
+			if validator.Status() == sdk.Bonded {
+				panic(fmt.Sprintf("revoked validator cannot be bonded, address: %v\n", ownerAddr))
+			}
 		}
 
 		iterator.Next()
@@ -510,13 +516,13 @@ func (k Keeper) RemoveValidator(ctx sdk.Context, address sdk.Address) {
 //__________________________________________________________________________
 
 // get the current validator on the cliff
-func (k Keeper) getCliffValidator(ctx sdk.Context) []byte {
+func (k Keeper) GetCliffValidator(ctx sdk.Context) []byte {
 	store := ctx.KVStore(k.storeKey)
 	return store.Get(ValidatorCliffIndexKey)
 }
 
 // get the current power of the validator on the cliff
-func (k Keeper) getCliffValidatorPower(ctx sdk.Context) []byte {
+func (k Keeper) GetCliffValidatorPower(ctx sdk.Context) []byte {
 	store := ctx.KVStore(k.storeKey)
 	return store.Get(ValidatorPowerCliffKey)
 }

x/stake/keeper/validator_test.go

@@ -656,7 +656,7 @@ func TestGetTendermintUpdatesInserted(t *testing.T) {
 	require.Equal(t, validators[4].ABCIValidator(), updates[0])
 }
 
-func TestGetTendermintUpdatesNotValidatorCliff(t *testing.T) {
+func TestGetTendermintUpdatesWithCliffValidator(t *testing.T) {
 	ctx, _, keeper := CreateTestInput(t, false, 1000)
 	params := types.DefaultParams()
 	params.MaxValidators = 2