Apply the same reverse sort order as https://github.com/hyperledger/b… (#6146)

* Apply the same reverse sort order as https://github.com/hyperledger/besu/pull/6106 but to the base fee sorter

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Fix unit tests

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update eviction unit tests to expect highest-sequence TXs be evicted first

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Update change log

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

* Spotless fixes

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

---------

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## 23.10.2
 
 ### Breaking Changes
+- TX pool eviction in the legacy TX pool now favours keeping oldest transactions (more likely to evict higher nonces, less likely to introduce nonce gaps) [#6106](https://github.com/hyperledger/besu/pull/6106) and [#6146](https://github.com/hyperledger/besu/pull/6146)
 
 ### Deprecations
 

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java

@@ -73,8 +73,7 @@ public class BaseFeePendingTransactionsSorter extends AbstractPendingTransaction
                           .orElse(Wei.ZERO)
                           .getAsBigInteger()
                           .longValue())
-              .thenComparing(PendingTransaction::getAddedAt)
-              .thenComparing(PendingTransaction::getSequence)
+              .thenComparing(PendingTransaction::getSequence, Comparator.reverseOrder())
               .reversed());
 
   private final NavigableSet<PendingTransaction> prioritizedTransactionsDynamicRange =
@@ -87,8 +86,7 @@ public class BaseFeePendingTransactionsSorter extends AbstractPendingTransaction
                           .getMaxFeePerGas()
                           .map(maxFeePerGas -> maxFeePerGas.getAsBigInteger().longValue())
                           .orElse(pendingTx.getGasPrice().toLong()))
-              .thenComparing(PendingTransaction::getAddedAt)
-              .thenComparing(PendingTransaction::getSequence)
+              .thenComparing(PendingTransaction::getSequence, Comparator.reverseOrder())
               .reversed());
 
   @Override

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsTestBase.java

@@ -591,8 +591,8 @@ public abstract class AbstractPendingTransactionsTestBase {
 
   @Test
   public void shouldNotForceNonceOrderWhenSendersDiffer() {
-    final Transaction transaction1 = transactionWithNonceAndSender(0, KEYS1);
-    final Transaction transaction2 = transactionWithNonceAndSender(1, KEYS2);
+    final Transaction transaction1 = transactionWithNonceAndSender(1, KEYS2);
+    final Transaction transaction2 = transactionWithNonceAndSender(0, KEYS1);
 
     transactions.addTransaction(createLocalPendingTransaction(transaction1), Optional.empty());
     transactions.addTransaction(createLocalPendingTransaction(transaction2), Optional.empty());
@@ -604,7 +604,7 @@ public abstract class AbstractPendingTransactionsTestBase {
           return SELECTED;
         });
 
-    assertThat(iterationOrder).containsExactly(transaction2, transaction1);
+    assertThat(iterationOrder).containsExactly(transaction1, transaction2);
   }
 
   @Test
@@ -614,10 +614,10 @@ public abstract class AbstractPendingTransactionsTestBase {
     final Transaction transaction3 = transactionWithNonceAndSender(2, KEYS1);
     final Transaction transaction4 = transactionWithNonceAndSender(4, KEYS2);
 
-    transactions.addTransaction(createLocalPendingTransaction(transaction1), Optional.empty());
+    transactions.addTransaction(createLocalPendingTransaction(transaction3), Optional.empty());
     transactions.addTransaction(createLocalPendingTransaction(transaction4), Optional.empty());
     transactions.addTransaction(createLocalPendingTransaction(transaction2), Optional.empty());
-    transactions.addTransaction(createLocalPendingTransaction(transaction3), Optional.empty());
+    transactions.addTransaction(createLocalPendingTransaction(transaction1), Optional.empty());
 
     final List<Transaction> iterationOrder = new ArrayList<>();
     transactions.selectTransactions(
@@ -626,7 +626,7 @@ public abstract class AbstractPendingTransactionsTestBase {
           return SELECTED;
         });
 
-    // Ignoring nonces, the order would be 3, 2, 4, 1 but we have to delay 3 and 2 until after 1.
+    // Ignoring nonces, the order would be 3, 4, 2, 1 but we have to delay 3 and 2 until after 1.
     assertThat(iterationOrder)
         .containsExactly(transaction4, transaction1, transaction2, transaction3);
   }
@@ -853,6 +853,7 @@ public abstract class AbstractPendingTransactionsTestBase {
 
   @Test
   public void shouldPrioritizeGasPriceThenTimeAddedToPool() {
+    // Make sure the 100 gas price TX isn't dropped
     transactions.subscribeDroppedTransactions(
         transaction -> assertThat(transaction.getGasPrice().get().toLong()).isLessThan(100));
 
@@ -871,7 +872,8 @@ public abstract class AbstractPendingTransactionsTestBase {
                 })
             .collect(Collectors.toUnmodifiableList());
 
-    // This should kick the oldest tx with the low gas price out, namely the first one we added
+    // This should kick the highest-sequence tx with the low gas price out, namely the most-recent
+    // one we added
     final Account highPriceSender = mock(Account.class);
     final Transaction highGasPriceTransaction =
         transactionWithNonceSenderAndGasPrice(0, KEYS1, 100);
@@ -880,7 +882,9 @@ public abstract class AbstractPendingTransactionsTestBase {
     assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS);
 
     assertTransactionPending(highGasPriceTransaction);
-    assertTransactionNotPending(lowGasPriceTransactions.get(0));
-    lowGasPriceTransactions.stream().skip(1).forEach(this::assertTransactionPending);
+    assertTransactionNotPending(lowGasPriceTransactions.get(lowGasPriceTransactions.size() - 1));
+    lowGasPriceTransactions.stream()
+        .limit(lowGasPriceTransactions.size() - 1)
+        .forEach(this::assertTransactionPending);
   }
 }