Skip to content

fix: gas deduction with disable_balance_check#2699

Merged
rakita merged 6 commits intobluealloy:mainfrom
NomicFoundation:fix/disable-balance-check-deduct
Jul 21, 2025
Merged

fix: gas deduction with disable_balance_check#2699
rakita merged 6 commits intobluealloy:mainfrom
NomicFoundation:fix/disable-balance-check-deduct

Conversation

@frangio
Copy link
Contributor

@frangio frangio commented Jul 8, 2025

The merge of validate_tx_against_state and deduct_caller done in #2460 seems to have broken gas deduction when disable_balance_check is enabled. This PR adds a test for this error and proposes a possible fix.

The result is that when balance checks are disabled the effective cost is first deducted with saturation from the caller balance, and then increased if necessary to cover the transaction value.

@codspeed-hq
Copy link

codspeed-hq bot commented Jul 8, 2025

CodSpeed Performance Report

Merging #2699 will improve performances by 3.18%

Comparing NomicFoundation:fix/disable-balance-check-deduct (42d4bc7) with main (356b138)

Summary

⚡ 2 improvements
✅ 157 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
transfer 12.9 µs 12.5 µs +3.18%
transfer_finalize 16.5 µs 16 µs +3.11%

@frangio frangio force-pushed the fix/disable-balance-check-deduct branch from 3d93167 to 710f44e Compare July 8, 2025 00:18
Copy link
Contributor

@Wodann Wodann left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm, good catch

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

Can you do the same thing for op-revm?

@frangio frangio requested a review from rakita July 14, 2025 18:46
@frangio frangio force-pushed the fix/disable-balance-check-deduct branch from 9e066ce to 42d4bc7 Compare July 14, 2025 18:47
@frangio
Copy link
Contributor Author

frangio commented Jul 14, 2025

I've applied the same fix to op-revm. I tried to implement a similar test but I wasn't able to get it to show the failure, I'm not sure why.

@rakita
Copy link
Member

rakita commented Jul 15, 2025

I've applied the same fix to op-revm. I tried to implement a similar test but I wasn't able to get it to show the failure, I'm not sure why.

I will need to check this, but at first look PR lgtm.

@rakita
Copy link
Member

rakita commented Jul 21, 2025

All good, have added test for op-revm in local but couldn't push it to repo branch. Will do it in separate branch

@rakita rakita merged commit 6f1d029 into bluealloy:main Jul 21, 2025
29 checks passed
@github-actions github-actions bot mentioned this pull request Jul 21, 2025
@frangio frangio deleted the fix/disable-balance-check-deduct branch July 21, 2025 13:34
theochap pushed a commit to ethereum-optimism/optimism that referenced this pull request Jan 21, 2026
* add failing test

* fix gas deduction

* fix minimum tx.value balance

* fmt

* improve test to cover tx value handling

* apply fix to op-revm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants