R4R: Ensure Tendermint Validator Update Invariants - Part II #2298

alexanderbez · 2018-09-10T14:49:03Z

During the debugging and implementation of #2238, another case of invalid Tendermint updates arose but with a different cause. I narrowed it down to what I believe was the trace of events. Can be generalized as such:

Given ValidatorA,

Height 1: Bonded/enters validator set (bondHeight: 1, unbondingHeight: 0)
Height 10: Unbonded for whatever reason (bondHeight: 1, unbondingHeight: 10)
Height 11: Receives a delegation, however, not enough to enter validator set. Note, due to the way Keeper#UpdateValidator is implemented, the bond height and intra counter will be updated (see: here). (bondHeight: 11, unbondingHeight: 10)
Height 15: Due to some series of txs in the same block, ValidatorA bonds and unbonds causing an update to the Tendermint updates store (unbond). However, because it's bondHeight is 10, Keeper#GetValidTendermintUpdates believes it was previously bonded and as such sends an update to remove ValidatorA (unbonds/power zero). However, ValidatorA was never in the bonded set to begin with (removed at height 10).

tl;dr when a previously unbonded validator goes from unbonded, to bonded, back to unbonded in the same block as a result of some other change (e.g. cliff validator losing enough power and then gaining it back), the previously unbonded validator never has it's bond height updated.

Not sure if implemented solution is the correct/best one, but I simply set the bondHeight in bondValidator. Also, always setting the bondHeight for an unbonded validator in Keeper#UpdateValidator is a bit confusing to me. Why do we do this?

closes: #2277
closes by proxy: #2189
ref: #2238, #2189

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2018-09-10T16:08:25Z

Codecov Report

❗ No coverage uploaded for pull request base (bez/2189-del-non-exist-val-bug@ebb2b2e). Click here to learn what that means.
The diff coverage is 50%.

@@                        Coverage Diff                        @@
##             bez/2189-del-non-exist-val-bug    #2298   +/-   ##
=================================================================
  Coverage                                  ?   63.75%           
=================================================================
  Files                                     ?      140           
  Lines                                     ?     8652           
  Branches                                  ?        0           
=================================================================
  Hits                                      ?     5516           
  Misses                                    ?     2756           
  Partials                                  ?      380

…t-val-update-fix

x/stake/keeper/validator.go

cwgoes · 2018-09-10T23:26:16Z

x/stake/keeper/validator.go

@@ -482,10 +481,15 @@ func (k Keeper) UpdateBondedValidators(

 		// if we've reached jailed validators no further bonded validators exist
 		if validator.Jailed {
+			if validator.Status == sdk.Bonded {


Additional defensive coding 👍

cwgoes · 2018-09-10T23:26:40Z

x/stake/keeper/validator.go

@@ -494,13 +498,6 @@ func (k Keeper) UpdateBondedValidators(
 			newValidatorBonded = true
 		}

-		// increment the total number of bonded validators and potentially mark


Why did we delete these lines?

if validator.Status != sdk.Bonded { // ... } was done twice and the internal logic was essentially duplicated. I put it into a single conditional/block.

x/stake/keeper/validator.go

cwgoes · 2018-09-10T23:28:54Z

x/stake/keeper/validator.go

@@ -683,6 +679,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()


I'm not sure this is right. The original intent was to copy the bondHeight from the old validator in bondIncrement if already bonded, else set it to the current block height. Can we figure out why that isn't happening first?

@cwgoes the bond height never gets set for a validator that enters the set (temporarily) as a result of another leaving...at least in the case I saw.

That would be the bug then, makes sense.

We just need to ensure that this doesn't break the existing logic - which it might not, but it does overlap I think.

cwgoes · 2018-09-10T23:29:46Z

Using BondHeight to determine whether a validator was previously bonded definitely won't work if we don't update the bond height when the validator is unbonded, bonded, then unbonded again. We should have done that, though, in bondIncrement, since the old validator wouldn't have been bonded. Let's see why that didn't happen.

alexanderbez · 2018-09-10T23:49:12Z

@cwgoes I'll address most of your comments shortly, but maybe we should sync up -- perhaps I'm missing something.

cwgoes · 2018-09-11T09:02:55Z

@cwgoes I'll address most of your comments shortly, but maybe we should sync up -- perhaps I'm missing something.

I'm not sure (replied re: sync up on Slack) - this fix might be OK but let's definitely read over the logic again, and I think we're at minimum duplicating a case that previously worked (in addition to the case that didn't).

Setting the bond height in bondValidator seems like the right approach - at least to me - I would suggest we get rid of the previous logic, which is now redundant, and also ensure that we set the intra-tx counter etc. correctly in bondValidator.

alexanderbez · 2018-09-11T15:29:52Z

@cwgoes indeed, I agree we should have one place that does this -- bondValidator.

However, when I tried this, it screwed up iteration in other methods such as UpdateBondedValidators and updateCliffValidator because the ValidatorsByPowerIndexKey is used in conjunction with BondHeight and BondIntraTxCounter which are currently set before any iterator is created.

See: getValidatorPowerRank.

alexanderbez · 2018-09-11T19:10:23Z

@cwgoes getting pretty deep into trying to figure out another way to set bond height and still have valid iteration.

I've come to the conclusion that in order to move setting the bond height, UpdateBondedValidators will need significant refactor. I think we should merge this as is and create an issue for a cleaner approach to setting the bond height and by proxy, UpdateBondedValidators.

I think this is a safe approach for now.

cwgoes

Tested ACK

I think this PR is safe and does fix the immediate issue, but we should absolutely simplify this logic, #prelaunch.

cwgoes · 2018-09-12T06:52:54Z

See #2312.

* Update statesync.bash * Update statesync.bash * Update statesync.bash Removed evmos comments, added a few other comments. Commented out go install --------- Co-authored-by: Milan Mulji <98309852+mmulji-ic@users.noreply.github.com>

Add a unit test showing invalid TM validator updates with bonded vals

d895c07

alexanderbez added T:Bug C:x/staking prelaunch labels Sep 10, 2018

alexanderbez requested review from cwgoes, ebuchman and rigelrozanski as code owners September 10, 2018 14:49

Aleksandr Bezobchuk added 3 commits September 10, 2018 10:51

Re-add defensive check in UpdateBondedValidators for jailed validators

4bfd4bb

Cleanup and set bond height in keeper#bondValidator

cee6feb

Get pool after getting context with new block height in unit test

4a000d6

Aleksandr Bezobchuk added 3 commits September 10, 2018 12:19

Merge branch 'bez/2189-del-non-exist-val-bug' into bez/2277-tendermin…

832a71b

…t-val-update-fix

Fix broken unit tests

8ef395d

Update prevBonded value

d54aea1

alexanderbez changed the title ~~WIP: Ensure Tendermint Validator Update Invariants - Part II~~ R4R: Ensure Tendermint Validator Update Invariants - Part II Sep 10, 2018

alexanderbez added ready-for-review and removed wip labels Sep 10, 2018

cwgoes mentioned this pull request Sep 10, 2018

Invalid validator state in simulation #2277

Closed

cwgoes suggested changes Sep 10, 2018

View reviewed changes

Rename validator to oldValidator in bondIncrement

220f0b4

cwgoes approved these changes Sep 12, 2018

View reviewed changes

cwgoes merged commit f7f20f1 into bez/2189-del-non-exist-val-bug Sep 12, 2018

cwgoes deleted the bez/2277-tendermint-val-update-fix branch September 12, 2018 06:45

cwgoes mentioned this pull request Sep 12, 2018

Radically simplify staking logic #2312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Ensure Tendermint Validator Update Invariants - Part II #2298

R4R: Ensure Tendermint Validator Update Invariants - Part II #2298

alexanderbez commented Sep 10, 2018 •

edited

Loading

codecov bot commented Sep 10, 2018 •

edited

Loading

cwgoes Sep 10, 2018

cwgoes Sep 10, 2018

alexanderbez Sep 11, 2018 •

edited

Loading

cwgoes Sep 12, 2018

cwgoes Sep 10, 2018

alexanderbez Sep 10, 2018

cwgoes Sep 11, 2018 •

edited

Loading

cwgoes commented Sep 10, 2018 •

edited

Loading

alexanderbez commented Sep 10, 2018

cwgoes commented Sep 11, 2018 •

edited

Loading

alexanderbez commented Sep 11, 2018 •

edited

Loading

alexanderbez commented Sep 11, 2018

cwgoes left a comment

cwgoes commented Sep 12, 2018

R4R: Ensure Tendermint Validator Update Invariants - Part II #2298

R4R: Ensure Tendermint Validator Update Invariants - Part II #2298

Conversation

alexanderbez commented Sep 10, 2018 • edited Loading

codecov bot commented Sep 10, 2018 • edited Loading

Codecov Report

cwgoes Sep 10, 2018

Choose a reason for hiding this comment

cwgoes Sep 10, 2018

Choose a reason for hiding this comment

alexanderbez Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

cwgoes Sep 12, 2018

Choose a reason for hiding this comment

cwgoes Sep 10, 2018

Choose a reason for hiding this comment

alexanderbez Sep 10, 2018

Choose a reason for hiding this comment

cwgoes Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

cwgoes commented Sep 10, 2018 • edited Loading

alexanderbez commented Sep 10, 2018

cwgoes commented Sep 11, 2018 • edited Loading

alexanderbez commented Sep 11, 2018 • edited Loading

alexanderbez commented Sep 11, 2018

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes commented Sep 12, 2018

alexanderbez commented Sep 10, 2018 •

edited

Loading

codecov bot commented Sep 10, 2018 •

edited

Loading

alexanderbez Sep 11, 2018 •

edited

Loading

cwgoes Sep 11, 2018 •

edited

Loading

cwgoes commented Sep 10, 2018 •

edited

Loading

cwgoes commented Sep 11, 2018 •

edited

Loading

alexanderbez commented Sep 11, 2018 •

edited

Loading