Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions substrate/frame/revive/fixtures/contracts/System.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
7 changes: 7 additions & 0 deletions substrate/frame/revive/src/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`].
Expand Down Expand Up @@ -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.
Expand Down
9 changes: 8 additions & 1 deletion substrate/frame/revive/src/exec/mock_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -239,6 +242,10 @@ impl<T: Config> PrecompileExt for MockExt<T> {
}

fn charge_storage(&mut self, _diff: &Diff) {}

fn entry_point(&self) -> ExportedFunction {
panic!("MockExt::entry_point")
}
}

impl<T: Config> PrecompileWithInfoExt for MockExt<T> {
Expand Down
76 changes: 53 additions & 23 deletions substrate/frame/revive/src/tests/sol/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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))
.data(SystemFixture::constructorCall { panic: false }.abi_encode())
.build_and_unwrap_contract();

let pre = b"revive";
let expected = keccak_256(pre);
Expand All @@ -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))
.data(SystemFixture::constructorCall { panic: false }.abi_encode())
.build_and_unwrap_contract();

let result = builder::bare_call(addr)
.data(SystemFixture::addressFuncCall {}.abi_encode())
Expand All @@ -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))
.data(SystemFixture::constructorCall { panic: false }.abi_encode())
.build_and_unwrap_contract();

let result = builder::bare_call(addr)
.data(SystemFixture::callerCall {}.abi_encode())
Expand All @@ -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))
.data(SystemFixture::constructorCall { panic: false }.abi_encode())
.build_and_unwrap_contract();

let value = 1337u64;

Expand All @@ -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))
.data(SystemFixture::constructorCall { panic: false }.abi_encode())
.build_and_unwrap_contract();

let result = builder::bare_call(addr)
.data(SystemFixture::calldataloadCall { offset: 4u64 }.abi_encode())
Expand All @@ -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))
.data(SystemFixture::constructorCall { panic: false }.abi_encode())
.build_and_unwrap_contract();

// calldata = selector + encoded argument
let result = builder::bare_call(addr)
Expand All @@ -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))
.data(SystemFixture::constructorCall { panic: false }.abi_encode())
.build_and_unwrap_contract();

let call_data = SystemFixture::calldatacopyCall {
destOffset: 0u64, // unused
Expand Down Expand Up @@ -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()))
.data(SystemFixture::constructorCall { panic: false }.abi_encode())
.build_and_unwrap_contract();

let result = builder::bare_call(addr)
.data(SystemFixture::codesizeCall {}.abi_encode())
Expand All @@ -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()))
.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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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))
.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());
});
}
8 changes: 7 additions & 1 deletion substrate/frame/revive/src/vm/evm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -140,3 +140,9 @@ fn run_plain<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Halt, Infa
exec_instruction(interpreter, opcode)?;
}
}

impl ExportedFunction {
fn is_evm_constructor(&self) -> bool {
matches!(self, ExportedFunction::Constructor)
}
}
108 changes: 79 additions & 29 deletions substrate/frame/revive/src/vm/evm/instructions/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,28 +84,16 @@ pub fn caller<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Halt> {
/// Pushes the size of running contract's bytecode onto the stack.
pub fn codesize<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Halt> {
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.
///
/// Copies running contract's bytecode to memory.
pub fn codecopy<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Halt> {
let [memory_offset, code_offset, len] = interpreter.stack.popn()?;
let len = as_usize_or_halt::<E::T>(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.
Expand All @@ -116,7 +104,11 @@ pub fn calldataload<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Hal
let ([], offset_ptr) = interpreter.stack.popn_top()?;
let mut word = [0u8; 32];
let offset = as_usize_saturated(*offset_ptr);
let input = &interpreter.input;
let is_constructor = interpreter.ext.entry_point().is_evm_constructor();
let input = interpreter.input.as_slice();

let input = if is_constructor { &[] } else { input };

let input_len = input.len();
if offset < input_len {
let count = 32.min(input_len - offset);
Expand All @@ -131,7 +123,8 @@ pub fn calldataload<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Hal
/// Pushes the size of input data onto the stack.
pub fn calldatasize<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Halt> {
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.
Expand All @@ -148,17 +141,7 @@ pub fn callvalue<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Halt>
/// Copies input data to memory.
pub fn calldatacopy<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Halt> {
let [memory_offset, data_offset, len] = interpreter.stack.popn()?;
let len = as_usize_or_halt::<E::T>(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
Expand Down Expand Up @@ -207,7 +190,7 @@ pub fn gas<E: Ext>(interpreter: &mut Interpreter<E>) -> ControlFlow<Halt> {
/// 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,
Expand All @@ -221,3 +204,70 @@ 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<E: Ext>(
interpreter: &mut Interpreter<E>,
memory_offset: U256,
data_offset: U256,
len: U256,
is_codecopy: bool,
) -> ControlFlow<Halt> {
let len = as_usize_or_halt::<E::T>(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();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fix itself is valid. But it seems odd that we need distinguished logic here first hand. I think the call data and code buffers should be set up properly during frame construction instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to do any of this
the test breaks down because you call bare_instantiate in these test with code and data, when in reallity everything is passed through the code.

as stated above, I believe we should return an Err when calling bae_instantiate with evm code and Some(data)

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<E: Ext>(interpreter: &mut Interpreter<E>, 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()
}
Loading