Add test to check that TX selection doesn't prioritize TXs from the same sender (#6022)

* Add bool TX pool flag to allow TX selection to skip finding all TXs for the same sender Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Implement one approach to eliminating sender TX grouping. Note - this is WIP for discussion with others. Not sure this is the implementation we want. Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Make the no-sdr-grouping flag experimental, remove code no longer needed since https://github.com/hyperledger/besu/pull/6106 merged Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Update tests, tidy up empty lines Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Fix tests to be inline with recent sender-priority changes Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Address PR comments Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * More unit test updates Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Remove --Xtx-pool-disable-sender-grouping - the new legacy TX pool ordering removes the need for it Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Fix test merge error Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> --------- Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
1 year ago · 12fd0846b3
parent d6a9633794
commit 12fd0846b3
1 changed files with 94 additions and 0 deletions
--- 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
@ -65,6 +65,7 @@ import org.junit.jupiter.api.Test;
 public abstract class AbstractPendingTransactionsTestBase {

  protected static final int MAX_TRANSACTIONS = 5;
+  protected static final int MAX_TRANSACTIONS_LARGE_POOL = 15;
  private static final float LIMITED_TRANSACTIONS_BY_SENDER_PERCENTAGE = 0.8f;
  protected static final Supplier<SignatureAlgorithm> SIGNATURE_ALGORITHM =
      Suppliers.memoize(SignatureAlgorithmFactory::getInstance);
@ -93,9 +94,24 @@ public abstract class AbstractPendingTransactionsTestBase {
          .build();
  protected PendingTransactions senderLimitedTransactions =
      getPendingTransactions(senderLimitedConfig, Optional.empty());
+  protected AbstractPendingTransactionsSorter transactionsLarge =
+      getPendingTransactions(
+          ImmutableTransactionPoolConfiguration.builder()
+              .txPoolMaxSize(MAX_TRANSACTIONS_LARGE_POOL)
+              .txPoolLimitByAccountPercentage(Fraction.fromFloat(1.0f))
+              .build(),
+          Optional.empty());

  protected final Transaction transaction1 = createTransaction(2);
  protected final Transaction transaction2 = createTransaction(1);
+  protected final Transaction transaction3 = createTransaction(3);
+
+  protected final Transaction zeroGasPriceTX1Sdr1 = createZeroGasPriceTransactionSender1(1);
+  protected final Transaction zeroGasPriceTX2Sdr1 = createZeroGasPriceTransactionSender1(2);
+  protected final Transaction zeroGasPriceTX3Sdr1 = createZeroGasPriceTransactionSender1(3);
+  protected final Transaction zeroGasPriceTX1Sdr2 = createZeroGasPriceTransactionSender2(1);
+  protected final Transaction zeroGasPriceTX2Sdr2 = createZeroGasPriceTransactionSender2(2);
+  protected final Transaction zeroGasPriceTX3Sdr2 = createZeroGasPriceTransactionSender2(3);

  protected final PendingTransactionAddedListener listener =
      mock(PendingTransactionAddedListener.class);
@ -109,6 +125,7 @@ public abstract class AbstractPendingTransactionsTestBase {

  @Test
  public void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() {
+
    final Transaction localTransaction0 = createTransaction(0);
    transactions.addTransaction(createLocalPendingTransaction(localTransaction0), Optional.empty());
    assertThat(transactions.size()).isEqualTo(1);
@ -320,6 +337,62 @@ public abstract class AbstractPendingTransactionsTestBase {
    verifyNoInteractions(droppedListener);
  }

+  @Test
+  public void selectTransactionsInCorrectOrder() {
+    assertThat(
+            transactionsLarge.addTransaction(
+                createRemotePendingTransaction(zeroGasPriceTX2Sdr1), Optional.empty()))
+        .isEqualTo(ADDED);
+    assertThat(
+            transactionsLarge.addTransaction(
+                createRemotePendingTransaction(zeroGasPriceTX3Sdr1), Optional.empty()))
+        .isEqualTo(ADDED);
+    assertThat(
+            transactionsLarge.addTransaction(
+                createRemotePendingTransaction(zeroGasPriceTX1Sdr2), Optional.empty()))
+        .isEqualTo(ADDED);
+    assertThat(
+            transactionsLarge.addTransaction(
+                createRemotePendingTransaction(zeroGasPriceTX1Sdr1), Optional.empty()))
+        .isEqualTo(ADDED);
+    assertThat(
+            transactionsLarge.addTransaction(
+                createRemotePendingTransaction(zeroGasPriceTX2Sdr2), Optional.empty()))
+        .isEqualTo(ADDED);
+    assertThat(
+            transactionsLarge.addTransaction(
+                createRemotePendingTransaction(zeroGasPriceTX3Sdr2), Optional.empty()))
+        .isEqualTo(ADDED);
+
+    final List<Transaction> parsedTransactions = Lists.newArrayList();
+    transactionsLarge.selectTransactions(
+        pendingTx -> {
+          parsedTransactions.add(pendingTx.getTransaction());
+
+          if (parsedTransactions.size() == 6) {
+            return TransactionSelectionResult.BLOCK_OCCUPANCY_ABOVE_THRESHOLD;
+          }
+          return SELECTED;
+        });
+
+    // All 6 transactions should have been selected
+    assertThat(parsedTransactions.size()).isEqualTo(6);
+
+    // The order should be:
+    // sender 2, nonce 1
+    // sender 1, nonce 1
+    // sender 1, nonce 2
+    // sender 1, nonce 3
+    // sender 2, nonce 2
+    // sender 2, nonce 3
+    assertThat(parsedTransactions.get(0)).isEqualTo(zeroGasPriceTX1Sdr2);
+    assertThat(parsedTransactions.get(1)).isEqualTo(zeroGasPriceTX1Sdr1);
+    assertThat(parsedTransactions.get(2)).isEqualTo(zeroGasPriceTX2Sdr1);
+    assertThat(parsedTransactions.get(3)).isEqualTo(zeroGasPriceTX3Sdr1);
+    assertThat(parsedTransactions.get(4)).isEqualTo(zeroGasPriceTX2Sdr2);
+    assertThat(parsedTransactions.get(5)).isEqualTo(zeroGasPriceTX3Sdr2);
+  }
+
  @Test
  public void selectTransactionsUntilSelectorRequestsNoMore() {
    transactions.addTransaction(createRemotePendingTransaction(transaction1), Optional.empty());
@ -659,9 +732,30 @@ public abstract class AbstractPendingTransactionsTestBase {
    return new TransactionTestFixture()
        .value(Wei.of(transactionNumber))
        .nonce(transactionNumber)
+        .gasPrice(Wei.of(0))
        .createTransaction(KEYS1);
  }

+  protected Transaction createZeroGasPriceTransactionSender1(final long transactionNumber) {
+    return new TransactionTestFixture()
+        .value(Wei.of(transactionNumber))
+        .nonce(transactionNumber)
+        .gasPrice(Wei.of(0))
+        .maxFeePerGas(Optional.of(Wei.of(0)))
+        .maxPriorityFeePerGas(Optional.of(Wei.of(0)))
+        .createTransaction(KEYS1);
+  }
+
+  protected Transaction createZeroGasPriceTransactionSender2(final long transactionNumber) {
+    return new TransactionTestFixture()
+        .value(Wei.of(transactionNumber))
+        .nonce(transactionNumber)
+        .gasPrice(Wei.of(0))
+        .maxFeePerGas(Optional.of(Wei.of(0)))
+        .maxPriorityFeePerGas(Optional.of(Wei.of(0)))
+        .createTransaction(KEYS2);
+  }
+
  private PendingTransaction createRemotePendingTransaction(
      final Transaction transaction, final long addedAt) {
    return PendingTransaction.newPendingTransaction(transaction, false, false, addedAt);