diff --git a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java index b353a1a01d5..fcb31e21eb7 100644 --- a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java +++ b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java @@ -30,6 +30,7 @@ import org.hyperledger.besu.ethereum.blockcreation.txselection.selectors.MinPriorityFeePerGasTransactionSelector; import org.hyperledger.besu.ethereum.blockcreation.txselection.selectors.PriceTransactionSelector; import org.hyperledger.besu.ethereum.blockcreation.txselection.selectors.ProcessingResultTransactionSelector; +import org.hyperledger.besu.ethereum.blockcreation.txselection.selectors.SkipSenderTransactionSelector; import org.hyperledger.besu.ethereum.chain.Blockchain; import org.hyperledger.besu.ethereum.core.MiningConfiguration; import org.hyperledger.besu.ethereum.core.MutableWorldState; @@ -151,6 +152,7 @@ public BlockTransactionSelector( private List createTransactionSelectors( final BlockSelectionContext context) { return List.of( + new SkipSenderTransactionSelector(context), new BlockSizeTransactionSelector(context), new BlobSizeTransactionSelector(context), new PriceTransactionSelector(context), @@ -442,7 +444,8 @@ private TransactionSelectionResult handleTransactionSelected( evaluationContext, BLOCK_SELECTION_TIMEOUT, txWorldStateUpdater); } - pluginTransactionSelector.onTransactionSelected(evaluationContext, processingResult); + notifySelected(evaluationContext, processingResult); + blockWorldStateUpdater = worldState.updater(); LOG.atTrace() .setMessage("Selected {} for block creation, evaluated in {}") @@ -475,7 +478,7 @@ private TransactionSelectionResult handleTransactionNotSelected( : selectionResult; transactionSelectionResults.updateNotSelected(evaluationContext.getTransaction(), actualResult); - pluginTransactionSelector.onTransactionNotSelected(evaluationContext, actualResult); + notifyNotSelected(evaluationContext, actualResult); LOG.atTrace() .setMessage( "Not selected {} for block creation with result {} (original result {}), evaluated in {}") @@ -550,6 +553,26 @@ private TransactionSelectionResult handleTransactionNotSelected( return handleTransactionNotSelected(evaluationContext, selectionResult); } + private void notifySelected( + final TransactionEvaluationContext evaluationContext, + final TransactionProcessingResult processingResult) { + + for (var selector : transactionSelectors) { + selector.onTransactionSelected(evaluationContext, processingResult); + } + pluginTransactionSelector.onTransactionSelected(evaluationContext, processingResult); + } + + private void notifyNotSelected( + final TransactionEvaluationContext evaluationContext, + final TransactionSelectionResult selectionResult) { + + for (var selector : transactionSelectors) { + selector.onTransactionNotSelected(evaluationContext, selectionResult); + } + pluginTransactionSelector.onTransactionNotSelected(evaluationContext, selectionResult); + } + private void checkCancellation() { if (isCancelled.get()) { throw new CancellationException("Cancelled during transaction selection."); diff --git a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/AbstractTransactionSelector.java b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/AbstractTransactionSelector.java index afb426e90b6..120a9c4f845 100644 --- a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/AbstractTransactionSelector.java +++ b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/AbstractTransactionSelector.java @@ -25,7 +25,7 @@ * transactions. */ public abstract class AbstractTransactionSelector { - final BlockSelectionContext context; + protected final BlockSelectionContext context; public AbstractTransactionSelector(final BlockSelectionContext context) { this.context = context; @@ -55,4 +55,24 @@ public abstract TransactionSelectionResult evaluateTransactionPostProcessing( final TransactionEvaluationContext evaluationContext, final TransactionSelectionResults blockTransactionResults, final TransactionProcessingResult processingResult); + + /** + * Method called when a transaction is selected to be added to a block. + * + * @param evaluationContext The current selection context + * @param processingResult The result of processing the selected transaction. + */ + public void onTransactionSelected( + final TransactionEvaluationContext evaluationContext, + final TransactionProcessingResult processingResult) {} + + /** + * Method called when a transaction is not selected to be added to a block. + * + * @param evaluationContext The current selection context + * @param transactionSelectionResult The transaction selection result + */ + public void onTransactionNotSelected( + final TransactionEvaluationContext evaluationContext, + final TransactionSelectionResult transactionSelectionResult) {} } diff --git a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/SkipSenderTransactionSelector.java b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/SkipSenderTransactionSelector.java new file mode 100644 index 00000000000..d29f045b7f6 --- /dev/null +++ b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/selectors/SkipSenderTransactionSelector.java @@ -0,0 +1,80 @@ +/* + * Copyright contributors to Besu. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ +package org.hyperledger.besu.ethereum.blockcreation.txselection.selectors; + +import org.hyperledger.besu.datatypes.Address; +import org.hyperledger.besu.ethereum.blockcreation.txselection.BlockSelectionContext; +import org.hyperledger.besu.ethereum.blockcreation.txselection.TransactionEvaluationContext; +import org.hyperledger.besu.ethereum.blockcreation.txselection.TransactionSelectionResults; +import org.hyperledger.besu.ethereum.processing.TransactionProcessingResult; +import org.hyperledger.besu.plugin.data.TransactionSelectionResult; + +import java.util.HashSet; +import java.util.Set; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class SkipSenderTransactionSelector extends AbstractTransactionSelector { + private static final Logger LOG = LoggerFactory.getLogger(SkipSenderTransactionSelector.class); + private final Set
skippedSenders = new HashSet<>(); + + public SkipSenderTransactionSelector(final BlockSelectionContext context) { + super(context); + } + + @Override + public TransactionSelectionResult evaluateTransactionPreProcessing( + final TransactionEvaluationContext evaluationContext, + final TransactionSelectionResults ignored) { + final var sender = evaluationContext.getTransaction().getSender(); + if (skippedSenders.contains(sender)) { + LOG.atTrace() + .setMessage("Not selecting tx {} since its sender {} is in the skip list") + .addArgument(() -> evaluationContext.getPendingTransaction().toTraceLog()) + .addArgument(sender) + .log(); + + return TransactionSelectionResult.SENDER_WITH_PREVIOUS_TX_NOT_SELECTED; + } + return TransactionSelectionResult.SELECTED; + } + + @Override + public TransactionSelectionResult evaluateTransactionPostProcessing( + final TransactionEvaluationContext evaluationContext, + final TransactionSelectionResults blockTransactionResults, + final TransactionProcessingResult processingResult) { + // All necessary checks were done in the pre-processing method, so nothing to do here. + return TransactionSelectionResult.SELECTED; + } + + /** + * When a transaction is not selected we can skip processing any following transaction from the + * same sender, since it will never be selected due to the nonce gap, so we add the sender to the + * skip list. + * + * @param evaluationContext The current selection context + * @param transactionSelectionResult The transaction selection result + */ + @Override + public void onTransactionNotSelected( + final TransactionEvaluationContext evaluationContext, + final TransactionSelectionResult transactionSelectionResult) { + final var sender = evaluationContext.getTransaction().getSender(); + skippedSenders.add(sender); + LOG.trace("Sender {} added to the skip list", sender); + } +} diff --git a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java index 70fd8f0d67a..8d9b51b97ff 100644 --- a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java +++ b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockTransactionSelectorTest.java @@ -18,6 +18,11 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.entry; import static org.awaitility.Awaitility.await; +import static org.hyperledger.besu.ethereum.blockcreation.AbstractBlockTransactionSelectorTest.Sender.SENDER1; +import static org.hyperledger.besu.ethereum.blockcreation.AbstractBlockTransactionSelectorTest.Sender.SENDER2; +import static org.hyperledger.besu.ethereum.blockcreation.AbstractBlockTransactionSelectorTest.Sender.SENDER3; +import static org.hyperledger.besu.ethereum.blockcreation.AbstractBlockTransactionSelectorTest.Sender.SENDER4; +import static org.hyperledger.besu.ethereum.blockcreation.AbstractBlockTransactionSelectorTest.Sender.SENDER5; import static org.hyperledger.besu.ethereum.core.MiningConfiguration.DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME; import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.EXECUTION_INTERRUPTED; import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.NONCE_TOO_LOW; @@ -62,7 +67,6 @@ import org.hyperledger.besu.ethereum.core.MutableWorldState; import org.hyperledger.besu.ethereum.core.ProcessableBlockHeader; import org.hyperledger.besu.ethereum.core.Transaction; -import org.hyperledger.besu.ethereum.core.TransactionReceipt; import org.hyperledger.besu.ethereum.difficulty.fixed.FixedDifficultyProtocolSchedule; import org.hyperledger.besu.ethereum.eth.manager.EthContext; import org.hyperledger.besu.ethereum.eth.manager.EthScheduler; @@ -80,7 +84,6 @@ import org.hyperledger.besu.evm.gascalculator.GasCalculator; import org.hyperledger.besu.evm.gascalculator.LondonGasCalculator; import org.hyperledger.besu.evm.internal.EvmConfiguration; -import org.hyperledger.besu.evm.worldstate.WorldState; import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; import org.hyperledger.besu.plugin.data.TransactionSelectionResult; import org.hyperledger.besu.plugin.services.MetricsSystem; @@ -127,10 +130,6 @@ public abstract class AbstractBlockTransactionSelectorTest { protected static final double MIN_OCCUPANCY_80_PERCENT = 0.8; protected static final double MIN_OCCUPANCY_100_PERCENT = 1; protected static final BigInteger CHAIN_ID = BigInteger.valueOf(42L); - protected static final KeyPair keyPair = - SignatureAlgorithmFactory.getInstance().generateKeyPair(); - protected static final Address sender = - Address.extract(Hash.hash(keyPair.getPublicKey().getEncodedBytes())); protected final MetricsSystem metricsSystem = new NoOpMetricsSystem(); protected GenesisConfig genesisConfig; @@ -180,7 +179,9 @@ public void setup() { worldState = InMemoryKeyValueStorageProvider.createInMemoryWorldState(); final var worldStateUpdater = worldState.updater(); - worldStateUpdater.createAccount(sender, 0, Wei.of(1_000_000_000L)); + Arrays.stream(Sender.values()) + .map(Sender::address) + .forEach(address -> worldStateUpdater.createAccount(address, 0, Wei.of(1_000_000_000L))); worldStateUpdater.commit(); when(protocolContext.getWorldStateArchive().getWorldState(any(WorldStateQueryParams.class))) @@ -303,9 +304,9 @@ public void invalidTransactionsAreSkippedButBlockStillFills() { Wei.ZERO, transactionSelectionService); - final List transactionsToInject = Lists.newArrayList(); + final List transactionsToInject = new ArrayList<>(5); for (int i = 0; i < 5; i++) { - final Transaction tx = createTransaction(i, Wei.of(7), 100_000); + final Transaction tx = createTransaction(i, Wei.of(7), 100_000, Sender.values()[i]); transactionsToInject.add(tx); if (i == 1) { ensureTransactionIsInvalid(tx, TransactionInvalidReason.NONCE_TOO_LOW); @@ -389,9 +390,9 @@ public void transactionTooLargeForBlockDoesNotPreventMoreBeingAddedIfBlockOccupa // NOTE - PendingTransactions outputs these in nonce order final Transaction[] txs = new Transaction[] { - createTransaction(1, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.79)), - createTransaction(2, Wei.of(10), blockHeader.getGasLimit()), - createTransaction(3, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.1)) + createTransaction(0, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.79), SENDER1), + createTransaction(0, Wei.of(10), blockHeader.getGasLimit(), SENDER2), + createTransaction(0, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.1), SENDER3) }; for (final Transaction tx : txs) { @@ -425,10 +426,10 @@ public void transactionSelectionStopsWhenSufficientBlockOccupancyIsReached() { // NOTE - PendingTransactions will output these in nonce order. final Transaction[] txs = new Transaction[] { - createTransaction(0, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.15)), - createTransaction(1, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.79)), - createTransaction(2, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.25)), - createTransaction(3, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.1)) + createTransaction(0, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.15), SENDER1), + createTransaction(0, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.79), SENDER2), + createTransaction(0, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.25), SENDER3), + createTransaction(0, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.1), SENDER4) }; for (Transaction tx : txs) { @@ -443,6 +444,44 @@ public void transactionSelectionStopsWhenSufficientBlockOccupancyIsReached() { .containsOnly(entry(txs[2], TransactionSelectionResult.BLOCK_OCCUPANCY_ABOVE_THRESHOLD)); } + @Test + public void ifATransactionIsNotSelectedFollowingOnesFromTheSameSenderAreSkipped() { + final ProcessableBlockHeader blockHeader = createBlock(300_000); + final Address miningBeneficiary = AddressHelpers.ofValue(1); + final BlockTransactionSelector selector = + createBlockSelectorAndSetupTxPool( + defaultTestMiningConfiguration, + transactionProcessor, + blockHeader, + miningBeneficiary, + Wei.ZERO, + transactionSelectionService); + + // Add 3 transactions from the same sender to the Pending Transactions + // first is selected + // second id not selected + // third is skipped, not processed, since cannot be selected due to the nonce gap + final Transaction[] txs = + new Transaction[] { + createTransaction(0, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.5), SENDER1), + createTransaction(1, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.6), SENDER1), + createTransaction(2, Wei.of(10), (long) (blockHeader.getGasLimit() * 0.1), SENDER1) + }; + + for (Transaction tx : txs) { + ensureTransactionIsValid(tx); + } + transactionPool.addRemoteTransactions(Arrays.stream(txs).toList()); + + final TransactionSelectionResults results = selector.buildTransactionListForBlock(); + + assertThat(results.getSelectedTransactions()).containsExactly(txs[0]); + assertThat(results.getNotSelectedTransactions()) + .containsOnly( + entry(txs[1], TransactionSelectionResult.TX_TOO_LARGE_FOR_REMAINING_GAS), + entry(txs[2], TransactionSelectionResult.SENDER_WITH_PREVIOUS_TX_NOT_SELECTED)); + } + @Test public void transactionSelectionStopsWhenBlockIsFull() { final ProcessableBlockHeader blockHeader = createBlock(3_000_000); @@ -471,18 +510,18 @@ public void transactionSelectionStopsWhenBlockIsFull() { // 4) min gas cost (not selected since selection stopped after tx 3) // NOTE - PendingTransactions outputs these in nonce order - final long gasLimit0 = (long) (blockHeader.getGasLimit() * 0.9); - final long gasLimit1 = (long) (blockHeader.getGasLimit() * 0.9); - final long gasLimit2 = blockHeader.getGasLimit() - gasLimit0 - minTxGasCost; - final long gasLimit3 = minTxGasCost; - final long gasLimit4 = minTxGasCost; + final long gasLimit0s1 = (long) (blockHeader.getGasLimit() * 0.9); + final long gasLimit1s1 = (long) (blockHeader.getGasLimit() * 0.9); + final long gasLimit0s2 = blockHeader.getGasLimit() - gasLimit0s1 - minTxGasCost; + final long gasLimit1s2 = minTxGasCost; + final long gasLimit2s2 = minTxGasCost; final List transactionsToInject = Lists.newArrayList(); - transactionsToInject.add(createTransaction(0, Wei.of(7), gasLimit0)); - transactionsToInject.add(createTransaction(1, Wei.of(7), gasLimit1)); - transactionsToInject.add(createTransaction(2, Wei.of(7), gasLimit2)); - transactionsToInject.add(createTransaction(3, Wei.of(7), gasLimit3)); - transactionsToInject.add(createTransaction(4, Wei.of(7), gasLimit4)); + transactionsToInject.add(createTransaction(0, Wei.of(7), gasLimit0s1, SENDER1)); + transactionsToInject.add(createTransaction(1, Wei.of(7), gasLimit1s1, SENDER1)); + transactionsToInject.add(createTransaction(0, Wei.of(7), gasLimit0s2, SENDER2)); + transactionsToInject.add(createTransaction(1, Wei.of(7), gasLimit1s2, SENDER2)); + transactionsToInject.add(createTransaction(2, Wei.of(7), gasLimit2s2, SENDER2)); for (final Transaction tx : transactionsToInject) { ensureTransactionIsValid(tx); @@ -532,16 +571,16 @@ public void transactionSelectionStopsWhenRemainingGasIsNotEnoughForAnyMoreTransa // 3) min gas cost (skipped since not enough gas remaining) // NOTE - PendingTransactions outputs these in nonce order - final long gasLimit0 = (long) (blockHeader.getGasLimit() * 0.9); - final long gasLimit1 = (long) (blockHeader.getGasLimit() * 0.9); - final long gasLimit2 = blockHeader.getGasLimit() - gasLimit0 - (minTxGasCost - 1); - final long gasLimit3 = minTxGasCost; + final long gasLimit0s1 = (long) (blockHeader.getGasLimit() * 0.9); + final long gasLimit1s1 = (long) (blockHeader.getGasLimit() * 0.9); + final long gasLimit0s2 = blockHeader.getGasLimit() - gasLimit0s1 - (minTxGasCost - 1); + final long gasLimit1s2 = minTxGasCost; - final List transactionsToInject = Lists.newArrayList(); - transactionsToInject.add(createTransaction(0, Wei.of(10), gasLimit0)); - transactionsToInject.add(createTransaction(1, Wei.of(10), gasLimit1)); - transactionsToInject.add(createTransaction(2, Wei.of(10), gasLimit2)); - transactionsToInject.add(createTransaction(3, Wei.of(10), gasLimit3)); + final List transactionsToInject = new ArrayList<>(4); + transactionsToInject.add(createTransaction(0, Wei.of(10), gasLimit0s1, SENDER1)); + transactionsToInject.add(createTransaction(1, Wei.of(10), gasLimit1s1, SENDER1)); + transactionsToInject.add(createTransaction(0, Wei.of(10), gasLimit0s2, SENDER2)); + transactionsToInject.add(createTransaction(1, Wei.of(10), gasLimit1s2, SENDER2)); for (final Transaction tx : transactionsToInject) { ensureTransactionIsValid(tx); @@ -600,13 +639,13 @@ public void transactionSelectionPluginShouldWork_PreProcessing() { final ProcessableBlockHeader blockHeader = createBlock(300_000); final Address miningBeneficiary = AddressHelpers.ofValue(1); - final Transaction selected = createTransaction(0, Wei.of(10), 21_000); + final Transaction selected = createTransaction(0, Wei.of(10), 21_000, SENDER1); ensureTransactionIsValid(selected, 21_000, 0); - final Transaction notSelectedTransient = createTransaction(1, Wei.of(10), 21_000); + final Transaction notSelectedTransient = createTransaction(1, Wei.of(10), 21_000, SENDER1); ensureTransactionIsValid(notSelectedTransient, 21_000, 0); - final Transaction notSelectedInvalid = createTransaction(2, Wei.of(10), 21_000); + final Transaction notSelectedInvalid = createTransaction(2, Wei.of(10), 21_000, SENDER2); ensureTransactionIsValid(notSelectedInvalid, 21_000, 0); final PluginTransactionSelectorFactory transactionSelectorFactory = @@ -723,12 +762,12 @@ public TransactionSelectionResult evaluateTransactionPostProcessing( Wei.ZERO, transactionSelectionService); - transactionPool.addRemoteTransactions(List.of(selected, notSelected, selected3)); + transactionPool.addRemoteTransactions(List.of(selected, notSelected)); final TransactionSelectionResults transactionSelectionResults = selector.buildTransactionListForBlock(); - assertThat(transactionSelectionResults.getSelectedTransactions()).contains(selected, selected3); + assertThat(transactionSelectionResults.getSelectedTransactions()).contains(selected); assertThat(transactionSelectionResults.getNotSelectedTransactions()) .containsOnly( entry(notSelected, PluginTransactionSelectionResult.GENERIC_PLUGIN_INVALID_TRANSIENT)); @@ -1170,7 +1209,7 @@ private void internalBlockSelectionTimeoutSimulationInvalidTxs( final List transactionsToInject = new ArrayList<>(txCount); for (int i = 0; i < txCount - 1; i++) { - final Transaction tx = createTransaction(i, Wei.of(7), 100_000); + final Transaction tx = createTransaction(0, Wei.of(7), 100_000, Sender.values()[i]); transactionsToInject.add(tx); if (processingTooLate) { ensureTransactionIsInvalid(tx, txInvalidReason, fastProcessingTxTime); @@ -1179,7 +1218,7 @@ private void internalBlockSelectionTimeoutSimulationInvalidTxs( } } - final Transaction lateTx = createTransaction(2, Wei.of(7), 100_000); + final Transaction lateTx = createTransaction(0, Wei.of(7), 100_000, SENDER5); transactionsToInject.add(lateTx); if (processingTooLate) { ensureTransactionIsInvalid(lateTx, txInvalidReason, longProcessingTxTime); @@ -1293,7 +1332,7 @@ protected BlockTransactionSelector createBlockSelector( worldState, transactionPool, blockHeader, - this::createReceipt, + protocolSchedule.getByBlockHeader(blockHeader).getTransactionReceiptFactory(), this::isCancelled, miningBeneficiary, blobGasPrice, @@ -1317,6 +1356,11 @@ protected FeeMarket getFeeMarket() { protected Transaction createTransaction( final int nonce, final Wei gasPrice, final long gasLimit) { + return createTransaction(nonce, gasPrice, gasLimit, SENDER1); + } + + protected Transaction createTransaction( + final int nonce, final Wei gasPrice, final long gasLimit, final Sender sender) { return Transaction.builder() .gasLimit(gasLimit) .gasPrice(gasPrice) @@ -1324,10 +1368,10 @@ protected Transaction createTransaction( .payload(Bytes.EMPTY) .to(Address.ID) .value(Wei.of(nonce)) - .sender(sender) + .sender(sender.address()) .chainId(CHAIN_ID) .guessType() - .signAndBuild(keyPair); + .signAndBuild(sender.keyPair()); } protected Transaction createEIP1559Transaction( @@ -1335,6 +1379,15 @@ protected Transaction createEIP1559Transaction( final Wei maxFeePerGas, final Wei maxPriorityFeePerGas, final long gasLimit) { + return createEIP1559Transaction(nonce, maxFeePerGas, maxPriorityFeePerGas, gasLimit, SENDER1); + } + + protected Transaction createEIP1559Transaction( + final int nonce, + final Wei maxFeePerGas, + final Wei maxPriorityFeePerGas, + final long gasLimit, + final Sender sender) { return Transaction.builder() .type(TransactionType.EIP1559) .gasLimit(gasLimit) @@ -1344,19 +1397,9 @@ protected Transaction createEIP1559Transaction( .payload(Bytes.EMPTY) .to(Address.ID) .value(Wei.of(nonce)) - .sender(sender) + .sender(sender.address()) .chainId(CHAIN_ID) - .signAndBuild(keyPair); - } - - // This is a duplicate of the MainnetProtocolSpec::frontierTransactionReceiptFactory - private TransactionReceipt createReceipt( - final TransactionType __, - final TransactionProcessingResult result, - final WorldState worldState, - final long gasUsed) { - return new TransactionReceipt( - worldState.rootHash(), gasUsed, Lists.newArrayList(), Optional.empty()); + .signAndBuild(sender.keyPair()); } protected void ensureTransactionIsValid(final Transaction tx) { @@ -1507,4 +1550,34 @@ public static TransactionSelectionResult invalid(final String invalidReason) { return new PluginTransactionSelectionResult(PluginStatus.PLUGIN_INVALID, invalidReason); } } + + protected enum Sender { + // it is important to keep the addresses of the senders sorted, to make the tests reproducible, + // since a different sender address can change the order in which txs are selected, + // if all the other sorting fields are equal + SENDER1(4), // 0x1eff47bc3a10a45d4b230b5d10e37751fe6aa718 + SENDER2(2), // 0x2b5ad5c4795c026514f8317c7a215e218dccd6cf + SENDER3(3), // 0x6813eb9362372eef6200f3b1dbc3f819671cba69 + SENDER4(1), // 0x7e5f4552091a69125d5dfcb7b8c2659029395bdf + SENDER5(5); // 0xe1ab8145f7e55dc933d51a18c793f901a3a0b276 + + private final KeyPair keyPair; + private final Address address; + + Sender(final int seed) { + final var privateKey = + SignatureAlgorithmFactory.getInstance().createPrivateKey(BigInteger.valueOf(seed)); + final var publicKey = SignatureAlgorithmFactory.getInstance().createPublicKey(privateKey); + this.keyPair = new KeyPair(privateKey, publicKey); + this.address = Address.extract(Hash.hash(publicKey.getEncodedBytes())); + } + + public KeyPair keyPair() { + return keyPair; + } + + public Address address() { + return address; + } + } } diff --git a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/LondonFeeMarketBlockTransactionSelectorTest.java b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/LondonFeeMarketBlockTransactionSelectorTest.java index eb77c6df658..2fbea1dde4b 100644 --- a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/LondonFeeMarketBlockTransactionSelectorTest.java +++ b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/LondonFeeMarketBlockTransactionSelectorTest.java @@ -16,6 +16,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.entry; +import static org.hyperledger.besu.ethereum.blockcreation.AbstractBlockTransactionSelectorTest.Sender.SENDER1; +import static org.hyperledger.besu.ethereum.blockcreation.AbstractBlockTransactionSelectorTest.Sender.SENDER2; import static org.hyperledger.besu.ethereum.core.MiningConfiguration.DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME; import static org.mockito.Mockito.mock; @@ -246,19 +248,23 @@ public void shouldNotSelectTransactionsWithPriorityFeeLessThanConfig() { ImmutableMiningConfiguration.builder().from(defaultTestMiningConfiguration).build(); miningConfiguration.setMinPriorityFeePerGas(Wei.of(7)); - final Transaction txSelected1 = createEIP1559Transaction(1, Wei.of(8), Wei.of(8), 100_000); + final Transaction txSelected1 = + createEIP1559Transaction(0, Wei.of(8), Wei.of(8), 100_000, SENDER1); ensureTransactionIsValid(txSelected1); // transaction txNotSelected1 should not be selected - final Transaction txNotSelected1 = createEIP1559Transaction(2, Wei.of(7), Wei.of(7), 100_000); + final Transaction txNotSelected1 = + createEIP1559Transaction(1, Wei.of(7), Wei.of(7), 100_000, SENDER1); ensureTransactionIsValid(txNotSelected1); // transaction txSelected2 should be selected - final Transaction txSelected2 = createEIP1559Transaction(3, Wei.of(8), Wei.of(8), 100_000); + final Transaction txSelected2 = + createEIP1559Transaction(0, Wei.of(8), Wei.of(8), 100_000, SENDER2); ensureTransactionIsValid(txSelected2); // transaction txNotSelected2 should not be selected - final Transaction txNotSelected2 = createEIP1559Transaction(4, Wei.of(8), Wei.of(6), 100_000); + final Transaction txNotSelected2 = + createEIP1559Transaction(1, Wei.of(8), Wei.of(6), 100_000, SENDER2); ensureTransactionIsValid(txNotSelected2); final BlockTransactionSelector selector = diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactions.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactions.java index 4622b9a997f..945033ef764 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactions.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactions.java @@ -42,12 +42,10 @@ import org.hyperledger.besu.plugin.data.TransactionSelectionResult; import java.util.ArrayDeque; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.OptionalLong; -import java.util.Set; import java.util.stream.Collector; import java.util.stream.Collectors; @@ -315,8 +313,6 @@ public synchronized List getPriorityTransactions() { @Override public void selectTransactions(final PendingTransactions.TransactionSelector selector) { - final Set
skipSenders = new HashSet<>(); - final Map> candidateTxsByScore; synchronized (this) { // since selecting transactions for block creation is a potential long operation @@ -332,50 +328,39 @@ public void selectTransactions(final PendingTransactions.TransactionSelector sel for (final var senderTxs : entry.getValue()) { LOG.trace("Evaluating sender txs {}", senderTxs); - if (!skipSenders.contains(senderTxs.sender())) { - - for (final var candidatePendingTx : senderTxs.pendingTransactions()) { - final var selectionResult = selector.evaluateTransaction(candidatePendingTx); - + for (final var candidatePendingTx : senderTxs.pendingTransactions()) { + final var selectionResult = selector.evaluateTransaction(candidatePendingTx); + + LOG.atTrace() + .setMessage("Selection result {} for transaction {}") + .addArgument(selectionResult) + .addArgument(candidatePendingTx::toTraceLog) + .log(); + + if (selectionResult.discard()) { + ethScheduler.scheduleTxWorkerTask( + () -> { + synchronized (this) { + prioritizedTransactions.remove(candidatePendingTx, INVALIDATED); + } + }); + logDiscardedTransaction(candidatePendingTx, selectionResult); + } else if (selectionResult.penalize()) { + ethScheduler.scheduleTxWorkerTask( + () -> { + synchronized (this) { + prioritizedTransactions.penalize(candidatePendingTx); + } + }); LOG.atTrace() - .setMessage("Selection result {} for transaction {}") - .addArgument(selectionResult) + .setMessage("Transaction {} penalized") .addArgument(candidatePendingTx::toTraceLog) .log(); + } - if (selectionResult.discard()) { - ethScheduler.scheduleTxWorkerTask( - () -> { - synchronized (this) { - prioritizedTransactions.remove(candidatePendingTx, INVALIDATED); - } - }); - logDiscardedTransaction(candidatePendingTx, selectionResult); - } else if (selectionResult.penalize()) { - ethScheduler.scheduleTxWorkerTask( - () -> { - synchronized (this) { - prioritizedTransactions.penalize(candidatePendingTx); - } - }); - LOG.atTrace() - .setMessage("Transaction {} penalized") - .addArgument(candidatePendingTx::toTraceLog) - .log(); - } - - if (selectionResult.stop()) { - LOG.trace("Stopping selection"); - break selection; - } - - if (!selectionResult.selected()) { - // avoid processing other txs from this sender if this one is skipped - // since the following will not be selected due to the nonce gap - LOG.trace("Skipping remaining txs for sender {}", candidatePendingTx.getSender()); - skipSenders.add(candidatePendingTx.getSender()); - break; - } + if (selectionResult.stop()) { + LOG.trace("Stopping selection"); + break selection; } } } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactionsTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactionsTest.java index 28814790a09..4c87b374a84 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactionsTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/transactions/layered/LayeredPendingTransactionsTest.java @@ -15,6 +15,7 @@ package org.hyperledger.besu.ethereum.eth.transactions.layered; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.entry; import static org.hyperledger.besu.datatypes.TransactionType.BLOB; import static org.hyperledger.besu.ethereum.eth.transactions.TransactionAddedResult.ADDED; import static org.hyperledger.besu.ethereum.eth.transactions.TransactionAddedResult.ALREADY_KNOWN; @@ -23,15 +24,13 @@ import static org.hyperledger.besu.ethereum.eth.transactions.layered.AddReason.MOVE; import static org.hyperledger.besu.ethereum.eth.transactions.layered.AddReason.NEW; import static org.hyperledger.besu.ethereum.eth.transactions.layered.LayeredRemovalReason.PoolRemovalReason.DROPPED; +import static org.hyperledger.besu.ethereum.eth.transactions.layered.LayeredRemovalReason.PoolRemovalReason.INVALIDATED; import static org.hyperledger.besu.ethereum.eth.transactions.layered.LayeredRemovalReason.PoolRemovalReason.REPLACED; import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.GAS_PRICE_BELOW_CURRENT_BASE_FEE; import static org.hyperledger.besu.ethereum.transaction.TransactionInvalidReason.UPFRONT_COST_EXCEEDS_BALANCE; -import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.BLOB_PRICE_BELOW_CURRENT_MIN; import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.BLOCK_FULL; import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.BLOCK_OCCUPANCY_ABOVE_THRESHOLD; -import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.CURRENT_TX_PRICE_BELOW_MIN; import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.SELECTED; -import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.TX_TOO_LARGE_FOR_REMAINING_GAS; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -49,6 +48,7 @@ import org.hyperledger.besu.ethereum.eth.transactions.PendingTransaction; import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactionAddedListener; import org.hyperledger.besu.ethereum.eth.transactions.PendingTransactionDroppedListener; +import org.hyperledger.besu.ethereum.eth.transactions.RemovalReason; import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration; import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolMetrics; import org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolReplacementHandler; @@ -57,10 +57,12 @@ import org.hyperledger.besu.plugin.data.TransactionSelectionResult; import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.OptionalLong; +import java.util.SequencedMap; import java.util.function.BiFunction; import java.util.stream.Stream; @@ -479,46 +481,6 @@ public void selectTransactionsFromSameSenderInNonceOrder() { assertThat(iterationOrder).containsExactly(transaction0, transaction1, transaction2); } - @ParameterizedTest - @MethodSource - public void ignoreSenderTransactionsAfterASkippedOne( - final TransactionSelectionResult skipSelectionResult) { - final Transaction transaction0a = createTransaction(0, DEFAULT_BASE_FEE.add(Wei.of(20)), KEYS1); - final Transaction transaction1a = createTransaction(1, DEFAULT_BASE_FEE.add(Wei.of(20)), KEYS1); - final Transaction transaction2a = createTransaction(2, DEFAULT_BASE_FEE.add(Wei.of(20)), KEYS1); - final Transaction transaction0b = createTransaction(0, DEFAULT_BASE_FEE.add(Wei.of(10)), KEYS2); - - pendingTransactions.addTransaction( - createLocalPendingTransaction(transaction0a), Optional.empty()); - pendingTransactions.addTransaction( - createLocalPendingTransaction(transaction1a), Optional.empty()); - pendingTransactions.addTransaction( - createLocalPendingTransaction(transaction2a), Optional.empty()); - pendingTransactions.addTransaction( - createLocalPendingTransaction(transaction0b), Optional.empty()); - - final List iterationOrder = new ArrayList<>(3); - pendingTransactions.selectTransactions( - pendingTx -> { - iterationOrder.add(pendingTx.getTransaction()); - // pretending that the 2nd tx of the 1st sender is not selected - return pendingTx.getNonce() == 1 ? skipSelectionResult : SELECTED; - }); - - // the 3rd tx of the 1st must not be processed, since the 2nd is skipped - // but the 2nd sender must not be affected - assertThat(iterationOrder).containsExactly(transaction0a, transaction1a, transaction0b); - } - - static Stream ignoreSenderTransactionsAfterASkippedOne() { - return Stream.of( - CURRENT_TX_PRICE_BELOW_MIN, - BLOB_PRICE_BELOW_CURRENT_MIN, - TX_TOO_LARGE_FOR_REMAINING_GAS, - TransactionSelectionResult.invalidTransient(GAS_PRICE_BELOW_CURRENT_BASE_FEE.name()), - TransactionSelectionResult.invalid(UPFRONT_COST_EXCEEDS_BALANCE.name())); - } - @Test public void notForceNonceOrderWhenSendersDiffer() { final Account sender2 = mock(Account.class); @@ -547,9 +509,10 @@ public void notForceNonceOrderWhenSendersDiffer() { @Test public void invalidTransactionIsDeletedFromPendingTransactions() { final var pendingTx0 = createRemotePendingTransaction(transaction0); - final var pendingTx1 = createRemotePendingTransaction(transaction1); pendingTransactions.addTransaction(pendingTx0, Optional.empty()); - pendingTransactions.addTransaction(pendingTx1, Optional.empty()); + + final var droppedTxCollector = new DroppedTransactionCollector(); + pendingTransactions.subscribeDroppedTransactions(droppedTxCollector); final List parsedTransactions = new ArrayList<>(1); pendingTransactions.selectTransactions( @@ -558,11 +521,11 @@ public void invalidTransactionIsDeletedFromPendingTransactions() { return TransactionSelectionResult.invalid(UPFRONT_COST_EXCEEDS_BALANCE.name()); }); - // only the first is processed since not being selected will automatically skip the processing - // all the other txs from the same sender - + // assert that first tx is removed from the pool + assertThat(droppedTxCollector.droppedTransactions) + .containsExactly(entry(transaction0, INVALIDATED)); assertThat(parsedTransactions).containsExactly(pendingTx0); - assertThat(pendingTransactions.getPendingTransactions()).containsExactly(pendingTx1); + assertThat(pendingTransactions.getPendingTransactions()).isEmpty(); } @Test @@ -947,4 +910,13 @@ record CreatedLayers( ReadyTransactions readyTransactions, SparseTransactions sparseTransactions, EvictCollectorLayer evictedCollector) {} + + static class DroppedTransactionCollector implements PendingTransactionDroppedListener { + final SequencedMap droppedTransactions = new LinkedHashMap<>(); + + @Override + public void onTransactionDropped(final Transaction transaction, final RemovalReason reason) { + droppedTransactions.put(transaction, reason); + } + } } diff --git a/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionSelectionResult.java b/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionSelectionResult.java index 1c35972a582..b2fb5ab0c77 100644 --- a/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionSelectionResult.java +++ b/plugin-api/src/main/java/org/hyperledger/besu/plugin/data/TransactionSelectionResult.java @@ -176,6 +176,13 @@ public boolean penalize() { public static final TransactionSelectionResult PRIORITY_FEE_PER_GAS_BELOW_CURRENT_MIN = TransactionSelectionResult.invalidTransient("PRIORITY_FEE_PER_GAS_BELOW_CURRENT_MIN"); + /** + * The transaction has not been selected since its sender already had a previous transaction not + * selected + */ + public static final TransactionSelectionResult SENDER_WITH_PREVIOUS_TX_NOT_SELECTED = + TransactionSelectionResult.invalidTransient("SENDER_WITH_PREVIOUS_TX_NOT_SELECTED"); + private final Status status; private final Optional maybeInvalidReason;