fix!: x/staking - remove delegation with amount is zero #10254

alexanderbez · 2021-09-28T16:06:42Z

Description

Instead of using the shares to determine if a delegation should be removed, use the truncated (token) amount.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

codecov · 2021-09-28T18:17:20Z

Codecov Report

Merging #10254 (0051730) into master (88f39ad) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##           master   #10254   +/-   ##
=======================================
  Coverage   64.15%   64.15%           
=======================================
  Files         572      572           
  Lines       53983    53989    +6     
=======================================
+ Hits        34632    34636    +4     
- Misses      17375    17377    +2     
  Partials     1976     1976

Impacted Files	Coverage Δ
x/staking/keeper/invariants.go	`62.40% <ø> (+0.92%)`	⬆️
x/staking/migrations/v040/types.go	`0.00% <0.00%> (ø)`
x/staking/keeper/migrations.go	`50.00% <60.00%> (ø)`
x/staking/genesis.go	`60.32% <100.00%> (+0.43%)`	⬆️
x/staking/keeper/delegation.go	`72.66% <100.00%> (-0.05%)`	⬇️
x/staking/module.go	`63.88% <100.00%> (+0.50%)`	⬆️

x/staking/keeper/delegation.go

amaury1093 · 2021-09-29T09:42:31Z

looks good. This needs an in-place store migration (and possible a json dump migration too) imo. @alexanderbez I'll let you decide if you want to do it here or in a follow-up PR

robert-zaremba · 2021-09-29T11:35:14Z

Do we have a policy about when we should do a JSON migration? I think in-place migration should be present for each cleanup / state change.

amaury1093 · 2021-09-29T12:12:19Z

Do we have a policy about when we should do a JSON migration?

Imo it's not that much a human policy, but just to make sure that one version's ExportGenesis can be plugged into the next version's InitGenesis. If no, then we need a JSON migration.

ValarDragon · 2021-09-29T15:25:05Z

I don't think this needs to change anything in the export JSON format / merits a migration there. I'd recommend just making the import logic ignore / not initialize things with zero amounts, but non-zero shares.

In place state migration would be a nice state cleanup though!

x/staking/genesis.go

ValarDragon

LGTM! Thanks for fixing this

amaury1093

lgtm! the module's ConsensusVersion needs to be bumped, and I think an in-place migration would be useful to purge the state (both can be done in a follow-up though)

alexanderbez · 2021-10-05T12:34:26Z

lgtm! the module's ConsensusVersion needs to be bumped, and I think an in-place migration would be useful to purge the state (both can be done in a follow-up though)

I'll do it in this PR along with fixing tests.

x/staking/migrations/v045/store.go

amaury1093

nice!

amaury1093 · 2021-10-07T13:52:46Z

2 sims related to import/exports are failing though

alexanderbez · 2021-10-07T19:54:02Z

Yeah I've been trying to debug them @AmauryM. The issue lies in the import genesis changes. It doesn't like the fact that I manually remove delegations.

alexanderbez · 2021-10-07T20:12:21Z

@ValarDragon so turns out there are cases, at least in the simulation with seed 1, where in InitGenesis there is a validator with zero tokens, but non-zero shares and delegation shares. In this case, we can't apply this check because TokensFromShares won't give us the result we want. So

What cases can a validator have zero tokens but non-zero shares? Maybe this is okay...
If so, should we just skip over cases where validator tokens are zero?

ValarDragon · 2021-10-08T02:37:38Z

hrmm, I guess you can have 0 tokens and shares if you had 1uatom self-bond, and then get slashed to 0uatom.

I don't see why the validator has to have shares tho

alexanderbez · 2021-10-08T20:22:26Z

@ValarDragon I'm going to undo the InitGenesis changes and leave it as-is. The reason mainly being is that we don't have existing logic already like that in InitGenesis (i.e. skipping on empty shares). In addition, if I actually remove the core changes in this PR but still add the InitGenesis changes, it still fails.

LMK if this is OK with you and we can merge this PR.

ValarDragon · 2021-10-08T22:39:20Z

That sounds good to me! Then this is fixing things going forward, which is already a significant improvement =)

amaury1093 · 2021-12-13T10:27:35Z

x/staking/keeper/delegation.go

+	// Remove the delegation if the resulting shares yield a truncated zero amount
+	// of tokens in the delegation. Note, in this this case, the shares themselves
+	// may not be zero, but rather a small fractional amount. Otherwise, we update
+	// the delegation object.
+	if validator.TokensFromShares(delegation.Shares).TruncateInt().IsZero() || delegation.Shares.IsZero() {


@alexanderbez do you think this could break delegator shares invariant, causing #10750 ?

@aleem1314 did some debugging on this, here's some logs:

++++++++++++++++++++++++++++++++++++ TOKENS-FROM-SHARES = 0.937500000000000000 validator.TokensFromShares(delegation.Shares).TruncateInt() = 0 DELEGATIONS SHARES = 0.937500000000000000 VALIDATOR = cosmosvaloper14rmvn8ymw7mdjwyjhmkzestgkcdskvkx6ke9e5 DELEGATOR = cosmos15rxvlc2xaf6sjq5fjq45gfapdjsjr97jy6wmjc BLOCKHEIGHT = 32 BLOCKTIME = 2103-10-01 17:18:06 +0000 UTC ++++++++++++++++++++++++++++++++++++

just reverting this line and simulations pass.

Hmm could be! But what invariant is broken exactly? The delegation should be deleted here. Is there some invariant that expects this not to be deleted?

The DelegatorSharesInvariant one. Maybe it should be updated to take TruncateInt into account? The error message is:

broken delegator shares invariance: validator.DelegatorShares: 34148342106.937500000000000000 sum of Delegator.Shares: 34148342106.000000000000000000

which soulds like some truncating error

Yes, let's keep the discussion in the new issue created 👍

## Description Revert #10254 Closes: #10750 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

alexanderbez added 2 commits September 28, 2021 12:05

x/staking: update Unbond

8db3268

cl++

2ee9447

github-actions bot added the C:x/staking label Sep 28, 2021

Merge branch 'master' into bez/10216-del-removal

3144054

alexanderbez marked this pull request as ready for review September 28, 2021 20:28

alexanderbez requested a review from aaronc as a code owner September 28, 2021 20:28

alexanderbez requested review from robert-zaremba, ValarDragon, sunnya97 and amaury1093 September 28, 2021 20:28

ValarDragon reviewed Sep 28, 2021

View reviewed changes

x/staking/keeper/delegation.go Outdated Show resolved Hide resolved

alexanderbez added 5 commits October 4, 2021 09:17

add check

aaab239

update import genesis logic

f9d957b

Merge branch 'master' into bez/10216-del-removal

56599e4

Merge branch 'master' into bez/10216-del-removal

5b61ca5

update import genesis logic

a17ab96

alexanderbez commented Oct 4, 2021

View reviewed changes

x/staking/genesis.go Outdated Show resolved Hide resolved

ValarDragon approved these changes Oct 5, 2021

View reviewed changes

amaury1093 approved these changes Oct 5, 2021

View reviewed changes

alexanderbez added 5 commits October 5, 2021 08:45

x/staking: update consensus version

a8cdfbd

x/staking: prep in-place migration

e85e055

x/staking: prep in-place migration

18c5579

x/staking: prep in-place migration

5c8ed20

x/staking: prep in-place migration

fd1152b

alexanderbez commented Oct 5, 2021

View reviewed changes

x/staking/migrations/v045/store.go Show resolved Hide resolved

alexanderbez and others added 4 commits October 6, 2021 21:57

Merge branch 'master' into bez/10216-del-removal

6835009

updates

43834cf

updates

7f4e1a2

Merge branch 'master' into bez/10216-del-removal

395dd09

amaury1093 approved these changes Oct 7, 2021

View reviewed changes

alexanderbez and others added 2 commits October 8, 2021 16:20

remove genesis changes

25cbe22

Merge branch 'master' into bez/10216-del-removal

0051730

alexanderbez merged commit 3458f64 into master Oct 9, 2021

alexanderbez deleted the bez/10216-del-removal branch October 9, 2021 02:02

amaury1093 reviewed Dec 13, 2021

View reviewed changes

amaury1093 mentioned this pull request Dec 13, 2021

simulations are failing on some PR's #10750

Closed

4 tasks

This was referenced Dec 14, 2021

refactor: revert 10254 #10777

Merged

Empty amount delegation stored after unbonding/redelegation #10216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: x/staking - remove delegation with amount is zero #10254

fix!: x/staking - remove delegation with amount is zero #10254

alexanderbez commented Sep 28, 2021 •

edited

Loading

codecov bot commented Sep 28, 2021 •

edited

Loading

amaury1093 commented Sep 29, 2021

robert-zaremba commented Sep 29, 2021

amaury1093 commented Sep 29, 2021

ValarDragon commented Sep 29, 2021 •

edited

Loading

ValarDragon left a comment

amaury1093 left a comment

alexanderbez commented Oct 5, 2021

amaury1093 left a comment

amaury1093 commented Oct 7, 2021

alexanderbez commented Oct 7, 2021

alexanderbez commented Oct 7, 2021

ValarDragon commented Oct 8, 2021

alexanderbez commented Oct 8, 2021

ValarDragon commented Oct 8, 2021

amaury1093 Dec 13, 2021

alexanderbez Dec 13, 2021

amaury1093 Dec 13, 2021 •

edited

Loading

alexanderbez Dec 13, 2021

fix!: x/staking - remove delegation with amount is zero #10254

fix!: x/staking - remove delegation with amount is zero #10254

Conversation

alexanderbez commented Sep 28, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Sep 28, 2021 • edited Loading

Codecov Report

amaury1093 commented Sep 29, 2021

robert-zaremba commented Sep 29, 2021

amaury1093 commented Sep 29, 2021

ValarDragon commented Sep 29, 2021 • edited Loading

ValarDragon left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

alexanderbez commented Oct 5, 2021

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 commented Oct 7, 2021

alexanderbez commented Oct 7, 2021

alexanderbez commented Oct 7, 2021

ValarDragon commented Oct 8, 2021

alexanderbez commented Oct 8, 2021

ValarDragon commented Oct 8, 2021

amaury1093 Dec 13, 2021

Choose a reason for hiding this comment

alexanderbez Dec 13, 2021

Choose a reason for hiding this comment

amaury1093 Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

alexanderbez Dec 13, 2021

Choose a reason for hiding this comment

alexanderbez commented Sep 28, 2021 •

edited

Loading

codecov bot commented Sep 28, 2021 •

edited

Loading

ValarDragon commented Sep 29, 2021 •

edited

Loading

amaury1093 Dec 13, 2021 •

edited

Loading