pallet-evm: return Ok(()) when EVM execution fails (#6493)

* pallet-evm: return Ok(()) when EVM execution fails

* Bump spec version

* Init test module

* Add fail_call_return_ok test

* Fix tests and use full match pattern

Co-authored-by: Gav Wood <gavin@parity.io>

substrate/frame/evm/src/lib.rs

@@ -21,6 +21,7 @@
 #![cfg_attr(not(feature = "std"), no_std)]
 
 mod backend;
+mod tests;
 
 pub use crate::backend::{Account, Log, Vicinity, Backend};
 
@@ -144,7 +145,7 @@ pub trait Trait: frame_system::Trait + pallet_timestamp::Trait {
 	/// Precompiles associated with this EVM engine.
 	type Precompiles: Precompiles;
 	/// Chain ID of EVM.
-	type ChainId: Get<U256>;
+	type ChainId: Get<u64>;
 
 	/// EVM config used in the module.
 	fn config() -> &'static Config {
@@ -201,6 +202,12 @@ decl_event! {
 		Log(Log),
 		/// A contract has been created at given address.
 		Created(H160),
+		/// A contract was attempted to be created, but the execution failed.
+		CreatedFailed(H160),
+		/// A contract has been executed successfully with states applied.
+		Executed(H160),
+		/// A contract has been executed with errors. States are reverted with only gas fees applied.
+		ExecutedFailed(H160),
 		/// A deposit has been made at a given address.
 		BalanceDeposit(AccountId, H160, U256),
 		/// A withdrawal has been made from a given address.
@@ -220,12 +227,6 @@ decl_error! {
 		WithdrawFailed,
 		/// Gas price is too low.
 		GasPriceTooLow,
-		/// Call failed
-		ExitReasonFailed,
-		/// Call reverted
-		ExitReasonRevert,
-		/// Call returned VM fatal error
-		ExitReasonFatal,
 		/// Nonce is invalid
 		InvalidNonce,
 	}
@@ -300,7 +301,7 @@ decl_module! {
 			let sender = ensure_signed(origin)?;
 			let source = T::ConvertAccountId::convert_account_id(&sender);
 
-			Self::execute_call(
+			match Self::execute_call(
 				source,
 				target,
 				input,
@@ -308,7 +309,16 @@ decl_module! {
 				gas_limit,
 				gas_price,
 				nonce,
-			).map_err(Into::into)
+			)? {
+				ExitReason::Succeed(_) => {
+					Module::<T>::deposit_event(Event::<T>::Executed(target));
+				},
+				ExitReason::Error(_) | ExitReason::Revert(_) | ExitReason::Fatal(_) => {
+					Module::<T>::deposit_event(Event::<T>::ExecutedFailed(target));
+				},
+			}
+
+			Ok(())
 		}
 
 		/// Issue an EVM create operation. This is similar to a contract creation transaction in
@@ -327,16 +337,22 @@ decl_module! {
 			let sender = ensure_signed(origin)?;
 			let source = T::ConvertAccountId::convert_account_id(&sender);
 
-			let create_address = Self::execute_create(
+			match Self::execute_create(
 				source,
 				init,
 				value,
 				gas_limit,
 				gas_price,
 				nonce
-			)?;
+			)? {
+				(create_address, ExitReason::Succeed(_)) => {
+					Module::<T>::deposit_event(Event::<T>::Created(create_address));
+				},
+				(create_address, _) => {
+					Module::<T>::deposit_event(Event::<T>::CreatedFailed(create_address));
+				},
+			}
 
-			Module::<T>::deposit_event(Event::<T>::Created(create_address));
 			Ok(())
 		}
 
@@ -356,7 +372,7 @@ decl_module! {
 			let sender = ensure_signed(origin)?;
 			let source = T::ConvertAccountId::convert_account_id(&sender);
 
-			let create_address = Self::execute_create2(
+			match Self::execute_create2(
 				source,
 				init,
 				salt,
@@ -364,9 +380,15 @@ decl_module! {
 				gas_limit,
 				gas_price,
 				nonce
-			)?;
+			)? {
+				(create_address, ExitReason::Succeed(_)) => {
+					Module::<T>::deposit_event(Event::<T>::Created(create_address));
+				},
+				(create_address, _) => {
+					Module::<T>::deposit_event(Event::<T>::CreatedFailed(create_address));
+				},
+			}
 
-			Module::<T>::deposit_event(Event::<T>::Created(create_address));
 			Ok(())
 		}
 	}
@@ -413,7 +435,7 @@ impl<T: Trait> Module<T> {
 		gas_limit: u32,
 		gas_price: U256,
 		nonce: Option<U256>
-	) -> Result<H160, Error<T>> {
+	) -> Result<(H160, ExitReason), Error<T>> {
 		Self::execute_evm(
 			source,
 			value,
@@ -442,7 +464,7 @@ impl<T: Trait> Module<T> {
 		gas_limit: u32,
 		gas_price: U256,
 		nonce: Option<U256>
-	) -> Result<H160, Error<T>> {
+	) -> Result<(H160, ExitReason), Error<T>> {
 		let code_hash = H256::from_slice(Keccak256::digest(&init).as_slice());
 		Self::execute_evm(
 			source,
@@ -473,8 +495,8 @@ impl<T: Trait> Module<T> {
 		gas_limit: u32,
 		gas_price: U256,
 		nonce: Option<U256>,
-	) -> Result<(), Error<T>> {
-		Self::execute_evm(
+	) -> Result<ExitReason, Error<T>> {
+		Ok(Self::execute_evm(
 			source,
 			value,
 			gas_limit,
@@ -487,7 +509,7 @@ impl<T: Trait> Module<T> {
 				input,
 				gas_limit as usize,
 			)),
-		)
+		)?.1)
 	}
 
 	/// Execute an EVM operation.
@@ -498,7 +520,7 @@ impl<T: Trait> Module<T> {
 		gas_price: U256,
 		nonce: Option<U256>,
 		f: F,
-	) -> Result<R, Error<T>> where
+	) -> Result<(R, ExitReason), Error<T>> where
 		F: FnOnce(&mut StackExecutor<Backend<T>>) -> (R, ExitReason),
 	{
 		let vicinity = Vicinity {
@@ -527,19 +549,12 @@ impl<T: Trait> Module<T> {
 
 		let (retv, reason) = f(&mut executor);
 
-		let ret = match reason {
-			ExitReason::Succeed(_) => Ok(retv),
-			ExitReason::Error(_) => Err(Error::<T>::ExitReasonFailed),
-			ExitReason::Revert(_) => Err(Error::<T>::ExitReasonRevert),
-			ExitReason::Fatal(_) => Err(Error::<T>::ExitReasonFatal),
-		};
-
 		let actual_fee = executor.fee(gas_price);
 		executor.deposit(source, total_fee.saturating_sub(actual_fee));
 
 		let (values, logs) = executor.deconstruct();
 		backend.apply(values, logs, true);
 
-		ret
+		Ok((retv, reason))
 	}
 }

substrate/frame/evm/src/tests.rs

+#![cfg(test)]
+
+use super::*;
+
+use std::{str::FromStr, collections::BTreeMap};
+use frame_support::{
+	assert_ok, impl_outer_origin, parameter_types, impl_outer_dispatch,
+};
+use sp_core::H256;
+// The testing primitives are very useful for avoiding having to work with signatures
+// or public keys. `u64` is used as the `AccountId` and no `Signature`s are required.
+use sp_runtime::{
+	Perbill,
+	testing::Header,
+	traits::{BlakeTwo256, IdentityLookup},
+};
+
+impl_outer_origin! {
+	pub enum Origin for Test  where system = frame_system {}
+}
+
+impl_outer_dispatch! {
+	pub enum OuterCall for Test where origin: Origin {
+		self::EVM,
+	}
+}
+
+// For testing the pallet, we construct most of a mock runtime. This means
+// first constructing a configuration type (`Test`) which `impl`s each of the
+// configuration traits of pallets we want to use.
+#[derive(Clone, Eq, PartialEq)]
+pub struct Test;
+parameter_types! {
+	pub const BlockHashCount: u64 = 250;
+	pub const MaximumBlockWeight: Weight = 1024;
+	pub const MaximumBlockLength: u32 = 2 * 1024;
+	pub const AvailableBlockRatio: Perbill = Perbill::one();
+}
+impl frame_system::Trait for Test {
+	type BaseCallFilter = ();
+	type Origin = Origin;
+	type Index = u64;
+	type BlockNumber = u64;
+	type Hash = H256;
+	type Call = OuterCall;
+	type Hashing = BlakeTwo256;
+	type AccountId = H256;
+	type Lookup = IdentityLookup<Self::AccountId>;
+	type Header = Header;
+	type Event = ();
+	type BlockHashCount = BlockHashCount;
+	type MaximumBlockWeight = MaximumBlockWeight;
+	type DbWeight = ();
+	type BlockExecutionWeight = ();
+	type ExtrinsicBaseWeight = ();
+	type MaximumExtrinsicWeight = MaximumBlockWeight;
+	type MaximumBlockLength = MaximumBlockLength;
+	type AvailableBlockRatio = AvailableBlockRatio;
+	type Version = ();
+	type ModuleToIndex = ();
+	type AccountData = pallet_balances::AccountData<u64>;
+	type OnNewAccount = ();
+	type OnKilledAccount = ();
+}
+
+parameter_types! {
+	pub const ExistentialDeposit: u64 = 1;
+}
+impl pallet_balances::Trait for Test {
+	type Balance = u64;
+	type DustRemoval = ();
+	type Event = ();
+	type ExistentialDeposit = ExistentialDeposit;
+	type AccountStore = System;
+}
+
+parameter_types! {
+	pub const MinimumPeriod: u64 = 1000;
+}
+impl pallet_timestamp::Trait for Test {
+	type Moment = u64;
+	type OnTimestampSet = ();
+	type MinimumPeriod = MinimumPeriod;
+}
+
+/// Fixed gas price of `0`.
+pub struct FixedGasPrice;
+impl FeeCalculator for FixedGasPrice {
+	fn min_gas_price() -> U256 {
+		// Gas price is always one token per gas.
+		0.into()
+	}
+}
+parameter_types! {
+	pub const EVMModuleId: ModuleId = ModuleId(*b"py/evmpa");
+}
+impl Trait for Test {
+	type ChainId = SystemChainId;
+	type ModuleId = EVMModuleId;
+	type FeeCalculator = FixedGasPrice;
+	type ConvertAccountId = HashTruncateConvertAccountId<BlakeTwo256>;
+	type Currency = Balances;
+	type Event = Event<Test>;
+	type Precompiles = ();
+}
+
+type System = frame_system::Module<Test>;
+type Balances = pallet_balances::Module<Test>;
+type EVM = Module<Test>;
+
+// This function basically just builds a genesis storage key/value store according to
+// our desired mockup.
+pub fn new_test_ext() -> sp_io::TestExternalities {
+	let mut t = frame_system::GenesisConfig::default().build_storage::<Test>().unwrap();
+
+	let mut accounts = BTreeMap::new();
+	accounts.insert(
+		H160::from_str("1000000000000000000000000000000000000001").unwrap(),
+		GenesisAccount {
+			nonce: U256::from(1),
+			balance: U256::from(1000000),
+			storage: Default::default(),
+			code: vec![
+				0x00, // STOP
+			],
+		}
+	);
+	accounts.insert(
+		H160::from_str("1000000000000000000000000000000000000002").unwrap(),
+		GenesisAccount {
+			nonce: U256::from(1),
+			balance: U256::from(1000000),
+			storage: Default::default(),
+			code: vec![
+				0xff, // INVALID
+			],
+		}
+	);
+
+	// We use default for brevity, but you can configure as desired if needed.
+	pallet_balances::GenesisConfig::<Test>::default().assimilate_storage(&mut t).unwrap();
+	GenesisConfig { accounts }.assimilate_storage(&mut t).unwrap();
+	t.into()
+}
+
+#[test]
+fn fail_call_return_ok() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(EVM::call(
+			Origin::signed(H256::default()),
+			H160::from_str("1000000000000000000000000000000000000001").unwrap(),
+			Vec::new(),
+			U256::default(),
+			1000000,
+			U256::default(),
+			None,
+		));
+
+		assert_ok!(EVM::call(
+			Origin::signed(H256::default()),
+			H160::from_str("1000000000000000000000000000000000000002").unwrap(),
+			Vec::new(),
+			U256::default(),
+			1000000,
+			U256::default(),
+			None,
+		));
+	});
+}