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.

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

Merged
merged 5 commits into from
Oct 24, 2023

Conversation

PlasmaPower
Copy link
Collaborator

@PlasmaPower PlasmaPower commented Oct 17, 2023

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:

@PlasmaPower PlasmaPower requested a review from magicxyyz October 17, 2023 05:09
@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Oct 17, 2023
Copy link
Contributor

@magicxyyz magicxyyz left a 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

@PlasmaPower
Copy link
Collaborator Author

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

Copy link
Contributor

@magicxyyz magicxyyz left a comment

Choose a reason for hiding this comment

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

LGTM

@PlasmaPower PlasmaPower enabled auto-merge October 20, 2023 16:27
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #1929 (e34cc18) into master (11b09d9) will increase coverage by 0.13%.
The diff coverage is 21.21%.

@@            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     

@PlasmaPower PlasmaPower merged commit 6393265 into master Oct 24, 2023
13 checks passed
@PlasmaPower PlasmaPower deleted the retryable-estimate-zero-fee branch October 24, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants