From 5c939cfab7416c40979d608e7f0973ff96fd1c65 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 4 Jul 2018 00:08:37 +0200 Subject: [PATCH 1/9] Demonstrative testcase, export GetCliffValidator --- x/stake/handler_test.go | 46 +++++++++++++++++++++++++++++++++++++ x/stake/keeper/validator.go | 8 +++---- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index b18b3c98b222..fd4e55e089b8 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -560,6 +560,52 @@ func TestTransitiveRedelegation(t *testing.T) { require.True(t, got.IsOK(), "expected no error") } +func TestCliffValidator(t *testing.T) { + ctx, _, keeper := keep.CreateTestInput(t, false, 1000) + validatorAddr, 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) + + // create the validators + msgCreateValidator := newTestMsgCreateValidator(validatorAddr, keep.PKs[0], 10) + got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper) + require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") + + msgCreateValidator = newTestMsgCreateValidator(validatorAddr2, keep.PKs[1], 5) + got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper) + require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") + + msgCreateValidator = newTestMsgCreateValidator(validatorAddr3, keep.PKs[2], 10) + got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper) + require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") + + // initially should be the third validator + cliffVal := keeper.GetCliffValidator(ctx) + require.Equal(t, validatorAddr3, sdk.Address(cliffVal)) + + // unbond the third validator + msgBeginUnbonding := NewMsgBeginUnbonding(validatorAddr3, validatorAddr3, sdk.NewRat(10)) + got = handleMsgBeginUnbonding(ctx, msgBeginUnbonding, keeper) + require.True(t, got.IsOK(), "expected no error on runMsgBeginUnbonding") + + // unbond the second validator + msgBeginUnbonding = NewMsgBeginUnbonding(validatorAddr2, validatorAddr2, sdk.NewRat(5)) + 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, 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/validator.go b/x/stake/keeper/validator.go index 0d556f7dc4bb..109dd4a3cd29 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -254,7 +254,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 @@ -288,7 +288,7 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, 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 @@ -510,13 +510,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) } From 3cef4557234438fd688caf0aae8f555cdb0d1710 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Thu, 5 Jul 2018 20:24:31 -0400 Subject: [PATCH 2/9] fill in cliff validator test --- x/stake/handler_test.go | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index fd4e55e089b8..a3115462fe6a 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -564,34 +564,53 @@ func TestCliffValidator(t *testing.T) { ctx, _, keeper := keep.CreateTestInput(t, false, 1000) validatorAddr, 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) - // create the validators - msgCreateValidator := newTestMsgCreateValidator(validatorAddr, keep.PKs[0], 10) + // add the first validator + msgCreateValidator := newTestMsgCreateValidator(validatorAddr, keep.PKs[0], 30) got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper) require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") - msgCreateValidator = newTestMsgCreateValidator(validatorAddr2, keep.PKs[1], 5) + // 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], 50) got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper) require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") + // now that we've reached maximum validators, the val-1 should be added to the cliff (top) + cliffVal = keeper.GetCliffValidator(ctx) + require.Equal(t, validatorAddr2, 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") - // initially should be the third validator - cliffVal := keeper.GetCliffValidator(ctx) - require.Equal(t, validatorAddr3, sdk.Address(cliffVal)) + cliffVal = keeper.GetCliffValidator(ctx) + require.Equal(t, validatorAddr2, sdk.Address(cliffVal)) - // unbond the third validator - msgBeginUnbonding := NewMsgBeginUnbonding(validatorAddr3, validatorAddr3, sdk.NewRat(10)) + // unbond the first validator + msgBeginUnbonding := NewMsgBeginUnbonding(validatorAddr1, validatorAddr1, sdk.NewRat(30)) got = handleMsgBeginUnbonding(ctx, msgBeginUnbonding, keeper) require.True(t, got.IsOK(), "expected no error on runMsgBeginUnbonding") + // now the validator set should be updated, + // where val-3 enters the validator set on the cliff + cliffVal = keeper.GetCliffValidator(ctx) + require.Equal(t, validatorAddr2, sdk.Address(cliffVal)) + // unbond the second validator msgBeginUnbonding = NewMsgBeginUnbonding(validatorAddr2, validatorAddr2, sdk.NewRat(5)) got = handleMsgBeginUnbonding(ctx, msgBeginUnbonding, keeper) @@ -603,7 +622,7 @@ func TestCliffValidator(t *testing.T) { // cliff now should be empty cliffVal = keeper.GetCliffValidator(ctx) - require.Equal(t, nil, cliffVal) + require.Equal(t, []byte(nil), cliffVal) } func TestBondUnbondRedelegateSlashTwice(t *testing.T) { From 926a6160c99108ec3b478c5c8d492bd386bc5548 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Fri, 6 Jul 2018 03:04:01 -0400 Subject: [PATCH 3/9] rearrage cliff tests to reveal new problem --- x/stake/handler_test.go | 25 ++++++++++++----------- x/stake/keeper/validator.go | 34 +++++++++++++++++++++++++++----- x/stake/keeper/validator_test.go | 1 + 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index a3115462fe6a..0f79b253d059 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -562,7 +562,7 @@ func TestTransitiveRedelegation(t *testing.T) { func TestCliffValidator(t *testing.T) { ctx, _, keeper := keep.CreateTestInput(t, false, 1000) - validatorAddr, validatorAddr2, validatorAddr3 := keep.Addrs[0], keep.Addrs[1], keep.Addrs[2] + 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) @@ -575,7 +575,7 @@ func TestCliffValidator(t *testing.T) { keeper.SetParams(ctx, params) // add the first validator - msgCreateValidator := newTestMsgCreateValidator(validatorAddr, keep.PKs[0], 30) + msgCreateValidator := newTestMsgCreateValidator(validatorAddr1, keep.PKs[0], 50) got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper) require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") @@ -584,13 +584,13 @@ func TestCliffValidator(t *testing.T) { require.Equal(t, []byte(nil), cliffVal) // Add the second validator - msgCreateValidator = newTestMsgCreateValidator(validatorAddr2, keep.PKs[1], 50) + 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-1 should be added to the cliff (top) + // 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, cliffVal) + 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 @@ -599,25 +599,28 @@ func TestCliffValidator(t *testing.T) { require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") cliffVal = keeper.GetCliffValidator(ctx) - require.Equal(t, validatorAddr2, sdk.Address(cliffVal)) + require.Equal(t, validatorAddr2.Bytes(), cliffVal) - // unbond the first validator - msgBeginUnbonding := NewMsgBeginUnbonding(validatorAddr1, validatorAddr1, sdk.NewRat(30)) + // unbond the second validator + 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, validatorAddr2, sdk.Address(cliffVal)) + require.Equal(t, validatorAddr3.Bytes(), cliffVal) // unbond the second validator - msgBeginUnbonding = NewMsgBeginUnbonding(validatorAddr2, validatorAddr2, sdk.NewRat(5)) + msgBeginUnbonding = NewMsgBeginUnbonding(validatorAddr1, validatorAddr1, sdk.NewRat(5)) 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) + vals = keeper.GetValidatorsBonded(ctx) require.Equal(t, 1, len(vals)) // cliff now should be empty diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index 109dd4a3cd29..2d237e46b5fd 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -284,6 +284,7 @@ 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) { + fmt.Println("wackydebugoutput UpdateBondedValidators 0") store := ctx.KVStore(k.storeKey) @@ -296,14 +297,24 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, bondedValidatorsCount := 0 var validator types.Validator for { + fmt.Println("wackydebugoutput UpdateBondedValidators 1") if !iterator.Valid() || bondedValidatorsCount > int(maxValidators-1) { + fmt.Println("wackydebugoutput UpdateBondedValidators 2") + fmt.Printf("debug bondedValidatorsCount: %v\n", bondedValidatorsCount) + fmt.Printf("debug maxValidators: %v\n", maxValidators) // TODO benchmark if we should read the current power and not write if it's the same if bondedValidatorsCount == int(maxValidators) { // is cliff validator + fmt.Println("wackydebugoutput UpdateBondedValidators 3") k.setCliffValidator(ctx, validator, k.GetPool(ctx)) + } else { + fmt.Println("wackydebugoutput UpdateBondedValidators 4") + k.clearCliffValidator(ctx) } + fmt.Println("wackydebugoutput UpdateBondedValidators 5") break } + fmt.Println("wackydebugoutput UpdateBondedValidators 6") // either retrieve the original validator from the store, or under the // situation that this is the "new validator" just use the validator @@ -311,44 +322,59 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, // store ownerAddr := iterator.Value() if bytes.Equal(ownerAddr, newValidator.Owner) { + fmt.Println("wackydebugoutput UpdateBondedValidators 7") validator = newValidator } else { + fmt.Println("wackydebugoutput UpdateBondedValidators 8") var found bool validator, found = k.GetValidator(ctx, ownerAddr) if !found { + fmt.Println("wackydebugoutput UpdateBondedValidators 9") panic(fmt.Sprintf("validator record not found for address: %v\n", ownerAddr)) } + fmt.Println("wackydebugoutput UpdateBondedValidators 10") } + fmt.Println("wackydebugoutput UpdateBondedValidators 11") // if not previously a validator (and unrevoked), // kick the cliff validator / bond this new validator if validator.Status() != sdk.Bonded && !validator.Revoked { + fmt.Println("wackydebugoutput UpdateBondedValidators 12") kickCliffValidator = true validator = k.bondValidator(ctx, validator) if bytes.Equal(ownerAddr, newValidator.Owner) { + fmt.Println("wackydebugoutput UpdateBondedValidators 13") updatedVal = validator } + fmt.Println("wackydebugoutput UpdateBondedValidators 14") } + 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)) - } else { - bondedValidatorsCount++ } + fmt.Println("wackydebugoutput UpdateBondedValidators 18") + bondedValidatorsCount++ iterator.Next() } + fmt.Println("wackydebugoutput UpdateBondedValidators 19") iterator.Close() // perform the actual kicks if oldCliffValidatorAddr != nil && kickCliffValidator { + fmt.Println("wackydebugoutput UpdateBondedValidators 20") validator, found := k.GetValidator(ctx, oldCliffValidatorAddr) if !found { + fmt.Println("wackydebugoutput UpdateBondedValidators 21") panic(fmt.Sprintf("validator record not found for address: %v\n", oldCliffValidatorAddr)) } + fmt.Println("wackydebugoutput UpdateBondedValidators 22") k.unbondValidator(ctx, validator) } + fmt.Println("wackydebugoutput UpdateBondedValidators 23") return } @@ -405,10 +431,8 @@ func (k Keeper) UpdateBondedValidatorsFull(ctx sdk.Context) { if validator.Revoked && validator.Status() == sdk.Bonded { panic(fmt.Sprintf("revoked validator cannot be bonded, address: %v\n", ownerAddr)) - } else { - bondedValidatorsCount++ } - + bondedValidatorsCount++ iterator.Next() } iterator.Close() diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index c4d197a36b47..807c16012b2f 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -634,6 +634,7 @@ func TestGetTendermintUpdatesInserted(t *testing.T) { require.Equal(t, validators[4].ABCIValidator(), updates[0]) } +//TODO why is this called NotValidatorCliff? func TestGetTendermintUpdatesNotValidatorCliff(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) params := types.DefaultParams() From 5d2b6c81ea2f62c3dc36df3cb7c22db1cdb0c01a Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Fri, 6 Jul 2018 18:41:04 -0400 Subject: [PATCH 4/9] split out cliff problems to more tests --- x/stake/handler_test.go | 107 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 105 insertions(+), 2 deletions(-) diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index 0f79b253d059..de07b25a37cd 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -560,10 +560,47 @@ func TestTransitiveRedelegation(t *testing.T) { require.True(t, got.IsOK(), "expected no error") } -func TestCliffValidator(t *testing.T) { +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, 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))) +} + +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) @@ -591,6 +628,72 @@ func TestCliffValidator(t *testing.T) { // 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 @@ -601,7 +704,7 @@ func TestCliffValidator(t *testing.T) { cliffVal = keeper.GetCliffValidator(ctx) require.Equal(t, validatorAddr2.Bytes(), cliffVal) - // unbond the second validator + // 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") From 37a5f2fd4c7ebfea689604637670457bf3678bd9 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Fri, 6 Jul 2018 19:53:07 -0400 Subject: [PATCH 5/9] 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 4808b6db84a8..c8695add26fc 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 de07b25a37cd..684a6b700bf0 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 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 2c443ec219f8..71eb2920cc18 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()) From 062a683d11c9152bd8945dcb5978ee8d495cd7c6 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Fri, 6 Jul 2018 20:00:33 -0400 Subject: [PATCH 6/9] completely fix dem tests --- x/stake/handler_test.go | 10 +++---- x/stake/keeper/validator.go | 52 ------------------------------------- 2 files changed, 3 insertions(+), 59 deletions(-) diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index 684a6b700bf0..d842d25fae09 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -1,7 +1,6 @@ package stake import ( - "fmt" "testing" "github.com/stretchr/testify/assert" @@ -588,13 +587,10 @@ 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, the queued // validator (aka. validator-1) should make it into the bonded group, thus @@ -713,7 +709,7 @@ func TestCliffValidator(t *testing.T) { cliffVal = keeper.GetCliffValidator(ctx) require.Equal(t, validatorAddr2.Bytes(), cliffVal) - // unbond the valdator-2 + // 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") @@ -726,8 +722,8 @@ func TestCliffValidator(t *testing.T) { cliffVal = keeper.GetCliffValidator(ctx) require.Equal(t, validatorAddr3.Bytes(), cliffVal) - // unbond the second validator - msgBeginUnbonding = NewMsgBeginUnbonding(validatorAddr1, validatorAddr1, sdk.NewRat(5)) + // 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") diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index 71eb2920cc18..bd5ee4a62e34 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -195,23 +195,19 @@ 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 @@ -219,47 +215,37 @@ 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 @@ -267,27 +253,21 @@ 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 } @@ -306,8 +286,6 @@ 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, affectedValidator types.Validator) (updatedVal types.Validator) { - fmt.Printf("debug affectedValidator: %#v\n", affectedValidator) - fmt.Println("wackydebugoutput UpdateBondedValidators 0") store := ctx.KVStore(k.storeKey) @@ -320,63 +298,42 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, bondedValidatorsCount := 0 var validator types.Validator 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) - fmt.Printf("debug maxValidators: %v\n", maxValidators) // TODO benchmark if we should read the current power and not write if it's the same if bondedValidatorsCount == int(maxValidators) { // is cliff validator - fmt.Println("wackydebugoutput UpdateBondedValidators 3") k.setCliffValidator(ctx, validator, k.GetPool(ctx)) } else { - fmt.Println("wackydebugoutput UpdateBondedValidators 4") k.clearCliffValidator(ctx) } - fmt.Println("wackydebugoutput UpdateBondedValidators 5") break } - fmt.Println("wackydebugoutput UpdateBondedValidators 6") // either retrieve the original validator from the store, or under the // situation that this is the "new validator" just use the validator // provided because it has not yet been updated in the main validator // store ownerAddr := iterator.Value() - 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 = affectedValidator } else { - fmt.Println("wackydebugoutput UpdateBondedValidators 8") var found bool validator, found = k.GetValidator(ctx, ownerAddr) if !found { - fmt.Println("wackydebugoutput UpdateBondedValidators 9") panic(fmt.Sprintf("validator record not found for address: %v\n", ownerAddr)) } - fmt.Println("wackydebugoutput UpdateBondedValidators 10") } - fmt.Println("wackydebugoutput UpdateBondedValidators 11") // if not previously a validator (and unrevoked), // kick the cliff validator / bond this new validator if validator.Status() != sdk.Bonded && !validator.Revoked { - fmt.Println("wackydebugoutput UpdateBondedValidators 12") kickCliffValidator = true validator = k.bondValidator(ctx, validator) if bytes.Equal(ownerAddr, affectedValidator.Owner) { - fmt.Println("wackydebugoutput UpdateBondedValidators 13") updatedVal = validator } - fmt.Println("wackydebugoutput UpdateBondedValidators 14") } - fmt.Println("wackydebugoutput UpdateBondedValidators 15") if !validator.Revoked { bondedValidatorsCount++ @@ -388,21 +345,16 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, iterator.Next() } - fmt.Println("wackydebugoutput UpdateBondedValidators 19") iterator.Close() // perform the actual kicks if oldCliffValidatorAddr != nil && kickCliffValidator { - fmt.Println("wackydebugoutput UpdateBondedValidators 20") validator, found := k.GetValidator(ctx, oldCliffValidatorAddr) if !found { - fmt.Println("wackydebugoutput UpdateBondedValidators 21") panic(fmt.Sprintf("validator record not found for address: %v\n", oldCliffValidatorAddr)) } - fmt.Println("wackydebugoutput UpdateBondedValidators 22") k.unbondValidator(ctx, validator) } - fmt.Println("wackydebugoutput UpdateBondedValidators 23") return } @@ -506,17 +458,14 @@ 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) @@ -525,7 +474,6 @@ 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()) From f08f02d67dae4d2c31d687079c2e560cd14d8e69 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Fri, 6 Jul 2018 20:04:01 -0400 Subject: [PATCH 7/9] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8695add26fc..6f949b97ef70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,6 +116,7 @@ BUG FIXES * \#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 From d36dda026748c8ababeb5cfd3890ad22bd39fd6d Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Fri, 6 Jul 2018 20:29:39 -0400 Subject: [PATCH 8/9] address cwgoes comments --- x/stake/keeper/validator.go | 12 ++++++++---- x/stake/keeper/validator_test.go | 3 +-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index bd5ee4a62e34..233c7b6e87c0 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -303,7 +303,7 @@ 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 { + } else if len(oldCliffValidatorAddr) > 0 { k.clearCliffValidator(ctx) } break @@ -409,10 +409,14 @@ 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)) + if !validator.Revoked { + bondedValidatorsCount++ + } else { + if validator.Status() == sdk.Bonded { + panic(fmt.Sprintf("revoked validator cannot be bonded, address: %v\n", ownerAddr)) + } } - bondedValidatorsCount++ + iterator.Next() } iterator.Close() diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index f29fba7eb207..41aca3ea529f 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -634,8 +634,7 @@ func TestGetTendermintUpdatesInserted(t *testing.T) { require.Equal(t, validators[4].ABCIValidator(), updates[0]) } -//TODO why is this called NotValidatorCliff? -func TestGetTendermintUpdatesNotValidatorCliff(t *testing.T) { +func TestGetTendermintUpdatesWithCliffValidator(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) params := types.DefaultParams() params.MaxValidators = 2 From 3b7fbf66ab1504d5f005395097d71ba720d02094 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Sat, 7 Jul 2018 02:37:59 +0200 Subject: [PATCH 9/9] assert -> require --- x/stake/handler_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index d842d25fae09..e4441bc2e6a1 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -3,7 +3,6 @@ package stake import ( "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/tendermint/tendermint/crypto" @@ -596,10 +595,10 @@ func TestUnbondingWhenExcessValidators(t *testing.T) { // 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) + require.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) + require.Equal(t, sdk.Bonded, val1.Status(), "%v", val1) } func TestJoiningAsCliffValidator(t *testing.T) {