Feat: Refactore Executor #64
Conversation
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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) { | |||
There was a problem hiding this comment.
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.
| @@ -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>; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. ✊
|
@mrLSD it couldn't be related to the milestone |
Description
Refactoring:
create_innerandgas_limit calculation.