-
Notifications
You must be signed in to change notification settings - Fork 41
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.
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(levm): add hooks support and implement DefaultHook
#1869
Conversation
|
Benchmark Results ComparisonPR ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
Main ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
|
Notes/suggestion (simply ideas):
|
|
7e1c913
to
ec41b03
Compare
DefaultHook::prepare_execution
crates/vm/levm/src/vm.rs
Outdated
pub fn new(env: Environment, db: Arc<dyn Database>, cache: CacheDB) -> Result<Self, VMError> { | ||
let default_hook: Box<dyn Hook> = Box::new(DefaultHook::new()); | ||
let hooks = vec![default_hook]; | ||
Ok(Self { | ||
db, | ||
env, | ||
cache, | ||
hooks, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the checks are made in the Environment, should we change the new
to not return a Result type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are totally right Tomi!
Updated here: 8e28a39
crates/vm/levm/src/environment.rs
Outdated
db: Arc<dyn Database>, | ||
value: U256, | ||
) -> Result<Self, VMError> { | ||
let to = TxKind::Call(Address::from_low_u64_be(42)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a fix number? Maybe defining a constant should be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that in tests.rs
42 was used everywhere. I don't know why 42 specifically, although I have my theories.
However, that can certainly lead to unwanted bugs. I'll change that so that it receives the to as an argument and I'll also add a comment indicating that it is only intended for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed here: f9f7a17
crates/vm/levm/src/environment.rs
Outdated
cache: &mut CacheDB, | ||
db: Arc<dyn Database>, | ||
value: U256, | ||
calldata: Bytes, | ||
origin: Address, | ||
tx_kind: TxKind, | ||
refunded_gas: u64, | ||
gas_limit: u64, | ||
config: EVMConfig, | ||
block_number: U256, | ||
coinbase: Address, | ||
timestamp: U256, | ||
prev_randao: Option<H256>, | ||
chain_id: U256, | ||
base_fee_per_gas: U256, | ||
gas_price: U256, // Effective gas price | ||
block_excess_blob_gas: Option<U256>, | ||
block_blob_gas_used: Option<U256>, | ||
tx_blob_hashes: Vec<H256>, | ||
tx_max_priority_fee_per_gas: Option<U256>, | ||
tx_max_fee_per_gas: Option<U256>, | ||
tx_max_fee_per_blob_gas: Option<U256>, | ||
tx_nonce: u64, | ||
block_gas_limit: u64, | ||
transient_storage: TransientStorage, | ||
access_list: AccessList, | ||
authorization_list: Option<AuthorizationList>, | ||
call_frames: Vec<CallFrame>, | ||
) -> Result<Environment, VMError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe using a struct Transaction containing all the transaction related fields should be better. All this values shouldn't change in the execution.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are totally right! As a matter of fact, I wanted to do that exact same thing (see comment: #1869 (comment)).
I think I simply forgot to do it, I was a big PR as it is. I'll change it.
Thank you Tomi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a struct is needed, if we can use the GenericTransaction
from ethrex/core
it would be awesome. However, if we have to implement a new struct only to be used by levm
or if he integration with GenericTransaction
isn't that obvious, we could create an issue and do it in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing!! left 2 minor suggestions
crates/vm/levm/src/vm.rs
Outdated
pub fn new(env: Environment, db: Arc<dyn Database>, cache: CacheDB) -> Self { | ||
let default_hook: Box<dyn Hook> = Box::new(DefaultHook::new()); | ||
let hooks = vec![default_hook]; | ||
Self { | ||
db, | ||
env, | ||
cache, | ||
hooks, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, in the future, a method for the VM called append_hook
could be handy.
So we can do something like:
VM::new(db, env, cache).append_hook(custom_logic_hook)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left some comments
f9f7a17
to
0279459
Compare
What do you think of the new directory structure? |
// NOTE: I'm leaving this in case there needs to be a custom new() | ||
// function when the post_execution_changes methods is | ||
// implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it makes a lot of sense. Either we need a custom new
that will take parameters and we'll need to make changes anyway, or we don't need it and we just have a redundant way to construct this.
Also, whenever we leave some constructor method "just in case", I recommend we go for a builder pattern, which is much better for future-proofing an interface than trying to predict the parameters a constructor might have in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point @Oppen; I'll remove it ASAP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crates/vm/levm/src/vm.rs
Outdated
@@ -166,6 +168,7 @@ pub struct VM { | |||
pub tx_kind: TxKind, | |||
pub access_list: AccessList, | |||
pub authorization_list: Option<AuthorizationList>, | |||
pub hooks: Vec<Rc<dyn Hook>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be Arc
. Otherwise, db
being Arc
would be pointless, given this makes the whole thing not Send
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted! Thanks for the feedback.
Changed here: aa1c1b8
crates/vm/levm/src/vm.rs
Outdated
// NOTE: ATTOW the default hook is created in VM::new(), so | ||
// (in theory) _at least_ the default prepare execution should | ||
// run | ||
for hook in self.hooks.clone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be enough:
for hook in self.hooks.clone() { | |
for hook in self.hooks.iter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm getting the following error:
402 | ...or hook in self.hooks.iter() {
| -----------------
| |
| immutable borrow occurs here
| immutable borrow later used here
403 | ... hook.prepare_execution(self, &mut initial_call_frame...
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
crates/vm/levm/src/vm.rs
Outdated
@@ -822,7 +428,9 @@ impl VM { | |||
|
|||
report.gas_used = self.gas_used(&initial_call_frame, &report)?; | |||
|
|||
self.finalize_execution(&initial_call_frame, &mut report)?; | |||
for hook in self.hooks.clone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same:
for hook in self.hooks.clone() { | |
for hook in self.hooks.iter() { |
crates/vm/levm/src/vm.rs
Outdated
@@ -928,119 +522,11 @@ impl VM { | |||
output: Bytes::new(), | |||
}; | |||
|
|||
self.finalize_execution(initial_call_frame, &mut report)?; | |||
for hook in self.hooks.clone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for hook in self.hooks.clone() { | |
for hook in self.hooks.iter() { |
crates/vm/levm/src/vm.rs
Outdated
@@ -166,6 +168,7 @@ pub struct VM { | |||
pub tx_kind: TxKind, | |||
pub access_list: AccessList, | |||
pub authorization_list: Option<AuthorizationList>, | |||
pub hooks: Vec<Rc<dyn Hook>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the above.
crates/vm/levm/src/vm.rs
Outdated
pub fn execute(&mut self) -> Result<ExecutionReport, VMError> { | ||
let mut initial_call_frame = self | ||
.call_frames | ||
.pop() | ||
.ok_or(VMError::Internal(InternalError::CouldNotPopCallframe))?; | ||
|
||
self.prepare_execution(&mut initial_call_frame)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this function, as it serves to keep the VM's main function very easy to understand high-level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted! Great point. I'll change it ASAP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed here: 62261a6
crates/vm/levm/src/vm.rs
Outdated
@@ -822,7 +428,9 @@ impl VM { | |||
|
|||
report.gas_used = self.gas_used(&initial_call_frame, &report)?; | |||
|
|||
self.finalize_execution(&initial_call_frame, &mut report)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
@@ -928,119 +522,11 @@ impl VM { | |||
output: Bytes::new(), | |||
}; | |||
|
|||
self.finalize_execution(initial_call_frame, &mut report)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
ed9a8cd
to
b41464c
Compare
…he hook logic inside
b41464c
to
62261a6
Compare
I reverted the last 3 commits because the Hive tests were failing, I want to check which commit broke it. |
|
This reverts commit 2245a96.
DefaultHook::prepare_execution
DefaultHook
Motivation
LEVM neeeds to support a L2 feature called "Privilege Transactions". To achieve this, some functionality of the EVM has to be able to be customized. In order to achieve this, the EVM will start to support "Hooks" which will modify the
prepare_execution
andfinalize_execution
functions of the EVM.Description
This PR implements the
DefaultHook
, which is a hook that maintains backwards compatibility with LEVM's previous way of operation; whilst allowing the introduction of future alternative hooks.In order to achieve this, the following changes were made:
eip7702_set_access_code
to a standalone function.access_account
to an standalone function.add_intrinsic_gas
to a standalone function .beacon_root_contract_call_levm
receive aArc<dyn LevmDatabase>
instead of aArc<StoreWrapper>
to make it more likeexecute_tx_levm
.&Arch
with simplyArc
inutils.rs
.DefaultHook::prepare_execution
.DefaultHook::finalize_execution
.Closes #1879
Closes #1880
Closes #1629