PAN-2057: Accept transactions in the pool with nonce above account se…

…nder nonce (#1065)

* PAN-2057: Accept transactions in the pool with nonce above account sender nonce

* Simplifying if statement

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java

@@ -156,11 +156,7 @@ private ValidationResult<TransactionInvalidReason> validateTransaction(
         .map(
             worldState -> {
               final Account senderAccount = worldState.get(transaction.getSender());
-              return getTransactionValidator()
-                  .validateForSender(
-                      transaction,
-                      senderAccount,
-                      pendingTransactions.getNextNonceForSender(transaction.getSender()));
+              return getTransactionValidator().validateForSender(transaction, senderAccount, true);
             })
         .orElseGet(() -> ValidationResult.invalid(CHAIN_HEAD_WORLD_STATE_NOT_AVAILABLE));
   }

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionProcessor.java

@@ -32,7 +32,6 @@
 
 import java.util.ArrayDeque;
 import java.util.Deque;
-import java.util.OptionalLong;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -158,8 +157,7 @@ public Result processTransaction(
 
     final Address senderAddress = transaction.getSender();
     final MutableAccount sender = worldState.getOrCreate(senderAddress);
-    validationResult =
-        transactionValidator.validateForSender(transaction, sender, OptionalLong.empty());
+    validationResult = transactionValidator.validateForSender(transaction, sender, false);
     if (!validationResult.isValid()) {
       LOG.warn("Invalid transaction: {}", validationResult.getErrorMessage());
       return Result.invalid(validationResult);

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidator.java

@@ -28,7 +28,6 @@
 import tech.pegasys.pantheon.ethereum.vm.GasCalculator;
 
 import java.util.OptionalInt;
-import java.util.OptionalLong;
 
 /**
  * Validates a transaction based on Frontier protocol runtime requirements.
@@ -86,47 +85,43 @@ public ValidationResult<TransactionInvalidReason> validate(final Transaction tra
 
   @Override
   public ValidationResult<TransactionInvalidReason> validateForSender(
-      final Transaction transaction, final Account sender, final OptionalLong maximumNonce) {
+      final Transaction transaction, final Account sender, final boolean allowFutureNonce) {
 
-    Wei balance = Account.DEFAULT_BALANCE;
-    long nonce = Account.DEFAULT_NONCE;
+    Wei senderBalance = Account.DEFAULT_BALANCE;
+    long senderNonce = Account.DEFAULT_NONCE;
 
     if (sender != null) {
-      balance = sender.getBalance();
-      nonce = sender.getNonce();
+      senderBalance = sender.getBalance();
+      senderNonce = sender.getNonce();
     }
 
-    if (transaction.getUpfrontCost().compareTo(balance) > 0) {
+    if (transaction.getUpfrontCost().compareTo(senderBalance) > 0) {
       return ValidationResult.invalid(
           UPFRONT_COST_EXCEEDS_BALANCE,
           String.format(
               "transaction up-front cost %s exceeds transaction sender account balance %s",
-              transaction.getUpfrontCost(), balance));
+              transaction.getUpfrontCost(), senderBalance));
     }
 
-    if (transaction.getNonce() < nonce) {
+    if (transaction.getNonce() < senderNonce) {
       return ValidationResult.invalid(
           NONCE_TOO_LOW,
           String.format(
-              "transaction nonce %s below sender account nonce %s", transaction.getNonce(), nonce));
+              "transaction nonce %s below sender account nonce %s",
+              transaction.getNonce(), senderNonce));
     }
 
-    if (violatesMaximumNonce(transaction, maximumNonce) && nonce != transaction.getNonce()) {
+    if (!allowFutureNonce && senderNonce != transaction.getNonce()) {
       return ValidationResult.invalid(
           INCORRECT_NONCE,
           String.format(
               "transaction nonce %s does not match sender account nonce %s.",
-              transaction.getNonce(), nonce));
+              transaction.getNonce(), senderNonce));
     }
 
     return ValidationResult.valid();
   }
 
-  private boolean violatesMaximumNonce(
-      final Transaction transaction, final OptionalLong maximumNonce) {
-    return !maximumNonce.isPresent() || transaction.getNonce() > maximumNonce.getAsLong();
-  }
-
   public ValidationResult<TransactionInvalidReason> validateTransactionSignature(
       final Transaction transaction) {
     if (chainId.isPresent()

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/TransactionValidator.java

@@ -15,8 +15,6 @@
 import tech.pegasys.pantheon.ethereum.core.Account;
 import tech.pegasys.pantheon.ethereum.core.Transaction;
 
-import java.util.OptionalLong;
-
 /** Validates transaction based on some criteria. */
 public interface TransactionValidator {
 
@@ -38,14 +36,16 @@ public interface TransactionValidator {
    *
    * @param transaction the transaction to validateMessageFrame.State.COMPLETED_FAILED
    * @param sender the sender account state to validate against
-   * @param maximumNonce the maximum transaction nonce. If not provided the transaction nonce must
-   *     equal the sender's current account nonce
+   * @param allowFutureNonce if true, transactions with nonce equal or higher than the account nonce
+   *     will be considered valid (used when received transactions in the transaction pool). If
+   *     false, only a transaction with the nonce equals the account nonce will be considered valid
+   *     (used when processing transactions).
    * @return An empty @{link Optional} if the transaction is considered valid; otherwise an @{code
    *     Optional} containing a {@link TransactionInvalidReason} that identifies why the transaction
    *     is invalid.
    */
   ValidationResult<TransactionInvalidReason> validateForSender(
-      Transaction transaction, Account sender, OptionalLong maximumNonce);
+      Transaction transaction, Account sender, boolean allowFutureNonce);
 
   enum TransactionInvalidReason {
     WRONG_CHAIN_ID,

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/core/TransactionPoolTest.java

@@ -19,6 +19,7 @@
 import static java.util.stream.Collectors.toList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.ArgumentMatchers.nullable;
@@ -44,7 +45,6 @@
 import tech.pegasys.pantheon.util.uint.UInt256;
 
 import java.util.List;
-import java.util.OptionalLong;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -211,7 +211,7 @@ public void shouldNotAddRemoteTransactionsThatAreInvalidAccordingToInvariantChec
   public void shouldNotAddRemoteTransactionsThatAreInvalidAccordingToStateDependentChecks() {
     givenTransactionIsValid(transaction2);
     when(transactionValidator.validate(transaction1)).thenReturn(valid());
-    when(transactionValidator.validateForSender(transaction1, null, OptionalLong.empty()))
+    when(transactionValidator.validateForSender(transaction1, null, true))
         .thenReturn(ValidationResult.invalid(NONCE_TOO_LOW));
 
     transactionPool.addRemoteTransactions(asList(transaction1, transaction2));
@@ -230,13 +230,13 @@ public void shouldAllowSequenceOfTransactionsWithIncreasingNonceFromSameSender()
 
     when(transactionValidator.validate(any(Transaction.class))).thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction1), nullable(Account.class), eq(OptionalLong.empty())))
+            eq(transaction1), nullable(Account.class), eq(true)))
         .thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction2), nullable(Account.class), eq(OptionalLong.of(2))))
+            eq(transaction2), nullable(Account.class), eq(true)))
         .thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction3), nullable(Account.class), eq(OptionalLong.of(3))))
+            eq(transaction3), nullable(Account.class), eq(true)))
         .thenReturn(valid());
 
     assertThat(transactionPool.addLocalTransaction(transaction1)).isEqualTo(valid());
@@ -258,13 +258,13 @@ public void shouldAllowSequenceOfTransactionsWithIncreasingNonceFromSameSender()
 
     when(transactionValidator.validate(any(Transaction.class))).thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction1), nullable(Account.class), eq(OptionalLong.empty())))
+            eq(transaction1), nullable(Account.class), eq(true)))
         .thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction2), nullable(Account.class), eq(OptionalLong.of(2))))
+            eq(transaction2), nullable(Account.class), eq(true)))
         .thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction3), nullable(Account.class), eq(OptionalLong.of(3))))
+            eq(transaction3), nullable(Account.class), eq(true)))
         .thenReturn(valid());
 
     transactionPool.addRemoteTransactions(asList(transaction3, transaction1, transaction2));
@@ -284,10 +284,10 @@ public void shouldNotNotifyBatchListenerWhenRemoteTransactionDoesNotReplaceExist
 
     when(transactionValidator.validate(any(Transaction.class))).thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction1), nullable(Account.class), eq(OptionalLong.empty())))
+            eq(transaction1), nullable(Account.class), eq(true)))
         .thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction2), nullable(Account.class), eq(OptionalLong.of(2))))
+            eq(transaction2), nullable(Account.class), eq(true)))
         .thenReturn(valid());
 
     transactionPool.addRemoteTransactions(singletonList(transaction1));
@@ -308,10 +308,10 @@ public void shouldNotNotifyBatchListenerWhenLocalTransactionDoesNotReplaceExisti
 
     when(transactionValidator.validate(any(Transaction.class))).thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction1), nullable(Account.class), eq(OptionalLong.empty())))
+            eq(transaction1), nullable(Account.class), eq(true)))
         .thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction2), nullable(Account.class), eq(OptionalLong.of(2))))
+            eq(transaction2), nullable(Account.class), eq(true)))
         .thenReturn(valid());
 
     transactionPool.addLocalTransaction(transaction1);
@@ -443,7 +443,7 @@ private Transaction createTransaction(final int transactionNumber) {
   private void givenTransactionIsValid(final Transaction transaction) {
     when(transactionValidator.validate(transaction)).thenReturn(valid());
     when(transactionValidator.validateForSender(
-            eq(transaction), nullable(Account.class), any(OptionalLong.class)))
+            eq(transaction), nullable(Account.class), anyBoolean()))
         .thenReturn(valid());
   }
 }

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidatorTest.java

@@ -31,8 +31,6 @@
 import tech.pegasys.pantheon.ethereum.core.Wei;
 import tech.pegasys.pantheon.ethereum.vm.GasCalculator;
 
-import java.util.OptionalLong;
-
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
@@ -80,7 +78,7 @@ public void shouldRejectTransactionWhenTransactionHasIncorrectChainId() {
   public void shouldRejectTransactionWhenSenderAccountDoesNotExist() {
     final MainnetTransactionValidator validator =
         new MainnetTransactionValidator(gasCalculator, false, 1);
-    assertThat(validator.validateForSender(basicTransaction, null, OptionalLong.empty()))
+    assertThat(validator.validateForSender(basicTransaction, null, false))
         .isEqualTo(ValidationResult.invalid(UPFRONT_COST_EXCEEDS_BALANCE));
   }
 
@@ -90,43 +88,29 @@ public void shouldRejectTransactionWhenTransactionNonceBelowAccountNonce() {
         new MainnetTransactionValidator(gasCalculator, false, 1);
 
     final Account account = accountWithNonce(basicTransaction.getNonce() + 1);
-    assertThat(validator.validateForSender(basicTransaction, account, OptionalLong.empty()))
+    assertThat(validator.validateForSender(basicTransaction, account, false))
         .isEqualTo(ValidationResult.invalid(NONCE_TOO_LOW));
   }
 
   @Test
-  public void shouldRejectTransactionWhenTransactionNonceAboveAccountNonce() {
+  public void
+      shouldRejectTransactionWhenTransactionNonceAboveAccountNonceAndFutureNonceIsNotAllowed() {
     final MainnetTransactionValidator validator =
         new MainnetTransactionValidator(gasCalculator, false, 1);
 
     final Account account = accountWithNonce(basicTransaction.getNonce() - 1);
-    assertThat(validator.validateForSender(basicTransaction, account, OptionalLong.empty()))
+    assertThat(validator.validateForSender(basicTransaction, account, false))
         .isEqualTo(ValidationResult.invalid(INCORRECT_NONCE));
   }
 
   @Test
-  public void shouldAcceptTransactionWhenNonceBetweenAccountNonceAndMaximumAllowedNonce() {
+  public void
+      shouldAcceptTransactionWhenTransactionNonceAboveAccountNonceAndFutureNonceIsAllowed() {
     final MainnetTransactionValidator validator =
         new MainnetTransactionValidator(gasCalculator, false, 1);
 
-    final Transaction transaction =
-        new TransactionTestFixture().nonce(10).createTransaction(senderKeys);
-    final Account account = accountWithNonce(5);
-
-    assertThat(validator.validateForSender(transaction, account, OptionalLong.of(15)))
-        .isEqualTo(ValidationResult.valid());
-  }
-
-  @Test
-  public void shouldAcceptTransactionWhenNonceEqualsMaximumAllowedNonce() {
-    final MainnetTransactionValidator validator =
-        new MainnetTransactionValidator(gasCalculator, false, 1);
-
-    final Transaction transaction =
-        new TransactionTestFixture().nonce(10).createTransaction(senderKeys);
-    final Account account = accountWithNonce(5);
-
-    assertThat(validator.validateForSender(transaction, account, OptionalLong.of(10)))
+    final Account account = accountWithNonce(basicTransaction.getNonce() - 1);
+    assertThat(validator.validateForSender(basicTransaction, account, true))
         .isEqualTo(ValidationResult.valid());
   }
 
@@ -139,7 +123,7 @@ public void shouldRejectTransactionWhenNonceExceedsMaximumAllowedNonce() {
         new TransactionTestFixture().nonce(11).createTransaction(senderKeys);
     final Account account = accountWithNonce(5);
 
-    assertThat(validator.validateForSender(transaction, account, OptionalLong.of(10)))
+    assertThat(validator.validateForSender(transaction, account, false))
         .isEqualTo(ValidationResult.invalid(INCORRECT_NONCE));
   }
 
@@ -153,9 +137,7 @@ public void transactionWithNullSenderCanBeValidIfGasPriceAndValueIsZero() {
     final Address arbitrarySender = Address.fromHexString("1");
     builder.gasPrice(Wei.ZERO).nonce(0).sender(arbitrarySender).value(Wei.ZERO);
 
-    assertThat(
-            validator.validateForSender(
-                builder.createTransaction(senderKeyPair), null, OptionalLong.of(10)))
+    assertThat(validator.validateForSender(builder.createTransaction(senderKeyPair), null, false))
         .isEqualTo(ValidationResult.valid());
   }