Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Precompute authorities when importing blocks #8017

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Dec 11, 2024

PR description

Like we do for senders, when can precompute the authorities for EIP-7702 transactions, to improve the performance during block import

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:

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

final var constIndex = index++;
mergeCoordinator
.getEthScheduler()
.scheduleTxWorkerTask(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precompute is asynchronous here, are we sure that the authorities are not accessed before it is finished or there is a fallback to calculate them if they don't exist yet ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is not better to precalculate all the senders than precompute all the autorities of all the transactions. Currently, we calculate sender of txn, then precomupte autorities of txn, then calculate sender of txn+1, then precomupte autorities of txn+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about your first question, if this async computation does not finish before the processing, then like for the sender it will be computed on first usage

About the order, I do not know which is the best solution, because in serial mode, should be better to precompute the authorities to txn before the sender of txn+1, but in parallel mode not sure what is the best solution, what do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this case, do we want to improve the time it takes to execute all the transaction by precalculating all the sender or improve a large one by precalculating all the authorizers.
I think it makes sens here to improve the worst case, so let's keep the current order.

@fab-10 fab-10 force-pushed the precompute-authority-on-block-import branch from 1a9463c to fac1741 Compare December 11, 2024 13:15
@ahamlat
Copy link
Contributor

ahamlat commented Dec 13, 2024

I think it makes also sens here compute the recovery in parallel, even for small amount of delegations. I suggest to do it when the number of delegation is bigger or equals to the number of cores

Benchmark                                                     (delegationCount)  Mode  Cnt      Score      Error  Units
CodeDelegationProcessorBenchmark.parallelProcessing                          10  avgt    5    212.832 ±    9.387  us/op
CodeDelegationProcessorBenchmark.parallelProcessing                         100  avgt    5    827.758 ±   84.302  us/op
CodeDelegationProcessorBenchmark.parallelProcessing                        1000  avgt    5   8260.485 ± 3498.222  us/op
CodeDelegationProcessorBenchmark.sequentialProcessing                        10  avgt    5    509.073 ±    7.936  us/op
CodeDelegationProcessorBenchmark.sequentialProcessing                       100  avgt    5   4918.714 ±  488.391  us/op
CodeDelegationProcessorBenchmark.sequentialProcessing                      1000  avgt    5  49019.810 ±  876.575  us/op

@fab-10 fab-10 marked this pull request as ready for review December 13, 2024 17:31
@fab-10 fab-10 enabled auto-merge (squash) December 13, 2024 17:32
@fab-10 fab-10 force-pushed the precompute-authority-on-block-import branch from 297545c to 63dbb19 Compare December 16, 2024 15:55
@fab-10 fab-10 merged commit 929945a into hyperledger:main Dec 16, 2024
43 checks passed
@fab-10 fab-10 deleted the precompute-authority-on-block-import branch December 16, 2024 16:26
daniellehrner pushed a commit to daniellehrner/besu that referenced this pull request Dec 18, 2024
* Precompute authorities when importing blocks

Signed-off-by: Fabio Di Fabio <[email protected]>

* Using Supplier to make the authorizer thread safe

Signed-off-by: Fabio Di Fabio <[email protected]>

* Process code delegation in parallel if there are more that one

Signed-off-by: Fabio Di Fabio <[email protected]>

---------

Signed-off-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Daniel Lehrner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants