From 19afe72d2dccbcd64c60d72a9313595e175b7473 Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Fri, 7 Dec 2018 06:34:11 -0800 Subject: [PATCH 1/3] Fix negative stake & invariance bug --- types/decimal.go | 10 ++++- x/stake/genesis.go | 2 +- x/stake/handler_test.go | 8 ++-- x/stake/keeper/delegation_test.go | 12 +++--- x/stake/keeper/key.go | 2 +- x/stake/keeper/test_common.go | 3 +- x/stake/keeper/val_state_change.go | 19 ++++----- x/stake/keeper/validator.go | 32 ++++++++------- x/stake/keeper/validator_test.go | 62 ++++++++++++++---------------- x/stake/querier/querier_test.go | 8 ++-- x/stake/simulation/invariants.go | 3 +- x/stake/types/validator.go | 21 +++++----- 12 files changed, 91 insertions(+), 91 deletions(-) diff --git a/types/decimal.go b/types/decimal.go index 0d843f6cb775..6ce32a07d826 100644 --- a/types/decimal.go +++ b/types/decimal.go @@ -272,6 +272,10 @@ func (d Dec) Format(s fmt.State, verb rune) { } func (d Dec) String() string { + isNeg := d.IsNegative() + if d.IsNegative() { + d = d.Neg() + } bz, err := d.Int.MarshalText() if err != nil { return "" @@ -298,7 +302,11 @@ func (d Dec) String() string { bzWDec[inputSize-10] = byte('.') copy(bzWDec[inputSize-9:], bz[inputSize-10:]) } - return string(bzWDec) + if isNeg { + return "-" + string(bzWDec) + } else { + return string(bzWDec) + } } // ____ diff --git a/x/stake/genesis.go b/x/stake/genesis.go index bdd62d3769d4..0a5e3e25f093 100644 --- a/x/stake/genesis.go +++ b/x/stake/genesis.go @@ -38,7 +38,7 @@ func InitGenesis(ctx sdk.Context, keeper Keeper, data types.GenesisState) (res [ // Manually set indices for the first time keeper.SetValidatorByConsAddr(ctx, validator) - keeper.SetValidatorByPowerIndex(ctx, validator, data.Pool) + keeper.SetValidatorByPowerIndex(ctx, validator) keeper.OnValidatorCreated(ctx, validator.OperatorAddr) // Set timeslice if necessary diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index 2c82ee7add83..04e1e372f221 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -55,8 +55,7 @@ func TestValidatorByPowerIndex(t *testing.T) { // verify that the by power index exists validator, found := keeper.GetValidator(ctx, validatorAddr) require.True(t, found) - pool := keeper.GetPool(ctx) - power := keep.GetValidatorsByPowerIndexKey(validator, pool) + power := keep.GetValidatorsByPowerIndexKey(validator) require.True(t, keep.ValidatorByPowerIndexExists(ctx, keeper, power)) // create a second validator keep it bonded @@ -85,12 +84,11 @@ func TestValidatorByPowerIndex(t *testing.T) { // but the new power record should have been created validator, found = keeper.GetValidator(ctx, validatorAddr) require.True(t, found) - pool = keeper.GetPool(ctx) - power2 := GetValidatorsByPowerIndexKey(validator, pool) + power2 := GetValidatorsByPowerIndexKey(validator) require.True(t, keep.ValidatorByPowerIndexExists(ctx, keeper, power2)) // now the new record power index should be the same as the original record - power3 := GetValidatorsByPowerIndexKey(validator, pool) + power3 := GetValidatorsByPowerIndexKey(validator) require.Equal(t, power2, power3) // unbond self-delegation diff --git a/x/stake/keeper/delegation_test.go b/x/stake/keeper/delegation_test.go index 2310f849fd62..043f19193d1e 100644 --- a/x/stake/keeper/delegation_test.go +++ b/x/stake/keeper/delegation_test.go @@ -240,7 +240,7 @@ func TestUndelegateSelfDelegation(t *testing.T) { keeper.SetDelegation(ctx, selfDelegation) // create a second delegation to this validator - keeper.DeleteValidatorByPowerIndex(ctx, validator, pool) + keeper.DeleteValidatorByPowerIndex(ctx, validator) validator, pool, issuedShares = validator.AddTokensFromDel(pool, sdk.NewInt(10)) require.Equal(t, int64(10), issuedShares.RoundInt64()) keeper.SetPool(ctx, pool) @@ -288,7 +288,7 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) { keeper.SetDelegation(ctx, selfDelegation) // create a second delegation to this validator - keeper.DeleteValidatorByPowerIndex(ctx, validator, pool) + keeper.DeleteValidatorByPowerIndex(ctx, validator) validator, pool, issuedShares = validator.AddTokensFromDel(pool, sdk.NewInt(10)) require.Equal(t, int64(10), issuedShares.RoundInt64()) keeper.SetPool(ctx, pool) @@ -365,7 +365,7 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) { keeper.SetDelegation(ctx, selfDelegation) // create a second delegation to this validator - keeper.DeleteValidatorByPowerIndex(ctx, validator, pool) + keeper.DeleteValidatorByPowerIndex(ctx, validator) validator, pool, issuedShares = validator.AddTokensFromDel(pool, sdk.NewInt(10)) require.Equal(t, int64(10), issuedShares.RoundInt64()) keeper.SetPool(ctx, pool) @@ -444,7 +444,7 @@ func TestUnbondingAllDelegationFromValidator(t *testing.T) { keeper.SetDelegation(ctx, selfDelegation) // create a second delegation to this validator - keeper.DeleteValidatorByPowerIndex(ctx, validator, pool) + keeper.DeleteValidatorByPowerIndex(ctx, validator) validator, pool, issuedShares = validator.AddTokensFromDel(pool, sdk.NewInt(10)) require.Equal(t, int64(10), issuedShares.RoundInt64()) keeper.SetPool(ctx, pool) @@ -689,7 +689,7 @@ func TestRedelegateFromUnbondingValidator(t *testing.T) { keeper.SetDelegation(ctx, selfDelegation) // create a second delegation to this validator - keeper.DeleteValidatorByPowerIndex(ctx, validator, pool) + keeper.DeleteValidatorByPowerIndex(ctx, validator) validator, pool, issuedShares = validator.AddTokensFromDel(pool, sdk.NewInt(10)) require.Equal(t, int64(10), issuedShares.RoundInt64()) keeper.SetPool(ctx, pool) @@ -773,7 +773,7 @@ func TestRedelegateFromUnbondedValidator(t *testing.T) { keeper.SetDelegation(ctx, selfDelegation) // create a second delegation to this validator - keeper.DeleteValidatorByPowerIndex(ctx, validator, pool) + keeper.DeleteValidatorByPowerIndex(ctx, validator) validator, pool, issuedShares = validator.AddTokensFromDel(pool, sdk.NewInt(10)) validator.BondIntraTxCounter = 1 require.Equal(t, int64(10), issuedShares.RoundInt64()) diff --git a/x/stake/keeper/key.go b/x/stake/keeper/key.go index 243fe34ec57c..34fa1a5b230f 100644 --- a/x/stake/keeper/key.go +++ b/x/stake/keeper/key.go @@ -61,7 +61,7 @@ func AddressFromLastValidatorPowerKey(key []byte) []byte { // Power index is the key used in the power-store, and represents the relative // power ranking of the validator. // VALUE: validator operator address ([]byte) -func GetValidatorsByPowerIndexKey(validator types.Validator, pool types.Pool) []byte { +func GetValidatorsByPowerIndexKey(validator types.Validator) []byte { // NOTE the address doesn't need to be stored because counter bytes must always be different return getValidatorPowerRank(validator) } diff --git a/x/stake/keeper/test_common.go b/x/stake/keeper/test_common.go index 5a501f721070..244a8107fa3e 100644 --- a/x/stake/keeper/test_common.go +++ b/x/stake/keeper/test_common.go @@ -202,9 +202,8 @@ func ValidatorByPowerIndexExists(ctx sdk.Context, keeper Keeper, power []byte) b // update validator for testing func TestingUpdateValidator(keeper Keeper, ctx sdk.Context, validator types.Validator) types.Validator { - pool := keeper.GetPool(ctx) keeper.SetValidator(ctx, validator) - keeper.SetValidatorByPowerIndex(ctx, validator, pool) + keeper.SetValidatorByPowerIndex(ctx, validator) keeper.ApplyAndReturnValidatorSetUpdates(ctx) validator, found := keeper.GetValidator(ctx, validator.OperatorAddr) if !found { diff --git a/x/stake/keeper/val_state_change.go b/x/stake/keeper/val_state_change.go index 3f98f08c5601..e3e13ddceb76 100644 --- a/x/stake/keeper/val_state_change.go +++ b/x/stake/keeper/val_state_change.go @@ -160,10 +160,9 @@ func (k Keeper) jailValidator(ctx sdk.Context, validator types.Validator) { panic(fmt.Sprintf("cannot jail already jailed validator, validator: %v\n", validator)) } - pool := k.GetPool(ctx) validator.Jailed = true k.SetValidator(ctx, validator) - k.DeleteValidatorByPowerIndex(ctx, validator, pool) + k.DeleteValidatorByPowerIndex(ctx, validator) } // remove a validator from jail @@ -172,28 +171,26 @@ func (k Keeper) unjailValidator(ctx sdk.Context, validator types.Validator) { panic(fmt.Sprintf("cannot unjail already unjailed validator, validator: %v\n", validator)) } - pool := k.GetPool(ctx) validator.Jailed = false k.SetValidator(ctx, validator) - k.SetValidatorByPowerIndex(ctx, validator, pool) + k.SetValidatorByPowerIndex(ctx, validator) } // perform all the store operations for when a validator status becomes bonded func (k Keeper) bondValidator(ctx sdk.Context, validator types.Validator) types.Validator { - pool := k.GetPool(ctx) - - k.DeleteValidatorByPowerIndex(ctx, validator, pool) + k.DeleteValidatorByPowerIndex(ctx, validator) validator.BondHeight = ctx.BlockHeight() // set the status + pool := k.GetPool(ctx) validator, pool = validator.UpdateStatus(pool, sdk.Bonded) k.SetPool(ctx, pool) // save the now bonded validator record to the two referenced stores k.SetValidator(ctx, validator) - k.SetValidatorByPowerIndex(ctx, validator, pool) + k.SetValidatorByPowerIndex(ctx, validator) // delete from queue if present k.DeleteValidatorQueue(ctx, validator) @@ -209,10 +206,9 @@ func (k Keeper) bondValidator(ctx sdk.Context, validator types.Validator) types. // perform all the store operations for when a validator status begins unbonding func (k Keeper) beginUnbondingValidator(ctx sdk.Context, validator types.Validator) types.Validator { - pool := k.GetPool(ctx) params := k.GetParams(ctx) - k.DeleteValidatorByPowerIndex(ctx, validator, pool) + k.DeleteValidatorByPowerIndex(ctx, validator) // sanity check if validator.Status != sdk.Bonded { @@ -220,6 +216,7 @@ func (k Keeper) beginUnbondingValidator(ctx sdk.Context, validator types.Validat } // set the status + pool := k.GetPool(ctx) validator, pool = validator.UpdateStatus(pool, sdk.Unbonding) k.SetPool(ctx, pool) @@ -228,7 +225,7 @@ func (k Keeper) beginUnbondingValidator(ctx sdk.Context, validator types.Validat // save the now unbonded validator record and power index k.SetValidator(ctx, validator) - k.SetValidatorByPowerIndex(ctx, validator, pool) + k.SetValidatorByPowerIndex(ctx, validator) // Adds to unbonding validator queue k.InsertValidatorQueue(ctx, validator) diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index c7919537cf9c..a17a66e7cd9c 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -99,26 +99,25 @@ func (k Keeper) SetValidatorByConsAddr(ctx sdk.Context, validator types.Validato } // validator index -func (k Keeper) SetValidatorByPowerIndex(ctx sdk.Context, validator types.Validator, pool types.Pool) { +func (k Keeper) SetValidatorByPowerIndex(ctx sdk.Context, validator types.Validator) { // jailed validators are not kept in the power index if validator.Jailed { return } store := ctx.KVStore(k.storeKey) - store.Set(GetValidatorsByPowerIndexKey(validator, pool), validator.OperatorAddr) + store.Set(GetValidatorsByPowerIndexKey(validator), validator.OperatorAddr) } // validator index -func (k Keeper) DeleteValidatorByPowerIndex(ctx sdk.Context, validator types.Validator, pool types.Pool) { +func (k Keeper) DeleteValidatorByPowerIndex(ctx sdk.Context, validator types.Validator) { store := ctx.KVStore(k.storeKey) - store.Delete(GetValidatorsByPowerIndexKey(validator, pool)) + store.Delete(GetValidatorsByPowerIndexKey(validator)) } // validator index func (k Keeper) SetNewValidatorByPowerIndex(ctx sdk.Context, validator types.Validator) { store := ctx.KVStore(k.storeKey) - pool := k.GetPool(ctx) - store.Set(GetValidatorsByPowerIndexKey(validator, pool), validator.OperatorAddr) + store.Set(GetValidatorsByPowerIndexKey(validator), validator.OperatorAddr) } //___________________________________________________________________________ @@ -127,8 +126,8 @@ func (k Keeper) SetNewValidatorByPowerIndex(ctx sdk.Context, validator types.Val func (k Keeper) AddValidatorTokensAndShares(ctx sdk.Context, validator types.Validator, tokensToAdd sdk.Int) (valOut types.Validator, addedShares sdk.Dec) { + k.DeleteValidatorByPowerIndex(ctx, validator) pool := k.GetPool(ctx) - k.DeleteValidatorByPowerIndex(ctx, validator, pool) validator, pool, addedShares = validator.AddTokensFromDel(pool, tokensToAdd) // increment the intra-tx counter // in case of a conflict, the validator which least recently changed power takes precedence @@ -137,7 +136,7 @@ func (k Keeper) AddValidatorTokensAndShares(ctx sdk.Context, validator types.Val k.SetIntraTxCounter(ctx, counter+1) k.SetValidator(ctx, validator) k.SetPool(ctx, pool) - k.SetValidatorByPowerIndex(ctx, validator, pool) + k.SetValidatorByPowerIndex(ctx, validator) return validator, addedShares } @@ -145,24 +144,24 @@ func (k Keeper) AddValidatorTokensAndShares(ctx sdk.Context, validator types.Val func (k Keeper) RemoveValidatorTokensAndShares(ctx sdk.Context, validator types.Validator, sharesToRemove sdk.Dec) (valOut types.Validator, removedTokens sdk.Dec) { + k.DeleteValidatorByPowerIndex(ctx, validator) pool := k.GetPool(ctx) - k.DeleteValidatorByPowerIndex(ctx, validator, pool) validator, pool, removedTokens = validator.RemoveDelShares(pool, sharesToRemove) k.SetValidator(ctx, validator) k.SetPool(ctx, pool) - k.SetValidatorByPowerIndex(ctx, validator, pool) + k.SetValidatorByPowerIndex(ctx, validator) return validator, removedTokens } // Update the tokens of an existing validator, update the validators power index key func (k Keeper) RemoveValidatorTokens(ctx sdk.Context, validator types.Validator, tokensToRemove sdk.Dec) types.Validator { + k.DeleteValidatorByPowerIndex(ctx, validator) pool := k.GetPool(ctx) - k.DeleteValidatorByPowerIndex(ctx, validator, pool) validator, pool = validator.RemoveTokens(pool, tokensToRemove) k.SetValidator(ctx, validator) k.SetPool(ctx, pool) - k.SetValidatorByPowerIndex(ctx, validator, pool) + k.SetValidatorByPowerIndex(ctx, validator) return validator } @@ -195,12 +194,17 @@ func (k Keeper) RemoveValidator(ctx sdk.Context, address sdk.ValAddress) { panic("Cannot call RemoveValidator on bonded or unbonding validators") } + // if any tokens remain, remove from pool. + // this happens if shares are zero but tokens are not. + pool := k.GetPool(ctx) + pool.LooseTokens = pool.LooseTokens.Sub(validator.Tokens) + k.SetPool(ctx, pool) + // delete the old validator record store := ctx.KVStore(k.storeKey) - pool := k.GetPool(ctx) store.Delete(GetValidatorKey(address)) store.Delete(GetValidatorByConsAddrKey(sdk.ConsAddress(validator.ConsPubKey.Address()))) - store.Delete(GetValidatorsByPowerIndexKey(validator, pool)) + store.Delete(GetValidatorsByPowerIndexKey(validator)) // call hook if present if k.hooks != nil { diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index fe806335f093..ed89f0edf47b 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -30,7 +30,7 @@ func TestSetValidator(t *testing.T) { assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.DelegatorShares)) keeper.SetPool(ctx, pool) keeper.SetValidator(ctx, validator) - keeper.SetValidatorByPowerIndex(ctx, validator, pool) + keeper.SetValidatorByPowerIndex(ctx, validator) // ensure update updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx) @@ -90,11 +90,11 @@ func TestUpdateValidatorByPowerIndex(t *testing.T) { require.Equal(t, int64(100), validator.Tokens.RoundInt64(), "\nvalidator %v\npool %v", validator, pool) pool = keeper.GetPool(ctx) - power := GetValidatorsByPowerIndexKey(validator, pool) + power := GetValidatorsByPowerIndexKey(validator) require.True(t, validatorByPowerIndexExists(keeper, ctx, power)) // burn half the delegator shares - keeper.DeleteValidatorByPowerIndex(ctx, validator, pool) + keeper.DeleteValidatorByPowerIndex(ctx, validator) validator, pool, burned := validator.RemoveDelShares(pool, delSharesCreated.Quo(sdk.NewDec(2))) require.Equal(t, int64(50), burned.RoundInt64()) keeper.SetPool(ctx, pool) // update the pool @@ -104,7 +104,7 @@ func TestUpdateValidatorByPowerIndex(t *testing.T) { pool = keeper.GetPool(ctx) validator, found = keeper.GetValidator(ctx, addrVals[0]) require.True(t, found) - power = GetValidatorsByPowerIndexKey(validator, pool) + power = GetValidatorsByPowerIndexKey(validator) require.True(t, validatorByPowerIndexExists(keeper, ctx, power)) } @@ -143,7 +143,7 @@ func TestUpdateBondedValidatorsDecreaseCliff(t *testing.T) { // remove enough tokens to kick out the validator below the current cliff // validator and next in line cliff validator - keeper.DeleteValidatorByPowerIndex(ctx, nextCliffVal, pool) + keeper.DeleteValidatorByPowerIndex(ctx, nextCliffVal) nextCliffVal, pool, _ = nextCliffVal.RemoveDelShares(pool, sdk.NewDec(21)) keeper.SetPool(ctx, pool) nextCliffVal = TestingUpdateValidator(keeper, ctx, nextCliffVal) @@ -459,7 +459,7 @@ func TestGetValidatorsEdgeCases(t *testing.T) { assert.True(ValEq(t, validators[3], resValidators[1])) pool := keeper.GetPool(ctx) - keeper.DeleteValidatorByPowerIndex(ctx, validators[0], pool) + keeper.DeleteValidatorByPowerIndex(ctx, validators[0]) validators[0], pool, _ = validators[0].AddTokensFromDel(pool, sdk.NewInt(500)) keeper.SetPool(ctx, pool) validators[0] = TestingUpdateValidator(keeper, ctx, validators[0]) @@ -477,7 +477,7 @@ func TestGetValidatorsEdgeCases(t *testing.T) { validators[3], found = keeper.GetValidator(ctx, validators[3].OperatorAddr) require.True(t, found) - keeper.DeleteValidatorByPowerIndex(ctx, validators[3], pool) + keeper.DeleteValidatorByPowerIndex(ctx, validators[3]) validators[3], pool, _ = validators[3].AddTokensFromDel(pool, sdk.NewInt(1)) keeper.SetPool(ctx, pool) validators[3] = TestingUpdateValidator(keeper, ctx, validators[3]) @@ -487,7 +487,7 @@ func TestGetValidatorsEdgeCases(t *testing.T) { assert.True(ValEq(t, validators[3], resValidators[1])) // validator 3 kicked out temporarily - keeper.DeleteValidatorByPowerIndex(ctx, validators[3], pool) + keeper.DeleteValidatorByPowerIndex(ctx, validators[3]) validators[3], pool, _ = validators[3].RemoveDelShares(pool, sdk.NewDec(201)) keeper.SetPool(ctx, pool) validators[3] = TestingUpdateValidator(keeper, ctx, validators[3]) @@ -497,7 +497,7 @@ func TestGetValidatorsEdgeCases(t *testing.T) { assert.True(ValEq(t, validators[2], resValidators[1])) // validator 4 does not get spot back - keeper.DeleteValidatorByPowerIndex(ctx, validators[3], pool) + keeper.DeleteValidatorByPowerIndex(ctx, validators[3]) validators[3], pool, _ = validators[3].AddTokensFromDel(pool, sdk.NewInt(200)) keeper.SetPool(ctx, pool) validators[3] = TestingUpdateValidator(keeper, ctx, validators[3]) @@ -548,8 +548,8 @@ func TestValidatorBondHeight(t *testing.T) { assert.True(ValEq(t, validators[0], resValidators[0])) assert.True(ValEq(t, validators[1], resValidators[1])) - keeper.DeleteValidatorByPowerIndex(ctx, validators[1], pool) - keeper.DeleteValidatorByPowerIndex(ctx, validators[2], pool) + keeper.DeleteValidatorByPowerIndex(ctx, validators[1]) + keeper.DeleteValidatorByPowerIndex(ctx, validators[2]) validators[1], pool, _ = validators[1].AddTokensFromDel(pool, sdk.NewInt(50)) validators[2], pool, _ = validators[2].AddTokensFromDel(pool, sdk.NewInt(50)) keeper.SetPool(ctx, pool) @@ -624,11 +624,10 @@ func TestApplyAndReturnValidatorSetUpdatesAllNone(t *testing.T) { // test from nothing to something // tendermintUpdate set: {} -> {c1, c3} require.Equal(t, 0, len(keeper.ApplyAndReturnValidatorSetUpdates(ctx))) - pool := keeper.GetPool(ctx) keeper.SetValidator(ctx, validators[0]) - keeper.SetValidatorByPowerIndex(ctx, validators[0], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[0]) keeper.SetValidator(ctx, validators[1]) - keeper.SetValidatorByPowerIndex(ctx, validators[1], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[1]) updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx) assert.Equal(t, 2, len(updates)) @@ -734,9 +733,8 @@ func TestApplyAndReturnValidatorSetUpdatesInserted(t *testing.T) { // test validtor added at the beginning // tendermintUpdate set: {} -> {c0} - pool := keeper.GetPool(ctx) keeper.SetValidator(ctx, validators[2]) - keeper.SetValidatorByPowerIndex(ctx, validators[2], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[2]) updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx) validators[2], _ = keeper.GetValidator(ctx, validators[2].OperatorAddr) require.Equal(t, 1, len(updates)) @@ -744,9 +742,8 @@ func TestApplyAndReturnValidatorSetUpdatesInserted(t *testing.T) { // test validtor added at the beginning // tendermintUpdate set: {} -> {c0} - pool = keeper.GetPool(ctx) keeper.SetValidator(ctx, validators[3]) - keeper.SetValidatorByPowerIndex(ctx, validators[3], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[3]) updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx) validators[3], _ = keeper.GetValidator(ctx, validators[3].OperatorAddr) require.Equal(t, 1, len(updates)) @@ -754,9 +751,8 @@ func TestApplyAndReturnValidatorSetUpdatesInserted(t *testing.T) { // test validtor added at the end // tendermintUpdate set: {} -> {c0} - pool = keeper.GetPool(ctx) keeper.SetValidator(ctx, validators[4]) - keeper.SetValidatorByPowerIndex(ctx, validators[4], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[4]) updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx) validators[4], _ = keeper.GetValidator(ctx, validators[4].OperatorAddr) require.Equal(t, 1, len(updates)) @@ -795,7 +791,7 @@ func TestApplyAndReturnValidatorSetUpdatesWithCliffValidator(t *testing.T) { validators[2], pool, _ = validators[2].AddTokensFromDel(pool, sdk.NewInt(10)) keeper.SetPool(ctx, pool) keeper.SetValidator(ctx, validators[2]) - keeper.SetValidatorByPowerIndex(ctx, validators[2], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[2]) updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx) validators[2], _ = keeper.GetValidator(ctx, validators[2].OperatorAddr) require.Equal(t, 2, len(updates), "%v", updates) @@ -865,7 +861,7 @@ func TestApplyAndReturnValidatorSetUpdatesNewValidator(t *testing.T) { keeper.SetPool(ctx, pool) keeper.SetValidator(ctx, validators[i]) - keeper.SetValidatorByPowerIndex(ctx, validators[i], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[i]) } // verify initial Tendermint updates are correct @@ -881,12 +877,12 @@ func TestApplyAndReturnValidatorSetUpdatesNewValidator(t *testing.T) { // update initial validator set for i, amt := range amts { pool := keeper.GetPool(ctx) - keeper.DeleteValidatorByPowerIndex(ctx, validators[i], pool) + keeper.DeleteValidatorByPowerIndex(ctx, validators[i]) validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt)) keeper.SetPool(ctx, pool) keeper.SetValidator(ctx, validators[i]) - keeper.SetValidatorByPowerIndex(ctx, validators[i], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[i]) } // add a new validator that goes from zero power, to non-zero power, back to @@ -904,7 +900,7 @@ func TestApplyAndReturnValidatorSetUpdatesNewValidator(t *testing.T) { validator, pool, _ = validator.RemoveDelShares(pool, sdk.NewDecFromInt(amt)) keeper.SetValidator(ctx, validator) - keeper.SetValidatorByPowerIndex(ctx, validator, pool) + keeper.SetValidatorByPowerIndex(ctx, validator) // add a new validator that increases in power valPubKey = PKs[len(validators)+2] @@ -913,7 +909,7 @@ func TestApplyAndReturnValidatorSetUpdatesNewValidator(t *testing.T) { validator = types.NewValidator(valAddr, valPubKey, types.Description{}) validator, pool, _ = validator.AddTokensFromDel(pool, sdk.NewInt(500)) keeper.SetValidator(ctx, validator) - keeper.SetValidatorByPowerIndex(ctx, validator, pool) + keeper.SetValidatorByPowerIndex(ctx, validator) keeper.SetPool(ctx, pool) // verify initial Tendermint updates are correct @@ -949,7 +945,7 @@ func TestApplyAndReturnValidatorSetUpdatesBondTransition(t *testing.T) { validators[i].BondIntraTxCounter = int16(i) keeper.SetPool(ctx, pool) keeper.SetValidator(ctx, validators[i]) - keeper.SetValidatorByPowerIndex(ctx, validators[i], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[i]) } // verify initial Tendermint updates are correct @@ -970,11 +966,11 @@ func TestApplyAndReturnValidatorSetUpdatesBondTransition(t *testing.T) { validators[0], found = keeper.GetValidator(ctx, validators[0].OperatorAddr) require.True(t, found) - keeper.DeleteValidatorByPowerIndex(ctx, validators[0], pool) + keeper.DeleteValidatorByPowerIndex(ctx, validators[0]) validators[0], pool, _ = validators[0].AddTokensFromDel(pool, sdk.NewInt(1)) keeper.SetPool(ctx, pool) keeper.SetValidator(ctx, validators[0]) - keeper.SetValidatorByPowerIndex(ctx, validators[0], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[0]) // verify initial Tendermint updates are correct require.Equal(t, 0, len(keeper.ApplyAndReturnValidatorSetUpdates(ctx))) @@ -987,19 +983,19 @@ func TestApplyAndReturnValidatorSetUpdatesBondTransition(t *testing.T) { validators[1], found = keeper.GetValidator(ctx, validators[1].OperatorAddr) require.True(t, found) - keeper.DeleteValidatorByPowerIndex(ctx, validators[0], pool) + keeper.DeleteValidatorByPowerIndex(ctx, validators[0]) validators[0], pool, _ = validators[0].RemoveDelShares(pool, validators[0].DelegatorShares) keeper.SetPool(ctx, pool) keeper.SetValidator(ctx, validators[0]) - keeper.SetValidatorByPowerIndex(ctx, validators[0], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[0]) updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx) require.Equal(t, 0, len(updates)) - keeper.DeleteValidatorByPowerIndex(ctx, validators[1], pool) + keeper.DeleteValidatorByPowerIndex(ctx, validators[1]) validators[1], pool, _ = validators[1].AddTokensFromDel(pool, sdk.NewInt(250)) keeper.SetPool(ctx, pool) keeper.SetValidator(ctx, validators[1]) - keeper.SetValidatorByPowerIndex(ctx, validators[1], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[1]) // verify initial Tendermint updates are correct updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx) diff --git a/x/stake/querier/querier_test.go b/x/stake/querier/querier_test.go index eda5c6952525..d6ea3639e661 100644 --- a/x/stake/querier/querier_test.go +++ b/x/stake/querier/querier_test.go @@ -29,7 +29,7 @@ func TestNewQuerier(t *testing.T) { validators[i], pool, _ = validators[i].AddTokensFromDel(pool, amt) validators[i].BondIntraTxCounter = int16(i) keeper.SetValidator(ctx, validators[i]) - keeper.SetValidatorByPowerIndex(ctx, validators[i], pool) + keeper.SetValidatorByPowerIndex(ctx, validators[i]) } keeper.SetPool(ctx, pool) @@ -170,13 +170,11 @@ func TestQueryDelegation(t *testing.T) { // Create Validators and Delegation val1 := types.NewValidator(addrVal1, pk1, types.Description{}) keeper.SetValidator(ctx, val1) - pool := keeper.GetPool(ctx) - keeper.SetValidatorByPowerIndex(ctx, val1, pool) + keeper.SetValidatorByPowerIndex(ctx, val1) val2 := types.NewValidator(addrVal2, pk2, types.Description{}) keeper.SetValidator(ctx, val2) - pool = keeper.GetPool(ctx) - keeper.SetValidatorByPowerIndex(ctx, val2, pool) + keeper.SetValidatorByPowerIndex(ctx, val2) keeper.Delegate(ctx, addrAcc2, sdk.NewCoin(types.DefaultBondDenom, sdk.NewInt(20)), val1, true) diff --git a/x/stake/simulation/invariants.go b/x/stake/simulation/invariants.go index 8fac4df156e5..7476ea2fe1a3 100644 --- a/x/stake/simulation/invariants.go +++ b/x/stake/simulation/invariants.go @@ -115,7 +115,6 @@ func SupplyInvariants(ck bank.Keeper, k stake.Keeper, func PositivePowerInvariant(k stake.Keeper) simulation.Invariant { return func(ctx sdk.Context) error { iterator := k.ValidatorsPowerStoreIterator(ctx) - pool := k.GetPool(ctx) for ; iterator.Valid(); iterator.Next() { validator, found := k.GetValidator(ctx, iterator.Value()) @@ -123,7 +122,7 @@ func PositivePowerInvariant(k stake.Keeper) simulation.Invariant { panic(fmt.Sprintf("validator record not found for address: %X\n", iterator.Value())) } - powerKey := keeper.GetValidatorsByPowerIndexKey(validator, pool) + powerKey := keeper.GetValidatorsByPowerIndexKey(validator) if !bytes.Equal(iterator.Key(), powerKey) { return fmt.Errorf("power store invariance:\n\tvalidator.Power: %v"+ diff --git a/x/stake/types/validator.go b/x/stake/types/validator.go index ca56e0ea4aae..90558dd96d2d 100644 --- a/x/stake/types/validator.go +++ b/x/stake/types/validator.go @@ -360,11 +360,16 @@ func (v Validator) UpdateStatus(pool Pool, NewStatus sdk.BondStatus) (Validator, // removes tokens from a validator func (v Validator) RemoveTokens(pool Pool, tokens sdk.Dec) (Validator, Pool) { + if tokens.IsNegative() { + panic(fmt.Sprintf("should not happen: trying to remove negative tokens %v", tokens)) + } + if v.Tokens.LT(tokens) { + panic(fmt.Sprintf("should not happen: only have %v tokens, trying to remove %v", v.Tokens, tokens)) + } + v.Tokens = v.Tokens.Sub(tokens) if v.Status == sdk.Bonded { pool = pool.bondedTokensToLoose(tokens) } - - v.Tokens = v.Tokens.Sub(tokens) return v, pool } @@ -404,15 +409,11 @@ func (v Validator) AddTokensFromDel(pool Pool, amount sdk.Int) (Validator, Pool, // RemoveDelShares removes delegator shares from a validator. func (v Validator) RemoveDelShares(pool Pool, delShares sdk.Dec) (Validator, Pool, sdk.Dec) { - issuedTokens := v.DelegatorShareExRate().Mul(delShares) - v.Tokens = v.Tokens.Sub(issuedTokens) + delTokens := v.DelegatorShareExRate().Mul(delShares) + delTokens = sdk.MinDec(delTokens, v.Tokens) + v, pool = v.RemoveTokens(pool, delTokens) v.DelegatorShares = v.DelegatorShares.Sub(delShares) - - if v.Status == sdk.Bonded { - pool = pool.bondedTokensToLoose(issuedTokens) - } - - return v, pool, issuedTokens + return v, pool, delTokens } // DelegatorShareExRate gets the exchange rate of tokens over delegator shares. From 2c0217f8a1f57c4b68d48e58d5e5f8c71571d8f8 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 7 Dec 2018 20:45:45 +0100 Subject: [PATCH 2/3] Merge PR #3037: Updates to negative stake fix * Update invariant; fix lint * Fix linter --- cmd/gaia/app/invariants.go | 2 +- cmd/gaia/cmd/gaiareplay/main.go | 4 +++- types/decimal.go | 3 +-- x/stake/simulation/invariants.go | 10 +++++++--- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/cmd/gaia/app/invariants.go b/cmd/gaia/app/invariants.go index 632769e2b906..4a60a41aeac1 100644 --- a/cmd/gaia/app/invariants.go +++ b/cmd/gaia/app/invariants.go @@ -18,7 +18,7 @@ func (app *GaiaApp) runtimeInvariants() []simulation.Invariant { distrsim.ValAccumInvariants(app.distrKeeper, app.stakeKeeper), stakesim.SupplyInvariants(app.bankKeeper, app.stakeKeeper, app.feeCollectionKeeper, app.distrKeeper, app.accountKeeper), - stakesim.PositivePowerInvariant(app.stakeKeeper), + stakesim.NonNegativePowerInvariant(app.stakeKeeper), } } diff --git a/cmd/gaia/cmd/gaiareplay/main.go b/cmd/gaia/cmd/gaiareplay/main.go index deeacc1417c2..4f52889e2b37 100644 --- a/cmd/gaia/cmd/gaiareplay/main.go +++ b/cmd/gaia/cmd/gaiareplay/main.go @@ -127,7 +127,9 @@ func run(rootDir string) { if err != nil { panic(err) } - defer proxyApp.Stop() + defer func() { + _ = proxyApp.Stop() + }() var state tmsm.State = tmsm.LoadState(tmDB) if state.LastBlockHeight == 0 { diff --git a/types/decimal.go b/types/decimal.go index 6ce32a07d826..b4bd44fb27b0 100644 --- a/types/decimal.go +++ b/types/decimal.go @@ -304,9 +304,8 @@ func (d Dec) String() string { } if isNeg { return "-" + string(bzWDec) - } else { - return string(bzWDec) } + return string(bzWDec) } // ____ diff --git a/x/stake/simulation/invariants.go b/x/stake/simulation/invariants.go index 7476ea2fe1a3..7e875171d340 100644 --- a/x/stake/simulation/invariants.go +++ b/x/stake/simulation/invariants.go @@ -26,7 +26,7 @@ func AllInvariants(ck bank.Keeper, k stake.Keeper, return err } - err = PositivePowerInvariant(k)(ctx) + err = NonNegativePowerInvariant(k)(ctx) if err != nil { return err } @@ -111,8 +111,8 @@ func SupplyInvariants(ck bank.Keeper, k stake.Keeper, } } -// PositivePowerInvariant checks that all stored validators have > 0 power. -func PositivePowerInvariant(k stake.Keeper) simulation.Invariant { +// NonNegativePowerInvariant checks that all stored validators have >= 0 power. +func NonNegativePowerInvariant(k stake.Keeper) simulation.Invariant { return func(ctx sdk.Context) error { iterator := k.ValidatorsPowerStoreIterator(ctx) @@ -128,6 +128,10 @@ func PositivePowerInvariant(k stake.Keeper) simulation.Invariant { return fmt.Errorf("power store invariance:\n\tvalidator.Power: %v"+ "\n\tkey should be: %v\n\tkey in store: %v", validator.GetPower(), powerKey, iterator.Key()) } + + if validator.Tokens.LT(sdk.ZeroDec()) { + return fmt.Errorf("negative tokens for validator: %v", validator) + } } iterator.Close() return nil From e54601cd7e409ecb06aa56b3d437dd19ff608e7e Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Sat, 8 Dec 2018 00:55:00 +0100 Subject: [PATCH 3/3] Add comment & TODO --- x/stake/keeper/validator.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index a17a66e7cd9c..2d71ec513aac 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -194,8 +194,9 @@ func (k Keeper) RemoveValidator(ctx sdk.Context, address sdk.ValAddress) { panic("Cannot call RemoveValidator on bonded or unbonding validators") } - // if any tokens remain, remove from pool. + // if any tokens remain, remove from pool (burning the tokens). // this happens if shares are zero but tokens are not. + // TODO: Remove once https://github.com/cosmos/cosmos-sdk/pull/2958 is merged pool := k.GetPool(ctx) pool.LooseTokens = pool.LooseTokens.Sub(validator.Tokens) k.SetPool(ctx, pool)