[EIP-1559] Transaction pool pricing rules must handle both frontier a…

…nd EIP-1559 transactions. (hyperledger#919)

* The transaction gas price is computed when adding a transaction into the local pool using eth_sendRawTransaction JSON RPC endpoint. Transaction price must be computed properly depending on the type of the transaction.
For instance `shouldReplace` method of PendingTransactions must be updated to deal with EIP-1559 transactions.

- Updated `PendingTransactions` to add access to the chain header in order to retrieve the last base fee value.
- Updated `TransactionReplacementByPriceRule` to compute the transaction price depending on the type of the transaction (frontier or eip-1559).
- Added unit tests to cover all possible replacement scenarios.

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Sally MacFarlane <sally.macfarlane@consensys.net>

consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockCreatorTest.java

@@ -126,7 +126,8 @@ public void proposerAddressCanBeExtractFromAConstructedBlock() {
                 5,
                 5,
                 TestClock.fixed(),
-                metricsSystem),
+                metricsSystem,
+                blockchain::getChainHeadHeader),
             protocolContext,
             protocolSchedule,
             gasLimit -> gasLimit,
@@ -159,7 +160,8 @@ public void insertsValidVoteIntoConstructedBlock() {
                 5,
                 5,
                 TestClock.fixed(),
-                metricsSystem),
+                metricsSystem,
+                blockchain::getChainHeadHeader),
             protocolContext,
             protocolSchedule,
             gasLimit -> gasLimit,
@@ -191,7 +193,8 @@ public void insertsNoVoteWhenAuthInValidators() {
                 5,
                 5,
                 TestClock.fixed(),
-                metricsSystem),
+                metricsSystem,
+                blockchain::getChainHeadHeader),
             protocolContext,
             protocolSchedule,
             gasLimit -> gasLimit,
@@ -226,7 +229,8 @@ public void insertsNoVoteWhenAtEpoch() {
                 5,
                 5,
                 TestClock.fixed(),
-                metricsSystem),
+                metricsSystem,
+                blockchain::getChainHeadHeader),
             protocolContext,
             protocolSchedule,
             gasLimit -> gasLimit,

consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutorTest.java

@@ -100,7 +100,8 @@ public void extraDataCreatedOnEpochBlocksContainsValidators() {
                 1,
                 5,
                 TestClock.fixed(),
-                metricsSystem),
+                metricsSystem,
+                () -> null),
             proposerNodeKey,
             new MiningParameters(AddressHelpers.ofValue(1), Wei.ZERO, vanityData, false),
             mock(CliqueBlockScheduler.class),
@@ -138,7 +139,8 @@ public void extraDataForNonEpochBlocksDoesNotContainValidaors() {
                 1,
                 5,
                 TestClock.fixed(),
-                metricsSystem),
+                metricsSystem,
+                () -> null),
             proposerNodeKey,
             new MiningParameters(AddressHelpers.ofValue(1), Wei.ZERO, vanityData, false),
             mock(CliqueBlockScheduler.class),
@@ -176,7 +178,8 @@ public void shouldUseLatestVanityData() {
                 1,
                 5,
                 TestClock.fixed(),
-                metricsSystem),
+                metricsSystem,
+                () -> null),
             proposerNodeKey,
             new MiningParameters(AddressHelpers.ofValue(1), Wei.ZERO, initialVanityData, false),
             mock(CliqueBlockScheduler.class),

consensus/ibft/src/integration-test/java/org/hyperledger/besu/consensus/ibft/support/TestContextBuilder.java

@@ -298,7 +298,12 @@ private static ControllerAndState createControllerAndFinalState(
 
     final PendingTransactions pendingTransactions =
         new PendingTransactions(
-            TransactionPoolConfiguration.DEFAULT_TX_RETENTION_HOURS, 1, 1, clock, metricsSystem);
+            TransactionPoolConfiguration.DEFAULT_TX_RETENTION_HOURS,
+            1,
+            1,
+            clock,
+            metricsSystem,
+            blockChain::getChainHeadHeader);
 
     final IbftBlockCreatorFactory blockCreatorFactory =
         new IbftBlockCreatorFactory(

consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/blockcreation/IbftBlockCreatorTest.java

@@ -89,7 +89,8 @@ public void createdBlockPassesValidationRulesAndHasAppropriateHashAndMixHash() {
             1,
             5,
             TestClock.fixed(),
-            metricsSystem);
+            metricsSystem,
+            blockchain::getChainHeadHeader);
 
     final IbftBlockCreator blockCreator =
         new IbftBlockCreator(

consensus/ibftlegacy/src/test/java/org/hyperledger/besu/consensus/ibftlegacy/blockcreation/IbftBlockCreatorTest.java

@@ -103,7 +103,8 @@ public void headerProducedPassesValidationRules() {
                 1,
                 5,
                 TestClock.fixed(),
-                metricsSystem),
+                metricsSystem,
+                blockchain::getChainHeadHeader),
             protContext,
             protocolSchedule,
             parentGasLimit -> parentGasLimit,

ethereum/api/src/integration-test/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/EthGetFilterChangesIntegrationTest.java

@@ -80,13 +80,7 @@ public class EthGetFilterChangesIntegrationTest {
   private TransactionPool transactionPool;
   private final MetricsSystem metricsSystem = new NoOpMetricsSystem();
 
-  private final PendingTransactions transactions =
-      new PendingTransactions(
-          TransactionPoolConfiguration.DEFAULT_TX_RETENTION_HOURS,
-          MAX_TRANSACTIONS,
-          MAX_HASHES,
-          TestClock.fixed(),
-          metricsSystem);
+  private PendingTransactions transactions;
 
   private static final int MAX_TRANSACTIONS = 5;
   private static final int MAX_HASHES = 5;
@@ -100,6 +94,14 @@ public class EthGetFilterChangesIntegrationTest {
   public void setUp() {
     final ExecutionContextTestFixture executionContext = ExecutionContextTestFixture.create();
     blockchain = executionContext.getBlockchain();
+    transactions =
+        new PendingTransactions(
+            TransactionPoolConfiguration.DEFAULT_TX_RETENTION_HOURS,
+            MAX_TRANSACTIONS,
+            MAX_HASHES,
+            TestClock.fixed(),
+            metricsSystem,
+            blockchain::getChainHeadHeader);
     final ProtocolContext<Void> protocolContext = executionContext.getProtocolContext();
 
     PeerTransactionTracker peerTransactionTracker = mock(PeerTransactionTracker.class);

ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java

@@ -69,14 +69,15 @@ public class BlockTransactionSelectorTest {
   private static final KeyPair keyPair = KeyPair.generate();
   private final MetricsSystem metricsSystem = new NoOpMetricsSystem();
 
+  private final Blockchain blockchain = new TestBlockchain();
   private final PendingTransactions pendingTransactions =
       new PendingTransactions(
           TransactionPoolConfiguration.DEFAULT_TX_RETENTION_HOURS,
           5,
           5,
           TestClock.fixed(),
-          metricsSystem);
-  private final Blockchain blockchain = new TestBlockchain();
+          metricsSystem,
+          blockchain::getChainHeadHeader);
   private final MutableWorldState worldState = InMemoryStorageProvider.createInMemoryWorldState();
   private final Supplier<Boolean> isCancelled = () -> false;
   private final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);

ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/EthHashBlockCreatorTest.java

@@ -84,7 +84,8 @@ public void createMainnetBlock1() throws IOException {
             1,
             5,
             TestClock.fixed(),
-            metricsSystem);
+            metricsSystem,
+            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader);
 
     final EthHashBlockCreator blockCreator =
         new EthHashBlockCreator(
@@ -135,7 +136,8 @@ public void createMainnetBlock1_fixedDifficulty1() {
             1,
             5,
             TestClock.fixed(),
-            metricsSystem);
+            metricsSystem,
+            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader);
 
     final EthHashBlockCreator blockCreator =
         new EthHashBlockCreator(
@@ -181,7 +183,8 @@ public void rewardBeneficiary_zeroReward_skipZeroRewardsFalse() {
             1,
             5,
             TestClock.fixed(),
-            metricsSystem);
+            metricsSystem,
+            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader);
 
     final EthHashBlockCreator blockCreator =
         new EthHashBlockCreator(
@@ -243,7 +246,8 @@ public void rewardBeneficiary_zeroReward_skipZeroRewardsTrue() {
             1,
             5,
             TestClock.fixed(),
-            metricsSystem);
+            metricsSystem,
+            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader);
 
     final EthHashBlockCreator blockCreator =
         new EthHashBlockCreator(

ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/EthHashMinerExecutorTest.java

@@ -43,7 +43,8 @@ public void startingMiningWithoutCoinbaseThrowsException() {
             1,
             5,
             TestClock.fixed(),
-            metricsSystem);
+            metricsSystem,
+            () -> null);
 
     final EthHashMinerExecutor executor =
         new EthHashMinerExecutor(
@@ -69,7 +70,8 @@ public void settingCoinbaseToNullThrowsException() {
             1,
             5,
             TestClock.fixed(),
-            metricsSystem);
+            metricsSystem,
+            () -> null);
 
     final EthHashMinerExecutor executor =
         new EthHashMinerExecutor(

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

@@ -19,8 +19,10 @@
 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.config.experimental.ExperimentalEIPs;
 import org.hyperledger.besu.ethereum.core.AccountTransactionOrder;
 import org.hyperledger.besu.ethereum.core.Address;
+import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.Hash;
 import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.mainnet.TransactionValidator.TransactionInvalidReason;
@@ -48,6 +50,7 @@
 import java.util.TreeSet;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -87,17 +90,20 @@ public class PendingTransactions {
   private final long maxPendingTransactions;
   private final TransactionPoolReplacementHandler transactionReplacementHandler =
       new TransactionPoolReplacementHandler();
+  private final Supplier<BlockHeader> chainHeadHeaderSupplier;
 
   public PendingTransactions(
       final int maxTransactionRetentionHours,
       final int maxPendingTransactions,
       final int maxPooledTransactionHashes,
       final Clock clock,
-      final MetricsSystem metricsSystem) {
+      final MetricsSystem metricsSystem,
+      final Supplier<BlockHeader> chainHeadHeaderSupplier) {
     this.maxTransactionRetentionHours = maxTransactionRetentionHours;
     this.maxPendingTransactions = maxPendingTransactions;
     this.clock = clock;
     this.newPooledHashes = EvictingQueue.create(maxPooledTransactionHashes);
+    this.chainHeadHeaderSupplier = chainHeadHeaderSupplier;
     final LabelledMetric<Counter> transactionAddedCounter =
         metricsSystem.createLabelledCounter(
             BesuMetricCategory.TRANSACTION_POOL,
@@ -269,7 +275,14 @@ private TransactionAddedStatus addTransactionForSenderAndNonce(
     final TransactionInfo existingTransaction =
         getTrackedTransactionBySenderAndNonce(transactionInfo);
     if (existingTransaction != null) {
-      if (!transactionReplacementHandler.shouldReplace(existingTransaction, transactionInfo)) {
+      final Optional<Long> baseFee =
+          ExperimentalEIPs.eip1559Enabled
+                  && (existingTransaction.getTransaction().isEIP1559Transaction()
+                      || transactionInfo.getTransaction().isEIP1559Transaction())
+              ? chainHeadHeaderSupplier.get().getBaseFee()
+              : Optional.empty();
+      if (!transactionReplacementHandler.shouldReplace(
+          existingTransaction, transactionInfo, baseFee)) {
         return REJECTED_UNDERPRICED_REPLACEMENT;
       }
       removeTransaction(existingTransaction.getTransaction());

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

@@ -50,7 +50,8 @@ public static TransactionPool createTransactionPool(
             transactionPoolConfiguration.getTxPoolMaxSize(),
             transactionPoolConfiguration.getPooledTransactionHashesSize(),
             clock,
-            metricsSystem);
+            metricsSystem,
+            protocolContext.getBlockchain()::getChainHeadHeader);
 
     final PeerTransactionTracker transactionTracker = new PeerTransactionTracker();
     final TransactionsMessageSender transactionsMessageSender =

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

@@ -19,6 +19,7 @@
 import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionInfo;
 
 import java.util.List;
+import java.util.Optional;
 
 import com.google.common.annotations.VisibleForTesting;
 
@@ -36,12 +37,14 @@ public TransactionPoolReplacementHandler() {
 
   @Override
   public boolean shouldReplace(
-      final TransactionInfo existingTransactionInfo, final TransactionInfo newTransactionInfo) {
+      final TransactionInfo existingTransactionInfo,
+      final TransactionInfo newTransactionInfo,
+      final Optional<Long> baseFee) {
     assert existingTransactionInfo != null;
     if (newTransactionInfo == null) {
       return false;
     }
     return rules.stream()
-        .anyMatch(rule -> rule.shouldReplace(existingTransactionInfo, newTransactionInfo));
+        .anyMatch(rule -> rule.shouldReplace(existingTransactionInfo, newTransactionInfo, baseFee));
   }
 }

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

@@ -16,9 +16,13 @@
 
 import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionInfo;
 
+import java.util.Optional;
+
 @FunctionalInterface
 public interface TransactionPoolReplacementRule {
 
   boolean shouldReplace(
-      TransactionInfo existingTransactionInfo, TransactionInfo newTransactionInfo);
+      TransactionInfo existingTransactionInfo,
+      TransactionInfo newTransactionInfo,
+      Optional<Long> baseFee);
 }

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

@@ -14,18 +14,34 @@
  */
 package org.hyperledger.besu.ethereum.eth.transactions;
 
+import org.hyperledger.besu.ethereum.core.Transaction;
+import org.hyperledger.besu.ethereum.core.Wei;
+import org.hyperledger.besu.ethereum.core.fees.TransactionPriceCalculator;
 import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionInfo;
 
+import java.util.Optional;
+
 public class TransactionReplacementByPriceRule implements TransactionPoolReplacementRule {
+  private static final TransactionPriceCalculator FRONTIER_CALCULATOR =
+      TransactionPriceCalculator.frontier();
+  private static final TransactionPriceCalculator EIP1559_CALCULATOR =
+      TransactionPriceCalculator.eip1559();
+
   @Override
   public boolean shouldReplace(
-      final TransactionInfo existingTransactionInfo, final TransactionInfo newTransactionInfo) {
+      final TransactionInfo existingTransactionInfo,
+      final TransactionInfo newTransactionInfo,
+      final Optional<Long> baseFee) {
     assert existingTransactionInfo.getTransaction() != null
         && newTransactionInfo.getTransaction() != null;
-    return newTransactionInfo
-            .getTransaction()
-            .getGasPrice()
-            .compareTo(existingTransactionInfo.getTransaction().getGasPrice())
+    return priceOf(newTransactionInfo.getTransaction(), baseFee)
+            .compareTo(priceOf(existingTransactionInfo.getTransaction(), baseFee))
         > 0;
   }
+
+  private Wei priceOf(final Transaction transaction, final Optional<Long> baseFee) {
+    final TransactionPriceCalculator transactionPriceCalculator =
+        transaction.isEIP1559Transaction() ? EIP1559_CALCULATOR : FRONTIER_CALCULATOR;
+    return transactionPriceCalculator.price(transaction, baseFee);
+  }
 }

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

@@ -59,7 +59,8 @@ public class PendingTransactionsTest {
           MAX_TRANSACTIONS,
           MAX_TRANSACTION_HASHES,
           TestClock.fixed(),
-          metricsSystem);
+          metricsSystem,
+          () -> null);
   private final Transaction transaction1 = createTransaction(2);
   private final Transaction transaction2 = createTransaction(1);
 
@@ -559,7 +560,8 @@ public void shouldEvictMultipleOldTransactions() {
             MAX_TRANSACTIONS,
             MAX_TRANSACTION_HASHES,
             clock,
-            metricsSystem);
+            metricsSystem,
+            () -> null);
 
     transactions.addRemoteTransaction(transaction1);
     assertThat(transactions.size()).isEqualTo(1);
@@ -581,7 +583,8 @@ public void shouldEvictSingleOldTransaction() {
             MAX_TRANSACTIONS,
             MAX_TRANSACTION_HASHES,
             clock,
-            metricsSystem);
+            metricsSystem,
+            () -> null);
     transactions.addRemoteTransaction(transaction1);
     assertThat(transactions.size()).isEqualTo(1);
     clock.step(2L, ChronoUnit.HOURS);
@@ -599,7 +602,8 @@ public void shouldEvictExclusivelyOldTransactions() {
             MAX_TRANSACTIONS,
             MAX_TRANSACTION_HASHES,
             clock,
-            metricsSystem);
+            metricsSystem,
+            () -> null);
     transactions.addRemoteTransaction(transaction1);
     assertThat(transactions.size()).isEqualTo(1);
     clock.step(3L, ChronoUnit.HOURS);