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

rpc: Significantly speedup debug_traceCallMany #7700

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

geomad
Copy link

@geomad geomad commented Jun 9, 2023

Currently, when using the default value of transactionIndex (-1) in the debug_traceCallMany function, the state is obtained from the previous block and all the transactions are replayed.

This pull request retrieves the most recent state, resulting in a significant decrease in tracing time from 50-150 ms to just 1 ms.

I have tested the modification and it functions as intended. However, I kindly request you to review it, as I lack experience with the Go programming language and the client internals.

@AskAlexSharov
Copy link
Collaborator

  1. Unlikely this change can work - because: if latest block = 10, and you re-exec transactions from this block - you need exec them on state 10-1, not on latests state (latest state already has changes made by block 10).

  2. Seems this RPC method has no tests.
    To accept this PR:

  • you or @hrthaowang (or somebody else) - please create another PR with tests. And maybe add link to some docs/spec.
  1. Docs of this method: Request for a New RPC Endpoint eth_callMany #4471 - need copy docs fro github to source code.

@AskAlexSharov
Copy link
Collaborator

AskAlexSharov commented Jun 11, 2023

FYI: likely this PR will also improve speed: #7706

Copy link
Collaborator

@AskAlexSharov AskAlexSharov left a comment

Choose a reason for hiding this comment

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

  • Unlikely this change can work - because: if latest block = 10, and you re-exec transactions from this block - you need exec them on state 10-1, not on latests state (latest state already has changes made by block 10).

  • Seems this RPC method has no tests.
    To accept this PR:

  • you or @hrthaowang (or somebody else) - please create another PR with tests. And maybe add link to some docs/spec.
    Docs of this method: Request for a New RPC Endpoint eth_callMany #4471 - need copy docs fro github to source code.

@geomad
Copy link
Author

geomad commented Jun 12, 2023

  • Unlikely this change can work - because: if latest block = 10, and you re-exec transactions from this block - you need exec them on state 10-1, not on latests state (latest state already has changes made by block 10).

This scenario arises when the value of transactionIndex is not equal to -1. In this case, you aim to replay specific transactions within the block and perform the simulation afterwards.

On the other hand, when transactionIndex is equal to -1, it indicates that you want to include all the transactions in the block and conduct the simulation after all of them have occurred. This is where this PR comes into play. Instead of replaying all the transactions on top of state 10-1, we can simply extract the state from block 10 and exclude the replay of any transactions.

Although I'm unsure if my expertise permits me to accomplish this, I might attempt it when I have some free time available if no one else is willing to undertake the task.

@AskAlexSharov
Copy link
Collaborator

This scenario arises when the value of transactionIndex is not equal to -1
I see. Then I agree.

In this case need:

  • tests for both scenarios
  • add docs (including this scenario)

@github-actions
Copy link

This PR is stale because it has been open for 40 days with no activity.

@github-actions github-actions bot added the Stale label Jul 23, 2023
@geomad
Copy link
Author

geomad commented Jul 23, 2023

I tried to write some test for this but my experience doesn't allow it. Furthermore I couldn't find any existing tests, not even for trace.callMany to get a hang of it. if anyone wants to do it it would be cool. I have this change running one mont on my node and everything looks ok.

@github-actions github-actions bot removed the Stale label Jul 24, 2023
@github-actions
Copy link

github-actions bot commented Sep 3, 2023

This PR is stale because it has been open for 40 days with no activity.

@github-actions github-actions bot added the Stale label Sep 3, 2023
@subtletech
Copy link

I believe this PR is rather a "bugfix" than an "improvement". In case transactionIndex is -1, TraceCallMany() must not replay all the transactions of the specified block on top of the state of the block previous to it. This is a major overhead, there are usually 100-200 transactions per block. Instead it must just pick up the state of the specified block.

This PR does not requre any tests and shoud be accepted without adding any.

@yperbasis yperbasis removed the Stale label Nov 13, 2023
@yperbasis yperbasis changed the base branch from release/2.60 to main May 1, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants