From aa0971747a09bf1010d8834bd48d1771eb68b7d6 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Tue, 5 Sep 2023 00:37:25 +0300 Subject: [PATCH 1/3] fix: fixed callbacks out of gas handling --- modules/apps/callbacks/ibc_middleware.go | 8 ++++++-- modules/apps/callbacks/ibc_middleware_test.go | 13 +++++++++++-- modules/apps/callbacks/types/errors.go | 1 + 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go index 24d17a99370..60f7ea95f9d 100644 --- a/modules/apps/callbacks/ibc_middleware.go +++ b/modules/apps/callbacks/ibc_middleware.go @@ -3,6 +3,7 @@ package ibccallbacks import ( "fmt" + errorsmod "cosmossdk.io/errors" storetypes "cosmossdk.io/store/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -284,8 +285,11 @@ func (IBCMiddleware) processCallback( } // if the callback ran out of gas and the relayer has not reserved enough gas, then revert the state - if cachedCtx.GasMeter().IsPastLimit() && callbackData.AllowRetry() { - panic(storetypes.ErrorOutOfGas{Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", callbackType, callbackData.CommitGasLimit)}) + if cachedCtx.GasMeter().IsPastLimit() { + if callbackData.AllowRetry() { + panic(storetypes.ErrorOutOfGas{Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", callbackType, callbackData.CommitGasLimit)}) + } + err = errorsmod.Wrapf(types.ErrCallbackOutOfGas, "ibc %s callback out of gas", callbackType) } // allow the transaction to be committed, continuing the packet lifecycle diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index 8577aa4f6d2..ce72cba0d7e 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -137,7 +137,7 @@ func (s *CallbacksTestSuite) TestSendPacket() { ibcmock.MockApplicationCallbackError, // execution failure on SendPacket should prevent packet sends }, { - "failure: callback execution reach out of gas, but sufficient gas provided by relayer", + "failure: callback execution reach out of gas panic, but sufficient gas provided", func() { packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, simapp.OogPanicContract) }, @@ -145,6 +145,15 @@ func (s *CallbacksTestSuite) TestSendPacket() { true, storetypes.ErrorOutOfGas{Descriptor: fmt.Sprintf("mock %s callback oog panic", types.CallbackTypeSendPacket)}, }, + { + "failure: callback execution reach out of gas error, but sufficient gas provided", + func() { + packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, simapp.OogErrorContract) + }, + types.CallbackTypeSendPacket, + false, + errorsmod.Wrapf(types.ErrCallbackOutOfGas, "ibc %s callback out of gas", types.CallbackTypeSendPacket), + }, } for _, tc := range testCases { @@ -834,7 +843,7 @@ func (s *CallbacksTestSuite) TestProcessCallback() { } }, false, - nil, + errorsmod.Wrapf(types.ErrCallbackOutOfGas, "ibc %s callback out of gas", callbackType), }, { "failure: callbackExecutor error", diff --git a/modules/apps/callbacks/types/errors.go b/modules/apps/callbacks/types/errors.go index b1b37209625..b1d997decc1 100644 --- a/modules/apps/callbacks/types/errors.go +++ b/modules/apps/callbacks/types/errors.go @@ -9,4 +9,5 @@ var ( ErrNotPacketDataProvider = errorsmod.Register(ModuleName, 3, "packet is not a PacketDataProvider") ErrCallbackKeyNotFound = errorsmod.Register(ModuleName, 4, "callback key not found in packet data") ErrCallbackAddressNotFound = errorsmod.Register(ModuleName, 5, "callback address not found in packet data") + ErrCallbackOutOfGas = errorsmod.Register(ModuleName, 6, "callback out of gas") ) From c1fafe3967d22bb4b53cd913d06a5aeb8ff2a346 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Tue, 5 Sep 2023 10:16:45 +0300 Subject: [PATCH 2/3] fix: fixed panics and oog errors not showing up on events --- modules/apps/callbacks/ibc_middleware.go | 1 + modules/apps/callbacks/ibc_middleware_test.go | 2 +- modules/apps/callbacks/types/errors.go | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go index 60f7ea95f9d..1675691984d 100644 --- a/modules/apps/callbacks/ibc_middleware.go +++ b/modules/apps/callbacks/ibc_middleware.go @@ -282,6 +282,7 @@ func (IBCMiddleware) processCallback( if callbackType == types.CallbackTypeSendPacket { panic(r) } + err = errorsmod.Wrapf(types.ErrCallbackPanic, "ibc %s callback panicked with: %v", callbackType, r) } // if the callback ran out of gas and the relayer has not reserved enough gas, then revert the state diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index ce72cba0d7e..c6f2ff090f9 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/modules/apps/callbacks/ibc_middleware_test.go @@ -830,7 +830,7 @@ func (s *CallbacksTestSuite) TestProcessCallback() { } }, false, - nil, + errorsmod.Wrapf(types.ErrCallbackPanic, "ibc %s callback panicked with: %v", callbackType, "callbackExecutor panic"), }, { "success: callbackExecutor oog panic, but retry is not allowed", diff --git a/modules/apps/callbacks/types/errors.go b/modules/apps/callbacks/types/errors.go index b1d997decc1..df2d2ef2938 100644 --- a/modules/apps/callbacks/types/errors.go +++ b/modules/apps/callbacks/types/errors.go @@ -10,4 +10,5 @@ var ( ErrCallbackKeyNotFound = errorsmod.Register(ModuleName, 4, "callback key not found in packet data") ErrCallbackAddressNotFound = errorsmod.Register(ModuleName, 5, "callback address not found in packet data") ErrCallbackOutOfGas = errorsmod.Register(ModuleName, 6, "callback out of gas") + ErrCallbackPanic = errorsmod.Register(ModuleName, 7, "callback panic") ) From ee74616d9ec52c29c75ac592888d8dda5f6ffaa3 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Tue, 5 Sep 2023 14:22:46 +0300 Subject: [PATCH 3/3] docs(callbacks): added godocs for error precedence --- modules/apps/callbacks/ibc_middleware.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/modules/apps/callbacks/ibc_middleware.go b/modules/apps/callbacks/ibc_middleware.go index 1675691984d..4751883125d 100644 --- a/modules/apps/callbacks/ibc_middleware.go +++ b/modules/apps/callbacks/ibc_middleware.go @@ -262,6 +262,11 @@ func (im IBCMiddleware) WriteAcknowledgement( // processCallback executes the callbackExecutor and reverts contract changes if the callbackExecutor fails. // +// Error Precedence and Returns: +// - oogErr: Takes the highest precedence. If the callback runs out of gas, an error wrapped with types.ErrCallbackOutOfGas is returned. +// - panicErr: Takes the second-highest precedence. If a panic occurs and it is not propagated, an error wrapped with types.ErrCallbackPanic is returned. +// - callbackErr: If the callbackExecutor returns an error, it is returned as-is. +// // panics if // - the contractExecutor panics for any reason, and the callbackType is SendPacket, or // - the contractExecutor runs out of gas and the relayer has not reserved gas grater than or equal to