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)