Optimistic parallelization of transactions to improve performance by matkt · Pull Request #7296 · besu-eth/besu

matkt · 2024-07-05T15:10:39Z

PR description

The idea of this PR is to create a system for parallelizing transactions in an optimistic way. What's the parallelization mechanism we're proposing?

We plan to execute all transactions within a block in parallel, without checking for conflicts between them. We're starting with the assumption that everything can be parallelized. Then, without waiting for the results of these background executions, we'll process the block sequentially and fix conflict.

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

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

Initialize ConcurrentHashMap used in the accumulator to avoid collisions Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

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

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

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

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

…ns and the number of conflicting transactions Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

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

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

Signed-off-by: ahamlat <ameziane.hamlat@consensys.net>

fab-10 · 2024-07-16T13:35:22Z

      final Wei coinbaseWeiDelta =
          coinbaseCalculator.price(usedGas, transactionGasPrice, blockHeader.getBaseFee());

+      operationTracer.traceBeforeRewardTransaction(worldUpdater, transaction, coinbaseWeiDelta);


this seems unrelated

This was a hint used by Karim to handle the miningBeneficiary case. He used the tracing feature to access the context of miningBeneficiary in ParallelizedConcurrentTransactionProcessor.runTransaction, and check if the address of the miner was touched before executing the transaction.

Yes it is necessary for the PR and for conflict detection. I explain it in the comments normally

In fact I use it to know if we touched the address of the beneficiary before paying the reward. If we touch it before it is a conflict if we touch it just for the reward it's ok

Sorry for the late feedback here, but wouldn't paying the reward to the coinbase affect consensus? If one of the parallel transactions was from the coinbase, couldn't its result be affected by interim transaction rewards if the coinbase submitted a transaction that invoked BALANCE opcode?

~~I think we need to flatly ban coinbase from parallelization for this reason.~~

this ban exists already in the conflict detection, I just missed it because it was for miningBeneficiary and I was looking for coinbase

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

… common address is found Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

Signed-off-by: ahamlat <ameziane.hamlat@consensys.net>

garyschulte

I think transactions sent from a coinbase are a problem, and they should be banned from parallelization period.

If I am wrong, I would love to see a test block that proved parallelized coinbase transactions which use BALANCE opcode are safe/deterministic.

garyschulte · 2024-07-18T17:41:41Z

      final Wei coinbaseWeiDelta =
          coinbaseCalculator.price(usedGas, transactionGasPrice, blockHeader.getBaseFee());

+      operationTracer.traceBeforeRewardTransaction(worldUpdater, transaction, coinbaseWeiDelta);


Sorry for the late feedback here, but wouldn't paying the reward to the coinbase affect consensus? If one of the parallel transactions was from the coinbase, couldn't its result be affected by interim transaction rewards if the coinbase submitted a transaction that invoked BALANCE opcode?

~~I think we need to flatly ban coinbase from parallelization for this reason.~~

this ban exists already in the conflict detection, I just missed it because it was for miningBeneficiary and I was looking for coinbase

matkt · 2024-07-18T19:29:46Z

I think transactions sent from a coinbase are a problem, and they should be banned from parallelization period.

If I am wrong, I would love to see a test block that proved parallelized coinbase transactions which use BALANCE opcode are safe/deterministic.

A transaction that will interact with the coinbase to send, receive, or simply read or modify throughout the transaction will be in the accumulator at the time of this event #7296 (comment).

Therefore, before paying the reward, If we detect this coinbase access, it will necessarily be banned from parallelization, and that's why we have this code. We track the state of the accumulator before paying the reward because it allows us to detect these transactions that need the correct state of the coinbase, and we ban them. This allows us to differentiate a simple payment of transaction fees, which will not impact the transaction's behavior, from a transaction whose behavior will depend on the coinbase. Then we pay the rewards in the sequential part to have the correct coinbase balance for the start of each transaction. Thus, if a transaction is banned due to its access to the coinbase, it will no longer have a problem when processed sequentially because the balance of a coinbase has been correctly updated by previous transactions during the sequential part

Here is the part of the code where we ban transactions that have access to coinbase https://github.com/hyperledger/besu/pull/7296/files#diff-53805bb7138bc804441822c2d6acca3a1c72a26926563bc898b28eec5654c812R48

So unless I did not understand your remark for me it is already managed and was precisely an important element of the PR

garyschulte · 2024-07-18T19:42:51Z

So unless I did not understand your remark for me it is already managed and was precisely an important element of the PR

AFAIK, we do not need a transaction to directly interact with the coinbase in order to alter its state.

My thinking is that each transaction pays coinbase some bit at its conclusion. In a serial execution context this just means that any future transactions in the block would see the coinbase with an increased balance.

If in that same block, the coinbase is the originator of a transaction, nothing else would conflict with it because we are not considering coinbase reward as a conflict. If the coinbase transaction attempts to read its balance in isolation, without the tx rewards that might/would have accumulated before it executed its transaction, then a parallel coinbase tx would report a different value when executing the BALANCE opcode than if it had been executed serially.

garyschulte · 2024-07-18T20:05:57Z

So unless I did not understand your remark for me it is already managed and was precisely an important element of the PR

AFAIK, we do not need a transaction to directly interact with the coinbase in order to alter its state.

talking oob with Karim, he pointed out that the reason I didn't see the coinbase conflict detection is because I was searching for "coinbase", and the check is using "miningBeneficiary":
https://github.com/hyperledger/besu/pull/7296/files#diff-53805bb7138bc804441822c2d6acca3a1c72a26926563bc898b28eec5654c812R48

We should add explicit test coverage before this gets out of experimental, but otherwise lgtm

garyschulte · 2024-07-18T22:30:15Z

+                  if (transactionCollisionDetector
+                      .getAddressesTouchedByTransaction(
+                          transaction, Optional.of(roundWorldStateUpdater))
+                      .contains(miningBeneficiary)) {
+                    contextBuilder.isMiningBeneficiaryTouchedPreRewardByTransaction(true);
+                  }
+                  contextBuilder.miningBeneficiaryReward(miningReward);


Running gradle check with code coverage doesn't show this covered. Definitely would be nice to have this covered in test

fab-10

The idea behind this feature make sense, and as first step looks good to me, and I have nothing else to add, just waiting for the approval from @garyschulte since he has much more knowledge on this part.

Signed-off-by: ahamlat <ameziane.hamlat@consensys.net>

garyschulte

Lets merge and continue to test 👍

…su-eth#7296) Optimistic transaction parallelization execution during block processing to improve the performances. This feature can enabled with a flag --Xbonsai-parallel-tx-processing-enabled=true Signed-off-by: Karim Taam <karim.t2am@gmail.com> Co-authored-by: Ameziane H <ameziane.hamlat@consensys.net> Co-authored-by: garyschulte <garyschulte@gmail.com> Signed-off-by: gconnect <agatevureglory@gmail.com>

matkt and others added 23 commits July 2, 2024 17:00

add Parallel Transaction Processing System

a768488

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

merge main

3e9844e

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

clean code

466d1ca

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

fix build

a2dc27f

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

change config worldstate code

c431704

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

update reference tests

95170f7

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

async commit and modify hashcode for StorageSlotKey

81cdd37

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

remove preload of withdrawal address

96b3c23

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

add logs for map size

051236f

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

delete empty storage map

15b1f57

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

remove logs

fa801ba

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

execute commit and persist phase sequentially

444e991

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

Rollback sequential execution on persist and commit phase

32c6a2f

Initialize ConcurrentHashMap used in the accumulator to avoid collisions Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

clean code first step

05b8873

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

clean code

4728fd0

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

use int and not long for context map

a03843f

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

add javadoc and comment

f1e7dfc

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

revert preload during background processing and remove map initial ca…

3a35a1a

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

Set the executor size to the numbers of CPU threads

b937582

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

add flag and tests

a50dc04

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

merge main

7911566

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

clean code

7ec17b6

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

fix build

69f36b6

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

matkt force-pushed the feature/block_per_improvment branch from 46c7e3a to 69f36b6 Compare July 10, 2024 09:48

fix tests

6baafdc

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

matkt force-pushed the feature/block_per_improvment branch from 64ed174 to 6baafdc Compare July 10, 2024 12:37

matkt and others added 4 commits July 10, 2024 14:53

build again

594b7ec

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

fix unit tests

1fa6741

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

Add two new metrics to evaluate the number of parallelized transactio…

184aea7

…ns and the number of conflicting transactions Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

fix flag

18a12ac

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

ahamlat added 2 commits July 15, 2024 15:13

Add a changelog

2a823ff

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

Update changelog

027be5a

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

jflo mentioned this pull request Jul 15, 2024

Compose Protocol Spec with Dagger #7326

Closed

fab-10 reviewed Jul 16, 2024

View reviewed changes

Comment thread besu/src/main/java/org/hyperledger/besu/cli/options/stable/DataStorageOptions.java Outdated

Merge branch 'main' into feature/block_per_improvment

70af81f

Signed-off-by: ahamlat <ameziane.hamlat@consensys.net>

fab-10 reviewed Jul 16, 2024

View reviewed changes

ahamlat added 4 commits July 16, 2024 18:39

change flag name

b41c4c8

Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

Small optimization, return true in the collision check as soon as one…

1e149f8

… common address is found Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net>

Merge branch 'main' into feature/block_per_improvment

7da7a0e

Signed-off-by: ahamlat <ameziane.hamlat@consensys.net>

Merge branch 'main' into feature/block_per_improvment

7f14f29

Signed-off-by: ahamlat <ameziane.hamlat@consensys.net>

ahamlat enabled auto-merge (squash) July 18, 2024 09:11

ahamlat disabled auto-merge July 18, 2024 09:33

ahamlat enabled auto-merge (squash) July 18, 2024 09:33

ahamlat disabled auto-merge July 18, 2024 09:33

ahamlat enabled auto-merge (squash) July 18, 2024 09:45

ahamlat requested a review from fab-10 July 18, 2024 10:23

garyschulte previously requested changes Jul 18, 2024

View reviewed changes

garyschulte reviewed Jul 18, 2024

View reviewed changes

fab-10 reviewed Jul 19, 2024

View reviewed changes

Merge branch 'main' into feature/block_per_improvment

f9a451b

Signed-off-by: ahamlat <ameziane.hamlat@consensys.net>

garyschulte approved these changes Jul 19, 2024

View reviewed changes

ahamlat merged commit 30c96c7 into besu-eth:main Jul 19, 2024

ahamlat mentioned this pull request Jul 25, 2024

Add integration tests on block processing #7378

Merged

8 tasks

siladu added the performance label Jan 22, 2025

matkt deleted the feature/block_per_improvment branch January 27, 2025 09:39

Conversation

matkt commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description

Uh oh!

Uh oh!

Uh oh!

fab-10 Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

ahamlat Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

matkt Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

matkt Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

garyschulte Jul 18, 2024 • 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!

Uh oh!

Uh oh!

Uh oh!

garyschulte left a comment

Choose a reason for hiding this comment

Uh oh!

garyschulte Jul 18, 2024 • 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!

matkt commented Jul 18, 2024

Uh oh!

garyschulte commented Jul 18, 2024

Uh oh!

garyschulte commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garyschulte Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

fab-10 left a comment

Choose a reason for hiding this comment

Uh oh!

garyschulte left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

matkt commented Jul 5, 2024 •

edited

Loading

garyschulte Jul 18, 2024 •

edited

Loading

garyschulte Jul 18, 2024 •

edited

Loading

garyschulte commented Jul 18, 2024 •

edited

Loading