From 4a9cc3e8c4670df8e55a88981c5b2bbcc419c13c Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 23 Mar 2023 14:10:55 +0100 Subject: [PATCH 1/5] Add docs on Error handling for ibc_packet_receive --- docs/ERROR_HANDLING.md | 86 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 docs/ERROR_HANDLING.md diff --git a/docs/ERROR_HANDLING.md b/docs/ERROR_HANDLING.md new file mode 100644 index 0000000000..d19324d346 --- /dev/null +++ b/docs/ERROR_HANDLING.md @@ -0,0 +1,86 @@ +# Error handling for various entry points + +In this document we discuss how different types of errors during contract +execution are handled by wasmd and the blockchain. + +## Two levels of errors + +When cosmwasm-vm executes a contract, the caller receives a nested result type: +`VmResult>` with some success response `R`. The outer +`VmResult` is created by the host environment and the inner `ContractResult` is +created inside of the contract. Most application specific error should go into +`ContractResult` errors. This is what happens when you use `?` inside of your +contract implementations. The `VmResult` +[error cases](https://github.com/CosmWasm/cosmwasm/blob/v1.2.3/packages/vm/src/errors/vm_error.rs#L11-L148) +include e.g. + +- Caching errors such as a missing Wasm file or corrupted module +- Serialization problems in the contract-host communication +- Panics from panic handler in contract +- Errors in crypto API calls +- Out of gas +- Unreachable statements in the Wasm bytecode + +## Error handling + +In wasmvm those two error types are merged into one and handled as one thing in +the caller (wasmd): + +- [Instantiate](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L144-L151) +- [Execute](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L192-L199) +- [Migtate](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L275-L282) +- [Sudo](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L318-L325) +- [Reply](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L363-L370) +- [IBCChannelOpen](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L406-L413) +- [IBCChannelConnect](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L449-L456) +- [IBCChannelClose](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L492-L499) +- [IBCPacketAck](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L576-L583) +- [IBCPacketTimeout](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L620-L627) + +However, there is one exception: + +- [IBCPacketReceive](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L535-L539) + +Instead of returning only the contents of the `Ok` case, the whole +`IBCReceiveResult` is returned. This allows the caller to handle the two layers +of errors differently. + +As pointed out by our auditors from Oak Security, this +[is inconsistent](https://github.com/CosmWasm/wasmvm/issues/398). Historically +merging the two error types was the desired behaviour. When `IBCPacketReceive` +came in, we needed the differentiation to be available in wasmd, which is why +the API is different than the others. Ideally we always return the contract +Result and let wasmd handle it. + +## Handing ibc_packet_receive errors + +For wasmd before 0.32, contract errors and VM errors were handled the same. They +got the special treatment of reverting state changes, writing an error +acknowledgement but don't let the transaction fail. + +For wasmd >= 0.32, the special treatment only applies to contract errors. VM +errors in `IBCPacketReceive` let the transaction fail just like the `Execute` +case would. This has two major implications: + +1. Application specific errors (especially those which can be triggered by + untrusted users) should create contract errors and no panics. This ensures + that error acknowledgements are written and relayer transactions don't fail. +2. Using panics allow the contract developer to make the transaction fail + without writing an acknowledgement. This can be handy e.g. for allowlisting + relayer addresses. + +The following table shows the new handling logic. + +| Entry point | Contract error | VM error | +| --------------------- | --------------------------------------------- | --------------------------------------------- | +| `instantiate` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `execute` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `migrate` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `sudo` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `reply` | ⏮️ state reverted
❔ depends on `reply_on` | ⏮️ state reverted
❔ depends on `reply_on` | +| `ibc_channel_open` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `ibc_channel_connect` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `ibc_channel_close` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `ibc_packet_receive` | ⏮️ state reverted
✅ tx succeeds | ⏮️ state reverted
❌ tx fails | +| `ibc_packet_ack` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `ibc_packet_timeout` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | From fc80b089c4583c71ffee59ad068c68d79e4954de Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 23 Mar 2023 14:25:28 +0100 Subject: [PATCH 2/5] Mention error ack --- docs/ERROR_HANDLING.md | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/docs/ERROR_HANDLING.md b/docs/ERROR_HANDLING.md index d19324d346..15104a2d5e 100644 --- a/docs/ERROR_HANDLING.md +++ b/docs/ERROR_HANDLING.md @@ -71,16 +71,16 @@ case would. This has two major implications: The following table shows the new handling logic. -| Entry point | Contract error | VM error | -| --------------------- | --------------------------------------------- | --------------------------------------------- | -| `instantiate` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | -| `execute` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | -| `migrate` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | -| `sudo` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | -| `reply` | ⏮️ state reverted
❔ depends on `reply_on` | ⏮️ state reverted
❔ depends on `reply_on` | -| `ibc_channel_open` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | -| `ibc_channel_connect` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | -| `ibc_channel_close` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | -| `ibc_packet_receive` | ⏮️ state reverted
✅ tx succeeds | ⏮️ state reverted
❌ tx fails | -| `ibc_packet_ack` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | -| `ibc_packet_timeout` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| Entry point | Contract error | VM error | +| --------------------- | -------------------------------------------------- | --------------------------------------------- | +| `instantiate` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `execute` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `migrate` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `sudo` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `reply` | ⏮️ state reverted
❔ depends on `reply_on` | ⏮️ state reverted
❔ depends on `reply_on` | +| `ibc_channel_open` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `ibc_channel_connect` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `ibc_channel_close` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `ibc_packet_receive` | ⏮️ state reverted
✅ tx succeeds with error ack | ⏮️ state reverted
❌ tx fails | +| `ibc_packet_ack` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | +| `ibc_packet_timeout` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | From 89f20e285f73f87048a0fd8bbc83abaa86d9ae5b Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Thu, 23 Mar 2023 18:36:22 +0100 Subject: [PATCH 3/5] Add section on Error acknowledgement formatting --- docs/ERROR_HANDLING.md | 45 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/docs/ERROR_HANDLING.md b/docs/ERROR_HANDLING.md index 15104a2d5e..9bc5a71883 100644 --- a/docs/ERROR_HANDLING.md +++ b/docs/ERROR_HANDLING.md @@ -84,3 +84,48 @@ The following table shows the new handling logic. | `ibc_packet_receive` | ⏮️ state reverted
✅ tx succeeds with error ack | ⏮️ state reverted
❌ tx fails | | `ibc_packet_ack` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | | `ibc_packet_timeout` | ⏮️ state reverted
❌ tx fails | ⏮️ state reverted
❌ tx fails | + +## Error acknowledgement formatting + +In case of a contract error in `ibc_packet_receive`, wasmd creates an error +acknowledgement. The format used is a JSON object with a single top level +`error` string such as `{"error":"some error text"}`. This format is the JSON +serialization of the ibc-go +[Acknowledgement](https://github.com/cosmos/ibc-go/blob/v7.0.0/proto/ibc/core/channel/v1/channel.proto#L156-L162) +type and compatible with ICS-20. + +If you are using the acknowledgement types shipped with cosmwasm-std +([#1512](https://github.com/CosmWasm/cosmwasm/issues/1512)), your protocol's +acknowledgement is compatible with that. + +If you are using a customized acknowledgement type, you need to convert contract +errors to error acks yourself in `ibc_packet_receive`. The `Never` type provides +type-safety for that. See: + +```rust +// The error type Never ensures you handle all contract errors inside the function body +pub fn ibc_packet_receive( + deps: DepsMut, + _env: Env, + msg: IbcPacketReceiveMsg, +) -> Result { + // put this in a closure so we can convert all error responses into acknowledgements + (|| { + let packet = msg.packet; + let caller = packet.dest.channel_id; + let msg: PacketMsg = from_slice(&packet.data)?; + match msg { + // Some packet receive implementations which return results + PacketMsg::Dispatch { msgs } => receive_dispatch(deps, caller, msgs), + PacketMsg::WhoAmI {} => receive_who_am_i(deps, caller), + PacketMsg::Balances {} => receive_balances(deps, caller), + } + })() + .or_else(|e| { + // Here we encode the error to our own fancy ack type + let acknowledgement: Binary = make_my_ack(e); + Ok(IbcReceiveResponse::new() + .set_ack(acknowledgement)) + }) +} +``` From 85dc17ef7e55177b91162525963b905a7c3cbf5f Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Tue, 11 Apr 2023 13:15:02 +0200 Subject: [PATCH 4/5] Specify wasmd version range for current error handling behaviour --- docs/ERROR_HANDLING.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/ERROR_HANDLING.md b/docs/ERROR_HANDLING.md index 9bc5a71883..f90494cd84 100644 --- a/docs/ERROR_HANDLING.md +++ b/docs/ERROR_HANDLING.md @@ -54,9 +54,9 @@ Result and let wasmd handle it. ## Handing ibc_packet_receive errors -For wasmd before 0.32, contract errors and VM errors were handled the same. They -got the special treatment of reverting state changes, writing an error -acknowledgement but don't let the transaction fail. +From wasmd 0.22 to 0.31 (inclusive), contract errors and VM errors were handled +the same. They got the special treatment of reverting state changes, writing an +error acknowledgement but don't let the transaction fail. For wasmd >= 0.32, the special treatment only applies to contract errors. VM errors in `IBCPacketReceive` let the transaction fail just like the `Execute` From 0508f1c7ec121dcc00c8e8f2600ba2afbf20d6f1 Mon Sep 17 00:00:00 2001 From: Simon Warta <2603011+webmaster128@users.noreply.github.com> Date: Sat, 9 Mar 2024 13:25:26 +0100 Subject: [PATCH 5/5] Update docs/ERROR_HANDLING.md Co-authored-by: Christoph Otter --- docs/ERROR_HANDLING.md | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/docs/ERROR_HANDLING.md b/docs/ERROR_HANDLING.md index f90494cd84..8aaf8dd93d 100644 --- a/docs/ERROR_HANDLING.md +++ b/docs/ERROR_HANDLING.md @@ -23,34 +23,33 @@ include e.g. ## Error handling -In wasmvm those two error types are merged into one and handled as one thing in -the caller (wasmd): - -- [Instantiate](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L144-L151) -- [Execute](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L192-L199) -- [Migtate](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L275-L282) -- [Sudo](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L318-L325) -- [Reply](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L363-L370) -- [IBCChannelOpen](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L406-L413) -- [IBCChannelConnect](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L449-L456) -- [IBCChannelClose](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L492-L499) -- [IBCPacketAck](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L576-L583) -- [IBCPacketTimeout](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L620-L627) - -However, there is one exception: - -- [IBCPacketReceive](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L535-L539) - +Before version 2.0 those two error types were merged into one in wasmvm and +handled as one thing in the caller (wasmd). See for example +[Instantiate](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L144-L151). +However, there was one exception to this: +[IBCPacketReceive](https://github.com/CosmWasm/wasmvm/blob/v1.2.0/lib.go#L535-L539). Instead of returning only the contents of the `Ok` case, the whole `IBCReceiveResult` is returned. This allows the caller to handle the two layers of errors differently. As pointed out by our auditors from Oak Security, this -[is inconsistent](https://github.com/CosmWasm/wasmvm/issues/398). Historically +[was inconsistent](https://github.com/CosmWasm/wasmvm/issues/398). Historically merging the two error types was the desired behaviour. When `IBCPacketReceive` came in, we needed the differentiation to be available in wasmd, which is why -the API is different than the others. Ideally we always return the contract -Result and let wasmd handle it. +the API was different than the others. + +In wasmvm >= 2.0 (wasmd >= 0.51), we +[always return the contract result](https://github.com/CosmWasm/wasmvm/blob/v2.0.0-rc.2/lib.go#L132) +and let wasmd handle it. Apart from making everything more consistent, this also +allows wasmd to handle contract errors differently from VM errors. + +Most errors returned by sub-messages are +[redacted](https://github.com/CosmWasm/wasmd/blob/v0.51.0-rc.1/x/wasm/keeper/msg_dispatcher.go#L205) +by wasmd before passing them back into the contract. The reason for this is the +possible non-determinism of error messages. However, as contract errors come +from the contract, they have to be deterministic. With the new separation, wasmd +now passes the full contract error message back into the calling contract, +massively improving the debugging experience. ## Handing ibc_packet_receive errors