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: concurrency issues between simulation, grpc queries and ABCI commit flow #213

Merged
merged 11 commits into from
May 2, 2022

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Apr 29, 2022

Description

Fixes the problem in: osmosis-labs/osmosis#1356

Previously, queries were always directed to ABCI, getting eventually synchronized with the commit flow. In #137 , we allowed GRPC queries to be concurrent with the ABCI commit flow. However, integration tests were not configured correctly to test that update. Specifically, the GRPC client was never set up in the integration suite. As a result, the flow with concurrent GRPC queries was never tested.

Upon setting up the GRPC client correctly in testutil/network/util.go, 2 problems were exposed:

  1. A number of data races across multiple GRPC queries and simulation unit tests
  2. In some cases, the requested height was never propagated correctly by the GRPC call in the client. As a result, the GRPC server never read the requested height and always assumed the latest. According to the current design, height may come from grpc context or client context. Currently, if the caller provided height through grpc context, that would work. However, it wouldn't if they attempted to supply it via the client context, similar to how integration tests currently do. As a result, I added a function to choose the right height (the priority is described in the spec) and correctly pass it to either GRPC or ABCI query.

Also, I realized that a number of queries must never be concurrent with ABCI. Specifically, tendermint and simulation. Therefore, those are now always directed to the ABCI flow to avoid data races. This change should directly address the reported problem in osmosis-labs/osmosis#1356

I proceeded by writing table-driven tests for TestGRPCQuery in client and discovered that if a user request a height that does not exist, an error is returned. This is stemming from the fact that IAVL tree never returns an error from GetImmutable(< height >) when a non-existent height is requested. Instead it returns an empty tree with nil. This is contrary to what is stated in the following comment. This problem should be investigated further in a separate issue and potentially fixed. For now, I added an additional safeguard against non-existent heights here

Task Log

  • enable GRPC client in integration tests
  • fix simulation and Tendermint GRPC queries by directing it to the ABCI flow
  • correctly pass height metadata to fix a number of other GRPC query tests
  • add unit tests for the new selectHeight method
  • improve TestGRPCQuery integration tests with table-driven tests
  • fix querying non-existent heights.

@p0mvn p0mvn force-pushed the roman/simulate-tx branch from 5d920ae to 6fd8642 Compare April 30, 2022 18:58
client/grpc_query_test.go Outdated Show resolved Hide resolved
client/grpc_query_test.go Outdated Show resolved Hide resolved
testutil/network/network.go Outdated Show resolved Hide resolved
@p0mvn p0mvn changed the title fix: simulation concurrency race fix: concurrency issues between simulation, grpc queries and ABCI commit flow Apr 30, 2022
baseapp/abci.go Outdated
@@ -603,15 +603,19 @@ func (app *BaseApp) createQueryContext(height int64, prove bool) (sdk.Context, e
)
}

cacheMS, err := app.cms.CacheMultiStoreWithVersion(height)
if err != nil {
if height > app.LastBlockHeight() {

Choose a reason for hiding this comment

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

We should store app.LastBlockHeight() in a variable since we use it twice :)

baseapp/abci.go Outdated Show resolved Hide resolved

// selectHeight returns the height chosen from client context and grpc context.
// If exists, height extracted from grpcCtx takes precedence.
func selectHeight(clientContext Context, grpcCtx gocontext.Context) (height int64, err error) {

Choose a reason for hiding this comment

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

can we avoid named return vars?

@p0mvn p0mvn merged commit 18b0d2d into osmosis-main May 2, 2022
@p0mvn p0mvn deleted the roman/simulate-tx branch May 2, 2022 18:13
mergify bot pushed a commit that referenced this pull request May 2, 2022
…mit flow (#213)

* fix: simulation and grpc query concurrency with ABCI

* table tests for selectHeight

* add table-driven GRPC query tests and fix non-existent heights

* clean up

* clean up grpc query tests

* Apply suggestions from code review

* Update testutil/network/network.go

* fix typo in abci.go

* Update baseapp/abci.go

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

* store last block height in a var

* avoid returning named parameters

Co-authored-by: Aleksandr Bezobchuk <[email protected]>
(cherry picked from commit 18b0d2d)
Comment on lines +123 to +125
// selectHeight returns the height chosen from client context and grpc context.
// If exists, height extracted from grpcCtx takes precedence.
func selectHeight(clientContext Context, grpcCtx gocontext.Context) (int64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Are we choosing 0 when neither is set? If so maybe lets set this more explicitly?

Should we be having input validation for heights > latest block height?

Comment on lines +123 to +125
// selectHeight returns the height chosen from client context and grpc context.
// If exists, height extracted from grpcCtx takes precedence.
func selectHeight(clientContext Context, grpcCtx gocontext.Context) (int64, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Are we choosing 0 when neither is set? If so maybe lets set this more explicitly?

Should we be having input validation for heights > latest block height?

Copy link
Member Author

Choose a reason for hiding this comment

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

Zero is guaranteed to be returned in that case by var height int64 which is being initialized to 0. There is a test to cover this case as well. How are you suggesting making this more explicit? With height := int64(0) or adding a relevant comment to the spec? Just making sure I understand what you mean

The validation for heights > latest block height is done at the server layer. It would be challenging to have that information here in client.

Choose a reason for hiding this comment

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

No need for height := int64(0) IMO.

@p0mvn p0mvn mentioned this pull request May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants