From aa5e1880ff580af06e9e3e3580f5343b641d2faf Mon Sep 17 00:00:00 2001 From: Danno Ferrin Date: Tue, 5 Jan 2021 15:03:11 -0700 Subject: [PATCH] Rework Pending Transaction Gap Calculation (#1748) Rework the pending transaction gap calculation to store only one gap. Signed-off-by: Danno Ferrin --- .../eth/transactions/PendingTransactions.java | 27 +++------ .../TransactionsForSenderInfo.java | 58 ++++++++++++------- .../transactions/PendingTransactionsTest.java | 26 ++++++++- 3 files changed, 70 insertions(+), 41 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 8302970795..5974b15a6e 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 @@ -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) { diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionsForSenderInfo.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionsForSenderInfo.java index 0d0c0378cd..aa0ef1aacd 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionsForSenderInfo.java +++ b/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 transactionsInfos; - private final Queue 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 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 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 streamTransactionInfos() { + return transactionsInfos.values().stream(); + } + + TransactionInfo getTransactionInfoForNonce(final long nonce) { + return transactionsInfos.get(nonce); + } } 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 4d36d7e520..e907f9e642 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 @@ -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)); } }