Update transaction pool error handling. (#904)

* Update transaction pool error handling.
Transactions added locally into the pool via `eth_sendRawTransaction` can now generate 2 new JSON RPC errors:
- `ETH_SEND_TX_ALREADY_KNOWN`: occurs when adding a transaction already present in the pool.
    - code: -32000
    - message: `Known Transaction`
- `ETH_SEND_TX_REPLACEMENT_UNDERPRICED`: occurs when adding a transaction already present in the pool.
    - code: -32000
    - message: `Replacement transaction underpriced`

### Changes summary
- Created `TransactionAddedStatus` enum.
    - `ALREADY_KNOWN`
    - `REJECTED_UNDERPRICED_REPLACEMENT`
    - `ADDED`
- Created 2 new errors in `JsonRpcError`.
- Updated JsonRpcErrorConverter to handle newly created errors.
- Updated `addLocalTransaction` method of `PendingTransactions` to return `TransactionAddedStatus` instead of boolean.
- Updated unit tests accordingly.

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>

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

@@ -45,7 +45,10 @@ public class JsonRpcErrorConverter {
         return JsonRpcError.GAS_PRICE_TOO_LOW;
       case OFFCHAIN_PRIVACY_GROUP_DOES_NOT_EXIST:
         return JsonRpcError.OFFCHAIN_PRIVACY_GROUP_DOES_NOT_EXIST;
-
+      case TRANSACTION_ALREADY_KNOWN:
+        return JsonRpcError.ETH_SEND_TX_ALREADY_KNOWN;
+      case TRANSACTION_REPLACEMENT_UNDERPRICED:
+        return JsonRpcError.ETH_SEND_TX_REPLACEMENT_UNDERPRICED;
       default:
         return JsonRpcError.INVALID_PARAMS;
     }

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

@@ -33,7 +33,8 @@ public enum JsonRpcError {
   ETH_SEND_TX_NOT_AVAILABLE(
       -32604,
       "The method eth_sendTransaction is not supported. Use eth_sendRawTransaction to send a signed transaction to Besu."),
-
+  ETH_SEND_TX_ALREADY_KNOWN(-32000, "Known transaction"),
+  ETH_SEND_TX_REPLACEMENT_UNDERPRICED(-32000, "Replacement transaction underpriced"),
   // P2P related errors
   P2P_DISABLED(-32000, "P2P has been disabled. This functionality is not available"),
   P2P_NETWORK_NOT_RUNNING(-32000, "P2P network is not running"),

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/TransactionValidator.java

@@ -73,6 +73,8 @@ public interface TransactionValidator {
     EXCEEDS_PER_TRANSACTION_GAS_LIMIT,
     INVALID_TRANSACTION_FORMAT,
     TRANSACTION_PRICE_TOO_LOW,
+    TRANSACTION_ALREADY_KNOWN,
+    TRANSACTION_REPLACEMENT_UNDERPRICED,
     // Private Transaction Invalid Reasons
     PRIVATE_TRANSACTION_FAILED,
     PRIVATE_NONCE_TOO_LOW,

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

@@ -15,11 +15,15 @@
 package org.hyperledger.besu.ethereum.eth.transactions;
 
 import static java.util.Comparator.comparing;
+import static org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionAddedStatus.ADDED;
+import static org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionAddedStatus.ALREADY_KNOWN;
+import static org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionAddedStatus.REJECTED_UNDERPRICED_REPLACEMENT;
 
 import org.hyperledger.besu.ethereum.core.AccountTransactionOrder;
 import org.hyperledger.besu.ethereum.core.Address;
 import org.hyperledger.besu.ethereum.core.Hash;
 import org.hyperledger.besu.ethereum.core.Transaction;
+import org.hyperledger.besu.ethereum.mainnet.TransactionValidator.TransactionInvalidReason;
 import org.hyperledger.besu.metrics.BesuMetricCategory;
 import org.hyperledger.besu.plugin.services.MetricsSystem;
 import org.hyperledger.besu.plugin.services.metrics.Counter;
@@ -130,11 +134,12 @@ public class PendingTransactions {
   public boolean addRemoteTransaction(final Transaction transaction) {
     final TransactionInfo transactionInfo =
         new TransactionInfo(transaction, false, clock.instant());
-    final boolean transactionAdded = addTransaction(transactionInfo);
-    if (transactionAdded) {
+    final TransactionAddedStatus transactionAddedStatus = addTransaction(transactionInfo);
+    final boolean added = transactionAddedStatus.equals(ADDED);
+    if (added) {
       remoteTransactionAddedCounter.inc();
     }
-    return transactionAdded;
+    return added;
   }
 
   boolean addTransactionHash(final Hash transactionHash) {
@@ -149,10 +154,10 @@ public class PendingTransactions {
   }
 
   @VisibleForTesting
-  public boolean addLocalTransaction(final Transaction transaction) {
-    final boolean transactionAdded =
+  public TransactionAddedStatus addLocalTransaction(final Transaction transaction) {
+    final TransactionAddedStatus transactionAdded =
         addTransaction(new TransactionInfo(transaction, true, clock.instant()));
-    if (transactionAdded) {
+    if (transactionAdded.equals(ADDED)) {
       localTransactionAddedCounter.inc();
     }
     return transactionAdded;
@@ -230,15 +235,17 @@ public class PendingTransactions {
             .map(TransactionInfo::getTransaction));
   }
 
-  private boolean addTransaction(final TransactionInfo transactionInfo) {
+  private TransactionAddedStatus addTransaction(final TransactionInfo transactionInfo) {
     Optional<Transaction> droppedTransaction = Optional.empty();
     synchronized (pendingTransactions) {
       if (pendingTransactions.containsKey(transactionInfo.getHash())) {
-        return false;
+        return ALREADY_KNOWN;
       }
 
-      if (!addTransactionForSenderAndNonce(transactionInfo)) {
-        return false;
+      final TransactionAddedStatus transactionAddedStatus =
+          addTransactionForSenderAndNonce(transactionInfo);
+      if (!transactionAddedStatus.equals(ADDED)) {
+        return transactionAddedStatus;
       }
       prioritizedTransactions.add(transactionInfo);
       pendingTransactions.put(transactionInfo.getHash(), transactionInfo);
@@ -252,20 +259,21 @@ public class PendingTransactions {
     }
     notifyTransactionAdded(transactionInfo.getTransaction());
     droppedTransaction.ifPresent(this::notifyTransactionDropped);
-    return true;
+    return ADDED;
   }
 
-  private boolean addTransactionForSenderAndNonce(final TransactionInfo transactionInfo) {
+  private TransactionAddedStatus addTransactionForSenderAndNonce(
+      final TransactionInfo transactionInfo) {
     final TransactionInfo existingTransaction =
         getTrackedTransactionBySenderAndNonce(transactionInfo);
     if (existingTransaction != null) {
       if (!shouldReplace(existingTransaction, transactionInfo)) {
-        return false;
+        return REJECTED_UNDERPRICED_REPLACEMENT;
       }
       removeTransaction(existingTransaction.getTransaction());
     }
     trackTransactionBySenderAndNonce(transactionInfo);
-    return true;
+    return ADDED;
   }
 
   private void trackTransactionBySenderAndNonce(final TransactionInfo transactionInfo) {
@@ -435,4 +443,24 @@ public class PendingTransactions {
 
     TransactionSelectionResult evaluateTransaction(final Transaction transaction);
   }
+
+  public enum TransactionAddedStatus {
+    ALREADY_KNOWN(TransactionInvalidReason.TRANSACTION_ALREADY_KNOWN),
+    REJECTED_UNDERPRICED_REPLACEMENT(TransactionInvalidReason.TRANSACTION_REPLACEMENT_UNDERPRICED),
+    ADDED();
+
+    private final Optional<TransactionInvalidReason> invalidReason;
+
+    TransactionAddedStatus() {
+      this.invalidReason = Optional.empty();
+    }
+
+    TransactionAddedStatus(final TransactionInvalidReason invalidReason) {
+      this.invalidReason = Optional.of(invalidReason);
+    }
+
+    public Optional<TransactionInvalidReason> getInvalidReason() {
+      return invalidReason;
+    }
+  }
 }

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

@@ -35,6 +35,7 @@ import org.hyperledger.besu.ethereum.eth.EthProtocol;
 import org.hyperledger.besu.ethereum.eth.manager.EthContext;
 import org.hyperledger.besu.ethereum.eth.manager.EthPeer;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
+import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionAddedStatus;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
 import org.hyperledger.besu.ethereum.mainnet.TransactionValidationParams;
 import org.hyperledger.besu.ethereum.mainnet.TransactionValidator;
@@ -153,18 +154,18 @@ public class TransactionPool implements BlockAddedObserver {
     }
     final ValidationResult<TransactionInvalidReason> validationResult =
         validateTransaction(transaction);
+    if (validationResult.isValid()) {
+      final TransactionAddedStatus transactionAddedStatus =
+          pendingTransactions.addLocalTransaction(transaction);
+      if (!transactionAddedStatus.equals(TransactionAddedStatus.ADDED)) {
+        duplicateTransactionCounter.labels(LOCAL).inc();
+        return ValidationResult.invalid(transactionAddedStatus.getInvalidReason().orElseThrow());
+      }
+      final Collection<Transaction> txs = singletonList(transaction);
+      transactionBatchAddedListener.onTransactionsAdded(txs);
+      pendingTransactionBatchAddedListener.ifPresent(it -> it.onTransactionsAdded(txs));
+    }
 
-    validationResult.ifValid(
-        () -> {
-          final boolean added = pendingTransactions.addLocalTransaction(transaction);
-          if (added) {
-            final Collection<Transaction> txs = singletonList(transaction);
-            transactionBatchAddedListener.onTransactionsAdded(txs);
-            pendingTransactionBatchAddedListener.ifPresent(it -> it.onTransactionsAdded(txs));
-          } else {
-            duplicateTransactionCounter.labels(LOCAL).inc();
-          }
-        });
     return validationResult;
   }
 

ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/task/GetPooledTransactionsFromPeerTaskTest.java

@@ -22,6 +22,7 @@ import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.core.TransactionTestFixture;
 import org.hyperledger.besu.ethereum.eth.manager.EthPeer;
 import org.hyperledger.besu.ethereum.eth.manager.ethtaskutils.PeerMessageTaskTest;
+import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionAddedStatus;
 import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem;
 import org.hyperledger.besu.plugin.services.MetricsSystem;
 
@@ -48,7 +49,8 @@ public class GetPooledTransactionsFromPeerTaskTest extends PeerMessageTaskTest<L
               .gasLimit(100000)
               .chainId(Optional.empty())
               .createTransaction(keyPair);
-      assertThat(transactionPool.getPendingTransactions().addLocalTransaction(tx)).isTrue();
+      assertThat(transactionPool.getPendingTransactions().addLocalTransaction(tx))
+          .isEqualTo(TransactionAddedStatus.ADDED);
       requestedData.add(tx);
     }
     return requestedData;

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

@@ -27,6 +27,7 @@ import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.core.TransactionTestFixture;
 import org.hyperledger.besu.ethereum.core.Util;
 import org.hyperledger.besu.ethereum.core.Wei;
+import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionAddedStatus;
 import org.hyperledger.besu.metrics.StubMetricsSystem;
 import org.hyperledger.besu.testutil.TestClock;
 
@@ -389,7 +390,8 @@ public class PendingTransactionsTest {
     }
     final Transaction finalReplacingTx = transactionWithNonceSenderAndGasPrice(1, KEYS1, 100);
     final Transaction independentTx = transactionWithNonceSenderAndGasPrice(2, KEYS1, 1);
-    assertThat(transactions.addLocalTransaction(finalReplacingTx)).isTrue();
+    assertThat(transactions.addLocalTransaction(finalReplacingTx))
+        .isEqualTo(TransactionAddedStatus.ADDED);
     assertThat(transactions.addRemoteTransaction(independentTx)).isTrue();
 
     // All tx's except the last duplicate should be removed
@@ -628,7 +630,8 @@ public class PendingTransactionsTest {
     assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(0);
     assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1);
 
-    assertThat(transactions.addLocalTransaction(transaction1)).isFalse();
+    assertThat(transactions.addLocalTransaction(transaction1))
+        .isEqualTo(TransactionAddedStatus.ALREADY_KNOWN);
     assertThat(transactions.size()).isEqualTo(1);
     assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(0);
     assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1);

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

@@ -41,6 +41,7 @@ import org.hyperledger.besu.ethereum.eth.manager.EthScheduler;
 import org.hyperledger.besu.ethereum.eth.manager.ForkIdManager;
 import org.hyperledger.besu.ethereum.eth.manager.RespondingEthPeer;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
+import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionAddedStatus;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec;
 import org.hyperledger.besu.ethereum.mainnet.TransactionValidator;
@@ -150,7 +151,7 @@ public class TransactionPoolFactoryTest {
     when(ethContext.getEthMessages()).thenReturn(mock(EthMessages.class));
     when(ethContext.getEthPeers()).thenReturn(ethPeers);
     when(ethContext.getScheduler()).thenReturn(ethScheduler);
-    when(pendingTransactions.addLocalTransaction(any())).thenReturn(true);
+    when(pendingTransactions.addLocalTransaction(any())).thenReturn(TransactionAddedStatus.ADDED);
     when(protocolSpec.getTransactionValidator()).thenReturn(transactionValidator);
     when(schedule.getByBlockNumber(anyLong())).thenReturn(protocolSpec);
     when(transactionValidator.validate(any())).thenReturn(ValidationResult.valid());