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

simulations are failing on some PR's #10750

Closed
4 tasks
aleem1314 opened this issue Dec 13, 2021 · 6 comments · Fixed by #10777
Closed
4 tasks

simulations are failing on some PR's #10750

aleem1314 opened this issue Dec 13, 2021 · 6 comments · Fixed by #10777
Labels

Comments

@aleem1314
Copy link
Contributor

Summary of Bug

Simulations are failing on #10714 and #9810 PRs with the following error

simulation halted due to panic on block 35
Logs to writing to /home/callum/.simapp/simulations/2021-11-25_12:50:24.log
--- FAIL: TestAppImportExport (25.18s)
panic: invariant broken: staking: delegator shares invariant
broken delegator shares invariance:
        validator.DelegatorShares: 34148342106.937500000000000000
        sum of Delegator.Shares: 34148342106.000000000000000000


        CRITICAL please submit the following transaction:
                 tx crisis invariant-broken staking delegator-shares [recovered]
        panic: invariant broken: staking: delegator shares invariant

After debugging looks like #10216 breaking delegator shares invariant.

Version

Currently on two PRs

Steps to Reproduce


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093
Copy link
Contributor

@alexanderbez it might be related to this logic: #10254 (comment)

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 13, 2021

Yes, totally! So the validator.TokensFromShares(delegation.Shares).TruncateInt().IsZero() case is getting triggered (good) which deletes the delegation object (good) but the shares remain.

The RemoveValidatorTokensAndShares call should be removing the shares. @aleem1314 can you by any chance run this again but also log the shares that we pass into RemoveValidatorTokensAndShares?

I'm curious if we need to remove the dust shares...

@alexanderbez
Copy link
Contributor

Either we:

  1. Update the invariant
  2. Evaluate if we need to remove the dust shares (and why we're currently not)

@aleem1314
Copy link
Contributor Author

Here's is simulation logs for #9810 2021-12-14_15:10:17.log.

TOKENS_FROM_SHARES validator.TokensFromShares(delegation.Shares) =  0.937500000000000000
TOKENS_FROM_SHARES_TRUNCATE 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

shares that we pass into RemoveValidatorTokensAndShares

RemoveValidatorTokensAndShares =  0.062500000000000000

The delegator shares invariant is breaking with the following error

CRITICAL please submit the following transaction:
                 tx crisis invariant-broken staking delegator-shares [recovered]
        panic: invariant broken: staking: delegator shares invariant
broken delegator shares invariance:
        validator.DelegatorShares: 34148342106.937500000000000000
        sum of Delegator.Shares: 34148342106.000000000000000000

And here are simulation logs for #10714 2021-12-14_15:21:14.log.

TOKENS_FROM_SHARES validator.TokensFromShares(delegation.Shares) =  0.000000000000000000
TOKENS_FROM_SHARES_TRUNCATE validator.TokensFromShares(delegation.Shares).TruncateInt() =  0
DELEGATIONS SHARES =  660271276.166666666355950772
Validator =  cosmosvaloper10gzcdq42gzkhd09uvmzhd48nke0yllaakl42jq
Delegator =  cosmos1p3yjpqn98weh9hxj590n06z08sv9hmjzmt4t75
BlockHeight =  9
BLOCKTIME =  2066-05-12 04:15:29 +0000 UTC

shares that we pass into RemoveValidatorTokensAndShares

RemoveValidatorTokensAndShares =  38839486.833333333644049228

The delegator shares invariant is breaking with the following error

CRITICAL please submit the following transaction:
                 tx crisis invariant-broken staking delegator-shares [recovered]
        panic: invariant broken: staking: delegator shares invariant
broken delegator shares invariance:
        validator.DelegatorShares: 730298878.166666666355950772
        sum of Delegator.Shares: 70027602.000000000000000000

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 14, 2021

I can see why the first seed/sim fails, but the later is a bit confusing. May I suggest we just revert the original PR I made until we can spend time on a proper solution.

WDYT @AmauryM? We don't have the delegation removal PR in an official release yet, do we?

@amaury1093
Copy link
Contributor

May I suggest we just revert the original PR I made until we can spend time on a proper solution.

SGTM 👍. Yeah it's fine, #10216 is only on master and not in any release.

@mergify mergify bot closed this as completed in #10777 Dec 15, 2021
mergify bot pushed a commit that referenced this issue Dec 15, 2021
## 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants