Allow plugins to propose transactions during block creation #8268

fab-10 · 2025-02-07T17:40:03Z

PR description

With this PR, plugins that could already apply custom rules during the evaluation of pending transactions for block inclusion, can now directly propose pending transactions for block inclusion. Of course plugin proposed pending transactions must pass all the filters and be processed successfully.

In order to propose transactions a plugin implements its selection logic extending PluginTransactionSelectorFactory::selectPendingTransactions method, via that method it has access to the pending block header and the BlockTransactionSelectionService that is the core if the workflow, since it allows for evaluating pending transactions and depending on the result of the evaluations, the plugin can decide to either commit or rollback them.

The process of evaluation a pending transaction, not only changes the world state, but also affected the internal state of some selectors, for example the selector that tracks the cumulative gas used in the block or the blobs numbers, we can refer to these selectors as stateful, and a common unified strategy has been introduce to support stateful selectors, via the SelectorsStateManager, that is now responsible to keep and manage the working and commited state for each stateful selector.

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

spotless: ./gradlew spotlessApply
unit tests: ./gradlew build
acceptance tests: ./gradlew acceptanceTest
integration tests: ./gradlew integrationTest
reference tests: ./gradlew ethereum:referenceTests:referenceTests

matkt · 2025-02-11T14:03:05Z

...ava/org/hyperledger/besu/ethereum/blockcreation/txselection/TransactionSelectionResults.java

    selectedTransactions.add(transaction);
    transactionsByType
        .computeIfAbsent(transaction.getType(), type -> new ArrayList<>())
        .add(transaction);
    receipts.add(receipt);
    cumulativeGasUsed += gasUsed;
-    cumulativeBlobGasUsed += blobGasUsed;


why we are removing cumulativeBlobGasUsed ?

Before this class was used to both (1)track selection results and (2)managed some state for the selectors, now the 2 tasks have been separated and in this class are only kept the selection results(1) and since the cumulativeBlobGasUsed is not needed as result, but only useful for the stateful selector it has been moved into the BlobSizeTransactionSelector

matkt · 2025-02-11T14:06:13Z

...r/besu/ethereum/blockcreation/txselection/selectors/AbstractStatefulTransactionSelector.java

+    return selectorsStateManager.getSelectorWorkingState(this);
+  }
+
+  protected void setWorkingState(final S newState) {


not completly sure about this name, for me it was the WorldState and after I saw that it's a long 🤔

This is about the selector specific state, for example the BlobSizeTransactionSelector that needs to keep the cumulative count of how many blobs have been already added by previous txs

I don't know if it's better but maybe WorkingContext? SelectionContext?

Not sure, we already have BlockSelectionContext and TransactionEvaluationContext here so it can also create confusion.
Do you think using state could be confusing just at a first look, and after reviewing the code for longer the distinction is clearer?

I can also rename this method to setSelectorWorkingState, I thought it was redundant since it is in a selector class already, but if it helps readability then no problem

I think it's ok, maybe just add a comment to explain what is this state

matkt · 2025-02-11T14:08:51Z

...erledger/besu/ethereum/blockcreation/txselection/selectors/BlockSizeTransactionSelector.java

@@ -29,37 +29,39 @@
 * evaluating transactions based on block size. It checks if a transaction is too large for the
 * block and determines the selection result accordingly.
 */
-public class BlockSizeTransactionSelector extends AbstractTransactionSelector {
+public class BlockSizeTransactionSelector extends AbstractStatefulTransactionSelector<Long> {


AbstractStatefulTransactionSelector. I feel that this name should be in conflict with Statefull and stateless worldstate. Just thniking I don't have a strong opinion about that

I agree that state can refer to different things here, so one should read it along with transaction selector to fully qualify, not sure what could be another naming for this

matkt · 2025-02-11T14:10:38Z

util/src/main/java/org/hyperledger/besu/util/log4j/plugin/StackTraceMatchFilter.java

+    category = "Core",
+    elementType = "filter",
+    printObject = true)
+public class StackTraceMatchFilter extends AbstractFilter {


is this class needed for this PR ?

not really, it is an helper to configure log4j to avoid logging some exception that are not relevant during the tests, do you prefer to have it in a separate PR?

No if It is necessary for your tests I think it's OK

ahamlat

I haven't finished the review, but this is my first round of comments

ahamlat · 2025-02-11T14:59:55Z

.../hyperledger/besu/plugin/services/txselection/AbstractStatefulPluginTransactionSelector.java

+package org.hyperledger.besu.plugin.services.txselection;
+
+/**
+ * This class represents an abstract plugin transaction selector which provides manage the selector


small typo to fix : which provides manage the selector

ahamlat · 2025-02-11T15:02:37Z

.../hyperledger/besu/plugin/services/txselection/AbstractStatefulPluginTransactionSelector.java

+
+  /**
+   * Get the working state for this selector. A working state contains changes that have not yet
+   * commited


typo fix : committed

ahamlat · 2025-02-11T15:21:21Z

...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java

-        transaction -> evaluateTransaction(new PendingTransaction.Local.Priority(transaction)));
+    selectorsStateManager.blockSelectionStarted();
+
+    transactions.stream()


Streams are not good for performance especially when using map, so I prefer the the previous version.

ahamlat · 2025-02-11T15:23:18Z

...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java

-      final PendingTransaction pendingTransaction) {
+  @Override
+  public TransactionSelectionResult evaluatePendingTransaction(
+      final org.hyperledger.besu.datatypes.PendingTransaction pendingTransaction) {


Why do we need to import PendingTransaction with the whole package here ? is there another PendingTransaction ?

yes, for example when we create the actual pending txs in the evaluateTransactions method, while this method is from the interface and uses the PendingTransaction interface

ahamlat · 2025-02-11T15:50:01Z

...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java

-        processTransaction(evaluationContext.getTransaction(), txWorldStateUpdater);
+        processTransaction(evaluationContext.getTransaction());
+
+    txWorldStateUpdater.markTransactionBoundary();


Could you elaborate on why this is needed ?

this is needed in case more that one tx is processed before the commit, for example a plugin can only decide to commit only if txA and txB are both selected, so in this case you use the same world state updater first to execute txA and then txB, but without marking the tx boundary, it is possible that when executing txB there are some addresses already warm-up by txA, thus breaking the gas calculation.
This has no effect without plugin, since here we are committing after every selected, but provides flexibility for plugin to implement selection logic.

matkt · 2025-02-11T15:59:33Z

...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java

-        processTransaction(evaluationContext.getTransaction(), txWorldStateUpdater);
+        processTransaction(evaluationContext.getTransaction());
+
+    txWorldStateUpdater.markTransactionBoundary();


Is this called before the commit?

https://github.com/hyperledger/besu/blob/main/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/trie/diffbased/common/worldview/accumulator/DiffBasedWorldStateUpdateAccumulator.java#L609

it is called on StackedUpdater so if I understood correctly it should be fine

ok just wanted to be sure we are not using it for Bonsai accumulator

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

ahamlat

LGTM, it would be great if we can have more coverage on BlockSizeTransactionSelector class, as this PR changes the way cumulative gas is updated.

ahamlat · 2025-02-12T11:03:18Z

...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java

+        txWorldStateUpdater.commit();
+        blockWorldStateUpdater.commit();
+        for (final var pendingAction : selectedTxPendingActions) {
+          pendingAction.run();


I guess the executions will be sequential here, otherwise a use of an executor is more convenient if we want to have asynchronous executions.

yes this must be sequential and all pending actions must be completed before leaving the synchronized block

ahamlat · 2025-02-12T13:11:02Z

...pi/src/main/java/org/hyperledger/besu/plugin/services/txselection/SelectorsStateManager.java

+     * @return a copy of the long
+     */
+    static long duplicateLong(final long l) {
+      return l;


This method is a bit confusing for me as it returns the input param, I dont see why we need to call it in this case.

This is like the UnaryOperator::identity method, since basically there is no need to duplicate a long, it is used by selectors that have a Long as state like BlockSelectionContext

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

fab-10 force-pushed the plugin-proposed-txs branch from d85013c to 4de05eb Compare February 11, 2025 09:23

fab-10 marked this pull request as ready for review February 11, 2025 09:44

matkt reviewed Feb 11, 2025

View reviewed changes

ahamlat reviewed Feb 11, 2025

View reviewed changes

matkt reviewed Feb 11, 2025

View reviewed changes

fab-10 force-pushed the plugin-proposed-txs branch from 4de05eb to 01b729b Compare February 11, 2025 16:25

fab-10 mentioned this pull request Feb 11, 2025

Support stateful block tx selectors Consensys/linea-sequencer#140

Merged

fab-10 force-pushed the plugin-proposed-txs branch from 01b729b to de8c049 Compare February 12, 2025 09:27

fab-10 added 2 commits February 12, 2025 10:41

Allow plugins to propose transactions during block creation

066ca5e

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

Apply suggestions from code review

4ac97a7

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

fab-10 force-pushed the plugin-proposed-txs branch from de8c049 to 4ac97a7 Compare February 12, 2025 09:41

Document AbstractStatefulTransactionSelector

8b7851c

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

ahamlat approved these changes Feb 12, 2025

View reviewed changes

Unit tests for BlockSizeTransactionSelectorTest

d892644

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

matkt approved these changes Feb 12, 2025

View reviewed changes

fab-10 merged commit 428a638 into hyperledger:main Feb 12, 2025
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow plugins to propose transactions during block creation #8268

Allow plugins to propose transactions during block creation #8268

fab-10 commented Feb 7, 2025 •

edited

Loading

matkt Feb 11, 2025

fab-10 Feb 11, 2025

matkt Feb 11, 2025

fab-10 Feb 11, 2025

matkt Feb 11, 2025

fab-10 Feb 11, 2025

matkt Feb 12, 2025

fab-10 Feb 12, 2025

matkt Feb 11, 2025

fab-10 Feb 11, 2025

matkt Feb 11, 2025

fab-10 Feb 11, 2025

matkt Feb 11, 2025

ahamlat left a comment

ahamlat Feb 11, 2025

ahamlat Feb 11, 2025

ahamlat Feb 11, 2025

fab-10 Feb 11, 2025

ahamlat Feb 11, 2025

fab-10 Feb 11, 2025

ahamlat Feb 11, 2025

fab-10 Feb 11, 2025

matkt Feb 11, 2025

matkt Feb 11, 2025

fab-10 Feb 11, 2025

matkt Feb 12, 2025

ahamlat left a comment

ahamlat Feb 12, 2025 •

edited

Loading

fab-10 Feb 12, 2025

ahamlat Feb 12, 2025 •

edited

Loading

fab-10 Feb 12, 2025

Allow plugins to propose transactions during block creation #8268

Allow plugins to propose transactions during block creation #8268

Conversation

fab-10 commented Feb 7, 2025 • edited Loading

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Locally, you can run these tests to catch failures early:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahamlat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahamlat left a comment

Choose a reason for hiding this comment

ahamlat Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahamlat Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fab-10 commented Feb 7, 2025 •

edited

Loading

ahamlat Feb 12, 2025 •

edited

Loading

ahamlat Feb 12, 2025 •

edited

Loading