core: API-less transaction time sorting

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 {

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{
-	return &Transaction{data: d}
+		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{
-	*tx = Transaction{data: dec}
+		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
+type TxByPriceAndTime Transactions
 
-func (s TxByPriceAndReceiveTime) Len() int { return len(s) }
+func (s TxByPriceAndTime) Len() int { return len(s) }
-func (s TxByPriceAndReceiveTime) Less(i, j int) bool {
+func (s TxByPriceAndTime) Less(i, j int) bool {
-	// If the price is equal, use the time the tx was received for deterministic sorting
+	// If the prices are equal, use the time the transaction was first seen for
-	if s[i].data.Price.Cmp(s[j].data.Price) == 0 {
+	// deterministic sorting
-		recvI, ok := s[i].ReceivedTime()
+	cmp := s[i].data.Price.Cmp(s[j].data.Price)
-		if !ok {
+	if cmp == 0 {
-			return true
+		return s[i].time.Before(s[j].time)
-		}
-
-		recvJ, ok := s[j].ReceivedTime()
-		if !ok {
-			return true
-		}
-		return recvI.UnixNano() < recvJ.UnixNano()
 	}
-
+	return cmp > 0
-	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 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

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, _ := SignTx(NewTransaction(uint64(start+i), common.Address{}, big.NewInt(100), 100, big.NewInt(int64(start+i)), nil), signer, key)
-			tx.SetReceivedTime(time.Unix(0, int64(start)))
 			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())
 			}
+		}
+	}
+}
 
-			// Make sure receivedTime order is ascending if the txs have the same gas price
+// Tests that if multiple transactions have the same price, the ones seen earlier
-			if txi.GasPrice().Cmp(next.GasPrice()) == 0 && fromi != fromNext && txi.receivedTime.UnixNano() > next.receivedTime.UnixNano() {
+// are prioritized to avoid network spam attacks aiming for a specific ordering.
-				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())
+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))
+
+		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)
 			}
 		}
 	}