diff --git a/CHANGELOG.md b/CHANGELOG.md index 13816f6bf15..6df8c4991f1 100644 --- 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/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java index 2ccb4d476ee..88781fc622b 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/BaseFeePendingTransactionsSorter.java +++ b/ethereum/eth/src/main/java/org/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 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/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsTestBase.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsTestBase.java index 4511dbd585e..502ff65c3e0 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsTestBase.java +++ b/ethereum/eth/src/test/java/org/hyperledger/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 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); } }