R4R: Ensure Tendermint Validator Update Invariants

PENDING.md

@@ -121,6 +121,8 @@ BUG FIXES
     * [cli] [\#2265](https://github.com/cosmos/cosmos-sdk/issues/2265) Fix JSON formatting of the `gaiacli send` command.
 
 * 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](https://github.com/cosmos/cosmos-sdk/issues/1988) Make us compile on OpenBSD (disable ledger) [#1988] (https://github.com/cosmos/cosmos-sdk/issues/1988)

docs/spec/staking/end_block.md

@@ -12,7 +12,7 @@ the changes cleared
 
 ```golang
 EndBlock() ValidatorSetChanges
-    vsc = GetTendermintUpdates()
+    vsc = GetValidTendermintUpdates()
     ClearTendermintUpdates()
     return vsc
 ```

x/gov/simulation/msgs.go

@@ -156,13 +156,16 @@ func SimulateMsgVote(k gov.Keeper, sk stake.Keeper) simulation.Operation {
 	return operationSimulateMsgVote(k, sk, nil, -1)
 }
 
+
 // nolint: unparam
 func operationSimulateMsgVote(k gov.Keeper, sk stake.Keeper, key crypto.PrivKey, proposalID int64) simulation.Operation {
 	return func(r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, keys []crypto.PrivKey, event func(string)) (action string, fOp []simulation.FutureOperation, err error) {
 		if key == nil {
 			key = simulation.RandomKey(r, keys)
 		}
+
 		var ok bool
+
 		if proposalID < 0 {
 			proposalID, ok = randomProposalID(r, k, ctx)
 			if !ok {
@@ -171,15 +174,18 @@ func operationSimulateMsgVote(k gov.Keeper, sk stake.Keeper, key crypto.PrivKey,
 		}
 		addr := sdk.AccAddress(key.PubKey().Address())
 		option := randomVotingOption(r)
+
 		msg := gov.NewMsgVote(addr, proposalID, option)
 		if msg.ValidateBasic() != nil {
 			return "", nil, fmt.Errorf("expected msg to pass ValidateBasic: %s", msg.GetSignBytes())
 		}
+
 		ctx, write := ctx.CacheContext()
 		result := gov.NewHandler(k)(ctx, msg)
 		if result.IsOK() {
 			write()
 		}
+
 		event(fmt.Sprintf("gov/MsgVote/%v", result.IsOK()))
 		action = fmt.Sprintf("TestMsgVote: ok %v, msg %s", result.IsOK(), msg.GetSignBytes())
 		return action, nil, nil

x/mock/simulation/random_simulate_blocks.go

@@ -342,18 +342,14 @@ func RandomRequestBeginBlock(r *rand.Rand, validators map[string]mockValidator,
 // updateValidators mimicks Tendermint's update logic
 // nolint: unparam
 func updateValidators(tb testing.TB, r *rand.Rand, 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")
+			}
+
 			event("endblock/validatorupdates/kicked")
 			delete(current, string(update.PubKey.Data))
 		default:
@@ -368,5 +364,6 @@ func updateValidators(tb testing.TB, r *rand.Rand, current map[string]mockValida
 			}
 		}
 	}
+
 	return current
 }

x/stake/handler.go

@@ -52,7 +52,7 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) (ValidatorUpdates []abci.Valid
 	k.SetIntraTxCounter(ctx, 0)
 
 	// calculate validator set changes
-	ValidatorUpdates = k.GetTendermintUpdates(ctx)
+	ValidatorUpdates = k.GetValidTendermintUpdates(ctx)
 	k.ClearTendermintUpdates(ctx)
 	return
 }

x/stake/keeper/validator.go

@@ -198,16 +198,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
-func (k Keeper) GetTendermintUpdates(ctx sdk.Context) (updates []abci.Validator) {
+//
+// 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) GetValidTendermintUpdates(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() && val.BondHeight > val.UnbondingHeight
+			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
 }
@@ -240,7 +261,7 @@ func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) type
 
 	validator = k.updateForJailing(ctx, oldFound, oldValidator, validator)
 	powerIncreasing := k.getPowerIncreasing(ctx, oldFound, oldValidator, validator)
-	validator.BondHeight, validator.BondIntraTxCounter = k.bondIncrement(ctx, oldFound, oldValidator, validator)
+	validator.BondHeight, validator.BondIntraTxCounter = k.bondIncrement(ctx, oldFound, oldValidator)
 	valPower := k.updateValidatorPower(ctx, oldFound, oldValidator, validator, pool)
 	cliffPower := k.GetCliffValidatorPower(ctx)
 	cliffValExists := (cliffPower != nil)
@@ -316,7 +337,7 @@ func (k Keeper) updateCliffValidator(ctx sdk.Context, affectedVal types.Validato
 
 	oldCliffVal, found := k.GetValidator(ctx, cliffAddr)
 	if !found {
-		panic(fmt.Sprintf("cliff validator record not found for address: %v\n", cliffAddr))
+		panic(fmt.Sprintf("cliff validator record not found for address: %X\n", cliffAddr))
 	}
 
 	// Create a validator iterator ranging from smallest to largest by power
@@ -331,7 +352,7 @@ func (k Keeper) updateCliffValidator(ctx sdk.Context, affectedVal types.Validato
 		ensureValidatorFound(found, ownerAddr)
 
 		if currVal.Status != sdk.Bonded || currVal.Jailed {
-			panic(fmt.Sprintf("unexpected jailed or unbonded validator for address: %s\n", ownerAddr))
+			panic(fmt.Sprintf("unexpected jailed or unbonded validator for address: %X\n", ownerAddr))
 		}
 
 		newCliffVal = currVal
@@ -344,13 +365,10 @@ func (k Keeper) updateCliffValidator(ctx sdk.Context, affectedVal types.Validato
 	newCliffValRank := GetValidatorsByPowerIndexKey(newCliffVal, pool)
 
 	if bytes.Equal(affectedVal.OperatorAddr, newCliffVal.OperatorAddr) {
-
 		// The affected validator remains the cliff validator, however, since
 		// the store does not contain the new power, update the new power rank.
 		store.Set(ValidatorPowerCliffKey, affectedValRank)
-
 	} else if bytes.Compare(affectedValRank, newCliffValRank) > 0 {
-
 		// The affected validator no longer remains the cliff validator as it's
 		// power is greater than the new cliff validator.
 		k.setCliffValidator(ctx, newCliffVal, pool)
@@ -381,18 +399,20 @@ func (k Keeper) getPowerIncreasing(ctx sdk.Context, oldFound bool, oldValidator,
 
 // get the bond height and incremented intra-tx counter
 // nolint: unparam
-func (k Keeper) bondIncrement(ctx sdk.Context, oldFound bool, oldValidator,
-	newValidator types.Validator) (height int64, intraTxCounter int16) {
+func (k Keeper) bondIncrement(
+	ctx sdk.Context, found bool, oldValidator types.Validator) (height int64, intraTxCounter int16) {
 
-	// if already a validator, copy the old block height and counter, else set them
-	if oldFound && oldValidator.Status == sdk.Bonded {
+	// if already a validator, copy the old block height and counter
+	if found && oldValidator.Status == sdk.Bonded {
 		height = oldValidator.BondHeight
 		intraTxCounter = oldValidator.BondIntraTxCounter
 		return
 	}
+
 	height = ctx.BlockHeight()
 	counter := k.GetIntraTxCounter(ctx)
 	intraTxCounter = counter
+
 	k.SetIntraTxCounter(ctx, counter+1)
 	return
 }
@@ -458,10 +478,15 @@ func (k Keeper) UpdateBondedValidators(
 
 		// if we've reached jailed validators no further bonded validators exist
 		if validator.Jailed {
+			if validator.Status == sdk.Bonded {
+				panic(fmt.Sprintf("jailed validator cannot be bonded, address: %X\n", ownerAddr))
+			}
+
 			break
 		}
 
-		// increment bondedValidatorsCount / get the validator to bond
+		// increment the total number of bonded validators and potentially mark
+		// the validator to bond
 		if validator.Status != sdk.Bonded {
 			validatorToBond = validator
 			if newValidatorBonded {
@@ -470,13 +495,6 @@ func (k Keeper) UpdateBondedValidators(
 			newValidatorBonded = true
 		}
 
-		// increment the total number of bonded validators and potentially mark
-		// the validator to bond
-		if validator.Status != sdk.Bonded {
-			validatorToBond = validator
-			newValidatorBonded = true
-		}
-
 		bondedValidatorsCount++
 		iterator.Next()
 	}
@@ -507,7 +525,6 @@ func (k Keeper) UpdateBondedValidators(
 				// validator was newly bonded and has greater power
 				k.beginUnbondingValidator(ctx, oldCliffVal)
 			} else {
-
 				// otherwise begin unbonding the affected validator, which must
 				// have been kicked out
 				affectedValidator = k.beginUnbondingValidator(ctx, affectedValidator)
@@ -653,6 +670,8 @@ func (k Keeper) bondValidator(ctx sdk.Context, validator types.Validator) types.
 		panic(fmt.Sprintf("should not already be bonded, validator: %v\n", validator))
 	}
 
+	validator.BondHeight = ctx.BlockHeight()
+
 	// set the status
 	validator, pool = validator.UpdateStatus(pool, sdk.Bonded)
 	k.SetPool(ctx, pool)