Split up UpdateValidator per each state-transitions, & Tendermint updates in Endblock

[cwgoes]

Split off from #2312 (comment) into a separate issue.

Once we move all validator state changes from UpdateValidator, called on many different transactions, to EndBlock, called once per block (so where we can safely iterate over the top hundred validators), I think we can split up UpdateValidator into several functions to change the state of a validator, each responsible for changing the state of an individual validator from its current state to a known state.

In pseudocode the logic looks something like:

func (ctx sdk.Context) updateValidatorSet() {
  last := fetchOldValidatorSet()
  tendermintUpdates := make(map[sdk.ValAddress]uint64)

  for validator in iterate(top hundred) {
    switch validator.State {
      case Unbonded:
        unbondedToBonded(ctx, validator.Addr)
        tendermintUpdates[validator.Addr] = validator.Power
      case Unbonding:
        unbondingToBonded(ctx, validator.Addr)
        tendermintUpdates[validator.Addr] = validator.Power
      case Bonded: // do nothing
      delete last[validator.Addr]
      // jailed validators are ranked last, so if we get to a jailed validator
      // we have no more bonded validators
      if validator.Jailed break
  }

  for validator in last {
     bondedToUnbonding(ctx, validator.Addr)
     tendermintUpdates[validator.Addr] = 0
  }

  return tendermintUpdates
}

func (ctx sdk.Context, addr sdk.ValAddress) bondedToUnbonding() {
   // perform appropriate store updates
}

func (ctx sdk.Context, addr sdk.ValAddress) unbondingToBonded() {
  // perform appropriate store updates
}

func (ctx sdk.Context, addr sdk.ValAddress) unbondedToBonded() {
  // perform appropriate store updates
}

func (ctx sdk.Context, addr sdk.ValAddress) unbondingToUnbonded() {
  // perform appropriate store updates
}

bondedToUnbonding, unbondingToBonded, and unbondedToBonded should only ever need to be called in this function. With this model, we don't need to worry about validator's jailed status as a state (other than stopping if we hit a jailed validator), it just changes their rank in the power store.

unbondingToUnbonded will be called in the handler of MsgCompleteUnbonding.

Relative to the the current store, this will get rid of the cliff validator slot, the cliff validator power slot, and possibly the bonded validator prefix store - we no longer need to track the "bonded validators" between transactions, but we do need to lookup what the validator set was at the beginning of the block. I don't think that requires a store key, though, we can presumably just use ctx.SigningValidators (note: will have to be stored to offset a block).

cc @rigelrozanski @alexanderbez @ValarDragon Thoughts?

[alexanderbez]

This is great @cwgoes -- really simplifies the staking logic. Are you sure there are no corner cases with jailed validators? What about defensive coding -- what do we need to add?

re; getting the last set. I'm not too familiar with SigningValidators. Does this only include the validators that included precommits on the previous block? What about validators that that missed a precommit/signature...we could potentially need to send a zero power update for them.

[rigelrozanski]

With this model, we don't need to worry about validator's jailed status as a state (other than stopping if we hit a jailed validator), it just changes their rank in the power store

Correct/amazing

---

Commenting on the pseudocode there is a more efficient way to do this as per the old validator set code (which has been tested). We should probably model the end-block after the original validator set update endblock functionality from the Gaia Repo.

We probably won't want to use arrays as is done here, however this should give you the idea. Not only do we need to iterate over the top 100 current validators, but we also need to iterate over the previous validator set to see if any are missing.

// determine all changed validators between two validator sets
func (vs Validators) validatorsChanged(vs2 Validators) (changed []*abci.Validator) {

	//first sort the validator sets
	vs.Sort()
	vs2.Sort()

	max := len(vs) + len(vs2)
	changed = make([]*abci.Validator, max)
	i, j, n := 0, 0, 0 //counters for vs loop, vs2 loop, changed element

	for i < len(vs) && j < len(vs2) {

		if !vs[i].PubKey.Equals(vs2[j].PubKey) {
			// pk1 > pk2, a new validator was introduced between these pubkeys
			if bytes.Compare(vs[i].PubKey.Bytes(), vs2[j].PubKey.Bytes()) == 1 {
				changed[n] = vs2[j].ABCIValidator()
				n++
				j++
				continue
			} // else, the old validator has been removed
			changed[n] = &abci.Validator{vs[i].PubKey.Bytes(), 0}
			n++
			i++
			continue
		}

		if vs[i].VotingPower != vs2[j].VotingPower {
			changed[n] = vs2[j].ABCIValidator()
			n++
		}
		j++
		i++
	}

	// add any excess validators in set 2
	for ; j < len(vs2); j, n = j+1, n+1 {
		changed[n] = vs2[j].ABCIValidator()
	}

	// remove any excess validators left in set 1
	for ; i < len(vs); i, n = i+1, n+1 {
		changed[n] = &abci.Validator{vs[i].PubKey.Bytes(), 0}
	}

	return changed[:n]
}

[alexanderbez]

Find the above hard to read and comprehend (think the loops can be cleaned up), but overall makes sense. Why not compare by operator address?

[rigelrozanski]

This is fairly old code, which likely would not work for the SDK - however, I'm not totally sure we want to be using the OperatorAddress vs. the Consensus Address as we are comparing the existing validator set stored from Tendermint which should be saved with the Consensus Address

[rigelrozanski]

@cwgoes what is your thinking on development approach for this issue? I think it's between you and I - I don't mind doing what jae suggested and taking the first stab to rapidly create the initial refactor, which then I'd love you to add input onto? - how does that sound? we could also do the reverse if you're interested - I'll probably have enough time for that this weekend

[cwgoes]

I'm pretty sure we can also get rid of the Jailed check by just deleting jailed validators from the ranked power store and re-adding them once unjailed.

[rigelrozanski]

sounds like a good idea! See if you can get it working!

[cwgoes]

Sounds like a good idea! See if you can get it working!

Indeed, done in #2394.