Redesign IBC on packet recv error/ result.Err handling (backport #1353)…

… (#1358) * Redesign IBC on packet recv error/ result.Err handling (cherry picked from commit 7cd5893) # Conflicts: # x/wasm/ibc.go # x/wasm/ibc_test.go # x/wasm/keeper/relay.go * Resolve conflicts * Apply review feedback (#1376) * Apply review feedback * Better test name * Add contract integration test * Better doc in test and minor refactoring --------- Co-authored-by: Alex Peters <[email protected]>
CosmWasm · May 10, 2023 · 70f077d · 70f077d
1 parent a972834
commit 70f077d
Show file tree

Hide file tree

Showing 8 changed files with 415 additions and 66 deletions.
diff --git a/x/wasm/ibc.go b/x/wasm/ibc.go
@@ -264,26 +264,23 @@ func (i IBCHandler) OnRecvPacket(
 ) ibcexported.Acknowledgement {
 	contractAddr, err := ContractFromPortID(packet.DestinationPort)
 	if err != nil {
-		return channeltypes.NewErrorAcknowledgement(sdkerrors.Wrapf(err, "contract port id"))
+		// this must not happen as ports were registered before
+		panic(sdkerrors.Wrapf(err, "contract port id"))
 	}
+
+	em := sdk.NewEventManager()
 	msg := wasmvmtypes.IBCPacketReceiveMsg{Packet: newIBCPacket(packet), Relayer: relayer.String()}
-	ack, err := i.keeper.OnRecvPacket(ctx, contractAddr, msg)
+	ack, err := i.keeper.OnRecvPacket(ctx.WithEventManager(em), contractAddr, msg)
 	if err != nil {
-		return channeltypes.NewErrorAcknowledgement(err)
-	}
-	return ContractConfirmStateAck(ack)
-}
-
-var _ ibcexported.Acknowledgement = ContractConfirmStateAck{}
-
-type ContractConfirmStateAck []byte
-
-func (w ContractConfirmStateAck) Success() bool {
-	return true // always commit state
-}
-
-func (w ContractConfirmStateAck) Acknowledgement() []byte {
-	return w
+		ack = channeltypes.NewErrorAcknowledgement(err)
+		// the state gets reverted, so we drop all captured events
+	} else if ack == nil || ack.Success() {
+		// emit all contract and submessage events on success
+		// nil ack is a success case, see: https://github.com/cosmos/ibc-go/blob/v7.0.0/modules/core/keeper/msg_server.go#L453
+		ctx.EventManager().EmitEvents(em.Events())
+	}
+	types.EmitAcknowledgementEvent(ctx, contractAddr, ack, err)
+	return ack
 }
 
 // OnAcknowledgementPacket implements the IBCModule interface

diff --git a/x/wasm/ibc_integration_test.go b/x/wasm/ibc_integration_test.go
@@ -3,6 +3,10 @@ package wasm_test
 import (
 	"testing"
 
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/CosmWasm/wasmd/x/wasm/types"
+
 	wasmvm "github.com/CosmWasm/wasmvm"
 	wasmvmtypes "github.com/CosmWasm/wasmvm/types"
 	ibctransfertypes "github.com/cosmos/ibc-go/v4/modules/apps/transfer/types"
@@ -124,3 +128,141 @@ func TestOnChanOpenTryVersion(t *testing.T) {
 		})
 	}
 }
+
+func TestOnIBCPacketReceive(t *testing.T) {
+	// given 2 chains with a mock on chain A to control the IBC flow
+	// and  the ibc-reflect contract on chain B
+	// when the test package is relayed
+	// then the contract executes the flow defined for the packet data
+	// and  the ibc Ack captured is what we expect
+	specs := map[string]struct {
+		packetData          []byte
+		expAck              []byte
+		expPacketNotHandled bool
+	}{
+		"all good": {
+			packetData: []byte(`{"who_am_i":{}}`),
+			expAck:     []byte(`{"ok":{"account":"cosmos1suhgf5svhu4usrurvxzlgn54ksxmn8gljarjtxqnapv8kjnp4nrs2zhgh2"}}`),
+		},
+		"with result err": {
+			packetData: []byte(`{"return_err": {"text": "my error"}}`),
+			expAck:     []byte(`{"error":"invalid packet: Generic error: my error"}`),
+		},
+		"with returned msg fails": {
+			packetData: []byte(`{"return_msgs": {"msgs": [{"bank":{"send":{"to_address": "invalid-address", "amount": [{"denom": "ALX", "amount": "1"}]}}}]}}`),
+			expAck:     []byte(`{"error":"ABCI code: 7: error handling packet: see events for details"}`),
+		},
+		"with contract panic": {
+			packetData:          []byte(`{"panic":{}}`),
+			expPacketNotHandled: true,
+		},
+	}
+	for name, spec := range specs {
+		t.Run(name, func(t *testing.T) {
+			mockContractEngine := NewCaptureAckTestContractEngine()
+			chainAOpts := []wasmkeeper.Option{
+				wasmkeeper.WithWasmEngine(mockContractEngine),
+			}
+			var (
+				coord  = wasmibctesting.NewCoordinator(t, 2, chainAOpts)
+				chainA = coord.GetChain(wasmibctesting.GetChainID(0))
+				chainB = coord.GetChain(wasmibctesting.GetChainID(1))
+			)
+			// setup chain A contract metadata for mock
+			myMockContractAddr := chainA.SeedNewContractInstance() // setups env but uses mock contract
+
+			// setup chain B contracts
+			reflectID := chainB.StoreCodeFile("./keeper/testdata/reflect.wasm").CodeID
+			initMsg := wasmkeeper.IBCReflectInitMsg{ReflectCodeID: reflectID}.GetBytes(t)
+			codeID := chainB.StoreCodeFile("./keeper/testdata/ibc_reflect.wasm").CodeID
+			ibcReflectContractAddr := chainB.InstantiateContract(codeID, initMsg)
+
+			// establish IBC channels
+			var (
+				sourcePortID      = chainA.ContractInfo(myMockContractAddr).IBCPortID
+				counterpartPortID = chainB.ContractInfo(ibcReflectContractAddr).IBCPortID
+				path              = wasmibctesting.NewPath(chainA, chainB)
+			)
+			path.EndpointA.ChannelConfig = &ibctesting.ChannelConfig{
+				PortID: sourcePortID, Version: "ibc-reflect-v1", Order: channeltypes.ORDERED,
+			}
+			path.EndpointB.ChannelConfig = &ibctesting.ChannelConfig{
+				PortID: counterpartPortID, Version: "ibc-reflect-v1", Order: channeltypes.ORDERED,
+			}
+
+			coord.SetupConnections(path)
+			coord.CreateChannels(path)
+			coord.CommitBlock(chainA, chainB)
+			require.Equal(t, 0, len(chainA.PendingSendPackets))
+			require.Equal(t, 0, len(chainB.PendingSendPackets))
+
+			// when an ibc packet is sent from chain A to chain B
+			capturedAck := mockContractEngine.SubmitIBCPacket(t, path, chainA, myMockContractAddr, spec.packetData)
+			coord.CommitBlock(chainA, chainB)
+
+			require.Equal(t, 1, len(chainA.PendingSendPackets))
+			require.Equal(t, 0, len(chainB.PendingSendPackets))
+
+			err := coord.RelayAndAckPendingPackets(path)
+
+			// then
+			if spec.expPacketNotHandled {
+				const contractPanicToErrMsg = `recovered: Error calling the VM: Error executing Wasm: Wasmer runtime error: RuntimeError: Aborted: panicked at 'This page intentionally faulted', src/contract.rs:316:5`
+				assert.ErrorContains(t, err, contractPanicToErrMsg)
+				require.Nil(t, *capturedAck)
+				return
+			}
+			require.NoError(t, err)
+			if spec.expAck != nil {
+				assert.Equal(t, spec.expAck, *capturedAck, string(*capturedAck))
+			} else {
+				require.Nil(t, *capturedAck)
+			}
+		})
+	}
+}
+
+// mock to submit an ibc data package from given chain and capture the ack
+type captureAckTestContractEngine struct {
+	*wasmtesting.MockWasmer
+}
+
+// NewCaptureAckTestContractEngine constructor
+func NewCaptureAckTestContractEngine() *captureAckTestContractEngine {
+	m := wasmtesting.NewIBCContractMockWasmer(&wasmtesting.MockIBCContractCallbacks{
+		IBCChannelOpenFn: func(codeID wasmvm.Checksum, env wasmvmtypes.Env, msg wasmvmtypes.IBCChannelOpenMsg, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.IBC3ChannelOpenResponse, uint64, error) {
+			return &wasmvmtypes.IBC3ChannelOpenResponse{}, 0, nil
+		},
+		IBCChannelConnectFn: func(codeID wasmvm.Checksum, env wasmvmtypes.Env, msg wasmvmtypes.IBCChannelConnectMsg, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.IBCBasicResponse, uint64, error) {
+			return &wasmvmtypes.IBCBasicResponse{}, 0, nil
+		},
+	})
+	return &captureAckTestContractEngine{m}
+}
+
+// SubmitIBCPacket starts an IBC packet transfer on given chain and captures the ack returned
+func (x *captureAckTestContractEngine) SubmitIBCPacket(t *testing.T, path *wasmibctesting.Path, chainA *wasmibctesting.TestChain, senderContractAddr sdk.AccAddress, packetData []byte) *[]byte {
+	// prepare a bridge to send an ibc packet by an ordinary wasm execute message
+	x.MockWasmer.ExecuteFn = func(codeID wasmvm.Checksum, env wasmvmtypes.Env, info wasmvmtypes.MessageInfo, executeMsg []byte, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.Response, uint64, error) {
+		return &wasmvmtypes.Response{
+			Messages: []wasmvmtypes.SubMsg{{ID: 1, ReplyOn: wasmvmtypes.ReplyNever, Msg: wasmvmtypes.CosmosMsg{IBC: &wasmvmtypes.IBCMsg{SendPacket: &wasmvmtypes.SendPacketMsg{
+				ChannelID: path.EndpointA.ChannelID, Data: executeMsg, Timeout: wasmvmtypes.IBCTimeout{Block: &wasmvmtypes.IBCTimeoutBlock{Revision: 1, Height: 10000000}},
+			}}}}},
+		}, 0, nil
+	}
+	// capture acknowledgement
+	var gotAck []byte
+	x.MockWasmer.IBCPacketAckFn = func(codeID wasmvm.Checksum, env wasmvmtypes.Env, msg wasmvmtypes.IBCPacketAckMsg, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.IBCBasicResponse, uint64, error) {
+		gotAck = msg.Acknowledgement.Data
+		return &wasmvmtypes.IBCBasicResponse{}, 0, nil
+	}
+
+	// start the process
+	_, err := chainA.SendMsgs(&types.MsgExecuteContract{
+		Sender:   chainA.SenderAccount.GetAddress().String(),
+		Contract: senderContractAddr.String(),
+		Msg:      packetData,
+	})
+	require.NoError(t, err)
+	return &gotAck
+}
diff --git a/x/wasm/ibc_test.go b/x/wasm/ibc_test.go
@@ -4,11 +4,125 @@ import (
 	"testing"
 
 	wasmvmtypes "github.com/CosmWasm/wasmvm/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/types/address"
 	clienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types"
+	ibcexported "github.com/cosmos/ibc-go/v4/modules/core/exported"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	abci "github.com/tendermint/tendermint/abci/types"
+	"github.com/tendermint/tendermint/libs/rand"
+
+	"github.com/CosmWasm/wasmd/x/wasm/keeper"
+	"github.com/CosmWasm/wasmd/x/wasm/types"
 )
 
+func TestOnRecvPacket(t *testing.T) {
+	anyRelayerAddr := sdk.AccAddress(rand.Bytes(address.Len))
+	anyContractIBCPkg := IBCPacketFixture(func(p *channeltypes.Packet) {
+		p.DestinationPort = "wasm.cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f"
+	})
+	myCustomEvent := sdk.NewEvent("testing")
+	specs := map[string]struct {
+		ibcPkg               channeltypes.Packet
+		contractRsp          ibcexported.Acknowledgement
+		contractOkMsgExecErr error
+		expEvents            sdk.Events
+		expPanic             bool
+		expAck               ibcexported.Acknowledgement
+	}{
+		"contract returns success response": {
+			ibcPkg:      anyContractIBCPkg,
+			contractRsp: keeper.ContractConfirmStateAck([]byte{1}),
+			expAck:      keeper.ContractConfirmStateAck([]byte{1}),
+			expEvents: sdk.Events{
+				myCustomEvent,
+				{
+					Type: "ibc_packet_received",
+					Attributes: []abci.EventAttribute{
+						{Key: []byte("module"), Value: []byte("wasm")},
+						{Key: []byte("_contract_address"), Value: []byte("cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f")},
+						{Key: []byte("success"), Value: []byte("true")},
+					},
+				},
+			},
+		},
+		"contract returns err response": {
+			ibcPkg:      anyContractIBCPkg,
+			contractRsp: channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")),
+			expAck:      channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")),
+			expEvents: sdk.Events{
+				{
+					Type: "ibc_packet_received",
+					Attributes: []abci.EventAttribute{
+						{Key: []byte("module"), Value: []byte("wasm")},
+						{Key: []byte("_contract_address"), Value: []byte("cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f")},
+						{Key: []byte("success"), Value: []byte("false")},
+					},
+				},
+			},
+		},
+		"nil considered success response": { // regression only
+			ibcPkg: anyContractIBCPkg,
+			expEvents: sdk.Events{
+				myCustomEvent,
+				{
+					Type: "ibc_packet_received",
+					Attributes: []abci.EventAttribute{
+						{Key: []byte("module"), Value: []byte("wasm")},
+						{Key: []byte("_contract_address"), Value: []byte("cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f")},
+						{Key: []byte("success"), Value: []byte("true")},
+					},
+				},
+			},
+		},
+		"unknown contract port": {
+			ibcPkg: IBCPacketFixture(func(p *channeltypes.Packet) {
+				p.DestinationPort = "not-a-contract-port"
+			}),
+			expPanic: true,
+		},
+		"returned messages executed with error": {
+			ibcPkg:               anyContractIBCPkg,
+			contractOkMsgExecErr: types.ErrInvalid.Wrap("testing"),
+			expAck:               channeltypes.NewErrorAcknowledgement(types.ErrInvalid.Wrap("testing")),
+			expEvents: sdk.Events{{
+				Type: "ibc_packet_received",
+				Attributes: []abci.EventAttribute{
+					{Key: []byte("module"), Value: []byte("wasm")},
+					{Key: []byte("_contract_address"), Value: []byte("cosmos1w09vr7rpe2agu0kg2zlpkdckce865l3zps8mxjurxthfh3m7035qe5hh7f")},
+					{Key: []byte("success"), Value: []byte("false")},
+					{Key: []byte("error"), Value: []byte("testing: invalid")}, // not redacted
+				},
+			}},
+		},
+	}
+	for name, spec := range specs {
+		t.Run(name, func(t *testing.T) {
+			mock := IBCContractKeeperMock{
+				OnRecvPacketFn: func(ctx sdk.Context, contractAddr sdk.AccAddress, msg wasmvmtypes.IBCPacketReceiveMsg) (ibcexported.Acknowledgement, error) {
+					// additional custom event to confirm event handling on state commit/ rollback
+					ctx.EventManager().EmitEvent(myCustomEvent)
+					return spec.contractRsp, spec.contractOkMsgExecErr
+				},
+			}
+			h := NewIBCHandler(mock, nil, nil)
+			em := &sdk.EventManager{}
+			ctx := sdk.Context{}.WithEventManager(em)
+			if spec.expPanic {
+				require.Panics(t, func() {
+					_ = h.OnRecvPacket(ctx, spec.ibcPkg, anyRelayerAddr)
+				})
+				return
+			}
+			gotAck := h.OnRecvPacket(ctx, spec.ibcPkg, anyRelayerAddr)
+			assert.Equal(t, spec.expAck, gotAck)
+			assert.Equal(t, spec.expEvents, em.Events())
+		})
+	}
+}
+
 func TestMapToWasmVMIBCPacket(t *testing.T) {
 	var myTimestamp uint64 = 1
 	specs := map[string]struct {
@@ -80,3 +194,17 @@ func IBCPacketFixture(mutators ...func(p *channeltypes.Packet)) channeltypes.Pac
 	}
 	return r
 }
+
+var _ types.IBCContractKeeper = &IBCContractKeeperMock{}
+
+type IBCContractKeeperMock struct {
+	types.IBCContractKeeper
+	OnRecvPacketFn func(ctx sdk.Context, contractAddr sdk.AccAddress, msg wasmvmtypes.IBCPacketReceiveMsg) (ibcexported.Acknowledgement, error)
+}
+
+func (m IBCContractKeeperMock) OnRecvPacket(ctx sdk.Context, contractAddr sdk.AccAddress, msg wasmvmtypes.IBCPacketReceiveMsg) (ibcexported.Acknowledgement, error) {
+	if m.OnRecvPacketFn == nil {
+		panic("not expected to be called")
+	}
+	return m.OnRecvPacketFn(ctx, contractAddr, msg)
+}
diff --git a/x/wasm/keeper/relay.go b/x/wasm/keeper/relay.go
@@ -3,6 +3,9 @@ package keeper
 import (
 	"time"
 
+	channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types"
+	ibcexported "github.com/cosmos/ibc-go/v4/modules/core/exported"
+
 	wasmvmtypes "github.com/CosmWasm/wasmvm/types"
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -116,7 +119,7 @@ func (k Keeper) OnRecvPacket(
 	ctx sdk.Context,
 	contractAddr sdk.AccAddress,
 	msg wasmvmtypes.IBCPacketReceiveMsg,
-) ([]byte, error) {
+) (ibcexported.Acknowledgement, error) {
 	defer telemetry.MeasureSince(time.Now(), "wasm", "contract", "ibc-recv-packet")
 	contractInfo, codeInfo, prefixStore, err := k.contractInstance(ctx, contractAddr)
 	if err != nil {
@@ -130,13 +133,37 @@ func (k Keeper) OnRecvPacket(
 	res, gasUsed, execErr := k.wasmVM.IBCPacketReceive(codeInfo.CodeHash, env, msg, prefixStore, cosmwasmAPI, querier, ctx.GasMeter(), gas, costJSONDeserialization)
 	k.consumeRuntimeGas(ctx, gasUsed)
 	if execErr != nil {
-		panic(execErr)
+		panic(execErr) // let the contract fully abort an IBC packet receive.
+		// Throwing a panic here instead of an error ack will revert
+		// all state downstream and not persist any data in ibc-go.
+		// This can be triggered by throwing a panic in the contract
 	}
-	if res.Err != "" { // handle error case as before https://github.com/CosmWasm/wasmvm/commit/c300106fe5c9426a495f8e10821e00a9330c56c6
-		return nil, sdkerrors.Wrap(types.ErrExecuteFailed, res.Err)
+	if res.Err != "" {
+		// return error ACK with non-redacted contract message, state will be reverted
+		return channeltypes.Acknowledgement{
+			Response: &channeltypes.Acknowledgement_Error{Error: res.Err},
+		}, nil
 	}
 	// note submessage reply results can overwrite the `Acknowledgement` data
-	return k.handleContractResponse(ctx, contractAddr, contractInfo.IBCPortID, res.Ok.Messages, res.Ok.Attributes, res.Ok.Acknowledgement, res.Ok.Events)
+	data, err := k.handleContractResponse(ctx, contractAddr, contractInfo.IBCPortID, res.Ok.Messages, res.Ok.Attributes, res.Ok.Acknowledgement, res.Ok.Events)
+	if err != nil {
+		// submessage errors result in error ACK with state reverted. Error message is redacted
+		return nil, err
+	}
+	// success ACK, state will be committed
+	return ContractConfirmStateAck(data), nil
+}
+
+var _ ibcexported.Acknowledgement = ContractConfirmStateAck{}
+
+type ContractConfirmStateAck []byte
+
+func (w ContractConfirmStateAck) Success() bool {
+	return true // always commit state
+}
+
+func (w ContractConfirmStateAck) Acknowledgement() []byte {
+	return w
 }
 
 // OnAckPacket calls the contract to handle the "acknowledgement" data which can contain success or failure of a packet