Optimistic parallelization of transactions to improve performance

[matkt]

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.

[fab-10]

this seems unrelated

[ahamlat]

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.

[matkt]

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

[matkt]

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

[garyschulte]

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

[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]

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]

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]

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]

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]

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.

[garyschulte]

Lets merge and continue to test 👍