Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 70 commits into from
Oct 3, 2018

Conversation

rigelrozanski
Copy link
Contributor

@rigelrozanski rigelrozanski commented Sep 25, 2018

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
Copy link
Contributor

cwgoes commented Oct 2, 2018

@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
Copy link
Contributor

cwgoes commented Oct 2, 2018

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).

@cwgoes cwgoes mentioned this pull request Oct 2, 2018
23 tasks
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

operator := sdk.ValAddress(iterator.Value())
validator := k.mustGetValidator(ctx, operator)

if validator.Jailed {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@cwgoes
Copy link
Contributor

cwgoes commented Oct 2, 2018

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).

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

@ValarDragon ValarDragon Oct 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@rigelrozanski
Copy link
Contributor Author

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

Copy link
Contributor Author

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

x/stake/keeper/val_state_change.go Show resolved Hide resolved
x/stake/keeper/val_state_change.go Outdated Show resolved Hide resolved
x/stake/keeper/val_state_change.go Outdated Show resolved Hide resolved
x/stake/querier/queryable_test.go Outdated Show resolved Hide resolved
x/stake/keeper/val_state_change.go Outdated Show resolved Hide resolved
x/stake/keeper/val_state_change.go Show resolved Hide resolved
x/stake/keeper/validator.go Show resolved Hide resolved
x/stake/keeper/val_state_change.go Show resolved Hide resolved
x/stake/keeper/val_state_change.go Outdated Show resolved Hide resolved
x/stake/handler.go Show resolved Hide resolved
@cwgoes
Copy link
Contributor

cwgoes commented Oct 3, 2018

lcd test failing locally

This is caused by the underlying TM 0.24 issue.

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

cwgoes commented Oct 3, 2018

This is caused by the underlying TM 0.24 issue.

Now fixed since we temporarily disabled proof verification.

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cwgoes cwgoes merged commit 324bdaf into develop Oct 3, 2018
@rigelrozanski rigelrozanski deleted the rigel/stake-refactor branch October 3, 2018 16:38
@rigelrozanski
Copy link
Contributor Author

:D

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split up UpdateValidator per each state-transitions, & Tendermint updates in Endblock
4 participants