From 49e803c8dc1205e288b29be7645ee9d1db21953f Mon Sep 17 00:00:00 2001 From: mbaxter Date: Mon, 7 Oct 2019 14:19:27 -0400 Subject: [PATCH] Fix transaction tracking by sender (#93) Signed-off-by: Meredith Baxter --- .../eth/transactions/PendingTransactions.java | 39 +++++++---- .../transactions/PendingTransactionsTest.java | 65 +++++++++++++++++++ 2 files changed, 92 insertions(+), 12 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java index 21d75fec57..822fa0cec5 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java @@ -152,14 +152,7 @@ public class PendingTransactions { 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 @@ public class PendingTransactions { } private boolean addTransactionForSenderAndNonce(final TransactionInfo transactionInfo) { - final Map 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 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 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/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionsTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionsTest.java index 7d7cf42c94..3f0d0addf9 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionsTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactionsTest.java @@ -325,6 +325,71 @@ public class PendingTransactionsTest { assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1); } + @Test + public void shouldReplaceTransactionWithSameSenderAndNonce_multipleReplacements() { + final int replacedTxCount = 5; + final List 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 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);