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

Jump to bottom

fix!: Remove EVM emulator gas stipend #82

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix!: Remove EVM emulator gas stipend #82

wants to merge 4 commits into from
+17 −197

Conversation

slowli
Copy link
Contributor

@slowli slowli commented Jan 24, 2025 •
edited
Loading

What ❔

Removes the EVM emulator gas stipend and replaces it with a memory stipend.

Why ❔

Aligns with the changes in the legacy VM.

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Documentation comments have been added / updated.
  • Code has been formatted via cargo fmt and cargo clippy.

@slowli slowli marked this pull request as ready for review January 24, 2025 12:23
@slowli slowli requested a review from joonazan as a code owner January 24, 2025 12:23
@slowli
Copy link
Contributor Author

slowli commented Jan 24, 2025

  • I've tested this code on multivm unit tests (with the legacy VM revision updated, of course), so there's some insurance that it works correctly. OTOH, I had to remove the stipend test because it no longer made sense. Would it be possible to replace it somehow?
  • Breaking changes could theoretically be removed / moved to another PR.

joonazan
joonazan requested changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@joonazan joonazan left a comment

Choose a reason for hiding this comment

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

Good apart from a few little things.
Removing the test is fine because the system replacing stipends isn't nearly as dangerous.

crates/vm2/src/single_instruction_test/callframe.rs Outdated Show resolved Hide resolved
crates/vm2/src/single_instruction_test/state_to_zk_evm.rs
@@ -86,7 +86,7 @@ fn vm2_frame_to_zk_evm_frames<T, W>(
heap_bound: frame.heap_size,
aux_heap_bound: frame.aux_heap_size,
total_pubdata_spent: PubdataCost(0),
stipend: frame.stipend,
Copy link
Contributor

Choose a reason for hiding this comment

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

They forgot to remove the field? Or does the zk_evm dependency need updating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've postponed updating the zk_evm dependency until it's released, yes.

crates/vm2/src/tracing.rs Outdated Show resolved Hide resolved
@slowli slowli force-pushed the aov-pla-1084-remove-evm-gas-stipend branch from c168fdb to 194763b Compare January 27, 2025 10:56
@slowli slowli requested a review from joonazan January 27, 2025 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joonazan joonazan Awaiting requested review from joonazan joonazan is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@slowli @joonazan