R4R: Split up UpdateValidator into distinct state transitions applied only in EndBlock #2394

rigelrozanski · 2018-09-25T04:15:34Z

closes #2351
ref #2312

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)

cwgoes · 2018-10-02T10:08:10Z

@alexanderbez Thanks for the review, comments addressed.

This PR now additionally (sorry for slight mega-PR, but these updates have to happen together):

Removes jailed validators from the ranked power store entirely (which shortens all keys somewhat)
Removes all modifications of the bonded validator subspace outside of val_state_change.go
Fixes a bug in the simulation where validators weren't set correctly on app.InitChain

cwgoes · 2018-10-02T12:15:53Z

cc @HaoyangLiu, @kidinamoto01

This is the major staking refactor that will be included in Cosmos SDK 0.25, you might find it helpful to read over (and any comments are welcome).

alexanderbez

Changes look great -- hard to see if any corner cases arise. Curious how intra-block validator power changes will work out since that was a cause for a lot of bugs in the old code. Looks like this will address any concerns though.

Will approve once passing CI!

alexanderbez · 2018-10-02T14:16:48Z

x/stake/keeper/val_state_change.go

+		operator := sdk.ValAddress(iterator.Value())
+		validator := k.mustGetValidator(ctx, operator)
+
+		if validator.Jailed {


cwgoes · 2018-10-02T14:40:47Z

Hard to see if any corner cases arise

It is - I think this logic is simpler, which makes corner cases less likely, but we can still add more defensive asserts in the staking code later (I'm trying to avoid adding anything more to this PR).

ValarDragon · 2018-10-02T17:31:03Z

x/stake/keeper/delegation.go

-	// update then remove validator if necessary
-	validator = k.UpdateValidator(ctx, validator)
-	if validator.DelegatorShares.IsZero() {
+	if validator.DelegatorShares.IsZero() && validator.Status != sdk.Bonded {


I'm confused, why is this the correct logic? Is the validator's self bond included as a delegator share?

RemoveValidator is called because I think it needs to be removed from the new power store ranking.

Yes, the self-bond is a usual bond. Has to be removed here since we won't necessarily iterate over it in endblock (we'll only necessarily iterate over it if the validator was bonded).

x/stake/keeper/test_common.go

rigelrozanski · 2018-10-03T01:50:25Z

lcd test failing locally (after make get_vendor_dep) and on CI test_cover

LADDR tcp://0.0.0.0:54628
E[10-02|21:45:08.611] Couldn't connect to any seeds                module=p2p
--- FAIL: TestKeys (6.94s)
panic: nil [recovered]
	panic: test executed panic(nil) or runtime.Goexit

goroutine 21 [running]:
testing.tRunner.func1(0xc0001a0900)
	/usr/local/Cellar/go/1.11/libexec/src/testing/testing.go:792 +0x387
panic(0x0, 0x0)
	/usr/local/Cellar/go/1.11/libexec/src/runtime/panic.go:513 +0x1b9
github.com/cosmos/cosmos-sdk/tests.WaitForStart(0xc000925c20, 0x24)
	/Users/rigelrozanski/go/src/github.com/cosmos/cosmos-sdk/tests/util.go:167 +0xeb
github.com/cosmos/cosmos-sdk/tests.WaitForLCDStart(0xc0000357b0, 0x5)
	/Users/rigelrozanski/go/src/github.com/cosmos/cosmos-sdk/tests/util.go:128 +0xb8
github.com/cosmos/cosmos-sdk/client/lcd.InitializeTestLCD(0xc0001a0900, 0x1, 0xc000faff30, 0x1, 0x1, 0x1ddea80, 0xc0001ca370, 0xc000ada7a0, 0x14, 0x20, ...)
	/Users/rigelrozanski/go/src/github.com/cosmos/cosmos-sdk/client/lcd/test_helpers.go:202 +0x103e
github.com/cosmos/cosmos-sdk/client/lcd.TestKeys(0xc0001a0900)
	/Users/rigelrozanski/go/src/github.com/cosmos/cosmos-sdk/client/lcd/lcd_test.go:45 +0x138
testing.tRunner(0xc0001a0900, 0x1cead80)
	/usr/local/Cellar/go/1.11/libexec/src/testing/testing.go:827 +0xbf
created by testing.(*T).Run
	/usr/local/Cellar/go/1.11/libexec/src/testing/testing.go:878 +0x353
FAIL	github.com/cosmos/cosmos-sdk/client/lcd	7.059s

rigelrozanski

Frick yeah! thanks for your contribs - left some comments here :)

x/stake/keeper/val_state_change.go

x/stake/querier/queryable_test.go

x/stake/keeper/val_state_change.go

x/stake/keeper/validator.go

x/stake/keeper/val_state_change.go

x/stake/handler.go

cwgoes · 2018-10-03T15:29:27Z

lcd test failing locally

This is caused by the underlying TM 0.24 issue.

cwgoes · 2018-10-03T16:01:05Z

This is caused by the underlying TM 0.24 issue.

Now fixed since we temporarily disabled proof verification.

cwgoes

utACK for @rigelrozanski (hint hint Github: PR ownership change would be a great feature)

rigelrozanski · 2018-10-03T16:38:10Z

:D

Co-authored-by: Marius Poke <marius.poke@posteo.de>

cwgoes and others added 19 commits September 3, 2018 17:01

Start of changes for TM 0.24

73292e0

More TM 0.24 updates

14d5e68

Client updates for TM 0.24

532c9ee

Merge branch 'develop' into cwgoes/update-tendermint-upstream

f45cfb2

Pin to Tendermint 0.24.0-rc0

8f6c9e1

Update testcases for TM 0.24.0-rc0

06adc69

More testcase fixes

c667c56

Ignore Go src in linter

1f5fe29

Minor cleanup

dd75cd3

Check errors

e013b9f

Merge branch 'develop' into cwgoes/update-tendermint-upstream

2245b07

Merge branch 'develop' into cwgoes/update-tendermint-upstream

897c381

Pin final release

9dd4303

Merge branch 'develop' into cwgoes/update-tendermint-upstream

b144dc7

Merge branch 'develop' into cwgoes/update-tendermint-upstream

f95d26a

Merge branch 'develop' into cwgoes/update-tendermint-upstream

08d4f45

Merge branch 'develop' into cwgoes/update-tendermint-upstream

1d55673

make format

d85eb98

pulling in issue code

19080d0

rigelrozanski added C:x/staking wip labels Sep 25, 2018

rigelrozanski requested review from cwgoes and ebuchman as code owners September 25, 2018 04:15

rigelrozanski added 7 commits September 25, 2018 20:25

Merge remote-tracking branch 'origin/develop' into rigel/stake-refactor

6c78ac7

replace ensureValidatorFound with mustGetValidator

1aa65e4

split up keeper/validator.go

8d394f6

move validatorByPowerIndexExists from keeper to test file

8373329

reorganizing

b6575bb

GetValidatorsByPower -> GetBondedValidatorsByPower

52c413f

remove duplicate SetValidator in handleMsgEditValidator

7debae0

Remove redundant bonded validator store

413fa42

Fix simulator bug

1c1183a

cwgoes force-pushed the rigel/stake-refactor branch from 8cc2ccc to 1c1183a Compare October 2, 2018 10:09

Change key name to reflect usage

951cacc

cwgoes mentioned this pull request Oct 2, 2018

Split up UpdateValidator per each state-transitions, & Tendermint updates in Endblock #2351

Closed

cwgoes requested a review from jaekwon October 2, 2018 10:24

This was referenced Oct 2, 2018

Randomized Fuzzer: tried to delete a non-existent validator #2326

Closed

Gaiad network crash in executing redelegation transactions #2241

Closed

cwgoes mentioned this pull request Oct 2, 2018

SDK 0.25 / Gaia-9000 release checklist #2220

Closed

23 tasks

alexanderbez reviewed Oct 2, 2018

View reviewed changes

ValarDragon reviewed Oct 2, 2018

View reviewed changes

x/stake/keeper/test_common.go Outdated Show resolved Hide resolved

cwgoes added 2 commits October 3, 2018 02:27

updateValidator -> testingUpdateValidator

8e67f6d

'make format'

94b0ca9

rigelrozanski commented Oct 3, 2018

View reviewed changes

Address @rigelrozanski comments

5893e23

cwgoes changed the base branch from cwgoes/update-tendermint-upstream to develop October 3, 2018 15:48

Merge branch 'develop' into rigel/stake-refactor

1ad577b

Add descriptive comment

8853f2b

cwgoes approved these changes Oct 3, 2018

View reviewed changes

cwgoes merged commit 324bdaf into develop Oct 3, 2018

rigelrozanski deleted the rigel/stake-refactor branch October 3, 2018 16:38

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024

add thelper to linter and bump the linter's version (cosmos#2394)

e093d82

Co-authored-by: Marius Poke <marius.poke@posteo.de>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Split up UpdateValidator into distinct state transitions applied only in EndBlock #2394

R4R: Split up UpdateValidator into distinct state transitions applied only in EndBlock #2394

rigelrozanski commented Sep 25, 2018 •

edited by cwgoes

Loading

cwgoes commented Oct 2, 2018

cwgoes commented Oct 2, 2018

alexanderbez left a comment

alexanderbez Oct 2, 2018

cwgoes commented Oct 2, 2018

ValarDragon Oct 2, 2018 •

edited

Loading

alexanderbez Oct 2, 2018

cwgoes Oct 2, 2018

rigelrozanski commented Oct 3, 2018

rigelrozanski left a comment

cwgoes commented Oct 3, 2018

cwgoes commented Oct 3, 2018

cwgoes left a comment

rigelrozanski commented Oct 3, 2018

R4R: Split up UpdateValidator into distinct state transitions applied only in EndBlock #2394

R4R: Split up UpdateValidator into distinct state transitions applied only in EndBlock #2394

Conversation

rigelrozanski commented Sep 25, 2018 • edited by cwgoes Loading

cwgoes commented Oct 2, 2018

cwgoes commented Oct 2, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Oct 2, 2018

Choose a reason for hiding this comment

cwgoes commented Oct 2, 2018

ValarDragon Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

alexanderbez Oct 2, 2018

Choose a reason for hiding this comment

cwgoes Oct 2, 2018

Choose a reason for hiding this comment

rigelrozanski commented Oct 3, 2018

rigelrozanski left a comment

Choose a reason for hiding this comment

cwgoes commented Oct 3, 2018

cwgoes commented Oct 3, 2018

cwgoes left a comment

Choose a reason for hiding this comment

rigelrozanski commented Oct 3, 2018

rigelrozanski commented Sep 25, 2018 •

edited by cwgoes

Loading

ValarDragon Oct 2, 2018 •

edited

Loading