From 787cbe4121cde51caa3a03814133b8ad74944c06 Mon Sep 17 00:00:00 2001 From: Igor Crevar Date: Thu, 27 Jul 2023 12:04:25 +0200 Subject: [PATCH] EVM-749 Transaction pool Base Fee fix (#1747) --- consensus/dev/dev.go | 6 +-- consensus/ibft/consensus_backend.go | 6 +-- consensus/ibft/ibft.go | 2 +- consensus/polybft/block_builder.go | 2 +- consensus/polybft/block_builder_test.go | 2 +- consensus/polybft/consensus_runtime.go | 2 +- consensus/polybft/mocks_test.go | 4 +- server/server.go | 1 + txpool/mock_test.go | 26 ++++++++- txpool/query.go | 9 ++-- txpool/txpool.go | 32 ++++++----- txpool/txpool_test.go | 70 ++++++++++++++++++++----- 12 files changed, 117 insertions(+), 45 deletions(-) diff --git a/consensus/dev/dev.go b/consensus/dev/dev.go index eb0a9c1bae..f6942963f8 100644 --- a/consensus/dev/dev.go +++ b/consensus/dev/dev.go @@ -109,10 +109,10 @@ type transitionInterface interface { Write(txn *types.Transaction) error } -func (d *Dev) writeTransactions(baseFee, gasLimit uint64, transition transitionInterface) []*types.Transaction { +func (d *Dev) writeTransactions(gasLimit uint64, transition transitionInterface) []*types.Transaction { var successful []*types.Transaction - d.txpool.Prepare(baseFee) + d.txpool.Prepare() for { tx := d.txpool.Peek() @@ -183,7 +183,7 @@ func (d *Dev) writeNewBlock(parent *types.Header) error { return err } - txns := d.writeTransactions(baseFee, gasLimit, transition) + txns := d.writeTransactions(gasLimit, transition) // Commit the changes _, root, err := transition.Commit() diff --git a/consensus/ibft/consensus_backend.go b/consensus/ibft/consensus_backend.go index f241e1140a..3285be41ec 100644 --- a/consensus/ibft/consensus_backend.go +++ b/consensus/ibft/consensus_backend.go @@ -179,8 +179,6 @@ func (i *backendIBFT) buildBlock(parent *types.Header) (*types.Block, error) { } // calculate base fee - baseFee := i.blockchain.CalculateBaseFee(parent) - header.GasLimit = gasLimit if err := i.currentHooks.ModifyHeader(header, i.currentSigner.Address()); err != nil { @@ -209,7 +207,6 @@ func (i *backendIBFT) buildBlock(parent *types.Header) (*types.Block, error) { txs := i.writeTransactions( writeCtx, - baseFee, gasLimit, header.Number, transition, @@ -297,7 +294,6 @@ type transitionInterface interface { func (i *backendIBFT) writeTransactions( writeCtx context.Context, - baseFee, gasLimit, blockNumber uint64, transition transitionInterface, @@ -324,7 +320,7 @@ func (i *backendIBFT) writeTransactions( ) }() - i.txpool.Prepare(baseFee) + i.txpool.Prepare() write: for { diff --git a/consensus/ibft/ibft.go b/consensus/ibft/ibft.go index 5e388e4d4f..04ab7c5c2f 100644 --- a/consensus/ibft/ibft.go +++ b/consensus/ibft/ibft.go @@ -42,7 +42,7 @@ var ( ) type txPoolInterface interface { - Prepare(uint64) + Prepare() Length() uint64 Peek() *types.Transaction Pop(tx *types.Transaction) diff --git a/consensus/polybft/block_builder.go b/consensus/polybft/block_builder.go index f30d375c63..67e2e7d54d 100644 --- a/consensus/polybft/block_builder.go +++ b/consensus/polybft/block_builder.go @@ -159,7 +159,7 @@ func (b *BlockBuilder) WriteTx(tx *types.Transaction) error { func (b *BlockBuilder) Fill() { blockTimer := time.NewTimer(b.params.BlockTime) - b.params.TxPool.Prepare(b.params.BaseFee) + b.params.TxPool.Prepare() write: for { select { diff --git a/consensus/polybft/block_builder_test.go b/consensus/polybft/block_builder_test.go index aa4857dbf2..6476bab278 100644 --- a/consensus/polybft/block_builder_test.go +++ b/consensus/polybft/block_builder_test.go @@ -75,7 +75,7 @@ func TestBlockBuilder_BuildBlockTxOneFailedTxAndOneTakesTooMuchGas(t *testing.T) parentHeader := &types.Header{StateRoot: hash, GasLimit: 1_000_000_000_000_000} txPool := &txPoolMock{} - txPool.On("Prepare", uint64(0)).Once() + txPool.On("Prepare").Once() for i, acc := range accounts { receiver := types.Address(acc.Ecdsa.Address()) diff --git a/consensus/polybft/consensus_runtime.go b/consensus/polybft/consensus_runtime.go index c26ebe2aa7..acd5f741ee 100644 --- a/consensus/polybft/consensus_runtime.go +++ b/consensus/polybft/consensus_runtime.go @@ -37,7 +37,7 @@ var ( // txPoolInterface is an abstraction of transaction pool type txPoolInterface interface { - Prepare(uint64) + Prepare() Length() uint64 Peek() *types.Transaction Pop(*types.Transaction) diff --git a/consensus/polybft/mocks_test.go b/consensus/polybft/mocks_test.go index 750067ba92..6f3b3d55c1 100644 --- a/consensus/polybft/mocks_test.go +++ b/consensus/polybft/mocks_test.go @@ -303,8 +303,8 @@ type txPoolMock struct { mock.Mock } -func (tp *txPoolMock) Prepare(baseFee uint64) { - tp.Called(baseFee) +func (tp *txPoolMock) Prepare() { + tp.Called() } func (tp *txPoolMock) Length() uint64 { diff --git a/server/server.go b/server/server.go index 7feaf75706..bc31164271 100644 --- a/server/server.go +++ b/server/server.go @@ -416,6 +416,7 @@ func NewServer(config *Config) (*Server, error) { } } + m.txpool.SetBaseFee(m.blockchain.Header()) m.txpool.Start() return m, nil diff --git a/txpool/mock_test.go b/txpool/mock_test.go index 675cd7f5a6..30633207ff 100644 --- a/txpool/mock_test.go +++ b/txpool/mock_test.go @@ -15,11 +15,17 @@ var mockHeader = &types.Header{ type defaultMockStore struct { DefaultHeader *types.Header + + getBlockByHashFn func(types.Hash, bool) (*types.Block, bool) + calculateBaseFeeFn func(*types.Header) uint64 } func NewDefaultMockStore(header *types.Header) defaultMockStore { return defaultMockStore{ - header, + DefaultHeader: header, + calculateBaseFeeFn: func(h *types.Header) uint64 { + return h.BaseFee + }, } } @@ -31,7 +37,11 @@ func (m defaultMockStore) GetNonce(types.Hash, types.Address) uint64 { return 0 } -func (m defaultMockStore) GetBlockByHash(types.Hash, bool) (*types.Block, bool) { +func (m defaultMockStore) GetBlockByHash(hash types.Hash, full bool) (*types.Block, bool) { + if m.getBlockByHashFn != nil { + return m.getBlockByHashFn(hash, full) + } + return nil, false } @@ -41,6 +51,14 @@ func (m defaultMockStore) GetBalance(types.Hash, types.Address) (*big.Int, error return balance, nil } +func (m defaultMockStore) CalculateBaseFee(header *types.Header) uint64 { + if m.calculateBaseFeeFn != nil { + return m.calculateBaseFeeFn(header) + } + + return 0 +} + type faultyMockStore struct { } @@ -60,6 +78,10 @@ func (fms faultyMockStore) GetBalance(root types.Hash, addr types.Address) (*big return nil, fmt.Errorf("unable to fetch account state") } +func (fms faultyMockStore) CalculateBaseFee(*types.Header) uint64 { + return 0 +} + type mockSigner struct { } diff --git a/txpool/query.go b/txpool/query.go index 8e4c9ba91f..a950a4dd96 100644 --- a/txpool/query.go +++ b/txpool/query.go @@ -46,12 +46,15 @@ func (p *TxPool) GetPendingTx(txHash types.Hash) (*types.Transaction, bool) { func (p *TxPool) GetTxs(inclQueued bool) ( allPromoted, allEnqueued map[types.Address][]*types.Transaction, ) { - allPromoted, allEnqueued = p.accounts.allTxs(inclQueued) - - return + return p.accounts.allTxs(inclQueued) } // GetBaseFee returns current base fee func (p *TxPool) GetBaseFee() uint64 { return atomic.LoadUint64(&p.baseFee) } + +// SetBaseFee calculates base fee from the (current) header and sets value into baseFee field +func (p *TxPool) SetBaseFee(header *types.Header) { + atomic.StoreUint64(&p.baseFee, p.store.CalculateBaseFee(header)) +} diff --git a/txpool/txpool.go b/txpool/txpool.go index 58c31acb1a..ecb6738c09 100644 --- a/txpool/txpool.go +++ b/txpool/txpool.go @@ -90,6 +90,7 @@ type store interface { GetNonce(root types.Hash, addr types.Address) uint64 GetBalance(root types.Hash, addr types.Address) (*big.Int, error) GetBlockByHash(types.Hash, bool) (*types.Block, bool) + CalculateBaseFee(parent *types.Header) uint64 } type signer interface { @@ -331,15 +332,12 @@ func (p *TxPool) AddTx(tx *types.Transaction) error { // Prepare generates all the transactions // ready for execution. (primaries) -func (p *TxPool) Prepare(baseFee uint64) { - // set base fee - atomic.StoreUint64(&p.baseFee, baseFee) - +func (p *TxPool) Prepare() { // fetch primary from each account primaries := p.accounts.getPrimaries() // create new executables queue with base fee and initial transactions (primaries) - p.executables = newPricesQueue(baseFee, primaries) + p.executables = newPricesQueue(p.GetBaseFee(), primaries) } // Peek returns the best-price selected @@ -532,6 +530,11 @@ func (p *TxPool) processEvent(event *blockchain.Event) { } } + // update base fee + if ln := len(event.NewChain); ln > 0 { + p.SetBaseFee(event.NewChain[ln-1]) + } + // reset accounts with the new state p.resetAccounts(stateNonces) @@ -596,6 +599,13 @@ func (p *TxPool) validateTx(tx *types.Transaction) error { return runtime.ErrMaxCodeSizeExceeded } + // Grab the state root, current block number and block gas limit for the latest block + currentHeader := p.store.Header() + stateRoot := currentHeader.StateRoot + currentBlockNumber := currentHeader.Number + latestBlockGasLimit := currentHeader.GasLimit + baseFee := p.GetBaseFee() // base fee is calculated for the next block + if tx.Type == types.DynamicFeeTx { // Reject dynamic fee tx if london hardfork is not enabled if !p.forks.London { @@ -606,7 +616,7 @@ func (p *TxPool) validateTx(tx *types.Transaction) error { // DynamicFeeTx should be rejected if TxHashWithType fork is registered but not enabled for current block blockNumber, err := forkmanager.GetInstance().GetForkBlock(chain.TxHashWithType) - if err == nil && blockNumber > p.store.Header().Number { + if err == nil && blockNumber > currentBlockNumber { metrics.IncrCounter([]string{txPoolMetrics, "dynamic_tx_not_allowed"}, 1) return ErrDynamicTxNotAllowed @@ -638,23 +648,20 @@ func (p *TxPool) validateTx(tx *types.Transaction) error { } // Reject underpriced transactions - if tx.GasFeeCap.Cmp(new(big.Int).SetUint64(p.GetBaseFee())) < 0 { + if tx.GasFeeCap.Cmp(new(big.Int).SetUint64(baseFee)) < 0 { metrics.IncrCounter([]string{txPoolMetrics, "underpriced_tx"}, 1) return ErrUnderpriced } } else { // Legacy approach to check if the given tx is not underpriced - if tx.GetGasPrice(p.GetBaseFee()).Cmp(big.NewInt(0).SetUint64(p.priceLimit)) < 0 { + if tx.GetGasPrice(baseFee).Cmp(big.NewInt(0).SetUint64(p.priceLimit)) < 0 { metrics.IncrCounter([]string{txPoolMetrics, "underpriced_tx"}, 1) return ErrUnderpriced } } - // Grab the state root for the latest block - stateRoot := p.store.Header().StateRoot - // Check nonce ordering if p.store.GetNonce(stateRoot, tx.From) > tx.Nonce { metrics.IncrCounter([]string{txPoolMetrics, "nonce_too_low_tx"}, 1) @@ -690,9 +697,6 @@ func (p *TxPool) validateTx(tx *types.Transaction) error { return ErrIntrinsicGas } - // Grab the block gas limit for the latest block - latestBlockGasLimit := p.store.Header().GasLimit - if tx.Gas > latestBlockGasLimit { metrics.IncrCounter([]string{txPoolMetrics, "block_gas_limit_exceeded_tx"}, 1) diff --git a/txpool/txpool_test.go b/txpool/txpool_test.go index 5ce5b5392a..bc33a71a0c 100644 --- a/txpool/txpool_test.go +++ b/txpool/txpool_test.go @@ -1769,7 +1769,7 @@ func TestPop(t *testing.T) { assert.Equal(t, uint64(1), pool.accounts.get(addr1).promoted.length()) // pop the tx - pool.Prepare(0) + pool.Prepare() tx := pool.Peek() pool.Pop(tx) @@ -1803,7 +1803,7 @@ func TestDrop(t *testing.T) { assert.Equal(t, uint64(1), pool.accounts.get(addr1).promoted.length()) // pop the tx - pool.Prepare(0) + pool.Prepare() tx := pool.Peek() pool.Drop(tx) assert.Equal(t, tx1, tx) @@ -1838,7 +1838,7 @@ func TestDemote(t *testing.T) { assert.Equal(t, uint64(0), pool.accounts.get(addr1).Demotions()) // call demote - pool.Prepare(0) + pool.Prepare() tx := pool.Peek() pool.Demote(tx) @@ -1872,7 +1872,7 @@ func TestDemote(t *testing.T) { pool.accounts.get(addr1).demotions = maxAccountDemotions // call demote - pool.Prepare(0) + pool.Prepare() tx := pool.Peek() pool.Demote(tx) @@ -2027,11 +2027,15 @@ func Test_TxPool_validateTx(t *testing.T) { defaultKey, defaultAddr := tests.GenerateKeyAndAddr(t) setupPool := func() *TxPool { - pool, err := newTestPool() + header := mockHeader.Copy() + header.BaseFee = 1000 + + pool, err := newTestPool(NewDefaultMockStore(header)) if err != nil { t.Fatalf("cannot create txpool - err: %v\n", err) } + pool.SetBaseFee(header) pool.SetSigner(signer) return pool @@ -2088,7 +2092,6 @@ func Test_TxPool_validateTx(t *testing.T) { t.Parallel() pool := setupPool() - pool.baseFee = 1000 tx := newTx(defaultAddr, 0, 1) tx.Type = types.DynamicFeeTx @@ -2102,7 +2105,6 @@ func Test_TxPool_validateTx(t *testing.T) { t.Parallel() pool := setupPool() - pool.baseFee = 1000 tx := newTx(defaultAddr, 0, 1) tx.Type = types.DynamicFeeTx @@ -2119,7 +2121,6 @@ func Test_TxPool_validateTx(t *testing.T) { t.Parallel() pool := setupPool() - pool.baseFee = 1000 tx := newTx(defaultAddr, 0, 1) tx.Type = types.DynamicFeeTx @@ -2136,7 +2137,6 @@ func Test_TxPool_validateTx(t *testing.T) { t.Parallel() pool := setupPool() - pool.baseFee = 1000 // undefined gas tip cap tx := newTx(defaultAddr, 0, 1) @@ -2170,7 +2170,6 @@ func Test_TxPool_validateTx(t *testing.T) { bitLength := 512 pool := setupPool() - pool.baseFee = 1000 // very high gas fee cap tx := newTx(defaultAddr, 0, 1) @@ -2781,7 +2780,7 @@ func TestExecutablesOrder(t *testing.T) { require.Len(t, waitForEvents(ctx, subscription, expectedPromotedTx), expectedPromotedTx) require.Equal(t, uint64(len(test.expectedPriceOrder)), pool.accounts.promoted()) - pool.Prepare(1000) + pool.Prepare() var successful []*types.Transaction for { @@ -2984,7 +2983,7 @@ func TestRecovery(t *testing.T) { assert.Len(t, waitForEvents(ctx, promoteSubscription, totalTx), totalTx) func() { - pool.Prepare(0) + pool.Prepare() for { tx := pool.Peek() if tx == nil { @@ -3455,6 +3454,53 @@ func TestAddTxsInOrder(t *testing.T) { } } +func TestResetWithHeadersSetsBaseFee(t *testing.T) { + t.Parallel() + + blocks := []*types.Block{ + { + Header: &types.Header{ + BaseFee: 100, + Hash: types.Hash{1}, + }, + }, + { + Header: &types.Header{ + BaseFee: 1000, + Hash: types.Hash{1}, + }, + }, + { + Header: &types.Header{ + BaseFee: 2000, + Hash: types.Hash{2}, + }, + }, + } + + store := NewDefaultMockStore(blocks[0].Header) + store.getBlockByHashFn = func(h types.Hash, b bool) (*types.Block, bool) { + for _, b := range blocks { + if b.Header.Hash == h { + return b, true + } + } + + return nil, false + } + + pool, err := newTestPool(store) + require.NoError(t, err) + + pool.SetBaseFee(blocks[0].Header) + + pool.ResetWithHeaders() + assert.Equal(t, blocks[0].Header.BaseFee, pool.GetBaseFee()) + + pool.ResetWithHeaders(blocks[len(blocks)-2].Header, blocks[len(blocks)-1].Header) + assert.Equal(t, blocks[len(blocks)-1].Header.BaseFee, pool.GetBaseFee()) +} + func BenchmarkAddTxTime(b *testing.B) { b.Run("benchmark add one tx", func(b *testing.B) { signer := crypto.NewEIP155Signer(100, true)