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

eth: Set tx GasFeeCap to min(gasPriceEstimate, current GasFeeCap) #2583

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

yondonfu
Copy link
Member

@yondonfu yondonfu commented Sep 7, 2022

What does this pull request do? Explain your changes. (required)

At the moment, if the -maxGasPrice flag is used the GasFeeCap for all txs is also set to the maxGasPrice value. The GasFeeCap serves as the maximum fee/price that will be charged per unit of gas consumed by a tx. The minimum balance of an account is the gas used by the tx * GasFeeCap. If GasFeeCap exceeds the actual gas price that is required for txs then a refund will be provided to the account based on the difference between GasFeeCap and the actual gas price. But, if the account does not have enough balance cover gas used by the tx * GasFeeCap then an Insufficient funds... error will be encountered.

In #2574, some O operators set a value for -maxGasPrice that is quite a bit higher than the actual average gas price paid on Arbitrum (i.e. ~0.1 gwei). As a result, even though their account balances could cover a tx based on the actual gas price paid, their account balances could not cover a tx based on GasFeeCap = maxGasPrice. The current available workarounds are to either top off account balances to ensure the balance can cover gas * GasFeeCap (with the excess fee refunded to the balance at the end of the successful tx), set a lower value for -maxGasPrice or stop using the -maxGasPrice flag.

I felt that while there are available workarounds, the way the -maxGasPrice flag currently works is a bit weird since it can easily lead to a node operator thinking they are just being safe by setting a high value for -maxGasPrice, but then having that high value cause tx issues if they do not have super high account balances. So, I opted to modify the GasFeeCap of txs to min(gasPriceEstimate, current GasFeeCap where gasPriceEstimate = baseFee * 2 so that in these cases a node operator doesn't run into tx issues.

See in-line code comments for more details.

Specific updates (required)

See commit history.

How did you test each of these updates (required)

Tested on Arbitrum Rinkeby:

  1. Ran a node off the master branch

  2. Used livepeer_cli to set maxGasPrice = 300 gwei

  3. Invoked a LPT transfer and observed an insufficient funds... error

******************************Eth Transaction******************************

Invoking transaction: "transfer". Inputs: "recipient: 0x3F7bcA8c726Bc34a903ed3e838997d7d3A29B99d  amount: 0"
Transaction Failed: insufficient funds for gas * price + value: address 0x3F7bcA8c726Bc34a903ed3e838997d7d3A29B99d have 6233423641163 want 8282700000000000

***************************************************************************
E0907 22:23:51.538450       1 handlers.go:1421] HTTP Response Error 500: insufficient funds for gas * price + value: address 0x3F7bcA8c726Bc34a903ed3e838997d7d3A29B99d have 6233423641163 want 8282700000000000
  1. Restarted the node running off of the branch for this PR

  2. Used livepeer_cli to set maxGasPrice = 300 gwei

  3. Invoked a LPT transfer and observed a successful tx.

******************************Eth Transaction******************************

Invoking transaction: "transfer". Inputs: "recipient: 0x3F7bcA8c726Bc34a903ed3e838997d7d3A29B99d  amount: 0"  Hash: "0x5db565e84ba009689fd63792f80ecdb3cb16500ce2c23d4249c17dfb26245528".

***************************************************************************
I0907 22:21:12.384937       1 handlers.go:1018] Transferred 0 LPTU to 0x3F7bcA8c726Bc34a903ed3e838997d7d3A29B99d

Does this pull request close any open issues?

Fixes #2574

Checklist:

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #2583 (e2a15ee) into master (6bf2087) will decrease coverage by 0.04784%.
The diff coverage is 0.00000%.

❗ Current head e2a15ee differs from pull request most recent head c429705. Consider uploading reports for the commit c429705 to get more accurate results

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2583         +/-   ##
===================================================
- Coverage   56.42997%   56.38213%   -0.04785%     
===================================================
  Files             88          88                 
  Lines          18857       18873         +16     
===================================================
  Hits           10641       10641                 
- Misses          7637        7653         +16     
  Partials         579         579                 
Impacted Files Coverage Δ
eth/client.go 5.16039% <0.00000%> (-0.11779%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bf2087...c429705. Read the comment docs.

Impacted Files Coverage Δ
eth/client.go 5.16039% <0.00000%> (-0.11779%) ⬇️

Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added 2 comments. Other than that LGTM. Good investigation @yondonfu !

eth/client.go Outdated Show resolved Hide resolved
eth/client.go Outdated Show resolved Hide resolved
@yondonfu
Copy link
Member Author

yondonfu commented Sep 8, 2022

I'll see if there are any reports in #2574 and then rebase + merge.

@yondonfu yondonfu merged commit 3ff79bd into master Sep 9, 2022
@yondonfu yondonfu deleted the yf/gas-fee-cap-fix branch September 9, 2022 23:20
@cyberj0g cyberj0g mentioned this pull request Oct 21, 2022
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.

'Failed: insufficient funds for gas * price + value' - Cannot process transactions after Nitro upgrade
2 participants