diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e07e963f5b5..531ef1f5bc0b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index f08dc481a8e4..21cf6172096d 100644 --- a/x/stake/handler_test.go +++ b/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] diff --git a/x/stake/keeper/key.go b/x/stake/keeper/key.go index 32c54f51960f..824c45ca879b 100644 --- a/x/stake/keeper/key.go +++ b/x/stake/keeper/key.go @@ -66,6 +66,7 @@ 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) @@ -73,9 +74,9 @@ func getValidatorPowerRank(validator types.Validator, pool types.Pool) []byte { 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 diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index c32b536cea54..233c7b6e87c0 100644 --- a/x/stake/keeper/validator.go +++ b/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) } diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index 8320f259ce19..4e962420a5c2 100644 --- a/x/stake/keeper/validator_test.go +++ b/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