From c47052a580a1b150929de5de5e58b72495fbcbc1 Mon Sep 17 00:00:00 2001 From: Hendrik Hofstadt Date: Wed, 22 Jul 2020 15:37:44 +0200 Subject: [PATCH 1/2] core: sort txs at the same gas price by received time --- core/tx_pool.go | 1 + core/types/transaction.go | 54 +++++++++++++++++++++++++++------- core/types/transaction_test.go | 9 +++++- 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/core/tx_pool.go b/core/tx_pool.go index 7301102b41..de9d4f27e4 100644 --- a/core/tx_pool.go +++ b/core/tx_pool.go @@ -568,6 +568,7 @@ func (pool *TxPool) validateTx(tx *types.Transaction, local bool) error { // whitelisted, preventing any associated transaction from being dropped out of the pool // due to pricing constraints. func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err error) { + tx.SetReceivedTime(time.Now()) // If the transaction is already known, discard it hash := tx.Hash() if pool.all.Get(hash) != nil { diff --git a/core/types/transaction.go b/core/types/transaction.go index da691bb03f..8755dec3ac 100644 --- a/core/types/transaction.go +++ b/core/types/transaction.go @@ -22,6 +22,7 @@ import ( "io" "math/big" "sync/atomic" + "time" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/hexutil" @@ -37,6 +38,9 @@ var ( type Transaction struct { data txdata + + // Time when the transaction was added to the txPool + receivedTime *time.Time // caches hash atomic.Value size atomic.Value @@ -184,6 +188,13 @@ func (tx *Transaction) GasPriceIntCmp(other *big.Int) int { func (tx *Transaction) Value() *big.Int { return new(big.Int).Set(tx.data.Amount) } func (tx *Transaction) Nonce() uint64 { return tx.data.AccountNonce } func (tx *Transaction) CheckNonce() bool { return true } +func (tx *Transaction) ReceivedTime() (time.Time, bool) { + if tx.receivedTime == nil { + return time.Time{}, false + } + + return *tx.receivedTime, true +} // To returns the recipient address of the transaction. // It returns nil if the transaction is a contract creation. @@ -246,7 +257,7 @@ func (tx *Transaction) WithSignature(signer Signer, sig []byte) (*Transaction, e if err != nil { return nil, err } - cpy := &Transaction{data: tx.data} + cpy := &Transaction{data: tx.data, receivedTime: tx.receivedTime} cpy.data.R, cpy.data.S, cpy.data.V = r, s, v return cpy, nil } @@ -264,6 +275,11 @@ func (tx *Transaction) RawSignatureValues() (v, r, s *big.Int) { return tx.data.V, tx.data.R, tx.data.S } +// SetReceivedTime sets the time that this transaction was received at. +func (tx *Transaction) SetReceivedTime(time time.Time) { + tx.receivedTime = &time +} + // Transactions is a Transaction slice type for basic sorting. type Transactions []*Transaction @@ -306,19 +322,35 @@ func (s TxByNonce) Len() int { return len(s) } func (s TxByNonce) Less(i, j int) bool { return s[i].data.AccountNonce < s[j].data.AccountNonce } func (s TxByNonce) Swap(i, j int) { s[i], s[j] = s[j], s[i] } -// TxByPrice implements both the sort and the heap interface, making it useful +// TxByPriceAndReceiveTime implements both the sort and the heap interface, making it useful // for all at once sorting as well as individually adding and removing elements. -type TxByPrice Transactions +type TxByPriceAndReceiveTime Transactions + +func (s TxByPriceAndReceiveTime) Len() int { return len(s) } +func (s TxByPriceAndReceiveTime) Less(i, j int) bool { + // If the price is equal, use the time the tx was received for deterministic sorting + if s[i].data.Price.Cmp(s[j].data.Price) == 0 { + recvI, ok := s[i].ReceivedTime() + if !ok { + return true + } -func (s TxByPrice) Len() int { return len(s) } -func (s TxByPrice) Less(i, j int) bool { return s[i].data.Price.Cmp(s[j].data.Price) > 0 } -func (s TxByPrice) Swap(i, j int) { s[i], s[j] = s[j], s[i] } + recvJ, ok := s[j].ReceivedTime() + if !ok { + return true + } + return recvI.UnixNano() < recvJ.UnixNano() + } + + return s[i].data.Price.Cmp(s[j].data.Price) > 0 +} +func (s TxByPriceAndReceiveTime) Swap(i, j int) { s[i], s[j] = s[j], s[i] } -func (s *TxByPrice) Push(x interface{}) { +func (s *TxByPriceAndReceiveTime) Push(x interface{}) { *s = append(*s, x.(*Transaction)) } -func (s *TxByPrice) Pop() interface{} { +func (s *TxByPriceAndReceiveTime) Pop() interface{} { old := *s n := len(old) x := old[n-1] @@ -331,7 +363,7 @@ func (s *TxByPrice) Pop() interface{} { // entire batches of transactions for non-executable accounts. type TransactionsByPriceAndNonce struct { txs map[common.Address]Transactions // Per account nonce-sorted list of transactions - heads TxByPrice // Next transaction for each unique account (price heap) + heads TxByPriceAndReceiveTime // Next transaction for each unique account (price heap) signer Signer // Signer for the set of transactions } @@ -341,8 +373,8 @@ type TransactionsByPriceAndNonce struct { // Note, the input map is reowned so the caller should not interact any more with // if after providing it to the constructor. func NewTransactionsByPriceAndNonce(signer Signer, txs map[common.Address]Transactions) *TransactionsByPriceAndNonce { - // Initialize a price based heap with the head transactions - heads := make(TxByPrice, 0, len(txs)) + // Initialize a price and received time based heap with the head transactions + heads := make(TxByPriceAndReceiveTime, 0, len(txs)) for from, accTxs := range txs { heads = append(heads, accTxs[0]) // Ensure the sender address is from the signer diff --git a/core/types/transaction_test.go b/core/types/transaction_test.go index 7a1b6cd4d4..3961c6aa0f 100644 --- a/core/types/transaction_test.go +++ b/core/types/transaction_test.go @@ -22,6 +22,7 @@ import ( "encoding/json" "math/big" "testing" + "time" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/crypto" @@ -134,7 +135,8 @@ func TestTransactionPriceNonceSort(t *testing.T) { for start, key := range keys { addr := crypto.PubkeyToAddress(key.PublicKey) for i := 0; i < 25; i++ { - tx, _ := SignTx(NewTransaction(uint64(start+i), common.Address{}, big.NewInt(100), 100, big.NewInt(int64(start+i)), nil), signer, key) + tx, _ := SignTx(NewTransaction(uint64(i), common.Address{}, big.NewInt(100), 100, big.NewInt(int64(25-i)), nil), signer, key) + tx.SetReceivedTime(time.Unix(0, int64(start))) groups[addr] = append(groups[addr], tx) } } @@ -168,6 +170,11 @@ func TestTransactionPriceNonceSort(t *testing.T) { if fromi != fromNext && txi.GasPrice().Cmp(next.GasPrice()) < 0 { t.Errorf("invalid gasprice ordering: tx #%d (A=%x P=%v) < tx #%d (A=%x P=%v)", i, fromi[:4], txi.GasPrice(), i+1, fromNext[:4], next.GasPrice()) } + + // Make sure receivedTime order is ascending if the txs have the same gas price + if txi.GasPrice().Cmp(next.GasPrice()) == 0 && fromi != fromNext && txi.receivedTime.UnixNano() > next.receivedTime.UnixNano() { + t.Errorf("invalid received time ordering: tx #%d (A=%x T=%d) > tx #%d (A=%x T=%d)", i, fromi[:4], txi.receivedTime.UnixNano(), i+1, fromNext[:4], next.receivedTime.UnixNano()) + } } } } From 298a19bbc69fdef59eb292656b5a3a44c64dc470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Tue, 28 Jul 2020 17:13:04 +0300 Subject: [PATCH 2/2] core: API-less transaction time sorting --- core/tx_pool.go | 1 - core/types/transaction.go | 77 ++++++++++++++-------------------- core/types/transaction_test.go | 56 +++++++++++++++++++++---- 3 files changed, 79 insertions(+), 55 deletions(-) diff --git a/core/tx_pool.go b/core/tx_pool.go index de9d4f27e4..7301102b41 100644 --- a/core/tx_pool.go +++ b/core/tx_pool.go @@ -568,7 +568,6 @@ func (pool *TxPool) validateTx(tx *types.Transaction, local bool) error { // whitelisted, preventing any associated transaction from being dropped out of the pool // due to pricing constraints. func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err error) { - tx.SetReceivedTime(time.Now()) // If the transaction is already known, discard it hash := tx.Hash() if pool.all.Get(hash) != nil { diff --git a/core/types/transaction.go b/core/types/transaction.go index 8755dec3ac..ec19744881 100644 --- a/core/types/transaction.go +++ b/core/types/transaction.go @@ -37,10 +37,9 @@ var ( ) type Transaction struct { - data txdata + data txdata // Consensus contents of a transaction + time time.Time // Time first seen locally (spam avoidance) - // Time when the transaction was added to the txPool - receivedTime *time.Time // caches hash atomic.Value size atomic.Value @@ -104,8 +103,10 @@ func newTransaction(nonce uint64, to *common.Address, amount *big.Int, gasLimit if gasPrice != nil { d.Price.Set(gasPrice) } - - return &Transaction{data: d} + return &Transaction{ + data: d, + time: time.Now(), + } } // ChainId returns which chain id this transaction was signed for (if at all) @@ -138,8 +139,8 @@ func (tx *Transaction) DecodeRLP(s *rlp.Stream) error { err := s.Decode(&tx.data) if err == nil { tx.size.Store(common.StorageSize(rlp.ListSize(size))) + tx.time = time.Now() } - return err } @@ -157,7 +158,6 @@ func (tx *Transaction) UnmarshalJSON(input []byte) error { if err := dec.UnmarshalJSON(input); err != nil { return err } - withSignature := dec.V.Sign() != 0 || dec.R.Sign() != 0 || dec.S.Sign() != 0 if withSignature { var V byte @@ -171,8 +171,10 @@ func (tx *Transaction) UnmarshalJSON(input []byte) error { return ErrInvalidSig } } - - *tx = Transaction{data: dec} + *tx = Transaction{ + data: dec, + time: time.Now(), + } return nil } @@ -188,13 +190,6 @@ func (tx *Transaction) GasPriceIntCmp(other *big.Int) int { func (tx *Transaction) Value() *big.Int { return new(big.Int).Set(tx.data.Amount) } func (tx *Transaction) Nonce() uint64 { return tx.data.AccountNonce } func (tx *Transaction) CheckNonce() bool { return true } -func (tx *Transaction) ReceivedTime() (time.Time, bool) { - if tx.receivedTime == nil { - return time.Time{}, false - } - - return *tx.receivedTime, true -} // To returns the recipient address of the transaction. // It returns nil if the transaction is a contract creation. @@ -257,7 +252,10 @@ func (tx *Transaction) WithSignature(signer Signer, sig []byte) (*Transaction, e if err != nil { return nil, err } - cpy := &Transaction{data: tx.data, receivedTime: tx.receivedTime} + cpy := &Transaction{ + data: tx.data, + time: tx.time, + } cpy.data.R, cpy.data.S, cpy.data.V = r, s, v return cpy, nil } @@ -275,11 +273,6 @@ func (tx *Transaction) RawSignatureValues() (v, r, s *big.Int) { return tx.data.V, tx.data.R, tx.data.S } -// SetReceivedTime sets the time that this transaction was received at. -func (tx *Transaction) SetReceivedTime(time time.Time) { - tx.receivedTime = &time -} - // Transactions is a Transaction slice type for basic sorting. type Transactions []*Transaction @@ -322,35 +315,27 @@ func (s TxByNonce) Len() int { return len(s) } func (s TxByNonce) Less(i, j int) bool { return s[i].data.AccountNonce < s[j].data.AccountNonce } func (s TxByNonce) Swap(i, j int) { s[i], s[j] = s[j], s[i] } -// TxByPriceAndReceiveTime implements both the sort and the heap interface, making it useful +// TxByPriceAndTime implements both the sort and the heap interface, making it useful // for all at once sorting as well as individually adding and removing elements. -type TxByPriceAndReceiveTime Transactions - -func (s TxByPriceAndReceiveTime) Len() int { return len(s) } -func (s TxByPriceAndReceiveTime) Less(i, j int) bool { - // If the price is equal, use the time the tx was received for deterministic sorting - if s[i].data.Price.Cmp(s[j].data.Price) == 0 { - recvI, ok := s[i].ReceivedTime() - if !ok { - return true - } - - recvJ, ok := s[j].ReceivedTime() - if !ok { - return true - } - return recvI.UnixNano() < recvJ.UnixNano() +type TxByPriceAndTime Transactions + +func (s TxByPriceAndTime) Len() int { return len(s) } +func (s TxByPriceAndTime) Less(i, j int) bool { + // If the prices are equal, use the time the transaction was first seen for + // deterministic sorting + cmp := s[i].data.Price.Cmp(s[j].data.Price) + if cmp == 0 { + return s[i].time.Before(s[j].time) } - - return s[i].data.Price.Cmp(s[j].data.Price) > 0 + return cmp > 0 } -func (s TxByPriceAndReceiveTime) Swap(i, j int) { s[i], s[j] = s[j], s[i] } +func (s TxByPriceAndTime) Swap(i, j int) { s[i], s[j] = s[j], s[i] } -func (s *TxByPriceAndReceiveTime) Push(x interface{}) { +func (s *TxByPriceAndTime) Push(x interface{}) { *s = append(*s, x.(*Transaction)) } -func (s *TxByPriceAndReceiveTime) Pop() interface{} { +func (s *TxByPriceAndTime) Pop() interface{} { old := *s n := len(old) x := old[n-1] @@ -363,7 +348,7 @@ func (s *TxByPriceAndReceiveTime) Pop() interface{} { // entire batches of transactions for non-executable accounts. type TransactionsByPriceAndNonce struct { txs map[common.Address]Transactions // Per account nonce-sorted list of transactions - heads TxByPriceAndReceiveTime // Next transaction for each unique account (price heap) + heads TxByPriceAndTime // Next transaction for each unique account (price heap) signer Signer // Signer for the set of transactions } @@ -374,7 +359,7 @@ type TransactionsByPriceAndNonce struct { // if after providing it to the constructor. func NewTransactionsByPriceAndNonce(signer Signer, txs map[common.Address]Transactions) *TransactionsByPriceAndNonce { // Initialize a price and received time based heap with the head transactions - heads := make(TxByPriceAndReceiveTime, 0, len(txs)) + heads := make(TxByPriceAndTime, 0, len(txs)) for from, accTxs := range txs { heads = append(heads, accTxs[0]) // Ensure the sender address is from the signer diff --git a/core/types/transaction_test.go b/core/types/transaction_test.go index 3961c6aa0f..159cb0c4c4 100644 --- a/core/types/transaction_test.go +++ b/core/types/transaction_test.go @@ -128,15 +128,14 @@ func TestTransactionPriceNonceSort(t *testing.T) { for i := 0; i < len(keys); i++ { keys[i], _ = crypto.GenerateKey() } - signer := HomesteadSigner{} + // Generate a batch of transactions with overlapping values, but shifted nonces groups := map[common.Address]Transactions{} for start, key := range keys { addr := crypto.PubkeyToAddress(key.PublicKey) for i := 0; i < 25; i++ { - tx, _ := SignTx(NewTransaction(uint64(i), common.Address{}, big.NewInt(100), 100, big.NewInt(int64(25-i)), nil), signer, key) - tx.SetReceivedTime(time.Unix(0, int64(start))) + tx, _ := SignTx(NewTransaction(uint64(start+i), common.Address{}, big.NewInt(100), 100, big.NewInt(int64(start+i)), nil), signer, key) groups[addr] = append(groups[addr], tx) } } @@ -157,12 +156,10 @@ func TestTransactionPriceNonceSort(t *testing.T) { // Make sure the nonce order is valid for j, txj := range txs[i+1:] { fromj, _ := Sender(signer, txj) - if fromi == fromj && txi.Nonce() > txj.Nonce() { t.Errorf("invalid nonce ordering: tx #%d (A=%x N=%v) < tx #%d (A=%x N=%v)", i, fromi[:4], txi.Nonce(), i+j, fromj[:4], txj.Nonce()) } } - // If the next tx has different from account, the price must be lower than the current one if i+1 < len(txs) { next := txs[i+1] @@ -170,10 +167,53 @@ func TestTransactionPriceNonceSort(t *testing.T) { if fromi != fromNext && txi.GasPrice().Cmp(next.GasPrice()) < 0 { t.Errorf("invalid gasprice ordering: tx #%d (A=%x P=%v) < tx #%d (A=%x P=%v)", i, fromi[:4], txi.GasPrice(), i+1, fromNext[:4], next.GasPrice()) } + } + } +} + +// Tests that if multiple transactions have the same price, the ones seen earlier +// are prioritized to avoid network spam attacks aiming for a specific ordering. +func TestTransactionTimeSort(t *testing.T) { + // Generate a batch of accounts to start with + keys := make([]*ecdsa.PrivateKey, 5) + for i := 0; i < len(keys); i++ { + keys[i], _ = crypto.GenerateKey() + } + signer := HomesteadSigner{} + + // Generate a batch of transactions with overlapping prices, but different creation times + groups := map[common.Address]Transactions{} + for start, key := range keys { + addr := crypto.PubkeyToAddress(key.PublicKey) + + tx, _ := SignTx(NewTransaction(0, common.Address{}, big.NewInt(100), 100, big.NewInt(1), nil), signer, key) + tx.time = time.Unix(0, int64(len(keys)-start)) - // Make sure receivedTime order is ascending if the txs have the same gas price - if txi.GasPrice().Cmp(next.GasPrice()) == 0 && fromi != fromNext && txi.receivedTime.UnixNano() > next.receivedTime.UnixNano() { - t.Errorf("invalid received time ordering: tx #%d (A=%x T=%d) > tx #%d (A=%x T=%d)", i, fromi[:4], txi.receivedTime.UnixNano(), i+1, fromNext[:4], next.receivedTime.UnixNano()) + groups[addr] = append(groups[addr], tx) + } + // Sort the transactions and cross check the nonce ordering + txset := NewTransactionsByPriceAndNonce(signer, groups) + + txs := Transactions{} + for tx := txset.Peek(); tx != nil; tx = txset.Peek() { + txs = append(txs, tx) + txset.Shift() + } + if len(txs) != len(keys) { + t.Errorf("expected %d transactions, found %d", len(keys), len(txs)) + } + for i, txi := range txs { + fromi, _ := Sender(signer, txi) + if i+1 < len(txs) { + next := txs[i+1] + fromNext, _ := Sender(signer, next) + + if txi.GasPrice().Cmp(next.GasPrice()) < 0 { + t.Errorf("invalid gasprice ordering: tx #%d (A=%x P=%v) < tx #%d (A=%x P=%v)", i, fromi[:4], txi.GasPrice(), i+1, fromNext[:4], next.GasPrice()) + } + // Make sure time order is ascending if the txs have the same gas price + if txi.GasPrice().Cmp(next.GasPrice()) == 0 && txi.time.After(next.time) { + t.Errorf("invalid received time ordering: tx #%d (A=%x T=%v) > tx #%d (A=%x T=%v)", i, fromi[:4], txi.time, i+1, fromNext[:4], next.time) } } }