Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Refactore Executor #64

Merged
merged 3 commits into from
Oct 15, 2024
Merged

Feat: Refactore Executor #64

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2d94acc
Refactoring Executor: create_inner and gas_limit calc
mrLSD Oct 12, 2024
7424b3e
Added debug_assert, comments. Changed version to v0.46.0
mrLSD Oct 14, 2024
456d521
Improve gas variable
mrLSD Oct 15, 2024
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
4 changes: 2 additions & 2 deletions runtime/src/eval/system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,8 +448,8 @@ pub fn create<H: Handler>(runtime: &mut Runtime, is_create2: bool, handler: &mut
};

match handler.create(runtime.context.address, scheme, value, code, None) {
Capture::Exit((reason, address, return_data)) => {
match super::finish_create(runtime, reason, address, return_data) {
Copy link
Member

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 can remove the address return here because finish_create pushes the address onto the EVM stack which I think is required for the CREATE instruction.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we can. handler.create operates over create_inner and doesn't return either. In terms of internal refactoring, it's good enough to remove redundant code. There's no case for that logic, and it was added "because I can" without design rethinking. It's good practice to follow Keep It Simple (KIS), simplifying things for better productivity and clear algorithms and code.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. The Capture::Exit case used to always have address: None because it was only returned when the call to create_inner encountered an error and therefore no contract was created. The happy path is returning Capture::Trap and finish_create ends up being called later with the created address as part of the EVM call stack handling logic. Maybe it is worth adding a comment that the Capture::Exit case only happens if there was an error. Maybe also worth adding debug_assert!(!reason.is_succeed()) to make sure this assumption is upheld in the future.

As for removing unused code, I fully agree with doing that to make maintaining the code easier going forward! I just wanted to make sure the code path really was unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, thanks.
In order to get closer to proving correctness, I implemented 100% passing of tests from ethereum/tests and ethereum/execution-spec-tests. And this is a big step forward. For greater persuasiveness, it is necessary to cover the entire code with tests. However, this task will take, in my opinion, a lot of time, since the EVM logic is really complex. This does not mean that it should not be done - it is just a lower priority.

Capture::Exit((reason, return_data)) => {
match super::finish_create(runtime, reason, None, return_data) {
Ok(()) => Control::Continue,
Err(e) => Control::Exit(e),
}
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ pub trait Handler {
value: U256,
init_code: Vec<u8>,
target_gas: Option<u64>,
) -> Capture<(ExitReason, Option<H160>, Vec<u8>), Self::CreateInterrupt>;
) -> Capture<(ExitReason, Vec<u8>), Self::CreateInterrupt>;
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a breaking change from the perspective of sputnikvm as a library since we are changing the signature of a method on a public trait. This is not an issue, especially if we are not planning on ever syncing up with upstream again and we are the only users of the library. But I thought it is worth pointing out just in case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's breaking changes. With the release of Prague, we plan to de-fork into our own library aurora-evm, since upstream has completely abandoned the previous version and moved to the new version v1.0-alpha. Moreover, they do not have any development at all for a long time. At the moment, our fork is so different that it is ALREADY practically an independent library. ✊

/// Feed in create feedback.
///
/// # Errors
Expand Down
159 changes: 70 additions & 89 deletions src/executor/stack/executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,21 @@ macro_rules! emit_exit {
(reason, return_value)
}};
}
macro_rules! try_or_fail {
( $e:expr ) => {
match $e {
Ok(v) => v,
Err(e) => return Capture::Exit((e.into(), Vec::new())),
}
};
}

const DEFAULT_CALL_STACK_CAPACITY: usize = 4;

const fn l64(gas: u64) -> u64 {
gas - gas / 64
}

pub enum StackExitKind {
Succeeded,
Reverted,
Expand Down Expand Up @@ -542,7 +554,7 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
Some(gas_limit),
false,
) {
Capture::Exit((s, _, v)) => emit_exit!(s, v),
Capture::Exit((s, v)) => emit_exit!(s, v),
Capture::Trap(rt) => {
let mut cs = Vec::with_capacity(DEFAULT_CALL_STACK_CAPACITY);
cs.push(rt.0);
Expand Down Expand Up @@ -649,7 +661,7 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
Some(gas_limit),
false,
) {
Capture::Exit((s, _, v)) => emit_exit!(s, v),
Capture::Exit((s, v)) => emit_exit!(s, v),
Capture::Trap(rt) => {
let mut cs = Vec::with_capacity(DEFAULT_CALL_STACK_CAPACITY);
cs.push(rt.0);
Expand Down Expand Up @@ -821,6 +833,30 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
}
}

/// Calculate gas limit and record it in the gasometer.
fn calc_gas_limit_and_record(
&mut self,
target_gas: Option<u64>,
take_l64: bool,
) -> Result<u64, ExitError> {
let after_gas = if take_l64 && self.config.call_l64_after_gas {
mrLSD marked this conversation as resolved.
Show resolved Hide resolved
if self.config.estimate {
let initial_after_gas = self.state.metadata().gasometer.gas();
let diff = initial_after_gas - l64(initial_after_gas);
self.state.metadata_mut().gasometer.record_cost(diff)?;
self.state.metadata().gasometer.gas()
} else {
l64(self.state.metadata().gasometer.gas())
}
} else {
self.state.metadata().gasometer.gas()
};
let target_gas = target_gas.unwrap_or(after_gas);
let gas_limit = min(target_gas, after_gas);
self.state.metadata_mut().gasometer.record_cost(gas_limit)?;
Ok(gas_limit)
}

fn create_inner(
&mut self,
caller: H160,
Expand All @@ -829,26 +865,13 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
init_code: Vec<u8>,
target_gas: Option<u64>,
take_l64: bool,
) -> Capture<(ExitReason, Option<H160>, Vec<u8>), StackExecutorCreateInterrupt<'static>> {
const fn l64(gas: u64) -> u64 {
gas - gas / 64
}

) -> Capture<(ExitReason, Vec<u8>), StackExecutorCreateInterrupt<'static>> {
if self.nonce(caller) >= U64_MAX {
return Capture::Exit((ExitError::MaxNonce.into(), None, Vec::new()));
}

macro_rules! try_or_fail {
( $e:expr ) => {
match $e {
Ok(v) => v,
Err(e) => return Capture::Exit((e.into(), None, Vec::new())),
}
};
return Capture::Exit((ExitError::MaxNonce.into(), Vec::new()));
}

// Warm address for EIP-2929
let address = self.create_address(scheme);

self.state
.metadata_mut()
.access_addresses([caller, address].iter().copied());
Expand All @@ -867,49 +890,34 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
// early to verify exceeding Stack limit. It allows avoid
// issue with wrong detection `CallTooDeep` for Create.
if depth + 1 > self.config.call_stack_limit {
return Capture::Exit((ExitError::CallTooDeep.into(), None, Vec::new()));
return Capture::Exit((ExitError::CallTooDeep.into(), Vec::new()));
}
}

// Check is transfer value is enough
if self.balance(caller) < value {
return Capture::Exit((ExitError::OutOfFund.into(), None, Vec::new()));
return Capture::Exit((ExitError::OutOfFund.into(), Vec::new()));
}

let after_gas = if take_l64 && self.config.call_l64_after_gas {
if self.config.estimate {
let initial_after_gas = self.state.metadata().gasometer.gas();
let diff = initial_after_gas - l64(initial_after_gas);
try_or_fail!(self.state.metadata_mut().gasometer.record_cost(diff));
self.state.metadata().gasometer.gas()
} else {
l64(self.state.metadata().gasometer.gas())
}
} else {
self.state.metadata().gasometer.gas()
};
let gas_limit = try_or_fail!(self.calc_gas_limit_and_record(target_gas, take_l64));

let target_gas = target_gas.unwrap_or(after_gas);

let gas_limit = min(after_gas, target_gas);
try_or_fail!(self.state.metadata_mut().gasometer.record_cost(gas_limit));
// Check nonce and increment it for caller
try_or_fail!(self.state.inc_nonce(caller));

if let Err(e) = self.state.inc_nonce(caller) {
return Capture::Exit((e.into(), None, Vec::new()));
// Check create collision: EIP-7610
if self.is_create_collision(address) {
return Capture::Exit((ExitError::CreateCollision.into(), Vec::new()));
}

// Enter to execution substate
self.enter_substate(gas_limit, false);

// Check create collision: EIP-7610
if self.is_create_collision(address) {
let _ = self.exit_substate(&StackExitKind::Failed);
return Capture::Exit((ExitError::CreateCollision.into(), None, Vec::new()));
// Check nonce and increment it for created address after entering substate
if self.config.create_increase_nonce {
try_or_fail!(self.state.inc_nonce(address));
}

let context = Context {
address,
caller,
apparent_value: value,
};
// Transfer funds if needed
let transfer = Transfer {
source: caller,
target: address,
Expand All @@ -919,19 +927,19 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
Ok(()) => (),
Err(e) => {
let _ = self.exit_substate(&StackExitKind::Reverted);
return Capture::Exit((ExitReason::Error(e), None, Vec::new()));
return Capture::Exit((ExitReason::Error(e), Vec::new()));
}
}
// It needed for CANCUN hard fork EIP-6780 we should mark account as created
// to handle SELFDESTRUCT in the same transaction
self.state.set_created(address);

if self.config.create_increase_nonce {
if let Err(e) = self.state.inc_nonce(address) {
return Capture::Exit((e.into(), None, Vec::new()));
}
}

// Init EVM runtime in Context
let context = Context {
address,
caller,
apparent_value: value,
};
let runtime = Runtime::new(
Rc::new(init_code),
Rc::new(Vec::new()),
Expand All @@ -940,6 +948,7 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
self.config.memory_limit,
);

// Set Runtime kind with pre-init Runtime and return Trap, that mean continue execution
Capture::Trap(StackExecutorCreateInterrupt(TaggedRuntime {
kind: RuntimeKind::Create(address),
inner: MaybeBorrowed::Owned(runtime),
Expand All @@ -958,19 +967,6 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
take_stipend: bool,
context: Context,
) -> Capture<(ExitReason, Vec<u8>), StackExecutorCallInterrupt<'static>> {
macro_rules! try_or_fail {
( $e:expr ) => {
match $e {
Ok(v) => v,
Err(e) => return Capture::Exit((e.into(), Vec::new())),
}
};
}

const fn l64(gas: u64) -> u64 {
gas - gas / 64
}

event!(Call {
code_address,
transfer: &transfer,
Expand All @@ -980,23 +976,7 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
context: &context,
});

let after_gas = if take_l64 && self.config.call_l64_after_gas {
if self.config.estimate {
let initial_after_gas = self.state.metadata().gasometer.gas();
let diff = initial_after_gas - l64(initial_after_gas);
try_or_fail!(self.state.metadata_mut().gasometer.record_cost(diff));
self.state.metadata().gasometer.gas()
} else {
l64(self.state.metadata().gasometer.gas())
}
} else {
self.state.metadata().gasometer.gas()
};

let target_gas = target_gas.unwrap_or(after_gas);
let mut gas_limit = min(target_gas, after_gas);

try_or_fail!(self.state.metadata_mut().gasometer.record_cost(gas_limit));
let mut gas_limit = try_or_fail!(self.calc_gas_limit_and_record(target_gas, take_l64));

if let Some(transfer) = transfer.as_ref() {
if take_stipend && transfer.value != U256::zero() {
Expand All @@ -1016,6 +996,7 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet>
}
}

// Transfer funds if needed
if let Some(transfer) = transfer {
match self.state.transfer(transfer) {
Ok(()) => (),
Expand Down Expand Up @@ -1439,11 +1420,11 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet> Handler
value: U256,
init_code: Vec<u8>,
target_gas: Option<u64>,
) -> Capture<(ExitReason, Option<H160>, Vec<u8>), Self::CreateInterrupt> {
) -> Capture<(ExitReason, Vec<u8>), Self::CreateInterrupt> {
if let Err(e) = self.maybe_record_init_code_cost(&init_code) {
let reason: ExitReason = e.into();
emit_exit!(reason.clone());
return Capture::Exit((reason, None, Vec::new()));
return Capture::Exit((reason, Vec::new()));
}
self.create_inner(caller, scheme, value, init_code, target_gas, true)
}
Expand All @@ -1456,16 +1437,16 @@ impl<'config, 'precompiles, S: StackState<'config>, P: PrecompileSet> Handler
value: U256,
init_code: Vec<u8>,
target_gas: Option<u64>,
) -> Capture<(ExitReason, Option<H160>, Vec<u8>), Self::CreateInterrupt> {
) -> Capture<(ExitReason, Vec<u8>), Self::CreateInterrupt> {
if let Err(e) = self.maybe_record_init_code_cost(&init_code) {
let reason: ExitReason = e.into();
emit_exit!(reason.clone());
return Capture::Exit((reason, None, Vec::new()));
return Capture::Exit((reason, Vec::new()));
}

let capture = self.create_inner(caller, scheme, value, init_code, target_gas, true);

if let Capture::Exit((ref reason, _, ref return_value)) = capture {
if let Capture::Exit((ref reason, ref return_value)) = capture {
emit_exit!(reason, return_value);
}

Expand Down
Loading