Cosmos-sdk: Save height for when assigning validator power

[zramsay]

This will help order validators for when deciding who is the validator if two nodes have the same bonded stake. - The older validator should always retain power.

ref: https://github.com/cosmos/gaia/issues/49

[rigelrozanski]

@cwgoes want to take this one on? The existing keeper tests need to be extended to test this case of two validators with the same voting power but only one of them can make it into the validator set - and it must the one which was first a validator - Also need to reset the "age" of a validator if it is kicked out (and test that too)

[cwgoes]

Should height be reset whenever the validator changes stake, or only when the validator increases stake (are we measuring "age at a particular stake" or "age at or above a particular stake")?

[cwgoes]

Also, isn't this still underdefined? What if two validators both switch to the same voting power in the same block? Seems like we also have to include a transaction sequence number or something.

[rigelrozanski]

Good Questions - for your second question whichever transaction is processed first should be the winner. To your first question, The height should ONLY reset if the validator is leaves the validator set (either being kicked or revoked) and then reenters the validator set - if the validator changes it's bond amount this height field should be unaffected.

[cwgoes]

Hmm, this is a strange definition to use, since "the validator set" is really only defined at the end of each block - during an individual transaction, all we know is what the validator set is at the moment.

Currently (in the referenced PR) this ordering is implemented, but it will result in counter-intuitive behavior in a few edge cases, for example:

Starting configuration:
max_validators = 2

Validators ranked:
power_A = 100, height_A = 50
power_B = 100, height_B = 60
power_C = 100, height_C = 70

Transactions:

1. 10 power unbonded from A. Rank: B, C, A - A is now out of the validator set.
2. 10 power bonded back to A. Rank: B, C, A - A had been kicked out of the validator set, so height_A was reset to the current height.

Despite A having no net change in power and originally (at the start of the block) an older age, A is no longer a validator due to the sequence of these transactions within the block (had they been sequenced the other way, A would still be a validator).

Either we (a) accept this oddity or (b) keep a separate copy of the validator set from the last block, and use that to determine whether or not to reset the age of a validator instead of the transient in-block validator set.

@rigelrozanski Am I understanding this correctly?

[rigelrozanski]

Are these transactions in different blocks or the same block? If in separate blocks I don't find it odd at all, By A temporarily unbonding they temporarily reduce the security of the hub, and thus it would make sense that if it comes down to equal validator power (again very rare edge case) they are the party who has most negatively impacted the security and should lose out.
If those two transactions are in the same block, yeah maybe it's a bit weird because technically the voting power hasn't been changed and thus security not reduced, however I think it's a pretty extreme edge case and we should just accept the oddity, the overhead to make this less odd is too great to justify it's implementation.

[cwgoes]

In different blocks - agreed - I mean the scenario in which the transactions are in the same block.

OK, fair enough, just wanted to be sure.

[cwgoes]

for your second question whichever transaction is processed first should be the winner

To implement this in the current sorted KVStore model, we either need to get the transaction index from the Context somehow or store a transient counter in the staking state (can be reset every block). Which do you think would be better (or something else)?

See testcases in https://github.com/cosmos/cosmos-sdk/commit/0d65dcdba530d8cabc6cf79435fc9cb414df897a, which intentionally fail at the moment.

[rigelrozanski]

Transactions are processed in the order which they are submitted correct? That would indicate that we do not need to explicitly track the transaction index correct? I'll take a peek at your tests

[cwgoes]

Yes - but we need to sort the validators upon retrieval at the end of the block according to transaction index if we want the first transaction to always win, e.g. in the following case:

Starting configuration:
max_validators = 2

Validators ranked:
power_A = 60
power_B = 50
power_C = 40
power_D = 30

Transactions:

1. C bonds 15, and is now a validator.
2. D bonds 25, and is now equal in power to C, but should not be a validator (per above rule).

The validators are retrieved using the KVStore iterator - so we need some way to sort D after C in that sorted order (and that way needs to increase when the transaction index increases to provide this first-validator wins property, so it seems easiest to have it be the transaction index, although we could also keep a separate counter).

The testcases should cover this.

[rigelrozanski]

Excellent scenario - totally correct - let's just keep a local counter which could be kept as a field in the pool and reset each block. Each voting power change would have a counter associated with it and during ordering if there is an equal voting power, the one with the earliest counter will be chosen as the validator.

[cwgoes]

Closed by https://github.com/cosmos/cosmos-sdk/pull/804.