From 778ee5c4bfab1a88a3d796701abc539e3f39d40f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Tue, 9 May 2017 11:55:21 +0300 Subject: [PATCH] core: enforce min 10% price bump for replacement txs --- core/tx_list.go | 13 ++++++- core/tx_pool.go | 89 +++++++++++++++++++++++++++++++------------- core/tx_pool_test.go | 79 ++++++++++++++++++++++++++++++++++----- 3 files changed, 143 insertions(+), 38 deletions(-) diff --git a/core/tx_list.go b/core/tx_list.go index d7f4a71dde..760ac46ec1 100644 --- a/core/tx_list.go +++ b/core/tx_list.go @@ -236,6 +236,12 @@ func newTxList(strict bool) *txList { } } +// Overlaps returns whether the transaction specified has the same nonce as one +// already contained within the list. +func (l *txList) Overlaps(tx *types.Transaction) bool { + return l.txs.Get(tx.Nonce()) != nil +} + // Add tries to insert a new transaction into the list, returning whether the // transaction was accepted, and if yes, any previous transaction it replaced. // @@ -244,8 +250,11 @@ func newTxList(strict bool) *txList { func (l *txList) Add(tx *types.Transaction) (bool, *types.Transaction) { // If there's an older better transaction, abort old := l.txs.Get(tx.Nonce()) - if old != nil && old.GasPrice().Cmp(tx.GasPrice()) >= 0 { - return false, nil + if old != nil { + threshold := new(big.Int).Div(new(big.Int).Mul(old.GasPrice(), big.NewInt(100+minPriceBumpPercent)), big.NewInt(100)) + if threshold.Cmp(tx.GasPrice()) >= 0 { + return false, nil + } } // Otherwise overwrite the old transaction with the current one l.txs.Put(tx) diff --git a/core/tx_pool.go b/core/tx_pool.go index b142174c29..ae8c6b92d3 100644 --- a/core/tx_pool.go +++ b/core/tx_pool.go @@ -36,14 +36,15 @@ import ( var ( // Transaction Pool Errors - ErrInvalidSender = errors.New("invalid sender") - ErrNonce = errors.New("nonce too low") - ErrUnderpriced = errors.New("underpriced transaction") - ErrBalance = errors.New("tnsufficient balance") - ErrInsufficientFunds = errors.New("tnsufficient funds for gas * price + value") - ErrIntrinsicGas = errors.New("tntrinsic gas too low") - ErrGasLimit = errors.New("exceeds block gas limit") - ErrNegativeValue = errors.New("negative value") + ErrInvalidSender = errors.New("invalid sender") + ErrNonce = errors.New("nonce too low") + ErrUnderpriced = errors.New("transaction underpriced") + ErrReplaceUnderpriced = errors.New("replacement transaction underpriced") + ErrBalance = errors.New("insufficient balance") + ErrInsufficientFunds = errors.New("insufficient funds for gas * price + value") + ErrIntrinsicGas = errors.New("intrinsic gas too low") + ErrGasLimit = errors.New("exceeds block gas limit") + ErrNegativeValue = errors.New("negative value") ) var ( @@ -52,6 +53,7 @@ var ( maxQueuedPerAccount = uint64(64) // Max limit of queued transactions per address maxQueuedTotal = uint64(1024) // Max limit of queued transactions from all accounts maxQueuedLifetime = 3 * time.Hour // Max amount of time transactions from idle accounts are queued + minPriceBumpPercent = int64(10) // Minimum price bump needed to replace an old transaction evictionInterval = time.Minute // Time interval to check for evictable transactions statsReportInterval = 8 * time.Second // Time interval to report transaction pool stats ) @@ -368,19 +370,21 @@ func (pool *TxPool) validateTx(tx *types.Transaction) error { } // add validates a transaction and inserts it into the non-executable queue for -// later pending promotion and execution. -func (pool *TxPool) add(tx *types.Transaction) error { +// later pending promotion and execution. If the transaction is a replacement for +// an already pending or queued one, it overwrites the previous and returns this +// so outer code doesn't uselessly call promote. +func (pool *TxPool) add(tx *types.Transaction) (bool, error) { // If the transaction is already known, discard it hash := tx.Hash() if pool.all[hash] != nil { log.Trace("Discarding already known transaction", "hash", hash) - return fmt.Errorf("known transaction: %x", hash) + return false, fmt.Errorf("known transaction: %x", hash) } // If the transaction fails basic validation, discard it if err := pool.validateTx(tx); err != nil { log.Trace("Discarding invalid transaction", "hash", hash, "err", err) invalidTxCounter.Inc(1) - return err + return false, err } // If the transaction pool is full, discard underpriced transactions if uint64(len(pool.all)) >= maxPendingTotal+maxQueuedTotal { @@ -388,7 +392,7 @@ func (pool *TxPool) add(tx *types.Transaction) error { if pool.priced.Underpriced(tx, pool.locals) { log.Trace("Discarding underpriced transaction", "hash", hash, "price", tx.GasPrice()) underpricedTxCounter.Inc(1) - return ErrUnderpriced + return false, ErrUnderpriced } // New transaction is better than our worse ones, make room for it drop := pool.priced.Discard(len(pool.all)-int(maxPendingTotal+maxQueuedTotal-1), pool.locals) @@ -398,17 +402,40 @@ func (pool *TxPool) add(tx *types.Transaction) error { pool.removeTx(tx.Hash()) } } - // Great, queue the transaction - pool.enqueueTx(hash, tx) + // If the transaction is replacing an already pending one, do directly + from, _ := types.Sender(pool.signer, tx) // already validated + if list := pool.pending[from]; list != nil && list.Overlaps(tx) { + // Nonce already pending, check if required price bump is met + inserted, old := list.Add(tx) + if !inserted { + pendingDiscardCounter.Inc(1) + return false, ErrReplaceUnderpriced + } + // New transaction is better, replace old one + if old != nil { + delete(pool.all, old.Hash()) + pool.priced.Remove(old) + pendingReplaceCounter.Inc(1) + } + pool.all[tx.Hash()] = tx + pool.priced.Put(tx) - log.Trace("Pooled new transaction", "hash", hash, "from", log.Lazy{Fn: func() common.Address { from, _ := types.Sender(pool.signer, tx); return from }}, "to", tx.To()) - return nil + log.Trace("Pooled new executable transaction", "hash", hash, "from", from, "to", tx.To()) + return old != nil, nil + } + // New transaction isn't replacing a pending one, push into queue + replace, err := pool.enqueueTx(hash, tx) + if err != nil { + return false, err + } + log.Trace("Pooled new future transaction", "hash", hash, "from", from, "to", tx.To()) + return replace, nil } // enqueueTx inserts a new transaction into the non-executable transaction queue. // // Note, this method assumes the pool lock is held! -func (pool *TxPool) enqueueTx(hash common.Hash, tx *types.Transaction) { +func (pool *TxPool) enqueueTx(hash common.Hash, tx *types.Transaction) (bool, error) { // Try to insert the transaction into the future queue from, _ := types.Sender(pool.signer, tx) // already validated if pool.queue[from] == nil { @@ -416,18 +443,19 @@ func (pool *TxPool) enqueueTx(hash common.Hash, tx *types.Transaction) { } inserted, old := pool.queue[from].Add(tx) if !inserted { + // An older transaction was better, discard this queuedDiscardCounter.Inc(1) - return // An older transaction was better, discard this + return false, ErrReplaceUnderpriced } // Discard any previous transaction and mark this if old != nil { delete(pool.all, old.Hash()) pool.priced.Remove(old) - queuedReplaceCounter.Inc(1) } pool.all[hash] = tx pool.priced.Put(tx) + return old != nil, nil } // promoteTx adds a transaction to the pending (processable) list of transactions. @@ -472,15 +500,19 @@ func (pool *TxPool) Add(tx *types.Transaction) error { pool.mu.Lock() defer pool.mu.Unlock() - if err := pool.add(tx); err != nil { + // Try to inject the transaction and update any state + replace, err := pool.add(tx) + if err != nil { return err } state, err := pool.currentState() if err != nil { return err } - pool.promoteExecutables(state) - + // If we added a new transaction, run promotion checks and return + if !replace { + pool.promoteExecutables(state) + } return nil } @@ -490,10 +522,13 @@ func (pool *TxPool) AddBatch(txs []*types.Transaction) error { defer pool.mu.Unlock() // Add the batch of transaction, tracking the accepted ones - added := 0 + replaced, added := true, 0 for _, tx := range txs { - if err := pool.add(tx); err == nil { + if replace, err := pool.add(tx); err == nil { added++ + if !replace { + replaced = false + } } } // Only reprocess the internal state if something was actually added @@ -502,7 +537,9 @@ func (pool *TxPool) AddBatch(txs []*types.Transaction) error { if err != nil { return err } - pool.promoteExecutables(state) + if !replaced { + pool.promoteExecutables(state) + } } return nil } diff --git a/core/tx_pool_test.go b/core/tx_pool_test.go index a634590cf5..e773daa2c0 100644 --- a/core/tx_pool_test.go +++ b/core/tx_pool_test.go @@ -266,14 +266,14 @@ func TestTransactionChainFork(t *testing.T) { resetState() tx := transaction(0, big.NewInt(100000), key) - if err := pool.add(tx); err != nil { + if _, err := pool.add(tx); err != nil { t.Error("didn't expect error", err) } pool.RemoveBatch([]*types.Transaction{tx}) // reset the pool's internal state resetState() - if err := pool.add(tx); err != nil { + if _, err := pool.add(tx); err != nil { t.Error("didn't expect error", err) } } @@ -297,11 +297,11 @@ func TestTransactionDoubleNonce(t *testing.T) { tx3, _ := types.SignTx(types.NewTransaction(0, common.Address{}, big.NewInt(100), big.NewInt(1000000), big.NewInt(1), nil), signer, key) // Add the first two transaction, ensure higher priced stays only - if err := pool.add(tx1); err != nil { - t.Error("didn't expect error", err) + if replace, err := pool.add(tx1); err != nil || replace { + t.Errorf("first transaction insert failed (%v) or reported replacement (%v)", err, replace) } - if err := pool.add(tx2); err != nil { - t.Error("didn't expect error", err) + if replace, err := pool.add(tx2); err != nil || !replace { + t.Errorf("second transaction insert failed (%v) or not reported replacement (%v)", err, replace) } state, _ := pool.currentState() pool.promoteExecutables(state) @@ -312,9 +312,7 @@ func TestTransactionDoubleNonce(t *testing.T) { t.Errorf("transaction mismatch: have %x, want %x", tx.Hash(), tx2.Hash()) } // Add the thid transaction and ensure it's not saved (smaller price) - if err := pool.add(tx3); err != nil { - t.Error("didn't expect error", err) - } + pool.add(tx3) pool.promoteExecutables(state) if pool.pending[addr].Len() != 1 { t.Error("expected 1 pending transactions, got", pool.pending[addr].Len()) @@ -334,7 +332,7 @@ func TestMissingNonce(t *testing.T) { currentState, _ := pool.currentState() currentState.AddBalance(addr, big.NewInt(100000000000000)) tx := transaction(1, big.NewInt(100000), key) - if err := pool.add(tx); err != nil { + if _, err := pool.add(tx); err != nil { t.Error("didn't expect error", err) } if len(pool.pending) != 0 { @@ -955,6 +953,67 @@ func TestTransactionPoolUnderpricing(t *testing.T) { } } +// Tests that the pool rejects replacement transactions that don't meet the minimum +// price bump required. +func TestTransactionReplacement(t *testing.T) { + // Create the pool to test the pricing enforcement with + db, _ := ethdb.NewMemDatabase() + statedb, _ := state.New(common.Hash{}, db) + + pool := NewTxPool(params.TestChainConfig, new(event.TypeMux), func() (*state.StateDB, error) { return statedb, nil }, func() *big.Int { return big.NewInt(1000000) }) + pool.resetState() + + // Create a a test account to add transactions with + key, _ := crypto.GenerateKey() + + state, _ := pool.currentState() + state.AddBalance(crypto.PubkeyToAddress(key.PublicKey), big.NewInt(1000000000)) + + // Add pending transactions, ensuring the minimum price bump is enforced for replacement (for ultra low prices too) + price := int64(100) + threshold := (price * (100 + minPriceBumpPercent)) / 100 + + if err := pool.Add(pricedTransaction(0, big.NewInt(100000), big.NewInt(1), key)); err != nil { + t.Fatalf("failed to add original cheap pending transaction: %v", err) + } + if err := pool.Add(pricedTransaction(0, big.NewInt(100001), big.NewInt(1), key)); err != ErrReplaceUnderpriced { + t.Fatalf("original cheap pending transaction replacement error mismatch: have %v, want %v", err, ErrReplaceUnderpriced) + } + if err := pool.Add(pricedTransaction(0, big.NewInt(100000), big.NewInt(2), key)); err != nil { + t.Fatalf("failed to replace original cheap pending transaction: %v", err) + } + + if err := pool.Add(pricedTransaction(0, big.NewInt(100000), big.NewInt(price), key)); err != nil { + t.Fatalf("failed to add original proper pending transaction: %v", err) + } + if err := pool.Add(pricedTransaction(0, big.NewInt(100000), big.NewInt(threshold), key)); err != ErrReplaceUnderpriced { + t.Fatalf("original proper pending transaction replacement error mismatch: have %v, want %v", err, ErrReplaceUnderpriced) + } + if err := pool.Add(pricedTransaction(0, big.NewInt(100000), big.NewInt(threshold+1), key)); err != nil { + t.Fatalf("failed to replace original proper pending transaction: %v", err) + } + // Add queued transactions, ensuring the minimum price bump is enforced for replacement (for ultra low prices too) + if err := pool.Add(pricedTransaction(2, big.NewInt(100000), big.NewInt(1), key)); err != nil { + t.Fatalf("failed to add original queued transaction: %v", err) + } + if err := pool.Add(pricedTransaction(2, big.NewInt(100001), big.NewInt(1), key)); err != ErrReplaceUnderpriced { + t.Fatalf("original queued transaction replacement error mismatch: have %v, want %v", err, ErrReplaceUnderpriced) + } + if err := pool.Add(pricedTransaction(2, big.NewInt(100000), big.NewInt(2), key)); err != nil { + t.Fatalf("failed to replace original queued transaction: %v", err) + } + + if err := pool.Add(pricedTransaction(2, big.NewInt(100000), big.NewInt(price), key)); err != nil { + t.Fatalf("failed to add original queued transaction: %v", err) + } + if err := pool.Add(pricedTransaction(2, big.NewInt(100001), big.NewInt(threshold), key)); err != ErrReplaceUnderpriced { + t.Fatalf("original queued transaction replacement error mismatch: have %v, want %v", err, ErrReplaceUnderpriced) + } + if err := pool.Add(pricedTransaction(2, big.NewInt(100000), big.NewInt(threshold+1), key)); err != nil { + t.Fatalf("failed to replace original queued transaction: %v", err) + } +} + // Benchmarks the speed of validating the contents of the pending queue of the // transaction pool. func BenchmarkPendingDemotion100(b *testing.B) { benchmarkPendingDemotion(b, 100) }