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 f55ea963648..7a86c928f5e 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 @@ -125,6 +125,15 @@ public BlockTransactionSelector( .orElse(AllAcceptingTransactionSelector.INSTANCE); } + private List createTransactionSelectors( + final BlockSelectionContext context) { + return List.of( + new BlockSizeTransactionSelector(context), + new PriceTransactionSelector(context), + new BlobPriceTransactionSelector(context), + new ProcessingResultTransactionSelector(context)); + } + /** * Builds a list of transactions for a block by iterating over all transactions in the * PendingTransactions pool. This operation can be long-running and, if executed in a separate @@ -139,16 +148,7 @@ public TransactionSelectionResults buildTransactionListForBlock() { .setMessage("Transaction pool stats {}") .addArgument(blockSelectionContext.transactionPool().logStats()) .log(); - blockSelectionContext - .transactionPool() - .selectTransactions( - pendingTransaction -> { - final var res = evaluateTransaction(pendingTransaction); - if (!res.selected()) { - updateTransactionRejected(pendingTransaction, res); - } - return res; - }); + blockSelectionContext.transactionPool().selectTransactions(this::evaluateTransaction); LOG.atTrace() .setMessage("Transaction selection result {}") .addArgument(transactionSelectionResults::toTraceLog) @@ -167,105 +167,40 @@ public TransactionSelectionResults buildTransactionListForBlock() { */ public TransactionSelectionResults evaluateTransactions(final List transactions) { transactions.forEach( - transaction -> { - var pendingTransaction = new PendingTransaction.Local(transaction); - final var res = evaluateTransaction(pendingTransaction); - if (!res.selected()) { - updateTransactionRejected(pendingTransaction, res); - } - }); + transaction -> evaluateTransaction(new PendingTransaction.Local(transaction))); return transactionSelectionResults; } - /* + /** * 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 called until the block under construction is suitably - * full (in terms of gasLimit) and the provided transaction's gasLimit does not fit within - * the space remaining in the block. + * transactions are found which fill a block worth of gas. This function will continue to be + * called until the block under construction is suitably full (in terms of gasLimit) and the + * 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) { - if (isCancelled.get()) { - throw new CancellationException("Cancelled during transaction selection."); - } - - final Transaction transaction = pendingTransaction.getTransaction(); + checkCancellation(); - TransactionSelectionResult selectionResult = - evaluateTransactionPreProcessing(pendingTransaction); + TransactionSelectionResult selectionResult = evaluatePreProcessing(pendingTransaction); if (!selectionResult.selected()) { - return selectionResult; + return handleTransactionNotSelected(pendingTransaction, selectionResult); } final WorldUpdater worldStateUpdater = worldState.updater(); - final BlockHashLookup blockHashLookup = - new CachingBlockHashLookup(blockSelectionContext.processableBlockHeader(), blockchain); - - final TransactionProcessingResult effectiveResult = - transactionProcessor.processTransaction( - blockchain, - worldStateUpdater, - blockSelectionContext.processableBlockHeader(), - transaction, - blockSelectionContext.miningBeneficiary(), - blockHashLookup, - false, - TransactionValidationParams.mining(), - blockSelectionContext.blobGasPrice()); + final TransactionProcessingResult processingResult = + processTransaction(pendingTransaction, worldStateUpdater); - var transactionWithProcessingContextResult = - evaluateTransactionPostProcessing(pendingTransaction, effectiveResult); - if (!transactionWithProcessingContextResult.selected()) { - return transactionWithProcessingContextResult; + var postProcessingSelectionResult = + evaluatePostProcessing(pendingTransaction, processingResult); + if (!postProcessingSelectionResult.selected()) { + return handleTransactionNotSelected(pendingTransaction, postProcessingSelectionResult); } - final long gasUsedByTransaction = transaction.getGasLimit() - effectiveResult.getGasRemaining(); - final long cumulativeGasUsed = - transactionSelectionResults.getCumulativeGasUsed() + gasUsedByTransaction; - - worldStateUpdater.commit(); - final TransactionReceipt receipt = - transactionReceiptFactory.create( - transaction.getType(), effectiveResult, worldState, cumulativeGasUsed); - - final long blobGasUsed = - blockSelectionContext.gasCalculator().blobGasCost(transaction.getBlobCount()); - - updateTransactionSelected(pendingTransaction, receipt, gasUsedByTransaction, blobGasUsed); - - LOG.atTrace() - .setMessage("Selected {} for block creation") - .addArgument(transaction::toTraceLog) - .log(); - - return TransactionSelectionResult.SELECTED; - } - - private void updateTransactionSelected( - final PendingTransaction pendingTransaction, - final TransactionReceipt receipt, - final long gasUsedByTransaction, - final long blobGasUsed) { - - transactionSelectionResults.updateSelected( - pendingTransaction.getTransaction(), receipt, gasUsedByTransaction, blobGasUsed); - - // notify external selector if any - externalTransactionSelector.onTransactionSelected(pendingTransaction); - } - - private void updateTransactionRejected( - final PendingTransaction pendingTransaction, - final TransactionSelectionResult processingResult) { - - transactionSelectionResults.updateNotSelected( - pendingTransaction.getTransaction(), processingResult); - - // notify external selector if any - externalTransactionSelector.onTransactionRejected(pendingTransaction); + return handleTransactionSelected(pendingTransaction, processingResult, worldStateUpdater); } /** @@ -277,15 +212,13 @@ private void updateTransactionRejected( * @param pendingTransaction The transaction to be evaluated. * @return The result of the transaction selection process. */ - private TransactionSelectionResult evaluateTransactionPreProcessing( + private TransactionSelectionResult evaluatePreProcessing( final PendingTransaction pendingTransaction) { - // Process the transaction through internal selectors for (var selector : transactionSelectors) { TransactionSelectionResult result = selector.evaluateTransactionPreProcessing( pendingTransaction, transactionSelectionResults); - // If the transaction is not selected by any internal selector, return the result if (!result.equals(TransactionSelectionResult.SELECTED)) { return result; } @@ -303,16 +236,14 @@ private TransactionSelectionResult evaluateTransactionPreProcessing( * @param processingResult The result of the transaction processing. * @return The result of the transaction selection process. */ - private TransactionSelectionResult evaluateTransactionPostProcessing( + private TransactionSelectionResult evaluatePostProcessing( final PendingTransaction pendingTransaction, final TransactionProcessingResult processingResult) { - // Process the transaction through internal selectors for (var selector : transactionSelectors) { TransactionSelectionResult result = selector.evaluateTransactionPostProcessing( pendingTransaction, transactionSelectionResults, processingResult); - // If the transaction is not selected by any selector, return the result if (!result.equals(TransactionSelectionResult.SELECTED)) { return result; } @@ -321,12 +252,94 @@ private TransactionSelectionResult evaluateTransactionPostProcessing( pendingTransaction, processingResult); } - private List createTransactionSelectors( - final BlockSelectionContext context) { - return List.of( - new BlockSizeTransactionSelector(context), - new PriceTransactionSelector(context), - new BlobPriceTransactionSelector(context), - new ProcessingResultTransactionSelector(context)); + /** + * Processes a transaction + * + * @param pendingTransaction The transaction to be processed. + * @param worldStateUpdater The world state updater. + * @return The result of the transaction processing. + */ + private TransactionProcessingResult processTransaction( + final PendingTransaction pendingTransaction, final WorldUpdater worldStateUpdater) { + final BlockHashLookup blockHashLookup = + new CachingBlockHashLookup(blockSelectionContext.processableBlockHeader(), blockchain); + return transactionProcessor.processTransaction( + blockchain, + worldStateUpdater, + blockSelectionContext.processableBlockHeader(), + pendingTransaction.getTransaction(), + blockSelectionContext.miningBeneficiary(), + blockHashLookup, + false, + TransactionValidationParams.mining(), + blockSelectionContext.blobGasPrice()); + } + + /** + * Handles a selected transaction by committing the world state updates, creating a transaction + * receipt, updating the TransactionSelectionResults with the selected transaction, and notifying + * the external transaction selector. + * + * @param pendingTransaction The pending transaction. + * @param processingResult The result of the transaction processing. + * @param worldStateUpdater The world state updater. + * @return The result of the transaction selection process. + */ + private TransactionSelectionResult handleTransactionSelected( + final PendingTransaction pendingTransaction, + final TransactionProcessingResult processingResult, + final WorldUpdater worldStateUpdater) { + worldStateUpdater.commit(); + final Transaction transaction = pendingTransaction.getTransaction(); + + final long gasUsedByTransaction = + transaction.getGasLimit() - processingResult.getGasRemaining(); + final long cumulativeGasUsed = + transactionSelectionResults.getCumulativeGasUsed() + gasUsedByTransaction; + final long blobGasUsed = + blockSelectionContext.gasCalculator().blobGasCost(transaction.getBlobCount()); + + final TransactionReceipt receipt = + transactionReceiptFactory.create( + transaction.getType(), processingResult, worldState, cumulativeGasUsed); + + logTransactionSelection(pendingTransaction.getTransaction()); + + transactionSelectionResults.updateSelected( + pendingTransaction.getTransaction(), receipt, gasUsedByTransaction, blobGasUsed); + externalTransactionSelector.onTransactionSelected(pendingTransaction); + + return TransactionSelectionResult.SELECTED; + } + + /** + * Handles the scenario when a transaction is not selected. It updates the + * TransactionSelectionResults with the unselected transaction, and notifies the external + * 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. + */ + private TransactionSelectionResult handleTransactionNotSelected( + final PendingTransaction pendingTransaction, + final TransactionSelectionResult selectionResult) { + transactionSelectionResults.updateNotSelected( + pendingTransaction.getTransaction(), selectionResult); + externalTransactionSelector.onTransactionNotSelected(pendingTransaction, selectionResult); + return selectionResult; + } + + private void checkCancellation() { + if (isCancelled.get()) { + throw new CancellationException("Cancelled during transaction selection."); + } + } + + private void logTransactionSelection(final Transaction transaction) { + LOG.atTrace() + .setMessage("Selected {} for block creation") + .addArgument(transaction::toTraceLog) + .log(); } } 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 b17eb8b5249..4c1f0018177 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 @@ -674,9 +674,10 @@ public void transactionSelectionPluginShouldBeNotifiedWhenTransactionSelectionCo final Transaction transaction = createTransaction(0, Wei.of(10), 21_000); ensureTransactionIsValid(transaction, 21_000, 0); + final TransactionInvalidReason invalidReason = + TransactionInvalidReason.PLUGIN_TX_VALIDATOR_INVALIDATED; final Transaction invalidTransaction = createTransaction(1, Wei.of(10), 21_000); - ensureTransactionIsInvalid( - invalidTransaction, TransactionInvalidReason.PLUGIN_TX_VALIDATOR_INVALIDATED); + ensureTransactionIsInvalid(invalidTransaction, invalidReason); transactionPool.addRemoteTransactions(List.of(transaction, invalidTransaction)); createBlockSelectorWithTxSelPlugin( @@ -692,11 +693,16 @@ public void transactionSelectionPluginShouldBeNotifiedWhenTransactionSelectionCo ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(PendingTransaction.class); + // selected transaction must be notified to the selector verify(transactionSelector).onTransactionSelected(argumentCaptor.capture()); PendingTransaction selected = argumentCaptor.getValue(); assertThat(selected.getTransaction()).isEqualTo(transaction); - verify(transactionSelector).onTransactionRejected(argumentCaptor.capture()); + // unselected transaction must be notified to the selector with correct reason + verify(transactionSelector) + .onTransactionNotSelected( + argumentCaptor.capture(), + eq(TransactionSelectionResult.invalid(invalidReason.toString()))); PendingTransaction rejectedTransaction = argumentCaptor.getValue(); assertThat(rejectedTransaction.getTransaction()).isEqualTo(invalidTransaction); } diff --git a/plugin-api/build.gradle b/plugin-api/build.gradle index 231fd953d38..28973cc1f52 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 = '+7wo9cABKEFyYvjtpDFAOXqVKBAkffdnb433hT0VQ7I=' + knownHash = 'Pfql+wKH6qarEvlb9TXZGVV/xwJuKWK5egKHt9uCpzE=' } check.dependsOn('checkAPIChanges') diff --git a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/txselection/TransactionSelector.java b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/txselection/TransactionSelector.java index 87ec0471bbe..1c124add998 100644 --- a/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/txselection/TransactionSelector.java +++ b/plugin-api/src/main/java/org/hyperledger/besu/plugin/services/txselection/TransactionSelector.java @@ -51,9 +51,12 @@ TransactionSelectionResult evaluateTransactionPostProcessing( */ default void onTransactionSelected(final PendingTransaction pendingTransaction) {} /** - * Method called when a transaction is rejected to be added to a block. + * Method called when a transaction is not selected to be added to a block. * - * @param pendingTransaction The transaction that has been rejected. + * @param pendingTransaction The transaction that has not been selected. + * @param transactionSelectionResult The transaction selection result */ - default void onTransactionRejected(final PendingTransaction pendingTransaction) {} + default void onTransactionNotSelected( + final PendingTransaction pendingTransaction, + final TransactionSelectionResult transactionSelectionResult) {} }