diff --git a/prdoc/pr_10214.prdoc b/prdoc/pr_10214.prdoc new file mode 100644 index 0000000000000..48d2c4b52a7bf --- /dev/null +++ b/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 diff --git a/substrate/frame/revive/fixtures/contracts/System.sol b/substrate/frame/revive/fixtures/contracts/System.sol index 01b1c39297657..5863c33b77ade 100644 --- a/substrate/frame/revive/fixtures/contracts/System.sol +++ b/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); } diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 4115cd5c6895b..726f8c07061b8 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/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(>::CodeRejected.into()); } + ensure!(input_data.is_empty(), >::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(), >::EvmConstructedFromHash); + executable + }, }; self.push_frame( FrameArgs::Instantiate { diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 2e3fee1a89b83..d706fc075afe0 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/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 Pallet { }, Code::Upload(code) => if T::AllowEVMBytecode::get() { + ensure!(data.is_empty(), >::EvmConstructorNonEmptyData); let origin = T::UploadOrigin::ensure_origin(origin)?; let executable = ContractBlob::from_evm_init_code(code, origin)?; (executable, Default::default()) } else { return Err(>::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(), >::EvmConstructedFromHash); + (executable, Default::default()) + }, }; let instantiate_origin = ExecOrigin::from_account_id(instantiate_account.clone()); let mut storage_meter = StorageMeter::new(storage_deposit_limit); diff --git a/substrate/frame/revive/src/test_utils/builder.rs b/substrate/frame/revive/src/test_utils/builder.rs index 3d52f0662b55b..e67de6fbf8f39 100644 --- a/substrate/frame/revive/src/test_utils/builder.rs +++ b/substrate/frame/revive/src/test_utils/builder.rs @@ -137,12 +137,16 @@ builder!( exec_config: ExecConfig, ) -> ContractResult>; - 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) -> 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. diff --git a/substrate/frame/revive/src/tests/sol/system.rs b/substrate/frame/revive/src/tests/sol/system.rs index 3e571bd85b5a7..5d833b1e7ae5e 100644 --- a/substrate/frame/revive/src/tests/sol/system.rs +++ b/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 _ = ::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 _ = ::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 _ = ::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 _ = ::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 _ = ::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 _ = ::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 _ = ::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 _ = ::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 _ = ::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 = ::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 _ = ::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()); + }); +}