[Bug]: Race condition on app.checkTxState

[nivasan1]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

The Issue

• Ref this stack-trace: https://github.com/skip-mev/block-sdk/actions/runs/7253739964/job/19761083299?pr=312#step:5:496
• This write in internalFinalizeBlock

  cosmos-sdk/baseapp/abci.go

  Line 760 in 47770f3

  app.checkState.ctx = app.checkState.ctx.

• This read in CreateQueryContext

  cosmos-sdk/baseapp/abci.go

  Line 1239 in 47770f3

  WithGasMeter(storetypes.NewGasMeter(app.queryGasLimit)).WithBlockHeader(app.checkState.ctx.BlockHeader())

  are not synchronized

Proposed Solution

• Add a CurrentHeader field to the baseApp that is wrapped by a mutex to avoid the need for unsynchronized accesses to the app.checkTxState.
• Technically, baseApp ABCI methods are not thead-safe, but all cometBFT ABCI methods are executed synchronously (localAppConn). A different solution could be to make the app-states thread-safe, but this is a much larger lift IMO

Cosmos SDK Version

v0.50

How to reproduce?

No response

[alexanderbez]

Great write up, thanks @nivasan1.

So while the AppManager/runtime stuff is being worked on, this should still be addressed for current and future v0.50.x Eden users.

I believe your proposal would work. Another proposal I would consider is making the private state type thread-safe, i.e. add a mutex to it and define a getter for the context.

func (s *state) getContext() sdk.Context {
  s.mtx.RLock()
  defer s.mtx.RUnlock()

  return s.ctx
}

And ensure all readers call getContext() rather than using the field directly.

cc @tac0turtle @julienrbrt

[nivasan1]

@alexanderbez How would you make runTx / extended usage of the state.ctx synchronized with accesses to getCtx? You would need some form of reference counting, I'd imagine? Maybe I'm misunderstanding tho?

[alexanderbez]

@alexanderbez How would you make runTx / extended usage of the state.ctx synchronized with accesses to getCtx? You would need some form of reference counting, I'd imagine? Maybe I'm misunderstanding tho?

So you don't need to make Context synchronized, after all, it's a non-mutative Context. What you do is you simply make the assumption that the underlying storage primative, in this case, RootStore, is synchronized.