R4R: Fix Cliff Validator Update Bugs #1858

cwgoes · 2018-07-27T23:19:38Z

Implement updateCliffValidator which is called via UpdateValidator when the power of a validator increases.
Updated and added to the original tests @cwgoes wrote to test the initial bugs.
Update some comments

closes: #1857

Linked to github-issue with discussion and accepted design
Updated all relevant documentation (docs/)
Updated all relevant code comments
Wrote tests
Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
Updated cmd/gaia and examples/

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)

rigelrozanski · 2018-07-30T17:48:13Z

@cwgoes are you working on this more or can @alexanderbez / myself work form here?

cwgoes · 2018-07-30T23:27:27Z

@cwgoes are you working on this more or can @alexanderbez / myself work form here?

Not presently - by all means take over this PR.

codecov · 2018-08-03T22:50:21Z

Codecov Report

Merging #1858 into develop will increase coverage by 0.04%.
The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           develop    #1858      +/-   ##
===========================================
+ Coverage    64.82%   64.87%   +0.04%     
===========================================
  Files          114      114              
  Lines         6804     6836      +32     
===========================================
+ Hits          4411     4435      +24     
- Misses        2114     2119       +5     
- Partials       279      282       +3

alexanderbez · 2018-08-03T22:54:21Z

@cwgoes since this was originally your PR, I cannot add you as a reviewer, but please still feel free to review.

Also, still getting acquainted with the staking code/logic, so lmk if there is a more immediate and efficient solution to what I have implemented 👍

/cc @rigelrozanski

ValarDragon · 2018-08-03T22:56:57Z

x/stake/keeper/validator.go

 	case powerIncreasing && !validator.Revoked &&
 		(oldFound && oldValidator.Status == sdk.Bonded):

 		bz := k.cdc.MustMarshalBinary(validator.ABCIValidator())
 		store.Set(GetTendermintUpdatesKey(validator.Owner), bz)

+		if cliffPower != nil {
+			k.updateCliffValidator(ctx, validator)


from the godoc of k.updateCliffVlaidator, it seems that we should have the cliffPower != nil check in there?

Not sure I totally agree, I think have the explicit check here makes the context more clear opposed to always calling updateCliffValidator.

ValarDragon · 2018-08-03T23:10:32Z

x/stake/keeper/validator.go

+	iterator := sdk.KVStorePrefixIterator(store, ValidatorsByPowerIndexKey)
+	offset := 0
+	for {
+		if !iterator.Valid() || offset == 1 {


I don't understand this for loop. Why are we breaking when offset == 1. It seems to me that the only code path here is offset incrementing after the first iteration, and this for loop breaking. (Which means it shouldn't be a loop)

I originally had it with an offset of two. In which case I needed the loop, but then discovered that would not work. I glossed over the fact that I no longer need the loop.

cwgoes · 2018-08-05T03:23:58Z

x/stake/keeper/validator.go

+
+	// Create a validator iterator ranging from smallest to largest in power
+	// where only the smallest by power is needed.
+	iterator := sdk.KVStorePrefixIterator(store, ValidatorsByPowerIndexKey)


Hmm I don't think this is the correct logic. The power store includes (a) many validator candidates which are not validators (not in the top hundred by stake) and (b) revoked validators, neither of which should be set as the cliff validator. The cliff validator is simply the "lowest-ranked bonded validator by stake" - but this logic reads the "lowest-ranked validator candidate", which is not necessarily the same.

For the case of "same cliff validator, increased power" this rough logic is fine. For the case of "cliff validator increased power, so now no longer cliff validator" I think we need to seek from the old cliff validator power store key and find the next-highest stake validator (which might become the cliff validator).

I see. It did not occur to me that the power store held those validators as well. I'll need to refactor this and add test cases where we also have revoked validators and candidates which are not validators.

cwgoes

I can't "request changes" on my own PR (apparently), but see comment - also, we should extend the testcases to cover the situation described in the comment.

jaekwon · 2018-08-07T09:29:29Z

x/stake/keeper/validator.go

 	}

 	k.SetValidator(ctx, validator)
 	return validator
 }

+// updateCliffValidator determines if the current cliff validator needs to be


I like to call this, "maybeUpdateCliffValidator", which implies that it only happens conditionally.
The comment could be better, as it does more than determine the conditional.

ehhhh I'm not totally sure about maybeUpdateCliffValidator, but I will certainly update the godoc 👍

Not of the idea of putting maybe in the func name. Something we've used for a similar idea for the sequence number is Ensure, -> I'd be okay with something more like EnsureCliffValidator or UpdateEnsureCliffValidator or UpdateOrEnsureCliffValidator

Use of this type of language for func naming would be cool to document in the Tenderming/coding - opened an issue here tendermint/coding#71

Agreed more appropriate nomenclature could be defined and standardized, but I'm not convinced Ensure* is it?

ensure implies a post-condition for the function, which is accurate

(then we should comment explaining what the postcondition is)

At first it seemed to me, ensure implies something must be done (in this, now old, case would imply that cliff must be updated, even though it's possible for it not to).

Now that I think about it more, it kinda makes sense.

@alexanderbez Did we reach consensus on updating the name to ensureCliffValidator?

rigelrozanski · 2018-08-07T18:39:02Z

x/stake/keeper/validator.go

 	case powerIncreasing && !validator.Revoked &&
 		(oldFound && oldValidator.Status == sdk.Bonded):

 		bz := k.cdc.MustMarshalBinary(validator.ABCIValidator())
 		store.Set(GetTendermintUpdatesKey(validator.Owner), bz)

+		if cliffPower != nil {
+			k.attemptUpdateCliffValidator(ctx, validator)


Why don't we just call this UpdateCliffValidator and move the code currently in this attempt function to the switch statement here:

if cliffPower != nil { cliffAddr := sdk.AccAddress(k.GetCliffValidator(ctx)) if bytes.Equal(cliffAddr, affectedVal.Owner) { k.UpdateCliffValidator(ctx, validator) } }

I bet there is an even nicer way to combine those if statements too

I like this idea as well. I'll update.

rigelrozanski · 2018-08-08T00:16:19Z

x/stake/keeper/validator.go

-	affectedValidator types.Validator) (updatedVal types.Validator, updated bool) {
+func (k Keeper) UpdateBondedValidators(
+	ctx sdk.Context, affectedValidator types.Validator,
+) (updatedVal types.Validator, updated bool) {


better indentation:

func (k Keeper) UpdateBondedValidators( ctx sdk.Context, affectedValidator types.Validator) ( updatedVal types.Validator, updated bool) {

Tbh, I don't necessarily agree here because it's hard for me to immediately see the return values (and their potential names) due to the location of the opening parenthesis. (e.g. immediate glance seems to show it has no returns).

But this is your baby and I'd like to keep in-line with what you've written and the team's standards, so I can update it 😄

cwgoes · 2018-08-08T11:06:46Z

x/stake/keeper/validator.go

+	offset := 0
+	var newCliffVal types.Validator
+
+	for ; iterator.Valid() && offset < 1; iterator.Next() {


Why a loop and an offset - we should only need to scan one record?

I believe Rige and I discussed to use an offset to be a bit cautious (offset should only be 1).

I think it'll be cleaner w/o one. I'll remove it.

cwgoes · 2018-08-08T11:07:19Z

x/stake/keeper/validator.go

+			panic(fmt.Sprintf("validator record not found for address: %v\n", ownerAddr))
+		}
+
+		if currVal.Status != sdk.Bonded || currVal.Revoked {


Good defensive coding!

cwgoes · 2018-08-08T11:08:26Z

x/stake/keeper/validator.go

+		// The affected validator remains the cliff validator, however, since
+		// the store does not contain the new power, set the new cliff
+		// validator to the affected validator.
+		k.setCliffValidator(ctx, affectedVal, pool)


We just need to update the power, right? I think setCliffValidator will unnecessarily re-set the cliff validator address.

Correct! I suppose we can save a write, huh?

cwgoes

A few minor suggestions but otherwise LGTM.

alexanderbez · 2018-08-08T12:45:52Z

Addressed comments @cwgoes .

cwgoes

Can't approve my own PR, but LGTM. Left one minor comment, just a nit.

cwgoes · 2018-08-08T15:36:46Z

x/stake/keeper/validator.go

+
+	var newCliffVal types.Validator
+
+	for ; iterator.Valid(); iterator.Next() {


I still don't understand the point of a for loop with only one iteration, why do we need a loop structure at all?

Yeah ++ this comment we shouldn't have a loop it should just panic at the higher level if it would have reached the end of the loop

Guys my brain was fried yesterday...sorry about this.

haha no worries (me too!) thanks for the updated

…cosmos-sdk into cwgoes/cliff-validator-power-bug

alexanderbez · 2018-08-08T17:15:24Z

Updated @cwgoes @rigelrozanski

cwgoes · 2018-08-08T18:44:28Z

x/stake/keeper/validator.go

+		newCliffVal = currVal
+		iterator.Close()
+	} else {
+		panic("failed to create valid validator power iterator")


I wonder if this might crash if the user is running only one validator. Not sure if that is a usecase we care about or not - but we could check it with params.MaxVals

We probably want to support that for convenience in testing.

This should not crash, but let me check.

Yeah it works with one validator 👍

Sweet! Lets merge this then!

cwgoes · 2018-08-08T18:44:43Z

x/stake/keeper/validator.go

 	}

 	k.SetValidator(ctx, validator)
 	return validator
 }

+// updateCliffValidator determines if the current cliff validator needs to be


@alexanderbez Did we reach consensus on updating the name to ensureCliffValidator?

alexanderbez · 2018-08-08T18:52:58Z

@cwgoes re ensureCliffValidator, I'm not totally sure because I removed the original conditional check out of the method. So I don't think it applies anymore?

Isolating testcase

c3aa256

cwgoes requested review from ebuchman and rigelrozanski as code owners July 27, 2018 23:19

Separate testcase

790f579

cwgoes mentioned this pull request Jul 27, 2018

Cliff validator power not correctly updated when cliff validator increases in power #1857

Closed

alexanderbez self-assigned this Jul 31, 2018

alexanderbez added T:Bug C:x/staking prelaunch labels Jul 31, 2018

Update Keeper#UpdateValidator to update cliff validator on power incr

374c71f

cwgoes mentioned this pull request Aug 3, 2018

R4R: Slashing, validator set, and governance simulation #1783

Merged

15 tasks

Aleksandr Bezobchuk added 2 commits August 3, 2018 18:41

Fix updating cliff validator bug

cc3c05f

Update pending changelog

5ac49aa

alexanderbez requested a review from ValarDragon August 3, 2018 22:47

alexanderbez added ready-for-review and removed wip labels Aug 3, 2018

alexanderbez changed the title ~~WIP: Cliff validator power increase bug~~ R4R: Fix Cliff Validator Update Bugs Aug 3, 2018

ValarDragon reviewed Aug 3, 2018

View reviewed changes

Cleanup updateCliffValidator iteration

f0277aa

cwgoes commented Aug 5, 2018

View reviewed changes

alexanderbez changed the title ~~R4R: Fix Cliff Validator Update Bugs~~ WIP: Fix Cliff Validator Update Bugs Aug 6, 2018

jaekwon reviewed Aug 7, 2018

View reviewed changes

Aleksandr Bezobchuk added 3 commits August 7, 2018 07:22

Remove redundant cliff address variable

72c227f

Update updateCliffValidator godoc

8c38a85

Rename updateCliffValidator

80c9e18

rigelrozanski mentioned this pull request Aug 7, 2018

Design Patterns & Language tendermint/coding#71

Open

rigelrozanski reviewed Aug 7, 2018

View reviewed changes

Update updateCliffValidator to have more sanity checks

a138f00

ebuchman mentioned this pull request Aug 8, 2018

v0.24 release #1935

Closed

6 tasks

rigelrozanski reviewed Aug 8, 2018

View reviewed changes

cwgoes commented Aug 8, 2018

View reviewed changes

cwgoes added 2 commits August 8, 2018 13:10

Merge branch 'develop' into cwgoes/cliff-validator-power-bug

26c9500

Fix typo

f4a639f

cwgoes commented Aug 8, 2018

View reviewed changes

Aleksandr Bezobchuk added 2 commits August 8, 2018 08:36

Remove redundent offset and avoid extra write in cliff update

53c1f64

Fix UpdateBondedValidators function style

0e8f864

cwgoes commented Aug 8, 2018

View reviewed changes

cwgoes and others added 3 commits August 8, 2018 17:38

Merge branch 'develop' into cwgoes/cliff-validator-power-bug

3aa715e

Avoid loop iteration in updateCliffValidator

20d8db3

Merge branch 'cwgoes/cliff-validator-power-bug' of github.com:cosmos/…

a293869

…cosmos-sdk into cwgoes/cliff-validator-power-bug

cwgoes commented Aug 8, 2018

View reviewed changes

rigelrozanski approved these changes Aug 8, 2018

View reviewed changes

cwgoes merged commit ac26d33 into develop Aug 8, 2018

ValarDragon deleted the cwgoes/cliff-validator-power-bug branch September 23, 2018 03:59


		var newCliffVal types.Validator

		for ; iterator.Valid(); iterator.Next() {

R4R: Fix Cliff Validator Update Bugs #1858

R4R: Fix Cliff Validator Update Bugs #1858

Conversation

cwgoes commented Jul 27, 2018 • edited by alexanderbez Loading

rigelrozanski commented Jul 30, 2018

cwgoes commented Jul 30, 2018

codecov bot commented Aug 3, 2018 • edited Loading

Codecov Report

alexanderbez commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

cwgoes Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez commented Aug 8, 2018

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez commented Aug 8, 2018

cwgoes commented Jul 27, 2018 •

edited by alexanderbez

Loading

codecov bot commented Aug 3, 2018 •

edited

Loading

cwgoes Aug 5, 2018 •

edited

Loading

rigelrozanski Aug 7, 2018 •

edited

Loading

alexanderbez Aug 8, 2018 •

edited

Loading

cwgoes Aug 8, 2018 •

edited

Loading