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

[Flow EVM] patch evm debug tracer to collect results and reset internal state after each tx execution #6327

Merged
merged 9 commits into from
Aug 13, 2024

Conversation

ramtinms
Copy link
Contributor

@ramtinms ramtinms commented Aug 12, 2024

The tracer can be used for multiple transactions and later collect results being called. We need to support storing interim results until the collection time.
Unfortunately, the call tracer is stateful and if not cleaned up could result in bugs. for example, a failed transaction might change the internal state of the call tracer and the next transaction would start from an invalid state. This usually result in an "incorrect number of top-level calls" error.
This PR updates the tracer to collect and hold on to the tracing results after each transaction execution (onTxEnd) and reset the internal state of the tracer before each transaction execution (onTxStart).
It also makes the trace collection during batchRun function more clear.

@ramtinms ramtinms changed the title [Flow EVM] patch tracing to collect results and reset internal state after each tx execution [Flow EVM] patch evm debug tracer to collect results and reset internal state after each tx execution Aug 12, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 46.66667% with 72 lines in your changes missing coverage. Please review.

Project coverage is 40.46%. Comparing base (9e31160) to head (e7b643a).

Files Patch % Lines
fvm/evm/debug/tracer.go 41.46% 62 Missing and 10 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6327      +/-   ##
==========================================
- Coverage   42.37%   40.46%   -1.92%     
==========================================
  Files        1584     1322     -262     
  Lines      116430    87238   -29192     
==========================================
- Hits        49341    35299   -14042     
+ Misses      62106    48603   -13503     
+ Partials     4983     3336    -1647     
Flag Coverage Δ
unittests 40.46% <46.66%> (-1.92%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramtinms ramtinms marked this pull request as ready for review August 13, 2024 06:43
Comment on lines -220 to -229
defer func() {
if proc.evm.Config.Tracer.OnTxEnd != nil {
j := i
res := batchResults[j]
if err == nil && res != nil {
proc.evm.Config.Tracer.OnTxEnd(res.Receipt(), res.ValidationError)
}
}
}()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need to use defer as we don't want to collect the results on unhappy cases

@ramtinms ramtinms requested a review from sideninja August 13, 2024 06:48
@ramtinms ramtinms requested a review from zhangchiqing August 13, 2024 14:46
fvm/evm/debug/tracer.go Outdated Show resolved Hide resolved
@ramtinms ramtinms added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 13, 2024
@ramtinms ramtinms added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 13, 2024
@ramtinms ramtinms added this pull request to the merge queue Aug 13, 2024
Merged via the queue into master with commit eeac479 Aug 13, 2024
55 checks passed
@ramtinms ramtinms deleted the ramtin/evm-patch-tracing branch August 13, 2024 19:05
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.

4 participants