From d2e2016e1ee8f2e424e1727e5b0836dc114f9727 Mon Sep 17 00:00:00 2001 From: Vitaly Volozhinov <60775046+VitalyVolozhinov@users.noreply.github.com> Date: Tue, 7 Nov 2023 01:02:13 +0100 Subject: [PATCH] fix!: queries that panic should return `ack-err` (#86) * adding ApplyFuncIfNoError * Moving cache_ctx.go to the keeper * updating comment * chore: address linter errors --------- Co-authored-by: jtieri (cherry picked from commit 83f866e7f1d2577477ce3ad38c67edb581ce29b4) # Conflicts: # modules/async-icq/testing/demo-simapp/app/export.go --- modules/async-icq/README.md | 9 +- modules/async-icq/keeper/cache_ctx.go | 80 ++++++++ modules/async-icq/keeper/relay.go | 7 +- .../testing/demo-simapp/app/export.go | 186 ++++++++++++++++++ 4 files changed, 279 insertions(+), 3 deletions(-) create mode 100644 modules/async-icq/keeper/cache_ctx.go create mode 100644 modules/async-icq/testing/demo-simapp/app/export.go diff --git a/modules/async-icq/README.md b/modules/async-icq/README.md index d8e8fe15..3e119597 100644 --- a/modules/async-icq/README.md +++ b/modules/async-icq/README.md @@ -178,7 +178,7 @@ 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()) @@ -186,7 +186,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 } diff --git a/modules/async-icq/keeper/cache_ctx.go b/modules/async-icq/keeper/cache_ctx.go new file mode 100644 index 00000000..42ce24e4 --- /dev/null +++ b/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) +} diff --git a/modules/async-icq/keeper/relay.go b/modules/async-icq/keeper/relay.go index 4929b9a2..61887b72 100644 --- a/modules/async-icq/keeper/relay.go +++ b/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 } diff --git a/modules/async-icq/testing/demo-simapp/app/export.go b/modules/async-icq/testing/demo-simapp/app/export.go new file mode 100644 index 00000000..41dcc83d --- /dev/null +++ b/modules/async-icq/testing/demo-simapp/app/export.go @@ -0,0 +1,186 @@ +package app + +import ( + "encoding/json" + "log" + + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + + servertypes "github.com/cosmos/cosmos-sdk/server/types" + sdk "github.com/cosmos/cosmos-sdk/types" + slashingtypes "github.com/cosmos/cosmos-sdk/x/slashing/types" + "github.com/cosmos/cosmos-sdk/x/staking" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" +) + +// ExportAppStateAndValidators exports the state of the application for a genesis +// file. +func (app *App) ExportAppStateAndValidators( + forZeroHeight bool, jailAllowedAddrs []string, +) (servertypes.ExportedApp, error) { + + // as if they could withdraw from the start of the next block + ctx := app.NewContext(true, tmproto.Header{Height: app.LastBlockHeight()}) + + // We export at last height + 1, because that's the height at which + // Tendermint will start InitChain. + height := app.LastBlockHeight() + 1 + if forZeroHeight { + height = 0 + app.prepForZeroHeightGenesis(ctx, jailAllowedAddrs) + } + + genState := app.mm.ExportGenesis(ctx, app.appCodec) + appState, err := json.MarshalIndent(genState, "", " ") + if err != nil { + return servertypes.ExportedApp{}, err + } + + validators, err := staking.WriteValidators(ctx, app.StakingKeeper) + if err != nil { + return servertypes.ExportedApp{}, err + } + return servertypes.ExportedApp{ + AppState: appState, + Validators: validators, + Height: height, + ConsensusParams: app.BaseApp.GetConsensusParams(ctx), + }, nil +} + +// prepare for fresh start at zero height +// NOTE zero height genesis is a temporary feature which will be deprecated +// +// in favour of export at a block height +func (app *App) prepForZeroHeightGenesis(ctx sdk.Context, jailAllowedAddrs []string) { + applyAllowedAddrs := false + + // check if there is a allowed address list + if len(jailAllowedAddrs) > 0 { + applyAllowedAddrs = true + } + + allowedAddrsMap := make(map[string]bool) + + for _, addr := range jailAllowedAddrs { + _, err := sdk.ValAddressFromBech32(addr) + if err != nil { + log.Fatal(err) + } + allowedAddrsMap[addr] = true + } + + /* Just to be safe, assert the invariants on current state. */ + app.CrisisKeeper.AssertInvariants(ctx) + + /* Handle fee distribution state. */ + + // withdraw all validator commission + app.StakingKeeper.IterateValidators(ctx, func(_ int64, val stakingtypes.ValidatorI) (stop bool) { + _, err := app.DistrKeeper.WithdrawValidatorCommission(ctx, val.GetOperator()) + if err != nil { + panic(err) + } + return false + }) + + // withdraw all delegator rewards + dels := app.StakingKeeper.GetAllDelegations(ctx) + for _, delegation := range dels { + _, err := app.DistrKeeper.WithdrawDelegationRewards(ctx, delegation.GetDelegatorAddr(), delegation.GetValidatorAddr()) + if err != nil { + panic(err) + } + } + + // clear validator slash events + app.DistrKeeper.DeleteAllValidatorSlashEvents(ctx) + + // clear validator historical rewards + app.DistrKeeper.DeleteAllValidatorHistoricalRewards(ctx) + + // set context height to zero + height := ctx.BlockHeight() + ctx = ctx.WithBlockHeight(0) + + // reinitialize all validators + app.StakingKeeper.IterateValidators(ctx, func(_ int64, val stakingtypes.ValidatorI) (stop bool) { + // donate any unwithdrawn outstanding reward fraction tokens to the community pool + scraps := app.DistrKeeper.GetValidatorOutstandingRewardsCoins(ctx, val.GetOperator()) + feePool := app.DistrKeeper.GetFeePool(ctx) + feePool.CommunityPool = feePool.CommunityPool.Add(scraps...) + app.DistrKeeper.SetFeePool(ctx, feePool) + + app.DistrKeeper.Hooks().AfterValidatorCreated(ctx, val.GetOperator()) + return false + }) + + // reinitialize all delegations + for _, del := range dels { + app.DistrKeeper.Hooks().BeforeDelegationCreated(ctx, del.GetDelegatorAddr(), del.GetValidatorAddr()) + app.DistrKeeper.Hooks().AfterDelegationModified(ctx, del.GetDelegatorAddr(), del.GetValidatorAddr()) + } + + // reset context height + ctx = ctx.WithBlockHeight(height) + + /* Handle staking state. */ + + // iterate through redelegations, reset creation height + app.StakingKeeper.IterateRedelegations(ctx, func(_ int64, red stakingtypes.Redelegation) (stop bool) { + for i := range red.Entries { + red.Entries[i].CreationHeight = 0 + } + app.StakingKeeper.SetRedelegation(ctx, red) + return false + }) + + // iterate through unbonding delegations, reset creation height + app.StakingKeeper.IterateUnbondingDelegations(ctx, func(_ int64, ubd stakingtypes.UnbondingDelegation) (stop bool) { + for i := range ubd.Entries { + ubd.Entries[i].CreationHeight = 0 + } + app.StakingKeeper.SetUnbondingDelegation(ctx, ubd) + return false + }) + + // Iterate through validators by power descending, reset bond heights, and + // update bond intra-tx counters. + store := ctx.KVStore(app.keys[stakingtypes.StoreKey]) + iter := sdk.KVStoreReversePrefixIterator(store, stakingtypes.ValidatorsKey) + counter := int16(0) + + for ; iter.Valid(); iter.Next() { + addr := sdk.ValAddress(iter.Key()[1:]) + validator, found := app.StakingKeeper.GetValidator(ctx, addr) + if !found { + panic("expected validator, not found") + } + + validator.UnbondingHeight = 0 + if applyAllowedAddrs && !allowedAddrsMap[addr.String()] { + validator.Jailed = true + } + + app.StakingKeeper.SetValidator(ctx, validator) + counter++ + } + + iter.Close() + + if _, err := app.StakingKeeper.ApplyAndReturnValidatorSetUpdates(ctx); err != nil { + panic(err) + } + + /* Handle slashing state. */ + + // reset start height on signing infos + app.SlashingKeeper.IterateValidatorSigningInfos( + ctx, + func(addr sdk.ConsAddress, info slashingtypes.ValidatorSigningInfo) (stop bool) { + info.StartHeight = 0 + app.SlashingKeeper.SetValidatorSigningInfo(ctx, addr, info) + return false + }, + ) +}