From ae68bb7285641aaddff6e725c599bb147f72cc7a Mon Sep 17 00:00:00 2001 From: beer-1 <147697694+beer-1@users.noreply.github.com> Date: Sat, 13 Jul 2024 17:29:23 +0900 Subject: [PATCH 1/5] return proper error message at grpc query handler --- baseapp/grpcserver.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/baseapp/grpcserver.go b/baseapp/grpcserver.go index b1befabe9e86..2fb74e2ab8ef 100644 --- a/baseapp/grpcserver.go +++ b/baseapp/grpcserver.go @@ -14,6 +14,7 @@ import ( "google.golang.org/grpc/status" errorsmod "cosmossdk.io/errors" + storetypes "cosmossdk.io/store/types" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" @@ -67,7 +68,20 @@ func (app *BaseApp) RegisterGRPCServer(server gogogrpc.Server) { app.logger.Debug("gRPC query received of type: " + fmt.Sprintf("%#v", req)) - return handler(grpcCtx, req) + // Catch an OutOfGasPanic caused in the query handlers + defer func() { + if r := recover(); r != nil { + switch rType := r.(type) { + case storetypes.ErrorOutOfGas: + err = errorsmod.Wrapf(sdkerrors.ErrOutOfGas, "out of gas in location: %v", rType.Descriptor) + default: + panic(r) + } + } + }() + resp, err = handler(grpcCtx, req) + + return } // Loop through all services and methods, add the interceptor, and register From 3e362d3954fc98fdf4596ed9a1b284bef715cb7e Mon Sep 17 00:00:00 2001 From: beer-1 <147697694+beer-1@users.noreply.github.com> Date: Sat, 13 Jul 2024 17:40:25 +0900 Subject: [PATCH 2/5] add changelog --- CHANGELOG.md | 1 + baseapp/grpcserver.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0915fbb45ff..cb47253ec1ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,6 +103,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i * (baseapp) [#20208](https://github.com/cosmos/cosmos-sdk/pull/20208) Skip running validateBasic for rechecking txs. * (baseapp) [#20380](https://github.com/cosmos/cosmos-sdk/pull/20380) Enhanced OfferSnapshot documentation. * (client) [#20771](https://github.com/cosmos/cosmos-sdk/pull/20771) Remove `ReadDefaultValuesFromDefaultClientConfig` from `client` package. (It was introduced in `v0.50.6` as a quick fix). +* (grpcserver) [#20945](https://github.com/cosmos/cosmos-sdk/pull/20945) Adds error handling for out-of-gas panics in grpc query handlers. ### Bug Fixes diff --git a/baseapp/grpcserver.go b/baseapp/grpcserver.go index 2fb74e2ab8ef..4dafd3f55e15 100644 --- a/baseapp/grpcserver.go +++ b/baseapp/grpcserver.go @@ -73,7 +73,7 @@ func (app *BaseApp) RegisterGRPCServer(server gogogrpc.Server) { if r := recover(); r != nil { switch rType := r.(type) { case storetypes.ErrorOutOfGas: - err = errorsmod.Wrapf(sdkerrors.ErrOutOfGas, "out of gas in location: %v", rType.Descriptor) + err = errorsmod.Wrapf(sdkerrors.ErrOutOfGas, "Query gas limit exceeded: %v, out of gas in location: %v", sdkCtx.GasMeter().Limit(), rType.Descriptor) default: panic(r) } From 3db611bcf300b7928918b125d401b53edc29e6ff Mon Sep 17 00:00:00 2001 From: beer-1 <147697694+beer-1@users.noreply.github.com> Date: Sat, 13 Jul 2024 17:53:37 +0900 Subject: [PATCH 3/5] directly return handler --- baseapp/grpcserver.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/baseapp/grpcserver.go b/baseapp/grpcserver.go index 4dafd3f55e15..c162bebf1ee3 100644 --- a/baseapp/grpcserver.go +++ b/baseapp/grpcserver.go @@ -79,9 +79,8 @@ func (app *BaseApp) RegisterGRPCServer(server gogogrpc.Server) { } } }() - resp, err = handler(grpcCtx, req) - return + return handler(grpcCtx, req) } // Loop through all services and methods, add the interceptor, and register From 7a90debebe25c245dec32a5d85ae3d3c377e6e7e Mon Sep 17 00:00:00 2001 From: beer-1 <147697694+beer-1@users.noreply.github.com> Date: Tue, 16 Jul 2024 01:20:20 +0900 Subject: [PATCH 4/5] add out of gas grpc testcase --- .../server/grpc/out_of_gas_test.go | 96 +++++++++++++++++++ testutil/network/network.go | 5 + 2 files changed, 101 insertions(+) create mode 100644 tests/integration/server/grpc/out_of_gas_test.go diff --git a/tests/integration/server/grpc/out_of_gas_test.go b/tests/integration/server/grpc/out_of_gas_test.go new file mode 100644 index 000000000000..ce2577e7b02b --- /dev/null +++ b/tests/integration/server/grpc/out_of_gas_test.go @@ -0,0 +1,96 @@ +package grpc_test + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/suite" + "google.golang.org/grpc" + "google.golang.org/grpc/metadata" + + _ "cosmossdk.io/x/accounts" + _ "cosmossdk.io/x/auth" + _ "cosmossdk.io/x/auth/tx/config" + _ "cosmossdk.io/x/bank" + banktypes "cosmossdk.io/x/bank/types" + _ "cosmossdk.io/x/consensus" + _ "cosmossdk.io/x/staking" + + "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/testutil/configurator" + "github.com/cosmos/cosmos-sdk/testutil/network" + "github.com/cosmos/cosmos-sdk/testutil/testdata" +) + +type IntegrationTestOutOfGasSuite struct { + suite.Suite + + cfg network.Config + network network.NetworkI + conn *grpc.ClientConn +} + +func (s *IntegrationTestOutOfGasSuite) SetupSuite() { + var err error + s.T().Log("setting up integration test suite") + + s.cfg, err = network.DefaultConfigWithAppConfigWithQueryGasLimit(configurator.NewAppConfig( + configurator.AccountsModule(), + configurator.AuthModule(), + configurator.BankModule(), + configurator.GenutilModule(), + configurator.StakingModule(), + configurator.ConsensusModule(), + configurator.TxModule(), + ), 10) + s.NoError(err) + s.cfg.NumValidators = 1 + + s.network, err = network.New(s.T(), s.T().TempDir(), s.cfg) + s.Require().NoError(err) + + _, err = s.network.WaitForHeight(2) + s.Require().NoError(err) + + val0 := s.network.GetValidators()[0] + s.conn, err = grpc.NewClient( + val0.GetAppConfig().GRPC.Address, + grpc.WithInsecure(), //nolint:staticcheck // ignore SA1019, we don't need to use a secure connection for tests + grpc.WithDefaultCallOptions(grpc.ForceCodec(codec.NewProtoCodec(s.cfg.InterfaceRegistry).GRPCCodec())), + ) + s.Require().NoError(err) +} + +func (s *IntegrationTestOutOfGasSuite) TearDownSuite() { + s.T().Log("tearing down integration test suite") + s.conn.Close() + s.network.Cleanup() +} + +func (s *IntegrationTestOutOfGasSuite) TestGRPCServer_TestService() { + // gRPC query to test service should work + testClient := testdata.NewQueryClient(s.conn) + testRes, err := testClient.Echo(context.Background(), &testdata.EchoRequest{Message: "hello"}) + s.Require().NoError(err) + s.Require().Equal("hello", testRes.Message) +} + +func (s *IntegrationTestOutOfGasSuite) TestGRPCServer_BankBalance() { + val0 := s.network.GetValidators()[0] + + // gRPC query to bank service should work + denom := fmt.Sprintf("%stoken", val0.GetMoniker()) + bankClient := banktypes.NewQueryClient(s.conn) + var header metadata.MD + _, err := bankClient.Balance( + context.Background(), + &banktypes.QueryBalanceRequest{Address: val0.GetAddress().String(), Denom: denom}, + grpc.Header(&header), // Also fetch grpc header + ) + s.Require().ErrorContains(err, "out of gas") +} + +func TestIntegrationTestOutOfGasSuite(t *testing.T) { + suite.Run(t, new(IntegrationTestOutOfGasSuite)) +} diff --git a/testutil/network/network.go b/testutil/network/network.go index 0d0e942a9fb2..fb2bbd54ae69 100644 --- a/testutil/network/network.go +++ b/testutil/network/network.go @@ -164,6 +164,10 @@ func DefaultConfig(factory TestFixtureFactory) Config { } func DefaultConfigWithAppConfig(appConfig depinject.Config) (Config, error) { + return DefaultConfigWithAppConfigWithQueryGasLimit(appConfig, 0) +} + +func DefaultConfigWithAppConfigWithQueryGasLimit(appConfig depinject.Config, queryGasLimit uint64) (Config, error) { var ( appBuilder *runtime.AppBuilder txConfig client.TxConfig @@ -221,6 +225,7 @@ func DefaultConfigWithAppConfig(appConfig depinject.Config) (Config, error) { baseapp.SetPruning(pruningtypes.NewPruningOptionsFromString(val.GetAppConfig().Pruning)), baseapp.SetMinGasPrices(val.GetAppConfig().MinGasPrices), baseapp.SetChainID(cfg.ChainID), + baseapp.SetQueryGasLimit(queryGasLimit), ) testdata.RegisterQueryServer(app.GRPCQueryRouter(), testdata.QueryImpl{}) From d3381734ef6d273bb4237b26065409a1cfef9fd9 Mon Sep 17 00:00:00 2001 From: beer-1 <147697694+beer-1@users.noreply.github.com> Date: Tue, 16 Jul 2024 01:26:34 +0900 Subject: [PATCH 5/5] fix error checking in test --- tests/integration/server/grpc/out_of_gas_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/integration/server/grpc/out_of_gas_test.go b/tests/integration/server/grpc/out_of_gas_test.go index ce2577e7b02b..f662d2a25ff8 100644 --- a/tests/integration/server/grpc/out_of_gas_test.go +++ b/tests/integration/server/grpc/out_of_gas_test.go @@ -21,6 +21,7 @@ import ( "github.com/cosmos/cosmos-sdk/testutil/configurator" "github.com/cosmos/cosmos-sdk/testutil/network" "github.com/cosmos/cosmos-sdk/testutil/testdata" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) type IntegrationTestOutOfGasSuite struct { @@ -76,7 +77,7 @@ func (s *IntegrationTestOutOfGasSuite) TestGRPCServer_TestService() { s.Require().Equal("hello", testRes.Message) } -func (s *IntegrationTestOutOfGasSuite) TestGRPCServer_BankBalance() { +func (s *IntegrationTestOutOfGasSuite) TestGRPCServer_BankBalance_OutOfGas() { val0 := s.network.GetValidators()[0] // gRPC query to bank service should work @@ -88,7 +89,8 @@ func (s *IntegrationTestOutOfGasSuite) TestGRPCServer_BankBalance() { &banktypes.QueryBalanceRequest{Address: val0.GetAddress().String(), Denom: denom}, grpc.Header(&header), // Also fetch grpc header ) - s.Require().ErrorContains(err, "out of gas") + + s.Require().ErrorContains(err, sdkerrors.ErrOutOfGas.Error()) } func TestIntegrationTestOutOfGasSuite(t *testing.T) {