Fix for NoSuchElementException when sending local transactions (#4569)

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

CHANGELOG.md

@@ -8,8 +8,10 @@
 - Updated jackson-databind library to version 2.13.4.2 addressing [CVE-2022-42003](https://nvd.nist.gov/vuln/detail/CVE-2022-42003)
 
 ### Bug Fixes
-- Fixed default fromBlock value and improved parameter interpetation in eth_getLogs RPC handler [#4513](https://github.com/hyperledger/besu/pull/4513)
+- Fixed default fromBlock value and improved parameter interpretation in eth_getLogs RPC handler [#4513](https://github.com/hyperledger/besu/pull/4513)
+- Fix for NoSuchElementException for missing invalid reason when rejecting a local sent transaction [#4569](https://github.com/hyperledger/besu/pull/4569) 
 - Corrects treating a block as bad on internal error during either validation or processing [#4512](https://github.com/hyperledger/besu/issues/4512)
+
 ## 22.10.0-RC2
 
 ### Breaking Changes

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/JsonRpcErrorConverter.java

@@ -63,6 +63,10 @@ public class JsonRpcErrorConverter {
         return JsonRpcError.GAS_PRICE_MUST_BE_ZERO;
       case ETHER_VALUE_NOT_SUPPORTED:
         return JsonRpcError.ETHER_VALUE_NOT_SUPPORTED;
+      case NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER:
+        return JsonRpcError.NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER;
+      case LOWER_NONCE_INVALID_TRANSACTION_EXISTS:
+        return JsonRpcError.LOWER_NONCE_INVALID_TRANSACTION_EXISTS;
       default:
         return JsonRpcError.INVALID_PARAMS;
     }

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/JsonRpcError.java

@@ -70,7 +70,11 @@ public enum JsonRpcError {
   GAS_PRICE_MUST_BE_ZERO(-3200, "gasPrice must be set to zero on a GoQuorum compatible network"),
   TRANSACTION_NOT_FOUND(-32000, "Transaction not found"),
   MAX_PRIORITY_FEE_PER_GAS_EXCEEDS_MAX_FEE_PER_GAS(
-      32000, "Max priority fee per gas exceeds max fee per gas"),
+      -32000, "Max priority fee per gas exceeds max fee per gas"),
+  NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER(
+      -32000, "Transaction nonce it too distant from current sender nonce"),
+  LOWER_NONCE_INVALID_TRANSACTION_EXISTS(
+      -32000, "An invalid transaction with a lower nonce exists"),
 
   // Execution engine failures
   UNKNOWN_PAYLOAD(-32001, "Payload does not exist / is not available"),

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/TransactionInvalidReason.java

@@ -50,5 +50,7 @@ public enum TransactionInvalidReason {
   GAS_PRICE_MUST_BE_ZERO,
   ETHER_VALUE_NOT_SUPPORTED,
   NONCE_TOO_HIGH,
-  UPFRONT_FEE_TOO_HIGH
+  UPFRONT_FEE_TOO_HIGH,
+  NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER,
+  LOWER_NONCE_INVALID_TRANSACTION_EXISTS
 }

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPool.java

@@ -17,8 +17,10 @@ package org.hyperledger.besu.ethereum.eth.transactions;
 import static java.util.Collections.singletonList;
 import static java.util.Optional.ofNullable;
 import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ADDED;
+import static org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter.TransactionAddedStatus.ALREADY_KNOWN;
 import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.CHAIN_HEAD_NOT_AVAILABLE;
 import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.CHAIN_HEAD_WORLD_STATE_NOT_AVAILABLE;
+import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.INTERNAL_ERROR;
 import static org.hyperledger.besu.util.Slf4jLambdaHelper.traceLambda;
 
 import org.hyperledger.besu.datatypes.Hash;
@@ -118,8 +120,17 @@ public class TransactionPool implements BlockAddedObserver {
       final TransactionAddedStatus transactionAddedStatus =
           pendingTransactions.addLocalTransaction(transaction, validationResult.maybeAccount);
       if (!transactionAddedStatus.equals(ADDED)) {
-        duplicateTransactionCounter.labels(LOCAL).inc();
-        return ValidationResult.invalid(transactionAddedStatus.getInvalidReason().orElseThrow());
+        if (transactionAddedStatus.equals(ALREADY_KNOWN)) {
+          duplicateTransactionCounter.labels(LOCAL).inc();
+        }
+        return ValidationResult.invalid(
+            transactionAddedStatus
+                .getInvalidReason()
+                .orElseGet(
+                    () -> {
+                      LOG.warn("Missing invalid reason for status {}", transactionAddedStatus);
+                      return INTERNAL_ERROR;
+                    }));
       }
       final Collection<Transaction> txs = singletonList(transaction);
       transactionBroadcaster.onTransactionsAdded(txs);

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java

@@ -560,8 +560,9 @@ public abstract class AbstractPendingTransactionsSorter {
   public enum TransactionAddedStatus {
     ALREADY_KNOWN(TransactionInvalidReason.TRANSACTION_ALREADY_KNOWN),
     REJECTED_UNDERPRICED_REPLACEMENT(TransactionInvalidReason.TRANSACTION_REPLACEMENT_UNDERPRICED),
-    NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER(),
-    LOWER_NONCE_INVALID_TRANSACTION_KNOWN(),
+    NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER(TransactionInvalidReason.NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER),
+    LOWER_NONCE_INVALID_TRANSACTION_KNOWN(
+        TransactionInvalidReason.LOWER_NONCE_INVALID_TRANSACTION_EXISTS),
     ADDED();
 
     private final Optional<TransactionInvalidReason> invalidReason;

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolTest.java

@@ -22,6 +22,7 @@ import static java.util.stream.Collectors.toList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.hyperledger.besu.ethereum.mainnet.ValidationResult.valid;
 import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.EXCEEDS_BLOCK_GAS_LIMIT;
+import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER;
 import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.NONCE_TOO_LOW;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
@@ -61,6 +62,8 @@ import org.hyperledger.besu.ethereum.eth.manager.EthProtocolManagerTestUtil;
 import org.hyperledger.besu.ethereum.eth.manager.EthScheduler;
 import org.hyperledger.besu.ethereum.eth.manager.RespondingEthPeer;
 import org.hyperledger.besu.ethereum.eth.messages.EthPV65;
+import org.hyperledger.besu.ethereum.eth.transactions.sorter.AbstractPendingTransactionsSorter;
+import org.hyperledger.besu.ethereum.eth.transactions.sorter.BaseFeePendingTransactionsSorter;
 import org.hyperledger.besu.ethereum.eth.transactions.sorter.GasPricePendingTransactionsSorter;
 import org.hyperledger.besu.ethereum.mainnet.MainnetBlockHeaderFunctions;
 import org.hyperledger.besu.ethereum.mainnet.MainnetTransactionValidator;
@@ -117,7 +120,7 @@ public class TransactionPoolTest {
   private MutableBlockchain blockchain;
   private TransactionBroadcaster transactionBroadcaster;
 
-  private GasPricePendingTransactionsSorter transactions;
+  private AbstractPendingTransactionsSorter transactions;
   private final Transaction transaction1 = createTransaction(1);
   private final Transaction transaction2 = createTransaction(2);
   private final ExecutionContextTestFixture executionContext = ExecutionContextTestFixture.create();
@@ -133,7 +136,7 @@ public class TransactionPoolTest {
   public void setUp() {
     blockchain = executionContext.getBlockchain();
     transactions =
-        new GasPricePendingTransactionsSorter(
+        new BaseFeePendingTransactionsSorter(
             ImmutableTransactionPoolConfiguration.builder()
                 .txPoolMaxSize(MAX_TRANSACTIONS)
                 .txPoolLimitByAccountPercentage(1)
@@ -619,6 +622,60 @@ public class TransactionPoolTest {
     verifyNoInteractions(transactionBroadcaster);
   }
 
+  @Test
+  public void shouldRejectRemoteTransactionsAnInvalidTransactionWithLowerNonceExists() {
+    final TransactionTestFixture builder = new TransactionTestFixture();
+    final Transaction invalidTx =
+        builder.gasLimit(genesisBlockGasLimit + 1).nonce(0).createTransaction(KEY_PAIR1);
+
+    final Transaction nextTx = builder.gasLimit(1).nonce(1).createTransaction(KEY_PAIR1);
+
+    givenTransactionIsValid(invalidTx);
+    givenTransactionIsValid(nextTx);
+
+    transactionPool.addRemoteTransactions(List.of(invalidTx));
+    transactionPool.addRemoteTransactions(List.of(nextTx));
+
+    assertTransactionNotPending(invalidTx);
+    assertTransactionNotPending(nextTx);
+    verifyNoInteractions(transactionBroadcaster);
+  }
+
+  @Test
+  public void shouldAcceptLocalTransactionsEvenIfAnInvalidTransactionWithLowerNonceExists() {
+    final TransactionTestFixture builder = new TransactionTestFixture();
+    final Transaction invalidTx =
+        builder.gasLimit(genesisBlockGasLimit + 1).nonce(0).createTransaction(KEY_PAIR1);
+
+    final Transaction nextTx = builder.gasLimit(1).nonce(1).createTransaction(KEY_PAIR1);
+
+    givenTransactionIsValid(invalidTx);
+    givenTransactionIsValid(nextTx);
+
+    assertThat(transactionPool.addLocalTransaction(invalidTx))
+        .isEqualTo(ValidationResult.invalid(EXCEEDS_BLOCK_GAS_LIMIT));
+    assertThat(transactionPool.addLocalTransaction(nextTx)).isEqualTo(ValidationResult.valid());
+
+    assertTransactionNotPending(invalidTx);
+    assertTransactionPending(nextTx);
+    verify(transactionBroadcaster).onTransactionsAdded(singletonList(nextTx));
+  }
+
+  @Test
+  public void shouldRejectLocalTransactionsWhenNonceTooFarInFuture() {
+    final TransactionTestFixture builder = new TransactionTestFixture();
+    final Transaction transaction1 =
+        builder.gasLimit(1).nonce(Integer.MAX_VALUE).createTransaction(KEY_PAIR1);
+
+    givenTransactionIsValid(transaction1);
+
+    assertThat(transactionPool.addLocalTransaction(transaction1))
+        .isEqualTo(ValidationResult.invalid(NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER));
+
+    assertTransactionNotPending(transaction1);
+    verifyNoInteractions(transactionBroadcaster);
+  }
+
   @Test
   public void shouldRejectRemoteTransactionsWhereGasLimitExceedBlockGasLimit() {
     final TransactionTestFixture builder = new TransactionTestFixture();