Check transaction types when replacing in the transaction pool. (hype…

…rledger#924) * Add checks on the replacement of a transaction in the pool: - reject EIP-1559 for pre-fork blocks - accept both frontier and EIP-1559 transactions during phase 1 - reject frontier transactions after phase 2 is finalized Signed-off-by: Abdelhamid Bakhta <[email protected]> Signed-off-by: Danno Ferrin <[email protected]>
shemnon · May 23, 2020 · 3b24694 · 3b24694
1 parent f2e54dd
commit 3b24694
Show file tree

Hide file tree

Showing 16 changed files with 122 additions and 67 deletions.
diff --git a/...test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockCreatorTest.java b/...test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockCreatorTest.java
@@ -54,6 +54,7 @@
 import org.hyperledger.besu.testutil.TestClock;
 
 import java.util.List;
+import java.util.Optional;
 
 import com.google.common.collect.Lists;
 import org.apache.tuweni.bytes.Bytes;
@@ -127,7 +128,8 @@ public void proposerAddressCanBeExtractFromAConstructedBlock() {
                 5,
                 TestClock.fixed(),
                 metricsSystem,
-                blockchain::getChainHeadHeader),
+                blockchain::getChainHeadHeader,
+                Optional.empty()),
             protocolContext,
             protocolSchedule,
             gasLimit -> gasLimit,
@@ -161,7 +163,8 @@ public void insertsValidVoteIntoConstructedBlock() {
                 5,
                 TestClock.fixed(),
                 metricsSystem,
-                blockchain::getChainHeadHeader),
+                blockchain::getChainHeadHeader,
+                Optional.empty()),
             protocolContext,
             protocolSchedule,
             gasLimit -> gasLimit,
@@ -194,7 +197,8 @@ public void insertsNoVoteWhenAuthInValidators() {
                 5,
                 TestClock.fixed(),
                 metricsSystem,
-                blockchain::getChainHeadHeader),
+                blockchain::getChainHeadHeader,
+                Optional.empty()),
             protocolContext,
             protocolSchedule,
             gasLimit -> gasLimit,
@@ -230,7 +234,8 @@ public void insertsNoVoteWhenAtEpoch() {
                 5,
                 TestClock.fixed(),
                 metricsSystem,
-                blockchain::getChainHeadHeader),
+                blockchain::getChainHeadHeader,
+                Optional.empty()),
             protocolContext,
             protocolSchedule,
             gasLimit -> gasLimit,

diff --git a/...est/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutorTest.java b/...est/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutorTest.java
@@ -47,6 +47,7 @@
 import org.hyperledger.besu.testutil.TestClock;
 
 import java.util.List;
+import java.util.Optional;
 import java.util.Random;
 import java.util.function.Function;
 
@@ -101,7 +102,8 @@ public void extraDataCreatedOnEpochBlocksContainsValidators() {
                 5,
                 TestClock.fixed(),
                 metricsSystem,
-                () -> null),
+                () -> null,
+                Optional.empty()),
             proposerNodeKey,
             new MiningParameters(AddressHelpers.ofValue(1), Wei.ZERO, vanityData, false),
             mock(CliqueBlockScheduler.class),
@@ -140,7 +142,8 @@ public void extraDataForNonEpochBlocksDoesNotContainValidaors() {
                 5,
                 TestClock.fixed(),
                 metricsSystem,
-                () -> null),
+                () -> null,
+                Optional.empty()),
             proposerNodeKey,
             new MiningParameters(AddressHelpers.ofValue(1), Wei.ZERO, vanityData, false),
             mock(CliqueBlockScheduler.class),
@@ -179,7 +182,8 @@ public void shouldUseLatestVanityData() {
                 5,
                 TestClock.fixed(),
                 metricsSystem,
-                () -> null),
+                () -> null,
+                Optional.empty()),
             proposerNodeKey,
             new MiningParameters(AddressHelpers.ofValue(1), Wei.ZERO, initialVanityData, false),
             mock(CliqueBlockScheduler.class),

diff --git a/...integration-test/java/org/hyperledger/besu/consensus/ibft/support/TestContextBuilder.java b/...integration-test/java/org/hyperledger/besu/consensus/ibft/support/TestContextBuilder.java
@@ -303,7 +303,8 @@ private static ControllerAndState createControllerAndFinalState(
             1,
             clock,
             metricsSystem,
-            blockChain::getChainHeadHeader);
+            blockChain::getChainHeadHeader,
+            Optional.empty());
 
     final IbftBlockCreatorFactory blockCreatorFactory =
         new IbftBlockCreatorFactory(

diff --git a/...src/test/java/org/hyperledger/besu/consensus/ibft/blockcreation/IbftBlockCreatorTest.java b/...src/test/java/org/hyperledger/besu/consensus/ibft/blockcreation/IbftBlockCreatorTest.java
@@ -90,7 +90,8 @@ public void createdBlockPassesValidationRulesAndHasAppropriateHashAndMixHash() {
             5,
             TestClock.fixed(),
             metricsSystem,
-            blockchain::getChainHeadHeader);
+            blockchain::getChainHeadHeader,
+            Optional.empty());
 
     final IbftBlockCreator blockCreator =
         new IbftBlockCreator(

diff --git a/...st/java/org/hyperledger/besu/consensus/ibftlegacy/blockcreation/IbftBlockCreatorTest.java b/...st/java/org/hyperledger/besu/consensus/ibftlegacy/blockcreation/IbftBlockCreatorTest.java
@@ -104,7 +104,8 @@ public void headerProducedPassesValidationRules() {
                 5,
                 TestClock.fixed(),
                 metricsSystem,
-                blockchain::getChainHeadHeader),
+                blockchain::getChainHeadHeader,
+                Optional.empty()),
             protContext,
             protocolSchedule,
             parentGasLimit -> parentGasLimit,

diff --git a/...org/hyperledger/besu/ethereum/api/jsonrpc/methods/EthGetFilterChangesIntegrationTest.java b/...org/hyperledger/besu/ethereum/api/jsonrpc/methods/EthGetFilterChangesIntegrationTest.java
@@ -101,7 +101,8 @@ public void setUp() {
             MAX_HASHES,
             TestClock.fixed(),
             metricsSystem,
-            blockchain::getChainHeadHeader);
+            blockchain::getChainHeadHeader,
+            Optional.empty());
     final ProtocolContext<Void> protocolContext = executionContext.getProtocolContext();
 
     PeerTransactionTracker peerTransactionTracker = mock(PeerTransactionTracker.class);

diff --git a/...c/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java b/...c/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java
@@ -77,7 +77,8 @@ public class BlockTransactionSelectorTest {
           5,
           TestClock.fixed(),
           metricsSystem,
-          blockchain::getChainHeadHeader);
+          blockchain::getChainHeadHeader,
+          Optional.empty());
   private final MutableWorldState worldState = InMemoryStorageProvider.createInMemoryWorldState();
   private final Supplier<Boolean> isCancelled = () -> false;
   private final TransactionProcessor transactionProcessor = mock(TransactionProcessor.class);

diff --git a/...on/src/test/java/org/hyperledger/besu/ethereum/blockcreation/EthHashBlockCreatorTest.java b/...on/src/test/java/org/hyperledger/besu/ethereum/blockcreation/EthHashBlockCreatorTest.java
@@ -41,6 +41,7 @@
 import java.io.IOException;
 import java.math.BigInteger;
 import java.util.Collections;
+import java.util.Optional;
 import java.util.function.Function;
 
 import com.google.common.collect.Lists;
@@ -85,7 +86,8 @@ public void createMainnetBlock1() throws IOException {
             5,
             TestClock.fixed(),
             metricsSystem,
-            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader);
+            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader,
+            Optional.empty());
 
     final EthHashBlockCreator blockCreator =
         new EthHashBlockCreator(
@@ -137,7 +139,8 @@ public void createMainnetBlock1_fixedDifficulty1() {
             5,
             TestClock.fixed(),
             metricsSystem,
-            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader);
+            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader,
+            Optional.empty());
 
     final EthHashBlockCreator blockCreator =
         new EthHashBlockCreator(
@@ -184,7 +187,8 @@ public void rewardBeneficiary_zeroReward_skipZeroRewardsFalse() {
             5,
             TestClock.fixed(),
             metricsSystem,
-            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader);
+            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader,
+            Optional.empty());
 
     final EthHashBlockCreator blockCreator =
         new EthHashBlockCreator(
@@ -247,7 +251,8 @@ public void rewardBeneficiary_zeroReward_skipZeroRewardsTrue() {
             5,
             TestClock.fixed(),
             metricsSystem,
-            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader);
+            executionContextTestFixture.getProtocolContext().getBlockchain()::getChainHeadHeader,
+            Optional.empty());
 
     final EthHashBlockCreator blockCreator =
         new EthHashBlockCreator(

diff --git a/...n/src/test/java/org/hyperledger/besu/ethereum/blockcreation/EthHashMinerExecutorTest.java b/...n/src/test/java/org/hyperledger/besu/ethereum/blockcreation/EthHashMinerExecutorTest.java
@@ -25,6 +25,7 @@
 import org.hyperledger.besu.testutil.TestClock;
 import org.hyperledger.besu.util.Subscribers;
 
+import java.util.Optional;
 import java.util.function.Function;
 
 import org.junit.Test;
@@ -44,7 +45,8 @@ public void startingMiningWithoutCoinbaseThrowsException() {
             5,
             TestClock.fixed(),
             metricsSystem,
-            () -> null);
+            () -> null,
+            Optional.empty());
 
     final EthHashMinerExecutor executor =
         new EthHashMinerExecutor(
@@ -71,7 +73,8 @@ public void settingCoinbaseToNullThrowsException() {
             5,
             TestClock.fixed(),
             metricsSystem,
-            () -> null);
+            () -> null,
+            Optional.empty());
 
     final EthHashMinerExecutor executor =
         new EthHashMinerExecutor(

diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/fees/EIP1559.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/fees/EIP1559.java
@@ -15,6 +15,9 @@
 package org.hyperledger.besu.ethereum.core.fees;
 
 import static java.lang.Math.floorDiv;
+import static org.hyperledger.besu.ethereum.core.AcceptedTransactionTypes.FEE_MARKET_TRANSACTIONS;
+import static org.hyperledger.besu.ethereum.core.AcceptedTransactionTypes.FEE_MARKET_TRANSITIONAL_TRANSACTIONS;
+import static org.hyperledger.besu.ethereum.core.AcceptedTransactionTypes.FRONTIER_TRANSACTIONS;
 
 import org.hyperledger.besu.config.experimental.ExperimentalEIPs;
 import org.hyperledger.besu.ethereum.core.AcceptedTransactionTypes;
@@ -117,6 +120,16 @@ public boolean isValidFormat(
     }
   }
 
+  public boolean isValidTransaction(final long blockNumber, final Transaction transaction) {
+    return isValidFormat(
+        transaction,
+        isEIP1559Finalized(blockNumber)
+            ? FEE_MARKET_TRANSACTIONS
+            : isEIP1559(blockNumber)
+                ? FEE_MARKET_TRANSITIONAL_TRANSACTIONS
+                : FRONTIER_TRANSACTIONS);
+  }
+
   public boolean isValidGasLimit(final Transaction transaction) {
     if (transaction == null) {
       return false;

diff --git a/...eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java b/...eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/PendingTransactions.java
@@ -19,12 +19,12 @@
 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.core.fees.EIP1559;
 import org.hyperledger.besu.ethereum.mainnet.TransactionValidator.TransactionInvalidReason;
 import org.hyperledger.besu.metrics.BesuMetricCategory;
 import org.hyperledger.besu.plugin.services.MetricsSystem;
@@ -88,8 +88,7 @@ public class PendingTransactions {
   private final Counter localTransactionHashesAddedCounter;
 
   private final long maxPendingTransactions;
-  private final TransactionPoolReplacementHandler transactionReplacementHandler =
-      new TransactionPoolReplacementHandler();
+  private final TransactionPoolReplacementHandler transactionReplacementHandler;
   private final Supplier<BlockHeader> chainHeadHeaderSupplier;
 
   public PendingTransactions(
@@ -98,12 +97,14 @@ public PendingTransactions(
       final int maxPooledTransactionHashes,
       final Clock clock,
       final MetricsSystem metricsSystem,
-      final Supplier<BlockHeader> chainHeadHeaderSupplier) {
+      final Supplier<BlockHeader> chainHeadHeaderSupplier,
+      final Optional<EIP1559> eip1559) {
     this.maxTransactionRetentionHours = maxTransactionRetentionHours;
     this.maxPendingTransactions = maxPendingTransactions;
     this.clock = clock;
     this.newPooledHashes = EvictingQueue.create(maxPooledTransactionHashes);
     this.chainHeadHeaderSupplier = chainHeadHeaderSupplier;
+    this.transactionReplacementHandler = new TransactionPoolReplacementHandler(eip1559);
     final LabelledMetric<Counter> transactionAddedCounter =
         metricsSystem.createLabelledCounter(
             BesuMetricCategory.TRANSACTION_POOL,
@@ -275,14 +276,8 @@ private TransactionAddedStatus addTransactionForSenderAndNonce(
     final TransactionInfo existingTransaction =
         getTrackedTransactionBySenderAndNonce(transactionInfo);
     if (existingTransaction != null) {
-      final Optional<Long> baseFee =
-          ExperimentalEIPs.eip1559Enabled
-                  && (existingTransaction.getTransaction().isEIP1559Transaction()
-                      || transactionInfo.getTransaction().isEIP1559Transaction())
-              ? chainHeadHeaderSupplier.get().getBaseFee()
-              : Optional.empty();
       if (!transactionReplacementHandler.shouldReplace(
-          existingTransaction, transactionInfo, baseFee)) {
+          existingTransaction, transactionInfo, chainHeadHeaderSupplier.get())) {
         return REJECTED_UNDERPRICED_REPLACEMENT;
       }
       removeTransaction(existingTransaction.getTransaction());

diff --git a/.../src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolFactory.java b/.../src/main/java/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolFactory.java
@@ -51,7 +51,8 @@ public static TransactionPool createTransactionPool(
             transactionPoolConfiguration.getPooledTransactionHashesSize(),
             clock,
             metricsSystem,
-            protocolContext.getBlockchain()::getChainHeadHeader);
+            protocolContext.getBlockchain()::getChainHeadHeader,
+            eip1559);
 
     final PeerTransactionTracker transactionTracker = new PeerTransactionTracker();
     final TransactionsMessageSender transactionsMessageSender =

diff --git a/...ava/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolReplacementHandler.java b/...ava/org/hyperledger/besu/ethereum/eth/transactions/TransactionPoolReplacementHandler.java
@@ -16,35 +16,48 @@
 
 import static java.util.Arrays.asList;
 
+import org.hyperledger.besu.ethereum.core.BlockHeader;
+import org.hyperledger.besu.ethereum.core.fees.EIP1559;
 import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions.TransactionInfo;
 
 import java.util.List;
 import java.util.Optional;
 
 import com.google.common.annotations.VisibleForTesting;
 
-public class TransactionPoolReplacementHandler implements TransactionPoolReplacementRule {
+public class TransactionPoolReplacementHandler {
   private final List<TransactionPoolReplacementRule> rules;
+  private final Optional<EIP1559> eip1559;
 
-  public TransactionPoolReplacementHandler() {
-    this(asList(new TransactionReplacementByPriceRule()));
+  public TransactionPoolReplacementHandler(final Optional<EIP1559> eip1559) {
+    this(asList(new TransactionReplacementByPriceRule()), eip1559);
   }
 
   @VisibleForTesting
-  TransactionPoolReplacementHandler(final List<TransactionPoolReplacementRule> rules) {
+  TransactionPoolReplacementHandler(
+      final List<TransactionPoolReplacementRule> rules, final Optional<EIP1559> eip1559) {
     this.rules = rules;
+    this.eip1559 = eip1559;
   }
 
-  @Override
   public boolean shouldReplace(
       final TransactionInfo existingTransactionInfo,
       final TransactionInfo newTransactionInfo,
-      final Optional<Long> baseFee) {
+      final BlockHeader chainHeadHeader) {
     assert existingTransactionInfo != null;
     if (newTransactionInfo == null) {
       return false;
     }
-    return rules.stream()
-        .anyMatch(rule -> rule.shouldReplace(existingTransactionInfo, newTransactionInfo, baseFee));
+    return eip1559
+            .map(
+                eip ->
+                    eip.isValidTransaction(
+                        chainHeadHeader.getNumber(), newTransactionInfo.getTransaction()))
+            .orElse(newTransactionInfo.getTransaction().isFrontierTransaction())
+        && rules.stream()
+            .anyMatch(
+                rule ->
+                    rule.shouldReplace(
+                        existingTransactionInfo, newTransactionInfo, chainHeadHeader.getBaseFee()));
   }
 }