Rework Pending Transaction Gap Calculation (#1748)

Rework the pending transaction gap calculation to store only one gap.

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java

@@ -235,7 +235,9 @@ public class PendingTransactions {
 
   private AccountTransactionOrder createSenderTransactionOrder(final Address address) {
     return new AccountTransactionOrder(
-        transactionsBySender.get(address).getTransactionsInfos().values().stream()
+        transactionsBySender
+            .get(address)
+            .streamTransactionInfos()
             .map(TransactionInfo::getTransaction));
   }
 
@@ -292,13 +294,8 @@ public class PendingTransactions {
   private void removeTransactionTrackedBySenderAndNonce(final Transaction transaction) {
     Optional.ofNullable(transactionsBySender.get(transaction.getSender()))
         .ifPresent(
-            transactionsForSender -> {
-              transactionsForSender.getTransactionsInfos().remove(transaction.getNonce());
-              if (transactionsForSender.getTransactionsInfos().isEmpty()) {
-                transactionsBySender.remove(transaction.getSender());
-                transactionsForSender.updateGaps();
-              }
-            });
+            transactionsForSender ->
+                transactionsForSender.removeTrackedTransaction(transaction.getNonce()));
   }
 
   private TransactionInfo getTrackedTransactionBySenderAndNonce(
@@ -306,7 +303,7 @@ public class PendingTransactions {
     final TransactionsForSenderInfo transactionsForSenderInfo =
         transactionsBySender.computeIfAbsent(
             transactionInfo.getSender(), key -> new TransactionsForSenderInfo());
-    return transactionsForSenderInfo.getTransactionsInfos().get(transactionInfo.getNonce());
+    return transactionsForSenderInfo.getTransactionInfoForNonce(transactionInfo.getNonce());
   }
 
   private void notifyTransactionAdded(final Transaction transaction) {
@@ -356,15 +353,9 @@ public class PendingTransactions {
 
   public OptionalLong getNextNonceForSender(final Address sender) {
     final TransactionsForSenderInfo transactionsForSenderInfo = transactionsBySender.get(sender);
-    if (transactionsForSenderInfo == null
-        || transactionsForSenderInfo.getTransactionsInfos().isEmpty()) {
-      return OptionalLong.empty();
-    } else {
-      final OptionalLong maybeNextGap = transactionsForSenderInfo.maybeNextGap();
-      return maybeNextGap.isEmpty()
-          ? OptionalLong.of(transactionsForSenderInfo.getTransactionsInfos().lastKey() + 1)
-          : maybeNextGap;
-    }
+    return transactionsForSenderInfo == null
+        ? OptionalLong.empty()
+        : transactionsForSenderInfo.maybeNextNonce();
   }
 
   public void tryEvictTransactionHash(final Hash hash) {

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionsForSenderInfo.java

@@ -17,17 +17,14 @@ package org.hyperledger.besu.ethereum.eth.transactions;
 
 import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionInfo;
 
-import java.util.Iterator;
 import java.util.NavigableMap;
 import java.util.OptionalLong;
-import java.util.PriorityQueue;
-import java.util.Queue;
 import java.util.TreeMap;
-import java.util.stream.LongStream;
+import java.util.stream.Stream;
 
 class TransactionsForSenderInfo {
   private final NavigableMap<Long, PendingTransactions.TransactionInfo> transactionsInfos;
-  private final Queue<Long> gaps = new PriorityQueue<>();
+  private OptionalLong nextGap = OptionalLong.empty();
 
   TransactionsForSenderInfo() {
     transactionsInfos = new TreeMap<>();
@@ -37,34 +34,55 @@ class TransactionsForSenderInfo {
       final long nonce, final PendingTransactions.TransactionInfo transactionInfo) {
     synchronized (transactionsInfos) {
       if (!transactionsInfos.isEmpty()) {
-        final long highestNonce = transactionsInfos.lastKey();
-        if (nonce > (highestNonce + 1)) {
-          LongStream.range(highestNonce + 1, nonce).forEach(gaps::add);
+        final long expectedNext = transactionsInfos.lastKey() + 1;
+        if (nonce > (expectedNext) && nextGap.isEmpty()) {
+          nextGap = OptionalLong.of(expectedNext);
         }
       }
       transactionsInfos.put(nonce, transactionInfo);
+      if (nonce == nextGap.orElse(-1)) {
+        findGap();
+      }
     }
   }
 
-  void updateGaps() {
+  void removeTrackedTransaction(final long nonce) {
+    transactionsInfos.remove(nonce);
     synchronized (transactionsInfos) {
-      final Iterator<Long> nonceIterator = transactionsInfos.keySet().iterator();
-      long previousNonce = -1;
-      while (nonceIterator.hasNext()) {
-        final long currentNonce = nonceIterator.next();
-        LongStream.range(previousNonce + 1, currentNonce).forEach(gaps::add);
-        previousNonce = currentNonce;
+      if (!transactionsInfos.isEmpty() && nonce != transactionsInfos.firstKey()) {
+        findGap();
       }
     }
   }
 
-  NavigableMap<Long, TransactionInfo> getTransactionsInfos() {
-    return transactionsInfos;
+  private void findGap() {
+    // find first gap
+    long expectedValue = transactionsInfos.firstKey();
+    for (final Long nonce : transactionsInfos.keySet()) {
+      if (expectedValue == nonce) {
+        // no gap, keep moving
+        expectedValue++;
+      } else {
+        nextGap = OptionalLong.of(expectedValue);
+        return;
+      }
+    }
+    nextGap = OptionalLong.empty();
   }
 
-  OptionalLong maybeNextGap() {
-    synchronized (transactionsInfos) {
-      return gaps.isEmpty() ? OptionalLong.empty() : OptionalLong.of(gaps.poll());
+  OptionalLong maybeNextNonce() {
+    if (transactionsInfos.isEmpty()) {
+      return OptionalLong.empty();
+    } else {
+      return nextGap.isEmpty() ? OptionalLong.of(transactionsInfos.lastKey() + 1) : nextGap;
     }
   }
+
+  Stream<TransactionInfo> streamTransactionInfos() {
+    return transactionsInfos.values().stream();
+  }
+
+  TransactionInfo getTransactionInfoForNonce(final long nonce) {
+    return transactionsInfos.get(nonce);
+  }
 }

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionsTest.java

@@ -572,7 +572,7 @@ public class PendingTransactionsTest {
     assertThat(transactions.getTransactionByHash(t.getHash())).isEmpty();
   }
 
-  private Transaction createTransaction(final int transactionNumber) {
+  private Transaction createTransaction(final long transactionNumber) {
     return new TransactionTestFixture()
         .value(Wei.of(transactionNumber))
         .nonce(transactionNumber)
@@ -689,8 +689,28 @@ public class PendingTransactionsTest {
         .hasValue(7);
   }
 
-  private void addLocalTransactions(final int... nonces) {
-    for (int nonce : nonces) {
+  @Test
+  public void assertThatCorrectNonceIsReturnedLargeGap() {
+    assertThat(transactions.getNextNonceForSender(transaction1.getSender())).isEmpty();
+    addLocalTransactions(1, 2, Long.MAX_VALUE);
+    assertThat(transactions.getNextNonceForSender(transaction1.getSender()))
+        .isPresent()
+        .hasValue(3);
+    addLocalTransactions(3);
+  }
+
+  @Test
+  public void assertThatCorrectNonceIsReturnedWithRepeatedTXes() {
+    assertThat(transactions.getNextNonceForSender(transaction1.getSender())).isEmpty();
+    addLocalTransactions(1, 2, 4, 4, 4, 4, 4, 4, 4, 4);
+    assertThat(transactions.getNextNonceForSender(transaction1.getSender()))
+        .isPresent()
+        .hasValue(3);
+    addLocalTransactions(3);
+  }
+
+  private void addLocalTransactions(final long... nonces) {
+    for (final long nonce : nonces) {
       transactions.addLocalTransaction(createTransaction(nonce));
     }
   }