From 4a4a56e12adfc3e0c1e7dad01b081a797d5d25d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 6 Nov 2025 00:32:05 +0100 Subject: [PATCH 1/7] Fix EVM constructors --- .../revive/fixtures/contracts/System.sol | 6 + substrate/frame/revive/src/exec.rs | 7 ++ substrate/frame/revive/src/exec/mock_ext.rs | 9 +- .../frame/revive/src/tests/sol/system.rs | 85 +++++++++---- substrate/frame/revive/src/vm/evm.rs | 8 +- .../revive/src/vm/evm/instructions/system.rs | 112 +++++++++++++----- 6 files changed, 173 insertions(+), 54 deletions(-) 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..e078a87b87a5b 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -470,6 +470,9 @@ pub trait PrecompileExt: sealing::Sealed { /// Charges `diff` from the meter. fn charge_storage(&mut self, diff: &Diff); + + /// Are we inside the constructor? + fn entry_point(&self) -> ExportedFunction; } /// Describes the different functions that can be exported by an [`Executable`]. @@ -2296,6 +2299,10 @@ where assert!(self.has_contract_info()); self.top_frame_mut().nested_storage.charge(diff) } + + fn entry_point(&self) -> ExportedFunction { + self.top_frame().entry_point + } } /// Returns true if the address has a precompile contract, else false. diff --git a/substrate/frame/revive/src/exec/mock_ext.rs b/substrate/frame/revive/src/exec/mock_ext.rs index 8338b66825bb9..360e9dea08409 100644 --- a/substrate/frame/revive/src/exec/mock_ext.rs +++ b/substrate/frame/revive/src/exec/mock_ext.rs @@ -17,7 +17,10 @@ #![cfg(test)] use crate::{ - exec::{AccountIdOf, ExecError, Ext, Key, Origin, PrecompileExt, PrecompileWithInfoExt}, + exec::{ + AccountIdOf, ExecError, ExportedFunction, Ext, Key, Origin, PrecompileExt, + PrecompileWithInfoExt, + }, gas::GasMeter, precompiles::Diff, storage::{ContractInfo, WriteOutcome}, @@ -239,6 +242,10 @@ impl PrecompileExt for MockExt { } fn charge_storage(&mut self, _diff: &Diff) {} + + fn entry_point(&self) -> ExportedFunction { + panic!("MockExt::entry_point") + } } impl PrecompileWithInfoExt for MockExt { diff --git a/substrate/frame/revive/src/tests/sol/system.rs b/substrate/frame/revive/src/tests/sol/system.rs index 3e571bd85b5a7..3a3d766038f18 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::{SolCall, SolConstructor}; 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)) + .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)) + .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)) + .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)) + .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)) + .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)) + .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)) + .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())) + .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())) + .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)) + .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)) + .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,31 @@ 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)) + .data(SystemFixture::constructorCall { panic: true }.abi_encode()) + .build() + .result + .unwrap() + .result; + assert!(result.did_revert()); + assert_eq!( + result.data, + [ + 8, 195, 121, 160, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 60, 82, 101, 118, 101, 114, 116, 101, + 100, 32, 98, 101, 99, 97, 117, 115, 101, 32, 114, 101, 118, 101, 114, 116, 61, 116, + 114, 117, 101, 32, 119, 97, 115, 32, 115, 101, 116, 32, 97, 115, 32, 99, 111, 110, + 115, 116, 114, 117, 99, 116, 111, 114, 32, 97, 114, 103, 117, 109, 101, 110, 116, + 0, 0, 0, 0, + ], + ) + }); +} diff --git a/substrate/frame/revive/src/vm/evm.rs b/substrate/frame/revive/src/vm/evm.rs index 91da9c18e5460..f2a0d0a742b31 100644 --- a/substrate/frame/revive/src/vm/evm.rs +++ b/substrate/frame/revive/src/vm/evm.rs @@ -17,7 +17,7 @@ use crate::{ debug::DebugSettings, precompiles::Token, - vm::{evm::instructions::exec_instruction, BytecodeType, ExecResult, Ext}, + vm::{evm::instructions::exec_instruction, BytecodeType, ExecResult, ExportedFunction, Ext}, weights::WeightInfo, AccountIdOf, CodeInfo, Config, ContractBlob, DispatchError, Error, Weight, H256, LOG_TARGET, }; @@ -140,3 +140,9 @@ fn run_plain(interpreter: &mut Interpreter) -> ControlFlow bool { + matches!(self, ExportedFunction::Constructor) + } +} diff --git a/substrate/frame/revive/src/vm/evm/instructions/system.rs b/substrate/frame/revive/src/vm/evm/instructions/system.rs index 411ed142e07f5..0c3c05c896229 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/system.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/system.rs @@ -84,7 +84,8 @@ pub fn caller(interpreter: &mut Interpreter) -> ControlFlow { /// Pushes the size of running contract's bytecode onto the stack. pub fn codesize(interpreter: &mut Interpreter) -> ControlFlow { interpreter.ext.charge_or_halt(EVMGas(BASE))?; - interpreter.stack.push(U256::from(interpreter.bytecode.len())) + let len = in_data_size(interpreter, true); + interpreter.stack.push(len) } /// Implements the CODECOPY instruction. @@ -92,20 +93,7 @@ pub fn codesize(interpreter: &mut Interpreter) -> ControlFlow { /// Copies running contract's bytecode to memory. pub fn codecopy(interpreter: &mut Interpreter) -> ControlFlow { let [memory_offset, code_offset, len] = interpreter.stack.popn()?; - let len = as_usize_or_halt::(len)?; - let Some(memory_offset) = memory_resize(interpreter, memory_offset, len)? else { - return ControlFlow::Continue(()) - }; - let code_offset = as_usize_saturated(code_offset); - - // Note: This can't panic because we resized memory. - interpreter.memory.set_data( - memory_offset, - code_offset, - len, - interpreter.bytecode.bytecode_slice(), - ); - ControlFlow::Continue(()) + in_data_copy(interpreter, memory_offset, code_offset, len, true) } /// Implements the CALLDATALOAD instruction. @@ -116,7 +104,17 @@ pub fn calldataload(interpreter: &mut Interpreter) -> ControlFlow (bytecode, offset), + (true, false) => (input, offset - bytecode.len()), + (false, _) => (input, offset), + }; + let input_len = input.len(); if offset < input_len { let count = 32.min(input_len - offset); @@ -131,7 +129,8 @@ pub fn calldataload(interpreter: &mut Interpreter) -> ControlFlow(interpreter: &mut Interpreter) -> ControlFlow { interpreter.ext.charge_or_halt(EVMGas(BASE))?; - interpreter.stack.push(U256::from(interpreter.input.len())) + let len = in_data_size(interpreter, false); + interpreter.stack.push(len) } /// Implements the CALLVALUE instruction. @@ -148,17 +147,7 @@ pub fn callvalue(interpreter: &mut Interpreter) -> ControlFlow /// Copies input data to memory. pub fn calldatacopy(interpreter: &mut Interpreter) -> ControlFlow { let [memory_offset, data_offset, len] = interpreter.stack.popn()?; - let len = as_usize_or_halt::(len)?; - - let Some(memory_offset) = memory_resize(interpreter, memory_offset, len)? else { - return ControlFlow::Continue(()); - }; - - let data_offset = as_usize_saturated(data_offset); - - // Note: This can't panic because we resized memory. - interpreter.memory.set_data(memory_offset, data_offset, len, &interpreter.input); - ControlFlow::Continue(()) + in_data_copy(interpreter, memory_offset, data_offset, len, false) } /// EIP-211: New opcodes: RETURNDATASIZE and RETURNDATACOPY @@ -207,7 +196,7 @@ pub fn gas(interpreter: &mut Interpreter) -> ControlFlow { /// Common logic for copying data from a source buffer to the EVM's memory. /// /// Handles memory expansion and gas calculation for data copy operations. -pub fn memory_resize<'a, E: Ext>( +fn memory_resize<'a, E: Ext>( interpreter: &mut Interpreter<'a, E>, memory_offset: U256, len: usize, @@ -221,3 +210,68 @@ pub fn memory_resize<'a, E: Ext>( interpreter.memory.resize(memory_offset, len)?; ControlFlow::Continue(Some(memory_offset)) } + +/// The shared logic for CODECOPY and CALLDATACOPY. +fn in_data_copy( + interpreter: &mut Interpreter, + memory_offset: U256, + data_offset: U256, + len: U256, + is_codecopy: bool, +) -> ControlFlow { + let len = as_usize_or_halt::(len)?; + let Some(memory_offset) = memory_resize(interpreter, memory_offset, len)? else { + return ControlFlow::Continue(()) + }; + let data_offset = as_usize_saturated(data_offset); + let is_constructor = interpreter.ext.entry_point().is_evm_constructor(); + let bytecode = interpreter.bytecode.bytecode_slice(); + let input = interpreter.input.as_slice(); + + struct Write<'a> { + memory_offset: usize, + data_offset: usize, + len: usize, + slice: &'a [u8], + } + + let writes = match (is_constructor, is_codecopy) { + (true, _) => { + let bytes_written = bytecode.len().saturating_sub(data_offset).min(len); + &[ + Write { memory_offset, data_offset, len: bytes_written, slice: bytecode }, + Write { + memory_offset: memory_offset.saturating_add(bytes_written), + data_offset: data_offset.saturating_sub(bytecode.len()), + len: len.saturating_sub(bytes_written), + slice: input, + }, + ][..] + }, + (false, true) => &[Write { memory_offset, data_offset, len, slice: bytecode }], + (false, false) => &[Write { memory_offset, data_offset, len, slice: input }], + }; + + for write in writes.into_iter().filter(|w| w.len != 0) { + // Note: This can't panic because we resized memory. + interpreter + .memory + .set_data(write.memory_offset, write.data_offset, write.len, write.slice); + } + + ControlFlow::Continue(()) +} + +/// The shared logic for CODESIZE and CALLDATASIZE +fn in_data_size(interpreter: &mut Interpreter, is_codecopy: bool) -> U256 { + let bytecode_len = interpreter.bytecode.len(); + let input_len = interpreter.input.len(); + let is_constructor = interpreter.ext.entry_point().is_evm_constructor(); + + match (is_constructor, is_codecopy) { + (true, _) => bytecode_len.saturating_add(input_len), + (false, true) => bytecode_len, + (false, false) => input_len, + } + .into() +} From c4656a907be5409c09dedd78620d6c92d972608a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Thu, 6 Nov 2025 06:49:02 +0100 Subject: [PATCH 2/7] Fix CALLDATA opcodes --- .../frame/revive/src/vm/evm/instructions/system.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/substrate/frame/revive/src/vm/evm/instructions/system.rs b/substrate/frame/revive/src/vm/evm/instructions/system.rs index 0c3c05c896229..c3801c0ed97fe 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/system.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/system.rs @@ -106,14 +106,8 @@ pub fn calldataload(interpreter: &mut Interpreter) -> ControlFlow (bytecode, offset), - (true, false) => (input, offset - bytecode.len()), - (false, _) => (input, offset), - }; + let input = if is_constructor { &[] } else { input }; let input_len = input.len(); if offset < input_len { @@ -236,7 +230,7 @@ fn in_data_copy( } let writes = match (is_constructor, is_codecopy) { - (true, _) => { + (true, true) => { let bytes_written = bytecode.len().saturating_sub(data_offset).min(len); &[ Write { memory_offset, data_offset, len: bytes_written, slice: bytecode }, @@ -248,6 +242,7 @@ fn in_data_copy( }, ][..] }, + (true, false) => &[Write { memory_offset, data_offset, len, slice: &[] }], (false, true) => &[Write { memory_offset, data_offset, len, slice: bytecode }], (false, false) => &[Write { memory_offset, data_offset, len, slice: input }], }; @@ -269,7 +264,8 @@ fn in_data_size(interpreter: &mut Interpreter, is_codecopy: bool) -> let is_constructor = interpreter.ext.entry_point().is_evm_constructor(); match (is_constructor, is_codecopy) { - (true, _) => bytecode_len.saturating_add(input_len), + (true, true) => bytecode_len.saturating_add(input_len), + (true, false) => 0, (false, true) => bytecode_len, (false, false) => input_len, } From e0d2cd5171f836dbc060653a61324ab9beee06f6 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Thu, 6 Nov 2025 18:19:26 +0100 Subject: [PATCH 3/7] Nit use string msg instead of byte array --- substrate/frame/revive/src/tests/sol/system.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/substrate/frame/revive/src/tests/sol/system.rs b/substrate/frame/revive/src/tests/sol/system.rs index 3a3d766038f18..c07187db03188 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, SolConstructor}; +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, @@ -330,17 +330,8 @@ fn constructor_with_argument_works(fixture_type: FixtureType) { .unwrap() .result; assert!(result.did_revert()); - assert_eq!( - result.data, - [ - 8, 195, 121, 160, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 60, 82, 101, 118, 101, 114, 116, 101, - 100, 32, 98, 101, 99, 97, 117, 115, 101, 32, 114, 101, 118, 101, 114, 116, 61, 116, - 114, 117, 101, 32, 119, 97, 115, 32, 115, 101, 116, 32, 97, 115, 32, 99, 111, 110, - 115, 116, 114, 117, 99, 116, 111, 114, 32, 97, 114, 103, 117, 109, 101, 110, 116, - 0, 0, 0, 0, - ], - ) + + let expected_message = "Reverted because revert=true was set as constructor argument"; + assert_eq!(result.data, Revert::from(expected_message).abi_encode()); }); } From e4caeb33a40ccec6d3075c808583409fd3891296 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Tue, 11 Nov 2025 10:06:56 +0100 Subject: [PATCH 4/7] Add `constructor_data` for BareInstantiateBuilder --- substrate/frame/revive/src/exec.rs | 7 -- substrate/frame/revive/src/exec/mock_ext.rs | 9 +- .../frame/revive/src/test_utils/builder.rs | 16 ++- .../frame/revive/src/tests/sol/system.rs | 24 ++-- substrate/frame/revive/src/vm/evm.rs | 8 +- .../revive/src/vm/evm/instructions/system.rs | 108 +++++------------- 6 files changed, 53 insertions(+), 119 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index e078a87b87a5b..4115cd5c6895b 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -470,9 +470,6 @@ pub trait PrecompileExt: sealing::Sealed { /// Charges `diff` from the meter. fn charge_storage(&mut self, diff: &Diff); - - /// Are we inside the constructor? - fn entry_point(&self) -> ExportedFunction; } /// Describes the different functions that can be exported by an [`Executable`]. @@ -2299,10 +2296,6 @@ where assert!(self.has_contract_info()); self.top_frame_mut().nested_storage.charge(diff) } - - fn entry_point(&self) -> ExportedFunction { - self.top_frame().entry_point - } } /// Returns true if the address has a precompile contract, else false. diff --git a/substrate/frame/revive/src/exec/mock_ext.rs b/substrate/frame/revive/src/exec/mock_ext.rs index 360e9dea08409..8338b66825bb9 100644 --- a/substrate/frame/revive/src/exec/mock_ext.rs +++ b/substrate/frame/revive/src/exec/mock_ext.rs @@ -17,10 +17,7 @@ #![cfg(test)] use crate::{ - exec::{ - AccountIdOf, ExecError, ExportedFunction, Ext, Key, Origin, PrecompileExt, - PrecompileWithInfoExt, - }, + exec::{AccountIdOf, ExecError, Ext, Key, Origin, PrecompileExt, PrecompileWithInfoExt}, gas::GasMeter, precompiles::Diff, storage::{ContractInfo, WriteOutcome}, @@ -242,10 +239,6 @@ impl PrecompileExt for MockExt { } fn charge_storage(&mut self, _diff: &Diff) {} - - fn entry_point(&self) -> ExportedFunction { - panic!("MockExt::entry_point") - } } impl PrecompileWithInfoExt for MockExt { 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 c07187db03188..5d833b1e7ae5e 100644 --- a/substrate/frame/revive/src/tests/sol/system.rs +++ b/substrate/frame/revive/src/tests/sol/system.rs @@ -41,7 +41,7 @@ fn keccak_256_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); let pre = b"revive"; @@ -62,7 +62,7 @@ fn address_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); let result = builder::bare_call(addr) @@ -81,7 +81,7 @@ fn caller_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); let result = builder::bare_call(addr) @@ -100,7 +100,7 @@ fn callvalue_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); let value = 1337u64; @@ -122,7 +122,7 @@ fn calldataload_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); let result = builder::bare_call(addr) @@ -142,7 +142,7 @@ fn calldatasize_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); // calldata = selector + encoded argument @@ -163,7 +163,7 @@ fn calldatacopy_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); let call_data = SystemFixture::calldatacopyCall { @@ -194,7 +194,7 @@ fn codesize_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code.clone())) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); let result = builder::bare_call(addr) @@ -218,7 +218,7 @@ fn gas_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code.clone())) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .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 @@ -258,7 +258,7 @@ fn returndatasize_works(caller_type: FixtureType, callee_type: FixtureType) { builder::bare_instantiate(Code::Upload(callee_code)).build_and_unwrap_contract(); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); let magic_number = 42u64; @@ -293,7 +293,7 @@ fn returndatacopy_works(caller_type: FixtureType, callee_type: FixtureType) { builder::bare_instantiate(Code::Upload(callee_code)).build_and_unwrap_contract(); let Contract { addr, .. } = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: false }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: false }.abi_encode()) .build_and_unwrap_contract(); let magic_number = U256::from(42); @@ -324,7 +324,7 @@ fn constructor_with_argument_works(fixture_type: FixtureType) { ExtBuilder::default().build().execute_with(|| { let _ = ::Currency::set_balance(&ALICE, 100_000_000_000); let result = builder::bare_instantiate(Code::Upload(code)) - .data(SystemFixture::constructorCall { panic: true }.abi_encode()) + .constructor_data(SystemFixture::constructorCall { panic: true }.abi_encode()) .build() .result .unwrap() diff --git a/substrate/frame/revive/src/vm/evm.rs b/substrate/frame/revive/src/vm/evm.rs index f2a0d0a742b31..91da9c18e5460 100644 --- a/substrate/frame/revive/src/vm/evm.rs +++ b/substrate/frame/revive/src/vm/evm.rs @@ -17,7 +17,7 @@ use crate::{ debug::DebugSettings, precompiles::Token, - vm::{evm::instructions::exec_instruction, BytecodeType, ExecResult, ExportedFunction, Ext}, + vm::{evm::instructions::exec_instruction, BytecodeType, ExecResult, Ext}, weights::WeightInfo, AccountIdOf, CodeInfo, Config, ContractBlob, DispatchError, Error, Weight, H256, LOG_TARGET, }; @@ -140,9 +140,3 @@ fn run_plain(interpreter: &mut Interpreter) -> ControlFlow bool { - matches!(self, ExportedFunction::Constructor) - } -} diff --git a/substrate/frame/revive/src/vm/evm/instructions/system.rs b/substrate/frame/revive/src/vm/evm/instructions/system.rs index c3801c0ed97fe..411ed142e07f5 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/system.rs +++ b/substrate/frame/revive/src/vm/evm/instructions/system.rs @@ -84,8 +84,7 @@ pub fn caller(interpreter: &mut Interpreter) -> ControlFlow { /// Pushes the size of running contract's bytecode onto the stack. pub fn codesize(interpreter: &mut Interpreter) -> ControlFlow { interpreter.ext.charge_or_halt(EVMGas(BASE))?; - let len = in_data_size(interpreter, true); - interpreter.stack.push(len) + interpreter.stack.push(U256::from(interpreter.bytecode.len())) } /// Implements the CODECOPY instruction. @@ -93,7 +92,20 @@ pub fn codesize(interpreter: &mut Interpreter) -> ControlFlow { /// Copies running contract's bytecode to memory. pub fn codecopy(interpreter: &mut Interpreter) -> ControlFlow { let [memory_offset, code_offset, len] = interpreter.stack.popn()?; - in_data_copy(interpreter, memory_offset, code_offset, len, true) + let len = as_usize_or_halt::(len)?; + let Some(memory_offset) = memory_resize(interpreter, memory_offset, len)? else { + return ControlFlow::Continue(()) + }; + let code_offset = as_usize_saturated(code_offset); + + // Note: This can't panic because we resized memory. + interpreter.memory.set_data( + memory_offset, + code_offset, + len, + interpreter.bytecode.bytecode_slice(), + ); + ControlFlow::Continue(()) } /// Implements the CALLDATALOAD instruction. @@ -104,11 +116,7 @@ pub fn calldataload(interpreter: &mut Interpreter) -> ControlFlow(interpreter: &mut Interpreter) -> ControlFlow(interpreter: &mut Interpreter) -> ControlFlow { interpreter.ext.charge_or_halt(EVMGas(BASE))?; - let len = in_data_size(interpreter, false); - interpreter.stack.push(len) + interpreter.stack.push(U256::from(interpreter.input.len())) } /// Implements the CALLVALUE instruction. @@ -141,7 +148,17 @@ pub fn callvalue(interpreter: &mut Interpreter) -> ControlFlow /// Copies input data to memory. pub fn calldatacopy(interpreter: &mut Interpreter) -> ControlFlow { let [memory_offset, data_offset, len] = interpreter.stack.popn()?; - in_data_copy(interpreter, memory_offset, data_offset, len, false) + let len = as_usize_or_halt::(len)?; + + let Some(memory_offset) = memory_resize(interpreter, memory_offset, len)? else { + return ControlFlow::Continue(()); + }; + + let data_offset = as_usize_saturated(data_offset); + + // Note: This can't panic because we resized memory. + interpreter.memory.set_data(memory_offset, data_offset, len, &interpreter.input); + ControlFlow::Continue(()) } /// EIP-211: New opcodes: RETURNDATASIZE and RETURNDATACOPY @@ -190,7 +207,7 @@ pub fn gas(interpreter: &mut Interpreter) -> ControlFlow { /// Common logic for copying data from a source buffer to the EVM's memory. /// /// Handles memory expansion and gas calculation for data copy operations. -fn memory_resize<'a, E: Ext>( +pub fn memory_resize<'a, E: Ext>( interpreter: &mut Interpreter<'a, E>, memory_offset: U256, len: usize, @@ -204,70 +221,3 @@ fn memory_resize<'a, E: Ext>( interpreter.memory.resize(memory_offset, len)?; ControlFlow::Continue(Some(memory_offset)) } - -/// The shared logic for CODECOPY and CALLDATACOPY. -fn in_data_copy( - interpreter: &mut Interpreter, - memory_offset: U256, - data_offset: U256, - len: U256, - is_codecopy: bool, -) -> ControlFlow { - let len = as_usize_or_halt::(len)?; - let Some(memory_offset) = memory_resize(interpreter, memory_offset, len)? else { - return ControlFlow::Continue(()) - }; - let data_offset = as_usize_saturated(data_offset); - let is_constructor = interpreter.ext.entry_point().is_evm_constructor(); - let bytecode = interpreter.bytecode.bytecode_slice(); - let input = interpreter.input.as_slice(); - - struct Write<'a> { - memory_offset: usize, - data_offset: usize, - len: usize, - slice: &'a [u8], - } - - let writes = match (is_constructor, is_codecopy) { - (true, true) => { - let bytes_written = bytecode.len().saturating_sub(data_offset).min(len); - &[ - Write { memory_offset, data_offset, len: bytes_written, slice: bytecode }, - Write { - memory_offset: memory_offset.saturating_add(bytes_written), - data_offset: data_offset.saturating_sub(bytecode.len()), - len: len.saturating_sub(bytes_written), - slice: input, - }, - ][..] - }, - (true, false) => &[Write { memory_offset, data_offset, len, slice: &[] }], - (false, true) => &[Write { memory_offset, data_offset, len, slice: bytecode }], - (false, false) => &[Write { memory_offset, data_offset, len, slice: input }], - }; - - for write in writes.into_iter().filter(|w| w.len != 0) { - // Note: This can't panic because we resized memory. - interpreter - .memory - .set_data(write.memory_offset, write.data_offset, write.len, write.slice); - } - - ControlFlow::Continue(()) -} - -/// The shared logic for CODESIZE and CALLDATASIZE -fn in_data_size(interpreter: &mut Interpreter, is_codecopy: bool) -> U256 { - let bytecode_len = interpreter.bytecode.len(); - let input_len = interpreter.input.len(); - let is_constructor = interpreter.ext.entry_point().is_evm_constructor(); - - match (is_constructor, is_codecopy) { - (true, true) => bytecode_len.saturating_add(input_len), - (true, false) => 0, - (false, true) => bytecode_len, - (false, false) => input_len, - } - .into() -} From 5f80b81e08d3e742d9d6130f0f933c5eb6a86187 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 11 Nov 2025 13:59:29 +0100 Subject: [PATCH 5/7] Ensure that data is empty for evm constructors --- substrate/frame/revive/src/exec.rs | 2 ++ substrate/frame/revive/src/lib.rs | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 4115cd5c6895b..fac111c82779b 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,6 +1834,7 @@ 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())?, diff --git a/substrate/frame/revive/src/lib.rs b/substrate/frame/revive/src/lib.rs index 2e3fee1a89b83..6c090302d6e9b 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -564,7 +564,10 @@ 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, /// Benchmarking only error. #[cfg(feature = "runtime-benchmarks")] BenchmarkingError = 0xFF, @@ -1564,6 +1567,7 @@ 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()) From 80724d96f04a9e380c81b269742bbdca77310429 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Tue, 11 Nov 2025 15:06:57 +0100 Subject: [PATCH 6/7] Prevent EVM construction from hash --- substrate/frame/revive/src/exec.rs | 6 +++++- substrate/frame/revive/src/lib.rs | 12 ++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index fac111c82779b..726f8c07061b8 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1837,7 +1837,11 @@ where 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 6c090302d6e9b..d706fc075afe0 100644 --- a/substrate/frame/revive/src/lib.rs +++ b/substrate/frame/revive/src/lib.rs @@ -568,6 +568,11 @@ pub mod pallet { /// /// 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, @@ -1574,8 +1579,11 @@ impl Pallet { } 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); From 334208f7cfdf1bd6ce0a750769ebab017ac51df2 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 11 Nov 2025 16:56:03 +0000 Subject: [PATCH 7/7] Update from github-actions[bot] running command 'prdoc --audience runtime_dev --audience runtime_user --bump major' --- prdoc/pr_10214.prdoc | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 prdoc/pr_10214.prdoc 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