R4R: Ensure Tendermint Validator Update Invariants

PENDING.md

@@ -100,6 +100,8 @@ BUG FIXES
     * [cli] \#1997 Handle panics gracefully when `gaiacli stake {delegation,unbond}` fail to unmarshal delegation.
 
 * Gaia
+  * [x/stake] Return correct Tendermint validator update set on `EndBlocker` by not
+  including non previously bonded validators that have zero power. [#2189](https://github.com/cosmos/cosmos-sdk/issues/2189)
 
 * SDK
     * \#1988 Make us compile on OpenBSD (disable ledger) [#1988] (https://github.com/cosmos/cosmos-sdk/issues/1988)

x/mock/simulation/random_simulate_blocks.go

@@ -129,7 +129,7 @@ func SimulateFromSeed(
 		request = RandomRequestBeginBlock(r, validators, livenessTransitionMatrix, evidenceFraction, pastTimes, pastSigningValidators, event, header, log)
 
 		// Update the validator set
-		validators = updateValidators(tb, r, validators, res.ValidatorUpdates, event)
+		validators = updateValidators(tb, r, log, validators, res.ValidatorUpdates, event)
 	}
 
 	fmt.Printf("\nSimulation complete. Final height (blocks): %d, final time (seconds), : %v, operations ran %d\n", header.Height, header.Time, opCount)
@@ -320,19 +320,14 @@ func AssertAllInvariants(t *testing.T, app *baseapp.BaseApp, tests []Invariant,
 }
 
 // updateValidators mimicks Tendermint's update logic
-func updateValidators(tb testing.TB, r *rand.Rand, current map[string]mockValidator, updates []abci.Validator, event func(string)) map[string]mockValidator {
+func updateValidators(tb testing.TB, r *rand.Rand, log string, current map[string]mockValidator, updates []abci.Validator, event func(string)) map[string]mockValidator {
 	for _, update := range updates {
 		switch {
 		case update.Power == 0:
-			// // TEMPORARY DEBUG CODE TO PROVE THAT THE OLD METHOD WAS BROKEN
-			// // (i.e. didn't catch in the event of problem)
-			// if val, ok := tb.(*testing.T); ok {
-			// 	require.NotNil(val, current[string(update.PubKey.Data)])
-			// }
-			// // CORRECT CHECK
-			// if _, ok := current[string(update.PubKey.Data)]; !ok {
-			// 	tb.Fatalf("tried to delete a nonexistent validator")
-			// }
+			if _, ok := current[string(update.PubKey.Data)]; !ok {
+				tb.Fatalf("tried to delete a nonexistent validator: %v", log)
+			}
+
 			event("endblock/validatorupdates/kicked")
 			delete(current, string(update.PubKey.Data))
 		default:

x/stake/keeper/validator.go

@@ -201,16 +201,37 @@ func (k Keeper) GetValidatorsByPower(ctx sdk.Context) []types.Validator {
 // Accumulated updates to the active/bonded validator set for tendermint
 
 // get the most recently updated validators
+//
+// CONTRACT: Only validators with non-zero power or zero-power that were bonded
+// at the previous block height or were removed from the validator set entirely
+// are returned to Tendermint.
 func (k Keeper) GetTendermintUpdates(ctx sdk.Context) (updates []abci.Validator) {
 	store := ctx.KVStore(k.storeKey)
 
-	iterator := sdk.KVStorePrefixIterator(store, TendermintUpdatesKey) //smallest to largest
+	iterator := sdk.KVStorePrefixIterator(store, TendermintUpdatesKey)
 	for ; iterator.Valid(); iterator.Next() {
-		valBytes := iterator.Value()
-		var val abci.Validator
-		k.cdc.MustUnmarshalBinary(valBytes, &val)
-		updates = append(updates, val)
+		var abciVal abci.Validator
+
+		abciValBytes := iterator.Value()
+		k.cdc.MustUnmarshalBinary(abciValBytes, &abciVal)
+
+		val, found := k.GetValidator(ctx, abciVal.GetAddress())
+		if found {
+			// The validator is new or already exists in the store and must adhere to
+			// Tendermint invariants.
+			prevBonded := val.BondHeight < ctx.BlockHeight()
+			zeroPower := val.GetPower().Equal(sdk.ZeroDec())
+
+			if !zeroPower || zeroPower && prevBonded {
+				updates = append(updates, abciVal)
+			}
+		} else {
+			// Add the ABCI validator in such a case where the validator was removed
+			// from the store as it must have existed before.
+			updates = append(updates, abciVal)
+		}
 	}
+
 	iterator.Close()
 	return
 }

x/stake/keeper/validator_test.go

@@ -16,8 +16,11 @@ func TestSetValidator(t *testing.T) {
 	ctx, _, keeper := CreateTestInput(t, false, 10)
 	pool := keeper.GetPool(ctx)
 
+	valPubKey := PKs[0]
+	valAddr := sdk.ValAddress(valPubKey.Address().Bytes())
+
 	// test how the validator is set from a purely unbonbed pool
-	validator := types.NewValidator(addrVals[0], PKs[0], types.Description{})
+	validator := types.NewValidator(valAddr, valPubKey, types.Description{})
 	validator, pool, _ = validator.AddTokensFromDel(pool, sdk.NewInt(10))
 	require.Equal(t, sdk.Unbonded, validator.Status)
 	assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.Tokens))
@@ -26,14 +29,14 @@ func TestSetValidator(t *testing.T) {
 	keeper.UpdateValidator(ctx, validator)
 
 	// after the save the validator should be bonded
-	validator, found := keeper.GetValidator(ctx, addrVals[0])
+	validator, found := keeper.GetValidator(ctx, valAddr)
 	require.True(t, found)
 	require.Equal(t, sdk.Bonded, validator.Status)
 	assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.Tokens))
 	assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.DelegatorShares))
 
 	// Check each store for being saved
-	resVal, found := keeper.GetValidator(ctx, addrVals[0])
+	resVal, found := keeper.GetValidator(ctx, valAddr)
 	assert.True(ValEq(t, validator, resVal))
 	require.True(t, found)
 
@@ -641,8 +644,13 @@ func TestClearTendermintUpdates(t *testing.T) {
 	validators := make([]types.Validator, len(amts))
 	for i, amt := range amts {
 		pool := keeper.GetPool(ctx)
-		validators[i] = types.NewValidator(sdk.ValAddress(Addrs[i]), PKs[i], types.Description{})
+
+		valPubKey := PKs[i]
+		valAddr := sdk.ValAddress(valPubKey.Address().Bytes())
+
+		validators[i] = types.NewValidator(valAddr, valPubKey, types.Description{})
 		validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt))
+
 		keeper.SetPool(ctx, pool)
 		keeper.UpdateValidator(ctx, validators[i])
 	}
@@ -661,7 +669,11 @@ func TestGetTendermintUpdatesAllNone(t *testing.T) {
 	var validators [2]types.Validator
 	for i, amt := range amts {
 		pool := keeper.GetPool(ctx)
-		validators[i] = types.NewValidator(sdk.ValAddress(Addrs[i]), PKs[i], types.Description{})
+
+		valPubKey := PKs[i+1]
+		valAddr := sdk.ValAddress(valPubKey.Address().Bytes())
+
+		validators[i] = types.NewValidator(valAddr, valPubKey, types.Description{})
 		validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt))
 		keeper.SetPool(ctx, pool)
 	}
@@ -895,3 +907,78 @@ func TestGetTendermintUpdatesPowerDecrease(t *testing.T) {
 	require.Equal(t, validators[0].ABCIValidator(), updates[0])
 	require.Equal(t, validators[1].ABCIValidator(), updates[1])
 }
+
+func TestGetTendermintUpdatesBonded(t *testing.T) {
+	ctx, _, keeper := CreateTestInput(t, false, 1000)
+	params := keeper.GetParams(ctx)
+	params.MaxValidators = uint16(3)
+
+	keeper.SetParams(ctx, params)
+
+	amts := []int64{100, 100}
+	var validators [2]types.Validator
+
+	// initialize some validators into the state
+	for i, amt := range amts {
+		pool := keeper.GetPool(ctx)
+		valPubKey := PKs[i+1]
+		valAddr := sdk.ValAddress(valPubKey.Address().Bytes())
+
+		validators[i] = types.NewValidator(valAddr, valPubKey, types.Description{})
+		validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt))
+
+		keeper.SetPool(ctx, pool)
+		validators[i] = keeper.UpdateValidator(ctx, validators[i])
+	}
+
+	// verify initial Tendermint updates are correct
+	updates := keeper.GetTendermintUpdates(ctx)
+	require.Equal(t, len(validators), len(updates))
+	require.Equal(t, validators[0].ABCIValidator(), updates[0])
+	require.Equal(t, validators[1].ABCIValidator(), updates[1])
+
+	keeper.ClearTendermintUpdates(ctx)
+	require.Equal(t, 0, len(keeper.GetTendermintUpdates(ctx)))
+
+	// update initial validator set
+	for i, amt := range amts {
+		pool := keeper.GetPool(ctx)
+		validators[i], pool, _ = validators[i].AddTokensFromDel(pool, sdk.NewInt(amt))
+
+		keeper.SetPool(ctx, pool)
+		validators[i] = keeper.UpdateValidator(ctx, validators[i])
+	}
+
+	// add a new validator that goes from zero power, to non-zero power, back to
+	// zero power
+	pool := keeper.GetPool(ctx)
+	valPubKey := PKs[len(validators)+1]
+	valAddr := sdk.ValAddress(valPubKey.Address().Bytes())
+	amt := sdk.NewInt(100)
+
+	validator := types.NewValidator(valAddr, valPubKey, types.Description{})
+	validator, pool, _ = validator.AddTokensFromDel(pool, amt)
+
+	keeper.SetPool(ctx, pool)
+	validator = keeper.UpdateValidator(ctx, validator)
+
+	validator, pool, _ = validator.RemoveDelShares(pool, sdk.NewDecFromInt(amt))
+	validator = keeper.UpdateValidator(ctx, validator)
+
+	// add a new validator that increases in power
+	valPubKey = PKs[len(validators)+2]
+	valAddr = sdk.ValAddress(valPubKey.Address().Bytes())
+
+	validator = types.NewValidator(valAddr, valPubKey, types.Description{})
+	validator, pool, _ = validator.AddTokensFromDel(pool, sdk.NewInt(500))
+
+	keeper.SetPool(ctx, pool)
+	validator = keeper.UpdateValidator(ctx, validator)
+
+	// verify initial Tendermint updates are correct
+	updates = keeper.GetTendermintUpdates(ctx)
+	require.Equal(t, len(validators)+1, len(updates))
+	require.Equal(t, validator.ABCIValidator(), updates[0])
+	require.Equal(t, validators[0].ABCIValidator(), updates[1])
+	require.Equal(t, validators[1].ABCIValidator(), updates[2])
+}