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

Make apply func if no err, out of gas panic, match std behavior #3746

Merged
merged 5 commits into from
Feb 1, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 logic preserve panics for OutOfGas behavior.

## v13.0.0

Expand Down
25 changes: 23 additions & 2 deletions osmoutils/cache_ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,20 @@ 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 {
PrintPanicRecoveryError(ctx, recoveryError)
err = errors.New("panic occurred during execution")
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")
}
}
}()
// makes a new cache context, which all state changes get wrapped inside of.
Expand All @@ -35,6 +43,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
Comment on lines +48 to +53
Copy link
Member

Choose a reason for hiding this comment

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

What about types.ErrorNegativeGasConsumed?

Copy link
Member Author

@ValarDragon ValarDragon Jan 3, 2023

Choose a reason for hiding this comment

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

oh wow didn't know that existed, let me think about it

great catch

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{}) {
Expand Down
59 changes: 59 additions & 0 deletions osmoutils/cache_ctx_test.go
Original file line number Diff line number Diff line change
@@ -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())
})
}
}