[pallet-revive] revm refactor

Cargo.toml

@@ -1172,6 +1172,7 @@ procfs = { version = "0.16.0" }
 prometheus = { version = "0.13.0", default-features = false }
 prometheus-endpoint = { path = "substrate/utils/prometheus", default-features = false, package = "substrate-prometheus-endpoint" }
 prometheus-parse = { version = "0.2.2" }
+proptest = { version = "1" }
 prost = { version = "0.12.4" }
 prost-build = { version = "0.13.2" }
 pyroscope = { version = "0.5.8" }

substrate/frame/revive/Cargo.toml

@@ -73,6 +73,7 @@ pallet-proxy = { workspace = true, default-features = true }
 pallet-revive-fixtures = { workspace = true, default-features = true }
 pallet-timestamp = { workspace = true, default-features = true }
 pallet-utility = { workspace = true, default-features = true }
+proptest = { workspace = true }
 sp-keystore = { workspace = true, default-features = true }
 sp-tracing = { workspace = true, default-features = true }
 

substrate/frame/revive/src/benchmarking.rs

@@ -29,7 +29,7 @@ use crate::{
 	storage::WriteOutcome,
 	vm::{
 		evm,
-		evm::{instructions::instruction_table, EVMInterpreter},
+		evm::{instructions::host, Interpreter},
 		pvm,
 	},
 	Pallet as Contracts, *,
@@ -47,13 +47,7 @@ use frame_support::{
 };
 use frame_system::RawOrigin;
 use pallet_revive_uapi::{pack_hi_lo, CallFlags, ReturnErrorCode, StorageFlags};
-use revm::{
-	bytecode::{opcode::EXTCODECOPY, Bytecode},
-	interpreter::{
-		host::DummyHost, interpreter_types::MemoryTr, InstructionContext, Interpreter, SharedMemory,
-	},
-	primitives,
-};
+use revm::bytecode::Bytecode;
 use sp_consensus_aura::AURA_ENGINE_ID;
 use sp_consensus_babe::{
 	digests::{PreDigest, PrimaryPreDigest},
@@ -2307,7 +2301,7 @@ mod benchmarks {
 	#[benchmark(pov_mode = Measured)]
 	fn evm_opcode(r: Linear<0, 10_000>) -> Result<(), BenchmarkError> {
 		let module = VmBinaryModule::evm_noop(r);
-		let inputs = evm::EVMInputs::new(vec![]);
+		let inputs = vec![];
 
 		let code = Bytecode::new_raw(revm::primitives::Bytes::from(module.code.clone()));
 		let mut setup = CallSetup::<T>::new(module);
@@ -2412,38 +2406,22 @@ mod benchmarks {
 		let mut setup = CallSetup::<T>::new(module);
 		let contract = setup.contract();
 
-		let mut address: [u8; 32] = [0; 32];
-		address[12..].copy_from_slice(&contract.address.0);
-
 		let (mut ext, _) = setup.ext();
-		let mut interpreter: Interpreter<EVMInterpreter<'_, _>> = Interpreter {
-			extend: &mut ext,
-			input: Default::default(),
-			bytecode: Default::default(),
-			gas: Default::default(),
-			stack: Default::default(),
-			return_data: Default::default(),
-			memory: SharedMemory::new(),
-			runtime_flag: Default::default(),
-		};
-
-		let table = instruction_table::<'_, _>();
-		let extcodecopy_fn = table[EXTCODECOPY as usize];
+		let mut interpreter = Interpreter::new(Default::default(), Default::default(), &mut ext);
 
 		// Setup stack for extcodecopy instruction: [address, dest_offset, offset, size]
-		let _ = interpreter.stack.push(primitives::U256::from(n));
-		let _ = interpreter.stack.push(primitives::U256::from(0u32));
-		let _ = interpreter.stack.push(primitives::U256::from(0u32));
-		let _ = interpreter.stack.push(primitives::U256::from_be_bytes(address));
-
-		let mut host = DummyHost {};
-		let context = InstructionContext { interpreter: &mut interpreter, host: &mut host };
+		let _ = interpreter.stack.push(U256::from(n));
+		let _ = interpreter.stack.push(U256::from(0u32));
+		let _ = interpreter.stack.push(U256::from(0u32));
+		let _ = interpreter.stack.push(contract.address);
 
+		let result;
 		#[block]
 		{
-			extcodecopy_fn(context);
+			result = host::extcodecopy(&mut interpreter);
 		}
 
+		assert!(result.is_continue());
 		assert_eq!(
 			*interpreter.memory.slice(0..n as usize),
 			PristineCode::<T>::get(contract.info()?.code_hash).unwrap()[0..n as usize],

substrate/frame/revive/src/exec.rs

@@ -30,7 +30,7 @@ use crate::{
 	Pallet as Contracts, RuntimeCosts, LOG_TARGET,
 };
 use alloc::vec::Vec;
-use core::{fmt::Debug, marker::PhantomData, mem};
+use core::{fmt::Debug, marker::PhantomData, mem, ops::ControlFlow};
 use frame_support::{
 	crypto::ecdsa::ECDSAExt,
 	dispatch::DispatchResult,
@@ -287,6 +287,14 @@ pub trait PrecompileExt: sealing::Sealed {
 			.adjust_gas(charged, RuntimeCosts::Precompile(actual_weight));
 	}
 
+	/// Charges the gas meter with the given token or halts execution if not enough gas is left.
+	fn charge_or_halt<Tok: crate::gas::Token<Self::T>>(
+		&mut self,
+		token: Tok,
+	) -> ControlFlow<crate::vm::evm::Halt, crate::gas::ChargedAmount> {
+		self.gas_meter_mut().charge_or_halt(token)
+	}
+
 	/// Call (possibly transferring some amount of funds) into the specified account.
 	///
 	/// Returns the code size of the called contract.

substrate/frame/revive/src/gas.rs

@@ -1,3 +1,4 @@
+use crate::vm::evm::HaltReason;
 // This file is part of Substrate.
 
 // Copyright (C) Parity Technologies (UK) Ltd.
@@ -14,9 +15,8 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
-
-use crate::{exec::ExecError, weights::WeightInfo, Config, Error};
-use core::marker::PhantomData;
+use crate::{exec::ExecError, vm::evm::Halt, weights::WeightInfo, Config, Error};
+use core::{marker::PhantomData, ops::ControlFlow};
 use frame_support::{
 	dispatch::{DispatchErrorWithPostInfo, DispatchResultWithPostInfo, PostDispatchInfo},
 	weights::Weight,
@@ -219,16 +219,13 @@ impl<T: Config> GasMeter<T> {
 		Ok(ChargedAmount(amount))
 	}
 
-	/// Charge the specified amount of EVM gas.
-	/// This is used for basic opcodes (e.g arithmetic, bitwise, ...) that don't have a dedicated
-	/// benchmark
-	pub fn charge_evm_gas(&mut self, gas: u64) -> Result<(), DispatchError> {
-		let base_cost = T::WeightInfo::evm_opcode(1).saturating_sub(T::WeightInfo::evm_opcode(0));
-		self.gas_left = self
-			.gas_left
-			.checked_sub(&base_cost.saturating_mul(gas))
-			.ok_or_else(|| Error::<T>::OutOfGas)?;
-		Ok(())
+	/// Charge the specified token amount of gas or halt if not enough gas is left.
+	pub fn charge_or_halt<Tok: Token<T>>(
+		&mut self,
+		token: Tok,
+	) -> ControlFlow<Halt, ChargedAmount> {
+		self.charge(token)
+			.map_or_else(|_| ControlFlow::Break(HaltReason::OutOfGas.into()), ControlFlow::Continue)
 	}
 
 	/// Adjust a previously charged amount down to its actual amount.

substrate/frame/revive/src/lib.rs

@@ -480,6 +480,8 @@ pub mod pallet {
 		CallDataTooLarge = 0x30,
 		/// The return data exceeds [`limits::CALLDATA_BYTES`].
 		ReturnDataTooLarge = 0x31,
+		/// EVM execution halted with the given reason.
+		Halt(crate::vm::evm::HaltReason) = 0x32,
 	}
 
 	/// A reason for the pallet revive placing a hold on funds.

substrate/frame/revive/src/precompiles/builtin/bn128.rs

@@ -94,7 +94,7 @@ impl<T: Config> PrimitivePrecompile for Bn128Pairing<T> {
 		input: Vec<u8>,
 		env: &mut impl Ext<T = Self::T>,
 	) -> Result<Vec<u8>, Error> {
-		if input.len() % 192 != 0 {
+		if !input.len().is_multiple_of(192) {
 			Err(DispatchError::from("invalid input length"))?;
 		}
 

substrate/frame/revive/src/precompiles/builtin/modexp.rs

@@ -164,7 +164,7 @@ fn calculate_gas_cost(
 	fn calculate_multiplication_complexity(base_length: u64, mod_length: u64) -> u64 {
 		let max_length = max(base_length, mod_length);
 		let mut words = max_length / 8;
-		if max_length % 8 > 0 {
+		if !max_length.is_multiple_of(8) {
 			words += 1;
 		}
 

substrate/frame/revive/src/tests/sol.rs

@@ -23,9 +23,9 @@ use crate::{
 		test_utils::{contract_base_deposit, ensure_stored, get_contract},
 		ExtBuilder, Test,
 	},
-	Code, Config, PristineCode,
+	Code, Config, PristineCode, U256,
 };
-use alloy_core::{primitives::U256, sol_types::SolInterface};
+use alloy_core::{primitives, sol_types::SolInterface};
 use frame_support::traits::fungible::Mutate;
 use pallet_revive_fixtures::{compile_module_with_type, Fibonacci, FixtureType};
 use pretty_assertions::assert_eq;
@@ -88,14 +88,13 @@ fn basic_evm_flow_works() {
 
 			let result = builder::bare_call(addr)
 				.data(
-					Fibonacci::FibonacciCalls::fib(Fibonacci::fibCall { n: U256::from(10u64) })
-						.abi_encode(),
+					Fibonacci::FibonacciCalls::fib(Fibonacci::fibCall {
+						n: primitives::U256::from(10u64),
+					})
+					.abi_encode(),
 				)
 				.build_and_unwrap_result();
-			assert_eq!(
-				U256::from(55u32),
-				U256::from_be_bytes::<32>(result.data.try_into().unwrap())
-			);
+			assert_eq!(U256::from(55u32), U256::from_big_endian(&result.data));
 		}
 
 		// init code is not stored
@@ -140,26 +139,27 @@ fn basic_evm_flow_tracing_works() {
 		let result = trace(&mut call_tracer, || {
 			builder::bare_call(addr)
 				.data(
-					Fibonacci::FibonacciCalls::fib(Fibonacci::fibCall { n: U256::from(10u64) })
-						.abi_encode(),
+					Fibonacci::FibonacciCalls::fib(Fibonacci::fibCall {
+						n: primitives::U256::from(10u64),
+					})
+					.abi_encode(),
 				)
 				.build_and_unwrap_result()
 		});
 
-		assert_eq!(
-			U256::from(55u32),
-			U256::from_be_bytes::<32>(result.data.clone().try_into().unwrap())
-		);
+		assert_eq!(crate::U256::from(55u32), crate::U256::from_big_endian(&result.data));
 
 		assert_eq!(
 			call_tracer.collect_trace().unwrap(),
 			CallTrace {
 				call_type: CallType::Call,
 				from: ALICE_ADDR,
 				to: addr,
-				input: Fibonacci::FibonacciCalls::fib(Fibonacci::fibCall { n: U256::from(10u64) })
-					.abi_encode()
-					.into(),
+				input: Fibonacci::FibonacciCalls::fib(Fibonacci::fibCall {
+					n: primitives::U256::from(10u64)
+				})
+				.abi_encode()
+				.into(),
 				output: result.data.into(),
 				value: Some(crate::U256::zero()),
 				..Default::default()

substrate/frame/revive/src/tests/sol/bitwise.rs

@@ -39,15 +39,14 @@ fn bitwise_works() {
 			let result = builder::bare_call(addr)
 				.data(Bitwise::BitwiseCalls::testBitwise(Bitwise::testBitwiseCall {}).abi_encode())
 				.build_and_unwrap_result();
+
 			if result.did_revert() {
 				if let Some(revert_msg) = decode_revert_reason(&result.data) {
-					log::error!("Revert message: {}", revert_msg);
+					panic!("Revert message: {revert_msg}");
 				} else {
-					log::error!("Revert without message, raw data: {:?}", result.data);
+					panic!("Revert without message, raw data: {:?}", result.data);
 				}
 			}
-
-			assert!(!result.did_revert(), "bitwise test reverted");
 		});
 	}
 }

substrate/frame/revive/src/tests/sol/block_info.rs

@@ -20,15 +20,15 @@
 use crate::{
 	test_utils::{builder::Contract, ALICE},
 	tests::{builder, Contracts, ExtBuilder, System, Test, Timestamp},
-	vm::evm::{U256Converter, BASE_FEE, DIFFICULTY},
+	vm::evm::{BASE_FEE, DIFFICULTY},
 	Code, Config,
 };
 
-use alloy_core::{primitives::U256, sol_types::SolInterface};
+use alloy_core::sol_types::SolInterface;
 use frame_support::traits::fungible::Mutate;
 use pallet_revive_fixtures::{compile_module_with_type, BlockInfo, FixtureType};
 use pretty_assertions::assert_eq;
-use sp_core::H160;
+use sp_core::{H160, U256};
 
 /// Tests that the blocknumber opcode works as expected.
 #[test]
@@ -48,10 +48,7 @@ fn block_number_works() {
 						.abi_encode(),
 				)
 				.build_and_unwrap_result();
-			assert_eq!(
-				U256::from(42u32),
-				U256::from_be_bytes::<32>(result.data.try_into().unwrap())
-			);
+			assert_eq!(U256::from(42u32), U256::from_big_endian(&result.data));
 		});
 	}
 }
@@ -93,7 +90,7 @@ fn chainid_works() {
 				.build_and_unwrap_result();
 			assert_eq!(
 				U256::from(<Test as Config>::ChainId::get()),
-				U256::from_be_bytes::<32>(result.data.try_into().unwrap())
+				U256::from_big_endian(&result.data)
 			);
 		});
 	}
@@ -119,7 +116,7 @@ fn timestamp_works() {
 				// Solidity expects timestamps in seconds, whereas pallet_timestamp uses
 				// milliseconds.
 				U256::from(Timestamp::get() / 1000),
-				U256::from_be_bytes::<32>(result.data.try_into().unwrap())
+				U256::from_big_endian(&result.data)
 			);
 		});
 	}
@@ -142,7 +139,7 @@ fn gaslimit_works() {
 				U256::from(
 					<Test as frame_system::Config>::BlockWeights::get().max_block.ref_time()
 				),
-				U256::from_be_bytes::<32>(result.data.try_into().unwrap())
+				U256::from_big_endian(&result.data)
 			);
 		});
 	}
@@ -161,10 +158,7 @@ fn basefee_works() {
 			let result = builder::bare_call(addr)
 				.data(BlockInfo::BlockInfoCalls::basefee(BlockInfo::basefeeCall {}).abi_encode())
 				.build_and_unwrap_result();
-			assert_eq!(
-				BASE_FEE.into_revm_u256(),
-				U256::from_be_bytes::<32>(result.data.try_into().unwrap())
-			);
+			assert_eq!(BASE_FEE, U256::from_big_endian(&result.data));
 		});
 	}
 }
@@ -188,7 +182,7 @@ fn difficulty_works() {
 			assert_eq!(
 				// Alligned with the value set for PVM
 				U256::from(DIFFICULTY),
-				U256::from_be_bytes::<32>(result.data.try_into().unwrap())
+				U256::from_big_endian(&result.data)
 			);
 		});
 	}