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

Spike error ack #1299

Closed
wants to merge 6 commits into from
Closed

Spike error ack #1299

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
03c264c
Update OnRecvPacket method to panic when an error is returned
pinosu Mar 27, 2023
610c677
Spike on packet recieve unhappy paths
alpe Mar 27, 2023
fcd0040
Merge branch 'main' into spike_error_ack
alpe Mar 31, 2023
a59ccb8
Updates
alpe Mar 31, 2023
2df007f
fix tests
pinosu Apr 3, 2023
dbc3483
Merge pull request #1313 from CosmWasm/spike_error_ack_updates
alpe Apr 18, 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
44 changes: 30 additions & 14 deletions x/wasm/ibc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package wasm

import (
"fmt"
"math"

errorsmod "cosmossdk.io/errors"
Expand Down Expand Up @@ -267,23 +268,38 @@ func (i IBCHandler) OnRecvPacket(
return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(err, "contract port id"))
Copy link
Member

Choose a reason for hiding this comment

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

Is this something where we can provide a better error message than the redacted error too?

Copy link
Contributor Author

@alpe alpe Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good 👁️ ! Unless we touch our contract-to-port mapping we should never run into this case. Either we panic here or we need to return the error event, too.
I would prefer panic to make clear in our code that this must not happen. This is not ideal as the packet may be re-submitted until timeout but it is not supposed to happen

}
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)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to preserve those events? I might be messing something but to me it's clearer if we discard them and add all relevent information in the error event below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a more complex contract-calls-contracts environment, the event trace may have revealed some infos to track the root cause of an error. But there are some costs of maintaining this in the future.
👌 let's drop them and keep things simple.

// 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(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we always emit an event on rcv or on only in the error case?

Copy link
Member

Choose a reason for hiding this comment

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

What about 2 events:

  1. EventTypePacketRecv: we receive a packet
  2. EventTypePacketRecvContractErr: we got a contract error which is turned into an error ack

While EventTypePacketRecvContractErr is an error, the absence of it does not mean success.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was also inspired by ics-20 and ics-27 . They use a single event with additional error attribute when set.

types.EventTypePacketRecv,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fyi: the event name is "ibc-acknowledgement-error" on Osmosis

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())),
Copy link
Member

Choose a reason for hiding this comment

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

We don't know if this us a success or not. It's just bytes from contract that can mean anything.

Copy link
Contributor Author

@alpe alpe Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should be sdk.NewAttribute(types.AttributeKeyAckSuccess, "true"), instead

This attribute was inspired by the ics-20 impl. At this stage we do know that the ibc packet was successfully handled by the contract (and any sub-messages succeeded). The state will be committed and the Ack data stored.

))
return ack
}

// OnAcknowledgementPacket implements the IBCModule interface
Expand Down
34 changes: 29 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/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"
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,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 {
return nil, errorsmod.Wrap(types.ErrExecuteFailed, execErr.Error())
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
Comment on lines +150 to +151
Copy link
Member

Choose a reason for hiding this comment

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

I think this code or comment is misleading because it can be either a success or and error or a non-binary acknowledgement type. We should not try to give it any meaning.

Copy link
Contributor Author

@alpe alpe Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On packet receive, success can be defined as case 1 + 2 of #697 (comment) . The contract was able to fully handle the packet. State will be committed.

}

var _ ibcexported.Acknowledgement = ContractConfirmStateAck{}

type ContractConfirmStateAck []byte

func (w ContractConfirmStateAck) Success() bool {
return true // always commit state
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there any documentation what Success() in the ibcexported.Acknowledgement means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best source for Success() that I found is the code that calls this in ibc-go.

Copy link
Member

Choose a reason for hiding this comment

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

I raised the issue here: cosmos/ibc-go#3434


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
10 changes: 9 additions & 1 deletion x/wasm/keeper/relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ func TestOnConnectChannel(t *testing.T) {
Channel: myChannel,
},
}

err := keepers.WasmKeeper.OnConnectChannel(ctx, spec.contractAddr, msg)

// then
Expand Down Expand Up @@ -325,6 +326,7 @@ func TestOnRecvPacket(t *testing.T) {
expContractGas sdk.Gas
expAck []byte
expErr bool
expPanic bool
expEventTypes []string
}{
"consume contract gas": {
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down
41 changes: 28 additions & 13 deletions x/wasm/relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand All @@ -44,26 +47,33 @@ 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{},
setupContract: func(t *testing.T, contract wasmtesting.IBCContractCallbacks, chain *wasmibctesting.TestChain) {
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{},
setupContract: func(t *testing.T, contract wasmtesting.IBCContractCallbacks, chain *wasmibctesting.TestChain) {
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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
27 changes: 27 additions & 0 deletions x/wasm/types/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "_"
Expand All @@ -31,4 +56,6 @@ const (
AttributeKeyNewAdmin = "new_admin_address"
AttributeKeyCodePermission = "code_permission"
AttributeKeyAuthorizedAddresses = "authorized_addresses"
AttributeKeyAckSuccess = "success"
AttributeKeyAckError = "error"
)
3 changes: 2 additions & 1 deletion x/wasm/types/exported_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down