Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix negative stake & invariance bug #3033

Merged
merged 3 commits into from
Dec 8, 2018
Merged

Fix negative stake & invariance bug #3033

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19afe72
Fix negative stake & invariance bug
jaekwon Dec 7, 2018
2c0217f
Merge PR #3037: Updates to negative stake fix
cwgoes Dec 7, 2018
e54601c
Add comment & TODO
cwgoes Dec 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion types/decimal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ""
Expand All @@ -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 {
rigelrozanski marked this conversation as resolved.
Show resolved Hide resolved
return string(bzWDec)
}
}

// ____
Expand Down
2 changes: 1 addition & 1 deletion x/stake/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 3 additions & 5 deletions x/stake/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
12 changes: 6 additions & 6 deletions x/stake/keeper/delegation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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())
Expand Down
2 changes: 1 addition & 1 deletion x/stake/keeper/key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
3 changes: 1 addition & 2 deletions x/stake/keeper/test_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
19 changes: 8 additions & 11 deletions x/stake/keeper/val_state_change.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -209,17 +206,17 @@ 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 {
panic(fmt.Sprintf("should not already be unbonded or unbonding, validator: %v\n", validator))
}

// set the status
pool := k.GetPool(ctx)
validator, pool = validator.UpdateStatus(pool, sdk.Unbonding)
k.SetPool(ctx, pool)

Expand All @@ -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)
Expand Down
32 changes: 18 additions & 14 deletions x/stake/keeper/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

//___________________________________________________________________________
Expand All @@ -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
Expand All @@ -137,32 +136,32 @@ 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
}

// Update the tokens of an existing validator, update the validators power index key
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
}

Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably indicate "burn" tokens here - because that's what's happening.
Also FYI this shouldn't be necessary once the Dec->Int Update is made due to an update to RemoveDelegatorShares https://github.com/cosmos/cosmos-sdk/pull/2958/files#diff-ff4682cf67c96b47c013393f099d1d0eR407

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comment, added TODO.


// 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 {
Expand Down
Loading