Skip to content
This repository has been archived by the owner on Oct 2, 2024. It is now read-only.

Fix eip1153_tstore from EF #428

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Fix eip1153_tstore from EF #428

wants to merge 15 commits into from

Conversation

iinaki
Copy link
Contributor

@iinaki iinaki commented Sep 17, 2024

closes lambdaclass/ethereum_rust#621

Changes

  • Moved the transient storage handling to the Journal.
  • Fixed the EF EIP-1153 tests, which checked the how the transient storage was being handled. To do this I updated the codegen_tstore and the codegen_tload, and their corresponding syscalls.

During testing I noticed that the changes made to the handle of the transient storage were positive and that the reason of the failing of the tests relies on MLIR taking up too much stack space, which causes to violently overflow at a given point.
The test passes if run with RUST_MIN_STACK=83886080 make test-eth. This means that our implementation of tstore and tload is correct. I propose to keep skipping the test and leave the problem of optimizing memory usage with MLIR for further on, while merging this PR as the changes proposed are useful I believe.

Copy link

github-actions bot commented Sep 17, 2024

Benchmarking results

Benchmark for program factorial

Open benchmarks
Command Mean [s] Min [s] Max [s] Relative
evm_mlir_factorial 2.391 ± 0.275 2.068 2.608 1.00
revm_factorial 7.810 ± 1.288 6.951 10.264 3.27 ± 0.66

Benchmark for program fibonacci

Open benchmarks
Command Mean [s] Min [s] Max [s] Relative
evm_mlir_fibonacci 2.181 ± 0.279 1.914 2.448 1.00
revm_fibonacci 7.834 ± 0.789 7.430 9.401 3.59 ± 0.58

src/syscall.rs Outdated
};

if gas_refund > 0 {
self.inner_context.gas_refund += gas_refund as u64;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should TSTORE refund gas? Here in EVM Codes the only opcode that seems to trigger refunds is SSTORE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I believed it did refund gas but it doesn't. Here in the EIP says so too. Thanks for finding that out!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(evm-mlir): Fix eip1153_tstore from EF
2 participants