From 7071a997315e2211d6084dcd170b62a48b53ed2a Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Thu, 15 Dec 2022 22:47:36 -0600 Subject: [PATCH 1/4] Remove logging for apply func if no err, out of gas --- osmoutils/cache_ctx.go | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/osmoutils/cache_ctx.go b/osmoutils/cache_ctx.go index 6e398cdbc4d..95ff4a1a00b 100644 --- a/osmoutils/cache_ctx.go +++ b/osmoutils/cache_ctx.go @@ -18,8 +18,15 @@ func ApplyFuncIfNoError(ctx sdk.Context, f func(ctx sdk.Context) error) (err err // Add a panic safeguard defer func() { if recoveryError := recover(); recoveryError != nil { - PrintPanicRecoveryError(ctx, recoveryError) - err = errors.New("panic occurred during execution") + if isErr, descriptor := IsOutOfGasError(recoveryError); isErr { + // don't log anything, this is routine. Just return an error. + // The CacheCtx shares the same gas meter as the underlying Ctx, per SDK design. + // so all gas writes are already on the underlying gas meter. + err = errors.New("out of gas occurred during execution: " + descriptor) + } else { + PrintPanicRecoveryError(ctx, recoveryError) + err = errors.New("panic occurred during execution") + } } }() // makes a new cache context, which all state changes get wrapped inside of. @@ -35,6 +42,19 @@ func ApplyFuncIfNoError(ctx sdk.Context, f func(ctx sdk.Context) error) (err err return err } +// Frustratingly, this has to return the error descriptor, not an actual error itself +// because the SDK errors here are not actually errors. (They don't implement error interface) +func IsOutOfGasError(err any) (bool, string) { + switch e := err.(type) { + case types.ErrorOutOfGas: + return true, e.Descriptor + case types.ErrorGasOverflow: + return true, e.Descriptor + default: + return false, "" + } +} + // PrintPanicRecoveryError error logs the recoveryError, along with the stacktrace, if it can be parsed. // If not emits them to stdout. func PrintPanicRecoveryError(ctx sdk.Context, recoveryError interface{}) { From ff634357df57a6b55f3988b133e092fb646412b2 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Thu, 15 Dec 2022 23:04:47 -0600 Subject: [PATCH 2/4] Add test case --- osmoutils/cache_ctx.go | 11 +++---- osmoutils/cache_ctx_test.go | 59 +++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 5 deletions(-) create mode 100644 osmoutils/cache_ctx_test.go diff --git a/osmoutils/cache_ctx.go b/osmoutils/cache_ctx.go index 95ff4a1a00b..0d4cbe6802b 100644 --- a/osmoutils/cache_ctx.go +++ b/osmoutils/cache_ctx.go @@ -14,15 +14,16 @@ import ( // drop the state machine change and log the error. // If there is no error, proceeds as normal (but with some slowdown due to SDK store weirdness) // Try to avoid usage of iterators in f. +// +// If its an out of gas panic, this function will also panic like in normal tx execution flow. +// This is still safe for beginblock / endblock code though, as they do not have out of gas panics. func ApplyFuncIfNoError(ctx sdk.Context, f func(ctx sdk.Context) error) (err error) { // Add a panic safeguard defer func() { if recoveryError := recover(); recoveryError != nil { - if isErr, descriptor := IsOutOfGasError(recoveryError); isErr { - // don't log anything, this is routine. Just return an error. - // The CacheCtx shares the same gas meter as the underlying Ctx, per SDK design. - // so all gas writes are already on the underlying gas meter. - err = errors.New("out of gas occurred during execution: " + descriptor) + if isErr, _ := IsOutOfGasError(recoveryError); isErr { + // We panic with the same error, to replicate the normal tx execution flow. + panic(recoveryError) } else { PrintPanicRecoveryError(ctx, recoveryError) err = errors.New("panic occurred during execution") diff --git a/osmoutils/cache_ctx_test.go b/osmoutils/cache_ctx_test.go new file mode 100644 index 00000000000..a8058ea0d71 --- /dev/null +++ b/osmoutils/cache_ctx_test.go @@ -0,0 +1,59 @@ +package osmoutils_test + +import ( + "github.com/cosmos/cosmos-sdk/store/types" + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/osmosis-labs/osmosis/v13/osmoutils" +) + +var expectedOutOfGasError = types.ErrorOutOfGas{Descriptor: "my func"} + +func consumeGas(ctx sdk.Context, gas uint64, numTimes int) error { + for i := 0; i < numTimes; i++ { + ctx.GasMeter().ConsumeGas(gas, "my func") + } + return nil +} + +func (s *TestSuite) TestCacheCtxConsumeGas() { + // every test case adds 1k gas 10 times + testcases := map[string]struct { + gasLimit uint64 + gasUsedPreCtx uint64 + gasUsedPostCtx uint64 + expectPanic bool + }{ + "no gas limit hit": { + gasLimit: 15_000, + gasUsedPreCtx: 111, + gasUsedPostCtx: 111 + 10_000, + expectPanic: false, + }, + "gas limit hit": { + gasLimit: 10_000, + gasUsedPreCtx: 111, + gasUsedPostCtx: 111 + 10_000, + expectPanic: true, + }, + } + for name, tc := range testcases { + s.Run(name, func() { + ctx := s.Ctx.WithGasMeter(sdk.NewGasMeter(tc.gasLimit)) + ctx.GasMeter().ConsumeGas(tc.gasUsedPreCtx, "pre ctx") + var err error + f := func() { + osmoutils.ApplyFuncIfNoError(ctx, func(c sdk.Context) error { + return consumeGas(c, 1000, 10) + }) + } + if tc.expectPanic { + s.PanicsWithValue(expectedOutOfGasError, f) + } else { + f() + s.Require().NoError(err) + } + s.Equal(tc.gasUsedPostCtx, ctx.GasMeter().GasConsumed()) + }) + } +} From 0cf2b8b2547a7889c06040a94d17db07187edfd2 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Thu, 15 Dec 2022 23:18:00 -0600 Subject: [PATCH 3/4] Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index feea8e04c1b..eb808bbd311 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#3634](https://github.com/osmosis-labs/osmosis/pull/3634) (Makefile) Ensure correct golang version in make build and make install. (Thank you @jhernandezb ) * [#3712](https://github.com/osmosis-labs/osmosis/pull/3712) replace `osmomath.BigDec` `Power` with `PowerInteger` * [#3711](https://github.com/osmosis-labs/osmosis/pull/3711) Use Dec instead of Int for additive `ErrTolerace` in `osmoutils`. +* [#3746](https://github.com/osmosis-labs/osmosis/pull/3746) Make ApplyFuncIfNoErr blocks preserve panics for OutOfGas behavior. ## v13.0.0 From c891ba2ca1d0e2c1ca962068fa41a4319f9d11b1 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Fri, 16 Dec 2022 11:16:58 -0600 Subject: [PATCH 4/4] Actually Add changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb808bbd311..f5e24adcbeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,7 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#3634](https://github.com/osmosis-labs/osmosis/pull/3634) (Makefile) Ensure correct golang version in make build and make install. (Thank you @jhernandezb ) * [#3712](https://github.com/osmosis-labs/osmosis/pull/3712) replace `osmomath.BigDec` `Power` with `PowerInteger` * [#3711](https://github.com/osmosis-labs/osmosis/pull/3711) Use Dec instead of Int for additive `ErrTolerace` in `osmoutils`. -* [#3746](https://github.com/osmosis-labs/osmosis/pull/3746) Make ApplyFuncIfNoErr blocks preserve panics for OutOfGas behavior. +* [#3746](https://github.com/osmosis-labs/osmosis/pull/3746) Make ApplyFuncIfNoErr logic preserve panics for OutOfGas behavior. ## v13.0.0