diff --git a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java index e8b7f3ddcf..8aa0d90d88 100644 --- a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java +++ b/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.getEip1559()); + protocolSpec.getGasBudgetCalculator()); if (transactions.isPresent()) { return selector.evaluateTransactions( diff --git a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelector.java b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelector.java index c1dde34e1d..6e4940d3de 100644 --- a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelector.java +++ b/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 transactions = Lists.newArrayList(); private final List receipts = Lists.newArrayList(); - private long frontierCumulativeGasUsed = 0; - private long eip1559CumulativeGasUsed = 0; + private long cumulativeGasUsed = 0; private void update( - final Transaction transaction, - final TransactionReceipt receipt, - final long gasUsed, - final Optional eip1559) { + final Transaction transaction, final TransactionReceipt receipt, final long gasUsed) { transactions.add(transaction); receipts.add(receipt); - if (eip1559.isPresent() && transaction.getType().equals(TransactionType.EIP1559)) { - eip1559CumulativeGasUsed += gasUsed; - } else { - frontierCumulativeGasUsed += gasUsed; - } + cumulativeGasUsed += gasUsed; } public List getTransactions() { @@ -104,16 +93,8 @@ public class BlockTransactionSelector { return receipts; } - public long getFrontierCumulativeGasUsed() { - return frontierCumulativeGasUsed; - } - - public long getEip1559CumulativeGasUsed() { - return eip1559CumulativeGasUsed; - } - - public long getTotalCumulativeGasUsed() { - return frontierCumulativeGasUsed + eip1559CumulativeGasUsed; + public long getCumulativeGasUsed() { + return cumulativeGasUsed; } } @@ -127,7 +108,6 @@ public class BlockTransactionSelector { private final Address miningBeneficiary; private final TransactionPriceCalculator transactionPriceCalculator; private final TransactionGasBudgetCalculator transactionGasBudgetCalculator; - private final Optional eip1559; private final TransactionSelectionResults transactionSelectionResult = new TransactionSelectionResults(); @@ -144,8 +124,7 @@ public class BlockTransactionSelector { final Supplier isCancelled, final Address miningBeneficiary, final TransactionPriceCalculator transactionPriceCalculator, - final TransactionGasBudgetCalculator transactionGasBudgetCalculator, - final Optional eip1559) { + final TransactionGasBudgetCalculator transactionGasBudgetCalculator) { 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; - if (eip1559.isPresent()) { - cumulativeGasUsed = - transactionSelectionResult.getTotalCumulativeGasUsed() + gasUsedByTransaction; - } else { - cumulativeGasUsed = - transactionSelectionResult.getFrontierCumulativeGasUsed() + gasUsedByTransaction; - } + final long cumulativeGasUsed = + transactionSelectionResult.getCumulativeGasUsed() + gasUsedByTransaction; transactionSelectionResult.update( transaction, transactionReceiptFactory.create( transaction.getType(), result, worldState, cumulativeGasUsed), - gasUsedByTransaction, - eip1559); + gasUsedByTransaction); } 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) { - - final long blockGasRemaining; - 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; - } + return !transactionGasBudgetCalculator.hasBudget( + transaction, blockNumber, gasLimit, transactionSelectionResult.cumulativeGasUsed); } private boolean blockOccupancyAboveThreshold() { - final double gasUsed, gasAvailable; - gasAvailable = processableBlockHeader.getGasLimit(); - - if (eip1559.isPresent()) { - gasUsed = transactionSelectionResult.getTotalCumulativeGasUsed(); - } else { - gasUsed = transactionSelectionResult.getFrontierCumulativeGasUsed(); - } + final double gasAvailable = processableBlockHeader.getGasLimit(); + final double gasUsed = transactionSelectionResult.getCumulativeGasUsed(); return (gasUsed / gasAvailable) >= minBlockOccupancyRatio; } } diff --git a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java index c45d3a81a3..ed91203f93 100644 --- a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockTransactionSelectorTest.java +++ b/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(), - Optional.empty()); + TransactionGasBudgetCalculator.frontier()); 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(), - Optional.empty()); + TransactionGasBudgetCalculator.frontier()); 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(), - Optional.empty()); + TransactionGasBudgetCalculator.frontier()); 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(), - Optional.empty()); + TransactionGasBudgetCalculator.frontier()); 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(), - Optional.empty()); + TransactionGasBudgetCalculator.frontier()); 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(), - Optional.empty()); + TransactionGasBudgetCalculator.frontier()); 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(), - Optional.empty()); + TransactionGasBudgetCalculator.frontier()); 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(), - Optional.empty()); + TransactionGasBudgetCalculator.frontier()); final TransactionTestFixture txTestFixture = new TransactionTestFixture(); final Transaction validTransaction = @@ -549,8 +604,7 @@ public class BlockTransactionSelectorTest { this::isCancelled, miningBeneficiary, TransactionPriceCalculator.frontier(), - TransactionGasBudgetCalculator.frontier(), - Optional.empty()); + TransactionGasBudgetCalculator.frontier()); final BlockTransactionSelector.TransactionSelectionResults results = selector.buildTransactionListForBlock(blockHeader.getNumber(), blockHeader.getGasLimit());