From 8afad4159403776f43b30fa5071c7d3e46689365 Mon Sep 17 00:00:00 2001 From: Matt Whitehead Date: Fri, 10 Nov 2023 10:26:05 +0000 Subject: [PATCH] =?UTF-8?q?Apply=20the=20same=20reverse=20sort=20order=20a?= =?UTF-8?q?s=20https://github.com/hyperledger/b=E2=80=A6=20(#6146)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * Fix unit tests Signed-off-by: Matthew Whitehead * Update eviction unit tests to expect highest-sequence TXs be evicted first Signed-off-by: Matthew Whitehead * Update change log Signed-off-by: Matthew Whitehead * Spotless fixes Signed-off-by: Matthew Whitehead --------- Signed-off-by: Matthew Whitehead --- CHANGELOG.md | 1 + .../BaseFeePendingTransactionsSorter.java | 6 ++--- .../AbstractPendingTransactionsTestBase.java | 22 +++++++++++-------- 3 files changed, 16 insertions(+), 13 deletions(-) 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); } }