diff --git a/CHANGELOG.md b/CHANGELOG.md index 13816f6bf15..e00a9cb65c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ - Add `yParity` to GraphQL and JSON-RPC for relevant querise. [6119](https://github.com/hyperledger/besu/pull/6119) - Force tx replacement price bump to zero when zero base fee market is configured or `--min-gas-price` is set to 0. This allows for easier tx replacement in networks where there is not gas price. [#6079](https://github.com/hyperledger/besu/pull/6079) - Introduce the possibility to limit the time spent selecting pending transactions during block creation, using the new experimental option `Xblock-txs-selection-max-time` on PoS and PoW networks (by default set to 5000ms) or `Xpoa-block-txs-selection-max-time` on PoA networks (by default 75% of the min block time) [#6044](https://github.com/hyperledger/besu/pull/6044) +- Introduce the possibility to limit the time spent evaluating a single pending transactions during block creation, using the new experimental option `Xblock-txs-selection-per-tx-max-time` (by default set to the value of block txs selection max time for your network) [#6089](https://github.com/hyperledger/besu/pull/6089) ### Bug fixes - Upgrade netty to address CVE-2023-44487, CVE-2023-34462 [#6100](https://github.com/hyperledger/besu/pull/6100) diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/MiningOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/MiningOptions.java index 78e2032e4f6..6819a85f90b 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/options/MiningOptions.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/options/MiningOptions.java @@ -187,6 +187,15 @@ static class Unstable { + " To be only used on PoA networks, for other networks see Xblock-txs-selection-max-time." + " (default: ${DEFAULT-VALUE})") private Percentage poaBlockTxsSelectionMaxTime = DEFAULT_POA_BLOCK_TXS_SELECTION_MAX_TIME; + + @CommandLine.Option( + hidden = true, + names = {"--Xblock-txs-selection-per-tx-max-time"}, + description = + "Specifies the maximum time, in milliseconds, that could be spent selecting a single transaction to be included in the block." + + " Must be positive and ≤ the max time allocated for the block txs selection." + + " (default: will get the block txs selection max time as specified by --Xblock-txs-selection-max-time or --Xpoa-block-txs-selection-max-time)") + private Long blockTxsSelectionPerTxMaxTime; } private MiningOptions() {} @@ -324,6 +333,11 @@ static MiningOptions fromConfig(final MiningParameters miningParameters) { miningParameters.getCoinbase().ifPresent(coinbase -> miningOptions.coinbase = coinbase); miningParameters.getTargetGasLimit().ifPresent(tgl -> miningOptions.targetGasLimit = tgl); + miningParameters + .getUnstable() + .getConfiguredBlockTxsSelectionPerTxMaxTime() + .ifPresent(value -> miningOptions.unstableOptions.blockTxsSelectionPerTxMaxTime = value); + return miningOptions; } @@ -344,25 +358,31 @@ public MiningParameters toDomainObject() { updatableInitValuesBuilder.coinbase(coinbase); } + final var unstableParametersBuilder = + ImmutableMiningParameters.Unstable.builder() + .remoteSealersLimit(unstableOptions.remoteSealersLimit) + .remoteSealersTimeToLive(unstableOptions.remoteSealersTimeToLive) + .powJobTimeToLive(unstableOptions.powJobTimeToLive) + .maxOmmerDepth(unstableOptions.maxOmmersDepth) + .stratumExtranonce(unstableOptions.stratumExtranonce) + .posBlockCreationMaxTime(unstableOptions.posBlockCreationMaxTime) + .posBlockCreationRepetitionMinDuration( + unstableOptions.posBlockCreationRepetitionMinDuration) + .nonPoaBlockTxsSelectionMaxTime(unstableOptions.nonPoaBlockTxsSelectionMaxTime) + .poaBlockTxsSelectionMaxTime(unstableOptions.poaBlockTxsSelectionMaxTime); + + if (unstableOptions.blockTxsSelectionPerTxMaxTime != null) { + unstableParametersBuilder.configuredBlockTxsSelectionPerTxMaxTime( + unstableOptions.blockTxsSelectionPerTxMaxTime); + } + final var miningParametersBuilder = ImmutableMiningParameters.builder() .mutableInitValues(updatableInitValuesBuilder.build()) .isStratumMiningEnabled(iStratumMiningEnabled) .stratumNetworkInterface(stratumNetworkInterface) .stratumPort(stratumPort) - .unstable( - ImmutableMiningParameters.Unstable.builder() - .remoteSealersLimit(unstableOptions.remoteSealersLimit) - .remoteSealersTimeToLive(unstableOptions.remoteSealersTimeToLive) - .powJobTimeToLive(unstableOptions.powJobTimeToLive) - .maxOmmerDepth(unstableOptions.maxOmmersDepth) - .stratumExtranonce(unstableOptions.stratumExtranonce) - .posBlockCreationMaxTime(unstableOptions.posBlockCreationMaxTime) - .posBlockCreationRepetitionMinDuration( - unstableOptions.posBlockCreationRepetitionMinDuration) - .nonPoaBlockTxsSelectionMaxTime(unstableOptions.nonPoaBlockTxsSelectionMaxTime) - .poaBlockTxsSelectionMaxTime(unstableOptions.poaBlockTxsSelectionMaxTime) - .build()); + .unstable(unstableParametersBuilder.build()); return miningParametersBuilder.build(); } diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java index 0b7cc5e1224..7e3b37de70a 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java @@ -32,6 +32,8 @@ import java.io.IOException; import java.nio.file.Path; +import java.util.Arrays; +import java.util.List; import java.util.Optional; import org.apache.tuweni.bytes.Bytes; @@ -384,6 +386,48 @@ public void poaBlockTxsSelectionMaxTimeOnlyCompatibleWithPoaNetworks() { "90"); } + @Test + public void txsSelectionPerTxMaxTimeDefaultValue() { + // by default, it takes the value of the block txs selection max time + internalTestSuccess( + miningParams -> + assertThat(miningParams.getUnstable().getBlockTxsSelectionPerTxMaxTime()) + .isEqualTo(miningParams.getUnstable().getBlockTxsSelectionMaxTime()) + .isEqualTo(DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME)); + } + + @Test + public void txsSelectionPerTxMaxTimeOption() { + internalTestSuccess( + miningParams -> + assertThat(miningParams.getUnstable().getBlockTxsSelectionPerTxMaxTime()) + .isEqualTo(700L), + "--Xblock-txs-selection-per-tx-max-time", + "700"); + } + + @Test + public void txsSelectionPerTxMaxTimeOptionGetsTheValueOfTxsSelectionMaxTimeIfNotConfigured() { + // by default, it takes the value of the block txs selection max time + internalTestSuccess( + miningParams -> + assertThat(miningParams.getUnstable().getBlockTxsSelectionPerTxMaxTime()) + .isEqualTo(miningParams.getUnstable().getBlockTxsSelectionMaxTime()) + .isEqualTo(1000L), + "--Xblock-txs-selection-max-time", + "1000"); + } + + @Test + public void txsSelectionPerTxMaxTimeOutOfAllowedRange() { + internalTestFailure( + "blockTxsSelectionPerTxMaxTime (4000) is greater than the blockTxsSelectionMaxTime (3000)", + "--Xblock-txs-selection-max-time", + "3000", + "--Xblock-txs-selection-per-tx-max-time", + "4000"); + } + @Override protected MiningParameters createDefaultDomainObject() { return MiningParameters.newDefault(); @@ -413,4 +457,9 @@ protected MiningOptions optionsFromDomainObject(final MiningParameters domainObj protected MiningOptions getOptionsFromBesuCommand(final TestBesuCommand besuCommand) { return besuCommand.getMiningOptions(); } + + @Override + protected List getFieldsWithComputedDefaults() { + return Arrays.asList("unstableOptions.blockTxsSelectionPerTxMaxTime"); + } } diff --git a/build.gradle b/build.gradle index 63397b210e8..9ef9642d30e 100644 --- a/build.gradle +++ b/build.gradle @@ -174,6 +174,7 @@ allprojects { importOrder 'org.hyperledger', 'java', '' trimTrailingWhitespace() endWithNewline() + toggleOffOn() } groovyGradle { target '*.gradle' diff --git a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java index cad6e05d60c..e781376be24 100644 --- a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java +++ b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java @@ -90,6 +90,7 @@ import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; import com.google.common.base.Suppliers; import org.apache.tuweni.bytes.Bytes; @@ -182,6 +183,7 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper { private final BadBlockManager badBlockManager = spy(new BadBlockManager()); @BeforeEach + @SuppressWarnings("unchecked") public void setUp() { when(mergeContext.as(MergeContext.class)).thenReturn(mergeContext); when(mergeContext.getTerminalTotalDifficulty()) @@ -206,7 +208,7 @@ public void setUp() { genesisState.writeStateTo(mutable); mutable.persist(null); - when(ethScheduler.scheduleBlockCreationTask(any())) + when(ethScheduler.scheduleBlockCreationTask(any(Runnable.class))) .thenAnswer( invocation -> { final Runnable runnable = invocation.getArgument(0); @@ -217,6 +219,17 @@ public void setUp() { return blockCreationTask; }); + when(ethScheduler.scheduleBlockCreationTask(any(Supplier.class))) + .thenAnswer( + invocation -> { + final Supplier supplier = invocation.getArgument(0); + if (!invocation.toString().contains("MergeCoordinator")) { + return CompletableFuture.supplyAsync(supplier); + } + blockCreationTask = CompletableFuture.supplyAsync((Supplier) supplier); + return blockCreationTask; + }); + MergeConfigOptions.setMergeEnabled(true); when(ethContext.getEthPeers().subscribeConnect(any())).thenReturn(1L); @@ -234,6 +247,10 @@ public void setUp() { this.transactionPool.setEnabled(); + createMergeCoordinator(miningParameters); + } + + private void createMergeCoordinator(final MiningParameters miningParameters) { this.coordinator = new MergeCoordinator( protocolContext, @@ -558,6 +575,7 @@ public void shouldStopRetryBlockCreationIfTimeExpired() throws InterruptedExcept .from(miningParameters) .unstable(Unstable.builder().posBlockCreationMaxTime(100).build()) .build(); + createMergeCoordinator(miningParameters); doAnswer( invocation -> { retries.incrementAndGet(); @@ -752,15 +770,7 @@ public void shouldUseExtraDataFromMiningParameters() { .mutableInitValues(MutableInitValues.builder().extraData(extraData).build()) .build(); - this.coordinator = - new MergeCoordinator( - protocolContext, - protocolSchedule, - ethScheduler, - transactionPool, - miningParameters, - backwardSyncContext, - Optional.empty()); + createMergeCoordinator(miningParameters); final PayloadIdentifier payloadId = this.coordinator.preparePayload( 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 595bcb74332..49bb25af661 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 @@ -15,7 +15,9 @@ package org.hyperledger.besu.ethereum.blockcreation.txselection; import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.BLOCK_SELECTION_TIMEOUT; +import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.INTERNAL_ERROR; import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.SELECTED; +import static org.hyperledger.besu.plugin.data.TransactionSelectionResult.TX_EVALUATION_TIMEOUT; import org.hyperledger.besu.datatypes.Address; import org.hyperledger.besu.datatypes.Wei; @@ -94,8 +96,9 @@ public class BlockTransactionSelector { private final PluginTransactionSelector pluginTransactionSelector; private final BlockAwareOperationTracer pluginOperationTracer; private final EthScheduler ethScheduler; - private final AtomicBoolean isTimeout = new AtomicBoolean(false); - private WorldUpdater blockWorldStateUpdater; + private final AtomicBoolean isBlockTimeout = new AtomicBoolean(false); + private final long txsSelectionMaxTime; + private final long txEvaluationMaxTime; public BlockTransactionSelector( final MiningParameters miningParameters, @@ -132,7 +135,8 @@ public BlockTransactionSelector( transactionSelectors = createTransactionSelectors(blockSelectionContext); this.pluginTransactionSelector = pluginTransactionSelector; this.pluginOperationTracer = pluginTransactionSelector.getOperationTracer(); - blockWorldStateUpdater = worldState.updater(); + txsSelectionMaxTime = miningParameters.getUnstable().getBlockTxsSelectionMaxTime(); + txEvaluationMaxTime = miningParameters.getUnstable().getBlockTxsSelectionPerTxMaxTime(); } private List createTransactionSelectors( @@ -168,30 +172,28 @@ public TransactionSelectionResults buildTransactionListForBlock() { } private void timeLimitedSelection() { - final long blockTxsSelectionMaxTime = - blockSelectionContext.miningParameters().getUnstable().getBlockTxsSelectionMaxTime(); - final var txSelection = + final var selectionFuture = ethScheduler.scheduleBlockCreationTask( () -> blockSelectionContext .transactionPool() - .selectTransactions(this::evaluateTransaction)); + .selectTransactions(this::timeLimitedEvaluateTransaction)); try { - txSelection.get(blockTxsSelectionMaxTime, TimeUnit.MILLISECONDS); + selectionFuture.get(txsSelectionMaxTime, TimeUnit.MILLISECONDS); } catch (InterruptedException | ExecutionException e) { if (isCancelled.get()) { - throw new CancellationException("Cancelled during transaction selection"); + throw new CancellationException("Cancelled during transactions selection"); } - LOG.warn("Error during block transaction selection", e); + LOG.warn("Error during block transactions selection", e); } catch (TimeoutException e) { // synchronize since we want to be sure that there is no concurrent state update - synchronized (isTimeout) { - isTimeout.set(true); + synchronized (transactionSelectionResults) { + isBlockTimeout.set(true); } LOG.warn( - "Interrupting transaction selection since it is taking more than the max configured time of " - + blockTxsSelectionMaxTime + "Interrupting transactions selection since it is taking more than the max configured time of " + + txsSelectionMaxTime + "ms", e); } @@ -208,10 +210,62 @@ private void timeLimitedSelection() { */ public TransactionSelectionResults evaluateTransactions(final List transactions) { transactions.forEach( - transaction -> evaluateTransaction(new PendingTransaction.Local.Priority(transaction))); + transaction -> { + evaluateTransaction( + new PendingTransaction.Local.Priority(transaction), + new TxEvaluationState(worldState)); + }); return transactionSelectionResults; } + private TransactionSelectionResult timeLimitedEvaluateTransaction( + final PendingTransaction pendingTransaction) { + if (isBlockTimeout.get()) { + LOG.atTrace() + .setMessage( + "Skipping evaluation of tx {}, since there was already a block selection timeout") + .addArgument(pendingTransaction::toTraceLog) + .log(); + return BLOCK_SELECTION_TIMEOUT; + } + + final var txEvaluationState = new TxEvaluationState(worldState); + final var evaluationFuture = + ethScheduler.scheduleBlockCreationTask( + () -> evaluateTransaction(pendingTransaction, txEvaluationState)); + try { + evaluationFuture.get(txEvaluationMaxTime, TimeUnit.MILLISECONDS); + return txEvaluationState.getSelectionResult(); + } catch (InterruptedException | ExecutionException e) { + if (isCancelled.get()) { + throw new CancellationException("Cancelled during transaction evaluation"); + } + LOG.warn( + "Error during transaction evaluation of tx with hash: " + + pendingTransaction.getHash() + + ", removing it from the pool", + e); + return INTERNAL_ERROR; + } catch (TimeoutException e) { + // synchronize since we want to be sure that there is no concurrent state update + synchronized (transactionSelectionResults) { + // in the meantime the tx could have been evaluated, in that case the selection result is + // present + if (txEvaluationState.hasSelectionResult()) { + return txEvaluationState.getSelectionResult(); + } + LOG.warn( + "Interrupting transaction evaluation since it is taking more than the max configured time of " + + txEvaluationMaxTime + + "ms. Hash: " + + pendingTransaction.getHash(), + e); + txEvaluationState.triggerTimeout(); + return TX_EVALUATION_TIMEOUT; + } + } + } + /** * Passed into the PendingTransactions, and is called on each transaction until sufficient * transactions are found which fill a block worth of gas. This function will continue to be @@ -219,30 +273,32 @@ public TransactionSelectionResults evaluateTransactions(final List * provided transaction's gasLimit does not fit within the space remaining in the block. * * @param pendingTransaction The transaction to be evaluated. - * @return The result of the transaction evaluation process. * @throws CancellationException if the transaction selection process is cancelled. */ - private TransactionSelectionResult evaluateTransaction( - final PendingTransaction pendingTransaction) { + private void evaluateTransaction( + final PendingTransaction pendingTransaction, final TxEvaluationState txEvaluationState) { checkCancellation(); TransactionSelectionResult selectionResult = evaluatePreProcessing(pendingTransaction); if (!selectionResult.selected()) { - return handleTransactionNotSelected(pendingTransaction, selectionResult); - } + txEvaluationState.setSelectionResult(selectionResult); + handleTransactionNotSelected(pendingTransaction, txEvaluationState); + } else { - final WorldUpdater txWorldStateUpdater = blockWorldStateUpdater.updater(); - final TransactionProcessingResult processingResult = - processTransaction(pendingTransaction, txWorldStateUpdater); + final WorldUpdater txWorldStateUpdater = txEvaluationState.getTxWorldStateUpdater(); + final TransactionProcessingResult processingResult = + processTransaction(pendingTransaction, txWorldStateUpdater); - var postProcessingSelectionResult = - evaluatePostProcessing(pendingTransaction, processingResult); + var postProcessingSelectionResult = + evaluatePostProcessing(pendingTransaction, processingResult); + txEvaluationState.setSelectionResult(postProcessingSelectionResult); - if (postProcessingSelectionResult.selected()) { - return handleTransactionSelected(pendingTransaction, processingResult, txWorldStateUpdater); + if (postProcessingSelectionResult.selected()) { + handleTransactionSelected(pendingTransaction, processingResult, txEvaluationState); + } else { + handleTransactionNotSelected(pendingTransaction, txEvaluationState); + } } - return handleTransactionNotSelected( - pendingTransaction, postProcessingSelectionResult, txWorldStateUpdater); } /** @@ -325,13 +381,11 @@ private TransactionProcessingResult processTransaction( * * @param pendingTransaction The pending transaction. * @param processingResult The result of the transaction processing. - * @param txWorldStateUpdater The world state updater. - * @return The result of the transaction selection process. */ - private TransactionSelectionResult handleTransactionSelected( + private void handleTransactionSelected( final PendingTransaction pendingTransaction, final TransactionProcessingResult processingResult, - final WorldUpdater txWorldStateUpdater) { + final TxEvaluationState txEvaluationState) { final Transaction transaction = pendingTransaction.getTransaction(); final long gasUsedByTransaction = @@ -341,47 +395,66 @@ private TransactionSelectionResult handleTransactionSelected( final long blobGasUsed = blockSelectionContext.gasCalculator().blobGasCost(transaction.getBlobCount()); - final boolean tooLate; + final boolean blockTooLate; + final boolean txTooLate; // only add this tx to the selected set if it is not too late, // this need to be done synchronously to avoid that a concurrent timeout // could start packing a block while we are updating the state here - synchronized (isTimeout) { - if (!isTimeout.get()) { - txWorldStateUpdater.commit(); - blockWorldStateUpdater.commit(); + synchronized (transactionSelectionResults) { + blockTooLate = isBlockTimeout.get(); + txTooLate = txEvaluationState.isTimeout(); + + if (blockTooLate || txTooLate) { + LOG.atTrace() + .setMessage("Timeout block={}, tx={}, when processing {}") + .addArgument(blockTooLate) + .addArgument(txTooLate) + .addArgument(transaction::toTraceLog) + .log(); + } else { + txEvaluationState.commit(); final TransactionReceipt receipt = transactionReceiptFactory.create( transaction.getType(), processingResult, worldState, cumulativeGasUsed); transactionSelectionResults.updateSelected( - pendingTransaction.getTransaction(), receipt, gasUsedByTransaction, blobGasUsed); - tooLate = false; - } else { - tooLate = true; + transaction, receipt, gasUsedByTransaction, blobGasUsed); } } - if (tooLate) { + if (txTooLate) { + // this tx took much time to be evaluated and processed, and will not be included in the block + // and will be removed from the txpool, so we need to treat it as not selected + LOG.atTrace() + .setMessage( + "{} took too much to process, and will not be included in the block and will be removed from the txpool") + .addArgument(transaction::toTraceLog) + .log(); + // do not rely on the presence of this result, since by the time it is added, the code + // reading it could have been already executed by another thread + txEvaluationState.setSelectionResult(TX_EVALUATION_TIMEOUT); + handleTransactionNotSelected(pendingTransaction, txEvaluationState); + } else if (blockTooLate) { // even if this tx passed all the checks, it is too late to include it in this block, // so we need to treat it as not selected LOG.atTrace() - .setMessage("{} processed too late for block creation") + .setMessage( + "{} processed after a block selection timeout, and will not be included in the block") .addArgument(transaction::toTraceLog) .log(); // do not rely on the presence of this result, since by the time it is added, the code // reading it could have been already executed by another thread - return handleTransactionNotSelected( - pendingTransaction, BLOCK_SELECTION_TIMEOUT, txWorldStateUpdater); - } + txEvaluationState.setSelectionResult(BLOCK_SELECTION_TIMEOUT); + handleTransactionNotSelected(pendingTransaction, txEvaluationState); + } else { - pluginTransactionSelector.onTransactionSelected(pendingTransaction, processingResult); - blockWorldStateUpdater = worldState.updater(); - LOG.atTrace() - .setMessage("Selected {} for block creation") - .addArgument(transaction::toTraceLog) - .log(); - return SELECTED; + pluginTransactionSelector.onTransactionSelected(pendingTransaction, processingResult); + LOG.atTrace() + .setMessage("Selected {} for block creation") + .addArgument(transaction::toTraceLog) + .log(); + } } /** @@ -390,25 +463,15 @@ private TransactionSelectionResult handleTransactionSelected( * transaction selector. * * @param pendingTransaction The unselected pending transaction. - * @param selectionResult The result of the transaction selection process. - * @return The result of the transaction selection process. + * @param txEvaluationState The state of the transaction selection process. */ - private TransactionSelectionResult handleTransactionNotSelected( - final PendingTransaction pendingTransaction, - final TransactionSelectionResult selectionResult) { + private void handleTransactionNotSelected( + final PendingTransaction pendingTransaction, final TxEvaluationState txEvaluationState) { transactionSelectionResults.updateNotSelected( - pendingTransaction.getTransaction(), selectionResult); - pluginTransactionSelector.onTransactionNotSelected(pendingTransaction, selectionResult); - return selectionResult; - } - - private TransactionSelectionResult handleTransactionNotSelected( - final PendingTransaction pendingTransaction, - final TransactionSelectionResult selectionResult, - final WorldUpdater txWorldStateUpdater) { - txWorldStateUpdater.revert(); - return handleTransactionNotSelected(pendingTransaction, selectionResult); + pendingTransaction.getTransaction(), txEvaluationState.getSelectionResult()); + pluginTransactionSelector.onTransactionNotSelected( + pendingTransaction, txEvaluationState.getSelectionResult()); } private void checkCancellation() { @@ -416,4 +479,45 @@ private void checkCancellation() { throw new CancellationException("Cancelled during transaction selection."); } } + + private static class TxEvaluationState { + final AtomicBoolean isTimeout = new AtomicBoolean(false); + volatile TransactionSelectionResult selectionResult; + final WorldUpdater blockWorldStateUpdater; + final WorldUpdater txWorldStateUpdater; + + TxEvaluationState(final MutableWorldState worldState) { + blockWorldStateUpdater = worldState.updater(); + txWorldStateUpdater = blockWorldStateUpdater.updater(); + } + + WorldUpdater getTxWorldStateUpdater() { + return txWorldStateUpdater; + } + + void triggerTimeout() { + isTimeout.set(true); + } + + boolean isTimeout() { + return isTimeout.get(); + } + + void setSelectionResult(final TransactionSelectionResult selectionResult) { + this.selectionResult = selectionResult; + } + + boolean hasSelectionResult() { + return selectionResult != null; + } + + TransactionSelectionResult getSelectionResult() { + return selectionResult; + } + + void commit() { + txWorldStateUpdater.commit(); + blockWorldStateUpdater.commit(); + } + } } diff --git a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/TransactionSelectionResults.java b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/TransactionSelectionResults.java index 848789d9968..7790c2722af 100644 --- a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/TransactionSelectionResults.java +++ b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/txselection/TransactionSelectionResults.java @@ -36,7 +36,7 @@ public class TransactionSelectionResults { private static final Logger LOG = LoggerFactory.getLogger(TransactionSelectionResults.class); private final List selectedTransactions = Lists.newArrayList(); - private final Map> transactionsByType = + private final Map> selectedTransactionsByType = new EnumMap<>(TransactionType.class); private final List receipts = Lists.newArrayList(); /** @@ -55,7 +55,7 @@ void updateSelected( final long gasUsed, final long blobGasUsed) { selectedTransactions.add(transaction); - transactionsByType + selectedTransactionsByType .computeIfAbsent(transaction.getType(), type -> new ArrayList<>()) .add(transaction); receipts.add(receipt); @@ -81,7 +81,7 @@ public List getSelectedTransactions() { } public List getTransactionsByType(final TransactionType type) { - return transactionsByType.getOrDefault(type, List.of()); + return selectedTransactionsByType.getOrDefault(type, List.of()); } public List getReceipts() { 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 ae16f34d078..5e9090731e0 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 @@ -31,7 +31,6 @@ import org.hyperledger.besu.crypto.SignatureAlgorithmFactory; import org.hyperledger.besu.datatypes.Address; import org.hyperledger.besu.datatypes.Hash; -import org.hyperledger.besu.datatypes.PendingTransaction; import org.hyperledger.besu.datatypes.TransactionType; import org.hyperledger.besu.datatypes.Wei; import org.hyperledger.besu.ethereum.GasLimitCalculator; @@ -60,6 +59,7 @@ import org.hyperledger.besu.ethereum.difficulty.fixed.FixedDifficultyProtocolSchedule; import org.hyperledger.besu.ethereum.eth.manager.EthContext; import org.hyperledger.besu.ethereum.eth.manager.EthScheduler; +import org.hyperledger.besu.ethereum.eth.transactions.PendingTransaction; import org.hyperledger.besu.ethereum.eth.transactions.TransactionPool; import org.hyperledger.besu.ethereum.mainnet.MainnetBlockHeaderFunctions; import org.hyperledger.besu.ethereum.mainnet.MainnetTransactionProcessor; @@ -87,10 +87,15 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.concurrent.CompletableFuture; -import java.util.function.BiFunction; +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; +import java.util.function.Function; import java.util.function.Supplier; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import java.util.stream.Stream; import com.google.common.collect.Lists; @@ -130,7 +135,12 @@ public abstract class AbstractBlockTransactionSelectorTest { protected ProtocolSchedule protocolSchedule; protected final MiningParameters defaultTestMiningParameters = createMiningParameters( - Wei.ZERO, MIN_OCCUPANCY_80_PERCENT, DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME); + Wei.ZERO, + MIN_OCCUPANCY_80_PERCENT, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME); + + private final Executor executor = Executors.newCachedThreadPool(); @Mock protected EthScheduler ethScheduler; @@ -143,6 +153,7 @@ public abstract class AbstractBlockTransactionSelectorTest { protected EthContext ethContext; @BeforeEach + @SuppressWarnings("unchecked") public void setup() { genesisConfigFile = getGenesisConfigFile(); protocolSchedule = createProtocolSchedule(); @@ -170,7 +181,10 @@ public void setup() { .thenReturn(Optional.of(worldState)); when(ethContext.getEthPeers().subscribeConnect(any())).thenReturn(1L); when(ethScheduler.scheduleBlockCreationTask(any(Runnable.class))) - .thenAnswer(invocation -> CompletableFuture.runAsync(invocation.getArgument(0))); + .thenAnswer(invocation -> CompletableFuture.runAsync(invocation.getArgument(0), executor)); + when(ethScheduler.scheduleBlockCreationTask(any(Supplier.class))) + .thenAnswer( + invocation -> CompletableFuture.supplyAsync(invocation.getArgument(0), executor)); } protected abstract GenesisConfigFile getGenesisConfigFile(); @@ -418,7 +432,10 @@ public void transactionSelectionStopsWhenBlockIsFull() { final BlockTransactionSelector selector = createBlockSelectorAndSetupTxPool( createMiningParameters( - Wei.ZERO, MIN_OCCUPANCY_100_PERCENT, DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), transactionProcessor, blockHeader, miningBeneficiary, @@ -477,7 +494,10 @@ public void transactionSelectionStopsWhenRemainingGasIsNotEnoughForAnyMoreTransa final BlockTransactionSelector selector = createBlockSelectorAndSetupTxPool( createMiningParameters( - Wei.ZERO, MIN_OCCUPANCY_100_PERCENT, DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), transactionProcessor, blockHeader, miningBeneficiary, @@ -577,7 +597,7 @@ public void transactionSelectionPluginShouldWork_PreProcessing() { new PluginTransactionSelector() { @Override public TransactionSelectionResult evaluateTransactionPreProcessing( - final PendingTransaction pendingTransaction) { + final org.hyperledger.besu.datatypes.PendingTransaction pendingTransaction) { if (pendingTransaction.getTransaction().equals(notSelectedTransient)) return TransactionSelectionResult.invalidTransient("transient"); if (pendingTransaction.getTransaction().equals(notSelectedInvalid)) @@ -587,7 +607,7 @@ public TransactionSelectionResult evaluateTransactionPreProcessing( @Override public TransactionSelectionResult evaluateTransactionPostProcessing( - final PendingTransaction pendingTransaction, + final org.hyperledger.besu.datatypes.PendingTransaction pendingTransaction, final org.hyperledger.besu.plugin.data.TransactionProcessingResult processingResult) { return TransactionSelectionResult.SELECTED; @@ -639,13 +659,13 @@ public void transactionSelectionPluginShouldWork_PostProcessing() { new PluginTransactionSelector() { @Override public TransactionSelectionResult evaluateTransactionPreProcessing( - final PendingTransaction pendingTransaction) { + final org.hyperledger.besu.datatypes.PendingTransaction pendingTransaction) { return TransactionSelectionResult.SELECTED; } @Override public TransactionSelectionResult evaluateTransactionPostProcessing( - final PendingTransaction pendingTransaction, + final org.hyperledger.besu.datatypes.PendingTransaction pendingTransaction, final org.hyperledger.besu.plugin.data.TransactionProcessingResult processingResult) { // the transaction with max gas +1 should fail @@ -660,7 +680,10 @@ public TransactionSelectionResult evaluateTransactionPostProcessing( final BlockTransactionSelector selector = createBlockSelectorAndSetupTxPool( createMiningParameters( - Wei.ZERO, MIN_OCCUPANCY_80_PERCENT, DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), + Wei.ZERO, + MIN_OCCUPANCY_80_PERCENT, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), transactionProcessor, blockHeader, miningBeneficiary, @@ -874,55 +897,57 @@ public void shouldNotSelectTransactionsWithPriorityFeeLessThanConfig() { txNotSelected, TransactionSelectionResult.PRIORITY_FEE_PER_GAS_BELOW_CURRENT_MIN)); } + // spotless:off + /** + * Txs selection and timeout timeline + * + * 0 300 600 700 + * | | | | + * 000000000111111111222BT + * | | + * TS (0) (0,1) + * + * TS: Txs selected + * BT: Block timeout + * + * @param isPoa simulate a PoA network? + * @param preProcessingTooLate should the preProcessing step take too long? + * @param processingTooLate should the processing step take too long? + * @param postProcessingTooLate should the postProcessing step take too long? + */ + // spotless:on + @ParameterizedTest - @MethodSource("subsetOfPendingTransactionsIncludedWhenTxSelectionMaxTimeIsOver") - public void subsetOfPendingTransactionsIncludedWhenTxSelectionMaxTimeIsOver( + @MethodSource("longProcessingTask") + public void subsetOfPendingTransactionsIncludedWhenTxsSelectionMaxTimeIsOver( final boolean isPoa, final boolean preProcessingTooLate, final boolean processingTooLate, final boolean postProcessingTooLate) { - final Supplier> inTime = - () -> invocation -> TransactionSelectionResult.SELECTED; - final BiFunction> tooLate = - (p, t) -> - invocation -> { - if (((PendingTransaction) invocation.getArgument(0)).getTransaction().equals(p)) { - Thread.sleep(t); - } - return TransactionSelectionResult.SELECTED; - }; - final ProcessableBlockHeader blockHeader = createBlock(301_000); final Address miningBeneficiary = AddressHelpers.ofValue(1); final int poaMinBlockTime = 1; final long blockTxsSelectionMaxTime = 750; - final long longProcessingTxTime = 500; + final long singleTxSelectionMaxTime = 700; + final long txProcessingTime = 300; - final List transactionsToInject = new ArrayList<>(3); - for (int i = 0; i < 2; i++) { + final List txs = new ArrayList<>(3); + for (int i = 0; i < 3; i++) { final Transaction tx = createTransaction(i, Wei.of(7), 100_000); - transactionsToInject.add(tx); - ensureTransactionIsValid(tx); + txs.add(tx); + ensureTransactionIsValid(tx, 0, 0, processingTooLate ? txProcessingTime : 0); } - final Transaction lateTx = createTransaction(2, Wei.of(7), 100_000); - transactionsToInject.add(lateTx); - ensureTransactionIsValid( - lateTx, 0, 0, processingTooLate ? blockTxsSelectionMaxTime + longProcessingTxTime : 0); + final Map txsProcessingTime = + txs.stream().collect(Collectors.toMap(Function.identity(), t -> txProcessingTime)); PluginTransactionSelector transactionSelector = mock(PluginTransactionSelector.class); when(transactionSelector.evaluateTransactionPreProcessing(any())) - .thenAnswer( - preProcessingTooLate - ? inTime.get() - : tooLate.apply(lateTx, blockTxsSelectionMaxTime + longProcessingTxTime)); + .thenAnswer(processingTx(preProcessingTooLate, txsProcessingTime)); when(transactionSelector.evaluateTransactionPostProcessing(any(), any())) - .thenAnswer( - postProcessingTooLate - ? inTime.get() - : tooLate.apply(lateTx, blockTxsSelectionMaxTime + longProcessingTxTime)); + .thenAnswer(processingTx(postProcessingTooLate, txsProcessingTime)); final PluginTransactionSelectorFactory transactionSelectorFactory = mock(PluginTransactionSelectorFactory.class); @@ -932,41 +957,299 @@ public void subsetOfPendingTransactionsIncludedWhenTxSelectionMaxTimeIsOver( createBlockSelectorAndSetupTxPool( isPoa ? createMiningParameters( - Wei.ZERO, MIN_OCCUPANCY_100_PERCENT, poaMinBlockTime, Percentage.fromInt(75)) + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + poaMinBlockTime, + Percentage.fromInt(75), + singleTxSelectionMaxTime) : createMiningParameters( - Wei.ZERO, MIN_OCCUPANCY_100_PERCENT, blockTxsSelectionMaxTime), + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + blockTxsSelectionMaxTime, + singleTxSelectionMaxTime), transactionProcessor, blockHeader, miningBeneficiary, Wei.ZERO, transactionSelectorFactory); - transactionPool.addRemoteTransactions(transactionsToInject); + transactionPool.addRemoteTransactions(txsProcessingTime.keySet()); final TransactionSelectionResults results = selector.buildTransactionListForBlock(); // third tx is not selected, even if it could fit in the block, // since the selection time was over - assertThat(results.getSelectedTransactions().size()).isEqualTo(2); + assertThat(results.getSelectedTransactions()).containsExactlyElementsOf(txs.subList(0, 2)); - assertThat(results.getSelectedTransactions().containsAll(transactionsToInject.subList(0, 2))) - .isTrue(); + // given enough time we can check the not selected tx + await().until(() -> !results.getNotSelectedTransactions().isEmpty()); + assertThat(results.getNotSelectedTransactions()) + .containsOnly(entry(txs.get(2), TransactionSelectionResult.BLOCK_SELECTION_TIMEOUT)); + } + + // spotless:off + /** + * Txs selection and timeout timeline + * + * 0 200 600 1100 + * | | | | + * 0000000111111111222222222TT + * | | + * TS (0) (0,1) + * + * TS: Txs selected + * TT: Tx 3 timeout + * + * @param isPoa simulate a PoA network? + * @param preProcessingTooLate should the preProcessing step take too long? + * @param processingTooLate should the processing step take too long? + * @param postProcessingTooLate should the postProcessing step take too long? + */ + // spotless:on + @ParameterizedTest + @MethodSource("longProcessingTask") + public void subsetOfPendingTransactionsIncludedWhenPerTxEvaluationMaxTimeIsOver( + final boolean isPoa, + final boolean preProcessingTooLate, + final boolean processingTooLate, + final boolean postProcessingTooLate) { - assertThat(results.getReceipts().size()).isEqualTo(2); - assertThat(results.getCumulativeGasUsed()).isEqualTo(200_000); + final ProcessableBlockHeader blockHeader = createBlock(301_000); + final Address miningBeneficiary = AddressHelpers.ofValue(1); + final long singleTxSelectionMaxTime = 500; + final List txs = new ArrayList<>(3); + final List processingTimes = new ArrayList<>(3); - // Ensure receipts have the correct cumulative gas - assertThat(results.getReceipts().get(0).getCumulativeGasUsed()).isEqualTo(100_000); - assertThat(results.getReceipts().get(1).getCumulativeGasUsed()).isEqualTo(200_000); + for (int i = 0; i < 3; i++) { + final Transaction tx = createTransaction(i, Wei.of(7), 100_000); + txs.add(tx); + final long processingTime = 200L * (i + 1); + processingTimes.add(processingTime); + ensureTransactionIsValid(tx, 0, 0, processingTooLate ? processingTime : 0); + } + + final Map txsProcessingTime = + IntStream.of(0, 1, 2).boxed().collect(Collectors.toMap(txs::get, processingTimes::get)); + + PluginTransactionSelector transactionSelector = mock(PluginTransactionSelector.class); + when(transactionSelector.evaluateTransactionPreProcessing(any())) + .thenAnswer(processingTx(preProcessingTooLate, txsProcessingTime)); + + when(transactionSelector.evaluateTransactionPostProcessing(any(), any())) + .thenAnswer(processingTx(postProcessingTooLate, txsProcessingTime)); + + final PluginTransactionSelectorFactory transactionSelectorFactory = + mock(PluginTransactionSelectorFactory.class); + when(transactionSelectorFactory.create()).thenReturn(transactionSelector); + + final BlockTransactionSelector selector = + createBlockSelectorAndSetupTxPool( + isPoa + ? createMiningParameters( + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + 5, + Percentage.fromInt(75), + singleTxSelectionMaxTime) + : createMiningParameters( + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME, + singleTxSelectionMaxTime), + transactionProcessor, + blockHeader, + miningBeneficiary, + Wei.ZERO, + transactionSelectorFactory); + + transactionPool.addRemoteTransactions(txsProcessingTime.keySet()); + + final TransactionSelectionResults results = selector.buildTransactionListForBlock(); + + // third tx is not selected, even if it could fit in the block, + // since it took too long to process + assertThat(results.getSelectedTransactions()).containsExactlyElementsOf(txs.subList(0, 2)); + + // given enough time we can check the not selected tx + await().until(() -> !results.getNotSelectedTransactions().isEmpty()); + assertThat(results.getNotSelectedTransactions()) + .containsOnly(entry(txs.get(2), TransactionSelectionResult.TX_EVALUATION_TIMEOUT)); + } + + // spotless:off + /** + * Txs selection and timeout timeline + * + * 0 200 600 1100 1300 + * | | | | | + * 00000001111111112222222222TT333BT + * | | + * TS (0) (0,1) + * + * TS: Txs selected + * TT: Tx 3 timeout + * BT: Block timeout + * + * @param isPoa simulate a PoA network? + * @param preProcessingTooLate should the preProcessing step take too long? + * @param processingTooLate should the processing step take too long? + * @param postProcessingTooLate should the postProcessing step take too long? + */ + // spotless:on + @ParameterizedTest + @MethodSource("longProcessingTask") + public void subsetOfPendingTransactionsIncludedWhenPerTxEvaluationAndTxsSelectionMaxTimeIsOver( + final boolean isPoa, + final boolean preProcessingTooLate, + final boolean processingTooLate, + final boolean postProcessingTooLate) { + + final ProcessableBlockHeader blockHeader = createBlock(301_000); + final Address miningBeneficiary = AddressHelpers.ofValue(1); + final int poaMinBlockTime = 2; + final long blockTxsSelectionMaxTime = 1300; + final long singleTxSelectionMaxTime = 500; + final List txs = new ArrayList<>(4); + final List processingTimes = Arrays.asList(200L, 400L, 600L, 300L); + + for (int i = 0; i < 4; i++) { + final Transaction tx = createTransaction(i, Wei.of(7), 100_000); + txs.add(tx); + ensureTransactionIsValid(tx, 0, 0, processingTooLate ? processingTimes.get(i) : 0); + } + + final Map txsProcessingTime = + IntStream.of(0, 1, 2, 3).boxed().collect(Collectors.toMap(txs::get, processingTimes::get)); + + PluginTransactionSelector transactionSelector = mock(PluginTransactionSelector.class); + when(transactionSelector.evaluateTransactionPreProcessing(any())) + .thenAnswer(processingTx(preProcessingTooLate, txsProcessingTime)); + + when(transactionSelector.evaluateTransactionPostProcessing(any(), any())) + .thenAnswer(processingTx(postProcessingTooLate, txsProcessingTime)); + + final PluginTransactionSelectorFactory transactionSelectorFactory = + mock(PluginTransactionSelectorFactory.class); + when(transactionSelectorFactory.create()).thenReturn(transactionSelector); + + final BlockTransactionSelector selector = + createBlockSelectorAndSetupTxPool( + isPoa + ? createMiningParameters( + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + poaMinBlockTime, + Percentage.fromInt(65), + singleTxSelectionMaxTime) + : createMiningParameters( + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + blockTxsSelectionMaxTime, + singleTxSelectionMaxTime), + transactionProcessor, + blockHeader, + miningBeneficiary, + Wei.ZERO, + transactionSelectorFactory); + + transactionPool.addRemoteTransactions(txsProcessingTime.keySet()); + + final TransactionSelectionResults results = selector.buildTransactionListForBlock(); + + // third tx is not selected, even if it could fit in the block, + // since it took too long to process + assertThat(results.getSelectedTransactions()).containsExactlyElementsOf(txs.subList(0, 2)); + + // given enough time we can check the not selected tx + await().until(() -> results.getNotSelectedTransactions().size() == 2); + assertThat(results.getNotSelectedTransactions()) + .containsOnly( + entry(txs.get(2), TransactionSelectionResult.TX_EVALUATION_TIMEOUT), + entry(txs.get(3), TransactionSelectionResult.BLOCK_SELECTION_TIMEOUT)); + } + + // spotless:off + /** + * Txs selection and timeout timeline + * + * 0 200 + * | | + * 000000BTT + * + * BTT: Block and Tx timeout at the same time + * + * @param isPoa simulate a PoA network? + * @param preProcessingTooLate should the preProcessing step take too long? + * @param processingTooLate should the processing step take too long? + * @param postProcessingTooLate should the postProcessing step take too long? + */ + // spotless:on + @ParameterizedTest + @MethodSource("longProcessingTask") + public void perTxEvaluationTimeoutIsProcessedBeforeBlockTxsSelectionTimeout( + final boolean isPoa, + final boolean preProcessingTooLate, + final boolean processingTooLate, + final boolean postProcessingTooLate) { + + final ProcessableBlockHeader blockHeader = createBlock(301_000); + final Address miningBeneficiary = AddressHelpers.ofValue(1); + final int poaMinBlockTime = 1; + final long blockTxsSelectionMaxTime = 200; + final long singleTxSelectionMaxTime = 200; + final long actualTxSelectionTime = 300; + + final Transaction tx = createTransaction(0, Wei.of(7), 100_000); + ensureTransactionIsValid(tx, 0, 0, processingTooLate ? actualTxSelectionTime : 0); + + final Map txsProcessingTime = Map.of(tx, actualTxSelectionTime); + + PluginTransactionSelector transactionSelector = mock(PluginTransactionSelector.class); + when(transactionSelector.evaluateTransactionPreProcessing(any())) + .thenAnswer(processingTx(preProcessingTooLate, txsProcessingTime)); + + when(transactionSelector.evaluateTransactionPostProcessing(any(), any())) + .thenAnswer(processingTx(postProcessingTooLate, txsProcessingTime)); + + final PluginTransactionSelectorFactory transactionSelectorFactory = + mock(PluginTransactionSelectorFactory.class); + when(transactionSelectorFactory.create()).thenReturn(transactionSelector); + + final BlockTransactionSelector selector = + createBlockSelectorAndSetupTxPool( + isPoa + ? createMiningParameters( + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + poaMinBlockTime, + Percentage.fromInt(20), + singleTxSelectionMaxTime) + : createMiningParameters( + Wei.ZERO, + MIN_OCCUPANCY_100_PERCENT, + blockTxsSelectionMaxTime, + singleTxSelectionMaxTime), + transactionProcessor, + blockHeader, + miningBeneficiary, + Wei.ZERO, + transactionSelectorFactory); + + transactionPool.addRemoteTransactions(txsProcessingTime.keySet()); + + final TransactionSelectionResults results = selector.buildTransactionListForBlock(); + + // tx is not selected, even if it could fit in the block, + // since it took too long to process + assertThat(results.getSelectedTransactions()).isEmpty(); // given enough time we can check the not selected tx await().until(() -> !results.getNotSelectedTransactions().isEmpty()); assertThat(results.getNotSelectedTransactions()) - .containsOnly(entry(lateTx, TransactionSelectionResult.BLOCK_SELECTION_TIMEOUT)); + .containsOnly(entry(tx, TransactionSelectionResult.TX_EVALUATION_TIMEOUT)); } - private static Stream - subsetOfPendingTransactionsIncludedWhenTxSelectionMaxTimeIsOver() { + private static Stream longProcessingTask() { return Stream.of( Arguments.of(false, true, false, false), @@ -977,6 +1260,22 @@ public void subsetOfPendingTransactionsIncludedWhenTxSelectionMaxTimeIsOver( Arguments.of(true, false, false, true)); } + private Answer processingTx( + final boolean isTooLate, final Map txsProcessingTime) { + final Answer inTime = + invocation -> TransactionSelectionResult.SELECTED; + final Answer tooLate = + invocation -> { + final Transaction tx = ((PendingTransaction) invocation.getArgument(0)).getTransaction(); + + Thread.sleep(txsProcessingTime.get(tx)); + + return TransactionSelectionResult.SELECTED; + }; + + return isTooLate ? tooLate : inTime; + } + protected BlockTransactionSelector createBlockSelectorAndSetupTxPool( final MiningParameters miningParameters, final MainnetTransactionProcessor transactionProcessor, @@ -1119,14 +1418,21 @@ private BlockHeader blockHeader(final long number) { } protected MiningParameters createMiningParameters( - final Wei minGasPrice, final double minBlockOccupancyRatio, final long txsSelectionMaxTime) { + final Wei minGasPrice, + final double minBlockOccupancyRatio, + final long txsSelectionMaxTime, + final long txsSelectionPerTxMaxTime) { return ImmutableMiningParameters.builder() .mutableInitValues( MutableInitValues.builder() .minTransactionGasPrice(minGasPrice) .minBlockOccupancyRatio(minBlockOccupancyRatio) .build()) - .unstable(Unstable.builder().nonPoaBlockTxsSelectionMaxTime(txsSelectionMaxTime).build()) + .unstable( + Unstable.builder() + .nonPoaBlockTxsSelectionMaxTime(txsSelectionMaxTime) + .configuredBlockTxsSelectionPerTxMaxTime(txsSelectionPerTxMaxTime) + .build()) .build(); } @@ -1134,7 +1440,8 @@ protected MiningParameters createMiningParameters( final Wei minGasPrice, final double minBlockOccupancyRatio, final int minBlockTime, - final Percentage minBlockTimePercentage) { + final Percentage minBlockTimePercentage, + final long txsSelectionPerTxMaxTime) { return ImmutableMiningParameters.builder() .mutableInitValues( MutableInitValues.builder() @@ -1145,6 +1452,7 @@ protected MiningParameters createMiningParameters( Unstable.builder() .minBlockTime(minBlockTime) .poaBlockTxsSelectionMaxTime(minBlockTimePercentage) + .configuredBlockTxsSelectionPerTxMaxTime(txsSelectionPerTxMaxTime) .build()) .build(); } 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 3ff84afa56a..3972f45eff9 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 @@ -110,7 +110,10 @@ public void eip1559TransactionCurrentGasPriceLessThanMinimumIsSkippedAndKeptInTh final BlockTransactionSelector selector = createBlockSelectorAndSetupTxPool( createMiningParameters( - Wei.of(6), MIN_OCCUPANCY_80_PERCENT, DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), + Wei.of(6), + MIN_OCCUPANCY_80_PERCENT, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), transactionProcessor, blockHeader, miningBeneficiary, @@ -139,7 +142,10 @@ public void eip1559TransactionCurrentGasPriceGreaterThanMinimumIsSelected() { final BlockTransactionSelector selector = createBlockSelectorAndSetupTxPool( createMiningParameters( - Wei.of(6), MIN_OCCUPANCY_80_PERCENT, DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), + Wei.of(6), + MIN_OCCUPANCY_80_PERCENT, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), transactionProcessor, blockHeader, miningBeneficiary, @@ -167,7 +173,10 @@ public void eip1559PriorityTransactionCurrentGasPriceLessThanMinimumIsSelected() final BlockTransactionSelector selector = createBlockSelectorAndSetupTxPool( createMiningParameters( - Wei.of(6), MIN_OCCUPANCY_80_PERCENT, DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), + Wei.of(6), + MIN_OCCUPANCY_80_PERCENT, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME, + DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME), transactionProcessor, blockHeader, miningBeneficiary, diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/MiningParameters.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/MiningParameters.java index 32ac5ee926a..c808453b594 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/MiningParameters.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/MiningParameters.java @@ -327,5 +327,25 @@ default long getBlockTxsSelectionMaxTime() { } return getNonPoaBlockTxsSelectionMaxTime(); } + + OptionalLong getConfiguredBlockTxsSelectionPerTxMaxTime(); + + @Value.Derived + default long getBlockTxsSelectionPerTxMaxTime() { + if (getConfiguredBlockTxsSelectionPerTxMaxTime().isPresent()) { + final long configuredMaxTime = getConfiguredBlockTxsSelectionPerTxMaxTime().getAsLong(); + if (configuredMaxTime > getBlockTxsSelectionMaxTime()) { + throw new IllegalArgumentException( + "blockTxsSelectionPerTxMaxTime (" + + configuredMaxTime + + ") is greater than the blockTxsSelectionMaxTime (" + + getBlockTxsSelectionMaxTime() + + ")"); + } + return configuredMaxTime; + } + // by default, a tx is allowed to consume all the time allocated for a block + return getBlockTxsSelectionMaxTime(); + } } } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthScheduler.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthScheduler.java index 43f8d6a4c35..1220aa030ff 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthScheduler.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthScheduler.java @@ -211,6 +211,10 @@ public CompletableFuture scheduleBlockCreationTask(final Runnable task) { return CompletableFuture.runAsync(task, blockCreationExecutor); } + public CompletableFuture scheduleBlockCreationTask(final Supplier task) { + return CompletableFuture.supplyAsync(task, blockCreationExecutor); + } + public CompletableFuture timeout(final EthTask task) { return timeout(task, defaultTimeout); } diff --git a/plugin-api/build.gradle b/plugin-api/build.gradle index 6db2af92a14..bb2739eb249 100644 --- a/plugin-api/build.gradle +++ b/plugin-api/build.gradle @@ -69,7 +69,7 @@ Calculated : ${currentHash} tasks.register('checkAPIChanges', FileStateChecker) { description = "Checks that the API for the Plugin-API project does not change without deliberate thought" files = sourceSets.main.allJava.files - knownHash = '7Aj0APsKs1wBVqaWQFdEs85/MNKxTiVzyjIeZ+zCWlw=' + knownHash = 'tdEW+DZdDAZlqMGkEgsiTEWnM+m/EGNjLlKcG5MwMb8=' } check.dependsOn('checkAPIChanges') 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 1a397259e6e..e363847cec0 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 @@ -29,8 +29,10 @@ private enum Status { BLOCK_FULL(true, false), BLOCK_OCCUPANCY_ABOVE_THRESHOLD(true, false), BLOCK_SELECTION_TIMEOUT(true, false), + TX_EVALUATION_TIMEOUT(false, true), INVALID_TRANSIENT(false, false), - INVALID(false, true); + INVALID(false, true), + INTERNAL_ERROR(false, true); private final boolean stop; private final boolean discard; @@ -57,11 +59,12 @@ public String toString() { /** The transaction has not been selected since the block is full. */ public static final TransactionSelectionResult BLOCK_FULL = new TransactionSelectionResult(Status.BLOCK_FULL); - /** There was no more time to add transaction to the block */ + /** There was no more time to add more transactions to the block */ public static final TransactionSelectionResult BLOCK_SELECTION_TIMEOUT = new TransactionSelectionResult(Status.BLOCK_SELECTION_TIMEOUT); - ; - + /** The evaluation of this transaction was taking too much */ + public static final TransactionSelectionResult TX_EVALUATION_TIMEOUT = + new TransactionSelectionResult(Status.TX_EVALUATION_TIMEOUT); /** * The transaction has not been selected since too large and the occupancy of the block is enough * to stop the selection. @@ -86,6 +89,9 @@ public String toString() { */ public static final TransactionSelectionResult BLOB_PRICE_BELOW_CURRENT_MIN = TransactionSelectionResult.invalidTransient("BLOB_PRICE_BELOW_CURRENT_MIN"); + /** The evaluation of the transaction caused an internal error */ + public static final TransactionSelectionResult INTERNAL_ERROR = + new TransactionSelectionResult(Status.INTERNAL_ERROR); /** * The transaction has not been selected since its priority fee is below the configured min