fix(levm): change gas type from U256 to u64 #1528

lima-limon-inc · 2024-12-18T16:54:21Z

Motivation

The execution client uses u64 for measuring gas, while LEVM uses U256. This PR makes LEVM also use u64 for gas

Description

Closes #1499

…s_type

crates/vm/levm/src/gas_cost.rs

lima-limon-inc · 2024-12-19T13:24:14Z

We'll see when it finishes running, but in theory all the uses of gas have been changed to u64.

The files that relate to parsing the tests and running the benchmarks remain in U256, simply to maintain how the parsing works. Those values are cast to u64 when the tests run.

lima-limon-inc · 2024-12-19T13:26:32Z

~~PS: Note to reviewers~~

I used the "VeryLargeNumber" when a function couldn't cast a value from U256 u64. Should I return a different error?
What error should it be?

Changed here: 7b3bccc

damiramirez

LGTM

crates/vm/levm/src/gas_cost.rs

JereSalo

Great job! There was a lot of effort in this

JereSalo · 2024-12-19T16:25:43Z

crates/vm/levm/src/opcode_handlers/system.rs

+            .stack
+            .pop()?
+            .try_into()
+            .map_err(|_| VMError::Internal(InternalError::ConversionError))?;


This shouldn't be internal

JereSalo · 2024-12-19T16:25:51Z

crates/vm/levm/src/opcode_handlers/system.rs

+            .stack
+            .pop()?
+            .try_into()
+            .map_err(|_| VMError::Internal(InternalError::ConversionError))?;


JereSalo · 2024-12-19T16:26:16Z

crates/vm/levm/src/opcode_handlers/system.rs

+            .stack
+            .pop()?
+            .try_into()
+            .map_err(|_| VMError::Internal(InternalError::ConversionError))?;


JereSalo · 2024-12-19T16:26:26Z

crates/vm/levm/src/opcode_handlers/system.rs

+            .stack
+            .pop()?
+            .try_into()
+            .map_err(|_| VMError::Internal(InternalError::ConversionError))?;


Suggested-by: Jeremias Salomon <jeremias.salomon@lambdaclass.com>

lima-limon-inc · 2024-12-20T17:50:58Z

I left some suggestions. If possible re-review the PR yourself and see if you find anything out of place. I think other than the things I mentioned it looks good

Hello Jere, thank you very much for your feedback. I incorporated your suggestions and re-reviewed the PR. Let me know if you find anything else that need correction.

PS: I also found some other places (ef21d78) where I had created a variable where there wasn't any before. I rolled back those changes so that is more aligned with the previous style.

LMKWYT!

JereSalo · 2024-12-20T18:21:19Z

crates/vm/levm/src/vm.rs

-    fn get_max_blob_gas_cost(&self) -> Result<U256, VMError> {
-        let blob_gas_used = U256::from(self.env.tx_blob_hashes.len())
+    fn get_max_blob_gas_price(&self) -> Result<U256, VMError> {
+        let blob_gas_used: u64 = self


I know you rewrite this variable but this is not blob_gas_used

Noted, I changed it to blob_gas_amout (e0a5eec), what do you think?

Well, it is actually not blob_gas_amount, in that case it is just the length of the blob hash list! Not related to gas

Sorry Jere, it was reading the variable name wrong in my head. It's been a rough day! Haha,

Changed here: 341ca8d

JereSalo · 2024-12-20T18:21:49Z

crates/vm/levm/src/vm.rs

-    fn get_blob_gas_cost(&self) -> Result<U256, VMError> {
-        let blob_gas_used = U256::from(self.env.tx_blob_hashes.len())
+    fn get_blob_gas_price(&self) -> Result<U256, VMError> {
+        let blob_gas_used: u64 = self


ditto ditto: e235e99

JereSalo · 2024-12-20T18:24:12Z

crates/vm/levm/src/vm.rs

    }

-    pub fn get_base_fee_per_blob_gas(&self) -> Result<U256, VMError> {
+    pub fn get_base_fee_per_blob_gas(&self) -> Result<u64, VMError> {


this sounds like it should be an U256 because it is a fee. I am not 100% sure but I think it should because in the validations you are comparing it against tx_max_fee_per_blob_gas

Hello Jere, this was changed here: 96b9bf5

I added an additional let binding (let blob_gas_price: U256 = blob_gas_price.into();, 96b9bf5#diff-e20d30a6efb08393eee586d44f3bc4a8df6cbd53ec8ea5bb4d5def88c9487de1R655) simply out of necessity. It was the cleanest way I could think of to cast the value to U256.

Suggested-by: Jeremias Salomon <jeremias.salomon@lambdaclass.com>

This reverts commit e235e99.

This reverts commit e0a5eec.

JereSalo

nice! sorry if I was pretty annoying with the suggestions. good work

lima-limon-inc · 2024-12-20T20:13:22Z

nice! sorry if I was pretty annoying with the suggestions. good work

Please Jere, no need to apologize! On the contrary, I not only value your diligence, I also admire it!

I have never taken any offense from your comments. Thank you very much for all your dedication!

😊

A small step for man, a large leap for mankind

fa31055

lima-limon-inc changed the title ~~A small step for man, a large leap for mankind~~ fix(levm): change gas type from U256 to u64 Dec 18, 2024

lima-limon-inc added 8 commits December 18, 2024 14:01

Change numeric values

7b7a8d8

Change other numeric values

e41f6b7

Make functions return u64 instead of U256

e5edf9a

WIP: Gradual changes towards U256 -> u64

0fce271

WIP: Change gas from U256 to u64

09178aa

IN THEORY, only one bit is missing

42c9025

Changed the types? Is it done? It appears to be done

c4b67cf

Fix test parsing using new u64 values

996d197

lima-limon-inc self-assigned this Dec 18, 2024

lima-limon-inc added 5 commits December 19, 2024 09:29

Fix lint errors

c0735b7

Fix linter errors

3f1aa51

Merge branch 'main' of github.com:lambdaclass/ethrex into fix/levm/ga…

510af03

…s_type

Lint error

7622e5d

Fix linter errors

6832786

lima-limon-inc commented Dec 19, 2024

View reviewed changes

crates/vm/levm/src/gas_cost.rs Outdated Show resolved Hide resolved

lima-limon-inc commented Dec 19, 2024

View reviewed changes

crates/vm/levm/src/gas_cost.rs Outdated Show resolved Hide resolved

Change comments

9403137

Change from VeryLargeNumber to InternalError

7b3bccc

lima-limon-inc marked this pull request as ready for review December 19, 2024 13:44

lima-limon-inc requested a review from a team as a code owner December 19, 2024 13:44

damiramirez approved these changes Dec 19, 2024

View reviewed changes

Merge branch 'main' into fix/levm/gas_type

80a253d

lima-limon-inc marked this pull request as draft December 19, 2024 15:40

JereSalo reviewed Dec 19, 2024

View reviewed changes

crates/vm/levm/src/gas_cost.rs Show resolved Hide resolved

Merge branch 'main' into fix/levm/gas_type

84c7b4f

JereSalo requested changes Dec 19, 2024

View reviewed changes

Spelling mistake

a6592d7

lima-limon-inc force-pushed the fix/levm/gas_type branch from bab45f2 to a6592d7 Compare December 20, 2024 17:17

lima-limon-inc added 6 commits December 20, 2024 14:18

Remove additional variable

1e5bf00

Rollback error type

17e8c68

Remove gas_limit variable and use from(u64) directly

3f7852c

Suggested-by: Jeremias Salomon <jeremias.salomon@lambdaclass.com>

Merge branch 'main' into fix/levm/gas_type

a0663bd

Remove unused import

85de951

Recreate previous style to reduce changes in this PR

ef21d78

Merge branch 'main' into fix/levm/gas_type

be608d8

JereSalo reviewed Dec 20, 2024

View reviewed changes

lima-limon-inc added 8 commits December 20, 2024 15:28

Change variable name

e0a5eec

Suggested-by: Jeremias Salomon <jeremias.salomon@lambdaclass.com>

Change variable name

e235e99

Suggested-by: Jeremias Salomon <jeremias.salomon@lambdaclass.com>

Make gas_fee return U256

96b9bf5

Merge branch 'main' into fix/levm/gas_type

df0a717

Fix linter

89edb87

Revert "Change variable name"

04759e1

This reverts commit e235e99.

blob_gas_used -> blobhash_amount

669fb66

Revert "Change variable name"

c3c8b33

This reverts commit e0a5eec.

lima-limon-inc force-pushed the fix/levm/gas_type branch from e748b0c to a5cec4d Compare December 20, 2024 19:33

Change variable name

341ca8d

lima-limon-inc force-pushed the fix/levm/gas_type branch from a5cec4d to 341ca8d Compare December 20, 2024 19:34

JereSalo approved these changes Dec 20, 2024

View reviewed changes

JereSalo added this pull request to the merge queue Dec 20, 2024

Merged via the queue into main with commit 2bb8fa9 Dec 20, 2024
18 checks passed

JereSalo deleted the fix/levm/gas_type branch December 20, 2024 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(levm): change gas type from U256 to u64 #1528

fix(levm): change gas type from U256 to u64 #1528

lima-limon-inc commented Dec 18, 2024 •

edited

Loading

lima-limon-inc commented Dec 19, 2024

lima-limon-inc commented Dec 19, 2024 •

edited

Loading

damiramirez left a comment

JereSalo left a comment

JereSalo Dec 19, 2024

JereSalo Dec 19, 2024

JereSalo Dec 19, 2024

JereSalo Dec 19, 2024

lima-limon-inc commented Dec 20, 2024

JereSalo Dec 20, 2024 •

edited

Loading

lima-limon-inc Dec 20, 2024

JereSalo Dec 20, 2024

lima-limon-inc Dec 20, 2024

JereSalo Dec 20, 2024

lima-limon-inc Dec 20, 2024

JereSalo Dec 20, 2024

lima-limon-inc Dec 20, 2024

JereSalo left a comment

lima-limon-inc commented Dec 20, 2024

fix(levm): change gas type from U256 to u64 #1528

fix(levm): change gas type from U256 to u64 #1528

Conversation

lima-limon-inc commented Dec 18, 2024 • edited Loading

lima-limon-inc commented Dec 19, 2024

lima-limon-inc commented Dec 19, 2024 • edited Loading

damiramirez left a comment

Choose a reason for hiding this comment

JereSalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lima-limon-inc commented Dec 20, 2024

JereSalo Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JereSalo left a comment

Choose a reason for hiding this comment

lima-limon-inc commented Dec 20, 2024

lima-limon-inc commented Dec 18, 2024 •

edited

Loading

lima-limon-inc commented Dec 19, 2024 •

edited

Loading

JereSalo Dec 20, 2024 •

edited

Loading