Unexpected validator set change

[abelliumnt]

If the voting power of cliff validator is the same as the new created validator, then they will kick each other out of validator set in every later blocks.

Reproduce steps:
https://github.com/HaoyangLiu/validator-candicate

[cwgoes]

I think this is true not only with a new validator and the cliff validator, but with any two validators with equal power where one is on the cliff and the other is just out of the bonded set.

At block height n, when the validator with the lower bond height (per ordering rules) is bonded, its BondHeight will be set to the current height in ApplyAndReturnValidatorSetUpdates - which will then give it the larger BondHeight, and the validators will swap positions next height, repeat ad infinitum.

The intended behavior of BondHeight is to preference the already-bonded validator when two validators with equal power are compared. This fails in two distinct cases: newly created validators are set to a bond height of zero, and existing unbonding/unbonded validators retain the bond height they had from the time they were last bonded, so they might be preferenced over an already-bonded validator (if it had bonded afterwards) when considered for inclusion.

Maybe we should ditch BondHeight altogether and instead sort by a boolean of whether or not the validator is presently bonded (where presently bonded validators are ranked higher). We would lose the total ordering between validators (two validators could change power in the same block and one would be randomly unbonded) but the bonded set would be stable (whichever one was unbonded would stay unbonded).

That doesn't seem too concerning to me, but if we really care about the ordering we could add back in BondHeight, and just sort by it after the boolean.

[cwgoes]

cc @rigelrozanski Thoughts on above proposal?

[rigelrozanski]

Couple comments - if I'm not mistaken the bond tx height was ditched (or left non-functional) with the staking refactor. Is the bond height also operation currently?

I think two validators having the same power is such an extreme edge case that it is fine to kick one out for an arbitrary reason (such as their ordering by pubkey). - aka, I think you're proposal is fine... just kick out (or keep not-bonded) whoever isn't already bonded. And if two validators are already bonded and one needs to get kicked out... just kick either one out (but deterministically - either just sort by pubkey or like hash of pubkey and block-hash or something).

I like your proposal - because it seems hella simple - and this mechanism is so rarely going to take effect

[abelliumnt]

So will this issue be fixed in 0.27.0 official release?

[cwgoes]

So will this issue be fixed in 0.27.0 official release?

Probably not, since it's a rare edge case which doesn't actually break anything (it's just inefficient to switch validators every block), but it should be fixed by the release after. Is it particularly problematic for your use case?

[abelliumnt]

No, it is a corner case for us too. Just curious about your plan.

[ValarDragon]

I thought we were going to do total ordering via a global message counter. (Also I'm not a fan of ordering by pubkey or address, feels like bad domain separation and creates minor incentives for grinding pubkeys for higher priority in this setting)

[cwgoes]

I thought we were going to do total ordering via a global message counter.

This doesn't fix the problem (if implemented in a similar way to the bond height, e.g. set when a validator is bonded). Are you proposing a different implementation (such as only setting when a validator is created)?

Also I'm not a fan of ordering by pubkey or address, feels like bad domain separation and creates minor incentives for grinding pubkeys for higher priority in this setting.

I think the incentive here is tiny; instead of bothering to grind a pubkey you could just acquire and bond an extra nano-Atom. Still, I agree that it is a bit unclean.

[ValarDragon]

I agree with the isBonded boolean you were suggesting. I meant using that counter for total ordering instead of bond height and intra bond height tx. Should have specified.

I think the incentive here is tiny; instead of bothering to grind a pubkey you could just acquire and bond an extra nano-Atom. Still, I agree that it is a bit unclean.

Domain separation is important to yielding an overall more secure system where its hard to shoot yourself in the foot. Random minor bad incentives that build up will eventually cause problems in some configurations.

[cwgoes]

Agreed, let's do exactly that - isBoolean plus the global counter.

[rigelrozanski]

I think the incentive here is tiny; instead of bothering to grind a pubkey you could just acquire and bond an extra nano-Atom. Still, I agree that it is a bit unclean.

lol correct, the incentive is the price of acquiring a single nano-atom

By global counter - does that mean the global message counter? If this is the only purpose for developing the global message counter then I'm opposed to complicating the code for the extremely rare edge case - however I'd imagine that there are many other important applications, so should probably implement anyways 👌

[jaekwon]

I think two validators having the same power is such an extreme edge case

This can be a targeted attack, we should fix it. It's a grief'ing factor, and generally not good to have such edge cases in code when it can be avoided. If we accumulate enough of these edge cases the system becomes unusable.

[jaekwon]

Also I'm not a fan of ordering by pubkey or address, feels like bad domain separation and creates minor incentives for grinding pubkeys for higher priority in this setting.

I think the incentive here is tiny; instead of bothering to grind a pubkey you could just acquire and bond an extra nano-Atom. Still, I agree that it is a bit unclean.

Using an address to sort is by far the simpler solution. Conceptually it's a bit stranger, but implementation simplicity wise, and thus overall comprehensibility wise, using an address is much simpler. I'm assuming that we want to de-duplicate staking from validators of the exact same atom amount, as per my above comment.

[jaekwon]

Fixed in #3055

[jaekwon]

Closing for now, we can re-open if necessary.

[jaekwon]

@HaoyangLiu can you check if the above PR fixes the problem for you?

[abelliumnt]

@jaekwon
I have verified #3055. It works well. Great job!