Delay validator set changes by 1 block in the SDK to reflect changes in Tendermint

[jaekwon]

Corresponding PR on Tendermint: tendermint/tendermint#1815
We need this for launch, as lite requires it.

Please sync with me for any issues or questions, and Ethan for timeline for merging on the tendermint side.

[rigelrozanski]

yeah bucky and I were talking about this -
It would be nice to have some kind of linkage here rather than just (more or less arbitrarily) having a block delay in the SDK with a comment explaining that it just happens this way in tendermint.

Ideally we need some kind of a response from abci that a validator has actually entered the tendermint validator set - OR alternatively - if this "1-block" is some kind of a genesis constant (I don't think it is though) - we could could just read it in - which could help code comprehension. What do you think? Not totally adverse to just hard-coding it with a comment, I just think it's not ideal (prone to slip-ups also if this changes in Tendermint at some point in the future too)

[jaekwon]

I think it won't change until Tendermint v2, so it's something hard-coded.
We can always upgrade in a backwards compatible way to pass more info, but I think it would be fine to assume that the sdk know and deal with this.

Tendermint <> SDK <> App, and I think we can assume that SDK developers can deal with these quirks for general app developers. And if not, we can introduce new measures down the line. For launch lets just hard-code it.

[cwgoes]

This needs to also change slashing logic since "contributing stake" is now offset by one block - if you unbond at block n, your stake now can contribute to a Byzantine fault at block n+1.

[cwgoes]

Subset of #2142 (or maybe a duplicate, but leaving both for now).

[rigelrozanski]

As Jae and I discussed a while ago these changes do not affect staking in any real way - only slashing

[cwgoes]

As Jae and I discussed a while ago these changes do not affect staking in any real way - only slashing

We need to modify Slash(), which is in the staking module (should it be? a question for another time...) - but if you mean otherwise, I guess not, although some of the queries will return confusing results (the SDK will think a validator is bonded before it actually signs blocks in Tendermint, for example).

[rigelrozanski]

In the medium term I think it makes sense to have these to entirely aligned to avoid confusion - it's only prelaunch that I'm not concerned about - there are a few very fundamental changes which are required to be made for staking to fully align with tendermint - but it barely affects the functionality.

Yeah I'll think about the best way to move Slash() out of staking - may still make sense there in the end however