callbacks: Error on RecvPacket callback execution (#8014)

* change behavior of recvPacket

* fix tests

* error on malformed callback data

* CHANGELOG

---------

Co-authored-by: Gjermund Garaba <gjermund@garaba.net>

CHANGELOG.md

@@ -93,6 +93,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### State Machine Breaking
 
 * (light-clients/06-solomachine) [\#6313](https://github.com/cosmos/ibc-go/pull/6313) Fix: No-op to avoid panicking on `UpdateState` for invalid misbehaviour submissions.
+* (apps/callbacks) [\#8014](https://github.com/cosmos/ibc-go/pull/8014) Callbacks will now return an error acknowledgement if the recvPacket callback fails. This reverts all app callback changes whereas before we only reverted the callback changes. We also error on all callbacks if the callback data is set but malformed whereas before we ignored the error and continued processing.
 
 ### Improvements
 

docs/architecture/adr-008-app-caller-cbs.md

@@ -5,6 +5,7 @@
 - 2022-08-10: Initial Draft
 - 2023-03-22: Merged
 - 2023-09-13: Updated with decisions made in implementation
+- 2025-02-24: RecvPacket callback error now returns error acknowledgement
 
 ## Status
 
@@ -245,10 +246,12 @@ type ContractKeeper interface {
 		version string,
 	) error
 	// IBCReceivePacketCallback is called in the destination chain when a packet acknowledgement is written.
-	// The contract is expected to handle the callback within the user defined gas limit, and handle any errors,
-	// out of gas, or panics gracefully.
-	// This entry point is called with a cached context. If an error is returned, then the changes in
-	// this context will not be persisted, but the packet lifecycle will not be blocked.
+	// The contract is expected to handle the callback within the user defined gas limit.
+	// This entry point is called with a cached context. If an error is returned, then the error
+	// will be written as an error acknowledgement. This will cause the context changes made by the contract
+	// to be reverted along with any state changes made by the underlying application.
+	// The error acknowledgement will then be relayed to the sending application which can perform
+	// its error acknowledgement logic (e.g. refunding tokens back to user)
 	//
 	// The version provided is the base application version for the given packet send. This allows
 	// contracts to determine how to unmarshal the packetData.
@@ -397,6 +400,9 @@ func (im IBCMiddleware) OnRecvPacket(
 		ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
 		types.CallbackTypeReceivePacket, callbackData, err,
 	)
+	if err != nil {
+		return channeltypes.NewErrorAcknowledgement(err)
+	}
 
 	return ack
 }

modules/apps/callbacks/callbacks_test.go

@@ -229,6 +229,7 @@ func GetExpectedEvent(
 ) (abci.Event, bool) {
 	var (
 		callbackData types.CallbackData
+		isCbPacket   bool
 		err          error
 	)
 
@@ -238,12 +239,12 @@ func GetExpectedEvent(
 
 	if callbackType == types.CallbackTypeReceivePacket {
 		packet := channeltypes.NewPacket(data, seq, "", "", eventPortID, eventChannelID, clienttypes.ZeroHeight(), 0)
-		callbackData, err = types.GetDestCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas)
+		callbackData, isCbPacket, err = types.GetDestCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas)
 	} else {
 		packet := channeltypes.NewPacket(data, seq, eventPortID, eventChannelID, "", "", clienttypes.ZeroHeight(), 0)
-		callbackData, err = types.GetSourceCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas)
+		callbackData, isCbPacket, err = types.GetSourceCallbackData(ctx, packetDataUnmarshaler, packet, maxCallbackGas)
 	}
-	if err != nil {
+	if !isCbPacket || err != nil {
 		return abci.Event{}, false
 	}
 

modules/apps/callbacks/ibc_middleware.go

@@ -100,11 +100,16 @@ func (im IBCMiddleware) SendPacket(
 	// packet is created without destination information present, GetSourceCallbackData does not use these.
 	packet := channeltypes.NewPacket(data, seq, sourcePort, sourceChannel, "", "", timeoutHeight, timeoutTimestamp)
 
-	callbackData, err := types.GetSourceCallbackData(ctx, im.app, packet, im.maxCallbackGas)
+	callbackData, isCbPacket, err := types.GetSourceCallbackData(ctx, im.app, packet, im.maxCallbackGas)
 	// SendPacket is not blocked if the packet does not opt-in to callbacks
-	if err != nil {
+	if !isCbPacket {
 		return seq, nil
 	}
+	// if the packet does opt-in to callbacks but the callback data is malformed,
+	// then the packet send is rejected.
+	if err != nil {
+		return 0, err
+	}
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
 		return im.contractKeeper.IBCSendPacketCallback(
@@ -139,13 +144,19 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 		return err
 	}
 
-	callbackData, err := types.GetSourceCallbackData(
+	callbackData, isCbPacket, err := types.GetSourceCallbackData(
 		ctx, im.app, packet, im.maxCallbackGas,
 	)
 	// OnAcknowledgementPacket is not blocked if the packet does not opt-in to callbacks
-	if err != nil {
+	if !isCbPacket {
 		return nil
 	}
+	// if the packet does opt-in to callbacks but the callback data is malformed,
+	// then the packet acknowledgement is rejected.
+	// This should never occur, since this error is already checked on `SendPacket`
+	if err != nil {
+		return err
+	}
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
 		return im.contractKeeper.IBCOnAcknowledgementPacketCallback(
@@ -173,13 +184,19 @@ func (im IBCMiddleware) OnTimeoutPacket(ctx sdk.Context, channelVersion string,
 		return err
 	}
 
-	callbackData, err := types.GetSourceCallbackData(
+	callbackData, isCbPacket, err := types.GetSourceCallbackData(
 		ctx, im.app, packet, im.maxCallbackGas,
 	)
 	// OnTimeoutPacket is not blocked if the packet does not opt-in to callbacks
-	if err != nil {
+	if !isCbPacket {
 		return nil
 	}
+	// if the packet does opt-in to callbacks but the callback data is malformed,
+	// then the packet timeout is rejected.
+	// This should never occur, since this error is already checked on `SendPacket`
+	if err != nil {
+		return err
+	}
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
 		return im.contractKeeper.IBCOnTimeoutPacketCallback(cachedCtx, packet, relayer, callbackData.CallbackAddress, callbackData.SenderAddress, callbackData.ApplicationVersion)
@@ -209,24 +226,35 @@ func (im IBCMiddleware) OnRecvPacket(ctx sdk.Context, channelVersion string, pac
 		return ack
 	}
 
-	callbackData, err := types.GetDestCallbackData(
+	callbackData, isCbPacket, err := types.GetDestCallbackData(
 		ctx, im.app, packet, im.maxCallbackGas,
 	)
 	// OnRecvPacket is not blocked if the packet does not opt-in to callbacks
-	if err != nil {
+	if !isCbPacket {
 		return ack
 	}
+	// if the packet does opt-in to callbacks but the callback data is malformed,
+	// then the packet receive is rejected.
+	if err != nil {
+		return channeltypes.NewErrorAcknowledgement(err)
+	}
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
 		return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, ack, callbackData.CallbackAddress, callbackData.ApplicationVersion)
 	}
 
-	// callback execution errors are not allowed to block the packet lifecycle, they are only used in event emissions
+	// callback execution errors in RecvPacket are allowed to write an error acknowledgement
+	// in this case, the receive logic of the underlying app is reverted
+	// and the error acknowledgement is processed on the sending chain
+	// Thus the sending application MUST be capable of processing the standard channel acknowledgement
 	err = internal.ProcessCallback(ctx, types.CallbackTypeReceivePacket, callbackData, callbackExecutor)
 	types.EmitCallbackEvent(
 		ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
 		types.CallbackTypeReceivePacket, callbackData, err,
 	)
+	if err != nil {
+		return channeltypes.NewErrorAcknowledgement(err)
+	}
 
 	return ack
 }
@@ -251,13 +279,17 @@ func (im IBCMiddleware) WriteAcknowledgement(
 		panic(fmt.Errorf("expected type %T, got %T", &channeltypes.Packet{}, packet))
 	}
 
-	callbackData, err := types.GetDestCallbackData(
+	callbackData, isCbPacket, err := types.GetDestCallbackData(
 		ctx, im.app, chanPacket, im.maxCallbackGas,
 	)
 	// WriteAcknowledgement is not blocked if the packet does not opt-in to callbacks
-	if err != nil {
+	if !isCbPacket {
 		return nil
 	}
+	// This should never occur, since this error is already checked on `OnRecvPacket`
+	if err != nil {
+		return err
+	}
 
 	callbackExecutor := func(cachedCtx sdk.Context) error {
 		return im.contractKeeper.IBCReceivePacketCallback(cachedCtx, packet, ack, callbackData.CallbackAddress, callbackData.ApplicationVersion)

modules/apps/callbacks/ibc_middleware_test.go

@@ -110,14 +110,24 @@ func (s *CallbacksTestSuite) TestSendPacket() {
 			nil,
 		},
 		{
-			"success: no-op on callback data is not valid",
+			"success: callback data doesn't exist",
+			func() {
+				//nolint:goconst
+				packetData.Memo = ""
+			},
+			"none", // nonexistent callback data should result in no callback execution
+			false,
+			nil,
+		},
+		{
+			"failure: callback data is not valid",
 			func() {
 				//nolint:goconst
 				packetData.Memo = `{"src_callback": {"address": ""}}`
 			},
 			"none", // improperly formatted callback data should result in no callback execution
 			false,
-			nil,
+			types.ErrCallbackAddressNotFound,
 		},
 		{
 			"failure: ics4Wrapper SendPacket call fails",
@@ -244,6 +254,16 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
 			callbackSuccess,
 			nil,
 		},
+		{
+			"success: callback data doesn't exist",
+			func() {
+				//nolint:goconst
+				packetData.Memo = ""
+				packet.Data = packetData.GetBytes()
+			},
+			noExecution,
+			nil,
+		},
 		{
 			"failure: underlying app OnAcknowledgePacket fails",
 			func() {
@@ -253,14 +273,14 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
 			ibcerrors.ErrUnknownRequest,
 		},
 		{
-			"success: no-op on callback data is not valid",
+			"failure: no-op on callback data is not valid",
 			func() {
 				//nolint:goconst
 				packetData.Memo = `{"src_callback": {"address": ""}}`
 				packet.Data = packetData.GetBytes()
 			},
 			noExecution,
-			nil,
+			types.ErrCallbackAddressNotFound,
 		},
 		{
 			"failure: callback execution reach out of gas, but sufficient gas provided by relayer",
@@ -405,6 +425,16 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
 			callbackSuccess,
 			nil,
 		},
+		{
+			"success: callback data doesn't exist",
+			func() {
+				//nolint:goconst
+				packetData.Memo = ""
+				packet.Data = packetData.GetBytes()
+			},
+			noExecution,
+			nil,
+		},
 		{
 			"failure: underlying app OnTimeoutPacket fails",
 			func() {
@@ -414,14 +444,14 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
 			ibcerrors.ErrInvalidType,
 		},
 		{
-			"success: no-op on callback data is not valid",
+			"failure: no-op on callback data is not valid",
 			func() {
 				//nolint:goconst
 				packetData.Memo = `{"src_callback": {"address": ""}}`
 				packet.Data = packetData.GetBytes()
 			},
 			noExecution,
-			nil,
+			types.ErrCallbackAddressNotFound,
 		},
 		{
 			"failure: callback execution reach out of gas, but sufficient gas provided by relayer",
@@ -571,6 +601,16 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
 			callbackSuccess,
 			successAck,
 		},
+		{
+			"success: callback data doesn't exist",
+			func() {
+				//nolint:goconst
+				packetData.Memo = ""
+				packet.Data = packetData.GetBytes()
+			},
+			noExecution,
+			successAck,
+		},
 		{
 			"failure: underlying app OnRecvPacket fails",
 			func() {
@@ -580,14 +620,14 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
 			channeltypes.NewErrorAcknowledgement(ibcerrors.ErrInvalidType),
 		},
 		{
-			"success: no-op on callback data is not valid",
+			"failure: callback data is not valid",
 			func() {
 				//nolint:goconst
 				packetData.Memo = `{"dest_callback": {"address": ""}}`
 				packet.Data = packetData.GetBytes()
 			},
 			noExecution,
-			successAck,
+			channeltypes.NewErrorAcknowledgement(types.ErrCallbackAddressNotFound),
 		},
 		{
 			"failure: callback execution reach out of gas, but sufficient gas provided by relayer",
@@ -730,13 +770,23 @@ func (s *CallbacksTestSuite) TestWriteAcknowledgement() {
 			nil,
 		},
 		{
-			"success: no-op on callback data is not valid",
+			"success: callback data doesn't exist",
+			func() {
+				//nolint:goconst
+				packetData.Memo = ""
+				packet.Data = packetData.GetBytes()
+			},
+			"none", // nonexistent callback data should result in no callback execution
+			nil,
+		},
+		{
+			"failure: callback data is not valid",
 			func() {
 				packetData.Memo = `{"dest_callback": {"address": ""}}`
 				packet.Data = packetData.GetBytes()
 			},
 			"none", // improperly formatted callback data should result in no callback execution
-			nil,
+			types.ErrCallbackAddressNotFound,
 		},
 		{
 			"failure: ics4Wrapper WriteAcknowledgement call fails",

modules/apps/callbacks/replay_test.go

@@ -226,7 +226,7 @@ func (s *CallbacksTestSuite) TestTransferSuccessAcknowledgementReplayProtection(
 			initialBalance := GetSimApp(s.chainA).BankKeeper.GetBalance(s.chainA.GetContext(), s.chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom)
 
 			// amountTransferred := ibctesting.TestCoin
-			s.ExecuteTransfer(tc.transferMemo)
+			s.ExecuteTransfer(tc.transferMemo, true)
 
 			// check that the callback is executed 1 times
 			s.Require().Equal(1, callbackCount)
@@ -290,7 +290,7 @@ func (s *CallbacksTestSuite) TestTransferRecvPacketReplayProtection() {
 			initialBalance := GetSimApp(s.chainB).BankKeeper.GetBalance(s.chainB.GetContext(), s.chainB.SenderAccount.GetAddress(), denom.IBCDenom())
 
 			// execute the transfer
-			s.ExecuteTransfer(tc.transferMemo)
+			s.ExecuteTransfer(tc.transferMemo, true)
 
 			// check that the callback is executed 1 times
 			s.Require().Equal(1, callbackCount)