-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Description
Is there an existing issue for this?
- I have searched the existing issues
What happened?
Cosmos is receiving simulation in async way(from the app side) and this can make app.Commit() or app.CheckTx() happened during simulation call. Then the checkState will be overwritten during simulation.
To avoid it, we have to make checkState as versioned cms to make it a snapshot. Also we should introduce RwMutex between app.CheckTx and app.Simulate to prevent that app.CheckTx updates app.checkState during app.Simulate.
Simulation comes directly from the cosmos side grpc interface not through comet ABCI, so we need mutex in cosmos level.
cosmos-sdk/x/auth/tx/service.go
Line 76 in 6d2f6ff
| func (s txServer) Simulate(ctx context.Context, req *txtypes.SimulateRequest) (*txtypes.SimulateResponse, error) { |
CheckTx and Commit mutex hold by comet, but Simulate & CheckTx and Simulate & Commit still need mutex in cosmos level.
Cosmos SDK Version
main, v0.50
How to reproduce?
Concurrently send txs which are creating multiple accounts and do tx simulation to create multiple accounts.
(need some kind of time wait between authkeeper.NextAccountNumber and SetAccount)
This is simple test what happen in the cms when we overwrite the contents.
func (suite *KeeperTestSuite) Test_CacheContext() {
ctx := suite.ctx
cacheCtx, _ := ctx.CacheContext()
pubKey1 := ed25519.GenPrivKey().PubKey()
pubKey2 := ed25519.GenPrivKey().PubKey()
addr1 := sdk.AccAddress(pubKey1.Address())
addr2 := sdk.AccAddress(pubKey2.Address())
// create account to ctx
acc1 := suite.accountKeeper.NewAccountWithAddress(ctx, sdk.AccAddress(addr1))
acc1.SetAccountNumber(100)
suite.accountKeeper.SetAccount(cacheCtx, acc1)
// create account to cacheCtx
acc2 := suite.accountKeeper.NewAccountWithAddress(cacheCtx, sdk.AccAddress(addr2))
acc2.SetAccountNumber(100)
suite.Require().Panics(func() {
suite.accountKeeper.SetAccount(cacheCtx, acc2)
})
}