-
Notifications
You must be signed in to change notification settings - Fork 62
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: improve UsedStateEVMInspector by adding more op-codes #161
Conversation
@nikoseven nice, thank you. I'll look into it soon. |
Hi! |
Fixed the typo. Thanks! |
Benchmark results for
|
Date (UTC) | 2024-09-14T20:14:26+00:00 |
Commit | 3e21dd2c7d9a828985e67641107cedefd9f50eb8 |
Base SHA | a00be29a28181baff42058fd1b1146a825566d2c |
Significant changes
Benchmark | Mean | Status |
---|---|---|
MEV-Boost SubmitBlock serialization/JSON encoding | 46.88% | Performance has degraded. |
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 don't see any issue. It looks great. One thing though, we updated reth and revm so can you please rebase.
re additonal allocations / performance: I don't think that this changes the pictrure in any way
- we were already instrumenting step
- this adds call/call_end instrumentation that runs very rarely
- we were already allocating hashmap for each slot and here it allocates a tiny bit of data that is much smaller than that (the order is just the number of internal calls)
…t_amount, created_contracts, destructed_contracts. Add test for evm inspector.
e83992c
to
3e21dd2
Compare
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.
There is a small issue that would make balance part of the trace incorrect. I don't think that it must be fixed for this to be merged because our slot logic was not handling reverts anyway.
} | ||
|
||
fn selfdestruct(&mut self, contract: Address, target: Address, value: U256) { | ||
self.used_state_trace.destructed_contracts.push(contract); |
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 check if contract was already destroyed and return if yes. Selfdestruct can be called multiple times in one tx.
if let Some(transfer_value) = inputs.transfer_value() { | ||
if !transfer_value.is_zero() { | ||
*self | ||
.used_state_trace |
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 would be incorrect if call fails.
## 📝 Summary Added support for SELFBALANCE, BALANCE, CALL, CREATE, and SELFDESTRUCT op-codes in UsedStateEVMInspector. New test cases for evm inspector have been added. ## 💡 Motivation and Context Related discussion in #58. For opcodes like EXTCODECOPY, EXTCODEHASH, and EXTCODESIZE, I thought we could add them in the future with a concrete use case, so that we can properly design the datastruct for it. --- ## ✅ I have completed the following steps: * [x] Run `make lint` * [x] Run `make test` * [x] Added tests (if applicable)
Adds comments from here #161
📝 Summary
Added support for SELFBALANCE, BALANCE, CALL, CREATE, and SELFDESTRUCT op-codes in UsedStateEVMInspector.
New test cases for evm inspector have been added.
💡 Motivation and Context
Related discussion in #58.
For opcodes like EXTCODECOPY, EXTCODEHASH, and EXTCODESIZE, I thought we could add them in the future with a concrete use case, so that we can properly design the datastruct for it.
✅ I have completed the following steps:
make lint
make test