Fix transaction tracking by sender (hyperledger#93)

Signed-off-by: Meredith Baxter <meredith.baxter@consensys.net> Signed-off-by: edwardmack <ed@edwardmack.com>
ChainSafe · Nov 4, 2019 · b339b73 · b339b73
1 parent 0424421
commit b339b73
Show file tree

Hide file tree

Showing 2 changed files with 92 additions and 12 deletions.
diff --git a/...eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java b/...eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java
@@ -152,14 +152,7 @@ private void doRemoveTransaction(final Transaction transaction, final boolean ad
       final TransactionInfo removedTransactionInfo = pendingTransactions.remove(transaction.hash());
       if (removedTransactionInfo != null) {
         prioritizedTransactions.remove(removedTransactionInfo);
-        Optional.ofNullable(transactionsBySender.get(transaction.getSender()))
-            .ifPresent(
-                transactionsForSender -> {
-                  transactionsForSender.remove(transaction.getNonce());
-                  if (transactionsForSender.isEmpty()) {
-                    transactionsBySender.remove(transaction.getSender());
-                  }
-                });
+        removeTransactionTrackedBySenderAndNonce(transaction);
         incrementTransactionRemovedCounter(
             removedTransactionInfo.isReceivedFromLocalSource(), addedToBlock);
       }
@@ -240,20 +233,42 @@ private boolean addTransaction(final TransactionInfo transactionInfo) {
   }
 
   private boolean addTransactionForSenderAndNonce(final TransactionInfo transactionInfo) {
-    final Map<Long, TransactionInfo> transactionsForSender =
-        transactionsBySender.computeIfAbsent(transactionInfo.getSender(), key -> new TreeMap<>());
     final TransactionInfo existingTransaction =
-        transactionsForSender.get(transactionInfo.getNonce());
+        getTrackedTransactionBySenderAndNonce(transactionInfo);
     if (existingTransaction != null) {
       if (!shouldReplace(existingTransaction, transactionInfo)) {
         return false;
       }
       removeTransaction(existingTransaction.getTransaction());
     }
-    transactionsForSender.put(transactionInfo.getNonce(), transactionInfo);
+    trackTransactionBySenderAndNonce(transactionInfo);
     return true;
   }
 
+  private void trackTransactionBySenderAndNonce(final TransactionInfo transactionInfo) {
+    final Map<Long, TransactionInfo> transactionsForSender =
+        transactionsBySender.computeIfAbsent(transactionInfo.getSender(), key -> new TreeMap<>());
+    transactionsForSender.put(transactionInfo.getNonce(), transactionInfo);
+  }
+
+  private void removeTransactionTrackedBySenderAndNonce(final Transaction transaction) {
+    Optional.ofNullable(transactionsBySender.get(transaction.getSender()))
+        .ifPresent(
+            transactionsForSender -> {
+              transactionsForSender.remove(transaction.getNonce());
+              if (transactionsForSender.isEmpty()) {
+                transactionsBySender.remove(transaction.getSender());
+              }
+            });
+  }
+
+  private TransactionInfo getTrackedTransactionBySenderAndNonce(
+      final TransactionInfo transactionInfo) {
+    final Map<Long, TransactionInfo> transactionsForSender =
+        transactionsBySender.computeIfAbsent(transactionInfo.getSender(), key -> new TreeMap<>());
+    return transactionsForSender.get(transactionInfo.getNonce());
+  }
+
   private boolean shouldReplace(
       final TransactionInfo existingTransaction, final TransactionInfo newTransaction) {
     return newTransaction

diff --git a/...src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionsTest.java b/...src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionsTest.java
@@ -325,6 +325,71 @@ public void shouldReplaceTransactionWithSameSenderAndNonce() {
     assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1);
   }
 
+  @Test
+  public void shouldReplaceTransactionWithSameSenderAndNonce_multipleReplacements() {
+    final int replacedTxCount = 5;
+    final List<Transaction> replacedTransactions = new ArrayList<>();
+    for (int i = 0; i < replacedTxCount; i++) {
+      final Transaction duplicateTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, i + 1);
+      replacedTransactions.add(duplicateTx);
+      transactions.addRemoteTransaction(duplicateTx);
+    }
+    final Transaction finalReplacingTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, 100);
+    final Transaction independentTx = transactionWithNonceSenderAndGasPrice(2, KEYS1, 1);
+    assertThat(transactions.addRemoteTransaction(independentTx)).isTrue();
+    assertThat(transactions.addRemoteTransaction(finalReplacingTx)).isTrue();
+
+    // All tx's except the last duplicate should be removed
+    replacedTransactions.forEach(this::assertTransactionNotPending);
+    assertTransactionPending(finalReplacingTx);
+    // Tx with distinct nonce should be maintained
+    assertTransactionPending(independentTx);
+
+    assertThat(transactions.size()).isEqualTo(2);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(replacedTxCount + 2);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED))
+        .isEqualTo(replacedTxCount);
+  }
+
+  @Test
+  public void
+      shouldReplaceTransactionWithSameSenderAndNonce_multipleReplacementsAddedLocallyAndRemotely() {
+    final int replacedTxCount = 11;
+    final List<Transaction> replacedTransactions = new ArrayList<>();
+    int remoteDuplicateCount = 0;
+    for (int i = 0; i < replacedTxCount; i++) {
+      final Transaction duplicateTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, i + 1);
+      replacedTransactions.add(duplicateTx);
+      if (i % 2 == 0) {
+        transactions.addRemoteTransaction(duplicateTx);
+        remoteDuplicateCount++;
+      } else {
+        transactions.addLocalTransaction(duplicateTx);
+      }
+    }
+    final Transaction finalReplacingTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, 100);
+    final Transaction independentTx = transactionWithNonceSenderAndGasPrice(2, KEYS1, 1);
+    assertThat(transactions.addLocalTransaction(finalReplacingTx)).isTrue();
+    assertThat(transactions.addRemoteTransaction(independentTx)).isTrue();
+
+    // All tx's except the last duplicate should be removed
+    replacedTransactions.forEach(this::assertTransactionNotPending);
+    assertTransactionPending(finalReplacingTx);
+    // Tx with distinct nonce should be maintained
+    assertTransactionPending(independentTx);
+
+    final int localDuplicateCount = replacedTxCount - remoteDuplicateCount;
+    assertThat(transactions.size()).isEqualTo(2);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE))
+        .isEqualTo(remoteDuplicateCount + 1);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL))
+        .isEqualTo(localDuplicateCount + 1);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED))
+        .isEqualTo(remoteDuplicateCount);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, LOCAL, DROPPED))
+        .isEqualTo(localDuplicateCount);
+  }
+
   @Test
   public void shouldReplaceOnlyTransactionFromSenderWhenItHasTheSameNonce() {
     final Transaction transaction1 = transactionWithNonceSenderAndGasPrice(1, KEYS1, 1);