disable bal for transaction selection when not activated by matkt · Pull Request #9189 · besu-eth/besu

matkt · 2025-09-18T06:04:32Z

PR description

Should disable bal for transaction selection when not activated

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
hive tests: Engine or other RPCs modified?

Signed-off-by: Karim Taam <karim.t2am@gmail.com>

mirgee · 2025-09-18T06:20:44Z

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

+    maybeBlockAccessListBuilder =
+        protocolSpec
+            .getBlockAccessListFactory()
+            .filter(BlockAccessListFactory::isEnabled)


This should be BlockAccessListFactory::isForkEnabled - including BALs built blocks without the fork being active might lead to building invalid blocks.

I agree so I think we should add the same check here

https://github.com/hyperledger/besu/blob/main/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java#L312

Well, currently newProtocolSpec.getBlockAccessListFactory.isPresent() is equivalent to BlockAccessListFactory::isForkEnabled, because if the factory is present it means that the BAL fork is enabled. But it is true that this may not be true in general so the check should be more specific.

modified the code in order to have a isForkActivated for block creation ; transaction evaluation and simulation/ To be safe for fusaka

Signed-off-by: Karim Taam <karim.t2am@gmail.com>

jflo

I think this might work to clear fusaka, but will likely break other stuff. I suggest revisiting the design and removing any knowledge of "why" it is active from the BlockAccessListFactory

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

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/BlockSimulator.java

Signed-off-by: Karim Taam <karim.t2am@gmail.com>

fab-10

LGTM, just a question and a suggestion

fab-10 · 2025-09-18T14:14:16Z

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

+        for (final var pendingAction : selectedTxPendingActions) {
+          pendingAction.run();
+        }


From what I can see there is no pending action that rely on the state to be committed, so it is fine to running the pending actions before the state commits.

fab-10 · 2025-09-18T14:15:37Z

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

+                            if (txWorldStateUpdater
+                                instanceof StackedUpdater<?, ?> stackedUpdater) {
+                              blockAccessListBuilder.addTransactionLevelAccessList(
+                                  transactionAccessList, stackedUpdater);


Why this check about the type? and if the type does not match it is fine to just ignore?

For now we are not yet compatible with Journaled updater. This is a work that will have to do. For now, no particular problem because it's just experimental.

fab-10 · 2025-09-18T14:20:19Z

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

+    maybeBlockAccessListBuilder =
+        protocolSpec
+            .getBlockAccessListFactory()


Non blocking suggestion, to keep the clean of all the checks about the presence or not of the BAL builder, what if instead having the factory that just return a NoOp BAL builder when it is disabled?

nit: Also prefer the NoopBal approach.

nit: Whether Noop or Optional, I prefer that the fork-dependent wiring happens in MainnetProtocolSpecs rather than in-line in this constructor, since that is our common convention.

e.g. similar approach to NoBlobSchedule https://github.com/hyperledger/besu/blob/3227dd647d1b52c06fd1cd624ae0ad1d5577c413/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ProtocolSpecBuilder.java#L81

https://github.com/hyperledger/besu/blob/3227dd647d1b52c06fd1cd624ae0ad1d5577c413/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetProtocolSpecs.java#L758-L762

Looking at the code a bit more I see it is indeed wired into the MainnetProtocolSpecs futureEipsDefinition, so I'm not sure why we need the extra isForkActivated filter, shouldn't the class just not exist for Osaka?

No, because for testing reasons for the moment the option can be activated by flag and also by fork. The idea is to be able to test BAL on the current mainnet too

@mirgee Do you remember exactly why re used this solution. I forgot the main reason. At first there was a noop but that was changed.

@matkt I am not sure I understand the question - what aspect of the solution are we talking about? There never was a noop builder in the first iteration. I think that the code can be refactored to use a noop builder, but I wouldn't say it's the first priority right now.

jframe · 2025-09-18T23:59:06Z

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

-    if (txWorldStateUpdater instanceof StackedUpdater<?, ?> stackedUpdater) {
-      balBuilder.addTransactionLevelAccessList(transactionAccessList, stackedUpdater);
-    }


just a question. why was the balBuilder.addTransactionLevelAccessList call moved from here to later to the handleTransactionSelected method?

Because that way we are sure to add only the transactions that are selected in the bal and not those that are in timeout. The reason for the bug you had.

siladu

Approving for expediency but couple of things to revisit, esp test coverage.

siladu · 2025-09-19T00:22:03Z

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

+    maybeBlockAccessListBuilder =
+        protocolSpec
+            .getBlockAccessListFactory()


nit: Also prefer the NoopBal approach.

nit: Whether Noop or Optional, I prefer that the fork-dependent wiring happens in MainnetProtocolSpecs rather than in-line in this constructor, since that is our common convention.

e.g. similar approach to NoBlobSchedule https://github.com/hyperledger/besu/blob/3227dd647d1b52c06fd1cd624ae0ad1d5577c413/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ProtocolSpecBuilder.java#L81

https://github.com/hyperledger/besu/blob/3227dd647d1b52c06fd1cd624ae0ad1d5577c413/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetProtocolSpecs.java#L758-L762

siladu · 2025-09-19T00:35:20Z

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

+    maybeBlockAccessListBuilder =
+        protocolSpec
+            .getBlockAccessListFactory()


Looking at the code a bit more I see it is indeed wired into the MainnetProtocolSpecs futureEipsDefinition, so I'm not sure why we need the extra isForkActivated filter, shouldn't the class just not exist for Osaka?

siladu · 2025-09-19T00:47:52Z

...tion/src/test/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreatorTest.java

  }

+  @Test
+  public void blockAccessListShouldNotIncludesAccountWithoutBalSupport() {


nit: This test doesn't appear to exercise the code that was causing the exception: #9186
(in BlockTransactionSelector.buildTransactionListForBlock)

Ideally, we'd ensure blockAccessListBuilder.build() never gets called.

matkt · 2025-09-19T04:16:59Z

Approving for expediency but couple of things to revisit, esp test coverage.

There is a references test but we can't merge it yet.

* disable bal for transaction selection when not activated Signed-off-by: Karim Taam <karim.t2am@gmail.com> * disable bal for block creation and simulator when not activated Signed-off-by: Karim Taam <karim.t2am@gmail.com> * update bal only for selected transaction Signed-off-by: Karim Taam <karim.t2am@gmail.com> * move pending task during block creation before commit for BAL Signed-off-by: Karim Taam <karim.t2am@gmail.com> --------- Signed-off-by: Karim Taam <karim.t2am@gmail.com> Signed-off-by: Ali <alijakparov.kz@gmail.com>

* disable bal for transaction selection when not activated Signed-off-by: Karim Taam <karim.t2am@gmail.com> * disable bal for block creation and simulator when not activated Signed-off-by: Karim Taam <karim.t2am@gmail.com> * update bal only for selected transaction Signed-off-by: Karim Taam <karim.t2am@gmail.com> * move pending task during block creation before commit for BAL Signed-off-by: Karim Taam <karim.t2am@gmail.com> --------- Signed-off-by: Karim Taam <karim.t2am@gmail.com> Signed-off-by: Ali <alijakparov.kz@gmail.com> Signed-off-by: Ali Zhagparov <alijakparov.kz@gmail.com>

* disable bal for transaction selection when not activated Signed-off-by: Karim Taam <karim.t2am@gmail.com> * disable bal for block creation and simulator when not activated Signed-off-by: Karim Taam <karim.t2am@gmail.com> * update bal only for selected transaction Signed-off-by: Karim Taam <karim.t2am@gmail.com> * move pending task during block creation before commit for BAL Signed-off-by: Karim Taam <karim.t2am@gmail.com> --------- Signed-off-by: Karim Taam <karim.t2am@gmail.com> Signed-off-by: jflo <justin+github@florentine.us>

disable bal for transaction selection when not activated

be2c512

Signed-off-by: Karim Taam <karim.t2am@gmail.com>

mirgee reviewed Sep 18, 2025

View reviewed changes

disable bal for block creation and simulator when not activated

a7fc454

Signed-off-by: Karim Taam <karim.t2am@gmail.com>

jflo reviewed Sep 18, 2025

View reviewed changes

...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java Show resolved Hide resolved

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/transaction/BlockSimulator.java Show resolved Hide resolved

update bal only for selected transaction

afe7333

Signed-off-by: Karim Taam <karim.t2am@gmail.com>

mirgee mentioned this pull request Sep 18, 2025

EIP-7928 - Adjust BAL implementation to pass spec tests #9190

Merged

2 tasks

matkt added 2 commits September 18, 2025 15:50

move pending task during block creation before commit for BAL

b163a86

Signed-off-by: Karim Taam <karim.t2am@gmail.com>

Merge branch 'main' into bal-trx-selector-issue

27b3512

fab-10 reviewed Sep 18, 2025

View reviewed changes

jflo approved these changes Sep 18, 2025

View reviewed changes

jframe reviewed Sep 18, 2025

View reviewed changes

jframe approved these changes Sep 19, 2025

View reviewed changes

siladu approved these changes Sep 19, 2025

View reviewed changes

jframe merged commit 0677923 into besu-eth:main Sep 19, 2025
56 of 65 checks passed

matkt added the block-access-lists label Sep 23, 2025

Conversation

matkt commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matkt Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirgee Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jflo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fab-10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jframe Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siladu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matkt commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

matkt commented Sep 18, 2025 •

edited

Loading

matkt Sep 18, 2025 •

edited

Loading

mirgee Sep 18, 2025 •

edited

Loading

jframe Sep 18, 2025 •

edited

Loading

matkt commented Sep 19, 2025 •

edited

Loading