Review error handling/rollback when recieve_packet errors (sends ack packet with error)

[ethanfrey]

This counts as a fail for the contract and we would assume state is rolled back.
But we return what looks like as a success to the ibc callback (which is correct)

Let's ensure this doesn't leave half-written state in ibc-reflect and ibc-reflect-send.

Note: relevant tracking issue in IBC is cosmos/ibc-go#91 (based on the bug analysis from cosmos/ibc-go#68)

[ethanfrey]

I have been thinking about this a bit more and followed some general discussion on this issue at cosmos/ibc-go#68

When reflecting more on this, I realized that even if a contract caches it's state internally and only commits writes on success, what if it return a message? And that message errors?

The only proper place to handle this would be in the Go app (wasmd). It should make a cached store and execute the task and all messages and all sub-messages and if any errors, then it reverts the local store cache (just inside the wasm handler, not for the ibc handler), and package this error into an "error" acknowledgement message, returning a success with error acknowledgement to the ibc handler.

The main issue is creating the error acknowledgement. There is a standardized (but not required) acknowledgement format that is used in ICS20: https://github.com/cosmos/ibc-go/blob/main/proto/ibc/core/channel/v1/channel.proto#L144-L157 (thanks to me). This is currently encoded as JSON in the ics20-1 protocol, but there was some discussion to make this protobuf in the future.

I would propose the following mapping in x/wasm (which needs to be well-documented). If a contract returns an error on OnRecvPacket, the Go keeper should revert local state changes, and check the value of the error string to determine the acknowledgement packet (for error):

• If 0x..., parse this as hex data and set the acknowledgement response binary
• If {...}, assume this is a custom JSON format and convert it directly to []byte as acknowledgement
• Else: Embed it in the Acknowledgement error field and serialize it as JSON. Basically {"error": err.String()} except with string escaping.

Open question: should we handle serializing the standard Acknowledgement in protobuf format?

Note: if a protocol requires a specific error ack format and an error may be returned by a message, it must return them as submessages, and if the submessage returns error, the contract can map that to a different custom error format (eg. 0xabcd) before returning that.

[alpe]

When reflecting more on this, I realized that even if a contract caches it's state internally and only commits writes on success, what if it return a message? And that message errors?

This is a scenario that is possible now but pretty complex to handle in wasmd. IMHO the contract is fully responsible to process or orchestrate the IBC data. We can simply remove Messages from the response and let the contract do everything with Submessages to be fully in charge and enforce the right behaviour. The ACK packet format is app level as well.

[webmaster128]

If Acknowledgement is a binary result or a string error, shouldn't we have an

pub enum Acknowledgement {
    /// The success case
    /// https://github.com/cosmos/ibc-go/blob/main/proto/ibc/core/channel/v1/channel.proto#L154
    Result(Binary),
    /// The error case
    /// https://github.com/cosmos/ibc-go/blob/main/proto/ibc/core/channel/v1/channel.proto#L155
    Error(String),
}

and use it instead of Binary in both IbcAcknowledgement::acknowledgement and IbcReceiveResponse::acknowledgement?

[ethanfrey]

That channel acknowledgement is a recommended format for acknowledgements, but not all protocols must use it. (They went overboard on ultra-flexibility in IBC as to make it hard to use). Further, it is unspecified if this is encoded as JSON or Protobuf if it is used (6 months ago they said protobuf, implementation is JSON, this may have something to do with the event system).

We could enforce that all contracts use this format and encode it with JSON, but it may then be impossible to provide compatibility with future IBC protocols someone designs. Any idea to provide a simple default, as well as a way to override it?

[ethanfrey]

This is a scenario that is possible now but pretty complex to handle in wasmd. IMHO the contract is fully responsible to process or orchestrate the IBC data. We can simply remove Messages from the response and let the contract do everything with Submessages to be fully in charge and enforce the right behaviour. The ACK packet format is app level as well.

Oh, this is MUCH more complex to handle in a contract. With submessages, I already handle the wrapping on the store and reverting if needed. Reverting state in execute is possible. But keeping that in some "to be committed" state and then only committing it after all submessages have returned success is an engineering nightmare.

I am happy to implement this in wasmd (simpler than submessages). The main point is some way to allow the contract to potentially be in full control of the acknowledgement (if they want to use a custom format), while providing an easy default (the type linked above, ics20 compatible)

[alpe]

Oh, this is MUCH more complex to handle in a contract. With submessages, I already handle the wrapping on the store and reverting if needed. Reverting state in execute is possible. But keeping that in some "to be committed" state and then only committing it after all submessages have returned success is an engineering nightmare.

Unless I understood the issue wrong it does belong to the app level and moving it to wasmd limits the flexibility that dynamic ibc gives us for custom IBC apps. Wasmd must not know anything about the data packages nor do assumptions what an error is. In my understanding the acknowledgement format is purely app specific although a recommended format exists.

Submessages do not alter state on error. So the contract has to keep track of submessages sent and their responses. It can abort and return an IBC ack error on the first issue or wait for the last submessage to succeed for the final OK ack. But also this is app dependent. Maybe there are scenarios where even a failing submessage does not cause an IBC ack error or different ack states exists....

Maybe we can have some contract framework that helps with the complexity on that level at some point.

[ethanfrey]

Look at this (common) case:

1. Contract processes incoming message, updates some internal bookkeeping.
2. Contract returns submessage to transfer some tokens, as well as the acknowledgement
3. Contracts gets an error from submessage and wants to convert it to a app-specific acknowledgement format.

This is hard because:

A. How do I revert the state changes from (1) in another call to the contract?
B. Since I returned a (success) acknowledgement in step (2), there is no clear way to change it with submessages.

My approach which says, contract returns error in (1) or (3) on failure and error string encodes the (failed) acknowledgement packet makes use of most of the plumbing we already have. We error on any sub-call that returns errors, and just a simple cache wrap and error serialization will let us customize the ack from a failed sub-call.

[ethanfrey]

I would note that the IBC team plans to support OnRecvPacket callbacks returning error to the IBC handler and manage this state reversion there (in a future IBC version). Meaning, we can be a step ahead with the contract interface using some glue code in tgrade, and then just remove most of the glue code and base on the Cosmos SDK in a future version.

I'm not sure how they plan to encode the acknowledgement message on failure, but probably some Go interface on the error.

[ethanfrey]

Please see cosmos/ibc-go#107 for the new ibc-go approach to revert on error.

From 0.43 on, this will be taken care of in the sdk. For now, we have to handle it in wasmd. But we should take a contract interface that works for the future

[ethanfrey]

I propose a different mechanism to re-write the "error acknoweldgement" (rather than some ad-hoc encoding format in an error string). I would appreciate a review of the description here #885

Once we have agreement on the approach, I will implement it and close this issue.