Remove Separation of EIP-1559 and pre-EIP-1559 Gas Tracking (#2359)

The segregated tracking was used in an older version of the spec but not anymore.

Signed-off-by: Ratan Rai Sur <ratan.r.sur@gmail.com>

ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java

@@ -177,7 +177,7 @@ public abstract class AbstractBlockCreator implements AsyncBlockCreator {
                   BodyValidation.transactionsRoot(transactionResults.getTransactions()))
               .receiptsRoot(BodyValidation.receiptsRoot(transactionResults.getReceipts()))
               .logsBloom(BodyValidation.logsBloom(transactionResults.getReceipts()))
-              .gasUsed(transactionResults.getTotalCumulativeGasUsed())
+              .gasUsed(transactionResults.getCumulativeGasUsed())
               .extraData(extraDataCalculator.get(parentHeader))
               .buildSealableBlockHeader();
 
@@ -220,8 +220,7 @@ public abstract class AbstractBlockCreator implements AsyncBlockCreator {
             isCancelled::get,
             miningBeneficiary,
             protocolSpec.getTransactionPriceCalculator(),
-            protocolSpec.getGasBudgetCalculator(),
+            protocolSpec.getGasBudgetCalculator());
-            protocolSpec.getEip1559());
 
     if (transactions.isPresent()) {
       return selector.evaluateTransactions(

ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelector.java

@@ -23,7 +23,6 @@ import org.hyperledger.besu.ethereum.core.Transaction;
 import org.hyperledger.besu.ethereum.core.TransactionReceipt;
 import org.hyperledger.besu.ethereum.core.Wei;
 import org.hyperledger.besu.ethereum.core.WorldUpdater;
-import org.hyperledger.besu.ethereum.core.fees.EIP1559;
 import org.hyperledger.besu.ethereum.core.fees.TransactionGasBudgetCalculator;
 import org.hyperledger.besu.ethereum.core.fees.TransactionPriceCalculator;
 import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactions;
@@ -36,11 +35,9 @@ import org.hyperledger.besu.ethereum.mainnet.ValidationResult;
 import org.hyperledger.besu.ethereum.processing.TransactionProcessingResult;
 import org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason;
 import org.hyperledger.besu.ethereum.vm.BlockHashLookup;
-import org.hyperledger.besu.plugin.data.TransactionType;
 
 import java.util.Collections;
 import java.util.List;
-import java.util.Optional;
 import java.util.concurrent.CancellationException;
 import java.util.function.Supplier;
 
@@ -79,21 +76,13 @@ public class BlockTransactionSelector {
 
     private final List<Transaction> transactions = Lists.newArrayList();
     private final List<TransactionReceipt> receipts = Lists.newArrayList();
-    private long frontierCumulativeGasUsed = 0;
+    private long cumulativeGasUsed = 0;
-    private long eip1559CumulativeGasUsed = 0;
 
     private void update(
-        final Transaction transaction,
+        final Transaction transaction, final TransactionReceipt receipt, final long gasUsed) {
-        final TransactionReceipt receipt,
-        final long gasUsed,
-        final Optional<EIP1559> eip1559) {
       transactions.add(transaction);
       receipts.add(receipt);
-      if (eip1559.isPresent() && transaction.getType().equals(TransactionType.EIP1559)) {
+      cumulativeGasUsed += gasUsed;
-        eip1559CumulativeGasUsed += gasUsed;
-      } else {
-        frontierCumulativeGasUsed += gasUsed;
-      }
     }
 
     public List<Transaction> getTransactions() {
@@ -104,16 +93,8 @@ public class BlockTransactionSelector {
       return receipts;
     }
 
-    public long getFrontierCumulativeGasUsed() {
+    public long getCumulativeGasUsed() {
-      return frontierCumulativeGasUsed;
+      return cumulativeGasUsed;
-    }
-
-    public long getEip1559CumulativeGasUsed() {
-      return eip1559CumulativeGasUsed;
-    }
-
-    public long getTotalCumulativeGasUsed() {
-      return frontierCumulativeGasUsed + eip1559CumulativeGasUsed;
     }
   }
 
@@ -127,7 +108,6 @@ public class BlockTransactionSelector {
   private final Address miningBeneficiary;
   private final TransactionPriceCalculator transactionPriceCalculator;
   private final TransactionGasBudgetCalculator transactionGasBudgetCalculator;
-  private final Optional<EIP1559> eip1559;
 
   private final TransactionSelectionResults transactionSelectionResult =
       new TransactionSelectionResults();
@@ -144,8 +124,7 @@ public class BlockTransactionSelector {
       final Supplier<Boolean> isCancelled,
       final Address miningBeneficiary,
       final TransactionPriceCalculator transactionPriceCalculator,
-      final TransactionGasBudgetCalculator transactionGasBudgetCalculator,
+      final TransactionGasBudgetCalculator transactionGasBudgetCalculator) {
-      final Optional<EIP1559> eip1559) {
     this.transactionProcessor = transactionProcessor;
     this.blockchain = blockchain;
     this.worldState = worldState;
@@ -158,7 +137,6 @@ public class BlockTransactionSelector {
     this.miningBeneficiary = miningBeneficiary;
     this.transactionPriceCalculator = transactionPriceCalculator;
     this.transactionGasBudgetCalculator = transactionGasBudgetCalculator;
-    this.eip1559 = eip1559;
   }
 
   /*
@@ -312,21 +290,14 @@ public class BlockTransactionSelector {
             ? 0
             : transaction.getGasLimit() - result.getGasRemaining();
 
-    final long cumulativeGasUsed;
+    final long cumulativeGasUsed =
-    if (eip1559.isPresent()) {
+        transactionSelectionResult.getCumulativeGasUsed() + gasUsedByTransaction;
-      cumulativeGasUsed =
-          transactionSelectionResult.getTotalCumulativeGasUsed() + gasUsedByTransaction;
-    } else {
-      cumulativeGasUsed =
-          transactionSelectionResult.getFrontierCumulativeGasUsed() + gasUsedByTransaction;
-    }
 
     transactionSelectionResult.update(
         transaction,
         transactionReceiptFactory.create(
             transaction.getType(), result, worldState, cumulativeGasUsed),
-        gasUsedByTransaction,
+        gasUsedByTransaction);
-        eip1559);
   }
 
   private TransactionProcessingResult publicResultForWhenWeHaveAPrivateTransaction(
@@ -339,49 +310,15 @@ public class BlockTransactionSelector {
         ValidationResult.valid());
   }
 
-  @SuppressWarnings("FallThrough")
   private boolean transactionTooLargeForBlock(
       final long blockNumber, final long gasLimit, final Transaction transaction) {
-
+    return !transactionGasBudgetCalculator.hasBudget(
-    final long blockGasRemaining;
+        transaction, blockNumber, gasLimit, transactionSelectionResult.cumulativeGasUsed);
-    if (eip1559.isPresent()) {
-      switch (transaction.getType()) {
-        case EIP1559:
-          return !transactionGasBudgetCalculator.hasBudget(
-              transaction,
-              blockNumber,
-              gasLimit,
-              transactionSelectionResult.eip1559CumulativeGasUsed);
-        case FRONTIER:
-        case ACCESS_LIST:
-          return !transactionGasBudgetCalculator.hasBudget(
-              transaction,
-              blockNumber,
-              gasLimit,
-              transactionSelectionResult.frontierCumulativeGasUsed);
-        default:
-          throw new IllegalStateException(
-              String.format(
-                  "Developer error. Supported transaction type %s doesn't have a block gas budget calculator",
-                  transaction.getType()));
-      }
-    } else {
-      blockGasRemaining =
-          processableBlockHeader.getGasLimit()
-              - transactionSelectionResult.getFrontierCumulativeGasUsed();
-      return transaction.getGasLimit() > blockGasRemaining;
-    }
   }
 
   private boolean blockOccupancyAboveThreshold() {
-    final double gasUsed, gasAvailable;
+    final double gasAvailable = processableBlockHeader.getGasLimit();
-    gasAvailable = processableBlockHeader.getGasLimit();
+    final double gasUsed = transactionSelectionResult.getCumulativeGasUsed();
-
-    if (eip1559.isPresent()) {
-      gasUsed = transactionSelectionResult.getTotalCumulativeGasUsed();
-    } else {
-      gasUsed = transactionSelectionResult.getFrontierCumulativeGasUsed();
-    }
     return (gasUsed / gasAvailable) >= minBlockOccupancyRatio;
   }
 }

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

@@ -105,6 +105,7 @@ public class BlockTransactionSelectorTest {
         .number(1)
         .gasLimit(gasLimit)
         .timestamp(Instant.now().toEpochMilli())
+        .baseFee(1L)
         .buildProcessableBlockHeader();
   }
 
@@ -133,15 +134,14 @@ public class BlockTransactionSelectorTest {
             this::isCancelled,
             miningBeneficiary,
             TransactionPriceCalculator.frontier(),
-            TransactionGasBudgetCalculator.frontier(),
+            TransactionGasBudgetCalculator.frontier());
-            Optional.empty());
 
     final BlockTransactionSelector.TransactionSelectionResults results =
         selector.buildTransactionListForBlock(blockHeader.getNumber(), blockHeader.getGasLimit());
 
     assertThat(results.getTransactions().size()).isEqualTo(0);
     assertThat(results.getReceipts().size()).isEqualTo(0);
-    assertThat(results.getFrontierCumulativeGasUsed()).isEqualTo(0);
+    assertThat(results.getCumulativeGasUsed()).isEqualTo(0);
   }
 
   @Test
@@ -172,8 +172,7 @@ public class BlockTransactionSelectorTest {
             this::isCancelled,
             miningBeneficiary,
             TransactionPriceCalculator.frontier(),
-            TransactionGasBudgetCalculator.frontier(),
+            TransactionGasBudgetCalculator.frontier());
-            Optional.empty());
 
     final BlockTransactionSelector.TransactionSelectionResults results =
         selector.buildTransactionListForBlock(blockHeader.getNumber(), blockHeader.getGasLimit());
@@ -181,7 +180,7 @@ public class BlockTransactionSelectorTest {
     assertThat(results.getTransactions().size()).isEqualTo(1);
     Assertions.assertThat(results.getTransactions()).contains(transaction);
     assertThat(results.getReceipts().size()).isEqualTo(1);
-    assertThat(results.getFrontierCumulativeGasUsed()).isEqualTo(95L);
+    assertThat(results.getCumulativeGasUsed()).isEqualTo(95L);
   }
 
   @Test
@@ -229,8 +228,7 @@ public class BlockTransactionSelectorTest {
             this::isCancelled,
             miningBeneficiary,
             TransactionPriceCalculator.frontier(),
-            TransactionGasBudgetCalculator.frontier(),
+            TransactionGasBudgetCalculator.frontier());
-            Optional.empty());
 
     final BlockTransactionSelector.TransactionSelectionResults results =
         selector.buildTransactionListForBlock(blockHeader.getNumber(), blockHeader.getGasLimit());
@@ -238,7 +236,7 @@ public class BlockTransactionSelectorTest {
     assertThat(results.getTransactions().size()).isEqualTo(4);
     assertThat(results.getTransactions().contains(transactionsToInject.get(1))).isFalse();
     assertThat(results.getReceipts().size()).isEqualTo(4);
-    assertThat(results.getFrontierCumulativeGasUsed()).isEqualTo(400);
+    assertThat(results.getCumulativeGasUsed()).isEqualTo(400);
   }
 
   @Test
@@ -274,8 +272,7 @@ public class BlockTransactionSelectorTest {
             this::isCancelled,
             miningBeneficiary,
             TransactionPriceCalculator.frontier(),
-            TransactionGasBudgetCalculator.frontier(),
+            TransactionGasBudgetCalculator.frontier());
-            Optional.empty());
 
     final BlockTransactionSelector.TransactionSelectionResults results =
         selector.buildTransactionListForBlock(blockHeader.getNumber(), blockHeader.getGasLimit());
@@ -284,7 +281,7 @@ public class BlockTransactionSelectorTest {
 
     assertThat(results.getTransactions().containsAll(transactionsToInject.subList(0, 3))).isTrue();
     assertThat(results.getReceipts().size()).isEqualTo(3);
-    assertThat(results.getFrontierCumulativeGasUsed()).isEqualTo(300);
+    assertThat(results.getCumulativeGasUsed()).isEqualTo(300);
 
     // Ensure receipts have the correct cumulative gas
     Assertions.assertThat(results.getReceipts().get(0).getCumulativeGasUsed()).isEqualTo(100);
@@ -310,8 +307,7 @@ public class BlockTransactionSelectorTest {
             this::isCancelled,
             miningBeneficiary,
             TransactionPriceCalculator.frontier(),
-            TransactionGasBudgetCalculator.frontier(),
+            TransactionGasBudgetCalculator.frontier());
-            Optional.empty());
 
     final Transaction tx = createTransaction(1);
     pendingTransactions.addRemoteTransaction(tx);
@@ -323,6 +319,68 @@ public class BlockTransactionSelectorTest {
     assertThat(pendingTransactions.size()).isEqualTo(0);
   }
 
+  @Test
+  public void useSingleGasSpaceForAllTransactions() {
+    final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(300);
+
+    final Address miningBeneficiary = AddressHelpers.ofValue(1);
+    final BlockTransactionSelector selector =
+        new BlockTransactionSelector(
+            transactionProcessor,
+            blockchain,
+            worldState,
+            pendingTransactions,
+            blockHeader,
+            this::createReceipt,
+            Wei.of(6),
+            0.8,
+            this::isCancelled,
+            miningBeneficiary,
+            TransactionPriceCalculator.eip1559(),
+            TransactionGasBudgetCalculator.frontier());
+
+    // this should fill up all the block space
+    final Transaction fillingLegacyTx =
+        Transaction.builder()
+            .type(TransactionType.FRONTIER)
+            .gasLimit(300)
+            .gasPrice(Wei.of(10))
+            .nonce(1)
+            .payload(Bytes.EMPTY)
+            .to(Address.ID)
+            .value(Wei.ZERO)
+            .sender(Address.ID)
+            .chainId(BigInteger.ONE)
+            .signAndBuild(keyPair);
+
+    // so we shouldn't include this
+    final Transaction extraEIP1559Tx =
+        Transaction.builder()
+            .type(TransactionType.EIP1559)
+            .nonce(0)
+            .maxPriorityFeePerGas(Wei.of(10))
+            .maxFeePerGas(Wei.of(10))
+            .gasLimit(50)
+            .to(Address.ID)
+            .value(Wei.of(0))
+            .payload(Bytes.EMPTY)
+            .chainId(BigInteger.ONE)
+            .signAndBuild(keyPair);
+
+    when(transactionProcessor.processTransaction(
+            any(), any(), any(), any(), any(), any(), anyBoolean(), any()))
+        .thenReturn(
+            TransactionProcessingResult.successful(
+                new ArrayList<>(), 0, 0, Bytes.EMPTY, ValidationResult.valid()));
+
+    pendingTransactions.addRemoteTransaction(fillingLegacyTx);
+    pendingTransactions.addRemoteTransaction(extraEIP1559Tx);
+    final BlockTransactionSelector.TransactionSelectionResults results =
+        selector.buildTransactionListForBlock(blockHeader.getNumber(), blockHeader.getGasLimit());
+
+    assertThat(results.getTransactions().size()).isEqualTo(1);
+  }
+
   @Test
   public void transactionTooLargeForBlockDoesNotPreventMoreBeingAddedIfBlockOccupancyNotReached() {
     final ProcessableBlockHeader blockHeader = createBlockWithGasLimit(300);
@@ -347,8 +405,7 @@ public class BlockTransactionSelectorTest {
             this::isCancelled,
             miningBeneficiary,
             TransactionPriceCalculator.frontier(),
-            TransactionGasBudgetCalculator.frontier(),
+            TransactionGasBudgetCalculator.frontier());
-            Optional.empty());
 
     final TransactionTestFixture txTestFixture = new TransactionTestFixture();
     // Add 3 transactions to the Pending Transactions, 79% of block, 100% of block and 10% of block
@@ -405,8 +462,7 @@ public class BlockTransactionSelectorTest {
             this::isCancelled,
             miningBeneficiary,
             TransactionPriceCalculator.frontier(),
-            TransactionGasBudgetCalculator.frontier(),
+            TransactionGasBudgetCalculator.frontier());
-            Optional.empty());
 
     final TransactionTestFixture txTestFixture = new TransactionTestFixture();
     // Add 4 transactions to the Pending Transactions 15% (ok), 79% (ok), 25% (too large), 10%
@@ -467,8 +523,7 @@ public class BlockTransactionSelectorTest {
             this::isCancelled,
             miningBeneficiary,
             TransactionPriceCalculator.frontier(),
-            TransactionGasBudgetCalculator.frontier(),
+            TransactionGasBudgetCalculator.frontier());
-            Optional.empty());
 
     final TransactionTestFixture txTestFixture = new TransactionTestFixture();
     final Transaction validTransaction =
@@ -549,8 +604,7 @@ public class BlockTransactionSelectorTest {
             this::isCancelled,
             miningBeneficiary,
             TransactionPriceCalculator.frontier(),
-            TransactionGasBudgetCalculator.frontier(),
+            TransactionGasBudgetCalculator.frontier());
-            Optional.empty());
 
     final BlockTransactionSelector.TransactionSelectionResults results =
         selector.buildTransactionListForBlock(blockHeader.getNumber(), blockHeader.getGasLimit());