From 37a5f2fd4c7ebfea689604637670457bf3678bd9 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Fri, 6 Jul 2018 19:53:07 -0400 Subject: [PATCH] fix count in UpdateBondedValidators to not include revoked validators --- CHANGELOG.md | 2 ++ x/stake/handler_test.go | 17 +++++++++---- x/stake/keeper/key.go | 5 ++-- x/stake/keeper/validator.go | 48 ++++++++++++++++++++++++++++++------- 4 files changed, 57 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4808b6db84..c8695add26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -114,6 +114,8 @@ 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 ## 0.19.0 diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index de07b25a37..684a6b700b 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -1,8 +1,10 @@ package stake import ( + "fmt" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/tendermint/tendermint/crypto" @@ -586,15 +588,22 @@ func TestUnbondingWhenExcessValidators(t *testing.T) { require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") require.Equal(t, 2, len(keeper.GetValidatorsBonded(ctx))) + fmt.Println("debug 1________________________________________________________________________________________") + // 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") + fmt.Println("debug 2________________________________________________________________________________________") - // because there are extra validators waiting to get in, we should one - // of the queued validators make it into the bonded group, thus the total - // number of validators should stay the same - require.Equal(t, 2, len(keeper.GetValidatorsBonded(ctx))) + // 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) + assert.Equal(t, 2, len(vals), "vals %v", vals) + val1, found := keeper.GetValidator(ctx, validatorAddr1) + require.True(t, found) + assert.Equal(t, sdk.Bonded, val1.Status(), "%v", val1) } func TestJoiningAsCliffValidator(t *testing.T) { diff --git a/x/stake/keeper/key.go b/x/stake/keeper/key.go index 32c54f5196..824c45ca87 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 2c443ec219..71eb2920cc 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -195,19 +195,23 @@ func (k Keeper) ClearTendermintUpdates(ctx sdk.Context) { // updates all validator stores as well as tendermint update store // may kick out validators if new validator is entering the bonded validator group func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) types.Validator { + fmt.Println("wackydebugoutput UpdateValidator 0") store := ctx.KVStore(k.storeKey) pool := k.GetPool(ctx) ownerAddr := validator.Owner // always update the main list ordered by owner address before exiting defer func() { + fmt.Println("wackydebugoutput UpdateValidator 1") k.SetValidator(ctx, validator) }() + fmt.Println("wackydebugoutput UpdateValidator 2") // retrieve the old validator record oldValidator, oldFound := k.GetValidator(ctx, ownerAddr) if validator.Revoked && oldValidator.Status() == sdk.Bonded { + fmt.Println("wackydebugoutput UpdateValidator 3") validator = k.unbondValidator(ctx, validator) // need to also clear the cliff validator spot because the revoke has @@ -215,37 +219,47 @@ func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) type // updateValidatorsBonded is called k.clearCliffValidator(ctx) } + fmt.Println("wackydebugoutput UpdateValidator 4") powerIncreasing := false if oldFound && oldValidator.PoolShares.Bonded().LT(validator.PoolShares.Bonded()) { + fmt.Println("wackydebugoutput UpdateValidator 5") powerIncreasing = true } + fmt.Println("wackydebugoutput UpdateValidator 6") // if already a validator, copy the old block height and counter, else set them if oldFound && oldValidator.Status() == sdk.Bonded { + fmt.Println("wackydebugoutput UpdateValidator 7") validator.BondHeight = oldValidator.BondHeight validator.BondIntraTxCounter = oldValidator.BondIntraTxCounter } else { + fmt.Println("wackydebugoutput UpdateValidator 8") validator.BondHeight = ctx.BlockHeight() counter := k.GetIntraTxCounter(ctx) validator.BondIntraTxCounter = counter k.SetIntraTxCounter(ctx, counter+1) } + fmt.Println("wackydebugoutput UpdateValidator 9") // update the list ordered by voting power if oldFound { + fmt.Println("wackydebugoutput UpdateValidator 10") store.Delete(GetValidatorsByPowerIndexKey(oldValidator, pool)) } + fmt.Println("wackydebugoutput UpdateValidator 11") valPower := GetValidatorsByPowerIndexKey(validator, pool) store.Set(valPower, validator.Owner) // efficiency case: // if already bonded and power increasing only need to update tendermint if powerIncreasing && !validator.Revoked && oldValidator.Status() == sdk.Bonded { + fmt.Println("wackydebugoutput UpdateValidator 12") bz := k.cdc.MustMarshalBinary(validator.ABCIValidator()) store.Set(GetTendermintUpdatesKey(ownerAddr), bz) return validator } + fmt.Println("wackydebugoutput UpdateValidator 13") // efficiency case: // if was unbonded/or is a new validator - and the new power is less than the cliff validator @@ -253,21 +267,27 @@ func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) type if cliffPower != nil && (!oldFound || (oldFound && oldValidator.Status() == sdk.Unbonded)) && bytes.Compare(valPower, cliffPower) == -1 { //(valPower < cliffPower + fmt.Println("wackydebugoutput UpdateValidator 14") return validator } + fmt.Println("wackydebugoutput UpdateValidator 15") // update the validator set for this validator updatedVal := k.UpdateBondedValidators(ctx, validator) if updatedVal.Owner != nil { // updates to validator occurred to be updated + fmt.Println("wackydebugoutput UpdateValidator 16") validator = updatedVal } + fmt.Println("wackydebugoutput UpdateValidator 17") // if decreased in power but still bonded, update Tendermint validator // (if updatedVal is set, the validator has changed bonding status) stillBonded := oldFound && oldValidator.Status() == sdk.Bonded && updatedVal.Owner == nil if stillBonded && oldValidator.PoolShares.Bonded().GT(validator.PoolShares.Bonded()) { + fmt.Println("wackydebugoutput UpdateValidator 18") bz := k.cdc.MustMarshalBinary(validator.ABCIValidator()) store.Set(GetTendermintUpdatesKey(ownerAddr), bz) } + fmt.Println("wackydebugoutput UpdateValidator 19") return validator } @@ -285,7 +305,8 @@ 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) { + fmt.Printf("debug affectedValidator: %#v\n", affectedValidator) fmt.Println("wackydebugoutput UpdateBondedValidators 0") store := ctx.KVStore(k.storeKey) @@ -301,6 +322,8 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, for { fmt.Println("wackydebugoutput UpdateBondedValidators 1") if !iterator.Valid() || bondedValidatorsCount > int(maxValidators-1) { + fmt.Printf("debug !iterator.Valid(): %v\n", !iterator.Valid()) + fmt.Printf("debug bondedValidatorsCount > int(maxValidators-1): %v\n", bondedValidatorsCount > int(maxValidators-1)) fmt.Println("wackydebugoutput UpdateBondedValidators 2") fmt.Printf("debug bondedValidatorsCount: %v\n", bondedValidatorsCount) @@ -323,9 +346,11 @@ 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) { + fmt.Printf("debug iterator.Value: %v\n", ownerAddr) + fmt.Printf("debug iterator.Key: %v\n", iterator.Key()) + if bytes.Equal(ownerAddr, affectedValidator.Owner) { fmt.Println("wackydebugoutput UpdateBondedValidators 7") - validator = newValidator + validator = affectedValidator } else { fmt.Println("wackydebugoutput UpdateBondedValidators 8") var found bool @@ -345,7 +370,7 @@ 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) { fmt.Println("wackydebugoutput UpdateBondedValidators 13") updatedVal = validator } @@ -353,12 +378,13 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, } fmt.Println("wackydebugoutput UpdateBondedValidators 15") - if validator.Revoked && validator.Status() == sdk.Bonded { - fmt.Println("wackydebugoutput UpdateBondedValidators 16") - panic(fmt.Sprintf("revoked validator cannot be bonded, address: %v\n", ownerAddr)) + if !validator.Revoked { + bondedValidatorsCount++ + } else { + if validator.Status() == sdk.Bonded { + panic(fmt.Sprintf("revoked validator cannot be bonded, address: %v\n", ownerAddr)) + } } - fmt.Println("wackydebugoutput UpdateBondedValidators 18") - bondedValidatorsCount++ iterator.Next() } @@ -480,14 +506,17 @@ func (k Keeper) unbondValidator(ctx sdk.Context, validator types.Validator) type // perform all the store operations for when a validator status becomes bonded func (k Keeper) bondValidator(ctx sdk.Context, validator types.Validator) types.Validator { + fmt.Println("wackydebugoutput bondValidator 0") store := ctx.KVStore(k.storeKey) pool := k.GetPool(ctx) // sanity check if validator.Status() == sdk.Bonded { + fmt.Println("wackydebugoutput bondValidator 1") panic(fmt.Sprintf("should not already be bonded, validator: %v\n", validator)) } + fmt.Println("wackydebugoutput bondValidator 2") // set the status validator, pool = validator.UpdateStatus(pool, sdk.Bonded) @@ -496,6 +525,7 @@ func (k Keeper) bondValidator(ctx sdk.Context, validator types.Validator) types. // save the now bonded validator record to the three referenced stores k.SetValidator(ctx, validator) store.Set(GetValidatorsBondedIndexKey(validator.Owner), []byte{}) + fmt.Println("wackydebugoutput bondValidator 3") // add to accumulated changes for tendermint bzABCI := k.cdc.MustMarshalBinary(validator.ABCIValidator())