[BP: release/v4 <- #86] fix!: queries that panic should return ack-err

modules/async-icq/README.md

@@ -178,15 +178,20 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) ([]byt
 
 	if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
 		// UnmarshalJSON errors are indeterminate and therefore are not wrapped and included in failed acks
-		return nil, sdkerrors.Wrapf(types.ErrUnknownDataType, "cannot unmarshal ICQ packet data")
+		return nil, errors.Wrapf(types.ErrUnknownDataType, "cannot unmarshal ICQ packet data")
 	}
 
 	reqs, err := types.DeserializeCosmosQuery(data.GetData())
 	if err != nil {
 		return nil, err
 	}
 
-	response, err := k.executeQuery(ctx, reqs)
+	// If we panic when executing a query it should be returned as an error.
+	var response []byte
+	err = applyFuncIfNoError(ctx, func(ctx sdk.Context) error {
+		response, err = k.executeQuery(ctx, reqs)
+		return err
+	})
 	if err != nil {
 		return nil, err
 	}

modules/async-icq/keeper/cache_ctx.go

@@ -0,0 +1,80 @@
+package keeper
+
+// This file was copied from here: https://github.com/osmosis-labs/osmosis/blob/62757d309957fa9e02e6fb0b5dc8caf1ca68e696/osmoutils/cache_ctx.go
+
+import (
+	"errors"
+	"runtime"
+	"runtime/debug"
+
+	"github.com/cosmos/cosmos-sdk/store/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+)
+
+// This function lets you run the function f, but if theres an error or panic
+// 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, _ := 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.
+	cacheCtx, write := ctx.CacheContext()
+	err = f(cacheCtx)
+	if err != nil {
+		ctx.Logger().Error(err.Error())
+	} else {
+		// no error, write the output of f
+		write()
+		ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
+	}
+	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{}) {
+	errStackTrace := string(debug.Stack())
+	switch e := recoveryError.(type) {
+	case types.ErrorOutOfGas:
+		ctx.Logger().Debug("out of gas error inside panic recovery block: " + e.Descriptor)
+		return
+	case string:
+		ctx.Logger().Error("Recovering from (string) panic: " + e)
+	case runtime.Error:
+		ctx.Logger().Error("recovered (runtime.Error) panic: " + e.Error())
+	case error:
+		ctx.Logger().Error("recovered (error) panic: " + e.Error())
+	default:
+		ctx.Logger().Error("recovered (default) panic. Could not capture logs in ctx, see stdout")
+		debug.PrintStack()
+		return
+	}
+	ctx.Logger().Error("stack trace: " + errStackTrace)
+}

modules/async-icq/keeper/relay.go

@@ -27,7 +27,12 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) ([]byt
 		return nil, err
 	}
 
-	response, err := k.executeQuery(ctx, reqs)
+	// If we panic when executing a query it should be returned as an error.
+	var response []byte
+	err = applyFuncIfNoError(ctx, func(ctx sdk.Context) error {
+		response, err = k.executeQuery(ctx, reqs)
+		return err
+	})
 	if err != nil {
 		return nil, err
 	}