-
Notifications
You must be signed in to change notification settings - Fork 464
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
Fix retryable gas estimation when overriding gas price to zero #1929
Conversation
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.
looks good, but it seems that we need to do a similar fix for refund code in EndTxHook
https://github.com/OffchainLabs/nitro/blob/487f6eeba172805c8be6ce2a0bb8914e55c30e5d/arbos/tx_processor.go#L460C1-L460C1
while running:
go test -v -run TestSubmitRetryableImmediateSuccess
following error is logged:
ERROR[10-19|15:59:09.136] fee address doesn't have enough funds to give user refund err="insufficient balance for transfer: addr 0x0000000000000000000000000000000000000000 have 2100000000000 want 2521239500000000" feeAddress=0x0000000000000000000000000000000000000000
Great catch, thanks! I've updated the EndTxHook to account for this, and as part of this I also needed a geth side change: OffchainLabs/go-ethereum#263 |
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.
LGTM
Codecov Report
@@ Coverage Diff @@
## master #1929 +/- ##
==========================================
+ Coverage 21.74% 21.88% +0.13%
==========================================
Files 206 206
Lines 28857 28871 +14
==========================================
+ Hits 6275 6317 +42
+ Misses 21518 21501 -17
+ Partials 1064 1053 -11 |
We previously prevented the error that would occur in retryable gas estimation when the gas price was set to zero, as that's intended functionality of gas estimation, but we didn't actually disable charging in this case. We only disabled the code that checked if there was enough balance. That meant that the sender might have the wrong balance in gas estimation if the gas price was overridden to zero (as if the gas price was not overridden), and the node might log an error because the gas fee transfer failed in gas estimation.
Pulls in: