eth: Ensure `maxFeePerGas` in all transactions never exceed `-masGasPrice` defined by the user #2111

leszko · 2021-11-19T14:10:27Z

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

Limit the maxFeePerGas in the eth transaction to the -maxGasPrice value defined by the user. Before it was possible that the user paid more than they specified as -maxGasPrice.

Additionally, I removed the calcGasPrice() logic from transactionManager.go, because I believe it was incorrect. It assumed that maxFeePerGas will be calculated in the go-ethereum code, but I think we override it in our code here.

Specific updates (required)

Adjust GasFeeCap before sending the transaction
Remove calcGasPrice function in replace()

How did you test each of these updates (required)

I did test the change both locally and using Rinkeby.

Local testing

Set up devtool environment
Start orchestrator and wait for the first transaction to happen

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

Invoking transaction: "rewardWithHint". Inputs: "_newPosPrev: 0x0000000000000000000000000000000000000000  _newPosNext: 0x0000000000000000000000000000000000000000"  Hash: "0x175ff3b3eaebbabc0014526a8dcac53d9505129cbec0f33122da3c9ee70eb509". 

***************************************************************************

Check the transaction, note the default value of maxFeePerGas: 1000000014 (2 * base fee + priority fee)

$ seth tx 0x175ff3b3eaebbabc0014526a8dcac53d9505129cbec0f33122da3c9ee70eb509
accessList              []
blockHash               0xc880bb7bb980bc6e5de23976a20b12d02ce8431c3af25cc4e35da0478bbf020e
...
maxFeePerGas            1000000014
...

Set max gas price (via livepeer CLI) to 1000000013
Wait for the next transaction
Check the transaction, note the value maxFeePerGas is set to 1000000013

$ seth tx 0x2222d3d7a1026fe0712352e482a55cac167d491b3f4b17b05467bd5a92de08b1
accessList              []
blockHash               0xf8729bb0ba9b6925ca77bf03817fb648985e304cde08bf6ab16814146ba4e12a
...
maxFeePerGas            1000000013
...

Set max gas price to a value which is too low, e.g. 5
Note that the transaction is not sent

E1125 12:57:56.999314    8399 roundinitializer.go:87] current gas price exceeds maximum gas price max=0.000000005 GWei current=1.000000007 GWei

Rinkeby testing

Start livepeer connected to rinkeby
Invoke any transaction (e.g. in case of broadcaster, deposit ETH)

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

Invoking transaction: "fundDepositAndReserve". Inputs: "_depositAmount: 100000000000000000  _reserveAmount: 300000000000000000"  Hash: "0x382e9a5b7635862fd8a5ac64ddf7e1b1d6111c4b67cb1318c13d7fede3fa31f0". 

***************************************************************************

Check the transaction in Etherscan

https://rinkeby.etherscan.io/tx/0x382e9a5b7635862fd8a5ac64ddf7e1b1d6111c4b67cb1318c13d7fede3fa31f0

Base: 0.000000024 Gwei |Max: 1.000000044 Gwei |Max Priority: 1 Gwei

Set max gas price to 1000000055 and run the transaction again

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

Invoking transaction: "fundDepositAndReserve". Inputs: "_depositAmount: 1000000000000000  _reserveAmount: 0"  Hash: "0xc6cfc72bb7aef2e4f026332ebcf9eec00ba10b4a0a94290d7c59a3bc0d1f8e9f". 

***************************************************************************

Check the transaction in Etherscan

https://rinkeby.etherscan.io/tx/0xc6cfc72bb7aef2e4f026332ebcf9eec00ba10b4a0a94290d7c59a3bc0d1f8e9f

Base: 0.00000002 Gwei |Max: 1.000000055 Gwei |Max Priority: 1 Gwei

Does this pull request close any open issues?

fix #2084

Checklist:

Read the contribution guide
make runs successfully
All tests in ./test.sh pass
README and other documentation updated
Pending changelog updated

yondonfu · 2021-11-22T02:40:44Z

Reaction based on a quick skim:

I think this draft PR is going in the right direction. My understanding of the transaction flow after these changes:

Contract binding triggers a call to transact()
transact() sets the GasFeeCap on the tx to 2x base fee + GasTipCap
transact() passes the created tx to SendTransaction() which is actually implemented by the TransactionManager
The SendTransaction() method on TransactionManager uses newAdjustedTx() to adjust the GasFeeCap on the tx if needed - if it is less than than GasPriceMonitor.MaxGasPrice() then we keep it, otherwise we set the GasFeeCap to GasPriceMonitor.MaxGasPrice(). As a result the GasFeeCap never exceeds the value of GasPriceMonitor.MaxGasPrice()

leszko · 2021-11-22T07:56:08Z

Thanks for the quick feedback @yondonfu . I understand it exactly the same as you do. So, I'll perform the testing and if everything works fine, I'll update this draft PR.

…efined by the user

eth/transactionManager.go

yondonfu · 2021-11-30T03:08:58Z

eth/transactionManager.go

+func (tm *TransactionManager) newAdjustedTx(tx *types.Transaction) *types.Transaction {
+	baseTx := newDynamicFeeTx(tx)
+	if tm.gpm.MaxGasPrice() != nil {
+		baseTx.GasFeeCap = tm.gpm.MaxGasPrice()


I forget if we talked about this, but is there a reason for always setting this to the max gas price as opposed to only setting it to the max gas price if the max gas price exceeds the current value?

Both approaches will work. However, IMO the always setting max gas price is slightly better because:

It's simpler for the user what happens.

Since the replacement transaction needs to be at least 10% higher, we may end up never reaching exactly maxGasPrice.

What do you think?

Sounds reasonable. But, this also means that a replacement tx would never be sent if the user sets a maxGasPrice because GasFeeCap is always set to maxGasPrice meaning that we are never able to increase GasFeeCap for a replacement tx. And the only scenario where a replacement tx could be sent is if the user does not set a maxGasPrice. I think this is okay, but just want to make sure we're on the same page about the expected behavior.

You're right. Actually, there is one more scenario for the replacement if the user manually increases maxGasPrice while the transaction is pending. Then the replacement can also happen.

leszko

@yondonfu I addressed your comemnts. PTAL

leszko · 2021-11-30T10:01:33Z

eth/transactionManager.go

+func (tm *TransactionManager) newAdjustedTx(tx *types.Transaction) *types.Transaction {
+	baseTx := newDynamicFeeTx(tx)
+	if tm.gpm.MaxGasPrice() != nil {
+		baseTx.GasFeeCap = tm.gpm.MaxGasPrice()


Both approaches will work. However, IMO the always setting max gas price is slightly better because:

It's simpler for the user what happens.

Since the replacement transaction needs to be at least 10% higher, we may end up never reaching exactly maxGasPrice.

What do you think?

yondonfu

As of this PR, I think the -maxGasPrice flag is basically equivalent to maxFeePerGas since its value is always set as GasTipCap for transactions. The gas price check in backend.SuggestGasPrice() can be interpreted as:

The gas price cached by the GasPriceMonitor could be interpreted as "a suggested maxFeePerGas" i.e. the current base fee + a suggested priority fee
We compare the user provided maxFeePerGas (via -maxGasPrice) against this suggested maxFeePerGas

If this is the case, I think we should make some small updates to docs in a few places:

yondonfu · 2021-11-30T15:55:56Z

eth/transactionManager.go

+func (tm *TransactionManager) newAdjustedTx(tx *types.Transaction) *types.Transaction {
+	baseTx := newDynamicFeeTx(tx)
+	if tm.gpm.MaxGasPrice() != nil {
+		baseTx.GasFeeCap = tm.gpm.MaxGasPrice()


Sounds reasonable. But, this also means that a replacement tx would never be sent if the user sets a maxGasPrice because GasFeeCap is always set to maxGasPrice meaning that we are never able to increase GasFeeCap for a replacement tx. And the only scenario where a replacement tx could be sent is if the user does not set a maxGasPrice. I think this is okay, but just want to make sure we're on the same page about the expected behavior.

leszko · 2021-12-01T09:41:30Z

As of this PR, I think the -maxGasPrice flag is basically equivalent to maxFeePerGas since its value is always set as GasTipCap for transactions.

That is correct.

The gas price check in backend.SuggestGasPrice() can be interpreted as:

The gas price cached by the GasPriceMonitor could be interpreted as "a suggested maxFeePerGas" i.e. the current base fee + a suggested priority fee

We compare the user provided maxFeePerGas (via -maxGasPrice) against this suggested maxFeePerGas

That is not correct. "suggested gas price" from GasPriceMonitor is not interpreted as maxFeePerGas. If maxGasPrice is not set by the user, then maxFeePerGas is set to the default value 2 * base fee + tip cap. I believe that backend.SuggestGasPrice() is not used at all. It was used before EIP-1559.

You made me think we should make some cleanup, but I believe it should be a separate PR, not to mix to many things here.

If this is the case, I think we should make some small updates to docs in a few places:

I updated the doc here: ff84dec
I believe the param description in livepeer.go is correct, it says "Maximum gas price (priority fee + base fee) for ETH transactions in wei, 40 Gwei = 40000000000"

leszko

@yondonfu Added my comments. PTAL

yondonfu · 2021-12-01T17:34:06Z

I believe that backend.SuggestGasPrice() is not used at all.

That method is actually called in backend.SuggestGasTipCap() right now. And backend.SuggestGasTipCap() is called when a transaction is created for a contract binding if the GasTipCap is not explicitly specified in the transact opts used. This means backend.SuggestGasPrice() is called every time a transaction is created since we never explicitly set the GasTipCap in the transact opts in order to always use the suggested GasTipCap based on current ETH network transaction activity.

"suggested gas price" from GasPriceMonitor is not interpreted as maxFeePerGas. If maxGasPrice is not set by the user, then maxFeePerGas is set to the default value 2 * base fee + tip cap

Yeah I was a bit off there.

So, the GasPriceMonitor.GasPrice() method will return the cached value for the latest gas price returned from an ETH node which is really base fee + suggested priority fee (i.e. GasTipCap). If -maxGasPrice is set, maxGasPrice = maxFeePerGas so backend.SuggestGasPrice() is comparing the latest base fee + suggested priority fee against maxFeePerGas.

I was just thinking about how to describe what "gas price" means. I think the way to describe this is:

Gas price = base fee + priority fee
Max gas price = maximum value of base fee + priority fee aka maxFeePerGas
Min gas price = minimum value of base fee + priority fee

How does that sound to you?

I believe the param description in livepeer.go is correct, it says

Okay yeah so if -maxGasPrice is maxFeePerGas then that description should still be correct.

leszko · 2021-12-02T09:17:49Z

That method is actually called in backend.SuggestGasTipCap() right now. And backend.SuggestGasTipCap() is called when a transaction is created for a contract binding if the GasTipCap is not explicitly specified in the transact opts used. This means backend.SuggestGasPrice() is called every time a transaction is created since we never explicitly set the GasTipCap in the transact opts in order to always use the suggested GasTipCap based on current ETH network transaction activity.

This is correct with one comment. The function backend.SuggestGasPrice() is called inside in every transaction, but its result is not usually used. Its result is used only if calling to Eth endpoint eth_maxPriorityFeePerGas results in an error. That means that in most cases (happy path), we don't use backend.SuggestGasPrice().

So, the GasPriceMonitor.GasPrice() method will return the cached value for the latest gas price returned from an ETH node which is really base fee + suggested priority fee (i.e. GasTipCap). If -maxGasPrice is set, maxGasPrice = maxFeePerGas so backend.SuggestGasPrice() is comparing the latest base fee + suggested priority fee against maxFeePerGas.

Yes, I think it's right. gas price is the value from the Eth endpoint eth_gasPrice, which might be base fee + suggested priority as you wrote.

I was just thinking about how to describe what "gas price" means. I think the way to describe this is:

Gas price = base fee + priority fee

Max gas price = maximum value of base fee + priority fee aka maxFeePerGas

Min gas price = minimum value of base fee + priority fee

I'm not sure what's the meaning of gas price for the user if we don't set it anywhere in the transaction. And it's not the Livepeer parameter, maybe we should not describe it at all 🤔

leszko · 2021-12-03T09:35:10Z

@yondonfu I updated the doc again after our conversation. PTAL.

yondonfu

LGTM!

leszko changed the title ~~eth: Make maxFeePerGas in eth transactions never exceed~~ eth: Ensure maxFeePerGas in all transactions never exceed -masGasPrice defined by the user Nov 25, 2021

leszko force-pushed the 2084-max-gas-price branch 2 times, most recently from 8b9a5bd to 1566bef Compare November 25, 2021 15:06

leszko changed the base branch from master to docker-build November 25, 2021 15:08

leszko changed the base branch from docker-build to master November 25, 2021 15:08

leszko added 2 commits November 25, 2021 16:09

eth: Make maxFeePerGas in eth transactions never exceed maxGasPrice d…

d7488f2

…efined by the user

Update CHANGELOG_PENDING

fa6d530

leszko force-pushed the 2084-max-gas-price branch from 1566bef to fa6d530 Compare November 25, 2021 15:11

leszko marked this pull request as ready for review November 25, 2021 15:11

leszko requested a review from yondonfu November 25, 2021 15:11

leszko mentioned this pull request Nov 25, 2021

Set -maxGasPrice flag equal to max fee per gas, current implementation makes spending unpredictable #2084

Closed

yondonfu reviewed Nov 30, 2021

View reviewed changes

eth: Remove logging error when signing the transaction

04bc0ff

leszko commented Nov 30, 2021

View reviewed changes

leszko requested a review from yondonfu November 30, 2021 10:01

yondonfu reviewed Nov 30, 2021

View reviewed changes

doc: Update documentation about maxGasPrice

ff84dec

leszko commented Dec 1, 2021

View reviewed changes

leszko requested a review from yondonfu December 1, 2021 09:42

doc: Update documentation about maxGasPrice

c5ab81b

yondonfu approved these changes Dec 7, 2021

View reviewed changes

leszko merged commit d7c7536 into livepeer:master Dec 7, 2021

leszko deleted the 2084-max-gas-price branch December 7, 2021 08:53

leszko mentioned this pull request Jan 4, 2022

Session crashes (without errors in log) right after the O sends a tx #2152

Closed

This was referenced Jan 5, 2022

Revert eth: Ensure maxFeePerGas in all transactions never exceed -masGasPrice #2163

Merged

Make transactions compatible with Arbitrum and fix setting max gas price #2171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eth: Ensure `maxFeePerGas` in all transactions never exceed `-masGasPrice` defined by the user #2111

eth: Ensure `maxFeePerGas` in all transactions never exceed `-masGasPrice` defined by the user #2111

leszko commented Nov 19, 2021 •

edited

Loading

yondonfu commented Nov 22, 2021

leszko commented Nov 22, 2021 •

edited

Loading

yondonfu Nov 30, 2021

leszko Nov 30, 2021

yondonfu Nov 30, 2021

leszko Nov 30, 2021

leszko left a comment

leszko Nov 30, 2021

yondonfu left a comment

yondonfu Nov 30, 2021

leszko commented Dec 1, 2021

leszko left a comment

yondonfu commented Dec 1, 2021

leszko commented Dec 2, 2021

leszko commented Dec 3, 2021

yondonfu left a comment

eth: Ensure maxFeePerGas in all transactions never exceed -masGasPrice defined by the user #2111

eth: Ensure maxFeePerGas in all transactions never exceed -masGasPrice defined by the user #2111

Conversation

leszko commented Nov 19, 2021 • edited Loading

yondonfu commented Nov 22, 2021

leszko commented Nov 22, 2021 • edited Loading

yondonfu Nov 30, 2021

Choose a reason for hiding this comment

leszko Nov 30, 2021

Choose a reason for hiding this comment

yondonfu Nov 30, 2021

Choose a reason for hiding this comment

leszko Nov 30, 2021

Choose a reason for hiding this comment

leszko left a comment

Choose a reason for hiding this comment

leszko Nov 30, 2021

Choose a reason for hiding this comment

yondonfu left a comment

Choose a reason for hiding this comment

yondonfu Nov 30, 2021

Choose a reason for hiding this comment

leszko commented Dec 1, 2021

leszko left a comment

Choose a reason for hiding this comment

yondonfu commented Dec 1, 2021

leszko commented Dec 2, 2021

leszko commented Dec 3, 2021

yondonfu left a comment

Choose a reason for hiding this comment

eth: Ensure `maxFeePerGas` in all transactions never exceed `-masGasPrice` defined by the user #2111

eth: Ensure `maxFeePerGas` in all transactions never exceed `-masGasPrice` defined by the user #2111

leszko commented Nov 19, 2021 •

edited

Loading

leszko commented Nov 22, 2021 •

edited

Loading