From aa142514a4b58561f0a2b6883929ba4073d2c3a2 Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Thu, 9 Nov 2023 10:41:47 +0000 Subject: [PATCH 1/5] 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 --- .../sorter/BaseFeePendingTransactionsSorter.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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 From 3207b43b81e1efe78269eac04572ff37af75b4bd Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Thu, 9 Nov 2023 14:08:25 +0000 Subject: [PATCH 2/5] Fix unit tests Signed-off-by: Matthew Whitehead --- .../sorter/AbstractPendingTransactionsTestBase.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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..a45afd2d655 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); } From f71c0281007d6273b7908b0ca6167d25dc85f0cf Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Fri, 10 Nov 2023 09:25:30 +0000 Subject: [PATCH 3/5] Update eviction unit tests to expect highest-sequence TXs be evicted first Signed-off-by: Matthew Whitehead --- .../sorter/AbstractPendingTransactionsTestBase.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 a45afd2d655..b7ba0b56bb4 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 @@ -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,7 @@ 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 +881,7 @@ 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); } } From 5b9778e26224f49e5168f3c4e4d18ccd5e2b637f Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Fri, 10 Nov 2023 09:28:47 +0000 Subject: [PATCH 4/5] Update change log Signed-off-by: Matthew Whitehead --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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 From 9d4566b446a227c8f4233ca046b1e5687c5584ca Mon Sep 17 00:00:00 2001 From: Matthew Whitehead Date: Fri, 10 Nov 2023 09:48:46 +0000 Subject: [PATCH 5/5] Spotless fixes Signed-off-by: Matthew Whitehead --- .../sorter/AbstractPendingTransactionsTestBase.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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 b7ba0b56bb4..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 @@ -872,7 +872,8 @@ public void shouldPrioritizeGasPriceThenTimeAddedToPool() { }) .collect(Collectors.toUnmodifiableList()); - // This should kick the highest-sequence tx with the low gas price out, namely the most-recent 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); @@ -881,7 +882,9 @@ public void shouldPrioritizeGasPriceThenTimeAddedToPool() { assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS); assertTransactionPending(highGasPriceTransaction); - assertTransactionNotPending(lowGasPriceTransactions.get(lowGasPriceTransactions.size()-1)); - lowGasPriceTransactions.stream().limit(lowGasPriceTransactions.size()-1).forEach(this::assertTransactionPending); + assertTransactionNotPending(lowGasPriceTransactions.get(lowGasPriceTransactions.size() - 1)); + lowGasPriceTransactions.stream() + .limit(lowGasPriceTransactions.size() - 1) + .forEach(this::assertTransactionPending); } }