Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
May 10, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
451950a
Redesign IBC on packet recv error/ result.Err handling
alpe Apr 21, 2023
60f3f81
Resolve conflicts
alpe Apr 24, 2023
29481db
Apply review feedback (#1376)
alpe May 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 14 additions & 17 deletions x/wasm/ibc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
142 changes: 142 additions & 0 deletions x/wasm/ibc_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
128 changes: 128 additions & 0 deletions x/wasm/ibc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still surprised ibc-go module doesn't do this for us

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 {
Expand Down Expand Up @@ -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)
}
37 changes: 32 additions & 5 deletions x/wasm/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down
Loading