Fix negative stake & invariance bug #3033
Conversation
jaekwon
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
|
OK - here's a PR for linter fixes and the non-negative tokens check: #3037 Which simulations are still failing? They seem to pass on CI and for me. |
* Update invariant; fix lint * Fix linter
Codecov Report
@@ Coverage Diff @@
## develop #3033 +/- ##
===========================================
- Coverage 52.2% 52.17% -0.04%
===========================================
Files 140 140
Lines 9764 9766 +2
===========================================
- Hits 5097 5095 -2
- Misses 4328 4330 +2
- Partials 339 341 +2 |
| // this happens if shares are zero but tokens are not. | ||
| pool := k.GetPool(ctx) | ||
| pool.LooseTokens = pool.LooseTokens.Sub(validator.Tokens) | ||
| k.SetPool(ctx, pool) |
There was a problem hiding this comment.
We should probably indicate "burn" tokens here - because that's what's happening.
Also FYI this shouldn't be necessary once the Dec->Int Update is made due to an update to RemoveDelegatorShares https://github.com/cosmos/cosmos-sdk/pull/2958/files#diff-ff4682cf67c96b47c013393f099d1d0eR407
There was a problem hiding this comment.
Updated comment, added TODO.
| issuedTokens := v.DelegatorShareExRate().Mul(delShares) | ||
| v.Tokens = v.Tokens.Sub(issuedTokens) | ||
| delTokens := v.DelegatorShareExRate().Mul(delShares) | ||
| delTokens = sdk.MinDec(delTokens, v.Tokens) |
There was a problem hiding this comment.
Why is this necessary? When are there insufficient tokens? Would that mean some other delegator got more than they ought to?
There was a problem hiding this comment.
All this logic is updated in #2958 p.s. as mentioned in the previous comment
Fixes regression introduced by #2984. Continuiation of #3033 , which didn't fix the simulation issues. (candidate) Complete solution for #3019, 9002 halt bug. From #2984, it isn't sufficient to take the fee pool rewards of a validator. Since we don't track delegator accums (as we do with validator accums), and because onValidatorModified >updateValidatorDistInfoFromPool is also being called upon delegation updates (or at least I believe this is the reason), it is necessary to also withdraw self delegation. TODO: I don't think self-delegation should be required to be modified here... consider using a delegation hook to do the self-delegation withdraw part instead, e.g. splitting the updateValidatorDistInfoFromPool function into two. It might not result in cleaner code, however. Think hard.
Upon debugging #3019, I discovered, in order,:
(1) the transactions of the offending block aren't relevant.
(2) stake.EndBlocker is the offender.
(3) UnbondAllMatureValidatorQueue is the offender.
(4) that RemoveValidator was being called.
(5) the pool is being passed around in places where it is unnecessary.
(6) that this validator had 0 shares and negative staking tokens.
(7) RemoveTokens is not being re-used, and it lacks sanity checks.
There are 2 bugs here that are addressed here:
Also, the pool is removed where unnecessary, and RemoveTokens is reused.
I've verified that these changes would have made 9002 pass at least that offending block, though I'm not certain that I've found all the places where negative staked validators can occur.
TODO: Run simulations. --- Simulation is still failing.
TODO: Create PositiveValidatorStakeInvariant