[Plugin API] - TransactionSelector - Send TransactionSelectionResult to the plugin when not transaction is not selected (#6010)

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

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

@@ -125,6 +125,15 @@ public class BlockTransactionSelector {
             .orElse(AllAcceptingTransactionSelector.INSTANCE);
   }
 
+  private List<AbstractTransactionSelector> 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 class BlockTransactionSelector {
         .setMessage("Transaction pool stats {}")
         .addArgument(blockSelectionContext.transactionPool().logStats())
         .log();
-    blockSelectionContext
+    blockSelectionContext.transactionPool().selectTransactions(this::evaluateTransaction);
-        .transactionPool()
-        .selectTransactions(
-            pendingTransaction -> {
-              final var res = evaluateTransaction(pendingTransaction);
-              if (!res.selected()) {
-                updateTransactionRejected(pendingTransaction, res);
-              }
-              return res;
-            });
     LOG.atTrace()
         .setMessage("Transaction selection result {}")
         .addArgument(transactionSelectionResults::toTraceLog)
@@ -167,105 +167,40 @@ public class BlockTransactionSelector {
    */
   public TransactionSelectionResults evaluateTransactions(final List<Transaction> transactions) {
     transactions.forEach(
-        transaction -> {
+        transaction -> evaluateTransaction(new PendingTransaction.Local(transaction)));
-          var pendingTransaction = new PendingTransaction.Local(transaction);
-          final var res = evaluateTransaction(pendingTransaction);
-          if (!res.selected()) {
-            updateTransactionRejected(pendingTransaction, res);
-          }
-        });
     return transactionSelectionResults;
   }
 
-  /*
+  /**
    * Passed into the PendingTransactions, and is called on each transaction until sufficient
-   * transactions are found which fill a block worth of gas.
+   * 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
-   * This function will continue to be called until the block under construction is suitably
+   * provided transaction's gasLimit does not fit within the space remaining in the block.
-   * 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()) {
+    checkCancellation();
-      throw new CancellationException("Cancelled during transaction selection.");
-    }
-
-    final Transaction transaction = pendingTransaction.getTransaction();
 
-    TransactionSelectionResult selectionResult =
+    TransactionSelectionResult selectionResult = evaluatePreProcessing(pendingTransaction);
-        evaluateTransactionPreProcessing(pendingTransaction);
     if (!selectionResult.selected()) {
-      return selectionResult;
+      return handleTransactionNotSelected(pendingTransaction, selectionResult);
     }
 
     final WorldUpdater worldStateUpdater = worldState.updater();
-    final BlockHashLookup blockHashLookup =
+    final TransactionProcessingResult processingResult =
-        new CachingBlockHashLookup(blockSelectionContext.processableBlockHeader(), blockchain);
+        processTransaction(pendingTransaction, worldStateUpdater);
-
-    final TransactionProcessingResult effectiveResult =
-        transactionProcessor.processTransaction(
-            blockchain,
-            worldStateUpdater,
-            blockSelectionContext.processableBlockHeader(),
-            transaction,
-            blockSelectionContext.miningBeneficiary(),
-            blockHashLookup,
-            false,
-            TransactionValidationParams.mining(),
-            blockSelectionContext.blobGasPrice());
 
-    var transactionWithProcessingContextResult =
+    var postProcessingSelectionResult =
-        evaluateTransactionPostProcessing(pendingTransaction, effectiveResult);
+        evaluatePostProcessing(pendingTransaction, processingResult);
-    if (!transactionWithProcessingContextResult.selected()) {
+    if (!postProcessingSelectionResult.selected()) {
-      return transactionWithProcessingContextResult;
+      return handleTransactionNotSelected(pendingTransaction, postProcessingSelectionResult);
     }
 
-    final long gasUsedByTransaction = transaction.getGasLimit() - effectiveResult.getGasRemaining();
+    return handleTransactionSelected(pendingTransaction, processingResult, worldStateUpdater);
-    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);
   }
 
   /**
@@ -277,15 +212,13 @@ public class BlockTransactionSelector {
    * @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 @@ public class BlockTransactionSelector {
    * @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 @@ public class BlockTransactionSelector {
         pendingTransaction, processingResult);
   }
 
-  private List<AbstractTransactionSelector> createTransactionSelectors(
+  /**
-      final BlockSelectionContext context) {
+   * Processes a transaction
-    return List.of(
+   *
-        new BlockSizeTransactionSelector(context),
+   * @param pendingTransaction The transaction to be processed.
-        new PriceTransactionSelector(context),
+   * @param worldStateUpdater The world state updater.
-        new BlobPriceTransactionSelector(context),
+   * @return The result of the transaction processing.
-        new ProcessingResultTransactionSelector(context));
+   */
+  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();
   }
 }

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

@@ -674,9 +674,10 @@ public abstract class AbstractBlockTransactionSelectorTest {
     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(
+    ensureTransactionIsInvalid(invalidTransaction, invalidReason);
-        invalidTransaction, TransactionInvalidReason.PLUGIN_TX_VALIDATOR_INVALIDATED);
     transactionPool.addRemoteTransactions(List.of(transaction, invalidTransaction));
 
     createBlockSelectorWithTxSelPlugin(
@@ -692,11 +693,16 @@ public abstract class AbstractBlockTransactionSelectorTest {
     ArgumentCaptor<PendingTransaction> 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);
   }

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')
 

plugin-api/src/main/java/org/hyperledger/besu/plugin/services/txselection/TransactionSelector.java

@@ -51,9 +51,12 @@ public interface TransactionSelector {
    */
   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) {}
 }