[pallet-revive] revm refactor (#9818)

Refactor REVM implementation in `pallet-revive`

This PR removes technical debt and eliminates tight coupling to specific
REVM versions, facilitating integration with other projects (e.g.,
Foundry). After this refactoring, we primarily depend only on REVM's
[`Bytecode`](https://docs.rs/revm/latest/revm/bytecode/struct.Bytecode.html)
struct.

## Background

Most of REVM's generic type system and trait abstractions are unused or
had to be ignored to prevent bugs. Interactions with the host in
pallet-revive are handled through the `Ext` trait, making REVM's other
abstractions unnecessary or potentially harmful.

Unused REVM abstractions included:
- **Host trait**: Unused in the pallet, we relied on the `DummyHost`
default mocked implementation
- **Gas field**: Unused, the pallet uses its own gas accounting system
- **Methods from `InputsTr`**: Unused most methods had panic
implementations since they couldn't be relied upon
- **Spec**: Unused: We only maintain the latest fork for each runtime

## Changes

This refactor introduces:
- **Interpreter**: Simplified struct containing only the fields actually
needed
- **Stack**: Simplified implementation using `sp_core::*` instead of
`alloy_core::primitives::*` for better integration with the rest of the
pallet
- **Memory**: Simplified implementation providing only the methods
actually needed
- **Instructions**:
- New instructions don't rely on macros and have a simplified signature:
`Fn(&mut interpreter) -> ControlFlow<Halt>`
- Removed function pointer table lookup for instructions in favor of
match statements,
- Unified gas charging: legacy u64 gas charging is now wrapped in
`EVMGas` that implements `Token<T>` to provide the associated weight
- Removed the `InterpreterAction`, this simplify the interpreter loop
to:
```rust
loop {
	let opcode = interpreter.bytecode.opcode();
	interpreter.bytecode.relative_jump(1);
	exec_instruction(interpreter, opcode)?;
}
```
- **Error handling**: Opcode that fail return Halt::Err(DispatchError),
this remove the need from converting between InstructionResult and
`ExecError` like we were previously doing and standardize errors
generated on both backends

---------

Co-authored-by: 0xRVE <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Robert van Eerdewijk <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>

Author: 5 people

Cargo.lock

@@ -1181,6 +1181,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" }

Cargo.toml

@@ -0,0 +1,28 @@
+title: '[pallet-revive] revm refactor'
+doc:
+- audience: Runtime Dev
+  description: |
+    This PR refactors the REVM implementation in `pallet-revive`, reducing technical debt and decoupling it from specific REVM versions. This enables easier integration with other projects, such as Foundry, with dependencies limited to REVM's [`Bytecode`](https://docs.rs/revm/latest/revm/bytecode/struct.Bytecode.html).
+
+    **Background:**  
+    Many of REVM's generics and trait abstractions were unused or ignored to avoid bugs. The pallet interacts with the host via the `Ext` trait, rendering other REVM abstractions unnecessary.
+
+    Previously unused REVM components included:
+    - `Host` trait: Unused, relied on the `DummyHost` mock.
+    - Gas field: Not used, as the pallet uses its own accounting.
+    - Methods from `InputsTr`: Most methods unused or had panic implementations.
+    - `Spec`: Only maintaining the latest fork per runtime.
+
+    **Key Changes:**
+    - Interpreter: Now a minimal struct with only necessary fields.
+    - Stack: Uses `sp_core::U256` for better integration.
+    - Memory: Simplified to needed methods only.
+    - Instructions: Rewritten with simple signatures, using match statements instead of function pointer tables. Gas charging uses `EVMGas` wrapped in `Token<T>`. The interpreter loop has been significantly simplified.
+    - Error Handling: Failed opcodes now return `Halt::Err(DispatchError)`, removing previous error conversions.
+
+crates:
+- name: pallet-revive
+  bump: patch
+- name: pallet-revive-fixtures
+  bump: patch
+

prdoc/pr_9818.prdoc

@@ -0,0 +1,7 @@
+title: Rve/9565 pallet revive evm backend ensure memory dos safety2
+doc:
+- audience: Runtime Dev
+  description: fixes https://github.com/paritytech/polkadot-sdk/issues/9565
+crates:
+- name: pallet-revive
+  bump: patch

prdoc/pr_9887.prdoc

@@ -74,6 +74,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/Cargo.toml

@@ -6,7 +6,7 @@ contract TransactionInfo {
         return tx.origin;
     }
 
-    function gasprice() public view returns (uint256) {
+    function gasprice() public view returns (uint64) {
         return uint64(tx.gasprice);
     }
 

substrate/frame/revive/fixtures/contracts/TransactionInfo.sol

@@ -34,7 +34,7 @@ use crate::{
 	storage::WriteOutcome,
 	vm::{
 		evm,
-		evm::{instructions::instruction_table, EVMInterpreter},
+		evm::{instructions::host, Interpreter},
 		pvm,
 	},
 	Pallet as Contracts, *,
@@ -54,13 +54,7 @@ use frame_system::RawOrigin;
 use pallet_revive_uapi::{
 	pack_hi_lo, precompiles::system::ISystem, 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},
@@ -2367,7 +2361,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);
@@ -2472,38 +2466,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/benchmarking.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,
@@ -290,6 +290,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.
 	fn call(
 		&mut self,

substrate/frame/revive/src/exec.rs

@@ -14,9 +14,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 +218,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(Error::<T>::OutOfGas.into()), ControlFlow::Continue)
 	}
 
 	/// Adjust a previously charged amount down to its actual amount.

substrate/frame/revive/src/gas.rs

@@ -73,6 +73,12 @@ pub const PAGE_SIZE: u32 = 4 * 1024;
 /// Which should always be enough because Solidity allows for 16 local (stack) variables.
 pub const IMMUTABLE_BYTES: u32 = 4 * 1024;
 
+/// upperbound of memory that can be used by the EVM interpreter.
+pub const EVM_MEMORY_BYTES: u32 = 1024 * 1024;
+
+/// EVM interpreter stack limit.
+pub const EVM_STACK_LIMIT: u32 = 1024;
+
 /// Limits that are only enforced on code upload.
 ///
 /// # Note
@@ -254,7 +260,14 @@ const fn memory_required() -> u32 {
 	let max_call_depth = CALL_STACK_DEPTH + 1;
 
 	let per_stack_memory = code::PURGABLE_MEMORY_LIMIT + TRANSIENT_STORAGE_BYTES * 2;
-	let per_frame_memory = code::BASELINE_MEMORY_LIMIT + CALLDATA_BYTES * 2;
+
+	let evm_max_initcode_size = revm::primitives::eip3860::MAX_INITCODE_SIZE as u32;
+	let evm_overhead = EVM_MEMORY_BYTES + evm_max_initcode_size + EVM_STACK_LIMIT * 32;
+	let per_frame_memory = if code::BASELINE_MEMORY_LIMIT > evm_overhead {
+		code::BASELINE_MEMORY_LIMIT
+	} else {
+		evm_overhead
+	} + CALLDATA_BYTES * 2;
 
 	per_stack_memory + max_call_depth * per_frame_memory
 }