XVM error handling (#1011)

* XVM error & revert handling.

* Add integration tests.

* Reentrance integration test checks 'ReentranceDenied' error.

* Update EVM caller contract below ED failing test.

* XVM precompile uses 'EvmDataWriter' to build output on revert

* Fix XVM precompile tests.

* fmt

* Add docstring.

* Integration tests: contract addr variables renaming.

Cargo.toml

@@ -70,6 +70,7 @@ array-bytes = "6.0.0"
 smallvec = "1.9.0"
 async-trait = "0.1.59"
 clap = { version = "4.2.5", features = ["derive"] }
+env_logger = "0.10.0"
 futures = { version = "0.3.26" }
 serde = { version = "1.0.151", features = ["derive"] }
 serde_json = "1.0.92"

chain-extensions/types/xvm/src/lib.rs

@@ -18,7 +18,10 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use astar_primitives::{xvm::CallError, Balance};
+use astar_primitives::{
+    xvm::{FailureError, FailureReason, FailureRevert},
+    Balance,
+};
 use parity_scale_codec::{Decode, Encode};
 use sp_std::vec::Vec;
 
@@ -31,18 +34,20 @@ pub enum XvmExecutionResult {
     Err(u32),
 }
 
-impl From<CallError> for XvmExecutionResult {
-    fn from(input: CallError) -> Self {
-        use CallError::*;
-
+impl From<FailureReason> for XvmExecutionResult {
+    fn from(input: FailureReason) -> Self {
         // `0` is reserved for `Ok`
         let error_code = match input {
-            InvalidVmId => 1,
-            SameVmCallDenied => 2,
-            InvalidTarget => 3,
-            InputTooLarge => 4,
-            ReentranceDenied => 5,
-            ExecutionFailed(_) => 6,
+            // Revert failure: 1 - 127
+            FailureReason::Revert(FailureRevert::InvalidTarget) => 1,
+            FailureReason::Revert(FailureRevert::InputTooLarge) => 2,
+            FailureReason::Revert(FailureRevert::VmRevert(_)) => 3,
+
+            // Error failure: 128 - 255
+            FailureReason::Error(FailureError::InvalidVmId) => 128,
+            FailureReason::Error(FailureError::SameVmCallDenied) => 129,
+            FailureReason::Error(FailureError::ReentranceDenied) => 130,
+            FailureReason::Error(FailureError::VmError(_)) => 131,
         };
         Self::Err(error_code)
     }

chain-extensions/xvm/src/lib.rs

@@ -18,9 +18,14 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
+extern crate alloc;
+use alloc::format;
+
 use astar_primitives::xvm::{Context, VmId, XvmCall};
 use frame_support::dispatch::Encode;
-use pallet_contracts::chain_extension::{ChainExtension, Environment, Ext, InitState, RetVal};
+use pallet_contracts::chain_extension::{
+    ChainExtension, Environment, Ext, InitState, RetVal, ReturnFlags,
+};
 use sp_runtime::DispatchError;
 use sp_std::marker::PhantomData;
 use xvm_chain_extension_types::{XvmCallArgs, XvmExecutionResult};
@@ -92,9 +97,10 @@ where
                     match TryInto::<VmId>::try_into(vm_id) {
                         Ok(id) => id,
                         Err(err) => {
-                            // TODO: Propagate error
-                            let result = Into::<XvmExecutionResult>::into(err);
-                            return Ok(RetVal::Converging(result.into()));
+                            return Ok(RetVal::Diverging {
+                                flags: ReturnFlags::REVERT,
+                                data: format!("{:?}", err).into(),
+                            });
                         }
                     }
                 };
@@ -124,9 +130,12 @@ where
                             "err: {:?}", err
                         );
 
-                        // TODO Propagate error
-                        let result = Into::<XvmExecutionResult>::into(err.error);
-                        Ok(RetVal::Converging(result.into()))
+                        // `Diverging` is used instead of `Err` to make sure the control
+                        // doesn't return to the caller.
+                        Ok(RetVal::Diverging {
+                            flags: ReturnFlags::REVERT,
+                            data: format!("{:?}", err).into(),
+                        })
                     }
                 }
             }

pallets/xvm/Cargo.toml

@@ -24,16 +24,17 @@ sp-std = { workspace = true }
 frame-benchmarking = { workspace = true, optional = true }
 
 # EVM
+fp-evm = { workspace = true }
 pallet-evm = { workspace = true }
 
 # Substrate WASM VM support
 pallet-contracts = { workspace = true }
+pallet-contracts-primitives = { workspace = true }
 
 # Astar
 astar-primitives = { workspace = true }
 
 [dev-dependencies]
-fp-evm = { workspace = true }
 hex = { workspace = true }
 pallet-balances = { workspace = true, features = ["std"] }
 pallet-insecure-randomness-collective-flip = { workspace = true, features = ["std"] }
@@ -46,9 +47,11 @@ std = [
 	"environmental/std",
 	"log/std",
 	"parity-scale-codec/std",
+	"fp-evm/std",
 	"frame-support/std",
 	"frame-system/std",
 	"pallet-contracts/std",
+	"pallet-contracts-primitives/std",
 	"pallet-evm/std",
 	"pallet-insecure-randomness-collective-flip/std",
 	"scale-info/std",

pallets/xvm/src/lib.rs

@@ -37,8 +37,13 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
+extern crate alloc;
+use alloc::format;
+
+use fp_evm::ExitReason;
 use frame_support::{ensure, traits::Currency, weights::Weight};
 use pallet_contracts::{CollectEvents, DebugInfo, Determinism};
+use pallet_contracts_primitives::ReturnFlags;
 use pallet_evm::GasWeightMapping;
 use parity_scale_codec::Decode;
 use sp_core::{H160, U256};
@@ -49,7 +54,10 @@ use astar_primitives::{
     ethereum_checked::{
         AccountMapping, CheckedEthereumTransact, CheckedEthereumTx, EthereumTxInput,
     },
-    xvm::{CallError, CallErrorWithWeight, CallInfo, CallResult, Context, VmId, XvmCall},
+    xvm::{
+        CallFailure, CallOutput, CallResult, Context, FailureError::*, FailureRevert::*, VmId,
+        XvmCall,
+    },
     Balance,
 };
 
@@ -129,18 +137,12 @@ where
 
         ensure!(
             context.source_vm_id != vm_id,
-            CallErrorWithWeight {
-                error: CallError::SameVmCallDenied,
-                used_weight: overheads,
-            }
+            CallFailure::error(SameVmCallDenied, overheads)
         );
 
         // Set `IN_XVM` to true & check reentrance.
         if IN_XVM.with(|in_xvm| in_xvm.replace(true)) {
-            return Err(CallErrorWithWeight {
-                error: CallError::ReentranceDenied,
-                used_weight: overheads,
-            });
+            return Err(CallFailure::error(ReentranceDenied, overheads));
         }
 
         let res = match vm_id {
@@ -188,20 +190,12 @@ where
 
         ensure!(
             target.len() == H160::len_bytes(),
-            CallErrorWithWeight {
-                error: CallError::InvalidTarget,
-                used_weight: overheads,
-            }
+            CallFailure::revert(InvalidTarget, overheads)
         );
-        let target_decoded =
-            Decode::decode(&mut target.as_ref()).map_err(|_| CallErrorWithWeight {
-                error: CallError::InvalidTarget,
-                used_weight: overheads,
-            })?;
-        let bounded_input = EthereumTxInput::try_from(input).map_err(|_| CallErrorWithWeight {
-            error: CallError::InputTooLarge,
-            used_weight: overheads,
-        })?;
+        let target_decoded = Decode::decode(&mut target.as_ref())
+            .map_err(|_| CallFailure::revert(InvalidTarget, overheads))?;
+        let bounded_input = EthereumTxInput::try_from(input)
+            .map_err(|_| CallFailure::revert(InputTooLarge, overheads))?;
 
         let value_u256 = U256::from(value);
         // With overheads, less weight is available.
@@ -220,10 +214,7 @@ where
         // Note the skip execution check should be exactly before `T::EthereumTransact::xvm_transact`
         // to benchmark the correct overheads.
         if skip_execution {
-            return Ok(CallInfo {
-                output: vec![],
-                used_weight: overheads,
-            });
+            return Ok(CallOutput::new(vec![], overheads));
         }
 
         let transact_result = T::EthereumTransact::xvm_transact(source, tx);
@@ -232,28 +223,41 @@ where
             "EVM call result: {:?}", transact_result,
         );
 
-        transact_result
-            .map(|(post_dispatch_info, call_info)| {
+        match transact_result {
+            Ok((post_dispatch_info, call_info)) => {
                 let used_weight = post_dispatch_info
                     .actual_weight
                     .unwrap_or_default()
                     .saturating_add(overheads);
-                CallInfo {
-                    output: call_info.value,
-                    used_weight,
+                match call_info.exit_reason {
+                    ExitReason::Succeed(_) => Ok(CallOutput::new(call_info.value, used_weight)),
+                    ExitReason::Revert(_) => {
+                        // On revert, the `call_info.value` is the encoded error data. Refer to Contract
+                        // ABI specification for details. https://docs.soliditylang.org/en/latest/abi-spec.html#errors
+                        Err(CallFailure::revert(VmRevert(call_info.value), used_weight))
+                    }
+                    ExitReason::Error(err) => Err(CallFailure::error(
+                        VmError(format!("EVM call error: {:?}", err).into()),
+                        used_weight,
+                    )),
+                    ExitReason::Fatal(err) => Err(CallFailure::error(
+                        VmError(format!("EVM call error: {:?}", err).into()),
+                        used_weight,
+                    )),
                 }
-            })
-            .map_err(|e| {
+            }
+            Err(e) => {
                 let used_weight = e
                     .post_info
                     .actual_weight
                     .unwrap_or_default()
                     .saturating_add(overheads);
-                CallErrorWithWeight {
-                    error: CallError::ExecutionFailed(Into::<&str>::into(e.error).into()),
+                Err(CallFailure::error(
+                    VmError(format!("EVM call error: {:?}", e.error).into()),
                     used_weight,
-                }
-            })
+                ))
+            }
+        }
     }
 
     fn wasm_call(
@@ -272,10 +276,7 @@ where
         );
 
         let dest = {
-            let error = CallErrorWithWeight {
-                error: CallError::InvalidTarget,
-                used_weight: overheads,
-            };
+            let error = CallFailure::revert(InvalidTarget, overheads);
             let decoded = Decode::decode(&mut target.as_ref()).map_err(|_| error.clone())?;
             T::Lookup::lookup(decoded).map_err(|_| error)
         }?;
@@ -286,10 +287,7 @@ where
         // Note the skip execution check should be exactly before `pallet_contracts::bare_call`
         // to benchmark the correct overheads.
         if skip_execution {
-            return Ok(CallInfo {
-                output: vec![],
-                used_weight: overheads,
-            });
+            return Ok(CallOutput::new(vec![], overheads));
         }
 
         let call_result = pallet_contracts::Pallet::<T>::bare_call(
@@ -307,14 +305,17 @@ where
 
         let used_weight = call_result.gas_consumed.saturating_add(overheads);
         match call_result.result {
-            Ok(success) => Ok(CallInfo {
-                output: success.data,
-                used_weight,
-            }),
-            Err(error) => Err(CallErrorWithWeight {
-                error: CallError::ExecutionFailed(Into::<&str>::into(error).into()),
+            Ok(val) => {
+                if val.flags.contains(ReturnFlags::REVERT) {
+                    Err(CallFailure::revert(VmRevert(val.data), used_weight))
+                } else {
+                    Ok(CallOutput::new(val.data, used_weight))
+                }
+            }
+            Err(error) => Err(CallFailure::error(
+                VmError(format!("WASM call error: {:?}", error).into()),
                 used_weight,
-            }),
+            )),
         }
     }