EIP-1559 transactions support #2013

yondonfu · 2021-09-03T13:40:26Z

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

This PR adds support for EIP-1559 transactions.

Under the hood, all transactions submitted by the node will use the new EIP-1559 gas fee fields. Refer to #1972 and the linked resources there for an overview of the new gas fee fields.

At the moment, we still use gas prices in the following manner:

The node interprets the gas price as priority fee + base fee which serves as an estimation of what the user will pay to be included in the next block
The min/max gas price flags set min/max for the priority fee + base fee the user will pay
Later on, we could set the max gas price as the maxFeePerGas field on a transaction to control the maximum fee a user will pay since max gas price and maxFeePerGas should be semantically equivalent, but I left that out for now since we already have a max gas price check mechanism. This change would be simplification we could make later

I recommend reviewing each commit individually and sequentially.

Specific updates (required)

See commit history.

How did you test each of these updates (required)

Updated unit tests.

~~TODO: Manual testing.~~

Ran some manual tests using devtool to confirm that transactions are sent and mined properly
Currently using this branch for a node on mainnet - the latest reward transaction was a type 2 (i.e. 1559 compliant) transaction
Test replacement tx
Test connecting to an ETH provider that doesn't support the eth_maxPriorityFeePerGas RPC call

Does this pull request close any open issues?

Fixes #1147
Fixes #1973
Fixes #1974
Fixes #1975

Checklist:

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

yondonfu · 2021-09-28T14:47:21Z

eth/accountmanager.go

-			if address != am.account.Address {
-				return nil, errors.New("not authorized to sign this account")
-			}
+	opts, err := bind.NewKeyStoreTransactorWithChainID(am.keyStore, am.account, am.chainID)


bind.NewKeyStoreTransactorWithChainID basically does the same thing as the removed code snippet for creating a bind.TransactOpts.

However, note that before we were configuring a EIP-155 signer, but now this method configures a signer using types.LatestSignerForChainID. The signer returned by this method will still support EIP-155 replay protection, but will also support additional transaction types - see this comment.

yondonfu · 2021-09-28T14:48:09Z

eth/accountmanager.go

 }

 // Sign a transaction. Account must be unlocked
 func (am *accountManager) SignTx(tx *types.Transaction) (*types.Transaction, error) {
-	signature, err := am.keyStore.SignHash(am.account, am.signer.Hash(tx).Bytes())
+	signer := types.LatestSignerForChainID(am.chainID)


Note that we use types.LatestSignerForChainID to configure a signer to match the behavior of bind.NewKeyStoreTransactorWithChainID which we are now using to create the bind.TransactOpts used for contract binding transactions.

yondonfu · 2021-09-28T14:49:47Z

eth/backend.go

@@ -80,11 +81,10 @@ func (b *backend) SendTransaction(ctx context.Context, tx *types.Transaction) er
 		return err
 	}

-	msg, err := tx.AsMessage(b.signer)
+	sender, err := types.Sender(b.signer, tx)


types.Sender returns the address derived from the signature over the provided transaction.

yondonfu · 2021-09-28T14:50:34Z

eth/stubclient.go

@@ -383,5 +383,5 @@ func (c *StubClient) NextValidRequest(common.Address) (*big.Int, error) { return

 // Governance
 func (c *StubClient) Vote(pollAddr ethcommon.Address, choiceID *big.Int) (*types.Transaction, error) {
-	return &types.Transaction{}, c.Err
+	return types.NewTx(&types.DynamicFeeTx{}), c.Err


This is the new way of creating transaction objects - we have to specify whether we are creating a dynamic fee tx (i.e. supports EIP-1559) or a legacy tx (i.e. does not support EIP-1559).

yondonfu · 2021-09-28T14:52:50Z

eth/transactionManager_test.go

@@ -244,7 +244,6 @@ func TestTransactionManager_Replace(t *testing.T) {
 	assert.Nil(err)
 	expTx := types.NewTransaction(1, *stubTx.To(), stubTx.Value(), 100000, calcReplacementGasPrice(stubTx), stubTx.Data())
 	assert.Equal(tx.Hash(), expTx.Hash())
-	assert.Equal(tx, expTx)


I think these assertions for the actual transactions themselves needed to be removed because the transaction type contains a time field (used for spam avoidance use cases in the mem pool) that is set when a transaction is created which means that this internal field will be different for transactions created at different times.

yondonfu · 2021-09-28T14:56:17Z

eth/backend.go

+		// In the future, eth_maxPriorityFeePerGas can be replaced with an eth_feeHistory based algorithm (see https://github.com/ethereum/go-ethereum/issues/23479).
+		// For now, if the provider does not support eth_maxPriorityFeePerGas (i.e. not geth), then we calculate the priority fee as
+		// eth_gasPrice - baseFee.
+		if err.Error() == "Method not found" {


This logic should only be executed if the node is a) not using Infura/Alchemy [1] and b) not using geth.

The use of the fee history API would be better. This is just a temporary patch so that the node doesn't just completely fail if a) and b) mentioned above do not hold.

[1] I'm not sure if there are other hosted services to consider here, but at least these two support the eth_maxPriorityFeePerGas RPC method.

yondonfu · 2021-09-28T15:08:41Z

eth/transactionManager.go

-	// Note: If the suggested gas price is lower than the bumped gas price because market gas prices have dropped
-	// since the time of the original tx submission we cannot use the lower suggested gas price and we still need to use
-	// the bumped gas price in order to properly replace a still pending tx
-	if suggestedGasPrice.Cmp(gasPrice) == 1 {


Note that the node will no longer check if the bumped gas price is less than the suggested gas price. Removed to simplify a few things (avoid breaking up suggested gas price into priority fee, base fee in order to set the gas fee fields on the transaction) and because seemed like the situation is pretty rare and the current replacement behavior should largely cover it by allowing for another bump if the first is insufficient.

yondonfu · 2021-09-29T13:55:26Z

Successful transaction replacements on Rinkeby:

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

Invoking transaction: "transfer". Inputs: "recipient: 0x81A876960E71B9d07C33338822b8BBa3a1D56Bf2  amount: 1"  Hash: "0xddfe157bfe390e8276fd3c9d4fb44a9fbfe20176fbce9bb8aa8df6473f9d1c1a".

***************************************************************************
I0929 09:53:13.279458   12574 transactionManager.go:180]
******************************Eth Transaction******************************

Replacement transaction: "transfer".  Hash: "0xc312ffea34fb2d5804b5348b8c3742a8efd1dc19ac52915a318de49af7417ba6".  Priority Fee: 1110000000 Max Fee: 1110000022

***************************************************************************
I0929 09:53:15.350021   12574 transactionManager.go:180]
******************************Eth Transaction******************************

Replacement transaction: "transfer".  Hash: "0xd6a62eb95d52caa27246b5a3aac467a23f3cbbe2d446b28f9ae0bb9514392f92".  Priority Fee: 1232100000 Max Fee: 1232100024

***************************************************************************
I0929 09:53:17.418175   12574 transactionManager.go:180]
******************************Eth Transaction******************************

Replacement transaction: "transfer".  Hash: "0xf38df1ab995d1d90bbad2634d406306fa8d57262c3595fc1582e7bd239d51648".  Priority Fee: 1367631000 Max Fee: 1367631026

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

Ran this test by setting -transactionTimeout 2s -maxTransactionReplacements 3 on the node.

yondonfu · 2021-09-29T18:56:25Z

Holding off on a merge here because I discovered a few remaining issues with this PR:

The max gas price check is broken because SuggestGasPrice will not be called for the new EIP-1559 transactions
The conditional for executing the fallback logic in SuggestGasTipCap needs to be updated because ETH providers do not necessarily return "Method not found" if the RPC method is not supported - we can probably just execute the fallback logic if the eth_maxPriorityFeePerGas RPC method fails the first time
I believe there is a bug in the go-ethereum base contract binding code that causes gas fee fields to be cached in a TransactOpts object after the first transaction is sent. Should be fixed in this commit in livepeer/go-ethereum, but still need to update this branch to use the fork

yondonfu · 2021-09-30T02:21:17Z

The max gas price check is broken because SuggestGasPrice will not be called for the new EIP-1559 transactions

For now, this is fixed in e73a5a8. The one downside of the change in that commit is that if the value returned by eth_gasPrice is lower than the sum of eth_maxPriorityFeePerGas + the base fee then a transaction could go through with a priority fee + base fee that exceeds the max gas price. This is possible because the max gas price check is run via backend.SuggestGasPrice(), but the actual priority fee to use for the transaction is fetched immediately after via SuggestGasTipCap(). I think the probability of that happening is fairly low since the gas fee fields for the transaction are set immediately after the max gas price check so I opted to stick with this change for simplicity sake, but this possibility is worth being aware of.

The conditional for executing the fallback logic in SuggestGasTipCap needs to be updated because ETH providers do not necessarily return "Method not found" if the RPC method is not supported - we can probably just execute the fallback logic if the eth_maxPriorityFeePerGas RPC method fails the first time

This is fixed by 31eb840 (squashed a fixup commit in).

I believe there is a bug in the go-ethereum base contract binding code that causes gas fee fields to be cached in a TransactOpts object after the first transaction is sent. Should be fixed in this commit in livepeer/go-ethereum, but still need to update this branch to use the fork

I opened a PR to fix this in go-ethereum here. For now, while that PR is unmerged, I've switched the go-ethereum dependency to a fork with the fix in 63a8a2d.

Fallback logic is needed right now if the JSON-RPC provider does not support the eth_maxPriorityFeePerGas RPC method.

Mention gas price is treated as priority fee + base fee now

To fix TransactOpts gas fee overwrite bug

yondonfu force-pushed the yf/eip-1559 branch from 65ad322 to b40086c Compare September 9, 2021 02:58

yondonfu force-pushed the yf/eip-1559 branch 2 times, most recently from 40d49db to 6be85fb Compare September 28, 2021 14:41

yondonfu commented Sep 28, 2021

View reviewed changes

yondonfu changed the title ~~EIP-1559 support~~ EIP-1559 transactions support Sep 28, 2021

yondonfu marked this pull request as ready for review September 28, 2021 15:24

yondonfu requested review from kyriediculous, jailuthra, darkdarkdragon and kautukkundan September 28, 2021 15:24

yondonfu mentioned this pull request Sep 28, 2021

Consider updating gas price monitor to use EIP-1559 fee fields for gas price estimation instead of eth_gasPrice #1975

Closed

yondonfu force-pushed the yf/eip-1559 branch from 03f8634 to b7f0f38 Compare September 28, 2021 22:03

kyriediculous approved these changes Sep 29, 2021

View reviewed changes

yondonfu requested a review from kyriediculous September 30, 2021 02:26

yondonfu force-pushed the yf/eip-1559 branch 2 times, most recently from 39a763e to e73a5a8 Compare September 30, 2021 02:37

yondonfu added 11 commits September 30, 2021 10:12

mod: Bump go-ethereum to v1.10.8

9b9f3a9

eth: Update contract binding gen command

ac128a7

cmd/livepeer+eth: go-ethereum API fixes

3cf2238

eth+eth/contracts: New contract bindings w/ abigen v.1.10.8

7aa1e75

mod: Run go mod tidy

7e6cf80

eth: Add fallback logic for SuggestGasTipCap

fb0c308

Fallback logic is needed right now if the JSON-RPC provider does not support the eth_maxPriorityFeePerGas RPC method.

eth: Update replacement tx logic for EIP-1559

05da3f2

cmd/livepeer: Update help desc for gas price flags

57e985c

Mention gas price is treated as priority fee + base fee now

doc: Update gas price docs for EIP-1559

df3bc3b

mod: Temp use livepeer/go-ethereum

c621d20

To fix TransactOpts gas fee overwrite bug

eth: Run max gas price check in SuggestGasTipCap

e26b20f

CHANGELOG_PENDING.md: Add support for EIP-1559 txs

4ae526b

yondonfu force-pushed the yf/eip-1559 branch from e73a5a8 to 4ae526b Compare September 30, 2021 14:12

yondonfu merged commit e1518db into master Sep 30, 2021

yondonfu deleted the yf/eip-1559 branch September 30, 2021 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-1559 transactions support #2013

EIP-1559 transactions support #2013

yondonfu commented Sep 3, 2021 •

edited

yondonfu Sep 28, 2021

yondonfu Sep 28, 2021

yondonfu Sep 28, 2021

yondonfu Sep 28, 2021

yondonfu Sep 28, 2021

yondonfu Sep 28, 2021

yondonfu Sep 28, 2021

yondonfu commented Sep 29, 2021

yondonfu commented Sep 29, 2021

yondonfu commented Sep 30, 2021 •

edited

EIP-1559 transactions support #2013

EIP-1559 transactions support #2013

Conversation

yondonfu commented Sep 3, 2021 • edited

yondonfu Sep 28, 2021

Choose a reason for hiding this comment

yondonfu Sep 28, 2021

Choose a reason for hiding this comment

yondonfu Sep 28, 2021

Choose a reason for hiding this comment

yondonfu Sep 28, 2021

Choose a reason for hiding this comment

yondonfu Sep 28, 2021

Choose a reason for hiding this comment

yondonfu Sep 28, 2021

Choose a reason for hiding this comment

yondonfu Sep 28, 2021

Choose a reason for hiding this comment

yondonfu commented Sep 29, 2021

yondonfu commented Sep 29, 2021

yondonfu commented Sep 30, 2021 • edited

yondonfu commented Sep 3, 2021 •

edited

yondonfu commented Sep 30, 2021 •

edited