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

fix: app wiring simulation failures #12051

Merged
merged 6 commits into from
May 26, 2022
Merged

Conversation

kocubinski
Copy link
Member

@kocubinski kocubinski commented May 26, 2022

Description

Resolves simulation test failures introduced in #11924.

x/staking failure

Symptom of this failure is the descriptive log message

Simulating... block 5/50, operation 500/753. Logs to writing to /home/mkoco/.simapp/simulations/2022-05-25_16:39:13.log
    simulate.go:302: error on block  5/50, operation (534/753) from x/staking:
        failed to execute message; message index: 0: amount is greater than the unbonding delegation entry balance: invalid request [/home/mkoco/go/pkg/mod/cosmossdk.io/errors@v1.0.0-beta.6/errors.go:187]
        Comment: unable to deliver tx

originating in x/staking/keeper/msg_server.CancelUnbondingDelegation. I was not able to find a concrete fix other than re-adding a new instance of the params module back to SimApp's call to module.NewSimulationManager which evidently has a side-effect I do not understand.

store test failure

error log:

compared 0 different key/value pairs between KVStoreKey{0xc0010c5e00, acc} and KVStoreKey{0xc000b44560, acc}
compared 0 different key/value pairs between KVStoreKey{0xc0010c5e20, staking} and KVStoreKey{0xc000b44580, staking}
compared 0 different key/value pairs between KVStoreKey{0xc0010c5e50, slashing} and KVStoreKey{0xc000b445b0, slashing}
compared 0 different key/value pairs between KVStoreKey{0xc0010c5e30, mint} and KVStoreKey{0xc000b44590, mint}
compared 0 different key/value pairs between KVStoreKey{0xc0010c5e40, distribution} and KVStoreKey{0xc000b445a0, distribution}
compared 0 different key/value pairs between KVStoreKey{0xc0010c5e10, bank} and KVStoreKey{0xc000b44570, bank}
--- FAIL: TestAppImportExport (17.93s)
panic: kv store with key <nil> has not been registered in stores [recovered]

originating in sim_test.go. The root cause here seems to be test code which either fetches from SimApp.keys directly or uses SimApp.GetKey to fetch a StoreKey instance in order to build a keeper for various testing functions. Use of this pattern is ubiquitous:

image

To this end a store key fetching function was added to runtime/app. As more keepers are moved over to the runtime they should just work in test as SimApp.GetKey now falls back to App.FindStoreKey on not found. I am not a huge fan of this approach since it's exposing runtime internals, alternative approaches include:

  • maintain a duplicate list of storeKeys derived from runtime in SimApp
  • refactor all testing code (37 usages) to build/fetch keepers in a different way so this dependency can be removed (not sure how possible this without reviewing each usage)
  • as per @aaronc use an unsafe flag to disable FindStoreKey by deafult

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 in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • 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 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)

@@ -416,6 +416,7 @@ func NewSimApp(
staking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper),
distr.NewAppModule(appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper),
slashing.NewAppModule(appCodec, app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper),
params.NewAppModule(app.ParamsKeeper),
Copy link
Member Author

Choose a reason for hiding this comment

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

The (re)addition of this line resolves

Simulating... block 5/50, operation 500/753. Logs to writing to /home/mkoco/.simapp/simulations/2022-05-25_16:39:13.log
    simulate.go:302: error on block  5/50, operation (534/753) from x/staking:
        failed to execute message; message index: 0: amount is greater than the unbonding delegation entry balance: invalid request [/home/mkoco/go/pkg/mod/cosmossdk.io/errors@v1.0.0-beta.6/errors.go:187]
        Comment: unable to deliver tx

But I don't understand how or why.

Copy link
Member

Choose a reason for hiding this comment

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

Ah this is correct actually. The runtime app doesn't have a simulation manager yet. But we should add that

@kocubinski kocubinski force-pushed the kocubinski/app-wiring-sim-fixes branch from 0d0e129 to d94cee3 Compare May 26, 2022 15:40
@kocubinski kocubinski marked this pull request as ready for review May 26, 2022 16:00
@kocubinski kocubinski requested a review from a team as a code owner May 26, 2022 16:00
runtime/app.go Outdated Show resolved Hide resolved
@kocubinski kocubinski force-pushed the kocubinski/app-wiring-sim-fixes branch from b8222a2 to eeb4a10 Compare May 26, 2022 18:54
@kocubinski kocubinski mentioned this pull request May 26, 2022
19 tasks
@@ -544,7 +545,17 @@ func (app *SimApp) InterfaceRegistry() codectypes.InterfaceRegistry {
//
// NOTE: This is solely to be used for testing purposes.
func (app *SimApp) GetKey(storeKey string) *storetypes.KVStoreKey {
return app.keys[storeKey]
kvsk := app.keys[storeKey]
Copy link
Member

Choose a reason for hiding this comment

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

We must do this for MemKeys and TransientKeys too (found out while working on x/capability)

.github/workflows/test.yml Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
@kocubinski kocubinski force-pushed the kocubinski/app-wiring-sim-fixes branch from fb49ca1 to d90d8d9 Compare May 26, 2022 20:02
@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #12051 (d90d8d9) into main (e1413cb) will decrease coverage by 0.00%.
The diff coverage is 9.09%.

❗ Current head d90d8d9 differs from pull request most recent head 1993be8. Consider uploading reports for the commit 1993be8 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12051      +/-   ##
==========================================
- Coverage   66.09%   66.09%   -0.01%     
==========================================
  Files         671      671              
  Lines       70917    70927      +10     
==========================================
+ Hits        46874    46876       +2     
- Misses      21385    21394       +9     
+ Partials     2658     2657       -1     
Impacted Files Coverage Δ
simapp/app.go 83.48% <9.09%> (-2.28%) ⬇️
x/group/keeper/keeper.go 54.43% <0.00%> (-0.43%) ⬇️
x/params/module.go 66.66% <0.00%> (+1.58%) ⬆️
crypto/keys/internal/ecdsa/privkey.go 83.01% <0.00%> (+1.88%) ⬆️

@kocubinski kocubinski force-pushed the kocubinski/app-wiring-sim-fixes branch from d90d8d9 to 1993be8 Compare May 26, 2022 20:12
@github-actions github-actions bot removed the Type: CI label May 26, 2022
@alexanderbez alexanderbez merged commit 52fdb08 into main May 26, 2022
@alexanderbez alexanderbez deleted the kocubinski/app-wiring-sim-fixes branch May 26, 2022 20:28
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants