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

#6146) * Apply the same reverse sort order as #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>
hyperledger · Nov 10, 2023 · 8afad41 · 8afad41
1 parent 4b58d07
commit 8afad41
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 13 deletions.
diff --git a/CHANGELOG.md b/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
 

diff --git a/...g/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java b/...g/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java
@@ -73,8 +73,7 @@ public BaseFeePendingTransactionsSorter(
                           .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 BaseFeePendingTransactionsSorter(
                           .getMaxFeePerGas()
                           .map(maxFeePerGas -> maxFeePerGas.getAsBigInteger().longValue())
                           .orElse(pendingTx.getGasPrice().toLong()))
-              .thenComparing(PendingTransaction::getAddedAt)
-              .thenComparing(PendingTransaction::getSequence)
+              .thenComparing(PendingTransaction::getSequence, Comparator.reverseOrder())
               .reversed());
 
   @Override

diff --git a/...yperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsTestBase.java b/...yperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsTestBase.java
@@ -591,8 +591,8 @@ public void shouldIterateTransactionsFromSameSenderInNonceOrder() {
 
   @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 void shouldNotForceNonceOrderWhenSendersDiffer() {
           return SELECTED;
         });
 
-    assertThat(iterationOrder).containsExactly(transaction2, transaction1);
+    assertThat(iterationOrder).containsExactly(transaction1, transaction2);
   }
 
   @Test
@@ -614,10 +614,10 @@ public void shouldNotIncreasePriorityOfTransactionsBecauseOfNonceOrder() {
     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 void shouldNotIncreasePriorityOfTransactionsBecauseOfNonceOrder() {
           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 @@ protected static BlockHeader mockBlockHeader() {
 
   @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 void shouldPrioritizeGasPriceThenTimeAddedToPool() {
                 })
             .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 void shouldPrioritizeGasPriceThenTimeAddedToPool() {
     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);
   }
 }