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

[Bug]: race condition between baseapp.Commit and baseapp.CreateQueryContext #22650

Open
1 task done
beer-1 opened this issue Nov 26, 2024 · 4 comments · May be fixed by #22692
Open
1 task done

[Bug]: race condition between baseapp.Commit and baseapp.CreateQueryContext #22650

beer-1 opened this issue Nov 26, 2024 · 4 comments · May be fixed by #22692
Labels

Comments

@beer-1
Copy link
Contributor

beer-1 commented Nov 26, 2024

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

We found race condition rs.lastCommitInfo between baseapp.CreateQueryContext and baseapp.Commit.

case 1.

2024-11-26T07:47:34.3846975Z ==================
2024-11-26T07:47:34.3847765Z WARNING: DATA RACE
2024-11-26T07:47:34.3848509Z Read at 0x00c091570220 by goroutine 4277:
2024-11-26T07:47:34.3849460Z   cosmossdk.io/store/rootmulti.(*Store).LastCommitID()
2024-11-26T07:47:34.3855278Z       /home/runner/go/pkg/mod/cosmossdk.io/[email protected]/rootmulti/store.go:441 +0x4c
2024-11-26T07:47:34.3856100Z   cosmossdk.io/store/rootmulti.(*Store).LatestVersion()
2024-11-26T07:47:34.3856999Z       /home/runner/go/pkg/mod/cosmossdk.io/[email protected]/rootmulti/store.go:436 +0x26
2024-11-26T07:47:34.3858198Z   github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).CreateQueryContext()
2024-11-26T07:47:34.3859271Z       /home/runner/go/pkg/mod/github.com/cosmos/[email protected]/baseapp/abci.go:1197 +0x1a9
2024-11-26T07:47:34.3860264Z   github.com/initia-labs/minievm/jsonrpc/backend.(*JSONRPCBackend).getQueryCtx()
2024-11-26T07:47:34.3861705Z       /home/runner/work/minievm/minievm/jsonrpc/backend/tx.go:118 +0x192
2024-11-26T07:47:34.3862801Z   github.com/initia-labs/minievm/jsonrpc/backend.(*JSONRPCBackend).feeFetcher.func1()
2024-11-26T07:47:34.3863766Z       /home/runner/work/minievm/minievm/jsonrpc/backend/backend.go:149 +0xba
2024-11-26T07:47:34.3865063Z   github.com/initia-labs/minievm/jsonrpc/backend.(*JSONRPCBackend).feeFetcher()
2024-11-26T07:47:34.3866000Z       /home/runner/work/minievm/minievm/jsonrpc/backend/backend.go:179 +0x270
2024-11-26T07:47:34.3866976Z   github.com/initia-labs/minievm/jsonrpc/backend.NewJSONRPCBackend.gowrap1()
2024-11-26T07:47:34.3867835Z       /home/runner/work/minievm/minievm/jsonrpc/backend/backend.go:125 +0x33
2024-11-26T07:47:34.3868325Z 
2024-11-26T07:47:34.3868530Z Previous write at 0x00c091570220 by goroutine 3767:
2024-11-26T07:47:34.3869139Z   cosmossdk.io/store/rootmulti.(*Store).Commit()
2024-11-26T07:47:34.3869948Z       /home/runner/go/pkg/mod/cosmossdk.io/[email protected]/rootmulti/store.go:482 +0x3b0
2024-11-26T07:47:34.3870721Z   github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).Commit()
2024-11-26T07:47:34.3871790Z       /home/runner/go/pkg/mod/github.com/cosmos/[email protected]/baseapp/abci.go:933 +0x3da
2024-11-26T07:47:34.3872597Z   github.com/initia-labs/minievm/tests.ExecuteTxs()
2024-11-26T07:47:34.3873296Z       /home/runner/work/minievm/minievm/tests/tx_helper.go:206 +0x4f4
2024-11-26T07:47:34.3874110Z   github.com/initia-labs/minievm/jsonrpc/backend_test.Test_GetBalance()
2024-11-26T07:47:34.3874981Z       /home/runner/work/minievm/minievm/jsonrpc/backend/eth_test.go:27 +0x2af
2024-11-26T07:47:34.3875584Z   testing.tRunner()
2024-11-26T07:47:34.3876580Z       /home/runner/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1690 +0x226
2024-11-26T07:47:34.3877332Z   testing.(*T).Run.gowrap1()
2024-11-26T07:47:34.3878269Z       /home/runner/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1743 +0x44

case 2.

Read at 0x00c09d81cd40 by goroutine 4277:
  cosmossdk.io/store/rootmulti.(*Store).LastCommitID()
      /home/runner/go/pkg/mod/cosmossdk.io/[email protected]/rootmulti/store.go:449 +0x92
  cosmossdk.io/store/rootmulti.(*Store).LatestVersion()
      /home/runner/go/pkg/mod/cosmossdk.io/[email protected]/rootmulti/store.go:436 +0x26
  github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).CreateQueryContext()
      /home/runner/go/pkg/mod/github.com/cosmos/[email protected]/baseapp/abci.go:1197 +0x1a9
  github.com/initia-labs/minievm/jsonrpc/backend.(*JSONRPCBackend).getQueryCtx()
      /home/runner/work/minievm/minievm/jsonrpc/backend/tx.go:118 +0x192
  github.com/initia-labs/minievm/jsonrpc/backend.(*JSONRPCBackend).feeFetcher.func1()
      /home/runner/work/minievm/minievm/jsonrpc/backend/backend.go:149 +0xba
  github.com/initia-labs/minievm/jsonrpc/backend.(*JSONRPCBackend).feeFetcher()
      /home/runner/work/minievm/minievm/jsonrpc/backend/backend.go:179 +0x270
  github.com/initia-labs/minievm/jsonrpc/backend.NewJSONRPCBackend.gowrap1()
      /home/runner/work/minievm/minievm/jsonrpc/backend/backend.go:125 +0x33

Previous write at 0x00c09d81cd40 by goroutine 3767:
  cosmossdk.io/store/rootmulti.commitStores()
      /home/runner/go/pkg/mod/cosmossdk.io/[email protected]/rootmulti/store.go:1216 +0x458
  cosmossdk.io/store/rootmulti.(*Store).Commit()
      /home/runner/go/pkg/mod/cosmossdk.io/[email protected]/rootmulti/store.go:482 +0x39b
  github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).Commit()
      /home/runner/go/pkg/mod/github.com/cosmos/[email protected]/baseapp/abci.go:933 +0x3da
  github.com/initia-labs/minievm/tests.ExecuteTxs()
      /home/runner/work/minievm/minievm/tests/tx_helper.go:206 +0x4f4
  github.com/initia-labs/minievm/jsonrpc/backend_test.Test_GetBalance()
      /home/runner/work/minievm/minievm/jsonrpc/backend/eth_test.go:27 +0x2af
  testing.tRunner()
      /home/runner/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /home/runner/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1743 +0x44

see full logs

As I know, comet has entire lock between commit and query but GRPC still in this race condition risk I guess

sdkCtx, err := app.CreateQueryContext(height, false)

Cosmos SDK Version

0.50

How to reproduce?

maybe hitting tons of grpc queries at block commit timing?

@beer-1 beer-1 added the T:Bug label Nov 26, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Cosmos-SDK Nov 26, 2024
@beer-1 beer-1 changed the title [Bug]: race condition in root multi store rs.lastCommitInfo [Bug]: race condition between baseapp.Commit and ‘baseapp.CreateQueryContext’ Nov 26, 2024
@beer-1 beer-1 changed the title [Bug]: race condition between baseapp.Commit and ‘baseapp.CreateQueryContext’ [Bug]: race condition between baseapp.Commit and baseapp.CreateQueryContext Nov 26, 2024
@beer-1
Copy link
Contributor Author

beer-1 commented Nov 27, 2024

another race condition

WARNING: DATA RACE
Write at 0x00c00021d560 by goroutine 23:
  github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).setState()
      /Users/beer-1/Workspace/cosmos-sdk/baseapp/baseapp.go:497 +0x51c
  github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).Commit()
      /Users/beer-1/Workspace/cosmos-sdk/baseapp/abci.go:956 +0x3e4
  github.com/cosmos/cosmos-sdk/baseapp_test.TestABCI_Race_Commit_Query()
      /Users/beer-1/Workspace/cosmos-sdk/baseapp/abci_test.go:2537 +0x490
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1743 +0x40

Previous read at 0x00c00021d560 by goroutine 72:
  github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).CreateQueryContext()
      /Users/beer-1/Workspace/cosmos-sdk/baseapp/abci.go:1233 +0x23c
  github.com/cosmos/cosmos-sdk/baseapp_test.TestABCI_Race_Commit_Query.func1()
      /Users/beer-1/Workspace/cosmos-sdk/baseapp/abci_test.go:2521 +0x7c

Goroutine 23 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1743 +0x5e0
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:2168 +0x80
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x184
  testing.runTests()
      /usr/local/go/src/testing/testing.go:2166 +0x6e0
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:2034 +0xb74
  main.main()
      _testmain.go:203 +0x110

Goroutine 72 (running) created at:
  github.com/cosmos/cosmos-sdk/baseapp_test.TestABCI_Race_Commit_Query()
      /Users/beer-1/Workspace/cosmos-sdk/baseapp/abci_test.go:2530 +0x404
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1743 +0x40
==================

@beer-1
Copy link
Contributor Author

beer-1 commented Nov 27, 2024

test code I used (run this with race detector)

func TestABCI_Race_Commit_Query(t *testing.T) {
	suite := NewBaseAppSuite(t, baseapp.SetChainID("test-chain-id"))
	app := suite.baseApp

	_, err := app.InitChain(&abci.RequestInitChain{
		ChainId:         "test-chain-id",
		ConsensusParams: &cmtproto.ConsensusParams{Block: &cmtproto.BlockParams{MaxGas: 5000000}},
		InitialHeight:   1,
	})
	require.NoError(t, err)
	_, err = app.Commit()
	require.NoError(t, err)

	counter := atomic.Uint64{}
	counter.Store(0)

	ctx, cancel := context.WithCancel(context.Background())
	queryCreator := func() {
		for {
			select {
			case <-ctx.Done():
				return
			default:
				_, err := app.CreateQueryContext(0, false)
				require.NoError(t, err)

				counter.Add(1)
			}
		}
	}

	for i := 0; i < 100; i++ {
		go queryCreator()
	}

	for i := 0; i < 1000; i++ {
		_, err = app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: app.LastBlockHeight() + 1})
		require.NoError(t, err)

		_, err = app.Commit()
		require.NoError(t, err)
	}

	cancel()

	require.Equal(t, int64(1001), app.GetContextForCheckTx(nil).BlockHeight())
}

@tac0turtle
Copy link
Member

thank you for the issue, would you want to open a pull request on the sdk to add the test and a proposed fix?

@beer-1
Copy link
Contributor Author

beer-1 commented Nov 27, 2024

Okay, let me try it by EoW

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 📋 Backlog
Development

Successfully merging a pull request may close this issue.

2 participants