Skip to content

Commit

Permalink
Apply the same reverse sort order as https://github.com/hyperledger/b… (
Browse files Browse the repository at this point in the history
hyperledger#6146)

* Apply the same reverse sort order as hyperledger#6106 but to the base fee sorter

Signed-off-by: Matthew Whitehead <[email protected]>

* Fix unit tests

Signed-off-by: Matthew Whitehead <[email protected]>

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

Signed-off-by: Matthew Whitehead <[email protected]>

* Update change log

Signed-off-by: Matthew Whitehead <[email protected]>

* Spotless fixes

Signed-off-by: Matthew Whitehead <[email protected]>

---------

Signed-off-by: Matthew Whitehead <[email protected]>
  • Loading branch information
matthew1001 authored Nov 10, 2023
1 parent 4b58d07 commit 8afad41
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -604,7 +604,7 @@ public void shouldNotForceNonceOrderWhenSendersDiffer() {
return SELECTED;
});

assertThat(iterationOrder).containsExactly(transaction2, transaction1);
assertThat(iterationOrder).containsExactly(transaction1, transaction2);
}

@Test
Expand All @@ -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(
Expand All @@ -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);
}
Expand Down Expand Up @@ -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));

Expand All @@ -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);
Expand All @@ -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);
}
}

0 comments on commit 8afad41

Please sign in to comment.