From 03c264c912828f6cd6dda90bf3d24cabc655f572 Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Mon, 27 Mar 2023 14:48:58 +0200 Subject: [PATCH 1/4] Update OnRecvPacket method to panic when an error is returned by the VM --- x/wasm/keeper/relay.go | 2 +- x/wasm/keeper/relay_test.go | 10 ++++++++- x/wasm/relay_test.go | 41 +++++++++++++++++++++++++------------ 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/x/wasm/keeper/relay.go b/x/wasm/keeper/relay.go index 1e4e215bdb..fa1c4ac04f 100644 --- a/x/wasm/keeper/relay.go +++ b/x/wasm/keeper/relay.go @@ -130,7 +130,7 @@ 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 { - return nil, errorsmod.Wrap(types.ErrExecuteFailed, execErr.Error()) + panic(execErr) } if res.Err != "" { // handle error case as before https://github.com/CosmWasm/wasmvm/commit/c300106fe5c9426a495f8e10821e00a9330c56c6 return nil, errorsmod.Wrap(types.ErrExecuteFailed, res.Err) diff --git a/x/wasm/keeper/relay_test.go b/x/wasm/keeper/relay_test.go index a1d2819e50..9fb377a09c 100644 --- a/x/wasm/keeper/relay_test.go +++ b/x/wasm/keeper/relay_test.go @@ -174,6 +174,7 @@ func TestOnConnectChannel(t *testing.T) { Channel: myChannel, }, } + err := keepers.WasmKeeper.OnConnectChannel(ctx, spec.contractAddr, msg) // then @@ -325,6 +326,7 @@ func TestOnRecvPacket(t *testing.T) { expContractGas sdk.Gas expAck []byte expErr bool + expPanic bool expEventTypes []string }{ "consume contract gas": { @@ -349,7 +351,7 @@ func TestOnRecvPacket(t *testing.T) { Attributes: []wasmvmtypes.EventAttribute{{Key: "Foo", Value: "Bar"}}, }, contractErr: errors.New("test, ignore"), - expErr: true, + expPanic: true, }, "dispatch contract messages on success": { contractAddr: example.Contract, @@ -444,6 +446,12 @@ func TestOnRecvPacket(t *testing.T) { // when msg := wasmvmtypes.IBCPacketReceiveMsg{Packet: myPacket} + if spec.expPanic { + assert.Panics(t, func() { + keepers.WasmKeeper.OnRecvPacket(ctx, spec.contractAddr, msg) + }) + return + } gotAck, err := keepers.WasmKeeper.OnRecvPacket(ctx, spec.contractAddr, msg) // then diff --git a/x/wasm/relay_test.go b/x/wasm/relay_test.go index 5b903c6f33..1017522d4e 100644 --- a/x/wasm/relay_test.go +++ b/x/wasm/relay_test.go @@ -32,10 +32,13 @@ func TestFromIBCTransferToContract(t *testing.T) { transferAmount := sdk.NewInt(1) specs := map[string]struct { - contract wasmtesting.IBCContractCallbacks - setupContract func(t *testing.T, contract wasmtesting.IBCContractCallbacks, chain *wasmibctesting.TestChain) - expChainABalanceDiff math.Int - expChainBBalanceDiff math.Int + contract wasmtesting.IBCContractCallbacks + setupContract func(t *testing.T, contract wasmtesting.IBCContractCallbacks, chain *wasmibctesting.TestChain) + expChainAPendingSendPackets int + expChainBPendingSendPackets int + expChainABalanceDiff math.Int + expChainBBalanceDiff math.Int + expErr bool }{ "ack": { contract: &ackReceiverContract{}, @@ -44,8 +47,10 @@ func TestFromIBCTransferToContract(t *testing.T) { c.t = t c.chain = chain }, - expChainABalanceDiff: transferAmount.Neg(), - expChainBBalanceDiff: transferAmount, + expChainAPendingSendPackets: 0, + expChainBPendingSendPackets: 0, + expChainABalanceDiff: transferAmount.Neg(), + expChainBBalanceDiff: transferAmount, }, "nack": { contract: &nackReceiverContract{}, @@ -53,8 +58,10 @@ func TestFromIBCTransferToContract(t *testing.T) { c := contract.(*nackReceiverContract) c.t = t }, - expChainABalanceDiff: sdk.ZeroInt(), - expChainBBalanceDiff: sdk.ZeroInt(), + expChainAPendingSendPackets: 0, + expChainBPendingSendPackets: 0, + expChainABalanceDiff: sdk.ZeroInt(), + expChainBBalanceDiff: sdk.ZeroInt(), }, "error": { contract: &errorReceiverContract{}, @@ -62,8 +69,11 @@ func TestFromIBCTransferToContract(t *testing.T) { c := contract.(*errorReceiverContract) c.t = t }, - expChainABalanceDiff: sdk.ZeroInt(), - expChainBBalanceDiff: sdk.ZeroInt(), + expChainAPendingSendPackets: 1, + expChainBPendingSendPackets: 0, + expChainABalanceDiff: transferAmount.Neg(), + expChainBBalanceDiff: sdk.ZeroInt(), + expErr: true, }, } for name, spec := range specs { @@ -101,6 +111,7 @@ func TestFromIBCTransferToContract(t *testing.T) { // when transfer via sdk transfer from A (module) -> B (contract) coinToSendToB := sdk.NewCoin(sdk.DefaultBondDenom, transferAmount) timeoutHeight := clienttypes.NewHeight(1, 110) + msg := ibctransfertypes.NewMsgTransfer(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, coinToSendToB, chainA.SenderAccount.GetAddress().String(), chainB.SenderAccount.GetAddress().String(), timeoutHeight, 0, "") _, err := chainA.SendMsgs(msg) require.NoError(t, err) @@ -112,11 +123,15 @@ func TestFromIBCTransferToContract(t *testing.T) { // and when relay to chain B and handle Ack on chain A err = coordinator.RelayAndAckPendingPackets(path) - require.NoError(t, err) + if spec.expErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } // then - require.Equal(t, 0, len(chainA.PendingSendPackets)) - require.Equal(t, 0, len(chainB.PendingSendPackets)) + require.Equal(t, spec.expChainAPendingSendPackets, len(chainA.PendingSendPackets)) + require.Equal(t, spec.expChainBPendingSendPackets, len(chainB.PendingSendPackets)) // and source chain balance was decreased newChainABalance := chainA.Balance(chainA.SenderAccount.GetAddress(), sdk.DefaultBondDenom) From 610c677466555310b73f7cb7dd935a5adb473d6b Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Mon, 27 Mar 2023 16:54:52 +0200 Subject: [PATCH 2/4] Spike on packet recieve unhappy paths --- x/wasm/ibc.go | 44 ++++++++++++++++++++++---------- x/wasm/keeper/relay.go | 34 ++++++++++++++++++++---- x/wasm/types/events.go | 27 ++++++++++++++++++++ x/wasm/types/exported_keepers.go | 3 ++- 4 files changed, 88 insertions(+), 20 deletions(-) diff --git a/x/wasm/ibc.go b/x/wasm/ibc.go index b310e04cc9..79e4fb2e80 100644 --- a/x/wasm/ibc.go +++ b/x/wasm/ibc.go @@ -1,6 +1,7 @@ package wasm import ( + "fmt" "math" errorsmod "cosmossdk.io/errors" @@ -267,23 +268,38 @@ func (i IBCHandler) OnRecvPacket( return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(err, "contract port id")) } msg := wasmvmtypes.IBCPacketReceiveMsg{Packet: newIBCPacket(packet), Relayer: relayer.String()} - ack, err := i.keeper.OnRecvPacket(ctx, contractAddr, msg) + + em := sdk.NewEventManager() + ack, err := i.keeper.OnRecvPacket(ctx.WithEventManager(em), contractAddr, msg) if err != nil { + // the state gets reverted, we keep only wasm events that do not contain custom data + for _, e := range em.Events() { + if types.IsAcceptedEventOnRecvPacketErrorAck(e.Type) { + ctx.EventManager().EmitEvent(e) + } + } + // the `NewErrorAcknowledgement` redacts the error message, it is + // recommended by the ibc-module devs to log the raw error as event: + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypePacketRecv, + sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), + sdk.NewAttribute(types.AttributeKeyContractAddr, contractAddr.String()), + sdk.NewAttribute(types.AttributeKeyAckError, err.Error()), // contains original error message not redacted + sdk.NewAttribute(types.AttributeKeyAckSuccess, "false"), + )) 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 + // emit all contract and submessage events on success + ctx.EventManager().EmitEvents(em.Events()) + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypePacketRecv, + sdk.NewAttribute(types.AttributeKeyContractAddr, contractAddr.String()), + sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), + sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), + )) + return ack } // OnAcknowledgementPacket implements the IBCModule interface diff --git a/x/wasm/keeper/relay.go b/x/wasm/keeper/relay.go index fa1c4ac04f..c1106017e9 100644 --- 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/v7/modules/core/04-channel/types" + ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported" + errorsmod "cosmossdk.io/errors" wasmvmtypes "github.com/CosmWasm/wasmvm/types" "github.com/cosmos/cosmos-sdk/telemetry" @@ -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,34 @@ 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 contract fully abort IBC receive in certain case } - if res.Err != "" { // handle error case as before https://github.com/CosmWasm/wasmvm/commit/c300106fe5c9426a495f8e10821e00a9330c56c6 - return nil, errorsmod.Wrap(types.ErrExecuteFailed, res.Err) + if res.Err != "" { + // return error ACK with non-redacted contract message + 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 + return nil, err + } + // success ACK + 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 diff --git a/x/wasm/types/events.go b/x/wasm/types/events.go index 442c3ed369..2e27a22322 100644 --- a/x/wasm/types/events.go +++ b/x/wasm/types/events.go @@ -17,8 +17,33 @@ const ( EventTypeGovContractResult = "gov_contract_result" EventTypeUpdateContractAdmin = "update_contract_admin" EventTypeUpdateCodeAccessConfig = "update_code_access_config" + EventTypePacketRecv = "ibc_packet_received" + // add new types to IsAcceptedEventOnRecvPacketErrorAck ) +// IsAcceptedEventOnRecvPacketErrorAck returns true for all wasm event types that do not contain custom attributes +func IsAcceptedEventOnRecvPacketErrorAck(s string) bool { + for _, v := range []string{ + EventTypeStoreCode, + EventTypeInstantiate, + EventTypeExecute, + // EventTypeMigrate, not true as we rolled back + // EventTypePinCode, not relevant + // EventTypeUnpinCode, not relevant + EventTypeSudo, + EventTypeReply, + EventTypeGovContractResult, + // EventTypeUpdateContractAdmin, not true + // EventTypeUpdateCodeAccessConfig, not true + // EventTypePacketRecv, can not happen + } { + if s == v { + return true + } + } + return false +} + // event attributes returned from contract execution const ( AttributeReservedPrefix = "_" @@ -31,4 +56,6 @@ const ( AttributeKeyNewAdmin = "new_admin_address" AttributeKeyCodePermission = "code_permission" AttributeKeyAuthorizedAddresses = "authorized_addresses" + AttributeKeyAckSuccess = "success" + AttributeKeyAckError = "error" ) diff --git a/x/wasm/types/exported_keepers.go b/x/wasm/types/exported_keepers.go index dcf97cb2cc..4fa5c6ac59 100644 --- a/x/wasm/types/exported_keepers.go +++ b/x/wasm/types/exported_keepers.go @@ -4,6 +4,7 @@ import ( wasmvmtypes "github.com/CosmWasm/wasmvm/types" sdk "github.com/cosmos/cosmos-sdk/types" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported" ) // ViewKeeper provides read only operations @@ -100,7 +101,7 @@ type IBCContractKeeper interface { ctx sdk.Context, contractAddr sdk.AccAddress, msg wasmvmtypes.IBCPacketReceiveMsg, - ) ([]byte, error) + ) (ibcexported.Acknowledgement, error) OnAckPacket( ctx sdk.Context, contractAddr sdk.AccAddress, From a59ccb8e4971afec2b12be5ede32cc4f1a464ddd Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Fri, 31 Mar 2023 11:22:06 +0200 Subject: [PATCH 3/4] Updates --- x/wasm/ibc.go | 33 +++++++------------------------ x/wasm/types/events.go | 45 +++++++++++++++++++++++------------------- 2 files changed, 32 insertions(+), 46 deletions(-) diff --git a/x/wasm/ibc.go b/x/wasm/ibc.go index 79e4fb2e80..a4daec661f 100644 --- a/x/wasm/ibc.go +++ b/x/wasm/ibc.go @@ -1,7 +1,6 @@ package wasm import ( - "fmt" "math" errorsmod "cosmossdk.io/errors" @@ -265,40 +264,22 @@ func (i IBCHandler) OnRecvPacket( ) ibcexported.Acknowledgement { contractAddr, err := ContractFromPortID(packet.DestinationPort) if err != nil { - return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(err, "contract port id")) + // this must not happen as ports were registered before + panic(errorsmod.Wrapf(err, "contract port id")) } - msg := wasmvmtypes.IBCPacketReceiveMsg{Packet: newIBCPacket(packet), Relayer: relayer.String()} em := sdk.NewEventManager() + msg := wasmvmtypes.IBCPacketReceiveMsg{Packet: newIBCPacket(packet), Relayer: relayer.String()} ack, err := i.keeper.OnRecvPacket(ctx.WithEventManager(em), contractAddr, msg) if err != nil { - // the state gets reverted, we keep only wasm events that do not contain custom data - for _, e := range em.Events() { - if types.IsAcceptedEventOnRecvPacketErrorAck(e.Type) { - ctx.EventManager().EmitEvent(e) - } - } - // the `NewErrorAcknowledgement` redacts the error message, it is - // recommended by the ibc-module devs to log the raw error as event: - ctx.EventManager().EmitEvent( - sdk.NewEvent( - types.EventTypePacketRecv, - sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), - sdk.NewAttribute(types.AttributeKeyContractAddr, contractAddr.String()), - sdk.NewAttribute(types.AttributeKeyAckError, err.Error()), // contains original error message not redacted - sdk.NewAttribute(types.AttributeKeyAckSuccess, "false"), - )) + // the state gets reverted, so we drop all captured events + types.EmitAcknowledgementEvent(ctx, contractAddr, ack, err) return channeltypes.NewErrorAcknowledgement(err) } + // emit all contract and submessage events on success ctx.EventManager().EmitEvents(em.Events()) - ctx.EventManager().EmitEvent( - sdk.NewEvent( - types.EventTypePacketRecv, - sdk.NewAttribute(types.AttributeKeyContractAddr, contractAddr.String()), - sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), - sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), - )) + types.EmitAcknowledgementEvent(ctx, contractAddr, ack, err) return ack } diff --git a/x/wasm/types/events.go b/x/wasm/types/events.go index 2e27a22322..ee2048f4b4 100644 --- a/x/wasm/types/events.go +++ b/x/wasm/types/events.go @@ -1,5 +1,12 @@ package types +import ( + "fmt" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/ibc-go/v7/modules/core/exported" +) + const ( // WasmModuleEventType is stored with any contract TX that returns non empty EventAttributes WasmModuleEventType = "wasm" @@ -21,27 +28,25 @@ const ( // add new types to IsAcceptedEventOnRecvPacketErrorAck ) -// IsAcceptedEventOnRecvPacketErrorAck returns true for all wasm event types that do not contain custom attributes -func IsAcceptedEventOnRecvPacketErrorAck(s string) bool { - for _, v := range []string{ - EventTypeStoreCode, - EventTypeInstantiate, - EventTypeExecute, - // EventTypeMigrate, not true as we rolled back - // EventTypePinCode, not relevant - // EventTypeUnpinCode, not relevant - EventTypeSudo, - EventTypeReply, - EventTypeGovContractResult, - // EventTypeUpdateContractAdmin, not true - // EventTypeUpdateCodeAccessConfig, not true - // EventTypePacketRecv, can not happen - } { - if s == v { - return true - } +// EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error +// details if any. +func EmitAcknowledgementEvent(ctx sdk.Context, contractAddr sdk.AccAddress, ack exported.Acknowledgement, err error) { + attributes := []sdk.Attribute{ + sdk.NewAttribute(sdk.AttributeKeyModule, ModuleName), + sdk.NewAttribute(AttributeKeyContractAddr, contractAddr.String()), + sdk.NewAttribute(AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())), } - return false + + if err != nil { + attributes = append(attributes, sdk.NewAttribute(AttributeKeyAckError, err.Error())) + } + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + EventTypePacketRecv, + attributes..., + ), + ) } // event attributes returned from contract execution From 2df007facaaef8dcf85bac976c504a509396a92a Mon Sep 17 00:00:00 2001 From: Pino' Surace Date: Mon, 3 Apr 2023 14:41:04 +0200 Subject: [PATCH 4/4] fix tests --- x/wasm/keeper/relay_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x/wasm/keeper/relay_test.go b/x/wasm/keeper/relay_test.go index 9fb377a09c..beb9bad6c1 100644 --- a/x/wasm/keeper/relay_test.go +++ b/x/wasm/keeper/relay_test.go @@ -324,7 +324,7 @@ func TestOnRecvPacket(t *testing.T) { overwriteMessenger *wasmtesting.MockMessageHandler mockReplyFn func(codeID wasmvm.Checksum, env wasmvmtypes.Env, reply wasmvmtypes.Reply, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.Response, uint64, error) expContractGas sdk.Gas - expAck []byte + expAck ContractConfirmStateAck expErr bool expPanic bool expEventTypes []string @@ -370,7 +370,7 @@ func TestOnRecvPacket(t *testing.T) { Attributes: []wasmvmtypes.EventAttribute{{Key: "Foo", Value: "Bar"}}, }, expEventTypes: []string{types.WasmModuleEventType}, - expAck: []byte("myAck"), + expAck: ContractConfirmStateAck("myAck"), }, "emit contract events on success": { contractAddr: example.Contract, @@ -387,7 +387,7 @@ func TestOnRecvPacket(t *testing.T) { }}, }, expEventTypes: []string{types.WasmModuleEventType, "wasm-custom"}, - expAck: []byte("myAck"), + expAck: ContractConfirmStateAck("myAck"), }, "messenger errors returned, events stored": { contractAddr: example.Contract, @@ -411,7 +411,7 @@ func TestOnRecvPacket(t *testing.T) { mockReplyFn: func(codeID wasmvm.Checksum, env wasmvmtypes.Env, reply wasmvmtypes.Reply, store wasmvm.KVStore, goapi wasmvm.GoAPI, querier wasmvm.Querier, gasMeter wasmvm.GasMeter, gasLimit uint64, deserCost wasmvmtypes.UFraction) (*wasmvmtypes.Response, uint64, error) { return &wasmvmtypes.Response{Data: []byte("myBetterAck")}, 0, nil }, - expAck: []byte("myBetterAck"), + expAck: ContractConfirmStateAck("myBetterAck"), expEventTypes: []string{types.EventTypeReply}, }, "unknown contract address": {