diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java index ec8d994564..d6108df149 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java +++ b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/TransactionPool.java @@ -156,11 +156,7 @@ private ValidationResult 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)); } diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionProcessor.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionProcessor.java index bc9a546512..d205c98eef 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionProcessor.java +++ b/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); diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidator.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidator.java index 88d5be0248..f90a53ad2f 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidator.java +++ b/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 validate(final Transaction tra @Override public ValidationResult 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 validateTransactionSignature( final Transaction transaction) { if (chainId.isPresent() diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/TransactionValidator.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/TransactionValidator.java index 4b7e16db1c..668199ab9d 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/TransactionValidator.java +++ b/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 validateForSender( - Transaction transaction, Account sender, OptionalLong maximumNonce); + Transaction transaction, Account sender, boolean allowFutureNonce); enum TransactionInvalidReason { WRONG_CHAIN_ID, diff --git a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/core/TransactionPoolTest.java b/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/core/TransactionPoolTest.java index 094ac1e395..075c44d77b 100644 --- a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/core/TransactionPoolTest.java +++ b/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()); } } diff --git a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidatorTest.java b/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidatorTest.java index d68859b2f9..48753bf3da 100644 --- a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetTransactionValidatorTest.java +++ b/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()); }