pallet_revive: Fix EVM tests to pass data as part of code

prdoc/pr_10214.prdoc

@@ -0,0 +1,25 @@
+title: 'pallet_revive: Fix EVM tests to pass `data` as part of `code`'
+doc:
+- audience: Runtime User
+  description: |-
+    The test code was passing the constructor argument as `data` on EVM. But it should be passed as part of the `code`.  This is different from PVM where those are separate.
+
+    Failing to do so makes those opcodes return the wrong values when `data` is passed to the constructor:
+
+    ```
+    CODESIZE
+    CODECOPY
+    CALLDATASIZE
+    CALLDATACOPY
+    CALLDATALOAD
+    ```
+
+    Further changes:
+
+    - I also added some checks to fail instantiation if `data` is non empty when uploading new EVM bytecode.
+    - Return error when trying to construct EVM contract from code hash as this does not make sense since no initcode is stored on-chain.
+crates:
+- name: pallet-revive-fixtures
+  bump: major
+- name: pallet-revive
+  bump: major

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

@@ -3,6 +3,12 @@
 pragma solidity ^0.8.20;
 
 contract System {
+	constructor(bool panic) {
+		if (panic) {
+			revert("Reverted because revert=true was set as constructor argument");
+		}
+	}
+
     function keccak256Func(bytes memory data) public pure returns (bytes32) {
         return keccak256(data);
     }

substrate/frame/revive/src/exec.rs

@@ -42,6 +42,7 @@ use core::{fmt::Debug, marker::PhantomData, mem, ops::ControlFlow};
 use frame_support::{
 	crypto::ecdsa::ECDSAExt,
 	dispatch::DispatchResult,
+	ensure,
 	storage::{with_transaction, TransactionOutcome},
 	traits::{
 		fungible::{Inspect, Mutate},
@@ -1833,9 +1834,14 @@ where
 					if !T::AllowEVMBytecode::get() {
 						return Err(<Error<T>>::CodeRejected.into());
 					}
+					ensure!(input_data.is_empty(), <Error<T>>::EvmConstructorNonEmptyData);
 					E::from_evm_init_code(bytecode.clone(), sender.clone())?
 				},
-				Code::Existing(hash) => E::from_storage(*hash, self.gas_meter_mut())?,
+				Code::Existing(hash) => {
+					let executable = E::from_storage(*hash, self.gas_meter_mut())?;
+					ensure!(executable.code_info().is_pvm(), <Error<T>>::EvmConstructedFromHash);
+					executable
+				},
 			};
 			self.push_frame(
 				FrameArgs::Instantiate {

substrate/frame/revive/src/lib.rs

@@ -564,7 +564,15 @@ pub mod pallet {
 		///
 		/// This happens if the passed `gas` inside the ethereum transaction is too low.
 		TxFeeOverdraw = 0x35,
-
+		/// When calling an EVM constructor `data` has to be empty.
+		///
+		/// EVM constructors do not accept data. Their input data is part of the code blob itself.
+		EvmConstructorNonEmptyData = 0x36,
+		/// Tried to construct an EVM contract via code hash.
+		///
+		/// EVM contracts can only be instantiated via code upload as no initcode is
+		/// stored on-chain.
+		EvmConstructedFromHash = 0x37,
 		/// Benchmarking only error.
 		#[cfg(feature = "runtime-benchmarks")]
 		BenchmarkingError = 0xFF,
@@ -1564,14 +1572,18 @@ impl<T: Config> Pallet<T> {
 				},
 				Code::Upload(code) =>
 					if T::AllowEVMBytecode::get() {
+						ensure!(data.is_empty(), <Error<T>>::EvmConstructorNonEmptyData);
 						let origin = T::UploadOrigin::ensure_origin(origin)?;
 						let executable = ContractBlob::from_evm_init_code(code, origin)?;
 						(executable, Default::default())
 					} else {
 						return Err(<Error<T>>::CodeRejected.into())
 					},
-				Code::Existing(code_hash) =>
-					(ContractBlob::from_storage(code_hash, &mut gas_meter)?, Default::default()),
+				Code::Existing(code_hash) => {
+					let executable = ContractBlob::from_storage(code_hash, &mut gas_meter)?;
+					ensure!(executable.code_info().is_pvm(), <Error<T>>::EvmConstructedFromHash);
+					(executable, Default::default())
+				},
 			};
 			let instantiate_origin = ExecOrigin::from_account_id(instantiate_account.clone());
 			let mut storage_meter = StorageMeter::new(storage_deposit_limit);

substrate/frame/revive/src/test_utils/builder.rs

@@ -137,12 +137,16 @@ builder!(
 		exec_config: ExecConfig<T>,
 	) -> ContractResult<InstantiateReturnValue, BalanceOf<T>>;
 
-	pub fn concat_evm_data(mut self, more_data: &[u8]) -> Self {
-		let Code::Upload(code) = &mut self.code else {
-			panic!("concat_evm_data should only be used with Code::Upload");
-		};
-		code.extend_from_slice(more_data);
-		self
+	pub fn constructor_data(mut self, data: Vec<u8>) -> Self {
+		match self.code {
+			Code::Upload(ref mut code) if !code.starts_with(&polkavm_common::program::BLOB_MAGIC) => {
+				code.extend_from_slice(&data);
+				self
+			},
+			_ => {
+				self.data(data)
+			}
+		}
 	}
 
 	/// Set the call's evm_value using a native_value amount.

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

@@ -23,7 +23,7 @@ use crate::{
 	tests::{builder, Contracts, ExtBuilder, Test},
 	Code, Combinator, Config, ExecConfig, U256,
 };
-use alloy_core::sol_types::SolCall;
+use alloy_core::sol_types::{Revert, SolCall, SolConstructor, SolError};
 use frame_support::traits::fungible::{Balanced, Mutate};
 use pallet_revive_fixtures::{
 	compile_module_with_type, Callee, FixtureType, System as SystemFixture,
@@ -40,8 +40,9 @@ fn keccak_256_works(fixture_type: FixtureType) {
 	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
 	ExtBuilder::default().build().execute_with(|| {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		let pre = b"revive";
 		let expected = keccak_256(pre);
@@ -60,8 +61,9 @@ fn address_works(fixture_type: FixtureType) {
 	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
 	ExtBuilder::default().build().execute_with(|| {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		let result = builder::bare_call(addr)
 			.data(SystemFixture::addressFuncCall {}.abi_encode())
@@ -78,8 +80,9 @@ fn caller_works(fixture_type: FixtureType) {
 	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
 	ExtBuilder::default().build().execute_with(|| {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		let result = builder::bare_call(addr)
 			.data(SystemFixture::callerCall {}.abi_encode())
@@ -96,8 +99,9 @@ fn callvalue_works(fixture_type: FixtureType) {
 	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
 	ExtBuilder::default().build().execute_with(|| {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		let value = 1337u64;
 
@@ -117,8 +121,9 @@ fn calldataload_works(fixture_type: FixtureType) {
 	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
 	ExtBuilder::default().build().execute_with(|| {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		let result = builder::bare_call(addr)
 			.data(SystemFixture::calldataloadCall { offset: 4u64 }.abi_encode())
@@ -136,8 +141,9 @@ fn calldatasize_works(fixture_type: FixtureType) {
 	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
 	ExtBuilder::default().build().execute_with(|| {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		// calldata = selector + encoded argument
 		let result = builder::bare_call(addr)
@@ -156,8 +162,9 @@ fn calldatacopy_works(fixture_type: FixtureType) {
 	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
 	ExtBuilder::default().build().execute_with(|| {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		let call_data = SystemFixture::calldatacopyCall {
 			destOffset: 0u64, // unused
@@ -186,8 +193,9 @@ fn codesize_works(fixture_type: FixtureType) {
 	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
 	ExtBuilder::default().build().execute_with(|| {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code.clone())).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code.clone()))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		let result = builder::bare_call(addr)
 			.data(SystemFixture::codesizeCall {}.abi_encode())
@@ -209,8 +217,9 @@ fn gas_works(fixture_type: FixtureType) {
 	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
 	ExtBuilder::default().build().execute_with(|| {
 		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code.clone())).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code.clone()))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		// enable txhold collection which we expect to be on when using the evm backend
 		let hold_initial = <Test as Config>::FeeInfo::weight_to_fee(&GAS_LIMIT, Combinator::Max);
@@ -248,8 +257,9 @@ fn returndatasize_works(caller_type: FixtureType, callee_type: FixtureType) {
 		let Contract { addr: callee_addr, .. } =
 			builder::bare_instantiate(Code::Upload(callee_code)).build_and_unwrap_contract();
 
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		let magic_number = 42u64;
 		let result = builder::bare_call(addr)
@@ -282,8 +292,9 @@ fn returndatacopy_works(caller_type: FixtureType, callee_type: FixtureType) {
 		let Contract { addr: callee_addr, .. } =
 			builder::bare_instantiate(Code::Upload(callee_code)).build_and_unwrap_contract();
 
-		let Contract { addr, .. } =
-			builder::bare_instantiate(Code::Upload(code)).build_and_unwrap_contract();
+		let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode())
+			.build_and_unwrap_contract();
 
 		let magic_number = U256::from(42);
 		let result = builder::bare_call(addr)
@@ -305,3 +316,22 @@ fn returndatacopy_works(caller_type: FixtureType, callee_type: FixtureType) {
 		assert_eq!(magic_number, decoded_value)
 	});
 }
+
+#[test_case(FixtureType::Solc)]
+#[test_case(FixtureType::Resolc)]
+fn constructor_with_argument_works(fixture_type: FixtureType) {
+	let (code, _) = compile_module_with_type("System", fixture_type).unwrap();
+	ExtBuilder::default().build().execute_with(|| {
+		let _ = <Test as Config>::Currency::set_balance(&ALICE, 100_000_000_000);
+		let result = builder::bare_instantiate(Code::Upload(code))
+			.constructor_data(SystemFixture::constructorCall { panic: true }.abi_encode())
+			.build()
+			.result
+			.unwrap()
+			.result;
+		assert!(result.did_revert());
+
+		let expected_message = "Reverted because revert=true was set as constructor argument";
+		assert_eq!(result.data, Revert::from(expected_message).abi_encode());
+	});
+}