Delay automatic unbonding until consensus validator updates (cosmos#37)

* Execute validator unbonding at consensus changeover

* Fix producing block status of genesis validators

* Add comments

x/genutil/gentx.go

@@ -102,5 +102,7 @@ func DeliverGenTxs(ctx sdk.Context, cdc *codec.Codec, genTxs []json.RawMessage,
 			panic(res.Log)
 		}
 	}
-	return stakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	updates := stakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	stakingKeeper.ExecuteUnbonding(ctx, updates)
+	return updates
 }

x/genutil/types/expected_keepers.go

@@ -13,6 +13,7 @@ import (
 // StakingKeeper defines the expected staking keeper (noalias)
 type StakingKeeper interface {
 	ApplyAndReturnValidatorSetUpdates(sdk.Context) (updates []abci.ValidatorUpdate)
+	ExecuteUnbonding(sdk.Context, []abci.ValidatorUpdate)
 }
 
 // AccountKeeper defines the expected account keeper (noalias)

x/staking/genesis.go

@@ -33,6 +33,9 @@ func InitGenesis(ctx sdk.Context, keeper Keeper, accountKeeper types.AccountKeep
 	keeper.SetLastTotalPower(ctx, data.LastTotalPower)
 
 	for _, validator := range data.Validators {
+		// At genesis set all validators to producing blocks. The excess one will be removed
+		// in the ExecuteUnbonding call below
+		validator.ProducingBlocks = true
 		keeper.SetValidator(ctx, validator)
 
 		// Manually set indices for the first time
@@ -131,6 +134,7 @@ func InitGenesis(ctx sdk.Context, keeper Keeper, accountKeeper types.AccountKeep
 		}
 	} else {
 		res = keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+		keeper.ExecuteUnbonding(ctx, res)
 	}
 
 	return res

x/staking/genesis_test.go

@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/tendermint/tendermint/crypto/ed25519"
+	tmtypes "github.com/tendermint/tendermint/types"
 
 	"github.com/stretchr/testify/assert"
 
@@ -97,6 +98,15 @@ func TestInitGenesisLargeValidatorSet(t *testing.T) {
 	}
 
 	require.Equal(t, abcivals, vals)
+	for _, val := range vals {
+		pubKey, err := tmtypes.PB2TM.PubKey(val.PubKey)
+		if err != nil {
+			panic(fmt.Sprintf("Error converting public key %v in validator updates", val.PubKey))
+		}
+		validator, found := keeper.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(pubKey))
+		require.True(t, found)
+		require.True(t, validator.ProducingBlocks)
+	}
 }
 
 func TestValidateGenesis(t *testing.T) {

x/staking/handler_test.go

@@ -32,6 +32,7 @@ func TestValidatorByPowerIndex(t *testing.T) {
 
 	// must end-block
 	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 1, len(updates))
 
 	// verify the self-delegation exists
@@ -55,12 +56,14 @@ func TestValidatorByPowerIndex(t *testing.T) {
 	// must end-block
 	updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx)
 	require.Equal(t, 1, len(updates))
+	keeper.ExecuteUnbonding(ctx, updates)
 
 	// slash and jail the first validator
 	consAddr0 := sdk.ConsAddress(keep.PKs[0].Address())
 	keeper.Slash(ctx, consAddr0, 0, initPower, sdk.NewDecWithPrec(5, 1))
 	keeper.Jail(ctx, consAddr0)
-	keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 
 	validator, found = keeper.GetValidator(ctx, validatorAddr)
 	require.True(t, found)
@@ -1221,7 +1224,8 @@ func TestUnbondingWhenExcessValidators(t *testing.T) {
 	require.NotNil(t, res)
 
 	// apply TM updates
-	keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 1, len(keeper.GetLastValidators(ctx)))
 
 	valTokens2 := sdk.TokensFromConsensusPower(30)
@@ -1231,7 +1235,8 @@ func TestUnbondingWhenExcessValidators(t *testing.T) {
 	require.NotNil(t, res)
 
 	// apply TM updates
-	keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 2, len(keeper.GetLastValidators(ctx)))
 
 	valTokens3 := sdk.TokensFromConsensusPower(10)
@@ -1241,7 +1246,8 @@ func TestUnbondingWhenExcessValidators(t *testing.T) {
 	require.NotNil(t, res)
 
 	// apply TM updates
-	keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 2, len(keeper.GetLastValidators(ctx)))
 
 	// unbond the validator-2
@@ -1252,7 +1258,8 @@ func TestUnbondingWhenExcessValidators(t *testing.T) {
 	require.NotNil(t, res)
 
 	// apply TM updates
-	keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 
 	// because there are extra validators waiting to get in, the queued
 	// validator (aka. validator-1) should make it into the bonded group, thus

x/staking/keeper/delay_updates.go

@@ -33,13 +33,19 @@ func (k Keeper) CheckValidatorUpdates(ctx sdk.Context, header abci.Header) {
 func (k Keeper) DKGValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate {
 	store := ctx.KVStore(k.storeKey)
 	if len(store.Get(computeDKGValidatorUpdateKey)) == 0 {
-		// Check mature items in queues every block, regardless of whether return validator updates
-		// or not, in order for items to be removed as soon as possible
-		k.RemoveMatureQueueItems(ctx)
 		return []abci.ValidatorUpdate{}
 	}
 	store.Set(computeDKGValidatorUpdateKey, []byte{})
-	updates := k.BlockValidatorUpdates(ctx)
+	// Calculate validator set changes.
+	//
+	// NOTE: ApplyAndReturnValidatorSetUpdates has to come before
+	// UnbondAllMatureValidatorQueue.
+	// This fixes a bug when the unbonding period is instant (is the case in
+	// some of the tests). The test expected the validator to be completely
+	// unbonded after the Endblocker (go from Bonded -> Unbonding during
+	// ApplyAndReturnValidatorSetUpdates and then Unbonding -> Unbonded during
+	// UnbondAllMatureValidatorQueue).
+	updates := k.ApplyAndReturnValidatorSetUpdates(ctx)
 	store.Set(validatorUpdatesKey, k.cdc.MustMarshalBinaryLengthPrefixed(updates))
 	return updates
 }
@@ -49,11 +55,16 @@ func (k Keeper) DKGValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate {
 func (k Keeper) ValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate {
 	store := ctx.KVStore(k.storeKey)
 	if len(store.Get(computeValidatorUpdateKey)) == 0 {
+		// Check mature items in queues every block, regardless of whether return validator updates
+		// or not, in order for items to be removed as soon as possible
+		k.RemoveMatureQueueItems(ctx)
 		return []abci.ValidatorUpdate{}
 	}
 	store.Set(computeValidatorUpdateKey, []byte{})
 	updateBytes := store.Get(validatorUpdatesKey)
 	updates := []abci.ValidatorUpdate{}
 	k.cdc.UnmarshalBinaryLengthPrefixed(updateBytes, &updates)
+	k.ExecuteUnbonding(ctx, updates)
+	k.RemoveMatureQueueItems(ctx)
 	return updates
 }

x/staking/keeper/delegation_test.go

@@ -334,6 +334,7 @@ func TestUndelegateSelfDelegationBelowMinSelfDelegation(t *testing.T) {
 
 	// end block
 	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 1, len(updates))
 
 	validator, found := keeper.GetValidator(ctx, addrVals[0])
@@ -405,6 +406,7 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) {
 
 	// end block
 	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 1, len(updates))
 
 	validator, found := keeper.GetValidator(ctx, addrVals[0])
@@ -478,6 +480,7 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) {
 
 	// end block
 	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 1, len(updates))
 
 	validator, found := keeper.GetValidator(ctx, addrVals[0])
@@ -560,6 +563,7 @@ func TestUnbondingAllDelegationFromValidator(t *testing.T) {
 
 	// end block
 	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 1, len(updates))
 
 	// unbond all the remaining delegation
@@ -789,6 +793,7 @@ func TestRedelegateSelfDelegation(t *testing.T) {
 
 	// end block
 	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 2, len(updates))
 
 	validator, found := keeper.GetValidator(ctx, addrVals[0])
@@ -847,6 +852,7 @@ func TestRedelegateFromUnbondingValidator(t *testing.T) {
 
 	// end block
 	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 1, len(updates))
 
 	validator, found := keeper.GetValidator(ctx, addrVals[0])
@@ -923,6 +929,7 @@ func TestRedelegateFromUnbondedValidator(t *testing.T) {
 
 	// end block
 	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 1, len(updates))
 
 	validator, found := keeper.GetValidator(ctx, addrVals[0])

x/staking/keeper/historical_info_test.go

@@ -71,9 +71,11 @@ func TestTrackHistoricalInfo(t *testing.T) {
 
 	// Set last validators in keeper
 	val1 := types.NewValidator(sdk.ValAddress(Addrs[2]), PKs[2], types.Description{})
+	val1.ProducingBlocks = true
 	k.SetValidator(ctx, val1)
 	k.SetLastValidatorPower(ctx, val1.OperatorAddress, 10)
 	val2 := types.NewValidator(sdk.ValAddress(Addrs[3]), PKs[3], types.Description{})
+	val2.ProducingBlocks = true
 	vals := []types.Validator{val1, val2}
 	sort.Sort(types.Validators(vals))
 	k.SetValidator(ctx, val2)

x/staking/keeper/slash_test.go

@@ -262,6 +262,7 @@ func TestSlashWithUnbondingDelegation(t *testing.T) {
 
 	// end block
 	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	require.Equal(t, 1, len(updates))
 
 	// read updating unbonding delegation
@@ -342,7 +343,9 @@ func TestSlashWithUnbondingDelegation(t *testing.T) {
 	diffTokens = oldBondedPool.GetCoins().Sub(newBondedPool.GetCoins()).AmountOf(keeper.BondDenom(ctx))
 	require.Equal(t, sdk.TokensFromConsensusPower(10), diffTokens)
 	// apply TM updates
-	keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	updates = keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
+
 	// read updated validator
 	// power decreased by 1 again, validator is out of stake
 	// validator should be in unbonding period
@@ -453,7 +456,8 @@ func TestSlashWithRedelegation(t *testing.T) {
 	require.True(t, found)
 	require.Len(t, rd.Entries, 1)
 	// apply TM updates
-	keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+	keeper.ExecuteUnbonding(ctx, updates)
 	// read updated validator
 	// validator decreased to zero power, should be in unbonding period
 	validator, _ = keeper.GetValidatorByConsAddr(ctx, consAddr)

x/staking/keeper/test_common.go

@@ -246,6 +246,7 @@ func ValidatorByPowerIndexExists(ctx sdk.Context, keeper Keeper, power []byte) b
 // update validator for testing
 func TestingUpdateValidator(keeper Keeper, ctx sdk.Context, validator types.Validator, apply bool) types.Validator {
 	keeper.SetValidator(ctx, validator)
+	keeper.SetValidatorByConsAddr(ctx, validator)
 
 	// Remove any existing power key for validator.
 	store := ctx.KVStore(keeper.storeKey)
@@ -266,15 +267,17 @@ func TestingUpdateValidator(keeper Keeper, ctx sdk.Context, validator types.Vali
 
 	keeper.SetValidatorByPowerIndex(ctx, validator)
 	if apply {
-		keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+		updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
+		keeper.ExecuteUnbonding(ctx, updates)
 		validator, found := keeper.GetValidator(ctx, validator.OperatorAddress)
 		if !found {
 			panic("validator expected but not found")
 		}
 		return validator
 	}
 	cachectx, _ := ctx.CacheContext()
-	keeper.ApplyAndReturnValidatorSetUpdates(cachectx)
+	updates := keeper.ApplyAndReturnValidatorSetUpdates(cachectx)
+	keeper.ExecuteUnbonding(cachectx, updates)
 	validator, found := keeper.GetValidator(cachectx, validator.OperatorAddress)
 	if !found {
 		panic("validator expected but not found")